[PATCH] clone: Make use of the strip_suffix() helper method

[PATCH] clone: Make use of the strip_suffix() helper method

[Sebastian Schuberth]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/clone.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 00535d0..d35b2b9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
 	const char *end = repo + strlen(repo), *start;
+	size_t len;
 	char *dir;
 
 	/*
@@ -174,19 +175,17 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	 * Strip .{bundle,git}.
 	 */
 	if (is_bundle) {
-		if (end - start > 7 && !strncmp(end - 7, ".bundle", 7))
-			end -= 7;
+		strip_suffix(start, ".bundle", &len);
 	} else {
-		if (end - start > 4 && !strncmp(end - 4, ".git", 4))
-			end -= 4;
+		strip_suffix(start, ".git", &len);
 	}
 
 	if (is_bare) {
 		struct strbuf result = STRBUF_INIT;
-		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
+		strbuf_addf(&result, "%.*s.git", len, start);
 		dir = strbuf_detach(&result, NULL);
 	} else
-		dir = xstrndup(start, end - start);
+		dir = xstrndup(start, len);
 	/*
 	 * Replace sequences of 'control' characters and whitespace
 	 * with one ascii space, remove leading and trailing spaces.

---
https://github.com/git/git/pull/160

Re: [PATCH] clone: Make use of the strip_suffix() helper method

[Jeff King]

On Thu, Jul 09, 2015 at 03:33:46PM +0000, Sebastian Schuberth wrote:

> @@ -174,19 +175,17 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
>  	 * Strip .{bundle,git}.
>  	 */
>  	if (is_bundle) {
> -		if (end - start > 7 && !strncmp(end - 7, ".bundle", 7))
> -			end -= 7;
> +		strip_suffix(start, ".bundle", &len);
>  	} else {
> -		if (end - start > 4 && !strncmp(end - 4, ".git", 4))
> -			end -= 4;
> +		strip_suffix(start, ".git", &len);
>  	}

Yay, always glad to see complicated string handling like this go away.
As the resulting conditional blocks are one-liners, I think you can drop
the curly braces, which will match our usual style:

  if (is_bundle)
	strip_suffix(start, ".bundle", &len);
  else
	strip_suffix(start, ".git", &len);

If you wanted to get really fancy, I think you could put a ternary
operator in the middle of the strip_suffix call. That makes it clear
that "len" is set in all code paths, but I think some people find
ternary operators unreadable. :)

>  	if (is_bare) {
>  		struct strbuf result = STRBUF_INIT;
> -		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
> +		strbuf_addf(&result, "%.*s.git", len, start);
>  		dir = strbuf_detach(&result, NULL);

This one can also be simplified using xstrfmt to:

  if (is_bare)
	dir = xstrfmt("%.*s.git", len, start);

Do we still need to cast "len" to an int to use it with "%.*" (it is
defined by the standard as an int, not a size_t)?

-Peff

Re: [PATCH] clone: Make use of the strip_suffix() helper method

[Sebastian Schuberth]

On Thu, Jul 9, 2015 at 7:00 PM, Jeff King <peff@peff.net> wrote:

> If you wanted to get really fancy, I think you could put a ternary
> operator in the middle of the strip_suffix call. That makes it clear
> that "len" is set in all code paths, but I think some people find
> ternary operators unreadable. :)

I like the idea about the ternary operator, will do.

> This one can also be simplified using xstrfmt to:

Nice, will also do.

> Do we still need to cast "len" to an int to use it with "%.*" (it is
> defined by the standard as an int, not a size_t)?

I think we're more on the safe side by keeping the cast, so I'll do that, too.

-- 
Sebastian Schuberth

[PATCH v2] clone: Simplify string handling in guess_dir_name()

[Sebastian Schuberth]

