git-merge adds test to a message -- bug?

git-merge adds test to a message -- bug?

[Jörg Sommer]

Hi,

is it correct, that this test fails?

diff --git a/t/t6032-merge-message.sh b/t/t6032-merge-message.sh
new file mode 100755
index 0000000..97157bd
--- /dev/null
+++ b/t/t6032-merge-message.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='test for merge message'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	: > file1 &&
+	git add file1 &&
+        test_tick &&
+	git commit -m "commit 1" &&
+	: > file2 &&
+	git add file2 &&
+        test_tick &&
+	git commit -m "commit 2" &&
+
+	git checkout -b new-branch HEAD~1 &&
+	: > file3 &&
+	git add file3 &&
+        test_tick &&
+	git commit -m "commit 3"
+'
+
+test_expect_success merge 'git merge -m master-merge master'
+
+test_expect_failure 'merge message check' '
+	test "$(git cat-file commit HEAD | sed "1,/^\$/d")" = master-merge
+'
+
+test_done

Bye, Jörg.
-- 
Unsere Zweifel sind Verräter und oft genug verspielen wir den möglichen
Gewinn, weil wir den Versuch nicht wagen.

Re: git-merge adds test to a message -- bug?

[Junio C Hamano]

Jörg Sommer <joerg@alea.gnuu.de> writes:

> is it correct, that this test fails?

Interesting.

The following patch will change the bahaviour to what your test script
expects, but by looking at the original code, you can tell that it is
indeed the intended behaviour of "-M' to _allow_ you to _prepend_ your own
message to the standardized "Merge these" message, not _replace_ with it.

I unfortunately do not recall why _prepend_, and not _replace_, had to be
the right behaviour.  Unless somebody can point out there was a valid
reason but that reason is not relevant anymore, I am reluctant to apply
the patch below this late in the cycle.

 git-merge.sh |   27 +++++++++++++++------------
 1 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/git-merge.sh b/git-merge.sh
index 7dbbb1d..a5ce649 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -250,17 +250,19 @@ else
 	# We are invoked directly as the first-class UI.
 	head_arg=HEAD
 
-	# All the rest are the commits being merged; prepare
-	# the standard merge summary message to be appended to
-	# the given message.  If remote is invalid we will die
-	# later in the common codepath so we discard the error
-	# in this loop.
-	merge_name=$(for remote
-		do
-			merge_name "$remote"
-		done | git fmt-merge-msg
-	)
-	merge_msg="${merge_msg:+$merge_msg$LF$LF}$merge_name"
+	if test -z "$merge_msg"
+	then
+		# All the rest are the commits being merged; prepare
+		# the standard merge summary message to be appended to
+		# the given message.  If remote is invalid we will die
+		# later in the common codepath so we discard the error
+		# in this loop.
+		merge_msg=$(for remote
+			do
+				merge_name "$remote"
+			done | git fmt-merge-msg
+		)
+	fi
 fi
 head=$(git rev-parse --verify "$head_arg"^0) || usage
 

Re: git-merge adds test to a message -- bug?

[Johannes Schindelin]

Hi,

On Sun, 23 Mar 2008, Junio C Hamano wrote:

> I unfortunately do not recall why _prepend_, and not _replace_, had to be
> the right behaviour.

http://article.gmane.org/gmane.comp.version-control.git/31896/match=git+merge+make+usable

Hth,
Dscho

Re: git-merge adds test to a message -- bug?

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 23 Mar 2008, Junio C Hamano wrote:
>
>> I unfortunately do not recall why _prepend_, and not _replace_, had to be
>> the right behaviour.
>
> http://article.gmane.org/gmane.comp.version-control.git/31896/match=git+merge+make+usable
>
> Hth,

Ok, it helped.

So it was "my suspicion that people who would want to pass -m would want
it to behave this way".

I do not care deeply either way myself, as I never have found use for -m
to the merge command, but I think it could have been argued either way.
"If you want to include the standard message, you can do so yourself by
running the fmt-merge-msg yourself" would have been a valid argument too,
even though it would make it a lot more cumbersome if many people wanted
the standard message anyway.

I do not see any objections to the suspicion back then in the thread,
there may have been discussions and user requests that made me suspect
that nearby as well, and ever since the feature was defined that way there
wasn't any objection until Jörg brought this up.

So I'd say we should let the sleeping dog lie for now.

Re: git-merge adds test to a message -- bug?

[Jörg Sommer]

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

Hallo Junio,

