Re: A possible fmt-merge-msg update?

Linus Torvalds [off-list ref] writes:

On Sun, Mar 4, 2012 at 7:17 PM, Junio C Hamano [off-list ref] wrote:

quoted

Having observed a handful of your recent merge messages, I am wondering if
it would help to teach fmt-merge-msg to include "from Wim Van Sebroeck" in
its output by taking the committer of the MERGE_HEAD into account.

Not worth the trouble?

Hmm. Maybe worth it.
...
It might be interesting if the pre-written commit message had the top
committer in a comment (the same way pulling a tag has the tag author
in the comment about the tag verification). That way the information
would be right there when I edit the message, and since it's correct
99% of the time it would make it easier to just edit it in the editor
than have to cut-and-paste it from the email. But because it's not a
sure thing,...

The attached would give me:

| Merge branch 'jl/maint-submodule-relative'
| 
| # Jens Lehmann (3) and Johannes Sixt (1)
| * jl/maint-submodule-relative:
|   submodules: fix ambiguous absolute paths under Windows
|   submodules: refactor computation of relative gitdir path
|   submodules: always use a relative path from gitdir to work tree
|   submodules: always use a relative path to gitdir

It would be a sure thing for the commit-authorship, so we could use
"By " instead of "# " above, but it should be obvious either way.

The existing test vectors need to be taught about this change if we
were to do something like this.

-- >8 --
Subject: [PATCH] fmt-merge-msg: show primary authors of a merged series

As we already walk the history of the branch that gets merged to
come up with a short log, let's label it with names of the primary
authors, so that the user who summarizes the merge can easily give
credit to them in the log message.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/fmt-merge-msg.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index c81a7fe..7eea066 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c

@@ -180,6 +180,63 @@ static void add_branch_desc(struct strbuf *out, const char *name)
 	strbuf_release(&desc);
 }
 
