Re: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks

[Junio C Hamano <gitster@pobox.com>]

Calvin Wan <calvinwan@google.com> writes:

> Since the gc task is partially safe, there are two new tasks -- an async
> safe gc task and an async unsafe gc task. In order to properly invoke
> this in gc, `--run-async-safe` and `--run-async-unsafe` have been added
> as options to gc. Maintenance will only run these two new tasks if it
> was set to detach, otherwise the original gc task runs.

Would it essentially boil down to ensure that only one "maintenance"
is running at a time, and when it is running the "unsafe" part,
somehow the end-user MUST be made aware of that fact and told to
refrain from touching the repository?

> Additionally, if a user passes in tasks thru `--task`, we do not attempt
> to run separate async/sync tasks since the user sets the order of tasks.

In other words, the rope is long enough that the user can do
whatever they want, regardless of what we think the order should be.

Which probably makes sense.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index d52735354c..375d304c42 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -668,6 +668,8 @@ struct repository *repo UNUSED)
>  	pid_t pid;
>  	int daemonized = 0;
>  	int keep_largest_pack = -1;
> +	int run_async_safe = 0;
> +	int run_async_unsafe = 0;
>  	timestamp_t dummy;
>  	struct child_process rerere_cmd = CHILD_PROCESS_INIT;
>  	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
> @@ -694,6 +696,10 @@ struct repository *repo UNUSED)
>  			   PARSE_OPT_NOCOMPLETE),
>  		OPT_BOOL(0, "keep-largest-pack", &keep_largest_pack,
>  			 N_("repack all other packs except the largest pack")),
> +		OPT_BOOL(0, "run-async-safe", &run_async_safe,
> +			 N_("run only background safe gc tasks, should only be invoked thru maintenance")),
> +		OPT_BOOL(0, "run-async-unsafe", &run_async_unsafe,
> +			 N_("run only background unsafe gc tasks, should only be invoked thru maintenance")),
>  		OPT_END()
>  	};
>  
> @@ -718,6 +724,9 @@ struct repository *repo UNUSED)
>  			     builtin_gc_usage, 0);
>  	if (argc > 0)
>  		usage_with_options(builtin_gc_usage, builtin_gc_options);
> +	
> +	if (run_async_safe && run_async_unsafe)
> +		die(_("--run-async-safe cannot be used with --run-async-unsafe"));

So if the caller wants to eventually run both, it has to spawn this
program twice, the first time with one option and the second time
with the other option?  Somehow it feels a bit unsatisfying.  If the
caller says "I want to run both classes", and if your safe/unsafe
classification system knows which task belongs to which class and
the classification system that unsafe ones should be run before safe
ones (or whatever), wouldn't it be easier to use for the caller to
be able to say "run both", and let your classification system take
care of the ordering?