rejecting/closing a request?

[nicwest]

Hey team, what is the best approach to reject/close a request?

An argument could be made that keeping a history of comments on rejected requests might be useful so that mistakes are not repeated.

[barbastan]

I don't have the answer; I'm new to the team. Our Tech Lead is on vacation returning Jan 4th. He's the best person to answer this but I'll drill down on Monday and see if I can get you an answer. However, it might be best to wait for him. Will get back to you soon. I know he is strongly advocates keeping history.

Thanks for your question and interest!

Barbara

On Fri, Dec 18, 2015 at 7:21 PM, Nic West notifications@github.com wrote:

Hey team, what is the best approach to reject/close a request?

An argument could be made that keeping a history of comments on rejected requests might be useful so that mistakes are not repeated.

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/issues/17.

[hazbo]

A requested review can be rejected by just passing the -nmw (needs more work) flag in a comment to that particular request. There is an accept subcommand there, but no reject subcommand. In essence all the accept subcommand is, is a shortcut to leaving a comment with the -lgtm (looks good to me) flag added.

Comments can still be added to a rejected review - as far as I know the history of these is maintained.

The -nmw flag requires a message, as we agreed that it's pretty useless suggesting that more work needed to be done, but without providing any feedback. That being the case, the review will be rejected.

I feel it wouldn't be a bad idea to create the reject subcommand to work in pretty much the same way as the accept one works, only it must require a comment and then it'd reject the review.

Any thoughts on this?

[barbastan]

Sounds like a neat idea to me. I'll check with our Tech Lead when he's back on Jan. 4th.

Thanks so much!

Barbara

On Sat, Dec 19, 2015 at 11:18 AM, Harry Lawrence notifications@github.com wrote:

A requested review can be rejected by just passing the -nmw (needs more work) flag in a comment to that particular request. There is an accept subcommand there, but no reject subcommand. In essence all the accept subcommand is, is a shortcut to leaving a comment with the -lgtm (looks good to me) flag added.

Comments can still be added to a rejected review - as far as I know the history of these is maintained.

The -nmw flag requires a comment, as we agreed that it's pretty useless suggesting that more work needed to be done, but without providing any feedback. That being the case, the review will be rejected.

I feel it wouldn't be a bad idea to create the reject subcommand to work in pretty much the same way as the accept one works, only it must require a comment and then it'd reject the review.

Any thoughts on this?

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/issues/17#issuecomment-166017185.

[jishi9]

Just a clarification about the design of lgtm/nmw, please correct me if I'm wrong.

I am not sure this is what @nicwest meant:

what is the best approach to reject/close a request

Is this not about rejecting and discarding (still kept in history, but not to be submitted anymore) an entire review request?

However, if we are talking about rejecting an individual comment thread, then read below.

The proposed reject command would be used to add a comment with a resolved status of nmw. Presumably after some code changes and/or further replies to the comment, it will eventually be accepted.

In that case, should the command not be called some synonym of create issue / report issue rather than reject?

[hazbo]

I noticed @nicwest had been working on a vim plugin for git-appraise so I has assumed he might have been asking about how we may solve interfacing closing / rejection as a subcommand, much like how the accept subcommand works. I could be wrong here (so @nicwest please point give me a shout if I've missed the point!)

@jishi9 as far I understand, after a review thread has been rejected it then can't be accepted afterwards. Create / report issue makes perfect sense given that the review state can be changed from rejected to accepted but I don't think this is (at least currently) how it works, although @ojarjur would be the best person to further clarify this.

[barbastan]

Yes, Omar would definitely be the best person to clarify this.

I was assuming that the code review thread could contain something like:

• I submit a code review.
• You think it's crap; give me lots of comments and reject it or NMW. (Those 2 are equivalent, right?)
• I clean up my code and fix all of your comments.
• I send it back for your review.
• You think of a few more things you'd like me to fix. NMW or reject again.
• I fix those and send it back to you for review.
• You approve.
• I submit the code.

All of this within one code review thread. Is your understanding different?

Thanks! Barbara

On Tue, Dec 22, 2015 at 4:04 PM, Harry Lawrence notifications@github.com wrote:

I noticed @nicwest https://github.com/nicwest had been working on a vim plugin for git-appraise so I has assumed he might have been asking about how we may solve interfacing closing / rejection as a subcommand, much like how the accept subcommand works. I could be wrong here (so @nicwest https://github.com/nicwest please point give me a shout if I've missed the point!)

@jishi9 https://github.com/jishi9 as far I understand, after a review thread has been rejected it then can't be accepted afterwards. Create / report issue makes perfect sense given that the review state can be changed from rejected to accepted but I don't think this is (at least currently) how it works, although @ojarjur https://github.com/ojarjur would be the best person to further clarify this.

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/issues/17#issuecomment-166765592.

[jishi9]

You think it's crap; give me lots of comments and reject it or NMW. (Those 2 are equivalent, right?)

To me 'reject' means something like a) this change is so bad it needs to be done over from scratch b) this change is not applicable anymore. This is what I think @nicwest means, something like: git appraise reject -m 'this change was already merged in another review, so it is not needed'

