{"thread":{"id":"50986","subject":"[PATCH] Documentation/merge-options: clarify --squash behavior","startedAt":"2019-04-24T21:22:34Z","lastAt":"2019-04-26T21:17:09Z","messageCount":4,"participants":["Vishal Verma","Junio C Hamano","vishal@kernel.org"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"374426","messageId":"20190424212212.10039-1-vishal.l.verma@intel.com","threadId":"50986","inReplyTo":null,"subject":"[PATCH] Documentation/merge-options: clarify --squash behavior","fromName":"Vishal Verma","fromEmail":"vishal.l.verma@intel.com","sentAt":"2019-04-24T21:22:12Z","receivedAt":"2019-04-24T21:22:34Z","isPatch":true,"sender":{"key":"vishal.l.verma@intel.com","avatar":null},"body":"Add a note to the --squash option for git-merge to clarify its behavior\nwith respect to --commit. When --squash is supplied, 'option_commit' is\nsilently dropped. This can be surprising to a user who tries to override\nthe no-commit behavior of squash using --commit explicitly.\n\nSigned-off-by: Vishal Verma \n---\n\nThere may be an argument to make --commit 'just work' with squash, but\nthat might involve changing option_commit from OPT_BOOL to something\nthat can distinguish between the default, what's requested on the\ncommand line, or the --no- version.\n\n Documentation/merge-options.txt | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 92a7d936c1..0fd97720d8 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -95,6 +95,9 @@ 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, a --commit option does not make sense, and will be ignored\n+(HEAD will not be moved in spite of --commit).\n \n -s ::\n --strategy=::\n-- \n2.20.1\n\n"},{"id":"374453","messageId":"xmqqr29qyb1i.fsf@gitster-ct.c.googlers.com","threadId":"50986","inReplyTo":"20190424212212.10039-1-vishal.l.verma@intel.com","subject":"Re: [PATCH] Documentation/merge-options: clarify --squash behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-25T04:16:25Z","receivedAt":"2019-04-25T04:16:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vishal Verma writes:\n\n> Add a note to the --squash option for git-merge to clarify its behavior\n> with respect to --commit. When --squash is supplied, 'option_commit' is\n> silently dropped. This can be surprising to a user who tries to override\n> the no-commit behavior of squash using --commit explicitly.\n>\n> Signed-off-by: Vishal Verma \n> ---\n>\n> There may be an argument to make --commit 'just work' with squash, but\n> that might involve changing option_commit from OPT_BOOL to something\n> that can distinguish between the default, what's requested on the\n> command line, or the --no- version.\n\nI think it is bad to silently ignore the option. With or without\nthis documentation update, I think it is sensible to update the code\nso that it errors out when \"--squash --commit\" are both given at the\nsame time, just like when \"--squash --no-ff\" is given.\n\nOr make it \"just work\" as you said. Using a boolean variable as\ntristate is something we do in many places and it by itself is not a\nrocket science. You initialize the variable to -1 (unset), let\nparse_options() to set it to 0/1 when \"--[no-]commit\" is seen, and\ninspect after parse_options() finishes. If the variable is still\n-1, you know the user wants \"the default\" behaviour.\n\nThe \"default\" behaviour you are proposing would probably be\nsomething like\n\n if (option_commit < 0) {\n /* \n * default to record the result in a commit.\n * but --squash traditionally does not.\n */\n if (!squash)\n option_commit = 1;\n else\n option_commit = 0;\n }\n\nBut I suspect that the option parsing part is the least difficult in\nthe \"make it just work\" change. That is because I think that the\nmachinery to record the result in a commit is not expecting to be\nasked to create a single-parent commit to record the result of the\nsquashing, so there may be need for adjusting to how the result\nwants to be recorded before the code makes a commit.\n\n"},{"id":"374516","messageId":"dfc0a40233677ce0e1d9d073c41b6f0fea3bf746.camel@stellar.sh","threadId":"50986","inReplyTo":"xmqqr29qyb1i.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Documentation/merge-options: clarify --squash behavior","fromName":"Vishal Verma","fromEmail":"vishal@stellar.sh","sentAt":"2019-04-25T16:26:26Z","receivedAt":"2019-04-25T16:28:06Z","isPatch":true,"sender":{"key":"vishal@stellar.sh","avatar":null},"body":"\nOn Thu, 2019-04-25 at 13:16 +0900, Junio C Hamano wrote:\n> \n> I think it is bad to silently ignore the option. With or without\n> this documentation update, I think it is sensible to update the code\n> so that it errors out when \"--squash --commit\" are both given at the\n> same time, just like when \"--squash --no-ff\" is given.\n\nYes that makes sense.\n\n> Or make it \"just work\" as you said. Using a boolean variable as\n> tristate is something we do in many places and it by itself is not a\n> rocket science. You initialize the variable to -1 (unset), let\n> parse_options() to set it to 0/1 when \"--[no-]commit\" is seen, and\n> inspect after parse_options() finishes. If the variable is still\n> -1, you know the user wants \"the default\" behaviour.\n\nAh I see - I was conflating OPT_BOOL with the parameter being a boolean\nas well without checking, but I see now that isn't the case.\n\n> \n> The \"default\" behaviour you are proposing would probably be\n> something like\n> \n> if (option_commit < 0) {\n> /* \n> * default to record the result in a commit.\n> * but --squash traditionally does not.\n> */\n> if (!squash)\n> option_commit = 1;\n> else\n> option_commit = 0;\n> }\n> \n> But I suspect that the option parsing part is the least difficult in\n> the \"make it just work\" change. That is because I think that the\n> machinery to record the result in a commit is not expecting to be\n> asked to create a single-parent commit to record the result of the\n> squashing, so there may be need for adjusting to how the result\n> wants to be recorded before the code makes a commit.\n\nYes I was going to try to allow the commit option as an experiment, and\njust see what happens. For now I'll send a v2 that has a doc update as\nwell as prints a warning using the above technique.\n\nI'll dig more into what allowing --commit actually means (as time\nallows) - I'm definitely a newbie with git internals, and indeed this is\nmy first posting here.\n\nThanks for the feedback!\n\n\t-Vishal\n\n"},{"id":"374578","messageId":"20190426211703.25604-1-vishal@kernel.org","threadId":"50986","inReplyTo":"dfc0a40233677ce0e1d9d073c41b6f0fea3bf746.camel@stellar.sh","subject":"[PATCH v2] builtin/merges: clarify --squash behavior with --commit","fromName":"","fromEmail":"vishal@kernel.org","sentAt":"2019-04-26T21:17:03Z","receivedAt":"2019-04-26T21:17:09Z","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\nAdd a note to the --squash option for git-merge to clarify the\nincompatibility.\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\nCc: Junio C Hamano \nCc: Rafael Ascensão \nSigned-off-by: Vishal Verma \n---\n\nv2:\n - Error out when both --squash and --commit are supplied (Junio)\n - Adjust the documentation accordingly.\n\n Documentation/merge-options.txt | 2 ++\n builtin/merge.c | 18 +++++++++++++++++-\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 92a7d936c1..263b194247 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -95,6 +95,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 5ce8946d39..98f268ac57 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -57,7 +57,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@@ -1304,9 +1304,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 \n-- \n2.20.1\n\n"}]}