Re: [PATCH v7 09/11] config: add core.untrackedCache

[Christian Couder <christian.couder@gmail.com>]

On Mon, Jan 25, 2016 at 9:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> diff --git a/read-cache.c b/read-cache.c
>> index 5be7cd1..a04ec8c 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1497,10 +1497,23 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,
>>       return ce;
>>  }
>>
>> -static void check_ce_order(struct index_state *istate)
>> +static void post_read_index_from(struct index_state *istate)
>>  {
>>       unsigned int i;
>>
>> +     switch (git_config_get_untracked_cache()) {
>> +     case -1: /* keep: do nothing */
>> +             break;
>> +     case 0: /* false */
>> +             remove_untracked_cache(istate);
>> +             break;
>> +     case 1: /* true */
>> +             add_untracked_cache(istate);
>> +             break;
>> +     default: /* unknown value: do nothing */
>> +             break;
>> +     }
>> +
>>       for (i = 1; i < istate->cache_nr; i++) {
>>               struct cache_entry *ce = istate->cache[i - 1];
>>               struct cache_entry *next_ce = istate->cache[i];
>
> Bad manners.
>
>  * The new code added to an existing function, unless there is a
>    good reason, goes to the bottom.  In this case, the verification
>    of the ordering of cache entries and tweaking of UC extension are
>    two unrelated things that can be independently done, and there is
>    no justification why the new code has to come to top.
>
>  * The old function name served as a good documentation of what it
>    does.  That is no longer the case.  Each unrelated segment of
>    this new function needs to be commented.  Even better, perhaps
>    leave the original check_ce_order() as-is, introduce a new
>    function tweak_uc_extension(), and make the post_read_index()
>    to be just two-liner function:
>
>         static void post_read_index(struct index_state *istate)
>         {
>                 check_ce_order(istate);
>                 tweak_uc_extension(istate);
>         }
>
>    That way the documentation value of each function that does one
>    specific thing and named specific to its task will be kept, and
>    there is no need for extra comments.

Ok, I will do that, thanks.