git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH v3] builtin/merges: clarify --squash behavior with --commit
@ 2019-05-23 22:53 Vishal Verma
  2019-05-24 18:36 ` [PATCH v4] merge: refuse --commit with --squash Vishal Verma
  0 siblings, 1 reply; 3+ messages in thread
From: Vishal Verma @ 2019-05-23 22:53 UTC (permalink / raw)
  To: git; +Cc: Vishal Verma, Junio C Hamano, Rafael Ascensão

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(-)

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH v4] merge: refuse --commit with --squash
  2019-05-23 22:53 [PATCH v3] builtin/merges: clarify --squash behavior with --commit Vishal Verma
@ 2019-05-24 18:36 ` Vishal Verma
  2019-05-28 19:07   ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Vishal Verma @ 2019-05-24 18:36 UTC (permalink / raw)
  To: git
  Cc: Vishal Verma, Junio C Hamano, Rafael Ascensão, Johannes Schindelin

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(-)

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v4] merge: refuse --commit with --squash
  2019-05-24 18:36 ` [PATCH v4] merge: refuse --commit with --squash Vishal Verma
@ 2019-05-28 19:07   ` Junio C Hamano
  0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2019-05-28 19:07 UTC (permalink / raw)
  To: Vishal Verma; +Cc: git, Vishal Verma, Rafael Ascensão, Johannes Schindelin

Vishal Verma <vishal@kernel.org> writes:

> 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 ;-)

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-05-28 19:07 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-23 22:53 [PATCH v3] builtin/merges: clarify --squash behavior with --commit Vishal Verma
2019-05-24 18:36 ` [PATCH v4] merge: refuse --commit with --squash Vishal Verma
2019-05-28 19:07   ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git