[PATCH] git-contacts: ignore blame boundary commits

[Aleksei Sviridkin <f@lex.la>]

git-contacts asks "git blame" which commits last touched the lines a
patch modifies, limiting the annotation to the last five years. Blame
charges lines that did not change inside a limited range to the commit
where its traversal stopped and marks that entry with a "boundary"
line in the porcelain output, which the parser has ignored since
4d06402b1b (contrib: add git-contacts helper, 2013-07-21). The
boundary commit is therefore imported like any other, and its author
and the people named in its trailers end up in the list.

Such a commit usually did not touch the file at all. Blaming a change
to builtin/receive-pack.c stops on a commit that only touched
builtin/fetch.c, and its author is proposed as a reviewer while the
people who actually wrote the lines are left out, as the age limit
intends. Those commits also pad the commit count each name is weighed
against, hiding real contacts under the ten percent threshold. Over
the hundred non-merge commits below 3cb9185f65 (The 22nd batch,
2026-09-02), twenty-seven lists lose a name, nine of them becoming
empty because nothing inside the window touched the lines, and three
gain a name the padding had hidden.

Read each blame run to the end before deciding, and register only the
commits it did not mark as a boundary, so that a commit stays a
contact as long as some hunk is really blamed on it. Ask for --root as
well, because blame marks the initial commit of a repository as a
boundary too, and in a repository younger than the window that commit
did write the lines it is blamed for. The cut-off commit of a shallow
clone is parentless in the same way, so it still passes as a contact
inside the window, a wrong answer this change leaves alone. The manual
page says that every commit "git blame" mentions is consulted, so note
the exception there as well.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---

