{"thread":{"id":"41091","subject":"[PATCH 0/14] counting trailers with shortlogs","startedAt":"2015-12-29T07:18:47Z","lastAt":"2016-01-04T10:31:06Z","messageCount":29,"participants":["Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":14},"messages":[{"id":"275093","messageId":"20151229071847.GA8726@sigill.intra.peff.net","threadId":"41091","inReplyTo":null,"subject":"[PATCH 0/14] counting trailers with shortlogs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:18:47Z","receivedAt":"2015-12-29T07:18:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I happened to be reading an old discussion on trailers a few weeks ago,\nand the idea was mentioned of providing access to commit trailers in a\nmore structured way from traversal commands. E.g., we could presumably\nhave a log pretty-format for showing \"Signed-off-by\", like\n\"%(trailer:signed-off-by)\" or something. But that opens up a lot of\nquestions, like the exact formatting, whether we would allow wildcards\n(\"*-by:\"), and if so how that affects formatting.\n\nSo I took the cowards way out and implemented trailer support for\ngit-shortlog, which seemed much simpler. I'll admit I haven't used it\nfor anything so far beyond a few \"fun\" queries, but it does provide some\ninfrastructure we could use later for git-log.\n\nAnd things being what they are, I ended up with a few cleanups and\noptimizations along the way. After this series, a regular \"shortlog -ns\"\nis about 10% faster.\n\n [01/14]: move string functions out of git-compat-util\n [02/14]: log: refactor add_header to drop some magic numbers\n [03/14]: strutil: add skip_prefix_case\n [04/14]: shortlog: use skip_prefix_icase to parse \"Author\" lines\n [05/14]: shortlog: use strbufs to read from stdin\n [06/14]: shortlog: replace hand-parsing of author with pretty-printer\n [07/14]: shortlog: optimize \"--summary\" mode\n [08/14]: shortlog: optimize out useless \"\" normalization\n [09/14]: shortlog: optimize out useless string list\n [10/14]: shortlog: change \"author\" variables to \"ident\"\n [11/14]: shortlog: allow grouping by committer ident\n [12/14]: trailer: factor out config reading\n [13/14]: trailer: add interface for parsing commit trailers\n [14/14]: shortlog: match commit trailers with --ident\n\n-Peff\n"},{"id":"275094","messageId":"20151229071936.GA8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 01/14] move string functions out of git-compat-util","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:19:37Z","receivedAt":"2015-12-29T07:19:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have several inline string helpers (skip_prefix, etc)\ndefined in git-compat-util.h. This is convenient, as it\nmakes them available everywhere, but this is probably not\nthe best place for them. Besides cluttering a file which is\notherwise mostly about compatibility issues, defining inline\nfunctions in git-compat-util.h restricts what they can do.\n\nFor instance, if we wanted to use tolower() in one of the\nfunctions, we run into an ordering problem with our\nredefinitions of tolower(), which are done later in\ngit-compat-util.h.\n\nThis patch moves them to their own header file, strutil.h.\nHowever, it also includes them in strbuf.h (which needs them\nfor its inline strbuf_strip_suffix). This also puts them\nindirectly into cache.h, which means they are effectively\navailable everywhere (just as they were before).\n\nSigned-off-by: Jeff King \n---\n git-compat-util.h | 63 ---------------------------------------------------\n strbuf.h | 2 ++\n strutil.h | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 69 insertions(+), 63 deletions(-)\n create mode 100644 strutil.h\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 2da0a75..da67dc6 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -430,69 +430,6 @@ extern void set_error_routine(void (*routine)(const char *err, va_list params));\n extern void set_die_is_recursing_routine(int (*routine)(void));\n extern void set_error_handle(FILE *);\n \n-extern int starts_with(const char *str, const char *prefix);\n-\n-/*\n- * If the string \"str\" begins with the string found in \"prefix\", return 1.\n- * The \"out\" parameter is set to \"str + strlen(prefix)\" (i.e., to the point in\n- * the string right after the prefix).\n- *\n- * Otherwise, return 0 and leave \"out\" untouched.\n- *\n- * Examples:\n- *\n- * [extract branch name, fail if not a branch]\n- * if (!skip_prefix(ref, \"refs/heads/\", &branch)\n- *\treturn -1;\n- *\n- * [skip prefix if present, otherwise use whole string]\n- * skip_prefix(name, \"refs/heads/\", &name);\n- */\n-static inline int skip_prefix(const char *str, const char *prefix,\n-\t\t\t const char **out)\n-{\n-\tdo {\n-\t\tif (!*prefix) {\n-\t\t\t*out = str;\n-\t\t\treturn 1;\n-\t\t}\n-\t} while (*str++ == *prefix++);\n-\treturn 0;\n-}\n-\n-/*\n- * If buf ends with suffix, return 1 and subtract the length of the suffix\n- * from *len. Otherwise, return 0 and leave *len untouched.\n- */\n-static inline int strip_suffix_mem(const char *buf, size_t *len,\n-\t\t\t\t const char *suffix)\n-{\n-\tsize_t suflen = strlen(suffix);\n-\tif (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen))\n-\t\treturn 0;\n-\t*len -= suflen;\n-\treturn 1;\n-}\n-\n-/*\n- * If str ends with suffix, return 1 and set *len to the size of the string\n- * without the suffix. Otherwise, return 0 and set *len to the size of the\n- * string.\n- *\n- * Note that we do _not_ NUL-terminate str to the new length.\n- */\n-static inline int strip_suffix(const char *str, const char *suffix, size_t *len)\n-{\n-\t*len = strlen(str);\n-\treturn strip_suffix_mem(str, len, suffix);\n-}\n-\n-static inline int ends_with(const char *str, const char *suffix)\n-{\n-\tsize_t len;\n-\treturn strip_suffix(str, suffix, &len);\n-}\n-\n #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\n \n #ifndef PROT_READ\ndiff --git a/strbuf.h b/strbuf.h\nindex 7123fca..a1a2df9 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -1,6 +1,8 @@\n #ifndef STRBUF_H\n #define STRBUF_H\n \n+#include \"strutil.h\"\n+\n /**\n * strbuf's are meant to be used with all the usual C string and memory\n * APIs. Given that the length of the buffer is known, it's often better to\ndiff --git a/strutil.h b/strutil.h\nnew file mode 100644\nindex 0000000..4fa2071\n--- /dev/null\n+++ b/strutil.h\n@@ -0,0 +1,67 @@\n+#ifndef STRUTIL_H\n+#define STRUTIL_H\n+\n+extern int starts_with(const char *str, const char *prefix);\n+\n+/*\n+ * If the string \"str\" begins with the string found in \"prefix\", return 1.\n+ * The \"out\" parameter is set to \"str + strlen(prefix)\" (i.e., to the point in\n+ * the string right after the prefix).\n+ *\n+ * Otherwise, return 0 and leave \"out\" untouched.\n+ *\n+ * Examples:\n+ *\n+ * [extract branch name, fail if not a branch]\n+ * if (!skip_prefix(ref, \"refs/heads/\", &branch)\n+ *\treturn -1;\n+ *\n+ * [skip prefix if present, otherwise use whole string]\n+ * skip_prefix(name, \"refs/heads/\", &name);\n+ */\n+static inline int skip_prefix(const char *str, const char *prefix,\n+\t\t\t const char **out)\n+{\n+\tdo {\n+\t\tif (!*prefix) {\n+\t\t\t*out = str;\n+\t\t\treturn 1;\n+\t\t}\n+\t} while (*str++ == *prefix++);\n+\treturn 0;\n+}\n+\n+/*\n+ * If buf ends with suffix, return 1 and subtract the length of the suffix\n+ * from *len. Otherwise, return 0 and leave *len untouched.\n+ */\n+static inline int strip_suffix_mem(const char *buf, size_t *len,\n+\t\t\t\t const char *suffix)\n+{\n+\tsize_t suflen = strlen(suffix);\n+\tif (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen))\n+\t\treturn 0;\n+\t*len -= suflen;\n+\treturn 1;\n+}\n+\n+/*\n+ * If str ends with suffix, return 1 and set *len to the size of the string\n+ * without the suffix. Otherwise, return 0 and set *len to the size of the\n+ * string.\n+ *\n+ * Note that we do _not_ NUL-terminate str to the new length.\n+ */\n+static inline int strip_suffix(const char *str, const char *suffix, size_t *len)\n+{\n+\t*len = strlen(str);\n+\treturn strip_suffix_mem(str, len, suffix);\n+}\n+\n+static inline int ends_with(const char *str, const char *suffix)\n+{\n+\tsize_t len;\n+\treturn strip_suffix(str, suffix, &len);\n+}\n+\n+#endif /* STRUTIL_H */\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275095","messageId":"20151229072048.GB8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 02/14] log: refactor add_header to drop some magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:20:48Z","receivedAt":"2015-12-29T07:20:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We want to chomp newlines off the end of the \"value\" string.\nBut because it's const, we must track its length rather than\nwriting a NUL. This leads to us having to tweak that length\nlater, to account for moving the pointer forward.\n\nSince we are about to create a copy of it anyway, let's just\nwait and chomp at the end.\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e00cea7..76823e6 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -675,21 +675,18 @@ static struct string_list extra_cc;\n static void add_header(const char *value)\n {\n \tstruct string_list_item *item;\n-\tint len = strlen(value);\n-\twhile (len && value[len - 1] == '\\n')\n-\t\tlen--;\n+\tsize_t len;\n \n-\tif (!strncasecmp(value, \"to: \", 4)) {\n+\tif (!strncasecmp(value, \"to: \", 4))\n \t\titem = string_list_append(&extra_to, value + 4);\n-\t\tlen -= 4;\n-\t} else if (!strncasecmp(value, \"cc: \", 4)) {\n+\telse if (!strncasecmp(value, \"cc: \", 4))\n \t\titem = string_list_append(&extra_cc, value + 4);\n-\t\tlen -= 4;\n-\t} else {\n+\telse\n \t\titem = string_list_append(&extra_hdr, value);\n-\t}\n \n-\titem->string[len] = '\\0';\n+\tlen = strlen(item->string);\n+\twhile (len && item->string[len - 1] == '\\n')\n+\t\titem->string[--len] = '\\0';\n }\n \n #define THREAD_SHALLOW 1\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275096","messageId":"20151229072204.GC8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 03/14] strutil: add skip_prefix_icase","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:22:04Z","receivedAt":"2015-12-29T07:22:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some sites that otherwise would use skip_prefix cannot do\nso, because it has no way to do case-insensitive\ncomparisons. Such sites usually get around this by using\nstrncasecmp, at the cost of having to use magic numbers.\nWe can help them by providing a case-insensitive version of\nskip_prefix.\n\nUnfortunately, we don't share any code with the original\nskip_prefix. Since this is performance-sensitive code, we\nwould not want to introduce an extra \"do we are about case?\"\nconditional into the middle of the loop. We could instead\nuse macros or another technique to generate the\nalmost-identical implementations, but the function simply\nisn't long enough to merit that confusing boilerplate.\n\nTo show off the new function, we convert a simple case in\nlog's add_header function.\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 8 ++++----\n strutil.h | 15 +++++++++++++++\n 2 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 76823e6..44c6b26 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -677,10 +677,10 @@ static void add_header(const char *value)\n \tstruct string_list_item *item;\n \tsize_t len;\n \n-\tif (!strncasecmp(value, \"to: \", 4))\n-\t\titem = string_list_append(&extra_to, value + 4);\n-\telse if (!strncasecmp(value, \"cc: \", 4))\n-\t\titem = string_list_append(&extra_cc, value + 4);\n+\tif (skip_prefix_icase(value, \"to: \", &value))\n+\t\titem = string_list_append(&extra_to, value);\n+\telse if (skip_prefix_icase(value, \"cc: \", &value))\n+\t\titem = string_list_append(&extra_cc, value);\n \telse\n \t\titem = string_list_append(&extra_hdr, value);\n \ndiff --git a/strutil.h b/strutil.h\nindex 4fa2071..bb4c02d 100644\n--- a/strutil.h\n+++ b/strutil.h\n@@ -32,6 +32,21 @@ static inline int skip_prefix(const char *str, const char *prefix,\n }\n \n /*\n+ * Identical to skip_prefix, but compare characters case-insensitively.\n+ */\n+static inline int skip_prefix_icase(const char *str, const char *prefix,\n+\t\t\t\t const char **out)\n+{\n+\tdo {\n+\t\tif (!*prefix) {\n+\t\t\t*out = str;\n+\t\t\treturn 1;\n+\t\t}\n+\t} while (tolower(*str++) == tolower(*prefix++));\n+\treturn 0;\n+}\n+\n+/*\n * If buf ends with suffix, return 1 and subtract the length of the suffix\n * from *len. Otherwise, return 0 and leave *len untouched.\n */\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275097","messageId":"20151229072746.GD8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 04/14] shortlog: use skip_prefix_icase to parse \"Author\" lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:27:46Z","receivedAt":"2015-12-29T07:27:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because we must match both \"Author\" and \"author\" here, we\ncould not use skip_prefix, and had to hand-code a partial\ncase-insensitive match. Now that we have skip_prefix_case,\nwe can use it. This is technically more liberal in what it\nmatches (e.g., it will match AUTHOR), but in this particular\ncase that that's OK (we are matching git-log output, so we\nexpect arbitrary data like commit headers to be indented).\n\nIn addition to being easier to read, this will make the code\neasier to adapt for matching other lines.\n\nSigned-off-by: Jeff King \n---\nTo be honest, I'm not sure what the original was trying for. I assumed\nit was to match \"log --raw\" output, but because we always expect the\ncolon, it doesn't. I think this may actually have broken in b8ec592\n(Build in shortlog, 2006-10-22); the original perl script looked for:\n\n /^[Aa]uthor:?\\s*/\n\nWe could re-fix that, I guess, but it seems clear that nobody actually\ncares (and anybody sane uses the builtin traversal these days anyway).\n\nWe could similarly drop the case-insensitivity, as I don't think it is\nhelping anyone in practice (though that is less easy to know).\n\n builtin/shortlog.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 35ebd17..86e277a 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -94,8 +94,8 @@ static void read_from_stdin(struct shortlog *log)\n \tchar author[1024], oneline[1024];\n \n \twhile (fgets(author, sizeof(author), stdin) != NULL) {\n-\t\tif (!(author[0] == 'A' || author[0] == 'a') ||\n-\t\t !starts_with(author + 1, \"uthor: \"))\n+\t\tconst char *v;\n+\t\tif (!skip_prefix_icase(author, \"Author: \", &v))\n \t\t\tcontinue;\n \t\twhile (fgets(oneline, sizeof(oneline), stdin) &&\n \t\t oneline[0] != '\\n')\n@@ -103,7 +103,7 @@ static void read_from_stdin(struct shortlog *log)\n \t\twhile (fgets(oneline, sizeof(oneline), stdin) &&\n \t\t oneline[0] == '\\n')\n \t\t\t; /* discard blanks */\n-\t\tinsert_one_record(log, author + 8, oneline);\n+\t\tinsert_one_record(log, v, oneline);\n \t}\n }\n \n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275098","messageId":"20151229072803.GE8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 05/14] shortlog: use strbufs to read from stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:28:03Z","receivedAt":"2015-12-29T07:28:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently use fixed-size buffers with fgets(), which\ncould lead to incorrect results in the unlikely event that a\nline had something like \"Author:\" at exactly its 1024th\ncharacter.\n\nBut it's easy to convert this to a strbuf, and because we\ncan reuse the same buffer through the loop, we don't even\npay the extra allocation cost.\n\nSigned-off-by: Jeff King \n---\n builtin/shortlog.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 86e277a..668cdb4 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -91,20 +91,23 @@ static void insert_one_record(struct shortlog *log,\n \n static void read_from_stdin(struct shortlog *log)\n {\n-\tchar author[1024], oneline[1024];\n+\tstruct strbuf author = STRBUF_INIT;\n+\tstruct strbuf oneline = STRBUF_INIT;\n \n-\twhile (fgets(author, sizeof(author), stdin) != NULL) {\n+\twhile (strbuf_getline(&author, stdin, '\\n') != EOF) {\n \t\tconst char *v;\n-\t\tif (!skip_prefix_icase(author, \"Author: \", &v))\n+\t\tif (!skip_prefix_icase(author.buf, \"Author: \", &v))\n \t\t\tcontinue;\n-\t\twhile (fgets(oneline, sizeof(oneline), stdin) &&\n-\t\t oneline[0] != '\\n')\n+\t\twhile (strbuf_getline(&oneline, stdin, '\\n') != EOF &&\n+\t\t oneline.len)\n \t\t\t; /* discard headers */\n-\t\twhile (fgets(oneline, sizeof(oneline), stdin) &&\n-\t\t oneline[0] == '\\n')\n+\t\twhile (strbuf_getline(&oneline, stdin, '\\n') != EOF &&\n+\t\t !oneline.len)\n \t\t\t; /* discard blanks */\n-\t\tinsert_one_record(log, v, oneline);\n+\t\tinsert_one_record(log, v, oneline.buf);\n \t}\n+\tstrbuf_release(&author);\n+\tstrbuf_release(&oneline);\n }\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275099","messageId":"20151229072953.GF8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 06/14] shortlog: replace hand-parsing of author with pretty-printer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:29:53Z","receivedAt":"2015-12-29T07:29:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When gathering the author and oneline subject for each\ncommit, we hand-parse the commit headers to find the\n\"author\" line, and then continue past to the blank line at\nthe end of the header.\n\nWe can replace this tricky hand-parsing by simply asking the\npretty-printer for the relevant items. This also decouples\nthe author and oneline parsing, opening up some new\noptimizations in further commits.\n\nOne reason to avoid the pretty-printer is that it might be\nless efficient than hand-parsing. However, I measured no\nslowdown at all running \"git shortlog -ns HEAD\" on\nlinux.git.\n\nSigned-off-by: Jeff King \n---\n builtin/shortlog.c | 57 +++++++++++++++++++++---------------------------------\n 1 file changed, 22 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 668cdb4..10c92fa 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -112,45 +112,32 @@ static void read_from_stdin(struct shortlog *log)\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n-\tconst char *author = NULL, *buffer;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstruct strbuf ufbuf = STRBUF_INIT;\n-\n-\tpp_commit_easy(CMIT_FMT_RAW, commit, &buf);\n-\tbuffer = buf.buf;\n-\twhile (*buffer && *buffer != '\\n') {\n-\t\tconst char *eol = strchr(buffer, '\\n');\n-\n-\t\tif (eol == NULL)\n-\t\t\teol = buffer + strlen(buffer);\n-\t\telse\n-\t\t\teol++;\n-\n-\t\tif (starts_with(buffer, \"author \"))\n-\t\t\tauthor = buffer + 7;\n-\t\tbuffer = eol;\n-\t}\n-\tif (!author) {\n+\tstruct strbuf author = STRBUF_INIT;\n+\tstruct strbuf oneline = STRBUF_INIT;\n+\tstruct pretty_print_context ctx = {0};\n+\n+\tctx.fmt = CMIT_FMT_USERFORMAT;\n+\tctx.abbrev = log->abbrev;\n+\tctx.subject = \"\";\n+\tctx.after_subject = \"\";\n+\tctx.date_mode.type = DATE_NORMAL;\n+\tctx.output_encoding = get_log_output_encoding();\n+\n+\tformat_commit_message(commit, \"%an <%ae>\", &author, &ctx);\n+\tif (author.len <= 3) {\n \t\twarning(_(\"Missing author: %s\"),\n \t\t oid_to_hex(&commit->object.oid));\n \t\treturn;\n \t}\n-\tif (log->user_format) {\n-\t\tstruct pretty_print_context ctx = {0};\n-\t\tctx.fmt = CMIT_FMT_USERFORMAT;\n-\t\tctx.abbrev = log->abbrev;\n-\t\tctx.subject = \"\";\n-\t\tctx.after_subject = \"\";\n-\t\tctx.date_mode.type = DATE_NORMAL;\n-\t\tctx.output_encoding = get_log_output_encoding();\n-\t\tpretty_print_commit(&ctx, commit, &ufbuf);\n-\t\tbuffer = ufbuf.buf;\n-\t} else if (*buffer) {\n-\t\tbuffer++;\n-\t}\n-\tinsert_one_record(log, author, !*buffer ? \"\" : buffer);\n-\tstrbuf_release(&ufbuf);\n-\tstrbuf_release(&buf);\n+\n+\tif (log->user_format)\n+\t\tpretty_print_commit(&ctx, commit, &oneline);\n+\telse\n+\t\tformat_commit_message(commit, \"%s\", &oneline, &ctx);\n+\n+\tinsert_one_record(log, author.buf, oneline.len ? oneline.buf : \"\");\n+\tstrbuf_release(&author);\n+\tstrbuf_release(&oneline);\n }\n \n static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275100","messageId":"20151229073027.GG8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 07/14] shortlog: optimize \"--summary\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:30:27Z","receivedAt":"2015-12-29T07:30:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the user asked us only to show counts for each author,\nrather than the individual summary lines, then there is no\npoint in us generating the summaries only to throw them\naway. With this patch, I measured the following speedup for\n\"git shortlog -ns HEAD\" on linux.git (best-of-five):\n\n [before]\n real 0m5.644s\n user 0m5.472s\n sys 0m0.176s\n\n [after]\n real 0m5.257s\n user 0m5.104s\n sys 0m0.156s\n\nThat's only ~7%, but it's so easy to do, there's no good\nreason not to. We don't have to touch any downstream code,\nsince we already fill in the magic string \"\" to handle\ncommits without a message.\n\nSigned-off-by: Jeff King \n---\n builtin/shortlog.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 10c92fa..af24d84 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -130,10 +130,12 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\treturn;\n \t}\n \n-\tif (log->user_format)\n-\t\tpretty_print_commit(&ctx, commit, &oneline);\n-\telse\n-\t\tformat_commit_message(commit, \"%s\", &oneline, &ctx);\n+\tif (!log->summary) {\n+\t\tif (log->user_format)\n+\t\t\tpretty_print_commit(&ctx, commit, &oneline);\n+\t\telse\n+\t\t\tformat_commit_message(commit, \"%s\", &oneline, &ctx);\n+\t}\n \n \tinsert_one_record(log, author.buf, oneline.len ? oneline.buf : \"\");\n \tstrbuf_release(&author);\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275101","messageId":"20151229073108.GH8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 08/14] shortlog: optimize out useless \"\" normalization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:31:08Z","receivedAt":"2015-12-29T07:31:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we are in --summary mode, we will always pass to\ninsert_one_record, which will then do some normalization\n(e.g., cutting out \"[PATCH]\"). There's no point in doing so\nif we aren't going to use the result anyway.\n\nThis drops my best-of-five for \"git shortlog -ns HEAD\" on\nlinux.git from:\n\n real 0m5.257s\n user 0m5.104s\n sys 0m0.156s\n\nto:\n\n real 0m5.194s\n user 0m5.028s\n sys 0m0.168s\n\nThat's only 1%, but arguably the result is clearer to read,\nas we're able to group our variable declarations inside the\nconditional block. It also opens up further optimization\npossibilities for future patches.\n\nSigned-off-by: Jeff King \n---\n builtin/shortlog.c | 63 +++++++++++++++++++++++++++++-------------------------\n 1 file changed, 34 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex af24d84..400e03b 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -31,13 +31,9 @@ static void insert_one_record(struct shortlog *log,\n \t\t\t const char *author,\n \t\t\t const char *oneline)\n {\n-\tconst char *dot3 = log->common_repo_prefix;\n-\tchar *buffer, *p;\n \tstruct string_list_item *item;\n \tconst char *mailbuf, *namebuf;\n \tsize_t namelen, maillen;\n-\tconst char *eol;\n-\tstruct strbuf subject = STRBUF_INIT;\n \tstruct strbuf namemailbuf = STRBUF_INIT;\n \tstruct ident_split ident;\n \n@@ -59,34 +55,43 @@ static void insert_one_record(struct shortlog *log,\n \tif (item->util == NULL)\n \t\titem->util = xcalloc(1, sizeof(struct string_list));\n \n-\t/* Skip any leading whitespace, including any blank lines. */\n-\twhile (*oneline && isspace(*oneline))\n-\t\toneline++;\n-\teol = strchr(oneline, '\\n');\n-\tif (!eol)\n-\t\teol = oneline + strlen(oneline);\n-\tif (starts_with(oneline, \"[PATCH\")) {\n-\t\tchar *eob = strchr(oneline, ']');\n-\t\tif (eob && (!eol || eob < eol))\n-\t\t\toneline = eob + 1;\n-\t}\n-\twhile (*oneline && isspace(*oneline) && *oneline != '\\n')\n-\t\toneline++;\n-\tformat_subject(&subject, oneline, \" \");\n-\tbuffer = strbuf_detach(&subject, NULL);\n-\n-\tif (dot3) {\n-\t\tint dot3len = strlen(dot3);\n-\t\tif (dot3len > 5) {\n-\t\t\twhile ((p = strstr(buffer, dot3)) != NULL) {\n-\t\t\t\tint taillen = strlen(p) - dot3len;\n-\t\t\t\tmemcpy(p, \"/.../\", 5);\n-\t\t\t\tmemmove(p + 5, p + dot3len, taillen + 1);\n+\tif (log->summary)\n+\t\tstring_list_append(item->util, xstrdup(\"\"));\n+\telse {\n+\t\tconst char *dot3 = log->common_repo_prefix;\n+\t\tchar *buffer, *p;\n+\t\tstruct strbuf subject = STRBUF_INIT;\n+\t\tconst char *eol;\n+\n+\t\t/* Skip any leading whitespace, including any blank lines. */\n+\t\twhile (*oneline && isspace(*oneline))\n+\t\t\toneline++;\n+\t\teol = strchr(oneline, '\\n');\n+\t\tif (!eol)\n+\t\t\teol = oneline + strlen(oneline);\n+\t\tif (starts_with(oneline, \"[PATCH\")) {\n+\t\t\tchar *eob = strchr(oneline, ']');\n+\t\t\tif (eob && (!eol || eob < eol))\n+\t\t\t\toneline = eob + 1;\n+\t\t}\n+\t\twhile (*oneline && isspace(*oneline) && *oneline != '\\n')\n+\t\t\toneline++;\n+\t\tformat_subject(&subject, oneline, \" \");\n+\t\tbuffer = strbuf_detach(&subject, NULL);\n+\n+\t\tif (dot3) {\n+\t\t\tint dot3len = strlen(dot3);\n+\t\t\tif (dot3len > 5) {\n+\t\t\t\twhile ((p = strstr(buffer, dot3)) != NULL) {\n+\t\t\t\t\tint taillen = strlen(p) - dot3len;\n+\t\t\t\t\tmemcpy(p, \"/.../\", 5);\n+\t\t\t\t\tmemmove(p + 5, p + dot3len, taillen + 1);\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n-\t}\n \n-\tstring_list_append(item->util, buffer);\n+\t\tstring_list_append(item->util, buffer);\n+\t}\n }\n \n static void read_from_stdin(struct shortlog *log)\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275102","messageId":"20151229073219.GI8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 09/14] shortlog: optimize out useless string list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:32:19Z","receivedAt":"2015-12-29T07:32:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we are in \"--summary\" mode, then we do not care about the\nactual list of subject onelines associated with each author.\nWe care only about the number. So rather than store a\nstring-list for each author full of \"\", let's just\nkeep a count.\n\nThis drops my best-of-five for \"git shortlog -ns HEAD\" on\nlinux.git from:\n\n real 0m5.194s\n user 0m5.028s\n sys 0m0.168s\n\nto:\n\n real 0m5.057s\n user 0m4.916s\n sys 0m0.144s\n\nThat's about 2.5%.\n\nSigned-off-by: Jeff King \n---\n builtin/shortlog.c | 43 +++++++++++++++++++++++++++++++------------\n 1 file changed, 31 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 400e03b..7ec6b76 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -14,7 +14,26 @@ static char const * const shortlog_usage[] = {\n \tNULL\n };\n \n-static int compare_by_number(const void *a1, const void *a2)\n+/*\n+ * The util field of our string_list_items will contain one of two things:\n+ *\n+ * - if --summary is not in use, it will point to a string list of the\n+ * oneline subjects assigned to this author\n+ *\n+ * - if --summary is in use, we don't need that list; we only need to know\n+ * its size. So we abuse the pointer slot to store our integer counter.\n+ *\n+ * This macro accesses the latter.\n+ */\n+#define UTIL_TO_INT(x) ((intptr_t)(x)->util)\n+\n+static int compare_by_counter(const void *a1, const void *a2)\n+{\n+\tconst struct string_list_item *i1 = a1, *i2 = a2;\n+\treturn UTIL_TO_INT(i2) - UTIL_TO_INT(i1);\n+}\n+\n+static int compare_by_list(const void *a1, const void *a2)\n {\n \tconst struct string_list_item *i1 = a1, *i2 = a2;\n \tconst struct string_list *l1 = i1->util, *l2 = i2->util;\n@@ -52,11 +71,9 @@ static void insert_one_record(struct shortlog *log,\n \t\tstrbuf_addf(&namemailbuf, \" <%.*s>\", (int)maillen, mailbuf);\n \n \titem = string_list_insert(&log->list, namemailbuf.buf);\n-\tif (item->util == NULL)\n-\t\titem->util = xcalloc(1, sizeof(struct string_list));\n \n \tif (log->summary)\n-\t\tstring_list_append(item->util, xstrdup(\"\"));\n+\t\titem->util = (void *)(UTIL_TO_INT(item) + 1);\n \telse {\n \t\tconst char *dot3 = log->common_repo_prefix;\n \t\tchar *buffer, *p;\n@@ -90,6 +107,8 @@ static void insert_one_record(struct shortlog *log,\n \t\t\t}\n \t\t}\n \n+\t\tif (item->util == NULL)\n+\t\t\titem->util = xcalloc(1, sizeof(struct string_list));\n \t\tstring_list_append(item->util, buffer);\n \t}\n }\n@@ -291,14 +310,14 @@ void shortlog_output(struct shortlog *log)\n \n \tif (log->sort_by_number)\n \t\tqsort(log->list.items, log->list.nr, sizeof(struct string_list_item),\n-\t\t\tcompare_by_number);\n+\t\t log->summary ? compare_by_counter : compare_by_list);\n \tfor (i = 0; i < log->list.nr; i++) {\n-\t\tstruct string_list *onelines = log->list.items[i].util;\n-\n+\t\tconst struct string_list_item *item = &log->list.items[i];\n \t\tif (log->summary) {\n-\t\t\tprintf(\"%6d\\t%s\\n\", onelines->nr, log->list.items[i].string);\n+\t\t\tprintf(\"%6d\\t%s\\n\", (int)UTIL_TO_INT(item), item->string);\n \t\t} else {\n-\t\t\tprintf(\"%s (%d):\\n\", log->list.items[i].string, onelines->nr);\n+\t\t\tstruct string_list *onelines = item->util;\n+\t\t\tprintf(\"%s (%d):\\n\", item->string, onelines->nr);\n \t\t\tfor (j = onelines->nr - 1; j >= 0; j--) {\n \t\t\t\tconst char *msg = onelines->items[j].string;\n \n@@ -311,11 +330,11 @@ void shortlog_output(struct shortlog *log)\n \t\t\t\t\tprintf(\" %s\\n\", msg);\n \t\t\t}\n \t\t\tputchar('\\n');\n+\t\t\tonelines->strdup_strings = 1;\n+\t\t\tstring_list_clear(onelines, 0);\n+\t\t\tfree(onelines);\n \t\t}\n \n-\t\tonelines->strdup_strings = 1;\n-\t\tstring_list_clear(onelines, 0);\n-\t\tfree(onelines);\n \t\tlog->list.items[i].util = NULL;\n \t}\n \n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275103","messageId":"20151229073241.GJ8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 10/14] shortlog: change \"author\" variables to \"ident\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:32:41Z","receivedAt":"2015-12-29T07:32:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is in preparation for shortlog counting more things\nthan just authors. Breaking it out into a separate patch\nkeeps the noise down when the real changes come.\n\nSigned-off-by: Jeff King \n---\n builtin/shortlog.c | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 7ec6b76..d7eb0cb 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -47,7 +47,7 @@ static int compare_by_list(const void *a1, const void *a2)\n }\n \n static void insert_one_record(struct shortlog *log,\n-\t\t\t const char *author,\n+\t\t\t const char *ident_string,\n \t\t\t const char *oneline)\n {\n \tstruct string_list_item *item;\n@@ -56,7 +56,7 @@ static void insert_one_record(struct shortlog *log,\n \tstruct strbuf namemailbuf = STRBUF_INIT;\n \tstruct ident_split ident;\n \n-\tif (split_ident_line(&ident, author, strlen(author)))\n+\tif (split_ident_line(&ident, ident_string, strlen(ident_string)))\n \t\treturn;\n \n \tnamebuf = ident.name_begin;\n@@ -115,12 +115,12 @@ static void insert_one_record(struct shortlog *log,\n \n static void read_from_stdin(struct shortlog *log)\n {\n-\tstruct strbuf author = STRBUF_INIT;\n+\tstruct strbuf ident = STRBUF_INIT;\n \tstruct strbuf oneline = STRBUF_INIT;\n \n-\twhile (strbuf_getline(&author, stdin, '\\n') != EOF) {\n+\twhile (strbuf_getline(&ident, stdin, '\\n') != EOF) {\n \t\tconst char *v;\n-\t\tif (!skip_prefix_icase(author.buf, \"Author: \", &v))\n+\t\tif (!skip_prefix_icase(ident.buf, \"Author: \", &v))\n \t\t\tcontinue;\n \t\twhile (strbuf_getline(&oneline, stdin, '\\n') != EOF &&\n \t\t oneline.len)\n@@ -130,13 +130,13 @@ static void read_from_stdin(struct shortlog *log)\n \t\t\t; /* discard blanks */\n \t\tinsert_one_record(log, v, oneline.buf);\n \t}\n-\tstrbuf_release(&author);\n+\tstrbuf_release(&ident);\n \tstrbuf_release(&oneline);\n }\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n-\tstruct strbuf author = STRBUF_INIT;\n+\tstruct strbuf ident = STRBUF_INIT;\n \tstruct strbuf oneline = STRBUF_INIT;\n \tstruct pretty_print_context ctx = {0};\n \n@@ -147,8 +147,8 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tctx.date_mode.type = DATE_NORMAL;\n \tctx.output_encoding = get_log_output_encoding();\n \n-\tformat_commit_message(commit, \"%an <%ae>\", &author, &ctx);\n-\tif (author.len <= 3) {\n+\tformat_commit_message(commit, \"%an <%ae>\", &ident, &ctx);\n+\tif (ident.len <= 3) {\n \t\twarning(_(\"Missing author: %s\"),\n \t\t oid_to_hex(&commit->object.oid));\n \t\treturn;\n@@ -161,8 +161,8 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\t\tformat_commit_message(commit, \"%s\", &oneline, &ctx);\n \t}\n \n-\tinsert_one_record(log, author.buf, oneline.len ? oneline.buf : \"\");\n-\tstrbuf_release(&author);\n+\tinsert_one_record(log, ident.buf, oneline.len ? oneline.buf : \"\");\n+\tstrbuf_release(&ident);\n \tstrbuf_release(&oneline);\n }\n \n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275104","messageId":"20151229073515.GK8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 11/14] shortlog: allow grouping by committer ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:35:16Z","receivedAt":"2015-12-29T07:35:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Shortlog always groups commits by the author field. It can\nbe interesting to group by other fields, as well. The\nobvious other identity in each commit is the committer\nfield. This might be interesting if your project follows a\nworkflow where committers and authors are not the same and\nwhere there is not simply one maintainer picking up all of\nthe patches.\n\nFor instance, you can use this in git.git to see interim and\nsubsystem maintainers. Likewise, you can see in linux.git\nthe patches picked up by lieutenants and merged into Linus's\ntree.\n\nThis patch also provides some of the necessary\ninfrastructure for adding more ident types (e.g., from\ntrailers) in the future.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-shortlog.txt | 8 +++++++\n builtin/shortlog.c | 51 +++++++++++++++++++++++++++++++++++++-----\n shortlog.h | 5 +++++\n t/t4201-shortlog.sh | 13 +++++++++++\n 4 files changed, 71 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex 31af7f2..a89a01e 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -47,6 +47,14 @@ OPTIONS\n \n \tEach pretty-printed commit will be rewrapped before it is shown.\n \n+--ident=::\n+\tBy default, `shortlog` collects and collates author identities;\n+\tusing `--ident` will collect other types of identity. If\n+\t`` is:\n++\n+ - `author`, commits are grouped by author (this is the default)\n+ - `committer`, commits are grouped by committer\n+\n -w[[,[,]]]::\n \tLinewrap the output by wrapping each line at `width`. The first\n \tline of each entry is indented by `indent1` spaces, and the second\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex d7eb0cb..39da2d4 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -115,12 +115,22 @@ static void insert_one_record(struct shortlog *log,\n \n static void read_from_stdin(struct shortlog *log)\n {\n+\tconst char *ident_header = NULL;\n \tstruct strbuf ident = STRBUF_INIT;\n \tstruct strbuf oneline = STRBUF_INIT;\n \n+\tswitch (log->ident_type) {\n+\tcase SHORTLOG_IDENT_AUTHOR:\n+\t\tident_header = \"Author: \";\n+\t\tbreak;\n+\tcase SHORTLOG_IDENT_COMMITTER:\n+\t\tident_header = \"Commit: \";\n+\t\tbreak;\n+\t}\n+\n \twhile (strbuf_getline(&ident, stdin, '\\n') != EOF) {\n \t\tconst char *v;\n-\t\tif (!skip_prefix_icase(ident.buf, \"Author: \", &v))\n+\t\tif (!skip_prefix_icase(ident.buf, ident_header, &v))\n \t\t\tcontinue;\n \t\twhile (strbuf_getline(&oneline, stdin, '\\n') != EOF &&\n \t\t oneline.len)\n@@ -147,11 +157,23 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tctx.date_mode.type = DATE_NORMAL;\n \tctx.output_encoding = get_log_output_encoding();\n \n-\tformat_commit_message(commit, \"%an <%ae>\", &ident, &ctx);\n-\tif (ident.len <= 3) {\n-\t\twarning(_(\"Missing author: %s\"),\n-\t\t oid_to_hex(&commit->object.oid));\n-\t\treturn;\n+\tswitch (log->ident_type) {\n+\tcase SHORTLOG_IDENT_AUTHOR:\n+\t\tformat_commit_message(commit, \"%an <%ae>\", &ident, &ctx);\n+\t\tif (ident.len <= 3) {\n+\t\t\twarning(_(\"Missing author: %s\"),\n+\t\t\t\toid_to_hex(&commit->object.oid));\n+\t\t\treturn;\n+\t\t}\n+\t\tbreak;\n+\tcase SHORTLOG_IDENT_COMMITTER:\n+\t\tformat_commit_message(commit, \"%cn <%ce>\", &ident, &ctx);\n+\t\tif (ident.len <= 3) {\n+\t\t\twarning(_(\"Missing committer: %s\"),\n+\t\t\t\toid_to_hex(&commit->object.oid));\n+\t\t\treturn;\n+\t\t}\n+\t\tbreak;\n \t}\n \n \tif (!log->summary) {\n@@ -223,6 +245,21 @@ static int parse_wrap_args(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int parse_ident_option(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct shortlog *log = opt->value;\n+\n+\tif (unset || !strcasecmp(arg, \"author\"))\n+\t\tlog->ident_type = SHORTLOG_IDENT_AUTHOR;\n+\telse if (!strcasecmp(arg, \"committer\"))\n+\t\tlog->ident_type = SHORTLOG_IDENT_COMMITTER;\n+\telse\n+\t\tdie(\"unknown ident type: %s\", arg);\n+\n+\treturn 0;\n+}\n+\n+\n void shortlog_init(struct shortlog *log)\n {\n \tmemset(log, 0, sizeof(*log));\n@@ -250,6 +287,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"Show the email address of each author\")),\n \t\t{ OPTION_CALLBACK, 'w', NULL, &log, N_(\"w[,i1[,i2]]\"),\n \t\t\tN_(\"Linewrap output\"), PARSE_OPT_OPTARG, &parse_wrap_args },\n+\t\t{ OPTION_CALLBACK, 0, \"ident\", &log, N_(\"field\"),\n+\t\t\tN_(\"Use ident from field\"), 0, parse_ident_option },\n \t\tOPT_END(),\n \t};\n \ndiff --git a/shortlog.h b/shortlog.h\nindex de4f86f..a365620 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -14,6 +14,11 @@ struct shortlog {\n \tint user_format;\n \tint abbrev;\n \n+\tenum {\n+\t\tSHORTLOG_IDENT_AUTHOR = 0,\n+\t\tSHORTLOG_IDENT_COMMITTER\n+\t} ident_type;\n+\n \tchar *common_repo_prefix;\n \tint email;\n \tstruct string_list mailmap;\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 7600a3e..867a7ae 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -194,4 +194,17 @@ test_expect_success 'shortlog with revision pseudo options' '\n \tgit shortlog --exclude=refs/heads/m* --all\n '\n \n+test_expect_success 'shortlog --ident=committer (internal)' '\n+\tcat >expect <<-\\EOF &&\n+\t 5\tC O Mitter\n+\tEOF\n+\tgit shortlog -ns --ident=committer HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'shortlog --ident=committer (external)' '\n+\tgit log --format=full | git shortlog -ns --ident=committer >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275105","messageId":"20151229073536.GL8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 12/14] trailer: factor out config reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:35:37Z","receivedAt":"2015-12-29T07:35:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The only entry point into the trailer.c code is\nprocess_trailers, which initializes itself by reading the\ntrailer config. Let's pull that out into a separate function\nand protect it with a lazy-initialization flag. That will\nlet us easily add new entry points in future patches.\n\nSigned-off-by: Jeff King \n---\n trailer.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 6f3416f..18bf209 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -842,6 +842,19 @@ static void free_all(struct trailer_item **first)\n \t}\n }\n \n+static void init_trailer_config(void)\n+{\n+\tstatic int initialized;\n+\n+\tif (initialized)\n+\t\treturn;\n+\n+\tgit_config(git_trailer_default_config, NULL);\n+\tgit_config(git_trailer_config, NULL);\n+\n+\tinitialized = 1;\n+}\n+\n void process_trailers(const char *file, int trim_empty, struct string_list *trailers)\n {\n \tstruct trailer_item *in_tok_first = NULL;\n@@ -850,10 +863,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \tstruct strbuf **lines;\n \tint trailer_end;\n \n-\t/* Default config must be setup first */\n-\tgit_config(git_trailer_default_config, NULL);\n-\tgit_config(git_trailer_config, NULL);\n-\n+\tinit_trailer_config();\n \tlines = read_input_file(file);\n \n \t/* Print the lines before the trailers */\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275106","messageId":"20151229073658.GM8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 13/14] trailer: add interface for parsing commit trailers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:36:58Z","receivedAt":"2015-12-29T07:36:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git-trailer command and its subfunctions work in only\none way: they take a set of trailer operations, and pass\nthrough a file while performing those operations on it.\n\nHowever, other parts of the system may want to simply parse\ntrailers, and we should be able to reuse the code here. This\npatch provides a simple interface for parsing a commit\nmessage with trailers, iterating over them, and retrieving\nindividual keys.\n\nThe trailer code is a little heavy on the use of strbufs, so\nthis is perhaps a bit slower than it would be if we were\nable to parse the message in place (and this speed matters\nwhen you are iterating over every commit in the repository).\nHowever, there's nothing in this interface that paints us\ntoo far into a corner; we can always optimize the internals\nlater.\n\nSigned-off-by: Jeff King \n---\n trailer.c | 43 +++++++++++++++++++++++++++++++++++++++++++\n trailer.h | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 74 insertions(+)\n\ndiff --git a/trailer.c b/trailer.c\nindex 18bf209..36ba476 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -882,3 +882,46 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \n \tstrbuf_list_free(lines);\n }\n+\n+void trailer_parse_init(struct trailer_parse_context *ctx, const struct strbuf *buf)\n+{\n+\tint nr_lines = 0;\n+\n+\tinit_trailer_config();\n+\n+\tctx->lines = strbuf_split(buf, '\\n');\n+\twhile (ctx->lines[nr_lines])\n+\t\tnr_lines++;\n+\n+\tctx->end = find_trailer_end(ctx->lines, nr_lines);\n+\tctx->start = find_trailer_start(ctx->lines, ctx->end);\n+\n+\tstrbuf_init(&ctx->token, 0);\n+\tstrbuf_init(&ctx->value, 0);\n+}\n+\n+void trailer_parse_clear(struct trailer_parse_context *ctx)\n+{\n+\tstrbuf_list_free(ctx->lines);\n+\tstrbuf_release(&ctx->token);\n+\tstrbuf_release(&ctx->value);\n+}\n+\n+const char *trailer_parse_match(struct trailer_parse_context *ctx, int line, const char *match)\n+{\n+\tsize_t len;\n+\n+\tif (ctx->lines[line]->buf[0] == comment_line_char)\n+\t\treturn NULL;\n+\n+\tstrbuf_reset(&ctx->token);\n+\tstrbuf_reset(&ctx->value);\n+\tif (parse_trailer(&ctx->token, &ctx->value, ctx->lines[line]->buf))\n+\t\treturn NULL;\n+\n+\tlen = token_len_without_separator(ctx->token.buf, ctx->token.len);\n+\tif (strncasecmp(ctx->token.buf, match, len) || match[len])\n+\t\treturn NULL;\n+\n+\treturn ctx->value.buf;\n+}\ndiff --git a/trailer.h b/trailer.h\nindex 8eb25d5..1f985f6 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -3,4 +3,35 @@\n \n void process_trailers(const char *file, int trim_empty, struct string_list *trailers);\n \n+struct trailer_parse_context {\n+\tstruct strbuf **lines;\n+\tint start;\n+\tint end;\n+\n+\t/* These fields are private to the parser. */\n+\tstruct strbuf token;\n+\tstruct strbuf value;\n+};\n+\n+/*\n+ * Parse the commit message found in \"buf\", looking for trailers. Any data in\n+ * ctx is overwritten, and should later be freed with trailer_parse_clear().\n+ *\n+ * The caller can iterate over all trailers using the \"start\" and \"end\" indices\n+ * into \"lines\".\n+ */\n+void trailer_parse_init(struct trailer_parse_context *ctx, const struct strbuf *buf);\n+\n+/*\n+ * If the line contains a trailer with key \"trailer\", returns a pointer into\n+ * \"line\" for the value. Otherwise, returns NULL.\n+ */\n+const char *trailer_parse_match(struct trailer_parse_context *ctx, int line,\n+\t\t\t\tconst char *trailer);\n+\n+/*\n+ * Free resources allocated by trailer_parse_init().\n+ */\n+void trailer_parse_clear(struct trailer_parse_context *ctx);\n+\n #endif /* TRAILER_H */\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275107","messageId":"20151229073832.GN8842@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229071847.GA8726@sigill.intra.peff.net","subject":"[PATCH 14/14] shortlog: match commit trailers with --ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:38:32Z","receivedAt":"2015-12-29T07:38:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a project uses commit trailers, this patch lets you use\nshortlog to see who is performing each action. For example,\nrunning:\n\n git shortlog -ns --ident=reviewed-by\n\nin git.git shows who has reviewed. You can even use a custom\nformat to see things like who has helped whom:\n\n git shortlog --format=\"...helped %an (%ad)\" --ident=helped-by\n\nThis does run a bit more slowly than a normal\nauthor-grouping (about 33% slower in my tests). Some of that\nis natural, because we have to spend time parsing the\ntrailers. But I suspect a fair bit of that could be cut off\nby optimizing the trailer code, which involves quite a few\nmore copies of the data than we actually need.\n\nStill, it is certainly fast enough to be usable, and\noptimization can come later.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-shortlog.txt | 9 +++++++++\n builtin/shortlog.c | 38 +++++++++++++++++++++++++++++++++++---\n shortlog.h | 4 +++-\n t/t4201-shortlog.sh | 9 +++++++++\n 4 files changed, 56 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex a89a01e..16080c4 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -54,6 +54,15 @@ OPTIONS\n +\n - `author`, commits are grouped by author (this is the default)\n - `committer`, commits are grouped by committer\n+ - any other value, the `` is interpreted as a case-insensitive\n+ commit message trailer (see linkgit:git-interpret-trailers[1]). For\n+ example, if your project uses `Reviewed-by` trailers, you might want\n+ to see who has been reviewing with\n+ `git shortlog -ns --ident=reviewed-by`.\n++\n+Note that commits that do not include the trailer will not be counted.\n+Likewise, commits with multiple trailers (e.g., multiple signoffs) may\n+count more than once.\n \n -w[[,[,]]]::\n \tLinewrap the output by wrapping each line at `width`. The first\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 39da2d4..f774c84 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -8,6 +8,7 @@\n #include \"mailmap.h\"\n #include \"shortlog.h\"\n #include \"parse-options.h\"\n+#include \"trailer.h\"\n \n static char const * const shortlog_usage[] = {\n \tN_(\"git shortlog [] [] [[--] [...]]\"),\n@@ -126,6 +127,8 @@ static void read_from_stdin(struct shortlog *log)\n \tcase SHORTLOG_IDENT_COMMITTER:\n \t\tident_header = \"Commit: \";\n \t\tbreak;\n+\tcase SHORTLOG_IDENT_TRAILER:\n+\t\tdie(_(\"sorry, using a trailer --ident with stdin is not supported\"));\n \t}\n \n \twhile (strbuf_getline(&ident, stdin, '\\n') != EOF) {\n@@ -149,6 +152,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tstruct strbuf ident = STRBUF_INIT;\n \tstruct strbuf oneline = STRBUF_INIT;\n \tstruct pretty_print_context ctx = {0};\n+\tchar *oneline_str;\n \n \tctx.fmt = CMIT_FMT_USERFORMAT;\n \tctx.abbrev = log->abbrev;\n@@ -174,6 +178,12 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\t\treturn;\n \t\t}\n \t\tbreak;\n+\tcase SHORTLOG_IDENT_TRAILER:\n+\t\t/*\n+\t\t * We might have multiple matches, so deal with it in the loop\n+\t\t * below.\n+\t\t */\n+\t\tbreak;\n \t}\n \n \tif (!log->summary) {\n@@ -183,7 +193,26 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\t\tformat_commit_message(commit, \"%s\", &oneline, &ctx);\n \t}\n \n-\tinsert_one_record(log, ident.buf, oneline.len ? oneline.buf : \"\");\n+\toneline_str = oneline.len ? oneline.buf : \"\";\n+\tif (log->ident_type != SHORTLOG_IDENT_TRAILER)\n+\t\tinsert_one_record(log, ident.buf, oneline_str);\n+\telse {\n+\t\tstruct strbuf msg = STRBUF_INIT;\n+\t\tstruct trailer_parse_context tp;\n+\t\tint i;\n+\n+\t\tformat_commit_message(commit, \"%B\", &msg, &ctx);\n+\t\ttrailer_parse_init(&tp, &msg);\n+\t\tfor (i = tp.start; i < tp.end; i++) {\n+\t\t\tconst char *v = trailer_parse_match(&tp, i, log->trailer);\n+\t\t\tif (!v)\n+\t\t\t\tcontinue;\n+\t\t\tinsert_one_record(log, v, oneline_str);\n+\t\t}\n+\t\ttrailer_parse_clear(&tp);\n+\t\tstrbuf_release(&msg);\n+\t}\n+\n \tstrbuf_release(&ident);\n \tstrbuf_release(&oneline);\n }\n@@ -253,8 +282,11 @@ static int parse_ident_option(const struct option *opt, const char *arg, int uns\n \t\tlog->ident_type = SHORTLOG_IDENT_AUTHOR;\n \telse if (!strcasecmp(arg, \"committer\"))\n \t\tlog->ident_type = SHORTLOG_IDENT_COMMITTER;\n-\telse\n-\t\tdie(\"unknown ident type: %s\", arg);\n+\telse {\n+\t\tlog->ident_type = SHORTLOG_IDENT_TRAILER;\n+\t\tfree(log->trailer);\n+\t\tlog->trailer = xstrdup(arg);\n+\t}\n \n \treturn 0;\n }\ndiff --git a/shortlog.h b/shortlog.h\nindex a365620..718b49d 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -16,8 +16,10 @@ struct shortlog {\n \n \tenum {\n \t\tSHORTLOG_IDENT_AUTHOR = 0,\n-\t\tSHORTLOG_IDENT_COMMITTER\n+\t\tSHORTLOG_IDENT_COMMITTER,\n+\t\tSHORTLOG_IDENT_TRAILER,\n \t} ident_type;\n+\tchar *trailer;\n \n \tchar *common_repo_prefix;\n \tint email;\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 867a7ae..9c00ccb 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -207,4 +207,13 @@ test_expect_success 'shortlog --ident=committer (external)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shortlog --ident=signed-off-by' '\n+\tgit commit --allow-empty -m foo -s &&\n+\tcat >expect <<-\\EOF &&\n+\t 1\tC O Mitter\n+\tEOF\n+\tgit shortlog -ns --ident=signed-off-by HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275108","messageId":"20151229075013.GA9191@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20151229073832.GN8842@sigill.intra.peff.net","subject":"Re: [PATCH 14/14] shortlog: match commit trailers with --ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T07:50:13Z","receivedAt":"2015-12-29T07:50:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A few comments on my own patch...\n\nOn Tue, Dec 29, 2015 at 02:38:32AM -0500, Jeff King wrote:\n\n> If a project uses commit trailers, this patch lets you use\n> shortlog to see who is performing each action. For example,\n> running:\n> \n> git shortlog -ns --ident=reviewed-by\n\nSo obviously you can do something similar by piping git-log into grep.\nWhat I think this buys us is the ability to use the trailer-parsing code\nrather than hacking together a regex. And it lets you do stuff like\nthis:\n\n> in git.git shows who has reviewed. You can even use a custom\n> format to see things like who has helped whom:\n> \n> git shortlog --format=\"...helped %an (%ad)\" --ident=helped-by\n\nWhich relies on correlating trailers and their matching commits.\n\nSince there may be multiple entries per commit, this always ends up as:\n\n Helper Ident (1):\n ...helped Author Ident\n\t ...helped Other Author\n\t etc\n\nIt would be neat to be able to format it so that each line was a full\nrecord (which would make further stats easier). We'd need a new\nformatting option for that.\n\n> diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\n> index a89a01e..16080c4 100644\n> --- a/Documentation/git-shortlog.txt\n> +++ b/Documentation/git-shortlog.txt\n> @@ -54,6 +54,15 @@ OPTIONS\n> +\n> - `author`, commits are grouped by author (this is the default)\n> - `committer`, commits are grouped by committer\n> + - any other value, the `` is interpreted as a case-insensitive\n> + commit message trailer (see linkgit:git-interpret-trailers[1]). For\n> + example, if your project uses `Reviewed-by` trailers, you might want\n> + to see who has been reviewing with\n> + `git shortlog -ns --ident=reviewed-by`.\n\nThis doesn't leave us many syntactic \"outs\" for adding new ident types\nin the future (nor can you match a trailer called \"Author\"). I guess we\ncould call this \"--ident=trailer:reviewed-by\" to be more precise, but\nit's more annoying to type. Perhaps we could consider that the\n\"official\" syntax, and DWIM \"--ident=foo\" to \"--ident=trailer:foo\", with\nthe caveat that it may change in the future. I dunno.\n\n> ++\n> +Note that commits that do not include the trailer will not be counted.\n> +Likewise, commits with multiple trailers (e.g., multiple signoffs) may\n> +count more than once.\n\nI think counting multiples is the only sensible thing here. If you have\ntwo helped-by fields, for example it would make sense to credit each\nhelper. For S-o-b, where the same person may sign-off twice, it does\nmean you get credited twice for a single commit. I'm not sure if that's\na feature or not. :)\n\n-Peff\n"},{"id":"275230","messageId":"CAPig+cRgK5Ey8WNDLLOnAhR+xh6NEHk-hGhGccj4SkZO-RV_-Q@mail.gmail.com","threadId":"41091","inReplyTo":"20151229072048.GB8842@sigill.intra.peff.net","subject":"Re: [PATCH 02/14] log: refactor add_header to drop some magic numbers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-31T06:21:42Z","receivedAt":"2015-12-31T06:21:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 29, 2015 at 2:20 AM, Jeff King wrote:\n> We want to chomp newlines off the end of the \"value\" string.\n> But because it's const, we must track its length rather than\n> writing a NUL. This leads to us having to tweak that length\n> later, to account for moving the pointer forward.\n>\n> Since we are about to create a copy of it anyway, let's just\n> wait and chomp at the end.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/builtin/log.c b/builtin/log.c\n> @@ -675,21 +675,18 @@ static struct string_list extra_cc;\n> static void add_header(const char *value)\n> {\n> struct string_list_item *item;\n> - int len = strlen(value);\n> - while (len && value[len - 1] == '\\n')\n> - len--;\n> + size_t len;\n>\n> - if (!strncasecmp(value, \"to: \", 4)) {\n> + if (!strncasecmp(value, \"to: \", 4))\n> item = string_list_append(&extra_to, value + 4);\n> - len -= 4;\n> - } else if (!strncasecmp(value, \"cc: \", 4)) {\n> + else if (!strncasecmp(value, \"cc: \", 4))\n> item = string_list_append(&extra_cc, value + 4);\n> - len -= 4;\n> - } else {\n> + else\n> item = string_list_append(&extra_hdr, value);\n> - }\n>\n> - item->string[len] = '\\0';\n> + len = strlen(item->string);\n> + while (len && item->string[len - 1] == '\\n')\n> + item->string[--len] = '\\0';\n\nNot a strong objection, but this implementation may make the reader\nwonder why NUL needs to be assigned to all \"stripped\" characters. I'd\nhave expected to see:\n\n len = strlen(item->string);\n while (len && item->string[len - 1] == '\\n')\n len--;\n item->string[len] = '\\0';\n\nwhich indicates clearly that this is a simple truncation rather than\nsome odd NUL-fill operation, and is slightly more easy to reason about\nsince it doesn't involve a pre-decrement as an array subscript.\n\n> }\n>\n> #define THREAD_SHALLOW 1\n> --\n> 2.7.0.rc3.367.g09631da\n"},{"id":"275231","messageId":"CAPig+cTF_HF-xoiV2osEi6RnGBKQTmkvmUcUn1GbYHVP15_hDA@mail.gmail.com","threadId":"41091","inReplyTo":"20151229072204.GC8842@sigill.intra.peff.net","subject":"Re: [PATCH 03/14] strutil: add skip_prefix_icase","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-31T06:40:46Z","receivedAt":"2015-12-31T06:40:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 29, 2015 at 2:22 AM, Jeff King wrote:\n> Some sites that otherwise would use skip_prefix cannot do\n> so, because it has no way to do case-insensitive\n> comparisons. Such sites usually get around this by using\n> strncasecmp, at the cost of having to use magic numbers.\n> We can help them by providing a case-insensitive version of\n> skip_prefix.\n>\n> Unfortunately, we don't share any code with the original\n> skip_prefix. Since this is performance-sensitive code, we\n> would not want to introduce an extra \"do we are about case?\"\n> conditional into the middle of the loop. We could instead\n> use macros or another technique to generate the\n> almost-identical implementations, but the function simply\n> isn't long enough to merit that confusing boilerplate.\n>\n> To show off the new function, we convert a simple case in\n> log's add_header function.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/builtin/log.c b/builtin/log.c\n> @@ -677,10 +677,10 @@ static void add_header(const char *value)\n> - if (!strncasecmp(value, \"to: \", 4))\n> - item = string_list_append(&extra_to, value + 4);\n> - else if (!strncasecmp(value, \"cc: \", 4))\n> - item = string_list_append(&extra_cc, value + 4);\n> + if (skip_prefix_icase(value, \"to: \", &value))\n> + item = string_list_append(&extra_to, value);\n> + else if (skip_prefix_icase(value, \"cc: \", &value))\n> + item = string_list_append(&extra_cc, value);\n\nIs it worth holding this patch, with its introduction of\nskip_prefix_icase(), hostage to the unrelated change in the previous\npatch (2/14) which touches the same bit of code? Would it make sense\nto split this change out?\n\n> else\n> item = string_list_append(&extra_hdr, value);\n>\n> diff --git a/strutil.h b/strutil.h\n> @@ -32,6 +32,21 @@ static inline int skip_prefix(const char *str, const char *prefix,\n> /*\n> + * Identical to skip_prefix, but compare characters case-insensitively.\n> + */\n> +static inline int skip_prefix_icase(const char *str, const char *prefix,\n> + const char **out)\n> +{\n> + do {\n> + if (!*prefix) {\n> + *out = str;\n> + return 1;\n> + }\n> + } while (tolower(*str++) == tolower(*prefix++));\n\nI wondered initially if we should be concerned about invoking\ntolower() with an expression with side-effects since some older and/or\nnon-compliant libraries implement it as a macro which doesn't ensure\nthat the argument is evaluated only once. However, it seems that Git\nalready uses tolower(*p++) in a few other places, so I guess we're not\nworrying about those broken implementations.\n\n> + return 0;\n> +}\n> +\n> +/*\n"},{"id":"275232","messageId":"CAPig+cTxRbASyLhQH_WZDLX_RTY4t=F-QJeo5Uc5zWUxF1jvZw@mail.gmail.com","threadId":"41091","inReplyTo":"20151229072746.GD8842@sigill.intra.peff.net","subject":"Re: [PATCH 04/14] shortlog: use skip_prefix_icase to parse \"Author\" lines","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-31T06:47:21Z","receivedAt":"2015-12-31T06:47:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 29, 2015 at 2:27 AM, Jeff King wrote:\n> Because we must match both \"Author\" and \"author\" here, we\n> could not use skip_prefix, and had to hand-code a partial\n> case-insensitive match. Now that we have skip_prefix_case,\n\ns/skip_prefix_case/skip_prefix_icase/\n\n> we can use it. This is technically more liberal in what it\n> matches (e.g., it will match AUTHOR), but in this particular\n> case that that's OK (we are matching git-log output, so we\n\ns/that that's/that's/\n\n> expect arbitrary data like commit headers to be indented).\n>\n> In addition to being easier to read, this will make the code\n> easier to adapt for matching other lines.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> @@ -94,8 +94,8 @@ static void read_from_stdin(struct shortlog *log)\n> char author[1024], oneline[1024];\n>\n> while (fgets(author, sizeof(author), stdin) != NULL) {\n> - if (!(author[0] == 'A' || author[0] == 'a') ||\n> - !starts_with(author + 1, \"uthor: \"))\n> + const char *v;\n> + if (!skip_prefix_icase(author, \"Author: \", &v))\n> continue;\n> while (fgets(oneline, sizeof(oneline), stdin) &&\n> oneline[0] != '\\n')\n> @@ -103,7 +103,7 @@ static void read_from_stdin(struct shortlog *log)\n> while (fgets(oneline, sizeof(oneline), stdin) &&\n> oneline[0] == '\\n')\n> ; /* discard blanks */\n> - insert_one_record(log, author + 8, oneline);\n> + insert_one_record(log, v, oneline);\n> }\n> }\n>\n> --\n> 2.7.0.rc3.367.g09631da\n"},{"id":"275249","messageId":"20160101084206.GB20715@sigill.intra.peff.net","threadId":"41091","inReplyTo":"CAPig+cRgK5Ey8WNDLLOnAhR+xh6NEHk-hGhGccj4SkZO-RV_-Q@mail.gmail.com","subject":"Re: [PATCH 02/14] log: refactor add_header to drop some magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-01T08:42:06Z","receivedAt":"2016-01-01T08:42:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 31, 2015 at 01:21:42AM -0500, Eric Sunshine wrote:\n\n> > - item->string[len] = '\\0';\n> > + len = strlen(item->string);\n> > + while (len && item->string[len - 1] == '\\n')\n> > + item->string[--len] = '\\0';\n> \n> Not a strong objection, but this implementation may make the reader\n> wonder why NUL needs to be assigned to all \"stripped\" characters. I'd\n> have expected to see:\n> \n> len = strlen(item->string);\n> while (len && item->string[len - 1] == '\\n')\n> len--;\n> item->string[len] = '\\0';\n> \n> which indicates clearly that this is a simple truncation rather than\n> some odd NUL-fill operation, and is slightly more easy to reason about\n> since it doesn't involve a pre-decrement as an array subscript.\n\nHmm. I consider the \"write NULs backward\" strategy to be pretty\nidiomatic (you can find several similar ones grepping for `\\[--` in the\ngit codebase). But that may just be me (I didn't look, but it's possible\nI wrote the other ones, too :) ).\n\nI don't have a strong preference, though. What you've written is quite\nreadable.\n\n-Peff\n"},{"id":"275250","messageId":"20160101084621.GA22563@sigill.intra.peff.net","threadId":"41091","inReplyTo":"20160101084206.GB20715@sigill.intra.peff.net","subject":"Re: [PATCH 02/14] log: refactor add_header to drop some magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-01T08:46:21Z","receivedAt":"2016-01-01T08:46:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 01, 2016 at 03:42:06AM -0500, Jeff King wrote:\n\n> On Thu, Dec 31, 2015 at 01:21:42AM -0500, Eric Sunshine wrote:\n> \n> > > - item->string[len] = '\\0';\n> > > + len = strlen(item->string);\n> > > + while (len && item->string[len - 1] == '\\n')\n> > > + item->string[--len] = '\\0';\n> > \n> > Not a strong objection, but this implementation may make the reader\n> > wonder why NUL needs to be assigned to all \"stripped\" characters. I'd\n> > have expected to see:\n> > \n> > len = strlen(item->string);\n> > while (len && item->string[len - 1] == '\\n')\n> > len--;\n> > item->string[len] = '\\0';\n> > \n> > which indicates clearly that this is a simple truncation rather than\n> > some odd NUL-fill operation, and is slightly more easy to reason about\n> > since it doesn't involve a pre-decrement as an array subscript.\n> \n> Hmm. I consider the \"write NULs backward\" strategy to be pretty\n> idiomatic (you can find several similar ones grepping for `\\[--` in the\n> git codebase). But that may just be me (I didn't look, but it's possible\n> I wrote the other ones, too :) ).\n> \n> I don't have a strong preference, though. What you've written is quite\n> readable.\n\nActually, looking at the diff, the original already has your final line\n(it _has_ to do the search and NUL-write separately because it does the\nformer before allocating the copy). So between the two options, yours\nmakes the diff more obvious, too, which is a good thing. I'll take your\nsuggestion.\n\n-Peff\n"},{"id":"275251","messageId":"20160101085045.GC20715@sigill.intra.peff.net","threadId":"41091","inReplyTo":"CAPig+cTF_HF-xoiV2osEi6RnGBKQTmkvmUcUn1GbYHVP15_hDA@mail.gmail.com","subject":"Re: [PATCH 03/14] strutil: add skip_prefix_icase","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-01T08:50:46Z","receivedAt":"2016-01-01T08:50:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 31, 2015 at 01:40:46AM -0500, Eric Sunshine wrote:\n\n> > diff --git a/builtin/log.c b/builtin/log.c\n> > @@ -677,10 +677,10 @@ static void add_header(const char *value)\n> > - if (!strncasecmp(value, \"to: \", 4))\n> > - item = string_list_append(&extra_to, value + 4);\n> > - else if (!strncasecmp(value, \"cc: \", 4))\n> > - item = string_list_append(&extra_cc, value + 4);\n> > + if (skip_prefix_icase(value, \"to: \", &value))\n> > + item = string_list_append(&extra_to, value);\n> > + else if (skip_prefix_icase(value, \"cc: \", &value))\n> > + item = string_list_append(&extra_cc, value);\n> \n> Is it worth holding this patch, with its introduction of\n> skip_prefix_icase(), hostage to the unrelated change in the previous\n> patch (2/14) which touches the same bit of code? Would it make sense\n> to split this change out?\n\nI assumed that this was at least as contentious as 2/14, so it wouldn't\nbe a problem. I'm inclined to leave it, unless it looks like we'll scrap\n2/14.\n\n> > +static inline int skip_prefix_icase(const char *str, const char *prefix,\n> > + const char **out)\n> > +{\n> > + do {\n> > + if (!*prefix) {\n> > + *out = str;\n> > + return 1;\n> > + }\n> > + } while (tolower(*str++) == tolower(*prefix++));\n> \n> I wondered initially if we should be concerned about invoking\n> tolower() with an expression with side-effects since some older and/or\n> non-compliant libraries implement it as a macro which doesn't ensure\n> that the argument is evaluated only once. However, it seems that Git\n> already uses tolower(*p++) in a few other places, so I guess we're not\n> worrying about those broken implementations.\n\nRight. There are a few other gotchas with tolower(), and we work around\nit by implementing the ctype functions ourselves (even on non-broken\nplatforms). It's one of the oldest bits of git-compat-util.h.\n\n-Peff\n"},{"id":"275252","messageId":"20160101085317.GD20715@sigill.intra.peff.net","threadId":"41091","inReplyTo":"CAPig+cTxRbASyLhQH_WZDLX_RTY4t=F-QJeo5Uc5zWUxF1jvZw@mail.gmail.com","subject":"Re: [PATCH 04/14] shortlog: use skip_prefix_icase to parse \"Author\" lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-01T08:53:17Z","receivedAt":"2016-01-01T08:53:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 31, 2015 at 01:47:21AM -0500, Eric Sunshine wrote:\n\n> On Tue, Dec 29, 2015 at 2:27 AM, Jeff King wrote:\n> > Because we must match both \"Author\" and \"author\" here, we\n> > could not use skip_prefix, and had to hand-code a partial\n> > case-insensitive match. Now that we have skip_prefix_case,\n> \n> s/skip_prefix_case/skip_prefix_icase/\n\nThanks. I originally called it the former, and thought I had managed to\nfix up all of the references, but clearly didn't run \"git log --grep\".\n\n> > we can use it. This is technically more liberal in what it\n> > matches (e.g., it will match AUTHOR), but in this particular\n> > case that that's OK (we are matching git-log output, so we\n> \n> s/that that's/that's/\n\nFixed, thanks.\n\n-Peff\n"},{"id":"275290","messageId":"CAPig+cRxBHp9hRBbFPYtEdytnmGdVYVMfJLKzjT6qoQ8EOSxQw@mail.gmail.com","threadId":"41091","inReplyTo":"20151229072953.GF8842@sigill.intra.peff.net","subject":"Re: [PATCH 06/14] shortlog: replace hand-parsing of author with pretty-printer","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-04T09:43:23Z","receivedAt":"2016-01-04T09:43:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 29, 2015 at 2:29 AM, Jeff King wrote:\n> When gathering the author and oneline subject for each\n> commit, we hand-parse the commit headers to find the\n> \"author\" line, and then continue past to the blank line at\n> the end of the header.\n>\n> We can replace this tricky hand-parsing by simply asking the\n> pretty-printer for the relevant items. This also decouples\n> the author and oneline parsing, opening up some new\n> optimizations in further commits.\n>\n> One reason to avoid the pretty-printer is that it might be\n> less efficient than hand-parsing. However, I measured no\n> slowdown at all running \"git shortlog -ns HEAD\" on\n> linux.git.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> @@ -112,45 +112,32 @@ static void read_from_stdin(struct shortlog *log)\n> void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n> {\n> - const char *author = NULL, *buffer;\n> - struct strbuf buf = STRBUF_INIT;\n> - struct strbuf ufbuf = STRBUF_INIT;\n> -\n> - pp_commit_easy(CMIT_FMT_RAW, commit, &buf);\n> - buffer = buf.buf;\n> - while (*buffer && *buffer != '\\n') {\n> - const char *eol = strchr(buffer, '\\n');\n> -\n> - if (eol == NULL)\n> - eol = buffer + strlen(buffer);\n> - else\n> - eol++;\n> -\n> - if (starts_with(buffer, \"author \"))\n> - author = buffer + 7;\n> - buffer = eol;\n> - }\n> - if (!author) {\n> + struct strbuf author = STRBUF_INIT;\n> + struct strbuf oneline = STRBUF_INIT;\n> + struct pretty_print_context ctx = {0};\n> +\n> + ctx.fmt = CMIT_FMT_USERFORMAT;\n> + ctx.abbrev = log->abbrev;\n> + ctx.subject = \"\";\n> + ctx.after_subject = \"\";\n> + ctx.date_mode.type = DATE_NORMAL;\n> + ctx.output_encoding = get_log_output_encoding();\n> +\n> + format_commit_message(commit, \"%an <%ae>\", &author, &ctx);\n> + if (author.len <= 3) {\n\nI suppose magic number 3 is the space, '<', and '>'...\n\n> warning(_(\"Missing author: %s\"),\n> oid_to_hex(&commit->object.oid));\n> return;\n\nIs this leaking strbuf 'author'?\n\n> }\n> - if (log->user_format) {\n> - struct pretty_print_context ctx = {0};\n> - ctx.fmt = CMIT_FMT_USERFORMAT;\n> - ctx.abbrev = log->abbrev;\n> - ctx.subject = \"\";\n> - ctx.after_subject = \"\";\n> - ctx.date_mode.type = DATE_NORMAL;\n> - ctx.output_encoding = get_log_output_encoding();\n> - pretty_print_commit(&ctx, commit, &ufbuf);\n> - buffer = ufbuf.buf;\n> - } else if (*buffer) {\n> - buffer++;\n> - }\n> - insert_one_record(log, author, !*buffer ? \"\" : buffer);\n> - strbuf_release(&ufbuf);\n> - strbuf_release(&buf);\n> +\n> + if (log->user_format)\n> + pretty_print_commit(&ctx, commit, &oneline);\n> + else\n> + format_commit_message(commit, \"%s\", &oneline, &ctx);\n> +\n> + insert_one_record(log, author.buf, oneline.len ? oneline.buf : \"\");\n> + strbuf_release(&author);\n> + strbuf_release(&oneline);\n> }\n"},{"id":"275292","messageId":"CAPig+cT1pNXt2U1bd-KVapiUD=-6TQ8KRBNC8y+0wLAtuqUhQQ@mail.gmail.com","threadId":"41091","inReplyTo":"20151229073515.GK8842@sigill.intra.peff.net","subject":"Re: [PATCH 11/14] shortlog: allow grouping by committer ident","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-04T09:44:00Z","receivedAt":"2016-01-04T09:44:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 29, 2015 at 2:35 AM, Jeff King wrote:\n> Shortlog always groups commits by the author field. It can\n> be interesting to group by other fields, as well. The\n> obvious other identity in each commit is the committer\n> field. This might be interesting if your project follows a\n> workflow where committers and authors are not the same and\n> where there is not simply one maintainer picking up all of\n> the patches.\n>\n> For instance, you can use this in git.git to see interim and\n> subsystem maintainers. Likewise, you can see in linux.git\n> the patches picked up by lieutenants and merged into Linus's\n> tree.\n>\n> This patch also provides some of the necessary\n> infrastructure for adding more ident types (e.g., from\n> trailers) in the future.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\n> @@ -47,6 +47,14 @@ OPTIONS\n> +--ident=::\n\nShould this be called --group-by?\n\n> + By default, `shortlog` collects and collates author identities;\n> + using `--ident` will collect other types of identity. If\n> + `` is:\n> ++\n> + - `author`, commits are grouped by author (this is the default)\n> + - `committer`, commits are grouped by committer\n\nThere is a bit of redundancy here. I wonder if it could be described\nmore succinctly. For instance, instead of all the above, perhaps:\n\n Group commits by ``, which can be one of:\n\n - `author` (default)\n - `committer`\n\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index d7eb0cb..39da2d4 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -147,11 +157,23 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n> - format_commit_message(commit, \"%an <%ae>\", &ident, &ctx);\n> - if (ident.len <= 3) {\n> - warning(_(\"Missing author: %s\"),\n> - oid_to_hex(&commit->object.oid));\n> - return;\n> + switch (log->ident_type) {\n> + case SHORTLOG_IDENT_AUTHOR:\n> + format_commit_message(commit, \"%an <%ae>\", &ident, &ctx);\n> + if (ident.len <= 3) {\n> + warning(_(\"Missing author: %s\"),\n> + oid_to_hex(&commit->object.oid));\n> + return;\n> + }\n> + break;\n> + case SHORTLOG_IDENT_COMMITTER:\n> + format_commit_message(commit, \"%cn <%ce>\", &ident, &ctx);\n> + if (ident.len <= 3) {\n> + warning(_(\"Missing committer: %s\"),\n> + oid_to_hex(&commit->object.oid));\n> + return;\n\nIs this leaking strbuf 'ident'?\n\n(Ditto for the \"author\" case as mentioned already in the patch 6/14 review.)\n\n> + }\n> + break;\n> }\n"},{"id":"275291","messageId":"CAPig+cTqfd4QezR09QwUb_7viML_4LB5ChNF_eXVCsbGHLaDMQ@mail.gmail.com","threadId":"41091","inReplyTo":"20151229075013.GA9191@sigill.intra.peff.net","subject":"Re: [PATCH 14/14] shortlog: match commit trailers with --ident","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-04T09:44:15Z","receivedAt":"2016-01-04T09:44:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 29, 2015 at 2:50 AM, Jeff King wrote:\n> This doesn't leave us many syntactic \"outs\" for adding new ident types\n> in the future (nor can you match a trailer called \"Author\"). I guess we\n> could call this \"--ident=trailer:reviewed-by\" to be more precise, but\n> it's more annoying to type. Perhaps we could consider that the\n> \"official\" syntax, and DWIM \"--ident=foo\" to \"--ident=trailer:foo\", with\n> the caveat that it may change in the future. I dunno.\n\nWould anyone ever want to be able to specify multiple trailers for\ncounting? That is:\n\n --ident=trailer:helped-by,reviewed-by\n\nor\n\n --ident=trailer:helped-by --ident=trailer:reviewed-by\n"},{"id":"275294","messageId":"20160104101712.GA32086@sigill.intra.peff.net","threadId":"41091","inReplyTo":"CAPig+cRxBHp9hRBbFPYtEdytnmGdVYVMfJLKzjT6qoQ8EOSxQw@mail.gmail.com","subject":"Re: [PATCH 06/14] shortlog: replace hand-parsing of author with pretty-printer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-04T10:17:13Z","receivedAt":"2016-01-04T10:17:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 04, 2016 at 04:43:23AM -0500, Eric Sunshine wrote:\n\n> > + format_commit_message(commit, \"%an <%ae>\", &author, &ctx);\n> > + if (author.len <= 3) {\n> \n> I suppose magic number 3 is the space, '<', and '>'...\n\nYeah. That's worthy of a comment, I think (see below).\n\nI don't think we will detect a case with just a missing name _or_ email,\nbut neither did the original.\n\n> > warning(_(\"Missing author: %s\"),\n> > oid_to_hex(&commit->object.oid));\n> > return;\n> \n> Is this leaking strbuf 'author'?\n\nYes. Looks like the original leaked \"buf\" in the same way.\n\nRather than repeat the strbuf_release, I think we can introduce a goto,\nlike:\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 10c92fa..4180a2d 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -124,10 +124,11 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tctx.output_encoding = get_log_output_encoding();\n \n \tformat_commit_message(commit, \"%an <%ae>\", &author, &ctx);\n+\t/* we can detect a total failure only by seeing \" <>\" in the output */\n \tif (author.len <= 3) {\n \t\twarning(_(\"Missing author: %s\"),\n \t\t oid_to_hex(&commit->object.oid));\n-\t\treturn;\n+\t\tgoto out;\n \t}\n \n \tif (log->user_format)\n@@ -136,6 +137,8 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\tformat_commit_message(commit, \"%s\", &oneline, &ctx);\n \n \tinsert_one_record(log, author.buf, oneline.len ? oneline.buf : \"\");\n+\n+out:\n \tstrbuf_release(&author);\n \tstrbuf_release(&oneline);\n }\n\nThat should make it easier to handle the cases added later in the\nseries, too.\n\n-Peff\n"},{"id":"275295","messageId":"20160104102300.GB32086@sigill.intra.peff.net","threadId":"41091","inReplyTo":"CAPig+cT1pNXt2U1bd-KVapiUD=-6TQ8KRBNC8y+0wLAtuqUhQQ@mail.gmail.com","subject":"Re: [PATCH 11/14] shortlog: allow grouping by committer ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-04T10:23:00Z","receivedAt":"2016-01-04T10:23:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 04, 2016 at 04:44:00AM -0500, Eric Sunshine wrote:\n\n> > diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\n> > @@ -47,6 +47,14 @@ OPTIONS\n> > +--ident=::\n> \n> Should this be called --group-by?\n\nYeah, that may be a more sensible name. For committer/author, I think\n--ident is accurate. But for a trailer, we technically do not know what\nis in it. I expect people would use this primarily with ident-based\ntrailers (like \"reviewed-by\" or whatever), but there's no reason you\ncould not collate references to a bug id, or something.\n\n> > + By default, `shortlog` collects and collates author identities;\n> > + using `--ident` will collect other types of identity. If\n> > + `` is:\n> > ++\n> > + - `author`, commits are grouped by author (this is the default)\n> > + - `committer`, commits are grouped by committer\n> \n> There is a bit of redundancy here. I wonder if it could be described\n> more succinctly. For instance, instead of all the above, perhaps:\n> \n> Group commits by ``, which can be one of:\n> \n> - `author` (default)\n> - `committer`\n\nI avoided that kind of parallel structure because I knew that later in\nthe series I would be adding a new bullet that would need more\nexplanation. I'm still pondering `trailer:` as the syntax, which\nwould involve reworking this text. I'll keep it in mind when I do.\n\n> > + case SHORTLOG_IDENT_COMMITTER:\n> > + format_commit_message(commit, \"%cn <%ce>\", &ident, &ctx);\n> > + if (ident.len <= 3) {\n> > + warning(_(\"Missing committer: %s\"),\n> > + oid_to_hex(&commit->object.oid));\n> > + return;\n> \n> Is this leaking strbuf 'ident'?\n> \n> (Ditto for the \"author\" case as mentioned already in the patch 6/14 review.)\n\nYep. :) I think these can become \"goto out\" per my previous fixup.\n\n-Peff\n"},{"id":"275296","messageId":"20160104103106.GC32086@sigill.intra.peff.net","threadId":"41091","inReplyTo":"CAPig+cTqfd4QezR09QwUb_7viML_4LB5ChNF_eXVCsbGHLaDMQ@mail.gmail.com","subject":"Re: [PATCH 14/14] shortlog: match commit trailers with --ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-04T10:31:06Z","receivedAt":"2016-01-04T10:31:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 04, 2016 at 04:44:15AM -0500, Eric Sunshine wrote:\n\n> On Tue, Dec 29, 2015 at 2:50 AM, Jeff King wrote:\n> > This doesn't leave us many syntactic \"outs\" for adding new ident types\n> > in the future (nor can you match a trailer called \"Author\"). I guess we\n> > could call this \"--ident=trailer:reviewed-by\" to be more precise, but\n> > it's more annoying to type. Perhaps we could consider that the\n> > \"official\" syntax, and DWIM \"--ident=foo\" to \"--ident=trailer:foo\", with\n> > the caveat that it may change in the future. I dunno.\n> \n> Would anyone ever want to be able to specify multiple trailers for\n> counting? That is:\n> \n> --ident=trailer:helped-by,reviewed-by\n> \n> or\n> \n> --ident=trailer:helped-by --ident=trailer:reviewed-by\n\nGenerally I'd prefer the latter, as it builds on the existing option\nparsing rather than inventing a new syntax. But it would also allow\nmixing trailer and non-trailer items, like:\n\n git shortlog --ident=trailer:helped-by --ident=author\n\nI guess that is well-formed, though I would expect it to be rare. I\ndunno.\n\nI'd rather punt on handling multiple types in this initial attempt, and\nlet somebody implement that later if they want. I suppose if we did a\nrelease with one and then the other, it would technically change the\nmeaning of:\n\n git shortlog --ident=foo --ident=bar\n\nfrom \"just bar, overriding foo\" to \"both foo and bar\". I think I could\nlive with that, though.\n\n-Peff\n"}]}