From: Michael G Schwern <schwern@pobox.com>
To: Eric Wong <normalperson@yhbt.net>
Cc: git@vger.kernel.org, gitster@pobox.com, robbat2@gentoo.org,
bwalton@artsci.utoronto.ca, jrnieder@gmail.com
Subject: Re: Fix git-svn for SVN 1.7
Date: Mon, 30 Jul 2012 14:10:10 -0700 [thread overview]
Message-ID: <5016F832.7030604@pobox.com> (raw)
In-Reply-To: <20120730203844.GA23892@dcvr.yhbt.net>
On 2012.7.30 1:38 PM, Eric Wong wrote:
>> A better solution would be to have path and URL objects which overload
>> the eq operator and automatically stringify canonicalized and escaped.
>
> 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...).
If you want to go down the road of having CPAN dependencies, then it should
definitely be used rather than rolling our own and generating our own bugs.
It's a very commonly needed Perl module.
You'd make a subclass and put any special work arounds for SVN in there.
> Anyways, I don't like relying on operator overloading, it makes code
> harder to read and review.
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.
Right now I'm pretty sure there's still a ton of bugs.
It also slows things down. As strings, URLs and paths have to be
canonicalized every time they're used or compared. An object representing the
URI or path can cache the canonicalization.
With string comparison overloaded, you'd no longer have to meticulously check
that URLs and paths are always in the same form when they're compared. It
just does it. The logic is in one place. We don't even have to care if one
of them is a string (or which one), it works even if only one half of the
comparison is an object. A new coder to the project doesn't need to know
anything special about URIs and paths, they just treat them as strings.
Finally, they can be slipped into existing code without having to rewrite
everything.
Overloaded comparison and stringification can even be used as a tool to find
all the places in the code where URLs and paths are being used, where they're
being turned into strings, and where URL and path manipulation is being done
ad-hoc. For example, if comparison sees one of its arguments as a string, or
if concatenation is used.
The only downside is when chasing down a bug related to canonicalization one
might have to realize that eq is overloaded. But we'd have far less bugs due
to canonicalization. So worth it.
--
Being faith-based doesn't trump reality.
-- Bruce Sterling
next prev parent reply other threads:[~2012-07-30 21:10 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 [this message]
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
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5016F832.7030604@pobox.com \
--to=schwern@pobox.com \
--cc=bwalton@artsci.utoronto.ca \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=normalperson@yhbt.net \
--cc=robbat2@gentoo.org \
/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.
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 NNTP newsgroup(s).