Re: Fix git-svn for SVN 1.7

Eric Wong <normalperson@yhbt.net> writes:

I think code that uses operator overloading, when printed in a textbook, cast in stone and makes the reader aware that it is never going to change, is indeed "easy" to read through. But I suspect that it may be merely giving a false illusion that it is easy to readers.

The problem is that use of such obscure overloading tends to hurt maintainability. If the initial version Michael produces converts all the external strings into instances of CanonicalizedPath class, according to the "convert as early as possible" principle, you can be assured that all "eq" you see are about the normalized strings the svn library wants to see, and that may allow us sleep safely.

But the real problem begins six months down the road, when somebody wants to add a new codepath that reads a new string from an external source (e.g. perhaps you add a new configuration variable that specifies a path in the svn repository and does something special when that path is touched by a revision; the exact nature of the new feature does not matter in this discussion). The new code can forget to follow the "convert early" principle, and pass a bare string read from the configuration around.

A comparison between such a new string and another variable that holds path that comes from the existing codepath (i.e. Michael's initial code that perfectly follows the "convert early" principle) will still use the overloaded eq in "$new_str eq $old_path", thanks to the language rule of Perl (namely, even though the new string is a non object, the other side is still an instance of the class).

When the code needs to compare two or more such "new" strings (e.g. perhaps it wants to remove duplicates from the set of paths it reads from the configuration), however, "eq" silently turns back to a simple string comparison, as "$new_1 eq $new_2" will not magically turn into "Canonicalize($new_1)->cmp(Canonicalize($new_2))".

This kind of error is unnecessarily hard to catch mostly because the previous "$new_str eq $old_path" does work; it masks the problem. Overloading of "eq" is making it harder to spot new bugs.

If the code never uses "eq" to compare canonicalized paths, and all the surrounding code compare paths using explicit method call on objects, it makes it crystal clear to the readers that paths held in a bare string is unwelcome in the codepath. It makes it harder to add new code that uses and passes around a bare string by mistake to such a codepath, I would think.