Re: [PATCH 2/2] Loop index increases monotonically when reading unmerged entries

[Junio C Hamano <gitster@pobox.com>]

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

> I think this line is dangerous, if add_cache_entry is not able to
> remove higher-stages it will be looping forever, as happens in the
> case of this thread.
> I cannot see why it's even needed, and removing it doesn't break any test.
>
> On Sun, Aug 24, 2014 at 7:57 PM, Jaime Soriano Pastor
> <jsorianopastor@gmail.com> wrote:
>> Signed-off-by: Jaime Soriano Pastor <jsorianopastor@gmail.com>
>> ---
>>  read-cache.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/read-cache.c b/read-cache.c
>> index c1a9619..3d70386 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1971,7 +1971,6 @@ int read_index_unmerged(struct index_state *istate)
>>                 if (add_index_entry(istate, new_ce, 0))
>>                         return error("%s: cannot drop to stage #0",
>>                                      new_ce->name);
>> -               i = index_name_pos(istate, new_ce->name, len);

I think the original idea was that regardless of how many entries
with the same name were removed because of the replacement (or
addition) of "new_ce", by making "i" point at the newly added
"new_ce", we would make sure that the loop will continue from the
next entry.  The if/return expected that add_index_entry() will get
rid of all the other entries with the same name as "new_ce" has or it
will return an error.

Without the "bug" in add_index_entry(), because "new_ce" always has
the same name as "ce", the entry we found at "i" by the loop, we know
that index_name_pos() will give the same "i" we already have, so
removing this line should be a no-op.

Now, add_index_entry() in your case did not notice that it failed to
remove all other entries with the same name as "new_ce", resulting
in your "looping forever".  Among the "merged and unmerged entries
with the same name exists in the index file" class of index file
corruption, we could treat the "merged and unmerged entries with the
same name not just exists but next to each other, unmerged ones
coming immediately after merged one" case specially (i.e. declaring
that it is more likely for a broken software to leave both next to
each other than otherwise) and try to accomodate it as your original
patch did.  I am not absolutely sure if such a special case is worth
it, and with your updated "[1/2] read_index_from(): check order of
entries when reading index" we will not be doing so, which is good.

With that safety in place, the "bug" in add_index_entry() will
disappear; it is safe not to adjust "i" by calling index_name_pos()
and this patch, "[2/2] read_index_unmerged(): remove unnecessary
loop index adjustment", will be a good thing to do.

Thanks.

>>         }
>>         return unmerged;
>>  }
>> --
>> 2.0.4.1.g0b8a4f9.dirty
>>