A possible fmt-merge-msg update?

A possible fmt-merge-msg update?

[Junio C Hamano]

Subject: Re: Merge git://www.linux-watchdog.org/linux-watchdog
From: Junio C Hamano <junio@pobox.com>
Bcc: junio@pobox.com
--text follows this line--
Linus Torvalds <torvalds@linux-foundation.org> writes:

> Watchdog updates from Wim Van Sebroeck:
>
> * git://www.linux-watchdog.org/linux-watchdog:
>   watchdog: fix GETTIMEOUT ioctl in booke_wdt
>   watchdog: update maintainers git entry
>   watchdog: Fix typo in pnx4008_wdt.c
>   watchdog: Fix typo in Kconfig
>   watchdog: fix error in probe() of s3c2410_wdt (reset at booting)
>   watchdog: hpwdt: clean up set_memory_x call for 32 bit

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?

Re: A possible fmt-merge-msg update?

[Linus Torvalds]

On Sun, Mar 4, 2012 at 7:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

So the reason I've started to try to do it is that I had a person
email me about appreciating the new merge messages, but he also said
he wanted to know who I pulled from. And I agree that that makes sense
as a real piece of information.

It's not *necessarily* the same person as the committer info, though.
It probably matches a very high percentage of the time, but the pull
request doesn't always come from the person doing the commits. The x86
people have this "tip" thing, and while branches tend to be owned by
the people doing the pull request, it's not a given. And sometimes you
have a submaintainer who pulled from *his* submaintainer, and didn't
have any work of his own, so he asks me to pull something where the
top is not him, but his submaintainer.

So I really dunno.

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, it would be a comment-only thing where I can choose to
remove the '# ' in front of it. Maybe that would be the best of both
worlds - helping write a merge message without assuming it has to be
that way..

                       Linus

Re: A possible fmt-merge-msg update?

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sun, Mar 4, 2012 at 7:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> 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 <gitster@pobox.com>
---
 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

Re: A possible fmt-merge-msg update?

[Linus Torvalds]

On Mon, Mar 5, 2012 at 11:04 AM, Junio C Hamano <gitster@pobox.com> 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

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

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

Fair enough.

I do not think a simple population count on the committer field is a
good heuristics.  For example, I suspect that you would want to see
a lot more weight given to names of committers near the tip of the
history you are pulling from, iow, if your lieutenant pulled 100
commit series by a leaf contributor, you still want to see the name
of the lieutenant, not the leaf contributor, even the latter may
have far more commits.

This patch punts the deep thinking part and just picks the tip
committer and counts only merges in the history you are pulling in.
It may not be a very good heuristics, but the code structure should
be a good place to start.

Changes to the test vector in t6200 illustrate how the output should
look like.

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

Also infer the names of "lieutents" to help integrators at higher
level of the food-chain to give credit to them, by counting:

 * The committer of the 'tip' commit that is merged
 * The committer of merge commits that are merged

Often the first one gives the owner of the history being pulled, but
his last pull from his sublieutenants may have been a fast-forward,
in which case the first one would not be.  The latter rule will
count the integrator of the history, so together it might be a
reasonable heuristics.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/fmt-merge-msg.c         |  101 +++++++++++++++++++++++++++++++++++++--
 t/t6200-fmt-merge-msg.sh        |   32 +++++++++++--
 t/t7600-merge.sh                |    1 +
 t/t7604-merge-custom-message.sh |    1 +
 4 files changed, 128 insertions(+), 7 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index c81a7fe..b2465b4 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -27,6 +27,8 @@ int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 			merge_log_config = DEFAULT_MERGE_LOG_LEN;
 	} else if (!strcmp(key, "merge.branchdesc")) {
 		use_branch_desc = git_config_bool(key, value);
+	} else {
+		return git_default_config(key, value, cb);
 	}
 	return 0;
 }
@@ -180,6 +182,89 @@ static void add_branch_desc(struct strbuf *out, const char *name)
 	strbuf_release(&desc);
 }
 
