Unexpected or wrong ff, no-ff and ff-only behaviour

Unexpected or wrong ff, no-ff and ff-only behaviour

[usbuser]

Hello,

I'm rather confused about --ff, --no-ff and --ff-only. They seam to be all mutual exclusive while the man page documentation and help reads as if only the first two are mutual exclusive, while the later is a check that can be enabled additionally to the former ones. I'm not sure why I couldn't find anything online about this issue.

The expected behaviour would be that a git merge --ff-only --no-ff only accepts merges where HEAD is up to date and generates then a merge commit. The observed behaviour is that a merge with conflict / non up to data HEAD is entered. If the two flags are swapped it can be observed that no merge commit is created and the branch is fast forwarded.

Sincerely,
eyenseo

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Junio C Hamano]

usbuser@mailbox.org writes:

> I'm rather confused about --ff, --no-ff and --ff-only. They seam
> to be all mutual exclusive...

A clean result left by "git merge" can be either a fast-forward, or
a real merge (i.e. 2 possible outcomes).

The --ff option lets you say "If the other history I am attempting
to merge is a descendant of the current commit, not creating a real
merge and instead fast-forwarding is permitted".  As this is the
default, case you actually type --ff on the command line is rather
limited (e.g. to countermand an earlier --no-ff on the command
line).

The --no-ff option lets you say "I never accept a fast-forward as
the result; please make a real merge instead, even if it is
redundant".

The --ff-only option lets you say "I never accept a real merge as 
the result; please fail if this does not fast-forward".

So, the only "real" options are between --[no-]ff (which allows or
disallows one of the two possible outcomes, which is "fast-forward")
and [--ff-only] (which allows or disallows the other one of the two
possible outcomes, which is "real merge").

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Roland Jäger]

Thanks for answering Junio. 

I get what git does. But I believe that either the documentation ist wrong/ambiguous or --no-ff and --ff-only should be able to be combined and either should be fixed - preferably the later. What I want to say to git is "I never accept a real merge; please make a merge commit, even if it is redundant/empty". And I believe that github and gitlab allow to configure something like that.

My manpage tells me the following:

--ff When the merge resolves as a fast-forward, only update the branch pointer, without creating a merge commit. This is the default behavior.
=> Allow either

--no-ff Create a merge commit even when the merge resolves as a fast-forward. This is the default behaviour when merging an annotated (and possibly signed) tag that is not stored in its natural place in refs/tags/ hierarchy.
=> Always create a commit, even when FF

--ff-only Refuse to merge and exit with a non-zero status unless the current HEAD is already up to date or the merge can be resolved as a fast-forward.
=> Fail if FF is not possible

 

man page: 
On 9 July 2019 16:51:14 CEST, Junio C Hamano <gitster@pobox.com> wrote:> usbuser@mailbox.org writes:
> 
> > I'm rather confused about --ff, --no-ff and --ff-only. They seam
> >      
> > to be all mutual exclusive...
> A clean result left by "git merge" can be either a fast-forward, or
> a real merge (i.e. 2 possible outcomes).
> 
> The --ff option lets you say "If the other history I am attempting
> to merge is a descendant of the current commit, not creating a real
> merge and instead fast-forwarding is permitted".  As this is the
> default, case you actually type --ff on the command line is rather
> limited (e.g. to countermand an earlier --no-ff on the command
> line).
> 
> The --no-ff option lets you say "I never accept a fast-forward as
> the result; please make a real merge instead, even if it is
> redundant".
> 
> The --ff-only option lets you say "I never accept a real merge as 
> the result; please fail if this does not fast-forward".
> 
> So, the only "real" options are between --[no-]ff (which allows or
> disallows one of the two possible outcomes, which is "fast-forward")
> and [--ff-only] (which allows or disallows the other one of the two
> possible outcomes, which is "real merge").

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Elijah Newren]

Hi Roland,

On Tue, Jul 9, 2019 at 9:17 AM Roland Jäger <eyenseo@mailbox.org> wrote:
>
> Thanks for answering Junio.
>
> I get what git does. But I believe that either the documentation ist wrong/ambiguous or --no-ff and --ff-only should be able to be combined and either should be fixed - preferably the later. What I want to say to git is "I never accept a real merge; please make a merge commit, even if it is redundant/empty". And I believe that github and gitlab allow to configure something like that.

Please don't top-post on this list.

I agree, the documentation is wrong or misleading and there is a
wording change we could make to improve it.  But, in particular,
--no-ff and -ff-only are completely incompatible.  A fast forward
implies no commits of any kind are created, while --no-ff explicitly
requires one to be created.  More on that below...

> My manpage tells me the following:
>
> --ff When the merge resolves as a fast-forward, only update the branch pointer, without creating a merge commit. This is the default behavior.
> => Allow either

Yes.

> --no-ff Create a merge commit even when the merge resolves as a fast-forward. This is the default behaviour when merging an annotated (and possibly signed) tag that is not stored in its natural place in refs/tags/ hierarchy.
> => Always create a commit, even when FF

Not quite; I'd instead say:

=> Always create a merge commit, even if FF is instead possible.

In particular, FF means there is no commit creation.  I agree the
documentation needs correction here, it should be:

"--no-ff: Create a merge commit even when the merge could instead
resolve as a fast-forward..."

Would you like to try your hand at submitting a patch with this change?

> --ff-only Refuse to merge and exit with a non-zero status unless the current HEAD is already up to date or the merge can be resolved as a fast-forward.
> => Fail if FF is not possible

Yes.


Hope that helps,
Elijah

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[usbuser]

