transports: Avoid walking over non-commit objects by asandroq · Pull Request #4383 · libgit2/libgit2

asandroq · 2017-10-20T09:53:03Z

References may point to objects that are not commits, such as tags to
blobs. In that case the reference must not be walked over further.

Fixes #3595.

References may point to objects that are not commits, such as tags to blobs. In that case the reference must not be walked over further. Fixes libgit2#3595.

ethomson · 2017-10-20T10:15:10Z

+                if ((error = git_object_lookup(&oobj, repo, oid, GIT_OBJ_ANY)) < 0)
+                        goto on_error;
+
+                error = git_object_peel(&obj, oobj, GIT_OBJ_COMMIT);


Can you explain why are you peeling here? What type of objects could you find above (in the lookup) that would need to be peeled? It seems like this could be simplified as:

oid = git_reference_target(ref); if ((error = git_object_lookup(&obj, repo, oid, GIT_OBJ_ANY)) < 0) goto on_error; if (git_object_type(obj) != GIT_OBJ_COMMIT) { git_object_free(obj); continue; }

It might be nicer to specify the type to git_object_lookup and modify git_object_lookup to return a unique error code when the type of the object is not the type requested.

In my use-case the first object is actually an annotated tag, which when peeled points to a blob. But in other cases it could point to a commit, and in this case the commit can be added to the revwalk.

Got it, thanks for the explanation.

ethomson · 2017-10-20T10:18:25Z

Nice - thanks for the pull request and for adding tests! ✨

My assumption here is that there's not a significant perf impact to looking up these objects ahead of time just to validate their type... presumably they'll get cached and then the revwalk will pull them from the cache.

Another option is to move this logic into the revwalk enqueue functionality, which could prevent a second odb lookup.

ethomson · 2017-10-20T11:20:48Z

Right - looking at git_revwalk_push, it does this exact lookup and peel step... I think it would be nice to not duplicate that right before calling it.

Couple of options:

Introduce a new API that only pushes commits and ignores non-commits that get peeled that then calls push_commit with the from_glob option as false. (And probably renames from_globs to something like ignore_non_commits).
Disambiguate the error in push_commit so that it's catchable as a "this is not a commit". This seems like it would have broader uses for end users.

@carlosmn do you have an opinion here?

pks-t

This review is regarding your changes only, without judgement whether it'd be better to push this into git_revwalk_push.

Re @ethomson. I'm not a fan of the first option. It seems a bit fragile to simply pretend everything is alright when we in fact just ignore what we were told to do. The second possibility looks much saner to me. I think we should aim for that solution -- I'm not a fan of premature optimization, but if we can easily avoid a second lookup/peel while making the API more useful for callers it sounds like a win-win to me.

pks-t · 2017-10-20T12:44:34Z

 			continue;

-		if ((error = git_revwalk_push(walk, git_reference_target(ref))) < 0)
+                /* only walk over commits */


The comment here is a bit terse, in my opinion. I'd love to have it mention why we need the separate lookup/peel steps and why we need to filter out tags-to-blobs here if there's a comment 10 lines before which pretends to filter out tags already. I've been confused by that, as well, as I didn't expect you to have tags outside of the "refs/tags" hierarchy.

pks-t · 2017-10-20T12:44:56Z

+                git_object_free(oobj);
+                if (error == GIT_ENOTFOUND || error == GIT_EPEEL) {
+                        continue;
+                } else if (error != 0)


We usually avoid braces around single-line statements

pks-t · 2017-10-20T12:47:12Z


 	for (i = 0; i < refs.count; ++i) {
+                const git_oid *oid;
+                git_object *obj, *oobj;


These object are leaking as soon as we encounter an error. You could instead just initialize them to NULL, git_object_free at the beginning of each iteration as well as freeing in the on_error section. This requires us to define these variables at the beginning of the function.

pks-t · 2017-10-20T12:48:22Z

 }

 static const char *refnames[] = {
+	"refs/blobs/annotated_tag_to_blob",


This thing here was missing to complete my picture of your issue. I didn't realize you had tags in non-standard locations. Anyway, we should definitly be able to cope with that as users do strange things.

asandroq · 2017-10-23T08:50:47Z

Right - looking at git_revwalk_push, it does this exact lookup and peel step... I think it would be nice to not duplicate that right before calling it.

@ethomson, I've had this patch for a couple of years now so I forgot how I wrote it, but probably I got the code from there. :)

This situation was caught previously but returned a generic error code.

Instead, use the return code to check if reference does not point to a committish.

carlosmn · 2017-10-28T17:48:10Z

I'm not fond of adding a new error case for this. We already have error codes for peeling (though they are admittedly more complex than I'd like) which is what we're doing.

