Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add git stashing #29138

Merged
merged 4 commits into from Aug 15, 2017
Merged

Add git stashing #29138

merged 4 commits into from Aug 15, 2017

Conversation

@Krzysztof-Cieslak
Copy link
Contributor

Krzysztof-Cieslak commented Jun 20, 2017

Addresses #1904 which is one of the oldest and most upvoted issues.

Adds 3 new commands for git integration:

  • git stash
  • git stash pop using latest stash
  • git stash pop with choosing stash
@msftclas

This comment has been minimized.

Copy link

msftclas commented Jun 20, 2017

@Krzysztof-Cieslak,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@Krzysztof-Cieslak

This comment has been minimized.

Copy link
Contributor Author

Krzysztof-Cieslak commented Jun 20, 2017

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Aug 11, 2017

@Krzysztof-Cieslak We don't really need to have the stashes in the model itself. This will just make git status slower since we'd need to call git once more in order to get the stashes. You can just make this call once one of the stash actions runs. Can you fix that?

@Krzysztof-Cieslak

This comment has been minimized.

Copy link
Contributor Author

Krzysztof-Cieslak commented Aug 11, 2017

Yes, I'll rebase and fix it.

@Krzysztof-Cieslak Krzysztof-Cieslak force-pushed the Krzysztof-Cieslak:stashing branch Aug 14, 2017
@Krzysztof-Cieslak Krzysztof-Cieslak force-pushed the Krzysztof-Cieslak:stashing branch to b5bf353 Aug 14, 2017
@Krzysztof-Cieslak

This comment has been minimized.

Copy link
Contributor Author

Krzysztof-Cieslak commented Aug 14, 2017

Ready to review again.

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Aug 15, 2017

@Krzysztof-Cieslak

These changes are not formatted and violate some linting rules we have set. We have configured a git pre-commit hook to validate these for you, which gets set up when you run ./scripts/npm.sh install. You were only able to commit by not running that or by commenting out the pre-commit hook. Next time, please commit while running through these rules. I've fixed those issues for you on this one.

Also, this goes into infinite recursion. Next time, please run your changes before handing them over to us.

Also, this condition will fail when index is zero.

Also, I've added functionality to provide a stash message when stashing changes.

I've fixed all that up.

We do appreciate your contributions very much. But next time, try to follow the guidelines better so we make this smoother.

@joaomoreno joaomoreno merged commit 8dcb181 into microsoft:master Aug 15, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Krzysztof-Cieslak

This comment has been minimized.

Copy link
Contributor Author

Krzysztof-Cieslak commented Aug 15, 2017

Sorry for the trouble @joaomoreno, I haven't paid enough attention to this PR, indeed. My bad :(

@Krzysztof-Cieslak Krzysztof-Cieslak deleted the Krzysztof-Cieslak:stashing branch Aug 15, 2017
@joaomoreno joaomoreno modified the milestones: Backlog, August 2017 Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.