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

[Duy Nguyen <pclouds@gmail.com>]

On Tue, Nov 26, 2013 at 4:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

Except that deepen/shorten history is a different mode that this
function is not used at all. I should have made that clear. This and
the next patch are about "stick to our base and add something on top"

Any suggestions about a good phase? I've been swinging between
"shallow points" (from 4 months ago) and "shallow roots" (recently).

>>  - 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???

Not really broken because the new ref will be cut at the troublesome
shallow root before it goes out of bound, so the user may be surprised
that he got a history shorter than he wanted. It's when the root is
removed that we have a problem. But commits in .git/shallow are only
removed by

1) deepening history
2) the prune patch 28/28

#1 should send the missing objects and insert a new commit to
.git/shallow to plug the hole, so we're good. #2 only removes commits
from .git/shallow if they are not reachable from any refs, which is no
longer true.

>> +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?

Eck, yes a ref can. No the code is not :P Thanks for pointing this
out. We don't care about non-commit refs, so we just need to filter
them out.
-- 
Duy