{"thread":{"id":"47890","subject":"Question about get_cached_commit_buffer()","startedAt":"2018-02-20T22:12:58Z","lastAt":"2018-02-22T01:52:43Z","messageCount":17,"participants":["Derrick Stolee","Jeff King","Junio C Hamano","Stefan Beller","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"339752","messageId":"ecbbe515-b7a8-3dc8-7d14-32412e7b12c3@gmail.com","threadId":"47890","inReplyTo":null,"subject":"Question about get_cached_commit_buffer()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-02-20T22:12:50Z","receivedAt":"2018-02-20T22:12:58Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"While working on my commit-graph patch [1] and using a local build in my \nusual workflows, I found a bug in my branch.\n\nEssentially, when calling `git rev-list --header`, the header \ninformation is actually missing for many commits except the first. This \nwas not caught in my testing since t6000-rev-list-misc.sh creates a test \nrepo with only one commit.\n\nThe root cause is that the serialized commit graph gets its speedup by \nnot loading buffers for every commit. For many use-cases (merge bases, \n--topo-order, etc.) we do not need the buffer for most of the commits. \nIn the rev-list example, the buffer is loaded due to a side-effect of \nbeing referenced by HEAD or a branch. A similar effect happened in `git \nlog`, hence the following change in my patch:\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 580b3a9..14735d4 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -647,8 +647,7 @@ void show_log(struct rev_info *opt)\n  \t\tshow_mergetag(opt, commit);\n  \t}\n  \n-\tif (!get_cached_commit_buffer(commit, NULL))\n-\t\treturn;\n+\tget_commit_buffer(commit, NULL);\n  \n  \tif (opt->show_notes) {\n  \t\tint raw;\n\n\nIn rev-list, the \"--header\" option outputs a value and expects the \nbuffer to be cached. It outputs the header info only if \nget_cached_commit_buffer() returns a non-null buffer, giving incorrect \noutput. If it called get_commit_buffer() instead, it would immediately \ncall get_cached_commit_buffer() and on failure actually load the buffer.\n\nThis has not been a problem before, since the buffer was always loaded \nat some point for each commit (and saved because of the \nsave_commit_buffer global).\n\nI propose to make get_cached_commit_buffer() static to commit.c and \nconvert all callers to get_commit_buffer(). Is there any reason to _not_ \ndo this? It seems that there is no functional or performance change.\n\nAfter the serialized commit graph exists and is used, the only change is \nthat we delay loading the buffer until we need to read the commit \nmetadata that is not stored in the graph (message, author/committer).\n\nThanks,\n-Stolee\n\n[1] \nhttps://public-inbox.org/git/4d1ee202-7d79-d73c-6e05-d0fc85db943c@gmail.com/T/#m381bfd3f2eafbd254e35a5147cd198bc35055e92\n     [Patch v4 00/14] Serialized Git Commit Graph\n\n[2] \nhttps://public-inbox.org/git/20140610214039.GJ19147@sigill.intra.peff.net/\n     [Patch 10/17] provide helpers to access the commit buffer\n"},{"id":"339760","messageId":"20180220225726.GA17496@sigill.intra.peff.net","threadId":"47890","inReplyTo":"ecbbe515-b7a8-3dc8-7d14-32412e7b12c3@gmail.com","subject":"Re: Question about get_cached_commit_buffer()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-20T22:57:27Z","receivedAt":"2018-02-20T22:57:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 20, 2018 at 05:12:50PM -0500, Derrick Stolee wrote:\n\n> In rev-list, the \"--header\" option outputs a value and expects the buffer to\n> be cached. It outputs the header info only if get_cached_commit_buffer()\n> returns a non-null buffer, giving incorrect output. If it called\n> get_commit_buffer() instead, it would immediately call\n> get_cached_commit_buffer() and on failure actually load the buffer.\n> \n> This has not been a problem before, since the buffer was always loaded at\n> some point for each commit (and saved because of the save_commit_buffer\n> global).\n> \n> I propose to make get_cached_commit_buffer() static to commit.c and convert\n> all callers to get_commit_buffer(). Is there any reason to _not_ do this? It\n> seems that there is no functional or performance change.\n\nThat helper was added in 152ff1cceb (provide helpers to access the\ncommit buffer, 2014-06-10). I think interesting part is the final\nparagraph:\n\n      Note that we also need to add a \"get_cached\" variant which\n      returns NULL when we do not have a cached buffer. At first\n      glance this seems to defeat the purpose of \"get\", which is\n      to always provide a return value. However, some log code\n      paths actually use the NULL-ness of commit->buffer as a\n      boolean flag to decide whether to try printing the\n      commit. At least for now, we want to continue supporting\n      that use.\n\nSo I think a conversion to get_commit_buffer() would break the callers\nthat use the boolean nature for something useful. Unfortunately the\nauthor did not enumerate exactly what those uses are, so we'll have to\ndig. :)\n\nMy guess is that it has to do with showing some commits twice, since we\nwould normally have the buffer available the first time we hit the\ncommit, and then free it in finish_commit().\n\nIf we blame that rev-list line (and then walk back over a bunch of\nuninteresting commits via parent re-blaming), it comes from 3131b71301\n(Add \"--show-all\" revision walker flag for debugging, 2008-02-09).\n\nSo there it is. It does show commits multiple times, but suppresses the\nverbose header after the first showing. If we do something like this:\n\n  git rev-list --show-all --pretty --boundary c93150cfb0^-\n\nyou'll see some boundary commits that _don't_ have their pretty headers\nshown. And with your proposed patch, we'd show them again. To keep the\nsame behavior we need to store that \"we've already seen this\" boolean\nsomewhere else (e.g., in an object flag; possibly SEEN, but that might\nrun afoul of other logic).\n\nIt looks like the call in log-tree comes from the same commit, and\nserves the same purpose.\n\nAside from storing the boolean \"did we show it\" in another way, the\nother option is to simply change the behavior and accept that we might\npretty-print the commit twice. This is a backwards-incompatible change,\nbut I'm not sure if anybody would care. According to that commit,\n--show-all was added explicitly for debugging, and it has never been\ndocumented.  I couldn't find any reference to people actually using it\non the list (a grep of the whole archive turns up 32 messages, most of\nwhich are just it appearing in context; the only person mentioning its\nactual use was Linus in 2008.\n\n-Peff\n"},{"id":"339807","messageId":"22169205-8020-c816-0968-f6293e8d40bc@gmail.com","threadId":"47890","inReplyTo":"20180220225726.GA17496@sigill.intra.peff.net","subject":"Re: Question about get_cached_commit_buffer()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-02-21T14:13:22Z","receivedAt":"2018-02-21T14:13:35Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/20/2018 5:57 PM, Jeff King wrote:\n> On Tue, Feb 20, 2018 at 05:12:50PM -0500, Derrick Stolee wrote:\n>\n>> In rev-list, the \"--header\" option outputs a value and expects the buffer to\n>> be cached. It outputs the header info only if get_cached_commit_buffer()\n>> returns a non-null buffer, giving incorrect output. If it called\n>> get_commit_buffer() instead, it would immediately call\n>> get_cached_commit_buffer() and on failure actually load the buffer.\n>>\n>> This has not been a problem before, since the buffer was always loaded at\n>> some point for each commit (and saved because of the save_commit_buffer\n>> global).\n>>\n>> I propose to make get_cached_commit_buffer() static to commit.c and convert\n>> all callers to get_commit_buffer(). Is there any reason to _not_ do this? It\n>> seems that there is no functional or performance change.\n> That helper was added in 152ff1cceb (provide helpers to access the\n> commit buffer, 2014-06-10). I think interesting part is the final\n> paragraph:\n>\n>        Note that we also need to add a \"get_cached\" variant which\n>        returns NULL when we do not have a cached buffer. At first\n>        glance this seems to defeat the purpose of \"get\", which is\n>        to always provide a return value. However, some log code\n>        paths actually use the NULL-ness of commit->buffer as a\n>        boolean flag to decide whether to try printing the\n>        commit. At least for now, we want to continue supporting\n>        that use.\n>\n> So I think a conversion to get_commit_buffer() would break the callers\n> that use the boolean nature for something useful. Unfortunately the\n> author did not enumerate exactly what those uses are, so we'll have to\n> dig. :)\n>\n> My guess is that it has to do with showing some commits twice, since we\n> would normally have the buffer available the first time we hit the\n> commit, and then free it in finish_commit().\n>\n> If we blame that rev-list line (and then walk back over a bunch of\n> uninteresting commits via parent re-blaming), it comes from 3131b71301\n> (Add \"--show-all\" revision walker flag for debugging, 2008-02-09).\n\nThanks for doing this digging. I appreciate the breadcrumbs, too, so I \ncan do a better job of digging next time.\n\n> So there it is. It does show commits multiple times, but suppresses the\n> verbose header after the first showing. If we do something like this:\n>\n>    git rev-list --show-all --pretty --boundary c93150cfb0^-\n>\n> you'll see some boundary commits that _don't_ have their pretty headers\n> shown. And with your proposed patch, we'd show them again. To keep the\n> same behavior we need to store that \"we've already seen this\" boolean\n> somewhere else (e.g., in an object flag; possibly SEEN, but that might\n> run afoul of other logic).\n\nWhat confuses me about this behavior is that the OID is still shown on \nthe repeat (and in the case of `git log --oneline` will not actually \nhave a line break between two short-OIDs). I don't believe this behavior \nis something to preserve.\n\nYou are right that we definitely don't want to show the full content twice.\n\n> It looks like the call in log-tree comes from the same commit, and\n> serves the same purpose.\n>\n> Aside from storing the boolean \"did we show it\" in another way, the\n> other option is to simply change the behavior and accept that we might\n> pretty-print the commit twice. This is a backwards-incompatible change,\n> but I'm not sure if anybody would care. According to that commit,\n> --show-all was added explicitly for debugging, and it has never been\n> documented.  I couldn't find any reference to people actually using it\n> on the list (a grep of the whole archive turns up 32 messages, most of\n> which are just it appearing in context; the only person mentioning its\n> actual use was Linus in 2008.\n\nUnless I am misunderstanding, the current behavior on a repeated commit \nis already incorrect: some amount of output occurs before checking the \nbuffer, so the output includes repeated records but with formatting that \nviolates the expectation. By doing the simple change of swapping \nget_cached_commit_buffer() with get_commit_buffer(), we correct that \nformat violation but have duplicate copies.\n\nThe most-correct thing to do (in my opinion) is to put the requirement \nof \"no repeats\" into the revision walk logic and stop having the \nformatting methods expect them. Then, however we change this boolean \nsetting of \"we have seen this before\" it will not require the formatting \nmethods to change.\n\nI can start working on a patch to move the duplicate-removal logic into \nrevision.c instead of these three callers:\n\nbuiltin/rev-list.c:     if (revs->verbose_header && \nget_cached_commit_buffer(commit, NULL)) {\nlog-tree.c:     if (!get_cached_commit_buffer(commit, NULL))\nobject.c:                       if (!get_cached_commit_buffer(commit, \nNULL)) {\n\nBut this caller seems pretty important in pretty.c:\n\n         /*\n          * Otherwise, we still want to munge the encoding header in the\n          * result, which will be done by modifying the buffer. If we\n          * are using a fresh copy, we can reuse it. But if we are using\n          * the cached copy from get_commit_buffer, we need to duplicate it\n          * to avoid munging the cached copy.\n          */\n         if (msg == get_cached_commit_buffer(commit, NULL))\n                 out = xstrdup(msg);\n         else\n                 out = (char *)msg\n\nThanks,\n-Stolee\n"},{"id":"339819","messageId":"20180221184811.GD4333@sigill.intra.peff.net","threadId":"47890","inReplyTo":"22169205-8020-c816-0968-f6293e8d40bc@gmail.com","subject":"Re: Question about get_cached_commit_buffer()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-21T18:48:12Z","receivedAt":"2018-02-21T18:48:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2018 at 09:13:22AM -0500, Derrick Stolee wrote:\n\n> > So there it is. It does show commits multiple times, but suppresses the\n> > verbose header after the first showing. If we do something like this:\n> > \n> >    git rev-list --show-all --pretty --boundary c93150cfb0^-\n> > \n> > you'll see some boundary commits that _don't_ have their pretty headers\n> > shown. And with your proposed patch, we'd show them again. To keep the\n> > same behavior we need to store that \"we've already seen this\" boolean\n> > somewhere else (e.g., in an object flag; possibly SEEN, but that might\n> > run afoul of other logic).\n> \n> What confuses me about this behavior is that the OID is still shown on the\n> repeat (and in the case of `git log --oneline` will not actually have a line\n> break between two short-OIDs). I don't believe this behavior is something to\n> preserve.\n\nI think that repeating the oid is intentional; the point is to dump how\nthe traversal code is hitting the endpoints, even if we do so multiple\ntimes.\n\nThe --oneline behavior just looks like a bug. I think --format is broken\nwith --show-all, too (it does not show anything!).\n\n> Unless I am misunderstanding, the current behavior on a repeated commit is\n> already incorrect: some amount of output occurs before checking the buffer,\n> so the output includes repeated records but with formatting that violates\n> the expectation. By doing the simple change of swapping\n> get_cached_commit_buffer() with get_commit_buffer(), we correct that format\n> violation but have duplicate copies.\n\nYeah, I'd agree with that assessment.\n\n> The most-correct thing to do (in my opinion) is to put the requirement of\n> \"no repeats\" into the revision walk logic and stop having the formatting\n> methods expect them. Then, however we change this boolean setting of \"we\n> have seen this before\" it will not require the formatting methods to change.\n\nBut then you wouldn't show repeats at all. If I'm understanding you\ncorrectly.\n\nTBH, I do not think it is worth spending a lot of effort on this\n--show-all feature. It seems mostly like forgotten debugging cruft to\nme. That's why I'd be OK with showing the whole header as the simplest\nfix (i.e., just removing those calls entirely, not even converting them\nto get_commit_buffer).\n\n> I can start working on a patch to move the duplicate-removal logic into\n> revision.c instead of these three callers:\n> \n> builtin/rev-list.c:     if (revs->verbose_header &&\n> get_cached_commit_buffer(commit, NULL)) {\n> log-tree.c:     if (!get_cached_commit_buffer(commit, NULL))\n> object.c:                       if (!get_cached_commit_buffer(commit, NULL))\n\nThose first two are duplicate detection. The third one in object.c\nshould stay, though. We've been fed a commit buffer to parse, and we\nwant to know whether we should attach it as the cached buffer for that\ncommit. But if we already have a cached buffer, there's no point in\ndoing so. And that's what we're checking there.\n\nThough I think it would be equally correct to have set_commit_buffer()\njust throw away the existing cache entry and replace it with this one. I\ndon't think there's a real reason to prefer the old to the new. And that\nmight be worth doing if it would let us drop get_cached_commit_buffer()\nas a public function. But...\n\n> But this caller seems pretty important in pretty.c:\n> \n>         /*\n>          * Otherwise, we still want to munge the encoding header in the\n>          * result, which will be done by modifying the buffer. If we\n>          * are using a fresh copy, we can reuse it. But if we are using\n>          * the cached copy from get_commit_buffer, we need to duplicate it\n>          * to avoid munging the cached copy.\n>          */\n>         if (msg == get_cached_commit_buffer(commit, NULL))\n>                 out = xstrdup(msg);\n>         else\n>                 out = (char *)msg\n\nLike the one in object.c, this really does want to know about the cached\nentry. And it should be unaffected by your patch, since we will have\ncalled get_commit_buffer() at the top of that function.\n\nIf we wanted to write this one without get_cached_commit_buffer(), we'd\nreally need a function to ask \"did this pointer come from the cache, or\nwas it freshly allocated?\". That's the same thing we do for\nunuse_commit_buffer(). So in theory we could have a boolean function\nthat would check that, and that would let us make\nget_cached_commit_buffer() private.\n\nIn my opinion it's not really worth trying to make it private. The\nconfusion you're fixing in the first two calls is not due to a bad API,\nbut due to some subtly confusing logic in that code's use of the API. ;)\n\nSo I'd probably do this:\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex d94062bc84..3af56921c8 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -150,7 +150,7 @@ static void show_commit(struct commit *commit, void *data)\n \telse\n \t\tputchar('\\n');\n \n-\tif (revs->verbose_header && get_cached_commit_buffer(commit, NULL)) {\n+\tif (revs->verbose_header) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstruct pretty_print_context ctx = {0};\n \t\tctx.abbrev = revs->abbrev;\ndiff --git a/log-tree.c b/log-tree.c\nindex cab9353f45..cb2dab8a1c 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -690,9 +690,6 @@ void show_log(struct rev_info *opt)\n \t\tshow_mergetag(opt, commit);\n \t}\n \n-\tif (!get_cached_commit_buffer(commit, NULL))\n-\t\treturn;\n-\n \tif (opt->show_notes) {\n \t\tint raw;\n \t\tstruct strbuf notebuf = STRBUF_INIT;\n\nwith the rationale that:\n\n  1. Nobody really cares about this verbose-output suppression anyway.\n\n  2. The code is confusing and fragile, since it uses the cached commit\n     buffer as an implicit boolean for \"did we show the commit already\".\n\n  3. It's broken for --oneline and user-formats, and this fixes it.\n\n-Peff\n"},{"id":"339823","messageId":"20180221185204.GA8476@sigill.intra.peff.net","threadId":"47890","inReplyTo":"20180221184811.GD4333@sigill.intra.peff.net","subject":"Re: Question about get_cached_commit_buffer()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-21T18:52:04Z","receivedAt":"2018-02-21T18:52:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2018 at 01:48:11PM -0500, Jeff King wrote:\n\n> > What confuses me about this behavior is that the OID is still shown on the\n> > repeat (and in the case of `git log --oneline` will not actually have a line\n> > break between two short-OIDs). I don't believe this behavior is something to\n> > preserve.\n> \n> I think that repeating the oid is intentional; the point is to dump how\n> the traversal code is hitting the endpoints, even if we do so multiple\n> times.\n> \n> The --oneline behavior just looks like a bug. I think --format is broken\n> with --show-all, too (it does not show anything!).\n\nI poked at one of the examples a little more closely. I actually think\nthese are not repeats, but simply UNINTERESTING parents that we never\nneeded to look at in our traversal (because we hit a point where\neverything was UNINTERESTING).\n\nSo we are relying not on finish_commit() to have freed the buffer, but\non the traversal code to have never parsed those commits in the first\nplace. Which is doubly subtle.\n\nI think the rest of my email stands, though: we should just show the\nfull headers for those commits.\n\n-Peff\n"},{"id":"339826","messageId":"1519240631-221761-1-git-send-email-dstolee@microsoft.com","threadId":"47890","inReplyTo":"20180221185204.GA8476@sigill.intra.peff.net","subject":"[PATCH] commit: drop uses of get_cached_commit_buffer()","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-02-21T19:17:11Z","receivedAt":"2018-02-21T19:17:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The get_cached_commit_buffer() method provides access to the buffer\nloaded for a struct commit, if it was ever loadead and was not freed.\n\nTwo places use this to inform how to output information about commits.\n\nlog-tree.c uses this method to short-circuit the output of commit\ninformation when the buffer is not cached. However, this leads to\nincorrect output in 'git log --oneline' where the short-OID is written\nbut then the rest of the commit information is dropped and the next\ncommit is written on the same line.\n\nrev-list uses this method for two reasons:\n\n- First, if the revision walk visits a commit twice, the buffer was\n  freed by rev-list in the first write. The output then does not\n  match the format expectations, since the OID is written without the\n  rest of the content.\n\n- Second, if the revision walk visits a commit that was marked\n  UNINTERESTING, the walk may never load a buffer and hence rev-list\n  will not output the verbose information.\n\nThese behaviors are undocumented, untested, and unlikely to be\nexpected by users or other software attempting to parse this output.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/rev-list.c | 2 +-\n log-tree.c         | 3 ---\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 48300d9..d320b6f 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -134,7 +134,7 @@ static void show_commit(struct commit *commit, void *data)\n \telse\n \t\tputchar('\\n');\n \n-\tif (revs->verbose_header && get_cached_commit_buffer(commit, NULL)) {\n+\tif (revs->verbose_header) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstruct pretty_print_context ctx = {0};\n \t\tctx.abbrev = revs->abbrev;\ndiff --git a/log-tree.c b/log-tree.c\nindex fc0cc0d..22b2fb6 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -659,9 +659,6 @@ void show_log(struct rev_info *opt)\n \t\tshow_mergetag(opt, commit);\n \t}\n \n-\tif (!get_cached_commit_buffer(commit, NULL))\n-\t\treturn;\n-\n \tif (opt->show_notes) {\n \t\tint raw;\n \t\tstruct strbuf notebuf = STRBUF_INIT;\n-- \n2.7.4\n\n"},{"id":"339827","messageId":"07690508-e6c6-5b3b-6c03-5ad83c9c3501@gmail.com","threadId":"47890","inReplyTo":"1519240631-221761-1-git-send-email-dstolee@microsoft.com","subject":"Re: [PATCH] commit: drop uses of get_cached_commit_buffer()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-02-21T19:19:17Z","receivedAt":"2018-02-21T19:19:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/21/2018 2:17 PM, Derrick Stolee wrote:\n> The get_cached_commit_buffer() method provides access to the buffer\n> loaded for a struct commit, if it was ever loadead and was not freed.\n>\n> Two places use this to inform how to output information about commits.\n>\n> log-tree.c uses this method to short-circuit the output of commit\n> information when the buffer is not cached. However, this leads to\n> incorrect output in 'git log --oneline' where the short-OID is written\n> but then the rest of the commit information is dropped and the next\n> commit is written on the same line.\n>\n> rev-list uses this method for two reasons:\n>\n> - First, if the revision walk visits a commit twice, the buffer was\n>    freed by rev-list in the first write. The output then does not\n>    match the format expectations, since the OID is written without the\n>    rest of the content.\n>\n> - Second, if the revision walk visits a commit that was marked\n>    UNINTERESTING, the walk may never load a buffer and hence rev-list\n>    will not output the verbose information.\n>\n> These behaviors are undocumented, untested, and unlikely to be\n> expected by users or other software attempting to parse this output.\n>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n\nThis would be a good time to allow multiple authors, or to just change \nthe author, since this is exactly the diff you (Peff) provided in an \nearlier email. The commit message hopefully summarizes our discussion, \nbut I welcome edits.\n\n> ---\n>   builtin/rev-list.c | 2 +-\n>   log-tree.c         | 3 ---\n>   2 files changed, 1 insertion(+), 4 deletions(-)\n>\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 48300d9..d320b6f 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -134,7 +134,7 @@ static void show_commit(struct commit *commit, void *data)\n>   \telse\n>   \t\tputchar('\\n');\n>   \n> -\tif (revs->verbose_header && get_cached_commit_buffer(commit, NULL)) {\n> +\tif (revs->verbose_header) {\n>   \t\tstruct strbuf buf = STRBUF_INIT;\n>   \t\tstruct pretty_print_context ctx = {0};\n>   \t\tctx.abbrev = revs->abbrev;\n> diff --git a/log-tree.c b/log-tree.c\n> index fc0cc0d..22b2fb6 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -659,9 +659,6 @@ void show_log(struct rev_info *opt)\n>   \t\tshow_mergetag(opt, commit);\n>   \t}\n>   \n> -\tif (!get_cached_commit_buffer(commit, NULL))\n> -\t\treturn;\n> -\n>   \tif (opt->show_notes) {\n>   \t\tint raw;\n>   \t\tstruct strbuf notebuf = STRBUF_INIT;\n\n"},{"id":"339846","messageId":"xmqqtvuax9te.fsf@gitster-ct.c.googlers.com","threadId":"47890","inReplyTo":"20180221184811.GD4333@sigill.intra.peff.net","subject":"Re: Question about get_cached_commit_buffer()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-21T22:22:05Z","receivedAt":"2018-02-21T22:22:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think that repeating the oid is intentional; the point is to dump how\n> the traversal code is hitting the endpoints, even if we do so multiple\n> times.\n>\n> The --oneline behavior just looks like a bug. I think --format is broken\n> with --show-all, too (it does not show anything!).\n\nI do not know about the --format thing, but the part about --oneline\nbeing a bug is correct.  I've known about the oneline that does not\nshow anything other than the oid (not even end-of-line) for unparsed\ncommits for a long time---I just didn't bother looking into fixing\nit exactly because this is only a debugging aid ;-)\n\n> Though I think it would be equally correct to have set_commit_buffer()\n> just throw away the existing cache entry and replace it with this one. I\n> don't think there's a real reason to prefer the old to the new. And that\n> might be worth doing if it would let us drop get_cached_commit_buffer()\n> as a public function. But...\n> ...\n> In my opinion it's not really worth trying to make it private. The\n> confusion you're fixing in the first two calls is not due to a bad API,\n> but due to some subtly confusing logic in that code's use of the API. ;)\n\nYup.\n\n> So I'd probably do this:\n> ...\n\nMakes sense to me.\n"},{"id":"339851","messageId":"20180221225002.GB7944@sigill.intra.peff.net","threadId":"47890","inReplyTo":"xmqqtvuax9te.fsf@gitster-ct.c.googlers.com","subject":"Re: Question about get_cached_commit_buffer()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-21T22:50:02Z","receivedAt":"2018-02-21T22:50:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2018 at 02:22:05PM -0800, Junio C Hamano wrote:\n\n> > I think that repeating the oid is intentional; the point is to dump how\n> > the traversal code is hitting the endpoints, even if we do so multiple\n> > times.\n> >\n> > The --oneline behavior just looks like a bug. I think --format is broken\n> > with --show-all, too (it does not show anything!).\n> \n> I do not know about the --format thing,[...]\n\nHmm, maybe it is fine. I thought before that I got funny output out of\n\"git log --show-all --format\", but I can't seem to reproduce it now.\n\n> being a bug is correct.  I've known about the oneline that does not\n> show anything other than the oid (not even end-of-line) for unparsed\n> commits for a long time---I just didn't bother looking into fixing\n> it exactly because this is only a debugging aid ;-)\n\nOut of curiosity, do you actually use --show-all for anything? I didn't\neven know it existed until this thread, and AFAICT nobody but Linus has\never recommended its use. And it does not even seem that useful as a\ngeneral debugging aid.\n\nSo what I'm wondering is whether we should consider just ripping it out\n(but I'm OK with keeping it, as once the commit-buffer stuff is fixed,\nit's probably not hurting anybody).\n\n-Peff\n"},{"id":"339856","messageId":"xmqqinaqx7wp.fsf@gitster-ct.c.googlers.com","threadId":"47890","inReplyTo":"20180221225002.GB7944@sigill.intra.peff.net","subject":"Re: Question about get_cached_commit_buffer()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-21T23:03:18Z","receivedAt":"2018-02-21T23:03:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Out of curiosity, do you actually use --show-all for anything?\n\nAbsolutely not.  I'd actually love it if I could say \"not anymore\"\ninstead, but I haven't had an opportunity to debug the revision\ntraversal code for quite some time so I do not even remember when\nwas the last time I used it, which disqualifies me from saying even\nthat.\n\n> So what I'm wondering is whether we should consider just ripping it out\n> (but I'm OK with keeping it, as once the commit-buffer stuff is fixed,\n> it's probably not hurting anybody).\n\nI see no problem in removing it.  With more \"interesting\" features\nrelying on post-processing (like 'simplify-merges'), show_all whose\nprimary focus was how limit_list() behaves soft of outlived its\nusefulness, I would think.\n\n"},{"id":"339858","messageId":"20180221231338.GC7944@sigill.intra.peff.net","threadId":"47890","inReplyTo":"1519240631-221761-1-git-send-email-dstolee@microsoft.com","subject":"Re: [PATCH] commit: drop uses of get_cached_commit_buffer()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-21T23:13:38Z","receivedAt":"2018-02-21T23:13:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2018 at 02:17:11PM -0500, Derrick Stolee wrote:\n\n> The get_cached_commit_buffer() method provides access to the buffer\n> loaded for a struct commit, if it was ever loadead and was not freed.\n> \n> Two places use this to inform how to output information about commits.\n> \n> log-tree.c uses this method to short-circuit the output of commit\n> information when the buffer is not cached. However, this leads to\n> incorrect output in 'git log --oneline' where the short-OID is written\n> but then the rest of the commit information is dropped and the next\n> commit is written on the same line.\n> \n> rev-list uses this method for two reasons:\n> \n> - First, if the revision walk visits a commit twice, the buffer was\n>   freed by rev-list in the first write. The output then does not\n>   match the format expectations, since the OID is written without the\n>   rest of the content.\n\nI'm not sure after my earlier digging if there is even a way to trigger\nthis (and if so, it is probably accidental, since those lines were added\nexplicitly for --show-all).\n\nAnd actually after re-reading the commit message for 3131b7130 again, I\nthink the current behavior is definitely not something that was\ncarefully planned. So I'd propose a commit message like below.\n\n-- >8 --\nSubject: [PATCH] commit: drop uses of get_cached_commit_buffer()\n\nThe \"--show-all\" revision option shows UNINTERESTING\ncommits. Some of these commits may be unparsed when we try\nto show them (since we may or may not need to walk their\nparents to fulfill the request).\n\nCommit 3131b71301 (Add \"--show-all\" revision walker flag for\ndebugging, 2008-02-09) resolved this by just skipping\npretty-printing for commits without their object contents\ncached, saying:\n\n  Because we now end up listing commits we may not even have been parsed\n  at all \"show_log\" and \"show_commit\" need to protect against commits\n  that don't have a commit buffer entry.\n\nThat was the easy fix to avoid the pretty-printer segfaulting,\nbut:\n\n  1. It doesn't work for all formats. E.g., --oneline\n     prints the oid for each such commit but not a trailing\n     newline, leading to jumbled output.\n\n  2. It only affects some commits, depending on whether we\n     happened to parse them or not (so if they were at the\n     tip of an UNINTERESTING starting point, or if we\n     happened to traverse over them, you'd see more data).\n\n  3. It unncessarily ties the decision to show the verbose\n     header to whether the commit buffer was cached. That\n     makes it harder to change the logic around caching\n     (e.g., if we could traverse without actually loading\n     the full commit objects).\n\nThese days it's safe to feed such a commit to the\npretty-print code. Since be5c9fb904 (logmsg_reencode: lazily\nload missing commit buffers, 2013-01-26), we'll load it on\ndemand in such a case. So let's just always show the verbose\nheaders.\n\nThis does change the behavior of plumbing, but:\n\n  a. The --show-all option was explicitly introduced as a\n     debugging aid, and was never documented (and has rarely\n     even been mentioned on the list by git devs).\n\n  b. Avoiding the commits was already not deterministic due\n     to (2) above. So the caller might have seen full\n     headers for these commits anyway, and would need to be\n     prepared for it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-list.c | 2 +-\n log-tree.c         | 3 ---\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 48300d9e11..d320b6f1e3 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -134,7 +134,7 @@ static void show_commit(struct commit *commit, void *data)\n \telse\n \t\tputchar('\\n');\n \n-\tif (revs->verbose_header && get_cached_commit_buffer(commit, NULL)) {\n+\tif (revs->verbose_header) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstruct pretty_print_context ctx = {0};\n \t\tctx.abbrev = revs->abbrev;\ndiff --git a/log-tree.c b/log-tree.c\nindex fc0cc0d6d1..22b2fb6c58 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -659,9 +659,6 @@ void show_log(struct rev_info *opt)\n \t\tshow_mergetag(opt, commit);\n \t}\n \n-\tif (!get_cached_commit_buffer(commit, NULL))\n-\t\treturn;\n-\n \tif (opt->show_notes) {\n \t\tint raw;\n \t\tstruct strbuf notebuf = STRBUF_INIT;\n-- \n2.16.2.555.g885a024879\n\n"},{"id":"339859","messageId":"CAGZ79kZz+hC+eXxa2A-NpnCZa-LikWDaegfijUjHp5CCenwQvA@mail.gmail.com","threadId":"47890","inReplyTo":"20180221231338.GC7944@sigill.intra.peff.net","subject":"Re: [PATCH] commit: drop uses of get_cached_commit_buffer()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-02-21T23:22:02Z","receivedAt":"2018-02-21T23:22:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Subject: [PATCH] commit: drop uses of get_cached_commit_buffer()\n> ---\n>  builtin/rev-list.c | 2 +-\n>  log-tree.c         | 3 ---\n>  2 files changed, 1 insertion(+), 4 deletions(-)\n\nNow if we'd get around to rewrite pretty.c as well, we could make it static,\ngiving a stronger reason of not using that function. But it looks a bit\ncomplicated to me, who is not familiar in that area of the code.\n\nThanks for making less use of this suboptimal API,\nStefan\n"},{"id":"339860","messageId":"20180221232724.GD7944@sigill.intra.peff.net","threadId":"47890","inReplyTo":"xmqqinaqx7wp.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] revision: drop --show-all option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-21T23:27:24Z","receivedAt":"2018-02-21T23:27:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2018 at 03:03:18PM -0800, Junio C Hamano wrote:\n\n> > So what I'm wondering is whether we should consider just ripping it out\n> > (but I'm OK with keeping it, as once the commit-buffer stuff is fixed,\n> > it's probably not hurting anybody).\n> \n> I see no problem in removing it.  With more \"interesting\" features\n> relying on post-processing (like 'simplify-merges'), show_all whose\n> primary focus was how limit_list() behaves soft of outlived its\n> usefulness, I would think.\n\nSo here's a patch to do that (textually independent but conceptually on\ntop of the earlier one to fix the commit-buffer thing). It actually\ndoesn't remove that much code, so I could go either way on it.\n\n+cc Linus in case he's secretly in love with this feature.\n\n-- >8 --\nSubject: [PATCH] revision: drop --show-all option\n\nThis was an undocumented debugging aid that does not seem to\nhave come in handy in the past decade, judging from its lack\nof mentions on the mailing list.\n\nLet's drop it in the name of simplicity. This is morally a\nrevert of 3131b71301 (Add \"--show-all\" revision walker flag\nfor debugging, 2008-02-09), but note that I did leave in the\nmapping of UNINTERESTING to \"^\" in get_revision_mark(). I\ndon't think this would be possible to trigger with the\ncurrent code, but it's the only sensible marker.\n\nWe'll skip the usual deprecation period because this was\nexplicitly a debugging aid that was never documented.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n revision.c                           | 10 ---------\n revision.h                           |  1 -\n t/t6015-rev-list-show-all-parents.sh | 31 ----------------------------\n 3 files changed, 42 deletions(-)\n delete mode 100755 t/t6015-rev-list-show-all-parents.sh\n\ndiff --git a/revision.c b/revision.c\nindex 5ce9b93baa..5c1cb7277c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1065,14 +1065,9 @@ static int limit_list(struct rev_info *revs)\n \t\t\treturn -1;\n \t\tif (obj->flags & UNINTERESTING) {\n \t\t\tmark_parents_uninteresting(commit);\n-\t\t\tif (revs->show_all)\n-\t\t\t\tp = &commit_list_insert(commit, p)->next;\n \t\t\tslop = still_interesting(list, date, slop, &interesting_cache);\n \t\t\tif (slop)\n \t\t\t\tcontinue;\n-\t\t\t/* If showing all, add the whole pending list to the end */\n-\t\t\tif (revs->show_all)\n-\t\t\t\t*p = list;\n \t\t\tbreak;\n \t\t}\n \t\tif (revs->min_age != -1 && (commit->date > revs->min_age))\n@@ -1864,8 +1859,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->dense = 1;\n \t} else if (!strcmp(arg, \"--sparse\")) {\n \t\trevs->dense = 0;\n-\t} else if (!strcmp(arg, \"--show-all\")) {\n-\t\trevs->show_all = 1;\n \t} else if (!strcmp(arg, \"--in-commit-order\")) {\n \t\trevs->tree_blobs_in_commit_order = 1;\n \t} else if (!strcmp(arg, \"--remove-empty\")) {\n@@ -3094,8 +3087,6 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n \t\treturn commit_ignore;\n \tif (revs->unpacked && has_sha1_pack(commit->object.oid.hash))\n \t\treturn commit_ignore;\n-\tif (revs->show_all)\n-\t\treturn commit_show;\n \tif (commit->object.flags & UNINTERESTING)\n \t\treturn commit_ignore;\n \tif (revs->min_age != -1 &&\n@@ -3194,7 +3185,6 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n \tenum commit_action action = get_commit_action(revs, commit);\n \n \tif (action == commit_show &&\n-\t    !revs->show_all &&\n \t    revs->prune && revs->dense && want_ancestry(revs)) {\n \t\t/*\n \t\t * --full-diff on simplified parents is no good: it\ndiff --git a/revision.h b/revision.h\nindex 3dee97bfb9..b8c47b98e2 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -90,7 +90,6 @@ struct rev_info {\n \tunsigned int\tdense:1,\n \t\t\tprune:1,\n \t\t\tno_walk:2,\n-\t\t\tshow_all:1,\n \t\t\tremove_empty_trees:1,\n \t\t\tsimplify_history:1,\n \t\t\ttopo_order:1,\ndiff --git a/t/t6015-rev-list-show-all-parents.sh b/t/t6015-rev-list-show-all-parents.sh\ndeleted file mode 100755\nindex 3c73c93ba6..0000000000\n--- a/t/t6015-rev-list-show-all-parents.sh\n+++ /dev/null\n@@ -1,31 +0,0 @@\n-#!/bin/sh\n-\n-test_description='--show-all --parents does not rewrite TREESAME commits'\n-\n-. ./test-lib.sh\n-\n-test_expect_success 'set up --show-all --parents test' '\n-\ttest_commit one foo.txt &&\n-\tcommit1=$(git rev-list -1 HEAD) &&\n-\ttest_commit two bar.txt &&\n-\tcommit2=$(git rev-list -1 HEAD) &&\n-\ttest_commit three foo.txt &&\n-\tcommit3=$(git rev-list -1 HEAD)\n-\t'\n-\n-test_expect_success '--parents rewrites TREESAME parents correctly' '\n-\techo $commit3 $commit1 > expected &&\n-\techo $commit1 >> expected &&\n-\tgit rev-list --parents HEAD -- foo.txt > actual &&\n-\ttest_cmp expected actual\n-\t'\n-\n-test_expect_success '--parents --show-all does not rewrites TREESAME parents' '\n-\techo $commit3 $commit2 > expected &&\n-\techo $commit2 $commit1 >> expected &&\n-\techo $commit1 >> expected &&\n-\tgit rev-list --parents --show-all HEAD -- foo.txt > actual &&\n-\ttest_cmp expected actual\n-\t'\n-\n-test_done\n-- \n2.16.2.555.g885a024879\n\n"},{"id":"339862","messageId":"20180221232939.GE7944@sigill.intra.peff.net","threadId":"47890","inReplyTo":"CAGZ79kZz+hC+eXxa2A-NpnCZa-LikWDaegfijUjHp5CCenwQvA@mail.gmail.com","subject":"Re: [PATCH] commit: drop uses of get_cached_commit_buffer()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-21T23:29:40Z","receivedAt":"2018-02-21T23:29:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2018 at 03:22:02PM -0800, Stefan Beller wrote:\n\n> > Subject: [PATCH] commit: drop uses of get_cached_commit_buffer()\n> > ---\n> >  builtin/rev-list.c | 2 +-\n> >  log-tree.c         | 3 ---\n> >  2 files changed, 1 insertion(+), 4 deletions(-)\n> \n> Now if we'd get around to rewrite pretty.c as well, we could make it static,\n> giving a stronger reason of not using that function. But it looks a bit\n> complicated to me, who is not familiar in that area of the code.\n> \n> Thanks for making less use of this suboptimal API,\n\nI'm not sure the API is suboptimal. It's not wrong to ask \"do you have a\ncached copy of this?\". It was just being used poorly here. :)\n\nSee the discussion in\n\n  https://public-inbox.org/git/20180221184811.GD4333@sigill.intra.peff.net/\n\n-Peff\n"},{"id":"339863","messageId":"20180221233433.GA29992@sigill.intra.peff.net","threadId":"47890","inReplyTo":"07690508-e6c6-5b3b-6c03-5ad83c9c3501@gmail.com","subject":"Re: [PATCH] commit: drop uses of get_cached_commit_buffer()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-21T23:34:33Z","receivedAt":"2018-02-21T23:34:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2018 at 02:19:17PM -0500, Derrick Stolee wrote:\n\n> > These behaviors are undocumented, untested, and unlikely to be\n> > expected by users or other software attempting to parse this output.\n> > \n> > Helped-by: Jeff King <peff@peff.net>\n> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> \n> This would be a good time to allow multiple authors, or to just change the\n> author, since this is exactly the diff you (Peff) provided in an earlier\n> email. The commit message hopefully summarizes our discussion, but I welcome\n> edits.\n\nThe point is moot if we take the revision I just sent (though in\nretrospect I really ought to have put in a Reported-by: for you there).\n\nBut some communities are settling on Co-authored-by as a trailer for\nthis case. And GitHub has started parsing and showing that along with\nauthor information:\n\n  https://github.com/blog/2496-commit-together-with-co-authors\n\n-Peff\n"},{"id":"339865","messageId":"CA+55aFxLU+KVs0s43chYpiw0NhFb=t8QsxotcJSWVrNcx7Z=NQ@mail.gmail.com","threadId":"47890","inReplyTo":"20180221232724.GD7944@sigill.intra.peff.net","subject":"Re: [PATCH] revision: drop --show-all option","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-02-21T23:45:48Z","receivedAt":"2018-02-21T23:45:53Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Feb 21, 2018 at 3:27 PM, Jeff King <peff@peff.net> wrote:\n>\n> We'll skip the usual deprecation period because this was\n> explicitly a debugging aid that was never documented.\n\nAck. I don't think I've used it since, and probably nobody else ever used it.\n\n              Linus\n"},{"id":"339896","messageId":"562f72bd-1917-6c29-97fc-6098bd109d52@gmail.com","threadId":"47890","inReplyTo":"20180221231338.GC7944@sigill.intra.peff.net","subject":"Re: [PATCH] commit: drop uses of get_cached_commit_buffer()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-02-22T01:52:32Z","receivedAt":"2018-02-22T01:52:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/21/2018 6:13 PM, Jeff King wrote:\n> On Wed, Feb 21, 2018 at 02:17:11PM -0500, Derrick Stolee wrote:\n>\n>> The get_cached_commit_buffer() method provides access to the buffer\n>> loaded for a struct commit, if it was ever loadead and was not freed.\n>>\n>> Two places use this to inform how to output information about commits.\n>>\n>> log-tree.c uses this method to short-circuit the output of commit\n>> information when the buffer is not cached. However, this leads to\n>> incorrect output in 'git log --oneline' where the short-OID is written\n>> but then the rest of the commit information is dropped and the next\n>> commit is written on the same line.\n>>\n>> rev-list uses this method for two reasons:\n>>\n>> - First, if the revision walk visits a commit twice, the buffer was\n>>    freed by rev-list in the first write. The output then does not\n>>    match the format expectations, since the OID is written without the\n>>    rest of the content.\n> I'm not sure after my earlier digging if there is even a way to trigger\n> this (and if so, it is probably accidental, since those lines were added\n> explicitly for --show-all).\n>\n> And actually after re-reading the commit message for 3131b7130 again, I\n> think the current behavior is definitely not something that was\n> carefully planned. So I'd propose a commit message like below.\n\nI only submitted my patch to avoid making you do the work of writing the \ncommit message. My messages still don't have quite the right amount of \ndetail (or the correct details, in this case).\n\nJunio: please add\n\nReported-by: Derrick Stolee <dstolee@microsoft.com>\n\nThanks,\n\n-Stolee\n\n\n>\n> -- >8 --\n> Subject: [PATCH] commit: drop uses of get_cached_commit_buffer()\n>\n> The \"--show-all\" revision option shows UNINTERESTING\n> commits. Some of these commits may be unparsed when we try\n> to show them (since we may or may not need to walk their\n> parents to fulfill the request).\n>\n> Commit 3131b71301 (Add \"--show-all\" revision walker flag for\n> debugging, 2008-02-09) resolved this by just skipping\n> pretty-printing for commits without their object contents\n> cached, saying:\n>\n>    Because we now end up listing commits we may not even have been parsed\n>    at all \"show_log\" and \"show_commit\" need to protect against commits\n>    that don't have a commit buffer entry.\n>\n> That was the easy fix to avoid the pretty-printer segfaulting,\n> but:\n>\n>    1. It doesn't work for all formats. E.g., --oneline\n>       prints the oid for each such commit but not a trailing\n>       newline, leading to jumbled output.\n>\n>    2. It only affects some commits, depending on whether we\n>       happened to parse them or not (so if they were at the\n>       tip of an UNINTERESTING starting point, or if we\n>       happened to traverse over them, you'd see more data).\n>\n>    3. It unncessarily ties the decision to show the verbose\n>       header to whether the commit buffer was cached. That\n>       makes it harder to change the logic around caching\n>       (e.g., if we could traverse without actually loading\n>       the full commit objects).\n>\n> These days it's safe to feed such a commit to the\n> pretty-print code. Since be5c9fb904 (logmsg_reencode: lazily\n> load missing commit buffers, 2013-01-26), we'll load it on\n> demand in such a case. So let's just always show the verbose\n> headers.\n>\n> This does change the behavior of plumbing, but:\n>\n>    a. The --show-all option was explicitly introduced as a\n>       debugging aid, and was never documented (and has rarely\n>       even been mentioned on the list by git devs).\n>\n>    b. Avoiding the commits was already not deterministic due\n>       to (2) above. So the caller might have seen full\n>       headers for these commits anyway, and would need to be\n>       prepared for it.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>   builtin/rev-list.c | 2 +-\n>   log-tree.c         | 3 ---\n>   2 files changed, 1 insertion(+), 4 deletions(-)\n>\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 48300d9e11..d320b6f1e3 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -134,7 +134,7 @@ static void show_commit(struct commit *commit, void *data)\n>   \telse\n>   \t\tputchar('\\n');\n>   \n> -\tif (revs->verbose_header && get_cached_commit_buffer(commit, NULL)) {\n> +\tif (revs->verbose_header) {\n>   \t\tstruct strbuf buf = STRBUF_INIT;\n>   \t\tstruct pretty_print_context ctx = {0};\n>   \t\tctx.abbrev = revs->abbrev;\n> diff --git a/log-tree.c b/log-tree.c\n> index fc0cc0d6d1..22b2fb6c58 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -659,9 +659,6 @@ void show_log(struct rev_info *opt)\n>   \t\tshow_mergetag(opt, commit);\n>   \t}\n>   \n> -\tif (!get_cached_commit_buffer(commit, NULL))\n> -\t\treturn;\n> -\n>   \tif (opt->show_notes) {\n>   \t\tint raw;\n>   \t\tstruct strbuf notebuf = STRBUF_INIT;\n"}]}