maintenance: core.commitGraph=false prevents writes
PATCH 1/1 maintenance: core.commitGraph=false prevents writes
From: Derrick Stolee <[email protected]>
Recently, a user had an issue due to combining fetch.writeCommitGraph=true with core.commitGraph=false. The root bug has been resolved by preventing commit-graph writes when core.commitGraph is disabled. This happens inside the 'git commit-graph write' command, but we can be more aware of this situation and prevent that process from ever starting in the 'commit-graph' maintenance task.
---
maintenance: core.commitGraph=false prevents writes
As requested [1], this prevents the extra process when core.commitGraph is disabled.
This is based on ds/maintenance-commit-graph-auto-fix.
[1] https://lore.kernel.org/git/[email protected]/
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-749%2Fderrickstolee%2Fmaintenance-core-commit-graph-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-749/derrickstolee/maintenance-core-commit-graph-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/749
builtin/gc.c | 4 ++++ t/t7900-maintenance.sh | 8 ++++++++ 2 files changed, 12 insertions(+)
builtin/gc.c +4 -0
@@ -813,6 +813,10 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)static int maintenance_task_commit_graph (struct maintenance_run_opts *opts) { +prepare_repo_settings (the_repository); +if (!the_repository->settings.core_commit_graph) +return 0 ; +close_object_store (the_repository->objects);if (run_write_commit_graph (opts)) {error (_ ("failed to write commit-graph" ));
t/t7900-maintenance.sh +8 -0
@@ -52,6 +52,14 @@ test_expect_success 'run --task=<task>' ' test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt ' +test_expect_success 'core.commitGraph=false prevents write process' ' + GIT_TRACE2_EVENT="$(pwd)/no-commit-graph.txt" \ + git -c core.commitGraph=false maintenance run \ + --task=commit-graph 2>/dev/null && + test_subcommand ! git commit-graph write --split --reachable --no-progress \ + <no-commit-graph.txt +' + test_expect_success 'commit-graph auto condition' ' COMMAND="maintenance run --task=commit-graph --auto --quiet" &&
base-commit: 8f801804befa12a9c4ddff91275cf03612f1895d
Signature · 1 line
gitgitgadget
Quoted 9 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
Recently, a user had an issue due to combining fetch.writeCommitGraph=true with core.commitGraph=false. The root bug has been resolved by preventing commit-graph writes when core.commitGraph is disabled. This happens inside the 'git commit-graph write' command, but we can be more aware of this situation and prevent that process from ever starting in the 'commit-graph' maintenance task.
Quoted 6 lines
Signed-off-by Derrick Stolee <[email protected]>---
maintenance: core.commitGraph=false prevents writesAs requested [1], this prevents the extra process when core.commitGraph is disabled.
That's not a request. I was just wondering aloud.
If you took inspiration from my thinking aloud, that is wonderful, but the actual work to ensure it is not an idea that horribly breaks some underlying assumption I didn't know about in the code and deciding it is a good idea to do so is all done by you, so please take the credit due.
Quoted 5 lines
This is based on ds/maintenance-commit-graph-auto-fix.
[1] https://lore.kernel.org/git/[email protected]/
Thanks, -Stolee
Hmph.
There is a call to prepare_repo_settings() in cmd_gc().
I have to wonder if it should be done much earlier and in a more central place, perhaps in cmd_maintenance() before anything else happens. Even though commit-graph may feel somewhat special only because it is relatively new, it is not hard to imagine that other maintenance tasks (both older ones and future ones) would eventually want to have similar access to the feature settings.
It is OK to keep "the maintenance command works only in the single repository", and not passing a "repo" that cmd_maintenance() would prepare by calling prepare_repo_settings() down in the callchain, at least right now, but we might want to consider doing so in the future.
Thanks.
Quoted 44 lines
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-749%2Fderrickstolee%2Fmaintenance-core-commit-graph-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-749/derrickstolee/maintenance-core-commit-graph-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/749builtin/gc.c | 4 ++++ t/t7900-maintenance.sh | 8 ++++++++ 2 files changed, 12 insertions(+)builtin/gc.c +4
-0@@ -813,6 +813,10 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)static int maintenance_task_commit_graph (struct maintenance_run_opts *opts) { +prepare_repo_settings (the_repository); +if (!the_repository->settings.core_commit_graph) +return 0 ; +close_object_store (the_repository->objects);if (run_write_commit_graph (opts)) {error (_ ("failed to write commit-graph" ));t/t7900-maintenance.sh +8
-0@@ -52,6 +52,14 @@ test_expect_success 'run --task=<task>' ' test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt ' +test_expect_success 'core.commitGraph=false prevents write process' ' + GIT_TRACE2_EVENT="$(pwd)/no-commit-graph.txt" \ + git -c core.commitGraph=false maintenance run \ + --task=commit-graph 2>/dev/null && + test_subcommand ! git commit-graph write --split --reachable --no-progress \ + <no-commit-graph.txt +' + test_expect_success 'commit-graph auto condition' ' COMMAND="maintenance run --task=commit-graph --auto --quiet" &&base-commit:
8f801804befa12a9c4ddff91275cf03612f1895d
Quoted 18 lines · Junio C Hamano
Quoted 9 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
Recently, a user had an issue due to combining fetch.writeCommitGraph=true with core.commitGraph=false. The root bug has been resolved by preventing commit-graph writes when core.commitGraph is disabled. This happens inside the 'git commit-graph write' command, but we can be more aware of this situation and prevent that process from ever starting in the 'commit-graph' maintenance task.
Quoted 6 lines
Signed-off-by Derrick Stolee <[email protected]>---
maintenance: core.commitGraph=false prevents writesAs requested [1], this prevents the extra process when core.commitGraph is disabled.
Quoted 7 lines
That's not a request. I was just wondering aloud.
If you took inspiration from my thinking aloud, that is wonderful, but the actual work to ensure it is not an idea that horribly breaks some underlying assumption I didn't know about in the code and deciding it is a good idea to do so is all done by you, so please take the credit due.
Ok, I saw your comment and I thought "no harm in dropping an extra process." The patch to no-op the write does more work than this one, and the commit-graph maintenance task would automatically stop writing the file but will output a warning.
Quoted 8 lines
There is a call to
prepare_repo_settings()incmd_gc().I have to wonder if it should be done much earlier and in a more central place, perhaps in
cmd_maintenance()before anything else happens. Even though commit-graph may feel somewhat special only because it is relatively new, it is not hard to imagine that other maintenance tasks (both older ones and future ones) would eventually want to have similar access to the feature settings.
This "prepare_" pattern is like using "prepare_packed_git()" before iterating on the packed_git list. We use prepare_repo_settings() to ensure they are loaded before we use the settings. If the settings are already loaded, then prepare_repo_settings() exits quickly.
Perhaps it is worth claiming a region of code requiring that the settings are initialized before calling, but that may lead to issues in the future that I'd like to avoid. Having a few extra calls to prepare_repo_settings() is the right trade-off in my opinion.
Quoted 5 lines
It is OK to keep "the maintenance command works only in the single repository", and not passing a "repo" that
cmd_maintenance()would prepare by callingprepare_repo_settings()down in the callchain, at least right now, but we might want to consider doing so in the future.
Removing the use of the_repository is a worthwhile discussion to save for another day.
Thanks,
-Stolee