threads / patch / 48862

patch, 3 partsrebase -r: support octopus merges

Subject: [PATCH 0/3] rebase -r: support octopus merges

## tl;dr

20 messages between Dec 21, 2017 and Jul 16, 2018. Diffs are folded; open one to read it.

replies: 19people: 6as markdown or json

Johannes Schindelin via GitGitGadget· Jul 11, 2018, 12:38 UTC · lore
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
-- 
gitgitgadget
Johannes Schindelin via GitGitGadget· Dec 21, 2017, 14:52 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 2/3] rebase --rebase-merges: add support for octopus merges

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 sequencer.c              | 168 +++++++++++++++++++++++++++++----------
 t/t3430-rebase-merges.sh |  34 ++++++++
 2 files changed, 159 insertions(+), 43 deletions(-)
Show changes to 2 files +159 −43

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

diff --git a/sequencer.c b/sequencer.c
index 5354d4d51..bcc43699c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2841,6 +2841,26 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 	return ret;
 }
 
+static struct commit *lookup_label(const char *label, int len,
+				   struct strbuf *buf)
+{
+	struct commit *commit;
+
+	strbuf_reset(buf);
+	strbuf_addf(buf, "refs/rewritten/%.*s", len, label);
+	commit = lookup_commit_reference_by_name(buf->buf);
+	if (!commit) {
+		/* fall back to non-rewritten ref or commit */
+		strbuf_splice(buf, 0, strlen("refs/rewritten/"), "", 0);
+		commit = lookup_commit_reference_by_name(buf->buf);
+	}
+
+	if (!commit)
+		error(_("could not resolve '%s'"), buf->buf);
+
+	return commit;
+}
+
 static int do_merge(struct commit *commit, const char *arg, int arg_len,
 		    int flags, struct replay_opts *opts)
 {
@@ -2849,8 +2869,9 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
 	struct strbuf ref_name = STRBUF_INIT;
 	struct commit *head_commit, *merge_commit, *i;
 	struct commit_list *bases, *j, *reversed = NULL;
+	struct commit_list *to_merge = NULL, **tail = &to_merge;
 	struct merge_options o;
-	int merge_arg_len, oneline_offset, can_fast_forward, ret;
+	int merge_arg_len, oneline_offset, can_fast_forward, ret, k;
 	static struct lock_file lock;
 	const char *p;
 
@@ -2865,26 +2886,34 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
 		goto leave_merge;
 	}
 
