Re: [PATCH] fetch: when deepening, check connectivity fully

[Jonathan Tan <jonathantanmy@google.com>]

> Hmph, remind me how old and/or new shallow cut-off points affect
> this traversal?  In order to verify that everything above the new
> cut-off points, opt->shallow_file should be pointing at the new
> cut-off points, then we do the full sweep (without --not --all) to
> ensure we won't find missing or broken objects but still stop at the
> new cut-off points, and then only after check_connected() passes,
> update the shallow file to make new shallow cut-off points active
> (and if the traversal fails, then we do nto install the new shallow
> cut-off points)?

That is the way it should work, but after thinking about it once more, I
realize that it isn't.

opt->shallow_file is not set to anything. And fetch-pack updates the
shallow file by itself (at least, that is my understanding of
update_shallow() in fetch-pack.c) before fetch calls check_connected(),
meaning that if check_connected() fails, there is still no rollback of
the shallow file.

So to directly answer your first question, only the new shallow cut-off
points affect this traversal, and not the old ones.

> If so, that sounds sensible.  By not having "--not --all", we
> potentially would do a full sweep, but if we are really deepening to
> the root commits, then we do need one full sweep anyway (as these
> deepest parts of the history were previously hidden by the shallow
> cutoff points), and if we are only deepening the history by a bit,
> even if we do not have "--not --all", we would hit the updated
> shallow cutoff points (which may be at older parts of the history)
> and stop, and for correctness we need to visit there anyway, so I
> think we are not being overly pessimistic with this change, either.

Yes, this analysis makes sense.