From: Junio C Hamano <gitster@pobox.com>
Date: Sat, 19 Jul 2008 00:32:26 GMT
Subject: Re: [PATCH] builtin-clone: Use is_dir_sep() instead of '/'
Message-ID: <7vk5fi67dx.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1216366485-12201-3-git-send-email-johannes.sixt@telecom.at>

Johannes Sixt <johannes.sixt@telecom.at> writes:

> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
>  builtin-clone.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-clone.c b/builtin-clone.c
> index 643c7d4..fddf47f 100644
> --- a/builtin-clone.c
> +++ b/builtin-clone.c
> @@ -115,7 +115,7 @@ static char *guess_dir_name(const char *repo, int is_bundle)
>  			if (!after_slash_or_colon)
>  				end = p;
>  			p += 7;
> -		} else if (*p == '/' || *p == ':') {
> +		} else if (is_dir_sep(*p) || *p == ':') {
>  			if (end == limit)
>  				end = p;
>  			after_slash_or_colon = 1;

Ok, but the surrounding code in this function look very suspicious.

 * The variable "prefix" is actually about suffix.

 * "else if" for explicit ".bundle" case look redundant; it would never
   trigger, I suspect.

 * Values used to increment p for "prefix" case and ".bundle" case are
   inconsistent.  I think the redundant one increments by the right value
   (i.e. strlen(prefix)-1 is bogus).