Re: [PATCH v2 4/5] index-pack, unpack-objects: add --not-so-strict for connectivity check

Duy Nguyen [off-list ref] writes:

On Thu, May 2, 2013 at 6:35 AM, Junio C Hamano [off-list ref] wrote:

quoted

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

quoted

--not-so-strict only checks if all links from objects in the pack
point to real objects (either in current repo, or from the pack
itself). It's like check_everything_connected() except that:

 - it does not follow DAG in order
 - it can detect incomplete object islands

Could you clarify what this second point means?

"rev-list --objects --all --not $this $that" does not detect
"islands" but checking with the updated index-pack does?

Object islands (in the new pack) by definition are not connected to
the main DAG and so invisible to/unreachable from rev-list. index-pack
examines all objects in the pack and checks links of each object. With
this approach, islands are no different than reachable objects.

OK, so if you are fetching an updated tip of the main history, and a
new tip of a history that is disjoint. If we imagine that my public
repository just added the 'todo' branch and you are fecting them for
the first time. The history of 'todo' branch is an island that is
not connected anywhere from your refs namespace yet. In order to
ensure that updating the tip of fetched 'todo' is safe, you would
need to verify the island is free of dangling pointers and the only
thing you need to be sure is the tip of 'todo' is _in_ that island.

quoted

I am guessing that the code assumes that we are updating our refs to
objects that are in the pack that we are looking at, and I can see
how the new check in sha1_object() may detect an object that points
at another object that is missing.  But that assumption (which I
think is correct) is probably the most important thing to say in the
log message.

Yes, we need to make sure the new value of our refs are existing
objects. But it does not need to be in the new pack.

It is a bit more tricky than that.  A malicious (or simply buggy)
other side can send a subset of my 'todo' branch, which is an island
that is free of dangling pointers (think: 'rev-list --objects
todo~8').  Further imagine that you earlier attempted a fetch of the
same history from me over a commit walker and you happen to have
partial history near the tip of 'todo' but not connected to the
island.  sha1_object() will find it, but that does not say anything
useful.  The tip _must_ appear in the island for your check to yield
a usable result, no?

The existing "everything connected" was designed to protect against
that kind of breakage as well.

I might be reading your change incorrectly, but I am not sure how
the new code protects against such a breakage.

After index-pack
is run, we're guaranteed that all objects in repo are connected and
any of them could be new ref. This is also why I add has_sha1_file()
in clone.c.