Re: [PATCH] fmt-merge-msg: show those involved in a merged series

Jonathan Nieder [off-list ref] writes:

....  As a person reading the history, I admit I don't like it.
I enjoyed being able to get a simple overview at a higher level of
what has been happening in "pu" with "git log --merges junio/pu" or
"git log --first-parent junio/pu", and these extra lines before and
adjacent to the "* name of topic:" header interfere with that.

I'd hold making that judgement for a several weeks until my eyes get
used to if I were you. I've seen that people (including myself)
react really badly to _any_ change and make loud noises (including
"we will never get used to this updated output, it is horrible!"),
and then eventually get used to it as if nothing happened, and that
happened often enough recently.

In any case, if you only look at "git log --first-parent" output and
search for your own topic, it of course is useless to see your name
there, as you already know.

By contrast, the
...
descriptions in Linus's repo are very pleasant.

When you compare Linus's history and my history between master..pu,
you are literally comparing apples and oranges.

The merges between master..pu are made several times a day, with a
series of mechanical "merge --no-edit" process and automated tree
tweaking (including but not limited to rerere).  The purpose of
these merges is primarily to reduce the risk of mismerges to master
(and next to a lessor degree), especially when one topic among many
that have been cooking between master..pu gets closer to graduation.
By shuffling the order of topics that are merged between master..pu
so that topics close to graduation come earlier in the fully rebuilt
pu, a mid-point in master..pu is verified to exactly match the tree
of next (otherwise you may have spotted a mismerge to next, and I
did spot a few mismerges to next this way). This also allows earlier
parts of the master..pu to be tested individually.

The purpose of these merges is _not_ about describing what the side
branches are about. Unlike Linus's lieutenants' "for-linus" branch
names, the branch names are often enough to describe that they are.

On the other hand, the merges on Linus's tree are etched in stone,
and he has every incentive to record what happened in the side
branch for the _last_ time with carefully chosen words.

Having said that, I tweaked the automated rebuilding procedure a bit
today, and made it annotate these merges with snippets from the
branch description in the "What's cooking" document, so the commits
on master..pu are hopefully "very pleasant"ly annotated.  This not
only prettifies the merges between master..pu, but more importantly,
would save effort to explain the merges when a topic finally hits
master. If I have a good description in "What's cooking", I can then
reuse it in these merges and also in the release notes.

The update to the rebuild procedure is not published yet. I'll be
playing with it for a few days before publishing the changes.

Junio C Hamano wrote:

Having said that, I tweaked the automated rebuilding procedure a bit
today, and made it annotate these merges with snippets from the
branch description in the "What's cooking" document, so the commits
on master..pu are hopefully "very pleasant"ly annotated.

Yes, that helps.

I also wonder if it would make sense to change

	Merge branch 'mm/push-default-switch-warning' into pu

	... description ...

	By Matthieu Moy
	* mm/push-default-switch-warning:
	  push: start warning upcoming default change for push.default

to

	Merge branch 'mm/push-default-switch-warning' into pu

	... description ...

	* 'mm/push-default-switch-warning' by Matthieu Moy:
	  push: start warning upcoming default change for push.default

which makes the author less distractingly prominent (since they will
be right there in the log soon later once we hit the commits
themselves) and makes the start of the list of commits easier to find
by eye.  This is what I was inarticulately hinting at in my message
before.  With two authors it still looks reasonable:

	* 'jc/pickaxe-ignore-case' by Junio C Hamano (2) and Ramsay Jones (1):
	  ctype.c: Fix a sparse warning
	  pickaxe: allow -i to search in patch case-insensitively
	  grep: use static trans-case table

If this seems worth a patch, I'd be glad to try it (and even gladder
to learn that someone else already tried it).

Jonathan

Am 3/13/2012 2:55, schrieb Junio C Hamano:

I'd hold making that judgement for a several weeks until my eyes get
used to if I were you. I've seen that people (including myself)
react really badly to _any_ change and make loud noises (including
"we will never get used to this updated output, it is horrible!"),
and then eventually get used to it as if nothing happened, and that
happened often enough recently.

I can buy that. I won't mind reading the new lines in foreign projects,
but in at least one project I'm working in I prefer not to have these new
lines in the merge commit messages. Can it be opted-out?

-- Hannes

Johannes Sixt [off-list ref] writes:

Am 3/13/2012 2:55, schrieb Junio C Hamano:

quoted

I'd hold making that judgement for a several weeks until my eyes get
used to if I were you. I've seen that people (including myself)
react really badly to _any_ change and make loud noises (including
"we will never get used to this updated output, it is horrible!"),
and then eventually get used to it as if nothing happened, and that
happened often enough recently.

I can buy that.

Assuming "that" refers to my "hold ... for several weeks", let me stop
reading right here.  We can talk about the rest of your message in several
weeks.

Am 3/13/2012 19:26, schrieb Junio C Hamano:

Johannes Sixt [off-list ref] writes:

quoted

I can buy that.

