{"thread":{"id":"51162","subject":"[PATCH v3] builtin/merges: clarify --squash behavior with --commit","startedAt":"2019-05-23T22:54:10Z","lastAt":"2019-05-28T19:07:37Z","messageCount":3,"participants":["Vishal Verma","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"376135","messageId":"20190523225338.14619-1-vishal@kernel.org","threadId":"51162","inReplyTo":null,"subject":"[PATCH v3] builtin/merges: clarify --squash behavior with --commit","fromName":"Vishal Verma","fromEmail":"vishal@kernel.org","sentAt":"2019-05-23T22:53:38Z","receivedAt":"2019-05-23T22:54:10Z","isPatch":true,"sender":{"key":"vishal@kernel.org","avatar":null},"body":"From: Vishal Verma \n\nConvert option_commit to tristate, representing the states of\n'default/untouched', 'enabled-by-cli', 'disabled-by-cli'. With this in\nplace, check whether option_commit was enabled by cli when squashing a\nmerge. If so, error out, as this is not supported.\n\nPreviously, when --squash was supplied, 'option_commit' was silently\ndropped. This could have been surprising to a user who tried to override\nthe no-commit behavior of squash using --commit explicitly.\n\nAdd a note to the --squash option for git-merge to clarify the\nincompatibility, and add a test case to t7600-merge.sh\n\nCc: Junio C Hamano \nCc: Rafael Ascensão \nSigned-off-by: Vishal Verma \n---\n\nv3: Add a test case for this behavior in t7600-merge.sh\n\n Documentation/merge-options.txt | 2 ++\n builtin/merge.c | 18 +++++++++++++++++-\n t/t7600-merge.sh | 6 ++++++\n 3 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 61876dbc33..79a00d2a4a 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -102,6 +102,8 @@ merge.\n +\n With --no-squash perform the merge and commit the result. This\n option can be used to override --squash.\n++\n+With --squash, --commit is not allowed, and will fail.\n \n -s ::\n --strategy=::\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex e96f72af80..4730b075c1 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -58,7 +58,7 @@ static const char * const builtin_merge_usage[] = {\n };\n \n static int show_diffstat = 1, shortlog_len = -1, squash;\n-static int option_commit = 1;\n+static int option_commit = -1;\n static int option_edit = -1;\n static int allow_trivial = 1, have_message, verify_signatures;\n static int overwrite_ignore = 1;\n@@ -1336,9 +1336,25 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (verbosity < 0)\n \t\tshow_diffstat = 0;\n \n+\t/*\n+\t * This indicates option_commit was influenced by the command line.\n+\t * Check and error out for the squash case.\n+\t */\n+\tif ((option_commit > 0) && squash)\n+\t\tdie(_(\"You cannot combine --squash with --commit.\"));\n+\n+\t/* If option_commit is the default '-1', we can 'enable' it */\n+\tif (option_commit < 0)\n+\t\toption_commit = 1;\n+\n \tif (squash) {\n \t\tif (fast_forward == FF_NO)\n \t\t\tdie(_(\"You cannot combine --squash with --no-ff.\"));\n+\t\t/*\n+\t\t * squash can now silently disable option_commit - this is not\n+\t\t * a problem as it is only overriding the default, not a user\n+\t\t * supplied option.\n+\t\t */\n \t\toption_commit = 0;\n \t}\n \ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 7f9c68cbe7..4ec5d9ec79 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -570,6 +570,12 @@ test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --no-ff --squash c1\n '\n \n+test_expect_success 'combining --squash and --commit is refused' '\n+\tgit reset --hard c0 &&\n+\ttest_must_fail git merge --squash --commit c1 &&\n+\ttest_must_fail git merge --commit --squash c1\n+'\n+\n test_expect_success 'option --ff-only overwrites --no-ff' '\n \tgit merge --no-ff --ff-only c1 &&\n \ttest_must_fail git merge --no-ff --ff-only c2\n-- \n2.21.0\n\n"},{"id":"376198","messageId":"20190524183617.8291-1-vishal@kernel.org","threadId":"51162","inReplyTo":"20190523225338.14619-1-vishal@kernel.org","subject":"[PATCH v4] merge: refuse --commit with --squash","fromName":"Vishal Verma","fromEmail":"vishal@kernel.org","sentAt":"2019-05-24T18:36:17Z","receivedAt":"2019-05-24T18:36:29Z","isPatch":true,"sender":{"key":"vishal@kernel.org","avatar":null},"body":"From: Vishal Verma \n\nConvert option_commit to tristate, representing the states of\n'default/untouched', 'enabled-by-cli', 'disabled-by-cli'. With this in\nplace, check whether option_commit was enabled by cli when squashing a\nmerge. If so, error out, as this is not supported.\n\nPreviously, when --squash was supplied, 'option_commit' was silently\ndropped. This could have been surprising to a user who tried to override\nthe no-commit behavior of squash using --commit explicitly.\n\nAdd a note to the --squash option for git-merge to clarify the\nincompatibility, and add a test case to t7600-merge.sh\n\nCc: Junio C Hamano \nCc: Rafael Ascensão \nCc: Johannes Schindelin \nSigned-off-by: Vishal Verma \n---\n\nv4:\n - Change subject line to \"merge: \". Also reword it slightly (Johannes)\n - Rework the logic slightly to group the deaths together (Rafael)\n\n Documentation/merge-options.txt | 2 ++\n builtin/merge.c | 12 +++++++++++-\n t/t7600-merge.sh | 6 ++++++\n 3 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 61876dbc33..79a00d2a4a 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -102,6 +102,8 @@ merge.\n +\n With --no-squash perform the merge and commit the result. This\n option can be used to override --squash.\n++\n+With --squash, --commit is not allowed, and will fail.\n \n -s ::\n --strategy=::\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex e96f72af80..57c2a24f6d 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -58,7 +58,7 @@ static const char * const builtin_merge_usage[] = {\n };\n \n static int show_diffstat = 1, shortlog_len = -1, squash;\n-static int option_commit = 1;\n+static int option_commit = -1;\n static int option_edit = -1;\n static int allow_trivial = 1, have_message, verify_signatures;\n static int overwrite_ignore = 1;\n@@ -1339,9 +1339,19 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (squash) {\n \t\tif (fast_forward == FF_NO)\n \t\t\tdie(_(\"You cannot combine --squash with --no-ff.\"));\n+\t\tif (option_commit > 0)\n+\t\t\tdie(_(\"You cannot combine --squash with --commit.\"));\n+\t\t/*\n+\t\t * squash can now silently disable option_commit - this is not\n+\t\t * a problem as it is only overriding the default, not a user\n+\t\t * supplied option.\n+\t\t */\n \t\toption_commit = 0;\n \t}\n \n+\tif (option_commit < 0)\n+\t\toption_commit = 1;\n+\n \tif (!argc) {\n \t\tif (default_to_upstream)\n \t\t\targc = setup_with_upstream(&argv);\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 7f9c68cbe7..4ec5d9ec79 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -570,6 +570,12 @@ test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --no-ff --squash c1\n '\n \n+test_expect_success 'combining --squash and --commit is refused' '\n+\tgit reset --hard c0 &&\n+\ttest_must_fail git merge --squash --commit c1 &&\n+\ttest_must_fail git merge --commit --squash c1\n+'\n+\n test_expect_success 'option --ff-only overwrites --no-ff' '\n \tgit merge --no-ff --ff-only c1 &&\n \ttest_must_fail git merge --no-ff --ff-only c2\n-- \n2.21.0\n\n"},{"id":"376340","messageId":"xmqq1s0i2ya6.fsf@gitster-ct.c.googlers.com","threadId":"51162","inReplyTo":"20190524183617.8291-1-vishal@kernel.org","subject":"Re: [PATCH v4] merge: refuse --commit with --squash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-28T19:07:29Z","receivedAt":"2019-05-28T19:07:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vishal Verma writes:\n\n> From: Vishal Verma \n>\n> Convert option_commit to tristate, representing the states of\n> 'default/untouched', 'enabled-by-cli', 'disabled-by-cli'. With this in\n> place, check whether option_commit was enabled by cli when squashing a\n> merge. If so, error out, as this is not supported.\n>\n> Previously, when --squash was supplied, 'option_commit' was silently\n> dropped. This could have been surprising to a user who tried to override\n> the no-commit behavior of squash using --commit explicitly.\n>\n> Add a note to the --squash option for git-merge to clarify the\n> incompatibility, and add a test case to t7600-merge.sh\n>\n> Cc: Junio C Hamano \n> Cc: Rafael Ascensão \n> Cc: Johannes Schindelin \n> Signed-off-by: Vishal Verma \n> ---\n\nI personally feel that \"merge --squash --commit\" that does not\ncomplain and leaves the result in the working tree to be tweaked and\nthen committed is perfectly fine, especially given that the reason\nwhy \"--commit\" option exists is primarily because we need to allow\n\"--no-commit\" to stop the normal merge from recording the result in\na new commit.\n\nA user who really wanted to record what a merge would bring to the\ncurrent state in a single-parent commit would have to do \"git\ncommit\" after that anyway, and with this patch, the user would\ninstead need to run the same \"git merge\" command again, without\n\"--commit\", before being able to do so, so in the sense, it is\nlikely that this change makes it more cumbersome to use for such a\nuser to use the command. \n\nBut I guess that this change will give an incentive to actually\nallow the combination to \"just work\" by making it more obvious that\nthe combination is not supported, so it would be a slight\nimprovement in that sense ;-)\n"}]}