rebase -r: support octopus merges

From: Johannes Schindelin <[email protected]>

Now that we support octopus merges in the --rebase-merges mode, we should give users who actually read the manuals a chance to know about this fact.

Signed-off-by Johannes Schindelin <[email protected]>

---
 Documentation/git-rebase.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Documentation/git-rebase.txt +2 -2

index 0e20a66e7..c4bcd24bb 100644
@@ -879,8 +879,8 @@ rescheduled immediately, with a helpful message how to edit the todo list
 (this typically happens when a `reset` command was inserted into the todo
 list manually and contains a typo).
 
-The `merge` command will merge the specified revision into whatever is
-HEAD at that time. With `-C <original-commit>`, the commit message of
+The `merge` command will merge the specified revision(s) into whatever
+is HEAD at that time. With `-C <original-commit>`, the commit message of
 the specified merge commit will be used. When the `-C` is changed to
 a lower-case `-c`, the message will be opened in an editor after a
 successful merge so that the user can edit the message.

Documentation/git-rebase.txt +0 -1

 this, an `exec` command can be used to call `git merge` explicitly,
 using the fact that the labels are worktree-local refs (the ref
 `refs/rewritten/onto` would correspond to the label `onto`, for example).
--

gitgitgadget

The --rebase-merges option of git rebase generates todo lists that include the merge commits that are to be rebased.

To keep things simpler to review, I decided to support only regular, 2-parent merge commits first.

With this patch series, support is extended to cover also octopus merges.

Johannes Schindelin (3):
merge: allow reading the merge commit message from a file rebase --rebase-merges: add support for octopus merges rebase --rebase-merges: adjust man page for octopus support

 Documentation/git-merge.txt  |  10 ++-
 Documentation/git-rebase.txt |   7 +-
 builtin/merge.c              |  32 +++++++
 sequencer.c                  | 168 ++++++++++++++++++++++++++---------
 t/t3430-rebase-merges.sh     |  34 +++++++
 5 files changed, 204 insertions(+), 47 deletions(-)

base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-8%2Fdscho%2Fsequencer-and-octopus-merges-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-8/dscho/sequencer-and-octopus-merges-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/8

To be honest, I am not sure if there still are people who use octopus

Previously, we introduced the merge command for use in todo lists, to allow to recreate and modify branch topology.

For ease of implementation, and to make review easier, the initial implementation only supported merge commits with exactly two parents.

This patch adds support for octopus merges, making use of the just-introduced -F <file> option for the git merge command: to keep things simple, we spawn a new Git command instead of trying to call a library function, also opening an easier door to enhance rebase --rebase-merges to optionally use a merge strategy different from recursive for regular merges: this feature would use the same code path as octopus merges and simply spawn a git merge.

Signed-off-by Johannes Schindelin <[email protected]>

t/t3430-rebase-merges.sh +21 -0

+       : fast forward if possible &&
+       before="$(git rev-parse --verify HEAD)" &&
+       test_tick &&
+       git rebase -i -r HEAD^^ &&
+       test_cmp_rev HEAD $before &&
+
+       test_tick &&
+       git rebase -i --force -r HEAD^^ &&
+       test "Hank" = "$(git show -s --format=%an HEAD)" &&
+       test "$before" != $(git rev-parse HEAD) &&
+       test_cmp_graph HEAD^^.. <<-\EOF
+       *-.   Tüntenfüsch
+       |\ \
+       | | * three
+       | * | two
+       | |/
+       * | one
+       |/
+       o before-octopus
+       EOF
+'

