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

From: Duy Nguyen <pclouds@gmail.com>
Date: May 2, 2013, 09:53 UTC
Message-ID: <CACsJy8A2dYssdV7JHutYKgo-nZswBAuedXoJ=aygrVSR=JeTrw@mail.gmail.com>
In-Reply-To: <7vvc722s0h.fsf@alter.siamese.dyndns.org>

On Thu, May 2, 2013 at 6:35 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> --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.

Show 12 quoted lines

>>  - it seems to be faster than "rev-list --objects --all"
>
> More important is that it makes sure that it is safe to update our
> refs to the new value, just like the check this attempts to replace.
> If that is not the case, the speed does not matter.
>
> 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. 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. I forget if I have checked for similar call in fetch.c and receive-pack.c. Will look again (and update log message)

>> +--not-so-strict::
>
> Perhaps "--check-connectivity" is a better name than this?

Definitely. -- Duy