{"thread":{"id":"62445","subject":"[PATCH 0/2] A few --shallow-exclude fixes","startedAt":"2024-11-04T19:02:48Z","lastAt":"2024-11-05T06:55:03Z","messageCount":7,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"506554","messageId":"pull.1822.git.1730746964.gitgitgadget@gmail.com","threadId":"62445","inReplyTo":null,"subject":"[PATCH 0/2] A few --shallow-exclude fixes","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-04T19:02:42Z","receivedAt":"2024-11-04T19:02:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Here's a couple patches to fix up an error message and the documentation\naround the --shallow-exclude option.\n\nElijah Newren (2):\n upload-pack: fix ambiguous error message\n doc: correct misleading descriptions for --shallow-exclude\n\n Documentation/fetch-options.txt | 2 +-\n Documentation/git-clone.txt | 2 +-\n Documentation/git-fetch-pack.txt | 2 +-\n builtin/clone.c | 2 +-\n builtin/fetch.c | 4 ++--\n builtin/pull.c | 4 ++--\n t/t5500-fetch-pack.sh | 7 +++++++\n upload-pack.c | 6 +++++-\n 8 files changed, 20 insertions(+), 9 deletions(-)\n\n\nbase-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1822%2Fnewren%2Fshallow-exclude-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1822/newren/shallow-exclude-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1822\n-- \ngitgitgadget\n"},{"id":"506555","messageId":"bb8dbc5c7086ce5c09f8980c923ec85da38c9147.1730746964.git.gitgitgadget@gmail.com","threadId":"62445","inReplyTo":"pull.1822.git.1730746964.gitgitgadget@gmail.com","subject":"[PATCH 1/2] upload-pack: fix ambiguous error message","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-04T19:02:43Z","receivedAt":"2024-11-04T19:02:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nupload-pack.c takes any --shallow-exclude argument(s) from\nclone/fetch/etc. and passes them through expand_ref(). If it does not\nget back exactly one ref from the call to expand_ref(), it will die with\nthe following error:\n\n fatal: git upload-pack: ambiguous deepen-not: %s\n\nGiven that the documentation suggests to users that --shallow-exclude\naccepts a revision rather than a ref (which will be corrected in a\nsubsequent commit), users may try to pass a revision. In such a case,\nexpand_ref() will return 0 matches, but the error message we print will\nbe misleading since \"ambiguous\" suggests there are multiple matches.\nProvide a clearer error message for such a case.\n\nSigned-off-by: Elijah Newren \n---\n t/t5500-fetch-pack.sh | 7 +++++++\n upload-pack.c | 6 +++++-\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 8da8e7fe423..6552da78d19 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -926,6 +926,13 @@ test_expect_success 'fetch exclude tag one' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'fetch exclude tag one as revision' '\n+\ttest_when_finished rm -f rev err &&\n+\tgit -C shallow-exclude rev-parse one >rev &&\n+\ttest_must_fail git -C shallow12 fetch --shallow-exclude $(cat rev) origin 2>err &&\n+\tgrep \"deepen-not is not a ref:\" err\n+'\n+\n test_expect_success 'fetching deepen' '\n \ttest_create_repo shallow-deepen &&\n \t(\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 6d6e0f9f980..640d45295e1 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1025,10 +1025,14 @@ static int process_deepen_not(const char *line, struct oidset *deepen_not, int *\n {\n \tconst char *arg;\n \tif (skip_prefix(line, \"deepen-not \", &arg)) {\n+\t\tint cnt;\n \t\tchar *ref = NULL;\n \t\tstruct object_id oid;\n-\t\tif (expand_ref(the_repository, arg, strlen(arg), &oid, &ref) != 1)\n+\t\tcnt = expand_ref(the_repository, arg, strlen(arg), &oid, &ref);\n+\t\tif (cnt > 1)\n \t\t\tdie(\"git upload-pack: ambiguous deepen-not: %s\", line);\n+\t\tif (cnt < 1)\n+\t\t\tdie(\"git upload-pack: deepen-not is not a ref: %s\", line);\n \t\toidset_insert(deepen_not, &oid);\n \t\tfree(ref);\n \t\t*deepen_rev_list = 1;\n-- \ngitgitgadget\n\n"},{"id":"506556","messageId":"4c7f2dba686266be50cf11420beca633e1691dd4.1730746964.git.gitgitgadget@gmail.com","threadId":"62445","inReplyTo":"pull.1822.git.1730746964.gitgitgadget@gmail.com","subject":"[PATCH 2/2] doc: correct misleading descriptions for --shallow-exclude","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-04T19:02:44Z","receivedAt":"2024-11-04T19:02:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe documentation for the --shallow-exclude option to clone/fetch/etc.\nclaims that the option takes a revision, but it does not. As per\nupload-pack.c's process_deepen_not(), it passes the option to\nexpand_ref() and dies if it does not find exactly one ref matching the\nname passed. Further, this has always been the case ever since these\noptions were introduced by the commits merged in a460ea4a3cb1 (Merge\nbranch 'nd/shallow-deepen', 2016-10-10). Fix the documentation to\nmatch the implementation.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/fetch-options.txt | 2 +-\n Documentation/git-clone.txt | 2 +-\n Documentation/git-fetch-pack.txt | 2 +-\n builtin/clone.c | 2 +-\n builtin/fetch.c | 4 ++--\n builtin/pull.c | 4 ++--\n 6 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 9dc7ac8dbdc..b01372e4b3c 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -29,7 +29,7 @@\n \tDeepen or shorten the history of a shallow repository to\n \tinclude all reachable commits after .\n \n---shallow-exclude=::\n+--shallow-exclude=::\n \tDeepen or shorten the history of a shallow repository to\n \texclude commits reachable from a specified remote branch or tag.\n \tThis option can be specified multiple times.\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 116ad648201..7acb4cb1761 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -255,7 +255,7 @@ corresponding `--mirror` and `--no-tags` options instead.\n `--shallow-since=`::\n \tCreate a shallow clone with a history after the specified time.\n \n-`--shallow-exclude=`::\n+`--shallow-exclude=`::\n \tCreate a shallow clone with a history, excluding commits\n \treachable from a specified remote branch or tag. This option\n \tcan be specified multiple times.\ndiff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\nindex b3467664d30..b5223576a75 100644\n--- a/Documentation/git-fetch-pack.txt\n+++ b/Documentation/git-fetch-pack.txt\n@@ -91,7 +91,7 @@ be in a separate packet, and the list must end with a flush packet.\n \tDeepen or shorten the history of a shallow repository to\n \tinclude all reachable commits after .\n \n---shallow-exclude=::\n+--shallow-exclude=::\n \tDeepen or shorten the history of a shallow repository to\n \texclude commits reachable from a specified remote branch or tag.\n \tThis option can be specified multiple times.\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 59fcb317a68..93fe6d69659 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -147,7 +147,7 @@ static struct option builtin_clone_options[] = {\n \t\t N_(\"create a shallow clone of that depth\")),\n \tOPT_STRING(0, \"shallow-since\", &option_since, N_(\"time\"),\n \t\t N_(\"create a shallow clone since a specific time\")),\n-\tOPT_STRING_LIST(0, \"shallow-exclude\", &option_not, N_(\"revision\"),\n+\tOPT_STRING_LIST(0, \"shallow-exclude\", &option_not, N_(\"ref\"),\n \t\t\tN_(\"deepen history of shallow clone, excluding rev\")),\n \tOPT_BOOL(0, \"single-branch\", &option_single_branch,\n \t\t N_(\"clone only one branch, HEAD or --branch\")),\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex d9027e4dc92..18eff4e5fa1 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2216,8 +2216,8 @@ int cmd_fetch(int argc,\n \t\t\t N_(\"deepen history of shallow clone\")),\n \t\tOPT_STRING(0, \"shallow-since\", &deepen_since, N_(\"time\"),\n \t\t\t N_(\"deepen history of shallow repository based on time\")),\n-\t\tOPT_STRING_LIST(0, \"shallow-exclude\", &deepen_not, N_(\"revision\"),\n-\t\t\t\tN_(\"deepen history of shallow clone, excluding rev\")),\n+\t\tOPT_STRING_LIST(0, \"shallow-exclude\", &deepen_not, N_(\"ref\"),\n+\t\t\t\tN_(\"deepen history of shallow clone, excluding ref\")),\n \t\tOPT_INTEGER(0, \"deepen\", &deepen_relative,\n \t\t\t N_(\"deepen history of shallow clone\")),\n \t\tOPT_SET_INT_F(0, \"unshallow\", &unshallow,\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 388ef3d1306..edc56907aa2 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -218,8 +218,8 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU_ARGV(0, \"shallow-since\", &opt_fetch, N_(\"time\"),\n \t\tN_(\"deepen history of shallow repository based on time\"),\n \t\t0),\n-\tOPT_PASSTHRU_ARGV(0, \"shallow-exclude\", &opt_fetch, N_(\"revision\"),\n-\t\tN_(\"deepen history of shallow clone, excluding rev\"),\n+\tOPT_PASSTHRU_ARGV(0, \"shallow-exclude\", &opt_fetch, N_(\"ref\"),\n+\t\tN_(\"deepen history of shallow clone, excluding ref\"),\n \t\t0),\n \tOPT_PASSTHRU_ARGV(0, \"deepen\", &opt_fetch, N_(\"n\"),\n \t\tN_(\"deepen history of shallow clone\"),\n-- \ngitgitgadget\n"},{"id":"506586","messageId":"xmqqzfmetqq4.fsf@gitster.g","threadId":"62445","inReplyTo":"pull.1822.git.1730746964.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] A few --shallow-exclude fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-05T01:27:15Z","receivedAt":"2024-11-05T01:27:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> Here's a couple patches to fix up an error message and the documentation\n> around the --shallow-exclude option.\n>\n> Elijah Newren (2):\n> upload-pack: fix ambiguous error message\n> doc: correct misleading descriptions for --shallow-exclude\n\nBoth patches look sensible.\n\nWill queue. Thanks.\n"},{"id":"506630","messageId":"Zym65pRWPqu9VIvq@pks.im","threadId":"62445","inReplyTo":"bb8dbc5c7086ce5c09f8980c923ec85da38c9147.1730746964.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] upload-pack: fix ambiguous error message","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-05T06:27:56Z","receivedAt":"2024-11-05T06:28:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Nov 04, 2024 at 07:02:43PM +0000, Elijah Newren via GitGitGadget wrote:\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 6d6e0f9f980..640d45295e1 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1025,10 +1025,14 @@ static int process_deepen_not(const char *line, struct oidset *deepen_not, int *\n> {\n> \tconst char *arg;\n> \tif (skip_prefix(line, \"deepen-not \", &arg)) {\n> +\t\tint cnt;\n> \t\tchar *ref = NULL;\n> \t\tstruct object_id oid;\n> -\t\tif (expand_ref(the_repository, arg, strlen(arg), &oid, &ref) != 1)\n> +\t\tcnt = expand_ref(the_repository, arg, strlen(arg), &oid, &ref);\n> +\t\tif (cnt > 1)\n> \t\t\tdie(\"git upload-pack: ambiguous deepen-not: %s\", line);\n> +\t\tif (cnt < 1)\n> +\t\t\tdie(\"git upload-pack: deepen-not is not a ref: %s\", line);\n\nI was wondering whether `expand_ref()` could ever return negative, but\nafter reading through its implementation that doesn't seem to be the\ncase. It's somewhat misleading that it returns `int`, as `size_t` would\nconvey the return value in a better spirit.\n\nAnyway, that is not an issue of this patch series, and the change looks\ngood.\n\nPatrick\n"},{"id":"506631","messageId":"Zym6723csrUdiC5A@pks.im","threadId":"62445","inReplyTo":"4c7f2dba686266be50cf11420beca633e1691dd4.1730746964.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] doc: correct misleading descriptions for --shallow-exclude","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-05T06:27:59Z","receivedAt":"2024-11-05T06:28:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Nov 04, 2024 at 07:02:44PM +0000, Elijah Newren via GitGitGadget wrote:\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 59fcb317a68..93fe6d69659 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -147,7 +147,7 @@ static struct option builtin_clone_options[] = {\n> \t\t N_(\"create a shallow clone of that depth\")),\n> \tOPT_STRING(0, \"shallow-since\", &option_since, N_(\"time\"),\n> \t\t N_(\"create a shallow clone since a specific time\")),\n> -\tOPT_STRING_LIST(0, \"shallow-exclude\", &option_not, N_(\"revision\"),\n> +\tOPT_STRING_LIST(0, \"shallow-exclude\", &option_not, N_(\"ref\"),\n> \t\t\tN_(\"deepen history of shallow clone, excluding rev\")),\n\nWe also need to replace \"rev\" with \"ref\" here. We already do that in the\nother cases.\n\nPatrick\n"},{"id":"506635","messageId":"xmqq34k6tbjv.fsf@gitster.g","threadId":"62445","inReplyTo":"Zym6723csrUdiC5A@pks.im","subject":"Re: [PATCH 2/2] doc: correct misleading descriptions for --shallow-exclude","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-05T06:55:00Z","receivedAt":"2024-11-05T06:55:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Nov 04, 2024 at 07:02:44PM +0000, Elijah Newren via GitGitGadget wrote:\n>> diff --git a/builtin/clone.c b/builtin/clone.c\n>> index 59fcb317a68..93fe6d69659 100644\n>> --- a/builtin/clone.c\n>> +++ b/builtin/clone.c\n>> @@ -147,7 +147,7 @@ static struct option builtin_clone_options[] = {\n>> \t\t N_(\"create a shallow clone of that depth\")),\n>> \tOPT_STRING(0, \"shallow-since\", &option_since, N_(\"time\"),\n>> \t\t N_(\"create a shallow clone since a specific time\")),\n>> -\tOPT_STRING_LIST(0, \"shallow-exclude\", &option_not, N_(\"revision\"),\n>> +\tOPT_STRING_LIST(0, \"shallow-exclude\", &option_not, N_(\"ref\"),\n>> \t\t\tN_(\"deepen history of shallow clone, excluding rev\")),\n>\n> We also need to replace \"rev\" with \"ref\" here. We already do that in the\n> other cases.\n\nAh, I missed that one. Let me locally amend, as there does not seem\nto be any other issues.\n"}]}