Re: [PATCH 1/1] diffcore: add a filter to find a specific blob

From: Stefan Beller <sbeller@google.com>
Date: Dec 8, 2017, 21:11 UTC
Message-ID: <CAGZ79kbE9ePfxdD+rxp=P_jZwv4RPiVSbhdpjEf48dAMsKWYrQ@mail.gmail.com>
In-Reply-To: <xmqq1sk5b6rx.fsf@gitster.mtv.corp.google.com>

Show 11 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)?

Show 9 quoted lines

>> +     if (revs->diffopt.blobfind)
>> +             revs->simplify_history = 0;
>>       return 0;
>>  }
>
> It makes sense to clear this bit by default, but is this an
> unconditonal clearing?  In other words, is there a way for the user
> to countermand this default and ask for merge simplification from
> the command line, e.g. "diff --simplify-history --blobfind=<blob>"?

As noted in your reply, this is consistent with other occurrences of simplify_history, so let's keep it this way.

Show 7 quoted lines

>> +
>> +     git log --abbrev=12 --oneline --blobfind=greeting^{blob} >actual.raw &&
>> +     cut -c 14- actual.raw >actual &&
>> +     test_cmp expect actual
>
> The hardcoded numbers look strange and smell like a workaround of
> not asking for full object names.

You mean the 12 and 14 ? Yeah I can fix that to 40 and 42 if you want. I wrote it this way to be agnostic of the actual object id, but thought 12 would be enough for this test no matter the future hash.

> Also, now it has the simplify-history bit in the change, don't we
> want to check a mergy history, and also running "diff-files" while
> the index has unmerged entries?

yup, I am working on that.