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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Aug 12, 2015 at 5:34 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Aug 12, 2015 at 2:13 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller <sbeller@google.com> wrote:
>>>         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.
>
> They were slightly less obvious to me, fixed now as well!
>
>>> @@ -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.
>
> Not quite. Note the `if (submodule)` in the earlier version, so in case
> cache_lookup_{name, path} return NULL, we keep going. The change I
> propose is at the end of the function and we definitely return no matter
> if it is NULL or not.

Okay, cache_lookup_{name, path}() can indeed return NULL, so the same
transformation won't work. Sorry for the noise.