google / git-appraise

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

Unable to Create a Request #10

Closed m4dc4p closed 8 years ago

m4dc4p commented 8 years ago

Downloaded and installed using "go get". On an existing git repo, every reasonable invocation of "git appraise request" results in "exit code 128".

For example, using the "git-appraise" repo as an example (thanks to Y-combinator user ktt for showing how to fetch notes https://news.ycombinator.com/item?id=10733953):

$ git appraise request -source remotes/origin/ojarjur/improve-error-messages -target origin/master
exit status 128

I'm using git 1.8.2 on CentOS 6.6. Tried both Go 1.5.1 and 1.5.2 with the same results.

Any suggestions?

I love the idea of this and really want to check it out!

ojarjur commented 8 years ago

Sorry you hit that; I have a pending change (2c9bff89f0f8) that will fix the bad error message and tell you what is actually wrong.

In this particular case, I suspect the issue is that "origin/master" is not a known ref. Changing that to "refs/remotes/origin/master" will probably fix it (although that's also going to cause issues later on since "refs/remotes/origin/master" is not a local ref). What you really want for the target is "refs/heads/master", but that is the default.

m4dc4p commented 8 years ago

Awesome! I switched to your branch and, indeed, git-appraise complained that I did not specify a valid ref.

However, now I can't get past the "There are no commits included in the review request." Again, using the git-appraise repo (and while on the 'improve-error-messages' branch):

$ git appraise request -source refs/heads/ojarjur/improve-error-messages -target refs/heads/master
There are no commits included in the review request

I also tried to create a request with a brand-new branch (where I just added a zero-byte file) and had the same result.

ojarjur commented 8 years ago

Did you commit the addition of the zero-byte file?

The workflow we use is usually:

  1. Create a feature branch (e.g. "ojarjur/my-feature"): git checkout -b ojarjur/my-feature
  2. Write some code...
  3. Commit the changes: git commit -a -m "My new awesome feature"
  4. Request a review: git appraise request -r "reviewer1,reviewer2,..."
  5. Push the feature branch: git push --set-upstream origin ojarjur/my-feature
  6. Pull any conflicting updates to reviews: git appraise pull
  7. Push the new review request: git appraise push
m4dc4p commented 8 years ago

Yep, committed the file. The only thing I didn't do is include reviewers - but that doesn't seem to help. Is there any sort of validation or enforcement that might be failing when I specify a reviewer? Hopefully this helps:

$ git branch
* foo/bar
  master
  ojarjur/improve-error-messages

$ git log --stat
commit 2d3720196c71953b246d7949af38f7bd2ab5881a
Author: ...
Date:   Wed Dec 16 16:27:11 2015 -0800

    Test

 bar | 0
 1 file changed, 0 insertions(+), 0 deletions(-)

commit edb8c562e766adc574a90ddbbdf52dbf99fa2d49
Author: Omar Jarjur <ojarjur@google.com>
Date:   Tue Dec 15 16:34:34 2015 -0800

    Changed to allow callers who want to check for errors of type exec.ExitError to continue to do so

 repository/git.go | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

...

$ git appraise request -r ojarjur@google.com
There are no commits included in the review request

$ git appraise request -target refs/heads/master -source refs/heads/foo/bar -r ojarjur@google.com -r ojarjur@google.com
There are no commits included in the review request

$ git appraise request -source refs/heads/foo/bar  -target refs/heads/master -r ojarjur@google.com
There are no commits included in the review request

$ git appraise request -source refs/heads/foo/bar -r ojarjur@google.com
There are no commits included in the review request

$ git appraise request -target refs/heads/master -r ojarjur@google.com
There are no commits included in the review request
ojarjur commented 8 years ago

The line it's hitting is this one

What that's doing is "git rev-list --reverse --ancestry-path refs/heads/master..refs/heads/foo/bar"

I think what's going wrong is that there isn't an ancestry-path from refs/heads/foo/bar to refs/heads/master, since refs/heads/foo/bar is no longer a fast-forward of refs/heads/master (they diverged).

This is a bug. Instead we should either:

  1. Have the request command tell you to merge the target ref and then try again. or
  2. Include all of the commits from the merge-base of the target and source to the source (e.g. "git rev-list --reverse --ancestry-path git merge-base $TARGET $SOURCE..$SOURCE" instead of "git rev-list --reverse --ancestry-path $TARGET..$SOURCE")

As a work-around, you can do #1 (git merge master), and then it should work.

ojarjur commented 8 years ago

Commit 8cb887077783 should fix this.

m4dc4p commented 8 years ago

Looks like its working!