(Cc git@xxxxxxxxxxxxxxx) On Mon, 28 Oct 2019 at 21:51, Mihail Atanassov <m.atanassov92@xxxxxxxxx> wrote: > > Hi Jonathan, > > Thanks for the quick turnaround! And apologies in advance for the delayed > and potentially mangled response, I can't get into my gmail account from > a sensible MUA... > > On Sat, 26 Oct 2019 at 03:26, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > > > > Hi, > > > > Mihail Atanassov wrote: > > > > > The hotfix application example uses `git merge --no-commit` to apply > > > temporary changes to the working tree during a bisect operation. In some > > > situations this can be a fast-forward and `merge` will apply the hotfix > > > branch's commits regardless of `--no-commit` (as documented in the `git > > > merge` manual). > > > > > > In the pathological case this will make a `git bisect > > > run` invocation to loop indefinitely between the first bisect step and > > > the fast-forwarded post-merge HEAD. > > > > > > Add `--no-ff` to the merge command to avoid this issue, and make a note > > > of it for the reader. > > > > > > Signed-off-by: Mihail Atanassov <m.atanassov92@xxxxxxxxx> > > > --- > > > Documentation/git-bisect.txt | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > Good catch. Thanks for fixing it. > > > > > diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt > > > index 4b45d837a7..58b5585874 100644 > > > --- a/Documentation/git-bisect.txt > > > +++ b/Documentation/git-bisect.txt > > > @@ -412,8 +412,10 @@ $ cat ~/test.sh > > > #!/bin/sh > > > > > > # tweak the working tree by merging the hot-fix branch > > > -# and then attempt a build > > > +# and then attempt a build. Note the `--no-ff`: `git merge` > > > +# will otherwise still apply commits if the current HEAD can be > > > +# fast-forwarded to the hot-fix branch. > > > > Hmm. I think the comment might put a bit too much emphasis on the > > "how" instead of the "why". Is it necessary to describe why --no-ff > > is used at all here? After all, a reader wondering about it is likely > > to check "git help merge", which says > > > > Fast-forward updates do not create a merge commit and > > therefore there is no way to stop those merges with > > --no-commit. Thus, if you want to ensure your branch is not > > changed or updated by the merge command, use --no-ff with > > --no-commit. > > > > So I'd be tempted to leave the comment ending with "and then attempt a > > build". > > Fair point, I actually did spend a bit of time on the fence between your > suggestion and what I ultimately submitted. I ended up expanding on it > precisely because the '--no-ff' seems a bit arbitrary to the casual observer > and requires cross-referencing other documentation (which is how I figured > out I ought to produce this patch :)). > > I can't think of any wording that would be any better, so I'll push a v2 with > no comment changes, and leave it to the reader's curiosity (or lack thereof). > > On a related note, if the user reads all the docs fully, they'll know to use a > suitable merge-base for the hotfix branch and they won't get into the > predicament in the first place. So this patch is hiding the underlying issue > slightly. I'd still prefer to have that failsafe in there, though, for the cases > where going into an infinite loop is costly (e.g. unattended bisect with > long-running tests). > > > > > Alternatively: the wording says "will still apply commits", but the > > reader might not think of a merge as applying patches (that's closer > > to what cherry-pick does. Is there some alternative wording that > > would convey the intent more clearly? > > > > > -if git merge --no-commit hot-fix && > > > +if git merge --no-commit --no-ff hot-fix && > > > > Good. > > > > Thanks and hope that helps, > > Jonathan > > -- > Mihail -- Mihail
