Re: [PATCH 03/14] make is_submodule_populated gently

From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, bmwill@google.com, jrnieder@gmail.com,
	sandals@crustytoothpaste.net
Subject: Re: [PATCH 03/14] make is_submodule_populated gently
Date: Wed, 15 Feb 2017 10:10:32 -0800	[thread overview]
Message-ID: <xmqqlgt7495z.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170215003423.20245-4-sbeller@google.com> (Stefan Beller's message of "Tue, 14 Feb 2017 16:34:12 -0800")

Stefan Beller <sbeller@google.com> writes:

> We need the gentle version in a later patch. As we have just one caller,
> migrate the caller.

Ordinarily, we keep the original helper implemented as a thin
wrapper that passes NULL as retun_error_code, which causes it to
die() on error for existing callers.  But because we only have one
caller (and topics in-flight do not add new ones), we do not bother
with that.

The reasoning makes sense, at least to me.

We may want to add a comment about the behaviour upon error for the
helper function?  I see resolve_gitdir_gently() does not do so and
the readers have to follow the callflow down to read_gitfile_gently()
which does have the comment, so perhaps we are OK without any.

Looks good to me.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/grep.c | 2 +-
>  submodule.c    | 4 ++--
>  submodule.h    | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 2c727ef499..b17835aed6 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -616,7 +616,7 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
>  {
>  	if (!is_submodule_initialized(path))
>  		return 0;
> -	if (!is_submodule_populated(path)) {
> +	if (!is_submodule_populated_gently(path, NULL)) {
>  		/*
>  		 * If searching history, check for the presense of the
>  		 * submodule's gitdir before skipping the submodule.
> diff --git a/submodule.c b/submodule.c
> index 3b98766a6b..9bbdd3ce7c 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -237,12 +237,12 @@ int is_submodule_initialized(const char *path)
>  /*
>   * Determine if a submodule has been populated at a given 'path'
>   */
> -int is_submodule_populated(const char *path)
> +int is_submodule_populated_gently(const char *path, int *return_error_code)
>  {
>  	int ret = 0;
>  	char *gitdir = xstrfmt("%s/.git", path);
>  
> -	if (resolve_gitdir(gitdir))
> +	if (resolve_gitdir_gently(gitdir, return_error_code))
>  		ret = 1;
>  
>  	free(gitdir);
> diff --git a/submodule.h b/submodule.h
> index 05ab674f06..689033e538 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -41,7 +41,7 @@ extern int submodule_config(const char *var, const char *value, void *cb);
>  extern void gitmodules_config(void);
>  extern void gitmodules_config_sha1(const unsigned char *commit_sha1);
>  extern int is_submodule_initialized(const char *path);
> -extern int is_submodule_populated(const char *path);
> +extern int is_submodule_populated_gently(const char *path, int *return_error_code);
>  extern int parse_submodule_update_strategy(const char *value,
>  		struct submodule_update_strategy *dst);
>  extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);