Closed katcharov closed 7 months ago
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. https://github.com/mongodb/specifications/commit/a2886304afb006cc39808cd67a3503cc1b221116) 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):
.rst
file -> select annotated revision which broke git blame history -> f017041 -> blame the .rst
file from the parent commit (prior to deletion) -> bcde8cdI recommend other reviewers examine the quality of line-based git blame history of affected files when considering the proposed changes in this PR.
I agree with @eramongodb's assessment.
Alright, I'll close this PR. (The file can at least be applied locally.)
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.