On Thu, Jul 23, 2020 at 01:21:55PM -0700, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > +--[no-]maintenance::
> > --[no-]auto-gc::
> > - Run `git gc --auto` at the end to perform garbage collection
> > - if needed. This is enabled by default.
> > + Run `git maintenance run --auto` at the end to perform garbage
> > + collection if needed. This is enabled by default.
>
> Shouldn't the new synonym be called --auto-maintenance or an
> abbreviation thereof? It is not like we will run the full
> maintenance suite when "--no-maintenance" is omitted, which
> certainly is not the impression we want to give our readers.
>
> > These objects may be removed by normal Git operations (such as `git commit`)
> > -which automatically call `git gc --auto`. (See linkgit:git-gc[1].)
> > -If these objects are removed and were referenced by the cloned repository,
> > -then the cloned repository will become corrupt.
> > +which automatically call `git maintenance run --auto` and `git gc --auto`.
>
> Hmph. Perhaps the picture may change in the end of the series but I
> got an impression that "gc --auto" would eventually become just part
> of "maintenance --auto" and the users won't have to be even aware of
> its existence? Wouldn't we rather want to say something like
>
> --[no-]auto-maintenance::
> --[no-]auto-gc::
> Run `git maintenance run --auto` at the end to perform
> garbage collection if needed (`--[no-]auto-gc` is a
> synonym). This is enabled by default.
>
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index 82ac4be8a5..49a4d727d4 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -196,8 +196,10 @@ static struct option builtin_fetch_options[] = {
> > OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
> > N_("report that we have only objects reachable from this object")),
> > OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
>
> > + OPT_BOOL(0, "maintenance", &enable_auto_gc,
> > + N_("run 'maintenance --auto' after fetching")),
> > OPT_BOOL(0, "auto-gc", &enable_auto_gc,
> > + N_("run 'maintenance --auto' after fetching")),
>
> OK, so this is truly a backward-compatible synonym at this point.
I wouldn't be opposed to making the 'auto-gc' option an
'OPT_HIDDEN_BOOL', but I realize that users may not want to move as
quickly as that. Perhaps we should visit this in a couple of releases
(or perhaps you are getting to it in a later patch that I haven't read
yet).
> > diff --git a/run-command.c b/run-command.c
> > index 9b3a57d1e3..82ad241638 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -1865,14 +1865,17 @@ int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
> > return result;
> > }
> >
> > -int run_auto_gc(int quiet)
> > +int run_auto_maintenance(int quiet)
> > {
> > struct argv_array argv_gc_auto = ARGV_ARRAY_INIT;
> > int status;
> >
> > - argv_array_pushl(&argv_gc_auto, "gc", "--auto", NULL);
> > + argv_array_pushl(&argv_gc_auto, "maintenance", "run", "--auto", NULL);
> > if (quiet)
> > argv_array_push(&argv_gc_auto, "--quiet");
> > + else
> > + argv_array_push(&argv_gc_auto, "--no-quiet");
> > +
> > status = run_command_v_opt(argv_gc_auto.argv, RUN_GIT_CMD);
> > argv_array_clear(&argv_gc_auto);
> > return status;
>
> Don't we want to replace all _gc_ with _maintenance_ in this
> function? I think the first business before we can do so would be
> to rethink if spelling out "maintenance" fully in code is a good
> idea in the first space. It would make names for variables,
> structures and fields unnecessarily long without contributing to
> ease of understanding an iota, and a easy-to-remember short-form or
> an abbreviation may be needed. Using a short-form/abbreviation
> wouldn't worsen the end-user experience, and not the developer
> experience for that matter.
>
> If we choose "gc" as the short-hand, most of the change in this step
> would become unnecessary. I also do not mind if we some other words
> or word-fragment (perhaps "maint"???) is chosen.
Yeah, writing out 'maintenance' every time in the code and in
command-line arguments is kind of a mouthful. I'm more willing to accept
that '--maintenance' is something that users would write or script
around, but 'maint' makes sense to me as a shorthand in the code.
I could go either way on calling the command-line flag '--maint',
though.
>
> > diff --git a/run-command.h b/run-command.h
> > index 191dfcdafe..d9a800e700 100644
> > --- a/run-command.h
> > +++ b/run-command.h
> > @@ -221,7 +221,7 @@ int run_hook_ve(const char *const *env, const char *name, va_list args);
> > /*
> > * Trigger an auto-gc
> > */
> > -int run_auto_gc(int quiet);
> > +int run_auto_maintenance(int quiet);
> >
> > #define RUN_COMMAND_NO_STDIN 1
> > #define RUN_GIT_CMD 2 /*If this is to be git sub-command */
> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> > index a66dbe0bde..9850ecde5d 100755
> > --- a/t/t5510-fetch.sh
> > +++ b/t/t5510-fetch.sh
> > @@ -919,7 +919,7 @@ test_expect_success 'fetching with auto-gc does not lock up' '
> > git config fetch.unpackLimit 1 &&
> > git config gc.autoPackLimit 1 &&
> > git config gc.autoDetach false &&
> > - GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
> > + GIT_ASK_YESNO="$D/askyesno" git fetch --verbose >fetch.out 2>&1 &&
> > test_i18ngrep "Auto packing the repository" fetch.out &&
> > ! grep "Should I try again" fetch.out
> > )
Thanks,
Taylor
