git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eric Wong <normalperson@yhbt.net>
Cc: "Michael G. Schwern" <schwern@pobox.com>,
	git@vger.kernel.org, robbat2@gentoo.org,
	bwalton@artsci.utoronto.ca, jrnieder@gmail.com
Subject: Re: Fix git-svn for SVN 1.7
Date: Mon, 30 Jul 2012 23:53:30 -0700	[thread overview]
Message-ID: <7v1ujsl8ut.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <20120730203844.GA23892@dcvr.yhbt.net> (Eric Wong's message of "Mon, 30 Jul 2012 20:38:44 +0000")

Eric Wong <normalperson@yhbt.net> writes:

> Perhaps we can depend on the URI.pm module?  It seems to be
> widely-available and not be a significant barrier to installation.  On
> the other hand, I don't know its history, either (especially since we're
> now dealing with SVN changes...).
>
> Anyways, I don't like relying on operator overloading, it makes code
> harder to read and review.

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.

  parent reply	other threads:[~2012-07-31  6:53 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-07-28  9:47 Fix git-svn for SVN 1.7 Michael G. Schwern
2012-07-28  9:47 ` [PATCH 1/8] SVN 1.7 will truncate "not-a%40{0}" to just "not-a" Michael G. Schwern
2012-07-28 14:16   ` Jonathan Nieder
2012-07-28 19:32     ` Michael G Schwern
2012-10-09  8:41       ` [PATCH/RFC] svn test: escape peg revision separator using empty peg rev Jonathan Nieder
2012-10-09  9:47         ` Michael J Gruber
2012-10-09 10:19           ` Jonathan Nieder
2012-10-10 20:37             ` Eric Wong
2012-10-10 21:02               ` Jonathan Nieder
2012-10-10 21:31                 ` Eric Wong
2012-10-10 21:42                   ` Jonathan Nieder
2012-10-10 22:16                     ` Eric Wong
2012-10-10 22:33               ` Junio C Hamano
2012-07-28  9:47 ` [PATCH 2/8] Fix typo in test Michael G. Schwern
2012-07-28  9:47 ` [PATCH 3/8] Improve our URL canonicalization to be more like SVN 1.7's Michael G. Schwern
2012-07-28  9:47 ` [PATCH 4/8] Replace hand rolled URL escapes with canonicalization Michael G. Schwern
2012-07-28  9:47 ` [PATCH 5/8] Canonicalize earlier in a couple spots Michael G. Schwern
2012-07-28  9:47 ` [PATCH 6/8] Add function to append a path to a URL Michael G. Schwern
2012-07-28  9:47 ` [PATCH 7/8] Turn on canonicalization on newly minted URLs Michael G. Schwern
2012-10-06 19:24   ` [PATCH/RFC] test: work around SVN 1.7 mishandling of svn:special changes Jonathan Nieder
2012-10-09 10:12     ` [PATCH/RFC v2] git svn: " Jonathan Nieder
2012-10-10 20:11       ` Eric Wong
2012-10-10 20:47         ` [PATCH v3] " Jonathan Nieder
2012-07-28  9:47 ` [PATCH 8/8] Remove some ad hoc canonicalizations Michael G. Schwern
2012-07-30 20:38 ` Fix git-svn for SVN 1.7 Eric Wong
2012-07-30 21:10   ` Michael G Schwern
2012-07-30 22:15     ` Eric Wong
2012-07-31  1:04       ` Michael G Schwern
2012-07-31  2:18         ` Eric Wong
2012-07-31  4:30           ` Michael G Schwern
2012-07-31  6:53   ` Junio C Hamano [this message]
2012-07-31  9:54     ` Michael G Schwern
2012-07-31 20:01       ` Eric Wong
2012-07-31 23:05         ` Junio C Hamano
2012-07-31 23:28           ` Michael G Schwern
2012-07-31 23:24         ` Michael G Schwern
2012-08-01 21:30           ` Eric Wong
2012-08-02 10:31 ` Eric Wong
2012-08-02 16:07   ` Jonathan Nieder
2012-08-02 18:58     ` Junio C Hamano
2012-08-02 19:50       ` Robin H. Johnson
2012-08-02 22:10         ` Eric Wong
2012-08-21  4:04       ` Junio C Hamano
2012-08-21 21:03         ` Eric Wong
2012-08-21 21:34           ` Junio C Hamano
2012-08-02 20:51     ` Eric Wong
2012-08-02 21:22       ` Junio C Hamano
2012-08-02 21:42         ` Eric Wong
2012-08-02 21:55           ` Eric Wong
2012-08-02 22:05             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7v1ujsl8ut.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=bwalton@artsci.utoronto.ca \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=normalperson@yhbt.net \
    --cc=robbat2@gentoo.org \
    --cc=schwern@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).