Thread (15 messages) 15 messages, 3 authors, 2024-10-09

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

From: Phillip Wood <hidden>
Date: 2024-10-09 14:21:59

Hi Nicolas

On 09/10/2024 08:58, Nicolas Guichard via GitGitGadget wrote:
From: Nicolas Guichard <redacted>

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
quoted hunk ↗ jump to hunk
Signed-off-by: Nicolas Guichard <redacted>
---
  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 &&
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help