On 4/30/2025 4:28 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> These patches add a new --no-maintenance option to the scalar register and >> scalar clone commands. My motivation is based on setting up Scalar clones in >> automated environments that set up a repo onto a disk image for use later. >> If background maintenance runs during later setup steps, then this >> introduces a variable that is unexpected at minimum and disruptive at worst. >> The disruption comes in if the automation has steps to run git maintenance >> run --task=<X> commands but those commands are blocked due to the >> maintenance.lock file. >> >> Functionally, these leave the default behavior as-is but allow disabling the >> git maintenance start step when users opt-in to this difference. The idea of >> Scalar is to recommend the best practices for a typical user, but allowing >> customization for expert users. > > The feature itself I do not have any objections to and I found the > reasoning given above very sound. > > With these two patches, we still have an unconditional call to > toggle_maintenance(1) in cmd_reconfigure(). Shoudln't the call be > at least removed, which would mean that reconfiguring would not > change the auto-maintenance states, or made controllable from the > command line of "maintenance reconfigure"? I agree that this was a miss on my part. Thanks for the careful eye. > It somehow looks to me that the real culprit of making the result of > applying two patches still unsatisfactory is the original design to > have the toggle_maintenance() call made from inside register_dir() > in the first place. Shouldn't a much higher layer entry points like > cmd_register() and cmd_clone() be where the decision is made if > maintenance task should be set up (or not set up) by calling > toggle_maintenance(), leaving the register_dir() responsible only > for "enlist the directory to the system"? > > IOW it feels to me that enabling (and now optionally disabling) > maintenance is tied too deeply into the act of enlisting a > directory; if we need to disable maintenance (and a mode to add > enlistment without enabling maintenance), it is a sign that it > shouldn't be a parameter into the register_dir() function that > controls what register_dir() does, and rather it should be done by > letting the caller who calls register_dir() decide to call (or not) > toggle_maintenance(). I will continue thinking about this while playing with different options for a v2. My gut reaction is that register_dir() is our centralized way to "set up recommended configuration" which includes config options and background maintenance. We're now introducing a way to customize this centralized operation based on decentralized options, hence a new option to the method. Is the right solution to move the toggle_maintenance() out of register_dir()? If this is the only way we plan to customize the config, then yes. Otherwise, the second or third customization will start to lead to copied logic through these three locations. Again, I'm mostly thinking out loud here before I go and dig into the code. I'll report back with v2. Thanks, -Stolee
