{"thread":{"id":"29058","subject":"[PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch","startedAt":"2011-12-01T01:38:56Z","lastAt":"2011-12-01T18:59:36Z","messageCount":6,"participants":["Samuel Bronson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"180185","messageId":"1322703537-3914-1-git-send-email-naesten@gmail.com","threadId":"29058","inReplyTo":null,"subject":"[PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2011-12-01T01:38:56Z","receivedAt":"2011-12-01T01:38:56Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"Without this, pull becomes unusable for merging branches when the config\noption `merge.ff` is set to `only`.\n\nSigned-off-by: Samuel Bronson \n---\n git-pull.sh | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 9868a0b..a09fcbc 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -76,6 +76,8 @@ do\n \t\tno_ff=--no-ff ;;\n \t--ff-only)\n \t\tff_only=--ff-only ;;\n+\t--no-ff-only)\n+\t\tff_only=--no-ff-only ;;\n \t-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\\\n \t\t--strateg=*|--strategy=*|\\\n \t-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)\n-- \n1.7.7.3\n"},{"id":"180186","messageId":"1322703537-3914-2-git-send-email-naesten@gmail.com","threadId":"29058","inReplyTo":"1322703537-3914-1-git-send-email-naesten@gmail.com","subject":"[PATCH/RFC 2/2] merge, pull: Document the --no-ff-only merge option","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2011-12-01T01:38:57Z","receivedAt":"2011-12-01T01:38:57Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"Now that --no-ff-only will work with pull and not just with merge, we\nshould probably tell the users about it.\n\nSigned-off-by: Samuel Bronson \n---\n Documentation/merge-options.txt | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 6bd0b04..2adc4f5 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -56,9 +56,13 @@ With --no-squash perform the merge and commit the result. This\n option can be used to override --squash.\n \n --ff-only::\n+--no-ff-only::\n \tRefuse to merge and exit with a non-zero status unless the\n \tcurrent `HEAD` is already up-to-date or the merge can be\n \tresolved as a fast-forward.\n++\n+With --no-ff-only, don't refuse. This is can be used to override\n+--ff-only, or the \"`only`\" value for the `merge.ff` configuration option.\n \n -s ::\n --strategy=::\n-- \n1.7.7.3\n"},{"id":"180191","messageId":"7vborsq45x.fsf@alter.siamese.dyndns.org","threadId":"29058","inReplyTo":"1322703537-3914-1-git-send-email-naesten@gmail.com","subject":"Re: [PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-01T04:58:18Z","receivedAt":"2011-12-01T04:58:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Bronson writes:\n\n> Without this, pull becomes unusable for merging branches when the config\n> option `merge.ff` is set to `only`.\n>\n> Signed-off-by: Samuel Bronson \n\nI wonder why you need this. We have \"git config --unset merge.ff\" after\nall. From purely mechanstic point of view, being able to temporarily\ndefeat a configuration variable makes perfect sense, but from the point of\nview of workflow, I am not sure if it is a good thing to even allow it in\nthe first place in this particular case.\n\nSetting merge.ff to 'only' means you are following along other people's\nwork and making nothing new on your own in this particular repository, no?\nHence you won't be asking the upstream to pull from this repository, which\nin turn means that even if you made a merge by temporarily defeating the\nconfiguration setting with this patch, your future pulls will no longer\nfast forward, until somehow the upstream gets hold of your merge commit.\n\nBy the way (this is a digression), I also have to say --no-ff-only is too\n*ugly* as a UI element, even though I know \"git merge\" already allows it\nby accident.\n\n\"ff\" is a tristate. By default, fast-forward is done when appropriate, and\npeople can _refuse_ to fast-forward and force Git to create an extra merge\ncommit. Or if you are strictly following along, you can say you _require_\nfast-forward and reject anything else. So it may make the UI saner if we\nupdated the UI to allow users to say:\n\n\t--ff=normal\tthe default\n --ff=never\tsame as --no-ff that forces an extra merge commit\n --ff=required\tsame as --ff-only\n\nwhile keeping the current --ff-only and --no-ff as backward compatibility\nwart.\n\nI dunno.\n"},{"id":"180205","messageId":"CAJYzjmep7sKxiSNhMzAX2DRYJhANDQkPL5pX4HOZ9CssJxcWbw@mail.gmail.com","threadId":"29058","inReplyTo":"7vborsq45x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2011-12-01T17:18:54Z","receivedAt":"2011-12-01T17:18:54Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"On Wed, Nov 30, 2011 at 11:58 PM, Junio C Hamano wrote:\n> Samuel Bronson writes:\n>\n>> Without this, pull becomes unusable for merging branches when the config\n>> option `merge.ff` is set to `only`.\n>>\n>> Signed-off-by: Samuel Bronson \n>\n> I wonder why you need this. We have \"git config --unset merge.ff\" after\n> all. From purely mechanstic point of view, being able to temporarily\n> defeat a configuration variable makes perfect sense, but from the point of\n> view of workflow, I am not sure if it is a good thing to even allow it in\n> the first place in this particular case.\n>\n> Setting merge.ff to 'only' means you are following along other people's\n> work and making nothing new on your own in this particular repository, no?\n> Hence you won't be asking the upstream to pull from this repository, which\n> in turn means that even if you made a merge by temporarily defeating the\n> configuration setting with this patch, your future pulls will no longer\n> fast forward, until somehow the upstream gets hold of your merge commit.\n\nActually, I wanted to set merge.ff to only avoid *accidentally*\nmerging origin/master onto my local master, when I would likely to\nprefer to either rebase my work onto it, or retroactively put my work\nin a branch and merge that *into* the real master branch; I would then\noverride only when I explicitly did want a merge.\n\n(I actually have commit access to the repository in question, but wish\nto avoid making others' commits to master look as if they were on side\nbranches.)\n\n> By the way (this is a digression), I also have to say --no-ff-only is too\n> *ugly* as a UI element, even though I know \"git merge\" already allows it\n> by accident.\n>\n> \"ff\" is a tristate. By default, fast-forward is done when appropriate, and\n> people can _refuse_ to fast-forward and force Git to create an extra merge\n> commit. Or if you are strictly following along, you can say you _require_\n> fast-forward and reject anything else. So it may make the UI saner if we\n> updated the UI to allow users to say:\n>\n>        --ff=normal     the default\n>        --ff=never      same as --no-ff that forces an extra merge commit\n>        --ff=required   same as --ff-only\n>\n> while keeping the current --ff-only and --no-ff as backward compatibility\n> wart.\n\nHmm, yes, I had noticed that it was a tristate (merge.ff clearly is),\nand I guess --no-ff-only is a pretty ugly flag. I do have to ask,\nthough: why give --ff these new values? Wouldn't it make more sense to\nreuse the values accepted by merge.ff; namely, 'true' (the implied\ndefault), 'false', and 'only'?\n\nOtherwise, this looks like a very nice way to implement what I want: I\nguess it is probably a mistake that the existing (documented) flags do\nnot behave in this way?\n"},{"id":"180206","messageId":"7vvcq0np35.fsf@alter.siamese.dyndns.org","threadId":"29058","inReplyTo":"CAJYzjmep7sKxiSNhMzAX2DRYJhANDQkPL5pX4HOZ9CssJxcWbw@mail.gmail.com","subject":"Re: [PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-01T18:06:54Z","receivedAt":"2011-12-01T18:06:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Bronson writes:\n\n> Hmm, yes, I had noticed that it was a tristate (merge.ff clearly is),\n> and I guess --no-ff-only is a pretty ugly flag. I do have to ask,\n> though: why give --ff these new values? Wouldn't it make more sense to\n> reuse the values accepted by merge.ff; namely, 'true' (the implied\n> default), 'false', and 'only'?\n\nThe 'true' and 'false' values to merge.ff are carry-over from the days\nwhen it was a boolean, _not_ a tristate. If we were to make the UI more\nrational by making it clear that this is not a boolean, it is a good time\nfor us to aim a bit higher than merely repeating the mistakes we made in\nthe past due to historical accident. In other words, we could add a\nsynonym for the \"default\" mode in addition to \"--ff=true\" (and for the\n\"always merge\" mode in addition to \"--ff=false\") that makes it clear that\nthe value is _not_ a boolean [*1*]. If we were to go the \"--ff=\"\nroute, we have to add support for other ways to spell boolean 'true'\n(e.g. 'yes', '1', and 'on') anyway, so it is not that much extra work to\ndo so, I would think.\n\n> Otherwise, this looks like a very nice way to implement what I want: I\n> guess it is probably a mistake that the existing (documented) flags do\n> not behave in this way?\n\nYeah, right now if you say \"merge --ff-only --no-ff\", we say these are\nmutually exclusive (which is true), but if you think about the tristate\nnature of the 'ff' option and spell it differently in your head, i.e.\n\"merge --ff=only --ff=never\", it is reasonable to argue that we should\napply the usual \"last one overrides\" rule and behave as if \"merge --no-ff\"\nwere given (for the purpose of \"last one overrides\", the configured\ndefaults can be treated as if they come very early on the command line).\nAfter all \"merge --no-ff --ff\" does seem to use the \"last one overrides\"\nrule.\n\n[Footnote]\n\n*1* Perhaps 'allowed' instead of 'normal' (which I wrote out of thin-air;\nI do not have any strong preference on the actual values) may be a better\nchoice for such a \"this is not a boolean\" spelling for the default mode.\n"},{"id":"180214","messageId":"CAJYzjmcePWriGLr5a0oe_=6qUkfKp5OyFGGEabj6S8vy+hb4+g@mail.gmail.com","threadId":"29058","inReplyTo":"7vvcq0np35.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2011-12-01T18:59:36Z","receivedAt":"2011-12-01T18:59:36Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"On Thu, Dec 1, 2011 at 1:06 PM, Junio C Hamano wrote:\n> Samuel Bronson writes:\n>\n>> Hmm, yes, I had noticed that it was a tristate (merge.ff clearly is),\n>> and I guess --no-ff-only is a pretty ugly flag. I do have to ask,\n>> though: why give --ff these new values? Wouldn't it make more sense to\n>> reuse the values accepted by merge.ff; namely, 'true' (the implied\n>> default), 'false', and 'only'?\n>\n> The 'true' and 'false' values to merge.ff are carry-over from the days\n> when it was a boolean, _not_ a tristate. If we were to make the UI more\n> rational by making it clear that this is not a boolean, it is a good time\n> for us to aim a bit higher than merely repeating the mistakes we made in\n> the past due to historical accident. In other words, we could add a\n> synonym for the \"default\" mode in addition to \"--ff=true\" (and for the\n> \"always merge\" mode in addition to \"--ff=false\") that makes it clear that\n> the value is _not_ a boolean [*1*]. If we were to go the \"--ff=\"\n> route, we have to add support for other ways to spell boolean 'true'\n> (e.g. 'yes', '1', and 'on') anyway, so it is not that much extra work to\n> do so, I would think.\n\nSure, that makes sense. I was just a little worried that you might be\n(accidentally) proposing that --ff use a different set of names than\nmerge.ff for a moment there...\n\n>> Otherwise, this looks like a very nice way to implement what I want: I\n>> guess it is probably a mistake that the existing (documented) flags do\n>> not behave in this way?\n>\n> Yeah, right now if you say \"merge --ff-only --no-ff\", we say these are\n> mutually exclusive (which is true), but if you think about the tristate\n> nature of the 'ff' option and spell it differently in your head, i.e.\n> \"merge --ff=only --ff=never\", it is reasonable to argue that we should\n> apply the usual \"last one overrides\" rule and behave as if \"merge --no-ff\"\n> were given (for the purpose of \"last one overrides\", the configured\n> defaults can be treated as if they come very early on the command line).\n> After all \"merge --no-ff --ff\" does seem to use the \"last one overrides\"\n> rule.\n\nYes, I totally agree that it would make more sense that way; I\ncertainly tried that before I even began to look at any of the code.\n\n> [Footnote]\n>\n> *1* Perhaps 'allowed' instead of 'normal' (which I wrote out of thin-air;\n> I do not have any strong preference on the actual values) may be a better\n> choice for such a \"this is not a boolean\" spelling for the default mode.\n"}]}