Skip to content

Convert local file: URLs to paths before handing over to git_clone. #280

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

Closed
wants to merge 7 commits into from

Conversation

tiennou
Copy link
Contributor

@tiennou tiennou commented Oct 24, 2013

Fixes a bug where a locally-cloned remote would get a file: URL as it's
origin URL, making the push machinery shrivel in fear.

@tiennou tiennou mentioned this pull request Oct 24, 2013
@ghost ghost assigned jspahrsummers Oct 25, 2013
const char *workingDirectoryPath = workdirURL.path.UTF8String;
// If our originURL is local, convert to a path before handing down.
const char *remoteURL = NULL;
if (originURL.isFileURL || originURL.isFileReferenceURL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This second check is redundant. (Reference URLs should return YES for isFileURL.)

@jspahrsummers
Copy link
Contributor

Can you add tests for cloning a file path URL and a file reference URL?

@tiennou
Copy link
Contributor Author

tiennou commented Oct 29, 2013

Added test. And also converted the old ones to the new Expecta-based format. And also also added more methods to GTRepository. And also also wiht fixed documentation. Wait for the intermission ;-).

Can anyone confirm that the old OCUnit-based tests don't run anymore ? After converting the +clone one, it wouldn't work without me changing originURL = self.bareRepositoryFixture.fileURL to originURL = self.bareRepositoryFixture.gitDirectoryURL, and the debugger was nice enough to not stop on the breakpoint I've set in the Test file... Also, if there's a way to restrict which test suits/test cases are ran, I'm listening ;-).

@jspahrsummers
Copy link
Contributor

Can anyone confirm that the old OCUnit-based tests don't run anymore?

No, they do. We should remove any of the old tests that have been converted over.

the debugger was nice enough to not stop on the breakpoint I've set in the Test file

Are you using the Test action? It won't stop if testing as part of a Build action.

Also, if there's a way to restrict which test suits/test cases are ran, I'm listening

See fdescribe, fit, etc. in the Specta README.

@tiennou
Copy link
Contributor Author

tiennou commented Oct 29, 2013

Are you definitive whey you say that they run ? Because I'm sure they don't here (I'm testing with ⌘U, Breakpoints enabled, I've tried to add a bogus STAssertFalse(YES) and it doesn't matter). Travis also seem to agree with me — I can't find any reference to GTRepositoryTest in the log, while GTRepositorySpec shows up as expected.

Thanks for the pointers to Specta, I'll go look.

@jspahrsummers
Copy link
Contributor

Are you definitive whey you say that they run ?

They should if they're added to the test target and not commented out.

@tiennou
Copy link
Contributor Author

tiennou commented Oct 29, 2013

Then there's something going on here... They're in the target, not commented, my Xcode 5.0.1 doesn't run them, and Travis is happy too.

@tiennou
Copy link
Contributor Author

tiennou commented Oct 30, 2013

Rebased because I'm trying to keep things concentrated and the other commits weren't relative to that bug fix. Expect another PR with more shiny new stuff soon ;-).

@tiennou tiennou mentioned this pull request Nov 7, 2013
@jspahrsummers
Copy link
Contributor

@tiennou Is this ready for review? Not entirely sure what the status is, based on your last comment.

@tiennou
Copy link
Contributor Author

tiennou commented Nov 8, 2013

Good for review. This is blocking #292 (and the stuff I'm doing locally afterwards) so I need it ;-).


createRepository = ^(BOOL bare) {
NSURL *newRepoURL = [NSURL fileURLWithPath:[NSTemporaryDirectory() stringByAppendingPathComponent:@"unit_test"]];
[NSFileManager.defaultManager removeItemAtURL:newRepoURL error:NULL];
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to put some of this functionality into GHTestCase, since it's more generally useful than just this spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point taken (I actually looked there first). But I'll do it another forthcoming PR and update that spec at the same time.

@jspahrsummers
Copy link
Contributor

🍔

@tiennou
Copy link
Contributor Author

tiennou commented Nov 9, 2013

Okay, I'm Not Pleased At All. I just noticed that I've rewritten GTRepostoryTest in both that PR and #292. I'll close this one and cherry-pick its constituent parts to the other instead (nobody likes conflict merges). Sorry for the (slightly) duplicated review effort :-S.

@tiennou tiennou closed this Nov 9, 2013
tiennou added a commit that referenced this pull request Nov 9, 2013
@jspahrsummers
Copy link
Contributor

@tiennou Ha, I didn't notice that either! Whoops. 😟

tiennou added a commit that referenced this pull request Nov 11, 2013
phatblat pushed a commit to phatblat/objective-git that referenced this pull request Sep 13, 2014
@jspahrsummers jspahrsummers removed their assignment May 22, 2015
@tiennou tiennou deleted the fix-local-clone branch August 10, 2018 22:09
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