[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/clone.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 00535d0..afdc004 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
 	const char *end = repo + strlen(repo), *start;
+	size_t len;
 	char *dir;
 
 	/*
@@ -173,20 +174,9 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	/*
 	 * Strip .{bundle,git}.
 	 */
-	if (is_bundle) {
-		if (end - start > 7 && !strncmp(end - 7, ".bundle", 7))
-			end -= 7;
-	} else {
-		if (end - start > 4 && !strncmp(end - 4, ".git", 4))
-			end -= 4;
-	}
+	strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
 
-	if (is_bare) {
-		struct strbuf result = STRBUF_INIT;
-		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
-		dir = strbuf_detach(&result, NULL);
-	} else
-		dir = xstrndup(start, end - start);
+	dir = is_bare ? xstrfmt("%.*s.git", (int)len, start) : xstrndup(start, len);
 	/*
 	 * Replace sequences of 'control' characters and whitespace
 	 * with one ascii space, remove leading and trailing spaces.

---
https://github.com/git/git/pull/160

Re: [PATCH v2] clone: Simplify string handling in guess_dir_name()

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> Subject: Re: [PATCH v2] clone: Simplify string handling in guess_dir_name()

We seem not to capitalize the first word on the subject line.

> Content-Type: multipart/mixed;  boundary="----=_Part_8_836493213.1436462597065"

Please don't.

> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
>  builtin/clone.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 00535d0..afdc004 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
>  static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
>  {
>  	const char *end = repo + strlen(repo), *start;
> +	size_t len;
>  	char *dir;
>  
>  	/*
> @@ -173,20 +174,9 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
>  	/*
>  	 * Strip .{bundle,git}.
>  	 */
> -	if (is_bundle) {
> -		if (end - start > 7 && !strncmp(end - 7, ".bundle", 7))
> -			end -= 7;
> -	} else {
> -		if (end - start > 4 && !strncmp(end - 4, ".git", 4))
> -			end -= 4;
> -	}
> +	strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);

This looks vastly nicer than the original.

> -	if (is_bare) {
> -		struct strbuf result = STRBUF_INIT;
> -		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
> -		dir = strbuf_detach(&result, NULL);
> -	} else
> -		dir = xstrndup(start, end - start);
> +	dir = is_bare ? xstrfmt("%.*s.git", (int)len, start) : xstrndup(start, len);

This however I had to read twice.  I'd say

	if (is_bare)
        	dir = xstrfmt(...);
	else
        	dir = xstrndup(...);

is much easier to read.

Re: [PATCH v2] clone: Simplify string handling in guess_dir_name()

[Sebastian Schuberth]

On Thu, Jul 9, 2015 at 8:05 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> Subject: Re: [PATCH v2] clone: Simplify string handling in guess_dir_name()
>
> We seem not to capitalize the first word on the subject line.

Will change that.

>> Content-Type: multipart/mixed;  boundary="----=_Part_8_836493213.1436462597065"
>
> Please don't.

This seems to come from submitgit, I've filed an issue about it:

https://github.com/rtyley/submitgit/issues/17

What content type(s) would you accept? Only text/plain?

>> -     if (is_bare) {
>> -             struct strbuf result = STRBUF_INIT;
>> -             strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
>> -             dir = strbuf_detach(&result, NULL);
>> -     } else
>> -             dir = xstrndup(start, end - start);
>> +     dir = is_bare ? xstrfmt("%.*s.git", (int)len, start) : xstrndup(start, len);
>
> This however I had to read twice.  I'd say
>
>         if (is_bare)
>                 dir = xstrfmt(...);
>         else
>                 dir = xstrndup(...);
>
> is much easier to read.

That's what I had locally before. Will revert to that.

-- 
Sebastian Schuberth

[PATCH v3] clone: Simplify string handling in guess_dir_name()

[Sebastian Schuberth]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/clone.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 00535d0..ebcb849 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
 	const char *end = repo + strlen(repo), *start;
+	size_t len;
 	char *dir;
 
 	/*
@@ -173,19 +174,11 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	/*
 	 * Strip .{bundle,git}.
 	 */
-	if (is_bundle) {
-		if (end - start > 7 && !strncmp(end - 7, ".bundle", 7))
-			end -= 7;
-	} else {
-		if (end - start > 4 && !strncmp(end - 4, ".git", 4))
-			end -= 4;
-	}
+	strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
 
