* A possible fmt-merge-msg update?
@ 2012-03-05 3:17 Junio C Hamano
2012-03-05 5:24 ` Linus Torvalds
0 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2012-03-05 3:17 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git
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?
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: A possible fmt-merge-msg update? 2012-03-05 3:17 A possible fmt-merge-msg update? Junio C Hamano @ 2012-03-05 5:24 ` Linus Torvalds 2012-03-05 19:04 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Linus Torvalds @ 2012-03-05 5:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: A possible fmt-merge-msg update? 2012-03-05 5:24 ` Linus Torvalds @ 2012-03-05 19:04 ` Junio C Hamano 2012-03-05 20:33 ` Linus Torvalds 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2012-03-05 19:04 UTC (permalink / raw) To: Linus Torvalds; +Cc: git 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: A possible fmt-merge-msg update? 2012-03-05 19:04 ` Junio C Hamano @ 2012-03-05 20:33 ` Linus Torvalds 2012-03-05 21:34 ` [PATCH] fmt-merge-msg: show those involved in a merged series Junio C Hamano 2012-03-06 7:59 ` A possible fmt-merge-msg update? Jeff King 0 siblings, 2 replies; 36+ messages in thread From: Linus Torvalds @ 2012-03-05 20:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-05 20:33 ` Linus Torvalds @ 2012-03-05 21:34 ` Junio C Hamano 2012-03-05 21:46 ` Linus Torvalds ` (2 more replies) 2012-03-06 7:59 ` A possible fmt-merge-msg update? Jeff King 1 sibling, 3 replies; 36+ messages in thread From: Junio C Hamano @ 2012-03-05 21:34 UTC (permalink / raw) To: Linus Torvalds; +Cc: git 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-05 21:34 ` [PATCH] fmt-merge-msg: show those involved in a merged series Junio C Hamano @ 2012-03-05 21:46 ` Linus Torvalds 2012-03-05 21:49 ` Junio C Hamano 2012-03-07 21:22 ` René Scharfe 2012-03-12 7:11 ` Jonathan Nieder 2 siblings, 1 reply; 36+ messages in thread From: Linus Torvalds @ 2012-03-05 21:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-05 21:46 ` Linus Torvalds @ 2012-03-05 21:49 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2012-03-05 21:49 UTC (permalink / raw) To: Linus Torvalds; +Cc: git 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-05 21:34 ` [PATCH] fmt-merge-msg: show those involved in a merged series Junio C Hamano 2012-03-05 21:46 ` Linus Torvalds @ 2012-03-07 21:22 ` René Scharfe 2012-03-07 21:59 ` Junio C Hamano 2012-03-12 7:11 ` Jonathan Nieder 2 siblings, 1 reply; 36+ messages in thread From: René Scharfe @ 2012-03-07 21:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, git 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é ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-07 21:22 ` René Scharfe @ 2012-03-07 21:59 ` Junio C Hamano 2012-03-08 17:46 ` René Scharfe 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2012-03-07 21:59 UTC (permalink / raw) To: René Scharfe; +Cc: Linus Torvalds, git 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-07 21:59 ` Junio C Hamano @ 2012-03-08 17:46 ` René Scharfe 2012-03-08 19:18 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: René Scharfe @ 2012-03-08 17:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, git 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é ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-08 17:46 ` René Scharfe @ 2012-03-08 19:18 ` Junio C Hamano 2012-03-08 21:31 ` Junio C Hamano 2012-03-12 21:37 ` Phil Hord 0 siblings, 2 replies; 36+ messages in thread From: Junio C Hamano @ 2012-03-08 19:18 UTC (permalink / raw) To: René Scharfe; +Cc: Linus Torvalds, git 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... ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-08 19:18 ` Junio C Hamano @ 2012-03-08 21:31 ` Junio C Hamano 2012-03-12 21:37 ` Phil Hord 1 sibling, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2012-03-08 21:31 UTC (permalink / raw) To: René Scharfe; +Cc: git 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-08 19:18 ` Junio C Hamano 2012-03-08 21:31 ` Junio C Hamano @ 2012-03-12 21:37 ` Phil Hord 2012-03-13 21:03 ` Jeff King 1 sibling, 1 reply; 36+ messages in thread From: Phil Hord @ 2012-03-12 21:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: René Scharfe, Linus Torvalds, git 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-12 21:37 ` Phil Hord @ 2012-03-13 21:03 ` Jeff King 2012-03-14 3:44 ` Junio C Hamano 2012-03-14 19:12 ` Phil Hord 0 siblings, 2 replies; 36+ messages in thread From: Jeff King @ 2012-03-13 21:03 UTC (permalink / raw) To: Phil Hord; +Cc: Junio C Hamano, René Scharfe, Linus Torvalds, git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-13 21:03 ` Jeff King @ 2012-03-14 3:44 ` Junio C Hamano 2012-03-14 19:12 ` Phil Hord 1 sibling, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2012-03-14 3:44 UTC (permalink / raw) To: Jeff King; +Cc: Phil Hord, René Scharfe, Linus Torvalds, git 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'); ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-13 21:03 ` Jeff King 2012-03-14 3:44 ` Junio C Hamano @ 2012-03-14 19:12 ` Phil Hord 1 sibling, 0 replies; 36+ messages in thread From: Phil Hord @ 2012-03-14 19:12 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, René Scharfe, Linus Torvalds, git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-05 21:34 ` [PATCH] fmt-merge-msg: show those involved in a merged series Junio C Hamano 2012-03-05 21:46 ` Linus Torvalds 2012-03-07 21:22 ` René Scharfe @ 2012-03-12 7:11 ` Jonathan Nieder 2012-03-13 1:55 ` Junio C Hamano 2 siblings, 1 reply; 36+ messages in thread From: Jonathan Nieder @ 2012-03-12 7:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, git, Jeff King 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-12 7:11 ` Jonathan Nieder @ 2012-03-13 1:55 ` Junio C Hamano 2012-03-13 5:23 ` Jonathan Nieder ` (2 more replies) 0 siblings, 3 replies; 36+ messages in thread From: Junio C Hamano @ 2012-03-13 1:55 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Linus Torvalds, git, Jeff King 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-13 1:55 ` Junio C Hamano @ 2012-03-13 5:23 ` Jonathan Nieder 2012-03-13 5:45 ` Junio C Hamano 2012-03-13 7:27 ` Johannes Sixt 2012-05-11 10:31 ` [PATCH/RFC] fmt-merge-msg: add a blank line after people info Jonathan Nieder 2 siblings, 1 reply; 36+ messages in thread From: Jonathan Nieder @ 2012-03-13 5:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, git, Jeff King 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-13 5:23 ` Jonathan Nieder @ 2012-03-13 5:45 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2012-03-13 5:45 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Linus Torvalds, git, Jeff King 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 " * ". ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-13 1:55 ` Junio C Hamano 2012-03-13 5:23 ` Jonathan Nieder @ 2012-03-13 7:27 ` Johannes Sixt 2012-03-13 18:26 ` Junio C Hamano 2012-03-13 18:28 ` [PATCH v2 1/1] " Junio C Hamano 2012-05-11 10:31 ` [PATCH/RFC] fmt-merge-msg: add a blank line after people info Jonathan Nieder 2 siblings, 2 replies; 36+ messages in thread From: Johannes Sixt @ 2012-03-13 7:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Linus Torvalds, git, Jeff King 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-13 7:27 ` Johannes Sixt @ 2012-03-13 18:26 ` Junio C Hamano 2012-03-14 6:37 ` Johannes Sixt 2012-03-13 18:28 ` [PATCH v2 1/1] " Junio C Hamano 1 sibling, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2012-03-13 18:26 UTC (permalink / raw) To: Johannes Sixt; +Cc: Jonathan Nieder, Linus Torvalds, git, Jeff King 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-13 18:26 ` Junio C Hamano @ 2012-03-14 6:37 ` Johannes Sixt 2012-03-14 20:34 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Johannes Sixt @ 2012-03-14 6:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Linus Torvalds, git, Jeff King 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] fmt-merge-msg: show those involved in a merged series 2012-03-14 6:37 ` Johannes Sixt @ 2012-03-14 20:34 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2012-03-14 20:34 UTC (permalink / raw) To: Johannes Sixt; +Cc: Jonathan Nieder, Linus Torvalds, git, Jeff King 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 1/1] fmt-merge-msg: show those involved in a merged series 2012-03-13 7:27 ` Johannes Sixt 2012-03-13 18:26 ` Junio C Hamano @ 2012-03-13 18:28 ` Junio C Hamano 1 sibling, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2012-03-13 18:28 UTC (permalink / raw) To: git; +Cc: Johannes Sixt, Jonathan Nieder, Linus Torvalds, Jeff King 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH/RFC] fmt-merge-msg: add a blank line after people info 2012-03-13 1:55 ` Junio C Hamano 2012-03-13 5:23 ` Jonathan Nieder 2012-03-13 7:27 ` Johannes Sixt @ 2012-05-11 10:31 ` Jonathan Nieder 2012-05-11 22:46 ` Junio C Hamano 2 siblings, 1 reply; 36+ messages in thread From: Jonathan Nieder @ 2012-05-11 10:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, git, Jeff King 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info 2012-05-11 10:31 ` [PATCH/RFC] fmt-merge-msg: add a blank line after people info Jonathan Nieder @ 2012-05-11 22:46 ` Junio C Hamano 2012-05-11 23:20 ` Linus Torvalds 2012-06-06 20:27 ` Jonathan Nieder 0 siblings, 2 replies; 36+ messages in thread From: Junio C Hamano @ 2012-05-11 22:46 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Linus Torvalds, git, Jeff King 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info 2012-05-11 22:46 ` Junio C Hamano @ 2012-05-11 23:20 ` Linus Torvalds 2012-05-14 18:31 ` Junio C Hamano 2012-06-06 20:27 ` Jonathan Nieder 1 sibling, 1 reply; 36+ messages in thread From: Linus Torvalds @ 2012-05-11 23:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git, Jeff King 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info 2012-05-11 23:20 ` Linus Torvalds @ 2012-05-14 18:31 ` Junio C Hamano 2012-05-15 20:24 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2012-05-14 18:31 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jonathan Nieder, git, Jeff King 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... ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info 2012-05-14 18:31 ` Junio C Hamano @ 2012-05-15 20:24 ` Junio C Hamano 2012-05-16 2:02 ` Linus Torvalds 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2012-05-15 20:24 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jonathan Nieder, git, Jeff King 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). ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info 2012-05-15 20:24 ` Junio C Hamano @ 2012-05-16 2:02 ` Linus Torvalds 2012-05-16 17:28 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Linus Torvalds @ 2012-05-16 2:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git, Jeff King 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info 2012-05-16 2:02 ` Linus Torvalds @ 2012-05-16 17:28 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2012-05-16 17:28 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jonathan Nieder, git, Jeff King 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info 2012-05-11 22:46 ` Junio C Hamano 2012-05-11 23:20 ` Linus Torvalds @ 2012-06-06 20:27 ` Jonathan Nieder 2012-06-06 20:46 ` Jonathan Nieder 1 sibling, 1 reply; 36+ messages in thread From: Jonathan Nieder @ 2012-06-06 20:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, git, Jeff King, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info 2012-06-06 20:27 ` Jonathan Nieder @ 2012-06-06 20:46 ` Jonathan Nieder 2012-06-06 21:11 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Jonathan Nieder @ 2012-06-06 20:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, git, Jeff King, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH/RFC] fmt-merge-msg: add a blank line after people info 2012-06-06 20:46 ` Jonathan Nieder @ 2012-06-06 21:11 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2012-06-06 21:11 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Linus Torvalds, git, Jeff King, Johannes Sixt 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); } ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: A possible fmt-merge-msg update? 2012-03-05 20:33 ` Linus Torvalds 2012-03-05 21:34 ` [PATCH] fmt-merge-msg: show those involved in a merged series Junio C Hamano @ 2012-03-06 7:59 ` Jeff King 1 sibling, 0 replies; 36+ messages in thread From: Jeff King @ 2012-03-06 7:59 UTC (permalink / raw) To: Linus Torvalds; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2012-06-06 21:11 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-03-05 3:17 A possible fmt-merge-msg update? Junio C Hamano 2012-03-05 5:24 ` Linus Torvalds 2012-03-05 19:04 ` Junio C Hamano 2012-03-05 20:33 ` Linus Torvalds 2012-03-05 21:34 ` [PATCH] fmt-merge-msg: show those involved in a merged series Junio C Hamano 2012-03-05 21:46 ` Linus Torvalds 2012-03-05 21:49 ` Junio C Hamano 2012-03-07 21:22 ` René Scharfe 2012-03-07 21:59 ` Junio C Hamano 2012-03-08 17:46 ` René Scharfe 2012-03-08 19:18 ` Junio C Hamano 2012-03-08 21:31 ` Junio C Hamano 2012-03-12 21:37 ` Phil Hord 2012-03-13 21:03 ` Jeff King 2012-03-14 3:44 ` Junio C Hamano 2012-03-14 19:12 ` Phil Hord 2012-03-12 7:11 ` Jonathan Nieder 2012-03-13 1:55 ` Junio C Hamano 2012-03-13 5:23 ` Jonathan Nieder 2012-03-13 5:45 ` Junio C Hamano 2012-03-13 7:27 ` Johannes Sixt 2012-03-13 18:26 ` Junio C Hamano 2012-03-14 6:37 ` Johannes Sixt 2012-03-14 20:34 ` Junio C Hamano 2012-03-13 18:28 ` [PATCH v2 1/1] " Junio C Hamano 2012-05-11 10:31 ` [PATCH/RFC] fmt-merge-msg: add a blank line after people info Jonathan Nieder 2012-05-11 22:46 ` Junio C Hamano 2012-05-11 23:20 ` Linus Torvalds 2012-05-14 18:31 ` Junio C Hamano 2012-05-15 20:24 ` Junio C Hamano 2012-05-16 2:02 ` Linus Torvalds 2012-05-16 17:28 ` Junio C Hamano 2012-06-06 20:27 ` Jonathan Nieder 2012-06-06 20:46 ` Jonathan Nieder 2012-06-06 21:11 ` Junio C Hamano 2012-03-06 7:59 ` A possible fmt-merge-msg update? Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.