{"thread":{"id":"46227","subject":"Truncating HEAD reflog on branch move","startedAt":"2017-06-21T21:41:01Z","lastAt":"2017-07-10T16:09:41Z","messageCount":69,"participants":["brian m. carlson","Junio C Hamano","Kyle Meyer","Jeff King","Ramsay Jones","Eric Wong"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"322876","messageId":"20170621213924.wh43i2h7v2zwihq3@genre.crustytoothpaste.net","threadId":"46227","inReplyTo":null,"subject":"Truncating HEAD reflog on branch move","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-06-21T21:40:51Z","receivedAt":"2017-06-21T21:41:01Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"In Git 2.11.0, using git branch -m caused an empty entry to be logged to\nthe reflog, but it preserved the previous history of the default (HEAD)\nreflog. In Git 2.13.1 (and in next and pu), we write a valid entry into\nthe reflog, but truncate it.\n\nSo for the following commands:\n\n git clone https://git.crustytoothpaste.net/git/bmc/homedir.git\n cd homedir\n git checkout cPanel\n git checkout master\n git branch -m master new\n\nI get the following on 2.11.0:\n\n2cbfbd5 HEAD@{0}:\n2cbfbd5 HEAD@{1}: checkout: moving from cPanel to master\neaf8db2 HEAD@{2}: checkout: moving from master to cPanel\n2cbfbd5 HEAD@{3}: clone: from https://bmc@git.crustytoothpaste.net/git/bmc/homedir.git\n\nand this on a recent next:\n\n2cbfbd5 HEAD@{0}: Branch: renamed refs/heads/master to refs/heads/new\n\nFor this test, any repo will work; I just picked this one because it had\ntwo branches I could use to generate dummy reflog entries.\n\nA colleague reported this to me as a bug. I don't see anything in the\nrelease notes about this as a desired behavior change, and it does seem\nundesirable to truncate the reflog this way. If this isn't intentional,\nI'm happy to work up a patch.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"322879","messageId":"xmqqpodxm2t8.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170621213924.wh43i2h7v2zwihq3@genre.crustytoothpaste.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-21T21:46:43Z","receivedAt":"2017-06-21T21:46:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> I get the following on 2.11.0:\n>\n> 2cbfbd5 HEAD@{0}:\n> 2cbfbd5 HEAD@{1}: checkout: moving from cPanel to master\n> eaf8db2 HEAD@{2}: checkout: moving from master to cPanel\n> 2cbfbd5 HEAD@{3}: clone: from https://bmc@git.crustytoothpaste.net/git/bmc/homedir.git\n>\n> and this on a recent next:\n>\n> 2cbfbd5 HEAD@{0}: Branch: renamed refs/heads/master to refs/heads/new\n>\n> For this test, any repo will work; I just picked this one because it had\n> two branches I could use to generate dummy reflog entries.\n>\n> A colleague reported this to me as a bug. I don't see anything in the\n> release notes about this as a desired behavior change, and it does seem\n> undesirable to truncate the reflog this way. If this isn't intentional,\n> I'm happy to work up a patch.\n\nI do not think either behaviour is intentional (old one that gives a\nmeaningless empty entry probably is probably not what we want, the\nnew one that truncates is not what we want, either).\n\n"},{"id":"322896","messageId":"87efud7xjd.fsf@kyleam.com","threadId":"46227","inReplyTo":"xmqqpodxm2t8.fsf@gitster.mtv.corp.google.com","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2017-06-21T23:04:22Z","receivedAt":"2017-06-21T23:04:36Z","isPatch":false,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Junio C Hamano writes:\n\n> \"brian m. carlson\" writes:\n>\n>> I get the following on 2.11.0:\n>>\n>> 2cbfbd5 HEAD@{0}:\n>> 2cbfbd5 HEAD@{1}: checkout: moving from cPanel to master\n>> eaf8db2 HEAD@{2}: checkout: moving from master to cPanel\n>> 2cbfbd5 HEAD@{3}: clone: from https://bmc@git.crustytoothpaste.net/git/bmc/homedir.git\n>>\n>> and this on a recent next:\n>>\n>> 2cbfbd5 HEAD@{0}: Branch: renamed refs/heads/master to refs/heads/new\n>>\n>> For this test, any repo will work; I just picked this one because it had\n>> two branches I could use to generate dummy reflog entries.\n>>\n>> A colleague reported this to me as a bug. I don't see anything in the\n>> release notes about this as a desired behavior change, and it does seem\n>> undesirable to truncate the reflog this way. If this isn't intentional,\n>> I'm happy to work up a patch.\n>\n> I do not think either behaviour is intentional (old one that gives a\n> meaningless empty entry probably is probably not what we want, the\n> new one that truncates is not what we want, either).\n\nEh, sorry about that. I haven't dug very deeply, but it seems like the\nentry is still in .git/logs/HEAD, while \"git reflog\" is only showing\nthey latest entry. (Maybe because we stop consuming the entries when we\nhit a null sha1 in the old id field?)\n\n-- \nKyle\n"},{"id":"322898","messageId":"87a8517x6h.fsf@kyleam.com","threadId":"46227","inReplyTo":"87efud7xjd.fsf@kyleam.com","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2017-06-21T23:12:06Z","receivedAt":"2017-06-21T23:12:20Z","isPatch":false,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Kyle Meyer writes:\n\n> Eh, sorry about that. I haven't dug very deeply, but it seems like the\n> entry is still in .git/logs/HEAD, while \"git reflog\" is only showing\n> they latest entry.\n\ns/entry is still/missing entries are still/\n\n"},{"id":"322932","messageId":"20170622151603.rrh2j7zsotyt2jxe@sigill.intra.peff.net","threadId":"46227","inReplyTo":"87efud7xjd.fsf@kyleam.com","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-22T15:16:03Z","receivedAt":"2017-06-22T15:16:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 21, 2017 at 07:04:22PM -0400, Kyle Meyer wrote:\n\n> Junio C Hamano writes:\n> \n> > \"brian m. carlson\" writes:\n> >\n> >> I get the following on 2.11.0:\n> >>\n> >> 2cbfbd5 HEAD@{0}:\n> >> 2cbfbd5 HEAD@{1}: checkout: moving from cPanel to master\n> >> eaf8db2 HEAD@{2}: checkout: moving from master to cPanel\n> >> 2cbfbd5 HEAD@{3}: clone: from https://bmc@git.crustytoothpaste.net/git/bmc/homedir.git\n> >>\n> >> and this on a recent next:\n> >>\n> >> 2cbfbd5 HEAD@{0}: Branch: renamed refs/heads/master to refs/heads/new\n> >>\n> >> For this test, any repo will work; I just picked this one because it had\n> >> two branches I could use to generate dummy reflog entries.\n> >>\n> >> A colleague reported this to me as a bug. I don't see anything in the\n> >> release notes about this as a desired behavior change, and it does seem\n> >> undesirable to truncate the reflog this way. If this isn't intentional,\n> >> I'm happy to work up a patch.\n> >\n> > I do not think either behaviour is intentional (old one that gives a\n> > meaningless empty entry probably is probably not what we want, the\n> > new one that truncates is not what we want, either).\n> \n> Eh, sorry about that. I haven't dug very deeply, but it seems like the\n> entry is still in .git/logs/HEAD, while \"git reflog\" is only showing\n> they latest entry. (Maybe because we stop consuming the entries when we\n> hit a null sha1 in the old id field?)\n\nYeah, I think that's it.\n\nI had trouble digging to find the place where this happens, but I\n_think_ it's unintentional, and is an artifact of the way the reflog\niteration is bolted onto the regular revision machinery. In\nget_revision_1(), we fake the commit parents as if the earlier reflog\nentries formed an actual commit history.\n\nThere's some magic in fake_reflog_parent() when we hit an entry with a\nnull sha1 in the old-oid field. But it depends on looking at the new-oid\nfield of the entry before. And because our entries look like this:\n\n real_sha1 null_sha1 ...\n null_sha1 real_sha1 ...\n\nLooking at the prior entry's new-oid field doesn't help at all. And we\nend up claiming that there are no parents, and this is the end of the\ntraversal. The patch below makes it work for me by falling back further\nto the previous entry's old oid. This feels like a terrible hack to make\nthis case work. I'd think we'd actually want some kind of loop to keep\nlooking. After all, what should:\n\n real_sha1 real_sha1 ...\n null_sha1 null_sha1 ...\n null_sha1 real_sha1 ...\n\nshow? Surely we should not stop traversing when we hit the double\nnull_sha1. We should either display it in some form, or skip it and\nissue a warning that there's a funky entry in your reflog.\n\nThis whole fake-parents things does just feel like a gigantic hack,\nthough. It seems like we should be able to just walk backwards down the\nreflog list and show the entries. The revision machinery already\nspecial-cases a bunch of reflog-walk bits; I don't know that adding one\nor two more would be the end of the world.\n\nAnother approach entirely would be to stop generating two entries in the\nreflog, and generate a single one:\n\n real_sha1 real_sha1 ... Rename from \"foo\" to \"bar\"\n\nI think we talked about that earlier, but I think the resolution was\njust that doing so was hard with the existing ref-update calls.\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex ed99437ad..b7e489ad3 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \t\t/* a root commit, but there are still more entries to show */\n \t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n \t\tlogobj = parse_object(&reflog->noid);\n+\t\tif (!logobj)\n+\t\t\tlogobj = parse_object(&reflog->ooid);\n \t}\n \n \tif (!logobj || logobj->type != OBJ_COMMIT) {\n\n"},{"id":"322940","messageId":"xmqqziczkh4k.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170622151603.rrh2j7zsotyt2jxe@sigill.intra.peff.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-22T18:32:43Z","receivedAt":"2017-06-22T18:32:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> There's some magic in fake_reflog_parent() when we hit an entry with a\n> ...\n> This whole fake-parents things does just feel like a gigantic hack,\n> though.\n> ...\n> It seems like we should be able to just walk backwards down the\n> reflog list and show the entries. The revision machinery already\n> special-cases a bunch of reflog-walk bits; I don't know that adding one\n> or two more would be the end of the world.\n\nUnfortunate but I tend to agree that at least such an addition would\nmake the \"gigantic hack\" a bit more complete one ;-)\n\n> diff --git a/reflog-walk.c b/reflog-walk.c\n> index ed99437ad..b7e489ad3 100644\n> --- a/reflog-walk.c\n> +++ b/reflog-walk.c\n> @@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n> \t\t/* a root commit, but there are still more entries to show */\n> \t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n> \t\tlogobj = parse_object(&reflog->noid);\n> +\t\tif (!logobj)\n> +\t\t\tlogobj = parse_object(&reflog->ooid);\n> \t}\n> \n> \tif (!logobj || logobj->type != OBJ_COMMIT) {\n\nWe already have a loop to find an entry that is a commit that\ndiscards any non-commit object before the pre-context of this hunk.\nThis \"oops, old side is NULL so let's cover it up by using the new\nside\" kicks in after that. I wonder if we can roll that cover-up\nlogic into the loop, perhaps like\n\n\tdo {\n\t\treflog = &commit_reflog->...[recno];\n\t\tcommit_reflog->recno--;\n-\t\tlogobj = parse_object(&reflog->ooid);\n+\t\tlogobj = parse_object(is_null_oid(&reflog->ooid) \n+\t\t\t\t? &reflog->noid : &reflog->ooid);\n-\t} while (commit_reflog->recno && (logobj && logobj->type != OBJ_COMMIT));\n+\t} while (commit_reflog->recno && (!logobj || logobj->type != OBJ_COMMIT));\n-\n-\tif (!logobj && commit_reflog->recno >= 0 && is_null_oid(&reflog->ooid)) {\n-\t\t/* a root commit ... */\n-\t\treflog = &commit_reflog->...[recno];\n-\t\tlogobj = parse_object(&reflog->noid);\n-\t}\n\nwhich may deal with your \"both old and new sides were NULL\" case\nbetter.\n\n\n"},{"id":"322962","messageId":"20170622184516.kq3y7nxwohm3coq4@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqqziczkh4k.fsf@gitster.mtv.corp.google.com","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-22T18:45:16Z","receivedAt":"2017-06-22T18:45:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 22, 2017 at 11:32:43AM -0700, Junio C Hamano wrote:\n\n> > diff --git a/reflog-walk.c b/reflog-walk.c\n> > index ed99437ad..b7e489ad3 100644\n> > --- a/reflog-walk.c\n> > +++ b/reflog-walk.c\n> > @@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n> > \t\t/* a root commit, but there are still more entries to show */\n> > \t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n> > \t\tlogobj = parse_object(&reflog->noid);\n> > +\t\tif (!logobj)\n> > +\t\t\tlogobj = parse_object(&reflog->ooid);\n> > \t}\n> > \n> > \tif (!logobj || logobj->type != OBJ_COMMIT) {\n> \n> We already have a loop to find an entry that is a commit that\n> discards any non-commit object before the pre-context of this hunk.\n> This \"oops, old side is NULL so let's cover it up by using the new\n> side\" kicks in after that. I wonder if we can roll that cover-up\n> logic into the loop, perhaps like\n\nYeah, I tried making the second conditional its own loop, but I agree it\nprobably ought to be part of a single loop looking for a sane parent\nentry.\n\n> \tdo {\n> \t\treflog = &commit_reflog->...[recno];\n> \t\tcommit_reflog->recno--;\n> -\t\tlogobj = parse_object(&reflog->ooid);\n> +\t\tlogobj = parse_object(is_null_oid(&reflog->ooid) \n> +\t\t\t\t? &reflog->noid : &reflog->ooid);\n> -\t} while (commit_reflog->recno && (logobj && logobj->type != OBJ_COMMIT));\n> +\t} while (commit_reflog->recno && (!logobj || logobj->type != OBJ_COMMIT));\n> -\n> -\tif (!logobj && commit_reflog->recno >= 0 && is_null_oid(&reflog->ooid)) {\n> -\t\t/* a root commit ... */\n> -\t\treflog = &commit_reflog->...[recno];\n> -\t\tlogobj = parse_object(&reflog->noid);\n> -\t}\n\nI don't think this is quite right, though. We've decremented \"recno\"\nafter assigning the old pointer to \"reflog\". So in the existing code,\n\"reflog\" in that second conditional pointing to the _next_ entry (or\nprevious, really, since we are going in reverse order).\n\nSo I think you'd need to look at commit->reflog again (after checking\nthat we didn't go past the start of the array).\n\n-Peff\n"},{"id":"322967","messageId":"xmqqvannkfp8.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170622184516.kq3y7nxwohm3coq4@sigill.intra.peff.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-22T19:03:31Z","receivedAt":"2017-06-22T19:03:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't think this is quite right, though. We've decremented \"recno\"\n> after assigning the old pointer to \"reflog\". So in the existing code,\n> \"reflog\" in that second conditional pointing to the _next_ entry (or\n> previous, really, since we are going in reverse order).\n>\n> So I think you'd need to look at commit->reflog again (after checking\n> that we didn't go past the start of the array).\n\nPerhaps. I did the illustration that way simply because I was not\nsure if the current \"the entry was NULL from something new, so skip\nand look at the previous entry's new\" was correct to begin with.\n\n"},{"id":"322982","messageId":"20170622202146.cxrkjca636xl4dgk@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqqvannkfp8.fsf@gitster.mtv.corp.google.com","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-22T20:21:46Z","receivedAt":"2017-06-22T20:21:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 22, 2017 at 12:03:31PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I don't think this is quite right, though. We've decremented \"recno\"\n> > after assigning the old pointer to \"reflog\". So in the existing code,\n> > \"reflog\" in that second conditional pointing to the _next_ entry (or\n> > previous, really, since we are going in reverse order).\n> >\n> > So I think you'd need to look at commit->reflog again (after checking\n> > that we didn't go past the start of the array).\n> \n> Perhaps. I did the illustration that way simply because I was not\n> sure if the current \"the entry was NULL from something new, so skip\n> and look at the previous entry's new\" was correct to begin with.\n\nI'm not sure it makes sense for an entry to have its \"after\" state as\nits own parent. On the other hand, I'm not sure it makes sense to\nconsider this fake parentage in the first place.\n\nI think in the old days we used truly rewrite the parents, and traversal\nlike \"git log -g -p\" would show the diff between reflog entries. But\nthat changed in 838f9a156 (log: use true parents for diff when walking\nreflogs, 2013-08-03). Given that we disable reflog-walking with things\nlike \"--graph\" that show the relationship, are the fake parents actually\naccomplishing anything anymore? The parents we show via \"log -g\n--parents\" seem like nonsense, and I'm not sure how we would do any\nhistory simplification based no the munged values.\n\nSo I'd be tempted to just ditch the whole thing and teach\nget_revision_1() to just walk through the list of logs, rather than this\nweird \"add a pending commit and then try to figure out which reflog it\nreferred to\". For instance, right now:\n\n git log -g HEAD $(git symbolic-ref HEAD)\n\nonly shows _one_ reflog. The patch below is the direction I'm thinking.\nIt fails two tests, but haven't dug yet.\n\n---\n reflog-walk.c | 112 +++++++++--------------------------\n reflog-walk.h | 4 +-\n revision.c | 24 ++++----\n 3 files changed, 43 insertions(+), 97 deletions(-)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex ed99437ad..77170a3cb 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -78,45 +78,6 @@ static int get_reflog_recno_by_time(struct complete_reflogs *array,\n \treturn -1;\n }\n \n-struct commit_info_lifo {\n-\tstruct commit_info {\n-\t\tstruct commit *commit;\n-\t\tvoid *util;\n-\t} *items;\n-\tint nr, alloc;\n-};\n-\n-static struct commit_info *get_commit_info(struct commit *commit,\n-\t\tstruct commit_info_lifo *lifo, int pop)\n-{\n-\tint i;\n-\tfor (i = 0; i < lifo->nr; i++)\n-\t\tif (lifo->items[i].commit == commit) {\n-\t\t\tstruct commit_info *result = &lifo->items[i];\n-\t\t\tif (pop) {\n-\t\t\t\tif (i + 1 < lifo->nr)\n-\t\t\t\t\tmemmove(lifo->items + i,\n-\t\t\t\t\t\tlifo->items + i + 1,\n-\t\t\t\t\t\t(lifo->nr - i) *\n-\t\t\t\t\t\tsizeof(struct commit_info));\n-\t\t\t\tlifo->nr--;\n-\t\t\t}\n-\t\t\treturn result;\n-\t\t}\n-\treturn NULL;\n-}\n-\n-static void add_commit_info(struct commit *commit, void *util,\n-\t\tstruct commit_info_lifo *lifo)\n-{\n-\tstruct commit_info *info;\n-\tALLOC_GROW(lifo->items, lifo->nr + 1, lifo->alloc);\n-\tinfo = lifo->items + lifo->nr;\n-\tinfo->commit = commit;\n-\tinfo->util = util;\n-\tlifo->nr++;\n-}\n-\n struct commit_reflog {\n \tint recno;\n \tenum selector_type {\n@@ -128,7 +89,8 @@ struct commit_reflog {\n };\n \n struct reflog_walk_info {\n-\tstruct commit_info_lifo reflogs;\n+\tstruct commit_reflog **logs;\n+\tsize_t nr, alloc, cur;\n \tstruct string_list complete_reflogs;\n \tstruct commit_reflog *last_commit_reflog;\n };\n@@ -226,50 +188,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n \tcommit_reflog->selector = selector;\n \tcommit_reflog->reflogs = reflogs;\n \n-\tadd_commit_info(commit, commit_reflog, &info->reflogs);\n-\treturn 0;\n-}\n-\n-void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n-{\n-\tstruct commit_info *commit_info =\n-\t\tget_commit_info(commit, &info->reflogs, 0);\n-\tstruct commit_reflog *commit_reflog;\n-\tstruct object *logobj;\n-\tstruct reflog_info *reflog;\n-\n-\tinfo->last_commit_reflog = NULL;\n-\tif (!commit_info)\n-\t\treturn;\n+\tALLOC_GROW(info->logs, info->nr + 1, info->alloc);\n+\tinfo->logs[info->nr++] = commit_reflog;\n \n-\tcommit_reflog = commit_info->util;\n-\tif (commit_reflog->recno < 0) {\n-\t\tcommit->parents = NULL;\n-\t\treturn;\n-\t}\n-\tinfo->last_commit_reflog = commit_reflog;\n-\n-\tdo {\n-\t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n-\t\tcommit_reflog->recno--;\n-\t\tlogobj = parse_object(&reflog->ooid);\n-\t} while (commit_reflog->recno && (logobj && logobj->type != OBJ_COMMIT));\n-\n-\tif (!logobj && commit_reflog->recno >= 0 && is_null_oid(&reflog->ooid)) {\n-\t\t/* a root commit, but there are still more entries to show */\n-\t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n-\t\tlogobj = parse_object(&reflog->noid);\n-\t}\n-\n-\tif (!logobj || logobj->type != OBJ_COMMIT) {\n-\t\tcommit_info->commit = NULL;\n-\t\tcommit->parents = NULL;\n-\t\treturn;\n-\t}\n-\tcommit_info->commit = (struct commit *)logobj;\n-\n-\tcommit->parents = xcalloc(1, sizeof(struct commit_list));\n-\tcommit->parents->item = commit_info->commit;\n+\treturn 0;\n }\n \n void get_reflog_selector(struct strbuf *sb,\n@@ -356,3 +278,27 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n \t\tstrbuf_release(&selector);\n \t}\n }\n+\n+struct commit *next_entry_in_log(struct commit_reflog *log)\n+{\n+\twhile (log->recno >= 0) {\n+\t\tstruct reflog_info *entry = &log->reflogs->items[log->recno--];\n+\t\tstruct object *obj = parse_object(&entry->noid);\n+\n+\t\tif (obj && obj->type == OBJ_COMMIT)\n+\t\t\treturn (struct commit *)obj;\n+\t}\n+\treturn NULL;\n+}\n+\n+struct commit *next_reflog_entry(struct reflog_walk_info *walk)\n+{\n+\tfor (; walk->cur < walk->nr; walk->cur++) {\n+\t\tstruct commit *ret = next_entry_in_log(walk->logs[walk->cur]);\n+\t\tif (ret) {\n+\t\t\twalk->last_commit_reflog = walk->logs[walk->cur];\n+\t\t\treturn ret;\n+\t\t}\n+\t}\n+\treturn NULL;\n+}\ndiff --git a/reflog-walk.h b/reflog-walk.h\nindex 27886f793..5cc6b7bd4 100644\n--- a/reflog-walk.h\n+++ b/reflog-walk.h\n@@ -8,8 +8,6 @@ struct reflog_walk_info;\n extern void init_reflog_walk(struct reflog_walk_info **info);\n extern int add_reflog_for_walk(struct reflog_walk_info *info,\n \t\tstruct commit *commit, const char *name);\n-extern void fake_reflog_parent(struct reflog_walk_info *info,\n-\t\tstruct commit *commit);\n extern void show_reflog_message(struct reflog_walk_info *info, int,\n \t\t\t\tconst struct date_mode *, int force_date);\n extern void get_reflog_message(struct strbuf *sb,\n@@ -20,4 +18,6 @@ extern void get_reflog_selector(struct strbuf *sb,\n \t\tconst struct date_mode *dmode, int force_date,\n \t\tint shorten);\n \n+struct commit *next_reflog_entry(struct reflog_walk_info *reflog_info);\n+\n #endif\ndiff --git a/revision.c b/revision.c\nindex 12eb33235..675247cd9 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -148,16 +148,14 @@ static void add_pending_object_with_path(struct rev_info *revs,\n \tif (revs->reflog_info && obj->type == OBJ_COMMIT) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint len = interpret_branch_name(name, 0, &buf, 0);\n-\t\tint st;\n \n \t\tif (0 < len && name[len] && buf.len)\n \t\t\tstrbuf_addstr(&buf, name + len);\n-\t\tst = add_reflog_for_walk(revs->reflog_info,\n-\t\t\t\t\t (struct commit *)obj,\n-\t\t\t\t\t buf.buf[0] ? buf.buf: name);\n+\t\tadd_reflog_for_walk(revs->reflog_info,\n+\t\t\t\t (struct commit *)obj,\n+\t\t\t\t buf.buf[0] ? buf.buf: name);\n \t\tstrbuf_release(&buf);\n-\t\tif (st)\n-\t\t\treturn;\n+\t\treturn; /* do not add the commit itself */\n \t}\n \tadd_object_array_with_path(obj, name, &revs->pending, mode, path);\n }\n@@ -3114,18 +3112,20 @@ static void track_linear(struct rev_info *revs, struct commit *commit)\n \n static struct commit *get_revision_1(struct rev_info *revs)\n {\n+\tif (revs->reflog_info) {\n+\t\tstruct commit *commit = next_reflog_entry(revs->reflog_info);\n+\t\tif (commit) {\n+\t\t\tcommit->object.flags &= ~(ADDED | SEEN | SHOWN);\n+\t\t\treturn commit;\n+\t\t}\n+\t}\n+\n \tif (!revs->commits)\n \t\treturn NULL;\n \n \tdo {\n \t\tstruct commit *commit = pop_commit(&revs->commits);\n \n-\t\tif (revs->reflog_info) {\n-\t\t\tsave_parents(revs, commit);\n-\t\t\tfake_reflog_parent(revs->reflog_info, commit);\n-\t\t\tcommit->object.flags &= ~(ADDED | SEEN | SHOWN);\n-\t\t}\n-\n \t\t/*\n \t\t * If we haven't done the list limiting, we need to look at\n \t\t * the parents here. We also need to do the date-based limiting\n"},{"id":"322985","messageId":"xmqqd19vix03.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170622202146.cxrkjca636xl4dgk@sigill.intra.peff.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-22T20:32:44Z","receivedAt":"2017-06-22T20:32:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I'd be tempted to just ditch the whole thing and teach\n> get_revision_1() to just walk through the list of logs, rather than this\n> weird \"add a pending commit and then try to figure out which reflog it\n> referred to\". For instance, right now:\n>\n> git log -g HEAD $(git symbolic-ref HEAD)\n>\n> only shows _one_ reflog. The patch below is the direction I'm thinking.\n> It fails two tests, but haven't dug yet.\n>\n> ---\n> reflog-walk.c | 112 +++++++++--------------------------\n> reflog-walk.h | 4 +-\n> revision.c | 24 ++++----\n> 3 files changed, 43 insertions(+), 97 deletions(-)\n\nYeah, I agree with the \"we now show diffs with true parents\"\nreasoning, and I like the above code reduction, obviously ;-)\n\n> @@ -3114,18 +3112,20 @@ static void track_linear(struct rev_info *revs, struct commit *commit)\n> \n> static struct commit *get_revision_1(struct rev_info *revs)\n> {\n> +\tif (revs->reflog_info) {\n> +\t\tstruct commit *commit = next_reflog_entry(revs->reflog_info);\n> +\t\tif (commit) {\n> +\t\t\tcommit->object.flags &= ~(ADDED | SEEN | SHOWN);\n> +\t\t\treturn commit;\n> +\t\t}\n> +\t}\n> +\n> \tif (!revs->commits)\n> \t\treturn NULL;\n> \n> \tdo {\n> \t\tstruct commit *commit = pop_commit(&revs->commits);\n> \n> -\t\tif (revs->reflog_info) {\n> -\t\t\tsave_parents(revs, commit);\n> -\t\t\tfake_reflog_parent(revs->reflog_info, commit);\n> -\t\t\tcommit->object.flags &= ~(ADDED | SEEN | SHOWN);\n> -\t\t}\n> -\n> \t\t/*\n> \t\t * If we haven't done the list limiting, we need to look at\n> \t\t * the parents here. We also need to do the date-based limiting\n\nThis part of the patch I can 100% agree with ;-) \n\nI do not think command line parser does not allow \"log -g\nmaint..master\" so all the \"limited\" processing the remainder of\nget_revision_1() does shouldn't matter.\n\nI however think pathspec will affect simplify_commit() and suspect\nthat \"git log -g -20 HEAD path\" will behave differently. Perhaps\nthe difference is \"it used to use path in an unexplainable way, now\nit ignores\", in which case this is an improvement.\n\nThanks.\n\n\n\n\n\n"},{"id":"323003","messageId":"20170622215235.to6yleo3adt5klv2@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqqd19vix03.fsf@gitster.mtv.corp.google.com","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-22T21:52:36Z","receivedAt":"2017-06-22T21:52:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 22, 2017 at 01:32:44PM -0700, Junio C Hamano wrote:\n\n> I do not think command line parser does not allow \"log -g\n> maint..master\" so all the \"limited\" processing the remainder of\n> get_revision_1() does shouldn't matter.\n\nYeah, I don't think negative endpoints work at all, and \"foo...bar\"\nseems to also break (though with a confusing message). It seems clear to\nme that multiple positive endpoints don't work well either, if they have\noverlapping commits.\n\n> I however think pathspec will affect simplify_commit() and suspect\n> that \"git log -g -20 HEAD path\" will behave differently. Perhaps\n> the difference is \"it used to use path in an unexplainable way, now\n> it ignores\", in which case this is an improvement.\n\nThe current behavior there does seem like nonsense, because it's based\non the fake parents. For instance, if I set up a simple two-branch case:\n\n commit() {\n\techo \"$1\" >\"$1\" && git add \"$1\" && git commit -m \"$1\"\n }\n\n git init repo\n cd repo\n commit base\n commit master\n git checkout -b side HEAD^\n commit side\n git merge --no-edit master\n commit combined\n\nThen I get:\n\n $ git log -g --oneline --name-status -- master\n f06c3cd HEAD@{1}: merge master: Merge made by the 'recursive' strategy.\n 5bf12c4 HEAD@{3}: checkout: moving from master to side\n dfa408b HEAD@{4}: commit: master\n A master\n\nEven though only one of those commits touched master. But with my patch,\nit's also somewhat confusing. We ignore the pathspec when picking which\ncommits to show, but still apply it for diffing. So:\n\n 03cf1ad HEAD@{0}: commit: combined\n f06c3cd HEAD@{1}: merge master: Merge made by the 'recursive' strategy.\n 277042b HEAD@{2}: commit: side\n 5bf12c4 HEAD@{3}: checkout: moving from master to side\n dfa408b HEAD@{4}: commit: master\n A master\n 5bf12c4 HEAD@{5}: commit (initial): base\n\nI think we'd want to just omit any entries that are TREESAME to their\nparents. We don't actually care about true parent rewriting (since we're\nnot walking the parents), but if it happened as a side effect that would\nprobably be OK.\n\nIt looks like simplify_commit() is also where we apply bits like\n--no-merges, which doesn't work with my patch. It _also_ behaves\nnonsensically in the current code, because of course the fake reflog\nparents are never merges.\n\nWhich really makes me feel like this patch is going in the right\ndirection, as it makes all of this behave conceptually like:\n\n while read old new etc ...\n do\n git show $new\n done <.git/logs/$ref\n\nwhich is simple to explain and is what I'd expect (and is certainly the\ndirection we went with the \"diff uses real parents\" commit).\n\nWe just need to hit the simplify_commit() code path here.\n\n-Peff\n"},{"id":"323012","messageId":"20170622222545.yewnynklle24ebtf@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170622215235.to6yleo3adt5klv2@sigill.intra.peff.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-22T22:25:46Z","receivedAt":"2017-06-22T22:25:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 22, 2017 at 05:52:35PM -0400, Jeff King wrote:\n\n> Which really makes me feel like this patch is going in the right\n> direction, as it makes all of this behave conceptually like:\n> \n> while read old new etc ...\n> do\n> git show $new\n> done <.git/logs/$ref\n> \n> which is simple to explain and is what I'd expect (and is certainly the\n> direction we went with the \"diff uses real parents\" commit).\n> \n> We just need to hit the simplify_commit() code path here.\n\nSo here's a patch on top of what I posted before that pushes the reflog\ncheck into the loop (it just decides whether to pull from the reflogs or\nfrom the commit queue at the top of the loop).\n\nI was surprised to find, though, that simplify_commit() does not\nactually do the pathspec limiting! It's done by\ntry_to_simplify_commit(), which is part of add_parents_to_list(). I\nhacked around it in the later part of the loop by calling\ntry_to_simplify myself and checking the TREESAME flag. But I have a\nfeeling I'm missing something about how the traversal is supposed to\nwork.\n\nThis does behave sensibly with \"--no-merges\" and \"--merges\", as well as\npathspec limiting.\n\ndiff --git a/revision.c b/revision.c\nindex 675247cd9..203468ddf 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3112,19 +3112,19 @@ static void track_linear(struct rev_info *revs, struct commit *commit)\n \n static struct commit *get_revision_1(struct rev_info *revs)\n {\n-\tif (revs->reflog_info) {\n-\t\tstruct commit *commit = next_reflog_entry(revs->reflog_info);\n-\t\tif (commit) {\n-\t\t\tcommit->object.flags &= ~(ADDED | SEEN | SHOWN);\n-\t\t\treturn commit;\n-\t\t}\n-\t}\n+\twhile (1) {\n+\t\tstruct commit *commit;\n \n-\tif (!revs->commits)\n-\t\treturn NULL;\n+\t\tif (revs->reflog_info)\n+\t\t\tcommit = next_reflog_entry(revs->reflog_info);\n+\t\telse\n+\t\t\tcommit = pop_commit(&revs->commits);\n \n-\tdo {\n-\t\tstruct commit *commit = pop_commit(&revs->commits);\n+\t\tif (!commit)\n+\t\t\treturn NULL;\n+\n+\t\tif (revs->reflog_info)\n+\t\t\tcommit->object.flags &= ~(ADDED | SEEN | SHOWN);\n \n \t\t/*\n \t\t * If we haven't done the list limiting, we need to look at\n@@ -3135,7 +3135,8 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\t\tif (revs->max_age != -1 &&\n \t\t\t (commit->date < revs->max_age))\n \t\t\t\tcontinue;\n-\t\t\tif (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {\n+\t\t\tif (!revs->reflog_info &&\n+\t\t\t add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {\n \t\t\t\tif (!revs->ignore_missing_links)\n \t\t\t\t\tdie(\"Failed to traverse parents of commit %s\",\n \t\t\t\t\t\toid_to_hex(&commit->object.oid));\n@@ -3151,10 +3152,14 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\tdefault:\n \t\t\tif (revs->track_linear)\n \t\t\t\ttrack_linear(revs, commit);\n+\t\t\tif (revs->reflog_info) {\n+\t\t\t\ttry_to_simplify_commit(revs, commit);\n+\t\t\t\tif (commit->object.flags & TREESAME)\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\treturn commit;\n \t\t}\n-\t} while (revs->commits);\n-\treturn NULL;\n+\t}\n }\n \n /*\n"},{"id":"323051","messageId":"20170623031315.7aw5qd7c4wdqlyf6@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170622222545.yewnynklle24ebtf@sigill.intra.peff.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-23T03:13:15Z","receivedAt":"2017-06-23T03:13:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 22, 2017 at 06:25:46PM -0400, Jeff King wrote:\n\n> So here's a patch on top of what I posted before that pushes the reflog\n> check into the loop (it just decides whether to pull from the reflogs or\n> from the commit queue at the top of the loop).\n> \n> I was surprised to find, though, that simplify_commit() does not\n> actually do the pathspec limiting! It's done by\n> try_to_simplify_commit(), which is part of add_parents_to_list(). I\n> hacked around it in the later part of the loop by calling\n> try_to_simplify myself and checking the TREESAME flag. But I have a\n> feeling I'm missing something about how the traversal is supposed to\n> work.\n> \n> This does behave sensibly with \"--no-merges\" and \"--merges\", as well as\n> pathspec limiting.\n\nAnd here's one more patch on top of those that's necessary to get the\ntests to pass (I don't expect anybody to necessarily be applying this\nslow string of patches; it's just to show the direction I'm looking in).\n\n---\n builtin/log.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 998437b23..512538479 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -373,9 +373,9 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\tif (!rev->reflog_info) {\n \t\t\t/* we allow cycles in reflog ancestry */\n \t\t\tfree_commit_buffer(commit);\n+\t\t\tfree_commit_list(commit->parents);\n+\t\t\tcommit->parents = NULL;\n \t\t}\n-\t\tfree_commit_list(commit->parents);\n-\t\tcommit->parents = NULL;\n \t\tif (saved_nrl < rev->diffopt.needed_rename_limit)\n \t\t\tsaved_nrl = rev->diffopt.needed_rename_limit;\n \t\tif (rev->diffopt.degraded_cc_to_c)\n"},{"id":"323806","messageId":"20170704195806.ndbykl776t3vigya@genre.crustytoothpaste.net","threadId":"46227","inReplyTo":"20170623031315.7aw5qd7c4wdqlyf6@sigill.intra.peff.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-07-04T19:58:06Z","receivedAt":"2017-07-04T19:58:26Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Jun 22, 2017 at 11:13:15PM -0400, Jeff King wrote:\n> On Thu, Jun 22, 2017 at 06:25:46PM -0400, Jeff King wrote:\n> \n> > So here's a patch on top of what I posted before that pushes the reflog\n> > check into the loop (it just decides whether to pull from the reflogs or\n> > from the commit queue at the top of the loop).\n> > \n> > I was surprised to find, though, that simplify_commit() does not\n> > actually do the pathspec limiting! It's done by\n> > try_to_simplify_commit(), which is part of add_parents_to_list(). I\n> > hacked around it in the later part of the loop by calling\n> > try_to_simplify myself and checking the TREESAME flag. But I have a\n> > feeling I'm missing something about how the traversal is supposed to\n> > work.\n> > \n> > This does behave sensibly with \"--no-merges\" and \"--merges\", as well as\n> > pathspec limiting.\n> \n> And here's one more patch on top of those that's necessary to get the\n> tests to pass (I don't expect anybody to necessarily be applying this\n> slow string of patches; it's just to show the direction I'm looking in).\n\nI've looked at your original patch, which modified reflog-walk.c, and it\ndoes fix the issue. I'm happy to send in a patch with that and a test\n(provided you're okay with me adding your sign-off), or if you wanted to\nsend in something a bit more complete, like the series of patches here,\nthat's fine, too.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"323809","messageId":"20170704212408.xy6jciggoueq6qsu@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170704195806.ndbykl776t3vigya@genre.crustytoothpaste.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-04T21:24:08Z","receivedAt":"2017-07-04T21:24:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 04, 2017 at 07:58:06PM +0000, brian m. carlson wrote:\n\n> > And here's one more patch on top of those that's necessary to get the\n> > tests to pass (I don't expect anybody to necessarily be applying this\n> > slow string of patches; it's just to show the direction I'm looking in).\n> \n> I've looked at your original patch, which modified reflog-walk.c, and it\n> does fix the issue. I'm happy to send in a patch with that and a test\n> (provided you're okay with me adding your sign-off), or if you wanted to\n> send in something a bit more complete, like the series of patches here,\n> that's fine, too.\n\nI've been on vacation for the past week, but wrapping this up is on my\ntodo. I'll try to get to it tonight.\n\n-Peff\n"},{"id":"323810","messageId":"20170704212709.vqymnj6kkz3prwty@genre.crustytoothpaste.net","threadId":"46227","inReplyTo":"20170704212408.xy6jciggoueq6qsu@sigill.intra.peff.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-07-04T21:27:09Z","receivedAt":"2017-07-04T21:27:20Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jul 04, 2017 at 05:24:08PM -0400, Jeff King wrote:\n> On Tue, Jul 04, 2017 at 07:58:06PM +0000, brian m. carlson wrote:\n> > I've looked at your original patch, which modified reflog-walk.c, and it\n> > does fix the issue. I'm happy to send in a patch with that and a test\n> > (provided you're okay with me adding your sign-off), or if you wanted to\n> > send in something a bit more complete, like the series of patches here,\n> > that's fine, too.\n> \n> I've been on vacation for the past week, but wrapping this up is on my\n> todo. I'll try to get to it tonight.\n\nOkay, if it's on your radar, that's fine. Certainly you should enjoy\nyour vacation and not feel obligated to work on Git unless you want to.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"323811","messageId":"20170704212849.n4bvp7crhd7p7d7w@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170704212709.vqymnj6kkz3prwty@genre.crustytoothpaste.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-04T21:28:49Z","receivedAt":"2017-07-04T21:28:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 04, 2017 at 09:27:09PM +0000, brian m. carlson wrote:\n\n> On Tue, Jul 04, 2017 at 05:24:08PM -0400, Jeff King wrote:\n> > On Tue, Jul 04, 2017 at 07:58:06PM +0000, brian m. carlson wrote:\n> > > I've looked at your original patch, which modified reflog-walk.c, and it\n> > > does fix the issue. I'm happy to send in a patch with that and a test\n> > > (provided you're okay with me adding your sign-off), or if you wanted to\n> > > send in something a bit more complete, like the series of patches here,\n> > > that's fine, too.\n> > \n> > I've been on vacation for the past week, but wrapping this up is on my\n> > todo. I'll try to get to it tonight.\n> \n> Okay, if it's on your radar, that's fine. Certainly you should enjoy\n> your vacation and not feel obligated to work on Git unless you want to.\n\nHeh, no, \"tonight\" is the first work day after the vacation ends. Once\nthe fireworks are done, it's back to work. ;)\n\n-Peff\n"},{"id":"323816","messageId":"xmqqmv8jk5zm.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170704212408.xy6jciggoueq6qsu@sigill.intra.peff.net","subject":"Re: Truncating HEAD reflog on branch move","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-05T01:49:33Z","receivedAt":"2017-07-05T01:49:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jul 04, 2017 at 07:58:06PM +0000, brian m. carlson wrote:\n>\n>> > And here's one more patch on top of those that's necessary to get the\n>> > tests to pass (I don't expect anybody to necessarily be applying this\n>> > slow string of patches; it's just to show the direction I'm looking in).\n>> \n>> I've looked at your original patch, which modified reflog-walk.c, and it\n>> does fix the issue. I'm happy to send in a patch with that and a test\n>> (provided you're okay with me adding your sign-off), or if you wanted to\n>> send in something a bit more complete, like the series of patches here,\n>> that's fine, too.\n>\n> I've been on vacation for the past week, but wrapping this up is on my\n> todo. I'll try to get to it tonight.\n\n;-) Welcome back. \n\nHopefully I haven't advanced topics to 'next' that are sub-par\nduring your absence.\n\n\n"},{"id":"323822","messageId":"20170705075508.c5ul23vivzpklpy6@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170704212408.xy6jciggoueq6qsu@sigill.intra.peff.net","subject":"[PATCH 0/6] fixing reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T07:55:08Z","receivedAt":"2017-07-05T07:55:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 04, 2017 at 05:24:08PM -0400, Jeff King wrote:\n\n> On Tue, Jul 04, 2017 at 07:58:06PM +0000, brian m. carlson wrote:\n> \n> > > And here's one more patch on top of those that's necessary to get the\n> > > tests to pass (I don't expect anybody to necessarily be applying this\n> > > slow string of patches; it's just to show the direction I'm looking in).\n> > \n> > I've looked at your original patch, which modified reflog-walk.c, and it\n> > does fix the issue. I'm happy to send in a patch with that and a test\n> > (provided you're okay with me adding your sign-off), or if you wanted to\n> > send in something a bit more complete, like the series of patches here,\n> > that's fine, too.\n> \n> I've been on vacation for the past week, but wrapping this up is on my\n> todo. I'll try to get to it tonight.\n\nOK, so here's what I came up with.\n\nThe first patch is my original small fix with an extra test. I think\nthat would be appropriate for 'maint'. Its behavior still has some\nquirks, but it avoids the confusion that you experienced and has a low\nrisk of breaking anything else.\n\nThe rest of it replaces the fake-parent thing with a more\nstraight-forward iteration over the reflogs (i.e., a cleanup of the\nfurther patches I've been posting). After digging into it and especially\nafter writing the new tests, I think I've convinced myself that this is\nthe right way forward.\n\nI tried to anticipate the behavior changes and I think all of them are\nimprovements. I won't be surprised if there's some hidden gotcha,\nthough, so this is definitely not for 'maint'. The patches do textually\ndepend on the fix from 1/6; my intent was that they'd be applied in\nsequence and only merge up the first one to maint.\n\n [1/6]: reflog-walk: skip over double-null oid due to HEAD rename\n [2/6]: t1414: document some reflog-walk oddities\n [3/6]: log: do not free parents when walking reflog\n [4/6]: get_revision_1(): replace do-while with an early return\n [5/6]: rev-list: check reflog_info before showing usage\n [6/6]: reflog-walk: stop using fake parents\n\n builtin/log.c | 4 +-\n builtin/rev-list.c | 3 +-\n reflog-walk.c | 117 ++++++++++++++-----------------------------------\n reflog-walk.h | 6 ++-\n revision.c | 39 ++++++++++-------\n t/t1414-reflog-walk.sh | 86 ++++++++++++++++++++++++++++++++++++\n t/t3200-branch.sh | 10 +++++\n 7 files changed, 160 insertions(+), 105 deletions(-)\n create mode 100755 t/t1414-reflog-walk.sh\n\n-Peff\n"},{"id":"323823","messageId":"20170705075737.6qsotqkzxdxofni6@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170705075508.c5ul23vivzpklpy6@sigill.intra.peff.net","subject":"[PATCH 1/6] reflog-walk: skip over double-null oid due to HEAD rename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T07:57:37Z","receivedAt":"2017-07-05T07:57:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 39ee4c6c2f (branch: record creation of renamed branch\nin HEAD's log, 2017-02-20), a rename on the currently\nchecked out branch will create two entries in the HEAD\nreflog: one where the branch goes away (switching to the\nnull oid), and one where it comes back (switching away from\nthe null oid).\n\nThis confuses the reflog-walk code. When walking backwards,\nit first sees the null oid in the \"old\" field of the second\nentry. Thanks to the \"root commit\" logic added by 71abeb753f\n(reflog: continue walking the reflog past root commits,\n2016-06-03), we keep looking for the next entry by scanning\nthe \"new\" field from the previous entry. But that field is\nalso null! We need to go just a tiny bit further, and look\nat its \"old\" field. But with the current code, we decide the\nreflog has nothing else to show and just give up. To the\nuser this looks like the reflog was truncated by the rename\noperation, when in fact those entries are still there.\n\nThis patch does the absolute minimal fix, which is to look\nback that one extra level and keep traversing.\n\nThe resulting behavior may not be the _best_ thing to do in\nthe long run (for example, we show both reflog entries each\nwith the same commit id), but it's a simple way to fix the\nproblem without risking further regressions.\n\nSigned-off-by: Jeff King \n---\nI do still think it would be worth looking into making this rename\ncreate a single reflog entry, but that's largely orthogonal to making\nthe display code sane(r).\n\n reflog-walk.c | 2 ++\n t/t3200-branch.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex ed99437ad2..b7e489ad32 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \t\t/* a root commit, but there are still more entries to show */\n \t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n \t\tlogobj = parse_object(&reflog->noid);\n+\t\tif (!logobj)\n+\t\t\tlogobj = parse_object(&reflog->ooid);\n \t}\n \n \tif (!logobj || logobj->type != OBJ_COMMIT) {\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 48d152b9a9..dd37ac47c5 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -162,6 +162,17 @@ test_expect_success 'git branch -M baz bam should add entries to .git/logs/HEAD'\n \tgrep \"^0\\{40\\}.*$msg$\" .git/logs/HEAD\n '\n \n+test_expect_success 'resulting reflog can be shown by log -g' '\n+\toid=$(git rev-parse HEAD) &&\n+\tcat >expect <<-EOF &&\n+\tHEAD@{0} $oid $msg\n+\tHEAD@{1} $oid $msg\n+\tHEAD@{2} $oid checkout: moving from foo to baz\n+\tEOF\n+\tgit log -g --format=\"%gd %H %gs\" -3 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '\n \tgit checkout master &&\n \tgit worktree add -b baz bazdir &&\n-- \n2.13.2.892.g25f9b59978\n\n"},{"id":"323824","messageId":"20170705080036.qxzy7havcy72mvff@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170705075508.c5ul23vivzpklpy6@sigill.intra.peff.net","subject":"[PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T08:00:36Z","receivedAt":"2017-07-05T08:00:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since its inception, the general strategy of the reflog-walk\ncode has been to start with the tip commit for the ref, and\nas we traverse replace each commit's parent pointers with\nfake parents pointing to the previous reflog entry.\n\nThis lets us traverse the reflog as if it were a real\nhistory, but it has some user-visible oddities. Namely:\n\n 1. The fake parents are used for commit selection and\n display. So for example, \"--merges\" or \"--no-merges\"\n are useful, because the history appears as a linear\n string. Likewise, pathspec limiting is based on the\n diff between adjacent entries, not the changes actually\n introduced by a commit.\n\n These are often the same (e.g., because the entry was\n just running \"git commit\" and the adjacent entry _is_\n the true parent), but it may not be in several common\n cases. For instance, using \"git reset\" to jump around\n history, or \"git checkout\" to move HEAD.\n\n 2. We reverse-map each commit back to its reflog. So when\n it comes time to show commit X, we say \"a-ha, we added\n X because it was at the tip of the 'foo' reflog, so\n let's show the foo reflog\". But this leads to nonsense\n results when you ask to traverse multiple reflogs: if\n two reflogs have the same tip commit, we only map back\n to one of them.\n\n Instead, we should show each reflog individually, in\n the order the user gave us on the command line.\n\n 2. If the tip of the reflog and the ref tip disagree on\n the current value, we show the ref tip but give no\n indication of the value in the reflog. This situation\n isn't supposed to happen (since any ref update should\n touch the reflog). But if it does, given that the\n requested operation is to show the reflog, it makes\n sense to prefer that.\n\nThis commit adds several expect_failure tests, to show how\nthe tool ought to behave.\n\nSigned-off-by: Jeff King \n---\n t/t1414-reflog-walk.sh | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 83 insertions(+)\n create mode 100755 t/t1414-reflog-walk.sh\n\ndiff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh\nnew file mode 100755\nindex 0000000000..bb847f797d\n--- /dev/null\n+++ b/t/t1414-reflog-walk.sh\n@@ -0,0 +1,83 @@\n+#!/bin/sh\n+\n+test_description='various tests of reflog walk (log -g) behavior'\n+. ./test-lib.sh\n+\n+test_expect_success 'set up some reflog entries' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\tgit checkout -b side HEAD^ &&\n+\ttest_commit three &&\n+\tgit merge --no-commit master &&\n+\techo evil-merge-content >>one.t &&\n+\ttest_tick &&\n+\tgit commit --no-edit -a\n+'\n+\n+do_walk () {\n+\tgit log -g --format=\"%gd %gs\" \"$@\"\n+}\n+\n+sq=\"'\"\n+test_expect_success 'set up expected reflog' '\n+\tcat >expect.all <<-EOF\n+\tHEAD@{0} commit (merge): Merge branch ${sq}master${sq} into side\n+\tHEAD@{1} commit: three\n+\tHEAD@{2} checkout: moving from master to side\n+\tHEAD@{3} commit: two\n+\tHEAD@{4} commit (initial): one\n+\tEOF\n+'\n+\n+test_expect_success 'reflog walk shows expected logs' '\n+\tdo_walk >actual &&\n+\ttest_cmp expect.all actual\n+'\n+\n+test_expect_failure 'reflog can limit with --no-merges' '\n+\tgrep -v merge expect.all >expect &&\n+\tdo_walk --no-merges >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'reflog can limit with pathspecs' '\n+\tgrep two expect.all >expect &&\n+\tdo_walk -- two.t >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'pathspec limiting handles merges' '\n+\tsed -n \"1p;3p;5p\" expect.all >expect &&\n+\tdo_walk -- one.t >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure '--parents shows true parents' '\n+\t# convert newlines to spaces\n+\techo $(git rev-parse HEAD HEAD^1 HEAD^2) >expect &&\n+\tgit rev-list -g --parents -1 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'walking multiple reflogs shows both' '\n+\t{\n+\t\tdo_walk HEAD &&\n+\t\tdo_walk side\n+\t} >expect &&\n+\tdo_walk HEAD side >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'walk prefers reflog to ref tip' '\n+\thead=$(git rev-parse HEAD) &&\n+\tone=$(git rev-parse one) &&\n+\tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\" &&\n+\techo \"$head $one $ident\tbroken reflog entry\" >>.git/logs/HEAD &&\n+\n+\techo $one >expect &&\n+\tgit log -g --format=%H -1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n+test_done\n-- \n2.13.2.892.g25f9b59978\n\n"},{"id":"323825","messageId":"20170705080254.jf7wlye3fejhqfm2@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170705075508.c5ul23vivzpklpy6@sigill.intra.peff.net","subject":"[PATCH 3/6] log: do not free parents when walking reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T08:02:54Z","receivedAt":"2017-07-05T08:03:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're doing a reflog walk (instead of walking the\nactual parent pointers), we may see commits multiple times.\nFor this reason, we hold on to the commit buffer for each\ncommit rather than freeing it after we've showed the commit.\n\nWe should do the same for the parent list. Right now this is\njust a minor optimization. But once we refactor how reflog\nwalks are performed, keeping the parents will avoid\nconfusing us the second time we see the commit.\n\nSigned-off-by: Jeff King \n---\nI didn't dig deep into the details of \"confusing\", so there may be other\nways to solve it. But this seems like the right thing to do regardless.\n\n builtin/log.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8ca1de9894..9c8bb3b5c3 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\tif (!rev->reflog_info) {\n \t\t\t/* we allow cycles in reflog ancestry */\n \t\t\tfree_commit_buffer(commit);\n+\t\t\tfree_commit_list(commit->parents);\n+\t\t\tcommit->parents = NULL;\n \t\t}\n-\t\tfree_commit_list(commit->parents);\n-\t\tcommit->parents = NULL;\n \t\tif (saved_nrl < rev->diffopt.needed_rename_limit)\n \t\t\tsaved_nrl = rev->diffopt.needed_rename_limit;\n \t\tif (rev->diffopt.degraded_cc_to_c)\n-- \n2.13.2.892.g25f9b59978\n\n"},{"id":"323826","messageId":"20170705080433.sqscxa76dgsosc5q@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170705075508.c5ul23vivzpklpy6@sigill.intra.peff.net","subject":"[PATCH 4/6] get_revision_1(): replace do-while with an early return","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T08:04:33Z","receivedAt":"2017-07-05T08:04:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The get_revision_1() function tries to avoid entering its\nmain loop at all when there are no commits to look at. But\nit's perfectly safe to call pop_commit() on an empty list\n(in which case it will return NULL). Switching to an early\nreturn from the loop lets us skip repeating the loop\ncondition before we enter the do-while. That will get more\nimportant when we start pulling reflog-walk commits from a\nsource besides the revs->commits queue, as that condition\nwill get much more complicated.\n\nSigned-off-by: Jeff King \n---\n revision.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex e181ad1b70..4dc7c63654 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3109,12 +3109,12 @@ static void track_linear(struct rev_info *revs, struct commit *commit)\n \n static struct commit *get_revision_1(struct rev_info *revs)\n {\n-\tif (!revs->commits)\n-\t\treturn NULL;\n-\n-\tdo {\n+\twhile (1) {\n \t\tstruct commit *commit = pop_commit(&revs->commits);\n \n+\t\tif (!commit)\n+\t\t\treturn NULL;\n+\n \t\tif (revs->reflog_info) {\n \t\t\tsave_parents(revs, commit);\n \t\t\tfake_reflog_parent(revs->reflog_info, commit);\n@@ -3148,8 +3148,7 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\t\t\ttrack_linear(revs, commit);\n \t\t\treturn commit;\n \t\t}\n-\t} while (revs->commits);\n-\treturn NULL;\n+\t}\n }\n \n /*\n-- \n2.13.2.892.g25f9b59978\n\n"},{"id":"323828","messageId":"20170705080655.giw6wjsblepcf5aw@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170705075508.c5ul23vivzpklpy6@sigill.intra.peff.net","subject":"[PATCH 5/6] rev-list: check reflog_info before showing usage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T08:06:56Z","receivedAt":"2017-07-05T08:07:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git-rev-list sees no pending commits, it shows a usage\nmessage. This works even when reflog-walking is requested,\nbecause the reflog-walk code currently puts the reflog tips\ninto the pending queue.\n\nIn preparation for refactoring the reflog-walk code, let's\nexplicitly check whether we have any reflogs to walk. For\nnow this is a noop, but the existing reflog tests will make\nsure that it kicks in after the refactoring. Likewise, we'll\nadd a test that \"rev-list -g\" without specifying any reflogs\ncontinues to fail (so that we know our check does not kick\nin too aggressively).\n\nNote that the implementation needs to go into its own\nsub-function, as the walk code does not expose its innards\noutside of reflog-walk.c.\n\nSigned-off-by: Jeff King \n---\nThis is actually the main \"gotcha\" I'm worried about with this series.\nI'm not sure if any other code would care about seeing the pending items\nin revs->commits. I still think the series is the right direction; if\nthere is such a place, we'd want to teach it to handle reflog walking in\na similar way, too.\n\n builtin/rev-list.c | 3 ++-\n reflog-walk.c | 5 +++++\n reflog-walk.h | 2 ++\n t/t1414-reflog-walk.sh | 3 +++\n 4 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 95d84d5cda..53a746dd89 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -11,6 +11,7 @@\n #include \"graph.h\"\n #include \"bisect.h\"\n #include \"progress.h\"\n+#include \"reflog-walk.h\"\n \n static const char rev_list_usage[] =\n \"git rev-list [OPTION] ... [ -- paths... ]\\n\"\n@@ -348,7 +349,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t/* Only --header was specified */\n \t\trevs.commit_format = CMIT_FMT_RAW;\n \n-\tif ((!revs.commits &&\n+\tif ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&\n \t (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&\n \t !revs.pending.nr)) ||\n \t revs.diff)\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex b7e489ad32..89e719c459 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -358,3 +358,8 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n \t\tstrbuf_release(&selector);\n \t}\n }\n+\n+int reflog_walk_empty(struct reflog_walk_info *info)\n+{\n+\treturn !info || !info->reflogs.nr;\n+}\ndiff --git a/reflog-walk.h b/reflog-walk.h\nindex 27886f793e..af32361072 100644\n--- a/reflog-walk.h\n+++ b/reflog-walk.h\n@@ -20,4 +20,6 @@ extern void get_reflog_selector(struct strbuf *sb,\n \t\tconst struct date_mode *dmode, int force_date,\n \t\tint shorten);\n \n+extern int reflog_walk_empty(struct reflog_walk_info *walk);\n+\n #endif\ndiff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh\nindex bb847f797d..fba6788e94 100755\n--- a/t/t1414-reflog-walk.sh\n+++ b/t/t1414-reflog-walk.sh\n@@ -79,5 +79,8 @@ test_expect_failure 'walk prefers reflog to ref tip' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list -g complains when there are no reflogs' '\n+\ttest_must_fail git rev-list -g\n+'\n \n test_done\n-- \n2.13.2.892.g25f9b59978\n\n"},{"id":"323829","messageId":"20170705080904.idlpm6ecslzfbm75@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170705075508.c5ul23vivzpklpy6@sigill.intra.peff.net","subject":"[PATCH 6/6] reflog-walk: stop using fake parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T08:09:04Z","receivedAt":"2017-07-05T08:09:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The reflog-walk system works by putting a ref's tip into the\npending queue, and then \"traversing\" the reflog by\npretending that the parent of each commit is the previous\nreflog entry.\n\nThis causes a number of user-visible oddities, as documented\nin t1414 (and the commit message which introduced it). We\ncan fix all of them in one go by replacing the fake-reflog\nsystem with a much simpler one: just keeping a list of\nreflogs to show, and walking through them entry by entry.\n\nThe implementation is fairly straight-forward, but there are\na few items to note:\n\n 1. We obviously must skip calling add_parents_to_list()\n when we are traversing reflogs, since we do not want to\n walk the original parents at all.\n\n As a result, we must call try_to_simplify_commit()\n ourselves and skip any TREESAME commits.\n\n There are other parts of add_parents_to_list() we skip,\n as well, but none of them should matter for a reflog\n traversal:\n\n - We do not allow UNINTERESTING commits, nor\n symmetric ranges (and we bail when these are used\n with \"-g\").\n\n - Using --source makes no sense, since we aren't\n traversing. The reflog selector shows the same\n information with more detail.\n\n - Using --first-parent is still sensible, since you\n may want to see the first-parent diff for each\n entry. But since we're not traversing, we don't\n need to cull the parent list here.\n\n 2. Since we now just walk the reflog entries themselves,\n rather than starting with the ref tip, we now look at\n the \"new\" field of each entry rather than the \"old\"\n (i.e., we are showing entries, not faking parents).\n This removes all of the tricky logic around skipping\n past root commits.\n\n But note that we have no way to show an entry with the\n null sha1 in its \"new\" field (because such a commit\n obviously does not exist). Normally this would not\n happen, since we delete reflogs along with refs, but\n there is one special case. When we rename the currently\n checked out branch, we write two reflog entries into\n the HEAD log: one where the commit goes away, and\n another where it comes back.\n\n Prior to this commit, we show both entries with\n identical reflog messages. After this commit, we show\n only the \"comes back\" entry. See the update in t3200\n which demonstrates this.\n\n Arguably either is fine, as the whole double-entry\n thing is a bit hacky in the first place. And until a\n recent fix, we truncated the traversal in such a case\n anyway, which was _definitely_ wrong.\n\nSigned-off-by: Jeff King \n---\n reflog-walk.c | 116 +++++++++++++------------------------------------\n reflog-walk.h | 4 +-\n revision.c | 30 ++++++++-----\n t/t1414-reflog-walk.sh | 12 ++---\n t/t3200-branch.sh | 3 +-\n 5 files changed, 57 insertions(+), 108 deletions(-)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 89e719c459..a7644d944e 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -78,45 +78,6 @@ static int get_reflog_recno_by_time(struct complete_reflogs *array,\n \treturn -1;\n }\n \n-struct commit_info_lifo {\n-\tstruct commit_info {\n-\t\tstruct commit *commit;\n-\t\tvoid *util;\n-\t} *items;\n-\tint nr, alloc;\n-};\n-\n-static struct commit_info *get_commit_info(struct commit *commit,\n-\t\tstruct commit_info_lifo *lifo, int pop)\n-{\n-\tint i;\n-\tfor (i = 0; i < lifo->nr; i++)\n-\t\tif (lifo->items[i].commit == commit) {\n-\t\t\tstruct commit_info *result = &lifo->items[i];\n-\t\t\tif (pop) {\n-\t\t\t\tif (i + 1 < lifo->nr)\n-\t\t\t\t\tmemmove(lifo->items + i,\n-\t\t\t\t\t\tlifo->items + i + 1,\n-\t\t\t\t\t\t(lifo->nr - i) *\n-\t\t\t\t\t\tsizeof(struct commit_info));\n-\t\t\t\tlifo->nr--;\n-\t\t\t}\n-\t\t\treturn result;\n-\t\t}\n-\treturn NULL;\n-}\n-\n-static void add_commit_info(struct commit *commit, void *util,\n-\t\tstruct commit_info_lifo *lifo)\n-{\n-\tstruct commit_info *info;\n-\tALLOC_GROW(lifo->items, lifo->nr + 1, lifo->alloc);\n-\tinfo = lifo->items + lifo->nr;\n-\tinfo->commit = commit;\n-\tinfo->util = util;\n-\tlifo->nr++;\n-}\n-\n struct commit_reflog {\n \tint recno;\n \tenum selector_type {\n@@ -128,7 +89,8 @@ struct commit_reflog {\n };\n \n struct reflog_walk_info {\n-\tstruct commit_info_lifo reflogs;\n+\tstruct commit_reflog **logs;\n+\tsize_t nr, alloc, cur;\n \tstruct string_list complete_reflogs;\n \tstruct commit_reflog *last_commit_reflog;\n };\n@@ -226,52 +188,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n \tcommit_reflog->selector = selector;\n \tcommit_reflog->reflogs = reflogs;\n \n-\tadd_commit_info(commit, commit_reflog, &info->reflogs);\n-\treturn 0;\n-}\n-\n-void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n-{\n-\tstruct commit_info *commit_info =\n-\t\tget_commit_info(commit, &info->reflogs, 0);\n-\tstruct commit_reflog *commit_reflog;\n-\tstruct object *logobj;\n-\tstruct reflog_info *reflog;\n-\n-\tinfo->last_commit_reflog = NULL;\n-\tif (!commit_info)\n-\t\treturn;\n+\tALLOC_GROW(info->logs, info->nr + 1, info->alloc);\n+\tinfo->logs[info->nr++] = commit_reflog;\n \n-\tcommit_reflog = commit_info->util;\n-\tif (commit_reflog->recno < 0) {\n-\t\tcommit->parents = NULL;\n-\t\treturn;\n-\t}\n-\tinfo->last_commit_reflog = commit_reflog;\n-\n-\tdo {\n-\t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n-\t\tcommit_reflog->recno--;\n-\t\tlogobj = parse_object(&reflog->ooid);\n-\t} while (commit_reflog->recno && (logobj && logobj->type != OBJ_COMMIT));\n-\n-\tif (!logobj && commit_reflog->recno >= 0 && is_null_oid(&reflog->ooid)) {\n-\t\t/* a root commit, but there are still more entries to show */\n-\t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n-\t\tlogobj = parse_object(&reflog->noid);\n-\t\tif (!logobj)\n-\t\t\tlogobj = parse_object(&reflog->ooid);\n-\t}\n-\n-\tif (!logobj || logobj->type != OBJ_COMMIT) {\n-\t\tcommit_info->commit = NULL;\n-\t\tcommit->parents = NULL;\n-\t\treturn;\n-\t}\n-\tcommit_info->commit = (struct commit *)logobj;\n-\n-\tcommit->parents = xcalloc(1, sizeof(struct commit_list));\n-\tcommit->parents->item = commit_info->commit;\n+\treturn 0;\n }\n \n void get_reflog_selector(struct strbuf *sb,\n@@ -361,5 +281,29 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n \n int reflog_walk_empty(struct reflog_walk_info *info)\n {\n-\treturn !info || !info->reflogs.nr;\n+\treturn !info || !info->nr;\n+}\n+\n+struct commit *next_entry_in_log(struct commit_reflog *log)\n+{\n+\twhile (log->recno >= 0) {\n+\t\tstruct reflog_info *entry = &log->reflogs->items[log->recno--];\n+\t\tstruct object *obj = parse_object(&entry->noid);\n+\n+\t\tif (obj && obj->type == OBJ_COMMIT)\n+\t\t\treturn (struct commit *)obj;\n+\t}\n+\treturn NULL;\n+}\n+\n+struct commit *next_reflog_entry(struct reflog_walk_info *walk)\n+{\n+\tfor (; walk->cur < walk->nr; walk->cur++) {\n+\t\tstruct commit *ret = next_entry_in_log(walk->logs[walk->cur]);\n+\t\tif (ret) {\n+\t\t\twalk->last_commit_reflog = walk->logs[walk->cur];\n+\t\t\treturn ret;\n+\t\t}\n+\t}\n+\treturn NULL;\n }\ndiff --git a/reflog-walk.h b/reflog-walk.h\nindex af32361072..373388cd14 100644\n--- a/reflog-walk.h\n+++ b/reflog-walk.h\n@@ -8,8 +8,6 @@ struct reflog_walk_info;\n extern void init_reflog_walk(struct reflog_walk_info **info);\n extern int add_reflog_for_walk(struct reflog_walk_info *info,\n \t\tstruct commit *commit, const char *name);\n-extern void fake_reflog_parent(struct reflog_walk_info *info,\n-\t\tstruct commit *commit);\n extern void show_reflog_message(struct reflog_walk_info *info, int,\n \t\t\t\tconst struct date_mode *, int force_date);\n extern void get_reflog_message(struct strbuf *sb,\n@@ -22,4 +20,6 @@ extern void get_reflog_selector(struct strbuf *sb,\n \n extern int reflog_walk_empty(struct reflog_walk_info *walk);\n \n+struct commit *next_reflog_entry(struct reflog_walk_info *reflog_info);\n+\n #endif\ndiff --git a/revision.c b/revision.c\nindex 4dc7c63654..5fc01f2d26 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -148,16 +148,14 @@ static void add_pending_object_with_path(struct rev_info *revs,\n \tif (revs->reflog_info && obj->type == OBJ_COMMIT) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint len = interpret_branch_name(name, 0, &buf, 0);\n-\t\tint st;\n \n \t\tif (0 < len && name[len] && buf.len)\n \t\t\tstrbuf_addstr(&buf, name + len);\n-\t\tst = add_reflog_for_walk(revs->reflog_info,\n-\t\t\t\t\t (struct commit *)obj,\n-\t\t\t\t\t buf.buf[0] ? buf.buf: name);\n+\t\tadd_reflog_for_walk(revs->reflog_info,\n+\t\t\t\t (struct commit *)obj,\n+\t\t\t\t buf.buf[0] ? buf.buf: name);\n \t\tstrbuf_release(&buf);\n-\t\tif (st)\n-\t\t\treturn;\n+\t\treturn; /* do not add the commit itself */\n \t}\n \tadd_object_array_with_path(obj, name, &revs->pending, mode, path);\n }\n@@ -3110,16 +3108,18 @@ static void track_linear(struct rev_info *revs, struct commit *commit)\n static struct commit *get_revision_1(struct rev_info *revs)\n {\n \twhile (1) {\n-\t\tstruct commit *commit = pop_commit(&revs->commits);\n+\t\tstruct commit *commit;\n+\n+\t\tif (revs->reflog_info)\n+\t\t\tcommit = next_reflog_entry(revs->reflog_info);\n+\t\telse\n+\t\t\tcommit = pop_commit(&revs->commits);\n \n \t\tif (!commit)\n \t\t\treturn NULL;\n \n-\t\tif (revs->reflog_info) {\n-\t\t\tsave_parents(revs, commit);\n-\t\t\tfake_reflog_parent(revs->reflog_info, commit);\n+\t\tif (revs->reflog_info)\n \t\t\tcommit->object.flags &= ~(ADDED | SEEN | SHOWN);\n-\t\t}\n \n \t\t/*\n \t\t * If we haven't done the list limiting, we need to look at\n@@ -3130,7 +3130,8 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\t\tif (revs->max_age != -1 &&\n \t\t\t (commit->date < revs->max_age))\n \t\t\t\tcontinue;\n-\t\t\tif (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {\n+\t\t\tif (!revs->reflog_info &&\n+\t\t\t add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {\n \t\t\t\tif (!revs->ignore_missing_links)\n \t\t\t\t\tdie(\"Failed to traverse parents of commit %s\",\n \t\t\t\t\t\toid_to_hex(&commit->object.oid));\n@@ -3146,6 +3147,11 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\tdefault:\n \t\t\tif (revs->track_linear)\n \t\t\t\ttrack_linear(revs, commit);\n+\t\t\tif (revs->reflog_info) {\n+\t\t\t\ttry_to_simplify_commit(revs, commit);\n+\t\t\t\tif (commit->object.flags & TREESAME)\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\treturn commit;\n \t\t}\n \t}\ndiff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh\nindex fba6788e94..7cc52d946d 100755\n--- a/t/t1414-reflog-walk.sh\n+++ b/t/t1414-reflog-walk.sh\n@@ -34,32 +34,32 @@ test_expect_success 'reflog walk shows expected logs' '\n \ttest_cmp expect.all actual\n '\n \n-test_expect_failure 'reflog can limit with --no-merges' '\n+test_expect_success 'reflog can limit with --no-merges' '\n \tgrep -v merge expect.all >expect &&\n \tdo_walk --no-merges >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'reflog can limit with pathspecs' '\n+test_expect_success 'reflog can limit with pathspecs' '\n \tgrep two expect.all >expect &&\n \tdo_walk -- two.t >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'pathspec limiting handles merges' '\n+test_expect_success 'pathspec limiting handles merges' '\n \tsed -n \"1p;3p;5p\" expect.all >expect &&\n \tdo_walk -- one.t >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_failure '--parents shows true parents' '\n+test_expect_success '--parents shows true parents' '\n \t# convert newlines to spaces\n \techo $(git rev-parse HEAD HEAD^1 HEAD^2) >expect &&\n \tgit rev-list -g --parents -1 HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'walking multiple reflogs shows both' '\n+test_expect_success 'walking multiple reflogs shows both' '\n \t{\n \t\tdo_walk HEAD &&\n \t\tdo_walk side\n@@ -68,7 +68,7 @@ test_expect_failure 'walking multiple reflogs shows both' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'walk prefers reflog to ref tip' '\n+test_expect_success 'walk prefers reflog to ref tip' '\n \thead=$(git rev-parse HEAD) &&\n \tone=$(git rev-parse one) &&\n \tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\" &&\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex dd37ac47c5..9d707d2a40 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -166,10 +166,9 @@ test_expect_success 'resulting reflog can be shown by log -g' '\n \toid=$(git rev-parse HEAD) &&\n \tcat >expect <<-EOF &&\n \tHEAD@{0} $oid $msg\n-\tHEAD@{1} $oid $msg\n \tHEAD@{2} $oid checkout: moving from foo to baz\n \tEOF\n-\tgit log -g --format=\"%gd %H %gs\" -3 HEAD >actual &&\n+\tgit log -g --format=\"%gd %H %gs\" -2 HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.13.2.892.g25f9b59978\n"},{"id":"323849","messageId":"xmqqo9syiy9g.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170705075737.6qsotqkzxdxofni6@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] reflog-walk: skip over double-null oid due to HEAD rename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-05T17:34:03Z","receivedAt":"2017-07-05T17:34:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Since 39ee4c6c2f (branch: record creation of renamed branch\n> in HEAD's log, 2017-02-20), a rename on the currently\n> checked out branch will create two entries in the HEAD\n> reflog: one where the branch goes away (switching to the\n> null oid), and one where it comes back (switching away from\n> the null oid).\n> ...\n> The resulting behavior may not be the _best_ thing to do in\n> the long run (for example, we show both reflog entries each\n> with the same commit id), but it's a simple way to fix the\n> problem without risking further regressions.\n>\n> Signed-off-by: Jeff King \n> ---\n> I do still think it would be worth looking into making this rename\n> create a single reflog entry, but that's largely orthogonal to making\n> the display code sane(r).\n\nI agree with this assessment.\n\n> reflog-walk.c | 2 ++\n> t/t3200-branch.sh | 11 +++++++++++\n> 2 files changed, 13 insertions(+)\n>\n> diff --git a/reflog-walk.c b/reflog-walk.c\n> index ed99437ad2..b7e489ad32 100644\n> --- a/reflog-walk.c\n> +++ b/reflog-walk.c\n> @@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n> \t\t/* a root commit, but there are still more entries to show */\n> \t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n> \t\tlogobj = parse_object(&reflog->noid);\n> +\t\tif (!logobj)\n> +\t\t\tlogobj = parse_object(&reflog->ooid);\n> \t}\n\nFor the current 'maint', this would need to be backported to the\nuchar[20] interface (which is trivial to do, and merging it upwards\nwhile adjusting it back to \"struct object_id\" is also trivial; there\nis no need to resend).\n\nThanks. Will queue.\n\n> \tif (!logobj || logobj->type != OBJ_COMMIT) {\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 48d152b9a9..dd37ac47c5 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -162,6 +162,17 @@ test_expect_success 'git branch -M baz bam should add entries to .git/logs/HEAD'\n> \tgrep \"^0\\{40\\}.*$msg$\" .git/logs/HEAD\n> '\n> \n> +test_expect_success 'resulting reflog can be shown by log -g' '\n> +\toid=$(git rev-parse HEAD) &&\n> +\tcat >expect <<-EOF &&\n> +\tHEAD@{0} $oid $msg\n> +\tHEAD@{1} $oid $msg\n> +\tHEAD@{2} $oid checkout: moving from foo to baz\n> +\tEOF\n> +\tgit log -g --format=\"%gd %H %gs\" -3 HEAD >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '\n> \tgit checkout master &&\n> \tgit worktree add -b baz bazdir &&\n"},{"id":"323852","messageId":"xmqqh8yqix7p.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170705080036.qxzy7havcy72mvff@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-05T17:56:42Z","receivedAt":"2017-07-05T17:56:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Since its inception, the general strategy of the reflog-walk\n> code has been to start with the tip commit for the ref, and\n> as we traverse replace each commit's parent pointers with\n> fake parents pointing to the previous reflog entry.\n>\n> This lets us traverse the reflog as if it were a real\n> history, but it has some user-visible oddities. Namely:\n>\n> 1. The fake parents are used for commit selection and\n> display. So for example, \"--merges\" or \"--no-merges\"\n> are useful, because the history appears as a linear\n\ns/useful/useless/ perhaps?\n\n> string. Likewise, pathspec limiting is based on the\n> diff between adjacent entries, not the changes actually\n> introduced by a commit.\n> ...\n> This commit adds several expect_failure tests, to show how\n> the tool ought to behave.\n>\n> Signed-off-by: Jeff King \n> ---\n\n> diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh\n> new file mode 100755\n> index 0000000000..bb847f797d\n> --- /dev/null\n> +++ b/t/t1414-reflog-walk.sh\n> @@ -0,0 +1,83 @@\n> +#!/bin/sh\n> +\n> +test_description='various tests of reflog walk (log -g) behavior'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'set up some reflog entries' '\n> +\ttest_commit one &&\n> +\ttest_commit two &&\n> +\tgit checkout -b side HEAD^ &&\n> +\ttest_commit three &&\n> +\tgit merge --no-commit master &&\n> +\techo evil-merge-content >>one.t &&\n> +\ttest_tick &&\n> +\tgit commit --no-edit -a\n> +'\n\nMental note: logically, what we want to see in the log are:\n\n master: one-->two\n side: one-->three-->(evil)\n HEAD: one-->two-->one-->three-->(evil)\n\nwhere the middle one in HEAD is \"switching from master to side\".\n\n> +test_expect_failure 'pathspec limiting handles merges' '\n> +\tsed -n \"1p;3p;5p\" expect.all >expect &&\n> +\tdo_walk -- one.t >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nOK (it was a bit tricky to see why the topmost one should, but the\nevilness of the merge makes it eligible).\n\n> +test_expect_failure '--parents shows true parents' '\n> +\t# convert newlines to spaces\n> +\techo $(git rev-parse HEAD HEAD^1 HEAD^2) >expect &&\n\nI saw something related to this nearby this morning. Good thinking\nto add a comment on this 'echo' ;-).\n\n> +\tgit rev-list -g --parents -1 HEAD >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'walking multiple reflogs shows both' '\n> +\t{\n> +\t\tdo_walk HEAD &&\n> +\t\tdo_walk side\n> +\t} >expect &&\n> +\tdo_walk HEAD side >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nI somehow find this one a bit iffy. \n\nThe order that commits appear in the \"walk from HEAD and side at the\nsame time\" may want to be different from what this test expects.\n\"rev-list --since=3.days -g master next\", for example, would want to\nrefrain from reading the reflog of 'master' for all 90 days before\nswitching to the reflog of 'next', no?\n\nAll others I did not comment on and omitted from quoting make sense\nto me.\n\nThanks.\n"},{"id":"323856","messageId":"xmqqd19eiwpk.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170705080655.giw6wjsblepcf5aw@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] rev-list: check reflog_info before showing usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-05T18:07:35Z","receivedAt":"2017-07-05T18:07:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When git-rev-list sees no pending commits, it shows a usage\n> message. This works even when reflog-walking is requested,\n> because the reflog-walk code currently puts the reflog tips\n> into the pending queue.\n>\n> In preparation for refactoring the reflog-walk code, let's\n> explicitly check whether we have any reflogs to walk. For\n> now this is a noop, but the existing reflog tests will make\n> sure that it kicks in after the refactoring. Likewise, we'll\n> add a test that \"rev-list -g\" without specifying any reflogs\n> continues to fail (so that we know our check does not kick\n> in too aggressively).\n>\n> Note that the implementation needs to go into its own\n> sub-function, as the walk code does not expose its innards\n> outside of reflog-walk.c.\n>\n> Signed-off-by: Jeff King \n> ---\n> This is actually the main \"gotcha\" I'm worried about with this series.\n> I'm not sure if any other code would care about seeing the pending items\n> in revs->commits. I still think the series is the right direction; if\n> there is such a place, we'd want to teach it to handle reflog walking in\n> a similar way, too.\n\nAh, very good thinking---when I was following along your drafts to\nbypass the revs.commits queue for reflog walking, I didn't think of\nthis one. \n\nPerhaps \"!revs.commits && reflog_walk_empty(revs.reflog_info)\" may\nwant to become a macro \"walk_finished(&revs)\" or something to\nreplace existing !revs.commits that is not accompanied by the check\non .reflog_info field?\n\n"},{"id":"323865","messageId":"c4a4df1c-bf38-fbad-e7c2-9236fde87417@ramsayjones.plus.com","threadId":"46227","inReplyTo":"20170705080036.qxzy7havcy72mvff@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-07-05T20:05:14Z","receivedAt":"2017-07-05T20:05:38Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/07/17 09:00, Jeff King wrote:\n> Since its inception, the general strategy of the reflog-walk\n> code has been to start with the tip commit for the ref, and\n> as we traverse replace each commit's parent pointers with\n> fake parents pointing to the previous reflog entry.\n> \n> This lets us traverse the reflog as if it were a real\n> history, but it has some user-visible oddities. Namely:\n> \n> 1. The fake parents are used for commit selection and\n> display. So for example, \"--merges\" or \"--no-merges\"\n> are useful, because the history appears as a linear\n> string. Likewise, pathspec limiting is based on the\n> diff between adjacent entries, not the changes actually\n> introduced by a commit.\n> \n> These are often the same (e.g., because the entry was\n> just running \"git commit\" and the adjacent entry _is_\n> the true parent), but it may not be in several common\n> cases. For instance, using \"git reset\" to jump around\n> history, or \"git checkout\" to move HEAD.\n> \n> 2. We reverse-map each commit back to its reflog. So when\n> it comes time to show commit X, we say \"a-ha, we added\n> X because it was at the tip of the 'foo' reflog, so\n> let's show the foo reflog\". But this leads to nonsense\n> results when you ask to traverse multiple reflogs: if\n> two reflogs have the same tip commit, we only map back\n> to one of them.\n> \n> Instead, we should show each reflog individually, in\n> the order the user gave us on the command line.\n> \n> 2. If the tip of the reflog and the ref tip disagree on\n ^^\nIt seems hard to get off the second point! ;-)\n\nATB,\nRamsay Jones\n\n"},{"id":"323869","messageId":"20170705212105.3qjdzgtut23v6igw@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqqo9syiy9g.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/6] reflog-walk: skip over double-null oid due to HEAD rename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T21:21:05Z","receivedAt":"2017-07-05T21:21:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 05, 2017 at 10:34:03AM -0700, Junio C Hamano wrote:\n\n> > +\t\tif (!logobj)\n> > +\t\t\tlogobj = parse_object(&reflog->ooid);\n> > \t}\n> \n> For the current 'maint', this would need to be backported to the\n> uchar[20] interface (which is trivial to do, and merging it upwards\n> while adjusting it back to \"struct object_id\" is also trivial; there\n> is no need to resend).\n> \n> Thanks. Will queue.\n\nAh, right. I should probably actually apply the patches on \"maint\"\nbefore suggesting you do so. ;)\n\nIn theory it should be applied directly on 39ee4c6c2 and then merged up\n(but I think the same wiggling would apply either way).\n\nThe rest of the patches would need to be adjusted on top, but it should\nbe easy to resolve; all of that code just goes away.\n\n-Peff\n"},{"id":"323870","messageId":"20170705212757.3ygxiasbi2e7v27p@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqqh8yqix7p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T21:27:57Z","receivedAt":"2017-07-05T21:28:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 05, 2017 at 10:56:42AM -0700, Junio C Hamano wrote:\n\n> > 1. The fake parents are used for commit selection and\n> > display. So for example, \"--merges\" or \"--no-merges\"\n> > are useful, because the history appears as a linear\n> \n> s/useful/useless/ perhaps?\n\nOops, yes (\"not useful\" is probably what I was going for).\n\n> > +test_expect_success 'set up some reflog entries' '\n> > +\ttest_commit one &&\n> > +\ttest_commit two &&\n> > +\tgit checkout -b side HEAD^ &&\n> > +\ttest_commit three &&\n> > +\tgit merge --no-commit master &&\n> > +\techo evil-merge-content >>one.t &&\n> > +\ttest_tick &&\n> > +\tgit commit --no-edit -a\n> > +'\n> \n> Mental note: logically, what we want to see in the log are:\n> \n> master: one-->two\n> side: one-->three-->(evil)\n> HEAD: one-->two-->one-->three-->(evil)\n> \n> where the middle one in HEAD is \"switching from master to side\".\n\nYeah. I was tempted to document that, but I think the expect.all mostly\ndoes that for HEAD (and I don't really check the others). The grossest\nthing is this numeric selection:\n\n> > +test_expect_failure 'pathspec limiting handles merges' '\n> > +\tsed -n \"1p;3p;5p\" expect.all >expect &&\n\nI tried to think of an easy way to pick them out by name but couldn't\ncome up with one. I don't know if that sed invocation deserves a comment\nor not.\n\n> > +\tdo_walk -- one.t >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> OK (it was a bit tricky to see why the topmost one should, but the\n> evilness of the merge makes it eligible).\n\nYeah, that was why I added the evilness. A more real-world test would\nperhaps be a file with an actual conflict that got resolved (but not\nmatching either parent exactly). I actually started by adding the evil\ncontent to \"three\", which is a bit closer. But it passes even without\nthe patch, because the diff to the first parent still matches.\n\nSo I dunno. I think it's OK as is. My main goal was just to make sure\nthat my TREESAME hackery catches both normal and merge commits.\n\n> > +test_expect_failure 'walking multiple reflogs shows both' '\n> > +\t{\n> > +\t\tdo_walk HEAD &&\n> > +\t\tdo_walk side\n> > +\t} >expect &&\n> > +\tdo_walk HEAD side >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> I somehow find this one a bit iffy. \n> \n> The order that commits appear in the \"walk from HEAD and side at the\n> same time\" may want to be different from what this test expects.\n> \"rev-list --since=3.days -g master next\", for example, would want to\n> refrain from reading the reflog of 'master' for all 90 days before\n> switching to the reflog of 'next', no?\n\nI did make the ordering intentional. We process each reflog sequentially\nin turn. I don't think it would be wrong to interleave them by date, but\nI actually think it's useful for somebody who switches that behavior to\nconsciously update the test. Maybe it's worth calling out in the commit\nmessage.\n\nI stopped short of actually documenting and guaranteeing that behavior\nto the user. I don't know how comfortable we would be changing it later.\n\n(As an aside, I also prowled through the documentation looking for any\nguarantees we make to the user about the fake-parent thing, but I\ncouldn't find any. So I considered its user-visible effects an unwanted\nside effect of the implementation).\n\n-Peff\n"},{"id":"323871","messageId":"20170705213010.asbegr77pgvectuz@sigill.intra.peff.net","threadId":"46227","inReplyTo":"c4a4df1c-bf38-fbad-e7c2-9236fde87417@ramsayjones.plus.com","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T21:30:10Z","receivedAt":"2017-07-05T21:30:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 05, 2017 at 09:05:14PM +0100, Ramsay Jones wrote:\n\n> On 05/07/17 09:00, Jeff King wrote:\n> > Since its inception, the general strategy of the reflog-walk\n> > code has been to start with the tip commit for the ref, and\n> > as we traverse replace each commit's parent pointers with\n> > fake parents pointing to the previous reflog entry.\n> > \n> > This lets us traverse the reflog as if it were a real\n> > history, but it has some user-visible oddities. Namely:\n> > \n> > 1. The fake parents are used for commit selection and\n> > display. So for example, \"--merges\" or \"--no-merges\"\n> > are useful, because the history appears as a linear\n> > string. Likewise, pathspec limiting is based on the\n> > diff between adjacent entries, not the changes actually\n> > introduced by a commit.\n> > \n> > These are often the same (e.g., because the entry was\n> > just running \"git commit\" and the adjacent entry _is_\n> > the true parent), but it may not be in several common\n> > cases. For instance, using \"git reset\" to jump around\n> > history, or \"git checkout\" to move HEAD.\n> > \n> > 2. We reverse-map each commit back to its reflog. So when\n> > it comes time to show commit X, we say \"a-ha, we added\n> > X because it was at the tip of the 'foo' reflog, so\n> > let's show the foo reflog\". But this leads to nonsense\n> > results when you ask to traverse multiple reflogs: if\n> > two reflogs have the same tip commit, we only map back\n> > to one of them.\n> > \n> > Instead, we should show each reflog individually, in\n> > the order the user gave us on the command line.\n> > \n> > 2. If the tip of the reflog and the ref tip disagree on\n> ^^\n> It seems hard to get off the second point! ;-)\n\nHeh. You know, I even remember checking that I didn't screw that up\n(because I originally wrote the first and third, and later went back to\nadd in the second point). But somehow I botched the proofreading, too.\n\nI'll plan to re-roll this to update the little bits you and Junio have\npointed out. Junio, I'll probably just do the whole thing on \"maint\" and\nthen the merge-up to \"master\" should be straight-forward, I'd think.\n\n-Peff\n"},{"id":"323872","messageId":"20170705213135.wlijd5kum5lyh752@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqqd19eiwpk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/6] rev-list: check reflog_info before showing usage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T21:31:35Z","receivedAt":"2017-07-05T21:32:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 05, 2017 at 11:07:35AM -0700, Junio C Hamano wrote:\n\n> > This is actually the main \"gotcha\" I'm worried about with this series.\n> > I'm not sure if any other code would care about seeing the pending items\n> > in revs->commits. I still think the series is the right direction; if\n> > there is such a place, we'd want to teach it to handle reflog walking in\n> > a similar way, too.\n> \n> Ah, very good thinking---when I was following along your drafts to\n> bypass the revs.commits queue for reflog walking, I didn't think of\n> this one. \n> \n> Perhaps \"!revs.commits && reflog_walk_empty(revs.reflog_info)\" may\n> want to become a macro \"walk_finished(&revs)\" or something to\n> replace existing !revs.commits that is not accompanied by the check\n> on .reflog_info field?\n\nYeah, I had the same thought. But I don't think it's really worth adding\na helper if we don't end up with another call-site. So I was stalling to\nsee if that happened.\n\n(And just for reference in case we do end up adding it, it's not so much\nwalk_finished() as walk_empty_in_the_first_place()).\n\n-Peff\n"},{"id":"323874","messageId":"xmqqa84ih5ao.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170705212757.3ygxiasbi2e7v27p@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-05T22:45:03Z","receivedAt":"2017-07-05T22:45:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > +test_expect_failure 'walking multiple reflogs shows both' '\n>> > +\t{\n>> > +\t\tdo_walk HEAD &&\n>> > +\t\tdo_walk side\n>> > +\t} >expect &&\n>> > +\tdo_walk HEAD side >actual &&\n>> > +\ttest_cmp expect actual\n>> > +'\n>> \n>> I somehow find this one a bit iffy. \n>> \n>> The order that commits appear in the \"walk from HEAD and side at the\n>> same time\" may want to be different from what this test expects.\n>> \"rev-list --since=3.days -g master next\", for example, would want to\n>> refrain from reading the reflog of 'master' for all 90 days before\n>> switching to the reflog of 'next', no?\n>\n> I did make the ordering intentional. We process each reflog sequentially\n> in turn. I don't think it would be wrong to interleave them by date, but\n> I actually think it's useful for somebody who switches that behavior to\n> consciously update the test. Maybe it's worth calling out in the commit\n> message.\n>\n> I stopped short of actually documenting and guaranteeing that behavior\n> to the user. I don't know how comfortable we would be changing it later.\n\nI somehow feel that the \"showing all entries from HEAD and then\nshowing all from side\" is simply a buggy behaviour. I do not think\nour users would terribly mind if we changed it later. But I may be\nmissing the reason why (sometimes?) the sequential behaviour may be\nuseful.\n\n> (As an aside, I also prowled through the documentation looking for any\n> guarantees we make to the user about the fake-parent thing, but I\n> couldn't find any. So I considered its user-visible effects an unwanted\n> side effect of the implementation).\n\nYes, I think the corner cases you documented here in these tests are\nnot something we desired to have in the first place. Rather they\nare merely fallouts from the hackyness of the implementation.\n"},{"id":"323887","messageId":"20170706071606.doxirwm6cqx3duzg@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqqa84ih5ao.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-06T07:16:07Z","receivedAt":"2017-07-06T07:16:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 05, 2017 at 03:45:03PM -0700, Junio C Hamano wrote:\n\n> > I did make the ordering intentional. We process each reflog sequentially\n> > in turn. I don't think it would be wrong to interleave them by date, but\n> > I actually think it's useful for somebody who switches that behavior to\n> > consciously update the test. Maybe it's worth calling out in the commit\n> > message.\n> >\n> > I stopped short of actually documenting and guaranteeing that behavior\n> > to the user. I don't know how comfortable we would be changing it later.\n> \n> I somehow feel that the \"showing all entries from HEAD and then\n> showing all from side\" is simply a buggy behaviour. I do not think\n> our users would terribly mind if we changed it later. But I may be\n> missing the reason why (sometimes?) the sequential behaviour may be\n> useful.\n\nIf we think it's buggy, we can fix it now. But I'm not convinced that\nsequential iteration is that bad. It's at least _simple_ and easy to\nexplain. The only other thing that would make sense to me is sorting the\nentries by date (reflog date, not commit date) and showing them in that\norder. But that gives rise to some funny corner cases.\n\nWhat do we do if there's clock skew within the reflog (e.g., somebody\nfixed their skewed clock such that HEAD@{5} is less recent than\nHEAD@{6}). Do we show them out of order then (even if only a single\nreflog is being shown?).\n\nOr do we try some complicated sort where entries compare sequentially\nwithin a given reflog, but use date comparisons between different logs?\nI don't think that can work with a normal comparison sort, as it makes\nthe comparison non-transitive.\n\nBut maybe we could do a pseudo-list-merge, where we treat the reflogs as\nqueues and always pick from the queue with the most recent timestamp.\n\nThat would probably be pretty easy to retrofit on the iteration from the\ncurrent series.\n\n-Peff\n"},{"id":"323888","messageId":"20170706075506.qs7qxpi7uuwz73m2@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170706071606.doxirwm6cqx3duzg@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-06T07:55:06Z","receivedAt":"2017-07-06T07:55:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 06, 2017 at 03:16:06AM -0400, Jeff King wrote:\n\n> If we think it's buggy, we can fix it now. But I'm not convinced that\n> sequential iteration is that bad. It's at least _simple_ and easy to\n> explain. The only other thing that would make sense to me is sorting the\n> entries by date (reflog date, not commit date) and showing them in that\n> order. But that gives rise to some funny corner cases.\n> \n> What do we do if there's clock skew within the reflog (e.g., somebody\n> fixed their skewed clock such that HEAD@{5} is less recent than\n> HEAD@{6}). Do we show them out of order then (even if only a single\n> reflog is being shown?).\n> \n> Or do we try some complicated sort where entries compare sequentially\n> within a given reflog, but use date comparisons between different logs?\n> I don't think that can work with a normal comparison sort, as it makes\n> the comparison non-transitive.\n> \n> But maybe we could do a pseudo-list-merge, where we treat the reflogs as\n> queues and always pick from the queue with the most recent timestamp.\n> \n> That would probably be pretty easy to retrofit on the iteration from the\n> current series.\n\nSomething like the patch below. Looking over the output, the\ninterleaving makes it look confusing (especially if you don't use\n--date, since then the interleaved numbers have no relation to each\nother).\n\nIt would help if we actually had a use case where somebody really did\nwant to show two reflogs from a single command. Then we'd know what they\nwere expecting and which output would be most useful, at least for that\ncase. But I can't really think of why you'd want to do so (which is\nprobably why nobody noticed for years that the current behavior is so\nbroken).\n\n---\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex a7644d944e..6cdf7bfb0f 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -90,7 +90,7 @@ struct commit_reflog {\n \n struct reflog_walk_info {\n \tstruct commit_reflog **logs;\n-\tsize_t nr, alloc, cur;\n+\tsize_t nr, alloc;\n \tstruct string_list complete_reflogs;\n \tstruct commit_reflog *last_commit_reflog;\n };\n@@ -284,26 +284,43 @@ int reflog_walk_empty(struct reflog_walk_info *info)\n \treturn !info || !info->nr;\n }\n \n-struct commit *next_entry_in_log(struct commit_reflog *log)\n+static void skip_unusable_entries(struct commit_reflog *log)\n {\n \twhile (log->recno >= 0) {\n-\t\tstruct reflog_info *entry = &log->reflogs->items[log->recno--];\n+\t\tstruct reflog_info *entry = &log->reflogs->items[log->recno];\n \t\tstruct object *obj = parse_object(&entry->noid);\n \n \t\tif (obj && obj->type == OBJ_COMMIT)\n-\t\t\treturn (struct commit *)obj;\n+\t\t\treturn;\n+\t\tlog->recno--;\n \t}\n-\treturn NULL;\n+}\n+\n+static timestamp_t next_timestamp(struct commit_reflog *log)\n+{\n+\treturn log->reflogs->items[log->recno].timestamp;\n }\n \n struct commit *next_reflog_entry(struct reflog_walk_info *walk)\n {\n-\tfor (; walk->cur < walk->nr; walk->cur++) {\n-\t\tstruct commit *ret = next_entry_in_log(walk->logs[walk->cur]);\n-\t\tif (ret) {\n-\t\t\twalk->last_commit_reflog = walk->logs[walk->cur];\n-\t\t\treturn ret;\n-\t\t}\n+\tstruct commit_reflog *best = NULL;\n+\tsize_t i;\n+\n+\tfor (i = 0; i < walk->nr; i++) {\n+\t\tstruct commit_reflog *log = walk->logs[i];\n+\n+\t\tskip_unusable_entries(log);\n+\t\tif (log->recno < 0)\n+\t\t\tcontinue;\n+\n+\t\tif (!best || next_timestamp(log) > next_timestamp(best))\n+\t\t\tbest = log;\n \t}\n+\n+\tif (best) {\n+\t\twalk->last_commit_reflog = best;\n+\t\treturn lookup_commit(&best->reflogs->items[best->recno--].noid);\n+\t}\n+\n \treturn NULL;\n }\n\nThe implementation is fairly naive. It would be O(nr_reflogs *\nnr_entries) in the worst case. I wouldn't expect nr_reflogs to get\nlarge. But if we cared, we could fix it by keeping the reflogs in a\nheap.\n\n-Peff\n"},{"id":"323896","messageId":"xmqq1spth8qy.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170706071606.doxirwm6cqx3duzg@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-06T15:42:45Z","receivedAt":"2017-07-06T15:42:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jul 05, 2017 at 03:45:03PM -0700, Junio C Hamano wrote:\n>\n>> > I did make the ordering intentional. We process each reflog sequentially\n>> > in turn. I don't think it would be wrong to interleave them by date, but\n>> > I actually think it's useful for somebody who switches that behavior to\n>> > consciously update the test. Maybe it's worth calling out in the commit\n>> > message.\n>> >\n>> > I stopped short of actually documenting and guaranteeing that behavior\n>> > to the user. I don't know how comfortable we would be changing it later.\n>> \n>> I somehow feel that the \"showing all entries from HEAD and then\n>> showing all from side\" is simply a buggy behaviour. I do not think\n>> our users would terribly mind if we changed it later. But I may be\n>> missing the reason why (sometimes?) the sequential behaviour may be\n>> useful.\n>\n> If we think it's buggy, we can fix it now. But I'm not convinced that\n> sequential iteration is that bad. It's at least _simple_ and easy to\n> explain.\n\nYes, I agree that sequential is easy to explain, but only when I\nconsider use of \"log\" family without \"-n 30\" or \"--since 3.days\".\nIt still is easy to explain---we show from one and then from the\nother, but because we stop after showing 30 of them, and the first\none has more than that, you do not see any from the latter. \n\nIt's just the easy-to-explian behaviour is not very useful and very\nhard to defend.\n\n> The only other thing that would make sense to me is sorting the\n> entries by date (reflog date, not commit date) and showing them in that\n> order. But that gives rise to some funny corner cases.\n>\n> What do we do if there's clock skew within the reflog (e.g., somebody\n> fixed their skewed clock such that HEAD@{5} is less recent than\n> HEAD@{6}). Do we show them out of order then (even if only a single\n> reflog is being shown?).\n\nI would think the easiest-to-explain thing is handle clock skew just\nlike how clock skew affects the traversal with commit date. At\nleast inside a single reflog, there is no need to sort---its\nappend-only nature gives a reliable \"newer to older\" order.\n\n> But maybe we could do a pseudo-list-merge, where we treat the reflogs as\n> queues and always pick from the queue with the most recent timestamp.\n\nYes, that was what I had in mind. An entry with an artificially old\ntimestamp due to clock skew would clog the output and prevent the\nentries behind it on the same reflog from getting shown before\nentries from other reflog, but that is the same as what happens in\nthe normal traversal to sane parents that can only be reached from a\nchild commit that has an artificially old timestamp.\n\nThanks.\n\n"},{"id":"323918","messageId":"20170707003239.GA13431@untitled","threadId":"46227","inReplyTo":"20170705080904.idlpm6ecslzfbm75@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] reflog-walk: stop using fake parents","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2017-07-07T00:32:39Z","receivedAt":"2017-07-07T00:32:45Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"I'm not sure why, but this is causing t1414.8 failures on 32-bit\nx86 with the latest pu with Debian jessie (oldstable).\n\nReverting this (beafb2c62947a6d4a97b9c3baf99fe62ec8e830f) in pu\nseems to fix the test for me.\n\n+Cc: Ramsay since he also had a 32-bit environment.\n\n--8<--\nok 7 - --parents shows true parents\n\nexpecting success: \n\t{\n\t\tdo_walk HEAD &&\n\t\tdo_walk side\n\t} >expect &&\n\tdo_walk HEAD side >actual &&\n\ttest_cmp expect actual\n\n--- expect\t2017-07-07 00:30:57.796325232 +0000\n+++ actual\t2017-07-07 00:30:57.796325232 +0000\n@@ -3,6 +3,8 @@\n HEAD@{2} checkout: moving from master to side\n HEAD@{3} commit: two\n HEAD@{4} commit (initial): one\n-side@{0} commit (merge): Merge branch 'master' into side\n-side@{1} commit: three\n-side@{2} branch: Created from HEAD^\n+HEAD@{0} commit (merge): Merge branch 'master' into side\n+HEAD@{1} commit: three\n+HEAD@{2} checkout: moving from master to side\n+HEAD@{3} commit: two\n+HEAD@{4} commit (initial): one\nnot ok 8 - walking multiple reflogs shows both\n#\t\n#\t\t{\n#\t\t\tdo_walk HEAD &&\n#\t\t\tdo_walk side\n#\t\t} >expect &&\n#\t\tdo_walk HEAD side >actual &&\n#\t\ttest_cmp expect actual\n#\t\n\nexpecting success: \n\thead=$(git rev-parse HEAD) &&\n\tone=$(git rev-parse one) &&\n\tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\" &&\n\techo \"$head $one $ident\tbroken reflog entry\" >>.git/logs/HEAD &&\n\n\techo $one >expect &&\n\tgit log -g --format=%H -1 >actual &&\n\ttest_cmp expect actual\n\nok 9 - walk prefers reflog to ref tip\n"},{"id":"323922","messageId":"20170707030224.4iubuzsz2qwdhgiw@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707003239.GA13431@untitled","subject":"Re: [PATCH 6/6] reflog-walk: stop using fake parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T03:02:24Z","receivedAt":"2017-07-07T03:02:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 07, 2017 at 12:32:39AM +0000, Eric Wong wrote:\n\n> I'm not sure why, but this is causing t1414.8 failures on 32-bit\n> x86 with the latest pu with Debian jessie (oldstable).\n> \n> Reverting this (beafb2c62947a6d4a97b9c3baf99fe62ec8e830f) in pu\n> seems to fix the test for me.\n> \n> +Cc: Ramsay since he also had a 32-bit environment.\n\nThanks, I was able to reproduce with CFLAGS=-m32.\n\n> --- expect\t2017-07-07 00:30:57.796325232 +0000\n> +++ actual\t2017-07-07 00:30:57.796325232 +0000\n> @@ -3,6 +3,8 @@\n> HEAD@{2} checkout: moving from master to side\n> HEAD@{3} commit: two\n> HEAD@{4} commit (initial): one\n> -side@{0} commit (merge): Merge branch 'master' into side\n> -side@{1} commit: three\n> -side@{2} branch: Created from HEAD^\n> +HEAD@{0} commit (merge): Merge branch 'master' into side\n> +HEAD@{1} commit: three\n> +HEAD@{2} checkout: moving from master to side\n> +HEAD@{3} commit: two\n> +HEAD@{4} commit (initial): one\n\nThat's quite an unexpected error (to show the HEAD reflog twice). Given\nthat it triggers with 32-bit builds, it's like there's some funny memory\nerror. And indeed, running it under valgrind shows a problem in\nadd_reflog_for_walk. I'll try to dig into it. Thanks for reporting.\n\n-Peff\n"},{"id":"323923","messageId":"20170707031508.kujsp6lsbgxdygvq@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707030224.4iubuzsz2qwdhgiw@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] reflog-walk: stop using fake parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T03:15:09Z","receivedAt":"2017-07-07T03:15:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 06, 2017 at 11:02:24PM -0400, Jeff King wrote:\n\n> On Fri, Jul 07, 2017 at 12:32:39AM +0000, Eric Wong wrote:\n> \n> > I'm not sure why, but this is causing t1414.8 failures on 32-bit\n> > x86 with the latest pu with Debian jessie (oldstable).\n> > \n> > Reverting this (beafb2c62947a6d4a97b9c3baf99fe62ec8e830f) in pu\n> > seems to fix the test for me.\n> > \n> > +Cc: Ramsay since he also had a 32-bit environment.\n> \n> Thanks, I was able to reproduce with CFLAGS=-m32.\n> \n> > --- expect\t2017-07-07 00:30:57.796325232 +0000\n> > +++ actual\t2017-07-07 00:30:57.796325232 +0000\n> > @@ -3,6 +3,8 @@\n> > HEAD@{2} checkout: moving from master to side\n> > HEAD@{3} commit: two\n> > HEAD@{4} commit (initial): one\n> > -side@{0} commit (merge): Merge branch 'master' into side\n> > -side@{1} commit: three\n> > -side@{2} branch: Created from HEAD^\n> > +HEAD@{0} commit (merge): Merge branch 'master' into side\n> > +HEAD@{1} commit: three\n> > +HEAD@{2} checkout: moving from master to side\n> > +HEAD@{3} commit: two\n> > +HEAD@{4} commit (initial): one\n> \n> That's quite an unexpected error (to show the HEAD reflog twice). Given\n> that it triggers with 32-bit builds, it's like there's some funny memory\n> error. And indeed, running it under valgrind shows a problem in\n> add_reflog_for_walk. I'll try to dig into it. Thanks for reporting.\n\nAh, it's a bug in the existing code. It inserts an entry into a string\nlist and then frees the memory, but the string list wasn't asked to make\na copy of the string. You can see the bug by running t1414 with\n--valgrind even before any of the code changes (though note that you'll\nhave to look at the \"-v\" output, since it's already marked as\nexpect-failure).\n\nThis fixes it:\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex b7e489ad32..c1f61c524a 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -136,6 +136,7 @@ struct reflog_walk_info {\n void init_reflog_walk(struct reflog_walk_info **info)\n {\n \t*info = xcalloc(1, sizeof(struct reflog_walk_info));\n+\t(*info)->complete_reflogs.strdup_strings = 1;\n }\n \n int add_reflog_for_walk(struct reflog_walk_info *info,\n\nI'll add that to the 'maint' portion of the series.\n\n-Peff\n"},{"id":"323924","messageId":"20170707051954.rpej3taew6onv3oo@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqq1spth8qy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T05:19:54Z","receivedAt":"2017-07-07T05:20:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 06, 2017 at 08:42:45AM -0700, Junio C Hamano wrote:\n\n> >> I somehow feel that the \"showing all entries from HEAD and then\n> >> showing all from side\" is simply a buggy behaviour. I do not think\n> >> our users would terribly mind if we changed it later. But I may be\n> >> missing the reason why (sometimes?) the sequential behaviour may be\n> >> useful.\n> >\n> > If we think it's buggy, we can fix it now. But I'm not convinced that\n> > sequential iteration is that bad. It's at least _simple_ and easy to\n> > explain.\n> \n> Yes, I agree that sequential is easy to explain, but only when I\n> consider use of \"log\" family without \"-n 30\" or \"--since 3.days\".\n> It still is easy to explain---we show from one and then from the\n> other, but because we stop after showing 30 of them, and the first\n> one has more than that, you do not see any from the latter.\n\nAh, right, I didn't think of limiting like that. I agree that makes a\nstrong argument in favor of the date-ordered queue.\n\nI suspect that \"--since 3.days\" is still quite buggy (even with a single\nreflog) because it checks commit timestamps and stops traversing when we\ngo too bar back. But in a reflog, the commits may be totally out of\norder. I'm not sure what it should do. Either:\n\n 1. During a reflog walk, --since and --until should respect reflog\n timestamps instead of commit timestamps. You can already do\n \"--until\" there by simply starting the traversal later, but there's\n no way to cut it off with --since.\n\n 2. Limit commits shown by --since/--until as usual, but skip the \"stop\n traversing\" optimization when we see too many \"old\" commits. I.e.,\n omit a 4.days.ago commit, but keep walking to find other recent\n commits.\n\n-Peff\n"},{"id":"323925","messageId":"xmqq37a8eqhe.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170707051954.rpej3taew6onv3oo@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-07T06:00:13Z","receivedAt":"2017-07-07T06:00:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I suspect that \"--since 3.days\" is still quite buggy (even with a single\n> reflog) because it checks commit timestamps and stops traversing when we\n> go too bar back. But in a reflog, the commits may be totally out of\n> order. I'm not sure what it should do. Either:\n>\n> 1. During a reflog walk, --since and --until should respect reflog\n> timestamps instead of commit timestamps. You can already do\n> \"--until\" there by simply starting the traversal later, but there's\n> no way to cut it off with --since.\n>\n> 2. Limit commits shown by --since/--until as usual, but skip the \"stop\n> traversing\" optimization when we see too many \"old\" commits. I.e.,\n> omit a 4.days.ago commit, but keep walking to find other recent\n> commits.\n\nI think 1. is more logical, and I was imagining that it should be\ndoable, now we are not constrained by (ab)using the commit_list with\nthe fake-parent thing, but are pulling the entries directly from the\nreflog iterator and the timestamp would be already available to the\niterator.\n\nBut I recall that the max_age and min_age cutoff is done long after\na commit is pulled out of the \"iterator mechanism\" (be it the\ncommit_list or your direct reflog iterator) by comparing\ncommit->date with the cut-off, so it may be a bit more involved to\narrange than I imagined X-<. Hmph...\n\n\n"},{"id":"323927","messageId":"20170707062211.a7336zi7rn7pal3z@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqq37a8eqhe.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/6] t1414: document some reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T06:22:11Z","receivedAt":"2017-07-07T06:22:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 06, 2017 at 11:00:13PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I suspect that \"--since 3.days\" is still quite buggy (even with a single\n> > reflog) because it checks commit timestamps and stops traversing when we\n> > go too bar back. But in a reflog, the commits may be totally out of\n> > order. I'm not sure what it should do. Either:\n> >\n> > 1. During a reflog walk, --since and --until should respect reflog\n> > timestamps instead of commit timestamps. You can already do\n> > \"--until\" there by simply starting the traversal later, but there's\n> > no way to cut it off with --since.\n> >\n> > 2. Limit commits shown by --since/--until as usual, but skip the \"stop\n> > traversing\" optimization when we see too many \"old\" commits. I.e.,\n> > omit a 4.days.ago commit, but keep walking to find other recent\n> > commits.\n> \n> I think 1. is more logical, and I was imagining that it should be\n> doable, now we are not constrained by (ab)using the commit_list with\n> the fake-parent thing, but are pulling the entries directly from the\n> reflog iterator and the timestamp would be already available to the\n> iterator.\n> \n> But I recall that the max_age and min_age cutoff is done long after\n> a commit is pulled out of the \"iterator mechanism\" (be it the\n> commit_list or your direct reflog iterator) by comparing\n> commit->date with the cut-off, so it may be a bit more involved to\n> arrange than I imagined X-<. Hmph...\n\nIt's probably not too bad.\n\nWe do some of the limiting in limit_list(), which tries to mark commits\nas UNINTERESTING. But I think in general that limit_list is incompatible\nwith reflog traversals (though I wouldn't be surprised if we fail to\nflag all the options that set revs->limited as incompatible).\n\nWe handle max_age in get_revision_1() itself, which should be pretty\nstraightforward. For min_age, we do it in get_commit_action(), which\nwould need access to the reflog timestamp. But we do have the rev_info\nthere.\n\nSo something like the patch below would work, I think.\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex fbee9e0126..74ebe5148f 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -264,6 +264,18 @@ const char *get_reflog_ident(struct reflog_walk_info *reflog_info)\n \treturn info->email;\n }\n \n+timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info)\n+{\n+\tstruct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;\n+\tstruct reflog_info *info;\n+\n+\tif (!commit_reflog)\n+\t\treturn 0;\n+\n+\tinfo = &commit_reflog->reflogs->items[commit_reflog->recno+1];\n+\treturn info->timestamp;\n+}\n+\n void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n \t\t\t const struct date_mode *dmode, int force_date)\n {\ndiff --git a/reflog-walk.h b/reflog-walk.h\nindex 373388cd14..7553c448fe 100644\n--- a/reflog-walk.h\n+++ b/reflog-walk.h\n@@ -13,6 +13,7 @@ extern void show_reflog_message(struct reflog_walk_info *info, int,\n extern void get_reflog_message(struct strbuf *sb,\n \t\tstruct reflog_walk_info *reflog_info);\n extern const char *get_reflog_ident(struct reflog_walk_info *reflog_info);\n+extern timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info);\n extern void get_reflog_selector(struct strbuf *sb,\n \t\tstruct reflog_walk_info *reflog_info,\n \t\tconst struct date_mode *dmode, int force_date,\ndiff --git a/revision.c b/revision.c\nindex 5fc01f2d26..c248a16974 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2973,8 +2973,13 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n \t\treturn commit_show;\n \tif (commit->object.flags & UNINTERESTING)\n \t\treturn commit_ignore;\n-\tif (revs->min_age != -1 && (commit->date > revs->min_age))\n-\t\treturn commit_ignore;\n+\tif (revs->min_age != -1) {\n+\t\ttimestamp_t date = revs->reflog_info ?\n+\t\t\t\t get_reflog_timestamp(revs->reflog_info) :\n+\t\t\t\t commit->date;\n+\t\tif (date > revs->min_age)\n+\t\t\treturn commit_ignore;\n+\t}\n \tif (revs->min_parents || (revs->max_parents >= 0)) {\n \t\tint n = commit_list_count(commit->parents);\n \t\tif ((n < revs->min_parents) ||\n@@ -3127,9 +3132,13 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\t * that we'd otherwise have done in limit_list().\n \t\t */\n \t\tif (!revs->limited) {\n-\t\t\tif (revs->max_age != -1 &&\n-\t\t\t (commit->date < revs->max_age))\n-\t\t\t\tcontinue;\n+\t\t\tif (revs->max_age != -1) {\n+\t\t\t\ttimestamp_t date = revs->reflog_info ?\n+\t\t\t\t\t\t get_reflog_timestamp(revs->reflog_info) :\n+\t\t\t\t\t\t commit->date;\n+\t\t\t\tif (date < revs->max_age)\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!revs->reflog_info &&\n \t\t\t add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {\n \t\t\t\tif (!revs->ignore_missing_links)\n"},{"id":"323928","messageId":"20170707083636.kjsr5ry3237paeiv@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170705075508.c5ul23vivzpklpy6@sigill.intra.peff.net","subject":"[PATCH v2 0/4] reflog-walk fixes for maint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T08:36:37Z","receivedAt":"2017-07-07T08:36:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 05, 2017 at 03:55:08AM -0400, Jeff King wrote:\n\n> The first patch is my original small fix with an extra test. I think\n> that would be appropriate for 'maint'. Its behavior still has some\n> quirks, but it avoids the confusion that you experienced and has a low\n> risk of breaking anything else.\n> \n> The rest of it replaces the fake-parent thing with a more\n> straight-forward iteration over the reflogs (i.e., a cleanup of the\n> further patches I've been posting). After digging into it and especially\n> after writing the new tests, I think I've convinced myself that this is\n> the right way forward.\n\nHere's an updated version of the bug-fix patch, along with the fix for\nthe problem that Eric noticed, and some other problems I noticed while\nfixing that one. So I've split these immediate fixes for maint off into\ntheir own series.\n\nThese are based on maint itself, rather than Kyle's original commit that\nintroduces the double-null reflog, since some of the bugs came later in\nthe v2.13 cycle (if we really wanted to, we could split it again into\ntwo more series, but I don't think it's worth the trouble).\n\n [1/4]: reflog-walk: skip over double-null oid due to HEAD rename\n\n This is the fix for the pseudo-truncation in v2.13, and is the same\n as the previous round.\n\n [2/4]: reflog-walk: duplicate strings in complete_reflogs list\n\n This fixes Eric's bug, and is the same as what I showed earlier.\n It's a triggerable use-after-free, which is why I think it's\n important to get it into maint.\n\n [3/4]: reflog-walk: don't free reflogs added to cache\n\n This is another use-after-free, though it's slightly harder to\n trigger.\n\n [4/4]: reflog-walk: include all fields when freeing complete_reflogs\n\n This one is an optional cleanup, but worth doing, I think.\n\n\n reflog-walk.c | 33 +++++++++++++++++++++------------\n t/t1411-reflog-show.sh | 10 ++++++++++\n t/t3200-branch.sh | 11 +++++++++++\n 3 files changed, 42 insertions(+), 12 deletions(-)\n\n-Peff\n"},{"id":"323929","messageId":"20170707083710.35efbobjysrti3w2@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707083636.kjsr5ry3237paeiv@sigill.intra.peff.net","subject":"[PATCH v2 1/4] reflog-walk: skip over double-null oid due to HEAD rename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T08:37:11Z","receivedAt":"2017-07-07T08:37:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 39ee4c6c2f (branch: record creation of renamed branch\nin HEAD's log, 2017-02-20), a rename on the currently\nchecked out branch will create two entries in the HEAD\nreflog: one where the branch goes away (switching to the\nnull oid), and one where it comes back (switching away from\nthe null oid).\n\nThis confuses the reflog-walk code. When walking backwards,\nit first sees the null oid in the \"old\" field of the second\nentry. Thanks to the \"root commit\" logic added by 71abeb753f\n(reflog: continue walking the reflog past root commits,\n2016-06-03), we keep looking for the next entry by scanning\nthe \"new\" field from the previous entry. But that field is\nalso null! We need to go just a tiny bit further, and look\nat its \"old\" field. But with the current code, we decide the\nreflog has nothing else to show and just give up. To the\nuser this looks like the reflog was truncated by the rename\noperation, when in fact those entries are still there.\n\nThis patch does the absolute minimal fix, which is to look\nback that one extra level and keep traversing.\n\nThe resulting behavior may not be the _best_ thing to do in\nthe long run (for example, we show both reflog entries each\nwith the same commit id), but it's a simple way to fix the\nproblem without risking further regressions.\n\nSigned-off-by: Jeff King \n---\n reflog-walk.c | 2 ++\n t/t3200-branch.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex c63eb1a3fd..f7ffd1caa3 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \t\t/* a root commit, but there are still more entries to show */\n \t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n \t\tlogobj = parse_object(reflog->noid.hash);\n+\t\tif (!logobj)\n+\t\t\tlogobj = parse_object(reflog->ooid.hash);\n \t}\n \n \tif (!logobj || logobj->type != OBJ_COMMIT) {\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 48d152b9a9..dd37ac47c5 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -162,6 +162,17 @@ test_expect_success 'git branch -M baz bam should add entries to .git/logs/HEAD'\n \tgrep \"^0\\{40\\}.*$msg$\" .git/logs/HEAD\n '\n \n+test_expect_success 'resulting reflog can be shown by log -g' '\n+\toid=$(git rev-parse HEAD) &&\n+\tcat >expect <<-EOF &&\n+\tHEAD@{0} $oid $msg\n+\tHEAD@{1} $oid $msg\n+\tHEAD@{2} $oid checkout: moving from foo to baz\n+\tEOF\n+\tgit log -g --format=\"%gd %H %gs\" -3 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '\n \tgit checkout master &&\n \tgit worktree add -b baz bazdir &&\n-- \n2.13.2.1000.g8590c1af5d\n\n"},{"id":"323930","messageId":"20170707083950.et7j6xzlvapggp2l@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707083636.kjsr5ry3237paeiv@sigill.intra.peff.net","subject":"[PATCH v2 2/4] reflog-walk: duplicate strings in complete_reflogs list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T08:39:50Z","receivedAt":"2017-07-07T08:40:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As part of the add_reflog_to_walk() function, we keep a\nstring_list mapping refnames to their reflog contents. This\nserves as a cache so that accessing the same reflog twice\nrequires only a single copy of the log in memory.\n\nThe string_list is initialized via xcalloc, meaning its\nstrdup_strings field is set to 0. But after inserting a\nstring into the list, we unconditionally call free() on the\nstring, leaving the list pointing to freed memory. If\nanother reflog is added (e.g., \"git log -g HEAD HEAD\"), then\nthe second one may have unpredictable results.\n\nThe extra free was added by 5026b47175 (add_reflog_for_walk:\navoid memory leak, 2017-05-04). Though if you look\ncarefully, you can see that the code was buggy even before\nthen. If we tried to read the reflogs by time but came up\nwith no entries, we exited with an error, freeing the string\nin that code path. So the bug was harder to trigger, but\nstill there.\n\nWe can fix it by just asking the string list to make a copy\nof the string. Technically we could fix the problem by not\ncalling free() on our string (and just handing over\nownership to the string list), but there are enough\nconditionals that it's quite hard to figure out which code\npaths need the free and which do not. Simpler is better\nhere.\n\nThe new test reliably shows the problem when run with\n--valgrind or ASAN.\n\nSigned-off-by: Jeff King \n---\n reflog-walk.c | 1 +\n t/t1411-reflog-show.sh | 6 ++++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex f7ffd1caa3..aec718beba 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -136,6 +136,7 @@ struct reflog_walk_info {\n void init_reflog_walk(struct reflog_walk_info **info)\n {\n \t*info = xcalloc(1, sizeof(struct reflog_walk_info));\n+\t(*info)->complete_reflogs.strdup_strings = 1;\n }\n \n int add_reflog_for_walk(struct reflog_walk_info *info,\ndiff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh\nindex 6ac7734d79..ae96eeb66a 100755\n--- a/t/t1411-reflog-show.sh\n+++ b/t/t1411-reflog-show.sh\n@@ -171,4 +171,10 @@ test_expect_success 'reflog exists works' '\n \t! git reflog exists refs/heads/nonexistent\n '\n \n+# The behavior with two reflogs is buggy and the output is in flux; for now\n+# we're just checking that the program works at all without segfaulting.\n+test_expect_success 'showing multiple reflogs works' '\n+\tgit log -g HEAD HEAD >actual\n+'\n+\n test_done\n-- \n2.13.2.1000.g8590c1af5d\n\n"},{"id":"323931","messageId":"20170707084148.albskufmqkeyfedn@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707083636.kjsr5ry3237paeiv@sigill.intra.peff.net","subject":"[PATCH v2 3/4] reflog-walk: don't free reflogs added to cache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T08:41:49Z","receivedAt":"2017-07-07T08:42:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The add_reflog_for_walk() function keeps a cache mapping\nrefnames to their reflog contents. We use a cached reflog\nentry if available, and otherwise allocate and store a new\none.\n\nSince 5026b47175 (add_reflog_for_walk: avoid memory leak,\n2017-05-04), when we hit an error parsing a date-based\nreflog spec, we free the reflog memory but leave the cache\nentry pointing to the now-freed memory.\n\nWe can fix this by just leaving the memory intact once it\nhas made it into the cache. This may leave an unused entry\nin the cache, but that's OK. And it means we also catch a\nsimilar situation: we may not have allocated at all in this\ninvocation, but simply be pointing to a cached entry from a\nprevious invocation (which is relying on that entry being\npresent).\n\nThe new test in t1411 exercises this case and fails when run\nwith --valgrind or ASan.\n\nSigned-off-by: Jeff King \n---\n reflog-walk.c | 4 ----\n t/t1411-reflog-show.sh | 4 ++++\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex aec718beba..2a43537326 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -215,10 +215,6 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n \tif (recno < 0) {\n \t\tcommit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp);\n \t\tif (commit_reflog->recno < 0) {\n-\t\t\tif (reflogs) {\n-\t\t\t\tfree(reflogs->ref);\n-\t\t\t\tfree(reflogs);\n-\t\t\t}\n \t\t\tfree(commit_reflog);\n \t\t\treturn -1;\n \t\t}\ndiff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh\nindex ae96eeb66a..b9cb76654b 100755\n--- a/t/t1411-reflog-show.sh\n+++ b/t/t1411-reflog-show.sh\n@@ -177,4 +177,8 @@ test_expect_success 'showing multiple reflogs works' '\n \tgit log -g HEAD HEAD >actual\n '\n \n+test_expect_success 'showing multiple reflogs with an old date' '\n+\tgit log -g HEAD@{1979-01-01} HEAD >actual\n+'\n+\n test_done\n-- \n2.13.2.1000.g8590c1af5d\n\n"},{"id":"323932","messageId":"20170707084316.jetdonpj75ricara@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707083636.kjsr5ry3237paeiv@sigill.intra.peff.net","subject":"[PATCH v2 4/4] reflog-walk: include all fields when freeing complete_reflogs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T08:43:16Z","receivedAt":"2017-07-07T08:43:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we encounter an error adding reflogs for a walk, we try\nto free any logs we have read. But we didn't free all\nfields, meaning that we could in theory leak all of the\n\"items\" array (which would consitute the bulk of the\nallocated memory).\n\nThis patch adds a helper which frees all of the entries and\nuses it as appropriate.\n\nAs it turns out, the leak seems impossible to trigger with\nthe current code. Of the three error paths that free the\ncomplete_reflogs struct, two only kick in when the items\narray is empty, and the third was removed entirely in the\nprevious commit.\n\nSo this patch should be a noop in terms of behavior, but it\nfixes a potential maintenance headache should anybody add a\nnew error path and copy the partial-free code. Which is\nwhat happened in 5026b47175 (add_reflog_for_walk: avoid\nmemory leak, 2017-05-04), though its leaky call was the\nthird one that was recently removed.\n\nSigned-off-by: Jeff King \n---\n reflog-walk.c | 26 ++++++++++++++++++--------\n 1 file changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 2a43537326..ba72020fc3 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -38,6 +38,22 @@ static int read_one_reflog(struct object_id *ooid, struct object_id *noid,\n \treturn 0;\n }\n \n+static void free_complete_reflog(struct complete_reflogs *array)\n+{\n+\tint i;\n+\n+\tif (!array)\n+\t\treturn;\n+\n+\tfor (i = 0; i < array->nr; i++) {\n+\t\tfree(array->items[i].email);\n+\t\tfree(array->items[i].message);\n+\t}\n+\tfree(array->items);\n+\tfree(array->ref);\n+\tfree(array);\n+}\n+\n static struct complete_reflogs *read_complete_reflog(const char *ref)\n {\n \tstruct complete_reflogs *reflogs =\n@@ -189,20 +205,14 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n \t\t\tif (ret > 1)\n \t\t\t\tfree(b);\n \t\t\telse if (ret == 1) {\n-\t\t\t\tif (reflogs) {\n-\t\t\t\t\tfree(reflogs->ref);\n-\t\t\t\t\tfree(reflogs);\n-\t\t\t\t}\n+\t\t\t\tfree_complete_reflog(reflogs);\n \t\t\t\tfree(branch);\n \t\t\t\tbranch = b;\n \t\t\t\treflogs = read_complete_reflog(branch);\n \t\t\t}\n \t\t}\n \t\tif (!reflogs || reflogs->nr == 0) {\n-\t\t\tif (reflogs) {\n-\t\t\t\tfree(reflogs->ref);\n-\t\t\t\tfree(reflogs);\n-\t\t\t}\n+\t\t\tfree_complete_reflog(reflogs);\n \t\t\tfree(branch);\n \t\t\treturn -1;\n \t\t}\n-- \n2.13.2.1000.g8590c1af5d\n"},{"id":"323933","messageId":"20170707090507.ko2ygry7j4zv7t3s@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707083636.kjsr5ry3237paeiv@sigill.intra.peff.net","subject":"[PATCH v2 0/7] fixing reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T09:05:07Z","receivedAt":"2017-07-07T09:05:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 07, 2017 at 04:36:36AM -0400, Jeff King wrote:\n\n> Here's an updated version of the bug-fix patch, along with the fix for\n> the problem that Eric noticed, and some other problems I noticed while\n> fixing that one. So I've split these immediate fixes for maint off into\n> their own series.\n\nAnd here's the parent-less walk for master. This must be applied on a\nmerge of the bug-fix series to the current master. I didn't do it all on\n\"maint\" because a lot of it depends on the timestamp_t work which is\nonly in master. And trying to use \"unsigned long\" and merge that up to\nmaster correctly is error-prone. It not only doesn't create textual\nconflicts, but the semantic conflicts it generates are really subtle and\nonly break on certain systems.\n\nThis should address all of the comments on v1, including the\nmulti-reflog iteration order and the --since/--until bits. There are a\nfew new fixes, too.\n\n [v2 1/7]: t1414: document some reflog-walk oddities\n\n The big change is that this expects the interleaved order in the\n multi-reflog test. There are a few updated comments and some\n adaptations to cover some bits from the bug-fix series.\n\n [v2 2/7]: revision: disallow reflog walking with revs->limited\n\n This is new, and just cleanly disallows some already-broken cases.\n\n [v2 3/7]: log: do not free parents when walking reflog\n [v2 4/7]: get_revision_1(): replace do-while with an early return\n [v2 5/7]: rev-list: check reflog_info before showing usage\n\n These ones are the same as before.\n\n [v2 6/7]: reflog-walk: stop using fake parents\n\n The big change here is the interleaved output. This is largely what\n I showed earlier, but with a few cleanups.\n\n [v2 7/7]: reflog-walk: apply --since/--until to reflog dates\n\n This is new, and is a cleaned-up version of what I showed earlier.\n See the commit message for some discussion. These options _do_\n actually work sanely after 6/7, but I think the semantics given here\n are probably more useful and what people would expect.\n\n builtin/log.c | 4 +-\n builtin/rev-list.c | 3 +-\n reflog-walk.c | 152 ++++++++++++++++++++++---------------------------\n reflog-walk.h | 7 ++-\n revision.c | 57 ++++++++++++-------\n t/t1411-reflog-show.sh | 10 ----\n t/t1414-reflog-walk.sh | 135 +++++++++++++++++++++++++++++++++++++++++++\n t/t3200-branch.sh | 3 +-\n 8 files changed, 249 insertions(+), 122 deletions(-)\n create mode 100755 t/t1414-reflog-walk.sh\n\n-Peff\n"},{"id":"323934","messageId":"20170707090610.x2vodxgc7yr6o5pi@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707090507.ko2ygry7j4zv7t3s@sigill.intra.peff.net","subject":"[PATCH v2 1/7] t1414: document some reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T09:06:10Z","receivedAt":"2017-07-07T09:06:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since its inception, the general strategy of the reflog-walk\ncode has been to start with the tip commit for the ref, and\nas we traverse replace each commit's parent pointers with\nfake parents pointing to the previous reflog entry.\n\nThis lets us traverse the reflog as if it were a real\nhistory, but it has some user-visible oddities. Namely:\n\n 1. The fake parents are used for commit selection and\n display. So for example, \"--merges\" or \"--no-merges\"\n are not useful, because the history appears as a linear\n string of commits. Likewise, pathspec limiting is based\n on the diff between adjacent entries, not the changes\n actually introduced by a commit.\n\n These are often the same (e.g., because the entry was\n just running \"git commit\" and the adjacent entry _is_\n the true parent), but it may not be in several common\n cases. For instance, using \"git reset\" to jump around\n history, or \"git checkout\" to move HEAD.\n\n 2. We reverse-map each commit back to its reflog. So when\n it comes time to show commit X, we say \"a-ha, we added\n X because it was at the tip of the 'foo' reflog, so\n let's show the foo reflog\". But this leads to nonsense\n results when you ask to traverse multiple reflogs: if\n two reflogs have the same tip commit, we only map back\n to one of them. Instead, we should show both.\n\n 3. If the tip of the reflog and the ref tip disagree on\n the current value, we show the ref tip but give no\n indication of the value in the reflog. This situation\n isn't supposed to happen (since any ref update should\n touch the reflog). But if it does, given that the\n requested operation is to show the reflog, it makes\n sense to prefer that.\n\nThis commit adds a new script with several expect_failure\ntests to demonstrate the problems. This could be part of\nthe existing t1411, but it's a bit easier to start from a\nfresh state, where we know exactly what will be in the log.\n\nSince the new multiple-reflog tests are checking the actual\noutput, we can drop the \"make sure we don't segfault\" tests\nfrom t1411, which are a strict subset of what we're doing\nhere.\n\nSigned-off-by: Jeff King \n---\n t/t1411-reflog-show.sh | 10 -----\n t/t1414-reflog-walk.sh | 105 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 105 insertions(+), 10 deletions(-)\n create mode 100755 t/t1414-reflog-walk.sh\n\ndiff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh\nindex b9cb76654b..6ac7734d79 100755\n--- a/t/t1411-reflog-show.sh\n+++ b/t/t1411-reflog-show.sh\n@@ -171,14 +171,4 @@ test_expect_success 'reflog exists works' '\n \t! git reflog exists refs/heads/nonexistent\n '\n \n-# The behavior with two reflogs is buggy and the output is in flux; for now\n-# we're just checking that the program works at all without segfaulting.\n-test_expect_success 'showing multiple reflogs works' '\n-\tgit log -g HEAD HEAD >actual\n-'\n-\n-test_expect_success 'showing multiple reflogs with an old date' '\n-\tgit log -g HEAD@{1979-01-01} HEAD >actual\n-'\n-\n test_done\ndiff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh\nnew file mode 100755\nindex 0000000000..6426829352\n--- /dev/null\n+++ b/t/t1414-reflog-walk.sh\n@@ -0,0 +1,105 @@\n+#!/bin/sh\n+\n+test_description='various tests of reflog walk (log -g) behavior'\n+. ./test-lib.sh\n+\n+test_expect_success 'set up some reflog entries' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\tgit checkout -b side HEAD^ &&\n+\ttest_commit three &&\n+\tgit merge --no-commit master &&\n+\techo evil-merge-content >>one.t &&\n+\ttest_tick &&\n+\tgit commit --no-edit -a\n+'\n+\n+do_walk () {\n+\tgit log -g --format=\"%gd %gs\" \"$@\"\n+}\n+\n+sq=\"'\"\n+test_expect_success 'set up expected reflog' '\n+\tcat >expect.all <<-EOF\n+\tHEAD@{0} commit (merge): Merge branch ${sq}master${sq} into side\n+\tHEAD@{1} commit: three\n+\tHEAD@{2} checkout: moving from master to side\n+\tHEAD@{3} commit: two\n+\tHEAD@{4} commit (initial): one\n+\tEOF\n+'\n+\n+test_expect_success 'reflog walk shows expected logs' '\n+\tdo_walk >actual &&\n+\ttest_cmp expect.all actual\n+'\n+\n+test_expect_failure 'reflog can limit with --no-merges' '\n+\tgrep -v merge expect.all >expect &&\n+\tdo_walk --no-merges >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'reflog can limit with pathspecs' '\n+\tgrep two expect.all >expect &&\n+\tdo_walk -- two.t >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'pathspec limiting handles merges' '\n+\t# we pick up:\n+\t# - the initial commit of one\n+\t# - the checkout back to commit one\n+\t# - the evil merge which touched one\n+\tsed -n \"1p;3p;5p\" expect.all >expect &&\n+\tdo_walk -- one.t >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure '--parents shows true parents' '\n+\t# convert newlines to spaces\n+\techo $(git rev-parse HEAD HEAD^1 HEAD^2) >expect &&\n+\tgit rev-list -g --parents -1 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'walking multiple reflogs shows all' '\n+\t# We expect to see all entries for all reflogs, but interleaved by\n+\t# date, with order no the command line breaking ties. We\n+\t# can use \"sort\" on the separate lists to generate this,\n+\t# but note two tricks:\n+\t#\n+\t# 1. We use \"{\" as the delimiter, which lets us skip to the reflog\n+\t# date specifier as our second field, and then our \"-n\" numeric\n+\t# sort ignores the bits after the timestamp.\n+\t#\n+\t# 2. POSIX leaves undefined whether this is a stable sort or not. So\n+\t# we use \"-k 1\" to ensure that we see HEAD before master before\n+\t# side when breaking ties.\n+\t{\n+\t\tdo_walk --date=unix HEAD &&\n+\t\tdo_walk --date=unix side &&\n+\t\tdo_walk --date=unix master\n+\t} >expect.raw &&\n+\tsort -t \"{\" -k 2nr -k 1 expect &&\n+\tdo_walk --date=unix HEAD master side >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'date-limiting does not interfere with other logs' '\n+\tdo_walk HEAD@{1979-01-01} HEAD >actual &&\n+\ttest_cmp expect.all actual\n+'\n+\n+test_expect_failure 'walk prefers reflog to ref tip' '\n+\thead=$(git rev-parse HEAD) &&\n+\tone=$(git rev-parse one) &&\n+\tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\" &&\n+\techo \"$head $one $ident\tbroken reflog entry\" >>.git/logs/HEAD &&\n+\n+\techo $one >expect &&\n+\tgit log -g --format=%H -1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.13.2.1000.g8590c1af5d\n\n"},{"id":"323935","messageId":"20170707090716.kj63rvq4aj4bj7lb@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707090507.ko2ygry7j4zv7t3s@sigill.intra.peff.net","subject":"[PATCH v2 2/7] revision: disallow reflog walking with revs->limited","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T09:07:16Z","receivedAt":"2017-07-07T09:07:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The reflog-walk code doesn't work with limit_list(). That\nfunction traverses down the real history graph, not the fake\nreflog history that get_revision() returns. So it's not\ngoing to actually examine all of the commits we're going to\nshow, because we'd add them to the pending list only during\nthe actual traversal.\n\nIn practice this limitation doesn't really matter, because\nthe options that require list-limiting generally need\nUNINTERESTING endpoints or symmetric ranges, which already\nare forbidden for reflog walks. Still, there are likely some\ncorner cases that would behave oddly. We're better off to\nwarn the user that we can't fulfill their request than to\ngenerate potentially wrong output.\n\nThis will also make it easier to refactor the reflog-walking\ncode, because it eliminates a whole area of corner cases\nwe'd have to consider (that already don't work anyway).\n\nSigned-off-by: Jeff King \n---\n revision.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex e181ad1b70..6678de04d9 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2366,6 +2366,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \tif (revs->reverse && revs->reflog_info)\n \t\tdie(\"cannot combine --reverse with --walk-reflogs\");\n+\tif (revs->reflog_info && revs->limited)\n+\t\tdie(\"cannot combine --walk-reflogs with history-limiting options\");\n \tif (revs->rewrite_parents && revs->children.name)\n \t\tdie(\"cannot combine --parents and --children\");\n \n-- \n2.13.2.1000.g8590c1af5d\n\n"},{"id":"323936","messageId":"20170707090758.pcwhc6pytzigs6ts@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707090507.ko2ygry7j4zv7t3s@sigill.intra.peff.net","subject":"[PATCH v2 4/7] get_revision_1(): replace do-while with an early return","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T09:07:58Z","receivedAt":"2017-07-07T09:08:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The get_revision_1() function tries to avoid entering its\nmain loop at all when there are no commits to look at. But\nit's perfectly safe to call pop_commit() on an empty list\n(in which case it will return NULL). Switching to an early\nreturn from the loop lets us skip repeating the loop\ncondition before we enter the do-while. That will get more\nimportant when we start pulling reflog-walk commits from a\nsource besides the revs->commits queue, as that condition\nwill get much more complicated.\n\nSigned-off-by: Jeff King \n---\n revision.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 6678de04d9..4019e8cf23 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3111,12 +3111,12 @@ static void track_linear(struct rev_info *revs, struct commit *commit)\n \n static struct commit *get_revision_1(struct rev_info *revs)\n {\n-\tif (!revs->commits)\n-\t\treturn NULL;\n-\n-\tdo {\n+\twhile (1) {\n \t\tstruct commit *commit = pop_commit(&revs->commits);\n \n+\t\tif (!commit)\n+\t\t\treturn NULL;\n+\n \t\tif (revs->reflog_info) {\n \t\t\tsave_parents(revs, commit);\n \t\t\tfake_reflog_parent(revs->reflog_info, commit);\n@@ -3150,8 +3150,7 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\t\t\ttrack_linear(revs, commit);\n \t\t\treturn commit;\n \t\t}\n-\t} while (revs->commits);\n-\treturn NULL;\n+\t}\n }\n \n /*\n-- \n2.13.2.1000.g8590c1af5d\n\n"},{"id":"323937","messageId":"20170707090829.45pcas7vwxn5hvby@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707090507.ko2ygry7j4zv7t3s@sigill.intra.peff.net","subject":"[PATCH v2 5/7] rev-list: check reflog_info before showing usage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T09:08:30Z","receivedAt":"2017-07-07T09:08:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git-rev-list sees no pending commits, it shows a usage\nmessage. This works even when reflog-walking is requested,\nbecause the reflog-walk code currently puts the reflog tips\ninto the pending queue.\n\nIn preparation for refactoring the reflog-walk code, let's\nexplicitly check whether we have any reflogs to walk. For\nnow this is a noop, but the existing reflog tests will make\nsure that it kicks in after the refactoring. Likewise, we'll\nadd a test that \"rev-list -g\" without specifying any reflogs\ncontinues to fail (so that we know our check does not kick\nin too aggressively).\n\nNote that the implementation needs to go into its own\nsub-function, as the walk code does not expose its innards\noutside of reflog-walk.c.\n\nSigned-off-by: Jeff King \n---\n builtin/rev-list.c | 3 ++-\n reflog-walk.c | 5 +++++\n reflog-walk.h | 2 ++\n t/t1414-reflog-walk.sh | 4 ++++\n 4 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 95d84d5cda..53a746dd89 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -11,6 +11,7 @@\n #include \"graph.h\"\n #include \"bisect.h\"\n #include \"progress.h\"\n+#include \"reflog-walk.h\"\n \n static const char rev_list_usage[] =\n \"git rev-list [OPTION] ... [ -- paths... ]\\n\"\n@@ -348,7 +349,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t/* Only --header was specified */\n \t\trevs.commit_format = CMIT_FMT_RAW;\n \n-\tif ((!revs.commits &&\n+\tif ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&\n \t (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&\n \t !revs.pending.nr)) ||\n \t revs.diff)\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 081f89b70d..98c2f42de9 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -365,3 +365,8 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n \t\tstrbuf_release(&selector);\n \t}\n }\n+\n+int reflog_walk_empty(struct reflog_walk_info *info)\n+{\n+\treturn !info || !info->reflogs.nr;\n+}\ndiff --git a/reflog-walk.h b/reflog-walk.h\nindex 27886f793e..af32361072 100644\n--- a/reflog-walk.h\n+++ b/reflog-walk.h\n@@ -20,4 +20,6 @@ extern void get_reflog_selector(struct strbuf *sb,\n \t\tconst struct date_mode *dmode, int force_date,\n \t\tint shorten);\n \n+extern int reflog_walk_empty(struct reflog_walk_info *walk);\n+\n #endif\ndiff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh\nindex 6426829352..289c502725 100755\n--- a/t/t1414-reflog-walk.sh\n+++ b/t/t1414-reflog-walk.sh\n@@ -102,4 +102,8 @@ test_expect_failure 'walk prefers reflog to ref tip' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list -g complains when there are no reflogs' '\n+\ttest_must_fail git rev-list -g\n+'\n+\n test_done\n-- \n2.13.2.1000.g8590c1af5d\n\n"},{"id":"323938","messageId":"20170707090734.x2ki7lluawf66g4a@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707090507.ko2ygry7j4zv7t3s@sigill.intra.peff.net","subject":"[PATCH v2 3/7] log: do not free parents when walking reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T09:07:34Z","receivedAt":"2017-07-07T09:08:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're doing a reflog walk (instead of walking the\nactual parent pointers), we may see commits multiple times.\nFor this reason, we hold on to the commit buffer for each\ncommit rather than freeing it after we've showed the commit.\n\nWe should do the same for the parent list. Right now this is\njust a minor optimization. But once we refactor how reflog\nwalks are performed, keeping the parents will avoid\nconfusing us the second time we see the commit.\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8ca1de9894..9c8bb3b5c3 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\tif (!rev->reflog_info) {\n \t\t\t/* we allow cycles in reflog ancestry */\n \t\t\tfree_commit_buffer(commit);\n+\t\t\tfree_commit_list(commit->parents);\n+\t\t\tcommit->parents = NULL;\n \t\t}\n-\t\tfree_commit_list(commit->parents);\n-\t\tcommit->parents = NULL;\n \t\tif (saved_nrl < rev->diffopt.needed_rename_limit)\n \t\t\tsaved_nrl = rev->diffopt.needed_rename_limit;\n \t\tif (rev->diffopt.degraded_cc_to_c)\n-- \n2.13.2.1000.g8590c1af5d\n\n"},{"id":"323939","messageId":"20170707091407.nm4pm3bdvx6alohs@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707090507.ko2ygry7j4zv7t3s@sigill.intra.peff.net","subject":"[PATCH v2 6/7] reflog-walk: stop using fake parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T09:14:07Z","receivedAt":"2017-07-07T09:14:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The reflog-walk system works by putting a ref's tip into the\npending queue, and then \"traversing\" the reflog by\npretending that the parent of each commit is the previous\nreflog entry.\n\nThis causes a number of user-visible oddities, as documented\nin t1414 (and the commit message which introduced it). We\ncan fix all of them in one go by replacing the fake-reflog\nsystem with a much simpler one: just keeping a list of\nreflogs to show, and walking through them entry by entry.\n\nThe implementation is fairly straight-forward, but there are\na few items to note:\n\n 1. We obviously must skip calling add_parents_to_list()\n when we are traversing reflogs, since we do not want to\n walk the original parents at all. As a result, we must call\n try_to_simplify_commit() ourselves.\n\n There are other parts of add_parents_to_list() we skip,\n as well, but none of them should matter for a reflog\n traversal:\n\n - We do not allow UNINTERESTING commits, nor\n symmetric ranges (and we bail when these are used\n with \"-g\").\n\n - Using --source makes no sense, since we aren't\n traversing. The reflog selector shows the same\n information with more detail.\n\n - Using --first-parent is still sensible, since you\n may want to see the first-parent diff for each\n entry. But since we're not traversing, we don't\n need to cull the parent list here.\n\n 2. Since we now just walk the reflog entries themselves,\n rather than starting with the ref tip, we now look at\n the \"new\" field of each entry rather than the \"old\"\n (i.e., we are showing entries, not faking parents).\n This removes all of the tricky logic around skipping\n past root commits.\n\n But note that we have no way to show an entry with the\n null sha1 in its \"new\" field (because such a commit\n obviously does not exist). Normally this would not\n happen, since we delete reflogs along with refs, but\n there is one special case. When we rename the currently\n checked out branch, we write two reflog entries into\n the HEAD log: one where the commit goes away, and\n another where it comes back.\n\n Prior to this commit, we show both entries with\n identical reflog messages. After this commit, we show\n only the \"comes back\" entry. See the update in t3200\n which demonstrates this.\n\n Arguably either is fine, as the whole double-entry\n thing is a bit hacky in the first place. And until a\n recent fix, we truncated the traversal in such a case\n anyway, which was _definitely_ wrong.\n\n 3. We show individual reflogs in order, but choose which\n reflog to show at each stage based on which has the\n most recent timestamp. This interleaves the output\n from multiple reflogs based on date order, which is\n probably what you'd want with limiting like \"-n 30\".\n\n Note that the implementation aims for simplicity. It\n does a linear walk over the reflog queue for each\n commit it pulls, which may perform badly if you\n interleave an enormous number of reflogs. That seems\n like an unlikely use case; if we did want to handle it,\n we could probably keep a priority queue of reflogs,\n ordered by the timestamp of their current tip entry.\n\nSigned-off-by: Jeff King \n---\n reflog-walk.c | 137 ++++++++++++++++++-------------------------------\n reflog-walk.h | 4 +-\n revision.c | 27 +++++-----\n t/t1414-reflog-walk.sh | 12 ++---\n t/t3200-branch.sh | 3 +-\n 5 files changed, 75 insertions(+), 108 deletions(-)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 98c2f42de9..fbee9e0126 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -94,45 +94,6 @@ static int get_reflog_recno_by_time(struct complete_reflogs *array,\n \treturn -1;\n }\n \n-struct commit_info_lifo {\n-\tstruct commit_info {\n-\t\tstruct commit *commit;\n-\t\tvoid *util;\n-\t} *items;\n-\tint nr, alloc;\n-};\n-\n-static struct commit_info *get_commit_info(struct commit *commit,\n-\t\tstruct commit_info_lifo *lifo, int pop)\n-{\n-\tint i;\n-\tfor (i = 0; i < lifo->nr; i++)\n-\t\tif (lifo->items[i].commit == commit) {\n-\t\t\tstruct commit_info *result = &lifo->items[i];\n-\t\t\tif (pop) {\n-\t\t\t\tif (i + 1 < lifo->nr)\n-\t\t\t\t\tmemmove(lifo->items + i,\n-\t\t\t\t\t\tlifo->items + i + 1,\n-\t\t\t\t\t\t(lifo->nr - i) *\n-\t\t\t\t\t\tsizeof(struct commit_info));\n-\t\t\t\tlifo->nr--;\n-\t\t\t}\n-\t\t\treturn result;\n-\t\t}\n-\treturn NULL;\n-}\n-\n-static void add_commit_info(struct commit *commit, void *util,\n-\t\tstruct commit_info_lifo *lifo)\n-{\n-\tstruct commit_info *info;\n-\tALLOC_GROW(lifo->items, lifo->nr + 1, lifo->alloc);\n-\tinfo = lifo->items + lifo->nr;\n-\tinfo->commit = commit;\n-\tinfo->util = util;\n-\tlifo->nr++;\n-}\n-\n struct commit_reflog {\n \tint recno;\n \tenum selector_type {\n@@ -144,7 +105,8 @@ struct commit_reflog {\n };\n \n struct reflog_walk_info {\n-\tstruct commit_info_lifo reflogs;\n+\tstruct commit_reflog **logs;\n+\tsize_t nr, alloc;\n \tstruct string_list complete_reflogs;\n \tstruct commit_reflog *last_commit_reflog;\n };\n@@ -233,52 +195,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n \tcommit_reflog->selector = selector;\n \tcommit_reflog->reflogs = reflogs;\n \n-\tadd_commit_info(commit, commit_reflog, &info->reflogs);\n-\treturn 0;\n-}\n-\n-void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n-{\n-\tstruct commit_info *commit_info =\n-\t\tget_commit_info(commit, &info->reflogs, 0);\n-\tstruct commit_reflog *commit_reflog;\n-\tstruct object *logobj;\n-\tstruct reflog_info *reflog;\n-\n-\tinfo->last_commit_reflog = NULL;\n-\tif (!commit_info)\n-\t\treturn;\n-\n-\tcommit_reflog = commit_info->util;\n-\tif (commit_reflog->recno < 0) {\n-\t\tcommit->parents = NULL;\n-\t\treturn;\n-\t}\n-\tinfo->last_commit_reflog = commit_reflog;\n-\n-\tdo {\n-\t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n-\t\tcommit_reflog->recno--;\n-\t\tlogobj = parse_object(&reflog->ooid);\n-\t} while (commit_reflog->recno && (logobj && logobj->type != OBJ_COMMIT));\n-\n-\tif (!logobj && commit_reflog->recno >= 0 && is_null_oid(&reflog->ooid)) {\n-\t\t/* a root commit, but there are still more entries to show */\n-\t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n-\t\tlogobj = parse_object(&reflog->noid);\n-\t\tif (!logobj)\n-\t\t\tlogobj = parse_object(&reflog->ooid);\n-\t}\n-\n-\tif (!logobj || logobj->type != OBJ_COMMIT) {\n-\t\tcommit_info->commit = NULL;\n-\t\tcommit->parents = NULL;\n-\t\treturn;\n-\t}\n-\tcommit_info->commit = (struct commit *)logobj;\n+\tALLOC_GROW(info->logs, info->nr + 1, info->alloc);\n+\tinfo->logs[info->nr++] = commit_reflog;\n \n-\tcommit->parents = xcalloc(1, sizeof(struct commit_list));\n-\tcommit->parents->item = commit_info->commit;\n+\treturn 0;\n }\n \n void get_reflog_selector(struct strbuf *sb,\n@@ -368,5 +288,50 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n \n int reflog_walk_empty(struct reflog_walk_info *info)\n {\n-\treturn !info || !info->reflogs.nr;\n+\treturn !info || !info->nr;\n+}\n+\n+static struct commit *next_reflog_commit(struct commit_reflog *log)\n+{\n+\tfor (; log->recno >= 0; log->recno--) {\n+\t\tstruct reflog_info *entry = &log->reflogs->items[log->recno];\n+\t\tstruct object *obj = parse_object(&entry->noid);\n+\n+\t\tif (obj && obj->type == OBJ_COMMIT)\n+\t\t\treturn (struct commit *)obj;\n+\t}\n+\treturn NULL;\n+}\n+\n+static timestamp_t log_timestamp(struct commit_reflog *log)\n+{\n+\treturn log->reflogs->items[log->recno].timestamp;\n+}\n+\n+struct commit *next_reflog_entry(struct reflog_walk_info *walk)\n+{\n+\tstruct commit_reflog *best = NULL;\n+\tstruct commit *best_commit = NULL;\n+\tsize_t i;\n+\n+\tfor (i = 0; i < walk->nr; i++) {\n+\t\tstruct commit_reflog *log = walk->logs[i];\n+\t\tstruct commit *commit = next_reflog_commit(log);\n+\n+\t\tif (!commit)\n+\t\t\tcontinue;\n+\n+\t\tif (!best || log_timestamp(log) > log_timestamp(best)) {\n+\t\t\tbest = log;\n+\t\t\tbest_commit = commit;\n+\t\t}\n+\t}\n+\n+\tif (best) {\n+\t\tbest->recno--;\n+\t\twalk->last_commit_reflog = best;\n+\t\treturn best_commit;\n+\t}\n+\n+\treturn NULL;\n }\ndiff --git a/reflog-walk.h b/reflog-walk.h\nindex af32361072..373388cd14 100644\n--- a/reflog-walk.h\n+++ b/reflog-walk.h\n@@ -8,8 +8,6 @@ struct reflog_walk_info;\n extern void init_reflog_walk(struct reflog_walk_info **info);\n extern int add_reflog_for_walk(struct reflog_walk_info *info,\n \t\tstruct commit *commit, const char *name);\n-extern void fake_reflog_parent(struct reflog_walk_info *info,\n-\t\tstruct commit *commit);\n extern void show_reflog_message(struct reflog_walk_info *info, int,\n \t\t\t\tconst struct date_mode *, int force_date);\n extern void get_reflog_message(struct strbuf *sb,\n@@ -22,4 +20,6 @@ extern void get_reflog_selector(struct strbuf *sb,\n \n extern int reflog_walk_empty(struct reflog_walk_info *walk);\n \n+struct commit *next_reflog_entry(struct reflog_walk_info *reflog_info);\n+\n #endif\ndiff --git a/revision.c b/revision.c\nindex 4019e8cf23..587199739a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -148,16 +148,14 @@ static void add_pending_object_with_path(struct rev_info *revs,\n \tif (revs->reflog_info && obj->type == OBJ_COMMIT) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint len = interpret_branch_name(name, 0, &buf, 0);\n-\t\tint st;\n \n \t\tif (0 < len && name[len] && buf.len)\n \t\t\tstrbuf_addstr(&buf, name + len);\n-\t\tst = add_reflog_for_walk(revs->reflog_info,\n-\t\t\t\t\t (struct commit *)obj,\n-\t\t\t\t\t buf.buf[0] ? buf.buf: name);\n+\t\tadd_reflog_for_walk(revs->reflog_info,\n+\t\t\t\t (struct commit *)obj,\n+\t\t\t\t buf.buf[0] ? buf.buf: name);\n \t\tstrbuf_release(&buf);\n-\t\tif (st)\n-\t\t\treturn;\n+\t\treturn; /* do not add the commit itself */\n \t}\n \tadd_object_array_with_path(obj, name, &revs->pending, mode, path);\n }\n@@ -3112,16 +3110,18 @@ static void track_linear(struct rev_info *revs, struct commit *commit)\n static struct commit *get_revision_1(struct rev_info *revs)\n {\n \twhile (1) {\n-\t\tstruct commit *commit = pop_commit(&revs->commits);\n+\t\tstruct commit *commit;\n+\n+\t\tif (revs->reflog_info)\n+\t\t\tcommit = next_reflog_entry(revs->reflog_info);\n+\t\telse\n+\t\t\tcommit = pop_commit(&revs->commits);\n \n \t\tif (!commit)\n \t\t\treturn NULL;\n \n-\t\tif (revs->reflog_info) {\n-\t\t\tsave_parents(revs, commit);\n-\t\t\tfake_reflog_parent(revs->reflog_info, commit);\n+\t\tif (revs->reflog_info)\n \t\t\tcommit->object.flags &= ~(ADDED | SEEN | SHOWN);\n-\t\t}\n \n \t\t/*\n \t\t * If we haven't done the list limiting, we need to look at\n@@ -3132,7 +3132,10 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\t\tif (revs->max_age != -1 &&\n \t\t\t (commit->date < revs->max_age))\n \t\t\t\tcontinue;\n-\t\t\tif (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {\n+\n+\t\t\tif (revs->reflog_info)\n+\t\t\t\ttry_to_simplify_commit(revs, commit);\n+\t\t\telse if (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {\n \t\t\t\tif (!revs->ignore_missing_links)\n \t\t\t\t\tdie(\"Failed to traverse parents of commit %s\",\n \t\t\t\t\t\toid_to_hex(&commit->object.oid));\ndiff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh\nindex 289c502725..c4c53bd209 100755\n--- a/t/t1414-reflog-walk.sh\n+++ b/t/t1414-reflog-walk.sh\n@@ -34,19 +34,19 @@ test_expect_success 'reflog walk shows expected logs' '\n \ttest_cmp expect.all actual\n '\n \n-test_expect_failure 'reflog can limit with --no-merges' '\n+test_expect_success 'reflog can limit with --no-merges' '\n \tgrep -v merge expect.all >expect &&\n \tdo_walk --no-merges >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'reflog can limit with pathspecs' '\n+test_expect_success 'reflog can limit with pathspecs' '\n \tgrep two expect.all >expect &&\n \tdo_walk -- two.t >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'pathspec limiting handles merges' '\n+test_expect_success 'pathspec limiting handles merges' '\n \t# we pick up:\n \t# - the initial commit of one\n \t# - the checkout back to commit one\n@@ -56,14 +56,14 @@ test_expect_failure 'pathspec limiting handles merges' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure '--parents shows true parents' '\n+test_expect_success '--parents shows true parents' '\n \t# convert newlines to spaces\n \techo $(git rev-parse HEAD HEAD^1 HEAD^2) >expect &&\n \tgit rev-list -g --parents -1 HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'walking multiple reflogs shows all' '\n+test_expect_success 'walking multiple reflogs shows all' '\n \t# We expect to see all entries for all reflogs, but interleaved by\n \t# date, with order no the command line breaking ties. We\n \t# can use \"sort\" on the separate lists to generate this,\n@@ -91,7 +91,7 @@ test_expect_success 'date-limiting does not interfere with other logs' '\n \ttest_cmp expect.all actual\n '\n \n-test_expect_failure 'walk prefers reflog to ref tip' '\n+test_expect_success 'walk prefers reflog to ref tip' '\n \thead=$(git rev-parse HEAD) &&\n \tone=$(git rev-parse one) &&\n \tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\" &&\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex dd37ac47c5..9d707d2a40 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -166,10 +166,9 @@ test_expect_success 'resulting reflog can be shown by log -g' '\n \toid=$(git rev-parse HEAD) &&\n \tcat >expect <<-EOF &&\n \tHEAD@{0} $oid $msg\n-\tHEAD@{1} $oid $msg\n \tHEAD@{2} $oid checkout: moving from foo to baz\n \tEOF\n-\tgit log -g --format=\"%gd %H %gs\" -3 HEAD >actual &&\n+\tgit log -g --format=\"%gd %H %gs\" -2 HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.13.2.1000.g8590c1af5d\n\n"},{"id":"323940","messageId":"20170707091620.5jljk42pppgeqork@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707090507.ko2ygry7j4zv7t3s@sigill.intra.peff.net","subject":"[PATCH v2 7/7] reflog-walk: apply --since/--until to reflog dates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T09:16:21Z","receivedAt":"2017-07-07T09:16:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When doing a reflog walk, we use the commit's date to\ndo any date limiting. In earlier versions of Git, this could\nlead to nonsense results, since a skipped commit would\ntruncate the traversal. So a sequence like:\n\n git commit ...\n git checkout week-old-branch\n git checkout -\n git log -g --since=1.day.ago\n\nwould stop at the week-old-branch, even though the \"git\ncommit\" entry further back is still interesting.\n\nAs of the prior commit, which uses a parent-less traversal\nof the reflog, you get the whole reflog minus any commits\nwhose dates do not match the specified options. This is\narguably useful, as you could scan the reflogs for commits\nthat originated in a certain range.\n\nBut more likely a user doing a reflog walk wants to limit\nbased on the reflog entries themselves. You can simulate\n--until with:\n\n git log -g @{1.day.ago}\n\nbut there's no way to ask Git to traverse only back to a\ncertain date. E.g.:\n\n # show me reflog entries from the past day\n git log -g --since=1.day.ago\n\nThis patch teaches the revision machinery to prefer the\nreflog entry dates to the commit dates when doing a reflog\nwalk. Technically this is a change in behavior that affects\nplumbing, but the previous behavior was so buggy that it's\nunlikely anyone was relying on it.\n\nSigned-off-by: Jeff King \n---\n reflog-walk.c | 12 ++++++++++++\n reflog-walk.h | 1 +\n revision.c | 19 ++++++++++++++++---\n t/t1414-reflog-walk.sh | 26 ++++++++++++++++++++++++++\n 4 files changed, 55 insertions(+), 3 deletions(-)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex fbee9e0126..74ebe5148f 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -264,6 +264,18 @@ const char *get_reflog_ident(struct reflog_walk_info *reflog_info)\n \treturn info->email;\n }\n \n+timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info)\n+{\n+\tstruct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;\n+\tstruct reflog_info *info;\n+\n+\tif (!commit_reflog)\n+\t\treturn 0;\n+\n+\tinfo = &commit_reflog->reflogs->items[commit_reflog->recno+1];\n+\treturn info->timestamp;\n+}\n+\n void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n \t\t\t const struct date_mode *dmode, int force_date)\n {\ndiff --git a/reflog-walk.h b/reflog-walk.h\nindex 373388cd14..7553c448fe 100644\n--- a/reflog-walk.h\n+++ b/reflog-walk.h\n@@ -13,6 +13,7 @@ extern void show_reflog_message(struct reflog_walk_info *info, int,\n extern void get_reflog_message(struct strbuf *sb,\n \t\tstruct reflog_walk_info *reflog_info);\n extern const char *get_reflog_ident(struct reflog_walk_info *reflog_info);\n+extern timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info);\n extern void get_reflog_selector(struct strbuf *sb,\n \t\tstruct reflog_walk_info *reflog_info,\n \t\tconst struct date_mode *dmode, int force_date,\ndiff --git a/revision.c b/revision.c\nindex 587199739a..41b4375c3c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2965,6 +2965,18 @@ static inline int want_ancestry(const struct rev_info *revs)\n \treturn (revs->rewrite_parents || revs->children.name);\n }\n \n+/*\n+ * Return a timestamp to be used for --since/--until comparisons for this\n+ * commit, based on the revision options.\n+ */\n+static timestamp_t comparison_date(const struct rev_info *revs,\n+\t\t\t\t struct commit *commit)\n+{\n+\treturn revs->reflog_info ?\n+\t\tget_reflog_timestamp(revs->reflog_info) :\n+\t\tcommit->date;\n+}\n+\n enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)\n {\n \tif (commit->object.flags & SHOWN)\n@@ -2975,8 +2987,9 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n \t\treturn commit_show;\n \tif (commit->object.flags & UNINTERESTING)\n \t\treturn commit_ignore;\n-\tif (revs->min_age != -1 && (commit->date > revs->min_age))\n-\t\treturn commit_ignore;\n+\tif (revs->min_age != -1 &&\n+\t comparison_date(revs, commit) > revs->min_age)\n+\t\t\treturn commit_ignore;\n \tif (revs->min_parents || (revs->max_parents >= 0)) {\n \t\tint n = commit_list_count(commit->parents);\n \t\tif ((n < revs->min_parents) ||\n@@ -3130,7 +3143,7 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\t */\n \t\tif (!revs->limited) {\n \t\t\tif (revs->max_age != -1 &&\n-\t\t\t (commit->date < revs->max_age))\n+\t\t\t comparison_date(revs, commit) < revs->max_age)\n \t\t\t\tcontinue;\n \n \t\t\tif (revs->reflog_info)\ndiff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh\nindex c4c53bd209..360754959e 100755\n--- a/t/t1414-reflog-walk.sh\n+++ b/t/t1414-reflog-walk.sh\n@@ -91,6 +91,32 @@ test_expect_success 'date-limiting does not interfere with other logs' '\n \ttest_cmp expect.all actual\n '\n \n+test_expect_success 'min/max age uses entry date to limit' '\n+\t# Flip between commits one and two so each ref update actually\n+\t# does something (and does not get optimized out). We know\n+\t# that the timestamps of those commits will be before our \"min\".\n+\n+\tgit update-ref -m before refs/heads/minmax one &&\n+\n+\ttest_tick &&\n+\tmin=$test_tick &&\n+\tgit update-ref -m min refs/heads/minmax two &&\n+\n+\ttest_tick &&\n+\tmax=$test_tick &&\n+\tgit update-ref -m max refs/heads/minmax one &&\n+\n+\ttest_tick &&\n+\tgit update-ref -m after refs/heads/minmax two &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tmax\n+\tmin\n+\tEOF\n+\tgit log -g --since=$min --until=$max --format=%gs minmax >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'walk prefers reflog to ref tip' '\n \thead=$(git rev-parse HEAD) &&\n \tone=$(git rev-parse one) &&\n-- \n2.13.2.1000.g8590c1af5d\n"},{"id":"323941","messageId":"20170707092059.rhimba5ebyzdaj4w@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707090610.x2vodxgc7yr6o5pi@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/7] t1414: document some reflog-walk oddities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T09:21:00Z","receivedAt":"2017-07-07T09:21:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 07, 2017 at 05:06:10AM -0400, Jeff King wrote:\n\n> +test_expect_failure 'walking multiple reflogs shows all' '\n> +\t# We expect to see all entries for all reflogs, but interleaved by\n> +\t# date, with order no the command line breaking ties. We\n> +\t# can use \"sort\" on the separate lists to generate this,\n> +\t# but note two tricks:\n> +\t#\n> +\t# 1. We use \"{\" as the delimiter, which lets us skip to the reflog\n> +\t# date specifier as our second field, and then our \"-n\" numeric\n> +\t# sort ignores the bits after the timestamp.\n> +\t#\n> +\t# 2. POSIX leaves undefined whether this is a stable sort or not. So\n> +\t# we use \"-k 1\" to ensure that we see HEAD before master before\n> +\t# side when breaking ties.\n> +\t{\n> +\t\tdo_walk --date=unix HEAD &&\n> +\t\tdo_walk --date=unix side &&\n> +\t\tdo_walk --date=unix master\n> +\t} >expect.raw &&\n> +\tsort -t \"{\" -k 2nr -k 1 expect &&\n\nI won't lie; the contortions needed for this \"sort\" made me a little\nsick to my stomach.\n\nIt would be much easier if we could do something like:\n\n git log -g --format=\"%gt %gd\"\n\nand just get the reflog timestamp separately. But we never added %gt, so\nwe'd have to munge it with sed or perl.\n\nOne thing that perl would buy is that we could rely on a stable sort,\nand ask for an order besides alphabetical. I.e., we could ask for:\n\n do_walk side master HEAD\n\nand confirm that \"side\" comes before \"HEAD\", even though it doesn't\nbyte-wise sort.\n\nI didn't think writing a perl snippet was worth the trouble for that\nminor benefit, though probably it would be shorter than the comment I\nneeded to explain the \"sort\" invocation. ;)\n\n-Peff\n"},{"id":"323942","messageId":"20170707092432.u7dz3zniprp4zvkr@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170707091407.nm4pm3bdvx6alohs@sigill.intra.peff.net","subject":"Re: [PATCH v2 6/7] reflog-walk: stop using fake parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-07T09:24:33Z","receivedAt":"2017-07-07T09:24:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 07, 2017 at 05:14:07AM -0400, Jeff King wrote:\n\n> @@ -3132,7 +3132,10 @@ static struct commit *get_revision_1(struct rev_info *revs)\n> \t\t\tif (revs->max_age != -1 &&\n> \t\t\t (commit->date < revs->max_age))\n> \t\t\t\tcontinue;\n> -\t\t\tif (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {\n> +\n> +\t\t\tif (revs->reflog_info)\n> +\t\t\t\ttry_to_simplify_commit(revs, commit);\n> +\t\t\telse if (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {\n> \t\t\t\tif (!revs->ignore_missing_links)\n> \t\t\t\t\tdie(\"Failed to traverse parents of commit %s\",\n> \t\t\t\t\t\toid_to_hex(&commit->object.oid));\n\nThere's one other subtle change from v1 here. In my original, we called\ntry_to_simplify_commit() much later, and then we had to check whether it\nmarked the commit as TREESAME. That felt really hacky to me, and the\nreason is because I was using the function wrong. The intent is for\ntry_to_simplify to be called earlier (i.e., here), when we start looking\nat the commit's parents. And then the TREESAME flag it sets is later\npicked up by get_commit_action(), which is called by simplify_commit(),\nwhich is used later in get_revision_1().\n\nSo by calling try_to_simplify here, where it would normally be called\nfor a non-reflog walk, that's all we have to do. I don't think the\noriginal patch had any visible bugs, but this way is much cleaner and\nmore future-proof.\n\n-Peff\n"},{"id":"323957","messageId":"87van48cpu.fsf@kyleam.com","threadId":"46227","inReplyTo":"20170707091407.nm4pm3bdvx6alohs@sigill.intra.peff.net","subject":"Re: [PATCH v2 6/7] reflog-walk: stop using fake parents","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2017-07-07T15:54:05Z","receivedAt":"2017-07-07T15:54:13Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Jeff King writes:\n\n> Prior to this commit, we show both entries with\n> identical reflog messages. After this commit, we show\n> only the \"comes back\" entry. See the update in t3200\n> which demonstrates this.\n>\n> Arguably either is fine, as the whole double-entry\n> thing is a bit hacky in the first place. And until a\n> recent fix, we truncated the traversal in such a case\n> anyway, which was _definitely_ wrong.\n\nYeah, I agree that the double-entry thing is a bit hacky and only\nshowing the \"comes back\" entry makes sense.\n\nAnd with this change, I believe that the display of a rename event will\nbe the same for HEAD's log and the renamed branch's log, despite the\nunderlying entries having a different representation.\n\n-- \nKyle\n"},{"id":"323958","messageId":"87tw2o8cpf.fsf@kyleam.com","threadId":"46227","inReplyTo":"20170707090610.x2vodxgc7yr6o5pi@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/7] t1414: document some reflog-walk oddities","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2017-07-07T15:54:20Z","receivedAt":"2017-07-07T15:54:26Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Jeff King writes:\n\n> +test_expect_failure 'walking multiple reflogs shows all' '\n> +\t# We expect to see all entries for all reflogs, but interleaved by\n> +\t# date, with order no the command line breaking ties. We\n\ns/order no/order on/\n\n-- \nKyle\n"},{"id":"323965","messageId":"xmqqwp7kb2ap.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170707090734.x2ki7lluawf66g4a@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/7] log: do not free parents when walking reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-07T17:10:54Z","receivedAt":"2017-07-07T17:11:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we're doing a reflog walk (instead of walking the\n> actual parent pointers), we may see commits multiple times.\n> For this reason, we hold on to the commit buffer for each\n> commit rather than freeing it after we've showed the commit.\n>\n> We should do the same for the parent list. Right now this is\n> just a minor optimization. But once we refactor how reflog\n> walks are performed, keeping the parents will avoid\n> confusing us the second time we see the commit.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/log.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 8ca1de9894..9c8bb3b5c3 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)\n> \t\tif (!rev->reflog_info) {\n> \t\t\t/* we allow cycles in reflog ancestry */\n> \t\t\tfree_commit_buffer(commit);\n> +\t\t\tfree_commit_list(commit->parents);\n> +\t\t\tcommit->parents = NULL;\n\nAfter step 6/7, we no longer \"allow cycles in reflog ancestry\", as\nthere will be no reflog ancestry to speak of ;-), so it would be\nnice to remove the comment above in that step. But alternatively,\nwe can rephrase the comment here, to say something like \"the same\ncommit can be shown multiple times while showing entries from the\nreflog\" instead.\n\n> \t\t}\n> -\t\tfree_commit_list(commit->parents);\n> -\t\tcommit->parents = NULL;\n> \t\tif (saved_nrl < rev->diffopt.needed_rename_limit)\n> \t\t\tsaved_nrl = rev->diffopt.needed_rename_limit;\n> \t\tif (rev->diffopt.degraded_cc_to_c)\n"},{"id":"323976","messageId":"xmqqy3s09f5x.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170707083636.kjsr5ry3237paeiv@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/4] reflog-walk fixes for maint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-07T20:15:54Z","receivedAt":"2017-07-07T20:16:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. All made sense to me after reading them twice.\n\n"},{"id":"324018","messageId":"20170709100837.kr5fs2kva6bdrole@sigill.intra.peff.net","threadId":"46227","inReplyTo":"87van48cpu.fsf@kyleam.com","subject":"Re: [PATCH v2 6/7] reflog-walk: stop using fake parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-09T10:08:38Z","receivedAt":"2017-07-09T10:08:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 07, 2017 at 11:54:05AM -0400, Kyle Meyer wrote:\n\n> Jeff King writes:\n> \n> > Prior to this commit, we show both entries with\n> > identical reflog messages. After this commit, we show\n> > only the \"comes back\" entry. See the update in t3200\n> > which demonstrates this.\n> >\n> > Arguably either is fine, as the whole double-entry\n> > thing is a bit hacky in the first place. And until a\n> > recent fix, we truncated the traversal in such a case\n> > anyway, which was _definitely_ wrong.\n> \n> Yeah, I agree that the double-entry thing is a bit hacky and only\n> showing the \"comes back\" entry makes sense.\n> \n> And with this change, I believe that the display of a rename event will\n> be the same for HEAD's log and the renamed branch's log, despite the\n> underlying entries having a different representation.\n\nThere's one minor difference: the numbering for HEAD will show a \"hole\"\nin the reflog. So on the branch you might see something like:\n\n $ git log -g --oneline other\n 269000b other@{0}: Branch: renamed refs/heads/master to refs/heads/other\n 269000b other@{1}: commit (initial): foo\n\nbut the HEAD reflog will show:\n\n $ git log -g --oneline HEAD\n 269000b HEAD@{0}: Branch: renamed refs/heads/master to refs/heads/other\n 269000b HEAD@{2}: commit (initial): foo\n\nThis is pretty minor. I do wonder if there are other bits that might be\nconfused, though. Looking at the hole is odd:\n\n $ git show --oneline HEAD@{1}\n warning: Log for ref HEAD unexpectedly ended on Sun, 9 Jul 2017 06:07:05 -0400.\n 269000b foo\n\nDespite the warning, we do seem to correctly walk past it, though:\n\n $ git show --oneline HEAD@{2}\n 269000b foo\n\n-Peff\n"},{"id":"324019","messageId":"20170709101351.qcwgtzly72wwvwmq@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqqwp7kb2ap.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/7] log: do not free parents when walking reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-09T10:13:51Z","receivedAt":"2017-07-09T10:13:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 07, 2017 at 10:10:54AM -0700, Junio C Hamano wrote:\n\n> > diff --git a/builtin/log.c b/builtin/log.c\n> > index 8ca1de9894..9c8bb3b5c3 100644\n> > --- a/builtin/log.c\n> > +++ b/builtin/log.c\n> > @@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)\n> > \t\tif (!rev->reflog_info) {\n> > \t\t\t/* we allow cycles in reflog ancestry */\n> > \t\t\tfree_commit_buffer(commit);\n> > +\t\t\tfree_commit_list(commit->parents);\n> > +\t\t\tcommit->parents = NULL;\n> \n> After step 6/7, we no longer \"allow cycles in reflog ancestry\", as\n> there will be no reflog ancestry to speak of ;-), so it would be\n> nice to remove the comment above in that step. But alternatively,\n> we can rephrase the comment here, to say something like \"the same\n> commit can be shown multiple times while showing entries from the\n> reflog\" instead.\n\nI actually think the comment is a bit obtuse in the first place. The\nreal issue is that we show commits multiple times. That's caused by\ncycles, yes, but also by us clearing the SEEN flag. ;)\n\nMaybe this on top?\n\n-- >8 --\nSubject: [PATCH] log: clarify comment about reflog cycles\n\nWhen we're walking reflogs, we leave the commit buffer and\nparents in place. A comment explains that this is due to\n\"cycles\". But the interesting thing is the unsaid\nimplication: that the cycles (plus our clearing of the SEEN\nflag) will cause us to show commits multiple times. Let's\nspell it out.\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 9c8bb3b5c..630d6cff2 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -372,7 +372,10 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\t\t */\n \t\t\trev->max_count++;\n \t\tif (!rev->reflog_info) {\n-\t\t\t/* we allow cycles in reflog ancestry */\n+\t\t\t/*\n+\t\t\t * We may show a given commit multiple times when\n+\t\t\t * walking the reflogs.\n+\t\t\t */\n \t\t\tfree_commit_buffer(commit);\n \t\t\tfree_commit_list(commit->parents);\n \t\t\tcommit->parents = NULL;\n-- \n2.13.2.1066.gabaed60bd\n\n"},{"id":"324035","messageId":"xmqqtw2l8s22.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170709101351.qcwgtzly72wwvwmq@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/7] log: do not free parents when walking reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-09T16:59:33Z","receivedAt":"2017-07-09T16:59:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jul 07, 2017 at 10:10:54AM -0700, Junio C Hamano wrote:\n>\n>> > diff --git a/builtin/log.c b/builtin/log.c\n>> > index 8ca1de9894..9c8bb3b5c3 100644\n>> > --- a/builtin/log.c\n>> > +++ b/builtin/log.c\n>> > @@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)\n>> > \t\tif (!rev->reflog_info) {\n>> > \t\t\t/* we allow cycles in reflog ancestry */\n>> > \t\t\tfree_commit_buffer(commit);\n>> > +\t\t\tfree_commit_list(commit->parents);\n>> > +\t\t\tcommit->parents = NULL;\n>> \n>> After step 6/7, we no longer \"allow cycles in reflog ancestry\", as\n>> there will be no reflog ancestry to speak of ;-), so it would be\n>> nice to remove the comment above in that step. But alternatively,\n>> we can rephrase the comment here, to say something like \"the same\n>> commit can be shown multiple times while showing entries from the\n>> reflog\" instead.\n>\n> I actually think the comment is a bit obtuse in the first place. The\n> real issue is that we show commits multiple times. That's caused by\n> cycles, yes, but also by us clearing the SEEN flag. ;)\n>\n> Maybe this on top?\n\nYup, that is a much better version of what I had in mind that can go\neither before this step as a preparatory cleanup, squashed into this\nas \"while at it\", or after the series as a finishing touches. The\nlast one will let the codebase lie for a short while, though, so I am\nlikely to squash it in or wiggle it under.\n\nThanks.\n\n\n\n>\n> -- >8 --\n> Subject: [PATCH] log: clarify comment about reflog cycles\n>\n> When we're walking reflogs, we leave the commit buffer and\n> parents in place. A comment explains that this is due to\n> \"cycles\". But the interesting thing is the unsaid\n> implication: that the cycles (plus our clearing of the SEEN\n> flag) will cause us to show commits multiple times. Let's\n> spell it out.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/log.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 9c8bb3b5c..630d6cff2 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -372,7 +372,10 @@ static int cmd_log_walk(struct rev_info *rev)\n> \t\t\t */\n> \t\t\trev->max_count++;\n> \t\tif (!rev->reflog_info) {\n> -\t\t\t/* we allow cycles in reflog ancestry */\n> +\t\t\t/*\n> +\t\t\t * We may show a given commit multiple times when\n> +\t\t\t * walking the reflogs.\n> +\t\t\t */\n> \t\t\tfree_commit_buffer(commit);\n> \t\t\tfree_commit_list(commit->parents);\n> \t\t\tcommit->parents = NULL;\n"},{"id":"324053","messageId":"20170710094255.GA8170@untitled","threadId":"46227","inReplyTo":"20170707030224.4iubuzsz2qwdhgiw@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] reflog-walk: stop using fake parents","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2017-07-10T09:42:55Z","receivedAt":"2017-07-10T09:43:06Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Fri, Jul 07, 2017 at 12:32:39AM +0000, Eric Wong wrote:\n> \n> > I'm not sure why, but this is causing t1414.8 failures on 32-bit\n> > x86 with the latest pu with Debian jessie (oldstable).\n> > \n> > Reverting this (beafb2c62947a6d4a97b9c3baf99fe62ec8e830f) in pu\n> > seems to fix the test for me.\n> > \n> > +Cc: Ramsay since he also had a 32-bit environment.\n> \n> Thanks, I was able to reproduce with CFLAGS=-m32.\n\nNo problem and thanks for the quick response! Latest pu is\nfine for me.\n\nTo notice similar errors sooner, I wonder if we should have a\ntest target for 64-bit users to test with -m32 enabled, somehow.\n\nFwiw, I started having a cronjob which runs \"make test-pu\" on my\n32-bit machine, where config.mak has this:\n\n==> config.mak <==\nDEVELOPER := 1\npulog := /tmp/git-test-pu.log\n\ncur_head = $(shell git rev-parse HEAD^0)\ncur_pu = $(shell git rev-parse refs/remotes/origin/pu^0)\n\ntest-pu:\n\tgit fetch -q origin\n\ttest x\"$(cur_pu)\" = x\"$(cur_head)\" || { \\\n\t\t$(MAKE) clean && \\\n\t\tgit reset --hard origin/pu && \\\n\t\t{ $(MAKE) -k test >$(pulog) 2>&1 || \\\n\t\t mutt -s 'pufail' e@80x24.org <$(pulog); }; }\n-- \nEW\n"},{"id":"324055","messageId":"20170710112000.vmxscs7fpop3c6pq@sigill.intra.peff.net","threadId":"46227","inReplyTo":"20170710094255.GA8170@untitled","subject":"Re: [PATCH 6/6] reflog-walk: stop using fake parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T11:20:00Z","receivedAt":"2017-07-10T11:20:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 09:42:55AM +0000, Eric Wong wrote:\n\n> > Thanks, I was able to reproduce with CFLAGS=-m32.\n> \n> No problem and thanks for the quick response! Latest pu is\n> fine for me.\n> \n> To notice similar errors sooner, I wonder if we should have a\n> test target for 64-bit users to test with -m32 enabled, somehow.\n\nIt's \"just\"\n\n make CFLAGS=-m32 test\n\nonce you are setup for building 32-bit binaries. But that setup can be a\nbit tricky. On Debian, I have multi-arch set up, and then I have to\n\"apt-get install zlib1g-dev:i386\" etc for all the dependencies. Which\nisn't _too_ bad except that libcurl4-openssl-dev conflicts between the\namd64 and i386 versions due to /usr/bin/curl-config. So I have to\ninstall the i386 package, do the -m32 build, and then reinstall the\namd64 one to go back to my regular builds.\n\nAt any rate, the 32-bit thing in this instance[1] mostly just tickled\nthe bug in a different way. Testing with either ASan or valgrind also\ndetected the problem on 64-bit machines, and people (including me) do\nrun those periodically. In this case the bug was years old. The reason\nit wasn't caught earlier is that there wasn't test coverage, and my\nseries added it for other reasons. So you really _did_ catch it as soon\nas it hit pu, which seems pretty reasonable.\n\n-Peff\n\n[1] There are bugs that really are 32-bit-only, of course, and\n instrumenting tools wouldn't catch those. I think we had a bug a\n while back where the code mistook sizeof(uint64_t*) for\n sizeof(uint64_t). They're the same on 64-bit platforms, but not on\n 32-bit ones. I do think such bugs are relatively rare, though.\n"},{"id":"324062","messageId":"20170710132744.scjkpaqzunrx7gnf@sigill.intra.peff.net","threadId":"46227","inReplyTo":"xmqqtw2l8s22.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/7] log: do not free parents when walking reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T13:27:44Z","receivedAt":"2017-07-10T13:27:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 09, 2017 at 09:59:33AM -0700, Junio C Hamano wrote:\n\n> >> After step 6/7, we no longer \"allow cycles in reflog ancestry\", as\n> >> there will be no reflog ancestry to speak of ;-), so it would be\n> >> nice to remove the comment above in that step. But alternatively,\n> >> we can rephrase the comment here, to say something like \"the same\n> >> commit can be shown multiple times while showing entries from the\n> >> reflog\" instead.\n> >\n> > I actually think the comment is a bit obtuse in the first place. The\n> > real issue is that we show commits multiple times. That's caused by\n> > cycles, yes, but also by us clearing the SEEN flag. ;)\n> >\n> > Maybe this on top?\n> \n> Yup, that is a much better version of what I had in mind that can go\n> either before this step as a preparatory cleanup, squashed into this\n> as \"while at it\", or after the series as a finishing touches. The\n> last one will let the codebase lie for a short while, though, so I am\n> likely to squash it in or wiggle it under.\n\nWhere you put it on jk/reflog-walk in your tree is fine, though note the\ncommit message is slightly inaccurate there (it says \"commit buffer and\nparents\" but at that point it is really just \"commit buffer\").\n\n-Peff\n"},{"id":"324079","messageId":"xmqq4luk8e9s.fsf@gitster.mtv.corp.google.com","threadId":"46227","inReplyTo":"20170710094255.GA8170@untitled","subject":"Re: [PATCH 6/6] reflog-walk: stop using fake parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-10T16:09:35Z","receivedAt":"2017-07-10T16:09:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> To notice similar errors sooner, I wonder if we should have a\n> test target for 64-bit users to test with -m32 enabled, somehow.\n\nThanks. Hopefully https://travis-ci.org/git/git/jobs/251772744 and\nits later incarnations should be sufficient?\n"}]}