-	if (is_bare) {
-		struct strbuf result = STRBUF_INIT;
-		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
-		dir = strbuf_detach(&result, NULL);
-	} else
+	if (is_bare)
+		dir = xstrfmt("%.*s.git", (int)len, start);
+	else
 		dir = xstrndup(start, end - start);
 	/*
 	 * Replace sequences of 'control' characters and whitespace

---
https://github.com/git/git/pull/160

[PATCH v4] clone: simplify string handling in guess_dir_name()

[Sebastian Schuberth]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/clone.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 00535d0..ebcb849 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
 	const char *end = repo + strlen(repo), *start;
+	size_t len;
 	char *dir;
 
 	/*
@@ -173,19 +174,11 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	/*
 	 * Strip .{bundle,git}.
 	 */
-	if (is_bundle) {
-		if (end - start > 7 && !strncmp(end - 7, ".bundle", 7))
-			end -= 7;
-	} else {
-		if (end - start > 4 && !strncmp(end - 4, ".git", 4))
-			end -= 4;
-	}
+	strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
 
-	if (is_bare) {
-		struct strbuf result = STRBUF_INIT;
-		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
-		dir = strbuf_detach(&result, NULL);
-	} else
+	if (is_bare)
+		dir = xstrfmt("%.*s.git", (int)len, start);
+	else
 		dir = xstrndup(start, end - start);
 	/*
 	 * Replace sequences of 'control' characters and whitespace

---
https://github.com/git/git/pull/160

Re: [PATCH v2] clone: Simplify string handling in guess_dir_name()

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> On Thu, Jul 9, 2015 at 8:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> Content-Type: multipart/mixed;  boundary="----=_Part_8_836493213.1436462597065"
>>
>> Please don't.
>
> This seems to come from submitgit, I've filed an issue about it:
>
> https://github.com/rtyley/submitgit/issues/17
>
> What content type(s) would you accept? Only text/plain?

I could take anything, even chicken scratches on a piece of paper,
for a small change like this.

But let's make sure we see text/plain out of "submitgit", as the
whole point of it is to allow people generate the common denominator
format out of a commit pushed to GitHub.

Thanks for letting them know.

Re: [PATCH v4] clone: simplify string handling in guess_dir_name()

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> -	if (is_bare) {
> -		struct strbuf result = STRBUF_INIT;
> -		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
> -		dir = strbuf_detach(&result, NULL);
> -	} else
> +	if (is_bare)
> +		dir = xstrfmt("%.*s.git", (int)len, start);
> +	else
>  		dir = xstrndup(start, end - start);

The last one needs to be adjusted with s/end - start/len/.  The
last-minute rewrite without testing shows; your first two patches
correctly used "len" ;-)

No need to resend.  Will locally tweak before queuing.

Thanks.

Re: [PATCH v4] clone: simplify string handling in guess_dir_name()

[Sebastian Schuberth]

On Thu, Jul 9, 2015 at 11:21 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> -     if (is_bare) {
>> -             struct strbuf result = STRBUF_INIT;
>> -             strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
>> -             dir = strbuf_detach(&result, NULL);
>> -     } else
>> +     if (is_bare)
>> +             dir = xstrfmt("%.*s.git", (int)len, start);
>> +     else
>>               dir = xstrndup(start, end - start);
>
> The last one needs to be adjusted with s/end - start/len/.  The
> last-minute rewrite without testing shows; your first two patches
> correctly used "len" ;-)

Doh, you're right, sorry for that.

> No need to resend.  Will locally tweak before queuing.

Thanks!

-- 
Sebastian Schuberth

Re: [PATCH v4] clone: simplify string handling in guess_dir_name()

[Lukas Fleischer]

On Thu, 09 Jul 2015 at 20:24:08, Sebastian Schuberth wrote:
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
>  builtin/clone.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 00535d0..ebcb849 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
>  static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
>  {
>         const char *end = repo + strlen(repo), *start;
> +       size_t len;
>         char *dir;
>  
>         /*
> @@ -173,19 +174,11 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
>         /*
>          * Strip .{bundle,git}.
>          */
> -       if (is_bundle) {
> -               if (end - start > 7 && !strncmp(end - 7, ".bundle", 7))
> -                       end -= 7;
> -       } else {
> -               if (end - start > 4 && !strncmp(end - 4, ".git", 4))
> -                       end -= 4;
> -       }
> +       strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
> [...]

