threads / patch / 51162

v3builtin/merges: clarify --squash behavior with --commit

Subject: [PATCH v3] builtin/merges: clarify --squash behavior with --commit

## tl;dr

3 messages between May 23, 2019 and May 28, 2019. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Vishal Verma· May 23, 2019, 22:53 UTC · lore
From: Vishal Verma <vishal@stellar.sh>

Convert option_commit to tristate, representing the states of 'default/untouched', 'enabled-by-cli', 'disabled-by-cli'. With this in place, check whether option_commit was enabled by cli when squashing a merge. If so, error out, as this is not supported.

Previously, when --squash was supplied, 'option_commit' was silently dropped. This could have been surprising to a user who tried to override the no-commit behavior of squash using --commit explicitly.

Add a note to the --squash option for git-merge to clarify the incompatibility, and add a test case to t7600-merge.sh

Cc: Junio C Hamano <gitster@pobox.com>
Cc: Rafael Ascensão <rafa.almas@gmail.com>
Signed-off-by: Vishal Verma <vishal@stellar.sh>
---
v3: Add a test case for this behavior in t7600-merge.sh
 Documentation/merge-options.txt |  2 ++
 builtin/merge.c                 | 18 +++++++++++++++++-
 t/t7600-merge.sh                |  6 ++++++
 3 files changed, 25 insertions(+), 1 deletion(-)
Show changes to 3 files +25 −1

Documentation/merge-options.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 61876dbc33..79a00d2a4a 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -102,6 +102,8 @@ merge.
 +
 With --no-squash perform the merge and commit the result. This
 option can be used to override --squash.
++
+With --squash, --commit is not allowed, and will fail.
 
 -s <strategy>::
 --strategy=<strategy>::
diff --git a/builtin/merge.c b/builtin/merge.c
index e96f72af80..4730b075c1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -58,7 +58,7 @@ static const char * const builtin_merge_usage[] = {
 };
 
 static int show_diffstat = 1, shortlog_len = -1, squash;
-static int option_commit = 1;
+static int option_commit = -1;
 static int option_edit = -1;
 static int allow_trivial = 1, have_message, verify_signatures;
 static int overwrite_ignore = 1;
@@ -1336,9 +1336,25 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (verbosity < 0)
 		show_diffstat = 0;
 
+	/*
+	 * This indicates option_commit was influenced by the command line.
+	 * Check and error out for the squash case.
+	 */
+	if ((option_commit > 0) && squash)
+		die(_("You cannot combine --squash with --commit."));
+
+	/* If option_commit is the default '-1', we can 'enable' it */
+	if (option_commit < 0)
+		option_commit = 1;
+
 	if (squash) {
 		if (fast_forward == FF_NO)
 			die(_("You cannot combine --squash with --no-ff."));
+		/*
+		 * 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;
 	}
 
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 7f9c68cbe7..4ec5d9ec79 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -570,6 +570,12 @@ test_expect_success 'combining --squash and --no-ff is refused' '
 	test_must_fail git merge --no-ff --squash c1
 '
 
+test_expect_success 'combining --squash and --commit is refused' '
+	git reset --hard c0 &&
+	test_must_fail git merge --squash --commit c1 &&
+	test_must_fail git merge --commit --squash c1
+'
+
 test_expect_success 'option --ff-only overwrites --no-ff' '
 	git merge --no-ff --ff-only c1 &&
 	test_must_fail git merge --no-ff --ff-only c2
-- 
2.21.0
Vishal Verma· May 24, 2019, 18:36 UTC · re: Vishal Verma · lore

[PATCH v4] merge: refuse --commit with --squash

From: Vishal Verma <vishal@stellar.sh>

Convert option_commit to tristate, representing the states of 'default/untouched', 'enabled-by-cli', 'disabled-by-cli'. With this in place, check whether option_commit was enabled by cli when squashing a merge. If so, error out, as this is not supported.

Previously, when --squash was supplied, 'option_commit' was silently dropped. This could have been surprising to a user who tried to override the no-commit behavior of squash using --commit explicitly.

Add a note to the --squash option for git-merge to clarify the incompatibility, and add a test case to t7600-merge.sh

Cc: Junio C Hamano <gitster@pobox.com>
Cc: Rafael Ascensão <rafa.almas@gmail.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Vishal Verma <vishal@stellar.sh>
---
v4:
 - Change subject line to "merge: ". Also reword it slightly (Johannes)
 - Rework the logic slightly to group the deaths together (Rafael)
 Documentation/merge-options.txt |  2 ++
 builtin/merge.c                 | 12 +++++++++++-
 t/t7600-merge.sh                |  6 ++++++
 3 files changed, 19 insertions(+), 1 deletion(-)
Show changes to 3 files +19 −1

Documentation/merge-options.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 61876dbc33..79a00d2a4a 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -102,6 +102,8 @@ merge.
 +
 With --no-squash perform the merge and commit the result. This
 option can be used to override --squash.
++
+With --squash, --commit is not allowed, and will fail.
 
 -s <strategy>::
 --strategy=<strategy>::
diff --git a/builtin/merge.c b/builtin/merge.c
index e96f72af80..57c2a24f6d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -58,7 +58,7 @@ static const char * const builtin_merge_usage[] = {
 };
 
 static int show_diffstat = 1, shortlog_len = -1, squash;
-static int option_commit = 1;
+static int option_commit = -1;
 static int option_edit = -1;
 static int allow_trivial = 1, have_message, verify_signatures;
 static int overwrite_ignore = 1;
@@ -1339,9 +1339,19 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	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 (option_commit < 0)
+		option_commit = 1;
+
 	if (!argc) {
 		if (default_to_upstream)
 			argc = setup_with_upstream(&argv);
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 7f9c68cbe7..4ec5d9ec79 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -570,6 +570,12 @@ test_expect_success 'combining --squash and --no-ff is refused' '
 	test_must_fail git merge --no-ff --squash c1
 '
 
+test_expect_success 'combining --squash and --commit is refused' '
+	git reset --hard c0 &&
+	test_must_fail git merge --squash --commit c1 &&
+	test_must_fail git merge --commit --squash c1
+'
+
 test_expect_success 'option --ff-only overwrites --no-ff' '
 	git merge --no-ff --ff-only c1 &&
 	test_must_fail git merge --no-ff --ff-only c2
-- 
2.21.0
Junio C Hamano· May 28, 2019, 19:07 UTC · re: Vishal Verma · lore

Re: [PATCH v4] merge: refuse --commit with --squash

Vishal Verma <vishal@kernel.org> writes:
Show 19 quoted lines
> From: Vishal Verma <vishal@stellar.sh>
>
> Convert option_commit to tristate, representing the states of
> 'default/untouched', 'enabled-by-cli', 'disabled-by-cli'. With this in
> place, check whether option_commit was enabled by cli when squashing a
> merge. If so, error out, as this is not supported.
>
> Previously, when --squash was supplied, 'option_commit' was silently
> dropped. This could have been surprising to a user who tried to override
> the no-commit behavior of squash using --commit explicitly.
>
> Add a note to the --squash option for git-merge to clarify the
> incompatibility, and add a test case to t7600-merge.sh
>
> Cc: Junio C Hamano <gitster@pobox.com>
> Cc: Rafael Ascensão <rafa.almas@gmail.com>
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Vishal Verma <vishal@stellar.sh>
> ---

I personally feel that "merge --squash --commit" that does not complain and leaves the result in the working tree to be tweaked and then committed is perfectly fine, especially given that the reason why "--commit" option exists is primarily because we need to allow "--no-commit" to stop the normal merge from recording the result in a new commit.

A user who really wanted to record what a merge would bring to the current state in a single-parent commit would have to do "git commit" after that anyway, and with this patch, the user would instead need to run the same "git merge" command again, without "--commit", before being able to do so, so in the sense, it is likely that this change makes it more cumbersome to use for such a user to use the command.

But I guess that this change will give an incentive to actually allow the combination to "just work" by making it more obvious that the combination is not supported, so it would be a slight improvement in that sense ;-)

← back to recent threads