tools: add .git-blame-ignore-revs file

[RaisinTen]

This would make the GitHub blame UI ignore the revisions mentioned in
the .git-blame-ignore-revs file. For now, it ignores the change
introduced in 6afd3fc as asked for in #42752 (comment).
This would make the GitHub blame UI ignore the revisions mentioned in
the .git-blame-ignore-revs file. For now, it ignores the changes
introduced in:

• 1a37fd6
• f20a88f
• fe027da
• d0bf5ca
• dc88d19

Refs: https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta
Signed-off-by: Darshan Sen [email protected]

cc @targos

[targos]

Changes to this file would need to be adapted to work on release branches. I don't know how we should handle this.

[RaisinTen]

Maybe we shouldn't backport this change to the release branches? Electron didn't backport the change that added this file - electron/electron#33171.

[targos]

Maybe we shouldn't backport this change to the release branches? Electron didn't backport the change that added this file - electron/electron#33171.

I'm fine with that. We should probably add a rule to label-pr-config.yml so we don't forget to add the label.

[RaisinTen]

Done, mapped it to a https://github.com/nodejs/node/labels/dont-backport label.

[targos]

Done, mapped it to a dont-backport label.

Is it a special placeholder for all dont-land-on-* labels?

[RaisinTen]

Yes, that's the intention.

[targos]

Okay, that seems useful but some things will have to be adapted for it.
I found:

• node/doc/contributing/releases.md

  Line 178 in 0950da1

  $ branch-diff v1.x-staging master --exclude-label=semver-major,semver-minor,dont-land-on-v1.x,backport-requested-v1.x,backport-blocked-v1.x,backport-open-v1.x,backported-to-v1.x --filter-release --format=simple

• node/doc/contributing/releases.md

  Line 198 in 0950da1

  $ branch-diff v1.x-staging master --exclude-label=semver-major,semver-minor,dont-land-on-v1.x,backport-requested-v1.x,backport-blocked-v1.x,backport-open-v1.x,backported-to-v1.x --filter-release --format=sha --reverse | xargs git cherry-pick

• node/doc/contributing/collaborator-guide.md

  Line 878 in 0950da1

  *`dont-land-on-v?.x`

• https://github.com/nodejs/node-core-utils/blob/5247dc0c64c700cd47c5221410e2359881bba046/lib/prepare_release.js#L580-L588

[mscdex]

Why are we using this feature?

[richardlau]

Done, mapped it to a dont-backport PRs that should not be backported to any release line. label.

Can we keep the individual dont-land-on-* labels? dont-backport is going to introduce ambiguity.

[RaisinTen]

@mscdex

Why are we using this feature?

From https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view:

This can be useful when a few commits make extensive changes to your code.

[RaisinTen]

@richardlau

Can we keep the individual dont-land-on-* labels? dont-backport is going to introduce ambiguity.

Yes, we could do that but that would leave us with 2 options:

1. apply all the dont-land-on-* labels manually whenever we make changes to this file OR
2. we could map this file to all the currently present dont-land-on-* labels in .github/label-pr-config.yml, so that the bot applies those automatically whenever a change touches this file but the downside is that it would require us to update the label list whenever there is a new major branch or if a branch goes EOL

I'll delete the dont-backport label and stick to option 1 if there are no objections by the EOD. (Done)

[mscdex]

This can be useful when a few commits make extensive changes to your code.

I get that, but in general it seems impossible to know if someone will want to find the changes in any particular commit (big or small), so ignoring commits isn't very helpful.

[RaisinTen]

@mscdex

I get that, but in general it seems impossible to know if someone will want to find the changes in any particular commit (big or small), so ignoring commits isn't very helpful.

In its current state, the PR is not about adding a general definition for all kinds of commits we want to include in this file. It's just about including 6afd3fc as asked for in #42752 (comment). The comment has some upvotes and no objections, so I took that as a clear indication of - yes, this looks like a good idea! That's why, I think it would be good if we add this commit to the list unless we have a concrete answer to this question - Why would someone want to find a change in 6afd3fc?

[mscdex]

@mscdex do you still feel -1 about ignoring this commit?

Considering my comments have all been about the feature being used here and not any specific commits, technically yes.

[BethGriggs]

Maybe we shouldn't backport this change to the release branches? Electron didn't backport the change that added this file - electron/electron#33171.

I'm not sure if this has already been considered - but this file will end up on future release branches (v19.x-staging onwards). That may not be a problem at all. But I figured I should mention because we will be in a state where this file exists on some release branches and not others.

[targos]

