Re: [PATCH v7 7/7] diff-lib: parallelize run_diff_files for submodules

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 7, 2023, 23:06 UTC
Message-ID: <230208.86ilgd0yej.gmgdl@evledraar.gmail.com>
In-Reply-To: <20230207181706.363453-8-calvinwan@google.com>

On Tue, Feb 07 2023, Calvin Wan wrote:

Show 13 quoted lines

> [...]
> +	sps->result = 1;
> +	strbuf_addf(err,
> +	    _(status_porcelain_start_error),
> +	    task->path);
> +	return 0;
> [...]
> +	if (retvalue) {
> +		sps->result = 1;
> +		strbuf_addf(err,
> +		    _(status_porcelain_fail_error),
> +		    task->path);
> [...]

This is nitpicky, but what's with the short lines and over-wrapping?

If you change these two to (just using my macro version on top, but it's the same with yours):

	strbuf_addf(err, _(STATUS_PORCELAIN_START_ERROR), task->path);

And:

	strbuf_addf(err, _(STATUS_PORCELAIN_FAIL_ERROR), task->path);

Both of these are under our usual line limit at their respective indentation (the latter at 77, rule of thumb is to wrap at 79-80).

> +	if (submodules.nr > 0) {

Don't compare unsigned to >0, just use "submodules.nr".

> +		int parallel_jobs;

nit: add extra \n, or maybe just call this "int v", as it's clear from the scope what it's about...

Show 6 quoted lines

> +		if (git_config_get_int("submodule.diffjobs", &parallel_jobs))
> +			parallel_jobs = 1;
> +		else if (!parallel_jobs)
> +			parallel_jobs = online_cpus();
> +		else if (parallel_jobs < 0)
> +			die(_("submodule.diffjobs cannot be negative"));

Can't you use the "ulong" instead of "int" and have it handle this "is negative?" error check for you?

> +
> +		if (get_submodules_status(&submodules, parallel_jobs))
> +			die(_("submodule status failed"));
> +		for (size_t i = 0; i < submodules.nr; i++) {

Another case that can use for_each_string_list_item().

Show 9 quoted lines

> +struct submodule_parallel_status {
> +	size_t index_count;
> +	int result;
> +
> +	struct string_list *submodule_names;
> +
> +	/* Pending statuses by OIDs */
> +	struct status_task **oid_status_tasks;
> +	int oid_status_tasks_nr, oid_status_tasks_alloc;

For new structs, let's use size_t, not "int" for alloc/nr.

Also, as this is 7/7 and we're not adding another such pattern for the forseeable future, can we just call these "size_t nr", "size_t alloc" and "tasks"?

And having said all that, it turns out this is just dead code that can be removed? Blindly copied from submodule_parallel_fetch?