Re: [PATCH 1/1] diffcore: add a filter to find a specific blob
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 8, 2017, 21:15 UTC
- Message-ID
- <xmqqefo47wgo.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <CAGZ79kbE9ePfxdD+rxp=P_jZwv4RPiVSbhdpjEf48dAMsKWYrQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
Show 16 quoted lines
>>> +
>>> + if ((DIFF_FILE_VALID(p->one) &&
>>> + oidset_contains(options->blobfind, &p->one->oid)) ||
>>> + (DIFF_FILE_VALID(p->two) &&
>>> + oidset_contains(options->blobfind, &p->two->oid))) {
>>
>> Shouldn't this make sure that !DIFF_PAIR_UNMERGED(p) before looking
>> at the sides of the pair? I think an unmerged pair is queued with
>> filespecs whose modes are cleared, but we should not be relying on
>> that---I think we even had bugs we fixed by introducing an explicit
>> is_unmerged field to the filepair struct.
>
> I am not sure I follow. IIUC 'unmerged' only ever happens in the
> index when there are multiple stages for one path (represented in the
> working tree with diff markers). Aren't we supposed to find such
> an unmerged blob as well (despite wrong mode, but the oid ought to fit)?I think diff_unmerged() records "this path is unmerged" without giving any object name to either side, so you do not get to compare (or even look at) p->{one,two}->oid with anything. That is why I think checking p->one and p->two like the above code, without making sure that you are looking at a pair that is not unmerged, is wrong.