> On 09 July 2019 at 18:35 Elijah Newren <newren@gmail.com> wrote:
> 
> 
> Hi Roland,
> 
> On Tue, Jul 9, 2019 at 9:17 AM Roland Jäger <eyenseo@mailbox.org> wrote:
> >
> > Thanks for answering Junio.
> >
> > I get what git does. But I believe that either the documentation ist wrong/ambiguous or --no-ff and --ff-only should be able to be combined and either should be fixed - preferably the later. What I want to say to git is "I never accept a real merge; please make a merge commit, even if it is redundant/empty". And I believe that github and gitlab allow to configure something like that.
> 
> Please don't top-post on this list.
> 
> I agree, the documentation is wrong or misleading and there is a
> wording change we could make to improve it.  But, in particular,
> --no-ff and -ff-only are completely incompatible.  A fast forward
> implies no commits of any kind are created, while --no-ff explicitly
> requires one to be created.  More on that below...
> 
> > My manpage tells me the following:
> >
> > --ff When the merge resolves as a fast-forward, only update the branch pointer, without creating a merge commit. This is the default behavior.
> > => Allow either
> 
> Yes.
> 
> > --no-ff Create a merge commit even when the merge resolves as a fast-forward. This is the default behaviour when merging an annotated (and possibly signed) tag that is not stored in its natural place in refs/tags/ hierarchy.
> > => Always create a commit, even when FF
> 
> Not quite; I'd instead say:
> 
> => Always create a merge commit, even if FF is instead possible.
> 
> In particular, FF means there is no commit creation.  I agree the
> documentation needs correction here, it should be:
> 
> "--no-ff: Create a merge commit even when the merge could instead
> resolve as a fast-forward..."
> 
> Would you like to try your hand at submitting a patch with this change?
> 
> > --ff-only Refuse to merge and exit with a non-zero status unless the current HEAD is already up to date or the merge can be resolved as a fast-forward.
> > => Fail if FF is not possible
> 
> Yes.
> 
> 
> Hope that helps,
> Elijah

I hope this is not-top-posting? I'm new to this and know nothing apparently.

If I were to write a patch then I would very much prefer to implement the behaviour that I expected and want to exist - either by a new flag and fixed wording, or just fixed behaviour. I guess the latter is a no go. Could you point me in the right direction where I would need to start to add such a flag?

Additionally I would also want to change the wording for --ff-only, as it currently reads as if it only performs a check (which would lead to the expected behaviour) but does more than that, as it prevents "real merges" altogether.

Thank you for your time,
Roland

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Elijah Newren]

On Tue, Jul 9, 2019 at 10:00 AM <usbuser@mailbox.org> wrote:
>
> > On 09 July 2019 at 18:35 Elijah Newren <newren@gmail.com> wrote:
> >
...
> > Hope that helps,
> > Elijah
>
> I hope this is not-top-posting? I'm new to this and know nothing apparently.

Yep, you're doing good.

> If I were to write a patch then I would very much prefer to implement the behaviour that I expected and want to exist - either by a new flag and fixed wording, or just fixed behaviour. I guess the latter is a no go. Could you point me in the right direction where I would need to start to add such a flag?

We can't break backward compatibility, so we can't change the meaning
of the existing flags.  However, I have no idea what you want this new
flag to do; what is the behavior you want?  The only three
possibilities I can think of are what the three flags we are currently
discussing do.

> Additionally I would also want to change the wording for --ff-only, as it currently reads as if it only performs a check (which would lead to the expected behaviour) but does more than that, as it prevents "real merges" altogether.

You've lost me again, I think.  You expect --ff-only to only perform a
check, i.e. to not update anything and thus only report on whether a
fast-forward would be possible, but leave the branch exactly where it
started no matter what?

Or is it just still not clear that a fast forward by definition is not
"a real merge", i.e. it means to update using a mechanism that doesn't
involve creating any new commits?


Elijah

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Bryan Turner]

On Tue, Jul 9, 2019 at 1:33 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Tue, Jul 9, 2019 at 10:00 AM <usbuser@mailbox.org> wrote:
> >
> > Additionally I would also want to change the wording for --ff-only, as it currently reads as if it only performs a check (which would lead to the expected behaviour) but does more than that, as it prevents "real merges" altogether.
>
> You've lost me again, I think.  You expect --ff-only to only perform a
> check, i.e. to not update anything and thus only report on whether a
> fast-forward would be possible, but leave the branch exactly where it
> started no matter what?
>
> Or is it just still not clear that a fast forward by definition is not
> "a real merge", i.e. it means to update using a mechanism that doesn't
> involve creating any new commits?

