{"thread":{"id":"51763","subject":"[PATCH] merge-options.txt: clarify meaning of various ff-related options","startedAt":"2019-08-28T00:13:16Z","lastAt":"2019-08-31T00:23:30Z","messageCount":16,"participants":["Elijah Newren","Sergey Organov","Martin Ågren","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"381409","messageId":"20190828001307.8042-1-newren@gmail.com","threadId":"51763","inReplyTo":null,"subject":"[PATCH] merge-options.txt: clarify meaning of various ff-related options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-28T00:13:07Z","receivedAt":"2019-08-28T00:13:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"As discovered on the mailing list, some of the descriptions of the\nff-related options were unclear. Try to be more precise with what these\noptions do.\n\nSigned-off-by: Elijah Newren \n---\nI noticed this patch sitting around in one of my branches, and noticed it\nwasn't upstream. I'm pretty sure I submitted it a few months back, but I\nthink it got lost in the cracks. Resubmitting and I'll see if I can do a\nbetter job following up on it.\n\n Documentation/merge-options.txt | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 79a00d2a4a..b39df5f126 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -40,20 +40,22 @@ set to `no` at the beginning of them.\n \tcase of a merge conflict.\n \n --ff::\n-\tWhen the merge resolves as a fast-forward, only update the branch\n-\tpointer, without creating a merge commit. This is the default\n+\tWhen the merge can resolve as a fast-forward, do so (only\n+\tupdate the branch pointer to match the merged branch; do not\n+\tcreate a merge commit). When a fast forward update is not\n+\tpossible, create a merge commit. This is the default\n \tbehavior.\n \n --no-ff::\n-\tCreate a merge commit even when the merge resolves as a\n-\tfast-forward. This is the default behaviour when merging an\n-\tannotated (and possibly signed) tag that is not stored in\n-\tits natural place in 'refs/tags/' hierarchy.\n+\tCreate a merge commit even when the merge could instead resolve\n+\tas a fast-forward. This is the default behaviour when merging\n+\tan annotated (and possibly signed) tag that is not stored in its\n+\tnatural place in 'refs/tags/' hierarchy.\n \n --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+\tWhen possible, resolve the merge as a fast-forward (do not\n+\tcreate a merge commit). When not possible, refuse to merge and\n+\texit with a non-zero status.\n \n -S[]::\n --gpg-sign[=]::\n-- \n2.23.0.38.gfc6987be7e\n\n"},{"id":"381426","messageId":"877e6x3bjj.fsf@osv.gnss.ru","threadId":"51763","inReplyTo":"20190828001307.8042-1-newren@gmail.com","subject":"Re: [PATCH] merge-options.txt: clarify meaning of various ff-related options","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2019-08-28T09:05:20Z","receivedAt":"2019-08-28T09:05:24Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Elijah Newren writes:\n\n> As discovered on the mailing list, some of the descriptions of the\n> ff-related options were unclear. Try to be more precise with what these\n> options do.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> I noticed this patch sitting around in one of my branches, and noticed it\n> wasn't upstream. I'm pretty sure I submitted it a few months back, but I\n> think it got lost in the cracks. Resubmitting and I'll see if I can do a\n> better job following up on it.\n>\n> Documentation/merge-options.txt | 20 +++++++++++---------\n> 1 file changed, 11 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> index 79a00d2a4a..b39df5f126 100644\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -40,20 +40,22 @@ set to `no` at the beginning of them.\n> \tcase of a merge conflict.\n> \n> --ff::\n> -\tWhen the merge resolves as a fast-forward, only update the branch\n> -\tpointer, without creating a merge commit. This is the default\n> +\tWhen the merge can resolve as a fast-forward, do so (only\n> +\tupdate the branch pointer to match the merged branch; do not\n> +\tcreate a merge commit). When a fast forward update is not\n> +\tpossible, create a merge commit. This is the default\n> \tbehavior.\n> \n> --no-ff::\n> -\tCreate a merge commit even when the merge resolves as a\n> -\tfast-forward. This is the default behaviour when merging an\n> -\tannotated (and possibly signed) tag that is not stored in\n> -\tits natural place in 'refs/tags/' hierarchy.\n> +\tCreate a merge commit even when the merge could instead resolve\n> +\tas a fast-forward. This is the default behaviour when merging\n> +\tan annotated (and possibly signed) tag that is not stored in its\n> +\tnatural place in 'refs/tags/' hierarchy.\n\nPlease notice that virtually all the other cases of\n--something/--no-something are formatted like this:\n\n--something::\n--no-something::\n [descriptions]\n\nSo, even only for consistency, it seems to be better to have this the\nsame way:\n\n --ff::\n --no-ff::\n --ff-only::\n [descriptions]\n\nthat, as a bonus, will make it explicit and crystal clear that these 3\nthings are alternatives, and thus the last one on the command line takes\nprecedence.\n\n-- Sergey\n"},{"id":"381454","messageId":"20190828155131.29821-1-newren@gmail.com","threadId":"51763","inReplyTo":"877e6x3bjj.fsf@osv.gnss.ru","subject":"[PATCH v2] merge-options.txt: clarify meaning of various ff-related options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-28T15:51:31Z","receivedAt":"2019-08-28T15:51:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"As discovered on the mailing list, some of the descriptions of the\nff-related options were unclear. Try to be more precise with what these\noptions do.\n\nSigned-off-by: Elijah Newren \n---\nChanges since v1:\n * Grouped much like --option/--no-option items are to make it clearer\n that these are related options, as suggested by Sergey.\n\n Documentation/merge-options.txt | 33 ++++++++++++++++++++-------------\n 1 file changed, 20 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 79a00d2a4a..348d66f54b 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -39,21 +39,28 @@ set to `no` at the beginning of them.\n \tto `MERGE_MSG` before being passed on to the commit machinery in the\n \tcase of a merge conflict.\n \n---ff::\n-\tWhen the merge resolves as a fast-forward, only update the branch\n-\tpointer, without creating a merge commit. This is the default\n-\tbehavior.\n-\n+--ff-only::\n --no-ff::\n-\tCreate a merge commit even when the merge resolves as a\n-\tfast-forward. This is the default behaviour when merging an\n-\tannotated (and possibly signed) tag that is not stored in\n-\tits natural place in 'refs/tags/' hierarchy.\n+--ff::\n+\tWhether to only allow resolving the merge as a fast forward\n+\t(only updating the branch pointer to match the merged branch\n+\tand not creating a merge commit), to never allow it (always\n+\tcreating a merge commit), or to prefer it when possible. The\n+\tdefault is --ff, except when merging an annotated (and\n+\tpossibly signed) tag that is not stored in its natural place\n+\tin 'refs/tags/' hierarchy (in which case --no-ff is the\n+\tdefault).\n++\n+With --ff-only, resolve the merge as a fast-forward when possible\n+(when the merged branch contains the current branch in its history).\n+When not possible, refuse to merge and exit with a non-zero status.\n++\n+With --no-ff, create a merge commit in all cases, even when the merge\n+could instead resolve as a fast-forward.\n++\n+With --ff, resolve the merge as a fast-forward when possible. When not\n+possible, create a merge commit.\n \n---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 -S[]::\n --gpg-sign[=]::\n-- \n2.23.0.9.g568fda2d03\n\n"},{"id":"381463","messageId":"CAN0heSoqy4sCY8NUWKuEkXwe2XxnYAN6Mn2N75hYwfQ_5WGYdQ@mail.gmail.com","threadId":"51763","inReplyTo":"20190828155131.29821-1-newren@gmail.com","subject":"Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-08-28T18:45:27Z","receivedAt":"2019-08-28T18:45:42Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Elijah,\n\nOn Wed, 28 Aug 2019 at 17:52, Elijah Newren wrote:\n> ---ff::\n> - When the merge resolves as a fast-forward, only update the branch\n> - pointer, without creating a merge commit. This is the default\n> - behavior.\n> -\n> +--ff-only::\n> --no-ff::\n> - Create a merge commit even when the merge resolves as a\n> - fast-forward. This is the default behaviour when merging an\n> - annotated (and possibly signed) tag that is not stored in\n> - its natural place in 'refs/tags/' hierarchy.\n> +--ff::\n> + Whether to only allow resolving the merge as a fast forward\n> + (only updating the branch pointer to match the merged branch\n> + and not creating a merge commit), to never allow it (always\n> + creating a merge commit), or to prefer it when possible. The\n> + default is --ff, except when merging an annotated (and\n\nIt would be great if you'd write this as `--ff` so that it got\nmonospaced in the rendered documentation. Same thing with `no-ff` later\nin this paragraph and a few more times in the next three paragraphs that\nyou're adding.\n\n> + possibly signed) tag that is not stored in its natural place\n> + in 'refs/tags/' hierarchy (in which case --no-ff is the\n> + default).\n\nI'd also write `refs/tags/`, but I realize that you're just inheriting\nwhat is already here. If you'd rather punt on that, that's understood.\nThis whole document could need a look-over with respect to monospacing\nanyway, but it seems unfortunate to introduce *new* non-monospaced\ninstances, especially for command-line options where it's pretty clear\nhow they should be handled (Documentation/CodingGuidelines, line ~610).\n\n> ++\n> +With --ff-only, resolve the merge as a fast-forward when possible\n> +(when the merged branch contains the current branch in its history).\n> +When not possible, refuse to merge and exit with a non-zero status.\n> ++\n> +With --no-ff, create a merge commit in all cases, even when the merge\n> +could instead resolve as a fast-forward.\n> ++\n> +With --ff, resolve the merge as a fast-forward when possible. When not\n> +possible, create a merge commit.\n>\n> ---ff-only::\n> - Refuse to merge and exit with a non-zero status unless the\n> - current `HEAD` is already up to date or the merge can be\n> - resolved as a fast-forward.\n\nI was sort of expecting these to be listed in the order \"--ff, --no-ff,\n--ff-only\", and I see Sergey suggested the same ordering. The way your\nproposed text reads does make sense though... Would it read as well\nturning it over and going through the options in the other order? That's\nthe way it is before your patch, so you could argue \"but people don't\ngrok that!\". What your patch does well is to offer an overview before\ndescribing each option in a bit more detail. Would that work with the\nreversed direction as well (compared to this patch)? Dunno.\n\nI wondered briefly whether it might make sense to float \"The default is\n`--no-ff`.\" to the top, but since it's really \"The default ... unless\nso-and-so\", it would probably complicate things more than it'd help.\n\nMartin\n"},{"id":"381465","messageId":"87a7btqeyx.fsf@osv.gnss.ru","threadId":"51763","inReplyTo":"CAN0heSoqy4sCY8NUWKuEkXwe2XxnYAN6Mn2N75hYwfQ_5WGYdQ@mail.gmail.com","subject":"Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2019-08-28T19:15:02Z","receivedAt":"2019-08-28T19:15:07Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Hi,\n\nMartin Ågren writes:\n\n\n[...]\n\n>> ++\n>> +With --ff-only, resolve the merge as a fast-forward when possible\n>> +(when the merged branch contains the current branch in its history).\n>> +When not possible, refuse to merge and exit with a non-zero status.\n>> ++\n>> +With --no-ff, create a merge commit in all cases, even when the merge\n>> +could instead resolve as a fast-forward.\n>> ++\n>> +With --ff, resolve the merge as a fast-forward when possible. When not\n>> +possible, create a merge commit.\n>>\n>> ---ff-only::\n>> - Refuse to merge and exit with a non-zero status unless the\n>> - current `HEAD` is already up to date or the merge can be\n>> - resolved as a fast-forward.\n>\n> I was sort of expecting these to be listed in the order \"--ff, --no-ff,\n> --ff-only\", and I see Sergey suggested the same ordering. The way your\n> proposed text reads does make sense though... Would it read as well\n> turning it over and going through the options in the other order? That's\n> the way it is before your patch, so you could argue \"but people don't\n> grok that!\". What your patch does well is to offer an overview before\n> describing each option in a bit more detail. Would that work with the\n> reversed direction as well (compared to this patch)? Dunno.\n>\n> I wondered briefly whether it might make sense to float \"The default is\n> `--no-ff`.\" to the top, but since it's really \"The default ... unless\n> so-and-so\", it would probably complicate things more than it'd help.\n\nDunno if it helps, but here is what I came up with somewhere in previous\ndiscussions:\n\n--ff::\n--no-ff::\n--ff-only::\n\tWhen the merge resolves as a fast-forward, only update the\n\tbranch pointer (without creating a merge commit). When a fast\n\tforward update is not possible, create a merge commit. This is\n\tthe default behavior, unless merging an annotated (and possibly\n\tsigned) tag that is not stored in its natural place in\n\t'refs/tags/' hierarchy, in which case --no-ff is assumed.\n+\nWith --no-ff create a merge commit even when the merge could instead\nresolve as a fast-forward.\n+\nWith --ff-only resolve the merge as a fast-forward (never create a merge\ncommit). When fast-forward is not possible, refuse to merge and exit\nwith non-zero status.\n\n-- \nSergey\n"},{"id":"381466","messageId":"CAN0heSpcFgNwBKHzUjcvb0W46LQwGoOSpihLg1arOPJJKC9PTQ@mail.gmail.com","threadId":"51763","inReplyTo":"87a7btqeyx.fsf@osv.gnss.ru","subject":"Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-08-28T19:53:47Z","receivedAt":"2019-08-28T19:54:02Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hiya,\n\nOn Wed, 28 Aug 2019 at 21:15, Sergey Organov wrote:\n> > I was sort of expecting these to be listed in the order \"--ff, --no-ff,\n> > --ff-only\", and I see Sergey suggested the same ordering. The way your\n> > proposed text reads does make sense though... Would it read as well\n> > turning it over and going through the options in the other order? That's\n> > the way it is before your patch, so you could argue \"but people don't\n> > grok that!\". What your patch does well is to offer an overview before\n> > describing each option in a bit more detail. Would that work with the\n> > reversed direction as well (compared to this patch)? Dunno.\n\n> Dunno if it helps, but here is what I came up with somewhere in previous\n> discussions:\n>\n> --ff::\n> --no-ff::\n> --ff-only::\n> When the merge resolves as a fast-forward, only update the\n> branch pointer (without creating a merge commit). When a fast\n> forward update is not possible, create a merge commit. This is\n> the default behavior, unless merging an annotated (and possibly\n> signed) tag that is not stored in its natural place in\n> 'refs/tags/' hierarchy, in which case --no-ff is assumed.\n> +\n> With --no-ff create a merge commit even when the merge could instead\n> resolve as a fast-forward.\n> +\n> With --ff-only resolve the merge as a fast-forward (never create a merge\n> commit). When fast-forward is not possible, refuse to merge and exit\n> with non-zero status.\n\nI think this could make clear that the first paragraph talks about\n`--ff`. I know that we often list `--foo` and `--no-foo`, say \"This\noption does bar\" and leave it to the reader to figure out which is which.\nMost of the time, that's fairly obvious though (foo=bar). With this\ntri-state option, we might be past the point of fair obviousness.\nThen again, seeing \"ff\" and \"fast forward\", it's not /that hard/ to\nfigure out.\n\nApart from that, this reads well. Of course with the usual caveat of\nthe topic being fresh on my mind, so in general, I'd be more likely to\nunderstand what it is I'm reading. ;-)\n\nMartin\n"},{"id":"381478","messageId":"CABPp-BEsRL-JipotZ2FyrXiPcry6aSAvL8e5cbOm5jrPM63j-g@mail.gmail.com","threadId":"51763","inReplyTo":"87a7btqeyx.fsf@osv.gnss.ru","subject":"Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-28T22:51:14Z","receivedAt":"2019-08-28T22:51:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 28, 2019 at 12:15 PM Sergey Organov wrote:\n>\n> Hi,\n>\n[...]\n> Dunno if it helps, but here is what I came up with somewhere in previous\n> discussions:\n>\n> --ff::\n> --no-ff::\n> --ff-only::\n> When the merge resolves as a fast-forward, only update the\n\nI think this loose wording (that you just took from the original) is\nproblematic. Saying that a \"merge resolves as a fast-forward\" seems\nto imply that there are circumstances when a fast-forward is the only\noption. An _individual_ can decide to resolve a merge as a\nfast-forward in some circumstances, but it's certainly not the only\nchoice in any circumstance. If you want to keep this wording short,\nyou could replace \"resolves\" with \"can be resolved\".\n\n> branch pointer (without creating a merge commit). When a fast\n\nOnly update the branch pointer to what? (Yes, I know the original\ntext we were improving left this unclear, but it's worth noting.)\n\n> forward update is not possible, create a merge commit. This is\n> the default behavior, unless merging an annotated (and possibly\n> signed) tag that is not stored in its natural place in\n> 'refs/tags/' hierarchy, in which case --no-ff is assumed.\n\nMaybe it's just me, but I think it takes extra human cycles to figure\nout that this paragraph is referring just to the --ff case, and that\nusers might not be able to do so until after reading the next 2-3\nsentences. While more brief, I think it will cause people to need to\nread the description for these three options twice, removing most the\nsavings from being shorter. It'd be better if it could be re-worded\nto not need re-reads.\n\n> +\n> With --no-ff create a merge commit even when the merge could instead\n> resolve as a fast-forward.\n> +\n> With --ff-only resolve the merge as a fast-forward (never create a merge\n> commit). When fast-forward is not possible, refuse to merge and exit\n> with non-zero status.\n\nSomething else I was trying to address with my patch that perhaps you\ncan see a different way to tackle: Using the wording \"when possible\"\nis probably going to make users wonder when a fast forward is\npossible; the \"can be resolved\" wording tweak also makes it more\nlikely they will wonder about this. Another question they will be\nwondering about is what a fast forward is (which you partially\nexplain). Some basic knowledge of both are probably very useful in\nhelping them decide which option to actually pick. As such, I think\ntrying to explain the answers to these sub-questions will assist them\nin knowing which option to use. Simply inserting a couple phrases\n(e.g. \"when the merged branch contains the current branch in its\nhistory\", and \"only update the branch pointer *to match the merged\nbranch* and do not create a merge commit\") may help a lot.\n\nAnyway, I'll send a v3 addressing Martin's comments; if you've got\nfurther suggestions for streamlining or rearranging, though, please do\nsend them along.\n"},{"id":"381479","messageId":"20190828225712.20041-1-newren@gmail.com","threadId":"51763","inReplyTo":"CAN0heSoqy4sCY8NUWKuEkXwe2XxnYAN6Mn2N75hYwfQ_5WGYdQ@mail.gmail.com","subject":"[PATCH v3] merge-options.txt: clarify meaning of various ff-related options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-28T22:57:12Z","receivedAt":"2019-08-28T22:57:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"As discovered on the mailing list, some of the descriptions of the\nff-related options were unclear. Try to be more precise with what these\noptions do.\n\nSigned-off-by: Elijah Newren \n---\nChanges since v2:\n * reordered the options\n * typeset the option flags differently to ensure they are monospace in\n the rendered documentation\n\n Documentation/merge-options.txt | 30 ++++++++++++++++++------------\n 1 file changed, 18 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 79a00d2a4a..ed3804650b 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -40,20 +40,26 @@ set to `no` at the beginning of them.\n \tcase of a merge conflict.\n \n --ff::\n-\tWhen the merge resolves as a fast-forward, only update the branch\n-\tpointer, without creating a merge commit. This is the default\n-\tbehavior.\n-\n --no-ff::\n-\tCreate a merge commit even when the merge resolves as a\n-\tfast-forward. This is the default behaviour when merging an\n-\tannotated (and possibly signed) tag that is not stored in\n-\tits natural place in 'refs/tags/' hierarchy.\n-\n --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+\tWhether to prefer resolving the merge as a fast forward (only\n+\tupdating the branch pointer to match the merged branch and not\n+\tcreating a merge commit), to never allow it (always creating a\n+\tmerge commit), or to only allow fast forward updates. The\n+\tdefault is `--ff`, except when merging an annotated (and\n+\tpossibly signed) tag that is not stored in its natural place\n+\tin the `refs/tags/` hierarchy (in which case `--no-ff` is\n+\tassumed).\n++\n+With `--ff`, resolve the merge as a fast-forward when possible (when the\n+merged branch contains the current branch in its history). When not\n+possible, create a merge commit.\n++\n+With `--no-ff`, create a merge commit in all cases, even when the merge\n+could instead be resolved as a fast-forward.\n++\n+With `--ff-only`, resolve the merge as a fast-forward when possible.\n+When not possible, refuse to merge and exit with a non-zero status.\n \n -S[]::\n --gpg-sign[=]::\n-- \n2.23.0.3.g93492fb5e6.dirty\n\n"},{"id":"381498","messageId":"874l20nxic.fsf@osv.gnss.ru","threadId":"51763","inReplyTo":"CABPp-BEsRL-JipotZ2FyrXiPcry6aSAvL8e5cbOm5jrPM63j-g@mail.gmail.com","subject":"Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2019-08-29T09:15:07Z","receivedAt":"2019-08-29T09:15:13Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Elijah Newren writes:\n\n> On Wed, Aug 28, 2019 at 12:15 PM Sergey Organov wrote:\n>>\n>> Hi,\n>>\n> [...]\n>> Dunno if it helps, but here is what I came up with somewhere in previous\n>> discussions:\n>>\n>> --ff::\n>> --no-ff::\n>> --ff-only::\n>> When the merge resolves as a fast-forward, only update the\n>\n> I think this loose wording (that you just took from the original) is\n> problematic. Saying that a \"merge resolves as a fast-forward\" seems\n> to imply that there are circumstances when a fast-forward is the only\n> option. An _individual_ can decide to resolve a merge as a\n> fast-forward in some circumstances, but it's certainly not the only\n> choice in any circumstance. If you want to keep this wording short,\n> you could replace \"resolves\" with \"can be resolved\".\n>\n>> branch pointer (without creating a merge commit). When a fast\n>\n> Only update the branch pointer to what? (Yes, I know the original\n> text we were improving left this unclear, but it's worth noting.)\n>\n>> forward update is not possible, create a merge commit. This is\n>> the default behavior, unless merging an annotated (and possibly\n>> signed) tag that is not stored in its natural place in\n>> 'refs/tags/' hierarchy, in which case --no-ff is assumed.\n>\n> Maybe it's just me, but I think it takes extra human cycles to figure\n> out that this paragraph is referring just to the --ff case, and that\n> users might not be able to do so until after reading the next 2-3\n> sentences. While more brief, I think it will cause people to need to\n> read the description for these three options twice, removing most the\n> savings from being shorter. It'd be better if it could be re-worded\n> to not need re-reads.\n>\n>> +\n>> With --no-ff create a merge commit even when the merge could instead\n>> resolve as a fast-forward.\n>> +\n>> With --ff-only resolve the merge as a fast-forward (never create a merge\n>> commit). When fast-forward is not possible, refuse to merge and exit\n>> with non-zero status.\n>\n> Something else I was trying to address with my patch that perhaps you\n> can see a different way to tackle: Using the wording \"when possible\"\n> is probably going to make users wonder when a fast forward is\n> possible; the \"can be resolved\" wording tweak also makes it more\n> likely they will wonder about this. Another question they will be\n> wondering about is what a fast forward is (which you partially\n> explain). Some basic knowledge of both are probably very useful in\n> helping them decide which option to actually pick. As such, I think\n> trying to explain the answers to these sub-questions will assist them\n> in knowing which option to use. Simply inserting a couple phrases\n> (e.g. \"when the merged branch contains the current branch in its\n> history\", and \"only update the branch pointer *to match the merged\n> branch* and do not create a merge commit\") may help a lot.\n>\n> Anyway, I'll send a v3 addressing Martin's comments; if you've got\n> further suggestions for streamlining or rearranging, though, please do\n> send them along.\n\nThanks for thorough reply!\n\nMy version was meant to show how to re-arrange the description\npreserving original wording as much as possible, so your version should\nbe better, as it addresses other problems as well.\n\n-- \nSergey\n"},{"id":"381500","messageId":"87zhjsmi0k.fsf@osv.gnss.ru","threadId":"51763","inReplyTo":"CAN0heSpcFgNwBKHzUjcvb0W46LQwGoOSpihLg1arOPJJKC9PTQ@mail.gmail.com","subject":"Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2019-08-29T09:35:07Z","receivedAt":"2019-08-29T09:35:12Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Martin Ågren writes:\n\n> Hiya,\n\nThis one was new to me :-)\n\n>\n> On Wed, 28 Aug 2019 at 21:15, Sergey Organov wrote:\n>> > I was sort of expecting these to be listed in the order \"--ff, --no-ff,\n>> > --ff-only\", and I see Sergey suggested the same ordering. The way your\n>> > proposed text reads does make sense though... Would it read as well\n>> > turning it over and going through the options in the other order? That's\n>> > the way it is before your patch, so you could argue \"but people don't\n>> > grok that!\". What your patch does well is to offer an overview before\n>> > describing each option in a bit more detail. Would that work with the\n>> > reversed direction as well (compared to this patch)? Dunno.\n>\n>> Dunno if it helps, but here is what I came up with somewhere in previous\n>> discussions:\n>>\n>> --ff::\n>> --no-ff::\n>> --ff-only::\n>> When the merge resolves as a fast-forward, only update the\n>> branch pointer (without creating a merge commit). When a fast\n>> forward update is not possible, create a merge commit. This is\n>> the default behavior, unless merging an annotated (and possibly\n>> signed) tag that is not stored in its natural place in\n>> 'refs/tags/' hierarchy, in which case --no-ff is assumed.\n>> +\n>> With --no-ff create a merge commit even when the merge could instead\n>> resolve as a fast-forward.\n>> +\n>> With --ff-only resolve the merge as a fast-forward (never create a merge\n>> commit). When fast-forward is not possible, refuse to merge and exit\n>> with non-zero status.\n>\n> I think this could make clear that the first paragraph talks about\n> `--ff`. I know that we often list `--foo` and `--no-foo`, say \"This\n> option does bar\" and leave it to the reader to figure out which is which.\n> Most of the time, that's fairly obvious though (foo=bar). With this\n> tri-state option, we might be past the point of fair obviousness.\n> Then again, seeing \"ff\" and \"fast forward\", it's not /that hard/ to\n> figure out.\n\nYeah, I've noticed that myself, but decided to keep it, as this was\nmeant to be a quick re-arrangement of already existing description, and\nbe as short as at all possible, as lengthy descriptions tend to obscure\nproblems.\n\nBeing this way it shows how difficult it is to document flawed design. I\nguess the author of --ff-only would think twice should --ff/--no-ff be\noriginally documented similarly to the rest of 2-way options. At least\nhe'd likely call it --only-ff, as:\n\n--ff::\n--no-ff::\n--only-ff::\n\nlooks much better. Yeah, I do realize all this is historical and such...\n\n>\n> Apart from that, this reads well. Of course with the usual caveat of\n> the topic being fresh on my mind, so in general, I'd be more likely to\n> understand what it is I'm reading. ;-)\n\n... and I'm in even worse position here ;-)\n\n-- \nSergey\n"},{"id":"381603","messageId":"xmqqef12xwr4.fsf@gitster-ct.c.googlers.com","threadId":"51763","inReplyTo":"CAN0heSoqy4sCY8NUWKuEkXwe2XxnYAN6Mn2N75hYwfQ_5WGYdQ@mail.gmail.com","subject":"Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-30T19:45:51Z","receivedAt":"2019-08-30T19:45:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n>> +--ff::\n>> + Whether to only allow resolving the merge as a fast forward\n>> + (only updating the branch pointer to match the merged branch\n>> + and not creating a merge commit), to never allow it (always\n>> + creating a merge commit), or to prefer it when possible. The\n>> + default is --ff, except when merging an annotated (and\n>\n> It would be great if you'd write this as `--ff` so that it got\n> monospaced in the rendered documentation. ...\n> I'd also write `refs/tags/`, but I realize that you're just inheriting\n> what is already here. ...\n\nThese comments may sound nitpicky at times, and some parts may be\nbeyond the scope of the discussion on the patch and are better left\nfor a later time, but the consistency is valuable.\n\nThanks for mentioning them; and it would be appreciated if these\nsuggesions are followed through after the dust settles.\n"},{"id":"381607","messageId":"CABPp-BGsPEikU=KZgZgMbSLFn7Cf+fy0L==w8m4Wk95BnpHj+Q@mail.gmail.com","threadId":"51763","inReplyTo":"xmqqef12xwr4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-30T19:48:59Z","receivedAt":"2019-08-30T19:49:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 30, 2019 at 12:45 PM Junio C Hamano wrote:\n>\n> Martin Ågren writes:\n>\n> >> +--ff::\n> >> + Whether to only allow resolving the merge as a fast forward\n> >> + (only updating the branch pointer to match the merged branch\n> >> + and not creating a merge commit), to never allow it (always\n> >> + creating a merge commit), or to prefer it when possible. The\n> >> + default is --ff, except when merging an annotated (and\n> >\n> > It would be great if you'd write this as `--ff` so that it got\n> > monospaced in the rendered documentation. ...\n> > I'd also write `refs/tags/`, but I realize that you're just inheriting\n> > what is already here. ...\n>\n> These comments may sound nitpicky at times, and some parts may be\n> beyond the scope of the discussion on the patch and are better left\n> for a later time, but the consistency is valuable.\n>\n> Thanks for mentioning them; and it would be appreciated if these\n> suggesions are followed through after the dust settles.\n\nI'm confused; these suggestions were incorporated into V3 already. Am\nI misunderstanding something here?\n\n(But yes, I agree that Martin's reviews are valuable.)\n"},{"id":"381609","messageId":"xmqq7e6uxw7r.fsf@gitster-ct.c.googlers.com","threadId":"51763","inReplyTo":"20190828225712.20041-1-newren@gmail.com","subject":"Re: [PATCH v3] merge-options.txt: clarify meaning of various ff-related options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-30T19:57:28Z","receivedAt":"2019-08-30T19:57:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> index 79a00d2a4a..ed3804650b 100644\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -40,20 +40,26 @@ set to `no` at the beginning of them.\n> \tcase of a merge conflict.\n> \n> --ff::\n> --no-ff::\n> --ff-only::\n> +\tWhether to prefer resolving the merge as a fast forward (only\n> +\tupdating the branch pointer to match the merged branch and not\n> +\tcreating a merge commit), to never allow it (always creating a\n> +\tmerge commit), or to only allow fast forward updates. The\n> +\tdefault is `--ff`, except when merging an annotated (and\n> +\tpossibly signed) tag that is not stored in its natural place\n> +\tin the `refs/tags/` hierarchy (in which case `--no-ff` is\n> +\tassumed).\n> ++\n> +With `--ff`, resolve the merge as a fast-forward when possible (when the\n> +merged branch contains the current branch in its history). When not\n> +possible, create a merge commit.\n> ++\n> +With `--no-ff`, create a merge commit in all cases, even when the merge\n> +could instead be resolved as a fast-forward.\n> ++\n> +With `--ff-only`, resolve the merge as a fast-forward when possible.\n> +When not possible, refuse to merge and exit with a non-zero status.\n\nI cannot shake the feeling that the above redundantly repeats the\nsame thing twice.\n\nIf we want to dedicate one paragraph for each of these options, we\ncan and should make the introductory paragraph lighter by saying\nsomething like\n\n\tSpecifies how a merge is handled when the merged-in history\n\tis already a descendant of the current history. `--ff` is\n\tthe default unless merging an annotated or signed tag that\n\tis not stored in the `refs/tags/` hierarchy, in which case\n\t`--no-ff` is the default.\n\nAlternatively, we could sprinkle the actual option name in the first\nparagraph and drop the last three paragraphs, while fattening the\ndescription as necessary, e.g.\n\n\tWhether to prefer resolving the merge as a fast-forward and\n\tupdate the branch pointer to match the merged branch without\n\tcreating an extra merge commit (`--ff`), never allow fast-forward\n\tand always creating an extra merge commit (`--no-ff`), or to\n\tonly allow fast forward updates and reject when a merge\n\tcommit needs to be created (`--ff-only`). The default is ...\n\nI think either approach shown above would reduce the redundancy. I\ndo not care too deeply which one of these approaches is used myself,\nbut the redundancy feels a bit disturbing.\n\nThanks.\n"},{"id":"381613","messageId":"CAPig+cRPGY4wEbUin3mDj0+nHigtndNWj6o-FHjKeCzgqY9Y5Q@mail.gmail.com","threadId":"51763","inReplyTo":"xmqq7e6uxw7r.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] merge-options.txt: clarify meaning of various ff-related options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-08-30T20:16:30Z","receivedAt":"2019-08-30T20:16:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 30, 2019 at 3:57 PM Junio C Hamano wrote:\n> If we want to dedicate one paragraph for each of these options, we\n> can and should make the introductory paragraph lighter by saying\n> something like\n>\n> Specifies how a merge is handled when the merged-in history\n> is already a descendant of the current history. `--ff` is\n> the default unless merging an annotated or signed tag that\n> is not stored in the `refs/tags/` hierarchy, in which case\n> `--no-ff` is the default.\n>\n> Alternatively, we could sprinkle the actual option name in the first\n> paragraph and drop the last three paragraphs, while fattening the\n> description as necessary, e.g.\n>\n> Whether to prefer resolving the merge as a fast-forward and\n> update the branch pointer to match the merged branch without\n> creating an extra merge commit (`--ff`), never allow fast-forward\n> and always creating an extra merge commit (`--no-ff`), or to\n> only allow fast forward updates and reject when a merge\n> commit needs to be created (`--ff-only`). The default is ...\n>\n> I think either approach shown above would reduce the redundancy. I\n> do not care too deeply which one of these approaches is used myself,\n> but the redundancy feels a bit disturbing.\n\nI have not been paying close attention to this thread, but upon\nreading your suggested rewrites, I find the lighter paragraph (the\nfirst of your options), followed by the three short paragraphs -- each\ndedicated to a distinct option -- easier to follow and grok. I think\nthat's because the lighter/shorter arrangement keeps the three cases\nreasonably separate -- thus the reader is able to absorb and\nunderstand each distinct option in isolation -- rather than having to\nmanually pluck out the meaning of each option from one long, run-on\nsentence.\n"},{"id":"381615","messageId":"xmqqef12wg9b.fsf@gitster-ct.c.googlers.com","threadId":"51763","inReplyTo":"CABPp-BGsPEikU=KZgZgMbSLFn7Cf+fy0L==w8m4Wk95BnpHj+Q@mail.gmail.com","subject":"Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-30T20:27:28Z","receivedAt":"2019-08-30T20:27:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Fri, Aug 30, 2019 at 12:45 PM Junio C Hamano wrote:\n>>\n>> Martin Ågren writes:\n>>\n>> >> +--ff::\n>> >> + Whether to only allow resolving the merge as a fast forward\n>> >> + (only updating the branch pointer to match the merged branch\n>> >> + and not creating a merge commit), to never allow it (always\n>> >> + creating a merge commit), or to prefer it when possible. The\n>> >> + default is --ff, except when merging an annotated (and\n>> >\n>> > It would be great if you'd write this as `--ff` so that it got\n>> > monospaced in the rendered documentation. ...\n>> > I'd also write `refs/tags/`, but I realize that you're just inheriting\n>> > what is already here. ...\n>>\n>> These comments may sound nitpicky at times, and some parts may be\n>> beyond the scope of the discussion on the patch and are better left\n>> for a later time, but the consistency is valuable.\n>>\n>> Thanks for mentioning them; and it would be appreciated if these\n>> suggesions are followed through after the dust settles.\n>\n> I'm confused; these suggestions were incorporated into V3 already. Am\n> I misunderstanding something here?\n\nI was assuming that outside the context of the patch there also are\nthe same malformatted reference to refs/blah and --option \"you're\njust inheriting\", which would be cleaned up outside of this topic\nafter the dust settles, if the suggestions are followed through.\n\n\n\n"},{"id":"381618","messageId":"20190831002313.8761-1-newren@gmail.com","threadId":"51763","inReplyTo":"CAPig+cRPGY4wEbUin3mDj0+nHigtndNWj6o-FHjKeCzgqY9Y5Q@mail.gmail.com","subject":"[PATCH v4] merge-options.txt: clarify meaning of various ff-related options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-31T00:23:13Z","receivedAt":"2019-08-31T00:23:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"As discovered on the mailing list, some of the descriptions of the\nff-related options were unclear. Try to be more precise with what these\noptions do.\n\nSigned-off-by: Elijah Newren \n---\nChanges since v3:\n * Shorten the leading paragraph, as suggested by Junio (and Eric).\n\n Documentation/merge-options.txt | 28 ++++++++++++++++------------\n 1 file changed, 16 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 79a00d2a4a..94be34f941 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -40,20 +40,24 @@ set to `no` at the beginning of them.\n \tcase of a merge conflict.\n \n --ff::\n-\tWhen the merge resolves as a fast-forward, only update the branch\n-\tpointer, without creating a merge commit. This is the default\n-\tbehavior.\n-\n --no-ff::\n-\tCreate a merge commit even when the merge resolves as a\n-\tfast-forward. This is the default behaviour when merging an\n-\tannotated (and possibly signed) tag that is not stored in\n-\tits natural place in 'refs/tags/' hierarchy.\n-\n --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+\tSpecifies how a merge is handled when the merged-in history is\n+\talready a descendant of the current history. `--ff` is the\n+\tdefault unless merging an annotated (and possibly signed) tag\n+\tthat is not stored in its natural place in the `refs/tags/`\n+\thierarchy, in which case `--no-ff` is assumed.\n++\n+With `--ff`, when possible resolve the merge as a fast-forward (only\n+update the branch pointer to match the merged branch; do not create a\n+merge commit). When not possible (when the merged-in history is not a\n+descendant of the current history), create a merge commit.\n++\n+With `--no-ff`, create a merge commit in all cases, even when the merge\n+could instead be resolved as a fast-forward.\n++\n+With `--ff-only`, resolve the merge as a fast-forward when possible.\n+When not possible, refuse to merge and exit with a non-zero status.\n \n -S[]::\n --gpg-sign[=]::\n-- \n2.23.0.39.gf92d9de5c3\n\n"}]}