Option to exclude items from Git branch checkout list #13506 #16792

Merged
merged 2 commits into from Dec 13, 2016

Projects

None yet

5 participants

@ryapapap
Contributor
ryapapap commented Dec 7, 2016 edited

Here are my proposed changes to solve issue #13506.. This is an implementation of what @joaomoreno proposed in the ticket.. I've added a setting named checkoutType which controls what is listed inside the quick open panel for the git checkout command (all is as it is today, local is only local branches, tags is local branches and tagged commits, remote is local branches and remote branches... Happy to change names/descriptions of anything :) ).

I believe these are the minimal amount of changes to do this. Poking around, I could see a couple other ways you'd do it, either getting the config service into the CheckoutCommand (which seemed like a large refactor) or using the setting as a filter on the GitService for what's exposed as model/head/refs (which also seemed like a large refactor and could change other consumers).. If one of those are the desired approach, I think that my changes should be abandoned and someone else would have to take over that effort (as I'm not familiar enough with the larger application).

There's still some things I'm unsure of. For example, we could branch inside the CheckoutCommand's getResult method instead of using empty arrays for tags/remoteHeads (I just thought it added an unnecessary level of complexity). Another example is maybe using a private _checkoutType on the GitService and set it as part of the onDidUpdateConfiguration handler (this would be more similar to allowHugeRepositories, but since there isn't an override I don't know that it's necessary (and very possible I'm misunderstanding some intents here).. It's also very possible there's other completely different things I overlooked/you guys would know more about.

I looked for tests related to either the CheckoutCommand or the git settings and had trouble finding anything that I could base things on. If you had any suggestions or places to look at for similar tests, I'd be happy to take a stab at automating something. I did run all local tests and linting and they passed and reported nothing (respectively).

I did test this locally with this repo to ensure that 1) The setting existed/had the description and options I expected 2) The drop down respected the settings I picked (and that it updated without restarting) 3) I could still switch branches

Thanks for the great product!

Edit: reread this this morning and noticed I had some bad English in some places.. Also signed the CLA (and got confirmation from it), but the bot hasn't updated this. Is there anything else I should do for that? I wasn't expecting one line for the address, so maybe I messed it up.

Ryan Patterson Option to exclude items from Git branch checkout list #13506
d55c1df
@msftclas
msftclas commented Dec 7, 2016

Hi @ryapapap, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@joaomoreno joaomoreno added this to the January 2017 milestone Dec 7, 2016
@joaomoreno joaomoreno self-assigned this Dec 7, 2016
@ryapapap ryapapap Typo fix
`inclueRemotes` -> `includeRemotes`
7831f77
@msftgits
msftgits commented Dec 7, 2016

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

@msftgits msftgits closed this Dec 7, 2016
@msftgits msftgits reopened this Dec 7, 2016
@msftclas
msftclas commented Dec 7, 2016

Hi @ryapapap, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@msftgits msftgits removed the cla-required label Dec 7, 2016
@csvn
Contributor
csvn commented Dec 12, 2016

This solves my issues with having millions of tags for npm release (e.g. "v1.0.5" tags). I'd be happy if this gets approved.

I was thinking of doing a pull request where tags existed in a new section at the bottom (image below), but this is probably better if anyone would (for some reason) prefer to have tags sorted with local branchnames.

checkout-new

@ryapapap
Contributor

My motivation was the same as yours (too many tags in the way). What I saw in the animation would also solve my issue... I also don't really understand when/why someone would want the tags and local branches combined (fwiw)

@joaomoreno joaomoreno merged commit 7831f77 into Microsoft:master Dec 13, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@joaomoreno
Member

Thanks for the PR! I've done a small change: directly use the configuration service from within the CheckoutCommand. 👍

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