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:
>> 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@xxxxxxxxxx>
>> ---
>> 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
--
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