Re: [PATCH] Check order when reading index
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 pathShow 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.
Jaime Soriano Pastor
