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

Clemens Buchacher [off-list ref] writes:

'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 <redacted>

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

(minor nit) I think

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

is shorter and easier on the eye.

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