{"thread":{"id":"54397","subject":"[PATCH] maintenance: core.commitGraph=false prevents writes","startedAt":"2020-10-12T13:28:42Z","lastAt":"2020-10-12T18:40:18Z","messageCount":3,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"407334","messageId":"pull.749.git.1602509314545.gitgitgadget@gmail.com","threadId":"54397","inReplyTo":null,"subject":"[PATCH] maintenance: core.commitGraph=false prevents writes","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-12T13:28:34Z","receivedAt":"2020-10-12T13:28:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nRecently, a user had an issue due to combining\nfetch.writeCommitGraph=true with core.commitGraph=false. The root bug\nhas been resolved by preventing commit-graph writes when\ncore.commitGraph is disabled. This happens inside the 'git commit-graph\nwrite' command, but we can be more aware of this situation and prevent\nthat process from ever starting in the 'commit-graph' maintenance task.\n\nSigned-off-by: Derrick Stolee \n---\n maintenance: core.commitGraph=false prevents writes\n \n As requested [1], this prevents the extra process when core.commitGraph\n is disabled.\n \n This is based on ds/maintenance-commit-graph-auto-fix.\n \n [1] https://lore.kernel.org/git/xmqqft6nrtlw.fsf@gitster.c.googlers.com/\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-749%2Fderrickstolee%2Fmaintenance-core-commit-graph-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-749/derrickstolee/maintenance-core-commit-graph-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/749\n\n builtin/gc.c | 4 ++++\n t/t7900-maintenance.sh | 8 ++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 12ddb68bba..e80331c4e2 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -813,6 +813,10 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)\n \n static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)\n {\n+\tprepare_repo_settings(the_repository);\n+\tif (!the_repository->settings.core_commit_graph)\n+\t\treturn 0;\n+\n \tclose_object_store(the_repository->objects);\n \tif (run_write_commit_graph(opts)) {\n \t\terror(_(\"failed to write commit-graph\"));\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex ee1f4a7ae4..9776154a2a 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -52,6 +52,14 @@ test_expect_success 'run --task=' '\n \ttest_subcommand git commit-graph write --split --reachable --no-progress /dev/null &&\n+\ttest_subcommand ! git commit-graph write --split --reachable --no-progress \\\n+\t\t writes:\n\n> From: Derrick Stolee \n>\n> Recently, a user had an issue due to combining\n> fetch.writeCommitGraph=true with core.commitGraph=false. The root bug\n> has been resolved by preventing commit-graph writes when\n> core.commitGraph is disabled. This happens inside the 'git commit-graph\n> write' command, but we can be more aware of this situation and prevent\n> that process from ever starting in the 'commit-graph' maintenance task.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> maintenance: core.commitGraph=false prevents writes\n> \n> As requested [1], this prevents the extra process when core.commitGraph\n> is disabled.\n\nThat's not a request. I was just wondering aloud.\n\nIf you took inspiration from my thinking aloud, that is wonderful,\nbut the actual work to ensure it is not an idea that horribly breaks\nsome underlying assumption I didn't know about in the code and\ndeciding it is a good idea to do so is all done by you, so please\ntake the credit due.\n\n> This is based on ds/maintenance-commit-graph-auto-fix.\n> \n> [1] https://lore.kernel.org/git/xmqqft6nrtlw.fsf@gitster.c.googlers.com/\n> \n> Thanks, -Stolee\n\nHmph. \n\nThere is a call to prepare_repo_settings() in cmd_gc().\n\nI have to wonder if it should be done much earlier and in a more\ncentral place, perhaps in cmd_maintenance() before anything else\nhappens. Even though commit-graph may feel somewhat special only\nbecause it is relatively new, it is not hard to imagine that other\nmaintenance tasks (both older ones and future ones) would eventually\nwant to have similar access to the feature settings.\n\nIt is OK to keep \"the maintenance command works only in the single\nrepository\", and not passing a \"repo\" that cmd_maintenance() would\nprepare by calling prepare_repo_settings() down in the callchain, at\nleast right now, but we might want to consider doing so in the\nfuture.\n\nThanks.\n\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-749%2Fderrickstolee%2Fmaintenance-core-commit-graph-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-749/derrickstolee/maintenance-core-commit-graph-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/749\n>\n> builtin/gc.c | 4 ++++\n> t/t7900-maintenance.sh | 8 ++++++++\n> 2 files changed, 12 insertions(+)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 12ddb68bba..e80331c4e2 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -813,6 +813,10 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)\n> \n> static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)\n> {\n> +\tprepare_repo_settings(the_repository);\n> +\tif (!the_repository->settings.core_commit_graph)\n> +\t\treturn 0;\n> +\n> \tclose_object_store(the_repository->objects);\n> \tif (run_write_commit_graph(opts)) {\n> \t\terror(_(\"failed to write commit-graph\"));\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index ee1f4a7ae4..9776154a2a 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -52,6 +52,14 @@ test_expect_success 'run --task=' '\n> \ttest_subcommand git commit-graph write --split --reachable --no-progress '\n> \n> +test_expect_success 'core.commitGraph=false prevents write process' '\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/no-commit-graph.txt\" \\\n> +\t\tgit -c core.commitGraph=false maintenance run \\\n> +\t\t--task=commit-graph 2>/dev/null &&\n> +\ttest_subcommand ! git commit-graph write --split --reachable --no-progress \\\n> +\t\t +'\n> +\n> test_expect_success 'commit-graph auto condition' '\n> \tCOMMAND=\"maintenance run --task=commit-graph --auto --quiet\" &&\n> \n>\n> base-commit: 8f801804befa12a9c4ddff91275cf03612f1895d\n"},{"id":"407361","messageId":"5e2b2da5-526e-efa6-a127-7aa7a8c07c88@gmail.com","threadId":"54397","inReplyTo":"xmqqwnzvpd1i.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] maintenance: core.commitGraph=false prevents writes","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-10-12T18:40:12Z","receivedAt":"2020-10-12T18:40:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/12/2020 1:30 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n>>\n>> Recently, a user had an issue due to combining\n>> fetch.writeCommitGraph=true with core.commitGraph=false. The root bug\n>> has been resolved by preventing commit-graph writes when\n>> core.commitGraph is disabled. This happens inside the 'git commit-graph\n>> write' command, but we can be more aware of this situation and prevent\n>> that process from ever starting in the 'commit-graph' maintenance task.\n>>\n>> Signed-off-by: Derrick Stolee \n>> ---\n>> maintenance: core.commitGraph=false prevents writes\n>> \n>> As requested [1], this prevents the extra process when core.commitGraph\n>> is disabled.\n> \n> That's not a request. I was just wondering aloud.\n> \n> If you took inspiration from my thinking aloud, that is wonderful,\n> but the actual work to ensure it is not an idea that horribly breaks\n> some underlying assumption I didn't know about in the code and\n> deciding it is a good idea to do so is all done by you, so please\n> take the credit due.\n\nOk, I saw your comment and I thought \"no harm in dropping an extra\nprocess.\" The patch to no-op the write does more work than this one,\nand the commit-graph maintenance task would automatically stop\nwriting the file but will output a warning.\n\n> There is a call to prepare_repo_settings() in cmd_gc().\n>\n> I have to wonder if it should be done much earlier and in a more\n> central place, perhaps in cmd_maintenance() before anything else\n> happens. Even though commit-graph may feel somewhat special only\n> because it is relatively new, it is not hard to imagine that other\n> maintenance tasks (both older ones and future ones) would eventually\n> want to have similar access to the feature settings.\n\nThis \"prepare_\" pattern is like using \"prepare_packed_git()\" before\niterating on the packed_git list. We use prepare_repo_settings() to\nensure they are loaded before we use the settings. If the settings are\nalready loaded, then prepare_repo_settings() exits quickly.\n\nPerhaps it is worth claiming a region of code requiring that the\nsettings are initialized before calling, but that may lead to issues\nin the future that I'd like to avoid. Having a few extra calls to\nprepare_repo_settings() is the right trade-off in my opinion.\n\n> It is OK to keep \"the maintenance command works only in the single\n> repository\", and not passing a \"repo\" that cmd_maintenance() would\n> prepare by calling prepare_repo_settings() down in the callchain, at\n> least right now, but we might want to consider doing so in the\n> future.\n\nRemoving the use of the_repository is a worthwhile discussion to\nsave for another day.\n\nThanks,\n-Stolee\n"}]}