git merge branch --no-commit does commit fast forward merges

Hi Git Mailinglist,

git merge branch --no-commit does commit fast forward merges leaving users no way to change the merge results. The command only works as expected when also adding the --no-ff flag. Looking at the help text of the --no-commit flag I think that this might be a unintended.

I've tested this on git 2.8.0.

This bug was first reported to magit (https://github.com/magit/magit/issues/2620) whose maintainer then suggested that it might be a git bug.

Best regards,
Christoph

-------------------------------------------------------
Christoph Paulik
Twitter, Github: @cpaulik
PGP: 8CFC D7DF 2867 B2DC 749B  1B0A 6E3B A262 5186 A0AC

To be fair, the user can undo it with "git reset HEAD@{1}" I believe?

Thanks,
Jake

Hi Christoph,

No, this is not a bug. Please note that a fast-forward does not perform a commit at all (and therefore "does commit fast forward" is an inaccurate description).

Then you need to fix your expectations ;-)

A fast-forward *avoids* committing altogether. In that light, the current behavior is correct.

Ciao,
Johannes

I *think* the core of this problem is that the intent of the end-user does not align with the command options available.

In this use case (as far as I can tell), the user wants to see what the result of a merge from somewhere else will look like, without changing their HEAD.

While you are correct in saying a fast-forward does not create any new commits, for the user it certainly looks like a whole slew of new commits have been added. Moreover, the nature of the option means that the user has to investigate if the merge is a fast-forward in order to know what the outcome of the command will be.

If the merge is a fast-forward, --no-commit has no effect on the outcome. If the merge is not a fast-forward, --no-commit has a huge effect on the outcome.

If I see a --no-commit option, as an inexperienced user, I would be quite surprised to find my HEAD changed after using it. It would be far more intuitive, for that user, for --no-commit to imply --no-ff however I suspect that such a change may well cause more problems then it fixes.

What I wonder is, in what situation is the current behaviour is desirable?

While I agree that the option works as designed, I think its behaviour is more surprising to the end user then it should be.

Regards,

Andrew Ardill

My expectations from what should happen came mainly from the description of the --no-commit flag in the help:

With --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.

So in the case of a fast-forward the flag does not pretend that the merge failed.

Regards,
Christoph

-------------------------------------------------------
Christoph Paulik
Twitter, Github: @cpaulik
PGP: 8CFC D7DF 2867 B2DC 749B  1B0A 6E3B A262 5186 A0AC

Yes, I think the mis-alignment in expectations comes from a technicality in the description you quote. The fast forward is in some ways not really counted as a true merge, and no new commits are created.