I think this is something I've seen come up on the list before[1]
(Roland can correct me if I'm wrong). What I've seen asked for before
is the ability to pass the combination "--ff-only --no-ff" and have
that:
* Ensure the branch to be merged is fast-forward from the current
branch (i.e., to ensure no merge commit is actually necessary), but
* Create a redundant merge commit anyway

This retains the ancestry (as in, it shows where the branches were
merged), but the merge is always effectively a no-op (no risk of
unintended interactions, the sort of subtle breakages where the merge
succeeds but the code on each "side" isn't entirely compatible,
resulting in broken compilation and/or tests and/or runtime).

Best regards,
Bryan Turner

[1] https://public-inbox.org/git/CAP4gbxqjHzqHhPuNK8UOwPMa46g2=vcNSk1AvGjxN8s+ou-0Dw@mail.gmail.com/

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[usbuser]

> On 09 July 2019 at 22:51 Bryan Turner <bturner@atlassian.com> wrote:
> 
> On Tue, Jul 9, 2019 at 1:33 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Tue, Jul 9, 2019 at 10:00 AM <usbuser@mailbox.org> wrote:
> > >
> > > Additionally I would also want to change the wording for --ff-only, as it currently reads as if it only performs a check (which would lead to the expected behaviour) but does more than that, as it prevents "real merges" altogether.
> >
> > You've lost me again, I think.  You expect --ff-only to only perform a
> > check, i.e. to not update anything and thus only report on whether a
> > fast-forward would be possible, but leave the branch exactly where it
> > started no matter what?
> >
> > Or is it just still not clear that a fast forward by definition is not
> > "a real merge", i.e. it means to update using a mechanism that doesn't
> > involve creating any new commits?
> 
> I think this is something I've seen come up on the list before[1]
> (Roland can correct me if I'm wrong). What I've seen asked for before
> is the ability to pass the combination "--ff-only --no-ff" and have
> that:
> * Ensure the branch to be merged is fast-forward from the current
> branch (i.e., to ensure no merge commit is actually necessary), but
> * Create a redundant merge commit anyway
> 
> This retains the ancestry (as in, it shows where the branches were
> merged), but the merge is always effectively a no-op (no risk of
> unintended interactions, the sort of subtle breakages where the merge
> succeeds but the code on each "side" isn't entirely compatible,
> resulting in broken compilation and/or tests and/or runtime).
> 
> Best regards,
> Bryan Turner
> 
> [1] https://public-inbox.org/git/CAP4gbxqjHzqHhPuNK8UOwPMa46g2=vcNSk1AvGjxN8s+ou-0Dw@mail.gmail.com/

This is exactly what I was expecting/wanting, thanks for clarifying.

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Sergey Organov]

Hi Elijah,

Elijah Newren <newren@gmail.com> writes:

> Hi Roland,
>
> On Tue, Jul 9, 2019 at 9:17 AM Roland Jäger <eyenseo@mailbox.org> wrote:
>>
>> Thanks for answering Junio.
>>
>> I get what git does. But I believe that either the documentation ist
>> wrong/ambiguous or --no-ff and --ff-only should be able to be
>> combined and either should be fixed - preferably the later. What I
>> want to say to git is "I never accept a real merge; please make a
>> merge commit, even if it is redundant/empty". And I believe that
>> github and gitlab allow to configure something like that.
>
> Please don't top-post on this list.
>
> I agree, the documentation is wrong or misleading and there is a
> wording change we could make to improve it.  But, in particular,
> --no-ff and -ff-only are completely incompatible.

They currently are compatible, but in a weird manner, see below.

I'd prefer they were perfectly compatible and orthogonal, as using both
would be the safe way to create cleanest possible merge -- the one that
introduces on the mainline the /exact/ changes made on side-branch:

--ff-only: tells git to refuse operation (actual merge) if FF is
impossible, that for this particular use-case would ensure the merge, when
actually created, is the "cleanest possible" one

and then

--no-ff: forces git to create actual (cleanest possible) merge commit,
that otherwise it wouldn't, as it defaults to --ff, and --ff-only above
ensures FF is possible.

Right now (in my git 2.10) it seems these options somehow cancel each
other, that is both useless and surprising:

When FF is possible:

$ git merge --ff-only --no-ff side_branch

Merge made by the 'recursive' strategy.
[...]

Good!

$ git reset --hard HEAD~1
[...]
$ git merge --no-ff --ff-only side_branch
Updating eafaed6..b83db07
Fast-forward    
[...]

??? I said --no-ff: how comes you did fast-forward?!

When FF is impossible:

$ git merge --ff-only --no-ff side_branch

Merge made by the 'recursive' strategy.
[...] 
??? I said --ff-only: how comes you didn't bail out?!

$ git reset --hard HEAD~1
[...]
$ git merge --no-ff --ff-only side_branch
fatal: Not possible to fast-forward, aborting.

Good!

-- Sergey

git version 2.10.0.1.g57b01a3

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Junio C Hamano]

Bryan Turner <bturner@atlassian.com> writes:

