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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2016, 21:26 UTC
Message-ID: <xmqqbn6mdnyn.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160310211052.GC30595@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 27 quoted lines

> On Thu, Mar 10, 2016 at 07:20:20PM +0700, Duy Nguyen wrote:
>
>> > +	else if (shallows.nr > 0) {
>> > +		struct rev_info revs;
>> > +		struct argv_array av = ARGV_ARRAY_INIT;
>> > +		struct commit *c;
>> > +		int i;
>> > +
>> > +		argv_array_push(&av, "rev-list");
>> > +		argv_array_push(&av, "--boundary");
>> 
>> Nice. I didn't know about --boundary. But will it work correctly in
>> this case?
>> 
>>        --- B ---- C ---- F
>>           /      /
>>      --- D ---- E ---- G
>> 
>> C and D will be current shallow cut points. People "want" F and G.
>> "rev-list --boundary F G ^C ^D" would mark E as boundary/shallow too,
>> correct? If so the history from G will be one depth short on a normal
>> fetch.
>
> IMHO, that is the right thing. They asked for "C" as a shallow cut-off
> point, so anything that is a parent of "C" should be omitted as shallow,
> too. It has nothing to do with the numeric depth, which was just the
> starting point for generating the shallow cutoffs.

I think that is the right mental model. The statement that "C and D are current cut points" does not make much sense. As you cannot rewrite parents of commits after the fact, you cannot construct a case like "when the shallow clone originally was made, two histories were forked long time before B and D, and the cloner ended up with C and D as the cutoff point, but now that we have the ancestry linkage between B and D (and C and E), we need to make E a new cutoff". The original "shallow" implementation does not store "starting point + number of depth" and instead translates that to the cut-off point for this exact reason.

Show 11 quoted lines

> Yeah, we definitely need an extension. I'm not sure if the extension
> should be "I know about spontaneous shallow/deepen responses; it's OK to
> send them to me" or "I want you to include the shallow points I send as
> boundary cutoffs for further shallow-ing of newly fetched history".
>
> They amount to the same thing when implementing _this_ feature, but the
> latter leaves us room in the future for a client to say "sure, I
> understand your spontaneous responses, but I explicitly _don't_ want you
> to do the boundary computation". I don't know if that is useful or not,
> but it might not hurt to have later on (and by adding it now, it "just
> works" later on with older servers/clients).

I am not sure what distinction you are worried about. An updated client that is capable of saying "you may give shallow/deepen responses to me" can optionally be told not to say it to the server, and that is equivalent to saying "I don't want you to send them", no?