+static void record_person(int which, struct string_list *people,
+			  struct commit *commit)
+{
+	char name_buf[MAX_GITNAME], *name, *name_end;
+	struct string_list_item *elem;
+	const char *field = (which == 'a') ? "\nauthor " : "\ncommitter ";
+
+	name = strstr(commit->buffer, field);
+	if (!name)
+		return;
+	name += strlen(field);
+	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(people, name_buf);
+	if (!elem) {
+		elem = string_list_insert(people, 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_people_count(struct strbuf *out, struct string_list *people)
+{
+	if (people->nr == 1)
+		strbuf_addf(out, "%s", people->items[0].string);
+	else if (people->nr == 2)
+		strbuf_addf(out, "%s (%d) and %s (%d)",
+			    people->items[0].string,
+			    (int)util_as_int(&people->items[0]),
+			    people->items[1].string,
+			    (int)util_as_int(&people->items[1]));
+	else if (people->nr)
+		strbuf_addf(out, "%s (%d) and others",
+			    people->items[0].string,
+			    (int)util_as_int(&people->items[0]));
+}
+
+static int committer_is_me(const char *name)
+{
+	int namelen = strlen(name);
+	const char *me = git_committer_info(IDENT_NO_DATE);
+	return (me && !memcmp(me, name, namelen) &&
+		!memcmp(me + namelen, " <", 2));
+}
+
+static void add_people_info(struct strbuf *out,
+			    struct string_list *authors,
+			    struct string_list *committers)
+{
+	if (authors->nr)
+		qsort(authors->items,
+		      authors->nr, sizeof(authors->items[0]),
+		      cmp_string_list_util_as_int);
+	if (committers->nr)
+		qsort(committers->items,
+		      committers->nr, sizeof(committers->items[0]),
+		      cmp_string_list_util_as_int);
+
+	strbuf_addstr(out, "\nBy ");
+	add_people_count(out, authors);
+	if (committers->nr == 1 &&
+	    committer_is_me(committers->items->string))
+		return;
+	strbuf_addstr(out, "\nvia ");
+	add_people_count(out, committers);
+}
+
 static void shortlog(const char *name,
 		     struct origin_data *origin_data,
 		     struct commit *head,
@@ -190,6 +275,8 @@ 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;
+	struct string_list committers = STRING_LIST_INIT_DUP;
 	int flags = UNINTERESTING | TREESAME | SEEN | SHOWN | ADDED;
 	struct strbuf sb = STRBUF_INIT;
 	const unsigned char *sha1 = origin_data->sha1;
@@ -199,7 +286,6 @@ static void shortlog(const char *name,
 		return;
 
 	setup_revisions(0, NULL, rev, NULL);
-	rev->ignore_merges = 1;
 	add_pending_object(rev, branch, name);
 	add_pending_object(rev, &head->object, "^HEAD");
 	head->object.flags |= UNINTERESTING;
@@ -208,10 +294,15 @@ static void shortlog(const char *name,
 	while ((commit = get_revision(rev)) != NULL) {
 		struct pretty_print_context ctx = {0};
 
-		/* ignore merges */
-		if (commit->parents && commit->parents->next)
+		if (commit->parents && commit->parents->next) {
+			/* do not list a merge but count committer */
+			record_person('c', &committers, commit);
 			continue;
-
+		}
+		if (!count)
+			/* the 'tip' committer */
+			record_person('c', &committers, commit);
+		record_person('a', &authors, commit);
 		count++;
 		if (subjects.nr > limit)
 			continue;
@@ -226,6 +317,7 @@ static void shortlog(const char *name,
 			string_list_append(&subjects, strbuf_detach(&sb, NULL));
 	}
 
+	add_people_info(out, &authors, &committers);
 	if (count > limit)
 		strbuf_addf(out, "\n* %s: (%d commits)\n", name, count);
 	else
@@ -246,6 +338,7 @@ static void shortlog(const char *name,
 	rev->commits = NULL;
 	rev->pending.nr = 0;
 
+	string_list_clear(&authors, 0);
 	string_list_clear(&subjects, 0);
 }
 
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 9a16806..b90015d 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -35,15 +35,18 @@ test_expect_success setup '
 
 	echo "l3" >two &&
 	test_tick &&
-	git commit -a -m "Left #3" &&
+	GIT_COMMITTER_NAME="Another Committer" \
+	GIT_AUTHOR_NAME="Another Author" git commit -a -m "Left #3" &&
 
 	echo "l4" >two &&
 	test_tick &&
-	git commit -a -m "Left #4" &&
+	GIT_COMMITTER_NAME="Another Committer" \
+	GIT_AUTHOR_NAME="Another Author" git commit -a -m "Left #4" &&
 
 	echo "l5" >two &&
 	test_tick &&
-	git commit -a -m "Left #5" &&
+	GIT_COMMITTER_NAME="Another Committer" \
+	GIT_AUTHOR_NAME="Another Author" git commit -a -m "Left #5" &&
 	git tag tag-l5 &&
 
 	git checkout right &&
@@ -99,6 +102,8 @@ test_expect_success '[merge] summary/log configuration' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left:
 	  Left #5
 	  Left #4
@@ -144,6 +149,8 @@ test_expect_success 'merge.log=3 limits shortlog length' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left: (5 commits)
 	  Left #5
 	  Left #4
@@ -159,6 +166,8 @@ test_expect_success 'merge.log=5 shows all 5 commits' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left:
 	  Left #5
 	  Left #4
@@ -181,6 +190,8 @@ test_expect_success '--log=3 limits shortlog length' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left: (5 commits)
 	  Left #5
 	  Left #4
@@ -196,6 +207,8 @@ test_expect_success '--log=5 shows all 5 commits' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left:
 	  Left #5
 	  Left #4
@@ -225,6 +238,8 @@ test_expect_success 'fmt-merge-msg -m' '
 	cat >expected.log <<-EOF &&
 	Sync with left
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* ${apos}left${apos} of $(pwd):
 	  Left #5
 	  Left #4
@@ -256,6 +271,8 @@ test_expect_success 'setup: expected shortlog for two branches' '
 	cat >expected <<-EOF
 	Merge branches ${apos}left${apos} and ${apos}right${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left:
 	  Left #5
 	  Left #4
@@ -263,6 +280,7 @@ test_expect_success 'setup: expected shortlog for two branches' '
 	  Common #2
 	  Common #1
 
+	By A U Thor
 	* right:
 	  Right #5
 	  Right #4
@@ -353,6 +371,7 @@ test_expect_success 'merge-msg tag' '
 	cat >expected <<-EOF &&
 	Merge tag ${apos}tag-r3${apos}
 
+	By A U Thor
 	* tag ${apos}tag-r3${apos}:
 	  Right #3
 	  Common #2
@@ -374,11 +393,14 @@ test_expect_success 'merge-msg two tags' '
 	cat >expected <<-EOF &&
 	Merge tags ${apos}tag-r3${apos} and ${apos}tag-l5${apos}
 
+	By A U Thor
 	* tag ${apos}tag-r3${apos}:
 	  Right #3
 	  Common #2
 	  Common #1
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* tag ${apos}tag-l5${apos}:
 	  Left #5
 	  Left #4
@@ -402,11 +424,14 @@ test_expect_success 'merge-msg tag and branch' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}, tag ${apos}tag-r3${apos}
 
+	By A U Thor
 	* tag ${apos}tag-r3${apos}:
 	  Right #3
 	  Common #2
 	  Common #1
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left:
 	  Left #5
 	  Left #4
@@ -431,6 +456,7 @@ test_expect_success 'merge-msg lots of commits' '
 		cat <<-EOF &&
 		Merge branch ${apos}long${apos}
 
+		By A U Thor
 		* long: (35 commits)
 		EOF
 
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 9e27bbf..e09b932 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -58,6 +58,7 @@ create_merge_msgs () {
 	} >squash.1-5-9 &&
 	echo >msg.nolog &&
 	{
+		echo "By A U Thor" &&
 		echo "* tag 'c3':" &&
 		echo "  commit 3" &&
 		echo
diff --git a/t/t7604-merge-custom-message.sh b/t/t7604-merge-custom-message.sh
index 89619cf..b84cea3 100755
--- a/t/t7604-merge-custom-message.sh
+++ b/t/t7604-merge-custom-message.sh
@@ -11,6 +11,7 @@ create_merge_msgs() {
 
 	cp exp.subject exp.log &&
 	echo >>exp.log "" &&
+	echo >>exp.log "By A U Thor" &&
 	echo >>exp.log "* tag 'c2':" &&
 	echo >>exp.log "  c2"
 }
-- 
1.7.9.2.413.ge58a8e

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

[Linus Torvalds]

On Mon, Mar 5, 2012 at 1:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> This patch punts the deep thinking part and just picks the tip
> committer and counts only merges in the history you are pulling in.
> It may not be a very good heuristics, but the code structure should
> be a good place to start.

Yeah, this looks good to me (but I didn't actually *test* it, so
that's just from looking at the patch).

                 Linus

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, Mar 5, 2012 at 1:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> This patch punts the deep thinking part and just picks the tip
>> committer and counts only merges in the history you are pulling in.
>> It may not be a very good heuristics, but the code structure should
>> be a good place to start.
>
> Yeah, this looks good to me (but I didn't actually *test* it, so
> that's just from looking at the patch).

As I said, I didn't actually *test* it, either, and the committer
based heuristics may have a huge room for improvements.

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

[René Scharfe]

Am 05.03.2012 22:34, schrieb Junio C Hamano:
> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
> index c81a7fe..b2465b4 100644
> --- a/builtin/fmt-merge-msg.c
> +++ b/builtin/fmt-merge-msg.c
> @@ -27,6 +27,8 @@ int fmt_merge_msg_config(const char *key, const char *value, void *cb)
>  			merge_log_config = DEFAULT_MERGE_LOG_LEN;
>  	} else if (!strcmp(key, "merge.branchdesc")) {
>  		use_branch_desc = git_config_bool(key, value);
> +	} else {
> +		return git_default_config(key, value, cb);
>  	}
>  	return 0;
>  }
> @@ -180,6 +182,89 @@ static void add_branch_desc(struct strbuf *out, const char *name)
>  	strbuf_release(&desc);
>  }
>
> +static void record_person(int which, struct string_list *people,
> +			  struct commit *commit)
> +{
> +	char name_buf[MAX_GITNAME], *name, *name_end;
> +	struct string_list_item *elem;
> +	const char *field = (which == 'a') ? "\nauthor " : "\ncommitter ";
> +
> +	name = strstr(commit->buffer, field);
> +	if (!name)
> +		return;
> +	name += strlen(field);
> +	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(people, name_buf);
> +	if (!elem) {
> +		elem = string_list_insert(people, name_buf);
> +		elem->util = (void *) 0;
> +	}
> +	elem->util = (void*)(((intptr_t)elem->util) + 1);
> +}
> +
> +#define util_as_int(elem) ((intptr_t)((elem)->util))

Something that actually returns an int would fit the name better. ;)

> +
> +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_people_count(struct strbuf *out, struct string_list *people)
> +{
> +	if (people->nr == 1)
> +		strbuf_addf(out, "%s", people->items[0].string);
> +	else if (people->nr == 2)
> +		strbuf_addf(out, "%s (%d) and %s (%d)",
> +			    people->items[0].string,
> +			    (int)util_as_int(&people->items[0]),
> +			    people->items[1].string,
> +			    (int)util_as_int(&people->items[1]));
> +	else if (people->nr)
> +		strbuf_addf(out, "%s (%d) and others",
> +			    people->items[0].string,
> +			    (int)util_as_int(&people->items[0]));
> +}
> +
> +static int committer_is_me(const char *name)
> +{
> +	int namelen = strlen(name);
> +	const char *me = git_committer_info(IDENT_NO_DATE);
> +	return (me && !memcmp(me, name, namelen) &&
> +		!memcmp(me + namelen, " <", 2));
> +}

This looks scary due to the missing length check of me before the 
memcmp() call, but is actually safe because git_committer_info() returns 
a pointer to a static buffer that is just as long as name can possibly 
be.  Still, perhaps this is nicer instead:

	const char *me = git_committer_info(IDENT_NO_DATE);
	const char *rest = skip_prefix(me, name);
	return rest && skip_prefix(rest, " <");

René

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

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Am 05.03.2012 22:34, schrieb Junio C Hamano:
>
>> +#define util_as_int(elem) ((intptr_t)((elem)->util))
>
> Something that actually returns an int would fit the name better. ;)

The particular type would not matter to the callers of the helper
macro, would it?

>> +static int committer_is_me(const char *name)
>> +{
>> +	int namelen = strlen(name);
>> +	const char *me = git_committer_info(IDENT_NO_DATE);
>> +	return (me && !memcmp(me, name, namelen) &&
>> +		!memcmp(me + namelen, " <", 2));
>> +}
>
> This looks scary due to the missing length check of me before the
> memcmp() call, but is actually safe because git_committer_info()
> returns a pointer to a static buffer that is just as long as name can
> possibly be.  Still, perhaps this is nicer instead:
>
> 	const char *me = git_committer_info(IDENT_NO_DATE);
> 	const char *rest = skip_prefix(me, name);
> 	return rest && skip_prefix(rest, " <");

Probably.  Let me fix it up.

Thanks.

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

[René Scharfe]

Am 07.03.2012 22:59, schrieb Junio C Hamano:
> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:
>
>> Am 05.03.2012 22:34, schrieb Junio C Hamano:
>>
>>> +#define util_as_int(elem) ((intptr_t)((elem)->util))
>>
>> Something that actually returns an int would fit the name better. ;)
>
> The particular type would not matter to the callers of the helper
> macro, would it?

Three of the five callers introduced in that commit cast the result to 
int and the remaining two don't care, so it actually does seem to matter 
for most of them, strictly speaking.  When I see a nit, I can't resist 
the urge to pick it, apparently.

René

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

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Am 07.03.2012 22:59, schrieb Junio C Hamano:
>> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:
>>
>>> Am 05.03.2012 22:34, schrieb Junio C Hamano:
>>>
>>>> +#define util_as_int(elem) ((intptr_t)((elem)->util))
>>>
>>> Something that actually returns an int would fit the name better. ;)
>>
>> The particular type would not matter to the callers of the helper
>> macro, would it?
>
> Three of the five callers introduced in that commit cast the result to
> int and the remaining two don't care, so it actually does seem to
> matter for most of them, strictly speaking.  When I see a nit, I can't
> resist the urge to pick it, apparently.

Unfortunately, replacing intptr_t with int or casting the above
again as int will result in

builtin/fmt-merge-msg.c: In function 'record_person':
builtin/fmt-merge-msg.c:213: error: cast to pointer from integer of different size

So...

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Three of the five callers introduced in that commit cast the result to
>> int and the remaining two don't care, so it actually does seem to
>> matter for most of them, strictly speaking.  When I see a nit, I can't
>> resist the urge to pick it, apparently.
>
> Unfortunately, replacing intptr_t with int or casting the above
> again as int will result in
>
> builtin/fmt-merge-msg.c: In function 'record_person':
> builtin/fmt-merge-msg.c:213: error: cast to pointer from integer of different size
>
> So...

-- >8 --
Subject: [PATCH] fmt-merge-msg.c: make util_as_int() return "int"

As its name says, the return value from util_as_int() should be
usable where an int is called for without casting.

Spotted-by: René Scharfe
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/fmt-merge-msg.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 40b90b1..8ddefb3 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -182,7 +182,7 @@ static void add_branch_desc(struct strbuf *out, const char *name)
 	strbuf_release(&desc);
 }
 
-#define util_as_int(elem) ((intptr_t)((elem)->util))
+#define util_as_int(elem) ((int)((elem)->util))
 
 static void record_person(int which, struct string_list *people,
 			  struct commit *commit)
@@ -210,7 +210,7 @@ static void record_person(int which, struct string_list *people,
 		elem = string_list_insert(people, name_buf);
 		elem->util = (void *)0;
 	}
-	elem->util = (void*)(util_as_int(elem) + 1);
+	elem->util = (void*)((intptr_t)(util_as_int(elem) + 1));
 }
 
 static int cmp_string_list_util_as_int(const void *a_, const void *b_)
@@ -226,13 +226,13 @@ static void add_people_count(struct strbuf *out, struct string_list *people)
 	else if (people->nr == 2)
 		strbuf_addf(out, "%s (%d) and %s (%d)",
 			    people->items[0].string,
-			    (int)util_as_int(&people->items[0]),
+			    util_as_int(&people->items[0]),
 			    people->items[1].string,
-			    (int)util_as_int(&people->items[1]));
+			    util_as_int(&people->items[1]));
 	else if (people->nr)
 		strbuf_addf(out, "%s (%d) and others",
 			    people->items[0].string,
-			    (int)util_as_int(&people->items[0]));
+			    util_as_int(&people->items[0]));
 }
 
 static int committer_is_me(const char *name)
