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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 11, 2024, 09:14 UTC
Message-ID: <xmqqv7wub08c.fsf@gitster.g>
In-Reply-To: <20241108173112.1240584-2-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

Show 5 quoted lines

> 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.

Show 31 quoted lines

> 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?