Revparse rework #818

Merged
merged 5 commits into from Jul 21, 2012

Conversation

Projects
None yet
6 participants
Member

nulltoken commented Jul 12, 2012

Most of the task focused on two different angles

  • Improve the chaining support
  • Ease the memory management from the caller standpoint by making sure a function never returns the same instance of an object it has been fed with

This pull request fails (merged 4491c65c into 9f99c5d).

This pull request fails (merged 7a1be40a into 0848ec2).

This pull request fails (merged c27099f2 into 0848ec2).

This pull request fails (merged b045287b into 7b8c9e1).

This pull request fails (merged 541a6187 into 7b8c9e1).

This pull request fails (merged 1a024b6a into 7b8c9e1).

This pull request passes (merged dafcacd8 into d1af70b).

This pull request passes (merged c28d9130 into d1af70b).

This pull request passes (merged f1f8c6df into d1af70b).

This pull request passes (merged ba133e0c into d1af70b).

Member

nulltoken commented Jul 14, 2012

@benstraub It's not final yet, but if you're still ok for a review, I'd be delighted!

@carlosmn It looks like I've plugged the remaining leaks

This pull request fails (merged c2d8f10d into d1af70b).

This pull request passes (merged ac690a2b into d1af70b).

This pull request passes (merged 9453ec53 into d1af70b).

This pull request passes (merged cbf9ad4e into d1af70b).

Member

nulltoken commented Jul 15, 2012

@carlosmn git_object_peel() proposal (cf #530)

@vmg vmg and 1 other commented on an outdated diff Jul 15, 2012

src/object.c
+{
+ git_object *source, *deref = NULL;
+
+ assert(object);
+
+ if (git_object_type(object) == target_type)
+ return git_object_lookup(
+ peeled,
+ git_object_owner(object),
+ git_object_id(object),
+ target_type);
+
+ if (target_type == GIT_OBJ_BLOB
+ || target_type == GIT_OBJ_ANY
+ || git_object_type(object) == GIT_OBJ_BLOB)
+ return GIT_EAMBIGUOUS;
@vmg

vmg Jul 15, 2012

Owner

wrong error code; also you need to set a message

@nulltoken

nulltoken Jul 15, 2012

Member

Would that be better?

git_object_type(object) == GIT_OBJ_BLOB -> GIT_ERROR
target_type == GIT_OBJ_BLOB || target_type == GIT_OBJ_ANY -> GIT_EAMBIGUOUS

@vmg vmg and 1 other commented on an outdated diff Jul 15, 2012

src/revparse.c
- for (i = 0; i < len; i++)
- if (!git__isdigit(str[i])) return 0;
+ if (git__strtol32(&content, curly_braces_content, &end_ptr, 10) < 0)
+ return 0;
+
+ if (*end_ptr != '\0')
+ return 0;
@vmg

vmg Jul 15, 2012

Owner

This is inconsistent with all the other helpers. It should return a negative value on failure.

This pull request passes (merged ad0a289b into d4b5735).

@vmg vmg commented on the diff Jul 17, 2012

src/revparse.c
@@ -27,34 +27,6 @@ static int revspec_error(const char *revspec)
return -1;
@vmg

vmg Jul 17, 2012

Owner

The revparse_state enum is no longer being used, you should kill it.

@ben ben and 2 others commented on an outdated diff Jul 17, 2012

src/object.c
+
+int git_object_peel(
+ git_object **peeled,
+ git_object *object,
+ git_otype target_type)
+{
+ git_object *source, *deref = NULL;
+
+ assert(object);
+
+ if (git_object_type(object) == target_type)
+ return git_object_lookup(
+ peeled,
+ git_object_owner(object),
+ git_object_id(object),
+ target_type);
@ben

ben Jul 17, 2012

Member

It looks like the intent here is just to increase the refcount of the input object. Should we make a git_object_dup utility for this? It would be less expensive than doing a full lookup, and more semantic.

I'm also inspired by the use of git_object_owner; we might be able to avoid passing a git_repository * in a lot of places.

@vmg

vmg Jul 17, 2012

Owner

Well, given that this is an internal operation, we could simply increase the refcount for the object before returning it. I'd rather do this than add a public git_object_dup.

@ben

ben Jul 17, 2012

Member

git_object__dup? I hate to be poking into struct internals in code outside of object.c.

@nulltoken

nulltoken Jul 17, 2012

Member

@tanoku @benstraub git_object__dup added and deployed.

@ben ben commented on the diff Jul 17, 2012

src/revparse.c
}
-static int walk_ref_history(git_object **out, git_repository *repo, const char *refspec, const char *reflogspec)
+static int retrieve_previously_checked_out_branch_or_revision(git_object **out, git_reference **base_ref, git_repository *repo, const char *spec, const char *identifier, unsigned int position)
@ben

ben Jul 17, 2012

Member

Your C# is showing: a 49-character function name. :wink2:

@nulltoken

nulltoken Jul 17, 2012

Member

You should peek at my tests, you might find some with even longer names :D

Member

ben commented Jul 17, 2012

Overall, 👍. I like the style, it's much more straightforward than the state machine was. Nice work!

This pull request passes (merged e2c81fc into 5a8204f).

Member

nulltoken commented Jul 17, 2012

it's much more straightforward than the state machine was. Nice work!

@benstraub You previously did all the heavy lifting. I only made some tests pass ❤️

@vmg vmg added a commit that referenced this pull request Jul 21, 2012

@vmg vmg Merge pull request #818 from nulltoken/rework
Revparse rework
5b78696

@vmg vmg merged commit 5b78696 into libgit2:development Jul 21, 2012

Contributor

KindDragon commented on src/revparse.c in b8748c1 Mar 16, 2013

Why you compare with 0 two times?

Member

arrbee replied Mar 16, 2013

Put another way: that test will always be false. ((x < 0) < 0) is zero for all x.

@phatblat phatblat pushed a commit to phatblat/libgit2 that referenced this pull request Sep 13, 2014

@vmg vmg Merge pull request #818 from nulltoken/rework
Revparse rework
fe9c99e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment