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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 25, 2017, 04:59 UTC
Message-ID: <xmqq8tevkl6x.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqpo88m896.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 6 quoted lines

> If that is the case, shouldn't we make this new mode imply
> --full-history to forbid history simplification?  "git log" is a
> tool to find _an_ explanation of the current state, and the usual
> history simplification makes tons of sense there, but blobfind is
> run most likely in order to find _all_ mention of the set of blobs
> given.

One scenario that I think we may want to be careful about is this:

 ---o---*---*---A*--M*--o---X
     \             /
      o---*---o---B

where commits marked with '*' has the same blob M:Makefile you are looking for at the same path Makefile, and we start traversal at X with "git log --blobfind=M:Makefile X" (or even with a pathspec, i.e. "git log --blobfind=M:Makefile X -- Makefile).

The usual merge simplification rules would say "Ah, M and A are TREESAME so we do not have to look at the side branch that ends at B". If the user is interested in finding all the introduction and the retirement of a specific blob object, we would miss the transition around the '*' on that side branch and ends up finding only the transitions after the fork point where the blob is introduced, and after M where the blob is retired.

Another interesting case we may want to be careful is this:

    ---A*--M*--o---X
          /
      ---B*

for the same reason. The usual merge simplification rules are designed to come up with _an_ explanation for the state in X, and because M is TREESAME with both A and B, it would pick just one (the first parent) while ignoring the other. Again, that would not be appropriate if the reason why the user is running the command is to find all the introduction and the retirement of an object.

It may be worth covering these in the tests (I didn't try to see specifically if the patch has these cases already, as I didn't think of the issue when I responded---sorry about that).

Thanks.