-- 
1.7.10.rc0.28.g709d0

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

[Phil Hord]

On Thu, Mar 8, 2012 at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>
>> Am 07.03.2012 22:59, schrieb Junio C Hamano:
>>> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:
>>>
>>>> Am 05.03.2012 22:34, schrieb Junio C Hamano:
>>>>
>>>>> +#define util_as_int(elem) ((intptr_t)((elem)->util))
>>>>
>>>> Something that actually returns an int would fit the name better. ;)
>>>
>>> The particular type would not matter to the callers of the helper
>>> macro, would it?
>>
>> Three of the five callers introduced in that commit cast the result to
>> int and the remaining two don't care, so it actually does seem to
>> matter for most of them, strictly speaking.  When I see a nit, I can't
>> resist the urge to pick it, apparently.
>
> Unfortunately, replacing intptr_t with int or casting the above
> again as int will result in
>
> builtin/fmt-merge-msg.c: In function 'record_person':
> builtin/fmt-merge-msg.c:213: error: cast to pointer from integer of different size
>
> So...

Out of the frying pan, into the fire...

   builtin/fmt-merge-msg.c: In function ‘record_person’:
   builtin/fmt-merge-msg.c:213:34: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
   builtin/fmt-merge-msg.c: In function ‘cmp_string_list_util_as_int’:
   builtin/fmt-merge-msg.c:219:9: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
   builtin/fmt-merge-msg.c:219:26: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
   builtin/fmt-merge-msg.c: In function ‘add_people_count’:
   builtin/fmt-merge-msg.c:229:8: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
   builtin/fmt-merge-msg.c:231:8: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
   builtin/fmt-merge-msg.c:235:8: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]

