git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Elijah Newren <newren@gmail.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Alex Henrie <alexhenrie24@gmail.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Son Luong Ngoc <sluongng@gmail.com>
Subject: Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins
Date: Fri, 16 Jul 2021 14:18:01 -0700	[thread overview]
Message-ID: <xmqqwnpqot4m.fsf@gitster.g> (raw)
In-Reply-To: <CABPp-BEHNpfm6mJBDZ30wCEraT03p+2-gGZyZcnUV34trAzAzw@mail.gmail.com> (Elijah Newren's message of "Fri, 16 Jul 2021 11:39:50 -0700")

Elijah Newren <newren@gmail.com> writes:

> the dozen+ recent messages in the various recent threads, I think I've
> figured out your opinion in all but two cases, but I have no idea your
> intent on those two (I would have thought --rebase override there too,
> but you excluded that), and I'm rather uncertain I've correctly
> understood you for the other ones (I really hope gmail doesn't
> whitespace damage the following table):

Yeah, it's about time somebody made an exhaustive table of cases ;-)

I appreciate it very much.  Without such a table, it is hard to have
discussion and keep track of which part is and which part is not
what people agree to.

Below, I rearranged some rows to group cases I think belong
together to make commenting on them easier, but I didn't remove or
add any rows.

>    pull.ff  pull.rebase  commandline            action
>      *          *        --ff-only --rebase     fast-forward only[1]
>      *          *        --ff-only --no-rebase  fast-forward only
>      *          *        --ff-only              fast-forward only[1]

I think these three make sense.  As you answered to a question in
another thread, I agree that --ff-only from the command line should
fail instead of letting rebase (or merge for that matter) go ahead
when their history is not a descendant of ours and it is a bug if it
does not.

>      *          *        --rebase --no-ff       rebase[1]
>      *          *        --rebase --ff          rebase[1]

OK.

>      *          *        --no-rebase --no-ff    merge --no-ff
>      *          *        --no-rebase --ff       merge --ff

OK.

>     <unset>     *        --no-rebase            merge --ff
>     only        *        --no-rebase            merge --ff[2]
>     false       *        --no-rebase            merge --no-ff
>     true        *        --no-rebase            merge --ff

I think the second one deserves an explanation.  The rationale for
ignoring --ff-only is because the act of giving an explicit
"--rebase" or "--no-rebase" from the command line, when the
configured default is "I expect to have no development on my own
here, and only want to follow along", is a sign enough that the user
does not want to follow along in this particular invocation of
"pull".  And the rationale for the entire thing to become --ff is
only because between --ff and --no-ff, the former is the default.

About the second one, I would understand it if it became "merge
--ff-only", too.  That is more trivially explained.  I however
suspect that it would be less useful, but that is open to
discussion.

All three others I think are not controversial.

>     <unset>     *        --rebase               rebase
>     only        *        --rebase               rebase[2]
>     false       *        --rebase               ?[2]
>     true        *        --rebase               ?[2]

Again the same reasoning applies to the second case that defeats the
configured pull.ff=only setting.  pull.ff=unset and pull.ff=true
would be handled the same, so only the pull.ff=false might deserve
an explanation.  I not think there needs anything special, as there
is no "I want to do something special to mark the fact that we tried
to integrate their work" special case needed even when their history
is descendant of ours in "rebase" workflow, where we replay what we
did on top of theirs (in other words, in "merge" workflow, the shape
of history left with --no-ff would be different from --ff; in "rebase",
there is no difference---either you had your own development before
you ran "pull --rebase" and they got replayed on top of their history,
or you had nothing before you ran "pull --rebase" and you did your
development after that on top of their history, you'd end up with
histories of the same shape.  This would not happen with "merge"
workflow and that is why --no-ff may matter).

So, I agree with the above 4 (and the other 4 for --no-rebase)
entries.


>      *       <unset>     --no-ff                merge --no-ff
>      *        false      --no-ff                merge --no-ff
>      *       <unset>     --ff                   merge --ff
>      *        false      --ff                   merge --ff

OK.

>      *       !false      --no-ff                rebase (ignore --no-ff)[2][3]
>      *       !false      --ff                   rebase (ignore --ff)[2][3]

OK.  We do specified rebase, allowing fast-forward.

pull.rebase=<something> combined with --no-ff could mean "I do
expect I have some development of my own, and please stop me if I
don't" and instead of rebasing nothing, error out without even
repointing our branch tip to their history when it fast forwards,
but I suspect that it would be less useful.  Again, that (and
anything else I said here) is open to discussion.

> It appears you, Phillip, and I all had different opinions about
> correct behavior and in a few cases though the documentation clearly
> implied what we thought.  So, I'd have to say the documentation is
> rather unclear as well.  However, even if the above table is filled
> out, it may be complicated enough that I'm at a bit of a loss about
> how to update the documentation to explain it short of including the
> table in the documentation.

Thanks.

  reply	other threads:[~2021-07-16 21:18 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-15  2:40 [PATCH 0/5] Handle conflicting pull options Elijah Newren via GitGitGadget
2021-07-15  2:40 ` [PATCH 1/5] pull: move definitions of parse_config_rebase and parse_opt_rebase Elijah Newren via GitGitGadget
2021-07-15  2:40 ` [PATCH 2/5] pull: convert OPT_PASSTHRU for fast-forward options to OPT_CALLBACK Elijah Newren via GitGitGadget
2021-07-15  2:40 ` [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins Elijah Newren via GitGitGadget
2021-07-15  4:59   ` Eric Sunshine
2021-07-15 17:13     ` Elijah Newren
2021-07-15  9:44   ` Felipe Contreras
2021-07-15 17:33   ` Junio C Hamano
2021-07-15 17:46     ` Felipe Contreras
2021-07-15 19:04     ` Elijah Newren
2021-07-15 19:58       ` Junio C Hamano
2021-07-15 20:40         ` Elijah Newren
2021-07-15 21:12           ` Junio C Hamano
2021-07-16 18:39             ` Elijah Newren
2021-07-16 21:18               ` Junio C Hamano [this message]
2021-07-16 21:56                 ` Felipe Contreras
2021-07-15 20:17       ` Junio C Hamano
2021-07-15 20:38         ` Elijah Newren
2021-07-15  2:40 ` [PATCH 4/5] pull: abort if --ff-only is given and fast-forwarding is impossible Alex Henrie via GitGitGadget
2021-07-15  2:40 ` [PATCH 5/5] pull: abort by default when fast-forwarding is not possible Elijah Newren via GitGitGadget
2021-07-15  5:18   ` Eric Sunshine
2021-07-15 16:56     ` Elijah Newren
2021-07-15  9:48   ` Felipe Contreras
2021-07-16  9:32   ` Ævar Arnfjörð Bjarmason
2021-07-16 18:13     ` Felipe Contreras
2021-07-15  9:37 ` [PATCH 0/5] Handle conflicting pull options 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=xmqqwnpqot4m.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=alexhenrie24@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=newren@gmail.com \
    --cc=phillip.wood123@gmail.com \
    --cc=sluongng@gmail.com \
    --subject='Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins' \
    /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

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox