Feature: Maintainer workflow

[shana]

Entry point

A contributor opens a PR

Reviewing a PR

In the PR list, click on "Review PR" to open the PR Review (details) pane

• Adds PR remote to your local clone, if not there yet
• If it's the first time this PR is being reviewed through VS and we've just added the remote
  • Create local branch tracking remote PR branch
    • If local branch matching naming convention already exists and is tracking the correct remote PR branch, assume that as the local PR branch
  • If local branch matching naming convention already exists but is not tracking the correct remote branch
    • Create local branch with a numbered suffix
• Find local branch matching naming convention of PR with correct remote tracking branch of PR (should probably cache this info)
• Switch to local branch

PR Review UI:

• Tree view of changed files
  • Open individual file diff from tree view
• Link to conversation view in PR Review UI
  • so you can jump to github to comment on the PR
• If there are new commits since the last time this PR was opened:
  • Shows message saying "New commits available"
  • If there is a local branch for the PR, message says "would you like to update your local branch?"
    • If yes, fast-forwards the local branch.
      • If there are local changes then we warn and don't do anything

Merging and Closing a PR

The PR Review/Details pane has a Merge and a Close button.

• Click on "Merge PR" or "Close PR"
  • Perform the action via the API
  • In case of merge:
    • Ask if user wants to clean up the PR branch
      • If yes, delete the remote and local branches
      • Fast forward target branch with remote commits
      • Switch to target branch

[grokys]

Asks the user if they want to create a branch to test the PR or select an existing one

What is the use-case for this? I'd assumed that just checking out the PR into a new branch would suffice. At least that's the way that desktop does it.

[grokys]

Here are my notes from the initial spike I did last week. It goes into a little more detail on how things might work in the PR list pane.

Flow for Maintainer Workflow

• In PR list, each open PR has a "checkout" button if no local branch
• If there is a local branch for a PR it also has "merge" and "sync" icon buttons
• Clicking "checkout" checks out PR as local branch and switches to that branch
  • If already checked out into a branch, switches to that branch
• "Sync" refreshes from upstream PR
• "Merge" merges the PR to master and switches to master
• Once merged, a "Delete" button appears - this deletes the local and remote branch
  (if PR from same repos)

Questions