I see slightly different code in pu than in next, but it produces the
same warnings on my 64-bit Linux machine.

Here's a fix against next:

-- >8 --

Subject: [PATCH] Appease compiler pedantry with an extra cast

Recently git repurposed a pointer as an integer to hold some
counter which git fancies.

Casting directly from 'pointer' to 'int' ((int)(void*)&x) causes a
possible size mismatch because pointers can be bigger than ints.
In such a situation, the compiler complains:

   warning: cast from pointer to integer of different size
            [-Wpointer-to-int-cast]

Cast the value through intptr_t first to quell compiler complaints
about how this gun appears to be aimed near our feet.  Then cast this
value to an int; this path assures the compiler we are smarter than we
look, or at least that we intend to aim the gun this way for a reason.
---
 builtin/fmt-merge-msg.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 8ddefb3..fee65e0 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -182,7 +182,7 @@ static void add_branch_desc(struct strbuf *out,
const char *name)
        strbuf_release(&desc);
 }

-#define util_as_int(elem) ((int)((elem)->util))
+#define util_as_int(elem) ((int)(intptr_t)((elem)->util))

 static void record_person(int which, struct string_list *people,
                          struct commit *commit)
-- 
1.7.9.3

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

[Jeff King]

On Mon, Mar 12, 2012 at 05:37:57PM -0400, Phil Hord wrote:

> Subject: [PATCH] Appease compiler pedantry with an extra cast
> 
> Recently git repurposed a pointer as an integer to hold some
> counter which git fancies.
> 
> Casting directly from 'pointer' to 'int' ((int)(void*)&x) causes a
> possible size mismatch because pointers can be bigger than ints.
> In such a situation, the compiler complains:
> 
>    warning: cast from pointer to integer of different size
>             [-Wpointer-to-int-cast]

Yeah, I've been seeing the same warning on my x86_64 box, and came up
with the same fix. However...

> Cast the value through intptr_t first to quell compiler complaints
> about how this gun appears to be aimed near our feet.  Then cast this
> value to an int; this path assures the compiler we are smarter than we
> look, or at least that we intend to aim the gun this way for a reason.

This feels so hacky.  One of the callsites does:

    elem->util = (void*)((intptr_t)(util_as_int(elem) + 1));

which will truncate the value down to an int before replacing it back in
the void pointer. And that truncation is ultimately what the compiler is
warning about, and what we are sneaking around with the extra cast
(because casting between integer sizes of different types is OK, even
though it can cause truncation).

I don't think the truncation is a problem in practice, but it just feels
like we are not just silencing an over-zealous compiler, but actually
burying type-size assumption behind a set of four (4!) casts.

I wonder if we would be happier to declare the "util" field of
string_list as a union. Obviously that provides no safety that we read
the correct item out of the union, but that is no worse than the
situation with all of these casts, and I suspect the result would be
much more obvious and readable.

The downside is that all current users of the "util" field would need
s/util/&.ptr/ or similar to dereference the pointer field.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Mar 12, 2012 at 05:37:57PM -0400, Phil Hord wrote:
>
> This feels so hacky.  One of the callsites does:
>
>     elem->util = (void*)((intptr_t)(util_as_int(elem) + 1));
>
> which will truncate the value down to an int before replacing it back in
> the void pointer. And that truncation is ultimately what the compiler is
> warning about, and what we are sneaking around with the extra cast
> (because casting between integer sizes of different types is OK, even
> though it can cause truncation).

Yeah, I find it utterly disgusting that Phil's compiler is picky about
(int)(void *) and insists on (intptr_t)(void *), while totally ignoring
the same bit lossage coming from (int)(intptr_t)(void *).

I am again starting to think that the very original was probably the least
bad among the yuckies.

As the whole point of the helper macro is to cast the .util field as
"some" integral type, I'm tempted to squash this into the v2 patch I
posted earlier today.  Earlier complaint from René was very sensible that
it looked funny to cast xxx_as_int() explicitly to int, but having to cast
"some" integral value explicitly to int is required in the context of
sprintf() like vararg function, so it would no longer apply to this
version.

 builtin/fmt-merge-msg.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 4e7196a..1bc6b8b 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -182,7 +182,7 @@ static void add_branch_desc(struct strbuf *out, const char *name)
 	strbuf_release(&desc);
 }
 
-#define util_as_int(elem) ((int)(intptr_t)((elem)->util))
+#define util_as_integral(elem) ((intptr_t)((elem)->util))
 
 static void record_person(int which, struct string_list *people,
 			  struct commit *commit)
@@ -210,13 +210,13 @@ static void record_person(int which, struct string_list *people,
 		elem = string_list_insert(people, name_buf);
 		elem->util = (void *)0;
 	}
-	elem->util = (void*)((intptr_t)(util_as_int(elem) + 1));
+	elem->util = (void*)(util_as_integral(elem) + 1);
 }
 
-static int cmp_string_list_util_as_int(const void *a_, const void *b_)
+static int cmp_string_list_util_as_integral(const void *a_, const void *b_)
 {
 	const struct string_list_item *a = a_, *b = b_;
-	return util_as_int(b) - util_as_int(a);
+	return util_as_integral(b) - util_as_integral(a);
 }
 
 static void add_people_count(struct strbuf *out, struct string_list *people)
@@ -226,13 +226,13 @@ static void add_people_count(struct strbuf *out, struct string_list *people)
 	else if (people->nr == 2)
 		strbuf_addf(out, "%s (%d) and %s (%d)",
 			    people->items[0].string,
-			    util_as_int(&people->items[0]),
+			    (int)util_as_integral(&people->items[0]),
 			    people->items[1].string,
-			    util_as_int(&people->items[1]));
+			    (int)util_as_integral(&people->items[1]));
 	else if (people->nr)
 		strbuf_addf(out, "%s (%d) and others",
 			    people->items[0].string,
-			    util_as_int(&people->items[0]));
+			    (int)util_as_integral(&people->items[0]));
 }
 
 static void credit_people(struct strbuf *out,
@@ -267,11 +267,11 @@ static void add_people_info(struct strbuf *out,
 	if (authors->nr)
 		qsort(authors->items,
 		      authors->nr, sizeof(authors->items[0]),
-		      cmp_string_list_util_as_int);
+		      cmp_string_list_util_as_integral);
 	if (committers->nr)
 		qsort(committers->items,
 		      committers->nr, sizeof(committers->items[0]),
-		      cmp_string_list_util_as_int);
+		      cmp_string_list_util_as_integral);
 
 	credit_people(out, authors, 'a');
 	credit_people(out, committers, 'c');

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

[Phil Hord]

On Tue, Mar 13, 2012 at 5:03 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Mar 12, 2012 at 05:37:57PM -0400, Phil Hord wrote:
>
>> Subject: [PATCH] Appease compiler pedantry with an extra cast
>>
>> Recently git repurposed a pointer as an integer to hold some
>> counter which git fancies.
>>
>> Casting directly from 'pointer' to 'int' ((int)(void*)&x) causes a
>> possible size mismatch because pointers can be bigger than ints.
>> In such a situation, the compiler complains:
>>
>>    warning: cast from pointer to integer of different size
>>             [-Wpointer-to-int-cast]
>
> Yeah, I've been seeing the same warning on my x86_64 box, and came up
> with the same fix. However...
>
>> Cast the value through intptr_t first to quell compiler complaints
>> about how this gun appears to be aimed near our feet.  Then cast this
>> value to an int; this path assures the compiler we are smarter than we
>> look, or at least that we intend to aim the gun this way for a reason.
>
> This feels so hacky.

Well, that's because it is hacky.  But it's the original code that's suspect.

> One of the callsites does:
>
>    elem->util = (void*)((intptr_t)(util_as_int(elem) + 1));
>
> which will truncate the value down to an int before replacing it back in
> the void pointer. And that truncation is ultimately what the compiler is
> warning about, and what we are sneaking around with the extra cast
> (because casting between integer sizes of different types is OK, even
> though it can cause truncation).

I think this one is ok because it's really just "the hacky" bit
storing an integer in a variable meant to hold a pointer.  That's why
it's incrementing here, I suppose, not because it really wants to
point at the next byte.

> I don't think the truncation is a problem in practice, but it just feels
> like we are not just silencing an over-zealous compiler, but actually
> burying type-size assumption behind a set of four (4!) casts.

The compiler is doing its job here to warn us against storing big-ish
pointers in small-ish ints.  But if we know we will never accidentally
use this as a pointer and if the integer will never overflow the
32-bounds of the (int) representation, then it's all good.

But I agree, it is hacky.

Phil

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

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> * jc/fmt-merge-msg-people (2012-03-08) 3 commits
>   (merged to 'next' on 2012-03-08 at be31aa9)
>  + fmt-merge-msg.c: make util_as_int() return "int"
>   (merged to 'next' on 2012-03-07 at 76fbac3)
>  + fmt-merge-msg: finishing touches
>   (merged to 'next' on 2012-03-05 at 38de349)
>  + fmt-merge-msg: show those involved in a merged series
>
> The "fmt-merge-msg" command learns to list the primary contributors
> involved in the side topic you are merging.

Ah, so that's where the funny

	By Jonathan Nieder
	via Jonathan Nieder

lines in

	Merge branch 'jn/maint-fast-import-empty-ls' into pu

	By Jonathan Nieder
	via Jonathan Nieder
	* jn/maint-fast-import-empty-ls:
	  fast-import: don't allow 'ls' of path with empty components
	  fast-import: leakfix for 'ls' of dirty trees

