{"thread":{"id":"52123","subject":"[PATCH] Documentation/git-bisect.txt: add --no-ff to merge command","startedAt":"2019-10-25T22:21:02Z","lastAt":"2019-10-29T03:25:48Z","messageCount":6,"participants":["Mihail Atanassov","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"384891","messageId":"20191025222032.3399-1-m.atanassov92@gmail.com","threadId":"52123","inReplyTo":null,"subject":"[PATCH] Documentation/git-bisect.txt: add --no-ff to merge command","fromName":"Mihail Atanassov","fromEmail":"m.atanassov92@gmail.com","sentAt":"2019-10-25T22:20:32Z","receivedAt":"2019-10-25T22:21:02Z","isPatch":true,"sender":{"key":"m.atanassov92@gmail.com","avatar":null},"body":"The hotfix application example uses `git merge --no-commit` to apply\ntemporary changes to the working tree during a bisect operation. In some\nsituations this can be a fast-forward and `merge` will apply the hotfix\nbranch's commits regardless of `--no-commit` (as documented in the `git\nmerge` manual).\n\nIn the pathological case this will make a `git bisect\nrun` invocation to loop indefinitely between the first bisect step and\nthe fast-forwarded post-merge HEAD.\n\nAdd `--no-ff` to the merge command to avoid this issue, and make a note\nof it for the reader.\n\nSigned-off-by: Mihail Atanassov \n---\n Documentation/git-bisect.txt | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt\nindex 4b45d837a7..58b5585874 100644\n--- a/Documentation/git-bisect.txt\n+++ b/Documentation/git-bisect.txt\n@@ -412,8 +412,10 @@ $ cat ~/test.sh\n #!/bin/sh\n \n # tweak the working tree by merging the hot-fix branch\n-# and then attempt a build\n-if\tgit merge --no-commit hot-fix &&\n+# and then attempt a build. Note the `--no-ff`: `git merge`\n+# will otherwise still apply commits if the current HEAD can be\n+# fast-forwarded to the hot-fix branch.\n+if\tgit merge --no-commit --no-ff hot-fix &&\n \tmake\n then\n \t# run project specific test and report its status\n-- \n2.16.4\n\n"},{"id":"384901","messageId":"20191026022655.GF39574@google.com","threadId":"52123","inReplyTo":"20191025222032.3399-1-m.atanassov92@gmail.com","subject":"Re: [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-10-26T02:26:55Z","receivedAt":"2019-10-26T02:26:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMihail Atanassov wrote:\n\n> The hotfix application example uses `git merge --no-commit` to apply\n> temporary changes to the working tree during a bisect operation. In some\n> situations this can be a fast-forward and `merge` will apply the hotfix\n> branch's commits regardless of `--no-commit` (as documented in the `git\n> merge` manual).\n>\n> In the pathological case this will make a `git bisect\n> run` invocation to loop indefinitely between the first bisect step and\n> the fast-forwarded post-merge HEAD.\n>\n> Add `--no-ff` to the merge command to avoid this issue, and make a note\n> of it for the reader.\n>\n> Signed-off-by: Mihail Atanassov \n> ---\n> Documentation/git-bisect.txt | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n\nGood catch. Thanks for fixing it.\n\n> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt\n> index 4b45d837a7..58b5585874 100644\n> --- a/Documentation/git-bisect.txt\n> +++ b/Documentation/git-bisect.txt\n> @@ -412,8 +412,10 @@ $ cat ~/test.sh\n> #!/bin/sh\n> \n> # tweak the working tree by merging the hot-fix branch\n> -# and then attempt a build\n> +# and then attempt a build. Note the `--no-ff`: `git merge`\n> +# will otherwise still apply commits if the current HEAD can be\n> +# fast-forwarded to the hot-fix branch.\n\nHmm. I think the comment might put a bit too much emphasis on the\n\"how\" instead of the \"why\". Is it necessary to describe why --no-ff\nis used at all here? After all, a reader wondering about it is likely\nto check \"git help merge\", which says\n\n\tFast-forward updates do not create a merge commit and\n\ttherefore there is no way to stop those merges with\n\t--no-commit. Thus, if you want to ensure your branch is not\n\tchanged or updated by the merge command, use --no-ff with\n\t--no-commit.\n\nSo I'd be tempted to leave the comment ending with \"and then attempt a\nbuild\".\n\nAlternatively: the wording says \"will still apply commits\", but the\nreader might not think of a merge as applying patches (that's closer\nto what cherry-pick does. Is there some alternative wording that\nwould convey the intent more clearly?\n\n> -if\tgit merge --no-commit hot-fix &&\n> +if\tgit merge --no-commit --no-ff hot-fix &&\n\nGood.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"385020","messageId":"CALs020KFKOp4mFnY_2Mg5_H8Jc6PPV56O=1S66mriZA+XWd1Hg@mail.gmail.com","threadId":"52123","inReplyTo":"CALs020+0E=7wy-N46BRLrBcKmMSTpcMyZ9WybmgTzb60aCo5PQ@mail.gmail.com","subject":"Re: [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command","fromName":"Mihail Atanassov","fromEmail":"m.atanassov92@gmail.com","sentAt":"2019-10-28T22:10:51Z","receivedAt":"2019-10-28T22:11:06Z","isPatch":true,"sender":{"key":"m.atanassov92@gmail.com","avatar":null},"body":"(Cc git@vger.kernel.org)\n\nOn Mon, 28 Oct 2019 at 21:51, Mihail Atanassov wrote:\n>\n> Hi Jonathan,\n>\n> Thanks for the quick turnaround! And apologies in advance for the delayed\n> and potentially mangled response, I can't get into my gmail account from\n> a sensible MUA...\n>\n> On Sat, 26 Oct 2019 at 03:26, Jonathan Nieder wrote:\n> >\n> > Hi,\n> >\n> > Mihail Atanassov wrote:\n> >\n> > > The hotfix application example uses `git merge --no-commit` to apply\n> > > temporary changes to the working tree during a bisect operation. In some\n> > > situations this can be a fast-forward and `merge` will apply the hotfix\n> > > branch's commits regardless of `--no-commit` (as documented in the `git\n> > > merge` manual).\n> > >\n> > > In the pathological case this will make a `git bisect\n> > > run` invocation to loop indefinitely between the first bisect step and\n> > > the fast-forwarded post-merge HEAD.\n> > >\n> > > Add `--no-ff` to the merge command to avoid this issue, and make a note\n> > > of it for the reader.\n> > >\n> > > Signed-off-by: Mihail Atanassov \n> > > ---\n> > > Documentation/git-bisect.txt | 6 ++++--\n> > > 1 file changed, 4 insertions(+), 2 deletions(-)\n> >\n> > Good catch. Thanks for fixing it.\n> >\n> > > diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt\n> > > index 4b45d837a7..58b5585874 100644\n> > > --- a/Documentation/git-bisect.txt\n> > > +++ b/Documentation/git-bisect.txt\n> > > @@ -412,8 +412,10 @@ $ cat ~/test.sh\n> > > #!/bin/sh\n> > >\n> > > # tweak the working tree by merging the hot-fix branch\n> > > -# and then attempt a build\n> > > +# and then attempt a build. Note the `--no-ff`: `git merge`\n> > > +# will otherwise still apply commits if the current HEAD can be\n> > > +# fast-forwarded to the hot-fix branch.\n> >\n> > Hmm. I think the comment might put a bit too much emphasis on the\n> > \"how\" instead of the \"why\". Is it necessary to describe why --no-ff\n> > is used at all here? After all, a reader wondering about it is likely\n> > to check \"git help merge\", which says\n> >\n> > Fast-forward updates do not create a merge commit and\n> > therefore there is no way to stop those merges with\n> > --no-commit. Thus, if you want to ensure your branch is not\n> > changed or updated by the merge command, use --no-ff with\n> > --no-commit.\n> >\n> > So I'd be tempted to leave the comment ending with \"and then attempt a\n> > build\".\n>\n> Fair point, I actually did spend a bit of time on the fence between your\n> suggestion and what I ultimately submitted. I ended up expanding on it\n> precisely because the '--no-ff' seems a bit arbitrary to the casual observer\n> and requires cross-referencing other documentation (which is how I figured\n> out I ought to produce this patch :)).\n>\n> I can't think of any wording that would be any better, so I'll push a v2 with\n> no comment changes, and leave it to the reader's curiosity (or lack thereof).\n>\n> On a related note, if the user reads all the docs fully, they'll know to use a\n> suitable merge-base for the hotfix branch and they won't get into the\n> predicament in the first place. So this patch is hiding the underlying issue\n> slightly. I'd still prefer to have that failsafe in there, though, for the cases\n> where going into an infinite loop is costly (e.g. unattended bisect with\n> long-running tests).\n>\n> >\n> > Alternatively: the wording says \"will still apply commits\", but the\n> > reader might not think of a merge as applying patches (that's closer\n> > to what cherry-pick does. Is there some alternative wording that\n> > would convey the intent more clearly?\n> >\n> > > -if git merge --no-commit hot-fix &&\n> > > +if git merge --no-commit --no-ff hot-fix &&\n> >\n> > Good.\n> >\n> > Thanks and hope that helps,\n> > Jonathan\n>\n> --\n> Mihail\n\n-- \nMihail\n"},{"id":"385022","messageId":"20191028222405.GE12487@google.com","threadId":"52123","inReplyTo":"CALs020KFKOp4mFnY_2Mg5_H8Jc6PPV56O=1S66mriZA+XWd1Hg@mail.gmail.com","subject":"Re: [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-10-28T22:24:05Z","receivedAt":"2019-10-28T22:24:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMihail Atanassov wrote:\n\n> Thanks for the quick turnaround! And apologies in advance for the delayed\n> and potentially mangled response, I can't get into my gmail account from\n> a sensible MUA...\n\nInteresting. https://support.google.com/mail/thread/11736136 tells me\nthere's an issue with Kmail's oauth support. You might want to get in\ntouch with the Kmail authors, or, as a fallback, use an application\nspecific password or other mail client.\n\n[...]\n> On Sat, 26 Oct 2019 at 03:26, Jonathan Nieder wrote:\n\n>> Hmm. I think the comment might put a bit too much emphasis on the\n>> \"how\" instead of the \"why\".\n[...]\n>> So I'd be tempted to leave the comment ending with \"and then attempt a\n>> build\".\n>\n> Fair point, I actually did spend a bit of time on the fence between your\n> suggestion and what I ultimately submitted. I ended up expanding on it\n> precisely because the '--no-ff' seems a bit arbitrary to the casual observer\n> and requires cross-referencing other documentation (which is how I figured\n> out I ought to produce this patch :)).\n>\n> I can't think of any wording that would be any better, so I'll push a v2 with\n> no comment changes, and leave it to the reader's curiosity (or lack thereof).\n\nThanks, that sounds good to me.\n\nAs an orthogonal point, I wonder whether we can start the multi-step\nmigration of making --no-commit imply --no-ff by default:\n\n 1. Act as --ff when --no-commit is passed without --ff or --no-ff\n (the state today)\n\n 2. Warn when performing a fast-forward merge and --no-commit was\n passed without --ff or --no-ff\n\n 3. Error out instead of performing a fast-forward merge when\n --no-commit is passed without --ff or --no-ff\n\n 4. Warn and refuse to perform a fast-forward merge when --no-commit\n is passed without --ff or --no-ff\n\n 5. Refuse to perform a fast-forward merge with --no-commit is passed\n without --ff or --no-ff, just as though --no-ff were passed.\n\n(A config setting could allow people to get the futuristic behavior\nearly. And it might be possible to skip some steps. :))\n\n[...]\n>>> -if git merge --no-commit hot-fix &&\n>>> +if git merge --no-commit --no-ff hot-fix &&\n>>\n>> Good.\n\nThis part still looks like a good change to me. :)\n\nSincerely,\nJonathan\n"},{"id":"385036","messageId":"xmqq4kzs8f1l.fsf@gitster-ct.c.googlers.com","threadId":"52123","inReplyTo":"20191028222405.GE12487@google.com","subject":"Re: [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-29T02:24:22Z","receivedAt":"2019-10-29T02:24:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> As an orthogonal point, I wonder whether we can start the multi-step\n> migration of making --no-commit imply --no-ff by default:\n>\n> 1. Act as --ff when --no-commit is passed without --ff or --no-ff\n> (the state today)\n\nwhich means \"--no-commit controls whether a new commit is created or\nnot and nothing else, and because --ff is the default for merge,\nmerging a true descendant will fast-forward\".\n\n> 5. Refuse to perform a fast-forward merge with --no-commit is passed\n> without --ff or --no-ff, just as though --no-ff were passed.\n\nIs that a good endgame, though? It is correct that \"--no-ff\" means\n\"do not allow the merge to be fast-forwarded and the way the option\ndoes so is by creating an otherwise unnecessary merge commit\", and\n\"--no-commit\" means \"do not allow creating any new commit\", so\ntechnically they are mutually incompatible, but would it be useful?\n\nI'd imagine that a more useful behaviour would be for \"git merge X\"\nwith any other options to honor this basic trait: the working tree\nand the index after the operation shows the result of merging X and\nHEAD, if the merge can cleanly be made, and otherwise the working\ntree and the index would show something close to the result of such\na merge with conflicts that would help recording the result of the\nmerge manually.\n\nFor that, wouldn't it make more sense ot change the semantics of the\n\"--no-commit\" option from \"no new commit gets created\" to \"HEAD is\nnot moved\"? \"git merge --no-commit X\" when X is a descendant of\nHEAD would then become \"git read-tree -m -u HEAD X\" plus perhaps\nstoring X in .git/MERGE_HEAD file etc. to prepare for concluding\n\"git commit\" to record the result manually.\n\nIn any case, as you said,\n\n>>>> -if git merge --no-commit hot-fix &&\n>>>> +if git merge --no-commit --no-ff hot-fix &&\n>>>\n>>> Good.\n>\n> This part still looks like a good change to me. :)\n\nThis looks good to me too.\n"},{"id":"385043","messageId":"xmqq7e4o6xmw.fsf@gitster-ct.c.googlers.com","threadId":"52123","inReplyTo":"xmqq4kzs8f1l.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-29T03:25:43Z","receivedAt":"2019-10-29T03:25:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>> As an orthogonal point, I wonder whether we can start the multi-step\n>> migration of making --no-commit imply --no-ff by default:\n>>\n>> 1. Act as --ff when --no-commit is passed without --ff or --no-ff\n>> (the state today)\n>\n> which means \"--no-commit controls whether a new commit is created or\n> not and nothing else, and because --ff is the default for merge,\n> merging a true descendant will fast-forward\".\n>\n>> 5. Refuse to perform a fast-forward merge with --no-commit is passed\n>> without --ff or --no-ff, just as though --no-ff were passed.\n>\n> Is that a good endgame, though?\n\nAh, I was confused by \"refuse to perform\". You were not trying to\nmake the command fail outright without doing anything. Yes, that\nwould be a good endgame, I would think.\n\nI am not sure if the transition would be smooth, though.\n"}]}