Skip to content

Refactor away TemporaryCloneOfTestRepo #386

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

Merged
merged 1 commit into from
Apr 8, 2013

Conversation

dahlbyk
Copy link
Member

@dahlbyk dahlbyk commented Apr 7, 2013

Following up on this conversation: #312 (diff)

Before merging I'd suggest this be squashed into a single commit. For consistency usage, I'd suggest we either go with the approach suggested by the third commit (a Clone___TestRepo() corresponding to each test repo), or replace CloneBareTestRepo() with Clone(BareTestRepoPath). I think I prefer the former (thus including it here).

@nulltoken
Copy link
Member

I think I prefer the former (thus including it here).

So do I.

Before merging I'd suggest this be squashed into a single commit.

Could you take care of this as well?

@nulltoken
Copy link
Member

Just one nitpick:

Could you please make the type explicit?

string path = CloneStandardTestRepo();

instead of

var path = CloneStandardTestRepo();

@dahlbyk
Copy link
Member Author

dahlbyk commented Apr 8, 2013

Could you take care of this as well?

Done

Could you please make the type explicit?

Done

@nulltoken nulltoken merged commit 8738461 into libgit2:vNext Apr 8, 2013
@nulltoken
Copy link
Member

Before merging

Done

@nulltoken
Copy link
Member

😍

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.

2 participants