Re: [PATCH v3 4/4] clone: open a shortcut for connectivity check

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

In order to make sure the cloned repository is good, we run "rev-list
--objects --not --all $new_refs" on the repository. This is expensive
on large repositories. This patch attempts to mitigate the impact in
this special case.

In the "good" clone case, we only have one pack.

If "On large repositories" is the focus, we need to take into
account the fact that pack.packSizeLimit can split and store the
incoming packstream to multiple packs, so "only have one pack" is
misleading.

I think you can still do the same trick even when we split the pack
as index-pack will keep track of the objects it saw in the same
incoming pack stream (but I am writing this from memory without
looking at the original code you are touching, so please double
check).

If all of the
following are met, we can be sure that all objects reachable from the
new refs exist, which is the intention of running "rev-list ...":

 - all refs point to an object in the pack
 - there are no dangling pointers in any object in the pack
 - no objects in the pack point to objects outside the pack

The second and third checks can be done with the help of index-pack as
a slight variation of --strict check (which introduces a new condition
for the shortcut: pack transfer must be used and the number of objects
large enough to call index-pack). The first is checked in
check_everything_connected after we get an "ok" from index-pack.

"index-pack + new checks" is still faster than the current "index-pack
+ rev-list", which is the whole point of this patch. If any of the

Does the same check apply if we end up on the unpack-objects
codepath?

This shortcut is not applied to shallow clones, partly because shallow
clones should have no more objects than a usual fetch and the cost of
rev-list is acceptable, partly to avoid dealing with corner cases when
grafting is involved.

On Fri, May 3, 2013 at 11:15 PM, Junio C Hamano [off-list ref] wrote:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

In order to make sure the cloned repository is good, we run "rev-list
--objects --not --all $new_refs" on the repository. This is expensive
on large repositories. This patch attempts to mitigate the impact in
this special case.

In the "good" clone case, we only have one pack.

If "On large repositories" is the focus, we need to take into
account the fact that pack.packSizeLimit can split and store the
incoming packstream to multiple packs, so "only have one pack" is
misleading.

I only had a quick look. But I don't think index-pack respects
packSizeLimit. pack-objects does but only when --stdout is not used,
which is not the case for pack transfer.

I think you can still do the same trick even when we split the pack
as index-pack will keep track of the objects it saw in the same
incoming pack stream (but I am writing this from memory without
looking at the original code you are touching, so please double
check).

Yeah. As long we have only one incoming stream, we can still do the
same verification.

quoted

"index-pack + new checks" is still faster than the current "index-pack
+ rev-list", which is the whole point of this patch. If any of the

Does the same check apply if we end up on the unpack-objects
codepath?

No. unpack-objects does not do this and check_everything_connected
should invoke rev-list like before.
--
Duy