Re: [PATCH v2 4/4] diff-lib: parallelize run_diff_files for submodules

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Oct 11 2022, Calvin Wan wrote:

Mostly style comments, but also others.:

> +				     unsigned *dirty_submodule, int *defer_submodule_status,
> +					 int *ignore_untracked_in_submodules)

If you can think of a (much) shorter name for this new paremeter, then...

> +		if (defer_submodule_status && *defer_submodule_status) {
> +			defer = 1;
> +			*ignore_untracked_in_submodules =
> +							diffopt->flags.ignore_untracked_in_submodules;
> +		} else {
> +			*dirty_submodule = is_submodule_modified(ce->name,
> +							diffopt->flags.ignore_untracked_in_submodules);

...code like this becomes a lot less verbose and doesn't need this much
indentation...

> +			if (defer_submodule_status) {
> +				struct string_list_item *item =
> +								string_list_append(&submodules, ce->name);

And e.g. here splittng up the two:

	struct string_list_item *item;

	item = ...

Makes for easier reading IMO.

> +				struct submodule_status_util *util = xmalloc(sizeof(*util));
> +				util->changed = changed;
> +				util->dirty_submodule = 0;
> +				util->ignore_untracked = ignore_untracked_in_submodules;
> +				util->newmode = newmode;
> +				util->ce = ce;

Maybe easier to read as:

	struct submodule_status_util tmp = {
		.changed = ...,
		.dirty_submodule =...,
	};

Then a single memcpy() to copy that data over to the malloc'd region.

> +				item->util = util;

And you can also do this idiomatically as:

	string_list_append(...)->util = util;

> +				continue;

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

Maybe we want something that uses git_parse_unsigned()?

> +		}

This should be cuddled with the "else if"

> +		else if (!parallel_jobs) {
> +			/*
> +			 * TODO: Decide what a reasonable default for parallel_jobs
> +			 * is. Currently mimics what other parallel config options
> +			 * default to.
> +			 */

It's OK to just drop this comment IMO.


> +			parallel_jobs = online_cpus();
> +		}

And as these are both one statement you can drop the {}'s altogether.

I think this would probably be more idiomatic as (untested):

	if (git_config_get_int(..., &v) || !v)
		jobs = online_cpus();
	else if (v < 0) /* or some API checks it for us? */
		die(...);
	else
		jobs = v;

I.e. we'd be explicitly using the "does the key exist" return value.

> +
> +		if (get_submodules_status(revs->repo, &submodules, parallel_jobs))
> +			BUG("Submodule status failed");

s/Sub/sub/, lower-case for bug(), die() etc.

> +		for (i = 0; i < submodules.nr; i++) {

You're iterating a string_list, so that "i" earlier should be size_t,
but better yet I think you can use for_each_string_list_item() here

> +struct submodule_parallel_status {
> +	int index_count;

Here you have an int...
> +	int result;
> +
> +	struct string_list *submodule_names;

..that'll be tracking the "nr" of this, which is size_t, let's have them
match.

Also, can we remove the "*" there and just have submodule.c populate
this struct directly, maybe not worth making it public, just a
thought...

..I didn't read further than this, ran out of time..