Re: [PATCH v3 07/28] shallow.c: add remove_reachable_shallow_points()

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> When we receive a pack and the shallow points from another repository,
>> we may want to add more shallow points to current repo to make sure no
>> commits point to nowhere. However we do not want to add unnecessary
>> shallow points and cut our history short because the other end is a
>> shallow version of this repo. The output shallow points may or may not
>> be added to .git/shallow, depending on whether they are actually
>> reachable in the new pack.
>>
>> This function filters such shallow points out, leaving ones that might
>> potentially be added. A simple has_sha1_file won't do because we may
>> have incomplete object islands (i.e. not connected to any refs) and
>> the shallow points are on these islands. In that case we want to keep
>> the shallow points as candidates until we figure out if the new pack
>> connects to such object islands.
>>
>> Typical cases that use remove_reachable_shallow_points() are:
>>
>>  - fetch from a repo that has longer history: in this case all remote
>>    shallow roots do not exist in the client repo, this function will
>>    be cheap as it just does a few lookup_commit_graft and
>>    has_sha1_file.
>
> It is unclear why.  If you fetched from a repository more complete
> than you, you may deepen your history which may allow you to unplug
> the shallow points you originally had, and remote "shallow root" (by
> the way, lets find a single good phrase to represent this thing and
> stick to it) may want to replace them, no?

>>  - fetch from a repo that has exactly the same shallow root set
>>    (e.g. a clone from a shallow repo): this case may trigger
>>    in_merge_bases_many all the way to roots. An exception is made to
>>    avoid such costly path with a faith that .git/shallow does not
>>    usually points to unreachable commit islands.
>
> ... and when the faith is broken, you will end up with a broken
> repository???

>> +static int add_ref(const char *refname,
>> +                const unsigned char *sha1, int flags, void *cb_data)
>> +{
>> +     struct commit_array *ca = cb_data;
>> +     ALLOC_GROW(ca->commits, ca->nr + 1, ca->alloc);
>> +     ca->commits[ca->nr++] = lookup_commit(sha1);
>> +     return 0;
>> +}
>
> Can't a ref point at a non-commit-ish?  Is the code prepared to deal
> with such an entry (possibly a NULL pointer) in the commit_array
> struct?