{"thread":{"id":"49860","subject":"[PATCH 0/1] Use new topo-order logic with GIT_TEST_COMMIT_GRAPH","startedAt":"2018-11-19T19:03:34Z","lastAt":"2018-11-20T18:45:22Z","messageCount":4,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"363657","messageId":"pull.83.git.gitgitgadget@gmail.com","threadId":"49860","inReplyTo":null,"subject":"[PATCH 0/1] Use new topo-order logic with GIT_TEST_COMMIT_GRAPH","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-19T19:03:31Z","receivedAt":"2018-11-19T19:03:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The recent Git test report for v2.20.0-rc0 shows that the logic around\nUNINTERESTING commits is not covered by the test suite. This is because the\ncode is currently unreachable! See the commit message for details.\n\nAn alternate approach would be to delete the code around UNINTERESTING\ncommits, but that doesn't seem necessary.\n\nThanks, -Stolee\n\nDerrick Stolee (1):\n  revision.c: use new topo-order logic in tests\n\n revision.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\n\nbase-commit: 561b583749b7428f1790f03164d0d0e75be71d7b\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-83%2Fderrickstolee%2Ftopo-order-test-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-83/derrickstolee/topo-order-test-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/83\n-- \ngitgitgadget\n"},{"id":"363658","messageId":"ab7c2dd46fb72523b865ecf34204c7ae31dee416.1542654209.git.gitgitgadget@gmail.com","threadId":"49860","inReplyTo":"pull.83.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] revision.c: use new topo-order logic in tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-19T19:03:33Z","receivedAt":"2018-11-19T19:03:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe revision-walk machinery is being rewritten to use generation\nnumbers in the commit-graph when availble. Due to some problematic\ncommit histories, the new logic can be slower than the previous\nmethod due to how commit dates and generation numbers interact.\nThus, the new logic is not used in comparison queries, such as\n\n\tgit log --topo-order A..B\n\nThe logic for these queries was implemented during the refactor,\nbut is unreachable due to the potential performance penalty. The\ncode came along with a larger block of code that was copied from\nthe old code. When generation numbers are updated to v2 (corrected\ncommit dates), then we will no longer have a performance penalty\nand this logic is what we will want to use.\n\nIn the meantime, use the new logic when GIT_TEST_COMMIT_GRAPH is\nenabled. This will demonstrate that the new logic works for all\ncomparison queries in the test suite, including these variants:\n\n\tgit log --topo-order --ancestry-path A..B\n\tgit log --topo-order A...B\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n revision.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 4ef47d2fb4..d52da6e24f 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -27,6 +27,7 @@\n #include \"commit-reach.h\"\n #include \"commit-graph.h\"\n #include \"prio-queue.h\"\n+#include \"config.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -3143,6 +3144,9 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\tcommit_list_sort_by_date(&revs->commits);\n \tif (revs->no_walk)\n \t\treturn 0;\n+\tif (revs->limited &&\n+\t    git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))\n+\t\trevs->limited = 0;\n \tif (revs->limited) {\n \t\tif (limit_list(revs) < 0)\n \t\t\treturn -1;\n-- \ngitgitgadget\n"},{"id":"363706","messageId":"xmqqsgzw9syx.fsf@gitster-ct.c.googlers.com","threadId":"49860","inReplyTo":"ab7c2dd46fb72523b865ecf34204c7ae31dee416.1542654209.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] revision.c: use new topo-order logic in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-20T06:13:42Z","receivedAt":"2018-11-20T06:13:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> @@ -3143,6 +3144,9 @@ int prepare_revision_walk(struct rev_info *revs)\n>  \t\tcommit_list_sort_by_date(&revs->commits);\n>  \tif (revs->no_walk)\n>  \t\treturn 0;\n> +\tif (revs->limited &&\n> +\t    git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))\n> +\t\trevs->limited = 0;\n>  \tif (revs->limited) {\n>  \t\tif (limit_list(revs) < 0)\n>  \t\t\treturn -1;\n\nThat is equivalent to say\n\n\tif (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))\n\t\trevs->limited = 0;\n\nWouldn't that make the codepath that involves limit_list()\ncompletely unreachable while testing, though?\n\nThe title only mentions \"topo-order\" logic, but the topo-order is\nnot the only reason why limited bit can be set, is it?  When showing\nmerges, simplifying merges, or post-processing to show ancestry\npath, or showing a bottom-limited revision range, the limited bit is\nautomatically set because all of these depend on first calling\nlimit_list() and then postprocessing its result.  Doesn't it hurt\nthese cases to unconditionally drop limited bit?\n\n"},{"id":"363762","messageId":"921606a2-3deb-2481-347a-6a2f5571ec9e@gmail.com","threadId":"49860","inReplyTo":"xmqqsgzw9syx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] revision.c: use new topo-order logic in tests","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-11-20T18:45:17Z","receivedAt":"2018-11-20T18:45:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/20/2018 1:13 AM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>> @@ -3143,6 +3144,9 @@ int prepare_revision_walk(struct rev_info *revs)\n>>   \t\tcommit_list_sort_by_date(&revs->commits);\n>>   \tif (revs->no_walk)\n>>   \t\treturn 0;\n>> +\tif (revs->limited &&\n>> +\t    git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))\n>> +\t\trevs->limited = 0;\n>>   \tif (revs->limited) {\n>>   \t\tif (limit_list(revs) < 0)\n>>   \t\t\treturn -1;\n> That is equivalent to say\n>\n> \tif (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))\n> \t\trevs->limited = 0;\n\nNot exactly equivalent, because we can use short-circuiting to avoid the \ngit_env_bool check, but I see what you mean.\n\n> Wouldn't that make the codepath that involves limit_list()\n> completely unreachable while testing, though?\n\nTesting with GIT_TEST_COMMIT_GRAPH=0 would still hit limit_list(). Both \nmodes are important to test (for instance, to ensure we still have \ncorrect behavior without a commit-graph file).\n\n> The title only mentions \"topo-order\" logic, but the topo-order is\n> not the only reason why limited bit can be set, is it?  When showing\n> merges, simplifying merges, or post-processing to show ancestry\n> path, or showing a bottom-limited revision range, the limited bit is\n> automatically set because all of these depend on first calling\n> limit_list() and then postprocessing its result.  Doesn't it hurt\n> these cases to unconditionally drop limited bit?\n\nYou're right that we only want to do this in the topo-order case, so \nperhaps the diff should instead be:\n\n  \tif (revs->no_walk)\n  \t\treturn 0;\n+\tif (revs->topo_order &&\n+\t    git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))\n+\t\trevs->limited = 0;\n  \tif (revs->limited) {\n  \t\tif (limit_list(revs) < 0)\n  \t\t\treturn -1;\n\nThanks,\n-Stolee\n\n"}]}