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

rewrite --insert "GitBlameInformation" throws NullReferenceException #2683

Closed
AvremelM opened this issue Jun 14, 2023 · 1 comment
Closed

Comments

@AvremelM
Copy link
Collaborator

AvremelM commented Jun 14, 2023

Issue:

NullReferenceException thrown when trying to insert blame information via sarif rewrite in.sarif --insert "GitBlameInformation"

Stacktrace
System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.CodeAnalysis.Sarif.Visitors.SarifTransformerUtilities.ParseBlameInformation(String blameText) in [...]\sarif-sdk\src\Sarif\Visitors\SarifTransformerUtilities.cs:line 329
   at Microsoft.CodeAnalysis.Sarif.Visitors.InsertOptionalDataVisitor.VisitResult(Result node) in [...]\sarif-sdk\src\Sarif\Visitors\InsertOptionalDataVisitor.cs:line 215
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitActual(ISarifNode node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 120
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.Visit(ISarifNode node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 28
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitNullChecked[T](T node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 167
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitRun(Run node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 1117
   at Microsoft.CodeAnalysis.Sarif.Visitors.InsertOptionalDataVisitor.VisitRun(Run node) in [...]\sarif-sdk\src\Sarif\Visitors\InsertOptionalDataVisitor.cs:line 96
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitActual(ISarifNode node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 124
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.Visit(ISarifNode node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 28
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitNullChecked[T](T node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 167
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitSarifLog(SarifLog node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 1211
   at Microsoft.CodeAnalysis.Sarif.Multitool.RewriteCommand.Run(RewriteOptions options) in [...]\sarif-sdk\src\Sarif.Multitool.Library\RewriteCommand.cs:line 61

Details

Issue seems to be two-fold:

  1. GitHelper.GetSimpleGitCommandOutput(string repoPath, ...) requires the repoPath argument to be the repository root (here), but GitHelper.GetBlame(...) passes in the full directory path to the file here. So GetBlame(...) will return null for any file that isn't in the repo's root.
  2. SarifTransformerUtilities.ParseBlameInformation(...) doesn't handle null results from _gitHelper.GetBlame(filePath) here

Solution

Simplest solution seems to be to modify GetBlame(...)
from

return GetSimpleGitCommandOutput(
    Path.GetDirectoryName(filePath),
    args: $"blame -f --porcelain {Path.GetFileName(filePath)}",
    trimLines: false);
return GetSimpleGitCommandOutput(
    GetTopLevel(filePath),
    args: $"blame -f --porcelain {filePath}",
    trimLines: false);

(Modifying GetSimpleGitCommandOutput() to remove the IsRepositoryRoot() check would also work, but obviously that may negatively affect other callers)

If it helps, I'm happy to open a PR.

AvremelM added a commit to AvremelM/sarif-sdk that referenced this issue Jun 14, 2023
@AvremelM
Copy link
Collaborator Author

Fixed in #2686

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 a pull request may close this issue.

1 participant