> I think this is something I've seen come up on the list before[1]
> (Roland can correct me if I'm wrong). What I've seen asked for before
> is the ability to pass the combination "--ff-only --no-ff" and have
> that:
> * Ensure the branch to be merged is fast-forward from the current
> branch (i.e., to ensure no merge commit is actually necessary), but
> * Create a redundant merge commit anyway
>
> This retains the ancestry (as in, it shows where the branches were
> merged), but the merge is always effectively a no-op (no risk of
> unintended interactions, the sort of subtle breakages where the merge
> succeeds but the code on each "side" isn't entirely compatible,
> resulting in broken compilation and/or tests and/or runtime).

Hmph.  I know that the above was totally outside of expected use
pattern back when --ff/--no-ff and --ff-only were invented.  As I
still cannot tell if the use pattern makes sense, or even is
internally consistent, I am not ready to say if the omission was a
bad thing, though.  We did not omit it deliberately, but we might
have been lucky to omit it, which may have prevented a wrong
workflow from spreading.  If it is a wrong workflow, that is, and I
am yet not sure it is not.

The "--no-ff" part is a way to ensure that the series of commits are
still grouped together after they hit the mainline of the history,
so it expresses a wish (by the project using the use pattern) that
they care about topic branch workflow.  A group of commits belonging
to one single topic is about doing one single thing, not a random
collection.  "Make sure we always create a merge to record which
ones are from the side branch and which ones are the mainline" is a
reasonable way to ensure that.

The "the tip being merged into the mainline must always be
fast-forwardable", however, is not consistent with the topic branch
workflow, and I do not mean this in the sense that you should never
rebase just before submitting (which is a bad practice).  For an
initial merge of the topic to the mainline, the project can keep
rebasing to the then-current tip of the mainline, and as long as
they can afford the cycle to test the result, "record the range of
the topic branch by making a redundant merge" would work.  

But that is true only when you have one mainline.  You cannot reuse
the topic to maintain an older mainline (i.e. a maintenance track),
because of the "topic must contain the then-current tip of the
mainline" rule.  A topic forked from the tip of master cannot be
merged to maint without dragging all the other crap happened since
maint to master, and a topic forked from the tip of maint, which can
be merged to maint without problem, cannot be merged to master due
to this "must contain the tip of master" rule.

It gets worse when you need to make a fix-up to the topic.  Has any
of those who advocate this workflow considered a case where a topic
later needs fixups?  Do they always produce a 100% bug-free code so
that they do not need to?

If you have a topic with initially 1 commit, merged to the mainline,
with the "record a redundant merge of fast-forwardable side branch",
you would get this:


    ----O---1--- mainline
         \ /
          A      topic


where the topic only has commit A.  How would you apply a fix this
commit?  "A merged branch must have forked from the then-current tip"
would mean you would do this:

    ----O---1-----2--- mainline
         \ / \   /
          A   \ /  topic
               B   topic-fix

i.e. you cannot touch 'topic' branch, which has 'A', that is sealed
by the fact that it was already merged to the mainline and the rule
that you reject a merge of any side branch that does not fork from
the then-current tip.  You are forced to create B forked from the
mainline to fix A and merge it back to mainline, losing the valuable
"these commits, together, achieves this thing" grouping you wanted
to achieve with the topic-branch workflow, which is what "--no-ff"
is asking.  This is the reason why I doubt the workflow that wants
to say "record a merge with a side branch that contains the current
tip of the mainline" is not even internally consistent.

So, I dunno.

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

[...]

> The "the tip being merged into the mainline must always be
> fast-forwardable",

It's rather "the tip being merged into the mainline must be fast-forwardable the
first time it is merged".

> however, is not consistent with the topic branch workflow, and I do
> not mean this in the sense that you should never rebase just before
> submitting (which is a bad practice). For an initial merge of the
> topic to the mainline, the project can keep rebasing to the
> then-current tip of the mainline, and as long as they can afford the
> cycle to test the result, "record the range of the topic branch by
> making a redundant merge" would work.

Yes, that's exactly it, and, as the rule holds for the first topic merge
only, the rest of workflow is as usual, no drastic changes.

Overall, it only ensures the first merge of the topic is semantically
simpler, nothing more.

-- Sergey

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

On 2019-07-09 at 20:51:39, Bryan Turner wrote:
> I think this is something I've seen come up on the list before[1]
> (Roland can correct me if I'm wrong). What I've seen asked for before
> is the ability to pass the combination "--ff-only --no-ff" and have
> that:
> * Ensure the branch to be merged is fast-forward from the current
> branch (i.e., to ensure no merge commit is actually necessary), but
> * Create a redundant merge commit anyway
> 
> This retains the ancestry (as in, it shows where the branches were
> merged), but the merge is always effectively a no-op (no risk of
> unintended interactions, the sort of subtle breakages where the merge
> succeeds but the code on each "side" isn't entirely compatible,
> resulting in broken compilation and/or tests and/or runtime).

I should point out that this is scriptable using something like the
following:

  git fetch origin topic && git merge-base --is-ancestor HEAD FETCH_HEAD && \
    git merge --no-ff FETCH_HEAD

or, if you're just merging a branch:

  git merge-base --is-ancestor HEAD topic && \
    git merge --no-ff topic

While I agree it's not as convenient as having this built-in (and I
understand why people want it), it is achievable with an alias without
much difficulty.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
> [...]
>
>> The "the tip being merged into the mainline must always be
>> fast-forwardable",
>
> It's rather "the tip being merged into the mainline must be fast-forwardable the
> first time it is merged".
>
>> however, is not consistent with the topic branch workflow, and I do
>> not mean this in the sense that you should never rebase just before
>> submitting (which is a bad practice). For an initial merge of the
>> topic to the mainline, the project can keep rebasing to the
>> then-current tip of the mainline, and as long as they can afford the
>> cycle to test the result, "record the range of the topic branch by
>> making a redundant merge" would work.
>
> Yes, that's exactly it, and, as the rule holds for the first topic merge
> only, the rest of workflow is as usual, no drastic changes.
>
> Overall, it only ensures the first merge of the topic is semantically
> simpler, nothing more.

But then you have to know to say or omit --ff-only if you are
mergint the topic for the first time.  The second and subsequent
merges, your merges won't be "semantically simple" at all.

And your first "semantically simple" merge is likely to be a merge
between the then-current tip, and a topic that has been prepared
over a few hours (or longer) and has sufficiently been tested, but
has gotten rebased immediately before being merged because of this
"must fast-forwardable to maintain semantic simplicity" policy to
catch up with the ever-moving-forward tip of the mainline.  The end
result of such a "semantically simpler" merge is as good as the
"freshly rebased, never had sufficient time to verify the result"
iteration of the topic branch.

How would such a state that was freshly rebased without chance of
enough validation be better than merging the "sufficiently been
tested in isolation" state without doing such a rebase?  If your
answer is "but the contributor would test after rebasing and before
doing the --ff-only-plus-no-ff merge", then perhaps the contributor
can also test the result of a (trial) merge before it actually gets
merged, no?

If we have a project like this:

        A               topic that is slightly stale
       /
  o---F---o---o---X     mainline

M, A', and N should end up with identical trees:


        A-----------M   topic that is slightly stale, merged into mainline
       /           /
  o---F---o---o---X---N mainline with A' merged
                   \ /
                    A'  mainline with A rebased on top as A'

And by forcing to rebase A to A' before merging into the mainline as
N, compared to advancing mainline from X to M, one major difference
the workflow is making is to _lose_ the information that the topic
was cooked in the context of an older mainline and did not take what
happened since F until X into account.  Rebasing it to A' without
giving the result sufficient vetting means you are forcing the
updated topic to pretend as if it also took F..X into account,
making it harder to diagnose subtle interactions with A and F..X
later that you did not spot when you made A'; merging to create M
does not have such a problem.  As A' and M would have the identical
trees, the same bug resulting from such a subtle interactions would
be there if A' has such an issue, but at least M is more honest and
lets us know that A itself predated what happened between F..X.

So...

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> [...]
>>
>>> The "the tip being merged into the mainline must always be
>>> fast-forwardable",
>>
>> It's rather "the tip being merged into the mainline must be fast-forwardable the
>> first time it is merged".
>>
>>> however, is not consistent with the topic branch workflow, and I do
>>> not mean this in the sense that you should never rebase just before
>>> submitting (which is a bad practice). For an initial merge of the
>>> topic to the mainline, the project can keep rebasing to the
>>> then-current tip of the mainline, and as long as they can afford the
>>> cycle to test the result, "record the range of the topic branch by
>>> making a redundant merge" would work.
>>
>> Yes, that's exactly it, and, as the rule holds for the first topic merge
>> only, the rest of workflow is as usual, no drastic changes.
>>
>> Overall, it only ensures the first merge of the topic is semantically
>> simpler, nothing more.
>
> But then you have to know to say or omit --ff-only if you are
> mergint the topic for the first time.  The second and subsequent
> merges, your merges won't be "semantically simple" at all.

Yes, the second and the rest (if any) won't differ, but having the first
one, that supposedly introduces most of changes, "semantically simple"
is still an advantage.

>
> And your first "semantically simple" merge is likely to be a merge
> between the then-current tip, and a topic that has been prepared
> over a few hours (or longer) and has sufficiently been tested, but
> has gotten rebased immediately before being merged because of this
> "must fast-forwardable to maintain semantic simplicity" policy to
> catch up with the ever-moving-forward tip of the mainline.  The end
> result of such a "semantically simpler" merge is as good as the
> "freshly rebased, never had sufficient time to verify the result"
> iteration of the topic branch.

Yes, this makes sense only in otherwise rebase-based workflows, when you
do rebase anyway.

> How would such a state that was freshly rebased without chance of
> enough validation be better than merging the "sufficiently been
> tested in isolation" state without doing such a rebase?  If your
> answer is "but the contributor would test after rebasing and before
> doing the --ff-only-plus-no-ff merge", then perhaps the contributor
> can also test the result of a (trial) merge before it actually gets
> merged, no?

Yes, I mean a workflow where testing of rebased (or merged, doesn't
matter, as they should actually be the same thing) version is possible
or even required, and I believe that in this particular case testing
rebased version and merging the result makes more sense, see below.

>
> If we have a project like this:
>
>         A               topic that is slightly stale
>        /
>   o---F---o---o---X     mainline
>
> M, A', and N should end up with identical trees:
>
>
>         A-----------M   topic that is slightly stale, merged into mainline
>        /           /
>   o---F---o---o---X---N mainline with A' merged
>                    \ /
>                     A'  mainline with A rebased on top as A'
>
> And by forcing to rebase A to A' before merging into the mainline as
> N, compared to advancing mainline from X to M, one major difference
> the workflow is making is to _lose_ the information that the topic
> was cooked in the context of an older mainline and did not take what
> happened since F until X into account.  Rebasing it to A' without
> giving the result sufficient vetting means you are forcing the
> updated topic to pretend as if it also took F..X into account,
> making it harder to diagnose subtle interactions with A and F..X
> later that you did not spot when you made A'; merging to create M
> does not have such a problem.  As A' and M would have the identical
> trees, the same bug resulting from such a subtle interactions would
> be there if A' has such an issue, but at least M is more honest and
> lets us know that A itself predated what happened between F..X.
>
> So...

So it has a disadvantage for the workflow you have in mind, and you
won't use the feature should you decide the disadvantage outweighs the
aforementioned advantage, as usual.

However, committing untested M still doesn't taste as the best possible
way of handling things in general. It'd be best to actually test M or N
before publishing. In a workflow where there are measures to do it,
testing A' (= N) and publishing N both makes more sense and results in
simpler history than merging A and then testing and publishing M.

That said, even if we rather all do agree rebase workflow is always
inferior to merge one, is it satisfactory excuse to actively resist
otherwise logical behavior of 'git merge' that is even documented? I
don't think so.

Thus, one way or another, I'd still vote for keeping options description
intact, and rather fix the implementation to match the manual, i.e.,
make the --[no]-ff actually orthogonal to --ff-only.

-- Sergey

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Elijah Newren]

