threads / patch / 62167

patch, 2 partsrebase-merges: try and use branch names for labels

Subject: [PATCH 0/2] rebase-merges: try and use branch names for labels

## tl;dr

18 messages between Sep 21, 2024 and Oct 9, 2024. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

Nicolas Guichard via GitGitGadget· Sep 21, 2024, 18:37 UTC · lore

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

Nicolas Guichard (2):
  sequencer.c: extract load_branch_decorations
  rebase-merges: try and use branch names as labels
 sequencer.c                   | 50 ++++++++++++++++++++++-------------
 t/t3404-rebase-interactive.sh |  4 +--
 t/t3430-rebase-merges.sh      | 12 ++++-----
 3 files changed, 40 insertions(+), 26 deletions(-)
base-commit: 94b60adee30619a05296cf5ed6addb0e6d4e25dc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1784%2Fnicolas-guichard%2Fuse-branch-names-for-rebase-merges-labels-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1784/nicolas-guichard/use-branch-names-for-rebase-merges-labels-v1
Pull-Request: https://github.com/git/git/pull/1784
-- 
gitgitgadget
Nicolas Guichard via GitGitGadget· Sep 21, 2024, 18:37 UTC · re: Nicolas Guichard via GitGitGadget · lore

[PATCH 1/2] sequencer.c: extract load_branch_decorations

From: Nicolas Guichard <nicolas@guichard.eu>

Extract load_branch_decorations from todo_list_add_update_ref_commands so it can be re-used in make_script_with_merges.

Signed-off-by: Nicolas Guichard <nicolas@guichard.eu>
---
 sequencer.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)
Show changes to sequencer.c +15 −10
diff --git a/sequencer.c b/sequencer.c
index 8d01cd50ac9..e5eb6f8cd76 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5810,6 +5810,20 @@ static const char *label_oid(struct object_id *oid, const char *label,
 	return string_entry->string;
 }
 
+static void load_branch_decorations(void)
+{
+	static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
+	static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
+	static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
+	struct decoration_filter decoration_filter = {
+		.include_ref_pattern = &decorate_refs_include,
+		.exclude_ref_pattern = &decorate_refs_exclude,
+		.exclude_ref_config_pattern = &decorate_refs_exclude_config,
+	};
+	string_list_append(&decorate_refs_include, "refs/heads/");
+	load_ref_decorations(&decoration_filter, 0);
+}
+
 static int make_script_with_merges(struct pretty_print_context *pp,
 				   struct rev_info *revs, struct strbuf *out,
 				   unsigned flags)