sequencer.c static int do_merge(struct commit *commit, const char *arg, int arg_len, · line 2967

Quoted 5 lines

sequencer.c +2 -1

@@ -2932,7 +2966,8 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
-                       strbuf_addf(&buf, "Merge branch '%.*s'",
+                       strbuf_addf(&buf, "Merge %s '%.*s'",
+                                   to_merge->next ? "branches" : "branch",

Error messages in this file are already localizable. If this text ever becomes localizable, then this "sentence lego" could be problematic for translators.

sequencer.c static int do_merge(struct commit *commit, const char *arg, int arg_len, · line 2994

Quoted 5 lines

sequencer.c +4 -0

@@ -2956,28 +2991,76 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
+               cmd.git_cmd = 1;
+               argv_array_push(&cmd.args, "merge");
+               argv_array_push(&cmd.args, "-s");
+               argv_array_push(&cmd.args, "octopus");

argv_array_pushl(&cmd.args, "-s", "octopus", NULL);

which would make it clear that these two arguments must remain together, and prevent someone from coming along and inserting a new argument between them.

sequencer.c static int do_merge(struct commit *commit, const char *arg, int arg_len, · line 2996

Quoted 2 lines

sequencer.c +2 -0

+       (GIT_AUTHOR_NAME="Hank" GIT_AUTHOR_EMAIL="[email protected]" \
+        git merge -m "Tüntenfüsch" two three) &&

Unnecessary subshell?

From: Johannes Schindelin <[email protected]>

This is consistent with git commit which, like git merge, supports passing the commit message via -m <msg> and, unlike git merge before this patch, via -F <file>.

It is useful to allow this for scripted use, or for the upcoming patch to allow (re-)creating octopus merges in git rebase --rebase-merges.

Signed-off-by Johannes Schindelin <[email protected]>

---
 Documentation/git-merge.txt | 10 +++++++++-
 builtin/merge.c             | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

Documentation/git-merge.txt +9 -1

index 6a5c00e2c..537dca7fa 100644
@@ -12,7 +12,7 @@ SYNOPSIS
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
 	[-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
 	[--[no-]allow-unrelated-histories]
-	[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
+	[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
 'git merge' --abort
 'git merge' --continue
 

@@ -75,6 +75,14 @@ The 'git fmt-merge-msg' command can be
 used to give a good default for automated 'git merge'
 invocations. The automated message can include the branch description.
 
+-F <file>::
+--file=<file>::
+	Read the commit message to be used for the merge commit (in
+	case one is created).
++
+If `--log` is specified, a shortlog of the commits being merged
+will be appended to the specified message.
+
 --[no-]rerere-autoupdate::
 	Allow the rerere mechanism to update the index with the
 	result of auto-conflict resolution if possible.

builtin/merge.c +0 -0

index 4a4c09496..b0e907751 100644
@@ -111,6 +111,35 @@ static int option_parse_message(const struct option *opt,
 	return 0;

builtin/merge.c +7 -0

+
+	return 0;
+}
+
 static struct strategy *get_strategy(const char *name)
 {
 	int i;

@@ -228,6 +257,9 @@ static struct option builtin_merge_options[] = {
 	OPT_CALLBACK('m', "message", &merge_msg, N_("message"),
 		N_("merge commit message (for a non-fast-forward merge)"),
 		option_parse_message),
+	{ OPTION_LOWLEVEL_CALLBACK, 'F', "file", &merge_msg, N_("path"),
+		N_("read message from file"), PARSE_OPT_NONEG,
+		(parse_opt_cb *) option_read_message },
 	OPT__VERBOSITY(&verbosity),
 	OPT_BOOL(0, "abort", &abort_current_merge,
 		N_("abort the current in-progress merge")),

Quoted 7 lines · Johannes Schindelin via GitGitGadget

Documentation/git-rebase.txt +0 -0

index 0e20a66e7..c4bcd24bb 100644
@@ -889,7 +889,8 @@ If a `merge` command fails for any reason other than merge conflicts (i.e.
 when the merge operation did not even start), it is rescheduled immediately.
 

Documentation/git-rebase.txt If a `merge` command fails for any reason other than merge conflicts (i.e. · line 893

Quoted 4 lines

Documentation/git-rebase.txt +2 -1

 At this time, the `merge` command will *always* use the `recursive`
-merge strategy, with no way to choose a different one. To work around
+merge strategy for regular merges, and `octopus` for octopus merges,
+strategy, with no way to choose a different one. To work around

The "...merges, strategy, with..." looks like an incomplete edit. Perhaps "...and octopus strategy for octopus merges, with no way..." ?

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 8 lines · writes:

The --rebase-merges option of git rebase generates todo lists that include the merge commits that are to be rebased.

To keep things simpler to review, I decided to support only regular, 2-parent merge commits first.

With this patch series, support is extended to cover also octopus merges.

;-)

To be honest, I am not sure if there still are people who use octopus (even though I freely admit that I am guilty of inventing the term and the mechanism), given its downsides, primary one of which is that it makes bisection less efficient.

Nevertheless, this *is* the right thing to do from feature completeness point of view. Thanks for following it through.

Quoted 2 lines · Junio C Hamano

To be honest, I am not sure if there still are people who use octopus

The latest merge with more than 2 parents in linux.git is df958569dbaa (Merge branches 'acpi-tables' and 'acpica', 2018-07-05), although looking through the log of octopusses I get the impression that mostly Rafael J. Wysocki <[email protected]> is really keen on these. :-)

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 6 lines · writes:

builtin/merge.c +0 -0

index 4a4c09496..b0e907751 100644
@@ -111,6 +111,35 @@ static int option_parse_message(const struct option *opt,
 	return 0;

builtin/merge.c static int option_parse_message(const struct option *opt, · line 121

Quoted 10 lines

builtin/merge.c +8 -0

 }
 
+static int option_read_message(struct parse_opt_ctx_t *ctx,
+			       const struct option *opt, int unset)
+{
+	struct strbuf *buf = opt->value;
+	const char *arg;
+
+	if (unset)
+		BUG("-F cannot be negated");

The message "-F cannot be negated" looks as if it is pointing out a mistake by the end user, and does not mesh well with the real reason why this is BUG() and is not die().

I understand that this is BUG() not die() because options[] array tells this callback not to be called with unset by having the PARSE_OPT_NONEG bit there.

builtin/merge.c static int option_parse_message(const struct option *opt, · line 133

Quoted 11 lines

builtin/merge.c +11 -0

+	if (ctx->opt) {
+		arg = ctx->opt;
+		ctx->opt = NULL;
+	} else if (ctx->argc > 1) {
+		ctx->argc--;
+		arg = *++ctx->argv;
+	} else
+		return opterror(opt, "requires a value", 0);
+
+	if (buf->len)
+		strbuf_addch(buf, '\n');

Do we assume that buf, if it is not empty, is properly terminated with LF already? I am wondering if the real reason we do these two lines is to make sure we have a separating blank line between what is already there (if there already is something) and what we add, in which case the above would want to say

	if (buf->len) {
		strbuf_complete_line(buf);
		strbuf_addch(buf, '\n');
	}

instead.

builtin/merge.c static int option_parse_message(const struct option *opt, · line 138

Quoted 5 lines

builtin/merge.c +5 -0

+	if (ctx->prefix && !is_absolute_path(arg))
+		arg = prefix_filename(ctx->prefix, arg);
+	if (strbuf_read_file(buf, arg, 0) < 0)
+		return error(_("could not read file '%s'"), arg);
+	have_message = 1;

A similar question is what we would want to do when the file ends with an incomplete line. With "--log", we would be appending more stuff to buf, and we'd want to complete such an incomplete line before that happens, either here or in the code immediately before "--log" is processed.

builtin/merge.c static int option_parse_message(const struct option *opt, · line 145

Quoted 7 lines

builtin/merge.c +4 -0

+
+	return 0;
+}
+
 static struct strategy *get_strategy(const char *name)
 {
 	int i;

sequencer.c static int do_merge(struct commit *commit, const char *arg, int arg_len, · line 2967

Quoted 5 lines · Johannes Schindelin via GitGitGadget

sequencer.c +2 -1

@@ -2932,7 +2966,8 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
-                       strbuf_addf(&buf, "Merge branch '%.*s'",
+                       strbuf_addf(&buf, "Merge %s '%.*s'",
+                                   to_merge->next ? "branches" : "branch",

Error messages in this file are already localizable. If this text ever becomes localizable, then this "sentence lego" could be problematic for translators.

sequencer.c static int do_merge(struct commit *commit, const char *arg, int arg_len, · line 2994

Quoted 5 lines

sequencer.c +4 -0

@@ -2956,28 +2991,76 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
+               cmd.git_cmd = 1;
+               argv_array_push(&cmd.args, "merge");
+               argv_array_push(&cmd.args, "-s");
+               argv_array_push(&cmd.args, "octopus");

argv_array_pushl(&cmd.args, "-s", "octopus", NULL);

which would make it clear that these two arguments must remain together, and prevent someone from coming along and inserting a new argument between them.

sequencer.c static int do_merge(struct commit *commit, const char *arg, int arg_len, · line 3041

Quoted 6 lines

sequencer.c +6 -0

+               argv_array_push(&cmd.args, "--no-edit");
+               argv_array_push(&cmd.args, "--no-ff");
+               argv_array_push(&cmd.args, "--no-log");
+               argv_array_push(&cmd.args, "--no-stat");
+               argv_array_push(&cmd.args, "-F");
+               argv_array_push(&cmd.args, git_path_merge_msg());

Ditto:
argv_array_pushl(&cmd.args, "-L", git_path_merge_msg(), NULL);

t/t3430-rebase-merges.sh test_expect_success 'labels that are object IDs are rewritten' ' · line 339

Quoted 13 lines

t/t3430-rebase-merges.sh +11 -0

@@ -329,4 +329,38 @@ test_expect_success 'labels that are object IDs are rewritten' '
+test_expect_success 'octopus merges' '
+       git checkout -b three &&
+       test_commit before-octopus &&
+       test_commit three &&
+       git checkout -b two HEAD^ &&
+       test_commit two &&
+       git checkout -b one HEAD^ &&
+       test_commit one &&
+       test_tick &&
+       (GIT_AUTHOR_NAME="Hank" GIT_AUTHOR_EMAIL="[email protected]" \
+        git merge -m "Tüntenfüsch" two three) &&

Unnecessary subshell?

Quoted 8 lines · Eric Sunshine

sequencer.c static int do_merge(struct commit *commit, const char *arg, int arg_len, · line 2994

Quoted 5 lines

sequencer.c +4 -0

@@ -2956,28 +2991,76 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
+               cmd.git_cmd = 1;
+               argv_array_push(&cmd.args, "merge");
+               argv_array_push(&cmd.args, "-s");
+               argv_array_push(&cmd.args, "octopus");

argv_array_pushl(&cmd.args, "-s", "octopus", NULL);

Quoted 3 lines

which would make it clear that these two arguments must remain together, and prevent someone from coming along and inserting a new argument between them.

A valid point. It is OK to break "merge" and "-s octopus" into separate push invocations, but not "-s" and "octopus". Or perhaps push it as a single "--strategy=octopus" argument, which would be a better approach anyway.

Quoted 13 lines

builtin/merge.c static int option_parse_message(const struct option *opt, · line 121

Quoted 8 lines

builtin/merge.c +8 -0

+static int option_read_message(struct parse_opt_ctx_t *ctx,
+			       const struct option *opt, int unset)
+{
+	struct strbuf *buf = opt->value;
+	const char *arg;
+
+	if (unset)
+		BUG("-F cannot be negated");

The message "-F cannot be negated" looks as if it is pointing out a mistake by the end user, and does not mesh well with the real reason why this is BUG() and is not die().

Quoted 3 lines

I understand that this is BUG() not die() because options[] array tells this callback not to be called with unset by having the PARSE_OPT_NONEG bit there.

Okay. I would have appreciated some sort of indication what you prefer instead. I went with "--no-file?!?"

Quoted 18 lines

builtin/merge.c static int option_parse_message(const struct option *opt, · line 133

Quoted 11 lines

builtin/merge.c +11 -0

+	if (ctx->opt) {
+		arg = ctx->opt;
+		ctx->opt = NULL;
+	} else if (ctx->argc > 1) {
+		ctx->argc--;
+		arg = *++ctx->argv;
+	} else
+		return opterror(opt, "requires a value", 0);
+
+	if (buf->len)
+		strbuf_addch(buf, '\n');

Do we assume that buf, if it is not empty, is properly terminated with LF already? I am wondering if the real reason we do these two lines is to make sure we have a separating blank line between what is already there (if there already is something) and what we add, in which case the above would want to say

Quoted 6 lines

	if (buf->len) {
		strbuf_complete_line(buf);
		strbuf_addch(buf, '\n');
	}

instead.

True. Thanks for the suggestion!

Quoted 11 lines

builtin/merge.c static int option_parse_message(const struct option *opt, · line 138

Quoted 5 lines

builtin/merge.c +5 -0

+	if (ctx->prefix && !is_absolute_path(arg))
+		arg = prefix_filename(ctx->prefix, arg);
+	if (strbuf_read_file(buf, arg, 0) < 0)
+		return error(_("could not read file '%s'"), arg);
+	have_message = 1;

A similar question is what we would want to do when the file ends with an incomplete line. With "--log", we would be appending more stuff to buf, and we'd want to complete such an incomplete line before that happens, either here or in the code immediately before "--log" is processed.

This is what I inserted here:

	strbuf_complete_line(buf);

Git is used in *so many* different scenarios, and both in terms of commits/day as well as overall repository size *and* development speed,

A much more meaningful measure would be: how many octopus merge commits have been pushed to GitHub in the past two weeks. I don't think I have the technical means to answer that question, though.

Quoted 3 lines · Johannes Schindelin

A much more meaningful measure would be: how many octopus merge commits have been pushed to GitHub in the past two weeks. I don't think I have the technical means to answer that question, though.

It does not mean that misusing a feature is a good thing and should be encouraged if many misguided people do so.

Am 12.07.2018 um 18:26 schrieb Junio C Hamano:

Quoted 7 lines

Quoted 3 lines · Johannes Schindelin

A much more meaningful measure would be: how many octopus merge commits have been pushed to GitHub in the past two weeks. I don't think I have the technical means to answer that question, though.

It does not mean that misusing a feature is a good thing and should be encouraged if many misguided people do so.

Just recently I had to rebuild the version of git-gui that comes with Git 2.18.0 before it was released:

https://github.com/j6t/git-gui-ng/commit/f07ae1d7f07b036d78a3d4706e6cb4102e623fb3

I think that an octopus merge is the right tool for the task. Am I misguided?