+static void record_author(struct string_list *authors, struct commit *commit)
+{
+	char name_buf[MAX_GITNAME], *name, *name_end;
+	struct string_list_item *elem;
+
+	name = strstr(commit->buffer, "\nauthor ");
+	if (!name)
+		return;
+	name += strlen("\nauthor ");
+	name_end = strchrnul(name, '<');
+	if (*name_end)
+		name_end--;
+	while (isspace(*name_end) && name <= name_end)
+		name_end--;
+	if (name_end < name || name + MAX_GITNAME <= name_end)
+		return;
+	memcpy(name_buf, name, name_end - name + 1);
+	name_buf[name_end - name + 1] = '\0';
+
+	elem = string_list_lookup(authors, name_buf);
+	if (!elem) {
+		elem = string_list_insert(authors, name_buf);
+		elem->util = (void *) 0;
+	}
+	elem->util = (void*)(((intptr_t)elem->util) + 1);
+}
+
+#define util_as_int(elem) ((intptr_t)((elem)->util))
+
+static int cmp_string_list_util_as_int(const void *a_, const void *b_)
+{
+	const struct string_list_item *a = a_, *b = b_;
+	return util_as_int(b) - util_as_int(a);
+}
+
+static void add_author_info(struct strbuf *out, struct string_list *authors)
+{
+	if (!authors->nr)
+		return;
+	qsort(authors->items, authors->nr, sizeof(authors->items[0]),
+	      cmp_string_list_util_as_int);
+
+	strbuf_addstr(out, "\n# ");
+	if (authors->nr == 1)
+		strbuf_addf(out, "%s", authors->items[0].string);
+	else if (authors->nr == 2)
+		strbuf_addf(out, "%s (%d) and %s (%d)",
+			    authors->items[0].string,
+			    (int)util_as_int(&authors->items[0]),
+			    authors->items[1].string,
+			    (int)util_as_int(&authors->items[1]));
+	else
+		strbuf_addf(out, "%s (%d) and others",
+			    authors->items[0].string,
+			    (int)util_as_int(&authors->items[0]));
+}
+
 static void shortlog(const char *name,
 		     struct origin_data *origin_data,
 		     struct commit *head,

@@ -190,6 +247,7 @@ static void shortlog(const char *name,
 	struct commit *commit;
 	struct object *branch;
 	struct string_list subjects = STRING_LIST_INIT_DUP;
+	struct string_list authors = STRING_LIST_INIT_DUP;
 	int flags = UNINTERESTING | TREESAME | SEEN | SHOWN | ADDED;
 	struct strbuf sb = STRBUF_INIT;
 	const unsigned char *sha1 = origin_data->sha1;

@@ -212,6 +270,7 @@ static void shortlog(const char *name,
 		if (commit->parents && commit->parents->next)
 			continue;
 
+		record_author(&authors, commit);
 		count++;
 		if (subjects.nr > limit)
 			continue;

@@ -226,6 +285,7 @@ static void shortlog(const char *name,
 			string_list_append(&subjects, strbuf_detach(&sb, NULL));
 	}
 
+	add_author_info(out, &authors);
 	if (count > limit)
 		strbuf_addf(out, "\n* %s: (%d commits)\n", name, count);
 	else

@@ -246,6 +306,7 @@ static void shortlog(const char *name,
 	rev->commits = NULL;
 	rev->pending.nr = 0;
 
+	string_list_clear(&authors, 0);
 	string_list_clear(&subjects, 0);
 }
 

-- 
1.7.9.2.413.ge58a8e

On Mon, Mar 5, 2012 at 11:04 AM, Junio C Hamano [off-list ref] wrote:

The attached would give me:

So this isn't interesting to me.

Authorship is less relevant than submaintainership. So I'm more
interested in *committer* information than authorship information.

Of course, since you do it in branches that you maintain, to you
committer information is pointless. But I pull from submaintainers,
and then it really is the committer part that is way more relevant.

                          Linus

On Mon, Mar 05, 2012 at 12:33:42PM -0800, Linus Torvalds wrote:

On Mon, Mar 5, 2012 at 11:04 AM, Junio C Hamano [off-list ref] wrote:

quoted

The attached would give me:

So this isn't interesting to me.

Authorship is less relevant than submaintainership. So I'm more
interested in *committer* information than authorship information.

Of course, since you do it in branches that you maintain, to you
committer information is pointless. But I pull from submaintainers,
and then it really is the committer part that is way more relevant.

If you're interested in the sub-maintainer, and the sub-maintainer is
who you pulled from, then isn't the right solution to better annotate
the source of the pull? For the kernel workflow, that often comes in the
form of pulled tags; would providing the tagger in that case be helpful?
(it's often already included in the commit template via the
commented-out GPG output, but there might be many UIDs attached to a
given GPG key).

That wouldn't help the git.git workflow, of course, but I think you are
talking about two fundamentally different things. The kernel thing is
about annotating the source of the pull. The git.git thing (and Junio's
patch) is about summarizing the contents of the branch not just with the
subject lines, but also with the author's names[1].

But looking through some recent kernel merges, the useful new thing in
the message doesn't seem to me to be the _who_, but rather the _what_.
For example, from f3969bf7:

  Pull perf fixes from Ingo Molnar:
   "It contains three cherry-picked fixes from perf/core, which turned out
    to be more urgent than we originally thought."

So rather than focus on the identity of the sub-maintainer, perhaps a
more useful thing is to make it easier to pass information from a pull
request into the resulting merge message. We already have "git am" for
regular patches, and it relies on a few easy-to-generate microformats,
so it's natural to use with "git format-patch", your own custom script,
or even by hand.  Could we do the same thing and have a "git
apply-pull-request" (or something with a less horrible name)?

Ingo's original message looked like:

    From: Ingo Molnar [off-list ref]
    Subject: [GIT PULL] perf fixes

    Linus,

    Please pull the latest perf-urgent-for-linus git tree from:

       git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git perf-urgent-for-linus

       HEAD: b7c924274c456499264d1cfa3d44063bb11eb5db Merge tag 'perf-urgent-for-mingo' of
    git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/urgent

    It contains three cherry-picked fixes from perf/core, which 
    turned out to be more urgent than we originally thought.

     Thanks,

            Ingo

If this were instead formatted as:

  From: Ingo Molnar [off-list ref]
  Subject: [GIT PULL] perf fixes

  Here are three cherry-picked fixes from perf/core, which turned out to
  be more urgent than we originally thought.

  ---
    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git perf-urgent-for-linus

    HEAD: b7c924274c456499264d1cfa3d44063bb11eb5db Merge tag 'perf-urgent-for-mingo'
      of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/urgent

we could trivially convert that into the same commit message you ended
up writing.  The format is simple enough that people who aren't
running it through a script can read and write it, and we retain the
single line with the repo and ref name for those who want to just cut
and paste.

-Peff