For the workflow you described, I would use something like git appraise report-issue (hopefully a command name nicer than that) as a synonym for git appraise comment -nmw.

I also am not sure about where a NMW is applied. Is it to individual comments, or to the review as a whole? In the case of individual comments, I mean something like this:

Filename: test.go ; Line: 13 comment1: Please change the variable name (resolved = NMW) comment2: (Reply to comment1): Ok, changed comment3: (Reply to comment2): (resolved = LGTM)

The above comments would be created using the commands:

1. git appraise report-issue -f test.go -l 13 -m 'Please change the variable name' OR git appraise comment -nmw -f test.go -l 13 -m 'Please change the variable name'
2. git appraise comment -p comment1 -m 'Ok, changed'
3. git appraise comment -p comment2 -lgtm

Or did you instead mean a workflow like this:

• Add multiple inline comments git appraise comment -f test.go -l 13, -l 20, etc
• Add a review-global NMW comment git appraise comment -nmw
• You address my comments
• Add another review-global LGTM comment overriding my previous NMW git appraise comment -lgtm AKA git appraise accept

[barbastan]

Ahhhh... If "reject" means the code needs to be redone from scratch, I totally see your point. I thought "reject" was a shortcut for "NMW".

I thought NMW applies to the entire code review -- not just individual comments.

Thanks!

Barbara

On Tue, Dec 22, 2015 at 5:07 PM, jishi9 notifications@github.com wrote:

You think it's crap; give me lots of comments and reject it or NMW. (Those 2 are equivalent, right?)

To me 'reject' means something like a) this change is so bad it needs to be done over from scratch b) this change is not applicable anymore. This is what I think @nicwest https://github.com/nicwest means, something like: git appraise reject -m 'this change was already merged in another review, so it is not needed'

For the workflow you described, I would use something like git appraise report-issue (hopefully a command name nicer than that) as a synonym for git appraise comment -nmw.

I also am not sure about where a NMW is applied, to individual comments, or to the review as a whole? In the case of individual comments, I mean something like this:

Filename: test.go ; Line: 13 comment1: Please change the variable name (resolved = NMW) comment2: (Reply to comment1): Ok, changed comment3: (Reply to comment2): (resolved = LGTM)

The above comments would be created using the commands:

1. git appraisal report-issue -f test.go -l 13 -m 'Please change the variable name' OR git appraisal comment -nmw -f test.go -l 13 -m 'Please change the variable name'
2. git appraisal comment -p comment1 -m 'Ok, changed'
3. git appraisal comment -p comment2 -lgtm

Or did you instead mean a workflow like this:

• Add multiple inline comments git appraisal comment -f test.go -l 13, -l 20, etc
• Add a review-global NMW comment git appraisal comment -nmw
• You address my comments
• Add another review-global LGTM comment overriding my previous NMW git appraisal comment -lgtm AKA git appraisal accept

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/issues/17#issuecomment-166771444.

[jishi9]

I thought NMW applies to the entire code review -- not just individual comments.

Yes exactly. It seems that comments can be applied at the review level, file level, or line level. lgtm and nmw are applied to comments, and hence they can be applied to any of the three scopes.

https://github.com/google/git-appraise/blob/master/commands/comment.go#L126

[hazbo]

When a new comment is left and the -p (parent comment) flag has not been specified, that comment starts a new thread for the review in question. Comments can specify a file and if a file has been specified a line can also be added, using both the -f and the -l flags.

If a file and line has been specified in a comment where the parent comment (-p) has also been set, the line will not show in the comment when running git appraise show hash. If the comment starts a new thread, the line of code will show, just above the comment.

The -nmw and -lgtm flags are ways of accepting or rejecting the review, through means of a comment (although there is already an accept subcommand that could be used instead). To my understanding, -lgtm and -nmw do not apply for all scopes of comments. They must be the first comment of a new thread to have either the accepted / rejected status apply to the review. You can test this out by specifying both -p and -lgtm in a comment.

A -nmw or -lgtm apply to the whole review, and not a single comment or thread. The review in question will change from pending to either rejected or accepted.

To my understanding this is how it all works currently. Does this make sense?

Thanks.

[nicwest]

the -nmw flag seems to provide the functionality I was thinking of, it even displays the status rejected in the list command.

If there is effectively a helper command for accepting a request via the -lgtm flag then the proposed reject command as a helper for -nmw seems like a logical addition.

[hazbo]

If there is effectively a helper command for accepting a request via the -lgtm flag then the proposed reject command as a helper for -nmw seems like a logical addition.

I agree with this. I think it's a fairly consistent approach with what is already there.

[hazbo]

@jishi9 I think your approach in regards to report-issue is reasonable, although IMO it should remain separate to -nmw. It's a perfectly valid point you make, to suggest improvements etc... without having the whole review being rejected. This doesn't exist yet but perhaps it would be a good idea to raise a new issue with this in mind.

A practical example of where this kind of thing has already happened with this project in particular is where I wrote the code to load the default git editor, Omar was happy with it, although pointed out that I should change one small part of it. He didn't reject the whole review, just left a comment in regards to what should be changed and then accepted it.

What are your thoughts on this?

[jishi9]

@hazbo

To my understanding, -lgtm and -nmw do not apply for all scopes of comments. They must be the first comment of a new thread to have either the accepted / rejected status apply to the review. You can test this out by specifying both -p and -lgtm in a comment.

I checked this, and it looks like you are able to specify -lgtm and -nmw for any comment scope, with or without parents.

N.B. it looks like git appraise show has a bug where status is always listed as status: fyi. Use git appraise show -json instead, and check the resolved property. I have raised issue #22 for this.

A -nmw or -lgtm apply to the whole request, and not a single comment of thread. The review in question will change from pending to either rejected or accepted.

I am not 100% clear on how the status of a comment thread, or the status of the entire review, is calculated. For a comment thread I would assume something like if last_comment.status == lgtm then thread.status == lgtm, but again don't really know.

It's a perfectly valid point you make, to suggest improvements etc... without having the whole review being rejected. This doesn't exist yet but perhaps it would be a good idea to raise a new issue with this in mind. What are your thoughts on this?

I think we first need to clarify the model of how a comment thread's status is (or should) calculated, and how a review's status is (or should) be calculated. Once the model is clear, we can then decide what actions make sense to build on top of that.

[hazbo]

I checked this, and it looks like you are able to specify -lgtm and -nmw for any comment scope, with or without parents.

This is what I tried, would you mind trying also and reporting back?

git appraise request
git appraise list
git appraise show [hash]
git appraise comment -m 'First comment' [hash]
git appraise show [hash]
git appraise comment -m 'Second comment' -p [parent-comment-hash] -lgtm [hash]
git appraise show [hash]

It's true, you're able to pass the -lgtm flag here, but I bet if you run that it won't change the status of the review to accepted, as it's not the first comment in that thread.

[hazbo]

I think we first need to clarify the model of how a comment thread's status is (or should) calculated, and how a review's status is (or should) be calculated. Once the model is clear, we can then decide what actions make sense to build on top of that.

Couldn't agree more with this.

[jishi9]

I bet if you run that it won't change the status of the review to accepted, as it's not the first comment in that thread.

You're right. It seems that the status of the review follows the status of the last global comment. Probably still need to check how it interacts with in-file and inline comments.

$ git appraise request
Review requested:
Commit: 34e0051d5745999b3cd721eae428a0678655d0b8
Target Ref: refs/heads/master
Review Ref: refs/heads/test
Message: "adding nums"

