Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects

From: Matthijs Kooijman <matthijs@stdin.nl>
Date: Jul 12, 2013, 07:11 UTC
Message-ID: <20130712071157.GL10217@login.drsnuggles.stderr.nl>
In-Reply-To: <7vsizkpv21.fsf@alter.siamese.dyndns.org>

Hi Junio,

> [administrivia: you seem to have mail-followup-to that points at you
> and the list; is that really needed???]

I'm not subscribed to the list, so yes :-)

> > This happens when a client issues a fetch with a depth bigger or equal
> > to the number of commits the server is ahead of the client.
> 
> Do you mean "smaller" (not "bigger")?

Yes, I meant smaller (reworded this first sentence a few times and then messed up :-)

Show 21 quoted lines

> > diff --git a/upload-pack.c b/upload-pack.c
> > index 59f43d1..5885f33 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -122,6 +122,14 @@ static int do_rev_list(int in, int out, void *user_data)
> >  	if (prepare_revision_walk(&revs))
> >  		die("revision walk setup failed");
> >  	mark_edges_uninteresting(revs.commits, &revs, show_edge);
> > +	/* In case we create a new shallow root, make sure that all
> > +	 * we don't send over objects that the client already has just
> > +	 * because their "have" revisions are no longer reachable from
> > +	 * the shallow root. */
> > +	for (i = 0; i < have_obj.nr; i++) {
> > +		struct commit *commit = (struct commit *)have_obj.objects[i].item;
> > +		mark_tree_uninteresting(commit->tree);
> > +	}
> 
> Hmph.
> 
> In your discussion (including the comment), you talk about "shallow
> root" (I think that is the same as what we call "shallow boundary"),

I think so, yes. I mean to refer to the commits referenced in .git/shallow, that have their parents "hidden".

Show 5 quoted lines

> but in this added block, there is nothing that checks CLIENT_SHALLOW
> or SHALLOW flags to special case that.
>
> Is it a good idea to unconditionally do this for all "have"
> revisions?

That's what I meant in my mail with "applying the fix unconditionally" - there is probably some check needed (I discussed a few options in the mail as well).

Note that this entire do_rev_list function is only called when there are shallow revisions involved, so there is also a basic "only when shallow" check in place.

> Also there is another loop that iterates over "have" revisions just
> above the precontext.  I wonder if this added code belongs in that
> loop.

I think we could add it there, yes. On the other hand, if we only want to execute this code when there are shallow boundaries in the list of revisions to send (as I suggested in my previous mail), then we can't move this code up.

Gr.

Matthijs