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

Jaime Soriano Pastor [off-list ref] writes:

In the problematic cases I've seen (specially git add and git reset
--hard) the final state of both, merged and unmerged files, is that
only an entry in stage 0 exists.
Also, the current implementation of git checkout -f silently removes
higher stage entries in this case.

quoted

Silently removing these at runtime may not be something we would
want to do; after all, we do not know if the broken tool actually
wanted to have the higher stage entries, or the merged entry.

Yes, I have to agree on that, the user should have the final decission
about what stage entry to use, although I'm not sure if in the
previously commented cases there could be such an additional loss as
the operations that can be modified are already intended to silently
remove stage entries.
...
Which option would be better? And what could be a good message?

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.

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.

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.

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

On Thu, Aug 14, 2014 at 1:04 AM, Junio C Hamano [off-list ref] 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.

quoted

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!