| Message ID | 20201214202647.3340193-1-gitster@pobox.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | making pull advice not to trigger when unneeded | expand |
Junio C Hamano wrote: > Just to show what was pushed out on 'seen' while I was cutting the > preview release... > > This is based on Felipe's v6 (which was mislabled as v5 but sent on > a different day from the true v5), with two clean-up commits > inserted between Felipe's second step (now 2/5) and the third step > (now 5/5, with necessary adjustments). > > Even with the "correct condition" clean-up, I am not quite happy > with the clarity of the logic around there. > > I think !opt_ff does not exactly belong to the condition, if we > consider that the eventual endgame should be to stop non-ff > operation without merge or rebase by default with an error, and that > should happen in this block. The error message there should say > "unable to fast-forward; must merge or rebase", which should equally > apply to those who gave "--ff-only" explicitly, even though they may > not need the advice message. Agreed. Additionally it doesn't make sense that --ff skips the warning entirely, even though the user has not specified a merge or a rebase (as you yourself noted in a review of the test cases). > To avoid the ugly-looking "strcmp()" in the above, we may need to > adjust "--ff" (fast-forward without creating an extra merge commit) > and "--no-ff" (create an extra merge commit when the history could > be fast-forwarded) to imply "merge", though. It would automatically > make rebase_unspecified would become false. With such a tweak, we > can then simplify it further to > > - if (rebase_unspecified && !can_ff && > - (!opt_ff || !strcmp("--ff-only", opt_ff))) { > + if (rebase_unspecified && !can_ff) { Currently this is a rebase: git pull --rebase --ff With your proposed change it would be an implied merge. I think it makes sense, but it's still a change in behavior. This patch should do the trick: --- a/builtin/pull.c +++ b/builtin/pull.c @@ -973,6 +973,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (opt_rebase < 0) opt_rebase = config_get_rebase(&rebase_unspecified); + if (opt_ff && (!strcmp(opt_ff, "--ff") || !strcmp(opt_ff, "--no-ff"))) { + opt_rebase = 0; + rebase_unspecified = 0; + } + if (read_cache_unmerged()) die_resolve_conflict("pull"); Or do you mean --ff doesn't override --rebase? Therefore it's more of an internal conceptual change.
Felipe Contreras <felipe.contreras@gmail.com> writes: >> To avoid the ugly-looking "strcmp()" in the above, we may need to >> adjust "--ff" (fast-forward without creating an extra merge commit) >> and "--no-ff" (create an extra merge commit when the history could >> be fast-forwarded) to imply "merge", though. >> ... > > Or do you mean --ff doesn't override --rebase? Therefore it's more of an > internal conceptual change. I meant "--ff/--no-ff" without saying anything between merge and rebase would make merge implied. It was merely for the purpose of getting rid of !opt_ff in the condition there and such an implied merge would be one way to ensure that rebase_unspecified becomes false. "--no-ff --rebase" (in any order) would be a nonsense combination, as it asks "please create an extra merge commit even when the history fast-forwards, but by the way I do not want merge I want rebase" [*1*]. It should error out when the history fast-forwards, I think, and it probably should also error out when the history does not fast-forward, instead of rebasing. "--ff --rebase" (in any order), on the other hand, would be "if the history fast-forwards, just do so without creating an extra merge commit, by the way, I prefer rebase and not merge". If the history fast-forwards, it is OK to just fast-forward (using the merge backend if needed), as rebasing against the history that is ahead of us would mean all our unique development since we diverged from them, which is nothing, will be replayed on top of their history. If the history does not fast-forward, then the precondition of "--ff" part does not hold, so just rebasing as if "--ff" does not exist would probably be OK, too. In any case, I haven't thought the opt_ff part of the expression through. During the review of v5 3/3 (and doing v7 5/5), what I was more interested in was to ensure the verbosity option, which would control how verbose a message we should be giving, can stay independent of other conditionals, which would control if we even need to error out and/or give hints. Thanks. [Footnote] *1* Even though the current code may simply ignore --no-ff after the control is given to the rebase codepath,
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > >> To avoid the ugly-looking "strcmp()" in the above, we may need to > >> adjust "--ff" (fast-forward without creating an extra merge commit) > >> and "--no-ff" (create an extra merge commit when the history could > >> be fast-forwarded) to imply "merge", though. > >> ... > > > > Or do you mean --ff doesn't override --rebase? Therefore it's more of an > > internal conceptual change. > > I meant "--ff/--no-ff" without saying anything between merge and > rebase would make merge implied. It was merely for the purpose of > getting rid of !opt_ff in the condition there and such an implied > merge would be one way to ensure that rebase_unspecified becomes > false. I see. > "--no-ff --rebase" (in any order) would be a nonsense combination, > as it asks "please create an extra merge commit even when the > history fast-forwards, but by the way I do not want merge I want > rebase" [*1*]. It should error out when the history fast-forwards, > I think, and it probably should also error out when the history does > not fast-forward, instead of rebasing. But we shoud not imply what the user didn't say. Yes, "--no-ff --rebase" is obviously nonsense, but that's a simplification of setups the user may have, for example: git config pull.ff false git pull --rebase Here, I think the user is saying: "please do a rebase, and ignore the pull.ff configuration". But the other way would be: git config pull.rebase true git pull --no-ff Following the same logic, the user is saying: "please do a non-fast-forward [merge], and ignore the pull.rebase configuration". Either we imply the merge, or we don't. I don't think it makes sense for the code to imply the user did say --merge, and therefore don't show the advice (or in the future error out), but then continue as if the user did say --rebase. > In any case, I haven't thought the opt_ff part of the expression > through. It is tricky. The reason I think I see it more clearly is that I have explored many options, and at this point I have 27 branches with different approaches of this topic. I see the dead-ends because I literally have a branch with the test-case failure on it. But as I said in the other thread [1], it all boils down to this: 1. git -c pull.ff=only pull 2. git -c pull.ff=only pull --merge Even if you remove the opt_ff part of the expression, and the logic of the advice/error is flawless, opt_ff is still going to come bite us in the end, because it's meant to be passed to cmd_merge(), and it's still going to fail, just at a different point, with a different error message. The best way to get rid of opt_ff from the expression, is to actually completely ignore it, and leave the current behavior as it is. By adding a different configuration, the logic becomes really simple: if (!can_ff) { if (!mode && opt_verbosity >= 0) show_advice_pull_non_ff(); } And then opt_ff doesn't interact with --rebase at all, just like it is the case right now. Simple. Cheers. [1] https://lore.kernel.org/git/5fd8213598c33_d7c4820837@natae.notmuch
Felipe Contreras wrote: > Junio C Hamano wrote: > > "--no-ff --rebase" (in any order) would be a nonsense combination, > > as it asks "please create an extra merge commit even when the > > history fast-forwards, but by the way I do not want merge I want > > rebase" [*1*]. It should error out when the history fast-forwards, > > I think, and it probably should also error out when the history does > > not fast-forward, instead of rebasing. > > But we shoud not imply what the user didn't say. > > Yes, "--no-ff --rebase" is obviously nonsense, but that's a > simplification of setups the user may have, for example: > > git config pull.ff false > git pull --rebase > > Here, I think the user is saying: "please do a rebase, and ignore the > pull.ff configuration". > > But the other way would be: > > git config pull.rebase true > git pull --no-ff > > Following the same logic, the user is saying: "please do a > non-fast-forward [merge], and ignore the pull.rebase configuration". > > Either we imply the merge, or we don't. > > I don't think it makes sense for the code to imply the user did say > --merge, and therefore don't show the advice (or in the future error > out), but then continue as if the user did say --rebase. I didn't see a response to this, but after thinking more about, I think I have a clean solution: just remove all the opt_ff logic altogether. It's clear --ff doesn't imply a merge, so we shouldn't act as if it was. The warning should still be displayed.
Felipe Contreras <felipe.contreras@gmail.com> writes:
> It's clear --ff doesn't imply a merge, so we shouldn't act as if it was.
Do you specifically mean --ff, or do you talk collectively about
anything that goes in opt_ff in the C code?
The "--ff" option means "we are allowing fast-forward, so please do
not make new commit object unnecessarily, but it is just we are
allowing---we are not limiting ourselves to fast-forard; feel free
to create a merge commit if necessary". So it does imply that the
user prefers to merge and does not want to rebase.
If you meant what opt_ff can relay, then there are "--no-ff" and
"--ff-only" to consider:
- "--no-ff" says "we do not allow fast-forward; when the other side
is pure descendant of ours, create a merge commit to make them
the second parent, so that our side of the history stays to be
the first-parent chain that merged them as a side topic." It may
not say what should happen when the history does not
fast-forward, and it _is_ possible to argue, for the sake of
argument, that it asks to rebase if not fast-forward (so that
their history becomes the primary and we build on top of them)
while asking to merge if fast-forward (so that our history stays
the primary and we absorb their work as a side branch), but that
is a behavior that does not make much sense. It is much easier
to reason about if we accept that the user who says "--no-ff"
expects a merge to happen, not a rebase.
- "--ff-only" says "when their history is pure descendant of ours,
just fast-forward our branch to match their history, and
otherwise fail." This one does not have to imply either merge or
rebase, as both would give us identical result (i.e. merge would
fast-forward and rebase would replay *no* work of our own on top
of theirs. Either case, the result is that our branch tip now
points at the tip of their history).
The topic under discussion is based on the "we do not have to
give advice between merge and rebase if the history
fast-forwards", and anybody in support of the topic would be in
agreement with this case.
In any case, I think what we have in 'seen' already is a good
stopping point for this cycle. We are not erroring out any new case
and simply not showing an advice in a situation that it would not
apply---the question "does --ff imply merge?" does not have to be
answered in order to evaluate the 5-patch series we have.
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > It's clear --ff doesn't imply a merge, so we shouldn't act as if it was. > > Do you specifically mean --ff, or do you talk collectively about > anything that goes in opt_ff in the C code? I meant --ff, but the rationale can be extended to all of opt_ff. > The "--ff" option means "we are allowing fast-forward, so please do > not make new commit object unnecessarily, but it is just we are > allowing---we are not limiting ourselves to fast-forard; feel free > to create a merge commit if necessary". Yes. *If* a rebase is not specified. > So it does imply that the user prefers to merge and does not want to > rebase. We could imply that, but currently it doesn't. Currently this does not do a merge: git config pull.rebase true git pull --ff > If you meant what opt_ff can relay, then there are "--no-ff" and > "--ff-only" to consider: > > - "--no-ff" says "we do not allow fast-forward; when the other side > is pure descendant of ours, create a merge commit to make them > the second parent, so that our side of the history stays to be > the first-parent chain that merged them as a side topic." It may > not say what should happen when the history does not > fast-forward, and it _is_ possible to argue, for the sake of > argument, that it asks to rebase if not fast-forward (so that > their history becomes the primary and we build on top of them) > while asking to merge if fast-forward (so that our history stays > the primary and we absorb their work as a side branch), but that > is a behavior that does not make much sense. I agree it doesn't make much sense; if the user wants a rebase in case of non-fast-forward, --no-ff is the only way. > It is much easier to reason about if we accept that the user who > says "--no-ff" expects a merge to happen, not a rebase. Yes, but currently that's not the case. Currently this doesn't do a merge: git config pull.rebase true git pull --no-ff We would need to change the semantics. > - "--ff-only" says "when their history is pure descendant of ours, > just fast-forward our branch to match their history, and > otherwise fail." This one does not have to imply either merge or > rebase, as both would give us identical result (i.e. merge would > fast-forward and rebase would replay *no* work of our own on top > of theirs. Either case, the result is that our branch tip now > points at the tip of their history). > > The topic under discussion is based on the "we do not have to > give advice between merge and rebase if the history > fast-forwards", and anybody in support of the topic would be in > agreement with this case. Yes. > In any case, I think what we have in 'seen' already is a good > stopping point for this cycle. It's not a bad stopping point. But the next patches are needed too. Up to the first 6 patches should be uncontroversial. > We are not erroring out any new case and simply not showing an advice > in a situation that it would not apply---the question "does --ff imply > merge?" does not have to be answered in order to evaluate the 5-patch > series we have. Not my patches. The patch you introduced regarding rebase_unspecified does depend on what happens next. If we decide to change the semantics of --ff* and imply a merge, then my patch to add REBASE_DEFAULT is needed, and as you can see in another patch series [1], that basically has to revert your patch. Cheers. [1] https://lore.kernel.org/git/20201218211026.1937168-8-felipe.contreras@gmail.com/
Just to show what was pushed out on 'seen' while I was cutting the preview release... This is based on Felipe's v6 (which was mislabled as v5 but sent on a different day from the true v5), with two clean-up commits inserted between Felipe's second step (now 2/5) and the third step (now 5/5, with necessary adjustments). Even with the "correct condition" clean-up, I am not quite happy with the clarity of the logic around there. I think !opt_ff does not exactly belong to the condition, if we consider that the eventual endgame should be to stop non-ff operation without merge or rebase by default with an error, and that should happen in this block. The error message there should say "unable to fast-forward; must merge or rebase", which should equally apply to those who gave "--ff-only" explicitly, even though they may not need the advice message. So with the help of can_ff bit introduced by 5/5, I _think_ this part should become like - if (rebase_unspecified && !opt_ff && !can_ff) { + if (rebase_unspecified && !can_ff && + (!opt_ff || !strcmp("--ff-only", opt_ff))) { before/when we make this codepath error out. We won't hit the body of this if statement when we can fast-forward. To avoid the ugly-looking "strcmp()" in the above, we may need to adjust "--ff" (fast-forward without creating an extra merge commit) and "--no-ff" (create an extra merge commit when the history could be fast-forwarded) to imply "merge", though. It would automatically make rebase_unspecified would become false. With such a tweak, we can then simplify it further to - if (rebase_unspecified && !can_ff && - (!opt_ff || !strcmp("--ff-only", opt_ff))) { + if (rebase_unspecified && !can_ff) { But these are not part of this round. Felipe Contreras (3): pull: refactor fast-forward check pull: give the advice for choosing rebase/merge much later pull: display default warning only when non-ff Junio C Hamano (2): pull: get rid of unnecessary global variable pull: correct condition to trigger non-ff advice builtin/pull.c | 70 ++++++++++++++++++++++-------------- t/t7601-merge-pull-config.sh | 66 +++++++++++++++++++++++++++++++--- 2 files changed, 104 insertions(+), 32 deletions(-)