rebase-merges: try and use branch names for labels

From: Nicolas Guichard <[email protected]>

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 <[email protected]>

---
 sequencer.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

sequencer.c +3 -0

index 8d01cd50ac9..e5eb6f8cd76 100644
@@ -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;

From: Nicolas Guichard <[email protected]>

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 <[email protected]>

---
 sequencer.c                   | 25 +++++++++++++++++--------
 t/t3404-rebase-interactive.sh |  4 ++--
 t/t3430-rebase-merges.sh      | 12 ++++++------
 3 files changed, 25 insertions(+), 16 deletions(-)

sequencer.c +17 -8

index e5eb6f8cd76..a092bd05692 100644
@@ -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);
 

t/t3404-rebase-interactive.sh +2 -2

index f171af3061d..4896a801ee2 100755
@@ -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
 

t/t3430-rebase-merges.sh +6 -6

index 2aa8593f77a..cb891eeb5fd 100755
@@ -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 &&

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 <[email protected]>

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 <[email protected]>

---

log-tree.c +9 -0

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

From: Nicolas Guichard <[email protected]>

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 <[email protected]>

---
  sequencer.c                   | 25 +++++++++++++++++--------
  t/t3404-rebase-interactive.sh |  4 ++--
  t/t3430-rebase-merges.sh      | 12 ++++++------
  3 files changed, 25 insertions(+), 16 deletions(-)

sequencer.c +17 -8

index 97959036b50..353d804999b 100644
@@ -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);
 

t/t3404-rebase-interactive.sh +2 -2

index f171af3061d..4896a801ee2 100755
@@ -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
 

t/t3430-rebase-merges.sh +6 -6

index 2aa8593f77a..cb891eeb5fd 100755
@@ -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 &&

Hi Nicolas

Quoted 26 lines · Nicolas Guichard via GitGitGadget

From: Nicolas Guichard <[email protected]>
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
```

Quoted 6 lines

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.

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