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

[Ravi Mistry <rmistry@google.com>]

"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?

Thanks,
Ravi