I am currently on vacation and cannot bisect or debug this but I am
pretty confident that this patch changes the behaviour of directory name
guessing. With Git 2.4.6, cloning http://foo.bar/foo.git/ results in a
directory named foo and with Git 2.5.0, the resulting directory is
called foo.git.

Note how the end variable is decreased when the repository name ends
with a slash but that isn't taken into account when simply using
strip_suffix() later...

Is this intended?

Re: [PATCH v4] clone: simplify string handling in guess_dir_name()

[Sebastian Schuberth]

On Tue, Aug 4, 2015 at 6:34 AM, Lukas Fleischer <lfleischer@lfos.de> wrote:

> I am currently on vacation and cannot bisect or debug this but I am
> pretty confident that this patch changes the behaviour of directory name
> guessing. With Git 2.4.6, cloning http://foo.bar/foo.git/ results in a
> directory named foo and with Git 2.5.0, the resulting directory is
> called foo.git.
>
> Note how the end variable is decreased when the repository name ends
> with a slash but that isn't taken into account when simply using
> strip_suffix() later...
>
> Is this intended?

I did not intend this change in behavior, and I can confirm that
reverting my patch restores the original behavior. Thanks for bringing
this to my attention, I'll work on a patch.

-- 
Sebastian Schuberth

Re: [PATCH v4] clone: simplify string handling in guess_dir_name()

[Jeff King]

On Tue, Aug 04, 2015 at 09:31:18AM +0200, Sebastian Schuberth wrote:

> On Tue, Aug 4, 2015 at 6:34 AM, Lukas Fleischer <lfleischer@lfos.de> wrote:
> 
> > I am currently on vacation and cannot bisect or debug this but I am
> > pretty confident that this patch changes the behaviour of directory name
> > guessing. With Git 2.4.6, cloning http://foo.bar/foo.git/ results in a
> > directory named foo and with Git 2.5.0, the resulting directory is
> > called foo.git.
> >
> > Note how the end variable is decreased when the repository name ends
> > with a slash but that isn't taken into account when simply using
> > strip_suffix() later...
> >
> > Is this intended?
> 
> I did not intend this change in behavior, and I can confirm that
> reverting my patch restores the original behavior. Thanks for bringing
> this to my attention, I'll work on a patch.

I think this regression is in v2.4.8, as well. We should be able to use
a running "len" instead of the "end" pointer in the earlier part, and
then use strip_suffix_mem later (to strip from our already-reduced
length, rather than the full NUL-terminated string). Like this:

diff --git a/builtin/clone.c b/builtin/clone.c
index 303a3a7..4b61e4c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -146,20 +146,19 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
-	const char *end = repo + strlen(repo), *start;
-	size_t len;
+	const char *start;
+	size_t len = strlen(repo);
 	char *dir;
 
 	/*
 	 * Strip trailing spaces, slashes and /.git
 	 */