@@ -6403,14 +6417,6 @@ static int add_decorations_to_list(const struct commit *commit,
 static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
 {
 	int i, res;
-	static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
-	static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
-	static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
-	struct decoration_filter decoration_filter = {
-		.include_ref_pattern = &decorate_refs_include,
-		.exclude_ref_pattern = &decorate_refs_exclude,
-		.exclude_ref_config_pattern = &decorate_refs_exclude_config,
-	};
 	struct todo_add_branch_context ctx = {
 		.buf = &todo_list->buf,
 		.refs_to_oids = STRING_LIST_INIT_DUP,
@@ -6419,8 +6425,7 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
 	ctx.items_alloc = 2 * todo_list->nr + 1;
 	ALLOC_ARRAY(ctx.items, ctx.items_alloc);
 
-	string_list_append(&decorate_refs_include, "refs/heads/");
-	load_ref_decorations(&decoration_filter, 0);
+	load_branch_decorations();
 
 	for (i = 0; i < todo_list->nr; ) {
 		struct todo_item *item = &todo_list->items[i];
-- 
gitgitgadget
Junio C Hamano· Sep 23, 2024, 19:32 UTC · re: Nicolas Guichard via GitGitGadget · lore

Re: [PATCH 1/2] sequencer.c: extract load_branch_decorations

"Nicolas Guichard via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 9 quoted lines
> From: Nicolas Guichard <nicolas@guichard.eu>
>
> Extract load_branch_decorations from todo_list_add_update_ref_commands so
> it can be re-used in make_script_with_merges.
>
> Signed-off-by: Nicolas Guichard <nicolas@guichard.eu>
> ---
>  sequencer.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)

This intends no behaviour change, and it indeed does not make any behaviour change. Good.

Show 21 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index 8d01cd50ac9..e5eb6f8cd76 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5810,6 +5810,20 @@ static const char *label_oid(struct object_id *oid, const char *label,
>  	return string_entry->string;
>  }
>  
> +static void load_branch_decorations(void)
> +{
> +	static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
> +	static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
> +	static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
> +	struct decoration_filter decoration_filter = {
> +		.include_ref_pattern = &decorate_refs_include,
> +		.exclude_ref_pattern = &decorate_refs_exclude,
> +		.exclude_ref_config_pattern = &decorate_refs_exclude_config,
> +	};
> +	string_list_append(&decorate_refs_include, "refs/heads/");
> +	load_ref_decorations(&decoration_filter, 0);
> +}

The load_ref_decorations() function can be called only once per process, so it does not matter whatever garbage your second and later invocations throw at it.

But if this function is ever called N times, you'll add N copies of "refs/heads/" in the _include list, and because the variable is static, it is not reported as a leak.

It may have been OK back when this implementation was inside todo_list_add_update_ref_commands(), which was called by complete_action() only once before the process exited.

But now you made it a "reusable" function, we should clean up after ourselves, perhaps by dropping "static" from these three string_list instances and clearing them after calling load_ref_decorations().

Nicolas Guichard via GitGitGadget· Sep 21, 2024, 18:38 UTC · re: Nicolas Guichard via GitGitGadget · lore

[PATCH 2/2] rebase-merges: try and use branch names as labels

From: Nicolas Guichard <nicolas@guichard.eu>

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 <nicolas@guichard.eu>
---
 sequencer.c                   | 25 +++++++++++++++++--------
 t/t3404-rebase-interactive.sh |  4 ++--
 t/t3430-rebase-merges.sh      | 12 ++++++------
 3 files changed, 25 insertions(+), 16 deletions(-)
Show changes to 3 files +25 −16

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

diff --git a/sequencer.c b/sequencer.c
index e5eb6f8cd76..a092bd05692 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5833,7 +5833,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;
@@ -5856,6 +5856,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;
@@ -5916,18 +5917,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",
@@ -5935,6 +5936,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, ' ');
 
@@ -5947,7 +5956,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);
 
@@ -6055,7 +6064,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 &&
-- 
gitgitgadget
Junio C Hamano· Sep 23, 2024, 19:38 UTC · re: Nicolas Guichard via GitGitGadget · lore

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

Running
    git blame -L'/^static int make_script_with_merges/,/^}/' sequencer.c

tells me that the code range this patch touches and the function whose feature this patch extends was mostly introduced in 1644c73c (rebase-helper --make-script: introduce a flag to rebase merges, 2018-04-25) by Johannes Schindelin (CC'ed), so let's ask input from him as an area expert.

Thanks.
"Nicolas Guichard via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 184 quoted lines
> From: Nicolas Guichard <nicolas@guichard.eu>
>
> 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 <nicolas@guichard.eu>
> ---
>  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 e5eb6f8cd76..a092bd05692 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5833,7 +5833,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;
> @@ -5856,6 +5856,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;
> @@ -5916,18 +5917,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",
> @@ -5935,6 +5936,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, ' ');
>  
> @@ -5947,7 +5956,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);
>  
> @@ -6055,7 +6064,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 &&
Nicolas Guichard via GitGitGadget· Oct 4, 2024, 23:22 UTC · re: Nicolas Guichard via GitGitGadget · lore

[PATCH v2 0/3] rebase-merges: try and use branch names for labels

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

Changes since v1:
 * moved load_branch_decorations to re-use the decoration_loaded guard and
   avoid pointlessly appending "refs/heads/" to a static string_list, as
   pointed out by Junio C Hamano (thanks!)
 * fixed a leak in load_branch_decorations found by making the filter
   string_lists non-static
Nicolas Guichard (3):
  load_branch_decorations: fix memory leak with non-static filters
  rebase-update-refs: extract load_branch_decorations
  rebase-merges: try and use branch names as labels
 log-tree.c                    | 26 +++++++++++++++++++++++++
 log-tree.h                    |  1 +
 sequencer.c                   | 36 +++++++++++++++++------------------
 t/t3404-rebase-interactive.sh |  4 ++--
 t/t3430-rebase-merges.sh      | 12 ++++++------
 5 files changed, 53 insertions(+), 26 deletions(-)
