Re: [PATCH] ignore trailing slash when creating leading directories

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 2, 2008, 18:38 UTC
Message-ID: <7vk5dul7f5.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080902081909.GA2059@localhost>

Clemens Buchacher <drizzd@aon.at> writes:

Show 7 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 a trailing slash is ignored.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>

Thanks.

I have a few comments.

 (1) Addition of strerror(errno) is a good thing, but it is a separate
     topic;

 (2) I always thought that it was a clever feature to allow callers that
     would want to prepare a directory in advance to ask for "xyzzy/" and
     cause the whole path created.  You are breaking it, which may or may
     not be a bad thing per-se, because I do not think any existing caller
     depends on this behaviour;

 (3) If you *are* to break that feature, then I think you should also
     handle a user input that is broken in the same fashion as your clone
     example, namely, "git clone <repo> path//".  It does not make much
     senseto say "path/" as the last parameter to clone is not a user
     error but "path//" is.

If a change in behaviour to strip trailing slashes inside safe_c_l_d() is agreed to be a good thing (I do not mind that myself, but there could be some private patches people are using in their trees that depend on the current behaviour --- we never know), I think it should go through the usual next-master cycle as a feature enhancement / clean-up patch, so that we have better chance to catch breakages this might cause to other people.

As a "bugfix" patch meant to apply to 'maint', I'd prefer a fix to the caller (builtin-clone.c that calls the function), which should be of much less impact. It is fine to include the change to add strerror(errno) in that patch, whose title would be "clone: fix creation of explicitly named target directory".

Show 10 quoted lines

> diff --git a/sha1_file.c b/sha1_file.c
> index 9ee1ed1..3cb9414 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -97,7 +97,7 @@ int safe_create_leading_directories(char *path)
>  
>  	while (pos) {
>  		pos = strchr(pos, '/');
> -		if (!pos)
> +		if (!pos || !*(pos + 1))

(minor nit) I think

		if (!pos || !pos[1])

is shorter and easier on the eye.

Show 5 quoted lines

>  			break;
>  		*pos = 0;
>  		if (!stat(path, &st)) {
> -- 
> 1.6.0