• How do we name local PR branches? Like in Desktop ("pr/123"), or could we name it more
  intelligent (e.g. "pr/123-this-does-that)?
• Would a user making changes on a PR branch cause problems? e.g. with subsequent sync?
• Can we always tell if a branch is the PR branch we're looking for? What if the user has
  already checked out the PR in desktop or with command-line? Do we reuse those?
• Should "delete" button always be present on local PR to delete local branch? Would the
  difference in behavior be confusing? i.e. that sometimes it just deletes local and
  sometimes both local and remote.

[shana]

What is the use-case for this? I'd assumed that just checking out the PR into a new branch would suffice. At least that's the way that desktop does it.

If there's already an existing branch that matches the naming pattern that we use, then it's likely the user has already created a branch for the PR and somehow we lost track of it. We need to decide whether to use a different name for the branch or ask the user if they want to use the existing one, or disable the feature entirely and let the user switch branches. But this would only happen if the repo is already set up to do PRs before, so this is on the wrong part of the flow.

[shana]

Once synced, a "Delete" button appears - this deletes the local and remote branch (if PR from same repos)

Do you mean "Once merged" here?

[shana]

If there is a local branch for a PR it also has "merge" and "sync" icon buttons

Sync is way too overloaded with meaning to use here, people will assume we're pushing things. "Update" or "Update local branch" or something more clear is probably preferable.

How do we name local PR branches? Like in Desktop ("pr/123"), or could we name it more intelligent (e.g. "pr/123-this-does-that)?

"pr/###-title-of-PR" would be nicer than just the number.

Would a user making changes on a PR branch cause problems? e.g. with subsequent sync?

If they make changes and don't commit, we can't do anything to the local branch. If they make changes and commit them, we need to decide whether we want to do a fetch&merge or tell them to do it themselves via the git UI.

Can we always tell if a branch is the PR branch we're looking for? What if the user has already checked out the PR in desktop or with command-line? Do we reuse those?

We can always tell if a local branch is appropriate by checking if it's tracking a remote branch and whether it matches the remote branch with no local changes. This is where we should probably ask the user if they want to reuse an existing branch.

Should "delete" button always be present on local PR to delete local branch? Would the difference in behavior be confusing? i.e. that sometimes it just deletes local and sometimes both local and remote.

The local one is always deletable once the PR is merged. Dunno what to do about the remote one, I agree that it's going to be confusing if "Clean up" means different things depending on whether you have permissions to the repo or not.

Separate feature idea: "Remote clean up" button that clears all remote branches whose tips are already on master, and "Local clean up" button that prunes all useless local branches whose tips are already on master.

[shana]

Ugh, I could really use a list of files that go with this PR right so I can quickly jump to them.

[grokys]

With regards to checking out a PR, above it was written "Adds PR remote to your local clone, if not there yet". This may not be necessary as all PRs are available from the refs/ID/head reference.

With this in mind, the method of checking out a PR may be:

• Detect if a PR is from the current repository or a fork
  • This can be got from the API
  • Or it can be done by looking for a tracking branch whose tracked branch's Tip points to the same commit as refs/[PR]/head
• If the PR is not from a fork then simply check out the branch
• If the PR is from a fork then use a refspec to check out the branch from refs/ID/head to a local branch named pr/ID-title-of-pull-request

We then need a method of identifying an already checked out PR and updating that checked out PR:

• If not from a fork then we're all set
• If from a fork then we need to pull from refs/ID/head to update. The problem here is identifying a locally checked out branch as an existing PR branch.
  • Desktop simply does this by name, so we could say that anything starting with pr/123 is the branch for PR 123. It's unlikely that users would use this scheme to mean anything else. Updating would simply be a merge with refs/ID/head
  • @shana suggested that we could also set the branch's pull remote to point to refs/ID/head. I need to look into this one further - I'm not currently certain what the push remote would be set to in this case

[shana]

@shana suggested that we could also set the branch's pull remote to point to refs/ID/head. I need to look into this one further - I'm not currently certain what the push remote would be set to in this case

It shouldn't need to be set to anything, since it's not going to be pushed.

If from a fork then we need to pull from refs/ID/head to update. The problem here is identifying a locally checked out branch as an existing PR branch.
Desktop simply does this by name, so we could say that anything starting with pr/123 is the branch for PR 123. It's unlikely that users would use this scheme to mean anything else. Updating would simply be a merge with refs/ID/head

Probably a good idea to standardize on branch names, definitely. But we can save information on which PRs we checkout, we have local storage for this, so we can match up things so we don't have to guess. We'll definitely need this if/when we let users pick an existing branch for checking out instead of us creating one for them.

[grokys]

It shouldn't need to be set to anything, since it's not going to be pushed.

I'm not sure this will always be the case. When I suggested this last week, @niik pointed out that in Desktop once a PR is checked out it's like any other branch, in that a user can make changes to it and push it to origin etc. I thought about it, and indeed that is a feature that I sometimes take advantage of, so I'd imagine it's not a completely unusual scenario.

[shana]

In that case, being like any other branch, push should only be set when the user actually pushes the branch somewhere, just like any other branch.

[grokys]

Right, but then the fetch and push remotes would be different, which would cause confusion I expect, e.g.

1. Someone submits PRs from a fork
2. Maintainer checks it out, wants feedback from other maintainer so pushes
3. Other maintainer makes a change
4. First maintainer tries to pull in second maintainer's changes, but they don't arrive because fetch ref is different to push ref.

I don't know if this is a scenario we want to support,

If we make "sync from upstream* fork" an operation that VS does by locating the PR by name/local storage and merging from /refs/pr/N/head then this would work fine.

*: Downstream?

[shana]

Maintainer checks it out, wants feedback from other maintainer so pushes

So let's see, why would they push to get feedback?

If they're going to push the branch manually to their repo, they're probably going to take ownership of the PR. No more changes are expected to come from the contributor and their PR probably gets closed, since it won't get merged. With protected branches, the maintainer still needs a PR, so we'd go through an "importing" process.

If the contributor pushes more things that the maintainer wants to pull, they need to do that manually, it's way outside a normal workflow. There's now two diverging branches.

If the maintainer wants to push some changes to show the contributor how something is done, the branch is no longer able to be synched with the contributor branch, and the maintainer needs to reset the branch if they want to get contributor changes again.

If two maintainers want to collaborate on a branch for the purpose of showing a contributor how to do things, again they have to reset the branch to be able to get contributor changes.

This means that if maintainers want to make changes to a pr branch, they are effectively breaking the connection with the contributor branch. If they then want to collaborate, they'll need to set up tracking information accordingly.

That means that there may be a pr/123 branch locally that doesn't actually match a pr head. If the maintainer wants to checkout the pr again, we need to account for that in the naming convention.

[grokys]

Like I said, I'm not sure this is something we want to support, however, I feel that by having PR branches for forks just being "normal" branches we're likely to avoid some user confusion. If they're special branches with a different fetch and push ref then I suspect we'll have people scratching their heads.

To address specific points:

If they're going to push the branch manually to their repo, they're probably going to take ownership of the PR. No more changes are expected to come from the contributor and their PR probably gets closed, since it won't get merged.

Yes, probably, however things are rarely so clean in real life, and git works so well at merging from different sources that I think we can take advantage of this:

1. Maintainer 1 checks out PR branch from fork into local repo
2. Pushes it, maintainer 2 works on it
3. Original contributor submits more changes

Even though this workflow is messy, it's completely supported by git and handled well through merges. Conflicts can be resolved just like anywhere else. When finally everyone is happy with the results, the local PR branch is merged to master and the PR gets automatically closed when pushed. (* see protected branches comment below)

With protected branches, the maintainer still needs a PR, so we'd go through an "importing" process.

With protected branches what you say is true, however I suspect that the majority of projects don't have protected branches enabled (are there metrics for this?) so we need to support the more laissez-faire maintainership that I suspect most projects use.

Case study: I've only just switched to protected branches for Avalonia: before I was manually checking out PRs locally, making small changes like whitespace fixes then merging them to master myself and pushing to close the PR. Sometimes contributors would also make changes, and an easy "incorporate PR changes here" button would have been well-received.

(Aside: I'm actually thinking of returning to this workflow: protected branches are really slowing down development and in reality nobody is even reviewing my PRs)

That means that there may be a pr/123 branch locally that doesn't actually match a pr head. If the maintainer wants to checkout the pr again, we need to account for that in the naming convention.

Yep, this sums up what's going on in my head: if we assume that pr/123 relates to PR 123 then even if it doesn't match the PR head, we can do a merge.

---

tldr; my desire comes from not wanting have any "special" branches lying around that work differently: partly because we can't actually control what the user does to these branches, and partly because git handles these scenarios fine anyway.

Again, this is just what I'm currently thinking, my thoughts are still a bit nebulous.