-	while (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))
-		end--;
-	if (end - repo > 5 && is_dir_sep(end[-5]) &&
-	    !strncmp(end - 4, ".git", 4)) {
-		end -= 5;
-		while (repo < end && is_dir_sep(end[-1]))
-			end--;
+	while (len > 0 && (is_dir_sep(repo[len-1]) || isspace(repo[len-1])))
+		len--;
+	if (len > 5 && is_dir_sep(repo[len-5]) &&
+	    strip_suffix_mem(repo, &len, ".git")) {
+		while (len > 0 && is_dir_sep(repo[len-1]))
+			len--;
 	}
 
 	/*
@@ -167,14 +166,14 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	 * the form  "remote.example.com:foo.git", i.e. no slash
 	 * in the directory part.
 	 */
-	start = end;
+	start = repo + len;
 	while (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')
 		start--;
 
 	/*
 	 * Strip .{bundle,git}.
 	 */
-	strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
+	strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git");
 
 	if (is_bare)
 		dir = xstrfmt("%.*s.git", (int)len, start);
@@ -187,6 +186,7 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	if (*dir) {
 		char *out = dir;
 		int prev_space = 1 /* strip leading whitespace */;
+		const char *end;
 		for (end = dir; *end; ++end) {
 			char ch = *end;
 			if ((unsigned char)ch < '\x20')

Sadly we cannot just `strip_suffix_mem(repo, &len, "/.git"))` in the
earlier code, as we have to account for multiple directory separators. I
believe the above code does the right thing, though. I haven't looked at
how badly it interacts with the other guess_dir_name work from Patrick
Steinhardt that has been going on, though.

-Peff

Re: [PATCH v4] clone: simplify string handling in guess_dir_name()

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On Tue, Aug 04, 2015 at 06:42:46PM -0400, Jeff King wrote:
> On Tue, Aug 04, 2015 at 09:31:18AM +0200, Sebastian Schuberth wrote:
[snip]
> Sadly we cannot just `strip_suffix_mem(repo, &len, "/.git"))` in the
> earlier code, as we have to account for multiple directory separators. I
> believe the above code does the right thing, though. I haven't looked at
> how badly it interacts with the other guess_dir_name work from Patrick
> Steinhardt that has been going on, though.
> 
> -Peff

It shouldn't be hard rebasing my work onto this. If it's being
applied I'll come up with a new version.

Patrick

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 0/2] fix clone guess_dir_name regression in v2.4.8

[Jeff King]

On Tue, Aug 04, 2015 at 06:42:46PM -0400, Jeff King wrote:

> > I did not intend this change in behavior, and I can confirm that
> > reverting my patch restores the original behavior. Thanks for bringing
> > this to my attention, I'll work on a patch.
> 
> I think this regression is in v2.4.8, as well. We should be able to use
> a running "len" instead of the "end" pointer in the earlier part, and
> then use strip_suffix_mem later (to strip from our already-reduced
> length, rather than the full NUL-terminated string). Like this:

Looks like "git clone --bare host:foo/.git" is broken, too. I've added
some tests to cover the recently broken cases, as well as some obvious
normal cases (which the patch I sent earlier break!). And as a bonus, we
can easily cover Patrick's root-repo problems (so people will actually
run the tests, unlike the stuff in t1509. :) ).

> @@ -167,14 +166,14 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
>  	 * the form  "remote.example.com:foo.git", i.e. no slash
>  	 * in the directory part.
>  	 */
> -	start = end;
> +	start = repo + len;
>  	while (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')
>  		start--;
>  
>  	/*
>  	 * Strip .{bundle,git}.
>  	 */
> -	strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
> +	strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git");

This is crap, of course. Our "len" variable is computed from the start
of "repo", of which "start" is a subset. So we are indexing way out of
bounds here.

As it turns out, this actually makes things simpler. We can stop using
"len" entirely in the early part, and leave it as-is with pointer math
(the patch I sent earlier did not really make anything simpler, anyway).
And then we can just compute the length of "start" here, minus
everything we've stripped off the end (i.e., "len = end - start").

Here are the patches.

  [1/2]: clone: add tests for output directory
  [2/2]: clone: use computed length in guess_dir_name

-Peff

[PATCH 1/2] clone: add tests for output directory

[Jeff King]

When we run "git clone $url", clone guesses from the $url
what to name the local output directory. We don't have any
test coverage of this, so let's add some basic tests.

This reveals a few problems:

  - cloning "foo.git/" does not properly remove the ".git";
    this is a recent regression from 7e837c6 (clone:
    simplify string handling in guess_dir_name(), 2015-07-09)

  - likewise, cloning foo/.git does not seem to handle the
    bare case (we should end up in foo.git, but we try to
    use foo/.git on the local end), which also comes from
    7e837c6.

  - cloning the root is not very smart about URL parsing,
    and usernames and port numbers may end up in the
    directory name

All of these tests are marked as failures.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5603-clone-dirname.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100755 t/t5603-clone-dirname.sh

diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
new file mode 100755
index 0000000..a0140b9
--- /dev/null
+++ b/t/t5603-clone-dirname.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='check output directory names used by git-clone'
+. ./test-lib.sh
+
+# we use a fake ssh wrapper that ignores the arguments
+# entirely; we really only care that we get _some_ repo,
+# as the real test is what clone does on the local side
+test_expect_success 'setup ssh wrapper' '
+	write_script "$TRASH_DIRECTORY/ssh-wrapper" <<-\EOF &&
+	git upload-pack "$TRASH_DIRECTORY"
+	EOF
+	GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper" &&
+	export GIT_SSH &&
+	export TRASH_DIRECTORY
+'
+
+# make sure that cloning $1 results in local directory $2
+test_clone_dir () {
+	url=$1; shift
+	dir=$1; shift
+	expect=success
+	bare=non-bare
+	clone_opts=
+	for i in "$@"; do
+		case "$i" in
+		fail)
+			expect=failure
+			;;
+		bare)
+			bare=bare
+			clone_opts=--bare
+			;;
+		esac
+	done
+	test_expect_$expect "clone of $url goes to $dir ($bare)" "
+		rm -rf $dir &&
+		git clone $clone_opts $url &&
+		test_path_is_dir $dir
+	"
+}
+
+# basic syntax with bare and non-bare variants
+test_clone_dir host:foo foo
+test_clone_dir host:foo foo.git bare
+test_clone_dir host:foo.git foo
+test_clone_dir host:foo.git foo.git bare
+test_clone_dir host:foo/.git foo
+test_clone_dir host:foo/.git foo.git bare fail
+
+# similar, but using ssh URL rather than host:path syntax
+test_clone_dir ssh://host/foo foo
+test_clone_dir ssh://host/foo foo.git bare
+test_clone_dir ssh://host/foo.git foo
+test_clone_dir ssh://host/foo.git foo.git bare
+test_clone_dir ssh://host/foo/.git foo
+test_clone_dir ssh://host/foo/.git foo.git bare fail
+
+# we should remove trailing slashes
+test_clone_dir ssh://host/foo/ foo
+test_clone_dir ssh://host/foo.git/ foo fail
+test_clone_dir ssh://host/foo/.git/ foo
+
+# omitting the path should default to the hostname
+test_clone_dir ssh://host/ host
+test_clone_dir ssh://host:1234/ host fail
+test_clone_dir ssh://user@host/ host fail
+
+test_done
-- 
2.5.0.148.g63828c1

