Re: [PATCH 2/2] cleanup submodule_config a bit.

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 12, 2015, 21:13 UTC
Message-ID: <CAPig+cTcWME_U+nYATK7kK0vOSSCjJmGgXmUVmSawBiV5M+02g@mail.gmail.com>
In-Reply-To: <1439406838-6290-3-git-send-email-sbeller@google.com>

On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller <sbeller@google.com> wrote:

Show 21 quoted lines

> In the first hunk, `submodule` is NULL all the time, so we can make it clearer
> by directly returning NULL.
>
> In the second hunk, we can directly return the lookup values as it also makes
> the coder clearer.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule-config.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index 199692b..08e93cc 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -387,7 +387,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
>         }
>
>         if (!gitmodule_sha1_from_commit(commit_sha1, sha1))
> -               return submodule;
> +               return NULL;

There are a couple other places which return 'submodule' when it is known to be NULL. One could rightly expect that they deserve the same treatment, otherwise, the code becomes more confusing since it's not obvious why 'return NULL' is used some places but not others.

Show 18 quoted lines

>         switch (lookup_type) {
>         case lookup_name:
> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct submodule_cache *cache,
>
>         switch (lookup_type) {
>         case lookup_name:
> -               submodule = cache_lookup_name(cache, sha1, key);
> -               break;
> +               return cache_lookup_name(cache, sha1, key);
>         case lookup_path:
> -               submodule = cache_lookup_path(cache, sha1, key);
> -               break;
> +               return cache_lookup_path(cache, sha1, key);
> +       default:
> +               return NULL;
>         }
> -
> -       return submodule;

Earlier in the function, there's effectively a clone of this logic to which you could apply the same transformation. Changing it here, while ignoring the clone, makes the code more confusing (or at least inconsistent) rather than less.

Show 5 quoted lines

>  }
>
>  static const struct submodule *config_from_path(struct submodule_cache *cache,
> --
> 2.5.0.234.gefc8a62