Re: [PATCH 1/2] Check order when reading index

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2014, 17:21 UTC
Message-ID: <xmqqvbpgmqmh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1408903047-8302-1-git-send-email-jsorianopastor@gmail.com>

Jaime Soriano Pastor <jsorianopastor@gmail.com> writes:

> Subject: Re: [PATCH 1/2] Check order when reading index

Please be careful when crafting the commit title. This single line will be the only one that readers will have to identify the change among hundreds of entries in "git shortlog" output when trying to see what kind of change went into the project during the given period. Something like:

    read_index_from(): catch out of order entries while reading an index file

perhaps?

Show 14 quoted lines

> Signed-off-by: Jaime Soriano Pastor <jsorianopastor@gmail.com>
> ---
>  read-cache.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index 7f5645e..c1a9619 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1438,6 +1438,21 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,
>  	return ce;
>  }
>  
> +void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)

Does this have to be global, i.e. not "static void ..."?

Show 8 quoted lines

> +{
> +	int name_compare = strcmp(ce->name, next_ce->name);
> +	if (0 < name_compare)
> +		die("Unordered stage entries in index");
> +	if (!name_compare) {
> +		if (!ce_stage(ce))
> +			die("Multiple stage entries for merged file '%s'",
> +				ce->name);

OK. If ce is at stage #0, no other entry can have the same name regardless of the stage, and next_ce having the same name violates that rule.

> +		if (ce_stage(ce) >= ce_stage(next_ce))
> +			die("Unordered stage entries for '%s'",
> +				ce->name);

Not quite. We do allow multiple higher stage entries; having two or more stage #1 entries is perfectly fine during a merge resolution, and both ce and next_ce may be pointing at the stage #1 entries of the same path. Replacing the comparison with ">" is sufficient, I think.

Thanks.

Show 16 quoted lines

> +	}
> +}
> +
>  /* remember to discard_cache() before reading a different cache! */
>  int read_index_from(struct index_state *istate, const char *path)
>  {
> @@ -1499,6 +1514,9 @@ int read_index_from(struct index_state *istate, const char *path)
>  		ce = create_from_disk(disk_ce, &consumed, previous_name);
>  		set_index_entry(istate, i, ce);
>  
> +		if (i > 0)
> +			check_ce_order(istate->cache[i - 1], ce);
> +
>  		src_offset += consumed;
>  	}
>  	strbuf_release(&previous_name_buf);