-	oneline_offset = arg_len;
-	merge_arg_len = strcspn(arg, " \t\n");
-	p = arg + merge_arg_len;
-	p += strspn(p, " \t\n");
-	if (*p == '#' && (!p[1] || isspace(p[1]))) {
-		p += 1 + strspn(p + 1, " \t\n");
-		oneline_offset = p - arg;
-	} else if (p - arg < arg_len)
-		BUG("octopus merges are not supported yet: '%s'", p);
-
-	strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg);
-	merge_commit = lookup_commit_reference_by_name(ref_name.buf);
-	if (!merge_commit) {
-		/* fall back to non-rewritten ref or commit */
-		strbuf_splice(&ref_name, 0, strlen("refs/rewritten/"), "", 0);
-		merge_commit = lookup_commit_reference_by_name(ref_name.buf);
+	/*
+	 * For octopus merges, the arg starts with the list of revisions to be
+	 * merged. The list is optionally followed by '#' and the oneline.
+	 */
+	merge_arg_len = oneline_offset = arg_len;
+	for (p = arg; p - arg < arg_len; p += strspn(p, " \t\n")) {
+		if (!*p)
+			break;
+		if (*p == '#' && (!p[1] || isspace(p[1]))) {
+			p += 1 + strspn(p + 1, " \t\n");
+			oneline_offset = p - arg;
+			break;
+		}
+		k = strcspn(p, " \t\n");
+		if (!k)
+			continue;
+		merge_commit = lookup_label(p, k, &ref_name);
+		if (!merge_commit) {
+			ret = error(_("unable to parse '%.*s'"), k, p);
+			goto leave_merge;
+		}
+		tail = &commit_list_insert(merge_commit, tail)->next;
+		p += k;
+		merge_arg_len = p - arg;
 	}
 
-	if (!merge_commit) {
-		ret = error(_("could not resolve '%s'"), ref_name.buf);
+	if (!to_merge) {
+		ret = error(_("nothing to merge: '%.*s'"), arg_len, arg);
 		goto leave_merge;
 	}
 
@@ -2895,8 +2924,13 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
 		 * "[new root]", let's simply fast-forward to the merge head.
 		 */
 		rollback_lock_file(&lock);
-		ret = fast_forward_to(&merge_commit->object.oid,
-				       &head_commit->object.oid, 0, opts);
+		if (to_merge->next)
+			ret = error(_("octopus merge cannot be executed on "
+				      "top of a [new root]"));
+		else
+			ret = fast_forward_to(&to_merge->item->object.oid,
+					      &head_commit->object.oid, 0,
+					      opts);
 		goto leave_merge;
 	}
 
@@ -2932,7 +2966,8 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
 			p = arg + oneline_offset;
 			len = arg_len - oneline_offset;
 		} else {
-			strbuf_addf(&buf, "Merge branch '%.*s'",
+			strbuf_addf(&buf, "Merge %s '%.*s'",
+				    to_merge->next ? "branches" : "branch",
 				    merge_arg_len, arg);
 			p = buf.buf;
 			len = buf.len;
@@ -2956,28 +2991,76 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
 			&head_commit->object.oid);
 
 	/*
-	 * If the merge head is different from the original one, we cannot
+	 * If any merge head is different from the original one, we cannot
 	 * fast-forward.
 	 */
 	if (can_fast_forward) {
-		struct commit_list *second_parent = commit->parents->next;
+		struct commit_list *p = commit->parents->next;
 
-		if (second_parent && !second_parent->next &&
-		    oidcmp(&merge_commit->object.oid,
-			   &second_parent->item->object.oid))
+		for (j = to_merge; j && p; j = j->next, p = p->next)
+			if (oidcmp(&j->item->object.oid,
+				   &p->item->object.oid)) {
+				can_fast_forward = 0;
+				break;
+			}
+		/*
+		 * If the number of merge heads differs from the original merge
+		 * commit, we cannot fast-forward.
+		 */
+		if (j || p)
 			can_fast_forward = 0;
 	}
 
-	if (can_fast_forward && commit->parents->next &&
-	    !commit->parents->next->next &&
-	    !oidcmp(&commit->parents->next->item->object.oid,
-		    &merge_commit->object.oid)) {
+	if (can_fast_forward) {
 		rollback_lock_file(&lock);
 		ret = fast_forward_to(&commit->object.oid,
 				      &head_commit->object.oid, 0, opts);
 		goto leave_merge;
 	}
 
+	if (to_merge->next) {
+		/* Octopus merge */
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		if (read_env_script(&cmd.env_array)) {
+			const char *gpg_opt = gpg_sign_opt_quoted(opts);
+
+			ret = error(_(staged_changes_advice), gpg_opt, gpg_opt);
+			goto leave_merge;
+		}
+
+		cmd.git_cmd = 1;
+		argv_array_push(&cmd.args, "merge");
+		argv_array_push(&cmd.args, "-s");
+		argv_array_push(&cmd.args, "octopus");
+		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());
+		if (opts->gpg_sign)
+			argv_array_push(&cmd.args, opts->gpg_sign);
+
+		/* Add the tips to be merged */
+		for (j = to_merge; j; j = j->next)
+			argv_array_push(&cmd.args,
+					oid_to_hex(&j->item->object.oid));
+
+		strbuf_release(&ref_name);
+		unlink(git_path_cherry_pick_head());
+		rollback_lock_file(&lock);
+
+		rollback_lock_file(&lock);
+		ret = run_command(&cmd);
+
+		/* force re-reading of the cache */
+		if (!ret && (discard_cache() < 0 || read_cache() < 0))
+			ret = error(_("could not read index"));
+		goto leave_merge;
+	}
+
+	merge_commit = to_merge->item;
 	write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
 		      git_path_merge_head(), 0);
 	write_message("no-ff", 5, git_path_merge_mode(), 0);
@@ -3040,6 +3123,7 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
 leave_merge:
 	strbuf_release(&ref_name);
 	rollback_lock_file(&lock);
+	free_commit_list(to_merge);
 	return ret;
 }
 