Assuming "that" refers to my "hold ... for several weeks", let me stop
reading right here.  We can talk about the rest of your message in several
weeks.

No, "that" refers to "make loud noises (including "we will never get used
to this updated output, it is horrible!"), and then eventually get used to
it as if nothing happened". Nevertheless, I would like to opt-out of the
new behavior for my own projects even if my eyes will have been trained to
see the new lines in the history of foreign projects.

-- Hannes

The new "credit people" feature in fmt-merge-msg changed the format of
a typical "merge --log" message from

	Merge branch for-upstream of git://git.example.com/subsystem

	* for-upstream: (51 commits)
	  foo: bar baz

to

	Merge branch for-upstream of git://git.example.com/subsystem

	By C O Ntributor
	* for-upstream: (51 commits)
	  foo: bar baz

The message feels more natural with a line of breathing room before
the list of one-line descriptions headed by the branch name, like
messages already get when attached to a merge by "fmt-merge-msg -m" or
automatically incorporated during a merge of an annotated tag.

Signed-off-by: Jonathan Nieder <redacted>
---
Hi again,

Junio C Hamano wrote:

I'd hold making that judgement for a several weeks until my eyes get
used to if I were you.

Turns out my reaction is pretty much the same as before.  I still like
the idea and still am distracted by the spacing.  After the small
patch below, the log seems peaceful again and I am totally fine with
it.

	Merge branch for-upstream of git://git.example.com/subsystem

	By Fred the Clown

	* for-upstream: (51 commits)
	  foo: bar baz

Hope that helps,
Jonathan

 builtin/fmt-merge-msg.c  |    5 +++++
 t/t6200-fmt-merge-msg.sh |    9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 1bc6b8b8..8f228781 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c

