From: Felipe Contreras <felipe.contreras@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
Felipe Contreras <felipe.contreras@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded
Date: Tue, 15 Dec 2020 06:22:02 -0600 [thread overview]
Message-ID: <5fd8aa6a52e81_190cd7208c8@natae.notmuch> (raw)
In-Reply-To: <xmqqmtyf8hfm.fsf@gitster.c.googlers.com>
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
next prev parent reply other threads:[~2020-12-15 12:23 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-14 20:26 [PATCH v7 0/5] making pull advice not to trigger when unneeded Junio C Hamano
2020-12-14 20:26 ` [PATCH v7 1/5] pull: refactor fast-forward check Junio C Hamano
2020-12-14 20:26 ` [PATCH v7 2/5] pull: give the advice for choosing rebase/merge much later Junio C Hamano
2020-12-14 20:26 ` [PATCH v7 3/5] pull: get rid of unnecessary global variable Junio C Hamano
2020-12-14 20:59 ` Felipe Contreras
2020-12-14 23:16 ` Junio C Hamano
2020-12-15 2:55 ` Felipe Contreras
2020-12-14 20:26 ` [PATCH v7 4/5] pull: correct condition to trigger non-ff advice Junio C Hamano
2020-12-14 21:17 ` Felipe Contreras
2020-12-14 23:19 ` Junio C Hamano
2020-12-15 6:35 ` Felipe Contreras
2020-12-14 20:26 ` [PATCH v7 5/5] pull: display default warning only when non-ff Junio C Hamano
2020-12-14 21:24 ` Felipe Contreras
2020-12-14 23:20 ` Junio C Hamano
2020-12-15 2:57 ` Felipe Contreras
2020-12-15 6:30 ` [PATCH v7 0/5] making pull advice not to trigger when unneeded Felipe Contreras
2020-12-15 10:58 ` Junio C Hamano
2020-12-15 12:22 ` Felipe Contreras [this message]
2020-12-18 20:46 ` Felipe Contreras
2020-12-23 10:04 ` Junio C Hamano
2020-12-23 14:10 ` Felipe Contreras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5fd8aa6a52e81_190cd7208c8@natae.notmuch \
--to=felipe.contreras@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).