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.

Sign up for GitHub

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

Jump to bottom

Fix gitblame to not use full path when chdir #3809

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Fix gitblame to not use full path when chdir #3809

merged 1 commit into from
Aug 1, 2023

Conversation

datengraben
Copy link
Contributor

@datengraben datengraben commented Apr 21, 2023 •
edited
Loading

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.

@datengraben
Reports/GitBlame: Fix invocation path of git blame
af44889 
After we change into the files directory, invocation of git blame with full path will end in fatal error.
@jrfnl
Copy link
Contributor

jrfnl commented Apr 23, 2023

@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
Copy link
Contributor

jrfnl commented Jul 13, 2023

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 jrfnl mentioned this pull request Jul 13, 2023
Closed
3 tasks
@jrfnl jrfnl added this to the 3.8.0 milestone Jul 13, 2023
@jrfnl
Copy link
Contributor

jrfnl commented Jul 13, 2023 •
edited
Loading

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 jrfnl mentioned this pull request Jul 13, 2023
Closed
5 tasks
@jrfnl
Copy link
Contributor

jrfnl commented Aug 1, 2023

@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
Copy link
Contributor Author

datengraben commented Aug 1, 2023 •
edited
Loading

@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
Copy link
Contributor

jrfnl commented Aug 1, 2023

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

@jrfnl jrfnl merged commit 50e1fb5 into squizlabs:master Aug 1, 2023
jrfnl added a commit that referenced this pull request Aug 1, 2023
@jrfnl
Changelog for #3809
7a733cd
@jrfnl jrfnl mentioned this pull request Nov 10, 2023
Merged
1 task
@jrfnl jrfnl mentioned this pull request Dec 5, 2023
Closed
@jrfnl
Copy link
Contributor

jrfnl commented Dec 8, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrfnl jrfnl jrfnl approved these changes

Assignees
No one assigned
Labels
Core Component: Reports Type: bug
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.
2 participants
@datengraben @jrfnl