come from.  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.

By contrast, the

	Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mason/linux-btrfs

	Pull btrfs updates from Chris Mason:
	[more words about that here]

	* 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mason/linux-btrfs:
	 Btrfs: fix casting error in scrub reada code
	[...]

descriptions in Linus's repo are very pleasant.  It's a subtle
difference, but the extra whitespace and the way it presents the
important bits like "Pull btrfs updates" before the mechanics "from
Chris Mason" help.

So I like the goal, but something about this execution does not seem
to be working.  Sorry I have no better news or more concrete feedback
to offer.

Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> 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.

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

[Jonathan Nieder]

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

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I also wonder if it would make sense to change
> ... to ...
> 	* '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

Having the author/subsystem attribution on separate lines was a
deliberate design decision made by me, so that (1) cutting and
pasting Linus would prefer to do stays easier, and (2) giving the
credits more prominently in a default format.  There is a paragraph
break before the attribution begins anyway, and I think eyes easily
learn to scan for paragraphs that begin "By " instead of " * ".

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

[Johannes Sixt]

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

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

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

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.

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

[Johannes Sixt]

Am 3/13/2012 19:26, schrieb Junio C Hamano:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>> 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

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 3/13/2012 19:26, schrieb Junio C Hamano:
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>> 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 understand that.  But ask yourself honestly and tell me (in several
weeks) why you do not think that your "would like to opt-out" part falls
into exactly the same "loud noises immediately after seeing a change"
category.

[PATCH v2 1/1] fmt-merge-msg: show those involved in a merged series

[Junio C Hamano]

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.

Also infer the names of "lieutents" to help integrators at higher
level of the food-chain to give credit to them, by counting:

 * The committer of the 'tip' commit that is merged
 * The committer of merge commits that are merged

Often the first one gives the owner of the history being pulled, but
his last pull from his sublieutenants may have been a fast-forward,
in which case the first one would not be.  The latter rule will
count the integrator of the history, so together it might be a
reasonable heuristics.

There are two special cases:

 - The "author" credit is omitted when the series is written solely
   by the same author who is making the merge. The name can be seen
   on the "Author" line of the "git log" output to view the log
   message anyway.

 - The "lieutenant" credit is omitted when there is only one key
   committer in the merged branch and it is the committer who is
   making the merge. Typically this applies to the case where the
   developer merges his own branch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is a fresh re-roll without all those "oops, this does not compile
   on a compiler that warns about casting between a pointer and intptr_t
   while being happy to see it further casted down to int" fix-up patches.

   A major difference can be seen by the lack of changes to existing
   tests. The tests earlier series touched are all about single developer
   case, and there is no need to update the test vectors for this change.

 builtin/fmt-merge-msg.c  |  114 ++++++++++++++++++++++++++++++++++++++++++++--
 t/t6200-fmt-merge-msg.sh |   27 +++++++++--
 2 files changed, 134 insertions(+), 7 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index c81a7fe..4e7196a 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -27,6 +27,8 @@ int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 			merge_log_config = DEFAULT_MERGE_LOG_LEN;
 	} else if (!strcmp(key, "merge.branchdesc")) {
 		use_branch_desc = git_config_bool(key, value);
+	} else {
+		return git_default_config(key, value, cb);
 	}
 	return 0;
 }
@@ -180,6 +182,101 @@ static void add_branch_desc(struct strbuf *out, const char *name)
 	strbuf_release(&desc);
 }
 
