search

squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

Fix gitblame to not use full path when `chdir` #3809

Closed datengraben closed 1 year ago

datengraben commented 1 year ago

Thanks for the great Tool!

When invoking with --report=gitblame most or all of the found sniffs do not show up with their author to blame. Instead the report table is empty.

The problem is the following: In #1249 chdir was introduced before the call to git blame, which was to change into a directory to resolve the .git-root-dir more easily. For example when the phpcs command is not started from a proper Git directory.

I'm not sure what happened since #1249 that --report=gitblame won't work, but it seemed to work that time. But now GitBlame.php requests the full path file, but from within the directory of that file.

I did not dig deeper why it seemed to work at that time. Anyway I'm sure it works now, tested it on my repos locally.

If you want I can attach a more detailed issue.

jrfnl commented 1 year ago

@datengraben Thanks for this PR.

I've tested with both the PHPCS master branch as well as with this PR branch and I cannot reproduce the issue. The reports I see are the same, so at the very least I can say that this change doesn't seem to cause any regressions.

To properly evaluate this PR, I believe more information is needed to reproduce the issue this is trying to solve.

What and how I tested this

Tested with a git clone of this repo on Windows 10.

Take note that I used the PEAR standard on purpose as that would yield some errors to run "blame" on.

Scenario 1: PHPCS run from project root

From the root directory of the PHPCS repo, I ran the following command first with the master branch, then with the datengraben/master branch (this PR):

phpcs -psl ./src --standard=PEAR --report=gitblame

Scenario 2: PHPCS run from a subdirectory under the project root

I changed into the src directory of the project.

From the src directory of the PHPCS repo, I ran the following command first with the master branch, then with the datengraben/master branch (this PR):

phpcs -psl . --standard=PEAR --report=gitblame

In all four cases, the output is exactly the same for me (see details in the fold-out).

Output ``` PHP CODE SNIFFER GIT BLAME SUMMARY --------------------------------------------------------------------------------------------------- AUTHOR SOURCE (Author %) (Overall %) COUNT --------------------------------------------------------------------------------------------------- Greg Sherwood (4.41) (79.86) 230 Generic.Files.LineLength.TooLong 183 PEAR.NamingConventions.ValidVariableName.PrivateNoUnderscore 16 PEAR.NamingConventions.ValidFunctionName.PrivateNoUnderscore 7 PEAR.Commenting.FileComment.MissingLinkTag 5 PEAR.Commenting.FileComment.MissingPackageTag 5 PEAR.Commenting.FileComment.MissingCategoryTag 5 PEAR.Commenting.FileComment.MissingVersion 5 PEAR.Commenting.ClassComment.Missing 4 jrfnl (19.63) (11.11) 32 Generic.Files.LineLength.TooLong 32 Vladyslav Startsev (38.64) (7.29) 21 Generic.Files.LineLength.TooLong 16 PEAR.Commenting.ClassComment.MissingLinkTag 1 PEAR.Commenting.ClassComment.MissingLicenseTag 1 PEAR.Commenting.ClassComment.MissingAuthorTag 1 PEAR.Commenting.ClassComment.MissingPackageTag 1 PEAR.Commenting.ClassComment.MissingCategoryTag 1 Willem Stuursma (20) (0.35) 1 PEAR.NamingConventions.ValidVariableName.PrivateNoUnderscore 1 Matthew Peveler (12.5) (0.35) 1 Generic.Files.LineLength.TooLong 1 Klaus Purer (20) (0.35) 1 Generic.Files.LineLength.TooLong 1 Michael Moravec (14.29) (0.35) 1 Generic.Files.LineLength.TooLong 1 webimpress (2.44) (0.35) 1 Generic.Files.LineLength.TooLong 1 --------------------------------------------------------------------------------------------------- A TOTAL OF 288 SNIFF VIOLATIONS WERE COMMITTED BY 8 AUTHORS --------------------------------------------------------------------------------------------------- ```
jrfnl commented 1 year ago

Ha! I ran into something similar the other day. I've just tested and even though this PR doesn't solve my issue, I can now see a case which it does solve.

As far as I can see, the problem is the combination with the --basepath=... directive.