@@ -3877,7 +3961,6 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 	 */
 	while ((commit = get_revision(revs))) {
 		struct commit_list *to_merge;
-		int is_octopus;
 		const char *p1, *p2;
 		struct object_id *oid;
 		int is_empty;
@@ -3909,11 +3992,6 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 			continue;
 		}
 
-		is_octopus = to_merge && to_merge->next;
-
-		if (is_octopus)
-			BUG("Octopus merges not yet supported");
-
 		/* Create a label */
 		strbuf_reset(&label);
 		if (skip_prefix(oneline.buf, "Merge ", &p1) &&
@@ -3935,13 +4013,17 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		strbuf_addf(&buf, "%s -C %s",
 			    cmd_merge, oid_to_hex(&commit->object.oid));
 
-		/* label the tip of merged branch */
-		oid = &to_merge->item->object.oid;
-		strbuf_addch(&buf, ' ');
+		/* label the tips of merged branches */
+		for (; to_merge; to_merge = to_merge->next) {
+			oid = &to_merge->item->object.oid;
+			strbuf_addch(&buf, ' ');
+
+			if (!oidset_contains(&interesting, oid)) {
+				strbuf_addstr(&buf, label_oid(oid, NULL,
+							      &state));
+				continue;
+			}
 
-		if (!oidset_contains(&interesting, oid))
-			strbuf_addstr(&buf, label_oid(oid, NULL, &state));
-		else {
 			tips_tail = &commit_list_insert(to_merge->item,
 							tips_tail)->next;
 
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 78f7c9958..9e6229727 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -329,4 +329,38 @@ test_expect_success 'labels that are object IDs are rewritten' '
 	! grep "^label $third$" .git/ORIGINAL-TODO
 '
 
+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="hank@sea.world" \
+	 git merge -m "Tüntenfüsch" two three) &&
+
+	: 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
+'
+
 test_done
-- 
gitgitgadget
Eric Sunshine· Jul 11, 2018, 18:49 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 2/3] rebase --rebase-merges: add support for octopus merges

On Wed, Jul 11, 2018 at 8:38 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 15 quoted lines
> 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 <johannes.schindelin@gmx.de>
A few minor notes below (not a proper review)...
Show 5 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> @@ -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.

Show 5 quoted lines
> @@ -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.

Show 6 quoted lines
> +               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);

Show 13 quoted lines
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> @@ -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="hank@sea.world" \
> +        git merge -m "Tüntenfüsch" two three) &&
Unnecessary subshell?
Show 21 quoted lines
> +       : 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
> +'
Junio C Hamano· Jul 11, 2018, 21:52 UTC · re: Eric Sunshine · lore

Re: [PATCH 2/3] rebase --rebase-merges: add support for octopus merges

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 9 quoted lines
>> diff --git a/sequencer.c b/sequencer.c
>> @@ -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.

I do not think we'd want to localize these default merge messages, though.

Show 11 quoted lines
>> @@ -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.

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.

>> +       (GIT_AUTHOR_NAME="Hank" GIT_AUTHOR_EMAIL="hank@sea.world" \
>> +        git merge -m "Tüntenfüsch" two three) &&
>
> Unnecessary subshell?
Right.
Johannes Schindelin· Jul 12, 2018, 13:11 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] rebase --rebase-merges: add support for octopus merges

Hi Junio,
On Wed, 11 Jul 2018, Junio C Hamano wrote:
Show 18 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >> @@ -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.
> 
> 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.

I had missed that in the documentation, as the synopsis does not mention it.

Thank you! Dscho

Johannes Schindelin· Jul 12, 2018, 13:08 UTC · re: Eric Sunshine · lore

Re: [PATCH 2/3] rebase --rebase-merges: add support for octopus merges

Hi Eric,
On Wed, 11 Jul 2018, Eric Sunshine wrote:
> On Wed, Jul 11, 2018 at 8:38 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
Show 9 quoted lines
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -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.

As pointed out by Junio, this is not really a message eligible for localization, at least not right now. If it becomes localized later, it will be very easy to change the sentence lego then.

I am just wary of making life harder for the compiler that wants to verify that the printf() style parameters match the format.

Show 11 quoted lines
> > @@ -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.

I use a single `argv_array_pushl()` call now, with intuitive line wrapping.

