From: Michael G Schwern <schwern@pobox.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, robbat2@gentoo.org,
bwalton@artsci.utoronto.ca, normalperson@yhbt.net
Subject: Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
Date: Sat, 28 Jul 2012 12:51:49 -0700 [thread overview]
Message-ID: <501442D5.6080207@pobox.com> (raw)
In-Reply-To: <20120728193029.GB3107@burratino>
On 2012.7.28 12:30 PM, Jonathan Nieder wrote:
>> I didn't know about that. I don't know what your SVN backwards compat
>> requirements are, but if that behavior goes back far enough in SVN to satisfy
>> you folks, then canonicalize_url() should fall back to
>> SVN::_Core::svn_path_canonicalize().
>
> svn_path_canonicalize() has been usable for this kind of thing since
> SVN 1.1, possibly earlier.
Great! Then _canonicalize_url_ourselves() can probably be replaced with that.
Just take my advice and do it after 1.7 is working and the code is ready for
canonicalization.
>> But try it at the end of the patch
>> series. The code has to be prepared for canonicalization first. Then how it
>> actually does it can be improved.
>
> Since this part of the series is not tested with SVN 1.7, this is
> basically adding dead code, right? That could be avoided by
> reordering the changes to keep "canonicalize_url" as-is until later in
> the series when the switchover is safe.
I would suggest that worrying whether a few lines of code are introduced now
or 10 patches later in the same branch which is all going to be merged in one
go (and retesting the patches after it) is not the most important thing. The
code needs humans looking over it and deciding if canonicalizations were
missed or applied inappropriately. Or hey, work on that path and url object
idea that makes a lot of real code mess go away.
--
ROCKS FALL! EVERYONE DIES!
http://www.somethingpositive.net/sp05032002.shtml
next prev parent reply other threads:[~2012-07-28 19:52 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-07-28 9:38 Canonicalize the git-svn path & url accessors Michael G. Schwern
2012-07-28 9:38 ` [PATCH 1/7] Move the canonicalization functions to Git::SVN::Utils Michael G. Schwern
2012-07-28 9:38 ` [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available Michael G. Schwern
2012-07-28 13:50 ` Jonathan Nieder
2012-07-28 19:01 ` Michael G Schwern
2012-07-28 19:30 ` Jonathan Nieder
2012-07-28 19:51 ` Michael G Schwern [this message]
2012-07-28 19:57 ` Jonathan Nieder
2012-07-28 20:02 ` Jonathan Nieder
2012-07-28 20:24 ` Michael G Schwern
2012-10-14 11:42 ` [PATCH/RFC 0/2] " Jonathan Nieder
2012-10-14 11:45 ` [PATCH 1/2] git svn: do not overescape URLs (fallback case) Jonathan Nieder
2012-10-14 11:48 ` [PATCH 2/2] git svn: canonicalize_url(): use svn_path_canonicalize when available Jonathan Nieder
2012-10-23 22:58 ` Eric Wong
2012-07-28 9:38 ` [PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths Michael G. Schwern
2012-07-30 19:51 ` Eric Wong
2012-07-30 20:46 ` Michael G Schwern
2012-09-26 19:45 ` Jonathan Nieder
2012-09-26 20:58 ` Eric Wong
2012-09-26 21:38 ` Jonathan Nieder
2012-09-26 21:54 ` Eric Wong
2012-09-26 22:43 ` Jonathan Nieder
2012-09-27 0:15 ` Eric Wong
2012-09-27 2:11 ` Jonathan Nieder
2012-07-28 9:38 ` [PATCH 4/7] Add join_paths() to safely concatenate paths Michael G. Schwern
2012-09-26 20:51 ` Jonathan Nieder
2012-07-28 9:38 ` [PATCH 5/7] Remove irrelevant comment Michael G. Schwern
2012-07-28 9:38 ` [PATCH 6/7] Switch path canonicalization to use the SVN API Michael G. Schwern
2012-07-28 13:55 ` Jonathan Nieder
2012-07-28 19:07 ` Michael G Schwern
2012-07-30 20:04 ` Eric Wong
2012-08-02 21:51 ` Eric Wong
2012-08-02 23:18 ` Michael G Schwern
2012-10-05 7:04 ` [PATCH] git-svn: keep leading slash when canonicalizing paths (fallback case) Jonathan Nieder
2012-10-05 23:12 ` Eric Wong
2012-10-06 5:36 ` Junio C Hamano
2012-07-28 9:38 ` [PATCH 7/7] Make Git::SVN and Git::SVN::Ra canonicalize paths and urls Michael G. Schwern
2012-07-28 14:11 ` Jonathan Nieder
2012-07-28 19:15 ` Michael G Schwern
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=501442D5.6080207@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.