threads / patch / 48589

patchsequencer: ensure labels that are object IDs are rewritten

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

## tl;dr

6 messages between May 29, 2018 and Jun 1, 2018. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

brian m. carlson· May 29, 2018, 16:32 UTC · lore

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 <sandals@crustytoothpaste.net>
---

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(-)

Show changes to 2 files +18 −1

sequencer.c, t/t3430-rebase-merges.sh

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

Johannes Schindelin· May 30, 2018, 09:54 UTC · re: brian m. carlson · lore

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

Hi Brian,

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

Show 10 quoted lines

> 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!

Show 10 quoted lines

> 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 &&

Obviously good!

Show 20 quoted lines

>  		    !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' '

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.

Show 5 quoted lines

> +	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

brian m. carlson· May 30, 2018, 22:20 UTC · re: Johannes Schindelin · lore

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

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

Show 5 quoted lines

> > +	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.

Show 21 quoted lines

> > +	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

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

Show 7 quoted lines

> > +	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

Johannes Schindelin· May 31, 2018, 11:49 UTC · re: brian m. carlson · lore

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

Hi Brian,

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

Show 26 quoted lines

> On Wed, May 30, 2018 at 11:54:27AM +0200, Johannes Schindelin wrote:
> 
> > > +	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
> 
> 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

brian m. carlson· Jun 1, 2018, 17:46 UTC · re: brian m. carlson · lore

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

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

Show changes to 2 files +17 −1

sequencer.c, t/t3430-rebase-merges.sh

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

Johannes Schindelin· Jun 1, 2018, 19:01 UTC · re: brian m. carlson · lore

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

Hi Brian,

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

Show 12 quoted lines

> 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 <sandals@crustytoothpaste.net>

ACK! Dscho