{"thread":{"id":"34600","subject":"[PATCH] log: use true parents for diff when walking reflogs","startedAt":"2013-08-03T10:36:15Z","lastAt":"2013-08-05T19:16:01Z","messageCount":3,"participants":["Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224502","messageId":"d6dadc4ab54d81490ca46bcfbd44a61be24f6eb7.1375524982.git.trast@inf.ethz.ch","threadId":"34600","inReplyTo":null,"subject":"[PATCH] log: use true parents for diff when walking reflogs","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-08-03T10:36:15Z","receivedAt":"2013-08-03T10:36:15Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The reflog walking logic (git log -g) replaces the true parent list\nwith the preceding commit in the reflog. This results in bogus commit\ndiffs when combined with options such as -p; the diff is against the\nreflog predecessor, not the parent of the commit.\n\nSave the true parents on the side, extending the functions from the\nprevious commit. The diff logic picks them up and uses them to show\nthe correct diffs.\n\nWe do have to be somewhat careful about repeated calling of\nsave_parents(), since the reflog may list a commit more than once. We\nnow store (commit_list*)-1 to distinguish the \"not saved yet\" and\n\"root commit\" cases. This lets us preserve an empty parent list even\nif save_parents() is repeatedly called.\n\nSuggested-by: Jeff King \nSigned-off-by: Thomas Rast \n---\n\nJeff King wrote:\n> \n> Your description (and solution) make a lot of sense to me. Another code\n> path that has a similar problem is the \"-g\" reflog walker. It rewrites\n> the parents based on the reflog, and the diffs it produces are mostly\n> useless (e.g., try \"git stash list -p\").\n> \n> Should we be applying the same technique there?\n\nGood point. This is how. It applies on top of the other patch.\n\nIt doesn't really help for 'git stash list -p', though, because\nstashes are merge commits. Now they just don't show anything. You\ncould try 'git stash list -p -m', though.\n\n\n revision.c | 28 +++++++++++++++++++++++++---\n t/t1411-reflog-show.sh | 22 ++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex e3ca936..ac20d1a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2848,6 +2848,7 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\tfree(entry);\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@@ -3083,6 +3084,8 @@ void put_revision_mark(const struct rev_info *revs, const struct commit *commit)\n \n define_commit_slab(saved_parents, struct commit_list *);\n \n+#define EMPTY_PARENT_LIST ((struct commit_list *)-1)\n+\n void save_parents(struct rev_info *revs, struct commit *commit)\n {\n \tstruct commit_list **pp;\n@@ -3093,16 +3096,35 @@ void save_parents(struct rev_info *revs, struct commit *commit)\n \t}\n \n \tpp = saved_parents_at(revs->saved_parents_slab, commit);\n-\tassert(*pp == NULL);\n-\t*pp = copy_commit_list(commit->parents);\n+\n+\t/*\n+\t * When walking with reflogs, we may visit the same commit\n+\t * several times: once for each appearance in the reflog.\n+\t *\n+\t * In this case, save_parents() will be called multiple times.\n+\t * We want to keep only the first set of parents. We need to\n+\t * store a sentinel value for an empty (i.e., NULL) parent\n+\t * list to distinguish it from a not-yet-saved list, however.\n+\t */\n+\tif (*pp)\n+\t\treturn;\n+\tif (commit->parents)\n+\t\t*pp = copy_commit_list(commit->parents);\n+\telse\n+\t\t*pp = EMPTY_PARENT_LIST;\n }\n \n struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit)\n {\n+\tstruct commit_list *parents;\n+\n \tif (!revs->saved_parents_slab)\n \t\treturn commit->parents;\n \n-\treturn *saved_parents_at(revs->saved_parents_slab, commit);\n+\tparents = *saved_parents_at(revs->saved_parents_slab, commit);\n+\tif (parents == EMPTY_PARENT_LIST)\n+\t\treturn NULL;\n+\treturn parents;\n }\n \n void free_saved_parents(struct rev_info *revs)\ndiff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh\nindex 9a105fe..6f47c0d 100755\n--- a/t/t1411-reflog-show.sh\n+++ b/t/t1411-reflog-show.sh\n@@ -144,4 +144,26 @@ test_expect_success 'empty reflog file' '\n \ttest_cmp expect actual\n '\n \n+# This guards against the alternative of showing the diffs vs. the\n+# reflog ancestor. The reflog used is designed to list the commits\n+# more than once, so as to exercise the corresponding logic.\n+test_expect_success 'git log -g -p shows diffs vs. parents' '\n+\ttest_commit two &&\n+\tgit branch flipflop &&\n+\tgit update-ref refs/heads/flipflop -m flip1 HEAD^ &&\n+\tgit update-ref refs/heads/flipflop -m flop1 HEAD &&\n+\tgit update-ref refs/heads/flipflop -m flip2 HEAD^ &&\n+\tgit log -g -p flipflop >reflog &&\n+\tgrep -v ^Reflog reflog >actual &&\n+\tgit log -1 -p HEAD^ >log.one &&\n+\tgit log -1 -p HEAD >log.two &&\n+\t(\n+\t\tcat log.one; echo\n+\t\tcat log.two; echo\n+\t\tcat log.one; echo\n+\t\tcat log.two\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.4.rc1.414.g34bc5b2\n"},{"id":"224503","messageId":"87mwoz6os8.fsf@hexa.v.cablecom.net","threadId":"34600","inReplyTo":"d6dadc4ab54d81490ca46bcfbd44a61be24f6eb7.1375524982.git.trast@inf.ethz.ch","subject":"Re: [PATCH] log: use true parents for diff when walking reflogs","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-08-03T10:40:07Z","receivedAt":"2013-08-03T10:40:07Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Doh, once again I forgot the in-reply-to. This patch continues the\nthread after <20130731225520.GB25882@sigill.intra.peff.net>. This is --\nstrangely enough -- missing from gmane; its immediate predecessor is\nthis:\n\n http://thread.gmane.org/gmane.comp.version-control.git/230968/focus=231453\n\nSorry for the noise.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"224617","messageId":"7vd2psj6dq.fsf@alter.siamese.dyndns.org","threadId":"34600","inReplyTo":"d6dadc4ab54d81490ca46bcfbd44a61be24f6eb7.1375524982.git.trast@inf.ethz.ch","subject":"Re: [PATCH] log: use true parents for diff when walking reflogs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-05T19:16:01Z","receivedAt":"2013-08-05T19:16:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> The reflog walking logic (git log -g) replaces the true parent list\n> with the preceding commit in the reflog. This results in bogus commit\n> diffs when combined with options such as -p; the diff is against the\n> reflog predecessor, not the parent of the commit.\n>\n> Save the true parents on the side, extending the functions from the\n> previous commit. The diff logic picks them up and uses them to show\n> the correct diffs.\n>\n> We do have to be somewhat careful about repeated calling of\n> save_parents(), since the reflog may list a commit more than once. We\n> now store (commit_list*)-1 to distinguish the \"not saved yet\" and\n> \"root commit\" cases. This lets us preserve an empty parent list even\n> if save_parents() is repeatedly called.\n>\n> Suggested-by: Jeff King \n> Signed-off-by: Thomas Rast \n> ---\n>\n> Jeff King wrote:\n>> \n>> Your description (and solution) make a lot of sense to me. Another code\n>> path that has a similar problem is the \"-g\" reflog walker. It rewrites\n>> the parents based on the reflog, and the diffs it produces are mostly\n>> useless (e.g., try \"git stash list -p\").\n>> \n>> Should we be applying the same technique there?\n>\n> Good point. This is how. It applies on top of the other patch.\n\nThanks.\n\n> It doesn't really help for 'git stash list -p', though, because\n> stashes are merge commits. Now they just don't show anything.\n> could try 'git stash list -p -m', though.\n\nUsing --first-parent may be more convenient and useful.\n"}]}