Skip to content
This repository was archived by the owner on Jun 27, 2019. It is now read-only.

Fix checkout bug in GitRepositoryHelper #39

Merged
merged 2 commits into from
Feb 28, 2017
Merged

Fix checkout bug in GitRepositoryHelper #39

merged 2 commits into from
Feb 28, 2017

Conversation

GeertvanHorrik
Copy link
Contributor

When a specific branch is specified in GitRepositoryHelper, it should not checkout the default branch of the repository with the specified branch name, but it should checkout the actual remote branch

… not checkout the *default branch* of the repository with the specified branch name, but it should checkout the actual remote branch
@asbjornu
Copy link
Member

Thanks, @GeertvanHorrik. Just for future reference, this is an attempt to fix GitTools/GitVersion#1173.

@GeertvanHorrik GeertvanHorrik removed the request for review from JakeGinnivan February 28, 2017 08:03
@GeertvanHorrik
Copy link
Contributor Author

@asbjornu if you agree, feel free to merge, then we can release another beta of GV.

@asbjornu
Copy link
Member

@GeertvanHorrik: Sure, I'm just a bit concerned about this (GitTools/GitVersion#1173 (comment)):

It does break a lot of unit tests. I can see where the idea is coming from to have to explicitly tag a commit (since some people want to include multiple commits into a beta). However, this breaks the auto-counting strategy I am using on my projects. Now I need to explicitly specific a new beta version number (as a tag) before releasing one. If I forget (this manual process), it will fail. Is introducing this as a configuration an option?

Perhaps @JakeGinnivan can weigh in with his thoughts?

@GeertvanHorrik
Copy link
Contributor Author

GeertvanHorrik commented Feb 28, 2017

That's just for the other change I proposed in there, it has nothing to do with this one ;-)

This really is a required bug fix (whether or not we make the other change).

@asbjornu
Copy link
Member

@GeertvanHorrik: Ah, ok. I thought the tests in here were green because the test coverage is a bit low. In that case, merge incoming! 😄 👍

@asbjornu asbjornu merged commit 98e7127 into master Feb 28, 2017
var repoTip = repo.Head.Tip;

// We currently have the rep.Head of the *default* branch, now we need to look up the right one
var originCanonicalName = string.Format("origin/{0}", currentBranch);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should update this to use remote.Name like everywhere else in this file

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JakeGinnivan Good point, will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@JakeGinnivan JakeGinnivan deleted the pr/checkout-bug branch March 1, 2017 14:03
@JakeGinnivan
Copy link
Contributor

Also can you put PR branches on forks @GeertvanHorrik, it keeps the main repo cleaner :)

@GeertvanHorrik
Copy link
Contributor Author

Will try to, I thought creating PR's in the first place would already keep things clean ;-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants