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

From: Jaime Soriano Pastor <jsorianopastor@gmail.com>
Date: Aug 13, 2014, 22:10 UTC
Message-ID: <CAPuZ2NFcJ5FFeJP=0-oG59fs=3ztenoPOGWOZKfZDdHpLGOYeA@mail.gmail.com>
In-Reply-To: <xmqq8umt5z8i.fsf@gitster.dls.corp.google.com>

On Tue, Aug 12, 2014 at 8:39 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

>
> Jaime Soriano Pastor <jsorianopastor@gmail.com> writes:
>
> > Wrong implementations of tools that modify the index can left
> > some files as merged and unmerged at the same time. Avoid undesiderable
> > behaviours by handling this situation.
>
> It is understandable that the way _you_ decided to "handle the
> situation" is so obvious to be spelled out to _you_, but that is the
> most important design decision that needs to be described here.  Do
> you silently remove higher-stage entries when an entry at stage 0
> exists?  Do you silently remove stage 0 entry when higher-stage
> entries exist?  Do you error out without doing neither?
>

Sorry, I didn't explain my decission enough, and my knowledge of git internals is not so good. The idea of my proposal is to remove higher stage entries when, after replacing an existing entry at stage 0, there are still entries in higher stages.

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.

Show 5 quoted lines

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

Show 6 quoted lines

> Ideally, I think we should error out and let the users figure out
> how to proceed (we may of course need to make sure they have the
> necessary tools to do so, e.g. "git cat-file blob 0:$path" to
> resurrect the contents and "git update-index --cacheinfo" to stuff
> the contents into the stages).
>

I have also tried a couple of implementations of this patch with die() and warning(). The implementation with die() would have a message like "There are other staged versions for merged file", and maybe some recomendation about how to see the blobs. The warning implementation could return -1, what would prevent git add to remove the higher-stage entries, but would still make git reset --hard to clean the index as it seems that it does it anyway if it manages to finish the call to read_index_unmerged. Another option would be to print the deleted entries as a warning but deleting them anyway.

Which option would be better? And what could be a good message?

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?

Thanks.