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

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.

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

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

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.

quoted

-		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