On Wed, Aug 12, 2015 at 5:34 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> On Wed, Aug 12, 2015 at 2:13 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller <sbeller@xxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html