Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:
> >> Junio C Hamano <gitster@pobox.com> writes:
> >> 
> >> > For completeness, here is what I think the end result (together with
> >> > Peff's series) of the test should look like.
> >> > ...
> >> > Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/
> >> > tests fail for the same reason (finding @ should be greedy, I think).
> >> 
> >> And I think this should make it pass.  Just remember the last
> >> occurrence of '@' by moving the 'start' every time we see an '@'
> >> sign.
> >> 
> >>  builtin/clone.c | 11 +++++------
> >>  1 file changed, 5 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/builtin/clone.c b/builtin/clone.c
> >> index cae288f..5d86439 100644
> >> --- a/builtin/clone.c
> >> +++ b/builtin/clone.c
> >> @@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
> >>  		start += 3;
> >>  
> >>  	/*
> >> -	 * Skip authentication data.
> >> +	 * Skip authentication data, if exists.
> >>  	 */
> >> -	ptr = start;
> >> -	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
> >> -		ptr++;
> >> -	if (*ptr == '@')
> >> -		start = ptr + 1;
> >> +	for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {
> >> +		if (*ptr == '@')
> >> +			start = ptr + 1;
> >> +	}
> >>  
> >>  	/*
> >>  	 * Strip trailing spaces, slashes and /.git
> >
> > I guess it makes sense to skip over @-signs greedily. Is there
> > anything I need to do here or will you squash those changes in?
> 
> Sorry but I won't have patience to split the "these need squashing
> in" into multiple pieces and add them to appropriate commits in the
> 5 patch series (counting Peff's two fixes on top of which you built
> your 3 patch series) correctly.  You as the original author are much
> better equipped to do so than I am, so I'd appreciate if you can
> reroll them as a 5-patch series.
> 
> Among the changes, the fix to builtin/clone.c needs to be squashed
> into your "clone: do not include authentication data in guessed
> dir", and the remainder of the patch to t5603 should go to Peff's
> "clone: add tests for output directory", but with most of them
> marked initially as failing.  And then as you fix them in your
> patches, some of the "fail" mark should be dropped.
> 
> Thanks.