Re: [PATCH] maintenance: core.commitGraph=false prevents writes

From: Derrick Stolee <stolee@gmail.com>
Date: Oct 12, 2020, 18:40 UTC
Message-ID: <5e2b2da5-526e-efa6-a127-7aa7a8c07c88@gmail.com>
In-Reply-To: <xmqqwnzvpd1i.fsf@gitster.c.googlers.com>

On 10/12/2020 1:30 PM, Junio C Hamano wrote:

Show 25 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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@microsoft.com>
>> ---
>>     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.

Show 8 quoted lines

> 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.

Show 5 quoted 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 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