Re: [PATCH v3 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

[Ravi Mistry <rmistry@google.com>]

Hi Phillip!

Phillip Wood <phillip.wood123@gmail.com> writes:

> I can see we'd want to use the committed version in a bare repository,
> but it seems strange to me to ignore uncommitted changes in a non-bare
> repository. We don't do that for other special files like
> .gitattributes. It is rather inconvinient if the user has to explicitly
> pass the file inorder to test the effect of local modification to
> .git-brame-ignore-revs. To me it would make more sense to automatically
> use .git-blame-ignore-revs file from the working tree when that path is
> tracked by git.

In v1, this patch originally read .git-blame-ignore-revs from the
working tree in non-bare repositories. I switched to reading the
HEAD:.git-blame-ignore-revs blob in v2 based on Junio's review and
suggestions on v1
(https://lore.kernel.org/git/xmqqse2kma4o.fsf@gitster.g/).

> I did wonder if we wanted to automatically support
> .git-blame-ignore-revs in the root of a subtree, but I don't think there
> is an easy way to do that without visiting all the directories
> containing tracked files which makes it too expensive.

Agreed. By convention across the ecosystem (GitHub, GitLab, and soon
Gerrit and Gitiles), .git-blame-ignore-revs is read from the root of
the repository, so only looking at the root keeps it simple and avoids
the overhead of walking subdirectories.

> One final thought - do we want to limit the size of the blob that we're
> prepared to parse as we do for .gitattributes? I had a very quick look
> at the khash code which is used by 'struct oidset' and it seems to use
> 32 bit integers for the number of entries. I did not spend anytime
> trying to figure out what happens if you try and store 2^32 entires.

In parse_oidset_line(), peel_to_commit_oid() runs before
oidset_insert() and skips any OID that is not an existing local commit
in the repository, and duplicate OIDs hash to the same entry. So the
oidset (and khash) will not have more entries than the number of local
commits in the repository. That said, if you and Junio would like a
blob size cap in parse_default_ignore_revs_blob() (similar to
ATTR_MAX_FILE_SIZE), I am happy to add one.

Thanks!