Junio C Hamano schrieb am Mon 24. Mar, 09:45 (-0700):
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > On Sun, 23 Mar 2008, Junio C Hamano wrote:
> >
> >> I unfortunately do not recall why _prepend_, and not _replace_, had to be
> >> the right behaviour.
> >
> > http://article.gmane.org/gmane.comp.version-control.git/31896/match=git+merge+make+usable
> >
> > Hth,
> 
> Ok, it helped.
> 
> So it was "my suspicion that people who would want to pass -m would want
> it to behave this way".
> 
> I do not care deeply either way myself, as I never have found use for -m
> to the merge command, but I think it could have been argued either way.

I would like to argue for the replace way. :) Take git rebase -p as an
example. If a merge is included in the rebase, it's redone with git merge
-m. Because git rebase works with detached heads you get merge messages
like this:

commit 580c95c6bb4bb74bdbf6776ca816560690c16c5d
Merge: 62b68ef... 9604163...
Author: A U Thor <author@example.com>
Date:   Thu Apr 7 15:26:13 2005 -0700

    Merge branch 'to-be-preserved' into to-be-rebased
    
    Merge commit '96041635cd9e0bd999384c1c30d7df40002a0742' into HEAD

This is an example from the test t3404. Simply run git log -g and search
for merges.

Maybe a new option -M for merge could help? -m with the old behaviour to
prepend and -M to replace.

Bye, Jörg.
-- 
IRC: Der [Prof. Andreas Pfitzmann, TU Dresden] hat gerade vorgeschlagen, sie
  sollen doch statt Trojanern die elektromagnetische Abstrahlung nutzen. Das
  sei nicht massenfähig, ginge ohne Eingriff ins System, sei zielgerichtet,
  und, der Hammer, das funktioniere ja bei Wahlcomputern schon sehr gut.

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: git-merge adds test to a message -- bug?

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1248 bytes --]

Hi,

On Tue, 25 Mar 2008, Jörg Sommer wrote:

> Junio C Hamano schrieb am Mon 24. Mar, 09:45 (-0700):
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > > On Sun, 23 Mar 2008, Junio C Hamano wrote:
> > >
> > >> I unfortunately do not recall why _prepend_, and not _replace_, had 
> > >> to be the right behaviour.
> > >
> > > http://article.gmane.org/gmane.comp.version-control.git/31896/match=git+merge+make+usable
> > >
> > > Hth,
> > 
> > Ok, it helped.
> > 
> > So it was "my suspicion that people who would want to pass -m would 
> > want it to behave this way".
> > 
> > I do not care deeply either way myself, as I never have found use for 
> > -m to the merge command, but I think it could have been argued either 
> > way.
> 
> I would like to argue for the replace way. :) Take git rebase -p as an 
> example. If a merge is included in the rebase, it's redone with git 
> merge -m. Because git rebase works with detached heads you get merge 
> messages like this: [...]

That only means that the original author of rebase -p was a lazy bastard 
and did not use the proper way to call git-merge, namely

	git <msg> HEAD <the-other-branch>

Now, let's try git log -S to find out who that inexcusably lazy bum was.

Ciao,
Dscho

Re: git-merge adds test to a message -- bug?

[Jörg Sommer]

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

Hallo Johannes,

Johannes Schindelin schrieb am Tue 25. Mar, 21:04 (+0100):
> On Tue, 25 Mar 2008, Jörg Sommer wrote:
> > Junio C Hamano schrieb am Mon 24. Mar, 09:45 (-0700):
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > > > On Sun, 23 Mar 2008, Junio C Hamano wrote:
> > > >
> > > >> I unfortunately do not recall why _prepend_, and not _replace_, had 
> > > >> to be the right behaviour.
> > > >
> > > > http://article.gmane.org/gmane.comp.version-control.git/31896/match=git+merge+make+usable
> > > 
> > > So it was "my suspicion that people who would want to pass -m would 
> > > want it to behave this way".
> > > 
> > > I do not care deeply either way myself, as I never have found use for 
> > > -m to the merge command, but I think it could have been argued either 
> > > way.
> > 
> > I would like to argue for the replace way. :) Take git rebase -p as an 
> > example. If a merge is included in the rebase, it's redone with git 
> > merge -m. Because git rebase works with detached heads you get merge 
> > messages like this: [...]
> 
> That only means that the original author of rebase -p was a lazy bastard 
> and did not use the proper way to call git-merge, namely
> 
> 	git <msg> HEAD <the-other-branch>

It this really a proper way to call git merge? The manpage says:

    The second syntax (<msg> HEAD <remote>) is supported for historical
    reasons. Do not use it from the command line or in new scripts. It is
    the same as git merge -m <msg> <remote>.

And how would you pass the option for the strategy to this form? Git
rebase calls git merge with -s.

Bye, Jörg.
-- 
But in the case of "git revert", it should be an ancestor (or the user
is just insane, in which case it doesn't matter - insane people can do
insane things)
Linus Torvalds <alpine.LFD.1.00.0801041031590.2811@woody.linux-foundation.org>

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 189 bytes --]