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

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

[Nicolas Guichard via GitGitGadget]

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

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

[Nicolas Guichard via GitGitGadget]

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

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

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

[Nicolas Guichard via GitGitGadget]

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

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

[Junio C Hamano]

"Nicolas Guichard via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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

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

[Junio C Hamano]

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

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

[Nicolas Guichard via GitGitGadget]

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

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

[Nicolas Guichard via GitGitGadget]

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

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

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

[Nicolas Guichard via GitGitGadget]

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

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

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

[Nicolas Guichard via GitGitGadget]

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

On Fri, Oct 4, 2024 at 7:22 PM Nicolas Guichard via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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`

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

[Nicolas Guichard via GitGitGadget]

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

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

[Nicolas Guichard via GitGitGadget]

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

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

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

[Nicolas Guichard via GitGitGadget]

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

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

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

[Nicolas Guichard via GitGitGadget]

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

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

[Phillip Wood]

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

Phillip

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

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

[Phillip Wood]

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

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

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