Review code while the PR's branch is checked out

[rakanalh]

Thanks for this package! I've been wanting to make something like this for a while.

It would be nice if the package can fetch "upstream" or "origin" (whatever the remote is called) and checkout the branch so that while viewing the diff, the branch is checked out which means i can open the file and see the code around.

[charignon]

Where would the repo be cloned, a temp dir? What do you think would be the ideal experience for that, could you give me more details?

[rakanalh]

I don't think you have to clone any repo.

Assuming that there's a PR from me for this repo: [email protected]:rakanalh/greview.git
Once i submit the PR, you provide the URL for the PR where greview fetches/constructs the remote URL then:

1. Adds a remove rakanalh-greview as an example to your git repo
2. git fetch rakanalh-greview
3. git checkout {branch}
4. Then creates the diff between {branch} of my PR and your origin's master.

Once the PR is reviewed whether by reject or approve ... etc, greview would clean up and remove that remote (or keep it) depending on a configurable value.

Does that make sense?

[charignon]

I understand better, you make the assumption that you have a local copy of the repo without local changes that would prevent a branch change. That isn't always my use-case but I understand how this is fairly common.

We can probably detect the case and run a of command with magit like the one you suggested. Do you want to send a PR to see what that would look like?

I foresee an issue though if we use step 4. to generate the diff. For inline comments GitHub expects relative offsets to the file separator markers in the diff (https://developer.github.com/v3/pulls/comments/#create-a-comment). If we create the diff locally and don't ask GitHub for it explicitly, it won't necessarily be the same diff (because of context lines), which may make the review comments point to the wrong inlines.

[parhamdoustdar]

Can't you use the forge package for this? They already have a workflow for checking out a pull request (forge-checkout-pullreq) that covers a variety of weird edge cases.

[charignon]

I added some support for forge in #13, but it does not seem to be quite what you want. @parhamdoustdar could you give more details about what you suggest?

[parhamdoustdar]

Yes sure. My suggestion was that instead of supporting working with pull requests, either the user or the package could use forge to check out the relevant PR. For example, from this package's point of view, it could work in several ways:

• I provide a link to the PR and github-review calls forge-checkout-pullreq to change my local copy (this will probably have to be an opt-in behavior because it changes the state of the local file system)
• I check out the pull request with forge and manually call github-review with the link to the PR

I would really think that integrating with magit and forge would provide a lot of useful functionality, but I'll discuss that in another issue.