Re: [PATCH v2 05/18] maintenance: add commit-graph task

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2020, 19:47 UTC
Message-ID: <xmqqy2n8lmq3.fsf@gitster.c.googlers.com>
In-Reply-To: <b0bc5d89-52fa-e0f4-e3af-5eb6c76d312e@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 19 quoted lines

> But you are discussing here how the _behavior_ can change when
> --auto is specified. And specifically, "git gc --auto" really
> meant "This is running after a foreground command, so only do
> work if necessary and do it quickly to minimize blocking time."
>
> I'd be happy to replace "--auto" with "--quick" in the
> maintenance builtin.
>
> This opens up some extra design space for how the individual
> tasks perform depending on "--quick" being specified or not.
> My intention was to create tasks that are already in "quick"
> mode:
>
> * loose-objects have a maximum batch size.
> * incremental-repack is capped in size.
> * commit-graph uses the --split option.
>
> But this "quick" distinction might be important for some of
> the tasks we intend to extract from the gc builtin.

Yup. To be honest, I came to this topic from a completely different direction. The field name "auto" alone (and no other field name) had to have an extra cruft (i.e. "_flag") attached to it, which is understandable but ugly. Then I started thinking if 'auto(matic)' is really the right word to describe what we want out of the option, and came to the realization that there may be better words.

> Since the tasks are frequently running subcommands, returning
> 0 for success and non-zero for error matches the error codes
> returned by those subcommands.

As long as these will _never_ be called from other helper functions but from the cmd_foo() top-level and their return values are only used directly as the top-level's return value, I do not mind too much.

But whenever I am writing such a code, I find myself not brave enough to make such a bold promise (I saw other people call the helpers I wrote in unintended ways and had to adjust the semantics of them to accomodate the new callers too many times), so I'd rather see the caller do "return !!helper_fn()" to allow helper_fn() to be written more naturally (e.g. letting them return error(...)).

Thanks.