Re: Fix git-svn for SVN 1.7

[Michael G Schwern <schwern@pobox.com>]

On 2012.7.30 7:18 PM, Eric Wong wrote:
> Michael G Schwern <schwern@pobox.com> wrote:
>> On 2012.7.30 3:15 PM, Eric Wong wrote:
>>>> Right now, canonicalization is a bug generator.  Paths and URLs have to be in
>>>> the same form when they're compared.  This requires meticulous care on the
>>>> part of the coder and reviewer to check every comparison.  It scatters the
>>>> logic for proper comparison all over the code.  Redundant logic scattered
>>>> around the code is a Bad Thing.  It makes it more likely a coder will forget
>>>> the logic, or get it wrong, and a human reviewer must be far more vigilant.
>>>
>>> <snip>  I agree completely with canonicalization.
>>
>> Sorry, I'm not sure what you're agreeing with.
> 
> That's it's a bug generator and we shouldn't have redundant logic.
> Having functions to compare objects themselves is a good thing.

That doesn't make it much better than what we have now.  One still has to
remember to pepper those special comparisons all over the code.


>>>> The only downside is when chasing down a bug related to canonicalization one
>>>> might have to realize that eq is overloaded.
>>>
>>> Having to realize eq is overloaded is a huge downside to me.
>>
>> Presumably you'd be reviewing the change which implements the overloaded
>> objects, so you'd know about it.  And it would be documented.
> 
> The change itself is easy to review.   Picking up the code a few
> months/years down the line and having to know "eq" is overloaded
> tends to bite people.

Why does a reviewer, or a reader of the code, have to know eq is overloaded?

How often would string comparing an overloaded uri/path object be the wrong
thing to do?  Just about never.  Compare that to how often it would be
incorrect to string compare a non-overloaded uri/path object.  Most of the
time.  Do you feel it would be otherwise?

If they're overloaded, somebody patching the code doesn't have to know to use
a special uri_eq() function.  It'll just happen when they naturally string
compare.  The coder doesn't have to know or do anything special.  The reviewer
doesn't have to do any special work.

If they're not overloaded, coders must know about the special URI and path
requirements.  Each string comparison is suspect and must be scrutinized by
the reviewer.  They have to think "is this actually a uri or path comparison?
 Should it be using the special comparison functions?"

Which procedure offers more opportunities for mistakes?


>> I've listed a bunch of concrete positives for using comparison overloaded
>> URI/path objects vs how it's currently being done.  How about you voice some
>> of the downsides in concrete terms?  Or an alternative that solves the current
>> problems?
> 
> Any custom comparison function would do the trick (e.g. URI::eq()).
>
> I _want_ URI/path objects.  I do not want a bare "eq" operator to
> obscure the fact it's calling URI::eq() behind-the-scenes.
>
> That said, I don't mind overloads when it's obvious an overload is being
> used (e.g. stringify).  It's things like "eq" which obscure the fact
> function calls are happening in the background.

Is that a problem?  If so, why?

If the objects stringify, but comparing them as strings is generally the wrong
thing to do (even if the object stringifies to the canonical form, you don't
know the other side of the operator is an object), isn't that asking for bugs?
 If the objects are going to act like strings, shouldn't they act like strings
completely?

Object overloading fails when the encapsulation is incomplete.


-- 
151. The proper way to report to my Commander is "Specialist Schwarz,
     reporting as ordered, Sir" not "You can't prove a thing!"
    -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army
           http://skippyslist.com/list/