I accidentally commit locally to protected branches

[billygriffin]

Please describe the problem you think should be solved

"I frequently commit locally to a branch that's protected on GitHub and one that I can therefore never push to. After I realize I committed to the wrong branch, then I have to either Undo however many commits I've made, switch branches, and do the commits again on a new branch or I have to use a different method to get those commits over to a different branch."

Branches on GitHub are protected for a reason (so there's a particular level of scrutiny before code makes its way onto them), and it doesn't make sense why GitHub Desktop shouldn't have knowledge of this central concept of GitHub's overall model.

A version of this was brought up in #1021 and we explored this a bit in #4647, but I'd like to refocus the discussion around the problem before we move toward solutions.

Reference GitHub protected branch docs: https://help.github.com/en/articles/about-protected-branches

Open questions

1. Is this only relevant for protected branches, or are there other instances where people never want to commit to a particular branch? One that has come up previously is the default (often master) branch, but I want to question that assumption.
2. If a branch is protected, is there ever a reason someone would want to commit to that branch locally? Our assumption thus far is no.

If you have thoughts about either of the open questions, we're really interested in understanding this better.

Success metrics

1. If our assumptions are correct and we succeed in solving this problem, we'd anticipate that the number of commits in Desktop to a branch that's protected would drop significantly (by at least half, or potentially even to zero).

[ampinsk]

I did some thinking here and chatted with @billygriffin. We believe this problem would better served by not letting users commit to protected branches in the first place, rather than letting them move commits after the fact. Here's the flow we're proposing:

Proposed flow

Lock commit area

If a branch is protected, we'll update the commit area to show a warning, and prompt you to switch your branch

Switch branch

Clicking on Switch branch... will open the branch panel and allow you to choose a branch to switch to, or make a new branch.

⚠️ We decided we would skip the stashing dialog here because it is implied that you'll be bringing your change with you, and we'd rather skip the unnecessary click.

Commit to new branch

Your changes would follow you to the new branch, and you can commit from here.

Alternate option

If this flow is too blocking, an alternate option would be to not disable the commit area, and simply show a warning that the branch is protected.

Feedback welcome! ✨

[tierninho]

The designs look great and both easy to comprehend.

Some things I was thinking about in regards to this:

• Transferring from one protected branch to another may be attempted unless we filter them out or show some sort of treatment in the branch list.
• What happens if changed files are already present and then the branch is marked as protected? API will update the button?
• Some users might want to stash on the protected branch, as the state of the branch may be temporary
• This will affect each individual user based on permissions, and whether repo is public/private, plus plan type?

[billygriffin]

Thanks for the feedback and great questions @tierninho! Initial thoughts below:

Transferring from one protected branch to another may be attempted unless we filter them out or show some sort of treatment in the branch list.

They would just be in the same state as before, which IMO is completely reasonable. I think it's ok to assume that if they know they are on a protected branch and need to switch, they likely aren't going to choose another one to move their work to.

What happens if changed files are already present and then the branch is marked as protected? API will update the button?

Good question - that feels really edge-casey and I'm not sure we need to solve for it. But we should ensure that the cadence at which we know about protected branches is frequent enough to make this a fairly unlikely scenario.

Some users might want to stash on the protected branch, as the state of the branch may be temporary

By "state of the branch," do you mean whether it's protected or not? If so, I'm willing to take that risk until we hear otherwise. I don't think people are frequently protecting and then unprotecting branches.

This will affect each individual user based on permissions, and whether repo is public/private, plus plan type?

Not sure I understand this question. It's just a matter of whether or not the user can push to that branch because it's protected. We could do something similar to handle forks, but since that's a separate repo and not just a different branch within the same repo, that feels more involved and a separate problem statement.

[tierninho]

Not sure I understand this question. It's just a matter of whether or not the user can push to that branch because it's protected.

I was reading about protected branches and saw "If your repository is owned by an organization, you can restrict users or teams from pushing to a protected branch" here.

For the MVP version, I agree that a protected branch is just that, full stop. 💯

[billygriffin]

Update: GitHub allows repo admins to exclude admins from branch protections, and also to whitelist users or teams to allow them to push to protected branches (shoutout to @tierninho for bringing it to our attention). Therefore, it seems like we have two choices here:

1. Use @ampinsk's proposed flow for repos where nobody is able to push protected branches to GitHub, and the alternate option for repos where some users can push to those branches.

2. Use the alternate option for everyone.

Unless it's really easy to get the knowledge required to differentiate to support (1) above, we probably ought to start with (2) as the first iteration and then potentially build out from there.

[shiftkey]

@billygriffin

exclude admins from branch protections

This bit of information you're looking for is what you get when you request the Get Repository endpoint as an authenticated user. You'll see this permissions hash that represents what the current user has rights to do with the repository:

and also to whitelist users or teams to allow them to push to protected branches

EDIT: Oops, I missed this comment. This should be covered by the List user teams endpoint. I'm less familiar with this API (and how Branch Protections itself is returned as JSON) but I think those should cover both gaps.

[billygriffin]

@shiftkey Thanks! If we can confidently manage the differentiation without too much additional effort and overhead, I'm definitely onboard with doing that as I really like @ampinsk's proposed solution for the cases when you definitely cannot push to that branch.

[shiftkey]

@billygriffin we'd need new places to cache the branch protections (per repository) and the teams a user belongs to (per account) but I think that's not too much additional work - we have the infrastructure to support this.

[shiftkey]

Gonna unassign myself from this issue now that #7826 is merged and ready to go out for the beta.

@billygriffin I'll defer to you about whether you want to leave this open post-beta and how to deal with feedback.

[billygriffin]

Thanks @shiftkey! I'd like to keep it open for further iteration since we do intend to do that.

[NickCraver]

I love the idea, but if we can't handle the "you're allowed to push to this protected branch", then this shouldn't go live. It will have false positives for 100% of our users. We use protected branches and ACLs for who can push as a normal business and compliance practice. The client warning about something is bad here because it makes the warning useless for main use case and the user will never know when to ignore it.

These ACLs look like this (under each protected branch's options):

For details, say we have a quarterly release of a thing: certain people can push to that branch. Others can't. That's a normal practice to lock down things in really the only way we're given. This going out will turn that feature into a fault. Please at the very least let people disable this - I'm going to get a lot of pings on what's broken and why we've removed their access due to these false positive warnings.

On timing: we are a paying GitHub Enterprise customer, so even if GitHub adds a new API to make this work, we likely won't get it for a few months after that.

[shiftkey]

@NickCraver thanks for the feedback. I'll leave it to others to take this further (I'm out on holidays next week) but I'll provide insight into the API details based on my experience fleshing this out:

I love the idea, but if we can't handle the "you're allowed to push to this protected branch", then this shouldn't go live.

API access to this list in a branch protection is only available for admin users - so you could be in a situation where you can push to the repository because you belong to a team listed, but you can't view the list to see these teams.

We're mindful of this gap, and we're talking through it with the team that owns this area. Hopefully we can figure out a solution that'll eventually be available in GHE.