From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 14 Sep 2016 21:46:56 GMT
Subject: Re: [PATCH v4 3/4] read-cache: introduce chmod_index_entry
Message-ID: <xmqqbmzqqhm7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160914210747.15485-4-t.gummerer@gmail.com>

Thomas Gummerer <t.gummerer@gmail.com> writes:

> As there are chmod options for both add and update-index, introduce a
> new chmod_index_entry function to do the work.  Use it in update-index,
> while it will be used in add in the next patch.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  builtin/update-index.c | 16 ++--------------
>  cache.h                |  2 ++
>  read-cache.c           | 29 +++++++++++++++++++++++++++++
>  3 files changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index bbdf0d9..9e9e040 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -423,26 +423,14 @@ static void chmod_path(char flip, const char *path)
>  {
> ...
> -	mode = ce->ce_mode;
> -	if (!S_ISREG(mode))
> -		goto fail;
> -	switch (flip) {
> -	case '+':
> -		ce->ce_mode |= 0111; break;
> -	case '-':
> -		ce->ce_mode &= ~0111; break;
> -	default:
> +	if (chmod_cache_entry(ce, flip) < 0)
>  		goto fail;
> -	}
> -	cache_tree_invalidate_path(&the_index, path);

This used to always work on the default index, hence the_index
reference is here, but ...

> +int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
> +		      char flip)
> +{
> +	if (!S_ISREG(ce->ce_mode))
> +		return -1;
> +	switch (flip) {
> +	case '+':
> +		ce->ce_mode |= 0111;
> +		break;
> +	case '-':
> +		ce->ce_mode &= ~0111;
> +		break;
> +	default:
> +		return -2;
> +	}
> +	cache_tree_invalidate_path(&the_index, ce->name);

... this one takes istate, so you need to use it, instead of the
hard-coded the_index reference.

> +	ce->ce_flags |= CE_UPDATE_IN_BASE;
> +	istate->cache_changed |= CE_ENTRY_CHANGED;
> +
> +	return 0;
> +}
> +
>  int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
>  {
>  	int len = ce_namelen(a);

Other than that, this looks good to me.