Re: [PATCH v3 08/28] shallow.c: add mark_new_shallow_refs()

From: Duy Nguyen <pclouds@gmail.com>
Date: Nov 26, 2013, 13:18 UTC
Message-ID: <CACsJy8DgaqtQUohRwek2+cPxTEssvtDGb812Uhjy+R7pib2O8Q@mail.gmail.com>
In-Reply-To: <xmqqli0cw2b9.fsf@gitster.dls.corp.google.com>

On Tue, Nov 26, 2013 at 5:20 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Hmph.  the use of ->util field in this patch feels that it was
> something commit-slab data structure was invented to solve.

Good stuff! Thanks.

Show 18 quoted lines

>> +             if (c->util == NULL)
>> +                     c->util = bitmap;
>> +             else {
>> +                     /*
>> +                      * Deliberately leak a lot in commit->util
>> +                      * because there can be many pointers to the
>> +                      * same bitmap. Probably should allocate in a
>> +                      * pool and free the whole pool at the end.
>> +                      */
>
> ... or perhaps make the bitmap into
>
>         struct {
>                 int refcnt;
>                 uint32_t bits[FLEX_ARRAY];
>         }
>
> and refcnt them?

I still prefer memory pools so I just need to do a few free() than walking through all the commits again and refcnt-- or free() them.

Show 42 quoted lines

>> +     /*
>> +      * Quick check to see if we may need to add new shallow
>> +      * roots. Go through the list of root candidates and check if
>> +      * they exist (either in current repo, or in the new pack, we
>> +      * can't distinguish).
>> +      *
>> +      * 1) If none of the new roots exist, the pack must connect to
>> +      *    the main object graph, which is already guarded by
>> +      *    current repo's shallow roots and we will not need to
>> +      *    consider adding new shallow roots, so we can exit early.
>> +      *
>> +      * 2) The pack may connect to some existing object islands in
>> +      *    current repo then add shallow roots to plug loose ends
>> +      *    from those islands. In that case, new shallow roots must
>> +      *    also exist in the repo as this stage (old objects plus
>> +      *    the new pack).
>> +      *
>> +      * 3) The last, easiest case, is the pack contains some
>> +      *    shallow roots, which may be used to tie up loose ends of
>> +      *    some new refs, or redundanty (tying up loose ends of new
>> +      *    object islands)
>> +      */
>> +     for (i = 0;i < shallow->nr; i++)
>> +             if (has_sha1_file(shallow->array[i]))
>> +                     break;
>> +     if (i == shallow->nr)
>> +             /*
>> +              * this is the first and also the common case, where
>> +              * the new pack does not carry any new shallow
>> +              * points. No need to to the expensive commit traverse
>> +              * dance below.
>> +              */
>> +             return 0;
>
> I am Confused.
>
> The loop only made sure that all the elements in the array[] is
> still missing (or, ... is this function supposed to be called before
> installing a new pack???  It is unclear.  But if new objects were
> unpacked while receiving, then there is no "not install the new
> objects and check" possible, so I'd assume this is called after
> receiving and registering a new pack to the object store).

Yes it's called after installing the new packs (or after unpacking). I'll mention about this.

> But then, can it be that you had N-1 "shallow points" originally,
> the pack has a reference to a new missing commit, and the array has
> N "shallow points" in total?  Or is the caller expected to call this
> function with shallow pointing at a pre-transfer shallow points?

Another thing I did not mention is all shallow commits we have are already filtered out by remove_reachable_shallow_points. By the time you get here, the array only contains the shallow commits we don't have that may or may not be referenced by a something (oh yeah I did not handle tags!) in the new pack. So if all of them are missing (i.e. the new pack does not reference to any of them), they're useless and can be thrown away.

Sorry to break the patches this way and lose the overall call flow. It's just too big to put all into one patch. 13/28 is the one that put the pieces together but basically

 1. receive the remote's .git/shallow
 2. call remote_reachable_shallow_points() to exclude our shallow commits
 3. get the pack and install it (or unpack it)
 4. call this function to determine what new ref needs new shallow
commits from the result of #2

Show 11 quoted lines

>> +     /*
>> +      * Prepare the commit graph to track what refs can reach what
>> +      * (new) shallow points.
>> +      */
>> +     nr = get_max_object_index();
>
> Hmph. At this point (again, there is no description on where in the
> overall sequence of events this function is designed to be called,
> so it is impossible to review the logic), is it expected that we
> have seen all the objects under the sun and marked them in a
> specific way?

No. At this point we just make sure to have clean flags in commit objects. paint_down() is the one that walks through and marks them as seen.

-- 
Duy