{"thread":{"id":"42055","subject":"git merge branch --no-commit does commit fast forward merges","startedAt":"2016-04-17T21:10:51Z","lastAt":"2016-04-27T16:02:20Z","messageCount":21,"participants":["Christoph Paulik","Jacob Keller","Johannes Schindelin","Andrew Ardill","Junio C Hamano","Stefan Beller","Eric Sunshine","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"283699","messageId":"874mb0kkkk.fsf@gmail.com","threadId":"42055","inReplyTo":null,"subject":"git merge branch --no-commit does commit fast forward merges","fromName":"Christoph Paulik","fromEmail":"cpaulik@gmail.com","sentAt":"2016-04-17T21:10:51Z","receivedAt":"2016-04-17T21:10:51Z","isPatch":false,"sender":{"key":"cpaulik@gmail.com","avatar":null},"body":"\nHi Git Mailinglist, \n\ngit merge branch --no-commit does commit fast forward merges \nleaving users no way to change the merge results. The command only \nworks as expected when also adding the --no-ff flag. Looking at \nthe help text of the --no-commit flag I think that this might be a \nunintended. \n\nI've tested this on git 2.8.0.\n\nThis bug was first reported to magit \n(https://github.com/magit/magit/issues/2620) whose maintainer then \nsuggested that it might be a git bug.\n\nBest regards,\nChristoph\n\n\n-- \n\n-------------------------------------------------------\nChristoph Paulik\nTwitter, Github: @cpaulik\nPGP: 8CFC D7DF 2867 B2DC 749B 1B0A 6E3B A262 5186 A0AC\n"},{"id":"283708","messageId":"CA+P7+xqpqc1Km842QF_NwXUzjFVTQCwmQU3L12EakDjzMTFk2w@mail.gmail.com","threadId":"42055","inReplyTo":"874mb0kkkk.fsf@gmail.com","subject":"Re: git merge branch --no-commit does commit fast forward merges","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-17T23:52:58Z","receivedAt":"2016-04-17T23:52:58Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sun, Apr 17, 2016 at 2:10 PM, Christoph Paulik wrote:\n>\n> Hi Git Mailinglist,\n> git merge branch --no-commit does commit fast forward merges leaving users\n> no way to change the merge results. The command only works as expected when\n> also adding the --no-ff flag. Looking at the help text of the --no-commit\n> flag I think that this might be a unintended.\n> I've tested this on git 2.8.0.\n>\n> This bug was first reported to magit\n> (https://github.com/magit/magit/issues/2620) whose maintainer then suggested\n> that it might be a git bug.\n>\n> Best regards,\n> Christoph\n>\n\n\nTo be fair, the user can undo it with \"git reset HEAD@{1}\" I believe?\n\nThanks,\nJake\n"},{"id":"283720","messageId":"alpine.DEB.2.20.1604180825170.2967@virtualbox","threadId":"42055","inReplyTo":"874mb0kkkk.fsf@gmail.com","subject":"Re: git merge branch --no-commit does commit fast forward merges","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-18T06:26:57Z","receivedAt":"2016-04-18T06:26:57Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christoph,\n\nOn Sun, 17 Apr 2016, Christoph Paulik wrote:\n\n> git merge branch --no-commit does commit fast forward merges leaving\n> users no way to change the merge results.\n\nNo, this is not a bug. Please note that a fast-forward does not perform a\ncommit at all (and therefore \"does commit fast forward\" is an inaccurate\ndescription).\n\n> The command only works as expected when also adding the --no-ff flag.\n\nThen you need to fix your expectations ;-)\n\nA fast-forward *avoids* committing altogether. In that light, the current\nbehavior is correct.\n\nCiao,\nJohannes\n"},{"id":"283721","messageId":"CAH5451kW3t1Y7oW=uHv85jzHwsnQcDK2jdLisauNF-x1LRwqLA@mail.gmail.com","threadId":"42055","inReplyTo":"alpine.DEB.2.20.1604180825170.2967@virtualbox","subject":"Re: git merge branch --no-commit does commit fast forward merges","fromName":"Andrew Ardill","fromEmail":"andrew.ardill@gmail.com","sentAt":"2016-04-18T07:09:18Z","receivedAt":"2016-04-18T07:09:18Z","isPatch":false,"sender":{"key":"andrew.ardill@gmail.com","avatar":"https://gravatar.com/avatar/da14cb7c091dd44dc6c63a4d3361b149acaf25226dc78eb4131a17b93d9b0993?d=mp&s=160"},"body":"On 18 April 2016 at 16:26, Johannes Schindelin\n wrote:\n>\n> > The command only works as expected when also adding the --no-ff flag.\n>\n> Then you need to fix your expectations ;-)\n\nI *think* the core of this problem is that the intent of the end-user\ndoes not align with the command options available.\n\nIn this use case (as far as I can tell), the user wants to see what\nthe result of a merge from somewhere else will look like, without\nchanging their HEAD.\n\nWhile you are correct in saying a fast-forward does not create any new\ncommits, for the user it certainly looks like a whole slew of new\ncommits have been added. Moreover, the nature of the option means that\nthe user has to investigate if the merge is a fast-forward in order to\nknow what the outcome of the command will be.\n\nIf the merge is a fast-forward, --no-commit has no effect on the\noutcome. If the merge is not a fast-forward, --no-commit has a huge\neffect on the outcome.\n\nIf I see a --no-commit option, as an inexperienced user, I would be\nquite surprised to find my HEAD changed after using it. It would be\nfar more intuitive, for that user, for --no-commit to imply --no-ff\nhowever I suspect that such a change may well cause more problems then\nit fixes.\n\nWhat I wonder is, in what situation is the current behaviour is desirable?\n\nWhile I agree that the option works as designed, I think its behaviour\nis more surprising to the end user then it should be.\n\nRegards,\n\nAndrew Ardill\n"},{"id":"283723","messageId":"87a8krpehl.fsf@gmail.com","threadId":"42055","inReplyTo":"CAH5451kW3t1Y7oW=uHv85jzHwsnQcDK2jdLisauNF-x1LRwqLA@mail.gmail.com","subject":"Re: git merge branch --no-commit does commit fast forward merges","fromName":"Christoph Paulik","fromEmail":"cpaulik@gmail.com","sentAt":"2016-04-18T07:23:18Z","receivedAt":"2016-04-18T07:23:18Z","isPatch":false,"sender":{"key":"cpaulik@gmail.com","avatar":null},"body":"\nMy expectations from what should happen came mainly from the \ndescription of the --no-commit flag in the help:\n\nWith --no-commit perform the merge but pretend the merge failed \nand do not autocommit, to give the user a chance to inspect and \nfurther tweak the merge result before committing. \n\nSo in the case of a fast-forward the flag does not pretend that \nthe merge failed. \n\nRegards,\nChristoph \n \n\nAndrew Ardill writes:\n\n> On 18 April 2016 at 16:26, Johannes Schindelin\n> wrote:\n>>\n>> > The command only works as expected when also adding the \n>> > --no-ff flag.\n>>\n>> Then you need to fix your expectations ;-)\n>\n> I *think* the core of this problem is that the intent of the \n> end-user does not align with the command options available.\n>\n> In this use case (as far as I can tell), the user wants to see \n> what the result of a merge from somewhere else will look like, \n> without changing their HEAD.\n>\n> While you are correct in saying a fast-forward does not create \n> any new commits, for the user it certainly looks like a whole \n> slew of new commits have been added. Moreover, the nature of the \n> option means that the user has to investigate if the merge is a \n> fast-forward in order to know what the outcome of the command \n> will be.\n>\n> If the merge is a fast-forward, --no-commit has no effect on the \n> outcome. If the merge is not a fast-forward, --no-commit has a \n> huge effect on the outcome.\n>\n> If I see a --no-commit option, as an inexperienced user, I would \n> be quite surprised to find my HEAD changed after using it. It \n> would be far more intuitive, for that user, for --no-commit to \n> imply --no-ff however I suspect that such a change may well \n> cause more problems then it fixes.\n>\n> What I wonder is, in what situation is the current behaviour is \n> desirable?\n>\n> While I agree that the option works as designed, I think its \n> behaviour is more surprising to the end user then it should be.\n>\n> Regards,\n>\n> Andrew Ardill\n\n\n-- \n\n-------------------------------------------------------\nChristoph Paulik\nTwitter, Github: @cpaulik\nPGP: 8CFC D7DF 2867 B2DC 749B 1B0A 6E3B A262 5186 A0AC\n"},{"id":"283724","messageId":"CAH5451mDYhavx_OLfXe6cC2WguCsFWEBBBBOCPyX3E6ZJw27+w@mail.gmail.com","threadId":"42055","inReplyTo":"87a8krpehl.fsf@gmail.com","subject":"Re: git merge branch --no-commit does commit fast forward merges","fromName":"Andrew Ardill","fromEmail":"andrew.ardill@gmail.com","sentAt":"2016-04-18T07:44:13Z","receivedAt":"2016-04-18T07:44:13Z","isPatch":false,"sender":{"key":"andrew.ardill@gmail.com","avatar":"https://gravatar.com/avatar/da14cb7c091dd44dc6c63a4d3361b149acaf25226dc78eb4131a17b93d9b0993?d=mp&s=160"},"body":"On 18 April 2016 at 17:23, Christoph Paulik wrote:\n> My expectations from what should happen came mainly from the description of\n> the --no-commit flag in the help:\n>\n> With --no-commit perform the merge but pretend the merge failed and do not\n> autocommit, to give the user a chance to inspect and further tweak the merge\n> result before committing.\n> So in the case of a fast-forward the flag does not pretend that the merge\n> failed.\n\nYes, I think the mis-alignment in expectations comes from a\ntechnicality in the description you quote. The fast forward is in some\nways not really counted as a true merge, and no new commits are\ncreated.\n\nThus, the merge progresses up to the point where a merge resolution\nwould have to take place, realises that there is no merge resolution\nto do (it's just a fast forward!) and so exits out. Unfortunately, a\nside effect of this is that the fast-forward has already happened and\nso you are left with something different from what was expected.\n\nI do think that the --no-commit option should imply --no-ff (as this\nwould make the behaviour consistent for end-users). I don't know if\nthis is something that would break scripts etc, but if so you could\nmake it implied only if we detect a terminal or something like is done\nin other places.\n\nRegards,\n\nAndrew Ardill\n"},{"id":"283731","messageId":"xmqqy48a6fht.fsf@gitster.mtv.corp.google.com","threadId":"42055","inReplyTo":"CAH5451mDYhavx_OLfXe6cC2WguCsFWEBBBBOCPyX3E6ZJw27+w@mail.gmail.com","subject":"Re: git merge branch --no-commit does commit fast forward merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-18T16:36:30Z","receivedAt":"2016-04-18T16:36:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Ardill writes:\n\n> Yes, I think the mis-alignment in expectations comes from a\n> technicality in the description you quote. The fast forward is in some\n> ways not really counted as a true merge, and no new commits are\n> created.\n\nLooking at 123ee3ca (Add --no-commit to git-merge/git-pull.,\n2005-11-01) and $gmane/10998 [*1*], it is clear that \"--no-commit\"\nwas never meant as a \"preview of what would happen\". The original\ndocumentation update at 37465016 (Documentation: -merge and -pull:\ndescribe merge strategies., 2005-11-04) was not great, but was\nclarified at d8ae1d10 (Document the --no-commit flag better,\n2005-11-04), and that version of text survives to this day.\n\nThe real reason why \"--no-commit\" was added was because back then\n\"git commit --amend\" did not even exist; it appeared only at\nb4019f04 (git-commit --amend, 2006-03-02).\n\nWhat is (and was back then) the recommended way to see what changes\nmerging the other branch brings in to your branch, then?\n\nThere are at least three ways, all of which are better suited than\n\"--no-commit\".\n\nWhen you want to study and understand what changes other branch \nmade since it forked from what you are working on, then\n\n $ git diff ...other_branch\n\nwould give you the change as a single ball of wax [*2*].\n\nIf you want to see individual changes explained by their authors,\nyou can also do\n\n $ git log -p ..other_branch\n\nFinally, if you want to see what the merge result would look like,\nyou just do the merge. Advancing the HEAD by one commit and then\ngoing back once you are done is a cheap operation. If you want to\navoid updating your branch for real, these days you can even do so\non a detached HEAD, unlike old days back when there was not even\n\"commit --amend\".\n\n $ git checkout HEAD^0\n $ git merge other_branch\n\n $ git diff ORIG_HEAD ;# what changed overall?\n $ git log -p ORIG_HEAD.. ;# inspect individual changes\n\n $ git checkout - ;# come back to the original branch\n\n> I do think that the --no-commit option should imply --no-ff (as this\n> would make the behaviour consistent for end-users). I don't know if\n> this is something that would break scripts etc, but if so you could\n> make it implied only if we detect a terminal or something like is done\n> in other places.\n\nIf we were living in an ideal world where \"git commit --amend\" were\nalready there in November 2005, we wouldn't have \"merge --no-commit\"\nor \"pull --no-commit\" in our system today, and in such a world, I\nwould agree that \"try to populate the working tree and the index\nwith result of a hypothetical merge and stop without updating HEAD\nnor creating MERGE_HEAD, only to show what would happen if I merged\"\noption could be a useful addition to these two commands. And we may\nchoose to call such an option \"--no-commit\". I agree that such an\noption should probably imply \"--no-ff\".\n\nBut we are not living in that world. Making \"--no-commit\" (which is\nnot that \"try to populate and show\" command) imply \"--no-ff\" will\nbreak existing scripts. And unlike cosmetic things like \"do we show\nin color\", changing the behaviour of a command in a fundamental way\nbased on term and istty() is a sure way to make commands harder to\nunderstand (\"it works this way from the terminal, but it works\ndifferently in my script. what is going on?\" is not a question we\nare better off not seeing on this list).\n\nThanks.\n\n[Notes and References]\n\n*1* http://thread.gmane.org/gmane.comp.version-control.git/10998 \n\n*2* Notice the three dots; it is a short-hand for\n\n $ git diff ^$(git merge-base HEAD other_branch) other_branch\n"},{"id":"283732","messageId":"xmqqtwiy6end.fsf@gitster.mtv.corp.google.com","threadId":"42055","inReplyTo":"xmqqy48a6fht.fsf@gitster.mtv.corp.google.com","subject":"Re: git merge branch --no-commit does commit fast forward merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-18T16:54:46Z","receivedAt":"2016-04-18T16:54:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Andrew Ardill writes:\n>\n>> I do think that the --no-commit option should imply --no-ff (as this\n>> would make the behaviour consistent for end-users). I don't know if\n>> this is something that would break scripts etc, but if so you could\n>> make it implied only if we detect a terminal or something like is done\n>> in other places.\n>\n> But we are not living in that world. Making \"--no-commit\" (which is\n> not that \"try to populate and show\" command) imply \"--no-ff\" will\n> break existing scripts....\n\nHaving said all that, there is one change we might want to consider,\nwith a plan to transition to cope with backward incompatibility.\n\nA user who uses \"--no-commit\" does so with the intention to record a\nresulting merge after amending the merge result in the working tree.\nBut there is nothing to amend and record, if the same \"git merge\"\nwithout \"--no-commit\" wouldn't have created a merge commit (there\nare two cases: (1) the other branch is a descendant of the current\nbranch, (2) the other branch is an ancestor of the current branch).\n\nAnd the user would want to know that before doing further damange to\nhis history, so we may want to start warn when \"--no-commit\"\nfast-forwarded or succeeded with \"already up-to-date\", with\ndeprecation notice, and eventually want to make it an error.\n\nThose who want to do a scripted\n\n\tgit merge --no-commit \"$1\" &&\n autoedit \"$1\" &&\n git commit\n\n(where the script takes a branch name $1 and uses auto-edit to\nfurther record the fact that a branch $1 was merged to somewhere in\nthe contents) would already be buggy if it wants to force no-ff, and\nwill get a warning (and later an error), with such a change. And\nfixing the script to add \"--no-ff\" next to \"--no-commit\" will also\nstop the new warning/error.\n"},{"id":"284606","messageId":"xmqqk2jkdpjr.fsf_-_@gitster.mtv.corp.google.com","threadId":"42055","inReplyTo":"xmqqtwiy6end.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 1/2] merge: do not contaminate option_commit with --squash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-26T21:32:24Z","receivedAt":"2016-04-26T21:32:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It is true that we do not make a commit when we are asked to do\n\"merge --squash\", and the code does so by setting option_commit\nvariable to zero when seeing the squash option. But this made it\nimpossible to see from the value of option_commit if --no-commit was\ngiven from the command line, or --squash turned it off.\n\nWe check for the value of option_commit at only two places. Check\nfor the value of squash at them, too.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Just a preliminary clean-up for the next one which is on topic.\n\n builtin/merge.c | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex bf2f261..de9d1b6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1237,11 +1237,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (verbosity < 0)\n \t\tshow_diffstat = 0;\n \n-\tif (squash) {\n-\t\tif (fast_forward == FF_NO)\n-\t\t\tdie(_(\"You cannot combine --squash with --no-ff.\"));\n-\t\toption_commit = 0;\n-\t}\n+\tif (squash && fast_forward == FF_NO)\n+\t\tdie(_(\"You cannot combine --squash with --no-ff.\"));\n \n \tif (!argc) {\n \t\tif (default_to_upstream)\n@@ -1449,10 +1446,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t * We are not doing octopus and not fast-forward. Need\n \t\t * a real merge.\n \t\t */\n-\telse if (!remoteheads->next && !common->next && option_commit) {\n+\telse if (!remoteheads->next && !common->next && option_commit && !squash)\n \t\t/*\n \t\t * We are not doing octopus, not fast-forward, and have\n-\t\t * only one common.\n+\t\t * only one common. And we do want to create a new commit.\n \t\t */\n \t\trefresh_cache(REFRESH_QUIET);\n \t\tif (allow_trivial && fast_forward != FF_ONLY) {\n@@ -1535,7 +1532,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tret = try_merge_strategy(use_strategies[i]->name,\n \t\t\t\t\t common, remoteheads,\n \t\t\t\t\t head_commit, head_arg);\n-\t\tif (!option_commit && !ret) {\n+\t\tif ((!option_commit || squash) && !ret) {\n \t\t\tmerge_was_ok = 1;\n \t\t\t/*\n \t\t\t * This is necessary here just to avoid writing\n-- \n2.8.1-491-g88b9e4a\n"},{"id":"284607","messageId":"xmqqd1pcdpb8.fsf_-_@gitster.mtv.corp.google.com","threadId":"42055","inReplyTo":"xmqqtwiy6end.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 2/2] merge: warn --no-commit merge when no new commit is created","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-26T21:37:31Z","receivedAt":"2016-04-26T21:37:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A user who uses \"--no-commit\" does so with the intention to record a\nresulting merge after amending the merge result in the working tree.\nBut there is nothing to amend and record, if the same \"git merge\"\nwithout \"--no-commit\" wouldn't have created a merge commit (there\nare two cases: (1) the other branch is a descendant of the current\nbranch, (2) the other branch is an ancestor of the current branch).\n\nThe user would want to know that before doing further damange to his\nhistory. When \"merge --no-commit\" fast-forwarded or succeeded with\n\"already up-to-date\" or \"fast-forward\", give a warning message.\n\nWe may want to turn this into a die() after a transition period.\n\nSigned-off-by: Junio C Hamano \n---\n\n * The necessary update to avoid end-user mistake would look like\n this. I am not queuing this or further working on it myself,\n as I am not sure if it is all that useful.\n\n builtin/merge.c | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f641751..66c536d 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1157,6 +1157,15 @@ static struct commit_list *collect_parents(struct commit *head_commit,\n \treturn remoteheads;\n }\n \n+static void no_commit_impossible(const char *message)\n+{\n+\tif (!option_commit) {\n+\t\twarning(\"%s\\n%s\", _(message),\n+\t\t\t_(\"--no-commit is impossible\"));\n+\t\twarning(_(\"In future versions of Git, this will become an error.\"));\n+\t}\n+}\n+\n int cmd_merge(int argc, const char **argv, const char *prefix)\n {\n \tunsigned char result_tree[20];\n@@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t * If head can reach all the merge then we are up to date.\n \t\t * but first the most common case of merging one remote.\n \t\t */\n+\t\tno_commit_impossible(_(\"Already up-to-date\"));\n \t\tfinish_up_to_date(\"Already up-to-date.\");\n \t\tgoto done;\n \t} else if (fast_forward != FF_NO && !remoteheads->next &&\n@@ -1412,6 +1422,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tstruct strbuf msg = STRBUF_INIT;\n \t\tstruct commit *commit;\n \n+\t\tno_commit_impossible(_(\"Fast-forward\"));\n \t\tif (verbosity >= 0) {\n \t\t\tchar from[GIT_SHA1_HEXSZ + 1], to[GIT_SHA1_HEXSZ + 1];\n \t\t\tfind_unique_abbrev_r(from, head_commit->object.oid.hash,\n@@ -1488,6 +1499,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t}\n \t\tif (up_to_date) {\n+\t\t\tno_commit_impossible(_(\"Already up-to-date\"));\n \t\t\tfinish_up_to_date(\"Already up-to-date. Yeeah!\");\n \t\t\tgoto done;\n \t\t}\n-- \n2.8.1-491-g88b9e4a\n"},{"id":"284609","messageId":"CAGZ79ka-HbB=oCidTtKSa32R9kqd2_c-cG8h+ttAVy=a8UfgFQ@mail.gmail.com","threadId":"42055","inReplyTo":"xmqqd1pcdpb8.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-26T21:53:31Z","receivedAt":"2016-04-26T21:53:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Apr 26, 2016 at 2:37 PM, Junio C Hamano wrote:\n> +static void no_commit_impossible(const char *message)\n> +{\n> + if (!option_commit) {\n> + warning(\"%s\\n%s\", _(message),\n> + _(\"--no-commit is impossible\"));\n> + warning(_(\"In future versions of Git, this will become an error.\"));\n> + }\n> +}\n\nDuring discussion of the parallel process framework\n(sb/submodule-parallel-fetch~3),\nyou seemed very inclined on not having major decisions made deep\ninside the helper\nfunction, but rather at the main function to easier see the program flow IIRC.\n\nThis looks very similar to me as we'll have the no_commit_impossible function\nwhich is a helper of cmd_merge. Following your advice there, I would\nhave expected to\nhave\n\n static void no_commit_impossible(const char *message)\n {\n warning(\"%s\\n%s\", _(message), _(\"--no-commit is impossible\"));\n warning(_(\"In future versions of Git, this will become an error.\"));\n }\n\nand later\n\n if (!option_commit)\n no_commit_impossible(_(\"Already up-to-date\"));\n\n\n> +\n> int cmd_merge(int argc, const char **argv, const char *prefix)\n> {\n> unsigned char result_tree[20];\n> @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> * If head can reach all the merge then we are up to date.\n> * but first the most common case of merging one remote.\n> */\n> + no_commit_impossible(_(\"Already up-to-date\"));\n> finish_up_to_date(\"Already up-to-date.\");\n\nComing back to this patch, in case of -v given, we'll\nsee (\"Already up-to-date\") twice?\n\nIf --quiet is given, do we want to suppress output\nin no_commit_impossible?\n"},{"id":"284610","messageId":"xmqq1t5sdo9j.fsf@gitster.mtv.corp.google.com","threadId":"42055","inReplyTo":"CAGZ79ka-HbB=oCidTtKSa32R9kqd2_c-cG8h+ttAVy=a8UfgFQ@mail.gmail.com","subject":"Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-26T22:00:08Z","receivedAt":"2016-04-26T22:00:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> and later\n>\n> if (!option_commit)\n> no_commit_impossible(_(\"Already up-to-date\"));\n\nIt would be more legible, but because there are so few callsites in\nan already shallow callchain, I do not think it makes that much of a\ndifference in this codepath either way.\n\n>> +\n>> int cmd_merge(int argc, const char **argv, const char *prefix)\n>> {\n>> unsigned char result_tree[20];\n>> @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>> * If head can reach all the merge then we are up to date.\n>> * but first the most common case of merging one remote.\n>> */\n>> + no_commit_impossible(_(\"Already up-to-date\"));\n>> finish_up_to_date(\"Already up-to-date.\");\n>\n> Coming back to this patch, in case of -v given, we'll\n> see (\"Already up-to-date\") twice?\n\nOne that explains why --no-commit is impossible in warning, and the\nother is the final report of what happened, so yes.\n\n> If --quiet is given, do we want to suppress output\n> in no_commit_impossible?\n\nWhile we are using warning(), we probably do want to. When we\nswitch to die() at a major version boundary, we don't.\n"},{"id":"284623","messageId":"CAPig+cTo09ECiPrtzHAHrr1a15khcYkL8QQpe_AjX2tEhRum2A@mail.gmail.com","threadId":"42055","inReplyTo":"xmqqd1pcdpb8.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-27T01:39:22Z","receivedAt":"2016-04-27T01:39:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 26, 2016 at 5:37 PM, Junio C Hamano wrote:\n> A user who uses \"--no-commit\" does so with the intention to record a\n> resulting merge after amending the merge result in the working tree.\n> But there is nothing to amend and record, if the same \"git merge\"\n> without \"--no-commit\" wouldn't have created a merge commit (there\n> are two cases: (1) the other branch is a descendant of the current\n> branch, (2) the other branch is an ancestor of the current branch).\n>\n> The user would want to know that before doing further damange to his\n\ns/damange/damage/\n\n> history. When \"merge --no-commit\" fast-forwarded or succeeded with\n> \"already up-to-date\" or \"fast-forward\", give a warning message.\n>\n> We may want to turn this into a die() after a transition period.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"284627","messageId":"572054B9.5080601@kdbg.org","threadId":"42055","inReplyTo":"xmqqd1pcdpb8.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-04-27T05:57:13Z","receivedAt":"2016-04-27T05:57:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 26.04.2016 um 23:37 schrieb Junio C Hamano:\n> * The necessary update to avoid end-user mistake would look like\n> this. I am not queuing this or further working on it myself,\n> as I am not sure if it is all that useful.\n\nWhoever picks up this patch, be warned that the i18n coding should be \ncorrected:\n\n> +static void no_commit_impossible(const char *message)\n> +{\n> +\tif (!option_commit) {\n> +\t\twarning(\"%s\\n%s\", _(message),\n\nThe i18n call around message is not required, because...\n\n> +\t\t\t_(\"--no-commit is impossible\"));\n> +\t\twarning(_(\"In future versions of Git, this will become an error.\"));\n> +\t}\n> +}\n> +\n> int cmd_merge(int argc, const char **argv, const char *prefix)\n> {\n> \tunsigned char result_tree[20];\n> @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \t\t * If head can reach all the merge then we are up to date.\n> \t\t * but first the most common case of merging one remote.\n> \t\t */\n> +\t\tno_commit_impossible(_(\"Already up-to-date\"));\n\n... the call sites takes care of it.\n\n-- Hannes\n"},{"id":"284633","messageId":"alpine.DEB.2.20.1604270846200.2896@virtualbox","threadId":"42055","inReplyTo":"xmqqk2jkdpjr.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] merge: do not contaminate option_commit with --squash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-27T06:46:38Z","receivedAt":"2016-04-27T06:46:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 26 Apr 2016, Junio C Hamano wrote:\n\n> It is true that we do not make a commit when we are asked to do\n> \"merge --squash\", and the code does so by setting option_commit\n> variable to zero when seeing the squash option. But this made it\n> impossible to see from the value of option_commit if --no-commit was\n> given from the command line, or --squash turned it off.\n> \n> We check for the value of option_commit at only two places. Check\n> for the value of squash at them, too.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * Just a preliminary clean-up for the next one which is on topic.\n\nI think it would make for a nice cleanup anyways.\n\nCiao,\nDscho\n"},{"id":"284634","messageId":"alpine.DEB.2.20.1604270846430.2896@virtualbox","threadId":"42055","inReplyTo":"xmqqd1pcdpb8.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-27T06:50:01Z","receivedAt":"2016-04-27T06:50:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 26 Apr 2016, Junio C Hamano wrote:\n\n> @@ -1157,6 +1157,15 @@ static struct commit_list *collect_parents(struct commit *head_commit,\n> \treturn remoteheads;\n> }\n> \n> +static void no_commit_impossible(const char *message)\n> +{\n> +\tif (!option_commit) {\n> +\t\twarning(\"%s\\n%s\", _(message),\n> +\t\t\t_(\"--no-commit is impossible\"));\n> +\t\twarning(_(\"In future versions of Git, this will become an error.\"));\n> +\t}\n> +}\n\nI think this would be a step forward in usability, and I agree that this\nis a great opportunity for the users who wish for this feature to get\ninvolved and drive this forward, based on your excellent initial version.\n\nI am not sure about this double negation \"no_commit_impossible\" (I only\nunderstood what you meant because I had read the commit message first,\nsomething I won't do when stumbling over this code later).\n\nMaybe something like `disallow_no_commit`?\n\nCiao,\nDscho\n"},{"id":"284651","messageId":"xmqqinz3axvc.fsf@gitster.mtv.corp.google.com","threadId":"42055","inReplyTo":"alpine.DEB.2.20.1604270846430.2896@virtualbox","subject":"Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T15:13:11Z","receivedAt":"2016-04-27T15:13:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I am not sure about this double negation \"no_commit_impossible\" (I only\n> understood what you meant because I had read the commit message first,\n> something I won't do when stumbling over this code later).\n>\n> Maybe something like `disallow_no_commit`?\n\nThat would be the best name once we start dying in there. It might\nbe still better, even while we merely warn but let it pass, than the\ndouble negative. Or it may not. I dunno.\n"},{"id":"284652","messageId":"xmqqeg9raxtb.fsf@gitster.mtv.corp.google.com","threadId":"42055","inReplyTo":"alpine.DEB.2.20.1604270846200.2896@virtualbox","subject":"Re: [PATCH 1/2] merge: do not contaminate option_commit with --squash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T15:14:24Z","receivedAt":"2016-04-27T15:14:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> * Just a preliminary clean-up for the next one which is on topic.\n>\n> I think it would make for a nice cleanup anyways.\n\nThere is a missing open-brace that causes a compilation error,\nthough ;-)\n"},{"id":"284654","messageId":"alpine.DEB.2.20.1604271718530.2896@virtualbox","threadId":"42055","inReplyTo":"xmqqeg9raxtb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] merge: do not contaminate option_commit with --squash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-27T15:19:20Z","receivedAt":"2016-04-27T15:19:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 27 Apr 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> * Just a preliminary clean-up for the next one which is on topic.\n> >\n> > I think it would make for a nice cleanup anyways.\n> \n> There is a missing open-brace that causes a compilation error,\n> though ;-)\n\nUh oh ;-) I guess I did not really test it...\n\nCiao,\nDscho\n"},{"id":"284656","messageId":"alpine.DEB.2.20.1604271732570.2896@virtualbox","threadId":"42055","inReplyTo":"xmqqinz3axvc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-27T15:37:04Z","receivedAt":"2016-04-27T15:37:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 27 Apr 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > I am not sure about this double negation \"no_commit_impossible\" (I only\n> > understood what you meant because I had read the commit message first,\n> > something I won't do when stumbling over this code later).\n> >\n> > Maybe something like `disallow_no_commit`?\n> \n> That would be the best name once we start dying in there. It might\n> be still better, even while we merely warn but let it pass, than the\n> double negative. Or it may not. I dunno.\n\nActually, I should admit that I was really puzzled by the name at first. I\nthought that some commits were impossible, but the function said that no\ncommit was impossible. So I thought: but what if a commit references\nitself as parent, would that not be impossible? But actually, once SHA-1\ncollision attacks become feasible, I guess it would not be impossible.\nMaking for an excellent attack vector, say, on repository hosting sites\n(which would now be stuck in infinite loops due to a violation of the\ntemporal prime directive).\n\nSo yeah, this was my thought process when I read no_commit_impossible.\n\n;-)\n\nCiao,\nDscho\n"},{"id":"284659","messageId":"xmqq37q7avlf.fsf@gitster.mtv.corp.google.com","threadId":"42055","inReplyTo":"alpine.DEB.2.20.1604271732570.2896@virtualbox","subject":"Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T16:02:20Z","receivedAt":"2016-04-27T16:02:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > Maybe something like `disallow_no_commit`?\n>> \n>> That would be the best name once we start dying in there. It might\n>> be still better, even while we merely warn but let it pass, than the\n>> double negative. Or it may not. I dunno.\n>\n> Actually, I should admit that I was really puzzled by the name at first. I\n> thought that some commits were impossible, but the function said that no\n> commit was impossible. So I thought: but what if a commit references\n> itself as parent, would that not be impossible? But actually, once SHA-1\n> collision attacks become feasible, I guess it would not be impossible.\n> Making for an excellent attack vector, say, on repository hosting sites\n> (which would now be stuck in infinite loops due to a violation of the\n> temporal prime directive).\n>\n> So yeah, this was my thought process when I read no_commit_impossible.\n\nWe both know that the original name was terrible now. It was meant\nto mean \"it is impossible to honor --no-commit option\", nothing\nmore, and left the door open for deciding what should happen when\nthat condition holds (either \"warn but succeed the merge anyway\" or\n\"die to refuse\"), but it is clear that the name did not convey that\nsuccessfully.\n"}]}