{"thread":{"id":"48589","subject":"[PATCH] sequencer: ensure labels that are object IDs are rewritten","startedAt":"2018-05-29T16:32:59Z","lastAt":"2018-06-01T19:02:18Z","messageCount":6,"participants":["brian m. carlson","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"348703","messageId":"20180529163236.646275-1-sandals@crustytoothpaste.net","threadId":"48589","inReplyTo":null,"subject":"[PATCH] sequencer: ensure labels that are object IDs are rewritten","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-05-29T16:32:36Z","receivedAt":"2018-05-29T16:32:59Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When writing the todo script for --rebase-merges, we try to find a label\nfor certain commits.  If the label ends up being a valid object ID, such\nas when we merge a detached commit, we want to rewrite it so it is no\nlonger a valid object ID.\n\nHowever, the code path that does this checks for its length to be\nequivalent to GIT_SHA1_RAWSZ, which isn't correct, since what we are\nreading is a hex object ID.  Instead, check for the length being\nequivalent to that of a hex object ID.  Use the_hash_algo so this code\nworks regardless of the hash size.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n\nI noticed this while cleaning up a few instances of GIT_SHA1_* constants\nand thought I'd send a patch.\n\n sequencer.c              |  2 +-\n t/t3430-rebase-merges.sh | 17 +++++++++++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1ce63261a3..75ed86a94e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3636,7 +3636,7 @@ static const char *label_oid(struct object_id *oid, const char *label,\n \t\t\t\tp[i] = save;\n \t\t\t}\n \t\t}\n-\t} else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&\n+\t} else if (((len = strlen(label)) == the_hash_algo->hexsz &&\n \t\t    !get_oid_hex(label, &dummy)) ||\n \t\t   (len == 1 && *label == '#') ||\n \t\t   hashmap_get_from_hash(&state->labels,\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 3d4dfdf7be..472ad9463c 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -70,6 +70,7 @@ test_expect_success 'create completely different structure' '\n \tmerge -C H second\n \tmerge onebranch # Merge the topic branch '\\''onebranch'\\''\n \tEOF\n+\tcp script-from-scratch script-from-scratch-orig &&\n \ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n \ttest_tick &&\n \tgit rebase -i -r A &&\n@@ -241,4 +242,20 @@ test_expect_success 'refuse to merge ancestors of HEAD' '\n \ttest_cmp_rev HEAD $before\n '\n \n+test_expect_success 'labels that are object IDs are rewritten' '\n+\tgit checkout -b third B &&\n+\ttest_tick &&\n+\ttest_commit I &&\n+\tthird=$(git rev-parse HEAD) &&\n+\tgit checkout -b labels master &&\n+\tgit merge --no-commit third &&\n+\ttest_tick &&\n+\tgit commit -m \"Merge commit '\\''$third'\\'' into labels\" &&\n+\tcp script-from-scratch-orig script-from-scratch &&\n+\ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n+\ttest_tick &&\n+\tgit rebase -i -r A &&\n+\t! grep \"^label $third$\" .git/ORIGINAL-TODO\n+'\n+\n test_done\n"},{"id":"348797","messageId":"nycvar.QRO.7.76.6.1805301144470.82@tvgsbejvaqbjf.bet","threadId":"48589","inReplyTo":"20180529163236.646275-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] sequencer: ensure labels that are object IDs are rewritten","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-05-30T09:54:27Z","receivedAt":"2018-05-30T09:54:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brian,\n\nOn Tue, 29 May 2018, brian m. carlson wrote:\n\n> When writing the todo script for --rebase-merges, we try to find a label\n> for certain commits.  If the label ends up being a valid object ID, such\n> as when we merge a detached commit, we want to rewrite it so it is no\n> longer a valid object ID.\n> \n> However, the code path that does this checks for its length to be\n> equivalent to GIT_SHA1_RAWSZ, which isn't correct, since what we are\n> reading is a hex object ID.  Instead, check for the length being\n> equivalent to that of a hex object ID.  Use the_hash_algo so this code\n> works regardless of the hash size.\n\nD'oh. Thank you so much for this fix.\n\n> I noticed this while cleaning up a few instances of GIT_SHA1_* constants\n> and thought I'd send a patch.\n\nYou thought well!\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 1ce63261a3..75ed86a94e 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3636,7 +3636,7 @@ static const char *label_oid(struct object_id *oid, const char *label,\n>  \t\t\t\tp[i] = save;\n>  \t\t\t}\n>  \t\t}\n> -\t} else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&\n> +\t} else if (((len = strlen(label)) == the_hash_algo->hexsz &&\n\nObviously good!\n\n>  \t\t    !get_oid_hex(label, &dummy)) ||\n>  \t\t   (len == 1 && *label == '#') ||\n>  \t\t   hashmap_get_from_hash(&state->labels,\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index 3d4dfdf7be..472ad9463c 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -70,6 +70,7 @@ test_expect_success 'create completely different structure' '\n>  \tmerge -C H second\n>  \tmerge onebranch # Merge the topic branch '\\''onebranch'\\''\n>  \tEOF\n> +\tcp script-from-scratch script-from-scratch-orig &&\n>  \ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n>  \ttest_tick &&\n>  \tgit rebase -i -r A &&\n> @@ -241,4 +242,20 @@ test_expect_success 'refuse to merge ancestors of HEAD' '\n>  \ttest_cmp_rev HEAD $before\n>  '\n>  \n> +test_expect_success 'labels that are object IDs are rewritten' '\n\nThanks for writing a test, I had been meaning to, but it slipped from my\nmind.\n\n> +\tgit checkout -b third B &&\n> +\ttest_tick &&\n> +\ttest_commit I &&\n\nThe test_tick before test_commit is unnecessary.\n\n> +\tthird=$(git rev-parse HEAD) &&\n> +\tgit checkout -b labels master &&\n> +\tgit merge --no-commit third &&\n> +\ttest_tick &&\n> +\tgit commit -m \"Merge commit '\\''$third'\\'' into labels\" &&\n\nHere, the test_tick is required because we commit via `git commit`.\n\nBTW another thing that I had been meaning to address but totally forgot is\nthis '\\'' ugliness. I had been meaning to define SQ=\"'\" before all test\ncases and then use $SQ everywhere. Not your problem, though.\n\n> +\tcp script-from-scratch-orig script-from-scratch &&\n\nThere is nothing in that script that you need. Why not simply\n\n\techo noop >script-from-scratch\n\nor if you care about the branch,\n\n\techo reset $third >script-from-scratch\n\n> +\ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n> +\ttest_tick &&\n> +\tgit rebase -i -r A &&\n> +\t! grep \"^label $third$\" .git/ORIGINAL-TODO\n\nI would like to verify in addition that /^label $third-/ is present in\nORIGINAL-TODO, what do you think?\n\nThank you,\nDscho\n"},{"id":"348849","messageId":"20180530222011.GC671367@genre.crustytoothpaste.net","threadId":"48589","inReplyTo":"nycvar.QRO.7.76.6.1805301144470.82@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] sequencer: ensure labels that are object IDs are rewritten","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-05-30T22:20:11Z","receivedAt":"2018-05-30T22:20:21Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, May 30, 2018 at 11:54:27AM +0200, Johannes Schindelin wrote:\n> > +\tgit checkout -b third B &&\n> > +\ttest_tick &&\n> > +\ttest_commit I &&\n> \n> The test_tick before test_commit is unnecessary.\n\nYou're right, it is.  Will fix.\n\n> > +\tthird=$(git rev-parse HEAD) &&\n> > +\tgit checkout -b labels master &&\n> > +\tgit merge --no-commit third &&\n> > +\ttest_tick &&\n> > +\tgit commit -m \"Merge commit '\\''$third'\\'' into labels\" &&\n> \n> Here, the test_tick is required because we commit via `git commit`.\n> \n> BTW another thing that I had been meaning to address but totally forgot is\n> this '\\'' ugliness. I had been meaning to define SQ=\"'\" before all test\n> cases and then use $SQ everywhere. Not your problem, though.\n> \n> > +\tcp script-from-scratch-orig script-from-scratch &&\n> \n> There is nothing in that script that you need. Why not simply\n> \n> \techo noop >script-from-scratch\n> \n> or if you care about the branch,\n> \n> \techo reset $third >script-from-scratch\n\nThat would be simpler.  You read my mind: I needed some script to make\nthe sequence editor work, but anything would be fine.\n\n> > +\ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n> > +\ttest_tick &&\n> > +\tgit rebase -i -r A &&\n> > +\t! grep \"^label $third$\" .git/ORIGINAL-TODO\n> \n> I would like to verify in addition that /^label $third-/ is present in\n> ORIGINAL-TODO, what do you think?\n\nSure, I can do that.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"348889","messageId":"nycvar.QRO.7.76.6.1805311347380.82@tvgsbejvaqbjf.bet","threadId":"48589","inReplyTo":"20180530222011.GC671367@genre.crustytoothpaste.net","subject":"Re: [PATCH] sequencer: ensure labels that are object IDs are rewritten","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-05-31T11:49:18Z","receivedAt":"2018-05-31T11:49:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brian,\n\nOn Wed, 30 May 2018, brian m. carlson wrote:\n\n> On Wed, May 30, 2018 at 11:54:27AM +0200, Johannes Schindelin wrote:\n> \n> > > +\tthird=$(git rev-parse HEAD) &&\n> > > +\tgit checkout -b labels master &&\n> > > +\tgit merge --no-commit third &&\n> > > +\ttest_tick &&\n> > > +\tgit commit -m \"Merge commit '\\''$third'\\'' into labels\" &&\n> > \n> > Here, the test_tick is required because we commit via `git commit`.\n> > \n> > BTW another thing that I had been meaning to address but totally forgot is\n> > this '\\'' ugliness. I had been meaning to define SQ=\"'\" before all test\n> > cases and then use $SQ everywhere. Not your problem, though.\n> > \n> > > +\tcp script-from-scratch-orig script-from-scratch &&\n> > \n> > There is nothing in that script that you need. Why not simply\n> > \n> > \techo noop >script-from-scratch\n> > \n> > or if you care about the branch,\n> > \n> > \techo reset $third >script-from-scratch\n> \n> That would be simpler.  You read my mind: I needed some script to make\n> the sequence editor work, but anything would be fine.\n\nI would not go that far. Sometimes I wish I could read minds. More often,\nI am glad that I cannot. I simply guessed correctly in this case ;-)\n\nBut yes, I think it would not only be simpler, but would also avoid the\nhead-scratching why the earlier `cp script-from-scratch\nscript-from-scratch-orig`, and it would also make it more robust to future\nchanges (e.g. if somebody decides to move test cases around, or introduce\nprereqs that skip some).\n\nCiao,\nDscho\n"},{"id":"349005","messageId":"20180601174639.875215-1-sandals@crustytoothpaste.net","threadId":"48589","inReplyTo":"20180529163236.646275-1-sandals@crustytoothpaste.net","subject":"[PATCH v2] sequencer: ensure labels that are object IDs are rewritten","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-06-01T17:46:39Z","receivedAt":"2018-06-01T17:46:57Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When writing the todo script for --rebase-merges, we try to find a label\nfor certain commits.  If the label ends up being a valid object ID, such\nas when we merge a detached commit, we want to rewrite it so it is no\nlonger a valid object ID.\n\nHowever, the code path that does this checks for its length to be\nequivalent to GIT_SHA1_RAWSZ, which isn't correct, since what we are\nreading is a hex object ID.  Instead, check for the length being\nequivalent to that of a hex object ID.  Use the_hash_algo so this code\nworks regardless of the hash size.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n sequencer.c              |  2 +-\n t/t3430-rebase-merges.sh | 16 ++++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1ce63261a3..75ed86a94e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3636,7 +3636,7 @@ static const char *label_oid(struct object_id *oid, const char *label,\n \t\t\t\tp[i] = save;\n \t\t\t}\n \t\t}\n-\t} else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&\n+\t} else if (((len = strlen(label)) == the_hash_algo->hexsz &&\n \t\t    !get_oid_hex(label, &dummy)) ||\n \t\t   (len == 1 && *label == '#') ||\n \t\t   hashmap_get_from_hash(&state->labels,\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 3d4dfdf7be..9fc4648ff8 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -241,4 +241,20 @@ test_expect_success 'refuse to merge ancestors of HEAD' '\n \ttest_cmp_rev HEAD $before\n '\n \n+test_expect_success 'labels that are object IDs are rewritten' '\n+\tgit checkout -b third B &&\n+\ttest_commit I &&\n+\tthird=$(git rev-parse HEAD) &&\n+\tgit checkout -b labels master &&\n+\tgit merge --no-commit third &&\n+\ttest_tick &&\n+\tgit commit -m \"Merge commit '\\''$third'\\'' into labels\" &&\n+\techo noop >script-from-scratch &&\n+\ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n+\ttest_tick &&\n+\tgit rebase -i -r A &&\n+\tgrep \"^label $third-\" .git/ORIGINAL-TODO &&\n+\t! grep \"^label $third$\" .git/ORIGINAL-TODO\n+'\n+\n test_done\n"},{"id":"349009","messageId":"nycvar.QRO.7.76.6.1806012101370.77@tvgsbejvaqbjf.bet","threadId":"48589","inReplyTo":"20180601174639.875215-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] sequencer: ensure labels that are object IDs are rewritten","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-01T19:01:48Z","receivedAt":"2018-06-01T19:02:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brian,\n\nOn Fri, 1 Jun 2018, brian m. carlson wrote:\n\n> When writing the todo script for --rebase-merges, we try to find a label\n> for certain commits.  If the label ends up being a valid object ID, such\n> as when we merge a detached commit, we want to rewrite it so it is no\n> longer a valid object ID.\n> \n> However, the code path that does this checks for its length to be\n> equivalent to GIT_SHA1_RAWSZ, which isn't correct, since what we are\n> reading is a hex object ID.  Instead, check for the length being\n> equivalent to that of a hex object ID.  Use the_hash_algo so this code\n> works regardless of the hash size.\n> \n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n\nACK!\nDscho\n"}]}