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

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

Tomas Carnecky <tom@dbservice.com> writes:

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.

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.

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: 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(-)

Greg Price <price@ksplice.com> writes:

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.

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.

On 09/19/2009 01:05 AM, Junio C Hamano wrote:

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

Paolo

On 2009.09.13 00:30:49 -0700, Junio C Hamano wrote:

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