{"thread":{"id":"35408","subject":"Re: [PATCH v3 07/28] shallow.c: add remove_reachable_shallow_points()","startedAt":"2013-11-26T12:56:04Z","lastAt":"2013-11-26T12:56:04Z","messageCount":1,"participants":["Duy Nguyen"],"isPatch":true,"patchVersion":3,"patchTotal":28},"messages":[{"id":"231135","messageId":"CACsJy8D+8DFL298_mqUDbp=Hk8b=oiDnWgpw=xf3V02=S0mdYA@mail.gmail.com","threadId":"35408","inReplyTo":null,"subject":"Re: [PATCH v3 07/28] shallow.c: add remove_reachable_shallow_points()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-11-26T12:56:04Z","receivedAt":"2013-11-26T12:56:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 26, 2013 at 4:53 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> When we receive a pack and the shallow points from another repository,\n>> we may want to add more shallow points to current repo to make sure no\n>> commits point to nowhere. However we do not want to add unnecessary\n>> shallow points and cut our history short because the other end is a\n>> shallow version of this repo. The output shallow points may or may not\n>> be added to .git/shallow, depending on whether they are actually\n>> reachable in the new pack.\n>>\n>> This function filters such shallow points out, leaving ones that might\n>> potentially be added. A simple has_sha1_file won't do because we may\n>> have incomplete object islands (i.e. not connected to any refs) and\n>> the shallow points are on these islands. In that case we want to keep\n>> the shallow points as candidates until we figure out if the new pack\n>> connects to such object islands.\n>>\n>> Typical cases that use remove_reachable_shallow_points() are:\n>>\n>>  - fetch from a repo that has longer history: in this case all remote\n>>    shallow roots do not exist in the client repo, this function will\n>>    be cheap as it just does a few lookup_commit_graft and\n>>    has_sha1_file.\n>\n> It is unclear why.  If you fetched from a repository more complete\n> than you, you may deepen your history which may allow you to unplug\n> the shallow points you originally had, and remote \"shallow root\" (by\n> the way, lets find a single good phrase to represent this thing and\n> stick to it) may want to replace them, no?\n\nExcept that deepen/shorten history is a different mode that this\nfunction is not used at all. I should have made that clear. This and\nthe next patch are about \"stick to our base and add something on top\"\n\nAny suggestions about a good phase? I've been swinging between\n\"shallow points\" (from 4 months ago) and \"shallow roots\" (recently).\n\n>>  - fetch from a repo that has exactly the same shallow root set\n>>    (e.g. a clone from a shallow repo): this case may trigger\n>>    in_merge_bases_many all the way to roots. An exception is made to\n>>    avoid such costly path with a faith that .git/shallow does not\n>>    usually points to unreachable commit islands.\n>\n> ... and when the faith is broken, you will end up with a broken\n> repository???\n\nNot really broken because the new ref will be cut at the troublesome\nshallow root before it goes out of bound, so the user may be surprised\nthat he got a history shorter than he wanted. It's when the root is\nremoved that we have a problem. But commits in .git/shallow are only\nremoved by\n\n1) deepening history\n2) the prune patch 28/28\n\n#1 should send the missing objects and insert a new commit to\n.git/shallow to plug the hole, so we're good. #2 only removes commits\nfrom .git/shallow if they are not reachable from any refs, which is no\nlonger true.\n\n>> +static int add_ref(const char *refname,\n>> +                const unsigned char *sha1, int flags, void *cb_data)\n>> +{\n>> +     struct commit_array *ca = cb_data;\n>> +     ALLOC_GROW(ca->commits, ca->nr + 1, ca->alloc);\n>> +     ca->commits[ca->nr++] = lookup_commit(sha1);\n>> +     return 0;\n>> +}\n>\n> Can't a ref point at a non-commit-ish?  Is the code prepared to deal\n> with such an entry (possibly a NULL pointer) in the commit_array\n> struct?\n\nEck, yes a ref can. No the code is not :P Thanks for pointing this\nout. We don't care about non-commit refs, so we just need to filter\nthem out.\n-- \nDuy\n"}]}