Notes:
    Notes for the list, deliberately kept out of the commit message.
    
    Where this is written down, since it bears on how the bug survived.
    git-blame(1) documents the behaviour under SPECIFYING RANGES: lines
    that have not changed since the range boundary are blamed for that
    range boundary commit, with --since=3.weeks given as the example.  So
    the script has been relying on documented blame behaviour and reading
    it wrongly, not fighting blame.  The porcelain marker itself is not
    documented anywhere: THE PORCELAIN FORMAT lists author, committer,
    filename and summary, and mentions neither boundary nor previous.
    That is the likely reason the field was ignored in 2013.
    
    Why no --is-shallow-repository gate was added.  In a shallow clone
    blame does attribute the truncated history, and it attributes it to
    the cut-off commit, which is grafted parentless.  The marking is
    decided in blame.c: a commit is reported as a boundary when it
    carries UNINTERESTING, which is set either because its date is older
    than --since or because it has no parents and --root was not given.
    Since --root is now always passed, the age limit is in practice the
    only source of boundary marks this script can still see.
    Asking for --root therefore spares the shallow cut-off commit, so it
    stays a contact, and it stays exactly as false a contact as it was
    before this change.  Gating on git rev-parse --is-shallow-repository
    would only let the script pick a different wrong answer, and picking
    one is a separate decision from the boundary bug fixed here.
    
    On the window condition in that sentence.  The age test runs before
    the root test, so --root cannot rescue a cut-off commit that is
    already older than five years, and such a commit is skipped like any
    other out-of-window commit.  Checked both ways on a four-commit
    repository shallow-cloned at depth two: with the cut-off dated 2012
    the old script offers it and this one does not, and with the cut-off
    inside the window both scripts offer it.  That is what a non-shallow
    repository does with an equally old commit, and it is what the age
    limit is for.
    
    Why the skip is per blame run rather than global.  %blamed and
    %boundary are lexicals inside get_blame() and are rebuilt on every
    call, while %seen stays file-scoped.  One hunk's blame may stop on a
    commit that another hunk's blame attributes real lines to.
    Accumulating boundary commits across runs would suppress the second
    attribution too, dropping a commit that genuinely wrote part of the
    patch because an unrelated hunk happened to end on it.  Per-run
    state registers the commit as soon as any single run blames it for
    real.
    
    Running the script on this patch demonstrates the change, and the
    result looks alarming until you work it out.  The script itself was
    last modified in 2017, so nothing inside the five-year window touched
    the lines this patch changes.  The current script prints one name,
    the author of whatever commit sits at the five-year mark, and the
    patched script prints nothing at all.  The empty list is the honest
    answer: inside the window there is no contact to offer, and the name
    the current script offers was never one.
    
    Falling back to the boundary commit when the list comes out empty was
    considered and rejected.  Under git send-email --cc-cmd an empty list
    means no Cc line, but the boundary commit is a stranger by
    construction, so a wrong Cc is worse than none, and the list address
    itself does not come from this script.
    
    The three lists that gain a name gain it because the denominator
    shrinks.  For 429dd07aa0 the current script weighs each name against
    eleven commits, one of them a boundary, and Robin Jarry lands at one
    mention in eleven, 9.1 percent, just under the threshold.  Drop the
    boundary commit and the same single mention is one in ten, and he is
    printed.
    
    The 100 commits measured are master's history rather than maint's,
    which is where the recent traffic is.  The proportions do not move
    elsewhere.  Over maint's own last 100 non-merge commits: 29 lists
    change, 24 lose a name, 8 of those go empty, 5 gain one.  Over 300
    commits of master: 88, 76, 27 and 12.  In every sample the emptied
    lists are a subset of the ones that lost a name, and no list both
    loses and gains.
    
    An unrelated defect in the same file, left alone on purpose.  Every
    object-name regex in the script demands exactly forty hex
    characters:
    
    	if ($line =~ /^([0-9a-f]{40}) commit (\d+)/) {
    	if (/^([0-9a-f]{40}) \d+ \d+ \d+$/) {
    	if (/^From ([0-9a-f]{40}) Mon Sep 17 00:00:00 2001$/) {
    
    In a SHA-256 repository the object names are sixty-four characters,
    nothing matches, and the script prints an empty list and exits
    successfully.  This is also why the boundary branch added here tests
    defined($cur) first: the group header does not match, so $cur is
    never set, while the boundary line itself still matches and would
    otherwise index the hash with undef and warn under use warnings.  The
    guard keeps such a repository as silent as it was before this patch
    rather than half-fixing it.  Behaviour today:
    
    	$ git init --object-format=sha256 r && cd r
    	$ ... two commits, each carrying a Signed-off-by ...
    	$ git contacts 'HEAD^!'
    	$ echo $?
    	0
    
    That silence is there before this patch as well, so it is not a
    regression, and repairing it means all three regexes at once.  It
    wants its own patch.
    
    A second one, noted for the same reason.  Three subprocess pipes are
    closed without checking the exit status -- in get_blame(),
    parse_rev_args() and scan_rev_args() -- while import_commits() and
    mailmap_contacts() both die on a non-zero $?.  A blame that fails is
    therefore indistinguishable from a blame that found nothing.  This
    patch does sharpen the consequence without causing it: an empty list
    is now a legitimate answer, so the silent-failure case and the
    correct case look alike where before an empty list was already
    suspicious.  Still older than this patch, and still not repaired
    here.
    
    A third, and the cheapest of them.  The final loop prints keys
    %$contacts unsorted, so Perl's hash randomisation reorders the output
    between runs: five runs over the same commit here produced five
    different orderings of the same four names.  That makes the output
    awkward to diff and awkward to pin in a test, which matters if anyone
    wants to add the test surface this directory has never had.  One sort
    fixes it.  This patch does not go near the print loop.
    
    Based on maint rather than master: the behaviour has been wrong in
    released versions since 4d06402b1b in 2013, and SubmittingPatches asks
    for the oldest integration branch a change is relevant to.

 contrib/contacts/git-contacts      | 16 +++++++++++-----
 contrib/contacts/git-contacts.adoc | 12 +++++++-----
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/contrib/contacts/git-contacts b/contrib/contacts/git-contacts
index 85ad732fc0..25a918ae92 100755
--- a/contrib/contacts/git-contacts
+++ b/contrib/contacts/git-contacts
@@ -62,18 +62,24 @@ sub get_blame {
 	my ($commits, $source, $from, $ranges) = @_;
 	return unless @$ranges;
 	open my $f, '-|',
-		qw(git blame --porcelain -C),
+		qw(git blame --porcelain -C --root),
 		map({"-L$_->[0],+$_->[1]"} @$ranges),
 		'--since', $since, "$from^", '--', $source or die;
+	my ($cur, %blamed, %boundary);
 	while (<$f>) {
 		if (/^([0-9a-f]{40}) \d+ \d+ \d+$/) {
-			my $id = $1;
-			$commits->{$id} = { id => $id, contacts => {} }
-				unless $seen{$id};
-			$seen{$id} = 1;
+			$cur = $1;
+			$blamed{$cur} = 1;
+		} elsif (defined($cur) && /^boundary$/) {
+			$boundary{$cur} = 1;
 		}
 	}
 	close $f;
+	for my $id (keys %blamed) {
+		next if $boundary{$id} || $seen{$id};
+		$commits->{$id} = { id => $id, contacts => {} };
+		$seen{$id} = 1;
+	}
 }
 
 sub blame_sources {
diff --git a/contrib/contacts/git-contacts.adoc b/contrib/contacts/git-contacts.adoc
index dd914d1261..725e2be6f4 100644
--- a/contrib/contacts/git-contacts.adoc
+++ b/contrib/contacts/git-contacts.adoc
@@ -37,11 +37,13 @@ DISCUSSION
 
 `git blame` is invoked for each hunk in a patch file or revision.  For each
 commit mentioned by `git blame`, the commit message is consulted for people who
-authored, reviewed, signed, acknowledged, or were Cc:'d.  Once the list of
-participants is known, each person's relevance is computed by considering how
-many commits mentioned that person compared with the total number of commits
-under consideration.  The final output consists only of participants who exceed
-a minimum threshold of participation.
+authored, reviewed, signed, acknowledged, or were Cc:'d.  Commits that `git
+blame` marks as a boundary of its search are skipped, since they need not have
+touched the lines at all, so a patch may end up with no participants.  Once the
+list of participants is known, each person's relevance is computed by
+considering how many commits mentioned that person compared with the total
+number of commits under consideration.  The final output consists only of
+participants who exceed a minimum threshold of participation.
 
 
 OUTPUT

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
-- 
2.55.0