Create a gist now

Instantly share code, notes, and snippets.

What would you like to do?

A placeholder to discuss compare view ideas.

Competitors

Pieces we already have

  • Comment on a line number / commit
  • See unique commits between two branches (/compare/branch1/branch2)
  • See diff of two branches (/compare/branch1/branch2)
  • Pull requests

See also

Thinking out loud

The more I think about this, the more that we need to combine a lot of existing ideas into one “code review” piece. The way I imagine it, the compare view has two states:

  • Temporary State – This is what we have right now. Just comparing two SHAs without any ability to comment on the files or compare itself
  • * Permanent State* – This would be a pull request object. Instead of just being a “Hey, pull from me” each pull request would consist of a message, users to notify, and two commit SHAs representing the beginning and end points. If people are using git properly, this is the diff between their topic branch and master (with the SHAs extracted).

This gives us an opportunity to make pull requests much smarter. It might be easier to write this out as a use-case, so here:

Example use case

I fork mojombo/clippy to make some changes. I find an interesting fork, merge in the changes, then create a few commits of my own. I browse to my temporary compare view page at http://github.com/kneath/clippy/compare/master/7329b72360100b9484bc6cc9f097ea35f08421ad make sure it looks good, and now I want to send a pull request for my changes to be included upstream.

Somewhere on this page (different from the existing pull request button) there is a new pull request button that says something like “Create pull request from this comparison” On this dialog/page I need to fill out the following fields:

- Message (“Updated clippy with internationalization options, allowed arbitrary clippy size, and added a copied callback”)
- Notify Users (Smarter, with the blessed repo up top, followed by recommended people [those who authored lines I changed], and then anyone else I want to notify. Get rid of idea of notifying everyone with a fork, fuck that)
- Create related issue (should we do this by default? It would be a place to discuss the pull request as a whole)

So, I send the pull request to Tom. Tom gets an email, and logs in. He goes to a screen listing out all his pull requests (something like the inbox, but less sucky). He clicks on my pull request and it goes to the compare page (shitty skitch comp):

On this page Tom can:

- Comment on the pull request (via related issue)
- Deny this pull request
- Comment on individual commits (as usual)
- Merge it in (fork-queue style) if he likes it
- Comment on file lines/diffs (see below)

If I realize I fucked up and need to include extra commits, I can make the new commits and edit the pull request object to change the SHAs. Doing this would update the related issue so Tom knows I changed stuff.

Commenting on file lines/diffs

Commenting on diffs in a pull request is difficult, because the pull request can change and the diffs can change. I think the solution is to pull all this into an issue with linked diffs. Say I have an issue with this bit:

What I do, is use something similar to the existing comment bubbles on diffs. I select a range, and it pops up a modal (or otherwise) that is commenting on the attached issue. The issue comment that gets posted ends up having this info:

- Latest commit in the compare view I was looking at when I made the comment
- File name, line begin, line end
- Text of my comment

This way line numbers/diffs are preserved even if the pull request changes the diff people were talking about (because it’s linked to the sha of when you commented, not the sha of the pull request)

Thanks for sharing this! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment