[PATCH] sequencer: ensure labels that are object IDs are rewritten

When writing the todo script for --rebase-merges, we try to find a label
for certain commits.  If the label ends up being a valid object ID, such
as when we merge a detached commit, we want to rewrite it so it is no
longer a valid object ID.

However, the code path that does this checks for its length to be
equivalent to GIT_SHA1_RAWSZ, which isn't correct, since what we are
reading is a hex object ID.  Instead, check for the length being
equivalent to that of a hex object ID.  Use the_hash_algo so this code
works regardless of the hash size.

Signed-off-by: brian m. carlson <redacted>
---

I noticed this while cleaning up a few instances of GIT_SHA1_* constants
and thought I'd send a patch.

 sequencer.c              |  2 +-
 t/t3430-rebase-merges.sh | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 1ce63261a3..75ed86a94e 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -3636,7 +3636,7 @@ static const char *label_oid(struct object_id *oid, const char *label,
 				p[i] = save;
 			}
 		}
-	} else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&
+	} else if (((len = strlen(label)) == the_hash_algo->hexsz &&
 		    !get_oid_hex(label, &dummy)) ||
 		   (len == 1 && *label == '#') ||
 		   hashmap_get_from_hash(&state->labels,

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 3d4dfdf7be..472ad9463c 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh

@@ -70,6 +70,7 @@ test_expect_success 'create completely different structure' '
 	merge -C H second
 	merge onebranch # Merge the topic branch '\''onebranch'\''
 	EOF
+	cp script-from-scratch script-from-scratch-orig &&
 	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
 	test_tick &&
 	git rebase -i -r A &&

@@ -241,4 +242,20 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
 	test_cmp_rev HEAD $before
 '
 
+test_expect_success 'labels that are object IDs are rewritten' '
+	git checkout -b third B &&
+	test_tick &&
+	test_commit I &&
+	third=$(git rev-parse HEAD) &&
+	git checkout -b labels master &&
+	git merge --no-commit third &&
+	test_tick &&
+	git commit -m "Merge commit '\''$third'\'' into labels" &&
+	cp script-from-scratch-orig script-from-scratch &&
+	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+	test_tick &&
+	git rebase -i -r A &&
+	! grep "^label $third$" .git/ORIGINAL-TODO
+'
+
 test_done

Hi Brian,

On Tue, 29 May 2018, brian m. carlson wrote:

When writing the todo script for --rebase-merges, we try to find a label
for certain commits.  If the label ends up being a valid object ID, such
as when we merge a detached commit, we want to rewrite it so it is no
longer a valid object ID.

However, the code path that does this checks for its length to be
equivalent to GIT_SHA1_RAWSZ, which isn't correct, since what we are
reading is a hex object ID.  Instead, check for the length being
equivalent to that of a hex object ID.  Use the_hash_algo so this code
works regardless of the hash size.

D'oh. Thank you so much for this fix.

I noticed this while cleaning up a few instances of GIT_SHA1_* constants
and thought I'd send a patch.

You thought well!

Obviously good!

Thanks for writing a test, I had been meaning to, but it slipped from my
mind.

+	git checkout -b third B &&
+	test_tick &&
+	test_commit I &&

The test_tick before test_commit is unnecessary.

+	third=$(git rev-parse HEAD) &&
+	git checkout -b labels master &&
+	git merge --no-commit third &&
+	test_tick &&
+	git commit -m "Merge commit '\''$third'\'' into labels" &&

Here, the test_tick is required because we commit via `git commit`.

BTW another thing that I had been meaning to address but totally forgot is
this '\'' ugliness. I had been meaning to define SQ="'" before all test
cases and then use $SQ everywhere. Not your problem, though.

+	cp script-from-scratch-orig script-from-scratch &&

There is nothing in that script that you need. Why not simply

	echo noop >script-from-scratch

or if you care about the branch,

	echo reset $third >script-from-scratch

+	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+	test_tick &&
+	git rebase -i -r A &&
+	! grep "^label $third$" .git/ORIGINAL-TODO

I would like to verify in addition that /^label $third-/ is present in
ORIGINAL-TODO, what do you think?

Thank you,
Dscho

On Wed, May 30, 2018 at 11:54:27AM +0200, Johannes Schindelin wrote:

quoted

+	git checkout -b third B &&
+	test_tick &&
+	test_commit I &&

The test_tick before test_commit is unnecessary.

You're right, it is.  Will fix.

quoted

+	third=$(git rev-parse HEAD) &&
+	git checkout -b labels master &&
+	git merge --no-commit third &&
+	test_tick &&
+	git commit -m "Merge commit '\''$third'\'' into labels" &&

Here, the test_tick is required because we commit via `git commit`.

BTW another thing that I had been meaning to address but totally forgot is
this '\'' ugliness. I had been meaning to define SQ="'" before all test
cases and then use $SQ everywhere. Not your problem, though.

quoted

+	cp script-from-scratch-orig script-from-scratch &&

There is nothing in that script that you need. Why not simply

	echo noop >script-from-scratch

or if you care about the branch,

	echo reset $third >script-from-scratch

That would be simpler.  You read my mind: I needed some script to make
the sequence editor work, but anything would be fine.

quoted

+	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+	test_tick &&
+	git rebase -i -r A &&
+	! grep "^label $third$" .git/ORIGINAL-TODO

I would like to verify in addition that /^label $third-/ is present in
ORIGINAL-TODO, what do you think?

Sure, I can do that.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

Hi Brian,

On Wed, 30 May 2018, brian m. carlson wrote:

On Wed, May 30, 2018 at 11:54:27AM +0200, Johannes Schindelin wrote:

quoted

quoted

+	third=$(git rev-parse HEAD) &&
+	git checkout -b labels master &&
+	git merge --no-commit third &&
+	test_tick &&
+	git commit -m "Merge commit '\''$third'\'' into labels" &&

Here, the test_tick is required because we commit via `git commit`.

BTW another thing that I had been meaning to address but totally forgot is
this '\'' ugliness. I had been meaning to define SQ="'" before all test
cases and then use $SQ everywhere. Not your problem, though.

quoted

+	cp script-from-scratch-orig script-from-scratch &&

There is nothing in that script that you need. Why not simply

	echo noop >script-from-scratch

or if you care about the branch,

	echo reset $third >script-from-scratch

That would be simpler.  You read my mind: I needed some script to make
the sequence editor work, but anything would be fine.

I would not go that far. Sometimes I wish I could read minds. More often,
I am glad that I cannot. I simply guessed correctly in this case ;-)

But yes, I think it would not only be simpler, but would also avoid the
head-scratching why the earlier `cp script-from-scratch
script-from-scratch-orig`, and it would also make it more robust to future
changes (e.g. if somebody decides to move test cases around, or introduce
prereqs that skip some).

Ciao,
Dscho

When writing the todo script for --rebase-merges, we try to find a label
for certain commits.  If the label ends up being a valid object ID, such
as when we merge a detached commit, we want to rewrite it so it is no
longer a valid object ID.

However, the code path that does this checks for its length to be
equivalent to GIT_SHA1_RAWSZ, which isn't correct, since what we are
reading is a hex object ID.  Instead, check for the length being
equivalent to that of a hex object ID.  Use the_hash_algo so this code
works regardless of the hash size.

Signed-off-by: brian m. carlson <redacted>
---
 sequencer.c              |  2 +-
 t/t3430-rebase-merges.sh | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 1ce63261a3..75ed86a94e 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -3636,7 +3636,7 @@ static const char *label_oid(struct object_id *oid, const char *label,
 				p[i] = save;
 			}
 		}
-	} else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&
+	} else if (((len = strlen(label)) == the_hash_algo->hexsz &&
 		    !get_oid_hex(label, &dummy)) ||
 		   (len == 1 && *label == '#') ||
 		   hashmap_get_from_hash(&state->labels,

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 3d4dfdf7be..9fc4648ff8 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh

@@ -241,4 +241,20 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
 	test_cmp_rev HEAD $before
 '
 
+test_expect_success 'labels that are object IDs are rewritten' '
+	git checkout -b third B &&
+	test_commit I &&
+	third=$(git rev-parse HEAD) &&
+	git checkout -b labels master &&
+	git merge --no-commit third &&
+	test_tick &&
+	git commit -m "Merge commit '\''$third'\'' into labels" &&
+	echo noop >script-from-scratch &&
+	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+	test_tick &&
+	git rebase -i -r A &&
+	grep "^label $third-" .git/ORIGINAL-TODO &&
+	! grep "^label $third$" .git/ORIGINAL-TODO
+'
+
 test_done

Hi Brian,

On Fri, 1 Jun 2018, brian m. carlson wrote:

When writing the todo script for --rebase-merges, we try to find a label
for certain commits.  If the label ends up being a valid object ID, such
as when we merge a detached commit, we want to rewrite it so it is no
longer a valid object ID.

However, the code path that does this checks for its length to be
equivalent to GIT_SHA1_RAWSZ, which isn't correct, since what we are
reading is a hex object ID.  Instead, check for the length being
equivalent to that of a hex object ID.  Use the_hash_algo so this code
works regardless of the hash size.

Signed-off-by: brian m. carlson <redacted>

ACK!
Dscho