Re: [BUG?] fetch into shallow sends a large number of objects

From: Jeff King <peff@peff.net>
Date: Mar 8, 2016, 13:30 UTC
Message-ID: <20160308133041.GA9465@sigill.intra.peff.net>
In-Reply-To: <20160308132524.GA22866@sigill.intra.peff.net>

On Tue, Mar 08, 2016 at 08:25:24AM -0500, Jeff King wrote:

Show 12 quoted lines

> > Good news. We have the mechanism in place, I think.
> > get_shallow_commits_by_rev_list() (from 'pu') will produce the right
> > shallow points for sending back to the client if you pass "--not
> > <current shallow points>" to it. It's meant to be used for
> > --shallow-exclude and --shallow-since, but if neither is given (nor
> > --depth) I guess we can run it with current shallow points. I wonder
> > if we can detect some common cases and avoid commit traversing this
> > way though.
> 
> I tried that, but I couldn't quite get it to work. I don't think we need
> any special rev-list, though; we can just find the boundary points of
> that traversal and mark them as new shallows.

By the way, I found a bug during my initial attempts with get_shallow_commits_by_rev_list(). One of the loops uses "p" to traverse a linked list, and then re-uses "p" again for another list traversal inside the body of the loop. When the inner loop finishes, "p" is left as NULL, and then the outer loop tries to access "p->next", which segfaults.

I _think_ this is just a mistaken re-use of the variable, and can be fixed with a new iteration variable for the inner loop, like:

diff --git a/shallow.c b/shallow.c
index 6ceb3f8..d600947 100644
--- a/shallow.c
+++ b/shallow.c
@@ -188,13 +188,14 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,
 	 */
 	for (p = not_shallow_list; p; p = p->next) {
 		struct commit *c = p->item;
+		struct commit_list *parent;
 
 		if (parse_commit(c))
 			die("unable to parse commit %s",
 			    oid_to_hex(&c->object.oid));
 
-		for (p = c->parents; p; p = p->next)
-			if (!(p->item->object.flags & not_shallow_flag)) {
+		for (parent = c->parents; parent; parent = parent->next)
+			if (!(parent->item->object.flags & not_shallow_flag)) {
 				c->object.flags |= shallow_flag;
 				commit_list_insert(c, &result);
 				break;

As I said, I didn't end up using this function either way, but you
probably want the fix above for the rest of your series. :)

-Peff