search

google / git-appraise

Distributed code review system for Git repos
Apache License 2.0
5.13k stars 146 forks source link

panic while execute accept command #73

Closed marryton007 closed 7 years ago

marryton007 commented 7 years ago

Base on the README.md file, while execute 'accept' command it panic:

/tmp/appraise: git appraise accept 41592 panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0xa0 pc=0x121de63]

goroutine 1 [running]: github.com/google/git-appraise/review.GetSummary(0x13d8f20, 0xc4200771f0, 0x7fff5fbff1f4, 0x5, 0xc42004fd00, 0x0, 0x0) /Users/jiaxi/code/golang/src/github.com/google/git-appraise/review/review.go:245 +0x173 github.com/google/git-appraise/review.Get(0x13d8f20, 0xc4200771f0, 0x7fff5fbff1f4, 0x5, 0x0, 0x0, 0x0) /Users/jiaxi/code/golang/src/github.com/google/git-appraise/review/review.go:286 +0x4d github.com/google/git-appraise/commands.acceptReview(0x13d8f20, 0xc4200771f0, 0xc42007c0b0, 0x1, 0x1, 0x100d374, 0x12b7601) /Users/jiaxi/code/golang/src/github.com/google/git-appraise/commands/accept.go:48 +0xb7 github.com/google/git-appraise/commands.glob..func4(0x13d8f20, 0xc4200771f0, 0xc42007c0b0, 0x1, 0x1, 0xc420064d00, 0x9) /Users/jiaxi/code/golang/src/github.com/google/git-appraise/commands/accept.go:93 +0x53 github.com/google/git-appraise/commands.(*Command).Run(0x13f60c0, 0x13d8f20, 0xc4200771f0, 0xc42007c0b0, 0x1, 0x1, 0x0, 0xc42007cdb0) /Users/jiaxi/code/golang/src/github.com/google/git-appraise/commands/commands.go:39 +0x5c main.main() /Users/jiaxi/code/golang/src/github.com/google/git-appraise/git-appraise/git-appraise.go:100 +0x38c

ojarjur commented 7 years ago

First off, thanks for taking the time to report the issue. I appreciate your help in improving the tool.

Secondly, I'm sorry that you hit this. Looking at the line where the panic occurs, I think the root cause is a bug on this line.

In that line of code, the tool is trying to find the most recent review request from the git notes on the specified object (The one with a hash starting with 41592). It does not find any matching review requests, so it cannot return the latest one, but it also does not return an error.

The calling code that invokes this method assumes that either one of the review summary or the error will be non-nil, but the code returns nil for both values. That is clearly a bug, and I will look to fix it (probably by making that line return a non-nil error indicating that there were no matching reviews).

Digging a little deeper, I think the repo.GetNotes method's API may be poorly chosen. It does not return any errors that occur when trying to read the notes for a specified object.

I also notice that the prefix you are giving is very short (just 5 characters), so it is much more likely to have conflicts (multiple objects that could be identified by that prefix).

Here is what I think happened:

  1. You ran the given command specifying a 5-character hash prefix for the review
  2. The tool then invoked the git command to read the notes for that object
  3. The git command returned an error reporting that the object name was ambiguous, so it didn't know which of the matching objects we wanted the notes for.
  4. The tool ignored that error and just continued on as though there were no notes (this is a bug)
  5. The tool then hit the bug where it tries to parse the review even when there are no review requests (this is also a bug).
  6. That resulted in the panic you saw.

So, in summary, there are two bugs that will need to be fixed.

As a work-around, try re-running the command with a longer hash prefix (for instance, use the full 12-character prefix that git appraise list will print).

marryton007 commented 7 years ago

/tmp/appraise(✔) git appraise accept f38f panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0xa0 pc=0x121de63]

goroutine 1 [running]: github.com/google/git-appraise/review.GetSummary(0x13d8f20, 0xc4200771f0, 0x7fff5fbff1f4, 0x4, 0xc42004fd00, 0x0, 0x0) /Users/jiaxi/code/golang/src/github.com/google/git-appraise/review/review.go:245 +0x173 github.com/google/git-appraise/review.Get(0x13d8f20, 0xc4200771f0, 0x7fff5fbff1f4, 0x4, 0x0, 0x0, 0x0) /Users/jiaxi/code/golang/src/github.com/google/git-appraise/review/review.go:286 +0x4d github.com/google/git-appraise/commands.acceptReview(0x13d8f20, 0xc4200771f0, 0xc42007c0b0, 0x1, 0x1, 0x100d374, 0x12b7601) /Users/jiaxi/code/golang/src/github.com/google/git-appraise/commands/accept.go:48 +0xb7 github.com/google/git-appraise/commands.glob..func4(0x13d8f20, 0xc4200771f0, 0xc42007c0b0, 0x1, 0x1, 0xc420064d00, 0x9) /Users/jiaxi/code/golang/src/github.com/google/git-appraise/commands/accept.go:93 +0x53 github.com/google/git-appraise/commands.(*Command).Run(0x13f60c0, 0x13d8f20, 0xc4200771f0, 0xc42007c0b0, 0x1, 0x1, 0x0, 0xc42007cdb0) /Users/jiaxi/code/golang/src/github.com/google/git-appraise/commands/commands.go:39 +0x5c main.main() /Users/jiaxi/code/golang/src/github.com/google/git-appraise/git-appraise/git-appraise.go:100 +0x38c /tmp/appraise(✔) git appraise accept f83fa03814f0 /tmp/appraise(✔)

the work-around is effectively.