$ git appraise list
Loaded 1 open reviews:
[pending] 34e0051d5745
  adding nums

$ git appraise show
[pending] 34e0051d5745
  adding nums
  "refs/heads/test" -> "refs/heads/master"
  reviewers: ""
  requester: "jishi9"
  build status: unknown
  analyses:  No analyses available
  comments (0 threads):

$ git appraise show -json
{
  "revision": "34e0051d5745999b3cd721eae428a0678655d0b8",
  "request": {
    "timestamp": "1450877384",
    "reviewRef": "refs/heads/test",
    "targetRef": "refs/heads/master",
    "requester": "jishi9",
    "description": "adding nums",
    "baseCommit": "2494e54ad5c24056a6775cff58651f595b788484"
  },
  "submitted": false
}

$ git appraise comment -m 'First comment'

$ git appraise show
[pending] 34e0051d5745
  adding nums
  "refs/heads/test" -> "refs/heads/master"
  reviewers: ""
  requester: "jishi9"
  build status: unknown
  analyses:  No analyses available
  comments (1 threads):
    comment: e447c7076adf3fdbca24626338c712eae132595f
      author: jishi9
      time:   Wed Dec 23 13:30:00 GMT 2015
      status: fyi
      First comment

$ git appraise show -json
{
  "revision": "34e0051d5745999b3cd721eae428a0678655d0b8",
  "request": {
    "timestamp": "1450877384",
    "reviewRef": "refs/heads/test",
    "targetRef": "refs/heads/master",
    "requester": "jishi9",
    "description": "adding nums",
    "baseCommit": "2494e54ad5c24056a6775cff58651f595b788484"
  },
  "comments": [
    {
      "hash": "e447c7076adf3fdbca24626338c712eae132595f",
      "comment": {
        "timestamp": "1450877400",
        "author": "jishi9",
        "location": {
          "commit": "34e0051d5745999b3cd721eae428a0678655d0b8"
        },
        "description": "First comment"
      }
    }
  ],
  "submitted": false
}

$ git appraise comment -m 'Second comment' -p e447c7076adf3fdbca24626338c712eae132595f -lgtm

$ git appraise show
[pending] 34e0051d5745
  adding nums
  "refs/heads/test" -> "refs/heads/master"
  reviewers: ""
  requester: "jishi9"
  build status: unknown
  analyses:  No analyses available
  comments (1 threads):
    comment: e447c7076adf3fdbca24626338c712eae132595f
      author: jishi9
      time:   Wed Dec 23 13:30:00 GMT 2015
      status: fyi
      First comment
      comment: 30d63d483975ec0d89723eaa2ded81403656a9b3
        author: jishi9
        time:   Wed Dec 23 13:30:23 GMT 2015
        status: fyi
        Second comment

$ git appraise show -json
{
  "revision": "34e0051d5745999b3cd721eae428a0678655d0b8",
  "request": {
    "timestamp": "1450877384",
    "reviewRef": "refs/heads/test",
    "targetRef": "refs/heads/master",
    "requester": "jishi9",
    "description": "adding nums",
    "baseCommit": "2494e54ad5c24056a6775cff58651f595b788484"
  },
  "comments": [
    {
      "hash": "e447c7076adf3fdbca24626338c712eae132595f",
      "comment": {
        "timestamp": "1450877400",
        "author": "jishi9",
        "location": {
          "commit": "34e0051d5745999b3cd721eae428a0678655d0b8"
        },
        "description": "First comment"
      },
      "children": [
        {
          "hash": "30d63d483975ec0d89723eaa2ded81403656a9b3",
          "comment": {
            "timestamp": "1450877423",
            "author": "jishi9",
            "parent": "e447c7076adf3fdbca24626338c712eae132595f",
            "location": {
              "commit": "34e0051d5745999b3cd721eae428a0678655d0b8"
            },
            "description": "Second comment",
            "resolved": true
          }
        }
      ]
    }
  ],
  "submitted": false
}

$ git appraise comment -lgtm -m 'Third commit'

$ git appraise show
[accepted] 34e0051d5745
  adding nums
  "refs/heads/test" -> "refs/heads/master"
  reviewers: ""
  requester: "jishi9"
  build status: unknown
  analyses:  No analyses available
  comments (2 threads):
    comment: e447c7076adf3fdbca24626338c712eae132595f
      author: jishi9
      time:   Wed Dec 23 13:30:00 GMT 2015
      status: fyi
      First comment
      comment: 30d63d483975ec0d89723eaa2ded81403656a9b3
        author: jishi9
        time:   Wed Dec 23 13:30:23 GMT 2015
        status: fyi
        Second comment
    comment: 54d7872d6f23101976c22da3f812b25ffd4c307c
      author: jishi9
      time:   Wed Dec 23 13:34:12 GMT 2015
      status: fyi
      Third commit

$ git appraise show -json
{
  "revision": "34e0051d5745999b3cd721eae428a0678655d0b8",
  "request": {
    "timestamp": "1450877384",
    "reviewRef": "refs/heads/test",
    "targetRef": "refs/heads/master",
    "requester": "jishi9",
    "description": "adding nums",
    "baseCommit": "2494e54ad5c24056a6775cff58651f595b788484"
  },
  "comments": [
    {
      "hash": "e447c7076adf3fdbca24626338c712eae132595f",
      "comment": {
        "timestamp": "1450877400",
        "author": "jishi9",
        "location": {
          "commit": "34e0051d5745999b3cd721eae428a0678655d0b8"
        },
        "description": "First comment"
      },
      "children": [
        {
          "hash": "30d63d483975ec0d89723eaa2ded81403656a9b3",
          "comment": {
            "timestamp": "1450877423",
            "author": "jishi9",
            "parent": "e447c7076adf3fdbca24626338c712eae132595f",
            "location": {
              "commit": "34e0051d5745999b3cd721eae428a0678655d0b8"
            },
            "description": "Second comment",
            "resolved": true
          }
        }
      ]
    },
    {
      "hash": "54d7872d6f23101976c22da3f812b25ffd4c307c",
      "comment": {
        "timestamp": "1450877652",
        "author": "jishi9",
        "location": {
          "commit": "34e0051d5745999b3cd721eae428a0678655d0b8"
        },
        "description": "Third commit",
        "resolved": true
      }
    }
  ],
  "resolved": true,
  "submitted": false
}

$ git appraise comment -nmw -m 'Fourth commit'

$ git appraise show
[rejected] 34e0051d5745
  adding nums
  "refs/heads/test" -> "refs/heads/master"
  reviewers: ""
  requester: "jishi9"
  build status: unknown
  analyses:  No analyses available
  comments (3 threads):
    comment: e447c7076adf3fdbca24626338c712eae132595f
      author: jishi9
      time:   Wed Dec 23 13:30:00 GMT 2015
      status: fyi
      First comment
      comment: 30d63d483975ec0d89723eaa2ded81403656a9b3
        author: jishi9
        time:   Wed Dec 23 13:30:23 GMT 2015
        status: fyi
        Second comment
    comment: 54d7872d6f23101976c22da3f812b25ffd4c307c
      author: jishi9
      time:   Wed Dec 23 13:34:12 GMT 2015
      status: fyi
      Third commit
    comment: ff43d1354c363f07a2a5f3401b3223a9f51a0ec2
      author: jishi9
      time:   Wed Dec 23 13:39:05 GMT 2015
      status: fyi
      Fourth commit

$ git appraise show -json 
{
  "revision": "34e0051d5745999b3cd721eae428a0678655d0b8",
  "request": {
    "timestamp": "1450877384",
    "reviewRef": "refs/heads/test",
    "targetRef": "refs/heads/master",
    "requester": "jishi9",
    "description": "adding nums",
    "baseCommit": "2494e54ad5c24056a6775cff58651f595b788484"
  },
  "comments": [
    {
      "hash": "e447c7076adf3fdbca24626338c712eae132595f",
      "comment": {
        "timestamp": "1450877400",
        "author": "jishi9",
        "location": {
          "commit": "34e0051d5745999b3cd721eae428a0678655d0b8"
        },
        "description": "First comment"
      },
      "children": [
        {
          "hash": "30d63d483975ec0d89723eaa2ded81403656a9b3",
          "comment": {
            "timestamp": "1450877423",
            "author": "jishi9",
            "parent": "e447c7076adf3fdbca24626338c712eae132595f",
            "location": {
              "commit": "34e0051d5745999b3cd721eae428a0678655d0b8"
            },
            "description": "Second comment",
            "resolved": true
          }
        }
      ]
    },
    {
      "hash": "54d7872d6f23101976c22da3f812b25ffd4c307c",
      "comment": {
        "timestamp": "1450877652",
        "author": "jishi9",
        "location": {
          "commit": "34e0051d5745999b3cd721eae428a0678655d0b8"
        },
        "description": "Third commit",
        "resolved": true
      }
    },
    {
      "hash": "ff43d1354c363f07a2a5f3401b3223a9f51a0ec2",
      "comment": {
        "timestamp": "1450877945",
        "author": "jishi9",
        "location": {
          "commit": "34e0051d5745999b3cd721eae428a0678655d0b8"
        },
        "description": "Fourth commit",
        "resolved": false
      }
    }
  ],
  "resolved": false,
  "submitted": false
}

