[v3,3/3] rebase-merges: try and use branch names as labels

Commit Message

Nicolas Guichard Oct. 9, 2024, 7:58 a.m. UTC

From: Nicolas Guichard <[e-mail]>

When interactively rebasing merge commits, the commit message is parsed to
extract a probably meaningful label name. For instance if the merge commit
is “Merge branch 'feature0'”, then the rebase script will have thes lines:
```
label feature0

merge -C $sha feature0 # “Merge branch 'feature0'
```

This heuristic fails in the case of octopus merges or when the merge commit
message is actually unrelated to the parent commits.

An example that combines both is:
```
*---.   967bfa4 (HEAD -> integration) Integration
|\ \ \
| | | * 2135be1 (feature2, feat2) Feature 2
| |_|/
|/| |
| | * c88b01a Feature 1
| |/
|/|
| * 75f3139 (feat0) Feature 0
|/
* 25c86d0 (main) Initial commit
```
yields the labels Integration, Integration-2 and Integration-3.

Fix this by using a branch name for each merge commit's parent that is the
tip of at least one branch, and falling back to a label derived from the
merge commit message otherwise.
In the example above, the labels become feat0, Integration and feature2.

Signed-off-by: Nicolas Guichard <[e-mail]>
---
 sequencer.c                   | 25 +++++++++++++++++--------
 t/t3404-rebase-interactive.sh |  4 ++--
 t/t3430-rebase-merges.sh      | 12 ++++++------
 3 files changed, 25 insertions(+), 16 deletions(-)

Comments

Phillip Wood Oct. 9, 2024, 2:21 p.m. UTC | #1

Hi Nicolas

On 09/10/2024 08:58, Nicolas Guichard via GitGitGadget wrote:
> From: Nicolas Guichard <[e-mail]>
> 
> When interactively rebasing merge commits, the commit message is parsed to
> extract a probably meaningful label name. For instance if the merge commit
> is “Merge branch 'feature0'”, then the rebase script will have thes lines:
> ```
> label feature0
> 
> merge -C $sha feature0 # “Merge branch 'feature0'
> ```
> 
> This heuristic fails in the case of octopus merges or when the merge commit
> message is actually unrelated to the parent commits.
> 
> An example that combines both is:
> ```
> *---.   967bfa4 (HEAD -> integration) Integration
> |\ \ \
> | | | * 2135be1 (feature2, feat2) Feature 2
> | |_|/
> |/| |
> | | * c88b01a Feature 1
> | |/
> |/|
> | * 75f3139 (feat0) Feature 0
> |/
> * 25c86d0 (main) Initial commit
> ```
> yields the labels Integration, Integration-2 and Integration-3.
> 
> Fix this by using a branch name for each merge commit's parent that is the
> tip of at least one branch, and falling back to a label derived from the
> merge commit message otherwise.
> In the example above, the labels become feat0, Integration and feature2.

This looks like a nicely described useful improvement, thank you for 
working on it. The way the code is structured means we always calculate 
the fallback label before seeing if there is a branch name we could use 
instead but as calculating the fallback is cheap I don't think that's a 
problem in practice.

Thanks

Phillip

> Signed-off-by: Nicolas Guichard <[e-mail]>
> ---
>   sequencer.c                   | 25 +++++++++++++++++--------
>   t/t3404-rebase-interactive.sh |  4 ++--
>   t/t3430-rebase-merges.sh      | 12 ++++++------
>   3 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 97959036b50..353d804999b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5819,7 +5819,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>   	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
>   	int skipped_commit = 0;
>   	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
> -	struct strbuf label = STRBUF_INIT;
> +	struct strbuf label_from_message = STRBUF_INIT;
>   	struct commit_list *commits = NULL, **tail = &commits, *iter;
>   	struct commit_list *tips = NULL, **tips_tail = &tips;
>   	struct commit *commit;
> @@ -5842,6 +5842,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>   	oidmap_init(&state.commit2label, 0);
>   	hashmap_init(&state.labels, labels_cmp, NULL, 0);
>   	strbuf_init(&state.buf, 32);
> +	load_branch_decorations();
>   
>   	if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
>   		struct labels_entry *onto_label_entry;
> @@ -5902,18 +5903,18 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>   			continue;
>   		}
>   
> -		/* Create a label */
> -		strbuf_reset(&label);
> +		/* Create a label from the commit message */
> +		strbuf_reset(&label_from_message);
>   		if (skip_prefix(oneline.buf, "Merge ", &p1) &&
>   		    (p1 = strchr(p1, '\'')) &&
>   		    (p2 = strchr(++p1, '\'')))
> -			strbuf_add(&label, p1, p2 - p1);
> +			strbuf_add(&label_from_message, p1, p2 - p1);
>   		else if (skip_prefix(oneline.buf, "Merge pull request ",
>   				     &p1) &&
>   			 (p1 = strstr(p1, " from ")))
> -			strbuf_addstr(&label, p1 + strlen(" from "));
> +			strbuf_addstr(&label_from_message, p1 + strlen(" from "));
>   		else
> -			strbuf_addbuf(&label, &oneline);
> +			strbuf_addbuf(&label_from_message, &oneline);
>   
>   		strbuf_reset(&buf);
>   		strbuf_addf(&buf, "%s -C %s",
> @@ -5921,6 +5922,14 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>   
>   		/* label the tips of merged branches */
>   		for (; to_merge; to_merge = to_merge->next) {
> +			const char *label = label_from_message.buf;
> +			const struct name_decoration *decoration =
> +				get_name_decoration(&to_merge->item->object);
> +
> +			if (decoration)
> +				skip_prefix(decoration->name, "refs/heads/",
> +					    &label);
> +
>   			oid = &to_merge->item->object.oid;
>   			strbuf_addch(&buf, ' ');
>   
> @@ -5933,7 +5942,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>   			tips_tail = &commit_list_insert(to_merge->item,
>   							tips_tail)->next;
>   
> -			strbuf_addstr(&buf, label_oid(oid, label.buf, &state));
> +			strbuf_addstr(&buf, label_oid(oid, label, &state));
>   		}
>   		strbuf_addf(&buf, " # %s", oneline.buf);
>   
> @@ -6041,7 +6050,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>   	free_commit_list(commits);
>   	free_commit_list(tips);
>   
> -	strbuf_release(&label);
> +	strbuf_release(&label_from_message);
>   	strbuf_release(&oneline);
>   	strbuf_release(&buf);
>   
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index f171af3061d..4896a801ee2 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1870,7 +1870,7 @@ test_expect_success '--update-refs adds commands with --rebase-merges' '
>   		pick $(git log -1 --format=%h branch2~1) F
>   		pick $(git log -1 --format=%h branch2) I
>   		update-ref refs/heads/branch2
> -		label merge
> +		label branch2
>   		reset onto
>   		pick $(git log -1 --format=%h refs/heads/second) J
>   		update-ref refs/heads/second
> @@ -1881,7 +1881,7 @@ test_expect_success '--update-refs adds commands with --rebase-merges' '
>   		update-ref refs/heads/third
>   		pick $(git log -1 --format=%h HEAD~2) M
>   		update-ref refs/heads/no-conflict-branch
> -		merge -C $(git log -1 --format=%h HEAD~1) merge # merge
> +		merge -C $(git log -1 --format=%h HEAD~1) branch2 # merge
>   		update-ref refs/heads/merge-branch
>   		EOF
>   
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index 2aa8593f77a..cb891eeb5fd 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -108,19 +108,19 @@ test_expect_success 'generate correct todo list' '
>   
>   	reset onto
>   	pick $b B
> -	label E
> +	label first
>   
>   	reset onto
>   	pick $c C
>   	label branch-point
>   	pick $f F
>   	pick $g G
> -	label H
> +	label second
>   
>   	reset branch-point # C
>   	pick $d D
> -	merge -C $e E # E
> -	merge -C $h H # H
> +	merge -C $e first # E
> +	merge -C $h second # H
>   
>   	EOF
>   
> @@ -462,11 +462,11 @@ test_expect_success 'A root commit can be a cousin, treat it that way' '
>   '
>   
>   test_expect_success 'labels that are object IDs are rewritten' '
> -	git checkout -b third B &&
> +	git checkout --detach B &&
>   	test_commit I &&
>   	third=$(git rev-parse HEAD) &&
>   	git checkout -b labels main &&
> -	git merge --no-commit third &&
> +	git merge --no-commit $third &&
>   	test_tick &&
>   	git commit -m "Merge commit '\''$third'\'' into labels" &&
>   	echo noop >script-from-scratch &&

Junio C Hamano Oct. 9, 2024, 5:57 p.m. UTC | #2

Phillip Wood <[e-mail]> writes:

> Hi Nicolas
>
> On 09/10/2024 08:58, Nicolas Guichard via GitGitGadget wrote:
>> From: Nicolas Guichard <[e-mail]>
>> When interactively rebasing merge commits, the commit message is
>> parsed to
>> extract a probably meaningful label name. For instance if the merge commit
>> is “Merge branch 'feature0'”, then the rebase script will have thes lines:
>> ```
>> label feature0
>> merge -C $sha feature0 # “Merge branch 'feature0'
>> ```
>> This heuristic fails in the case of octopus merges or when the merge
>> commit
>> message is actually unrelated to the parent commits.
>> An example that combines both is:
>> ```
>> *---.   967bfa4 (HEAD -> integration) Integration
>> |\ \ \
>> | | | * 2135be1 (feature2, feat2) Feature 2
>> | |_|/
>> |/| |
>> | | * c88b01a Feature 1
>> | |/
>> |/|
>> | * 75f3139 (feat0) Feature 0
>> |/
>> * 25c86d0 (main) Initial commit
>> ```
>> yields the labels Integration, Integration-2 and Integration-3.
>> Fix this by using a branch name for each merge commit's parent that
>> is the
>> tip of at least one branch, and falling back to a label derived from the
>> merge commit message otherwise.
>> In the example above, the labels become feat0, Integration and feature2.
>
> This looks like a nicely described useful improvement, thank you for
> working on it. The way the code is structured means we always
> calculate the fallback label before seeing if there is a branch name
> we could use instead but as calculating the fallback is cheap I don't
> think that's a problem in practice.

Thanks, both of you.

Will queue.

Patch

diff --git a/sequencer.c b/sequencer.c
index 97959036b50..353d804999b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5819,7 +5819,7 @@  static int make_script_with_merges(struct pretty_print_context *pp,
 	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
 	int skipped_commit = 0;
 	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
-	struct strbuf label = STRBUF_INIT;
+	struct strbuf label_from_message = STRBUF_INIT;
 	struct commit_list *commits = NULL, **tail = &commits, *iter;
 	struct commit_list *tips = NULL, **tips_tail = &tips;
 	struct commit *commit;
@@ -5842,6 +5842,7 @@  static int make_script_with_merges(struct pretty_print_context *pp,
 	oidmap_init(&state.commit2label, 0);
 	hashmap_init(&state.labels, labels_cmp, NULL, 0);
 	strbuf_init(&state.buf, 32);
+	load_branch_decorations();
 
 	if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
 		struct labels_entry *onto_label_entry;
@@ -5902,18 +5903,18 @@  static int make_script_with_merges(struct pretty_print_context *pp,
 			continue;
 		}
 
-		/* Create a label */
-		strbuf_reset(&label);
+		/* Create a label from the commit message */
+		strbuf_reset(&label_from_message);
 		if (skip_prefix(oneline.buf, "Merge ", &p1) &&
 		    (p1 = strchr(p1, '\'')) &&
 		    (p2 = strchr(++p1, '\'')))
-			strbuf_add(&label, p1, p2 - p1);
+			strbuf_add(&label_from_message, p1, p2 - p1);
 		else if (skip_prefix(oneline.buf, "Merge pull request ",
 				     &p1) &&
 			 (p1 = strstr(p1, " from ")))
-			strbuf_addstr(&label, p1 + strlen(" from "));
+			strbuf_addstr(&label_from_message, p1 + strlen(" from "));
 		else
-			strbuf_addbuf(&label, &oneline);
+			strbuf_addbuf(&label_from_message, &oneline);
 
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "%s -C %s",
@@ -5921,6 +5922,14 @@  static int make_script_with_merges(struct pretty_print_context *pp,
 
 		/* label the tips of merged branches */
 		for (; to_merge; to_merge = to_merge->next) {
+			const char *label = label_from_message.buf;
+			const struct name_decoration *decoration =
+				get_name_decoration(&to_merge->item->object);
+
+			if (decoration)
+				skip_prefix(decoration->name, "refs/heads/",
+					    &label);
+
 			oid = &to_merge->item->object.oid;
 			strbuf_addch(&buf, ' ');
 
@@ -5933,7 +5942,7 @@  static int make_script_with_merges(struct pretty_print_context *pp,
 			tips_tail = &commit_list_insert(to_merge->item,
 							tips_tail)->next;
 
-			strbuf_addstr(&buf, label_oid(oid, label.buf, &state));
+			strbuf_addstr(&buf, label_oid(oid, label, &state));
 		}
 		strbuf_addf(&buf, " # %s", oneline.buf);
 
@@ -6041,7 +6050,7 @@  static int make_script_with_merges(struct pretty_print_context *pp,
 	free_commit_list(commits);
 	free_commit_list(tips);
 
-	strbuf_release(&label);
+	strbuf_release(&label_from_message);
 	strbuf_release(&oneline);
 	strbuf_release(&buf);
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index f171af3061d..4896a801ee2 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1870,7 +1870,7 @@  test_expect_success '--update-refs adds commands with --rebase-merges' '
 		pick $(git log -1 --format=%h branch2~1) F
 		pick $(git log -1 --format=%h branch2) I
 		update-ref refs/heads/branch2
-		label merge
+		label branch2
 		reset onto
 		pick $(git log -1 --format=%h refs/heads/second) J
 		update-ref refs/heads/second
@@ -1881,7 +1881,7 @@  test_expect_success '--update-refs adds commands with --rebase-merges' '
 		update-ref refs/heads/third
 		pick $(git log -1 --format=%h HEAD~2) M
 		update-ref refs/heads/no-conflict-branch
-		merge -C $(git log -1 --format=%h HEAD~1) merge # merge
+		merge -C $(git log -1 --format=%h HEAD~1) branch2 # merge
 		update-ref refs/heads/merge-branch
 		EOF
 
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 2aa8593f77a..cb891eeb5fd 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -108,19 +108,19 @@  test_expect_success 'generate correct todo list' '
 
 	reset onto
 	pick $b B
-	label E
+	label first
 
 	reset onto
 	pick $c C
 	label branch-point
 	pick $f F
 	pick $g G
-	label H
+	label second
 
 	reset branch-point # C
 	pick $d D
-	merge -C $e E # E
-	merge -C $h H # H
+	merge -C $e first # E
+	merge -C $h second # H
 
 	EOF
 
@@ -462,11 +462,11 @@  test_expect_success 'A root commit can be a cousin, treat it that way' '
 '
 
 test_expect_success 'labels that are object IDs are rewritten' '
-	git checkout -b third B &&
+	git checkout --detach B &&
 	test_commit I &&
 	third=$(git rev-parse HEAD) &&
 	git checkout -b labels main &&
-	git merge --no-commit third &&
+	git merge --no-commit $third &&
 	test_tick &&
 	git commit -m "Merge commit '\''$third'\'' into labels" &&
 	echo noop >script-from-scratch &&