On Fri, Jul 12, 2019 at 6:50 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> That said, even if we rather all do agree rebase workflow is always
> inferior to merge one, is it satisfactory excuse to actively resist
> otherwise logical behavior of 'git merge' that is even documented? I
> don't think so.
>
> Thus, one way or another, I'd still vote for keeping options description
> intact, and rather fix the implementation to match the manual, i.e.,
> make the --[no]-ff actually orthogonal to --ff-only.

This seems like it's asking for trouble.  --ff-only and --no-ff,
ignoring any knowledge of what they do, are contradictory by their
names.  Attempting to explain to users that they are not opposites but
can be used together compatibly sounds very problematic to me.

But let me dig a little deeper into your statement of "resisting
otherwise logical behavior of 'git merge" that is even documented."
Going back to when the --ff-only option was introduced:

commit 134748353b2a71a34f899c9b1326ccf7ae082412
Author: Björn Gustavsson <bgustavsson@gmail.com>
Date:   Thu Oct 29 23:08:31 2009 +0100

    Teach 'git merge' and 'git pull' the option --ff-only
   
    For convenience in scripts and aliases, add the option
    --ff-only to only allow fast-forwards (and up-to-date,
    despite the name).
   
    Disallow combining --ff-only and --no-ff, since they
    flatly contradict each other.

