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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 13, 2014, 23:04 UTC
Message-ID: <xmqqiolw2dqc.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAPuZ2NFcJ5FFeJP=0-oG59fs=3ztenoPOGWOZKfZDdHpLGOYeA@mail.gmail.com>

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

Show 18 quoted lines

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