Make WordPress Core

Opened 2 years ago

Closed 22 months ago

#55422 closed task (blessed) (fixed)

Add a .git-blame-ignore-revs file

Reported by: helen's profile helen Owned by: helen's profile helen
Milestone: 6.0 Priority: normal
Severity: minor Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

While we still use SVN and Trac as our sources of truth in core development, plenty of contributors use GitHub for their contribution workflows. GitHub is currently rolling out support for a Git-standard .git-blame-ignore-revs file in visual blame and it has been turned on for the WordPress org per the Gutenberg team's request. It would be nice to have a file with things like the classic "pinking shears" commits and others that are purely formatting changes. Examples would be [34534] and [42343].

Change History (21)

This ticket was mentioned in PR #2433 on WordPress/wordpress-develop by helen.


2 years ago
#1

  • Keywords has-patch added

This adds a .git-blame-ignore-revs file to exclude certain commits from visual blame on GitHub.com, which can also be used on the command line via git blame --ignore-revs-file .git-blame-ignore-revs.

Trac ticket: https://core.trac.wordpress.org/ticket/55422

#2 @helen
2 years ago

My initial PR is not a complete list of commits, though it does include all the commits I found for "pinking shears" with the exception of one that I noticed contained other changes. I would really love some help double checking these commits to make sure we're not including any that are anything besides whitespace/style fixes; it's better to be conservative to start. Would also like feedback / suggestions on the format of the file, particularly around the comments and how we label things to be able to maintain this file now and into the future.

costdev commented on PR #2433:


2 years ago
#3

Some "Code is Poetry" commits:
{{{bash
e4e1a54567dca12048e5344ab5b60a85e0d96743 # [4375]
e37aff4320b89bd94e21517f65d298f9f1fe6b37 # [45559]
}}}
and you already have this one under # PHPCS:
{{{bash
8f95800d52c1736d651ae6e259f90ad4a0db2c3f # [42343]
}}}

helen commented on PR #2433:


2 years ago
#4

@costdev I personally see those two commits as more than purely stylistic; I don't believe anything would have been affected by those specifically but sometimes subtle things like that (especially that final falsey check) can cause side effects and I would want to see those in the history. Definitely open to the idea that being more liberal about leaving commits out of our default code archaeology is a good idea, but I think I'd need a lot of convincing :)

#5 @costdev
2 years ago

@helen I'll be going through the commits you have listed to check them as requested.

What do you think is the best way to make commit suggestions for PR 2433? Should we just leave a review comment on # PHPCS, for example, with the hashes and changesets so that they can be considered for the addition?

Aside from that, we have # Pinking shears and # PHPCS at the moment. This seems like a manageable limit for now given the number of "Coding Standards" commits to be checked.

I'm curious about everyone's thoughts on the approach and wonder when we should consider the PR for merging into Core. If the idea has support, should we commit the PR once commits have been reviewed and verified as stylistic-only changes, then proceed with additional PRs in a step-by-step approach, or?

#6 @helen
2 years ago

@costdev Thanks for taking a look! I think review comments and suggestions on the PR will work great.

I hadn't really thought a whole lot about the "MVP" point of this - I think your instinct is right, we can get these pinking shears and PHPCS ones verified and then commit this, and then work on adding coding standards ones in a second pass. However, before settling on that, does anybody have a sense of how many coding standards-type commits there are? A rough estimate based on keyword searches of commits is good enough.

#7 follow-up: @costdev
2 years ago

There are 421 commits beginning with "Coding Standard" (case-insensitive).

  • See here for the HASH # [REVISION] format.
    • The command I used is at the top of this file.
  • See here for the HASH # [REVISION] # SUBJECT format.
    • This one should make it easier to spot which commits actually deal with PHPCS/WPCS configuration rather than stylistic changes.

Edit: I just remembered that markdown is a thing.

See here for the same commits but with a link to each one on the wordpress-develop GitHub repo.

Last edited 2 years ago by costdev (previous) (diff)

peterwilsoncc commented on PR #2433:


2 years ago
#8

I personally see those two commits as more than purely stylistic; I don't believe anything would have been affected by those specifically but sometimes subtle things like that (especially that final falsey check)

Yeah, e37aff4320b89bd94e21517f65d298f9f1fe6b37 broke something and was caught in the following commit. My code was not poetry.

#9 @peterwilsoncc
2 years ago

  • Keywords commit added

I think this is good to go in as an MVP.

The initial commit can go in as a See rather than a Fixes and further commits added once the act of committing gets additional eyes on the ticket.

I consider this tooling related so commits can go in during source freeze.

#10 in reply to: ↑ 7 @helen
2 years ago

  • Milestone changed from Awaiting Review to 6.0

Replying to costdev:

There are 421 commits beginning with "Coding Standard" (case-insensitive).

Whew. Let's leave that for separate commits then. Thanks for looking into that!

I left one more thought about whether to leave commits in but commented out for future us, once we resolve that then I agree on a commit with see. I'll dust off these SVN muscles again :)

helen commented on PR #2433:


2 years ago
#11

I personally see those two commits as more than purely stylistic; I don't believe anything would have been affected by those specifically but sometimes subtle things like that (especially that final falsey check)

Yeah, e37aff4 broke something and was caught in the following commit. My code was not poetry.

@peterwilsoncc I feel both disappointed in myself that I didn't actually check on follow up commits and weirdly gratified that my worst instincts remain correct 😆

#12 @peterwilsoncc
2 years ago

  • Owner set to helen
  • Status changed from new to assigned

I've answered the question inline on the PR.

Assigning to Helen for commit as an MVP.

#13 @helen
2 years ago

In 53007:

Build/Test Tools: First pass at a .git-blame-ignore-revs file.

This suppresses certain commits from Git blame views, notably in the GitHub UI as well as the command line. These commits are purely stylistic and excluding them from these views helps to reduce the noise when doing code archaeology.

Still to come: More coding standards commits as they are reviewed.

Props helen, costdev, peterwilsoncc.
See #55422

helen commented on PR #2433:


2 years ago
#14

Committed in 921d2805a0ebaf4db9f75ea9f5af46e48d6f68ac

#15 @helen
2 years ago

Noticed [12733] / 8ef8b7bad509520896877fdac4882192d746d51a while checking out the much improved visual blame, just leaving a note here for further commits :)

#16 @peterwilsoncc
2 years ago

  • Keywords has-patch commit removed

This ticket was mentioned in PR #2491 on WordPress/wordpress-develop by helen.


2 years ago
#17

  • Keywords has-patch added

This is continued work on adding revisions to .git-blame-ignore-revs to be ignored in blame views. Let's use comments and suggestions to work through adding new ones collaboratively.

Trac ticket: https://core.trac.wordpress.org/ticket/55422

#18 @peterwilsoncc
2 years ago

  • Component changed from General to Build/Test Tools

#19 @costdev
23 months ago

  • Type changed from enhancement to task (blessed)

#20 @hellofromTonya
22 months ago

  • Milestone changed from 6.0 to 6.1

6.0 RC1 is tomorrow. Though build/test tools can be committed at any time, moving remaining work to 6.1 cycle to clear the 6.0 milestone for RC.

#21 @peterwilsoncc
22 months ago

  • Milestone changed from 6.1 to 6.0
  • Resolution set to fixed
  • Status changed from assigned to closed

I've moved this back to the 6.0 milestone so r53007 doesn't get detached from the milestone in which it was committed.

A follow up ticket can be created for continued work on this in 6.1, similar to the docs, coding standards and tests tickets.

Note: See TracTickets for help on using tickets.