* [PATCH 0/1] Use new topo-order logic with GIT_TEST_COMMIT_GRAPH @ 2018-11-19 19:03 Derrick Stolee via GitGitGadget 2018-11-19 19:03 ` [PATCH 1/1] revision.c: use new topo-order logic in tests Derrick Stolee via GitGitGadget 0 siblings, 1 reply; 4+ messages in thread From: Derrick Stolee via GitGitGadget @ 2018-11-19 19:03 UTC (permalink / raw) To: git; +Cc: Junio C Hamano The recent Git test report for v2.20.0-rc0 shows that the logic around UNINTERESTING commits is not covered by the test suite. This is because the code is currently unreachable! See the commit message for details. An alternate approach would be to delete the code around UNINTERESTING commits, but that doesn't seem necessary. Thanks, -Stolee Derrick Stolee (1): revision.c: use new topo-order logic in tests revision.c | 4 ++++ 1 file changed, 4 insertions(+) base-commit: 561b583749b7428f1790f03164d0d0e75be71d7b Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-83%2Fderrickstolee%2Ftopo-order-test-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-83/derrickstolee/topo-order-test-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/83 -- gitgitgadget ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/1] revision.c: use new topo-order logic in tests 2018-11-19 19:03 [PATCH 0/1] Use new topo-order logic with GIT_TEST_COMMIT_GRAPH Derrick Stolee via GitGitGadget @ 2018-11-19 19:03 ` Derrick Stolee via GitGitGadget 2018-11-20 6:13 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: Derrick Stolee via GitGitGadget @ 2018-11-19 19:03 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> The revision-walk machinery is being rewritten to use generation numbers in the commit-graph when availble. Due to some problematic commit histories, the new logic can be slower than the previous method due to how commit dates and generation numbers interact. Thus, the new logic is not used in comparison queries, such as git log --topo-order A..B The logic for these queries was implemented during the refactor, but is unreachable due to the potential performance penalty. The code came along with a larger block of code that was copied from the old code. When generation numbers are updated to v2 (corrected commit dates), then we will no longer have a performance penalty and this logic is what we will want to use. In the meantime, use the new logic when GIT_TEST_COMMIT_GRAPH is enabled. This will demonstrate that the new logic works for all comparison queries in the test suite, including these variants: git log --topo-order --ancestry-path A..B git log --topo-order A...B Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- revision.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/revision.c b/revision.c index 4ef47d2fb4..d52da6e24f 100644 --- a/revision.c +++ b/revision.c @@ -27,6 +27,7 @@ #include "commit-reach.h" #include "commit-graph.h" #include "prio-queue.h" +#include "config.h" volatile show_early_output_fn_t show_early_output; @@ -3143,6 +3144,9 @@ int prepare_revision_walk(struct rev_info *revs) commit_list_sort_by_date(&revs->commits); if (revs->no_walk) return 0; + if (revs->limited && + git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) + revs->limited = 0; if (revs->limited) { if (limit_list(revs) < 0) return -1; -- gitgitgadget ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] revision.c: use new topo-order logic in tests 2018-11-19 19:03 ` [PATCH 1/1] revision.c: use new topo-order logic in tests Derrick Stolee via GitGitGadget @ 2018-11-20 6:13 ` Junio C Hamano 2018-11-20 18:45 ` Derrick Stolee 0 siblings, 1 reply; 4+ messages in thread From: Junio C Hamano @ 2018-11-20 6:13 UTC (permalink / raw) To: Derrick Stolee via GitGitGadget; +Cc: git, Derrick Stolee "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > @@ -3143,6 +3144,9 @@ int prepare_revision_walk(struct rev_info *revs) > commit_list_sort_by_date(&revs->commits); > if (revs->no_walk) > return 0; > + if (revs->limited && > + git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) > + revs->limited = 0; > if (revs->limited) { > if (limit_list(revs) < 0) > return -1; That is equivalent to say if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) revs->limited = 0; Wouldn't that make the codepath that involves limit_list() completely unreachable while testing, though? The title only mentions "topo-order" logic, but the topo-order is not the only reason why limited bit can be set, is it? When showing merges, simplifying merges, or post-processing to show ancestry path, or showing a bottom-limited revision range, the limited bit is automatically set because all of these depend on first calling limit_list() and then postprocessing its result. Doesn't it hurt these cases to unconditionally drop limited bit? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] revision.c: use new topo-order logic in tests 2018-11-20 6:13 ` Junio C Hamano @ 2018-11-20 18:45 ` Derrick Stolee 0 siblings, 0 replies; 4+ messages in thread From: Derrick Stolee @ 2018-11-20 18:45 UTC (permalink / raw) To: Junio C Hamano, Derrick Stolee via GitGitGadget; +Cc: git, Derrick Stolee On 11/20/2018 1:13 AM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> @@ -3143,6 +3144,9 @@ int prepare_revision_walk(struct rev_info *revs) >> commit_list_sort_by_date(&revs->commits); >> if (revs->no_walk) >> return 0; >> + if (revs->limited && >> + git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) >> + revs->limited = 0; >> if (revs->limited) { >> if (limit_list(revs) < 0) >> return -1; > That is equivalent to say > > if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) > revs->limited = 0; Not exactly equivalent, because we can use short-circuiting to avoid the git_env_bool check, but I see what you mean. > Wouldn't that make the codepath that involves limit_list() > completely unreachable while testing, though? Testing with GIT_TEST_COMMIT_GRAPH=0 would still hit limit_list(). Both modes are important to test (for instance, to ensure we still have correct behavior without a commit-graph file). > The title only mentions "topo-order" logic, but the topo-order is > not the only reason why limited bit can be set, is it? When showing > merges, simplifying merges, or post-processing to show ancestry > path, or showing a bottom-limited revision range, the limited bit is > automatically set because all of these depend on first calling > limit_list() and then postprocessing its result. Doesn't it hurt > these cases to unconditionally drop limited bit? You're right that we only want to do this in the topo-order case, so perhaps the diff should instead be: if (revs->no_walk) return 0; + if (revs->topo_order && + git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) + revs->limited = 0; if (revs->limited) { if (limit_list(revs) < 0) return -1; Thanks, -Stolee ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-11-20 18:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-19 19:03 [PATCH 0/1] Use new topo-order logic with GIT_TEST_COMMIT_GRAPH Derrick Stolee via GitGitGadget 2018-11-19 19:03 ` [PATCH 1/1] revision.c: use new topo-order logic in tests Derrick Stolee via GitGitGadget 2018-11-20 6:13 ` Junio C Hamano 2018-11-20 18:45 ` Derrick Stolee
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).