[PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts

[PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts

[Edmundo Carmona Antoranz]

Using --squash made git stop regardless of conflicts so that the
user could finish the operation with a later call to git-commit.

Now --squash allows for the operation to finish with the new revision
if there are no conflicts (can still be controlled with --no-commit).

Option -m can be used to defined the message for the revision instead
of the default message that contains all squashed revisions info.

Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
---
 builtin/merge.c | 109 +++++++++++++++++++++++++-----------------------
 1 file changed, 57 insertions(+), 52 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index aad5a9504c..66fd57de02 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -64,6 +64,7 @@ static int option_edit = -1;
 static int allow_trivial = 1, have_message, verify_signatures;
 static int overwrite_ignore = 1;
 static struct strbuf merge_msg = STRBUF_INIT;
+static struct strbuf squash_msg = STRBUF_INIT;
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
 static const char **xopts;
@@ -390,39 +391,38 @@ static void finish_up_to_date(const char *msg)
 static void squash_message(struct commit *commit, struct commit_list *remoteheads)
 {
 	struct rev_info rev;
-	struct strbuf out = STRBUF_INIT;
 	struct commit_list *j;
 	struct pretty_print_context ctx = {0};
 
-	printf(_("Squash commit -- not updating HEAD\n"));
-
-	repo_init_revisions(the_repository, &rev, NULL);
-	rev.ignore_merges = 1;
-	rev.commit_format = CMIT_FMT_MEDIUM;
-
-	commit->object.flags |= UNINTERESTING;
-	add_pending_object(&rev, &commit->object, NULL);
-
-	for (j = remoteheads; j; j = j->next)
-		add_pending_object(&rev, &j->item->object, NULL);
-
-	setup_revisions(0, NULL, &rev, NULL);
-	if (prepare_revision_walk(&rev))
-		die(_("revision walk setup failed"));
-
-	ctx.abbrev = rev.abbrev;
-	ctx.date_mode = rev.date_mode;
-	ctx.fmt = rev.commit_format;
-
-	strbuf_addstr(&out, "Squashed commit of the following:\n");
-	while ((commit = get_revision(&rev)) != NULL) {
-		strbuf_addch(&out, '\n');
-		strbuf_addf(&out, "commit %s\n",
-			oid_to_hex(&commit->object.oid));
-		pretty_print_commit(&ctx, commit, &out);
+	if (merge_msg.len)
+		squash_msg = merge_msg;
+	else {
+		repo_init_revisions(the_repository, &rev, NULL);
+		rev.ignore_merges = 1;
+		rev.commit_format = CMIT_FMT_MEDIUM;
+
+		commit->object.flags |= UNINTERESTING;
+		add_pending_object(&rev, &commit->object, NULL);
+
+		for (j = remoteheads; j; j = j->next)
+			add_pending_object(&rev, &j->item->object, NULL);
+
+		setup_revisions(0, NULL, &rev, NULL);
+		if (prepare_revision_walk(&rev))
+			die(_("revision walk setup failed"));
+
+		ctx.abbrev = rev.abbrev;
+		ctx.date_mode = rev.date_mode;
+		ctx.fmt = rev.commit_format;
+
+		strbuf_addstr(&squash_msg, "Squashed commit of the following:\n");
+		while ((commit = get_revision(&rev)) != NULL) {
+			strbuf_addch(&squash_msg, '\n');
+			strbuf_addf(&squash_msg, "commit %s\n",
+				oid_to_hex(&commit->object.oid));
+			pretty_print_commit(&ctx, commit, &squash_msg);
+		}
 	}
-	write_file_buf(git_path_squash_msg(the_repository), out.buf, out.len);
-	strbuf_release(&out);
 }
 
 static void finish(struct commit *head_commit,
@@ -440,8 +440,11 @@ static void finish(struct commit *head_commit,
 		strbuf_addf(&reflog_message, "%s: %s",
 			getenv("GIT_REFLOG_ACTION"), msg);
 	}
-	if (squash) {
+	if (squash && !squash_msg.len) {
 		squash_message(head_commit, remoteheads);
+		write_file_buf(git_path_squash_msg(the_repository), squash_msg.buf, squash_msg.len);
+		if (option_commit > 0)
+			printf(_("Squash conflicts -- not updating HEAD\n"));
 	} else {
 		if (verbosity >= 0 && !merge_msg.len)
 			printf(_("No merge message -- not updating HEAD\n"));
@@ -893,14 +896,23 @@ static int finish_automerge(struct commit *head,
 	struct object_id result_commit;
 
 	free_commit_list(common);
-	parents = remoteheads;
-	if (!head_subsumed || fast_forward == FF_NO)
-		commit_list_insert(head, &parents);
-	prepare_to_commit(remoteheads);
-	if (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents,
-			&result_commit, NULL, sign_commit))
-		die(_("failed to write commit object"));
-	strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
+	if (squash) {
+		squash_message(head, remoteheads);
+		parents = commit_list_insert(head, &parents);
+		if (commit_tree(squash_msg.buf, squash_msg.len, result_tree, parents,
+				&result_commit, NULL, sign_commit))
+			die(_("failed to write commit object on squash"));
+	} else {
+		parents = remoteheads;
+		if (!head_subsumed || fast_forward == FF_NO)
+			commit_list_insert(head, &parents);
+		prepare_to_commit(remoteheads);
+		if (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents,
+				&result_commit, NULL, sign_commit))
+			die(_("failed to write commit object"));
+	}
+	strbuf_addf(&buf, "Merge made by the '%s' strategy", wt_strategy);
+	strbuf_addstr(&buf, squash ? " (squashed)." : ".");
 	finish(head, remoteheads, &result_commit, buf.buf);
 	strbuf_release(&buf);
 	remove_merge_branch_state(the_repository);
@@ -1342,18 +1354,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (verbosity < 0)
 		show_diffstat = 0;
 
-	if (squash) {
-		if (fast_forward == FF_NO)
-			die(_("You cannot combine --squash with --no-ff."));
-		if (option_commit > 0)
-			die(_("You cannot combine --squash with --commit."));
-		/*
-		 * squash can now silently disable option_commit - this is not
-		 * a problem as it is only overriding the default, not a user
-		 * supplied option.
-		 */
-		option_commit = 0;
-	}
+	if (squash && fast_forward == FF_NO)
+		die(_("You cannot combine --squash with --no-ff."));
 
 	if (option_commit < 0)
 		option_commit = 1;
@@ -1682,8 +1684,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		write_merge_state(remoteheads);
 
 	if (merge_was_ok)
-		fprintf(stderr, _("Automatic merge went well; "
-			"stopped before committing as requested\n"));
+		if (!option_commit)
+			fprintf(stderr, _("Automatic merge went well; "
+				"stopped before committing as requested\n"));
+		else
+			;
 	else
 		ret = suggest_conflicts();
 
-- 
2.20.1

Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts

[Edmundo Carmona Antoranz]

On Fri, Jul 12, 2019 at 11:18 PM Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> @@ -1342,18 +1354,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>         if (verbosity < 0)
>                 show_diffstat = 0;
>
> -       if (squash) {
> -               if (fast_forward == FF_NO)
> -                       die(_("You cannot combine --squash with --no-ff."));
> -               if (option_commit > 0)
> -                       die(_("You cannot combine --squash with --commit."));
> -               /*
> -                * squash can now silently disable option_commit - this is not
> -                * a problem as it is only overriding the default, not a user
> -                * supplied option.
> -                */
> -               option_commit = 0;
> -       }
> +       if (squash && fast_forward == FF_NO)
> +               die(_("You cannot combine --squash with --no-ff."));
>
>         if (option_commit < 0)
>                 option_commit = 1;

One question that I have is if it makes sense to set option_commit to
0 if the user didn't specify --commit when using --squash, so that the
current behavior of git is not broken. Like you run merge --squash,
git will stop as it currently does... but it would be possible to run
with --squash --commit so that the revision is created if there are no
issues to take care of (currently impossible, you would see that
message saying "You cannot combine --squash with --commit.").

Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts

[Edmundo Carmona Antoranz]

On Fri, Jul 12, 2019 at 11:18 PM Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
>
> Option -m can be used to defined the message for the revision instead
> of the default message that contains all squashed revisions info.
>

I have noticed that just adding the support for -m in squash is more
complex than this patch is reaching so I think I will break this patch
into two parts:
- squash in a shot if there are no conflicts
- support -m with squash
Disregard this patch, please.

Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts

[Junio C Hamano]

Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:

> One question that I have is if it makes sense to set option_commit
> to 0 if the user didn't specify --commit when using --squash, so
> that the current behavior of git is not broken.  

If you mean that "git merge --squash <other args but not
--[no-]commit>" should behave identically with or without your
patch, then I think the answer is definitely yes.

> Like you run merge --squash, git will stop as it currently
> does... but it would be possible to run with --squash --commit so
> that the revision is created if there are no issues to take care
> of (currently impossible, you would see that message saying "You
> cannot combine --squash with --commit.").

That is exactly a safe way to extend the system by adding a new mode
of operation in a backward compatible fashion.  Good thinking.

Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts

[Junio C Hamano]

Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:

> On Fri, Jul 12, 2019 at 11:18 PM Edmundo Carmona Antoranz
> <eantoranz@gmail.com> wrote:
>>
>> Option -m can be used to defined the message for the revision instead
>> of the default message that contains all squashed revisions info.
>>
>
> I have noticed that just adding the support for -m in squash is more
> complex than this patch is reaching so I think I will break this patch
> into two parts:
> - squash in a shot if there are no conflicts
> - support -m with squash
> Disregard this patch, please.

Sure.  I started skimming and then gave up after seeing that quite a
lot of code has been shuffled around without much explanation (e.g.
printing of "Squash commit -- not updating HEAD" is gone from the
callee and now it is a responsibility of the caller), making it
harder than necessary to see if there is any unintended behaviour
change when the new feature is not in use.  Whatever you are trying,
it does look like the change deserves to be split into a smaller
pieces to become more manageable.

Thanks.

Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts

[Edmundo Carmona Antoranz]

On Wed, Jul 17, 2019 at 12:07 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Sure.  I started skimming and then gave up after seeing that quite a
> lot of code has been shuffled around without much explanation (e.g.
> printing of "Squash commit -- not updating HEAD" is gone from the
> callee and now it is a responsibility of the caller), making it
> harder than necessary to see if there is any unintended behaviour
> change when the new feature is not in use.  Whatever you are trying,
> it does look like the change deserves to be split into a smaller
> pieces to become more manageable.
>
> Thanks.
>

yw!

I'm focusing on the squash --commit part only. I think I'm close to
getting the desired result and now I'm taking a close look at the unit
tests and a question came up on two tests of t7600-merge.sh:

merge c0 with c1 (squash)
merge c0 with c1 (squash, ff-only)

In both cases it's a FF (right?) so no new revision is created. The
unit tests are requiring that $GIT_DIR/squash_msg have some content:

not ok 20 - merge c0 with c1 (squash, ff-only)
#
#               git reset --hard c0 &&
#               git merge --squash --ff-only c1 &&
#               verify_merge file result.1 &&
#               verify_head $c0 &&
#               verify_no_mergehead &&
#               test_cmp squash.1 .git/SQUASH_MSG


not ok 18 - merge c0 with c1 (squash)
#
#               git reset --hard c0 &&
#               git merge --squash c1 &&
#               verify_merge file result.1 &&
#               verify_head $c0 &&
#               verify_no_mergehead &&
#               test_cmp squash.1 .git/SQUASH_MSG


Does it make sense to keep this file in those two situations?

Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts

[Edmundo Carmona Antoranz]

On Wed, Jul 17, 2019 at 6:41 PM Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
>
>
> Does it make sense to keep this file in those two situations?

yes it does. disregard the question.