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

From: Clemens Buchacher <drizzd@aon.at>
Date: Sep 2, 2008, 19:13 UTC
Message-ID: <20080902191322.GA11172@localhost>
In-Reply-To: <7vk5dul7f5.fsf@gitster.siamese.dyndns.org>

On Tue, Sep 02, 2008 at 11:38:38AM -0700, Junio C Hamano wrote:

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

Yes indeed. There are many more places that lack proper error reporting. I wonder if we should introduce some wrapper functions like mkdir_or_die, so the caller doesn't have check for errors. Such a function could (optionally?) create leading directories as well.

Show 5 quoted lines

>  (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;

Yes, I was afraid of that. So I checked all calls to c_l_d and it's not used that way anywhere.

Show 5 quoted lines

>  (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.

True enough.

Show 5 quoted lines

> 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".

Unfortunately, if we simply add strerror to the error message, in place of

fatal: could not create work tree dir 'path/'.

the new version would print

fatal: could not create work tree dir 'path/': File exists.

which makes things worse IMO. We could of course strip trailing slashes in builtin-clone.c for now and revert that as soon as the cleanup patch is in, but I think it's not worth the trouble. I suggest we live with the "bug" for now. The error reporting cleanups should be done at a greater scope anyways.

Show 8 quoted lines

> > -		if (!pos)
> > +		if (!pos || !*(pos + 1))
> 
> (minor nit) I think
> 
> 		if (!pos || !pos[1])
> 
> is shorter and easier on the eye.

Will be fixed in the patch to follow.

Clemens