Show 25 quoted lines
> > +               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);
> 
> > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> > @@ -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="hank@sea.world" \
> > +        git merge -m "Tüntenfüsch" two three) &&
> 
> Unnecessary subshell?

Yes, I agree. I don't recall why I did it that way, probably I tried something else originally that required a subshell, or something.

Thank you for helping me make the patches better.

Ciao, Dscho

Johannes Schindelin via GitGitGadget· Dec 22, 2017, 14:10 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 1/3] merge: allow reading the merge commit message from a file

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 Documentation/git-merge.txt | 10 +++++++++-
 builtin/merge.c             | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)
Show changes to 2 files +41 −1

Documentation/git-merge.txt, builtin/merge.c

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 6a5c00e2c..537dca7fa 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -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.
diff --git a/builtin/merge.c b/builtin/merge.c
index 4a4c09496..b0e907751 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -111,6 +111,35 @@ static int option_parse_message(const struct option *opt,
 	return 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");
+
+	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');
+	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;
+
+	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")),
-- 
gitgitgadget
Junio C Hamano· Jul 11, 2018, 22:06 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 1/3] merge: allow reading the merge commit message from a file

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 59 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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 <johannes.schindelin@gmx.de>
> ---
>  Documentation/git-merge.txt | 10 +++++++++-
>  builtin/merge.c             | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 6a5c00e2c..537dca7fa 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -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.
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 4a4c09496..b0e907751 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -111,6 +111,35 @@ static int option_parse_message(const struct option *opt,
>  	return 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.

Show 11 quoted lines
> +	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.
Show 5 quoted lines
> +	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.

Show 17 quoted lines
> +
> +	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")),
Johannes Schindelin· Jul 12, 2018, 12:58 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] merge: allow reading the merge commit message from a file

Hi Junio,
On Wed, 11 Jul 2018, Junio C Hamano wrote:
Show 27 quoted lines
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > diff --git a/builtin/merge.c b/builtin/merge.c
> > index 4a4c09496..b0e907751 100644
> > --- a/builtin/merge.c
> > +++ b/builtin/merge.c
> > @@ -111,6 +111,35 @@ static int option_parse_message(const struct option *opt,
> >  	return 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.

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

Show 24 quoted lines
> > +	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.
True. Thanks for the suggestion!
Show 11 quoted lines
> > +	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);
Show 7 quoted lines
> > +
> > +	return 0;
> > +}
> > +
> >  static struct strategy *get_strategy(const char *name)
> >  {
> >  	int i;

Thanks for the review, and especially for the suggestions how to improve the code.

Ciao, Dscho

Junio C Hamano· Jul 12, 2018, 16:20 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/3] merge: allow reading the merge commit message from a file

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 19 quoted lines
>> > +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.
>
> Okay. I would have appreciated some sort of indication what you prefer
> instead. I went with "--no-file?!?"

I have no strong preference; anything is OK as long as the message is unique and points reading developer in the right direction, and "--no-file?!?" signals quite strongly that the code is not expected that it has to handle that option at this point (instead, it expects somebody else has dealt with it), so it sounds fine.

I think doing all of these inside parse_options callback means that you can have "merge -F file1 -F file2" and slurp contents from both files as separate paragraphs. I briefly wondered if --no-file is something the end user might want to be able to use to discard what has been read so far, but "merge -m msg -F file --no-file" would have to discard everything, not just what we read from the file, so it would not be useful with the structure of the message assembly we have today, which this code builds on.

Show 42 quoted lines
>> > +	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.
>
> True. Thanks for the suggestion!
>
>> > +	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);

I had a slight suspicion that completing immediately before we append anything in a later step in the codepath would be safer. When we get a complaint:

    'merge -F file' when I am not using '--log' or adding sign-off,
    adds an extra newline at the end when I deliberately give a file
    that ends with an incomplete line for such and such reasons.

I do not think I would have a good argument why the then-current behaviour is not a bug but an intended behaviour.

And I do not think the fact that I am unable to fill "such and such" above means such a complaint is nonsense---it merely indicates that I lack imagination and that I am not thinking enough to accomodate other people's needs.

Johannes Schindelin via GitGitGadget· Mar 9, 2018, 16:36 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 3/3] rebase --rebase-merges: adjust man page for octopus support

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 Documentation/git-rebase.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Show changes to Documentation/git-rebase.txt +4 −3
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 0e20a66e7..c4bcd24bb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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.
@@ -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.
 
 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
 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
