Re: [RFC/PATCH] Fast forward strategies allow, never, and only

From: Sverre Hvammen Johansen <hvammen@gmail.com>
Date: Mar 12, 2008, 05:46 UTC
Message-ID: <402c10cd0803112246q4ec98018pe9a34b95e32cf1@mail.gmail.com>
In-Reply-To: <7vk5k9eqax.fsf@gitster.siamese.dyndns.org>

On Mon, Mar 10, 2008 at 10:19 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> "Sverre Hvammen Johansen" <hvammen@gmail.com> writes:
>
>  > @@ -153,8 +153,8 @@ parse_config () {
>  >               --summary)
>  >                       show_diffstat=t ;;
>  >               --squash)
>  > -                     test "$allow_fast_forward" = t ||
>  > -                             die "You cannot combine --squash with --no-ff."
>  > +                     test "$fast_forward" = allow ||
>  > +                             die "You cannot combine --squash with --ff=never."
>
>  Why does the user get this message after saying --ff=only?

Bug. What should the semantic be? To me it makes sense that a squash oweride the ff options instead of giving a error, but specifying a ff option after --squash is an error.

>  How does this code parse "git merge --ff my_other_branch"?

It is getting late so I need to get back to you about this.

>  Shouldn't you issue the same error message for these two inputs?
>
>         "git merge --ff=never --squash"
>         "git merge --squash --ff=never"

Allow the first one since --ff=never can be in the config file and give error on the last one.

>  What does this complex double loop compute differently from what "git
>  show-branch --independent" gives you?  Aside from that you will run slower
>  but you can take more than 25 branches?

The main issue is that show-branch --independent does not give me the desired order for these branches. I want the first branch to be head or something that can be fast forwarded from head. The second branch should be the next branch in the specified list that have not been eliminated or something that can be fast forwarded from this, and so on and so forth. This is an absolute requirement for the first argument (head). If show-branch had a documented order and meet the absolute requirement above I would prefer show-branch --independentr instead of this nasty loop.

Show 7 quoted lines

>  More generally, I doubt it is really useful to let the user throw millions
>  of potentially duplicate refs and have the merge silently record a
>  filtered out results.  Yes, you made the process of culling duplicates too
>  chatty in the above part of the patch, and fmt-merge-msg will hopefully
>  still show what the user gave on the command line, but the heads used by
>  the real merge process now is very different from it.  The merge comment
>  is totally disconnected from the reality.  Why is this an improvement?

We already do this in the case where we have head pluss one branch. If it results in only one real parent we throw one of them away resulting in a fast forward or an "up to date". The suggested patch is just a generalization over this to the case where we have head and more than one branch.

>  If the goal is to allow Octopus that is more complex than the simplest
>  kind, don't.  Octopus was deliberately written to allow the most simple
>  kind and nothing else for a reason: bisectability.

That is not the goal.

>  The user should know what he is merging; throwing many heads that he does
>  not even know how they relate to each other, and call the resulting mess a
>  merge feels like a sure way to encourage a bad workflow.

We do merges all the time without knowing what we actually are merging. That is something that happen in many work flows. I assume that you don't want a real merge in the case that you are "up to date" with your remote or your head can be fast forward. For the users convenience we do a fast forward or report it to be "up to date". Exactly the same argument holds where there are more than one remote involved. The user may not know who is ahead and who is behind and he usually want the commit to record a simple history as possible.

Show 9 quoted lines

>  > +then
>  > +     real_parents="$@"
>  > +     ff_head=$head
>  > +else
>  > +     find_real_parents "$@"
>  > +fi
>
>  This part is simply unacceptable.  At least please do not needlessly call
>  find_real_parents in the most common case of giving only one remote head.

I now keep common_b in common so subsequent calls to git merge-base --all can be optimized away for the most common case.

-- 
Sverre Hvammen Johansen