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

[Junio C Hamano <gitster@pobox.com>]

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.