Clemens Buchacher <drizzd@xxxxxx> 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 <drizzd@xxxxxx>
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".
> 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.
> break;
> *pos = 0;
> if (!stat(path, &st)) {
> --
> 1.6.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
