* [PATCH] Documentation/merge-options: clarify --squash behavior @ 2019-04-24 21:22 Vishal Verma 2019-04-25 4:16 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: Vishal Verma @ 2019-04-24 21:22 UTC (permalink / raw) To: git; +Cc: Rafael Ascensão, Vishal Verma Add a note to the --squash option for git-merge to clarify its behavior with respect to --commit. When --squash is supplied, 'option_commit' is silently dropped. This can be surprising to a user who tries to override the no-commit behavior of squash using --commit explicitly. Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- There may be an argument to make --commit 'just work' with squash, but that might involve changing option_commit from OPT_BOOL to something that can distinguish between the default, what's requested on the command line, or the --no- version. Documentation/merge-options.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt index 92a7d936c1..0fd97720d8 100644 --- a/Documentation/merge-options.txt +++ b/Documentation/merge-options.txt @@ -95,6 +95,9 @@ merge. + With --no-squash perform the merge and commit the result. This option can be used to override --squash. ++ +With --squash, a --commit option does not make sense, and will be ignored +(HEAD will not be moved in spite of --commit). -s <strategy>:: --strategy=<strategy>:: -- 2.20.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Documentation/merge-options: clarify --squash behavior 2019-04-24 21:22 [PATCH] Documentation/merge-options: clarify --squash behavior Vishal Verma @ 2019-04-25 4:16 ` Junio C Hamano 2019-04-25 16:26 ` Vishal Verma 0 siblings, 1 reply; 4+ messages in thread From: Junio C Hamano @ 2019-04-25 4:16 UTC (permalink / raw) To: Vishal Verma; +Cc: git, Rafael Ascensão Vishal Verma <vishal.l.verma@intel.com> writes: > Add a note to the --squash option for git-merge to clarify its behavior > with respect to --commit. When --squash is supplied, 'option_commit' is > silently dropped. This can be surprising to a user who tries to override > the no-commit behavior of squash using --commit explicitly. > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > > There may be an argument to make --commit 'just work' with squash, but > that might involve changing option_commit from OPT_BOOL to something > that can distinguish between the default, what's requested on the > command line, or the --no- version. I think it is bad to silently ignore the option. With or without this documentation update, I think it is sensible to update the code so that it errors out when "--squash --commit" are both given at the same time, just like when "--squash --no-ff" is given. Or make it "just work" as you said. Using a boolean variable as tristate is something we do in many places and it by itself is not a rocket science. You initialize the variable to -1 (unset), let parse_options() to set it to 0/1 when "--[no-]commit" is seen, and inspect after parse_options() finishes. If the variable is still -1, you know the user wants "the default" behaviour. The "default" behaviour you are proposing would probably be something like if (option_commit < 0) { /* * default to record the result in a commit. * but --squash traditionally does not. */ if (!squash) option_commit = 1; else option_commit = 0; } But I suspect that the option parsing part is the least difficult in the "make it just work" change. That is because I think that the machinery to record the result in a commit is not expecting to be asked to create a single-parent commit to record the result of the squashing, so there may be need for adjusting to how the result wants to be recorded before the code makes a commit. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Documentation/merge-options: clarify --squash behavior 2019-04-25 4:16 ` Junio C Hamano @ 2019-04-25 16:26 ` Vishal Verma 2019-04-26 21:17 ` [PATCH v2] builtin/merges: clarify --squash behavior with --commit vishal 0 siblings, 1 reply; 4+ messages in thread From: Vishal Verma @ 2019-04-25 16:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Rafael Ascensão On Thu, 2019-04-25 at 13:16 +0900, Junio C Hamano wrote: > > I think it is bad to silently ignore the option. With or without > this documentation update, I think it is sensible to update the code > so that it errors out when "--squash --commit" are both given at the > same time, just like when "--squash --no-ff" is given. Yes that makes sense. > Or make it "just work" as you said. Using a boolean variable as > tristate is something we do in many places and it by itself is not a > rocket science. You initialize the variable to -1 (unset), let > parse_options() to set it to 0/1 when "--[no-]commit" is seen, and > inspect after parse_options() finishes. If the variable is still > -1, you know the user wants "the default" behaviour. Ah I see - I was conflating OPT_BOOL with the parameter being a boolean as well without checking, but I see now that isn't the case. > > The "default" behaviour you are proposing would probably be > something like > > if (option_commit < 0) { > /* > * default to record the result in a commit. > * but --squash traditionally does not. > */ > if (!squash) > option_commit = 1; > else > option_commit = 0; > } > > But I suspect that the option parsing part is the least difficult in > the "make it just work" change. That is because I think that the > machinery to record the result in a commit is not expecting to be > asked to create a single-parent commit to record the result of the > squashing, so there may be need for adjusting to how the result > wants to be recorded before the code makes a commit. Yes I was going to try to allow the commit option as an experiment, and just see what happens. For now I'll send a v2 that has a doc update as well as prints a warning using the above technique. I'll dig more into what allowing --commit actually means (as time allows) - I'm definitely a newbie with git internals, and indeed this is my first posting here. Thanks for the feedback! -Vishal ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] builtin/merges: clarify --squash behavior with --commit 2019-04-25 16:26 ` Vishal Verma @ 2019-04-26 21:17 ` vishal 0 siblings, 0 replies; 4+ messages in thread From: vishal @ 2019-04-26 21:17 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. Add a note to the --squash option for git-merge to clarify the incompatibility. 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. Cc: Junio C Hamano <gitster@pobox.com> Cc: Rafael Ascensão <rafa.almas@gmail.com> Signed-off-by: Vishal Verma <vishal@stellar.sh> --- v2: - Error out when both --squash and --commit are supplied (Junio) - Adjust the documentation accordingly. Documentation/merge-options.txt | 2 ++ builtin/merge.c | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt index 92a7d936c1..263b194247 100644 --- a/Documentation/merge-options.txt +++ b/Documentation/merge-options.txt @@ -95,6 +95,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 5ce8946d39..98f268ac57 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -57,7 +57,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; @@ -1304,9 +1304,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; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-04-26 21:17 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-24 21:22 [PATCH] Documentation/merge-options: clarify --squash behavior Vishal Verma 2019-04-25 4:16 ` Junio C Hamano 2019-04-25 16:26 ` Vishal Verma 2019-04-26 21:17 ` [PATCH v2] builtin/merges: clarify --squash behavior with --commit vishal
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).