[PATCH 2/2] clone: use computed length in guess_dir_name

[Jeff King]

Commit 7e837c6 (clone: simplify string handling in
guess_dir_name(), 2015-07-09) changed clone to use
strip_suffix instead of hand-rolled pointer manipulation.
However, strip_suffix will strip from the end of a
NUL-terminated string, and we may have already stripped some
characters (like directory separators, or "/.git"). This
leads to commands like:

  git clone host:foo.git/

failing to strip the ".git".

We must instead convert our pointer arithmetic into a
computed length and feed that to strip_suffix_mem, which will
then reduce the length further for us.

It would be nicer if we could drop the pointer manipulation
entirely, and just continually strip using strip_suffix. But
that doesn't quite work for two reasons:

  1. The early suffixes we're stripping are not constant; we
     need to look for is_dir_sep, which could be one of
     several characters.

  2. Mid-way through the stripping we compute the pointer
     "start", which shows us the beginning of the pathname.
     Which really give us two lengths to work with: the
     offset from the start of the string, and from the start
     of the path. By using pointers for the early part, we
     can just compute the length from "start" when we need
     it.

Signed-off-by: Jeff King <peff@peff.net>
---
I suspect you _could_ clean up this logic further, but I
really wanted to do the minimal fix for the regression.
Especially because Patrick is hopefully going to sweep
through and make it all more robust soon enough. :)

 builtin/clone.c          | 3 ++-
 t/t5603-clone-dirname.sh | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 303a3a7..bf45199 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -174,7 +174,8 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	/*
 	 * Strip .{bundle,git}.
 	 */
-	strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
+	len = end - start;
+	strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git");
 
 	if (is_bare)
 		dir = xstrfmt("%.*s.git", (int)len, start);
diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index a0140b9..46725b9 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh
@@ -46,7 +46,7 @@ test_clone_dir host:foo foo.git bare
 test_clone_dir host:foo.git foo
 test_clone_dir host:foo.git foo.git bare
 test_clone_dir host:foo/.git foo
