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

Use git clone instead of git pull to support non-master bases #41

Merged
merged 6 commits into from
Sep 26, 2018

Conversation

itsdalmo
Copy link
Contributor

Ref #35 - Using git clone and specifying the base branch should ensure that the target branch for the PR is available locally when we run rev-parse. In effect, I don't think the previous implementation has ever supported non-master bases.

I've added an E2E test which reproduces the reported error, and does not complain when run against the code in this PR. So hopefully it is fixed 🤞

@itsdalmo
Copy link
Contributor Author

Ah, now I recall why I used pull instead of clone:
https://blog.github.com/2012-09-21-easier-builds-and-deployments-using-git-over-https-and-oauth/

To avoid writing tokens to disk, don’t clone. Instead, just use the full git URL in your push/pull operations

Need to explore alternate routes.

@itsdalmo
Copy link
Contributor Author

Switched back to pull since a clone would add the URL for origin in verbatim to the .git/config, which involved writing the access token to disk - no good 😂 The current method should be safe, and it passes all the tests (including E2E).

Copy link

@colincoleman colincoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@mikael-lindstrom mikael-lindstrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@itsdalmo itsdalmo merged commit 9f1b5f0 into master Sep 26, 2018
@itsdalmo itsdalmo deleted the explicit-git-pull-base branch September 26, 2018 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants