Re: [PATCH] clone: fix creation of explicitly named target directory

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 3, 2008, 19:24 UTC
Message-ID: <7vprnlav80.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080903185554.GA6738@localhost>

Clemens Buchacher <drizzd@aon.at> writes:

Show 5 quoted lines

> 'git clone <repo> path/' (note the trailing slash) fails, because the
> entire path is interpreted as leading directories. So when mkdir tries to
> create the actual path, it already exists.
>
> This makes sure trailing slashes are removed.

Thanks.

Show 7 quoted lines

> +static char *strip_dir_sep(char *dir)
> +{
> +	char *end = dir + strlen(dir);
> +
> +	while (dir < end && is_dir_sep(end[-1]))
> +		end--;
> +	*end = '\0';

It does not matter in this particular context, but I'd do "dir < end - 1" to avoid returning the root directory as an empty string, just as a disciplined style; also I'd rename this to strip_trailing_slashes(), make it of type void to make it more clear that it munges the string that is given as the input parameter.

Show 8 quoted lines

> @@ -394,7 +405,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		repo = repo_name;
>  
>  	if (argc == 2)
> -		dir = xstrdup(argv[1]);
> +		dir = strip_dir_sep(xstrdup(argv[1]));
>  	else
>  		dir = guess_dir_name(repo_name, is_bundle, option_bare);

Made me wonder if guess_dir_name() can return something with trailing slashes; it turns out that it doesn't, but not very nice. As people's braincycle is more precious, I'd rather say:

	if (argc == 2)
        	dir = xstrdup(argv[1]);
	else
        	dir = guess_dir_name(repo_name, is_bundle, option_bare);
	strip_trailing_slashes(dir);

I'll queue with the above changes to reduce one round of back-and-forth, but if you see any flaws in my above reasoning, please say so.