@BethGriggs I think it's fine for the file to end up in release branches.
We can also decide to let it be backported automatically to remove releaser burden, just knowing that some commit hashes might not exist in the branches (it probably won't break GitHub).

[LiviaMedeiros]

Would landing this alone give a greenish light to huge code churns (for example, enforcing non-controversal lint rules)?

[RaisinTen]

Would landing this alone give a greenish light to huge code churns (for example, enforcing non-controversal lint rules)?

I believe so, yes!

[BridgeAR]

I personally feel like this does not really benefit us. We have to go through lots of blame steps anyway. I would rather not skip anything in case it has indeed been added. I would for example compare changes between two revisions and if there's a change in-between that I did not notice, it'll definitely be confusing.

[mcollina]

I think maintaining this file will be too much of work to be useful.

[RaisinTen]

I think maintaining this file will be too much of work to be useful.

@mcollina how is this a maintenance burden? No one is obliged to update this file, it's more of an opt-in thing - if someone wants a commit to be ignored from the blame UI, they could add the commit info to this file. Not updating this file will not cause any issues.

[BethGriggs]

I'm somewhat -0 on this. I sometimes use the blame UI when comparing across release branches. Not that this PR should be blocked because of one person's workflow - I just think there's a non-zero chance of it introducing confusion by hiding the piece of blame history that someone is actively looking for.

[mcollina]

I think maintaining this file will be too much of work to be useful.

@mcollina how is this a maintenance burden? No one is obliged to update this file, it's more of an opt-in thing - if someone wants a commit to be ignored from the blame UI, they could add the commit info to this file. Not updating this file will not cause any issues.

I'm not seeing the reasoning behind doing so.
Landing PRs is hard enough already, doing this change to add a commit that will not work across multiple release lines it does not look appealing.

[RaisinTen]

I'm somewhat -0 on this. I sometimes use the blame UI when comparing across release branches. Not that this PR should be blocked because of one person's workflow - I just think there's a non-zero chance of it introducing confusion by hiding the piece of blame history that someone is actively looking for.

@BethGriggs when a commit gets backported to a release branch, doesn't it get a SHA that's different from the SHA in the HEAD branch, i.e., the one tracked in this file? If the SHA is different, this file won't hide it from the blame UI.

[RaisinTen]

I'm not seeing the reasoning behind doing so.

@mcollina it's because such commits pollute the GitHub blame output, see #41768 (comment).

Landing PRs is hard enough already

Why would landing a change to this file be hard? It won't even require a Jenkins CI run because it doesn't affect the node binary.

[BethGriggs]

@BethGriggs when a commit gets backported to a release branch, doesn't it get a SHA that's different from the SHA in the HEAD branch, i.e., the one tracked in this file? If the SHA is different, this file won't hide it from the blame UI.

I think the answer is both yes and no 😅 . For new majors, we mirror master up until the vN.0.0 release commit. So the SHAs could match those on master in some release branches. I think it would depend on where the specific commit is in history - but I do believe this means it's possible it could match in some release branches and not others.

I don't want to derail with obscure (likely rare?) cases, just to share my non-blocking concerns that this might get confusing.

Moving my concerns to a soft -0. I'm not approving this but if others feels strongly about it, do it.

[tniessen]

Would landing this alone give a greenish light to huge code churns (for example, enforcing non-controversal lint rules)?

I'm not so sure; messing up git blame is just one downside of large diffs. For example, huge diffs usually cause other open PRs or local branches to get stuck on merge conflicts.

[RaisinTen]

Would landing this alone give a greenish light to huge code churns (for example, enforcing non-controversal lint rules)?

I'm not so sure; messing up git blame is just one downside of large diffs. For example, huge diffs usually cause other open PRs or local branches to get stuck on merge conflicts.

I guess running into merge conflicts is a fate of PRs that stay open long enough. However, if the diff in question was introduced by a change that enforces a lint rule, it could be resolved by first accepting the changes already present in the PR branch during the rebase (git rebase -X theirs main) and then committing the changes introduced by the formatter.

[cjihrig]

I'm -1 on this for all of the reasons previously mentioned. I also won't block this, but I think you should consider the number of people who have raised concerns here, as well as the explicit request for changes.

[jasnell]

Really not a fan of this.

[LiviaMedeiros]

Would landing this alone give a greenish light to huge code churns (for example, enforcing non-controversal lint rules)?

I'm not so sure; messing up git blame is just one downside of large diffs. For example, huge diffs usually cause other open PRs or local branches to get stuck on merge conflicts.

Summarizing downsides of large diffs:

1. It pollutes git blame across whole codebase.
2. It causes merge conflicts on branches basing on commits before churn.
3. It pollutes diffs between release lines.

I'm seeing (1) as a huge problem that might be effectively solved by this file.
(2) and (3) are huge as well, but their significance is temporal.

On the other side we have accepted-but-not-enforced linter rules such as trailing commas and sorted keys. It doesn't look as important, but it burdens authors and reviewers constantly, making them to keep an eye on it.

On larger timescale, unless there are other important problems, I believe that getting rid of "eternal" issue is worth it, and eventually has to be done.

---

Regarding arguments that usually we have to do extra steps while using git blame anyway and usually we don't want to hide those steps: it is applicable to current state of repository, but might be changed if a churn happens in the future.

I've tried to emulate potential issue by making a copy of repository with grotesque churn (by messing with intent and quotes eslint rules and reverting back) injected at 2022-04-01, and .git-blame-ignore-revs pushed at 2022-06-28. For example, lib/assert.jswithout and with this feature added. I hope it helps to decide if churn can disrupt gitblaming workflow.

Currently I'm seeing two issues:

1. This feature still can't be toggled off in GitHub UI. I'm not sure if there is any non-ill-intentioned situation where anyone wants to hide a commit without possibility to unhide it. Perhaps it should be fixed on GH side.
   Lack of option to turn it off would block ignoring commits that have non-epsilon chance to be wanted by someone.
2. Some lines (for example, 165-166 or 170) still show commits that are supposed to be ignored.
   Regular git blame in terminal does the same.

Also worth pointing out that blame looks for ignore-revs file on "current" commit, so the file might be outdated or missing. It means that commits-to-be-hidden should be added as soon as possible to make it effective, and adding very old commits might not be useful.

Tl;dr I think it would be a good idea to add this file early but to not populate it with even slightly controversal commits at least until this feature becomes optional. Each commit candidate can be discussed individually later.

[RaisinTen]

I don't feel strongly about doing this anymore but if anyone else does, please feel free to take over the task of adding this file!