{"thread":{"id":"48549","subject":"bug: --shallow-since misbehaves on old branch heads","startedAt":"2018-05-22T20:19:20Z","lastAt":"2018-06-04T14:44:40Z","messageCount":8,"participants":["Andreas Krey","Duy Nguyen","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"348327","messageId":"20180522194854.GA29564@inner.h.apk.li","threadId":"48549","inReplyTo":null,"subject":"bug: --shallow-since misbehaves on old branch heads","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2018-05-22T19:48:54Z","receivedAt":"2018-05-22T20:19:20Z","isPatch":false,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"Hi everybody,\n\nI think I have discovered a problem with clone/fetch --shallow-since:\nWhen a ref that is fetches has a head that is already older than\nthe 'since' time, then the entire branch is fetched, instead of\njust the current commit.\n\nRepro:\n\n rm -rf tmp out deep\n git init tmp\n for i in `seq 10 31`; do\n d=\"2017-05-${i}T12:00\"\n GIT_AUTOR_DATE=\"$d\" GIT_COMMITTER_DATE=\"$d\" git -C tmp commit -m nix$i --allow-empty\n done\n git -C tmp checkout -b test HEAD^^^\n for i in `seq 10 14`; do\n d=\"2017-06-${i}T12:00\"\n GIT_AUTHOR_DATE=\"$d\" GIT_COMMITTER_DATE=\"$d\" git -C tmp commit -m nax$i --allow-empty\n done\n for i in `seq 1 4`; do\n git -C tmp commit -m new$i --allow-empty\n done\n\n echo \"** This is fine:\"\n\n git clone --shallow-since '1 month ago' file://`pwd`/tmp out --branch test\n git -C out log --oneline\n\n echo \"** This should show one commit but shows all:\"\n\n git clone --shallow-since '1 month ago' file://`pwd`/tmp deep --branch master\n git -C deep log --oneline\n\nDo I expect the wrong thing?\n\n- Andreas\n\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds \nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"},{"id":"348377","messageId":"CACsJy8ChpbNM8sLzdptmzVp5XXF-rmQqxP+9CX5uJRa4e1kJvA@mail.gmail.com","threadId":"48549","inReplyTo":"20180522194854.GA29564@inner.h.apk.li","subject":"Re: bug: --shallow-since misbehaves on old branch heads","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-23T16:02:43Z","receivedAt":"2018-05-23T16:03:17Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I probably can't look into this until the weekend. Just wanted to let\nyou know that I've seen this mail and, being the one who introduced\n--shallow-since, I will look into it soon (unless someone beats me to\nit of course).\n-- \nDuy\n"},{"id":"348583","messageId":"20180526113518.22403-1-pclouds@gmail.com","threadId":"48549","inReplyTo":"20180522194854.GA29564@inner.h.apk.li","subject":"[PATCH] upload-pack: reject shallow requests that would return nothing","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-26T11:35:18Z","receivedAt":"2018-05-26T11:35:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Shallow clones with --shallow-since or --shalow-exclude work by\nrunning rev-list to get all reachable commits, then draw a boundary\nbetween reachable and unreachable and send \"shallow\" requests based on\nthat.\n\nThe code does miss one corner case: if rev-list returns nothing, we'll\nhave no border and we'll send no shallow requests back to the client\n(i.e. no history cuts). This essentially means a full clone (or a full\nbranch if the client requests just one branch). One example is the\noldest commit is older than what is specified by --shallow-since.\n\nTo avoid this, if rev-list returns nothing, we abort the clone/fetch.\nThe user could adjust their request (e.g. --shallow-since further back\nin the past) and retry.\n\nAnother possible option for this case is to fall back to a default\ndepth (like depth 1). But I don't like too much magic that way because\nwe may return something unexpected to the user. If they request\n\"history since 2008\" and we return a single depth at 2000, that might\nbreak stuff for them. It is better to tell them that something is\nwrong and let them take the best course of action.\n\nNote that we need to die() in get_shallow_commits_by_rev_list()\ninstead of just checking for empty result from its caller\ndeepen_by_rev_list() and handling the error there. The reason is,\nempty result could be a valid case: if you have commits in year 2013\nand you request --shallow-since=year.2000 then you should get a full\nclone (i.e. empty result).\n\nReported-by: Andreas Krey \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n shallow.c | 3 +++\n t/t5500-fetch-pack.sh | 11 +++++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/shallow.c b/shallow.c\nindex df4d44ea7a..44fdca1ace 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -175,6 +175,9 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,\n \t\tdie(\"revision walk setup failed\");\n \ttraverse_commit_list(&revs, show_commit, NULL, ¬_shallow_list);\n \n+\tif (!not_shallow_list)\n+\t\tdie(\"no commits selected for shallow requests\");\n+\n \t/* Mark all reachable commits as NOT_SHALLOW */\n \tfor (p = not_shallow_list; p; p = p->next)\n \t\tp->item->object.flags |= not_shallow_flag;\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 0680dec808..c8f2d38a58 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -711,6 +711,17 @@ test_expect_success 'fetch shallow since ...' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'clone shallow since selects no commits' '\n+\ttest_create_repo shallow-since-the-future &&\n+\t(\n+\tcd shallow-since-the-future &&\n+\tGIT_COMMITTER_DATE=\"100000000 +0700\" git commit --allow-empty -m one &&\n+\tGIT_COMMITTER_DATE=\"200000000 +0700\" git commit --allow-empty -m two &&\n+\tGIT_COMMITTER_DATE=\"300000000 +0700\" git commit --allow-empty -m three &&\n+\ttest_must_fail git clone --shallow-since \"900000000 +0700\" \"file://$(pwd)/.\" ../shallow111\n+\t)\n+'\n+\n test_expect_success 'shallow clone exclude tag two' '\n \ttest_create_repo shallow-exclude &&\n \t(\n-- \n2.17.0.705.g3525833791\n\n"},{"id":"348648","messageId":"xmqqy3g4jpck.fsf@gitster-ct.c.googlers.com","threadId":"48549","inReplyTo":"20180526113518.22403-1-pclouds@gmail.com","subject":"Re: [PATCH] upload-pack: reject shallow requests that would return nothing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-28T05:55:55Z","receivedAt":"2018-05-28T05:56:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> To avoid this, if rev-list returns nothing, we abort the clone/fetch.\n> The user could adjust their request (e.g. --shallow-since further back\n> in the past) and retry.\n\nYeah, that makes sense.\n\n> Another possible option for this case is to fall back to a default\n> depth (like depth 1). But I don't like too much magic that way because\n> we may return something unexpected to the user.\n\nI agree that it would be a horrible fallback. I actually am\nwondering if we should just silently return no objects without even\ntelling the user there is something unexpected happening. After\nall, the user may well be expecting with --shallow-since that is too\nrecent that the fetch may not result in pulling anything new, and\ngiving a \"die\" message, which now needs to be distinguished from\nother forms of die's like network connectivity or auth failures, is\nnot all that helpful.\n\n> Note that we need to die() in get_shallow_commits_by_rev_list()\n> instead of just checking for empty result from its caller\n> deepen_by_rev_list() and handling the error there. The reason is,\n> empty result could be a valid case: if you have commits in year 2013\n> and you request --shallow-since=year.2000 then you should get a full\n> clone (i.e. empty result).\n\nYup, that latter example makes me more convinced that it also is a\nvalid outcome if we end up requesting \"no\" object when shallow-since\nnames too recent date, e.g. against a project that is dormant since\n2013 with --shallow-since=2018/01/01 or something like that, instead\nof dying.\n\n> Reported-by: Andreas Krey \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> shallow.c | 3 +++\n> t/t5500-fetch-pack.sh | 11 +++++++++++\n> 2 files changed, 14 insertions(+)\n>\n> diff --git a/shallow.c b/shallow.c\n> index df4d44ea7a..44fdca1ace 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -175,6 +175,9 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,\n> \t\tdie(\"revision walk setup failed\");\n> \ttraverse_commit_list(&revs, show_commit, NULL, ¬_shallow_list);\n> \n> +\tif (!not_shallow_list)\n> +\t\tdie(\"no commits selected for shallow requests\");\n> +\n> \t/* Mark all reachable commits as NOT_SHALLOW */\n> \tfor (p = not_shallow_list; p; p = p->next)\n> \t\tp->item->object.flags |= not_shallow_flag;\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 0680dec808..c8f2d38a58 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -711,6 +711,17 @@ test_expect_success 'fetch shallow since ...' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'clone shallow since selects no commits' '\n> +\ttest_create_repo shallow-since-the-future &&\n> +\t(\n> +\tcd shallow-since-the-future &&\n> +\tGIT_COMMITTER_DATE=\"100000000 +0700\" git commit --allow-empty -m one &&\n> +\tGIT_COMMITTER_DATE=\"200000000 +0700\" git commit --allow-empty -m two &&\n> +\tGIT_COMMITTER_DATE=\"300000000 +0700\" git commit --allow-empty -m three &&\n> +\ttest_must_fail git clone --shallow-since \"900000000 +0700\" \"file://$(pwd)/.\" ../shallow111\n> +\t)\n> +'\n> +\n> test_expect_success 'shallow clone exclude tag two' '\n> \ttest_create_repo shallow-exclude &&\n> \t(\n"},{"id":"348682","messageId":"CACsJy8B7g-J8XeWYpY9SG1p51-fBLE2-V5872uWbyBXY2aVoEA@mail.gmail.com","threadId":"48549","inReplyTo":"xmqqy3g4jpck.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] upload-pack: reject shallow requests that would return nothing","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-28T18:48:51Z","receivedAt":"2018-05-28T18:49:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, May 28, 2018 at 7:55 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> To avoid this, if rev-list returns nothing, we abort the clone/fetch.\n>> The user could adjust their request (e.g. --shallow-since further back\n>> in the past) and retry.\n>\n> Yeah, that makes sense.\n>\n>> Another possible option for this case is to fall back to a default\n>> depth (like depth 1). But I don't like too much magic that way because\n>> we may return something unexpected to the user.\n>\n> I agree that it would be a horrible fallback. I actually am\n> wondering if we should just silently return no objects without even\n> telling the user there is something unexpected happening. After\n> all, the user may well be expecting with --shallow-since that is too\n> recent that the fetch may not result in pulling anything new, and\n> giving a \"die\" message, which now needs to be distinguished from\n> other forms of die's like network connectivity or auth failures, is\n> not all that helpful.\n\nAn empty fetch is probably ok (though I would need to double check if\nanything bad would happen or git-fetch would give some helpful\nsuggestion). git-clone on the other hand should actually clean this up\nwith a good advice. I'll need to check and come back with v2 later.\n-- \nDuy\n"},{"id":"349071","messageId":"CACsJy8D_RDhJTbMp26gN291piDAr5pPp7zcSmY7RHc_o-anB9g@mail.gmail.com","threadId":"48549","inReplyTo":"CACsJy8B7g-J8XeWYpY9SG1p51-fBLE2-V5872uWbyBXY2aVoEA@mail.gmail.com","subject":"Re: [PATCH] upload-pack: reject shallow requests that would return nothing","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-06-02T06:06:40Z","receivedAt":"2018-06-02T06:07:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, May 28, 2018 at 8:48 PM, Duy Nguyen wrote:\n> On Mon, May 28, 2018 at 7:55 AM, Junio C Hamano wrote:\n>> Nguyễn Thái Ngọc Duy writes:\n>>\n>>> To avoid this, if rev-list returns nothing, we abort the clone/fetch.\n>>> The user could adjust their request (e.g. --shallow-since further back\n>>> in the past) and retry.\n>>\n>> Yeah, that makes sense.\n>>\n>>> Another possible option for this case is to fall back to a default\n>>> depth (like depth 1). But I don't like too much magic that way because\n>>> we may return something unexpected to the user.\n>>\n>> I agree that it would be a horrible fallback. I actually am\n>> wondering if we should just silently return no objects without even\n>> telling the user there is something unexpected happening. After\n>> all, the user may well be expecting with --shallow-since that is too\n>> recent that the fetch may not result in pulling anything new, and\n>> giving a \"die\" message, which now needs to be distinguished from\n>> other forms of die's like network connectivity or auth failures, is\n>> not all that helpful.\n>\n> An empty fetch is probably ok (though I would need to double check if\n> anything bad would happen or git-fetch would give some helpful\n> suggestion). git-clone on the other hand should actually clean this up\n> with a good advice. I'll need to check and come back with v2 later.\n\nIt turns out harder than I thought. Cutting history and want/have\nnegotiation are separate but must be consistent. If during the\nnegotiation you said \"I'm giving you this ref at this SHA-1\" then you\nsend nothing back, the client will complain at connectivity check. It\nexpects all the objects that lead to said SHA-1.\n\nPart of the problem is we advertise refs very early, before accepting\nshallow requests and it's kinda hard to tell the user \"ok with this\nset of shallow requests, only these refs are actually valid and could\nbe sent back to you\" before the want/have negotiation starts. At the\ncurrent state, the only thing we could do is tell the user \"nak you\ncan't have that ref\" even if we advertise the ref. This might confuse\nclients and does not sound great.\n\nI think for now die() may be a good enough quick fix. I'm not sure if\nit's worth messing with the want/have negotiation just to cover this\nrare edge case.\n-- \nDuy\n"},{"id":"349225","messageId":"xmqq602y6d8c.fsf@gitster-ct.c.googlers.com","threadId":"48549","inReplyTo":"20180526113518.22403-1-pclouds@gmail.com","subject":"Re: [PATCH] upload-pack: reject shallow requests that would return nothing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-04T10:46:27Z","receivedAt":"2018-06-04T10:46:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Shallow clones with --shallow-since or --shalow-exclude work by\n> running rev-list to get all reachable commits, then draw a boundary\n> between reachable and unreachable and send \"shallow\" requests based on\n> that.\n>\n> The code does miss one corner case: if rev-list returns nothing, we'll\n> have no border and we'll send no shallow requests back to the client\n> (i.e. no history cuts). This essentially means a full clone (or a full\n> branch if the client requests just one branch). One example is the\n> oldest commit is older than what is specified by --shallow-since.\n\n\"the newest commit is older than\", isn't it? That is, the cutoff\npoint specified is newer than the existing history.\n"},{"id":"349251","messageId":"CACsJy8D5105r0=_pzfz8fTciQeXWCBe-qhsobR+_TJvfz7cv4Q@mail.gmail.com","threadId":"48549","inReplyTo":"xmqq602y6d8c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] upload-pack: reject shallow requests that would return nothing","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-06-04T14:44:07Z","receivedAt":"2018-06-04T14:44:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jun 4, 2018 at 12:46 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Shallow clones with --shallow-since or --shalow-exclude work by\n>> running rev-list to get all reachable commits, then draw a boundary\n>> between reachable and unreachable and send \"shallow\" requests based on\n>> that.\n>>\n>> The code does miss one corner case: if rev-list returns nothing, we'll\n>> have no border and we'll send no shallow requests back to the client\n>> (i.e. no history cuts). This essentially means a full clone (or a full\n>> branch if the client requests just one branch). One example is the\n>> oldest commit is older than what is specified by --shallow-since.\n>\n> \"the newest commit is older than\", isn't it? That is, the cutoff\n> point specified is newer than the existing history.\n\nYes. As a result, the entirely history is cut, including the tip.\n--shallow-exclude could also lead to this situation if the user\naccidentally excludes everything.\n-- \nDuy\n"}]}