[barbastan]

+1 to clarifying the model and then documenting it!

Barbara

On Wed, Dec 23, 2015 at 5:31 AM, jishi9 notifications@github.com wrote:

I bet if you run that it won't change the status of the review to accepted, as it's not the first comment in that thread.

You're right. It seems that the status of the review follows the status of the last global comment. Probably still need to check how it interacts with in-file and inline comments.

$ git appraise request Review requested: Commit: 34e0051d5745999b3cd721eae428a0678655d0b8 Target Ref: refs/heads/master Review Ref: refs/heads/test Message: "adding nums"

$ git appraise list Loaded 1 open reviews: [pending] 34e0051d5745 adding nums

$ git appraise show [pending] 34e0051d5745 adding nums "refs/heads/test" -> "refs/heads/master" reviewers: "" requester: "jishi9" build status: unknown analyses: No analyses available comments (0 threads):

$ git appraise show -json { "revision": "34e0051d5745999b3cd721eae428a0678655d0b8", "request": { "timestamp": "1450877384", "reviewRef": "refs/heads/test", "targetRef": "refs/heads/master", "requester": "jishi9", "description": "adding nums", "baseCommit": "2494e54ad5c24056a6775cff58651f595b788484" }, "submitted": false }

$ git appraise comment -m 'First comment'

$ git appraise show [pending] 34e0051d5745 adding nums "refs/heads/test" -> "refs/heads/master" reviewers: "" requester: "jishi9" build status: unknown analyses: No analyses available comments (1 threads): comment: e447c7076adf3fdbca24626338c712eae132595f author: jishi9 time: Wed Dec 23 13:30:00 GMT 2015 status: fyi First comment

$ git appraise show -json { "revision": "34e0051d5745999b3cd721eae428a0678655d0b8", "request": { "timestamp": "1450877384", "reviewRef": "refs/heads/test", "targetRef": "refs/heads/master", "requester": "jishi9", "description": "adding nums", "baseCommit": "2494e54ad5c24056a6775cff58651f595b788484" }, "comments": [ { "hash": "e447c7076adf3fdbca24626338c712eae132595f", "comment": { "timestamp": "1450877400", "author": "jishi9", "location": { "commit": "34e0051d5745999b3cd721eae428a0678655d0b8" }, "description": "First comment" } } ], "submitted": false }

$ git appraise comment -m 'Second comment' -p e447c7076adf3fdbca24626338c712eae132595f -lgtm

$ git appraise show [pending] 34e0051d5745 adding nums "refs/heads/test" -> "refs/heads/master" reviewers: "" requester: "jishi9" build status: unknown analyses: No analyses available comments (1 threads): comment: e447c7076adf3fdbca24626338c712eae132595f author: jishi9 time: Wed Dec 23 13:30:00 GMT 2015 status: fyi First comment comment: 30d63d483975ec0d89723eaa2ded81403656a9b3 author: jishi9 time: Wed Dec 23 13:30:23 GMT 2015 status: fyi Second comment