We see that the original author even stated pretty clearly and
strongly that these were polar opposite options.  Our description
today of --ff-only is the same one he introduced in that patch, and I
always read the description the same way I think he did: that it
implied a polar opposite of --no-ff.  You and Bryan came along and
pointed out that the description was actually ambiguous and could be
interpreted a different way...and I then had to re-read the text a
time or two after you pointed it out to see the alternate reading.  I
totally admit that the wording is apparently ambiguous and can be read
the way you have, but I don't at all see that as a reason to keep the
documentation as-is.  Either it should be clarified to rule out what
you and usbuser wanted and perhaps misunderstood it to do, or if the
behavior is changed then it should be clarified so that folks like me
don't read it to behave as it has since Björn added it.

Personally, I think that if new functionality is wanted, it should
definitely go in a different flag (perhaps someone can find something
shorter than --no-ff-and-only-if-ff-is-possible).  But I think Junio
makes a pretty good argument to be leery of such a new option, and
Brian points out how to easily get this behavior by just stringing
together a merge-base command with merge --no-ff.

That's just my $0.02, but since this is the second time in the thread
that I've suggested improving the documentation, here's a patch to do
so:


-- 8< --
Subject: [PATCH] merge-options.txt: clarify meaning of various ff-related
 options

As discovered on the mailing list, some of the descriptions of the
ff-related options were unclear.  Try to be more precise with what these
options do.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/merge-options.txt | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..e888c99d48 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -40,20 +40,21 @@ set to `no` at the beginning of them.
 	case of a merge conflict.
 
 --ff::
-	When the merge resolves as a fast-forward, only update the branch
-	pointer, without creating a merge commit.  This is the default
-	behavior.
+	When the merge resolves as a fast-forward, only update the
+	branch pointer (without creating a merge commit).  When a fast
+	forward update is not possible, create a merge commit.  This is
+	the default behavior.
 
 --no-ff::
-	Create a merge commit even when the merge resolves as a
-	fast-forward.  This is the default behaviour when merging an
-	annotated (and possibly signed) tag that is not stored in
-	its natural place in 'refs/tags/' hierarchy.
+	Create a merge commit even when the merge could instead resolve
+	as a fast-forward.  This is the default behaviour when merging
+	an annotated (and possibly signed) tag that is not stored in its
+	natural place in 'refs/tags/' hierarchy.
 
 --ff-only::
-	Refuse to merge and exit with a non-zero status unless the
-	current `HEAD` is already up to date or the merge can be
-	resolved as a fast-forward.
+	When possible, resolve the merge as a fast-forward (do not
+	create a merge commit).  When not possible, refuse to merge and
+	exit with a non-zero status.
 
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
-- 
2.22.0.429.gbb0a79e5f4

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> If we have a project like this:
>>
>>         A               topic that is slightly stale
>>        /
>>   o---F---o---o---X     mainline
>>
>> M, A', and N should end up with identical trees:
>>
>>
>>         A-----------M   topic that is slightly stale, merged into mainline
>>        /           /
>>   o---F---o---o---X---N mainline with A' merged
>>                    \ /
>>                     A'  mainline with A rebased on top as A'
>>
>> And by forcing to rebase A to A' before merging into the mainline as
>> N, compared to advancing mainline from X to M, one major difference
>> the workflow is making is to _lose_ the information that the topic
>> was cooked in the context of an older mainline and did not take what
>> happened since F until X into account....
>
> However, committing untested M still doesn't taste as the best possible
> way of handling things in general. It'd be best to actually test M or N
> before publishing.

Oh, no question about it.  I am not advocating (and I do not do
personally) publishing an untested tip.  

But the point is, if M and N are equally well tested before
publication, they may still have bugs resulting from subtle
interactions between A and F..X that is not discovered during that
testing.  And N loses the information that would help diagnosing
what went wrong, which does not happen if you published M.

About the docs easily getting misinterpreted, I think Elijah covered
it pretty well.

Thanks.

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Sergey Organov]

Elijah Newren <newren@gmail.com> writes:

> On Fri, Jul 12, 2019 at 6:50 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> That said, even if we rather all do agree rebase workflow is always
>> inferior to merge one, is it satisfactory excuse to actively resist
>> otherwise logical behavior of 'git merge' that is even documented? I
>> don't think so.
>>
>> Thus, one way or another, I'd still vote for keeping options description
>> intact, and rather fix the implementation to match the manual, i.e.,
>> make the --[no]-ff actually orthogonal to --ff-only.
>
> This seems like it's asking for trouble.  --ff-only and --no-ff,
> ignoring any knowledge of what they do, are contradictory by their
> names.

I agree --ff-only is not the best name for what I have in mind. However,
it's not the best name for actual behavior either, as for this case
[--ff|--no-ff|--only-ff], or even '--ff=[prefer|never|only]' would look
more consistent.

> Attempting to explain to users that they are not opposites but
> can be used together compatibly sounds very problematic to me.

For me current documentation would be clear enough, provided the options
would indeed behave the way they are described. Unfortunately (for me),
they don't behave that way, and it looks like they never will.

> But let me dig a little deeper into your statement of "resisting
> otherwise logical behavior of 'git merge" that is even documented."
> Going back to when the --ff-only option was introduced:
>
> commit 134748353b2a71a34f899c9b1326ccf7ae082412
> Author: Björn Gustavsson <bgustavsson@gmail.com>
> Date:   Thu Oct 29 23:08:31 2009 +0100
>
>     Teach 'git merge' and 'git pull' the option --ff-only
>
>     For convenience in scripts and aliases, add the option
>     --ff-only to only allow fast-forwards (and up-to-date,
>     despite the name).
>
>     Disallow combining --ff-only and --no-ff, since they
>     flatly contradict each other.
>
> We see that the original author even stated pretty clearly and
> strongly that these were polar opposite options.  Our description
> today of --ff-only is the same one he introduced in that patch, and I
> always read the description the same way I think he did: that it
> implied a polar opposite of --no-ff.