base-commit: 8895aca99693e22ec4856744d5b2c25b8736a111
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1784%2Fnicolas-guichard%2Fuse-branch-names-for-rebase-merges-labels-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1784/nicolas-guichard/use-branch-names-for-rebase-merges-labels-v2
Pull-Request: https://github.com/git/git/pull/1784
Range-diff vs v1:
 -:  ----------- > 1:  6250a7f6d6c load_branch_decorations: fix memory leak with non-static filters
 1:  7f3d5e5da35 ! 2:  167418d10d1 sequencer.c: extract load_branch_decorations
     @@ Metadata
      Author: Nicolas Guichard <nicolas@guichard.eu>
      
       ## Commit message ##
     -    sequencer.c: extract load_branch_decorations
     +    rebase-update-refs: extract load_branch_decorations
      
          Extract load_branch_decorations from todo_list_add_update_ref_commands so
          it can be re-used in make_script_with_merges.
      
     +    Since it can now be called multiple times, use non-static lists and place
     +    it next to load_ref_decorations to re-use the decoration_loaded guard.
     +
          Signed-off-by: Nicolas Guichard <nicolas@guichard.eu>
      
     - ## sequencer.c ##
     -@@ sequencer.c: static const char *label_oid(struct object_id *oid, const char *label,
     - 	return string_entry->string;
     + ## log-tree.c ##
     +@@ log-tree.c: void load_ref_decorations(struct decoration_filter *filter, int flags)
     + 	}
       }
       
     -+static void load_branch_decorations(void)
     ++void load_branch_decorations(void)
      +{
     -+	static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
     -+	static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
     -+	static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
     -+	struct decoration_filter decoration_filter = {
     -+		.include_ref_pattern = &decorate_refs_include,
     -+		.exclude_ref_pattern = &decorate_refs_exclude,
     -+		.exclude_ref_config_pattern = &decorate_refs_exclude_config,
     -+	};
     -+	string_list_append(&decorate_refs_include, "refs/heads/");
     -+	load_ref_decorations(&decoration_filter, 0);
     ++	if (!decoration_loaded) {
     ++		struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
     ++		struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
     ++		struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
     ++		struct decoration_filter decoration_filter = {
     ++			.include_ref_pattern = &decorate_refs_include,
     ++			.exclude_ref_pattern = &decorate_refs_exclude,
     ++			.exclude_ref_config_pattern = &decorate_refs_exclude_config,
     ++		};
     ++
     ++		string_list_append(&decorate_refs_include, "refs/heads/");
     ++		load_ref_decorations(&decoration_filter, 0);
     ++
     ++		string_list_clear(&decorate_refs_exclude, false);
     ++		string_list_clear(&decorate_refs_exclude_config, false);
     ++		string_list_clear(&decorate_refs_include, false);
     ++	}
      +}
      +
     - static int make_script_with_merges(struct pretty_print_context *pp,
     - 				   struct rev_info *revs, struct strbuf *out,
     - 				   unsigned flags)
     + static void show_parents(struct commit *commit, int abbrev, FILE *file)
     + {
     + 	struct commit_list *p;
     +
     + ## log-tree.h ##
     +@@ log-tree.h: void log_write_email_headers(struct rev_info *opt, struct commit *commit,
     + 			     int *need_8bit_cte_p,
     + 			     int maybe_multipart);
     + void load_ref_decorations(struct decoration_filter *filter, int flags);
     ++void load_branch_decorations(void);
     + 
     + void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
     + void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
     +
     + ## sequencer.c ##
      @@ sequencer.c: static int add_decorations_to_list(const struct commit *commit,
       static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
       {
 2:  9afe428927d = 3:  dfa1f0a7648 rebase-merges: try and use branch names as labels
-- 
gitgitgadget
Nicolas Guichard via GitGitGadget· Oct 4, 2024, 23:22 UTC · re: Nicolas Guichard via GitGitGadget · lore

[PATCH v2 1/3] load_branch_decorations: fix memory leak with non-static filters

From: Nicolas Guichard <nicolas@guichard.eu>

load_branch_decorations calls normalize_glob_ref on each string of filter's string_lists. This effectively replaces the potentially non-owning char* of those items with an owning char*.

Set the strdup_string flag on those string_lists.
This was not caught until now because:
- when passing string_lists already with the strdup_string already set, the
  behaviour was correct
- when passing static string_lists, the new char* remain reachable until
  program exit
Signed-off-by: Nicolas Guichard <nicolas@guichard.eu>
---
 log-tree.c | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to log-tree.c +5 −0
diff --git a/log-tree.c b/log-tree.c
index 3758e0d3b8e..cd57de2424e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -232,6 +232,11 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)
 			for_each_string_list_item(item, filter->exclude_ref_config_pattern) {
 				normalize_glob_ref(item, NULL, item->string);
 			}
+
+			// normalize_glob_ref duplicates the strings
+			filter->exclude_ref_pattern->strdup_strings = true;
+			filter->include_ref_pattern->strdup_strings = true;
+			filter->exclude_ref_config_pattern->strdup_strings = true;
 		}
 		decoration_loaded = 1;
 		decoration_flags = flags;
-- 
gitgitgadget
Eric Sunshine· Oct 5, 2024, 03:43 UTC · re: Nicolas Guichard via GitGitGadget · lore

Re: [PATCH v2 1/3] load_branch_decorations: fix memory leak with non-static filters

On Fri, Oct 4, 2024 at 7:22 PM Nicolas Guichard via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 13 quoted lines
> load_branch_decorations calls normalize_glob_ref on each string of filter's
> string_lists. This effectively replaces the potentially non-owning char* of
> those items with an owning char*.
>
> Set the strdup_string flag on those string_lists.
>
> This was not caught until now because:
> - when passing string_lists already with the strdup_string already set, the
>   behaviour was correct
> - when passing static string_lists, the new char* remain reachable until
>   program exit
>
> Signed-off-by: Nicolas Guichard <nicolas@guichard.eu>
Not a proper review; just a couple style nits below...
Show 6 quoted lines
> diff --git a/log-tree.c b/log-tree.c
> @@ -232,6 +232,11 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)
> +                       // normalize_glob_ref duplicates the strings
> +                       filter->exclude_ref_pattern->strdup_strings = true;
> +                       filter->include_ref_pattern->strdup_strings = true;
> +                       filter->exclude_ref_config_pattern->strdup_strings = true;