@@ -264,6 +264,8 @@ static void add_people_info(struct strbuf *out,
 			    struct string_list *authors,
 			    struct string_list *committers)
 {
+	size_t pos = out->len;
+
 	if (authors->nr)
 		qsort(authors->items,
 		      authors->nr, sizeof(authors->items[0]),

@@ -275,6 +277,9 @@ static void add_people_info(struct strbuf *out,
 
 	credit_people(out, authors, 'a');
 	credit_people(out, committers, 'c');
+
+	if (out->len > pos)
+		strbuf_addch(out, '\n');
 }
 
 static void shortlog(const char *name,

diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 9b50f54c..d39417ba 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh

@@ -104,6 +104,7 @@ test_expect_success '[merge] summary/log configuration' '
 
 	By Another Author (3) and A U Thor (2)
 	via Another Committer
+
 	* left:
 	  Left #5
 	  Left #4

@@ -151,6 +152,7 @@ test_expect_success 'merge.log=3 limits shortlog length' '
 
 	By Another Author (3) and A U Thor (2)
 	via Another Committer
+
 	* left: (5 commits)
 	  Left #5
 	  Left #4

@@ -168,6 +170,7 @@ test_expect_success 'merge.log=5 shows all 5 commits' '
 
 	By Another Author (3) and A U Thor (2)
 	via Another Committer
+
 	* left:
 	  Left #5
 	  Left #4

@@ -192,6 +195,7 @@ test_expect_success '--log=3 limits shortlog length' '
 
 	By Another Author (3) and A U Thor (2)
 	via Another Committer
+
 	* left: (5 commits)
 	  Left #5
 	  Left #4

@@ -209,6 +213,7 @@ test_expect_success '--log=5 shows all 5 commits' '
 
 	By Another Author (3) and A U Thor (2)
 	via Another Committer
+
 	* left:
 	  Left #5
 	  Left #4

@@ -240,6 +245,7 @@ test_expect_success 'fmt-merge-msg -m' '
 
 	By Another Author (3) and A U Thor (2)
 	via Another Committer
+
 	* ${apos}left${apos} of $(pwd):
 	  Left #5
 	  Left #4

@@ -273,6 +279,7 @@ test_expect_success 'setup: expected shortlog for two branches' '
 
 	By Another Author (3) and A U Thor (2)
 	via Another Committer
+
 	* left:
 	  Left #5
 	  Left #4

@@ -398,6 +405,7 @@ test_expect_success 'merge-msg two tags' '
 
 	By Another Author (3) and A U Thor (2)
 	via Another Committer
+
 	* tag ${apos}tag-l5${apos}:
 	  Left #5
 	  Left #4

@@ -428,6 +436,7 @@ test_expect_success 'merge-msg tag and branch' '
 
 	By Another Author (3) and A U Thor (2)
 	via Another Committer
+
 	* left:
 	  Left #5
 	  Left #4

-- 
1.7.10.1

Jonathan Nieder [off-list ref] writes:

Turns out my reaction is pretty much the same as before.  I still like
the idea and still am distracted by the spacing.  After the small
patch below, the log seems peaceful again and I am totally fine with
it.

	Merge branch for-upstream of git://git.example.com/subsystem

	By Fred the Clown

	* for-upstream: (51 commits)
	  foo: bar baz

Two possible factors that may not be visible in the above example are

 (1) hand-written description of the merge itself by the integrator; and
 (2) octopus merges.

With both of these elements, with a blank line after the submaintainer and
the contributor attribution, a sample log output would look like this:

        Merge branches fix-foo and fix-bar of git://git.example.com/subsystem

        Two last minute fixes from Fred, so that we won't have to
        scramble and tell people to upgrade again immediately after
        the upcoming release.

        By Fred the Clown

        * fix-foo: (2 commits)
          foo: fix forboz
          foo: reindent

        By Fred the Clown

        * fix-bar: (1 commit)
          bar: fix nitfol

The attribution to the submaintainer and contributors, the name of the
branch merged, and the list of the individual changes form a single unit
of information "What was done for us by whom".  At least to me, the above
is easier to see without the additional blank line; the even-spacing
before and after the attribution line makes it harder to see where the
boundary between "description by the integrator on the merge" and
"information on the work that was done on the first branch that was
merged" is (and the boundary between the first and the second work, if in
an octopus).

We could add another blank line before the "credit" line.  We would have
two blank lines that separates the integrator comment and the per-branch
block, and also have two blank lines between the per-branch blocks, making
it easy again to see where the boundaries are.

But I do not know if it is an improvement from the current output before
your patch, or if it is just wasting vertical space.

On Fri, May 11, 2012 at 3:46 PM, Junio C Hamano [off-list ref] wrote:

But I do not know if it is an improvement from the current output before
your patch, or if it is just wasting vertical space.

So I tend to edit out the "By Xyzzy" and "via So-and-so" lines,
although I do find them useful for actually writing the message.

However, if I were to keep them, I think I'd prefer the extra line
that Jonathan added.

What I *would* like, though, is that the "via So-and-so" shows up on
the same line as the "By Xyzzy", probably together with appropriate
word-wrapping (using strbuf_add_wrapped_text()?)

Btw, the counting of commits is broken for the merge people. Do this
in the kernel tree, just to see an example of the breakage:

    git checkout -b test-merges 59068e369b6a
    git merge e9e7183fd267

and see the suggested people counts:

    By Mark Brown (2) and others
    via Takashi Iwai (3) and Liam Girdwood (1)

and those "via" numbers make very little sense.

So the "By" number makes sense (counting non-merges). But the "via"
numbers are just odd. Seven of the commits were committed by Mark
Brown, but he's not mentioned in "via". Presumably because he's
already mentioned in the "By" line. But Mark was actually *more* of a
"via" person than Liam was, although Takashi is perhaps the most
important one because he's the "latest" one things flowed through.

I dunno. But it looks odd, and the above is not the only example of
"those counts don't make sense".

Anyway, I think the "via" line is odd the way it is. Lower-case "via"
implies that it's a continuation of the "By" sentence, but then it's
given a line of its own. Hmm?

                   Linus


                       Linus

Hi again,

Junio C Hamano wrote:

Jonathan Nieder [off-list ref] writes:

quoted

                                                          I still like
the idea and still am distracted by the spacing.  After the small
patch below, the log seems peaceful again and I am totally fine with
it.

	Merge branch for-upstream of git://git.example.com/subsystem

	By Fred the Clown

	* for-upstream: (51 commits)
	  foo: bar baz

Two possible factors that may not be visible in the above example are

 (1) hand-written description of the merge itself by the integrator; and
 (2) octopus merges.

I've had way less time than I would like recently, but since a release
seems to be approaching and nothing has happened, let's revisit this.

To summarize the previous discussion:

 * Hannes prefers not to see the By and Via info in the merge message
   at all.

 * Linus always reformats and paraphrases so the exact format is not
   too important for him.  He has concerns about the accuracy of the
   Via line.

 * Jonathan thinks the By info is ok as part of a merge message but
   finds it jarring when pressed up against the --log summary.

 * Junio mentioned what sounds like anothing infelicity in the current
   format: for octopus merges, there is not just one list of authors
   and submaintiners summarizing what the merge commit does, but
   separate By and Via lines for each branch being merged.

Since everyone seems to agree that it is best when the integrator
sanity-checks the author info and rearranges it to taste, why not make
it commented by default, for example like this?

	Merge branch for-upstream of git://git.example.com/subsystem

	Foo, bar, baz.

	#
	# By Fred the Clown
	#
	* for-upstream: (51 commits)
	  qux: quux

Jonathan

Jonathan Nieder wrote:

 * Junio mentioned what sounds like anothing infelicity in the current
   format: for octopus merges, there is not just one list of authors
   and submaintiners summarizing what the merge commit does, but
   separate By and Via lines for each branch being merged.

Sorry for the lack of clarity.  This should say

	                           ... another infelicity ...
	... submaintainers summarizing ...

In other words, I think the objection given to the patch upthread was
a real one, but that a more appropriate fix than using strange spacing
would be to combine statistics from all branches being merged.

Thanks for your thoughtfulness.
Jonathan