Re: [PATCH] builtin-clone: Use is_dir_sep() instead of '/'

["Kristian Høgsberg" <krh@redhat.com>]

On Sat, 2008-07-19 at 13:44 -0400, Daniel Barkalow wrote:
> On Sat, 19 Jul 2008, Johannes Sixt wrote:
> 
> > On Samstag, 19. Juli 2008, Johannes Sixt wrote:
> > > On Samstag, 19. Juli 2008, Junio C Hamano wrote:
> > > > Ok, but the surrounding code in this function look very suspicious.
> > >
> > > How about this then?
> > >
> > > -- snip --
> > > builtin-clone: Rewrite guess_dir_name()
> > >
> > > The function has to do three small and independent tasks, but all of them
> > > were crammed into a single loop. This rewrites the function entirely by
> > > unrolling these tasks.
> > 
> > Sigh. I knew it, I knew it. If it had been that trivial, then Daniel had done
> > it this way in the first place. :-(
> > 
> > This needs to be squashed in. It makes sure that we handle 'foo/.git';
> > and .git was not stripped if we cloned from 'foo.git/'.
> 
> I actually got that from Johannes Schindelin, who added bundle support to 
> my clone patch. I remember looking at his change, thinking it looked 
> overly complicated, but finding that anything I tried to do to simplify it 
> failed tests. If this gets through the test suite (lots of the tests other 
> than the clone test try to do a wider variety of odd things than I expect 
> users do in practice most of the time), then it's probably a better 
> implementation.

I wrote the original guess_git_dir(), and it *is* pretty subtle, sorry.
I would not rewrite it unless you really have to, since git clone now is
pretty stable and the dir sep separator change doesn't need this kind of
rewrite to fix the issue.  That said, with the last couple of changes
from Johannes Sixt, I believe it handles the same case that I had in
mind when I first wrote the loop.

cheers,
Kristian