OK, so it was at the very beginning where opportunity for better design
has been missed.

> You and Bryan came along and
> pointed out that the description was actually ambiguous and could be
> interpreted a different way...and I then had to re-read the text a
> time or two after you pointed it out to see the alternate reading.  I
> totally admit that the wording is apparently ambiguous and can be read
> the way you have, but I don't at all see that as a reason to keep the
> documentation as-is.  Either it should be clarified to rule out what
> you and usbuser wanted and perhaps misunderstood it to do, or if the
> behavior is changed then it should be clarified so that folks like me
> don't read it to behave as it has since Björn added it.
>
> Personally, I think that if new functionality is wanted, it should
> definitely go in a different flag (perhaps someone can find something
> shorter than --no-ff-and-only-if-ff-is-possible).  But I think Junio
> makes a pretty good argument to be leery of such a new option, and
> Brian points out how to easily get this behavior by just stringing
> together a merge-base command with merge --no-ff.

My argument was *not* that we absolutely must have this functionality,
it was rather that if we had made '--ff-only' option orthogonal (that is
a good thing in general by itself), this functionality would come almost
for free. No documentation change then. However, now I see that such
change at this point will change behavior of certain combinations of
options in incompatible manner, and so is not a good way to go.

BTW, I did suggest a new option for this functionality once, Junio being
in opposition then as well, arguing that it looks like adding a feature
just for the sake of having a feature. At that time I found 'git merge'
happily takes both '--no-ff --ff-only', and made a mistake of assuming,
after re-reading the manual, that it in fact does what is documented, so
I dropped the issue of adding a new option then.

> That's just my $0.02, but since this is the second time in the thread
> that I've suggested improving the documentation, here's a patch to do
> so:
>
> -- 8< --
> Subject: [PATCH] merge-options.txt: clarify meaning of various ff-related
>  options
>
> As discovered on the mailing list, some of the descriptions of the
> ff-related options were unclear.  Try to be more precise with what these
> options do.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  Documentation/merge-options.txt | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 79a00d2a4a..e888c99d48 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -40,20 +40,21 @@ set to `no` at the beginning of them.
>  	case of a merge conflict.
>
>  --ff::
> -	When the merge resolves as a fast-forward, only update the branch
> -	pointer, without creating a merge commit.  This is the default
> -	behavior.
> +	When the merge resolves as a fast-forward, only update the
> +	branch pointer (without creating a merge commit).  When a fast
> +	forward update is not possible, create a merge commit.  This is
> +	the default behavior.

A side note: except it is *not* the default "when merging an
annotated...", described below.

>
>  --no-ff::
> -	Create a merge commit even when the merge resolves as a
> -	fast-forward.  This is the default behaviour when merging an
> -	annotated (and possibly signed) tag that is not stored in
> -	its natural place in 'refs/tags/' hierarchy.
> +	Create a merge commit even when the merge could instead resolve
> +	as a fast-forward.  This is the default behaviour when merging
> +	an annotated (and possibly signed) tag that is not stored in its
> +	natural place in 'refs/tags/' hierarchy.
>
>  --ff-only::
> -	Refuse to merge and exit with a non-zero status unless the
> -	current `HEAD` is already up to date or the merge can be
> -	resolved as a fast-forward.
> +	When possible, resolve the merge as a fast-forward (do not
> +	create a merge commit).  When not possible, refuse to merge and
> +	exit with a non-zero status.

Yeah, this resolves uncertainty.

However, I think it'd be better to format it after other similar cases,
to make it explicit these options are 3-way choice. I mean something
like this:

--ff::
--no-ff::
--ff-only::
	When the merge resolves as a fast-forward, only update the
	branch pointer (without creating a merge commit).  When a fast
	forward update is not possible, create a merge commit.  This is
	the default behavior, unless merging an annotated (and possibly
	signed) tag that is not stored in its natural place in
	'refs/tags/' hierarchy, in which case --no-ff is assumed.
+
With --no-ff create a merge commit even when the merge could instead
resolve as a fast-forward.
+
With --ff-only resolve the merge as a fast-forward (never create a merge
commit). When fast-forward is not possible, refuse to merge and exit
with non-zero status.


-- Sergey

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> If we have a project like this:
>>>
>>>         A               topic that is slightly stale
>>>        /
>>>   o---F---o---o---X     mainline
>>>
>>> M, A', and N should end up with identical trees:
>>>
>>>
>>>         A-----------M   topic that is slightly stale, merged into mainline
>>>        /           /
>>>   o---F---o---o---X---N mainline with A' merged
>>>                    \ /
>>>                     A'  mainline with A rebased on top as A'
>>>
>>> And by forcing to rebase A to A' before merging into the mainline as
>>> N, compared to advancing mainline from X to M, one major difference
>>> the workflow is making is to _lose_ the information that the topic
>>> was cooked in the context of an older mainline and did not take what
>>> happened since F until X into account....
>>
>> However, committing untested M still doesn't taste as the best possible
>> way of handling things in general. It'd be best to actually test M or N
>> before publishing.
>
> Oh, no question about it.  I am not advocating (and I do not do
> personally) publishing an untested tip.  
>
> But the point is, if M and N are equally well tested before
> publication, they may still have bugs resulting from subtle
> interactions between A and F..X that is not discovered during that
> testing.  And N loses the information that would help diagnosing
> what went wrong, which does not happen if you published M.

