On 5/7/25 5:46 PM, Junio C Hamano wrote:
"Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
Add a new --maintenance=<mode> option to 'scalar reconfigure' that
provides options for enabling (default), disabling, or leaving
background maintenance config as-is.
Hmph, this is a bit unexpected.
+--maintenance=<mode>::
+ By default, Scalar configures the enlistment to use Git's
+ background maintenance feature; this is the same as using the
+ `--maintenance=enable` value for this option. Use the
+ `--maintenance=disable` to remove each considered enlistment
+ from background maintenance. Use `--maitnenance=keep' to leave
+ the background maintenance configuration untouched for These
+ repositories.
If I understand it correctly, here is the only place that the users
can learn what the valid choices are, and it is not even an
enumeration. They are forced to read the entire paragraph to learn
what the choices are.
I suppose this could be fixed by changing the `<mode>` to be of the
form "[enable|disable|keep]".
+ OPT_STRING(0, "maintenance", &maintenance_str,
+ N_("<mode>"),
+ N_("signal how to adjust background maintenance")),
And there is no hint what are the right <mode> strings are.
const char * const usage[] = {
- N_("scalar reconfigure [--all | <enlistment>]"),
+ N_("scalar reconfigure [--maintenance=<mode>] [--all | <enlistment>]"),
NULL
};
So "scalar reconfigure -h" would not tell readers what the right
choices are, either.
+ if (maintenance_str) {
+ if (!strcmp(maintenance_str, "enable"))
+ maintenance = 1;
+ else if (!strcmp(maintenance_str, "disable"))
+ maintenance = 0;
+ else if (!strcmp(maintenance_str, "keep"))
+ maintenance = -1;
+ else
+ die(_("unknown mode for --maintenance option: %s"),
+ maintenance_str);
Those who say "scalar reconfigure --maintenance=yes" gets this
message that tells 'yes' is not a known mode, without saying that
they meant 'enable'.
The --opt=<mode> interface is good when we expect the vocabulary for
<mode> to grow, but I am not sure if it is warranted in this case.
Is there a strong reason why 'reconfigure' MUST enable the
maintenance by default, even if it were originally disabled in the
enlistment? If there isn't, initializing maintenance to -1 and
setting it with OPT_BOOL() would make the UI consistent with the
register and clone subcommands, and also we can lose the above block
to parse out a string. Also the code below ...
@@ -758,7 +776,8 @@ static int cmd_reconfigure(int argc, const char **argv)
the_repository = old_repo;
repo_clear(&r);
- if (toggle_maintenance(1) >= 0)
+ if (maintenance >= 0 &&
+ toggle_maintenance(maintenance) >= 0)
succeeded = 1;
... which does make perfect sense, would still be applicable.
I dunno. I just feel that 3-way "mode" interface is too much hassle
to get right (e.g., give hints to guide the users who forgot what
modes are available and how they are spelled) for this code path.
My intention was to bend over backwards to prevent a behavior change
in the default case. However, I'm coming around to understand that
we don't need this background maintenance to be redone every time
and can become a no-op by default. (Other new configuration will
still happen.)
In the case where we're fine changing the default behavior, then
the standard --[no-]maintenance option will work, though it is a
three-way switch where the lack of its existence means "don't do
either mode".
I've got a new version of this patch doing what you asked for in
the first place.
Thanks,
-Stolee