The point of the the revwalk during fetch is to convert as many reference tips into commits as we can, so we can reduce the amount of data that's transmitted, so we should go with that idea into the code. The loop under /* No tags */ is, as it has been pointed out, deficient, as it assumes only stuff under refs/tags/ is a commit, but then also fails to enqueue those, which could be a useful source of commits.

This code predates git_revwalk_push_glob which is why it simply uses git_revwalk_push and attempts to do the tag filtering on its own. I think the best way forward here would be to make use of that convenience function, which is made for precisely this kind of situation.

We should actually be able to get rid of the whole of fetch_setup_walk's body and replace it with

git_revwalk_new(&walk, repo);
git_revwalk_push_glob(&walk, "refs/*");

(plus error handling) with the extra advantage that we're using an iterator instead of having to allocate an array.

ethomson · 2019-07-31T09:01:16Z

Closed for #5195

transports: Avoid walking over non-commit objects

5cd67c3

References may point to objects that are not commits, such as tags to blobs. In that case the reference must not be walked over further. Fixes libgit2#3595.

ethomson reviewed Oct 20, 2017

View reviewed changes

pks-t requested changes Oct 20, 2017

View reviewed changes

asandroq added 2 commits October 27, 2017 14:38

errors: Add new error for objects which are not commits

93f7f95

This situation was caught previously but returned a generic error code.

transports: Do not duplicate code in git_revwalk_push

f126002

Instead, use the return code to check if reference does not point to a committish.

biodranik mentioned this pull request Jul 29, 2019

transports: Avoid walking over non-commit objects #5193

Closed

tiennou mentioned this pull request Jul 31, 2019

smart: use push_glob instead of manual filtering #5195

Merged

ethomson closed this Jul 31, 2019

Navigation Menu

Uh oh!

transports: Avoid walking over non-commit objects - #4383

transports: Avoid walking over non-commit objects#4383
asandroq wants to merge 3 commits into
libgit2:masterfrom
asandroq:fix-walk-on-tagged-blobs

asandroq commented Oct 20, 2017

Uh oh!

ethomson Oct 20, 2017

Uh oh!

asandroq Oct 20, 2017

Uh oh!

ethomson Oct 20, 2017

Uh oh!

ethomson commented Oct 20, 2017

Uh oh!

ethomson commented Oct 20, 2017

Uh oh!

pks-t left a comment

Uh oh!

pks-t Oct 20, 2017

Uh oh!

pks-t Oct 20, 2017

Uh oh!

pks-t Oct 20, 2017

Uh oh!

pks-t Oct 20, 2017

Uh oh!

asandroq commented Oct 23, 2017 •

edited

Loading

Uh oh!

carlosmn commented Oct 28, 2017

Uh oh!

ethomson commented Jul 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

asandroq commented Oct 20, 2017

Uh oh!

ethomson Oct 20, 2017

Choose a reason for hiding this comment

Uh oh!

asandroq Oct 20, 2017

Choose a reason for hiding this comment

Uh oh!

ethomson Oct 20, 2017

Choose a reason for hiding this comment

Uh oh!

ethomson commented Oct 20, 2017

Uh oh!

ethomson commented Oct 20, 2017

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

pks-t Oct 20, 2017

Choose a reason for hiding this comment

Uh oh!

pks-t Oct 20, 2017

Choose a reason for hiding this comment

Uh oh!

pks-t Oct 20, 2017

Choose a reason for hiding this comment

Uh oh!

pks-t Oct 20, 2017

Choose a reason for hiding this comment

Uh oh!

asandroq commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlosmn commented Oct 28, 2017

Uh oh!

ethomson commented Jul 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asandroq commented Oct 23, 2017 •

edited

Loading