{"thread":{"id":"28499","subject":"[PATCH] git-remote rename should match whole string when renaming remote ref directory","startedAt":"2011-09-26T13:53:06Z","lastAt":"2011-09-27T09:38:46Z","messageCount":5,"participants":["Benny Halevy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"176206","messageId":"1317045186-25206-1-git-send-email-benny@tonian.com","threadId":"28499","inReplyTo":null,"subject":"[PATCH] git-remote rename should match whole string when renaming remote ref directory","fromName":"Benny Halevy","fromEmail":"benny@tonian.com","sentAt":"2011-09-26T13:53:06Z","receivedAt":"2011-09-26T13:53:06Z","isPatch":true,"sender":{"key":"benny@tonian.com","avatar":null},"body":"From: Benny Halevy <bhalevy@tonian.com>\n\nOtherwise, with two remotes: test, test-2\n\tgit remote rename test test-\nends up with:\n\t.git/refs/remotes/test-\n\t.git/refs/remotes/test--2\n\nSigned-off-by: Benny Halevy <bhalevy@tonian.com>\n---\n\nBy the way, I also see the old names as empty directories in .git/refs/remotes\nnot sure if that's a bug or a feature... :)\n\nBenny\n\n builtin/remote.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex f2a9c26..5443e71 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -571,7 +571,7 @@ static int read_remote_branches(const char *refname,\n \tconst char *symref;\n \n \tstrbuf_addf(&buf, \"refs/remotes/%s\", rename->old);\n-\tif (!prefixcmp(refname, buf.buf)) {\n+\tif (!strcmp(refname, buf.buf)) {\n \t\titem = string_list_append(rename->remote_branches, xstrdup(refname));\n \t\tsymref = resolve_ref(refname, orig_sha1, 1, &flag);\n \t\tif (flag & REF_ISSYMREF)\n-- \n1.7.7.rc3.dirty\n"},{"id":"176239","messageId":"7v62kf41ud.fsf@alter.siamese.dyndns.org","threadId":"28499","inReplyTo":"1317045186-25206-1-git-send-email-benny@tonian.com","subject":"Re: [PATCH] git-remote rename should match whole string when renaming remote ref directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-26T18:04:26Z","receivedAt":"2011-09-26T18:04:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benny Halevy <benny@tonian.com> writes:\n\n> From: Benny Halevy <bhalevy@tonian.com>\n>\n> Otherwise, with two remotes: test, test-2\n> \tgit remote rename test test-\n> ends up with:\n> \t.git/refs/remotes/test-\n> \t.git/refs/remotes/test--2\n> ...\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index f2a9c26..5443e71 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -571,7 +571,7 @@ static int read_remote_branches(const char *refname,\n>  \tconst char *symref;\n>  \n>  \tstrbuf_addf(&buf, \"refs/remotes/%s\", rename->old);\n> -\tif (!prefixcmp(refname, buf.buf)) {\n> +\tif (!strcmp(refname, buf.buf)) {\n\nAt this point of the code, refname has \"refs/remotes/test/foo\" and it is\nqueued to later rename it to \"refs/remotes/test-/foo\" (the next invocation\nof this function will see \"refs/remotes/test/bar\" in refname). And the\nstrbuf buf.buf has \"refs/remotes/test\"; your !strcmp(refname, buf.buf)\nwould never trigger, I suspect.\n\nIsn't 60e5eee (remote: \"rename o foo\" should not rename ref \"origin/bar\",\n2011-09-01) the correct fix for this issue?  It makes buf.buf properly\nterminated with a slash, to contain \"refs/remotes/test/\" so that prefixcmp\nproperly matches it with \"refs/remotes/test/foo\" but not with refs under\nother hierarchies like \"refs/remotes/test-2/anything\".\n\nThanks.\n"},{"id":"176300","messageId":"4E8138B3.9090909@tonian.com","threadId":"28499","inReplyTo":"7v62kf41ud.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-remote rename should match whole string when renaming remote ref directory","fromName":"Benny Halevy","fromEmail":"bhalevy@tonian.com","sentAt":"2011-09-27T02:45:07Z","receivedAt":"2011-09-27T02:45:07Z","isPatch":true,"sender":{"key":"bhalevy@tonian.com","avatar":null},"body":"On 2011-09-26 21:04, Junio C Hamano wrote:\n> Benny Halevy <benny@tonian.com> writes:\n> \n>> From: Benny Halevy <bhalevy@tonian.com>\n>>\n>> Otherwise, with two remotes: test, test-2\n>> \tgit remote rename test test-\n>> ends up with:\n>> \t.git/refs/remotes/test-\n>> \t.git/refs/remotes/test--2\n>> ...\n>> diff --git a/builtin/remote.c b/builtin/remote.c\n>> index f2a9c26..5443e71 100644\n>> --- a/builtin/remote.c\n>> +++ b/builtin/remote.c\n>> @@ -571,7 +571,7 @@ static int read_remote_branches(const char *refname,\n>>  \tconst char *symref;\n>>  \n>>  \tstrbuf_addf(&buf, \"refs/remotes/%s\", rename->old);\n>> -\tif (!prefixcmp(refname, buf.buf)) {\n>> +\tif (!strcmp(refname, buf.buf)) {\n> \n> At this point of the code, refname has \"refs/remotes/test/foo\" and it is\n> queued to later rename it to \"refs/remotes/test-/foo\" (the next invocation\n> of this function will see \"refs/remotes/test/bar\" in refname). And the\n> strbuf buf.buf has \"refs/remotes/test\"; your !strcmp(refname, buf.buf)\n> would never trigger, I suspect.\n> \n> Isn't 60e5eee (remote: \"rename o foo\" should not rename ref \"origin/bar\",\n> 2011-09-01) the correct fix for this issue?  It makes buf.buf properly\n> terminated with a slash, to contain \"refs/remotes/test/\" so that prefixcmp\n> properly matches it with \"refs/remotes/test/foo\" but not with refs under\n> other hierarchies like \"refs/remotes/test-2/anything\".\n\nOK, 60e5eee solves the problem too.\nI wasn't aware of it as I was looking at the master branch.\nFWIW, here's the test I used:\n\n#!/bin/sh\n\ngit=git\ncwd=$(pwd)\n\nfunction fail ()\n{\n\techo $0: $*\n\texit 1\n}\n\nfor i in main test test-2; do\n\tmkdir $i || fail $i exists;\n\t$git init $i || fail git init $i failed\n\techo $i > $i/foo\n\t( cd $i; git add foo; git commit -m $i )\ndone\ncd main || fail cd main failed\nfor i in test test-2; do\n\t$git remote add $i file://$cwd/$i || fail git remote add $i failed\ndone\n$git remote update || fail git remote update fail\n$git remote rename test test-\n$git show test-2/master || fail FAILED\necho PASSED\n\n> \n> Thanks.\n"},{"id":"176308","messageId":"7voby6y0vs.fsf@alter.siamese.dyndns.org","threadId":"28499","inReplyTo":"4E8138B3.9090909@tonian.com","subject":"Re: [PATCH] git-remote rename should match whole string when renaming remote ref directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-27T06:07:03Z","receivedAt":"2011-09-27T06:07:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benny Halevy <bhalevy@tonian.com> writes:\n\n> On 2011-09-26 21:04, Junio C Hamano wrote:\n>> Benny Halevy <benny@tonian.com> writes:\n>> ...\n>>> -\tif (!prefixcmp(refname, buf.buf)) {\n>>> +\tif (!strcmp(refname, buf.buf)) {\n>> \n>> At this point of the code, refname has \"refs/remotes/test/foo\" and it is\n>> queued to later rename it to \"refs/remotes/test-/foo\" (the next invocation\n>> of this function will see \"refs/remotes/test/bar\" in refname). And the\n>> strbuf buf.buf has \"refs/remotes/test\"; your !strcmp(refname, buf.buf)\n>> would never trigger, I suspect.\n>> \n>> Isn't 60e5eee (remote: \"rename o foo\" should not rename ref \"origin/bar\",\n>> 2011-09-01) the correct fix for this issue?...\n>\n> OK, 60e5eee solves the problem too.\n\nHmm, what do you mean by \"too\" here?  Martin's patch fixes the issue, but\ndoes yours, too?\n\n> FWIW, here's the test I used:\n> ...\n> cd main || fail cd main failed\n> for i in test test-2; do\n> \t$git remote add $i file://$cwd/$i || fail git remote add $i failed\n> done\n> $git remote update || fail git remote update fail\n> $git remote rename test test-\n> $git show test-2/master || fail FAILED\n> echo PASSED\n\nBefore your last \"echo PASSED\", add this line:\n\n\t$git show test-/master || fail FAILED\n\nand see what happens with your patch.\n\nIt is unfortunately a rather common trap to fall into, so I wouldn't blame\nyou too much. People tend to concentrate only on an aspect of the problem\nthat originally motivated them, and forget about the other issues that are\nequally important, if not more. In this case, you were too thrilled to see\nthat your updated code no longer renames \"test-2\" mistakenly to \"test--2\",\nand you forgot that the primary task of the resulting code was to rename\n\"test\" to \"test-\" correctly. The additional line I gave you above is to\ntest that.\n\nWhen testing your own code, make it a habit to _always_ test both sides of\nthe coin. It is somewhat difficult until you get used to it [*1*], but it\nis a skill that is really worth acquiring.\n\nThanks.\n\n\n[Footnote]\n\n*1* ...and I do not claim that I myself never forget to fully enumerate\nother sides; even experienced people still overlook and embarrass\nthemselves in public ;-)\n"},{"id":"176322","messageId":"4E8199A6.5010804@tonian.com","threadId":"28499","inReplyTo":"7voby6y0vs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-remote rename should match whole string when renaming remote ref directory","fromName":"Benny Halevy","fromEmail":"bhalevy@tonian.com","sentAt":"2011-09-27T09:38:46Z","receivedAt":"2011-09-27T09:38:46Z","isPatch":true,"sender":{"key":"bhalevy@tonian.com","avatar":null},"body":"On 2011-09-27 09:07, Junio C Hamano wrote:\n> Benny Halevy <bhalevy@tonian.com> writes:\n> \n>> On 2011-09-26 21:04, Junio C Hamano wrote:\n>>> Benny Halevy <benny@tonian.com> writes:\n>>> ...\n>>>> -\tif (!prefixcmp(refname, buf.buf)) {\n>>>> +\tif (!strcmp(refname, buf.buf)) {\n>>>\n>>> At this point of the code, refname has \"refs/remotes/test/foo\" and it is\n>>> queued to later rename it to \"refs/remotes/test-/foo\" (the next invocation\n>>> of this function will see \"refs/remotes/test/bar\" in refname). And the\n>>> strbuf buf.buf has \"refs/remotes/test\"; your !strcmp(refname, buf.buf)\n>>> would never trigger, I suspect.\n>>>\n>>> Isn't 60e5eee (remote: \"rename o foo\" should not rename ref \"origin/bar\",\n>>> 2011-09-01) the correct fix for this issue?...\n>>\n>> OK, 60e5eee solves the problem too.\n> \n> Hmm, what do you mean by \"too\" here?  Martin's patch fixes the issue, but\n> does yours, too?\n> \n\ncorrect.\n\n>> FWIW, here's the test I used:\n>> ...\n>> cd main || fail cd main failed\n>> for i in test test-2; do\n>> \t$git remote add $i file://$cwd/$i || fail git remote add $i failed\n>> done\n>> $git remote update || fail git remote update fail\n>> $git remote rename test test-\n>> $git show test-2/master || fail FAILED\n>> echo PASSED\n> \n> Before your last \"echo PASSED\", add this line:\n> \n> \t$git show test-/master || fail FAILED\n> \n> and see what happens with your patch.\n\nYup, with my patch this test indeed fails, and with Martin's\nit passes.\n\nThanks!\n\nBenny\n\n> \n> It is unfortunately a rather common trap to fall into, so I wouldn't blame\n> you too much. People tend to concentrate only on an aspect of the problem\n> that originally motivated them, and forget about the other issues that are\n> equally important, if not more. In this case, you were too thrilled to see\n> that your updated code no longer renames \"test-2\" mistakenly to \"test--2\",\n> and you forgot that the primary task of the resulting code was to rename\n> \"test\" to \"test-\" correctly. The additional line I gave you above is to\n> test that.\n> \n> When testing your own code, make it a habit to _always_ test both sides of\n> the coin. It is somewhat difficult until you get used to it [*1*], but it\n> is a skill that is really worth acquiring.\n> \n> Thanks.\n> \n> \n> [Footnote]\n> \n> *1* ...and I do not claim that I myself never forget to fully enumerate\n> other sides; even experienced people still overlook and embarrass\n> themselves in public ;-)\n"}]}