search

matrix-org / sygnal

Sygnal: reference Push Gateway for Matrix
Apache License 2.0
160 stars 144 forks source link

Add .git-blame-ignore-revs #256

Closed DMRobertson closed 2 years ago

DMRobertson commented 2 years ago

Configurable with

git config --local blame.ignoreRevFile=.git-blame-ignore-revs
reivilibre commented 2 years ago

Can you explain what this does?

DMRobertson commented 2 years ago

Sure.

Git 2.23 apparently added the option to "ignore" certain commits when working out which commit to git blame for a particular line. See this stackoverflow answer ; there's also a mention in the Git 2.23 changelog.

The goal is to filter out commits from blame that are fluffy, non-semantic changes. Think mass reformatting, or some kind of mass renaming that shouldn't have an impact on runtime behaviour.

We use this in synapse too.

Adding this file by itself does nothing; the behaviour is opt-in. One either passes --ignore-revs-file=FILE to a git blame invocation, or otherwise uses git config to opt into this behaviour automatically.

DMRobertson commented 2 years ago

Hmmm. Taking a look at the commit that I wanted to ignore and the corresponding PR, it looks like a biggggg squash with both formatting and functional changes. I wanted to filter out

  • Reformat code using black

but there are lots of other things in there that we probably don't want to ignore, like

  • Add Sentry integration
  • Add Prometheus support Deal with errors by reporting 500 or 502 as appropriate when things g… Generate fresh UUIDs for APNs notifications as they are not just opaque

Given that, maybe we should drop this change

reivilibre commented 2 years ago

Thanks, interesting to know that this tool exists, though yeah, from the description I'm not sure this commit (the Sygnal 'rewrite') is the best use for it, since does that mean you won't be able to see a git blame for lines that go back to Sygnal-rewrite's 'birth'?

There are a few lines that still bleed through (mostly where the logic was sound and not dependent on either gevent or Twisted), but largely I don't know if it's so bad that most things wind up pointing to the birth of the Sygnal rewrite.

I'm wondering what specifically motivated this?

clokep commented 2 years ago

I took a quick look and I don't think we want to drop this commit since it has functional changes too.

DMRobertson commented 2 years ago

I'm wondering what specifically motivated this?

I was browsing around in git blame to see how often the GCM pushkin had been changed. I wanted the soft-context of "what behaviour might have changed to cause #255?". I kept seeing this particular commit and saw

  • Reformat code using black

in its description, so assumed it was uninteresting for this kind of commit archaeology.

Having taken another look, that assumption now seems wrong (and Patrick concurs). So let's close this off for now.