Re: [PATCH] Check order when reading index
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:19
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] Check order when reading index · Jaime Soriano Pastor <hidden>
- 2016-06-15 · Re: [PATCH] Check order when reading index · Jaime Soriano Pastor <hidden>
- 2016-06-15 · Re: [PATCH] Check order when reading index · Duy Nguyen <hidden>
Jaime Soriano Pastor [off-list ref] writes:
quoted hunk
Signed-off-by: Jaime Soriano Pastor <redacted> --- 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
+ 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);quoted hunk
+ 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.