$ git appraise show -json { "revision": "34e0051d5745999b3cd721eae428a0678655d0b8", "request": { "timestamp": "1450877384", "reviewRef": "refs/heads/test", "targetRef": "refs/heads/master", "requester": "jishi9", "description": "adding nums", "baseCommit": "2494e54ad5c24056a6775cff58651f595b788484" }, "comments": [ { "hash": "e447c7076adf3fdbca24626338c712eae132595f", "comment": { "timestamp": "1450877400", "author": "jishi9", "location": { "commit": "34e0051d5745999b3cd721eae428a0678655d0b8" }, "description": "First comment" }, "children": [ { "hash": "30d63d483975ec0d89723eaa2ded81403656a9b3", "comment": { "timestamp": "1450877423", "author": "jishi9", "parent": "e447c7076adf3fdbca24626338c712eae132595f", "location": { "commit": "34e0051d5745999b3cd721eae428a0678655d0b8" }, "description": "Second comment", "resolved": true } } ] } ], "submitted": false }

$ git appraise comment -lgtm -m 'Third commit'

$ git appraise show [accepted] 34e0051d5745 adding nums "refs/heads/test" -> "refs/heads/master" reviewers: "" requester: "jishi9" build status: unknown analyses: No analyses available comments (2 threads): comment: e447c7076adf3fdbca24626338c712eae132595f author: jishi9 time: Wed Dec 23 13:30:00 GMT 2015 status: fyi First comment comment: 30d63d483975ec0d89723eaa2ded81403656a9b3 author: jishi9 time: Wed Dec 23 13:30:23 GMT 2015 status: fyi Second comment comment: 54d7872d6f23101976c22da3f812b25ffd4c307c author: jishi9 time: Wed Dec 23 13:34:12 GMT 2015 status: fyi Third commit

$ git appraise show -json { "revision": "34e0051d5745999b3cd721eae428a0678655d0b8", "request": { "timestamp": "1450877384", "reviewRef": "refs/heads/test", "targetRef": "refs/heads/master", "requester": "jishi9", "description": "adding nums", "baseCommit": "2494e54ad5c24056a6775cff58651f595b788484" }, "comments": [ { "hash": "e447c7076adf3fdbca24626338c712eae132595f", "comment": { "timestamp": "1450877400", "author": "jishi9", "location": { "commit": "34e0051d5745999b3cd721eae428a0678655d0b8" }, "description": "First comment" }, "children": [ { "hash": "30d63d483975ec0d89723eaa2ded81403656a9b3", "comment": { "timestamp": "1450877423", "author": "jishi9", "parent": "e447c7076adf3fdbca24626338c712eae132595f", "location": { "commit": "34e0051d5745999b3cd721eae428a0678655d0b8" }, "description": "Second comment", "resolved": true } } ] }, { "hash": "54d7872d6f23101976c22da3f812b25ffd4c307c", "comment": { "timestamp": "1450877652", "author": "jishi9", "location": { "commit": "34e0051d5745999b3cd721eae428a0678655d0b8" }, "description": "Third commit", "resolved": true } } ], "resolved": true, "submitted": false }

$ git appraise comment -nmw -m 'Fourth commit'

$ git appraise show [rejected] 34e0051d5745 adding nums "refs/heads/test" -> "refs/heads/master" reviewers: "" requester: "jishi9" build status: unknown analyses: No analyses available comments (3 threads): comment: e447c7076adf3fdbca24626338c712eae132595f author: jishi9 time: Wed Dec 23 13:30:00 GMT 2015 status: fyi First comment comment: 30d63d483975ec0d89723eaa2ded81403656a9b3 author: jishi9 time: Wed Dec 23 13:30:23 GMT 2015 status: fyi Second comment comment: 54d7872d6f23101976c22da3f812b25ffd4c307c author: jishi9 time: Wed Dec 23 13:34:12 GMT 2015 status: fyi Third commit comment: ff43d1354c363f07a2a5f3401b3223a9f51a0ec2 author: jishi9 time: Wed Dec 23 13:39:05 GMT 2015 status: fyi Fourth commit

