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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2014, 17:09 UTC
Message-ID: <xmqq1ts4o5qw.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAPuZ2NEPcRm4S8m76kJ+8L7zR2RiVwu3Z6bNbZgfzAcVTYgOKA@mail.gmail.com>

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

Show 21 quoted lines

> 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.

Show 6 quoted lines

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