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

git: Throw fetch error #36974

Merged
merged 1 commit into from Oct 27, 2017

Conversation

Projects
None yet
2 participants
@keegancsmith
Contributor

keegancsmith commented Oct 26, 2017

The only place that calls fetch will handle the error correctly. So this should fix a bug were we don't disable autofetch when fetch fails due to auth issues.

try {
await this.repository.fetch();
} catch (err) {
if (err.gitErrorCode === GitErrorCodes.AuthenticationFailed) {
this.disable();
}
}

@joaomoreno joaomoreno self-requested a review Oct 27, 2017

@joaomoreno

joaomoreno requested changes Oct 27, 2017 edited

I don't think this is correct behaviour. We want to see errors, if there are any, when the user explicitly runs the fetch command.

@joaomoreno joaomoreno closed this Oct 27, 2017

@keegancsmith

This comment has been minimized.

Show comment
Hide comment
@keegancsmith

keegancsmith Oct 27, 2017

Contributor

@joaomoreno I think you may have misread this PR. Currently we just catch and throw away the error for fetch. This PR makes it so we don't throw it away.

Contributor

keegancsmith commented Oct 27, 2017

@joaomoreno I think you may have misread this PR. Currently we just catch and throw away the error for fetch. This PR makes it so we don't throw it away.

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno
Member

joaomoreno commented Oct 27, 2017

Oh!

@joaomoreno joaomoreno reopened this Oct 27, 2017

@joaomoreno joaomoreno merged commit d3028fb into Microsoft:master Oct 27, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla All CLA requirements met.
@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Oct 27, 2017

Member

@keegancsmith You keep doing great work! Thanks man! 🍻

Member

joaomoreno commented Oct 27, 2017

@keegancsmith You keep doing great work! Thanks man! 🍻

@joaomoreno joaomoreno added this to the October 2017 milestone Oct 27, 2017

@joaomoreno joaomoreno added the git label Oct 27, 2017

@keegancsmith

This comment has been minimized.

Show comment
Hide comment
@keegancsmith

keegancsmith Oct 27, 2017

Contributor

np!

Contributor

keegancsmith commented Oct 27, 2017

np!

@keegancsmith keegancsmith deleted the keegancsmith:fetch-noop branch Oct 27, 2017

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