sequencer: handle rebase-merge for "onto" message
diff mbox series

Message ID 20191118112357.GA29922@danh.dev
State New
Headers show
Series
  • sequencer: handle rebase-merge for "onto" message
Related show

Commit Message

Danh Doan Nov. 18, 2019, 11:23 a.m. UTC
On 2019-11-18 12:42:41+0900, Junio C Hamano <gitster@pobox.com> wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > Those labels must be valid ref names, and therefore valid file names. The
> > initial patch came in via Git for Windows.
> >
> > Change since v1:
> >
> >  * moved the entire sanitizing logic to label_oid(), as a preparatory step.
> >
> > Johannes Schindelin (1):
> >   rebase-merges: move labels' whitespace mangling into `label_oid()`
> >
> > Matthew Rogers (1):
> >   rebase -r: let `label` generate safer labels
> >
> >  sequencer.c              | 72 +++++++++++++++++++++++++---------------
> >  t/t3430-rebase-merges.sh |  6 ++++
> >  2 files changed, 51 insertions(+), 27 deletions(-)
> 
> I think Dscho meant to Cc you as these two patches are meant to be a
> more complete solution to supersede your [*1*].

I've applied their patches over my branches,
my introduced test_expect_failure was flipped.
That's nice.

Anyway, when reading their patch, I discovered a problem with
git rebase --rebase-merges when its message is onto.

Here is a patch to fix it.

I applied Dscho's patches over my dd/sequencer-utf8 then writing this
patch, in case you have problem applying it.

The context for the diff is coming from Dscho's patches.

-------8<--------------------
From 48205889b404b82baa4b30c2eedd52243c349e3e Mon Sep 17 00:00:00 2001
From: Doan Tran Cong Danh <congdanhqx@gmail.com>
Date: Mon, 18 Nov 2019 18:02:05 +0700
Subject: [PATCH] sequencer: handle rebase-merge for "onto" message

In order to work correctly, git-rebase --rebase-merges needs to make
initial todo list with unique labels.

Those unique labels is being handled by employing a hashmap and
suffixing an unique number if any duplicate is found.

But we forgat that beside of those labels for side branches,
we also make a special label `onto' for our so-called new-base.

In a special case that any of those labels for side branches named
`onto', git will run into trouble.

Correct it.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 sequencer.c              |  5 +++++
 t/t3430-rebase-merges.sh | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+)

Comments

Junio C Hamano Nov. 21, 2019, 12:16 a.m. UTC | #1
Danh Doan <congdanhqx@gmail.com> writes:

> Anyway, when reading their patch, I discovered a problem with
> git rebase --rebase-merges when its message is onto.
>
> Here is a patch to fix it.
>
> I applied Dscho's patches over my dd/sequencer-utf8 then writing this
> patch, in case you have problem applying it.
>
> The context for the diff is coming from Dscho's patches.

Thanks.  While technically this is independent from the "safer
rebase-merges labels" topic (specifically its preparation step),
in the larger picture, this too is to ensure we do not use a wrong
string as a label ;-), so I'll queue it on top of those two patches,
just like how you developed.

Thanks.

> -------8<--------------------
> From 48205889b404b82baa4b30c2eedd52243c349e3e Mon Sep 17 00:00:00 2001
> From: Doan Tran Cong Danh <congdanhqx@gmail.com>
> Date: Mon, 18 Nov 2019 18:02:05 +0700
> Subject: [PATCH] sequencer: handle rebase-merge for "onto" message
>
> In order to work correctly, git-rebase --rebase-merges needs to make
> initial todo list with unique labels.
>
> Those unique labels is being handled by employing a hashmap and
> suffixing an unique number if any duplicate is found.
>
> But we forgat that beside of those labels for side branches,
> we also make a special label `onto' for our so-called new-base.
>
> In a special case that any of those labels for side branches named
> `onto', git will run into trouble.
>
> Correct it.
>
> Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
> ---
>  sequencer.c              |  5 +++++
>  t/t3430-rebase-merges.sh | 21 +++++++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 350045b1b4..fc81e43f0f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4569,10 +4569,15 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  	strbuf_init(&state.buf, 32);
>  
>  	if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
> +		struct labels_entry *onto_label_entry;
>  		struct object_id *oid = &revs->cmdline.rev[0].item->oid;
>  		FLEX_ALLOC_STR(entry, string, "onto");
>  		oidcpy(&entry->entry.oid, oid);
>  		oidmap_put(&state.commit2label, entry);
> +
> +		FLEX_ALLOC_STR(onto_label_entry, label, "onto");
> +		hashmap_entry_init(&onto_label_entry->entry, strihash("onto"));
> +		hashmap_add(&state.labels, &onto_label_entry->entry);
>  	}
>  
>  	/*
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index f728aba995..4e2c0ede51 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -474,4 +474,25 @@ test_expect_success '--rebase-merges with commit that can generate bad character
>  	git rebase --rebase-merges --force-rebase E
>  '
>  
> +test_expect_success '--rebase-merges with message matched with onto label' '
> +	git checkout -b onto-label E &&
> +	git merge -m onto G &&
> +	git rebase --rebase-merges --force-rebase E &&
> +	test_cmp_graph <<-\EOF
> +	*   onto
> +	|\
> +	| * G
> +	| * F
> +	* |   E
> +	|\ \
> +	| * | B
> +	* | | D
> +	| |/
> +	|/|
> +	* | C
> +	|/
> +	* A
> +	EOF
> +'
> +
>  test_done

Patch
diff mbox series

diff --git a/sequencer.c b/sequencer.c
index 350045b1b4..fc81e43f0f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4569,10 +4569,15 @@  static int make_script_with_merges(struct pretty_print_context *pp,
 	strbuf_init(&state.buf, 32);
 
 	if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
+		struct labels_entry *onto_label_entry;
 		struct object_id *oid = &revs->cmdline.rev[0].item->oid;
 		FLEX_ALLOC_STR(entry, string, "onto");
 		oidcpy(&entry->entry.oid, oid);
 		oidmap_put(&state.commit2label, entry);
+
+		FLEX_ALLOC_STR(onto_label_entry, label, "onto");
+		hashmap_entry_init(&onto_label_entry->entry, strihash("onto"));
+		hashmap_add(&state.labels, &onto_label_entry->entry);
 	}
 
 	/*
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index f728aba995..4e2c0ede51 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -474,4 +474,25 @@  test_expect_success '--rebase-merges with commit that can generate bad character
 	git rebase --rebase-merges --force-rebase E
 '
 
+test_expect_success '--rebase-merges with message matched with onto label' '
+	git checkout -b onto-label E &&
+	git merge -m onto G &&
+	git rebase --rebase-merges --force-rebase E &&
+	test_cmp_graph <<-\EOF
+	*   onto
+	|\
+	| * G
+	| * F
+	* |   E
+	|\ \
+	| * | B
+	* | | D
+	| |/
+	|/|
+	* | C
+	|/
+	* A
+	EOF
+'
+
 test_done