$ git appraise show -json { "revision": "34e0051d5745999b3cd721eae428a0678655d0b8", "request": { "timestamp": "1450877384", "reviewRef": "refs/heads/test", "targetRef": "refs/heads/master", "requester": "jishi9", "description": "adding nums", "baseCommit": "2494e54ad5c24056a6775cff58651f595b788484" }, "comments": [ { "hash": "e447c7076adf3fdbca24626338c712eae132595f", "comment": { "timestamp": "1450877400", "author": "jishi9", "location": { "commit": "34e0051d5745999b3cd721eae428a0678655d0b8" }, "description": "First comment" }, "children": [ { "hash": "30d63d483975ec0d89723eaa2ded81403656a9b3", "comment": { "timestamp": "1450877423", "author": "jishi9", "parent": "e447c7076adf3fdbca24626338c712eae132595f", "location": { "commit": "34e0051d5745999b3cd721eae428a0678655d0b8" }, "description": "Second comment", "resolved": true } } ] }, { "hash": "54d7872d6f23101976c22da3f812b25ffd4c307c", "comment": { "timestamp": "1450877652", "author": "jishi9", "location": { "commit": "34e0051d5745999b3cd721eae428a0678655d0b8" }, "description": "Third commit", "resolved": true } }, { "hash": "ff43d1354c363f07a2a5f3401b3223a9f51a0ec2", "comment": { "timestamp": "1450877945", "author": "jishi9", "location": { "commit": "34e0051d5745999b3cd721eae428a0678655d0b8" }, "description": "Fourth commit", "resolved": false } } ], "resolved": false, "submitted": false }

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/issues/17#issuecomment-166896379.

[shafreeck]

I think there is another question in this thread: How to close or discard a review request ? I git appraise request create a request and then I regret. I want to cancel or just delete it. How can I achieve this ?

[barbastan]

Thanks for bringing this up. Omar will be back from vacation on Monday. I'm looking forward to hearing his answer on this, along with some questions on other threads.

Barbara

On Wed, Dec 30, 2015 at 5:35 AM, Shafreeck Sea notifications@github.com wrote:

I think there is another question in this thread: How to close or discard a review request ? I git appraise request create a request and then I regret. I want to cancel or just delete it. How can I achieve this ?

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/issues/17#issuecomment-167999374.

[ojarjur]

Sorry for the confusion about the meaning of the resolved bit (which is what the "-lgtm" and "-nmw" flags set). @jishi9 got the intention exactly right in is issue comment here.

The "rejected" status really just means something like "needs more work", but that wording seemed too cumbersome (and "nmw" is probably not obvious enough to use for a human-readable status message).

There is no status that means something like "this request should never be merged". The closest you can get to that is to drop the review ref and have the commit be garbage collected. When that happens the request still exists in the history of the notes ref, but the tool will not display it.

We could add a flag to the list and show commands to display reviews that have been abandoned in this way, but since the commit is gone they will not be able to show the diffs, so that's not a great option.

What would probably be better would be to make the list command (by default) not show reviews that have a rejected status and have not been updated within some amount of time (e.g. 1 month). That would give us the option of keeping the commits around forever (if you want to do so), while still recognizing that the review has been abandoned, so it shouldn't clutter your command line output.

[cwmacdon]

How does one go about hiding rejected reviews from the list of open reviews?

Shouldn't rejected review be hidden from the "list" command with no arguments?

It is in a state that can never be accepted, so shouldn't it only shown with the -a flag?

If the purpose of it is that this code should be thrown away and never merged, why would it be considered in an unresolved "open" state?

[ojarjur]

@cwmacdon This is an area where the tool could use improvement.

How does one go about hiding rejected reviews from the list of open reviews?

Right now you can't but this does reflect a use case I want to support.

Shouldn't rejected review be hidden from the "list" command with no arguments?

I think that rather than hiding rejected reviews, we need to add another state a review can be in: abandoned. Those abandoned reviews would then be omitted from list output by default but still show up when listing with the -a flag.

My thoughts on this subject have evolved as I've gotten more experience as a user of the tool, so this is different from what I was previously mentioned in the comment from Jan 4.

This change would involve two things:

1. Make the 'targetRef' field in the request schema optional, and make sure all of the code that reads in requests properly treats a missing target ref as meaning the review is abandoned.
2. Add a new 'abandon' subcommand that updates the review to no longer have a target ref.

[cwmacdon]

@ojarjur So the reviewer rejects the review, and then the original author sees the rejection and abandons the work.

This sounds like a better workflow for the requester, rather than just seeing your pending review disappear from the list if rejected reviews did not show up there.

[ojarjur]

@cwmacdon Yes, that's the general idea.