@datengraben Could you confirm you are using the basepath directive in your project ? (either from the command-line or set within the ruleset ?

In that case, the usecase for accepting this fix would be the following:

Scenario 1: PHPCS run from project root

phpcs -psl ./src --standard=PEAR --report=gitblame --basepath=.

Output when using the branch-off point (3.7.2 tag):

PHP CODE SNIFFER GIT BLAME SUMMARY
---------------------------------------------------------------------------------------------------
AUTHOR   SOURCE                                                        (Author %) (Overall %) COUNT
---------------------------------------------------------------------------------------------------
Unknown                                                                   (96.42)       (100)   288
         Generic.Files.LineLength.TooLong                                                       235
         PEAR.NamingConventions.ValidVariableName.PrivateNoUnderscore                            17
         PEAR.NamingConventions.ValidFunctionName.PrivateNoUnderscore                             7
         PEAR.Commenting.FileComment.MissingLinkTag                                               5
         PEAR.Commenting.FileComment.MissingPackageTag                                            5
         PEAR.Commenting.FileComment.MissingCategoryTag                                           5
         PEAR.Commenting.FileComment.MissingVersion                                               5
         PEAR.Commenting.ClassComment.Missing                                                     4
         PEAR.Commenting.ClassComment.MissingLinkTag                                              1
         PEAR.Commenting.ClassComment.MissingLicenseTag                                           1
         PEAR.Commenting.ClassComment.MissingAuthorTag                                            1
         PEAR.Commenting.ClassComment.MissingPackageTag                                           1
         PEAR.Commenting.ClassComment.MissingCategoryTag                                          1
---------------------------------------------------------------------------------------------------
A TOTAL OF 288 SNIFF VIOLATIONS WERE COMMITTED BY 1 AUTHOR
---------------------------------------------------------------------------------------------------

Output when using the PR branch:

PHP CODE SNIFFER GIT BLAME SUMMARY
---------------------------------------------------------------------------------------------------
AUTHOR   SOURCE                                                        (Author %) (Overall %) COUNT
---------------------------------------------------------------------------------------------------
Greg Sherwood                                                              (4.41)     (79.86)   230
         Generic.Files.LineLength.TooLong                                                       183
         PEAR.NamingConventions.ValidVariableName.PrivateNoUnderscore                            16
         PEAR.NamingConventions.ValidFunctionName.PrivateNoUnderscore                             7
         PEAR.Commenting.FileComment.MissingLinkTag                                               5
         PEAR.Commenting.FileComment.MissingPackageTag                                            5
         PEAR.Commenting.FileComment.MissingCategoryTag                                           5
         PEAR.Commenting.FileComment.MissingVersion                                               5
         PEAR.Commenting.ClassComment.Missing                                                     4
jrfnl                                                                     (19.63)     (11.11)    32
         Generic.Files.LineLength.TooLong                                                        32
Vladyslav Startsev                                                        (38.64)      (7.29)    21
         Generic.Files.LineLength.TooLong                                                        16
         PEAR.Commenting.ClassComment.MissingLinkTag                                              1
         PEAR.Commenting.ClassComment.MissingLicenseTag                                           1
         PEAR.Commenting.ClassComment.MissingAuthorTag                                            1
         PEAR.Commenting.ClassComment.MissingPackageTag                                           1
         PEAR.Commenting.ClassComment.MissingCategoryTag                                          1
Willem Stuursma                                                              (20)      (0.35)     1
         PEAR.NamingConventions.ValidVariableName.PrivateNoUnderscore                             1
Matthew Peveler                                                            (12.5)      (0.35)     1
         Generic.Files.LineLength.TooLong                                                         1
Klaus Purer                                                                  (20)      (0.35)     1
         Generic.Files.LineLength.TooLong                                                         1
Michael Moravec                                                           (14.29)      (0.35)     1
         Generic.Files.LineLength.TooLong                                                         1
webimpress                                                                 (2.44)      (0.35)     1
         Generic.Files.LineLength.TooLong                                                         1
---------------------------------------------------------------------------------------------------
A TOTAL OF 288 SNIFF VIOLATIONS WERE COMMITTED BY 8 AUTHORS
---------------------------------------------------------------------------------------------------
jrfnl commented 1 year ago

FYI: I've opened #3854 for the issue I ran into, which led me to discovering what this PR solves. PR #3855 should fix that one (and probably fixes this issue too).

jrfnl commented 1 year ago

@datengraben Have you had a chance to check this ?

Could you confirm you are using the basepath directive in your project ? (either from the command-line or set within the ruleset ?

datengraben commented 1 year ago

@datengraben Have you had a chance to check this ?

Could you confirm you are using the basepath directive in your project ? (either from the command-line or set within the ruleset ?

@jrfnl Yes I can confirm, we are using basepath="./" from the ruleset.

jrfnl commented 1 year ago

@datengraben Thanks for confirming. In that case, I'll merge this now.

jrfnl commented 10 months ago

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).