For the sake of older platforms and older compilers, we're still avoiding certain modernisms on this project. Instead:

* use /* ... */ style comments instead of // style
* use 1 instead of `true` (and 0 instead of `false`)
Nicolas Guichard via GitGitGadget· Oct 4, 2024, 23:22 UTC · re: Nicolas Guichard via GitGitGadget · lore

[PATCH v2 2/3] rebase-update-refs: extract load_branch_decorations

From: Nicolas Guichard <nicolas@guichard.eu>

Extract load_branch_decorations from todo_list_add_update_ref_commands so it can be re-used in make_script_with_merges.

Since it can now be called multiple times, use non-static lists and place it next to load_ref_decorations to re-use the decoration_loaded guard.

Signed-off-by: Nicolas Guichard <nicolas@guichard.eu>
---
 log-tree.c  | 21 +++++++++++++++++++++
 log-tree.h  |  1 +
 sequencer.c | 11 +----------
 3 files changed, 23 insertions(+), 10 deletions(-)
Show changes to 3 files +23 −10

log-tree.c, log-tree.h, sequencer.c

diff --git a/log-tree.c b/log-tree.c
index cd57de2424e..5c17aff2265 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -248,6 +248,27 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)
 	}
 }
 
+void load_branch_decorations(void)
+{
+	if (!decoration_loaded) {
+		struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
+		struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
+		struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
+		struct decoration_filter decoration_filter = {
+			.include_ref_pattern = &decorate_refs_include,
+			.exclude_ref_pattern = &decorate_refs_exclude,
+			.exclude_ref_config_pattern = &decorate_refs_exclude_config,
+		};
+
+		string_list_append(&decorate_refs_include, "refs/heads/");
+		load_ref_decorations(&decoration_filter, 0);
+
+		string_list_clear(&decorate_refs_exclude, false);
+		string_list_clear(&decorate_refs_exclude_config, false);
+		string_list_clear(&decorate_refs_include, false);
+	}
+}
+
 static void show_parents(struct commit *commit, int abbrev, FILE *file)
 {
 	struct commit_list *p;
diff --git a/log-tree.h b/log-tree.h
index 94978e2c838..ebe491c543c 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -33,6 +33,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     int *need_8bit_cte_p,
 			     int maybe_multipart);
 void load_ref_decorations(struct decoration_filter *filter, int flags);
