{"thread":{"id":"66264","subject":"[PATCH] git-contacts: ignore blame boundary commits","startedAt":"2026-09-03T12:55:30Z","lastAt":"2026-09-03T12:55:30Z","messageCount":1,"participants":["Aleksei Sviridkin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"551862","messageId":"20260903125527.67934-1-f@lex.la","threadId":"66264","inReplyTo":null,"subject":"[PATCH] git-contacts: ignore blame boundary commits","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-03T12:55:27Z","receivedAt":"2026-09-03T12:55:30Z","isPatch":true,"body":"git-contacts asks \"git blame\" which commits last touched the lines a\npatch modifies, limiting the annotation to the last five years. Blame\ncharges lines that did not change inside a limited range to the commit\nwhere its traversal stopped and marks that entry with a \"boundary\"\nline in the porcelain output, which the parser has ignored since\n4d06402b1b (contrib: add git-contacts helper, 2013-07-21). The\nboundary commit is therefore imported like any other, and its author\nand the people named in its trailers end up in the list.\n\nSuch a commit usually did not touch the file at all. Blaming a change\nto builtin/receive-pack.c stops on a commit that only touched\nbuiltin/fetch.c, and its author is proposed as a reviewer while the\npeople who actually wrote the lines are left out, as the age limit\nintends. Those commits also pad the commit count each name is weighed\nagainst, hiding real contacts under the ten percent threshold. Over\nthe hundred non-merge commits below 3cb9185f65 (The 22nd batch,\n2026-09-02), twenty-seven lists lose a name, nine of them becoming\nempty because nothing inside the window touched the lines, and three\ngain a name the padding had hidden.\n\nRead each blame run to the end before deciding, and register only the\ncommits it did not mark as a boundary, so that a commit stays a\ncontact as long as some hunk is really blamed on it. Ask for --root as\nwell, because blame marks the initial commit of a repository as a\nboundary too, and in a repository younger than the window that commit\ndid write the lines it is blamed for. The cut-off commit of a shallow\nclone is parentless in the same way, so it still passes as a contact\ninside the window, a wrong answer this change leaves alone. The manual\npage says that every commit \"git blame\" mentions is consulted, so note\nthe exception there as well.\n\nAssisted-by: LLM\nSigned-off-by: Aleksei Sviridkin <f@lex.la>\n---\n\nNotes:\n    Notes for the list, deliberately kept out of the commit message.\n    \n    Where this is written down, since it bears on how the bug survived.\n    git-blame(1) documents the behaviour under SPECIFYING RANGES: lines\n    that have not changed since the range boundary are blamed for that\n    range boundary commit, with --since=3.weeks given as the example.  So\n    the script has been relying on documented blame behaviour and reading\n    it wrongly, not fighting blame.  The porcelain marker itself is not\n    documented anywhere: THE PORCELAIN FORMAT lists author, committer,\n    filename and summary, and mentions neither boundary nor previous.\n    That is the likely reason the field was ignored in 2013.\n    \n    Why no --is-shallow-repository gate was added.  In a shallow clone\n    blame does attribute the truncated history, and it attributes it to\n    the cut-off commit, which is grafted parentless.  The marking is\n    decided in blame.c: a commit is reported as a boundary when it\n    carries UNINTERESTING, which is set either because its date is older\n    than --since or because it has no parents and --root was not given.\n    Since --root is now always passed, the age limit is in practice the\n    only source of boundary marks this script can still see.\n    Asking for --root therefore spares the shallow cut-off commit, so it\n    stays a contact, and it stays exactly as false a contact as it was\n    before this change.  Gating on git rev-parse --is-shallow-repository\n    would only let the script pick a different wrong answer, and picking\n    one is a separate decision from the boundary bug fixed here.\n    \n    On the window condition in that sentence.  The age test runs before\n    the root test, so --root cannot rescue a cut-off commit that is\n    already older than five years, and such a commit is skipped like any\n    other out-of-window commit.  Checked both ways on a four-commit\n    repository shallow-cloned at depth two: with the cut-off dated 2012\n    the old script offers it and this one does not, and with the cut-off\n    inside the window both scripts offer it.  That is what a non-shallow\n    repository does with an equally old commit, and it is what the age\n    limit is for.\n    \n    Why the skip is per blame run rather than global.  %blamed and\n    %boundary are lexicals inside get_blame() and are rebuilt on every\n    call, while %seen stays file-scoped.  One hunk's blame may stop on a\n    commit that another hunk's blame attributes real lines to.\n    Accumulating boundary commits across runs would suppress the second\n    attribution too, dropping a commit that genuinely wrote part of the\n    patch because an unrelated hunk happened to end on it.  Per-run\n    state registers the commit as soon as any single run blames it for\n    real.\n    \n    Running the script on this patch demonstrates the change, and the\n    result looks alarming until you work it out.  The script itself was\n    last modified in 2017, so nothing inside the five-year window touched\n    the lines this patch changes.  The current script prints one name,\n    the author of whatever commit sits at the five-year mark, and the\n    patched script prints nothing at all.  The empty list is the honest\n    answer: inside the window there is no contact to offer, and the name\n    the current script offers was never one.\n    \n    Falling back to the boundary commit when the list comes out empty was\n    considered and rejected.  Under git send-email --cc-cmd an empty list\n    means no Cc line, but the boundary commit is a stranger by\n    construction, so a wrong Cc is worse than none, and the list address\n    itself does not come from this script.\n    \n    The three lists that gain a name gain it because the denominator\n    shrinks.  For 429dd07aa0 the current script weighs each name against\n    eleven commits, one of them a boundary, and Robin Jarry lands at one\n    mention in eleven, 9.1 percent, just under the threshold.  Drop the\n    boundary commit and the same single mention is one in ten, and he is\n    printed.\n    \n    The 100 commits measured are master's history rather than maint's,\n    which is where the recent traffic is.  The proportions do not move\n    elsewhere.  Over maint's own last 100 non-merge commits: 29 lists\n    change, 24 lose a name, 8 of those go empty, 5 gain one.  Over 300\n    commits of master: 88, 76, 27 and 12.  In every sample the emptied\n    lists are a subset of the ones that lost a name, and no list both\n    loses and gains.\n    \n    An unrelated defect in the same file, left alone on purpose.  Every\n    object-name regex in the script demands exactly forty hex\n    characters:\n    \n    \tif ($line =~ /^([0-9a-f]{40}) commit (\\d+)/) {\n    \tif (/^([0-9a-f]{40}) \\d+ \\d+ \\d+$/) {\n    \tif (/^From ([0-9a-f]{40}) Mon Sep 17 00:00:00 2001$/) {\n    \n    In a SHA-256 repository the object names are sixty-four characters,\n    nothing matches, and the script prints an empty list and exits\n    successfully.  This is also why the boundary branch added here tests\n    defined($cur) first: the group header does not match, so $cur is\n    never set, while the boundary line itself still matches and would\n    otherwise index the hash with undef and warn under use warnings.  The\n    guard keeps such a repository as silent as it was before this patch\n    rather than half-fixing it.  Behaviour today:\n    \n    \t$ git init --object-format=sha256 r && cd r\n    \t$ ... two commits, each carrying a Signed-off-by ...\n    \t$ git contacts 'HEAD^!'\n    \t$ echo $?\n    \t0\n    \n    That silence is there before this patch as well, so it is not a\n    regression, and repairing it means all three regexes at once.  It\n    wants its own patch.\n    \n    A second one, noted for the same reason.  Three subprocess pipes are\n    closed without checking the exit status -- in get_blame(),\n    parse_rev_args() and scan_rev_args() -- while import_commits() and\n    mailmap_contacts() both die on a non-zero $?.  A blame that fails is\n    therefore indistinguishable from a blame that found nothing.  This\n    patch does sharpen the consequence without causing it: an empty list\n    is now a legitimate answer, so the silent-failure case and the\n    correct case look alike where before an empty list was already\n    suspicious.  Still older than this patch, and still not repaired\n    here.\n    \n    A third, and the cheapest of them.  The final loop prints keys\n    %$contacts unsorted, so Perl's hash randomisation reorders the output\n    between runs: five runs over the same commit here produced five\n    different orderings of the same four names.  That makes the output\n    awkward to diff and awkward to pin in a test, which matters if anyone\n    wants to add the test surface this directory has never had.  One sort\n    fixes it.  This patch does not go near the print loop.\n    \n    Based on maint rather than master: the behaviour has been wrong in\n    released versions since 4d06402b1b in 2013, and SubmittingPatches asks\n    for the oldest integration branch a change is relevant to.\n\n contrib/contacts/git-contacts      | 16 +++++++++++-----\n contrib/contacts/git-contacts.adoc | 12 +++++++-----\n 2 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/contrib/contacts/git-contacts b/contrib/contacts/git-contacts\nindex 85ad732fc0..25a918ae92 100755\n--- a/contrib/contacts/git-contacts\n+++ b/contrib/contacts/git-contacts\n@@ -62,18 +62,24 @@ sub get_blame {\n \tmy ($commits, $source, $from, $ranges) = @_;\n \treturn unless @$ranges;\n \topen my $f, '-|',\n-\t\tqw(git blame --porcelain -C),\n+\t\tqw(git blame --porcelain -C --root),\n \t\tmap({\"-L$_->[0],+$_->[1]\"} @$ranges),\n \t\t'--since', $since, \"$from^\", '--', $source or die;\n+\tmy ($cur, %blamed, %boundary);\n \twhile (<$f>) {\n \t\tif (/^([0-9a-f]{40}) \\d+ \\d+ \\d+$/) {\n-\t\t\tmy $id = $1;\n-\t\t\t$commits->{$id} = { id => $id, contacts => {} }\n-\t\t\t\tunless $seen{$id};\n-\t\t\t$seen{$id} = 1;\n+\t\t\t$cur = $1;\n+\t\t\t$blamed{$cur} = 1;\n+\t\t} elsif (defined($cur) && /^boundary$/) {\n+\t\t\t$boundary{$cur} = 1;\n \t\t}\n \t}\n \tclose $f;\n+\tfor my $id (keys %blamed) {\n+\t\tnext if $boundary{$id} || $seen{$id};\n+\t\t$commits->{$id} = { id => $id, contacts => {} };\n+\t\t$seen{$id} = 1;\n+\t}\n }\n \n sub blame_sources {\ndiff --git a/contrib/contacts/git-contacts.adoc b/contrib/contacts/git-contacts.adoc\nindex dd914d1261..725e2be6f4 100644\n--- a/contrib/contacts/git-contacts.adoc\n+++ b/contrib/contacts/git-contacts.adoc\n@@ -37,11 +37,13 @@ DISCUSSION\n \n `git blame` is invoked for each hunk in a patch file or revision.  For each\n commit mentioned by `git blame`, the commit message is consulted for people who\n-authored, reviewed, signed, acknowledged, or were Cc:'d.  Once the list of\n-participants is known, each person's relevance is computed by considering how\n-many commits mentioned that person compared with the total number of commits\n-under consideration.  The final output consists only of participants who exceed\n-a minimum threshold of participation.\n+authored, reviewed, signed, acknowledged, or were Cc:'d.  Commits that `git\n+blame` marks as a boundary of its search are skipped, since they need not have\n+touched the lines at all, so a patch may end up with no participants.  Once the\n+list of participants is known, each person's relevance is computed by\n+considering how many commits mentioned that person compared with the total\n+number of commits under consideration.  The final output consists only of\n+participants who exceed a minimum threshold of participation.\n \n \n OUTPUT\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n-- \n2.55.0\n\n"}]}