Re: [PATCH 1/2] submodule.c: add has_submodules to check if we have any submodules

From: Brandon Williams <bmwill@google.com>
Date: Apr 20, 2017, 22:07 UTC
Message-ID: <20170420220736.GH142567@google.com>
In-Reply-To: <20170411194616.4963-2-sbeller@google.com>

On 04/11, Stefan Beller wrote:

Show 30 quoted lines

> +int has_submodules(unsigned what_to_check)
> +{
> +	if (what_to_check & SUBMODULE_CHECK_ANY_CONFIG) {
> +		if (submodule_config_reading == SUBMODULE_CONFIG_NOT_READ)
> +			load_submodule_config();
> +		if (submodule_config_reading == SUBMODULE_CONFIG_EXISTS)
> +			return 1;
> +	}
> +
> +	if ((what_to_check & SUBMODULE_CHECK_ABSORBED_GIT_DIRS) &&
> +	    file_exists(git_path("modules")))
> +		return 1;
> +
> +	if ((what_to_check & SUBMODULE_CHECK_GITMODULES_IN_WT) &&
> +	    (!is_bare_repository() && file_exists(".gitmodules")))
> +		return 1;
> +
> +	if (what_to_check & SUBMODULE_CHECK_GITLINKS_IN_TREE) {
> +		int i;
> +
> +		if (read_cache() < 0)
> +			die(_("index file corrupt"));
> +
> +		for (i = 0; i < active_nr; i++)
> +			if (S_ISGITLINK(active_cache[i]->ce_mode))
> +				return 1;
> +	}
> +
> +	return 0;
> +}

It may be a good idea to rearrange these by order to correctness. Correctness may not be the best way to describe it, but which is the strongest indicator that there is a submodule or that a repo 'has a submodule'. That way in the future we could have a #define that is SUBMODULE_CHECK_ANY or ALL or something....Now that I'm thinking harder about that we may not want that, and just require explicitly stating which check you want done.

Anyways good looking patch, and I like the idea of consolidating the checks into a single function.

Show 13 quoted lines

> diff --git a/submodule.h b/submodule.h
> index 8a8bc49dc9..5ec72fbb16 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -1,6 +1,12 @@
>  #ifndef SUBMODULE_H
>  #define SUBMODULE_H
>  
> +#define SUBMODULE_CHECK_ANY_CONFIG		(1<<0)
> +#define SUBMODULE_CHECK_ABSORBED_GIT_DIRS	(1<<1)
> +#define SUBMODULE_CHECK_GITMODULES_IN_WT	(1<<2)
> +#define SUBMODULE_CHECK_GITLINKS_IN_TREE 	(1<<3)
> +int has_submodules(unsigned what_to_check);

-- 
Brandon Williams