Re: [PATCH] Check order when reading index

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 21, 2014, 18:51 UTC
Message-ID: <xmqq38cpsmli.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1408628606-12975-1-git-send-email-jsorianopastor@gmail.com>

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

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..e117d3a 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_next_ce(struct cache_entry *ce, struct cache_entry *next_ce) {

Have opening brace for the function on its own line, i.e.

	void check_next_ce(struct cache_entry *ce, struct cache_entry *next_ce)
	{

The function might be misnamed (see below), though.

> +	if (!ce || !next_ce)
> +		return;

Hmph, would it be either a programming error or a corrupt index input to see a NULL in either of these variables?

> +	if (cache_name_compare(ce->name, ce_namelen(ce),
> +						   next_ce->name, ce_namelen(next_ce)) > 1)

An odd indentation that is overly deep to make it hard to read.

	if (cache_name_compare(ce->name, ce_namelen(ce),
			       next_ce->name, ce_namelen(next_ce)) > 1)

should be sufficient (replacing 7-SP before next_ce with a HT is OK if the existing code nearby does so).

What is the significance of the return value from cache_name_compare() that is strictly greater than 1 (as opposed to merely "is it positive?")?

Perhaps you want something that is modeled after ce_same_name() that ignores the stage, i.e.

	int ce_name_compare(const struct cache_entry *a, const struct cache_entry *b)
	{
		return strcmp(a->ce_name, b->ce_name);
	}

without reimplementing the cache-name-compare() as

	int bad_ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
	{
        	return !ce_same_name(a, b);
	}

to keep the "two names with different length could never be the same" optimization.

	- if (0 <= ce_name_compare(ce, next)) then the names are not sorted

        - if (!stage(ce) && !name_compare(ce, next)) then the merged
          entry 'ce' is not the only entry for the path

Show 5 quoted lines

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

Show 14 quoted lines

> +		if (ce_stage(ce) >= ce_stage(next_ce))
> +			die("Unordered stage entries for '%s'", ce->name);
> +	}
> +}
> +
>  /* 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_next_ce(istate->cache[i-1], ce);

Have a SP each on both sides of binary operator "-".

Judging from the way this helper function is used, it looks like check_with_previous_ce() is a more appropriate name. After all, you are not checking the next ce which you haven't even created yet ;-)

Thanks.