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.

Sign up for GitHub

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

Jump to bottom

Split GTRepository API #297

Closed
wants to merge 65 commits into from
Closed

Split GTRepository API #297

wants to merge 65 commits into from

Conversation

tiennou
Copy link
Contributor

@tiennou tiennou commented Nov 12, 2013

As was hinted in #224, this moves some parts of the API away from GTRepository. It's based on #292. This only leaves repository "properties" and high-level API in GTRepository.

This changes the following :

I'm not sure if the others are worthy of separate PRs or not...

@tiennou
Rewrite tests for GTBlob.
618570b
@tiennou
Use git_blob_create_fromdisk instead of going through the `fromwork…
ed0a2a8 
…dir` version.

Because I don't want to handle the pain of relativizing the passed URL.
@tiennou
Asserts.
b7f671f
@tiennou
Move the contents of GTBranchTest where they belong.
709c2b3
@tiennou
Remove GTBranchTest.
9e69d7c 
I know there's still a test in here, but I'm not sure what it's testing since we're ARC now…
@tiennou
Convert GTCommitTest to Expecta.
04fd3e7
@tiennou
Convert GTReferenceTest.
f578c4c
@tiennou
Convert GTRepositoryTest.
9c2c3e2
@tiennou
Cleanup GTBlob's header.
f655dec
@tiennou
Make error a local variable.
1ea8499
@tiennou
Expect errors to be nil here.
49ed41f
@tiennou
Reindent commented test.
9b1bd6c
@tiennou
Rename test.
8346974
@tiennou
Cruft--;
bc98f3c
@tiennou
Dot-syntaxify.
86d4f7b
@tiennou
Rename isValidReference and define it at declaration time.
173e177
@tiennou
Make error a local variable.
5246535
@tiennou
Fix the lookupObjectByRefspec:error: name.
79fcc2d
@tiennou
Whitespace.
d4a1ae8
@tiennou
Test that the returned branches are the expected ones.
e09ebb5
@tiennou
Test the complete reference array in one step.
d07941e
@tiennou
Clarify comment.
8f47b33
@tiennou
Test the actual error domain & code.
9bc13e2
@tiennou
Convert local file: URLs to paths before handing over to git_clone.
b88903d 
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
Dot-syntaxify.
2b2be90
@tiennou
Use -fileSystemRepresentation here.
8265839
@tiennou
File reference URLs are file URLs; no need for a separate test.
bcdd927
@tiennou
Useless call; file reference URLs know how to path-ify themselves.
aa510a6
@tiennou
Test that the remote set by +clone… looks fine to libgit2.
7f7cb21
@tiennou
De-oneline-ify.
2986a6b
@tiennou
Add new branch initializers based on the libgit2 functions.
0cdd6be
@tiennou
Use the reference's repository instead of asking for one.
f211658
@tiennou
Use the libgit2 enum for those.
b00b007
@tiennou
Cut down the number of initialization methods on GTReference.
04a4b82
@tiennou
Use libgit2 function to get the branch name.
bb261ee
@tiennou
Use libgit2 function to get remote name.
b561652
@tiennou
Some more API for GTBranch.
9c50e69
@tiennou
Add a way to restrict branch lookups by type.
2db6938
@tiennou
Remove success parameter from -trackingBranchWithError:success.
a17178f
@tiennou
Tests.
865c48a
@tiennou
Move the object lookup methods in GTObject.
15735d3 
You use the concrete subclass to restrict the object type you're expecting. Good bye, `GTObjectType` ;-).
@tiennou
Merge branch 'branch-cleanup' into split-gtrepo
e3531ec 
Conflicts:
	Classes/GTBranch.m
@tiennou
Rename -fileURL to -workingDirectoryURL and provide -displayURL.
7d8449e
@tiennou
Merge branch 'gtref-cleanup' into split-gtrepo
4a993b8 
Conflicts:
	Classes/GTReference.m
	Classes/GTRepository.m
@tiennou
#pragma marks.
fb0359d
@tiennou
Tag creation & deletion methods.
066cfad
@tiennou
Remove tag creation methods from GTRepository.
cc40bbb
@tiennou
Move tag creation specs to GTTagSpec.
b373d5d
@tiennou
Merge branch 'gttag-cleanup' into split-gtrepo
c544fa6 
Conflicts:
	Classes/GTRepository.m
	ObjectiveGitTests/GTRepositorySpec.m
	ObjectiveGitTests/GTTagSpec.m
@tiennou
Remove branch creation method from GTRepository.
14d6a5a
@dannygreg
Copy link
Contributor

Holy sweeping change Batman! This seems great but I don't really have time to dig through all of this at the mo.

@joshaber
Copy link
Member

I'm fairly 👎 on the inRepository: pattern. It doesn't feel idiomatic to me.

I love the idea of breaking up GTRepository a bit more. That said, some of these changes mean these objects will change in place. I'd rather not give up immutability in those cases.

@jspahrsummers jspahrsummers mentioned this pull request Nov 12, 2013
Merged
@tiennou
Copy link
Contributor Author

tiennou commented Jan 15, 2014

If you prefer, I can rebase against current master (that'll remove the test->spec that are showing up here), and submit separate PR for each of those splits.

@joshaber I do agree about how inRepository: feels too, but I was mostly following the CoreData insert*blah*:inManagedObjectContext: idiom, because I don't another way of splitting GTRepository (but I'm open to suggestions, as always).

@jspahrsummers
Copy link
Contributor

I can rebase against current master

We prefer merging instead of rebasing for published branches, because then the conflict resolution is apparent, and no history is changed or lost.

submit separate PR for each of those splits

👍

I was mostly following the CoreData insert*blah*:inManagedObjectContext: idiom

It's pretty bad in Core Data too. I don't particularly like that framework as inspiration for API design. :trollface:

GTRepository could be broken down into functional categories—like a category for branching, a category for refs, etc. Then the methods are still "on" GTRepository, but we don't have one .h/.m pair that's thousands of lines long.

@tiennou
Copy link
Contributor Author

tiennou commented Jan 17, 2014

OK, I'll publish PRs for some of those then.

+1 on the CoreData design. I just thought of "split" as "move stuff over to some other classes", while you see it as categories. Categories it will be then (I'll have to rewrite some of those PRs though).

@tiennou
Copy link
Contributor Author

tiennou commented Sep 12, 2014

Stop that. It's silly.

@tiennou tiennou closed this Sep 12, 2014
@tiennou tiennou deleted the split-gtrepo branch August 15, 2016 14:41
@tiennou tiennou mentioned this pull request Aug 15, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tiennou @dannygreg @joshaber @jspahrsummers