Re: [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs

[Junio C Hamano <gitster@pobox.com>]

Ravi Mistry <rmistry@google.com> writes:

> "Junio C Hamano" <gitster@pobox.com> writes:
>
>> While wanting consistency is reasonable, the description above does
>> not quite match that goal.  If an untracked '.git-blame-ignore-revs'
>> file exists at the root of the working tree, or if a tracked one has
>> local changes relative to HEAD, the local repository behaves
>> differently from hosting sites that operate on the
>> 'HEAD:.git-blame-ignore-revs' blob.  It may make more sense to say:
>> "If the 'HEAD:.git-blame-ignore-revs' blob exists, it is added as
>> the initial element in the list of ignore-revs files.  Other files
>> listed in the configuration are also used, but an empty element
>> makes all elements that appeared before in the list forgotten."
>> This rule should apply whether the repository is bare or not.
>
> Thank you very much for the detailed feedback, Junio! Reading the
> committed blob from HEAD instead of the working tree totally makes
> sense.
>
>> Somebody has to audit the parser for these files (one unabbreviated
>> object name per line, ignoring whitespace and lines starting with
>> '#') and ensure that the implementation is truly secure.
>
> I looked through the parser in oidset.c (which we can share for
> both the HEAD blob and configured files) and peel_to_commit_oid in
> builtin/blame.c. Mostly looks good, IMHO, but there may be two edge
> cases we can tighten up:
>
> 1. Rejecting lines with embedded NUL bytes via memchr in oidset.c
>    (where strchr and the check after parse_oid_hex_algop currently
>    stop at the first NUL byte and ignore trailing bytes on the
>    line).
>
> 2. Passing OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK in
>    peel_to_commit_oid and peeling tags step by step so missing OIDs
>    or tag targets do not trigger lazy promisor fetches in partial
>    clones.
>
> Does this plan sound good to you for v2?

Are you presenting a different plan, or just adding details to what
you quoted from my message above?

I delegated because I did not want to spend time on the auditing
part, so if you are asking me that these two are the only things we
need to address, that defeats the point of me delegating it to
"somebody else" X-<.  Hopefully a v2 with some tightening the OID
parsing may entice folks (who are hopefully interested in security
related work) to chime in and they would help us decide if it is
good enough to cover these two points and nothing else.

Thanks.