-test_clone_dir host:foo/.git foo.git bare fail
+test_clone_dir host:foo/.git foo.git bare
 
 # similar, but using ssh URL rather than host:path syntax
 test_clone_dir ssh://host/foo foo
@@ -54,11 +54,11 @@ test_clone_dir ssh://host/foo foo.git bare
 test_clone_dir ssh://host/foo.git foo
 test_clone_dir ssh://host/foo.git foo.git bare
 test_clone_dir ssh://host/foo/.git foo
-test_clone_dir ssh://host/foo/.git foo.git bare fail
+test_clone_dir ssh://host/foo/.git foo.git bare
 
 # we should remove trailing slashes
 test_clone_dir ssh://host/foo/ foo
-test_clone_dir ssh://host/foo.git/ foo fail
+test_clone_dir ssh://host/foo.git/ foo
 test_clone_dir ssh://host/foo/.git/ foo
 
 # omitting the path should default to the hostname
-- 
2.5.0.148.g63828c1

Re: [PATCH v4] clone: simplify string handling in guess_dir_name()

[Jeff King]

On Wed, Aug 05, 2015 at 08:08:52AM +0200, Patrick Steinhardt wrote:

> > Sadly we cannot just `strip_suffix_mem(repo, &len, "/.git"))` in the
> > earlier code, as we have to account for multiple directory separators. I
> > believe the above code does the right thing, though. I haven't looked at
> > how badly it interacts with the other guess_dir_name work from Patrick
> > Steinhardt that has been going on, though.
> 
> It shouldn't be hard rebasing my work onto this. If it's being
> applied I'll come up with a new version.

Thanks, it is always nice when contributors are flexible and easy to
work with. :)

Hopefully the new tests I've added can help you out, as well.

-Peff

Re: [PATCH 2/2] clone: use computed length in guess_dir_name

[Sebastian Schuberth]

On 8/5/2015 10:39, Jeff King wrote:

> Commit 7e837c6 (clone: simplify string handling in
> guess_dir_name(), 2015-07-09) changed clone to use
> strip_suffix instead of hand-rolled pointer manipulation.
> However, strip_suffix will strip from the end of a
> NUL-terminated string, and we may have already stripped some
> characters (like directory separators, or "/.git"). This
> leads to commands like:
> 
>    git clone host:foo.git/
> 
> failing to strip the ".git".

Thanks a lot Peff for fixing my bugs, I should have known that you'll be able to come up with something much sooner than I would ;-)

This all looks good to me!

Regards,
Sebastian

Re: [PATCH v4] clone: simplify string handling in guess_dir_name()

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

On Wed, Aug 05, 2015 at 04:41:48AM -0400, Jeff King wrote:
> On Wed, Aug 05, 2015 at 08:08:52AM +0200, Patrick Steinhardt wrote:
> 
> > > Sadly we cannot just `strip_suffix_mem(repo, &len, "/.git"))` in the
> > > earlier code, as we have to account for multiple directory separators. I
> > > believe the above code does the right thing, though. I haven't looked at
> > > how badly it interacts with the other guess_dir_name work from Patrick
> > > Steinhardt that has been going on, though.
> > 
> > It shouldn't be hard rebasing my work onto this. If it's being
> > applied I'll come up with a new version.
> 
> Thanks, it is always nice when contributors are flexible and easy to
> work with. :)
> 
> Hopefully the new tests I've added can help you out, as well.
> 
> -Peff

You're welcome. And yes, your tests help me quite a lot here. Got
tedious to always set up the chroot. Guess I'll still send my
fixes for the chroot-tests as a separate patch series, even
though I don't require them anymore.

Short question on how to proceed: should I mention that my patch
series builds upon your patches or just include them in my
series?

Patrick

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4] clone: simplify string handling in guess_dir_name()

[Jeff King]

On Wed, Aug 05, 2015 at 11:06:03AM +0200, Patrick Steinhardt wrote:

> You're welcome. And yes, your tests help me quite a lot here. Got
> tedious to always set up the chroot. Guess I'll still send my
> fixes for the chroot-tests as a separate patch series, even
> though I don't require them anymore.

Yeah, fixes for t1509 are definitely welcome.

> Short question on how to proceed: should I mention that my patch
> series builds upon your patches or just include them in my
> series?

I think we'll want to merge my patches separately, due to the
regression, so you should not include them. So hopefully the sequence
is:

  1. Junio picks them up as jk/guess-repo-name-regression or similar.

  2. They are merged to 'next', and then eventually to 'maint'.

  3. Mention the topic branch name (whatever Junio ends up picking for
     it) when you post your patches. Junio can then apply on top in his
     repo.

  4. If re-rolls keep going past step 2, then it becomes a non-issue, as
     'maint' gets merged to 'master'.

-Peff

Re: [PATCH 0/2] fix clone guess_dir_name regression in v2.4.8

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Aug 04, 2015 at 06:42:46PM -0400, Jeff King wrote:
>
>> > I did not intend this change in behavior, and I can confirm that
>> > reverting my patch restores the original behavior. Thanks for bringing
>> > this to my attention, I'll work on a patch.
>> 
>> I think this regression is in v2.4.8, as well. We should be able to use
>> a running "len" instead of the "end" pointer in the earlier part, and
>> then use strip_suffix_mem later (to strip from our already-reduced
>> length, rather than the full NUL-terminated string). Like this:
>
> Looks like "git clone --bare host:foo/.git" is broken, too. I've added
> some tests to cover the recently broken cases, as well as some obvious
> normal cases (which the patch I sent earlier break!). And as a bonus, we
> can easily cover Patrick's root-repo problems (so people will actually
> run the tests, unlike the stuff in t1509. :) ).

Sorry, my fault; I should have been much less trusting while queuing
a patch like that offending one that was meant to be a no-op.

> Here are the patches.
>
>   [1/2]: clone: add tests for output directory
>   [2/2]: clone: use computed length in guess_dir_name
>
Thanks.

Re: [PATCH 0/2] fix clone guess_dir_name regression in v2.4.8

[Jeff King]

On Wed, Aug 05, 2015 at 10:19:56AM -0700, Junio C Hamano wrote:

> >> I think this regression is in v2.4.8, as well. We should be able to use
> >> a running "len" instead of the "end" pointer in the earlier part, and
> >> then use strip_suffix_mem later (to strip from our already-reduced
> >> length, rather than the full NUL-terminated string). Like this:
> >
> > Looks like "git clone --bare host:foo/.git" is broken, too. I've added
> > some tests to cover the recently broken cases, as well as some obvious
> > normal cases (which the patch I sent earlier break!). And as a bonus, we
> > can easily cover Patrick's root-repo problems (so people will actually
> > run the tests, unlike the stuff in t1509. :) ).
> 
> Sorry, my fault; I should have been much less trusting while queuing
> a patch like that offending one that was meant to be a no-op.

I reviewed it, too. :-/

I actually did give some thought to that while working on the fix. Why
did we miss what in retrospect was a pretty obvious bug? I saw two
interesting bits:

  1. From the diff context, it looked like a perfectly reasonable
     change; the shrinking of the "end" pointer happened further up
     in the function.

     So I guess the lesson is not to trust reading just the diff, and
     to really read the whole of the modified function. But that's easy
     to say in retrospect; most of the time the bits outside the context
     aren't interesting, and we can't afford to read the whole code
     base for each patch. It's a judgement call where to stop looking at
     the surrounding context of a given change (e.g., the function, the
     callers, their callers, etc).

  2. We didn't have any test coverage in this area; when I wrote even
     basic tests, it caught the problem.

     I hate to set a rule like "if you are cleaning something up, make
     sure there is decent test coverage". Lots of trivial-looking
     patches really are trivial, and it doesn't make sense to insist the
     submitter add a new battery of tests.

So I dunno. This was definitely preventable, but that is all in
retrospect. Bugs will happen, and we usually catch them while cooking.
The biggest pain is that this slipped through to a release, and that may
just be a measure of how few people were impacted (the cases it affected
were relatively obscure).

-Peff