Thus, the merge progresses up to the point where a merge resolution would have to take place, realises that there is no merge resolution to do (it's just a fast forward!) and so exits out. Unfortunately, a side effect of this is that the fast-forward has already happened and so you are left with something different from what was expected.

I do think that the --no-commit option should imply --no-ff (as this would make the behaviour consistent for end-users). I don't know if this is something that would break scripts etc, but if so you could make it implied only if we detect a terminal or something like is done in other places.

Regards,

Andrew Ardill

Looking at 123ee3ca (Add --no-commit to git-merge/git-pull., 2005-11-01) and $gmane/10998 [*1*], it is clear that "--no-commit" was never meant as a "preview of what would happen". The original documentation update at 37465016 (Documentation: -merge and -pull:
describe merge strategies., 2005-11-04) was not great, but was clarified at d8ae1d10 (Document the --no-commit flag better, 2005-11-04), and that version of text survives to this day.

The real reason why "--no-commit" was added was because back then "git commit --amend" did not even exist; it appeared only at b4019f04 (git-commit --amend, 2006-03-02).

What is (and was back then) the recommended way to see what changes merging the other branch brings in to your branch, then?

There are at least three ways, all of which are better suited than "--no-commit".

When you want to study and understand what changes other branch made since it forked from what you are working on, then

    $ git diff ...other_branch

would give you the change as a single ball of wax [*2*].

If you want to see individual changes explained by their authors, you can also do

    $ git log -p ..other_branch

Finally, if you want to see what the merge result would look like, you just do the merge. Advancing the HEAD by one commit and then going back once you are done is a cheap operation. If you want to avoid updating your branch for real, these days you can even do so on a detached HEAD, unlike old days back when there was not even "commit --amend".

    $ git checkout HEAD^0
    $ git merge other_branch

    $ git diff ORIG_HEAD     ;# what changed overall?
    $ git log -p ORIG_HEAD.. ;# inspect individual changes

    $ git checkout - ;# come back to the original branch

If we were living in an ideal world where "git commit --amend" were already there in November 2005, we wouldn't have "merge --no-commit" or "pull --no-commit" in our system today, and in such a world, I would agree that "try to populate the working tree and the index with result of a hypothetical merge and stop without updating HEAD nor creating MERGE_HEAD, only to show what would happen if I merged" option could be a useful addition to these two commands. And we may choose to call such an option "--no-commit". I agree that such an option should probably imply "--no-ff".

But we are not living in that world. Making "--no-commit" (which is not that "try to populate and show" command) imply "--no-ff" will break existing scripts. And unlike cosmetic things like "do we show in color", changing the behaviour of a command in a fundamental way based on term and istty() is a sure way to make commands harder to understand ("it works this way from the terminal, but it works differently in my script. what is going on?" is not a question we are better off not seeing on this list).

Thanks.

[Notes and References]

*1* http://thread.gmane.org/gmane.comp.version-control.git/10998

*2* Notice the three dots; it is a short-hand for

    $ git diff ^$(git merge-base HEAD other_branch) other_branch

Having said all that, there is one change we might want to consider, with a plan to transition to cope with backward incompatibility.

A user who uses "--no-commit" does so with the intention to record a resulting merge after amending the merge result in the working tree. But there is nothing to amend and record, if the same "git merge" without "--no-commit" wouldn't have created a merge commit (there are two cases: (1) the other branch is a descendant of the current branch, (2) the other branch is an ancestor of the current branch).

And the user would want to know that before doing further damange to his history, so we may want to start warn when "--no-commit" fast-forwarded or succeeded with "already up-to-date", with deprecation notice, and eventually want to make it an error.

Those who want to do a scripted

	git merge --no-commit "$1" &&
        autoedit "$1" &&
        git commit

(where the script takes a branch name $1 and uses auto-edit to further record the fact that a branch $1 was merged to somewhere in the contents) would already be buggy if it wants to force no-ff, and will get a warning (and later an error), with such a change. And fixing the script to add "--no-ff" next to "--no-commit" will also stop the new warning/error.

It is true that we do not make a commit when we are asked to do "merge --squash", and the code does so by setting option_commit variable to zero when seeing the squash option. But this made it impossible to see from the value of option_commit if --no-commit was given from the command line, or --squash turned it off.

We check for the value of option_commit at only two places. Check for the value of squash at them, too.

---

* Just a preliminary clean-up for the next one which is on topic.

 builtin/merge.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

index bf2f261..de9d1b6 100644
@@ -1237,11 +1237,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (verbosity < 0)
 		show_diffstat = 0;
 
-	if (squash) {
-		if (fast_forward == FF_NO)
-			die(_("You cannot combine --squash with --no-ff."));
-		option_commit = 0;
-	}
+	if (squash && fast_forward == FF_NO)
+		die(_("You cannot combine --squash with --no-ff."));
 
 	if (!argc) {
 		if (default_to_upstream)

@@ -1449,10 +1446,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * We are not doing octopus and not fast-forward.  Need
 		 * a real merge.
 		 */
-	else if (!remoteheads->next && !common->next && option_commit) {
+	else if (!remoteheads->next && !common->next && option_commit && !squash)
 		/*
 		 * We are not doing octopus, not fast-forward, and have
-		 * only one common.
+		 * only one common.  And we do want to create a new commit.
 		 */
 		refresh_cache(REFRESH_QUIET);
 		if (allow_trivial && fast_forward != FF_ONLY) {

@@ -1535,7 +1532,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		ret = try_merge_strategy(use_strategies[i]->name,
 					 common, remoteheads,
 					 head_commit, head_arg);
-		if (!option_commit && !ret) {
+		if ((!option_commit || squash) && !ret) {
 			merge_was_ok = 1;
 			/*
 			 * This is necessary here just to avoid writing

2.8.1-491-g88b9e4a

Hi Junio,

I think it would make for a nice cleanup anyways.

Ciao,
Dscho

There is a missing open-brace that causes a compilation error, though ;-)

Hi Junio,

Uh oh ;-) I guess I did not really test it...

Ciao,
Dscho

A user who uses "--no-commit" does so with the intention to record a resulting merge after amending the merge result in the working tree. But there is nothing to amend and record, if the same "git merge" without "--no-commit" wouldn't have created a merge commit (there are two cases: (1) the other branch is a descendant of the current branch, (2) the other branch is an ancestor of the current branch).

The user would want to know that before doing further damange to his history. When "merge --no-commit" fast-forwarded or succeeded with "already up-to-date" or "fast-forward", give a warning message.

We may want to turn this into a die() after a transition period.

---

* The necessary update to avoid end-user mistake would look like this. I am not queuing this or further working on it myself, as I am not sure if it is all that useful.

 builtin/merge.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

index f641751..66c536d 100644
@@ -1157,6 +1157,15 @@ static struct commit_list *collect_parents(struct commit *head_commit,
 	return remoteheads;
 }
 
+static void no_commit_impossible(const char *message)
+{
+	if (!option_commit) {
+		warning("%s\n%s", _(message),
+			_("--no-commit is impossible"));
+		warning(_("In future versions of Git, this will become an error."));
+	}
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];

@@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * If head can reach all the merge then we are up to date.
 		 * but first the most common case of merging one remote.
 		 */
+		no_commit_impossible(_("Already up-to-date"));
 		finish_up_to_date("Already up-to-date.");
 		goto done;
 	} else if (fast_forward != FF_NO && !remoteheads->next &&

@@ -1412,6 +1422,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		struct strbuf msg = STRBUF_INIT;
 		struct commit *commit;
 
+		no_commit_impossible(_("Fast-forward"));
 		if (verbosity >= 0) {
 			char from[GIT_SHA1_HEXSZ + 1], to[GIT_SHA1_HEXSZ + 1];
 			find_unique_abbrev_r(from, head_commit->object.oid.hash,

@@ -1488,6 +1499,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			}
 		}
 		if (up_to_date) {
+			no_commit_impossible(_("Already up-to-date"));
 			finish_up_to_date("Already up-to-date. Yeeah!");
 			goto done;
 		}

2.8.1-491-g88b9e4a

Am 26.04.2016 um 23:37 schrieb Junio C Hamano:

Whoever picks up this patch, be warned that the i18n coding should be corrected:

It would be more legible, but because there are so few callsites in an already shallow callchain, I do not think it makes that much of a difference in this codepath either way.

One that explains why --no-commit is impossible in warning, and the other is the final report of what happened, so yes.

While we are using warning(), we probably do want to. When we switch to die() at a major version boundary, we don't.

s/damange/damage/

Hi Junio,

That would be the best name once we start dying in there. It might be still better, even while we merely warn but let it pass, than the double negative. Or it may not. I dunno.

Hi Junio,

Actually, I should admit that I was really puzzled by the name at first. I thought that some commits were impossible, but the function said that no commit was impossible. So I thought: but what if a commit references itself as parent, would that not be impossible? But actually, once SHA-1 collision attacks become feasible, I guess it would not be impossible. Making for an excellent attack vector, say, on repository hosting sites (which would now be stuck in infinite loops due to a violation of the temporal prime directive).

So yeah, this was my thought process when I read no_commit_impossible.

;-)

Ciao,
Dscho

We both know that the original name was terrible now. It was meant to mean "it is impossible to honor --no-commit option", nothing more, and left the door open for deciding what should happen when that condition holds (either "warn but succeed the merge anyway" or "die to refuse"), but it is clear that the name did not convey that successfully.