On 10/12/2020 1:30 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx> >> >> 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. >> >> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> >> --- >> maintenance: core.commitGraph=false prevents writes >> >> As 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. 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. > 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. 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. > 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. Removing the use of the_repository is a worthwhile discussion to save for another day. Thanks, -Stolee
