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

On Fri, Mar 11, 2016 at 4:40 AM, Jeff King [off-list ref] wrote:

On Thu, Mar 10, 2016 at 01:26:08PM -0800, Junio C Hamano wrote:

quoted

quoted

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.

Well, assume again that F and G are ref heads, and their respective
distance to C and D are the same (like the below graph), then "fetch
--deptch=<distance>" can mark C and D as shallow cut points because
--depth traverses from refs until the distance is met, it does not do
total exclusion ^C like rev-list.

       --- B ---- C ---- H ---- F
          /      /
     --- D ---- E ---- G

OK, good. Now there are at least two of us who view it that way. :)

But going that direction just gives me more headache. If you two are
ok with this and nobody else complains, I'm ok too :-D I guess it's a
corner case anyway that's probably hard to occur in practice.

quoted

quoted

So what next? I think there's some protocol work here, and I think the
overall design of that needs to be considered alongside the other
"deepen" options your topic in pu adds (and of which I'm largely
ignorant). Does this sufficiently interest you to pick up and roll into
your other shallow work?

I can pick it up if you are busy with other stuff. But I'm also having
a couple other topics at the moment, so it may not progress very fast.

Thanks. I don't think it is too urgent; it has been that way for a
while. I certainly have plenty of other things to work on, but mostly I
just feel a bit out of my depth on the shallow stuff. I haven't given it
any real thought, and you obviously have.

More people understanding this code is always better though. But don't
worry I'll take care of this.

quoted

quoted

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?

Mostly, I wondered if we would need to send spontaneous shallow lines
for any other cases, and the client would not be able to say "I
understand them and want them in case A, but not in case B".

I do not have any case A in mind; it was just a general sense of "let's
make feature flags as specific as possible to avoid painting ourselves
into a corner". I'm OK with implementing it either way.

Another note to myself is, we do not want these spontaneous cutoff
requests to cut client's history even shorter (possibly by accident
because of buggy servers). As long as they are about sealing commits
that lead to non-existing objects, i think it's ok to make it a
general feature.
-- 
Duy