Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add .git-blame-ignore-revs #1532

Closed
wants to merge 3 commits into from
Closed

Conversation

katcharov
Copy link
Contributor

@katcharov katcharov commented Feb 29, 2024

See:

This ignores some reformatting commits arising from DRIVERS-2789 when viewing git blame on GitHub and in other tools. Tested auth.md in IntelliJ.

Blame before, note most lines attributed to the Sync with 4b7d757 commit.

Blame after (as of this change). In some cases, tools do not identify the correct commit.

@katcharov katcharov marked this pull request as ready for review February 29, 2024 19:24
@katcharov katcharov requested a review from a team as a code owner February 29, 2024 19:24
@katcharov katcharov requested review from dariakp, blink1073 and eramongodb and removed request for a team February 29, 2024 19:24
Comment on lines 2 to 3
# DRIVERS-2789
ca372f0c6a1353742437d65131b37544492692c2
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend using git log -n 1 --format=format:"%H # %s (%cs)" <hash> to generate entries for this file. e.g.:

ca372f0c6a1353742437d65131b37544492692c2 # Sync with 4b7d7573 (2024-02-26)
634d6399bc166d27f290599d8facf034fa17be91 # Sync with 4b7d7573 (2024-02-26)
405e736abf6884c69669fcc060ef76c44f2e63e7 # Sync with 4b7d7573 (2024-02-26)
...
cea087babea68571fa4068ace6b2d326c3a0a22d # DRIVERS-2789 Convert BSON Corpus Spec to Markdown (#1499) (2024-01-23)
e9ae09da918dc7ca7e5ae12a1af6ca34e9c7854e # DRIVERS-2790 Add codespell checker (#1491) (2024-01-10)
eef59d9d386bb054e0eb59e6bd540a030186f899 # DRIVERS-2789 Use Markdown for Specifications Documentation (#1482) (2024-01-04)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used cat .git-blame-ignore-revs | xargs -n1 git log -n 1 --format=format:"%H # %s (%cs)" $1 to update.

@eramongodb
Copy link
Contributor

eramongodb commented Mar 1, 2024

I'd like to defer my explicit approval/disapproval until more feedback is given by other reviewers. However, I lean towards disapproval (rejecting proposed changes in this PR).

To reiterate my comments from external discussion in short summary, I would rather deal with the presence of the Markdown conversion and the "Sync with" commits in the line-based git blame history over allowing for the possibility of misleading git blame line-to-commit attribution resulting from the use of --ignore-rev on commits with significant change in content. The conversion from reStructuredText to Markdown alone is significant enough for me to be reluctant to ignoring them. The inclusion of "Sync with" commits that restore git blame history, which may also contain important spec changes (e.g. a288630) unrelated to Markdown conversion, further strengthens my reluctance.

Examples of the concerning line-based git blame behavior with ignored refs include (note: use of ~ suffix for hash in URL to disable ignoring revs):

I recommend other reviewers examine the quality of line-based git blame history of affected files when considering the proposed changes in this PR.

@@ -0,0 +1,39 @@
# DRIVERS-2789
ca372f0c6a1353742437d65131b37544492692c2 # Sync with 4b7d7573 (2024-02-26)
Copy link
Contributor

@eramongodb eramongodb Mar 1, 2024

Choose a reason for hiding this comment

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

GitHub's syntax highlighter/linter seems to dislike trailing comments (highlighting lines red), but I believe it should be fine. Per fsck.skipList documentation (the format used by --ignore-revs-file):

On versions of Git 2.20 and later, comments (#), empty lines, and any leading and trailing whitespace are ignored. Everything but a SHA-1 per line will error out on older versions.

And per v2.20 source code:

Allow trailing comments, leading whitespace (including before commits), and empty or whitespace only lines.

If this is undesirable and we want to make GitHub's syntax highlighter/linter happy, we can use git log -n 1 --format=format:"# %s (%cs)%n%H%n%n" instead:

# Sync with 4b7d7573 (2024-02-26)
ca372f0c6a1353742437d65131b37544492692c2

# Sync with 4b7d7573 (2024-02-26)
634d6399bc166d27f290599d8facf034fa17be91

# Sync with 4b7d7573 (2024-02-26)
405e736abf6884c69669fcc060ef76c44f2e63e7

...

However, I recommend sticking with the current format that uses trailing comments.

@blink1073
Copy link
Member

I agree with @eramongodb's assessment.

@jyemin jyemin removed the request for review from dariakp March 4, 2024 18:55
@katcharov
Copy link
Contributor Author

Alright, I'll close this PR. (The file can at least be applied locally.)

@katcharov katcharov closed this Mar 7, 2024
@katcharov katcharov deleted the DRIVERS-2789-ignore-revs branch April 10, 2024 14:05
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