+void load_branch_decorations(void);
 
 void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
 void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
diff --git a/sequencer.c b/sequencer.c
index 8d01cd50ac9..97959036b50 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6403,14 +6403,6 @@ static int add_decorations_to_list(const struct commit *commit,
 static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
 {
 	int i, res;
-	static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
-	static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
-	static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
-	struct decoration_filter decoration_filter = {
-		.include_ref_pattern = &decorate_refs_include,
-		.exclude_ref_pattern = &decorate_refs_exclude,
-		.exclude_ref_config_pattern = &decorate_refs_exclude_config,
-	};
 	struct todo_add_branch_context ctx = {
 		.buf = &todo_list->buf,
 		.refs_to_oids = STRING_LIST_INIT_DUP,
@@ -6419,8 +6411,7 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
 	ctx.items_alloc = 2 * todo_list->nr + 1;
 	ALLOC_ARRAY(ctx.items, ctx.items_alloc);
 
-	string_list_append(&decorate_refs_include, "refs/heads/");
-	load_ref_decorations(&decoration_filter, 0);
+	load_branch_decorations();
 
 	for (i = 0; i < todo_list->nr; ) {
 		struct todo_item *item = &todo_list->items[i];
-- 
gitgitgadget
Eric Sunshine· Oct 5, 2024, 03:44 UTC · re: Nicolas Guichard via GitGitGadget · lore

Re: [PATCH v2 2/3] rebase-update-refs: extract load_branch_decorations

On Fri, Oct 4, 2024 at 7:22 PM Nicolas Guichard via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 29 quoted lines
> Extract load_branch_decorations from todo_list_add_update_ref_commands so
> it can be re-used in make_script_with_merges.
>
> Since it can now be called multiple times, use non-static lists and place
> it next to load_ref_decorations to re-use the decoration_loaded guard.
>
> Signed-off-by: Nicolas Guichard <nicolas@guichard.eu>
> ---
> diff --git a/log-tree.c b/log-tree.c
> @@ -248,6 +248,27 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)
> +void load_branch_decorations(void)
> +{
> +       if (!decoration_loaded) {
> +               struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
> +               struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
> +               struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
> +               struct decoration_filter decoration_filter = {
> +                       .include_ref_pattern = &decorate_refs_include,
> +                       .exclude_ref_pattern = &decorate_refs_exclude,
> +                       .exclude_ref_config_pattern = &decorate_refs_exclude_config,
> +               };
> +
> +               string_list_append(&decorate_refs_include, "refs/heads/");
> +               load_ref_decorations(&decoration_filter, 0);
> +
> +               string_list_clear(&decorate_refs_exclude, false);
> +               string_list_clear(&decorate_refs_exclude_config, false);
> +               string_list_clear(&decorate_refs_include, false);
> +       }
Same minor style nit as previous patch:
* use 0 instead of `false`
Nicolas Guichard via GitGitGadget· Oct 4, 2024, 23:22 UTC · re: Nicolas Guichard via GitGitGadget · lore

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

From: Nicolas Guichard <nicolas@guichard.eu>

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 <nicolas@guichard.eu>
---
 sequencer.c                   | 25 +++++++++++++++++--------
 t/t3404-rebase-interactive.sh |  4 ++--
 t/t3430-rebase-merges.sh      | 12 ++++++------
 3 files changed, 25 insertions(+), 16 deletions(-)
Show changes to 3 files +25 −16

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

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 &&
-- 
gitgitgadget
Nicolas Guichard via GitGitGadget· Oct 9, 2024, 07:58 UTC · re: Nicolas Guichard via GitGitGadget · lore

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

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

Changes since v1:
 * moved load_branch_decorations to re-use the decoration_loaded guard and
   avoid pointlessly appending "refs/heads/" to a static string_list, as
   pointed out by Junio C Hamano (thanks!)
 * fixed a leak in load_branch_decorations found by making the filter
   string_lists non-static
Changes since v2:
 * style changes (true/false -> 1/0 and // -> /* */)
Nicolas Guichard (3):
  load_branch_decorations: fix memory leak with non-static filters
  rebase-update-refs: extract load_branch_decorations
  rebase-merges: try and use branch names as labels
 log-tree.c                    | 26 +++++++++++++++++++++++++
 log-tree.h                    |  1 +
 sequencer.c                   | 36 +++++++++++++++++------------------
 t/t3404-rebase-interactive.sh |  4 ++--
 t/t3430-rebase-merges.sh      | 12 ++++++------
 5 files changed, 53 insertions(+), 26 deletions(-)
base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1784%2Fnicolas-guichard%2Fuse-branch-names-for-rebase-merges-labels-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1784/nicolas-guichard/use-branch-names-for-rebase-merges-labels-v3
Pull-Request: https://github.com/git/git/pull/1784
Range-diff vs v2:
 1:  6250a7f6d6c ! 1:  e030ddd91f3 load_branch_decorations: fix memory leak with non-static filters
     @@ log-tree.c: void load_ref_decorations(struct decoration_filter *filter, int flag
       				normalize_glob_ref(item, NULL, item->string);
       			}
      +
     -+			// normalize_glob_ref duplicates the strings
     -+			filter->exclude_ref_pattern->strdup_strings = true;
     -+			filter->include_ref_pattern->strdup_strings = true;
     -+			filter->exclude_ref_config_pattern->strdup_strings = true;
     ++			/* normalize_glob_ref duplicates the strings */
     ++			filter->exclude_ref_pattern->strdup_strings = 1;
     ++			filter->include_ref_pattern->strdup_strings = 1;
     ++			filter->exclude_ref_config_pattern->strdup_strings = 1;
       		}
       		decoration_loaded = 1;
       		decoration_flags = flags;
 2:  167418d10d1 ! 2:  1dad6096eb7 rebase-update-refs: extract load_branch_decorations
     @@ log-tree.c: void load_ref_decorations(struct decoration_filter *filter, int flag
      +		string_list_append(&decorate_refs_include, "refs/heads/");
      +		load_ref_decorations(&decoration_filter, 0);
      +
     -+		string_list_clear(&decorate_refs_exclude, false);
     -+		string_list_clear(&decorate_refs_exclude_config, false);
     -+		string_list_clear(&decorate_refs_include, false);
     ++		string_list_clear(&decorate_refs_exclude, 0);
     ++		string_list_clear(&decorate_refs_exclude_config, 0);
     ++		string_list_clear(&decorate_refs_include, 0);
      +	}
      +}
      +
 3:  dfa1f0a7648 = 3:  19d8253da07 rebase-merges: try and use branch names as labels
-- 
gitgitgadget
Nicolas Guichard via GitGitGadget· Oct 9, 2024, 07:58 UTC · re: Nicolas Guichard via GitGitGadget · lore

[PATCH v3 1/3] load_branch_decorations: fix memory leak with non-static filters

From: Nicolas Guichard <nicolas@guichard.eu>

load_branch_decorations calls normalize_glob_ref on each string of filter's string_lists. This effectively replaces the potentially non-owning char* of those items with an owning char*.

Set the strdup_string flag on those string_lists.
This was not caught until now because:
- when passing string_lists already with the strdup_string already set, the
  behaviour was correct
- when passing static string_lists, the new char* remain reachable until
  program exit
Signed-off-by: Nicolas Guichard <nicolas@guichard.eu>
---
 log-tree.c | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to log-tree.c +5 −0
diff --git a/log-tree.c b/log-tree.c
index 3758e0d3b8e..c65ebd29202 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -232,6 +232,11 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)
 			for_each_string_list_item(item, filter->exclude_ref_config_pattern) {
 				normalize_glob_ref(item, NULL, item->string);
 			}
+
+			/* normalize_glob_ref duplicates the strings */
+			filter->exclude_ref_pattern->strdup_strings = 1;
+			filter->include_ref_pattern->strdup_strings = 1;
+			filter->exclude_ref_config_pattern->strdup_strings = 1;
 		}
 		decoration_loaded = 1;
 		decoration_flags = flags;
-- 
gitgitgadget
Nicolas Guichard via GitGitGadget· Oct 9, 2024, 07:58 UTC · re: Nicolas Guichard via GitGitGadget · lore

[PATCH v3 2/3] rebase-update-refs: extract load_branch_decorations

From: Nicolas Guichard <nicolas@guichard.eu>

Extract load_branch_decorations from todo_list_add_update_ref_commands so it can be re-used in make_script_with_merges.

Since it can now be called multiple times, use non-static lists and place it next to load_ref_decorations to re-use the decoration_loaded guard.

Signed-off-by: Nicolas Guichard <nicolas@guichard.eu>
---
 log-tree.c  | 21 +++++++++++++++++++++
 log-tree.h  |  1 +
 sequencer.c | 11 +----------
 3 files changed, 23 insertions(+), 10 deletions(-)
Show changes to 3 files +23 −10

log-tree.c, log-tree.h, sequencer.c

diff --git a/log-tree.c b/log-tree.c
index c65ebd29202..bf6d2ae40c8 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -248,6 +248,27 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)
 	}
 }
 
