{"thread":{"id":"39821","subject":"[PATCH] clone: Make use of the strip_suffix() helper method","startedAt":"2015-07-09T15:33:46Z","lastAt":"2015-08-05T21:04:54Z","messageCount":24,"participants":["Sebastian Schuberth","Jeff King","Junio C Hamano","Lukas Fleischer","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"265925","messageId":"0000014e73738297-cce3a38b-a85d-40be-b501-354686c25eee-000000@eu-west-1.amazonses.com","threadId":"39821","inReplyTo":null,"subject":"[PATCH] clone: Make use of the strip_suffix() helper method","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-07-09T15:33:46Z","receivedAt":"2015-07-09T15:33:46Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Signed-off-by: Sebastian Schuberth \n---\n builtin/clone.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 00535d0..d35b2b9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n {\n \tconst char *end = repo + strlen(repo), *start;\n+\tsize_t len;\n \tchar *dir;\n \n \t/*\n@@ -174,19 +175,17 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t * Strip .{bundle,git}.\n \t */\n \tif (is_bundle) {\n-\t\tif (end - start > 7 && !strncmp(end - 7, \".bundle\", 7))\n-\t\t\tend -= 7;\n+\t\tstrip_suffix(start, \".bundle\", &len);\n \t} else {\n-\t\tif (end - start > 4 && !strncmp(end - 4, \".git\", 4))\n-\t\t\tend -= 4;\n+\t\tstrip_suffix(start, \".git\", &len);\n \t}\n \n \tif (is_bare) {\n \t\tstruct strbuf result = STRBUF_INIT;\n-\t\tstrbuf_addf(&result, \"%.*s.git\", (int)(end - start), start);\n+\t\tstrbuf_addf(&result, \"%.*s.git\", len, start);\n \t\tdir = strbuf_detach(&result, NULL);\n \t} else\n-\t\tdir = xstrndup(start, end - start);\n+\t\tdir = xstrndup(start, len);\n \t/*\n \t * Replace sequences of 'control' characters and whitespace\n \t * with one ascii space, remove leading and trailing spaces.\n\n---\nhttps://github.com/git/git/pull/160"},{"id":"265926","messageId":"20150709170054.GA15820@peff.net","threadId":"39821","inReplyTo":"0000014e73738297-cce3a38b-a85d-40be-b501-354686c25eee-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] clone: Make use of the strip_suffix() helper method","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-09T17:00:54Z","receivedAt":"2015-07-09T17:00:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 09, 2015 at 03:33:46PM +0000, Sebastian Schuberth wrote:\n\n> @@ -174,19 +175,17 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n> \t * Strip .{bundle,git}.\n> \t */\n> \tif (is_bundle) {\n> -\t\tif (end - start > 7 && !strncmp(end - 7, \".bundle\", 7))\n> -\t\t\tend -= 7;\n> +\t\tstrip_suffix(start, \".bundle\", &len);\n> \t} else {\n> -\t\tif (end - start > 4 && !strncmp(end - 4, \".git\", 4))\n> -\t\t\tend -= 4;\n> +\t\tstrip_suffix(start, \".git\", &len);\n> \t}\n\nYay, always glad to see complicated string handling like this go away.\nAs the resulting conditional blocks are one-liners, I think you can drop\nthe curly braces, which will match our usual style:\n\n if (is_bundle)\n\tstrip_suffix(start, \".bundle\", &len);\n else\n\tstrip_suffix(start, \".git\", &len);\n\nIf you wanted to get really fancy, I think you could put a ternary\noperator in the middle of the strip_suffix call. That makes it clear\nthat \"len\" is set in all code paths, but I think some people find\nternary operators unreadable. :)\n\n> \tif (is_bare) {\n> \t\tstruct strbuf result = STRBUF_INIT;\n> -\t\tstrbuf_addf(&result, \"%.*s.git\", (int)(end - start), start);\n> +\t\tstrbuf_addf(&result, \"%.*s.git\", len, start);\n> \t\tdir = strbuf_detach(&result, NULL);\n\nThis one can also be simplified using xstrfmt to:\n\n if (is_bare)\n\tdir = xstrfmt(\"%.*s.git\", len, start);\n\nDo we still need to cast \"len\" to an int to use it with \"%.*\" (it is\ndefined by the standard as an int, not a size_t)?\n\n-Peff\n"},{"id":"265927","messageId":"CAHGBnuPkia6UYeN4jekfGzypV2MpyiMs2W+O=SSJR3hR=K3g0A@mail.gmail.com","threadId":"39821","inReplyTo":"20150709170054.GA15820@peff.net","subject":"Re: [PATCH] clone: Make use of the strip_suffix() helper method","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-07-09T17:16:33Z","receivedAt":"2015-07-09T17:16:33Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, Jul 9, 2015 at 7:00 PM, Jeff King wrote:\n\n> If you wanted to get really fancy, I think you could put a ternary\n> operator in the middle of the strip_suffix call. That makes it clear\n> that \"len\" is set in all code paths, but I think some people find\n> ternary operators unreadable. :)\n\nI like the idea about the ternary operator, will do.\n\n> This one can also be simplified using xstrfmt to:\n\nNice, will also do.\n\n> Do we still need to cast \"len\" to an int to use it with \"%.*\" (it is\n> defined by the standard as an int, not a size_t)?\n\nI think we're more on the safe side by keeping the cast, so I'll do that, too.\n\n-- \nSebastian Schuberth\n"},{"id":"265929","messageId":"0000014e73d7c3d8-413991dd-3907-430c-ab99-a0a3d93dcab0-000000@eu-west-1.amazonses.com","threadId":"39821","inReplyTo":"CAHGBnuPkia6UYeN4jekfGzypV2MpyiMs2W+O=SSJR3hR=K3g0A@mail.gmail.com","subject":"[PATCH v2] clone: Simplify string handling in guess_dir_name()","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-07-09T17:23:17Z","receivedAt":"2015-07-09T17:23:17Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Signed-off-by: Sebastian Schuberth \n---\n builtin/clone.c | 16 +++-------------\n 1 file changed, 3 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 00535d0..afdc004 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n {\n \tconst char *end = repo + strlen(repo), *start;\n+\tsize_t len;\n \tchar *dir;\n \n \t/*\n@@ -173,20 +174,9 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t/*\n \t * Strip .{bundle,git}.\n \t */\n-\tif (is_bundle) {\n-\t\tif (end - start > 7 && !strncmp(end - 7, \".bundle\", 7))\n-\t\t\tend -= 7;\n-\t} else {\n-\t\tif (end - start > 4 && !strncmp(end - 4, \".git\", 4))\n-\t\t\tend -= 4;\n-\t}\n+\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n \n-\tif (is_bare) {\n-\t\tstruct strbuf result = STRBUF_INIT;\n-\t\tstrbuf_addf(&result, \"%.*s.git\", (int)(end - start), start);\n-\t\tdir = strbuf_detach(&result, NULL);\n-\t} else\n-\t\tdir = xstrndup(start, end - start);\n+\tdir = is_bare ? xstrfmt(\"%.*s.git\", (int)len, start) : xstrndup(start, len);\n \t/*\n \t * Replace sequences of 'control' characters and whitespace\n \t * with one ascii space, remove leading and trailing spaces.\n\n---\nhttps://github.com/git/git/pull/160"},{"id":"265933","messageId":"xmqq1tghw6jz.fsf@gitster.dls.corp.google.com","threadId":"39821","inReplyTo":"0000014e73d7c3d8-413991dd-3907-430c-ab99-a0a3d93dcab0-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v2] clone: Simplify string handling in guess_dir_name()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-09T18:05:04Z","receivedAt":"2015-07-09T18:05:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> Subject: Re: [PATCH v2] clone: Simplify string handling in guess_dir_name()\n\nWe seem not to capitalize the first word on the subject line.\n\n> Content-Type: multipart/mixed; boundary=\"----=_Part_8_836493213.1436462597065\"\n\nPlease don't.\n\n> Signed-off-by: Sebastian Schuberth \n> ---\n> builtin/clone.c | 16 +++-------------\n> 1 file changed, 3 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 00535d0..afdc004 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n> static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n> {\n> \tconst char *end = repo + strlen(repo), *start;\n> +\tsize_t len;\n> \tchar *dir;\n> \n> \t/*\n> @@ -173,20 +174,9 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n> \t/*\n> \t * Strip .{bundle,git}.\n> \t */\n> -\tif (is_bundle) {\n> -\t\tif (end - start > 7 && !strncmp(end - 7, \".bundle\", 7))\n> -\t\t\tend -= 7;\n> -\t} else {\n> -\t\tif (end - start > 4 && !strncmp(end - 4, \".git\", 4))\n> -\t\t\tend -= 4;\n> -\t}\n> +\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n\nThis looks vastly nicer than the original.\n\n> -\tif (is_bare) {\n> -\t\tstruct strbuf result = STRBUF_INIT;\n> -\t\tstrbuf_addf(&result, \"%.*s.git\", (int)(end - start), start);\n> -\t\tdir = strbuf_detach(&result, NULL);\n> -\t} else\n> -\t\tdir = xstrndup(start, end - start);\n> +\tdir = is_bare ? xstrfmt(\"%.*s.git\", (int)len, start) : xstrndup(start, len);\n\nThis however I had to read twice. I'd say\n\n\tif (is_bare)\n \tdir = xstrfmt(...);\n\telse\n \tdir = xstrndup(...);\n\nis much easier to read.\n"},{"id":"265935","messageId":"CAHGBnuNLoNsxPK4YQ+HnT_q8F-HrVC_y9pZwB4G88jCq0-wCPg@mail.gmail.com","threadId":"39821","inReplyTo":"xmqq1tghw6jz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] clone: Simplify string handling in guess_dir_name()","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-07-09T18:16:42Z","receivedAt":"2015-07-09T18:16:42Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, Jul 9, 2015 at 8:05 PM, Junio C Hamano wrote:\n\n>> Subject: Re: [PATCH v2] clone: Simplify string handling in guess_dir_name()\n>\n> We seem not to capitalize the first word on the subject line.\n\nWill change that.\n\n>> Content-Type: multipart/mixed; boundary=\"----=_Part_8_836493213.1436462597065\"\n>\n> Please don't.\n\nThis seems to come from submitgit, I've filed an issue about it:\n\nhttps://github.com/rtyley/submitgit/issues/17\n\nWhat content type(s) would you accept? Only text/plain?\n\n>> - if (is_bare) {\n>> - struct strbuf result = STRBUF_INIT;\n>> - strbuf_addf(&result, \"%.*s.git\", (int)(end - start), start);\n>> - dir = strbuf_detach(&result, NULL);\n>> - } else\n>> - dir = xstrndup(start, end - start);\n>> + dir = is_bare ? xstrfmt(\"%.*s.git\", (int)len, start) : xstrndup(start, len);\n>\n> This however I had to read twice. I'd say\n>\n> if (is_bare)\n> dir = xstrfmt(...);\n> else\n> dir = xstrndup(...);\n>\n> is much easier to read.\n\nThat's what I had locally before. Will revert to that.\n\n-- \nSebastian Schuberth\n"},{"id":"265938","messageId":"0000014e740bffe3-3193405e-55b1-4526-96d2-fd059a75f6e8-000000@eu-west-1.amazonses.com","threadId":"39821","inReplyTo":"CAHGBnuNLoNsxPK4YQ+HnT_q8F-HrVC_y9pZwB4G88jCq0-wCPg@mail.gmail.com","subject":"[PATCH v3] clone: Simplify string handling in guess_dir_name()","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-07-09T18:20:20Z","receivedAt":"2015-07-09T18:20:20Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Signed-off-by: Sebastian Schuberth \n---\n builtin/clone.c | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 00535d0..ebcb849 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n {\n \tconst char *end = repo + strlen(repo), *start;\n+\tsize_t len;\n \tchar *dir;\n \n \t/*\n@@ -173,19 +174,11 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t/*\n \t * Strip .{bundle,git}.\n \t */\n-\tif (is_bundle) {\n-\t\tif (end - start > 7 && !strncmp(end - 7, \".bundle\", 7))\n-\t\t\tend -= 7;\n-\t} else {\n-\t\tif (end - start > 4 && !strncmp(end - 4, \".git\", 4))\n-\t\t\tend -= 4;\n-\t}\n+\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n \n-\tif (is_bare) {\n-\t\tstruct strbuf result = STRBUF_INIT;\n-\t\tstrbuf_addf(&result, \"%.*s.git\", (int)(end - start), start);\n-\t\tdir = strbuf_detach(&result, NULL);\n-\t} else\n+\tif (is_bare)\n+\t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\n+\telse\n \t\tdir = xstrndup(start, end - start);\n \t/*\n \t * Replace sequences of 'control' characters and whitespace\n\n---\nhttps://github.com/git/git/pull/160"},{"id":"265939","messageId":"0000014e740f7a8a-2c988a36-633e-4b30-8024-cb4a1de1a8a2-000000@eu-west-1.amazonses.com","threadId":"39821","inReplyTo":"CAHGBnuNLoNsxPK4YQ+HnT_q8F-HrVC_y9pZwB4G88jCq0-wCPg@mail.gmail.com","subject":"[PATCH v4] clone: simplify string handling in guess_dir_name()","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-07-09T18:24:08Z","receivedAt":"2015-07-09T18:24:08Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Signed-off-by: Sebastian Schuberth \n---\n builtin/clone.c | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 00535d0..ebcb849 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n {\n \tconst char *end = repo + strlen(repo), *start;\n+\tsize_t len;\n \tchar *dir;\n \n \t/*\n@@ -173,19 +174,11 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t/*\n \t * Strip .{bundle,git}.\n \t */\n-\tif (is_bundle) {\n-\t\tif (end - start > 7 && !strncmp(end - 7, \".bundle\", 7))\n-\t\t\tend -= 7;\n-\t} else {\n-\t\tif (end - start > 4 && !strncmp(end - 4, \".git\", 4))\n-\t\t\tend -= 4;\n-\t}\n+\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n \n-\tif (is_bare) {\n-\t\tstruct strbuf result = STRBUF_INIT;\n-\t\tstrbuf_addf(&result, \"%.*s.git\", (int)(end - start), start);\n-\t\tdir = strbuf_detach(&result, NULL);\n-\t} else\n+\tif (is_bare)\n+\t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\n+\telse\n \t\tdir = xstrndup(start, end - start);\n \t/*\n \t * Replace sequences of 'control' characters and whitespace\n\n---\nhttps://github.com/git/git/pull/160"},{"id":"265937","messageId":"xmqqsi8xuqcb.fsf@gitster.dls.corp.google.com","threadId":"39821","inReplyTo":"CAHGBnuNLoNsxPK4YQ+HnT_q8F-HrVC_y9pZwB4G88jCq0-wCPg@mail.gmail.com","subject":"Re: [PATCH v2] clone: Simplify string handling in guess_dir_name()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-09T18:40:36Z","receivedAt":"2015-07-09T18:40:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On Thu, Jul 9, 2015 at 8:05 PM, Junio C Hamano wrote:\n>\n>>> Content-Type: multipart/mixed; boundary=\"----=_Part_8_836493213.1436462597065\"\n>>\n>> Please don't.\n>\n> This seems to come from submitgit, I've filed an issue about it:\n>\n> https://github.com/rtyley/submitgit/issues/17\n>\n> What content type(s) would you accept? Only text/plain?\n\nI could take anything, even chicken scratches on a piece of paper,\nfor a small change like this.\n\nBut let's make sure we see text/plain out of \"submitgit\", as the\nwhole point of it is to allow people generate the common denominator\nformat out of a commit pushed to GitHub.\n\nThanks for letting them know.\n"},{"id":"265945","messageId":"xmqqfv4xuiwh.fsf@gitster.dls.corp.google.com","threadId":"39821","inReplyTo":"0000014e740f7a8a-2c988a36-633e-4b30-8024-cb4a1de1a8a2-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v4] clone: simplify string handling in guess_dir_name()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-09T21:21:18Z","receivedAt":"2015-07-09T21:21:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> -\tif (is_bare) {\n> -\t\tstruct strbuf result = STRBUF_INIT;\n> -\t\tstrbuf_addf(&result, \"%.*s.git\", (int)(end - start), start);\n> -\t\tdir = strbuf_detach(&result, NULL);\n> -\t} else\n> +\tif (is_bare)\n> +\t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\n> +\telse\n> \t\tdir = xstrndup(start, end - start);\n\nThe last one needs to be adjusted with s/end - start/len/. The\nlast-minute rewrite without testing shows; your first two patches\ncorrectly used \"len\" ;-)\n\nNo need to resend. Will locally tweak before queuing.\n\nThanks.\n"},{"id":"265946","messageId":"CAHGBnuO+Anxc_ftxTMFiYXoxu+9tfiR-zaRSzjTTA7PUwzTWKQ@mail.gmail.com","threadId":"39821","inReplyTo":"xmqqfv4xuiwh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] clone: simplify string handling in guess_dir_name()","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-07-09T21:23:01Z","receivedAt":"2015-07-09T21:23:01Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, Jul 9, 2015 at 11:21 PM, Junio C Hamano wrote:\n\n>> - if (is_bare) {\n>> - struct strbuf result = STRBUF_INIT;\n>> - strbuf_addf(&result, \"%.*s.git\", (int)(end - start), start);\n>> - dir = strbuf_detach(&result, NULL);\n>> - } else\n>> + if (is_bare)\n>> + dir = xstrfmt(\"%.*s.git\", (int)len, start);\n>> + else\n>> dir = xstrndup(start, end - start);\n>\n> The last one needs to be adjusted with s/end - start/len/. The\n> last-minute rewrite without testing shows; your first two patches\n> correctly used \"len\" ;-)\n\nDoh, you're right, sorry for that.\n\n> No need to resend. Will locally tweak before queuing.\n\nThanks!\n\n-- \nSebastian Schuberth\n"},{"id":"267382","messageId":"20150804043401.4494.43725@typhoon","threadId":"39821","inReplyTo":"0000014e740f7a8a-2c988a36-633e-4b30-8024-cb4a1de1a8a2-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v4] clone: simplify string handling in guess_dir_name()","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2015-08-04T04:34:01Z","receivedAt":"2015-08-04T04:34:01Z","isPatch":true,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"On Thu, 09 Jul 2015 at 20:24:08, Sebastian Schuberth wrote:\n> Signed-off-by: Sebastian Schuberth \n> ---\n> builtin/clone.c | 17 +++++------------\n> 1 file changed, 5 insertions(+), 12 deletions(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 00535d0..ebcb849 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -147,6 +147,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n> static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n> {\n> const char *end = repo + strlen(repo), *start;\n> + size_t len;\n> char *dir;\n> \n> /*\n> @@ -173,19 +174,11 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n> /*\n> * Strip .{bundle,git}.\n> */\n> - if (is_bundle) {\n> - if (end - start > 7 && !strncmp(end - 7, \".bundle\", 7))\n> - end -= 7;\n> - } else {\n> - if (end - start > 4 && !strncmp(end - 4, \".git\", 4))\n> - end -= 4;\n> - }\n> + strip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n> [...]\n\nI am currently on vacation and cannot bisect or debug this but I am\npretty confident that this patch changes the behaviour of directory name\nguessing. With Git 2.4.6, cloning http://foo.bar/foo.git/ results in a\ndirectory named foo and with Git 2.5.0, the resulting directory is\ncalled foo.git.\n\nNote how the end variable is decreased when the repository name ends\nwith a slash but that isn't taken into account when simply using\nstrip_suffix() later...\n\nIs this intended?\n"},{"id":"267384","messageId":"CAHGBnuMXkqhFUhen9tPfEsfFAHhbqMeFUxvePS_6A-TtMfZpzg@mail.gmail.com","threadId":"39821","inReplyTo":"20150804043401.4494.43725@typhoon","subject":"Re: [PATCH v4] clone: simplify string handling in guess_dir_name()","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-08-04T07:31:18Z","receivedAt":"2015-08-04T07:31:18Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Aug 4, 2015 at 6:34 AM, Lukas Fleischer wrote:\n\n> I am currently on vacation and cannot bisect or debug this but I am\n> pretty confident that this patch changes the behaviour of directory name\n> guessing. With Git 2.4.6, cloning http://foo.bar/foo.git/ results in a\n> directory named foo and with Git 2.5.0, the resulting directory is\n> called foo.git.\n>\n> Note how the end variable is decreased when the repository name ends\n> with a slash but that isn't taken into account when simply using\n> strip_suffix() later...\n>\n> Is this intended?\n\nI did not intend this change in behavior, and I can confirm that\nreverting my patch restores the original behavior. Thanks for bringing\nthis to my attention, I'll work on a patch.\n\n-- \nSebastian Schuberth\n"},{"id":"267488","messageId":"20150804224246.GA29051@sigill.intra.peff.net","threadId":"39821","inReplyTo":"CAHGBnuMXkqhFUhen9tPfEsfFAHhbqMeFUxvePS_6A-TtMfZpzg@mail.gmail.com","subject":"Re: [PATCH v4] clone: simplify string handling in guess_dir_name()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-04T22:42:46Z","receivedAt":"2015-08-04T22:42:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 04, 2015 at 09:31:18AM +0200, Sebastian Schuberth wrote:\n\n> On Tue, Aug 4, 2015 at 6:34 AM, Lukas Fleischer wrote:\n> \n> > I am currently on vacation and cannot bisect or debug this but I am\n> > pretty confident that this patch changes the behaviour of directory name\n> > guessing. With Git 2.4.6, cloning http://foo.bar/foo.git/ results in a\n> > directory named foo and with Git 2.5.0, the resulting directory is\n> > called foo.git.\n> >\n> > Note how the end variable is decreased when the repository name ends\n> > with a slash but that isn't taken into account when simply using\n> > strip_suffix() later...\n> >\n> > Is this intended?\n> \n> I did not intend this change in behavior, and I can confirm that\n> reverting my patch restores the original behavior. Thanks for bringing\n> this to my attention, I'll work on a patch.\n\nI think this regression is in v2.4.8, as well. We should be able to use\na running \"len\" instead of the \"end\" pointer in the earlier part, and\nthen use strip_suffix_mem later (to strip from our already-reduced\nlength, rather than the full NUL-terminated string). Like this:\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 303a3a7..4b61e4c 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -146,20 +146,19 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n \n static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n {\n-\tconst char *end = repo + strlen(repo), *start;\n-\tsize_t len;\n+\tconst char *start;\n+\tsize_t len = strlen(repo);\n \tchar *dir;\n \n \t/*\n \t * Strip trailing spaces, slashes and /.git\n \t */\n-\twhile (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))\n-\t\tend--;\n-\tif (end - repo > 5 && is_dir_sep(end[-5]) &&\n-\t !strncmp(end - 4, \".git\", 4)) {\n-\t\tend -= 5;\n-\t\twhile (repo < end && is_dir_sep(end[-1]))\n-\t\t\tend--;\n+\twhile (len > 0 && (is_dir_sep(repo[len-1]) || isspace(repo[len-1])))\n+\t\tlen--;\n+\tif (len > 5 && is_dir_sep(repo[len-5]) &&\n+\t strip_suffix_mem(repo, &len, \".git\")) {\n+\t\twhile (len > 0 && is_dir_sep(repo[len-1]))\n+\t\t\tlen--;\n \t}\n \n \t/*\n@@ -167,14 +166,14 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t * the form \"remote.example.com:foo.git\", i.e. no slash\n \t * in the directory part.\n \t */\n-\tstart = end;\n+\tstart = repo + len;\n \twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n \t\tstart--;\n \n \t/*\n \t * Strip .{bundle,git}.\n \t */\n-\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n+\tstrip_suffix_mem(start, &len, is_bundle ? \".bundle\" : \".git\");\n \n \tif (is_bare)\n \t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\n@@ -187,6 +186,7 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \tif (*dir) {\n \t\tchar *out = dir;\n \t\tint prev_space = 1 /* strip leading whitespace */;\n+\t\tconst char *end;\n \t\tfor (end = dir; *end; ++end) {\n \t\t\tchar ch = *end;\n \t\t\tif ((unsigned char)ch < '\\x20')\n\nSadly we cannot just `strip_suffix_mem(repo, &len, \"/.git\"))` in the\nearlier code, as we have to account for multiple directory separators. I\nbelieve the above code does the right thing, though. I haven't looked at\nhow badly it interacts with the other guess_dir_name work from Patrick\nSteinhardt that has been going on, though.\n\n-Peff\n"},{"id":"267495","messageId":"20150805060852.GA1103@pks-pc.localdomain","threadId":"39821","inReplyTo":"20150804224246.GA29051@sigill.intra.peff.net","subject":"Re: [PATCH v4] clone: simplify string handling in guess_dir_name()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-05T06:08:52Z","receivedAt":"2015-08-05T06:08:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 04, 2015 at 06:42:46PM -0400, Jeff King wrote:\n> On Tue, Aug 04, 2015 at 09:31:18AM +0200, Sebastian Schuberth wrote:\n[snip]\n> Sadly we cannot just `strip_suffix_mem(repo, &len, \"/.git\"))` in the\n> earlier code, as we have to account for multiple directory separators. I\n> believe the above code does the right thing, though. I haven't looked at\n> how badly it interacts with the other guess_dir_name work from Patrick\n> Steinhardt that has been going on, though.\n> \n> -Peff\n\nIt shouldn't be hard rebasing my work onto this. If it's being\napplied I'll come up with a new version.\n\nPatrick\n"},{"id":"267497","messageId":"20150805083526.GA22325@sigill.intra.peff.net","threadId":"39821","inReplyTo":"20150804224246.GA29051@sigill.intra.peff.net","subject":"[PATCH 0/2] fix clone guess_dir_name regression in v2.4.8","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-05T08:35:26Z","receivedAt":"2015-08-05T08:35:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 04, 2015 at 06:42:46PM -0400, Jeff King wrote:\n\n> > I did not intend this change in behavior, and I can confirm that\n> > reverting my patch restores the original behavior. Thanks for bringing\n> > this to my attention, I'll work on a patch.\n> \n> I think this regression is in v2.4.8, as well. We should be able to use\n> a running \"len\" instead of the \"end\" pointer in the earlier part, and\n> then use strip_suffix_mem later (to strip from our already-reduced\n> length, rather than the full NUL-terminated string). Like this:\n\nLooks like \"git clone --bare host:foo/.git\" is broken, too. I've added\nsome tests to cover the recently broken cases, as well as some obvious\nnormal cases (which the patch I sent earlier break!). And as a bonus, we\ncan easily cover Patrick's root-repo problems (so people will actually\nrun the tests, unlike the stuff in t1509. :) ).\n\n> @@ -167,14 +166,14 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n> \t * the form \"remote.example.com:foo.git\", i.e. no slash\n> \t * in the directory part.\n> \t */\n> -\tstart = end;\n> +\tstart = repo + len;\n> \twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n> \t\tstart--;\n> \n> \t/*\n> \t * Strip .{bundle,git}.\n> \t */\n> -\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n> +\tstrip_suffix_mem(start, &len, is_bundle ? \".bundle\" : \".git\");\n\nThis is crap, of course. Our \"len\" variable is computed from the start\nof \"repo\", of which \"start\" is a subset. So we are indexing way out of\nbounds here.\n\nAs it turns out, this actually makes things simpler. We can stop using\n\"len\" entirely in the early part, and leave it as-is with pointer math\n(the patch I sent earlier did not really make anything simpler, anyway).\nAnd then we can just compute the length of \"start\" here, minus\neverything we've stripped off the end (i.e., \"len = end - start\").\n\nHere are the patches.\n\n [1/2]: clone: add tests for output directory\n [2/2]: clone: use computed length in guess_dir_name\n\n-Peff\n"},{"id":"267498","messageId":"20150805083645.GA28212@sigill.intra.peff.net","threadId":"39821","inReplyTo":"20150805083526.GA22325@sigill.intra.peff.net","subject":"[PATCH 1/2] clone: add tests for output directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-05T08:36:46Z","receivedAt":"2015-08-05T08:36:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we run \"git clone $url\", clone guesses from the $url\nwhat to name the local output directory. We don't have any\ntest coverage of this, so let's add some basic tests.\n\nThis reveals a few problems:\n\n - cloning \"foo.git/\" does not properly remove the \".git\";\n this is a recent regression from 7e837c6 (clone:\n simplify string handling in guess_dir_name(), 2015-07-09)\n\n - likewise, cloning foo/.git does not seem to handle the\n bare case (we should end up in foo.git, but we try to\n use foo/.git on the local end), which also comes from\n 7e837c6.\n\n - cloning the root is not very smart about URL parsing,\n and usernames and port numbers may end up in the\n directory name\n\nAll of these tests are marked as failures.\n\nSigned-off-by: Jeff King \n---\n t/t5603-clone-dirname.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 69 insertions(+)\n create mode 100755 t/t5603-clone-dirname.sh\n\ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nnew file mode 100755\nindex 0000000..a0140b9\n--- /dev/null\n+++ b/t/t5603-clone-dirname.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+\n+test_description='check output directory names used by git-clone'\n+. ./test-lib.sh\n+\n+# we use a fake ssh wrapper that ignores the arguments\n+# entirely; we really only care that we get _some_ repo,\n+# as the real test is what clone does on the local side\n+test_expect_success 'setup ssh wrapper' '\n+\twrite_script \"$TRASH_DIRECTORY/ssh-wrapper\" <<-\\EOF &&\n+\tgit upload-pack \"$TRASH_DIRECTORY\"\n+\tEOF\n+\tGIT_SSH=\"$TRASH_DIRECTORY/ssh-wrapper\" &&\n+\texport GIT_SSH &&\n+\texport TRASH_DIRECTORY\n+'\n+\n+# make sure that cloning $1 results in local directory $2\n+test_clone_dir () {\n+\turl=$1; shift\n+\tdir=$1; shift\n+\texpect=success\n+\tbare=non-bare\n+\tclone_opts=\n+\tfor i in \"$@\"; do\n+\t\tcase \"$i\" in\n+\t\tfail)\n+\t\t\texpect=failure\n+\t\t\t;;\n+\t\tbare)\n+\t\t\tbare=bare\n+\t\t\tclone_opts=--bare\n+\t\t\t;;\n+\t\tesac\n+\tdone\n+\ttest_expect_$expect \"clone of $url goes to $dir ($bare)\" \"\n+\t\trm -rf $dir &&\n+\t\tgit clone $clone_opts $url &&\n+\t\ttest_path_is_dir $dir\n+\t\"\n+}\n+\n+# basic syntax with bare and non-bare variants\n+test_clone_dir host:foo foo\n+test_clone_dir host:foo foo.git bare\n+test_clone_dir host:foo.git foo\n+test_clone_dir host:foo.git foo.git bare\n+test_clone_dir host:foo/.git foo\n+test_clone_dir host:foo/.git foo.git bare fail\n+\n+# similar, but using ssh URL rather than host:path syntax\n+test_clone_dir ssh://host/foo foo\n+test_clone_dir ssh://host/foo foo.git bare\n+test_clone_dir ssh://host/foo.git foo\n+test_clone_dir ssh://host/foo.git foo.git bare\n+test_clone_dir ssh://host/foo/.git foo\n+test_clone_dir ssh://host/foo/.git foo.git bare fail\n+\n+# we should remove trailing slashes\n+test_clone_dir ssh://host/foo/ foo\n+test_clone_dir ssh://host/foo.git/ foo fail\n+test_clone_dir ssh://host/foo/.git/ foo\n+\n+# omitting the path should default to the hostname\n+test_clone_dir ssh://host/ host\n+test_clone_dir ssh://host:1234/ host fail\n+test_clone_dir ssh://user@host/ host fail\n+\n+test_done\n-- \n2.5.0.148.g63828c1\n"},{"id":"267499","messageId":"20150805083945.GB28212@sigill.intra.peff.net","threadId":"39821","inReplyTo":"20150805083526.GA22325@sigill.intra.peff.net","subject":"[PATCH 2/2] clone: use computed length in guess_dir_name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-05T08:39:46Z","receivedAt":"2015-08-05T08:39:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 7e837c6 (clone: simplify string handling in\nguess_dir_name(), 2015-07-09) changed clone to use\nstrip_suffix instead of hand-rolled pointer manipulation.\nHowever, strip_suffix will strip from the end of a\nNUL-terminated string, and we may have already stripped some\ncharacters (like directory separators, or \"/.git\"). This\nleads to commands like:\n\n git clone host:foo.git/\n\nfailing to strip the \".git\".\n\nWe must instead convert our pointer arithmetic into a\ncomputed length and feed that to strip_suffix_mem, which will\nthen reduce the length further for us.\n\nIt would be nicer if we could drop the pointer manipulation\nentirely, and just continually strip using strip_suffix. But\nthat doesn't quite work for two reasons:\n\n 1. The early suffixes we're stripping are not constant; we\n need to look for is_dir_sep, which could be one of\n several characters.\n\n 2. Mid-way through the stripping we compute the pointer\n \"start\", which shows us the beginning of the pathname.\n Which really give us two lengths to work with: the\n offset from the start of the string, and from the start\n of the path. By using pointers for the early part, we\n can just compute the length from \"start\" when we need\n it.\n\nSigned-off-by: Jeff King \n---\nI suspect you _could_ clean up this logic further, but I\nreally wanted to do the minimal fix for the regression.\nEspecially because Patrick is hopefully going to sweep\nthrough and make it all more robust soon enough. :)\n\n builtin/clone.c | 3 ++-\n t/t5603-clone-dirname.sh | 6 +++---\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 303a3a7..bf45199 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -174,7 +174,8 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t/*\n \t * Strip .{bundle,git}.\n \t */\n-\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n+\tlen = end - start;\n+\tstrip_suffix_mem(start, &len, is_bundle ? \".bundle\" : \".git\");\n \n \tif (is_bare)\n \t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nindex a0140b9..46725b9 100755\n--- a/t/t5603-clone-dirname.sh\n+++ b/t/t5603-clone-dirname.sh\n@@ -46,7 +46,7 @@ test_clone_dir host:foo foo.git bare\n test_clone_dir host:foo.git foo\n test_clone_dir host:foo.git foo.git bare\n test_clone_dir host:foo/.git foo\n-test_clone_dir host:foo/.git foo.git bare fail\n+test_clone_dir host:foo/.git foo.git bare\n \n # similar, but using ssh URL rather than host:path syntax\n test_clone_dir ssh://host/foo foo\n@@ -54,11 +54,11 @@ test_clone_dir ssh://host/foo foo.git bare\n test_clone_dir ssh://host/foo.git foo\n test_clone_dir ssh://host/foo.git foo.git bare\n test_clone_dir ssh://host/foo/.git foo\n-test_clone_dir ssh://host/foo/.git foo.git bare fail\n+test_clone_dir ssh://host/foo/.git foo.git bare\n \n # we should remove trailing slashes\n test_clone_dir ssh://host/foo/ foo\n-test_clone_dir ssh://host/foo.git/ foo fail\n+test_clone_dir ssh://host/foo.git/ foo\n test_clone_dir ssh://host/foo/.git/ foo\n \n # omitting the path should default to the hostname\n-- \n2.5.0.148.g63828c1\n"},{"id":"267500","messageId":"20150805084147.GC28212@sigill.intra.peff.net","threadId":"39821","inReplyTo":"20150805060852.GA1103@pks-pc.localdomain","subject":"Re: [PATCH v4] clone: simplify string handling in guess_dir_name()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-05T08:41:48Z","receivedAt":"2015-08-05T08:41:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 05, 2015 at 08:08:52AM +0200, Patrick Steinhardt wrote:\n\n> > Sadly we cannot just `strip_suffix_mem(repo, &len, \"/.git\"))` in the\n> > earlier code, as we have to account for multiple directory separators. I\n> > believe the above code does the right thing, though. I haven't looked at\n> > how badly it interacts with the other guess_dir_name work from Patrick\n> > Steinhardt that has been going on, though.\n> \n> It shouldn't be hard rebasing my work onto this. If it's being\n> applied I'll come up with a new version.\n\nThanks, it is always nice when contributors are flexible and easy to\nwork with. :)\n\nHopefully the new tests I've added can help you out, as well.\n\n-Peff\n"},{"id":"267501","messageId":"55C1CE07.9010602@gmail.com","threadId":"39821","inReplyTo":"20150805083945.GB28212@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] clone: use computed length in guess_dir_name","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2015-08-05T08:49:11Z","receivedAt":"2015-08-05T08:49:11Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 8/5/2015 10:39, Jeff King wrote:\n\n> Commit 7e837c6 (clone: simplify string handling in\n> guess_dir_name(), 2015-07-09) changed clone to use\n> strip_suffix instead of hand-rolled pointer manipulation.\n> However, strip_suffix will strip from the end of a\n> NUL-terminated string, and we may have already stripped some\n> characters (like directory separators, or \"/.git\"). This\n> leads to commands like:\n> \n> git clone host:foo.git/\n> \n> failing to strip the \".git\".\n\nThanks 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 ;-)\n\nThis all looks good to me!\n\nRegards,\nSebastian\n"},{"id":"267503","messageId":"20150805090603.GC1103@pks-pc.localdomain","threadId":"39821","inReplyTo":"20150805084147.GC28212@sigill.intra.peff.net","subject":"Re: [PATCH v4] clone: simplify string handling in guess_dir_name()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-05T09:06:03Z","receivedAt":"2015-08-05T09:06:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 05, 2015 at 04:41:48AM -0400, Jeff King wrote:\n> On Wed, Aug 05, 2015 at 08:08:52AM +0200, Patrick Steinhardt wrote:\n> \n> > > Sadly we cannot just `strip_suffix_mem(repo, &len, \"/.git\"))` in the\n> > > earlier code, as we have to account for multiple directory separators. I\n> > > believe the above code does the right thing, though. I haven't looked at\n> > > how badly it interacts with the other guess_dir_name work from Patrick\n> > > Steinhardt that has been going on, though.\n> > \n> > It shouldn't be hard rebasing my work onto this. If it's being\n> > applied I'll come up with a new version.\n> \n> Thanks, it is always nice when contributors are flexible and easy to\n> work with. :)\n> \n> Hopefully the new tests I've added can help you out, as well.\n> \n> -Peff\n\nYou're welcome. And yes, your tests help me quite a lot here. Got\ntedious to always set up the chroot. Guess I'll still send my\nfixes for the chroot-tests as a separate patch series, even\nthough I don't require them anymore.\n\nShort question on how to proceed: should I mention that my patch\nseries builds upon your patches or just include them in my\nseries?\n\nPatrick\n"},{"id":"267504","messageId":"20150805090947.GA3847@sigill.intra.peff.net","threadId":"39821","inReplyTo":"20150805090603.GC1103@pks-pc.localdomain","subject":"Re: [PATCH v4] clone: simplify string handling in guess_dir_name()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-05T09:09:47Z","receivedAt":"2015-08-05T09:09:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 05, 2015 at 11:06:03AM +0200, Patrick Steinhardt wrote:\n\n> You're welcome. And yes, your tests help me quite a lot here. Got\n> tedious to always set up the chroot. Guess I'll still send my\n> fixes for the chroot-tests as a separate patch series, even\n> though I don't require them anymore.\n\nYeah, fixes for t1509 are definitely welcome.\n\n> Short question on how to proceed: should I mention that my patch\n> series builds upon your patches or just include them in my\n> series?\n\nI think we'll want to merge my patches separately, due to the\nregression, so you should not include them. So hopefully the sequence\nis:\n\n 1. Junio picks them up as jk/guess-repo-name-regression or similar.\n\n 2. They are merged to 'next', and then eventually to 'maint'.\n\n 3. Mention the topic branch name (whatever Junio ends up picking for\n it) when you post your patches. Junio can then apply on top in his\n repo.\n\n 4. If re-rolls keep going past step 2, then it becomes a non-issue, as\n 'maint' gets merged to 'master'.\n\n-Peff\n"},{"id":"267522","messageId":"xmqq8u9p4pqb.fsf@gitster.dls.corp.google.com","threadId":"39821","inReplyTo":"20150805083526.GA22325@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] fix clone guess_dir_name regression in v2.4.8","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-05T17:19:56Z","receivedAt":"2015-08-05T17:19:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 04, 2015 at 06:42:46PM -0400, Jeff King wrote:\n>\n>> > I did not intend this change in behavior, and I can confirm that\n>> > reverting my patch restores the original behavior. Thanks for bringing\n>> > this to my attention, I'll work on a patch.\n>> \n>> I think this regression is in v2.4.8, as well. We should be able to use\n>> a running \"len\" instead of the \"end\" pointer in the earlier part, and\n>> then use strip_suffix_mem later (to strip from our already-reduced\n>> length, rather than the full NUL-terminated string). Like this:\n>\n> Looks like \"git clone --bare host:foo/.git\" is broken, too. I've added\n> some tests to cover the recently broken cases, as well as some obvious\n> normal cases (which the patch I sent earlier break!). And as a bonus, we\n> can easily cover Patrick's root-repo problems (so people will actually\n> run the tests, unlike the stuff in t1509. :) ).\n\nSorry, my fault; I should have been much less trusting while queuing\na patch like that offending one that was meant to be a no-op.\n\n> Here are the patches.\n>\n> [1/2]: clone: add tests for output directory\n> [2/2]: clone: use computed length in guess_dir_name\n>\nThanks.\n"},{"id":"267540","messageId":"20150805210454.GA21134@sigill.intra.peff.net","threadId":"39821","inReplyTo":"xmqq8u9p4pqb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/2] fix clone guess_dir_name regression in v2.4.8","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-05T21:04:54Z","receivedAt":"2015-08-05T21:04:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 05, 2015 at 10:19:56AM -0700, Junio C Hamano wrote:\n\n> >> I think this regression is in v2.4.8, as well. We should be able to use\n> >> a running \"len\" instead of the \"end\" pointer in the earlier part, and\n> >> then use strip_suffix_mem later (to strip from our already-reduced\n> >> length, rather than the full NUL-terminated string). Like this:\n> >\n> > Looks like \"git clone --bare host:foo/.git\" is broken, too. I've added\n> > some tests to cover the recently broken cases, as well as some obvious\n> > normal cases (which the patch I sent earlier break!). And as a bonus, we\n> > can easily cover Patrick's root-repo problems (so people will actually\n> > run the tests, unlike the stuff in t1509. :) ).\n> \n> Sorry, my fault; I should have been much less trusting while queuing\n> a patch like that offending one that was meant to be a no-op.\n\nI reviewed it, too. :-/\n\nI actually did give some thought to that while working on the fix. Why\ndid we miss what in retrospect was a pretty obvious bug? I saw two\ninteresting bits:\n\n 1. From the diff context, it looked like a perfectly reasonable\n change; the shrinking of the \"end\" pointer happened further up\n in the function.\n\n So I guess the lesson is not to trust reading just the diff, and\n to really read the whole of the modified function. But that's easy\n to say in retrospect; most of the time the bits outside the context\n aren't interesting, and we can't afford to read the whole code\n base for each patch. It's a judgement call where to stop looking at\n the surrounding context of a given change (e.g., the function, the\n callers, their callers, etc).\n\n 2. We didn't have any test coverage in this area; when I wrote even\n basic tests, it caught the problem.\n\n I hate to set a rule like \"if you are cleaning something up, make\n sure there is decent test coverage\". Lots of trivial-looking\n patches really are trivial, and it doesn't make sense to insist the\n submitter add a new battery of tests.\n\nSo I dunno. This was definitely preventable, but that is all in\nretrospect. Bugs will happen, and we usually catch them while cooking.\nThe biggest pain is that this slipped through to a release, and that may\njust be a measure of how few people were impacted (the cases it affected\nwere relatively obscure).\n\n-Peff\n"}]}