{"thread":{"id":"36786","subject":"[RFC PATCH] git log: support \"auto\" decorations","startedAt":"2014-05-29T22:31:58Z","lastAt":"2014-05-30T21:13:05Z","messageCount":13,"participants":["Linus Torvalds","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"242982","messageId":"alpine.LFD.2.11.1405291523520.8270@i7.linux-foundation.org","threadId":"36786","inReplyTo":null,"subject":"[RFC PATCH] git log: support \"auto\" decorations","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-05-29T22:31:58Z","receivedAt":"2014-05-29T22:31:58Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds <torvalds@linux-foundation.org>\nDate: Thu, 29 May 2014 15:19:40 -0700\nSubject: [RFC PATCH] git log: support \"auto\" decorations\n\nThis works kind of like \"--color=auto\" - add decorations for interactive\nuse, but do not change defaults when scripting or when piping the output\nto anything but a terminal.\n\nYou can use either\n\n    [log]\n         decorate=auto\n\nin the git config files, or the \"--decorate=auto\" command line option to\nchoose this behavior.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nI actually like seeing decorations by default, but I do *not* think our \ncurrent \"log.decorate\" options make sense, since they will change any \nrandom use of \"git log\" to have decorations. I much prefer the \n\"ui.color=auto\" behavior that we have for coloration. This is a trivial \npatch that tries to approximate that.\n\nIt's marked with RFC because\n\n (a) that \"isatty(1) || pager_in_use()\" test is kind of hacky, maybe we \n     would be better off sharing something with the auto-coloration?\n\n (b) I also think it would be nice to have the equivalent for \n     \"--show-signature\", but there we don't have any preexisting config \n     file option.\n\n (c) maybe somebody would like a way to combine \"auto\" and \"full\", \n     although personally that doesn't seem to strike me as all that useful \n     (would you really want to see the full refname when not scripting it)\n\nbut the patch is certainly simple and seems to work. Comments?\n\n builtin/log.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39e883635279..df6396c9c3d9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -63,6 +63,8 @@ static int parse_decoration_style(const char *var, const char *value)\n \t\treturn DECORATE_FULL_REFS;\n \telse if (!strcmp(value, \"short\"))\n \t\treturn DECORATE_SHORT_REFS;\n+\telse if (!strcmp(value, \"auto\"))\n+\t\treturn (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;\n \treturn -1;\n }\n \n-- \n2.0.0.1.g5beb60c\n"},{"id":"243000","messageId":"20140530015855.GG28683@sigill.intra.peff.net","threadId":"36786","inReplyTo":"alpine.LFD.2.11.1405291523520.8270@i7.linux-foundation.org","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-30T01:58:55Z","receivedAt":"2014-05-30T01:58:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 29, 2014 at 03:31:58PM -0700, Linus Torvalds wrote:\n\n> From: Linus Torvalds <torvalds@linux-foundation.org>\n> Date: Thu, 29 May 2014 15:19:40 -0700\n> Subject: [RFC PATCH] git log: support \"auto\" decorations\n\nI will spare you the usual lecture on having these lines in the message\nbody. ;)\n\n> I actually like seeing decorations by default, but I do *not* think our \n> current \"log.decorate\" options make sense, since they will change any \n> random use of \"git log\" to have decorations. I much prefer the \n> \"ui.color=auto\" behavior that we have for coloration. This is a trivial \n> patch that tries to approximate that.\n\nYeah, I think this makes a lot of sense. I do use log.decorate=true, and\nit is usually not a big deal. However, I think I have run into\nannoyances once or twice when piping it. I'd probably use\nlog.decorate=auto if we had it.\n\n> It's marked with RFC because\n> \n>  (a) that \"isatty(1) || pager_in_use()\" test is kind of hacky, maybe we \n>      would be better off sharing something with the auto-coloration?\n\nThe magic for this is in color.c, want_color() and check_auto_color().\n\nThe color code checks \"pager_use_color\" when the pager is in use, but I\ndo not think that makes any sense here.  It also checks that $TERM is\nnot \"dumb\", but that also does not make sense here.\n\nSo I think your check is fine. It would be nice to share with the color\ncode, but I doubt it will end up any more readable, because of\nconditionally dealing with those two differences.\n\n>  (b) I also think it would be nice to have the equivalent for \n>      \"--show-signature\", but there we don't have any preexisting config \n>      file option.\n\nPotentially yes, though there is a real performance impact for \"log\n--show-signature\" if you actually have a lot of signatures. Even on\nlinux.git, a full \"git log\" is 15s with --show-signature, and 5s\nwithout. Maybe that is acceptable for interactive use (and certainly it\nis not a reason to make it an _option_, if somebody wants to turn it\non).\n\n>  (c) maybe somebody would like a way to combine \"auto\" and \"full\", \n>      although personally that doesn't seem to strike me as all that useful \n>      (would you really want to see the full refname when not scripting it)\n\nYeah, \"full/short\" is really orthogonal to \"true/false/auto\". If we were\nstarting from scratch, I think putting \"full/short\" into\nlog.decorateStyle would make more sense, but it is probably not worth\nchanging now. I agree that \"full auto\" is probably not something useful,\nand we can live without it.\n\n-Peff\n"},{"id":"243004","messageId":"CA+55aFzwy09-i=hpBy-5bYS6eowGzkdcF65cFJpL2qnJvYq85w@mail.gmail.com","threadId":"36786","inReplyTo":"20140530015855.GG28683@sigill.intra.peff.net","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-05-30T04:54:10Z","receivedAt":"2014-05-30T04:54:10Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, May 29, 2014 at 6:58 PM, Jeff King <peff@peff.net> wrote:\n>\n> I will spare you the usual lecture on having these lines in the message\n> body. ;)\n\nWe do it for the kernel because they often get lost otherwise.\nParticularly the date/author.\n\ngit doesn't tend to have the same kind of deep email forwarding\nmodels, and nobody uses quilt to develop git (I hope!) but it's a\nhabit I like to encourage for the kernel, so I use it in general..\n\n> Yeah, I think this makes a lot of sense. I do use log.decorate=true, and\n> it is usually not a big deal. However, I think I have run into\n> annoyances once or twice when piping it. I'd probably use\n> log.decorate=auto if we had it.\n\nI have various scripts to generate releases etc, using \"git log\" and\npiping it to random other stuff. I don't _think_ they care, but quite\nfrankly, I'd rather not even have to think about it.\n\nAlso, I actually find the un-colorized version of the decorations to\nbe distracting. With color (not that I really like the default color\nscheme, but I'm too lazy to set it up to anything else), the\ncolorization ends up making the decorations much easier to visually\nseparate, so I like them there.\n\n>> It's marked with RFC because\n>>\n>>  (a) that \"isatty(1) || pager_in_use()\" test is kind of hacky, maybe we\n>>      would be better off sharing something with the auto-coloration?\n>\n> The magic for this is in color.c, want_color() and check_auto_color().\n\nOh, I know. That's where I stole it from. But the colorization\nactually has very different rules, and looks at TERM etc. So I only\nstole the actual non-color-specific parts of it, but I was wondering\nwhether it might make sense to unify them.\n\n>>  (b) I also think it would be nice to have the equivalent for\n>>      \"--show-signature\", but there we don't have any preexisting config\n>>      file option.\n>\n> Potentially yes, though there is a real performance impact for \"log\n> --show-signature\" if you actually have a lot of signatures. Even on\n> linux.git, a full \"git log\" is 15s with --show-signature, and 5s\n> without. Maybe that is acceptable for interactive use (and certainly it\n> is not a reason to make it an _option_, if somebody wants to turn it\n> on).\n\nYes. This is an example of where the whole \"tty is fundamentally\ndifferent from scripting\" happens. It really is about the whole \"user\nis looking at it\" vs \"scripting\". There is no way in hell that\n\"--show-signature\" should be on by default for anybody sane.\n\nThat said, part of it is just that show-signature is so suboptimal\nperformance-wise, re-parsing the commit buffer for each commit when\n\"show_signature\" is set. That's just crazy, we've already parsed the\ncommit text, we already *could* know if it has a signature or not, and\nskip it if it doesn't. That would require one of the flag bits in the\nobject, though, or something, so it's probably not worth doing.\n\nSure, performance might matter if you end up searching for something\nin the pager after you've done \"git log\", but personally I think I'd\nnever even notice..\n\n          Linus\n"},{"id":"243005","messageId":"20140530065737.GA13591@sigill.intra.peff.net","threadId":"36786","inReplyTo":"CA+55aFzwy09-i=hpBy-5bYS6eowGzkdcF65cFJpL2qnJvYq85w@mail.gmail.com","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-30T06:57:38Z","receivedAt":"2014-05-30T06:57:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 29, 2014 at 09:54:10PM -0700, Linus Torvalds wrote:\n\n> That said, part of it is just that show-signature is so suboptimal\n> performance-wise, re-parsing the commit buffer for each commit when\n> \"show_signature\" is set. That's just crazy, we've already parsed the\n> commit text, we already *could* know if it has a signature or not, and\n> skip it if it doesn't. That would require one of the flag bits in the\n> object, though, or something, so it's probably not worth doing.\n\nWow, it's really quite bad. Not only do we spend time on commits that we\ncould otherwise know do not have signatures, but we actually pull the\nbuffer from disk, even though we generally have it saved as\ncommit->buffer. And we do it twice! Once for the commit signature, and\nonce for the mergetag.\n\nBelow is a fairly straightforward patch to use commit->buffer when we\nhave it. Here are timings on the kernel repo:\n\n  [baseline, no signatures]\n  $ time git log >/dev/null\n  real    0m4.902s\n  user    0m4.784s\n  sys     0m0.120s\n\n  [before]\n  $ time git log --show-signature >/dev/null\n  real    0m14.735s\n  user    0m9.964s\n  sys     0m0.944s\n\n  [after]\n  $ time git log --show-signature >/dev/null\n  real    0m9.981s\n  user    0m5.260s\n  sys     0m0.936s\n\nIf you look at just the user CPU time, you can see we've reclaimed all\nbut 0.5s of the 5.2s wasted seconds. Some of that is presumably going to\nthe extra re-parsing, with a little to the actual gpg verification.\n\nThe wall-clock time improves, too, but we're still 5s slower. A little\nof that goes to system time, but presumably most of the rest of it is\nlatency due to waiting on gpg? Getting rid of that would probably\ninvolve caching the gpg output from run to run. That's not that hard to\ndo, but I don't like the idea of caching security information.\n\n---\n commit.c   | 36 ++++++++++++++++++++++++++++--------\n commit.h   |  2 +-\n log-tree.c |  2 +-\n 3 files changed, 30 insertions(+), 10 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex f479331..529ee50 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1080,14 +1080,34 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)\n \treturn 0;\n }\n \n-int parse_signed_commit(const unsigned char *sha1,\n+static const char *get_commit_buffer(const struct commit *commit,\n+\t\t\t\t     enum object_type *type,\n+\t\t\t\t     unsigned long *size)\n+{\n+\tif (commit->buffer) {\n+\t\t*type = OBJ_COMMIT;\n+\t\t*size = strlen(commit->buffer);\n+\t\treturn commit->buffer;\n+\t}\n+\n+\treturn read_sha1_file(commit->object.sha1, type, size);\n+}\n+\n+static void free_commit_buffer(const char *buffer, const struct commit *commit)\n+{\n+\tif (buffer != commit->buffer)\n+\t\tfree((char *)buffer);\n+}\n+\n+int parse_signed_commit(const struct commit *commit,\n \t\t\tstruct strbuf *payload, struct strbuf *signature)\n {\n+\n \tunsigned long size;\n \tenum object_type type;\n-\tchar *buffer = read_sha1_file(sha1, &type, &size);\n+\tconst char *buffer = get_commit_buffer(commit, &type, &size);\n \tint in_signature, saw_signature = -1;\n-\tchar *line, *tail;\n+\tconst char *line, *tail;\n \n \tif (!buffer || type != OBJ_COMMIT)\n \t\tgoto cleanup;\n@@ -1098,7 +1118,7 @@ int parse_signed_commit(const unsigned char *sha1,\n \tsaw_signature = 0;\n \twhile (line < tail) {\n \t\tconst char *sig = NULL;\n-\t\tchar *next = memchr(line, '\\n', tail - line);\n+\t\tconst char *next = memchr(line, '\\n', tail - line);\n \n \t\tnext = next ? next + 1 : tail;\n \t\tif (in_signature && line[0] == ' ')\n@@ -1120,7 +1140,7 @@ int parse_signed_commit(const unsigned char *sha1,\n \t\tline = next;\n \t}\n  cleanup:\n-\tfree(buffer);\n+\tfree_commit_buffer(buffer, commit);\n \treturn saw_signature;\n }\n \n@@ -1211,7 +1231,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check\n \n \tsigc->result = 'N';\n \n-\tif (parse_signed_commit(commit->object.sha1,\n+\tif (parse_signed_commit(commit,\n \t\t\t\t&payload, &signature) <= 0)\n \t\tgoto out;\n \tstatus = verify_signed_buffer(payload.buf, payload.len,\n@@ -1258,10 +1278,10 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,\n \tstruct commit_extra_header *extra = NULL;\n \tunsigned long size;\n \tenum object_type type;\n-\tchar *buffer = read_sha1_file(commit->object.sha1, &type, &size);\n+\tconst char *buffer = get_commit_buffer(commit, &type, &size);\n \tif (buffer && type == OBJ_COMMIT)\n \t\textra = read_commit_extra_header_lines(buffer, size, exclude);\n-\tfree(buffer);\n+\tfree_commit_buffer(buffer, commit);\n \treturn extra;\n }\n \ndiff --git a/commit.h b/commit.h\nindex a9f177b..a765f0f 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -287,7 +287,7 @@ struct merge_remote_desc {\n  */\n struct commit *get_merge_parent(const char *name);\n \n-extern int parse_signed_commit(const unsigned char *sha1,\n+extern int parse_signed_commit(const struct commit *commit,\n \t\t\t       struct strbuf *message, struct strbuf *signature);\n extern void print_commit_list(struct commit_list *list,\n \t\t\t      const char *format_cur,\ndiff --git a/log-tree.c b/log-tree.c\nindex cf2f86c..6358599 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -376,7 +376,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)\n \tstruct strbuf gpg_output = STRBUF_INIT;\n \tint status;\n \n-\tif (parse_signed_commit(commit->object.sha1, &payload, &signature) <= 0)\n+\tif (parse_signed_commit(commit, &payload, &signature) <= 0)\n \t\tgoto out;\n \n \tstatus = verify_signed_buffer(payload.buf, payload.len,\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"243015","messageId":"xmqqvbsn9pfx.fsf@gitster.dls.corp.google.com","threadId":"36786","inReplyTo":"20140530065737.GA13591@sigill.intra.peff.net","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-30T16:55:14Z","receivedAt":"2014-05-30T16:55:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, May 29, 2014 at 09:54:10PM -0700, Linus Torvalds wrote:\n>\n>> That said, part of it is just that show-signature is so suboptimal\n>> performance-wise, re-parsing the commit buffer for each commit when\n>> \"show_signature\" is set. That's just crazy, we've already parsed the\n>> commit text, we already *could* know if it has a signature or not, and\n>> skip it if it doesn't. That would require one of the flag bits in the\n>> object, though, or something, so it's probably not worth doing.\n>\n> Wow, it's really quite bad. Not only do we spend time on commits that we\n> could otherwise know do not have signatures, but we actually pull the\n> buffer from disk, even though we generally have it saved as\n> commit->buffer.\n\nThe one for the signature on the commit itself is me being lazy and\ndefensive; I did not want to have to worry about people mucking with\nwhat is in commit->buffer for whatever reason (e.g. re-encode in\ndifferent charset, etc.) and then asking the signature validated.\n\nThe other one for the merge-tag is me just being lazy, as it is\nunlikely to be corrupt by any reasonable kinds of mucking with\ncommit->buffer on a merge.\n"},{"id":"243016","messageId":"20140530170330.GA25443@sigill.intra.peff.net","threadId":"36786","inReplyTo":"xmqqvbsn9pfx.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-30T17:03:30Z","receivedAt":"2014-05-30T17:03:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 30, 2014 at 09:55:14AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Thu, May 29, 2014 at 09:54:10PM -0700, Linus Torvalds wrote:\n> >\n> >> That said, part of it is just that show-signature is so suboptimal\n> >> performance-wise, re-parsing the commit buffer for each commit when\n> >> \"show_signature\" is set. That's just crazy, we've already parsed the\n> >> commit text, we already *could* know if it has a signature or not, and\n> >> skip it if it doesn't. That would require one of the flag bits in the\n> >> object, though, or something, so it's probably not worth doing.\n> >\n> > Wow, it's really quite bad. Not only do we spend time on commits that we\n> > could otherwise know do not have signatures, but we actually pull the\n> > buffer from disk, even though we generally have it saved as\n> > commit->buffer.\n> \n> The one for the signature on the commit itself is me being lazy and\n> defensive; I did not want to have to worry about people mucking with\n> what is in commit->buffer for whatever reason (e.g. re-encode in\n> different charset, etc.) and then asking the signature validated.\n> \n> The other one for the merge-tag is me just being lazy, as it is\n> unlikely to be corrupt by any reasonable kinds of mucking with\n> commit->buffer on a merge.\n\nI don't think we need to worry about commit->buffer being mucked with.\nIt is always either NULL, or points to the original object contents.\nEncoded log messages are always placed in a separate buffer (and in fact\nwe use the same \"optionally point to commit->buffer\" trick there). And\nthings like mucking with parents always happen on the parsed form.\n\nOf course I may be missing a site, and it's certainly a maintenance risk\nfor the future. But I'd go so far as to say that anything modifying\ncommit->buffer is wrong, and that side should be fixed.\n\nDo you want me to roll it up with a real commit message?\n\nThe other option is to do something like Linus suggested, and note the\npresence/absence of signature and mergetag headers with a few bits (we\ncould even use a commit slab if we don't want to waste bits in the\nobject struct). That would prevent us hitting this code at all for most\ncommits, so we would save not only the read_sha1_file cost, but the\nextra parsing cost.\n\nHowever, that does nothing to help the cases where we _do_ have\nsignatures. A repo where somebody GPG-signed every commit, for example,\nwould still perform terribly. So even if we go that route, I think we'd\nwant to apply this technique, too.\n\n-Peff\n"},{"id":"243021","messageId":"xmqqmwdz9nl9.fsf@gitster.dls.corp.google.com","threadId":"36786","inReplyTo":"20140530170330.GA25443@sigill.intra.peff.net","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-30T17:35:14Z","receivedAt":"2014-05-30T17:35:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, May 30, 2014 at 09:55:14AM -0700, Junio C Hamano wrote:\n>\n> I don't think we need to worry about commit->buffer being mucked with.\n> It is always either NULL, or points to the original object contents.\n> Encoded log messages are always placed in a separate buffer (and in fact\n> we use the same \"optionally point to commit->buffer\" trick there). And\n> things like mucking with parents always happen on the parsed form.\n>\n> Of course I may be missing a site, and it's certainly a maintenance risk\n> for the future. But I'd go so far as to say that anything modifying\n> commit->buffer is wrong, and that side should be fixed.\n\nI fully agree, and \"that side should be fixed\" implying \"we should\nalways be on a look-out for such a change\" is something the lazyness\ntried to avoid.\n\n> Do you want me to roll it up with a real commit message?\n\nYes.  I think the change is sensible.\n"},{"id":"243034","messageId":"20140530183441.GA3704@sigill.intra.peff.net","threadId":"36786","inReplyTo":"xmqqmwdz9nl9.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-30T18:34:41Z","receivedAt":"2014-05-30T18:34:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 30, 2014 at 10:35:14AM -0700, Junio C Hamano wrote:\n\n> > Do you want me to roll it up with a real commit message?\n> \n> Yes.  I think the change is sensible.\n\nHere it is. We may want to make these helper functions available to\nother callers so they can use the same trick, but I do not know offhand\nof any others that would want it. pretty.c is the obvious place, and it\nalready uses a similar trick in logmsg_reencode (and I would expect most\nusers of the commit message would actually want the reencoded version,\nand would use that).\n\n-- >8 --\nSubject: [PATCH] reuse commit->buffer when parsing signatures\n\nWhen we call show_signature or show_mergetag, we read the\ncommit object fresh via read_sha1_file and reparse its\nheaders. However, in most cases we already have the object\ndata available as commit->buffer.  This is partially\nlaziness in dealing with the memory allocation issues, but\npartially defensive programming, in that we would always\nwant to verify a clean version of the buffer (not one that\nmight have been munged by other users of the commit).\n\nHowever, we do not currently ever munge commit->buffer, and\nnot using the already-available buffer carries a fairly big\nperformance penalty when we are looking at a large number of\ncommits. Here are timings on linux.git:\n\n  [baseline, no signatures]\n  $ time git log >/dev/null\n  real    0m4.902s\n  user    0m4.784s\n  sys     0m0.120s\n\n  [before]\n  $ time git log --show-signature >/dev/null\n  real    0m14.735s\n  user    0m9.964s\n  sys     0m0.944s\n\n  [after]\n  $ time git log --show-signature >/dev/null\n  real    0m9.981s\n  user    0m5.260s\n  sys     0m0.936s\n\nNote that our user CPU time drops almost in half, close to\nthe non-signature case, but we do still spend more\nwall-clock and system time, presumably from dealing with\ngpg.\n\nAn alternative to this is to note that most commits do not\nhave signatures (less than 1% in this repo), yet we pay the\nre-parsing cost for every commit just to find out if it has\na mergetag or signature. If we checked that when parsing the\ncommit initially, we could avoid re-examining most commits\nlater on. Even if we did pursue that direction, however,\nthis would still speed up the cases where we _do_ have\nsignatures. So it's probably worth doing either way.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c   | 44 ++++++++++++++++++++++++++++++++++++--------\n commit.h   |  2 +-\n log-tree.c |  2 +-\n 3 files changed, 38 insertions(+), 10 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex f479331..9e2abf7 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1080,14 +1080,42 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)\n \treturn 0;\n }\n \n-int parse_signed_commit(const unsigned char *sha1,\n+/*\n+ * Return the contents of the object pointed to by commit, as\n+ * if read by read_sha1_file. However, in cases where the commit's\n+ * data is already in memory, return that as an optimization.\n+ *\n+ * The resulting buffer may or may not be freshly allocated,\n+ * and should only be freed by free_commit_buffer.\n+ */\n+static const char *read_commit_buffer(const struct commit *commit,\n+\t\t\t\t      enum object_type *type,\n+\t\t\t\t      unsigned long *size)\n+{\n+\tif (commit->buffer) {\n+\t\t*type = OBJ_COMMIT;\n+\t\t*size = strlen(commit->buffer);\n+\t\treturn commit->buffer;\n+\t}\n+\n+\treturn read_sha1_file(commit->object.sha1, type, size);\n+}\n+\n+static void free_commit_buffer(const char *buffer, const struct commit *commit)\n+{\n+\tif (buffer != commit->buffer)\n+\t\tfree((char *)buffer);\n+}\n+\n+int parse_signed_commit(const struct commit *commit,\n \t\t\tstruct strbuf *payload, struct strbuf *signature)\n {\n+\n \tunsigned long size;\n \tenum object_type type;\n-\tchar *buffer = read_sha1_file(sha1, &type, &size);\n+\tconst char *buffer = read_commit_buffer(commit, &type, &size);\n \tint in_signature, saw_signature = -1;\n-\tchar *line, *tail;\n+\tconst char *line, *tail;\n \n \tif (!buffer || type != OBJ_COMMIT)\n \t\tgoto cleanup;\n@@ -1098,7 +1126,7 @@ int parse_signed_commit(const unsigned char *sha1,\n \tsaw_signature = 0;\n \twhile (line < tail) {\n \t\tconst char *sig = NULL;\n-\t\tchar *next = memchr(line, '\\n', tail - line);\n+\t\tconst char *next = memchr(line, '\\n', tail - line);\n \n \t\tnext = next ? next + 1 : tail;\n \t\tif (in_signature && line[0] == ' ')\n@@ -1120,7 +1148,7 @@ int parse_signed_commit(const unsigned char *sha1,\n \t\tline = next;\n \t}\n  cleanup:\n-\tfree(buffer);\n+\tfree_commit_buffer(buffer, commit);\n \treturn saw_signature;\n }\n \n@@ -1211,7 +1239,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check\n \n \tsigc->result = 'N';\n \n-\tif (parse_signed_commit(commit->object.sha1,\n+\tif (parse_signed_commit(commit,\n \t\t\t\t&payload, &signature) <= 0)\n \t\tgoto out;\n \tstatus = verify_signed_buffer(payload.buf, payload.len,\n@@ -1258,10 +1286,10 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,\n \tstruct commit_extra_header *extra = NULL;\n \tunsigned long size;\n \tenum object_type type;\n-\tchar *buffer = read_sha1_file(commit->object.sha1, &type, &size);\n+\tconst char *buffer = read_commit_buffer(commit, &type, &size);\n \tif (buffer && type == OBJ_COMMIT)\n \t\textra = read_commit_extra_header_lines(buffer, size, exclude);\n-\tfree(buffer);\n+\tfree_commit_buffer(buffer, commit);\n \treturn extra;\n }\n \ndiff --git a/commit.h b/commit.h\nindex a9f177b..a765f0f 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -287,7 +287,7 @@ struct merge_remote_desc {\n  */\n struct commit *get_merge_parent(const char *name);\n \n-extern int parse_signed_commit(const unsigned char *sha1,\n+extern int parse_signed_commit(const struct commit *commit,\n \t\t\t       struct strbuf *message, struct strbuf *signature);\n extern void print_commit_list(struct commit_list *list,\n \t\t\t      const char *format_cur,\ndiff --git a/log-tree.c b/log-tree.c\nindex cf2f86c..6358599 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -376,7 +376,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)\n \tstruct strbuf gpg_output = STRBUF_INIT;\n \tint status;\n \n-\tif (parse_signed_commit(commit->object.sha1, &payload, &signature) <= 0)\n+\tif (parse_signed_commit(commit, &payload, &signature) <= 0)\n \t\tgoto out;\n \n \tstatus = verify_signed_buffer(payload.buf, payload.len,\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"243035","messageId":"20140530183957.GA3909@sigill.intra.peff.net","threadId":"36786","inReplyTo":"20140530183441.GA3704@sigill.intra.peff.net","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-30T18:39:57Z","receivedAt":"2014-05-30T18:39:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 30, 2014 at 02:34:41PM -0400, Jeff King wrote:\n\n> On Fri, May 30, 2014 at 10:35:14AM -0700, Junio C Hamano wrote:\n> \n> > > Do you want me to roll it up with a real commit message?\n> > \n> > Yes.  I think the change is sensible.\n> \n> Here it is. [...]\n\nBy the way, I rather derailed Linus's original patch with this\nsub-thread. I think it actually looks fine as-is. The shortcomings he\nlisted are all there, but I think addressing them would end up even\nworse.\n\n-Peff\n"},{"id":"243050","messageId":"xmqqmwdz809b.fsf@gitster.dls.corp.google.com","threadId":"36786","inReplyTo":"20140530183441.GA3704@sigill.intra.peff.net","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-30T20:44:32Z","receivedAt":"2014-05-30T20:44:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: [PATCH] reuse commit->buffer when parsing signatures\n> ...\n> Signed-off-by: Jeff King <peff@peff.net>\n\nHmph, unfortunately this seems to break t7510.\n"},{"id":"243051","messageId":"20140530204853.GA9271@sigill.intra.peff.net","threadId":"36786","inReplyTo":"xmqqmwdz809b.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-30T20:48:53Z","receivedAt":"2014-05-30T20:48:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 30, 2014 at 01:44:32PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Subject: [PATCH] reuse commit->buffer when parsing signatures\n> > ...\n> > Signed-off-by: Jeff King <peff@peff.net>\n> \n> Hmph, unfortunately this seems to break t7510.\n\nUrgh, sorry for not testing more thoroughly.\n\nI imagine it is because of the strlen(commit->buffer) I introduced.\nUnfortunately I do not know if we have an easy way to know the length of\ncommit->buffer, short of hitting sha1_object_info (which is somewhat\nexpensive to do for every commit).\n\nI wonder if it would be sane to remove or quote NULs when attaching the\nbuffer to commit->buffer. That would _break_ signatures, but that is a\ngood thing. I do not think there is a reason to have NULs in your commit\nmessage unless you are doing something malicious (or using utf16, but\nthat already is horribly broken).\n\n-Peff\n"},{"id":"243052","messageId":"xmqqfvjr7zwk.fsf@gitster.dls.corp.google.com","threadId":"36786","inReplyTo":"xmqqmwdz809b.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-30T20:52:11Z","receivedAt":"2014-05-30T20:52:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> Subject: [PATCH] reuse commit->buffer when parsing signatures\n>> ...\n>> Signed-off-by: Jeff King <peff@peff.net>\n>\n> Hmph, unfortunately this seems to break t7510.\n\nAnd I think without re-reading the patch I know what is wrong.  The\nlength of the object and strlen(commit->buffer) would be different,\nno?\n"},{"id":"243056","messageId":"xmqqa99z7yxq.fsf@gitster.dls.corp.google.com","threadId":"36786","inReplyTo":"20140530204853.GA9271@sigill.intra.peff.net","subject":"Re: [RFC PATCH] git log: support \"auto\" decorations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-30T21:13:05Z","receivedAt":"2014-05-30T21:13:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I wonder if it would be sane to remove or quote NULs when attaching the\n> buffer to commit->buffer. That would _break_ signatures, but that is a\n> good thing. I do not think there is a reason to have NULs in your commit\n> message unless you are doing something malicious (or using utf16, but\n> that already is horribly broken).\n\nAhh, our messages crossed.  I do not think we are quite ready to\ndepart from our traditional position: the payload of a commit object\ncan be any bytestream, even though we do expect and encourage them\nto be human readable text in a reasonable encoding.  And there is no\nfundamental reason why we should forbid signing the payload that\nhappens to be a structured binary blob.\n\nThe user may need some way other than \"log --show-signature\" that\ncan be used to validate, because \"log\" itself will be useless for\nsuch a payload with or without signature.  But I think that may be\na more or less orthogonal issue.\n"}]}