+void load_branch_decorations(void)
+{
+	if (!decoration_loaded) {
+		struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
+		struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
+		struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
+		struct decoration_filter decoration_filter = {
+			.include_ref_pattern = &decorate_refs_include,
+			.exclude_ref_pattern = &decorate_refs_exclude,
+			.exclude_ref_config_pattern = &decorate_refs_exclude_config,
+		};
+
+		string_list_append(&decorate_refs_include, "refs/heads/");
+		load_ref_decorations(&decoration_filter, 0);
+
+		string_list_clear(&decorate_refs_exclude, 0);
+		string_list_clear(&decorate_refs_exclude_config, 0);
+		string_list_clear(&decorate_refs_include, 0);
+	}
+}
+
 static void show_parents(struct commit *commit, int abbrev, FILE *file)
 {
 	struct commit_list *p;
diff --git a/log-tree.h b/log-tree.h
index 94978e2c838..ebe491c543c 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -33,6 +33,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     int *need_8bit_cte_p,
 			     int maybe_multipart);
 void load_ref_decorations(struct decoration_filter *filter, int flags);
+void load_branch_decorations(void);
 
 void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
 void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
diff --git a/sequencer.c b/sequencer.c
index 8d01cd50ac9..97959036b50 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6403,14 +6403,6 @@ static int add_decorations_to_list(const struct commit *commit,
 static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
 {
 	int i, res;
-	static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
-	static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
-	static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
-	struct decoration_filter decoration_filter = {
-		.include_ref_pattern = &decorate_refs_include,
-		.exclude_ref_pattern = &decorate_refs_exclude,
-		.exclude_ref_config_pattern = &decorate_refs_exclude_config,
-	};
 	struct todo_add_branch_context ctx = {
 		.buf = &todo_list->buf,
 		.refs_to_oids = STRING_LIST_INIT_DUP,
@@ -6419,8 +6411,7 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
 	ctx.items_alloc = 2 * todo_list->nr + 1;
 	ALLOC_ARRAY(ctx.items, ctx.items_alloc);
 
-	string_list_append(&decorate_refs_include, "refs/heads/");
-	load_ref_decorations(&decoration_filter, 0);
+	load_branch_decorations();
 
 	for (i = 0; i < todo_list->nr; ) {
 		struct todo_item *item = &todo_list->items[i];
-- 
gitgitgadget
Nicolas Guichard via GitGitGadget· Oct 9, 2024, 07:58 UTC · re: Nicolas Guichard via GitGitGadget · lore

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

From: Nicolas Guichard <nicolas@guichard.eu>

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 <nicolas@guichard.eu>
---
 sequencer.c                   | 25 +++++++++++++++++--------
 t/t3404-rebase-interactive.sh |  4 ++--
 t/t3430-rebase-merges.sh      | 12 ++++++------
 3 files changed, 25 insertions(+), 16 deletions(-)
Show changes to 3 files +25 −16

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

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 &&
-- 
gitgitgadget
Phillip Wood· Oct 9, 2024, 14:21 UTC · re: Nicolas Guichard via GitGitGadget · lore

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

Hi Nicolas
On 09/10/2024 08:58, Nicolas Guichard via GitGitGadget wrote:
Show 34 quoted lines
> From: Nicolas Guichard <nicolas@guichard.eu>
> 
> 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
Show 149 quoted lines
> Signed-off-by: Nicolas Guichard <nicolas@guichard.eu>
> ---
>   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, 17:57 UTC · re: Phillip Wood · lore

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

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 41 quoted lines
> Hi Nicolas
>
> On 09/10/2024 08:58, Nicolas Guichard via GitGitGadget wrote:
>> From: Nicolas Guichard <nicolas@guichard.eu>
>> 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.
Phillip Wood· Oct 9, 2024, 14:22 UTC · re: Nicolas Guichard via GitGitGadget · lore

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

Hi Nicolas
Thanks for working on this. This version looks good to me
Thanks
Phillip
On 09/10/2024 08:58, Nicolas Guichard via GitGitGadget wrote:
Show 97 quoted lines
> 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
> 
> 
> which 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.
> 
> Changes since v1:
> 
>   * moved load_branch_decorations to re-use the decoration_loaded guard and
>     avoid pointlessly appending "refs/heads/" to a static string_list, as
>     pointed out by Junio C Hamano (thanks!)
>   * fixed a leak in load_branch_decorations found by making the filter
>     string_lists non-static
> 
> Changes since v2:
> 
>   * style changes (true/false -> 1/0 and // -> /* */)
> 
> Nicolas Guichard (3):
>    load_branch_decorations: fix memory leak with non-static filters
>    rebase-update-refs: extract load_branch_decorations
>    rebase-merges: try and use branch names as labels
> 
>   log-tree.c                    | 26 +++++++++++++++++++++++++
>   log-tree.h                    |  1 +
>   sequencer.c                   | 36 +++++++++++++++++------------------
>   t/t3404-rebase-interactive.sh |  4 ++--
>   t/t3430-rebase-merges.sh      | 12 ++++++------
>   5 files changed, 53 insertions(+), 26 deletions(-)
> 
> 
> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1784%2Fnicolas-guichard%2Fuse-branch-names-for-rebase-merges-labels-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1784/nicolas-guichard/use-branch-names-for-rebase-merges-labels-v3
> Pull-Request: https://github.com/git/git/pull/1784
> 
> Range-diff vs v2:
> 
>   1:  6250a7f6d6c ! 1:  e030ddd91f3 load_branch_decorations: fix memory leak with non-static filters
>       @@ log-tree.c: void load_ref_decorations(struct decoration_filter *filter, int flag
>         				normalize_glob_ref(item, NULL, item->string);
>         			}
>        +
>       -+			// normalize_glob_ref duplicates the strings
>       -+			filter->exclude_ref_pattern->strdup_strings = true;
>       -+			filter->include_ref_pattern->strdup_strings = true;
>       -+			filter->exclude_ref_config_pattern->strdup_strings = true;
>       ++			/* normalize_glob_ref duplicates the strings */
>       ++			filter->exclude_ref_pattern->strdup_strings = 1;
>       ++			filter->include_ref_pattern->strdup_strings = 1;
>       ++			filter->exclude_ref_config_pattern->strdup_strings = 1;
>         		}
>         		decoration_loaded = 1;
>         		decoration_flags = flags;
>   2:  167418d10d1 ! 2:  1dad6096eb7 rebase-update-refs: extract load_branch_decorations
>       @@ log-tree.c: void load_ref_decorations(struct decoration_filter *filter, int flag
>        +		string_list_append(&decorate_refs_include, "refs/heads/");
>        +		load_ref_decorations(&decoration_filter, 0);
>        +
>       -+		string_list_clear(&decorate_refs_exclude, false);
>       -+		string_list_clear(&decorate_refs_exclude_config, false);
>       -+		string_list_clear(&decorate_refs_include, false);
>       ++		string_list_clear(&decorate_refs_exclude, 0);
>       ++		string_list_clear(&decorate_refs_exclude_config, 0);
>       ++		string_list_clear(&decorate_refs_include, 0);
>        +	}
>        +}
>        +
>   3:  dfa1f0a7648 = 3:  19d8253da07 rebase-merges: try and use branch names as labels
> 

← back to recent threads