I see your point.

My point is that it's still a /choice/ between more information and
history simplification. It's not one way. I dispute that keeping
reference to the original branch has enough significance to /always/
overweight opportunity for history simplification, no matter what
workflow is in use.

> About the docs easily getting misinterpreted, I think Elijah covered
> it pretty well.

Yeah, sure, the docs should better be fixed.

Anyway, bare "git --no-ff" is still there, and I can live with no safety
belt that '--ff-only' could easily have been, it's just that it's a pity
to see lost opportunities in the design.

Thanks,

-- Sergey

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>> But the point is, if M and N are equally well tested before
>> publication, they may still have bugs resulting from subtle
>> interactions between A and F..X that is not discovered during that
>> testing.  And N loses the information that would help diagnosing
>> what went wrong, which does not happen if you published M.
>
> I see your point.
>
> My point is that it's still a /choice/ between more information and
> history simplification.

I actually fail to see that point.

If we are not constrained by that "first merge of a topic must be a
redundant fast-forward merge", a topic that originally had two
commits A and B, merged to the mainline to produce M and then
further corrected with a commit C before it gets merged back at O to
the mainline would leave this history:

          A-----------B-------C
         /             \       \
    o---F---o---o---X---M---o---O---

If you enforce the "first merge of a topic must be a redundant
fast-forward merge" rule, you'd end up with a history like this:

          A-----------B
         /
    o---F---o---o---X-------N---o---P---
                     \     /       /
                      A'--B'------C

Is the latter materially simpler than the former?  I do not think
so.

I was preparing myself to say "we rejected the combination because
it would encourage wrong workflow, but perhaps over the years people
like you and usbuser may have found good use patterns different from
what we considered back then, and these use patterns may not
encourage wrong workflows.  It may not be a bad idea to introduce a
new optional behaviour if that is the case", but what I heard so far
does not convince me that we have good use patterns.
>> About the docs easily getting misinterpreted, I think Elijah covered
>> it pretty well.
>
> Yeah, sure, the docs should better be fixed.
>
> Anyway, bare "git --no-ff" is still there, and I can live with no safety
> belt that '--ff-only' could easily have been, it's just that it's a pity
> to see lost opportunities in the design.

Lost opportunities to add an option to encourage bad workflow?  

No, thanks ;-)

But thanks for a discussion anyway.

Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Sergey Organov <sorganov@gmail.com> writes:
>>>
>>> But the point is, if M and N are equally well tested before
>>> publication, they may still have bugs resulting from subtle
>>> interactions between A and F..X that is not discovered during that
>>> testing.  And N loses the information that would help diagnosing
>>> what went wrong, which does not happen if you published M.
>>
>> I see your point.
>>
>> My point is that it's still a /choice/ between more information and
>> history simplification.
>
> I actually fail to see that point.

I'm sorry you do. Let me try again.

>
> If we are not constrained by that "first merge of a topic must be a
> redundant fast-forward merge", a topic that originally had two
> commits A and B, merged to the mainline to produce M and then
> further corrected with a commit C before it gets merged back at O to
> the mainline would leave this history:
>
>           A-----------B-------C
>          /             \       \
>     o---F---o---o---X---M---o---O---
>
> If you enforce the "first merge of a topic must be a redundant
> fast-forward merge" rule, you'd end up with a history like this:
>
>           A-----------B
>          /
>     o---F---o---o---X-------N---o---P---
>                      \     /       /
>                       A'--B'------C
>
> Is the latter materially simpler than the former?  I do not think
> so.

I think it is. To see it clearly, let's cut common parts, and then cut
irrelevant parts as well from the pictures you gave:

           A-----------B
          /             \
     o---F---o---o---X---M

                  ---X-------N---
                      \     /
                       A'--B'

There is both qualitative and quantitative difference here.

1. The qualitative difference.

In the former case we have outdated topic branch and a merge commit that
simultaneously serves two purposes: gives a recipe to bring outdated
branch up to date, and records the fact that the topic in now merged to
mainline.

In the letter case we have up to date topic branch and a merge commit
that servers exactly one one purpose: it records the fact that the topic
in now merged to mainline.

This is exactly the "material" part of the simplification I mean.

2. The quantitative difference.

In the latter case the horizon to reason about the topic branch and its
adoption to mainline is at commit X, while in the former case it is at
commit F.

The quantitative difference that we have here is proportional to the
size of diff between X and F, and once we aim at decreasing this
difference (i.e., not accepting very outdated branches), it's only
logical to get rid of this difference entirely, once again ending up
with the latter case.

Finally, please also notice that ability to reference outdated version
of the topic branch (or even multiple of them), provided there is actual
reason to, is still there:

$ git checkout topic
$ git pull --rebase
$ git merge -s ours topic@{1} # keep reference to original
$ git checkout mainline
$ git merge --no-ff topic

> I was preparing myself to say "we rejected the combination because
> it would encourage wrong workflow, but perhaps over the years people
> like you and usbuser may have found good use patterns different from
> what we considered back then, and these use patterns may not
> encourage wrong workflows.  It may not be a bad idea to introduce a
> new optional behaviour if that is the case", but what I heard so far
> does not convince me that we have good use patterns.
>
>>> About the docs easily getting misinterpreted, I think Elijah covered
>>> it pretty well.
>>
>> Yeah, sure, the docs should better be fixed.
>>
>> Anyway, bare "git --no-ff" is still there, and I can live with no safety
>> belt that '--ff-only' could easily have been, it's just that it's a pity
>> to see lost opportunities in the design.
>
> Lost opportunities to add an option to encourage bad workflow?  
>
> No, thanks ;-)

I dunno what is that "bad workflow" you have in mind, sorry.

-- Sergey