Re: [PATCH] read-cache.c: Ensure unmerged entries are removed

[Jaime Soriano Pastor <jsorianopastor@gmail.com>]

On Thu, Aug 14, 2014 at 1:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Being a conservative, I'd rather avoid doing any magic during
> read_cache() time. "ls-files -s" for example should show the four
> stages so that the "broken" state can be inspected.
>
Well, only read_cache_unmerged() is modified in the sent patch, so no
magic is done in read_cache(), I'd also avoid changes there. Indeed
with the patch, "ls-files -s" can be used to inspect the problem
without further problems.

> Instead, I suspect that the code paths with problematic iterations
> over the index entries that assume that having stage #0 entry for a
> path guarantees that there will not be any higher stage entry first
> need to be identified (you already said "add" and "reset" may be
> problematic, there may be others, or they may be the only ones, I
> dunno), and then the most sensible one, which would be different
> from case to case, among various possibilities need to be chosen as
> a fix to each of them:
>
> (1) the loop may be fixed to ignore/skip unmerged entries;
> (2) the loop may be fixed to ignore/skip the merged entry;
> (3) the loop may be fixed not to spin indefinitely on a path with
> mixed entries; or
> (4) the command should error out.
>
git reset will clean the index anyway if the loop finishes, would it
be ok? I think that it'd be acceptable for git reset --hard to clean
the index as git checkout -f already does it even in this case.

git merge is also affected by the loop in read_cache_unmerged(), but
any of the solutions would be enough for it as only by finishing the
loop with unmerged entries it will die without commiting the cache to
the index file.

For git add probably the best option is to error out and ask the user
to check "git ls-files -s" to investigate the problem and decide what
to do.

The error message given by "git commit -a" is a bit confusing in this
case, I can take a look to this too.

I'll try to prepare a patch with these cases, and rethinking the loop
to avoid future problems there, I think that is a bit dangerous to
look for the position of a path entry (with index_name_pos) for the
next iteration.

> Yes, it would be more work, but I'd feel safer if the following
> worked:
>
> $ git ls-files -s
> 100644 3cc58df83752123644fef39faab2393af643b1d2 0 conflict
> 100644 f70f10e4db19068f79bc43844b49f3eece45c4e8 1 conflict
> 100644 3cc58df83752123644fef39faab2393af643b1d2 2 conflict
> 100644 223b7836fb19fdf64ba2d3cd6173c6a283141f78 3 conflict
> $ >empty
> $ git add empty
> 100644 3cc58df83752123644fef39faab2393af643b1d2 0 conflict
> 100644 f70f10e4db19068f79bc43844b49f3eece45c4e8 1 conflict
> 100644 3cc58df83752123644fef39faab2393af643b1d2 2 conflict
> 100644 223b7836fb19fdf64ba2d3cd6173c6a283141f78 3 conflict
> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 empty
> $ git cat-file blob :empty >output
> $ cmp empty output && echo OK
> OK
>
> which would be impossible to do if we nuked the "problematic" stages
> whenever we read the index, I am afraid.
>
This works with the first patch as read_cache() is not modified, and
git add would only clean the entries for the paths passed as
arguments.

>> BTW, I didn't know "git cat-file blob 0:$path", but I only manage to
>> get "Not a valid object name" fatals. How is it supposed to be used?
>
> That was a typo of ":$n:$path" (where 0 <= $n <= 3).
Great, thanks!