Skip to content

Conversation

mzschwartz5
Copy link
Contributor

Description

Added a git-blame-ignore-revs file. This allows blame to skip certain pervasive changes. I included two major offenders to start: the commit that ran prettier on the whole codebase, and the commit that changed var/let to const.

To use the file:
git config blame.ignoreRevsFile .git-blame-ignore-revs

If using gitlens in vscode, you can add this setting:

    "gitlens.advanced.blame.customArguments": [
        "--ignore-revs-file",
        "${workspaceFolder}/.git-blame-ignore-revs"
    ],

Issue number and link

Testing plan

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@mzschwartz5 mzschwartz5 requested a review from jjspace August 26, 2025 14:47
Copy link

Thank you for the pull request, @mzschwartz5!

✅ We can confirm we have a CLA on file for you.

Copy link
Contributor

@jjspace jjspace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mzschwartz5, I definitely like the idea of this but have a couple comments

  1. Please add comments to the new ignore file so we know what those commits are. The Github docs also suggest this.
  2. Please add the commit from the prettier v3 upgrade, this also touches a majority of files: #12206. Commit 09a719b8fb4616ecbcd7370e81dcdc998b64b6e2
  3. I think it does make sense to include the gitlens setting in our workspace settings. It's not (currently) in our recommended extensions list but I assume a lot of people use it and it's specific to our project so could be helpful?
    • counter point, maybe it's generic enough it should be in my user settings? But if the file doesn't exist does it break gitlens's blame?
    • If it's not added to our workspace settings then it might also be good to call out in the dev setup guide
  4. Is the config command necessary? If so maybe we should add it to the developer guide as part of the setup process so all devs can use it.
    • Alternatively, is there a way to share common git settings so it's automatically set up for everyone? I've never looked into that so maybe not

@jjspace
Copy link
Contributor

jjspace commented Aug 26, 2025

Also I'm still seeing the prettier commit in gitlens even with the setting on. I added the prettier v3 commit to test and that one is getting hidden. Any ideas?

This is in the file engine/Source/Core/ApproximateTerrainHeights.js
image

@mzschwartz5
Copy link
Contributor Author

@jjspace
1 + 2. Can do.
3. It doesn't break gitlen's blame if the file exists but you don't use it. Personally, I think if we're going to commit the file, we should include the setting in our workspace settings. Can't see any reason not to use it, so why make people set it up themselves.
4. It appears it is necessary if you use git blame from the command line, rather than gitlens. I think we could potentially add the setting to a new .gitconfig repo file, but then users would have to run a command to use that config locally. So it's a question of running a git blame config command once, or running a git config command once.

@jjspace
Copy link
Contributor

jjspace commented Aug 26, 2025

Can't see any reason not to use it, so why make people set it up themselves.

I was looking into this a bit more and per this comment it seems gitlens will respect the git setting itself and no workspace setting is required. I tested it and it seems to work as expected, I say we leave it out for now and focus on the gitconfig itself.

I think we could potentially add the setting to a new .gitconfig repo file, but then users would have to run a command to use that config locally. So it's a question of running a git blame config command once, or running a git config command once.

Regardless it should probably be documented in the developer setup guide. If we're already going to have people run a command it might make more sense to opt for a generic one. Then we could potentially add more settings to it later and everyone will already be set up for it to work. I like the include statement like this SO answer points out: https://stackoverflow.com/a/18330114/7416863
(cc @ggetz any opinions on this?)

@mzschwartz5
Copy link
Contributor Author

mzschwartz5 commented Aug 26, 2025

  1. I think the reason some blames are still showing up in the above picture is because Prettier created those new lines (new whitespace, or moving brackets / parentheses to new lines). There's no blame before that for those lines, so it shows the Prettier commit.
  2. I'm still leaning towards including the gitlens settings, because it does apply automatically, whereas the git config does not. I'd still want both settings + documentation for the latter. edit - or just forget the new .gitconfig and just document the option to set it yourself.

@jjspace
Copy link
Contributor

jjspace commented Aug 26, 2025

There's no blame before that for those lines, so it shows the Prettier commit.

I didn't even think of that but I think you're totally right, looks good then.

Also pretty cool that github will respect this too!

image

@ggetz
Copy link
Contributor

ggetz commented Aug 26, 2025

Regardless it should probably be documented in the developer setup guide. If we're already going to have people run a command it might make more sense to opt for a generic one. Then we could potentially add more settings to it later and everyone will already be set up for it to work. I like the include statement like this SO answer points out: https://stackoverflow.com/a/18330114/7416863
(cc @ggetz any opinions on this?)

No strong opinions on either option, but a 👍 for documenting in in the build/setup guide.

@mzschwartz5 mzschwartz5 requested a review from jjspace September 3, 2025 19:12
@mzschwartz5
Copy link
Contributor Author

mzschwartz5 commented Sep 3, 2025

@jjspace Added documentation on these changes - should be good to merge now.

@jjspace jjspace added this pull request to the merge queue Sep 4, 2025
Merged via the queue into main with commit ad2deb3 Sep 4, 2025
9 checks passed
@jjspace jjspace deleted the git-blame-ignore branch September 4, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants