# git merge branch --no-commit does commit fast forward merges 21 messages from 2016-04-17 to 2016-04-27. Participants: Christoph Paulik, Jacob Keller, Johannes Schindelin, Andrew Ardill, Junio C Hamano, Stefan Beller, Eric Sunshine, Johannes Sixt. Thread: https://gitlist.dev/t/42055 ## Christoph Paulik, 2016-04-17 21:10 Subject: git merge branch --no-commit does commit fast forward merges Message-ID: <874mb0kkkk.fsf@gmail.com> URL: https://gitlist.dev/e/874mb0kkkk.fsf%40gmail.com ``` Hi Git Mailinglist, git merge branch --no-commit does commit fast forward merges leaving users no way to change the merge results. The command only works as expected when also adding the --no-ff flag. Looking at the help text of the --no-commit flag I think that this might be a unintended. I've tested this on git 2.8.0. This bug was first reported to magit (https://github.com/magit/magit/issues/2620) whose maintainer then suggested that it might be a git bug. Best regards, Christoph -- ------------------------------------------------------- Christoph Paulik Twitter, Github: @cpaulik PGP: 8CFC D7DF 2867 B2DC 749B 1B0A 6E3B A262 5186 A0AC ``` ## Jacob Keller, 2016-04-17 23:52 Subject: Re: git merge branch --no-commit does commit fast forward merges Message-ID: URL: https://gitlist.dev/e/CA%2BP7%2Bxqpqc1Km842QF_NwXUzjFVTQCwmQU3L12EakDjzMTFk2w%40mail.gmail.com In-Reply-To: <874mb0kkkk.fsf@gmail.com> ``` On Sun, Apr 17, 2016 at 2:10 PM, Christoph Paulik wrote: > > Hi Git Mailinglist, > git merge branch --no-commit does commit fast forward merges leaving users > no way to change the merge results. The command only works as expected when > also adding the --no-ff flag. Looking at the help text of the --no-commit > flag I think that this might be a unintended. > I've tested this on git 2.8.0. > > This bug was first reported to magit > (https://github.com/magit/magit/issues/2620) whose maintainer then suggested > that it might be a git bug. > > Best regards, > Christoph > To be fair, the user can undo it with "git reset HEAD@{1}" I believe? Thanks, Jake ``` ## Johannes Schindelin, 2016-04-18 06:26 Subject: Re: git merge branch --no-commit does commit fast forward merges Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1604180825170.2967%40virtualbox In-Reply-To: <874mb0kkkk.fsf@gmail.com> ``` Hi Christoph, On Sun, 17 Apr 2016, Christoph Paulik wrote: > git merge branch --no-commit does commit fast forward merges leaving > users no way to change the merge results. No, this is not a bug. Please note that a fast-forward does not perform a commit at all (and therefore "does commit fast forward" is an inaccurate description). > The command only works as expected when also adding the --no-ff flag. Then you need to fix your expectations ;-) A fast-forward *avoids* committing altogether. In that light, the current behavior is correct. Ciao, Johannes ``` ## Andrew Ardill, 2016-04-18 07:09 Subject: Re: git merge branch --no-commit does commit fast forward merges Message-ID: URL: https://gitlist.dev/e/CAH5451kW3t1Y7oW%3DuHv85jzHwsnQcDK2jdLisauNF-x1LRwqLA%40mail.gmail.com In-Reply-To: ``` On 18 April 2016 at 16:26, Johannes Schindelin wrote: > > > The command only works as expected when also adding the --no-ff flag. > > Then you need to fix your expectations ;-) I *think* the core of this problem is that the intent of the end-user does not align with the command options available. In this use case (as far as I can tell), the user wants to see what the result of a merge from somewhere else will look like, without changing their HEAD. While you are correct in saying a fast-forward does not create any new commits, for the user it certainly looks like a whole slew of new commits have been added. Moreover, the nature of the option means that the user has to investigate if the merge is a fast-forward in order to know what the outcome of the command will be. If the merge is a fast-forward, --no-commit has no effect on the outcome. If the merge is not a fast-forward, --no-commit has a huge effect on the outcome. If I see a --no-commit option, as an inexperienced user, I would be quite surprised to find my HEAD changed after using it. It would be far more intuitive, for that user, for --no-commit to imply --no-ff however I suspect that such a change may well cause more problems then it fixes. What I wonder is, in what situation is the current behaviour is desirable? While I agree that the option works as designed, I think its behaviour is more surprising to the end user then it should be. Regards, Andrew Ardill ``` ## Christoph Paulik, 2016-04-18 07:23 Subject: Re: git merge branch --no-commit does commit fast forward merges Message-ID: <87a8krpehl.fsf@gmail.com> URL: https://gitlist.dev/e/87a8krpehl.fsf%40gmail.com In-Reply-To: ``` My expectations from what should happen came mainly from the description of the --no-commit flag in the help: With --no-commit perform the merge but pretend the merge failed and do not autocommit, to give the user a chance to inspect and further tweak the merge result before committing. So in the case of a fast-forward the flag does not pretend that the merge failed. Regards, Christoph Andrew Ardill writes: > On 18 April 2016 at 16:26, Johannes Schindelin > wrote: >> >> > The command only works as expected when also adding the >> > --no-ff flag. >> >> Then you need to fix your expectations ;-) > > I *think* the core of this problem is that the intent of the > end-user does not align with the command options available. > > In this use case (as far as I can tell), the user wants to see > what the result of a merge from somewhere else will look like, > without changing their HEAD. > > While you are correct in saying a fast-forward does not create > any new commits, for the user it certainly looks like a whole > slew of new commits have been added. Moreover, the nature of the > option means that the user has to investigate if the merge is a > fast-forward in order to know what the outcome of the command > will be. > > If the merge is a fast-forward, --no-commit has no effect on the > outcome. If the merge is not a fast-forward, --no-commit has a > huge effect on the outcome. > > If I see a --no-commit option, as an inexperienced user, I would > be quite surprised to find my HEAD changed after using it. It > would be far more intuitive, for that user, for --no-commit to > imply --no-ff however I suspect that such a change may well > cause more problems then it fixes. > > What I wonder is, in what situation is the current behaviour is > desirable? > > While I agree that the option works as designed, I think its > behaviour is more surprising to the end user then it should be. > > Regards, > > Andrew Ardill -- ------------------------------------------------------- Christoph Paulik Twitter, Github: @cpaulik PGP: 8CFC D7DF 2867 B2DC 749B 1B0A 6E3B A262 5186 A0AC ``` ## Andrew Ardill, 2016-04-18 07:44 Subject: Re: git merge branch --no-commit does commit fast forward merges Message-ID: URL: https://gitlist.dev/e/CAH5451mDYhavx_OLfXe6cC2WguCsFWEBBBBOCPyX3E6ZJw27%2Bw%40mail.gmail.com In-Reply-To: <87a8krpehl.fsf@gmail.com> ``` On 18 April 2016 at 17:23, Christoph Paulik wrote: > My expectations from what should happen came mainly from the description of > the --no-commit flag in the help: > > With --no-commit perform the merge but pretend the merge failed and do not > autocommit, to give the user a chance to inspect and further tweak the merge > result before committing. > So in the case of a fast-forward the flag does not pretend that the merge > failed. Yes, I think the mis-alignment in expectations comes from a technicality in the description you quote. The fast forward is in some ways not really counted as a true merge, and no new commits are created. Thus, the merge progresses up to the point where a merge resolution would have to take place, realises that there is no merge resolution to do (it's just a fast forward!) and so exits out. Unfortunately, a side effect of this is that the fast-forward has already happened and so you are left with something different from what was expected. I do think that the --no-commit option should imply --no-ff (as this would make the behaviour consistent for end-users). I don't know if this is something that would break scripts etc, but if so you could make it implied only if we detect a terminal or something like is done in other places. Regards, Andrew Ardill ``` ## Junio C Hamano, 2016-04-18 16:36 Subject: Re: git merge branch --no-commit does commit fast forward merges Message-ID: URL: https://gitlist.dev/e/xmqqy48a6fht.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Andrew Ardill writes: > Yes, I think the mis-alignment in expectations comes from a > technicality in the description you quote. The fast forward is in some > ways not really counted as a true merge, and no new commits are > created. Looking at 123ee3ca (Add --no-commit to git-merge/git-pull., 2005-11-01) and $gmane/10998 [*1*], it is clear that "--no-commit" was never meant as a "preview of what would happen". The original documentation update at 37465016 (Documentation: -merge and -pull: describe merge strategies., 2005-11-04) was not great, but was clarified at d8ae1d10 (Document the --no-commit flag better, 2005-11-04), and that version of text survives to this day. The real reason why "--no-commit" was added was because back then "git commit --amend" did not even exist; it appeared only at b4019f04 (git-commit --amend, 2006-03-02). What is (and was back then) the recommended way to see what changes merging the other branch brings in to your branch, then? There are at least three ways, all of which are better suited than "--no-commit". When you want to study and understand what changes other branch made since it forked from what you are working on, then $ git diff ...other_branch would give you the change as a single ball of wax [*2*]. If you want to see individual changes explained by their authors, you can also do $ git log -p ..other_branch Finally, if you want to see what the merge result would look like, you just do the merge. Advancing the HEAD by one commit and then going back once you are done is a cheap operation. If you want to avoid updating your branch for real, these days you can even do so on a detached HEAD, unlike old days back when there was not even "commit --amend". $ git checkout HEAD^0 $ git merge other_branch $ git diff ORIG_HEAD ;# what changed overall? $ git log -p ORIG_HEAD.. ;# inspect individual changes $ git checkout - ;# come back to the original branch > I do think that the --no-commit option should imply --no-ff (as this > would make the behaviour consistent for end-users). I don't know if > this is something that would break scripts etc, but if so you could > make it implied only if we detect a terminal or something like is done > in other places. If we were living in an ideal world where "git commit --amend" were already there in November 2005, we wouldn't have "merge --no-commit" or "pull --no-commit" in our system today, and in such a world, I would agree that "try to populate the working tree and the index with result of a hypothetical merge and stop without updating HEAD nor creating MERGE_HEAD, only to show what would happen if I merged" option could be a useful addition to these two commands. And we may choose to call such an option "--no-commit". I agree that such an option should probably imply "--no-ff". But we are not living in that world. Making "--no-commit" (which is not that "try to populate and show" command) imply "--no-ff" will break existing scripts. And unlike cosmetic things like "do we show in color", changing the behaviour of a command in a fundamental way based on term and istty() is a sure way to make commands harder to understand ("it works this way from the terminal, but it works differently in my script. what is going on?" is not a question we are better off not seeing on this list). Thanks. [Notes and References] *1* http://thread.gmane.org/gmane.comp.version-control.git/10998 *2* Notice the three dots; it is a short-hand for $ git diff ^$(git merge-base HEAD other_branch) other_branch ``` ## Junio C Hamano, 2016-04-18 16:54 Subject: Re: git merge branch --no-commit does commit fast forward merges Message-ID: URL: https://gitlist.dev/e/xmqqtwiy6end.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > Andrew Ardill writes: > >> I do think that the --no-commit option should imply --no-ff (as this >> would make the behaviour consistent for end-users). I don't know if >> this is something that would break scripts etc, but if so you could >> make it implied only if we detect a terminal or something like is done >> in other places. > > But we are not living in that world. Making "--no-commit" (which is > not that "try to populate and show" command) imply "--no-ff" will > break existing scripts.... Having said all that, there is one change we might want to consider, with a plan to transition to cope with backward incompatibility. A user who uses "--no-commit" does so with the intention to record a resulting merge after amending the merge result in the working tree. But there is nothing to amend and record, if the same "git merge" without "--no-commit" wouldn't have created a merge commit (there are two cases: (1) the other branch is a descendant of the current branch, (2) the other branch is an ancestor of the current branch). And the user would want to know that before doing further damange to his history, so we may want to start warn when "--no-commit" fast-forwarded or succeeded with "already up-to-date", with deprecation notice, and eventually want to make it an error. Those who want to do a scripted git merge --no-commit "$1" && autoedit "$1" && git commit (where the script takes a branch name $1 and uses auto-edit to further record the fact that a branch $1 was merged to somewhere in the contents) would already be buggy if it wants to force no-ff, and will get a warning (and later an error), with such a change. And fixing the script to add "--no-ff" next to "--no-commit" will also stop the new warning/error. ``` ## Junio C Hamano, 2016-04-26 21:32 Subject: [PATCH 1/2] merge: do not contaminate option_commit with --squash Message-ID: URL: https://gitlist.dev/e/xmqqk2jkdpjr.fsf_-_%40gitster.mtv.corp.google.com In-Reply-To: ``` It is true that we do not make a commit when we are asked to do "merge --squash", and the code does so by setting option_commit variable to zero when seeing the squash option. But this made it impossible to see from the value of option_commit if --no-commit was given from the command line, or --squash turned it off. We check for the value of option_commit at only two places. Check for the value of squash at them, too. Signed-off-by: Junio C Hamano --- * Just a preliminary clean-up for the next one which is on topic. builtin/merge.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index bf2f261..de9d1b6 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1237,11 +1237,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (verbosity < 0) show_diffstat = 0; - if (squash) { - if (fast_forward == FF_NO) - die(_("You cannot combine --squash with --no-ff.")); - option_commit = 0; - } + if (squash && fast_forward == FF_NO) + die(_("You cannot combine --squash with --no-ff.")); if (!argc) { if (default_to_upstream) @@ -1449,10 +1446,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * We are not doing octopus and not fast-forward. Need * a real merge. */ - else if (!remoteheads->next && !common->next && option_commit) { + else if (!remoteheads->next && !common->next && option_commit && !squash) /* * We are not doing octopus, not fast-forward, and have - * only one common. + * only one common. And we do want to create a new commit. */ refresh_cache(REFRESH_QUIET); if (allow_trivial && fast_forward != FF_ONLY) { @@ -1535,7 +1532,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) ret = try_merge_strategy(use_strategies[i]->name, common, remoteheads, head_commit, head_arg); - if (!option_commit && !ret) { + if ((!option_commit || squash) && !ret) { merge_was_ok = 1; /* * This is necessary here just to avoid writing -- 2.8.1-491-g88b9e4a ``` ## Junio C Hamano, 2016-04-26 21:37 Subject: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created Message-ID: URL: https://gitlist.dev/e/xmqqd1pcdpb8.fsf_-_%40gitster.mtv.corp.google.com In-Reply-To: ``` A user who uses "--no-commit" does so with the intention to record a resulting merge after amending the merge result in the working tree. But there is nothing to amend and record, if the same "git merge" without "--no-commit" wouldn't have created a merge commit (there are two cases: (1) the other branch is a descendant of the current branch, (2) the other branch is an ancestor of the current branch). The user would want to know that before doing further damange to his history. When "merge --no-commit" fast-forwarded or succeeded with "already up-to-date" or "fast-forward", give a warning message. We may want to turn this into a die() after a transition period. Signed-off-by: Junio C Hamano --- * The necessary update to avoid end-user mistake would look like this. I am not queuing this or further working on it myself, as I am not sure if it is all that useful. builtin/merge.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/builtin/merge.c b/builtin/merge.c index f641751..66c536d 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1157,6 +1157,15 @@ static struct commit_list *collect_parents(struct commit *head_commit, return remoteheads; } +static void no_commit_impossible(const char *message) +{ + if (!option_commit) { + warning("%s\n%s", _(message), + _("--no-commit is impossible")); + warning(_("In future versions of Git, this will become an error.")); + } +} + int cmd_merge(int argc, const char **argv, const char *prefix) { unsigned char result_tree[20]; @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * If head can reach all the merge then we are up to date. * but first the most common case of merging one remote. */ + no_commit_impossible(_("Already up-to-date")); finish_up_to_date("Already up-to-date."); goto done; } else if (fast_forward != FF_NO && !remoteheads->next && @@ -1412,6 +1422,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) struct strbuf msg = STRBUF_INIT; struct commit *commit; + no_commit_impossible(_("Fast-forward")); if (verbosity >= 0) { char from[GIT_SHA1_HEXSZ + 1], to[GIT_SHA1_HEXSZ + 1]; find_unique_abbrev_r(from, head_commit->object.oid.hash, @@ -1488,6 +1499,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) } } if (up_to_date) { + no_commit_impossible(_("Already up-to-date")); finish_up_to_date("Already up-to-date. Yeeah!"); goto done; } -- 2.8.1-491-g88b9e4a ``` ## Stefan Beller, 2016-04-26 21:53 Subject: Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created Message-ID: URL: https://gitlist.dev/e/CAGZ79ka-HbB%3DoCidTtKSa32R9kqd2_c-cG8h%2BttAVy%3Da8UfgFQ%40mail.gmail.com In-Reply-To: ``` On Tue, Apr 26, 2016 at 2:37 PM, Junio C Hamano wrote: > +static void no_commit_impossible(const char *message) > +{ > + if (!option_commit) { > + warning("%s\n%s", _(message), > + _("--no-commit is impossible")); > + warning(_("In future versions of Git, this will become an error.")); > + } > +} During discussion of the parallel process framework (sb/submodule-parallel-fetch~3), you seemed very inclined on not having major decisions made deep inside the helper function, but rather at the main function to easier see the program flow IIRC. This looks very similar to me as we'll have the no_commit_impossible function which is a helper of cmd_merge. Following your advice there, I would have expected to have static void no_commit_impossible(const char *message) { warning("%s\n%s", _(message), _("--no-commit is impossible")); warning(_("In future versions of Git, this will become an error.")); } and later if (!option_commit) no_commit_impossible(_("Already up-to-date")); > + > int cmd_merge(int argc, const char **argv, const char *prefix) > { > unsigned char result_tree[20]; > @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > * If head can reach all the merge then we are up to date. > * but first the most common case of merging one remote. > */ > + no_commit_impossible(_("Already up-to-date")); > finish_up_to_date("Already up-to-date."); Coming back to this patch, in case of -v given, we'll see ("Already up-to-date") twice? If --quiet is given, do we want to suppress output in no_commit_impossible? ``` ## Junio C Hamano, 2016-04-26 22:00 Subject: Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created Message-ID: URL: https://gitlist.dev/e/xmqq1t5sdo9j.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Stefan Beller writes: > and later > > if (!option_commit) > no_commit_impossible(_("Already up-to-date")); It would be more legible, but because there are so few callsites in an already shallow callchain, I do not think it makes that much of a difference in this codepath either way. >> + >> int cmd_merge(int argc, const char **argv, const char *prefix) >> { >> unsigned char result_tree[20]; >> @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) >> * If head can reach all the merge then we are up to date. >> * but first the most common case of merging one remote. >> */ >> + no_commit_impossible(_("Already up-to-date")); >> finish_up_to_date("Already up-to-date."); > > Coming back to this patch, in case of -v given, we'll > see ("Already up-to-date") twice? One that explains why --no-commit is impossible in warning, and the other is the final report of what happened, so yes. > If --quiet is given, do we want to suppress output > in no_commit_impossible? While we are using warning(), we probably do want to. When we switch to die() at a major version boundary, we don't. ``` ## Eric Sunshine, 2016-04-27 01:39 Subject: Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTo09ECiPrtzHAHrr1a15khcYkL8QQpe_AjX2tEhRum2A%40mail.gmail.com In-Reply-To: ``` On Tue, Apr 26, 2016 at 5:37 PM, Junio C Hamano wrote: > A user who uses "--no-commit" does so with the intention to record a > resulting merge after amending the merge result in the working tree. > But there is nothing to amend and record, if the same "git merge" > without "--no-commit" wouldn't have created a merge commit (there > are two cases: (1) the other branch is a descendant of the current > branch, (2) the other branch is an ancestor of the current branch). > > The user would want to know that before doing further damange to his s/damange/damage/ > history. When "merge --no-commit" fast-forwarded or succeeded with > "already up-to-date" or "fast-forward", give a warning message. > > We may want to turn this into a die() after a transition period. > > Signed-off-by: Junio C Hamano ``` ## Johannes Sixt, 2016-04-27 05:57 Subject: Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created Message-ID: <572054B9.5080601@kdbg.org> URL: https://gitlist.dev/e/572054B9.5080601%40kdbg.org In-Reply-To: ``` Am 26.04.2016 um 23:37 schrieb Junio C Hamano: > * The necessary update to avoid end-user mistake would look like > this. I am not queuing this or further working on it myself, > as I am not sure if it is all that useful. Whoever picks up this patch, be warned that the i18n coding should be corrected: > +static void no_commit_impossible(const char *message) > +{ > + if (!option_commit) { > + warning("%s\n%s", _(message), The i18n call around message is not required, because... > + _("--no-commit is impossible")); > + warning(_("In future versions of Git, this will become an error.")); > + } > +} > + > int cmd_merge(int argc, const char **argv, const char *prefix) > { > unsigned char result_tree[20]; > @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > * If head can reach all the merge then we are up to date. > * but first the most common case of merging one remote. > */ > + no_commit_impossible(_("Already up-to-date")); ... the call sites takes care of it. -- Hannes ``` ## Johannes Schindelin, 2016-04-27 06:46 Subject: Re: [PATCH 1/2] merge: do not contaminate option_commit with --squash Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1604270846200.2896%40virtualbox In-Reply-To: ``` Hi Junio, On Tue, 26 Apr 2016, Junio C Hamano wrote: > It is true that we do not make a commit when we are asked to do > "merge --squash", and the code does so by setting option_commit > variable to zero when seeing the squash option. But this made it > impossible to see from the value of option_commit if --no-commit was > given from the command line, or --squash turned it off. > > We check for the value of option_commit at only two places. Check > for the value of squash at them, too. > > Signed-off-by: Junio C Hamano > --- > > * Just a preliminary clean-up for the next one which is on topic. I think it would make for a nice cleanup anyways. Ciao, Dscho ``` ## Johannes Schindelin, 2016-04-27 06:50 Subject: Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1604270846430.2896%40virtualbox In-Reply-To: ``` Hi Junio, On Tue, 26 Apr 2016, Junio C Hamano wrote: > @@ -1157,6 +1157,15 @@ static struct commit_list *collect_parents(struct commit *head_commit, > return remoteheads; > } > > +static void no_commit_impossible(const char *message) > +{ > + if (!option_commit) { > + warning("%s\n%s", _(message), > + _("--no-commit is impossible")); > + warning(_("In future versions of Git, this will become an error.")); > + } > +} I think this would be a step forward in usability, and I agree that this is a great opportunity for the users who wish for this feature to get involved and drive this forward, based on your excellent initial version. I am not sure about this double negation "no_commit_impossible" (I only understood what you meant because I had read the commit message first, something I won't do when stumbling over this code later). Maybe something like `disallow_no_commit`? Ciao, Dscho ``` ## Junio C Hamano, 2016-04-27 15:13 Subject: Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created Message-ID: URL: https://gitlist.dev/e/xmqqinz3axvc.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: > I am not sure about this double negation "no_commit_impossible" (I only > understood what you meant because I had read the commit message first, > something I won't do when stumbling over this code later). > > Maybe something like `disallow_no_commit`? That would be the best name once we start dying in there. It might be still better, even while we merely warn but let it pass, than the double negative. Or it may not. I dunno. ``` ## Junio C Hamano, 2016-04-27 15:14 Subject: Re: [PATCH 1/2] merge: do not contaminate option_commit with --squash Message-ID: URL: https://gitlist.dev/e/xmqqeg9raxtb.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: >> * Just a preliminary clean-up for the next one which is on topic. > > I think it would make for a nice cleanup anyways. There is a missing open-brace that causes a compilation error, though ;-) ``` ## Johannes Schindelin, 2016-04-27 15:19 Subject: Re: [PATCH 1/2] merge: do not contaminate option_commit with --squash Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1604271718530.2896%40virtualbox In-Reply-To: ``` Hi Junio, On Wed, 27 Apr 2016, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> * Just a preliminary clean-up for the next one which is on topic. > > > > I think it would make for a nice cleanup anyways. > > There is a missing open-brace that causes a compilation error, > though ;-) Uh oh ;-) I guess I did not really test it... Ciao, Dscho ``` ## Johannes Schindelin, 2016-04-27 15:37 Subject: Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1604271732570.2896%40virtualbox In-Reply-To: ``` Hi Junio, On Wed, 27 Apr 2016, Junio C Hamano wrote: > Johannes Schindelin writes: > > > I am not sure about this double negation "no_commit_impossible" (I only > > understood what you meant because I had read the commit message first, > > something I won't do when stumbling over this code later). > > > > Maybe something like `disallow_no_commit`? > > That would be the best name once we start dying in there. It might > be still better, even while we merely warn but let it pass, than the > double negative. Or it may not. I dunno. Actually, I should admit that I was really puzzled by the name at first. I thought that some commits were impossible, but the function said that no commit was impossible. So I thought: but what if a commit references itself as parent, would that not be impossible? But actually, once SHA-1 collision attacks become feasible, I guess it would not be impossible. Making for an excellent attack vector, say, on repository hosting sites (which would now be stuck in infinite loops due to a violation of the temporal prime directive). So yeah, this was my thought process when I read no_commit_impossible. ;-) Ciao, Dscho ``` ## Junio C Hamano, 2016-04-27 16:02 Subject: Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created Message-ID: URL: https://gitlist.dev/e/xmqq37q7avlf.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: >> > Maybe something like `disallow_no_commit`? >> >> That would be the best name once we start dying in there. It might >> be still better, even while we merely warn but let it pass, than the >> double negative. Or it may not. I dunno. > > Actually, I should admit that I was really puzzled by the name at first. I > thought that some commits were impossible, but the function said that no > commit was impossible. So I thought: but what if a commit references > itself as parent, would that not be impossible? But actually, once SHA-1 > collision attacks become feasible, I guess it would not be impossible. > Making for an excellent attack vector, say, on repository hosting sites > (which would now be stuck in infinite loops due to a violation of the > temporal prime directive). > > So yeah, this was my thought process when I read no_commit_impossible. We both know that the original name was terrible now. It was meant to mean "it is impossible to honor --no-commit option", nothing more, and left the door open for deciding what should happen when that condition holds (either "warn but succeed the merge anyway" or "die to refuse"), but it is clear that the name did not convey that successfully. ```