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

[Stefan Beller <sbeller@google.com>]

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:
>> 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.

They were slightly less obvious to me, fixed now as well!

>
>>         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.

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.

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