{"thread":{"id":"30843","subject":"[PATCH 1/2] Demonstrate git-show is broken with ranges","startedAt":"2012-06-19T20:04:57Z","lastAt":"2012-06-20T17:47:23Z","messageCount":7,"participants":["Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"193872","messageId":"d3e839101b031a7208e74a0b6e22d343d5a093e9.1340136145.git.trast@student.ethz.ch","threadId":"30843","inReplyTo":null,"subject":"[PATCH 1/2] Demonstrate git-show is broken with ranges","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-06-19T20:04:57Z","receivedAt":"2012-06-19T20:04:57Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The logic of git-show has remained largely unchanged since around\n5d7eeee (git-show: grok blobs, trees and tags, too, 2006-12-14): start\na revision walker with no_walk=1, look at its pending objects and\nhandle them one-by-one.  For commits, this means stuffing them into a\nnew queue all alone, and running the walker.\n\nThen Linus's f222abd (Make 'git show' more useful, 2009-07-13) came\nalong and set no_walk=0 whenever the user specifies a range.  Which\nappears to work fine, until you actually prod it hard enough, as the\npreceding commit shows: UNINTERESTING commits will be marked as such,\nbut not walked further to propagate the marks.\n\nDemonstrate this with the main tests of this patch: 'showing a range\nwalks (Y shape)'.  The Y shape of history ensures that propagating the\nUNINTERESTING marks is necessary to correctly exclude the main1\ncommit.  The only example I could find actually requires that the\nnegative revisions are listed later, and in this scenario a dotted\nrange actually works.  However, it is easy to find examples in git.git\nwhere a dotted range is wrong, e.g.\n\n  $ git show v1.7.0..v1.7.1 | grep ^commit | wc -l\n  1297\n  $ git rev-list v1.7.0..v1.7.1 | wc -l\n  702\n\nWhile there, also test a few other things that are not covered so far:\nthe -N way of triggering a range (added in 5853cae, DWIM 'git show -5'\nto 'git show --do-walk -5', 2010-06-01), and the interactions of tags,\ncommits and ranges.\n\nPointed out by Dr_Memory on #git.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n t/t7007-show.sh | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 92 insertions(+)\n\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex cce222f..1c43963 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -17,4 +17,96 @@ test_expect_success 'showing a tag that point at a missing object' '\n \ttest_must_fail git --no-pager show foo-tag\n '\n \n+test_expect_success 'set up a bit of history' '\n+\ttest_commit main1 &&\n+\ttest_commit main2 &&\n+\ttest_commit main3 &&\n+\tgit tag -m \"annotated tag\" annotated &&\n+\tgit checkout -b side HEAD^^ &&\n+\ttest_commit side2 &&\n+\ttest_commit side3\n+'\n+\n+test_expect_success 'showing two commits' '\n+\tcat >expect <<EOF &&\n+commit $(git rev-parse main2)\n+commit $(git rev-parse main3)\n+EOF\n+\tgit show main2 main3 >actual &&\n+\tgrep ^commit actual >actual.filtered &&\n+\ttest_cmp expect actual.filtered\n+'\n+\n+test_expect_success 'showing a range walks (linear)' '\n+\tcat >expect <<EOF &&\n+commit $(git rev-parse main3)\n+commit $(git rev-parse main2)\n+EOF\n+\tgit show main1..main3 >actual &&\n+\tgrep ^commit actual >actual.filtered &&\n+\ttest_cmp expect actual.filtered\n+'\n+\n+test_expect_success 'showing a range walks (Y shape, ^ first)' '\n+\tcat >expect <<EOF &&\n+commit $(git rev-parse main3)\n+commit $(git rev-parse main2)\n+EOF\n+\tgit show ^side3 main3 >actual &&\n+\tgrep ^commit actual >actual.filtered &&\n+\ttest_cmp expect actual.filtered\n+'\n+\n+test_expect_failure 'showing a range walks (Y shape, ^ last)' '\n+\tcat >expect <<EOF &&\n+commit $(git rev-parse main3)\n+commit $(git rev-parse main2)\n+EOF\n+\tgit show main3 ^side3 >actual &&\n+\tgrep ^commit actual >actual.filtered &&\n+\ttest_cmp expect actual.filtered\n+'\n+\n+test_expect_success 'showing with -N walks' '\n+\tcat >expect <<EOF &&\n+commit $(git rev-parse main3)\n+commit $(git rev-parse main2)\n+EOF\n+\tgit show -2 main3 >actual &&\n+\tgrep ^commit actual >actual.filtered &&\n+\ttest_cmp expect actual.filtered\n+'\n+\n+test_expect_success 'showing annotated tag' '\n+\tcat >expect <<EOF &&\n+tag annotated\n+commit $(git rev-parse annotated^{commit})\n+EOF\n+\tgit show annotated >actual &&\n+\tgrep -E \"^(commit|tag)\" actual >actual.filtered &&\n+\ttest_cmp expect actual.filtered\n+'\n+\n+test_expect_success 'showing annotated tag plus commit' '\n+\tcat >expect <<EOF &&\n+tag annotated\n+commit $(git rev-parse annotated^{commit})\n+commit $(git rev-parse side3)\n+EOF\n+\tgit show annotated side3 >actual &&\n+\tgrep -E \"^(commit|tag)\" actual >actual.filtered &&\n+\ttest_cmp expect actual.filtered\n+'\n+\n+test_expect_success 'showing annotated tag in range' '\n+\tcat >expect <<EOF &&\n+tag annotated\n+commit $(git rev-parse main3)\n+commit $(git rev-parse main2)\n+EOF\n+\tgit show ^side3 annotated >actual &&\n+\tgrep -E \"^(commit|tag)\" actual >actual.filtered &&\n+\ttest_cmp expect actual.filtered\n+'\n+\n test_done\n-- \n1.7.11.266.g2b10bc0\n"},{"id":"193873","messageId":"a598bb8c20221679e295caa743197c86219eda68.1340136145.git.trast@student.ethz.ch","threadId":"30843","inReplyTo":"d3e839101b031a7208e74a0b6e22d343d5a093e9.1340136145.git.trast@student.ethz.ch","subject":"[PATCH 2/2] Fix ranges with git-show","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-06-19T20:04:58Z","receivedAt":"2012-06-19T20:04:58Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"As explained in the previous commit, git-show's DWIM walking mode\nfails with ranges where propagating the UNINTERESTING marks is needed\nfor correctness.\n\nTo fix this issue, use a new strategy for of dealing with commits:\nhandle everything else first, then pass all commits to a single\nrevision walker \"in bulk\".  This keeps the UNINTERESTING commits and\ncorrectly shows all ranges.\n\nSadly we can use this only when actually walking.  In other cases,\nsuch as 'git show A B', it would seem more important to keep the order\nof the objects shown consistent with the command line.  There are no\nsuch guarantees when running the walker with several commits (even in\nno-walk mode), so we still feed them one by one.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n builtin/log.c   | 16 ++++++++++++----\n t/t7007-show.sh |  2 +-\n 2 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 4f1b42a..26f6c01 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -444,6 +444,7 @@ static void show_rev_tweak_rev(struct rev_info *rev, struct setup_revision_opt *\n int cmd_show(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info rev;\n+\tstruct object_array commits = {0};\n \tstruct object_array_entry *objects;\n \tstruct setup_revision_opt opt;\n \tstruct pathspec match_all;\n@@ -505,15 +506,22 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\trev.shown_one = 1;\n \t\t\tbreak;\n \t\tcase OBJ_COMMIT:\n-\t\t\trev.pending.nr = rev.pending.alloc = 0;\n-\t\t\trev.pending.objects = NULL;\n-\t\t\tadd_object_array(o, name, &rev.pending);\n-\t\t\tret = cmd_log_walk(&rev);\n+\t\t\tadd_object_array(o, name, &commits);\n+\t\t\tif (rev.no_walk) {\n+\t\t\t\trev.pending = commits;\n+\t\t\t\tret = cmd_log_walk(&rev);\n+\t\t\t\tcommits.nr = commits.alloc = 0;\n+\t\t\t\tcommits.objects = NULL;\n+\t\t\t}\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tret = error(_(\"Unknown type: %d\"), o->type);\n \t\t}\n \t}\n+\tif (!ret && commits.nr) {\n+\t\trev.pending = commits;\n+\t\tret = cmd_log_walk(&rev);\n+\t}\n \tfree(objects);\n \treturn ret;\n }\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex 1c43963..3936ff7 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -57,7 +57,7 @@ EOF\n \ttest_cmp expect actual.filtered\n '\n \n-test_expect_failure 'showing a range walks (Y shape, ^ last)' '\n+test_expect_success 'showing a range walks (Y shape, ^ last)' '\n \tcat >expect <<EOF &&\n commit $(git rev-parse main3)\n commit $(git rev-parse main2)\n-- \n1.7.11.266.g2b10bc0\n"},{"id":"193881","messageId":"7vhau7ypk6.fsf@alter.siamese.dyndns.org","threadId":"30843","inReplyTo":"a598bb8c20221679e295caa743197c86219eda68.1340136145.git.trast@student.ethz.ch","subject":"Re: [PATCH 2/2] Fix ranges with git-show","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-19T21:11:53Z","receivedAt":"2012-06-19T21:11:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> As explained in the previous commit, git-show's DWIM walking mode\n> fails with ranges where propagating the UNINTERESTING marks is needed\n> for correctness.\n\nFinally ;-)\n\nAnother bad thing about the Linus's walking hackery is it depends on\nthe order of parameters (e.g \"show maint master..next\" and \"show\nmaster..next maint\" gives us vastly different results).\n\n> To fix this issue, use a new strategy for of dealing with commits:\n> handle everything else first, then pass all commits to a single\n> revision walker \"in bulk\".  This keeps the UNINTERESTING commits and\n> correctly shows all ranges.\n\nIn other words, do not implement a half-hearted revision walking\nitself, but let the proper call to cmd_log_walk() take care of the\nwalking.  Good.\n\n> Sadly we can use this only when actually walking.\n\nI do not think it is sad at all.  If we were told to walk, we should\nwalk just like \"git log\" does, and otherwise we just show them one\nby one.  It might be even cleaner if we separate the command line\nparsing and showing into separate phases, instead of the current\nloop structure of parsing one object and deciding how to show.\n\nHow about doing it this way without applying your 2/2?\n\nAmong your new tests:\n\n\tgit show ^side3 annotated\n\nwhich is equivalent to\n\n\tgit show side3..annotate\n\nmust change, as it is not asking to show individual objects (in\nother words, I think the test expects a wrong thing), but everything\nelse should work as expected (I didn't check).\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 56bc555..9ea2eb7 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -451,6 +451,9 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \topt.tweak = show_rev_tweak_rev;\n \tcmd_log_init(argc, argv, prefix, &rev, &opt);\n \n+\tif (!rev.no_walk)\n+\t\treturn cmd_log_walk(&rev);\n+\n \tcount = rev.pending.nr;\n \tobjects = rev.pending.objects;\n \tfor (i = 0; i < count && !ret; i++) {\n"},{"id":"193882","messageId":"7vd34vyoo1.fsf@alter.siamese.dyndns.org","threadId":"30843","inReplyTo":"7vhau7ypk6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Fix ranges with git-show","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-19T21:31:10Z","receivedAt":"2012-06-19T21:31:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thomas Rast <trast@student.ethz.ch> writes:\n>\n>> As explained in the previous commit, git-show's DWIM walking mode\n>> fails with ranges where propagating the UNINTERESTING marks is needed\n>> for correctness.\n>\n> Finally ;-)\n>\n> Another bad thing about the Linus's walking hackery is it depends on\n> the order of parameters (e.g \"show maint master..next\" and \"show\n> master..next maint\" gives us vastly different results).\n\nBy the way, this reminds me of a totally separate topic we discussed\nhere recently.\n\nImagine you were on the maint-1.7.9 branch and gave this command.\n\n\tgit cherry-pick maint master..topic\n\nThe range makes the command walk, and because of that, the above\ndoes not result in transplanting a topic that depends on the commit\nat the tip of maint to an older maint-1.7.9 branch.  You would\ninstead need to write something like:\n\n\tgit cherry-pick maint $(git rev-list --reverse master..topic)\n\nto prevent cherry-pick from walking.\n\nBut we could introduce a new \"mode\" to the revision machinery so\nthat a single token on the command line is parsed as a set of\ncommits, and causes the \"walking\" machinery not to walk the commits\nin the union of the sets of commits specified from the command line,\ne.g. (pardon my terrible option name)\n\n    git cherry-pick --union-no-walk maint master..topic1 master..topic2\n\nwhich would instruct the revision machinery that:\n\n (1) \"maint\" (the first token) yields a set that consists of a\n     single commit at the tip of \"maint\";\n\n (2) \"master..topic1\" (another token) yields a separate set that\n     consists of commits on the \"topic1\" branch since it forked from\n     the \"master\" (similarly for \"topic2\");\n\n (3) the revision machinery is to compute these two sets\n     _independently_ (meaning, object flags like UNINTERESTING are\n     cleared after each run);\n\n (4) take union of the set, eliminating duplication (if \"topic1\" and\n     \"topic2\" share some commits at their bottom, they are\n     de-duped); and\n\n (5) the call to get_revision() will not walk, and instead return\n     the elements of the resulting union one-by-one.\n\nwhich would give us what most users of cherry-pick would naturally\nexpect.  For some commands (like cherry-pick and show), we might\neven want default to --union-no-walk behaviour, but that is probably\na Git 2.0 topic after we gain enough confidence with experience.\n"},{"id":"193884","messageId":"7v4nq7yoep.fsf@alter.siamese.dyndns.org","threadId":"30843","inReplyTo":"7vd34vyoo1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Fix ranges with git-show","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-19T21:36:46Z","receivedAt":"2012-06-19T21:36:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Thomas Rast <trast@student.ethz.ch> writes:\n>>\n>>> As explained in the previous commit, git-show's DWIM walking mode\n>>> fails with ranges where propagating the UNINTERESTING marks is needed\n>>> for correctness.\n>>\n>> Finally ;-)\n>>\n>> Another bad thing about the Linus's walking hackery is it depends on\n>> the order of parameters (e.g \"show maint master..next\" and \"show\n>> master..next maint\" gives us vastly different results).\n>\n> By the way, this reminds me of a totally separate topic we discussed\n> here recently.\n>\n> Imagine you were on the maint-1.7.9 branch and gave this command.\n>\n> \tgit cherry-pick maint master..topic\n>\n> The range makes the command walk, and because of that, the above\n> does not result in transplanting a topic that depends on the commit\n> at the tip of maint to an older maint-1.7.9 branch.  You would\n> instead need to write something like:\n>\n> \tgit cherry-pick maint $(git rev-list --reverse master..topic)\n>\n> to prevent cherry-pick from walking.\n>\n> But we could introduce a new \"mode\" to the revision machinery so\n> that a single token on the command line is parsed as a set of\n> commits, and causes the \"walking\" machinery not to walk the commits\n> in the union of the sets of commits specified from the command line,\n> e.g. (pardon my terrible option name)\n>\n>     git cherry-pick --union-no-walk maint master..topic1 master..topic2\n>\n> which would instruct the revision machinery that:\n>\n>  (1) \"maint\" (the first token) yields a set that consists of a\n>      single commit at the tip of \"maint\";\n>\n>  (2) \"master..topic1\" (another token) yields a separate set that\n>      consists of commits on the \"topic1\" branch since it forked from\n>      the \"master\" (similarly for \"topic2\");\n>\n>  (3) the revision machinery is to compute these two sets\n>      _independently_ (meaning, object flags like UNINTERESTING are\n>      cleared after each run);\n\nSorry, s/two sets/three sets/; I originally wrote without \"topic2\"\nand forgot to clean it up.\n\n>  (4) take union of the set, eliminating duplication (if \"topic1\" and\n>      \"topic2\" share some commits at their bottom, they are\n>      de-duped); and\n>\n>  (5) the call to get_revision() will not walk, and instead return\n>      the elements of the resulting union one-by-one.\n>\n> which would give us what most users of cherry-pick would naturally\n> expect.  For some commands (like cherry-pick and show), we might\n> even want default to --union-no-walk behaviour, but that is probably\n> a Git 2.0 topic after we gain enough confidence with experience.\n"},{"id":"193922","messageId":"87bokexwiq.fsf@thomas.inf.ethz.ch","threadId":"30843","inReplyTo":"7vhau7ypk6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Fix ranges with git-show","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-06-20T07:39:09Z","receivedAt":"2012-06-20T07:39:09Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I do not think it is sad at all.  If we were told to walk, we should\n> walk just like \"git log\" does, and otherwise we just show them one\n> by one.  It might be even cleaner if we separate the command line\n> parsing and showing into separate phases, instead of the current\n> loop structure of parsing one object and deciding how to show.\n>\n> How about doing it this way without applying your 2/2?\n[...]\n>  \topt.tweak = show_rev_tweak_rev;\n>  \tcmd_log_init(argc, argv, prefix, &rev, &opt);\n>  \n> +\tif (!rev.no_walk)\n> +\t\treturn cmd_log_walk(&rev);\n> +\n>  \tcount = rev.pending.nr;\n>  \tobjects = rev.pending.objects;\n>  \tfor (i = 0; i < count && !ret; i++) {\n\nClever.  But it eliminates all possibility of *simultaneously* showing a\nrange and some other objects, of which\n\n> Among your new tests:\n>\n> \tgit show ^side3 annotated\n>\n> must change, as it is not asking to show individual objects (in\n> other words, I think the test expects a wrong thing), but everything\n> else should work as expected (I didn't check).\n\nis just a symptom of.  Do you want to change it?  I'd be all for it, but\nit changes the meaning somewhat.  So far, showing \"anything plus ranges\"\nis broken only as far as the ranges are concerned.\n\nIf you do make this change, can we merge the log and show code?\nGranted, show defaults to -p --cc --no-walk, and log does not, but can\nwe then unify the main logic?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"193940","messageId":"7vmx3xx4d0.fsf@alter.siamese.dyndns.org","threadId":"30843","inReplyTo":"87bokexwiq.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 2/2] Fix ranges with git-show","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-20T17:47:23Z","receivedAt":"2012-06-20T17:47:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@inf.ethz.ch> writes:\n\n> Clever.  But it eliminates all possibility of *simultaneously* showing a\n> range and some other objects, of which\n>\n>> Among your new tests:\n>>\n>> \tgit show ^side3 annotated\n>>\n>> must change, as it is not asking to show individual objects (in\n>> other words, I think the test expects a wrong thing), but everything\n>> else should work as expected (I didn't check).\n>\n> is just a symptom of.  Do you want to change it?  \n\nI do not understand the question.  Do I think your patch is broken\nthat \"git show ^side3 annotated\" shows the annotated tag?  Yes, I\nthink it is broken, and it is the symptom of the same breakage as\nthe \"If we have a range, let's walk it outselves without doing\nexactly the same thing as the normal 'log' walk\".  In other words,\n\"git show\" is to show individual objects unless you feed ranges.  If\nyou feed ranges, it will act as \"git log\".\n\n> So far, showing \"anything plus ranges\"\n> is broken only as far as the ranges are concerned.\n\nYes, I think the command works correctly only when you do not give\nany range, and once you give any range, its output is utterly\nbroken.\n\n> If you do make this change, can we merge the log and show code?\n> Granted, show defaults to -p --cc --no-walk, and log does not, but can\n> we then unify the main logic?\n\nWhat \"main logic\" remains?  \n\nI offhand do not think of a need to do anything more than the above\npatch, but I may not be getting the benefit you are seeing.  \n\nThe remainder of cmd_show() are about showing individual objects\n(including commits), which has very different semantics from what\n\"log\" does, no?\n\n\"log\" asks get_revision() to grab each commit while digging the\nhistory deeper with add_parents_to_list() and feed the returned\ncommit to the internal diff-tree , but \"show\" does not have to use\nget_revision() because it already has list of objects it wants to\nshow, and calling into get_revision() is not even necessary as it\ndoes not want it to do add_parents_to_list() to find more commits to\nshow.\n"}]}