merge ignores --no-commit in fast-forward case

merge ignores --no-commit in fast-forward case

[Tomas Carnecky]

Bug or feature?

Three possible solutions that I see:

  2) Refuse to do anything if user gives --no-commit and merge is fast- 
forward
  3) Document this behavior in the manpage
  4) Quietly set deny_non_fast_forward when --no-commit was given

A fourth solution would be to not change anything, but users usually  
have a reason for using --no-commit, and if git commits behind their  
backs anyway that's just not nice.

tom

Re: merge ignores --no-commit in fast-forward case

[Junio C Hamano]

Tomas Carnecky <tom@dbservice.com> writes:

> Bug or feature?

Feature, perhaps a misleading one.  As a fast-forward merge does not _create_
a commit, and --no-commit is about not creating a commit, it is logically
consistent.  But it is not useful nor intuitive to be logically consistent
in this case.

> Three possible solutions that I see:
>
>  2) Refuse to do anything if user gives --no-commit and merge is fast- 
> forward
>  3) Document this behavior in the manpage
>  4) Quietly set deny_non_fast_forward when --no-commit was given

Heh, this is new.  People laugh at me when I number my bullets starting
from zero, like all good computer scientists do ;-)

Seriously, we should at least do #3, and as a backward incompatible change
at least _consider_ doing #2 (I think #4 is merely an implementation detail
of #2), and if list reaches concensus in favor of such a change, come up
with a transition plan and do so in the 1.7.0 release.

Re: merge ignores --no-commit in fast-forward case

[Greg Price]

I've hit this issue in scripts where someone has written

  if ! git merge some-branch --no-commit; then
    # deal or have the user deal
  fi

  git commit -m '...'

intending that if the merge succeeds it has left the changes from
some-branch uncommitted.  And the script works, until for some reason
some-branch is on top of HEAD.  So Tomas is not the only person who
was surprised or confused by the present behavior. =)

In this case the workaround is to add --no-ff to the command, which is
effectively what the proposed change would do automatically.

Suppose we were to make such a change in 1.7.0 or a later version.
What would the transition plan need to accomplish?  For instance,
we could print a warning every time 'merge --no-commit' does a
fast-forward, and make the change in a later version -- might that suffice?

Cheers,
Greg


PS - Here's one version of a documentation patch for #3.

>From 83282bbbd0c918016e71e4ff6fd8a823315fed0e Mon Sep 17 00:00:00 2001
From: Greg Price <price@ksplice.com>
Date: Fri, 18 Sep 2009 15:34:19 -0400
Subject: [PATCH] Document behavior of --no-commit on fast forward.

This behavior can be surprising, so document it.

Signed-off-by: Greg Price <price@ksplice.com>
---
 Documentation/merge-options.txt |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/Documentation/merge-options.txt
b/Documentation/merge-options.txt
index adadf8e..6015e5d 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -31,7 +31,10 @@
 --no-commit::
        Perform the merge but pretend the merge failed and do
        not autocommit, to give the user a chance to inspect and
-       further tweak the merge result before committing.
+       further tweak the merge result before committing.  If the
+       merge resolved as a fast-forward, the branch pointer will
+       be updated as usual; --no-ff can be combined with this
+       option to always preserve the branch pointer.
 
 --commit::
        Perform the merge and commit the result. This option can
-- 
1.6.3.1.499.ge7b8da

Re: merge ignores --no-commit in fast-forward case

[Junio C Hamano]

Greg Price <price@ksplice.com> writes:

> Suppose we were to make such a change in 1.7.0 or a later version.
> What would the transition plan need to accomplish?  For instance,
> we could print a warning every time 'merge --no-commit' does a
> fast-forward, and make the change in a later version -- might that
> suffice?

I guess so, but right now I don't want to (nor have bandwidth to) think
about things beyond the release we are currently in pre-release freeze
for.  Helps from other people's braincells are very welcome.

> From: Greg Price <price@ksplice.com>
> Date: Fri, 18 Sep 2009 15:34:19 -0400
> Subject: [PATCH] Document behavior of --no-commit on fast forward.
>
> This behavior can be surprising, so document it.

If "surprising" is the problem (and it certainly is), perhaps it would be
better to rewrite it not to be so surprising, like:

        --no-commit::
                Do not create a new commit.  If your current branch
                fast-forwards to the branch you are merging, this option
                has no effect (use --no-ff for that).  Otherwise, a merge
                is done and the index and the work tree files are updated,
                but the command stops before creating a new commit that
                records the result, in order to give the user a chance to
                inspect and tweak the merge result before committing.

I dunno.

> Signed-off-by: Greg Price <price@ksplice.com>
> ---
>  Documentation/merge-options.txt |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/merge-options.txt
> b/Documentation/merge-options.txt
> index adadf8e..6015e5d 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -31,7 +31,10 @@
>  --no-commit::
>         Perform the merge but pretend the merge failed and do
>         not autocommit, to give the user a chance to inspect and
> -       further tweak the merge result before committing.
> +       further tweak the merge result before committing.  If the
> +       merge resolved as a fast-forward, the branch pointer will
> +       be updated as usual; --no-ff can be combined with this
> +       option to always preserve the branch pointer.

Re: merge ignores --no-commit in fast-forward case

[Paolo Bonzini]

On 09/19/2009 01:05 AM, Junio C Hamano wrote:
> If "surprising" is the problem (and it certainly is), perhaps it would be
> better to rewrite it not to be so surprising, like:
>
>          --no-commit::
>                  Do not create a new commit.

Do not create a new _merge_ commit, and it's perfect.

Paolo

Re: merge ignores --no-commit in fast-forward case

[Björn Steinbrink]

On 2009.09.13 00:30:49 -0700, Junio C Hamano wrote:
> Tomas Carnecky <tom@dbservice.com> writes:
> > Three possible solutions that I see:
> >
> >  2) Refuse to do anything if user gives --no-commit and merge is fast- 
> > forward
> >  3) Document this behavior in the manpage
> >  4) Quietly set deny_non_fast_forward when --no-commit was given
> 
> Heh, this is new.  People laugh at me when I number my bullets starting
> from zero, like all good computer scientists do ;-)
> 
> Seriously, we should at least do #3, and as a backward incompatible change
> at least _consider_ doing #2 (I think #4 is merely an implementation detail
> of #2), and if list reaches concensus in favor of such a change, come up
> with a transition plan and do so in the 1.7.0 release.

Hm, I always treated --no-commit as a way of saying: I think this merge
might cause semantic problems, please let me fix those directly, instead
of having to deal with 'commit --amend' and 'diff HEAD^ HEAD'.

Obviously, in case of a fast-forward such semantic problems aren't to be
expected, and I've just been wrong in my expectations. And then I'm
happy with the fast-forward. I'd _not_ be happy if a merge commit would
be forced (that's what #4 was about, right? deny_non_fast_forward only
appears in builtin-receive-pack.c, so I guess allow_fast_forward=0
was meant...).

#2 would be ok with me, I guess. I expected the wrong thing from the
merge, so git may complain, though it means more typing ;-)

Björn