{"thread":{"id":"49600","subject":"[PATCH 0/3] Use commit-graph by default","startedAt":"2018-10-17T20:33:15Z","lastAt":"2018-10-19T00:36:56Z","messageCount":9,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"360780","messageId":"pull.50.git.gitgitgadget@gmail.com","threadId":"49600","inReplyTo":null,"subject":"[PATCH 0/3] Use commit-graph by default","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-17T20:33:10Z","receivedAt":"2018-10-17T20:33:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The commit-graph feature is starting to stabilize. Based on what is in\nmaster right now, we have:\n\nGit 2.18:\n\n * Ability to write commit-graph (requires user interaction).\n   \n   \n * Commit parsing is faster when commit-graph exists.\n   \n   \n * Must have core.commitGraph true to use.\n   \n   \n\nGit 2.19:\n\n * Ability to write commit-graph on GC with gc.writeCommitGraph.\n   \n   \n * Generation numbers written in commit-graph\n   \n   \n * A few reachability algorithms make use of generation numbers.\n   \n   \n\n(queued for) master:\n\n * The test suite passes with GIT_TEST_COMMIT_GRAPH=1\n   \n   \n * 'git commit-graph write' has progress indicators.\n   \n   \n * The commit-graph is automatically disabled when grafts or replace-objects\n   exist.\n   \n   \n\nThere are some other things coming that are in review (like 'git log\n--graph' speedups), but it is probably time to consider enabling the\ncommit-graph by default. This series does that.\n\nFor timing, I'm happy to leave this queued for a merge after the Git 2.20\nrelease. There are enough things in master to justify not enabling this by\ndefault until that goes out and more people use it.\n\nPATCH 3/3 is rather simple, and is the obvious thing to do to achieve\nenabling these config values by default.\n\nPATCH 1/3 is a required change to make the test suite work with this change.\nThis change isn't needed with GIT_TEST_COMMIT_GRAPH=1 because the\ncommit-graph is up-to-date for these 'git gc' calls, so no progress is\noutput.\n\nPATCH 2/3 is also a necessary evil, since we already had to disable\nGIT_TEST_COMMIT_GRAPH for some tests, we now also need to turn off\ncore.commitGraph.\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n  t6501: use --quiet when testing gc stderr\n  t: explicitly turn off core.commitGraph as needed\n  commit-graph: Use commit-graph by default\n\n Documentation/config.txt            | 4 ++--\n builtin/gc.c                        | 2 +-\n commit-graph.c                      | 6 +++---\n t/t0410-partial-clone.sh            | 3 ++-\n t/t5307-pack-missing-commit.sh      | 3 ++-\n t/t6011-rev-list-with-bad-commit.sh | 3 ++-\n t/t6024-recursive-merge.sh          | 3 ++-\n t/t6501-freshen-objects.sh          | 6 +++---\n 8 files changed, 17 insertions(+), 13 deletions(-)\n\n\nbase-commit: a4b8ab5363a32f283a61ef3a962853556d136c0e\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-50%2Fderrickstolee%2Fcommit-graph-default-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-50/derrickstolee/commit-graph-default-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/50\n-- \ngitgitgadget\n"},{"id":"360781","messageId":"ab118e60e9988076edf43d0ed8c1924e6569a9d0.1539808389.git.gitgitgadget@gmail.com","threadId":"49600","inReplyTo":"pull.50.git.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t6501: use --quiet when testing gc stderr","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-17T20:33:12Z","receivedAt":"2018-10-17T20:33:16Z","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 test script t6501-freshen-objects.sh has some tests that care\nif 'git gc' has any output to stderr. This is intended to say that\nno warnings occurred related to broken links. However, when we\nhave operations that output progress (like writing the commit-graph)\nthis causes the test to fail.\n\nUse 'git gc --quiet' to avoid these progress indicators from causing\na test failure.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t6501-freshen-objects.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh\nindex 033871ee5f..0973130f06 100755\n--- a/t/t6501-freshen-objects.sh\n+++ b/t/t6501-freshen-objects.sh\n@@ -137,7 +137,7 @@ test_expect_success 'do not complain about existing broken links (commit)' '\n \tsome message\n \tEOF\n \tcommit=$(git hash-object -t commit -w broken-commit) &&\n-\tgit gc 2>stderr &&\n+\tgit gc --quiet 2>stderr &&\n \tverbose git cat-file -e $commit &&\n \ttest_must_be_empty stderr\n '\n@@ -147,7 +147,7 @@ test_expect_success 'do not complain about existing broken links (tree)' '\n \t100644 blob 0000000000000000000000000000000000000003\tfoo\n \tEOF\n \ttree=$(git mktree --missing <broken-tree) &&\n-\tgit gc 2>stderr &&\n+\tgit gc --quiet 2>stderr &&\n \tgit cat-file -e $tree &&\n \ttest_must_be_empty stderr\n '\n@@ -162,7 +162,7 @@ test_expect_success 'do not complain about existing broken links (tag)' '\n \tthis is a broken tag\n \tEOF\n \ttag=$(git hash-object -t tag -w broken-tag) &&\n-\tgit gc 2>stderr &&\n+\tgit gc --quiet 2>stderr &&\n \tgit cat-file -e $tag &&\n \ttest_must_be_empty stderr\n '\n-- \ngitgitgadget\n\n"},{"id":"360782","messageId":"d2ea66630752f02f22e806725e5e3699d6dd2043.1539808389.git.gitgitgadget@gmail.com","threadId":"49600","inReplyTo":"pull.50.git.gitgitgadget@gmail.com","subject":"[PATCH 2/3] t: explicitly turn off core.commitGraph as needed","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-17T20:33:13Z","receivedAt":"2018-10-17T20:33:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThere are a few tests that already require GIT_TEST_COMMIT_GRAPH=0\nas they rely on an interaction with the commits in the object store\nthat is circumvented by parsing commit information from the\ncommit-graph instead. Before enabling core.commitGraph as true by\ndefault, explicitly turn the setting off for these tests.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t0410-partial-clone.sh            | 3 ++-\n t/t5307-pack-missing-commit.sh      | 3 ++-\n t/t6011-rev-list-with-bad-commit.sh | 3 ++-\n t/t6024-recursive-merge.sh          | 3 ++-\n 4 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex cfd0655ea1..f5277fafb1 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -193,7 +193,8 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tGIT_TEST_COMMIT_GRAPH=0 git -C repo rev-list --exclude-promisor-objects --objects bar >out &&\n+\tGIT_TEST_COMMIT_GRAPH=0 git -c core.commitGraph=false \\\n+\t\t-C repo rev-list --exclude-promisor-objects --objects bar >out &&\n \tgrep $(git -C repo rev-parse bar) out &&\n \t! grep $FOO out\n '\ndiff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh\nindex dacb440b27..dc4c19d0aa 100755\n--- a/t/t5307-pack-missing-commit.sh\n+++ b/t/t5307-pack-missing-commit.sh\n@@ -16,7 +16,8 @@ test_expect_success setup '\n \tobj=$(git rev-parse --verify tag3) &&\n \tfanout=$(expr \"$obj\" : \"\\(..\\)\") &&\n \tremainder=$(expr \"$obj\" : \"..\\(.*\\)\") &&\n-\trm -f \".git/objects/$fanout/$remainder\"\n+\trm -f \".git/objects/$fanout/$remainder\" &&\n+\tgit config core.commitGraph false\n '\n \n test_expect_success 'check corruption' '\ndiff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh\nindex 545b461e51..da6949743d 100755\n--- a/t/t6011-rev-list-with-bad-commit.sh\n+++ b/t/t6011-rev-list-with-bad-commit.sh\n@@ -42,7 +42,8 @@ test_expect_success 'corrupt second commit object' \\\n    '\n \n test_expect_success 'rev-list should fail' '\n-\ttest_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --all > /dev/null\n+\ttest_must_fail env GIT_TEST_COMMIT_GRAPH=0 \\\n+\t\tgit -c core.commitGraph=false rev-list --all > /dev/null\n '\n \n test_expect_success 'git repack _MUST_ fail' \\\ndiff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh\nindex 27c7de90ce..fccdf96f13 100755\n--- a/t/t6024-recursive-merge.sh\n+++ b/t/t6024-recursive-merge.sh\n@@ -61,7 +61,8 @@ GIT_AUTHOR_DATE=\"2006-12-12 23:00:08\" git commit -m F\n '\n \n test_expect_success 'combined merge conflicts' '\n-\ttest_must_fail env GIT_TEST_COMMIT_GRAPH=0 git merge -m final G\n+\ttest_must_fail env GIT_TEST_COMMIT_GRAPH=0 \\\n+\t\tgit -c core.commitGraph=false merge -m final G\n '\n \n cat > expect << EOF\n-- \ngitgitgadget\n\n"},{"id":"360783","messageId":"6f07487dcd6368982392613adb29438916c8738d.1539808389.git.gitgitgadget@gmail.com","threadId":"49600","inReplyTo":"pull.50.git.gitgitgadget@gmail.com","subject":"[PATCH 3/3] commit-graph: Use commit-graph by default","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-17T20:33:15Z","receivedAt":"2018-10-17T20:33:18Z","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 config setting \"core.commitGraph\" enables using the commit-graph\nfile to accelerate commit walks through parsing speed and generation\nnumbers. The setting \"gc.writeCommitGraph\" enables writing the\ncommit-graph file on every non-trivial 'git gc' operation. Together,\nthey help users automatically improve their performance.\n\nBy setting these config variables to true by default, we make the\ncommit-graph feature an \"opt-out\" feature instead of \"opt-in\".\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n Documentation/config.txt | 4 ++--\n builtin/gc.c             | 2 +-\n commit-graph.c           | 6 +++---\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f6f4c21a54..dc5ee7c145 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -923,7 +923,7 @@ the `GIT_NOTES_REF` environment variable.  See linkgit:git-notes[1].\n \n core.commitGraph::\n \tIf true, then git will read the commit-graph file (if it exists)\n-\tto parse the graph structure of commits. Defaults to false. See\n+\tto parse the graph structure of commits. Defaults to true. See\n \tlinkgit:git-commit-graph[1] for more information.\n \n core.useReplaceRefs::\n@@ -1639,7 +1639,7 @@ gc.writeCommitGraph::\n \tIf true, then gc will rewrite the commit-graph file when\n \tlinkgit:git-gc[1] is run. When using linkgit:git-gc[1]\n \t'--auto' the commit-graph will be updated if housekeeping is\n-\trequired. Default is false. See linkgit:git-commit-graph[1]\n+\trequired. Default is true. See linkgit:git-commit-graph[1]\n \tfor details.\n \n gc.logExpiry::\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 871a56f1c5..77e7413f94 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -41,7 +41,7 @@ static int aggressive_depth = 50;\n static int aggressive_window = 250;\n static int gc_auto_threshold = 6700;\n static int gc_auto_pack_limit = 50;\n-static int gc_write_commit_graph;\n+static int gc_write_commit_graph = 1;\n static int detach_auto = 1;\n static timestamp_t gc_log_expire_time;\n static const char *gc_log_expire = \"1.day.ago\";\ndiff --git a/commit-graph.c b/commit-graph.c\nindex a686758603..a459272466 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -232,15 +232,15 @@ static int prepare_commit_graph(struct repository *r)\n {\n \tstruct alternate_object_database *alt;\n \tchar *obj_dir;\n-\tint config_value;\n+\tint config_value = 1;\n \n \tif (r->objects->commit_graph_attempted)\n \t\treturn !!r->objects->commit_graph;\n \tr->objects->commit_graph_attempted = 1;\n \n+\trepo_config_get_bool(r, \"core.commitgraph\", &config_value);\n \tif (!git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&\n-\t    (repo_config_get_bool(r, \"core.commitgraph\", &config_value) ||\n-\t    !config_value))\n+\t    !config_value)\n \t\t/*\n \t\t * This repository is not configured to use commit graphs, so\n \t\t * do not load one. (But report commit_graph_attempted anyway\n-- \ngitgitgadget\n"},{"id":"360811","messageId":"xmqqlg6vvrur.fsf@gitster-ct.c.googlers.com","threadId":"49600","inReplyTo":"pull.50.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Use commit-graph by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T03:47:08Z","receivedAt":"2018-10-18T03:47:13Z","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> The commit-graph feature is starting to stabilize. Based on what is in\n> master right now, we have:\n>\n> Git 2.18:\n>\n>  * Ability to write commit-graph (requires user interaction).\n>    \n>    \n>  * Commit parsing is faster when commit-graph exists.\n>    \n>    \n>  * Must have core.commitGraph true to use.\n>    \n>    \n>\n> Git 2.19:\n>\n>  * Ability to write commit-graph on GC with gc.writeCommitGraph.\n>    \n>    \n>  * Generation numbers written in commit-graph\n>    \n>    \n>  * A few reachability algorithms make use of generation numbers.\n>    \n>    \n>\n> (queued for) master:\n>\n>  * The test suite passes with GIT_TEST_COMMIT_GRAPH=1\n>    \n>    \n>  * 'git commit-graph write' has progress indicators.\n>    \n>    \n>  * The commit-graph is automatically disabled when grafts or replace-objects\n>    exist.\n\nIf I recall correctly, one more task that was discussed but hasn't\nbeen addressed well is how the generation and incremental update of\nit should integrate with the normal repository maintenance workflow\n(perhaps \"gc --auto\").  If we are going to turn it on by default, it\nwould be good to see if we can avoid multiple independent walks done\nover the same history graph by repack, prune and now commit-graph,\nbefore such a change happens.\n\nThanks.\n\n\n\n"},{"id":"360815","messageId":"xmqq1s8nvndm.fsf@gitster-ct.c.googlers.com","threadId":"49600","inReplyTo":"ab118e60e9988076edf43d0ed8c1924e6569a9d0.1539808389.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t6501: use --quiet when testing gc stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T05:23:49Z","receivedAt":"2018-10-18T05:23:54Z","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> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> The test script t6501-freshen-objects.sh has some tests that care\n> if 'git gc' has any output to stderr. This is intended to say that\n> no warnings occurred related to broken links. However, when we\n> have operations that output progress (like writing the commit-graph)\n> this causes the test to fail.\n\nI see that the descriptor #2 is redirected into a regular file.  Why\nshould we be writing the progress indicator in that case in the\nfirst place?  Shoudln't we be doing the usual \"are we showing this\nto an interactive terminal?\" test?\n"},{"id":"360845","messageId":"90d051a5-8c02-518d-01a7-16746061e6b9@gmail.com","threadId":"49600","inReplyTo":"xmqq1s8nvndm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] t6501: use --quiet when testing gc stderr","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-10-18T12:59:32Z","receivedAt":"2018-10-18T12:59:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\n\nOn 10/18/2018 1:23 AM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> The test script t6501-freshen-objects.sh has some tests that care\n>> if 'git gc' has any output to stderr. This is intended to say that\n>> no warnings occurred related to broken links. However, when we\n>> have operations that output progress (like writing the commit-graph)\n>> this causes the test to fail.\n> I see that the descriptor #2 is redirected into a regular file.  Why\n> should we be writing the progress indicator in that case in the\n> first place?  Shoudln't we be doing the usual \"are we showing this\n> to an interactive terminal?\" test?\n\nThis code from builtin/gc.c makes it look like we are doing that:\n\n         if (gc_write_commit_graph)\n                 write_commit_graph_reachable(get_object_directory(), 0,\n                                              !quiet && !daemonized);\n\nBut really, daemonized is only for when running in the background.\n\nDo you have an example of a builtin that checks this interactive \nterminal behavior?\n\nThanks,\n-Stolee\n"},{"id":"360846","messageId":"daf46aa4-1eb5-86c2-c7c2-9198ed6473ce@gmail.com","threadId":"49600","inReplyTo":"xmqqlg6vvrur.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/3] Use commit-graph by default","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-10-18T13:01:21Z","receivedAt":"2018-10-18T13:01:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/17/2018 11:47 PM, Junio C Hamano wrote:\n> If I recall correctly, one more task that was discussed but hasn't\n> been addressed well is how the generation and incremental update of\n> it should integrate with the normal repository maintenance workflow\n> (perhaps \"gc --auto\").  If we are going to turn it on by default, it\n> would be good to see if we can avoid multiple independent walks done\n> over the same history graph by repack, prune and now commit-graph,\n> before such a change happens.\nI don't remember a discussion on this, but it is an interesting point. \nI'll give it some thought to see if we can combine these walks.\n\nThanks,\n-Stolee\n"},{"id":"360888","messageId":"xmqqk1me4vrw.fsf@gitster-ct.c.googlers.com","threadId":"49600","inReplyTo":"90d051a5-8c02-518d-01a7-16746061e6b9@gmail.com","subject":"Re: [PATCH 1/3] t6501: use --quiet when testing gc stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-19T00:36:51Z","receivedAt":"2018-10-19T00:36:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> This code from builtin/gc.c makes it look like we are doing that:\n>\n>         if (gc_write_commit_graph)\n>                 write_commit_graph_reachable(get_object_directory(), 0,\n>                                              !quiet && !daemonized);\n>\n> But really, daemonized is only for when running in the background.\n\nBut that is something we can easily fix, no?\n\n\"git grep isatty\" tells you that we use isatty(2) to decide if we\nwant to go verbose or show progress.  Another frequent use of isatty\nin our codebase of course is to use of a pager and columnar output\nformats.\n"}]}