Elijah Newren· Jul 11, 2018, 17:05 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 3/3] rebase --rebase-merges: adjust man page for octopus support

Hi Dscho,

On Fri, Mar 9, 2018 at 8:36 AM, Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 33 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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 <johannes.schindelin@gmx.de>
> ---
>  Documentation/git-rebase.txt | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 0e20a66e7..c4bcd24bb 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -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.
> @@ -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.
>
>  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..." ?

Show 5 quoted lines
>  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
Johannes Schindelin· Jul 12, 2018, 12:48 UTC · re: Elijah Newren · lore

Re: [PATCH 3/3] rebase --rebase-merges: adjust man page for octopus support

Hi Elijah,
On Wed, 11 Jul 2018, Elijah Newren wrote:
Show 18 quoted lines
> On Fri, Mar 9, 2018 at 8:36 AM, Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>
> > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> > index 0e20a66e7..c4bcd24bb 100644
> > --- a/Documentation/git-rebase.txt
> > +++ b/Documentation/git-rebase.txt
> > @@ -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.
> >
> >  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..."
> ?
Okay. Will be fixed in v2.

Ciao, Dscho

Junio C Hamano· Jul 11, 2018, 17:35 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 0/3] rebase -r: support octopus merges

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines
> 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.

Show 18 quoted lines
>
> 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
Stefan Beller· Jul 11, 2018, 17:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] rebase -r: support octopus merges

On Wed, Jul 11, 2018 at 10:35 AM Junio C Hamano <gitster@pobox.com> wrote:
> 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 <rafael.j.wysocki@intel.com> is really keen on these. :-)

Johannes Schindelin· Jul 12, 2018, 12:54 UTC · re: Stefan Beller · lore

Re: [PATCH 0/3] rebase -r: support octopus merges

Hi Stefan,
On Wed, 11 Jul 2018, Stefan Beller wrote:
Show 9 quoted lines
> On Wed, Jul 11, 2018 at 10:35 AM Junio C Hamano <gitster@pobox.com> wrote:
> > 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 <rafael.j.wysocki@intel.com> is really keen on these.
> :-)

IMO core Git contributors seriously need to forget about using the Linux kernel repository as the gold standard when looking how Git is used.

Git is used in *so many* different scenarios, and both in terms of commits/day as well as overall repository size *and* development speed, Linux is not even in the "smack down the middle" category. Compared to what is being done with Git on a daily basis, the Linux kernel repository (and project structure) is relatively small.

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.

In any case, the Git project is run in such a way that even having a feature used even by just single user whose name happens to be Andrew Morton declares that feature off-limits for deprecation.

When applying this context to `--rebase-merges` and Octopus merges, even a single user would be sufficient for us to support that feature. And I am sure that there are more than just a dozen users of this feature.

Ciao, Dscho

Junio C Hamano· Jul 12, 2018, 16:26 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/3] rebase -r: support octopus merges

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Git is used in *so many* different scenarios, and both in terms of
> commits/day as well as overall repository size *and* development speed,

That misses another important factor, though. How well the project uses the tool, iow, how canonical should its way of using the tool be considered and encouraged to be imitated by others.

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

Johannes Sixt· Jul 13, 2018, 16:42 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] rebase -r: support octopus merges

Am 12.07.2018 um 18:26 schrieb Junio C Hamano:
Show 7 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 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?

-- Hannes
Junio C Hamano· Jul 16, 2018, 17:56 UTC · re: Johannes Sixt · lore

Re: [PATCH 0/3] rebase -r: support octopus merges

Johannes Sixt <j6t@kdbg.org> writes:
Show 16 quoted lines
> Am 12.07.2018 um 18:26 schrieb Junio C Hamano:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>> 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?

It could be used and it is the right tool are two different things, I would think.

Johannes Schindelin· Jul 12, 2018, 12:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] rebase -r: support octopus merges

Hi Junio,
On Wed, 11 Jul 2018, Junio C Hamano wrote:
Show 21 quoted lines
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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.

--preserve-merges supports octopus merges, IIRC. And as I want to deprecate --preserve-merges, --rebase-merges *has* to support octopus merges, whether you or I would ever use them or not.

Ciao, Dscho

← back to recent threads