+#define util_as_int(elem) ((int)(intptr_t)((elem)->util))
+
+static void record_person(int which, struct string_list *people,
+			  struct commit *commit)
+{
+	char name_buf[MAX_GITNAME], *name, *name_end;
+	struct string_list_item *elem;
+	const char *field = (which == 'a') ? "\nauthor " : "\ncommitter ";
+
+	name = strstr(commit->buffer, field);
+	if (!name)
+		return;
+	name += strlen(field);
+	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(people, name_buf);
+	if (!elem) {
+		elem = string_list_insert(people, name_buf);
+		elem->util = (void *)0;
+	}
+	elem->util = (void*)((intptr_t)(util_as_int(elem) + 1));
+}
+
+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_people_count(struct strbuf *out, struct string_list *people)
+{
+	if (people->nr == 1)
+		strbuf_addf(out, "%s", people->items[0].string);
+	else if (people->nr == 2)
+		strbuf_addf(out, "%s (%d) and %s (%d)",
+			    people->items[0].string,
+			    util_as_int(&people->items[0]),
+			    people->items[1].string,
+			    util_as_int(&people->items[1]));
+	else if (people->nr)
+		strbuf_addf(out, "%s (%d) and others",
+			    people->items[0].string,
+			    util_as_int(&people->items[0]));
+}
+
+static void credit_people(struct strbuf *out,
+			  struct string_list *them,
+			  int kind)
+{
+	const char *label;
+	const char *me;
+
+	if (kind == 'a') {
+		label = "\nBy ";
+		me = git_author_info(IDENT_NO_DATE);
+	} else {
+		label = "\nvia ";
+		me = git_committer_info(IDENT_NO_DATE);
+	}
+
+	if (!them->nr ||
+	    (them->nr == 1 &&
+	     me &&
+	     (me = skip_prefix(me, them->items->string)) != NULL &&
+	     skip_prefix(me, " <")))
+		return;
+	strbuf_addstr(out, label);
+	add_people_count(out, them);
+}
+
+static void add_people_info(struct strbuf *out,
+			    struct string_list *authors,
+			    struct string_list *committers)
+{
+	if (authors->nr)
+		qsort(authors->items,
+		      authors->nr, sizeof(authors->items[0]),
+		      cmp_string_list_util_as_int);
+	if (committers->nr)
+		qsort(committers->items,
+		      committers->nr, sizeof(committers->items[0]),
+		      cmp_string_list_util_as_int);
+
+	credit_people(out, authors, 'a');
+	credit_people(out, committers, 'c');
+}
+
 static void shortlog(const char *name,
 		     struct origin_data *origin_data,
 		     struct commit *head,
@@ -190,6 +287,8 @@ 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;
+	struct string_list committers = STRING_LIST_INIT_DUP;
 	int flags = UNINTERESTING | TREESAME | SEEN | SHOWN | ADDED;
 	struct strbuf sb = STRBUF_INIT;
 	const unsigned char *sha1 = origin_data->sha1;
@@ -199,7 +298,6 @@ static void shortlog(const char *name,
 		return;
 
 	setup_revisions(0, NULL, rev, NULL);
-	rev->ignore_merges = 1;
 	add_pending_object(rev, branch, name);
 	add_pending_object(rev, &head->object, "^HEAD");
 	head->object.flags |= UNINTERESTING;
@@ -208,10 +306,15 @@ static void shortlog(const char *name,
 	while ((commit = get_revision(rev)) != NULL) {
 		struct pretty_print_context ctx = {0};
 
-		/* ignore merges */
-		if (commit->parents && commit->parents->next)
+		if (commit->parents && commit->parents->next) {
+			/* do not list a merge but count committer */
+			record_person('c', &committers, commit);
 			continue;
-
+		}
+		if (!count)
+			/* the 'tip' committer */
+			record_person('c', &committers, commit);
+		record_person('a', &authors, commit);
 		count++;
 		if (subjects.nr > limit)
 			continue;
@@ -226,6 +329,7 @@ static void shortlog(const char *name,
 			string_list_append(&subjects, strbuf_detach(&sb, NULL));
 	}
 
+	add_people_info(out, &authors, &committers);
 	if (count > limit)
 		strbuf_addf(out, "\n* %s: (%d commits)\n", name, count);
 	else
@@ -246,6 +350,8 @@ static void shortlog(const char *name,
 	rev->commits = NULL;
 	rev->pending.nr = 0;
 
+	string_list_clear(&authors, 0);
+	string_list_clear(&committers, 0);
 	string_list_clear(&subjects, 0);
 }
 
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 9a16806..9b50f54 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -35,15 +35,18 @@ test_expect_success setup '
 
 	echo "l3" >two &&
 	test_tick &&
-	git commit -a -m "Left #3" &&
+	GIT_COMMITTER_NAME="Another Committer" \
+	GIT_AUTHOR_NAME="Another Author" git commit -a -m "Left #3" &&
 
 	echo "l4" >two &&
 	test_tick &&
-	git commit -a -m "Left #4" &&
+	GIT_COMMITTER_NAME="Another Committer" \
+	GIT_AUTHOR_NAME="Another Author" git commit -a -m "Left #4" &&
 
 	echo "l5" >two &&
 	test_tick &&
-	git commit -a -m "Left #5" &&
+	GIT_COMMITTER_NAME="Another Committer" \
+	GIT_AUTHOR_NAME="Another Author" git commit -a -m "Left #5" &&
 	git tag tag-l5 &&
 
 	git checkout right &&
@@ -99,6 +102,8 @@ test_expect_success '[merge] summary/log configuration' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left:
 	  Left #5
 	  Left #4
@@ -144,6 +149,8 @@ test_expect_success 'merge.log=3 limits shortlog length' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left: (5 commits)
 	  Left #5
 	  Left #4
@@ -159,6 +166,8 @@ test_expect_success 'merge.log=5 shows all 5 commits' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left:
 	  Left #5
 	  Left #4
@@ -181,6 +190,8 @@ test_expect_success '--log=3 limits shortlog length' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left: (5 commits)
 	  Left #5
 	  Left #4
@@ -196,6 +207,8 @@ test_expect_success '--log=5 shows all 5 commits' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left:
 	  Left #5
 	  Left #4
@@ -225,6 +238,8 @@ test_expect_success 'fmt-merge-msg -m' '
 	cat >expected.log <<-EOF &&
 	Sync with left
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* ${apos}left${apos} of $(pwd):
 	  Left #5
 	  Left #4
@@ -256,6 +271,8 @@ test_expect_success 'setup: expected shortlog for two branches' '
 	cat >expected <<-EOF
 	Merge branches ${apos}left${apos} and ${apos}right${apos}
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left:
 	  Left #5
 	  Left #4
@@ -379,6 +396,8 @@ test_expect_success 'merge-msg two tags' '
 	  Common #2
 	  Common #1
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* tag ${apos}tag-l5${apos}:
 	  Left #5
 	  Left #4
@@ -407,6 +426,8 @@ test_expect_success 'merge-msg tag and branch' '
 	  Common #2
 	  Common #1
 
+	By Another Author (3) and A U Thor (2)
+	via Another Committer
 	* left:
 	  Left #5
 	  Left #4
-- 
1.7.10.rc0.65.g3445e

[PATCH/RFC] fmt-merge-msg: add a blank line after people info

[Jonathan Nieder]

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 <jrnieder@gmail.com>
---
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

Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> 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.

Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info

[Linus Torvalds]

On Fri, May 11, 2012 at 3:46 PM, Junio C Hamano <gitster@pobox.com> 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

Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> 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:
> ...
> I dunno. But it looks odd, and the above is not the only example of
> "those counts don't make sense".

"By" numbers were meant to give credits to people who wrote the code, and
"via" numbers were meant to give credits to people who helped usher code
by others' to the person who is making the merge.

The former is something like "git shortlog --no-merges -n -s ..MERGE_HEAD"
and is quite straightforward to compute.  I didn't think things through
for the latter and punted with an ad-hoc algorithm that does not require
us to traverse reachability when I wrote that code, I guess, and I suspect
that is what is causing the "odd" numbers.

Here are some things that "via" should count as "integrator's
contribution":

 - making a commit authored by others (1 "credit" per such commit);

 - merging a branch that has commits authored by others (1 "credit" per
   commit authored by others brought in with such a merge).

And here are some things that "via" should not count:

 - merging your own topic branches into one branch for the person who is
   making the (final) merge to pull;

 - merging backwards, pulling bunch of unrelated commits from upstream.

For that, I think the code needs to annotate each "new" commit that is
brought into the history by the (final) merge with "how many others'
commits does it pull into the history" number, or something.  But I am
still in "thinking aloud" phase here, so...

Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> 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:
>> ...
>> I dunno. But it looks odd, and the above is not the only example of
>> "those counts don't make sense".
>
> "By" numbers were meant to give credits to people who wrote the code, and
> "via" numbers were meant to give credits to people who helped usher code
> by others' to the person who is making the merge.

I took a look at this again today.

The implementation you saw was written before I did any of the thinking
below, and it merely counts the committer of merges plus the committer of
the tip commit you are pulling, or something.  It is slightly better than
random number generator, but not by a huge margin.

Here is an outline of my current thinking to give a good definition for
the "via" number, which is supposed to give credits to lieutenants (and
possibly sublieutenants).

Suppose the history behind the tip commit you are pulling looked like
this:

          E-----E-----E-----E-----E
                                   \
            A/D--A/D                E
                   \                 \
       A/B---A/B----B-----B-----B-----C-----C-----C
                         /
      A-----A-----A-----A

where a commit denoted by a single letter (e.g. A on the bottom line) is
authored and committed by that person (by definition a merge is authored
and committed by the same person), and a commit deonted as X/Y was
authored by X and committed by Y.  You are responding to a pull request to
integrate the tip commit authored and committed by C into your history.

The contributor B helped by applying patches from contributor A (the
leftmost two patches on the middle line), merging the work authored by A
and vetted by D (the first merge on the middle line), and the work
authored by A (the second merge on the middle line).  He even fixed things
up with the rightmost commit in his history before asking C to pull.  He
should get the credit for all this work to help getting A's changes to the
history, including the two commits made by D (which owe credit to D as
well).

For the same reason why the two commits in D's history owe credits both to
B and D, the whole thing owes "via" credit to C, as C was the lieutenant
who was ultimately responsible for delivering this result to you (in other
words, he could have decided not to pull from B).

What I am thinking is for each commit X (not necessarily merges), count
non-merge commits that are:

 - reachable from X;
 - are being merged by the final merge;
 - not authored by the same author as X itself; and
 - have not been counted to give credit to the author of X yet. 

For example, the first two commits by B on the middle line will give 2
credits (because B helped A's patch by applying them), the first merge by
B on the middle line will give 2 credits (because it contributes another 2
commits by A via D to the final history) to B, the second merge will give
another 4 credits (commits on the bottom line) but not for the commits
that were already counted for his first merge.  Total credit to B is 8 in
this example.

The merge made by C will *count* all 8 commits by A (even though they are
credited also to B), 1 commit by B (i.e. fix-up after merging 4 commit
series from A), and 6 commits by E.  D gets 2 credits for having applied
two patches from A.  A and E will get no "via" credits.

While I find the double-counting that appear in the example somewhat
disturbing, it inherently give larger credit to sub-lieutenant that is
closer to the tip, so it might after all match intuition.

Now, computing this efficiently may not be trivial, as you would need N^2
reachability analysis when pulling in N commits.  Among 2000 recent merges
I sampled from the kernel history, 70+ pull in more than 1000 commits (the
largest one d4bbf7e77 pulls in 21k commits).

Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info

[Linus Torvalds]

On Tue, May 15, 2012 at 1:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Now, computing this efficiently may not be trivial, as you would need N^2
> reachability analysis when pulling in N commits.  Among 2000 recent merges
> I sampled from the kernel history, 70+ pull in more than 1000 commits (the
> largest one d4bbf7e77 pulls in 21k commits).

So I have to say, for my purposes, it not only might be inefficient,
but it can still be very misleading.

I actually care most about the person I personally pull from. And if
he is a submaintainer who has other submaintainers, it can be that
following the commit history doesn't show him at *all*. He might have
done just a fast-forward merge, but he's still the person *I* want to
credit.

So I'm getting the feeling that the "count authors/committers" may be
cute, but it's not necessarily all that relevant. It's the kind of
information you can see later from the git tree itself.

(Admittedly, so it the shortlog we put in the merge, so that "you can
find it later in the git tree itself" not *that* great of an argument
- the real argument for me is that it doesn't matter what you count,
you'll not necessarily get the actual piece of information I care
about..).

So I think it's somewhat interesting information, and I haven't really
disliked seeing it, but I have mostly edited it away (although I see
that some other maintainers also run modern versions of git and have
left it in place, so we do have those in the kernel).

The one merge commit of mine that has it, I edited it so that the "via
C Committer" was on the same lne as the "By A Author" information.

So I'm not entirely convinced yet. I don't *dislike* the concept, but
I could definitely do without it (or maybe have it in the commented
part of the commit message, so that you'd have to explicitly edit it
to show up).

                       Linus

Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, May 15, 2012 at 1:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Now, computing this efficiently may not be trivial, as you would need N^2
>> reachability analysis when pulling in N commits. Among 2000 recent merges
>> I sampled from the kernel history, 70+ pull in more than 1000 commits (the
>> largest one d4bbf7e77 pulls in 21k commits).
>
> So I have to say, for my purposes, it not only might be inefficient,
> but it can still be very misleading.
> ...
> (Admittedly, so it the shortlog we put in the merge, so that "you can
> find it later in the git tree itself" not *that* great of an argument
> - the real argument for me is that it doesn't matter what you count,
> you'll not necessarily get the actual piece of information I care
> about..).

There is no way to mine "X pulled from Y" out of the topology if you allow
fast-forward anywhere, so "I care about whom I pulled from" is something
people need to give out of band.  Your lieutenant may have fast-forwarded
his history from his lieutenant's, or you may even be fast-forwarding your
history from your lieutenant's, in which case you do not even have a merge
commit to record that fact anyway.  If you do not fast-forward, at least
the merge subject would give you where you got the history from, so it is
not like that the information *must* be obtained by looking at the history
anyway (incidentally, that was the reason why the "better than random
number generator but not by a large margin" version gives an extra weight
to the tip commit you are pulling; unless your lieutenant fast-forwarded,
that is the person you pulled from).

> So I'm not entirely convinced yet. I don't *dislike* the concept, but
> I could definitely do without it (or maybe have it in the commented
> part of the commit message, so that you'd have to explicitly edit it
> to show up).

I am tempted to suggest removing the "via" part as a failed experiment for
now.

Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info

[Jonathan Nieder]

Hi again,

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

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

Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info

[Jonathan Nieder]

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

Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

It may be real for _you_ but not for everybody.  Especially, the
current spacing is a lot more logically consistent than your Fred
the Clown example, which *does* use much more strange spacing.

I do not deeply care either way, so let's do this sometime when the
tree is calm (but not today).

 builtin/fmt-merge-msg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index bf93b04..2c4d435 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -286,10 +286,10 @@ static void credit_people(struct strbuf *out,
 	const char *me;
 
 	if (kind == 'a') {
-		label = "\nBy ";
+		label = "\n# By ";
 		me = git_author_info(IDENT_NO_DATE);
 	} else {
-		label = "\nvia ";
+		label = "\n# Via ";
 		me = git_committer_info(IDENT_NO_DATE);
 	}
 

Re: A possible fmt-merge-msg update?

[Jeff King]

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 <gitster@pobox.com> 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.

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 <mingo@elte.hu>
    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 <mingo@elte.hu>
  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