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

Status Next #239

Merged
merged 97 commits into from
Oct 10, 2013
Merged

Status Next #239

merged 97 commits into from
Oct 10, 2013

Conversation

dannygreg
Copy link
Contributor

This brings Objective-Git's wrapping of the status up-to-date with the lurvely new status list API. Adding some unit tests in the process.

We also fix up our horrendous handling of fixtures, including their creation and cleanup.

@rowanj
Copy link
Contributor

rowanj commented Aug 19, 2013

I like the look of this; the status enumeration was tougher to get right than I'd like in my GUI project, so 👍 for any work to improve that.

Seems to still be missing a wrapper to get the status of a particular file (git_status_file(...)); have you got any thoughts on adding something like that?

@dannygreg
Copy link
Contributor Author

As of yet no… as I thought there would be very few cases where you would actually want to do that.

We could go with a GTStatusList object to match the API from libgit2 but I'm not sold on how great an idea that is.

@dannygreg
Copy link
Contributor Author

Removed the [WIP] tag so folks can review this for reals now. Still not sure how on earth we should test the staged status.

@jspahrsummers
Copy link
Contributor

I haven't reviewed this yet, but you can test staged stuff by manually manipulating GTIndex in the tests.

@dannygreg
Copy link
Contributor Author

Ended up using the libgit2 index API directly in the end. GTIndex could certainly use some love.

@dannygreg
Copy link
Contributor Author

Just noticed I need to document a bunch of stuff in this PR. Chucking [WIP] back on till I have cleared that up.

Conflicts:
	Classes/GTRepository.h
	Classes/ObjectiveGit.h
	External/libextobjc/extobjc/EXTScope.h
	External/libextobjc/extobjc/EXTScope.m
	ObjectiveGitFramework.xcodeproj/project.pbxproj
@dannygreg
Copy link
Contributor Author

🍍

It was all commented out anyway ¬.¬.
We seemed to have 2 completely different codepaths, which depending on
which you picked, would lead to a different set of fixtures being used
by your test. This was also the root cause of our faulty clean up.

Now there is but one set of fixtures and it's set up and torn down
properly.
Some test case classes needed to be changed to be GTTestCase but there
were no ill-effects.

Hell yeah duplicated code --;
@dannygreg
Copy link
Contributor Author

OK I yak shaved our horrendous fixture creation and management. Unfortunately that now makes this changeset huge, but hey.

@jspahrsummers this is good for another look through now.

git_strarray strArray = pathSpec.git_strarray;
if (pathSpec != nil) newOptions.pathspec = strArray;
@onExit {
git_strarray_free((git_strarray *)&strArray);
Copy link
Contributor

Choose a reason for hiding this comment

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

lol blocks and const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not entirely sure what you mean :p

@jspahrsummers
Copy link
Contributor

💖 for fixing up the unit tests.

@dannygreg
Copy link
Contributor Author

🚑

@jspahrsummers
Copy link
Contributor

🚒

jspahrsummers added a commit that referenced this pull request Oct 10, 2013
@jspahrsummers jspahrsummers merged commit 64e32d2 into master Oct 10, 2013
@jspahrsummers jspahrsummers deleted the status-take-2 branch October 10, 2013 07:08
phatblat pushed a commit to phatblat/objective-git that referenced this pull request Sep 13, 2014
@jspahrsummers jspahrsummers removed their assignment May 22, 2015
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.

None yet

7 participants