* [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command @ 2019-10-25 22:20 Mihail Atanassov 2019-10-26 2:26 ` Jonathan Nieder 0 siblings, 1 reply; 6+ messages in thread From: Mihail Atanassov @ 2019-10-25 22:20 UTC (permalink / raw) To: git; +Cc: Mihail Atanassov 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@gmail.com> --- Documentation/git-bisect.txt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 -if git merge --no-commit hot-fix && +# 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. +if git merge --no-commit --no-ff hot-fix && make then # run project specific test and report its status -- 2.16.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command 2019-10-25 22:20 [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command Mihail Atanassov @ 2019-10-26 2:26 ` Jonathan Nieder [not found] ` <CALs020+0E=7wy-N46BRLrBcKmMSTpcMyZ9WybmgTzb60aCo5PQ@mail.gmail.com> 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Nieder @ 2019-10-26 2:26 UTC (permalink / raw) To: Mihail Atanassov; +Cc: git 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@gmail.com> > --- > 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". 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CALs020+0E=7wy-N46BRLrBcKmMSTpcMyZ9WybmgTzb60aCo5PQ@mail.gmail.com>]
* Re: [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command [not found] ` <CALs020+0E=7wy-N46BRLrBcKmMSTpcMyZ9WybmgTzb60aCo5PQ@mail.gmail.com> @ 2019-10-28 22:10 ` Mihail Atanassov 2019-10-28 22:24 ` Jonathan Nieder 0 siblings, 1 reply; 6+ messages in thread From: Mihail Atanassov @ 2019-10-28 22:10 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git (Cc git@vger.kernel.org) On Mon, 28 Oct 2019 at 21:51, Mihail Atanassov <m.atanassov92@gmail.com> 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@gmail.com> 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@gmail.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command 2019-10-28 22:10 ` Mihail Atanassov @ 2019-10-28 22:24 ` Jonathan Nieder 2019-10-29 2:24 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Nieder @ 2019-10-28 22:24 UTC (permalink / raw) To: Mihail Atanassov; +Cc: git Hi, Mihail Atanassov wrote: > 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... Interesting. https://support.google.com/mail/thread/11736136 tells me there's an issue with Kmail's oauth support. You might want to get in touch with the Kmail authors, or, as a fallback, use an application specific password or other mail client. [...] > On Sat, 26 Oct 2019 at 03:26, Jonathan Nieder <jrnieder@gmail.com> wrote: >> Hmm. I think the comment might put a bit too much emphasis on the >> "how" instead of the "why". [...] >> 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). Thanks, that sounds good to me. As an orthogonal point, I wonder whether we can start the multi-step migration of making --no-commit imply --no-ff by default: 1. Act as --ff when --no-commit is passed without --ff or --no-ff (the state today) 2. Warn when performing a fast-forward merge and --no-commit was passed without --ff or --no-ff 3. Error out instead of performing a fast-forward merge when --no-commit is passed without --ff or --no-ff 4. Warn and refuse to perform a fast-forward merge when --no-commit is passed without --ff or --no-ff 5. Refuse to perform a fast-forward merge with --no-commit is passed without --ff or --no-ff, just as though --no-ff were passed. (A config setting could allow people to get the futuristic behavior early. And it might be possible to skip some steps. :)) [...] >>> -if git merge --no-commit hot-fix && >>> +if git merge --no-commit --no-ff hot-fix && >> >> Good. This part still looks like a good change to me. :) Sincerely, Jonathan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command 2019-10-28 22:24 ` Jonathan Nieder @ 2019-10-29 2:24 ` Junio C Hamano 2019-10-29 3:25 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2019-10-29 2:24 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Mihail Atanassov, git Jonathan Nieder <jrnieder@gmail.com> writes: > As an orthogonal point, I wonder whether we can start the multi-step > migration of making --no-commit imply --no-ff by default: > > 1. Act as --ff when --no-commit is passed without --ff or --no-ff > (the state today) which means "--no-commit controls whether a new commit is created or not and nothing else, and because --ff is the default for merge, merging a true descendant will fast-forward". > 5. Refuse to perform a fast-forward merge with --no-commit is passed > without --ff or --no-ff, just as though --no-ff were passed. Is that a good endgame, though? It is correct that "--no-ff" means "do not allow the merge to be fast-forwarded and the way the option does so is by creating an otherwise unnecessary merge commit", and "--no-commit" means "do not allow creating any new commit", so technically they are mutually incompatible, but would it be useful? I'd imagine that a more useful behaviour would be for "git merge X" with any other options to honor this basic trait: the working tree and the index after the operation shows the result of merging X and HEAD, if the merge can cleanly be made, and otherwise the working tree and the index would show something close to the result of such a merge with conflicts that would help recording the result of the merge manually. For that, wouldn't it make more sense ot change the semantics of the "--no-commit" option from "no new commit gets created" to "HEAD is not moved"? "git merge --no-commit X" when X is a descendant of HEAD would then become "git read-tree -m -u HEAD X" plus perhaps storing X in .git/MERGE_HEAD file etc. to prepare for concluding "git commit" to record the result manually. In any case, as you said, >>>> -if git merge --no-commit hot-fix && >>>> +if git merge --no-commit --no-ff hot-fix && >>> >>> Good. > > This part still looks like a good change to me. :) This looks good to me too. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command 2019-10-29 2:24 ` Junio C Hamano @ 2019-10-29 3:25 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2019-10-29 3:25 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Mihail Atanassov, git Junio C Hamano <gitster@pobox.com> writes: > Jonathan Nieder <jrnieder@gmail.com> writes: > >> As an orthogonal point, I wonder whether we can start the multi-step >> migration of making --no-commit imply --no-ff by default: >> >> 1. Act as --ff when --no-commit is passed without --ff or --no-ff >> (the state today) > > which means "--no-commit controls whether a new commit is created or > not and nothing else, and because --ff is the default for merge, > merging a true descendant will fast-forward". > >> 5. Refuse to perform a fast-forward merge with --no-commit is passed >> without --ff or --no-ff, just as though --no-ff were passed. > > Is that a good endgame, though? Ah, I was confused by "refuse to perform". You were not trying to make the command fail outright without doing anything. Yes, that would be a good endgame, I would think. I am not sure if the transition would be smooth, though. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-29 3:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-25 22:20 [PATCH] Documentation/git-bisect.txt: add --no-ff to merge command Mihail Atanassov 2019-10-26 2:26 ` Jonathan Nieder [not found] ` <CALs020+0E=7wy-N46BRLrBcKmMSTpcMyZ9WybmgTzb60aCo5PQ@mail.gmail.com> 2019-10-28 22:10 ` Mihail Atanassov 2019-10-28 22:24 ` Jonathan Nieder 2019-10-29 2:24 ` Junio C Hamano 2019-10-29 3:25 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.