{"thread":{"id":"56275","subject":"[PATCH v1 0/3] packfile-uri: http and https as default value of `--uri-protocol`","startedAt":"2021-08-13T09:15:15Z","lastAt":"2021-08-21T08:16:51Z","messageCount":7,"participants":["Teng Long","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"432663","messageId":"cover.1628845748.git.dyroneteng@gmail.com","threadId":"56275","inReplyTo":null,"subject":"[PATCH v1 0/3] packfile-uri: http and https as default value of `--uri-protocol`","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2021-08-13T09:14:53Z","receivedAt":"2021-08-13T09:15:15Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"The origin is from the reply by Ævar Arnfjörð Bjarmason:\n\n https://public-inbox.org/git/87a6m9ru85.fsf@evledraar.gmail.com\n\nTeng Long (3):\n packfile-uri: http and https as default value of `--uri-protocol`\n git-pack-objects.txt: introduce `--uri-protocol` option\n t5702: `fetch.uriprotocols` is configured without value\n\n Documentation/git-pack-objects.txt | 9 +++++++++\n builtin/pack-objects.c | 18 +++++++++++++++---\n fetch-pack.c | 13 ++++++++-----\n t/t5702-protocol-v2.sh | 23 +++++++++++++++++++++++\n 4 files changed, 55 insertions(+), 8 deletions(-)\n\n-- \n2.32.0.1.g0de8fe24d1.dirty\n\n"},{"id":"432664","messageId":"0754ea6472f2665a5fe77de080434d3734c67259.1628845748.git.dyroneteng@gmail.com","threadId":"56275","inReplyTo":"cover.1628845748.git.dyroneteng@gmail.com","subject":"[PATCH v1 1/3] packfile-uri: http and https as default value of `--uri-protocol`","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2021-08-13T09:14:54Z","receivedAt":"2021-08-13T09:15:21Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"HTTP(S) is usually one of the most common protocols, using \"http\" and\n\"https\" as the default parameter values of `--uri-protocol`, which can\nsimplify the use of packfile-uri feature on client side when run a\ngit-clone or git-fetch.\n\nSigned-off-by: Teng Long \n---\n builtin/pack-objects.c | 18 +++++++++++++++---\n fetch-pack.c | 13 ++++++++-----\n 2 files changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex de00adbb9e..5f6db92a4c 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3892,6 +3892,18 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n \treturn 0;\n }\n \n+static int option_parse_uri_protocol(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n+{\n+\tif (!arg) {\n+\t\tstring_list_append(&uri_protocols, \"http\");\n+\t\tstring_list_append(&uri_protocols, \"https\");\n+\t} else {\n+\t\tstring_list_append(&uri_protocols, arg);\n+\t}\n+\treturn 0;\n+}\n+\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n@@ -3995,9 +4007,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"do not pack objects in promisor packfiles\")),\n \t\tOPT_BOOL(0, \"delta-islands\", &use_delta_islands,\n \t\t\t N_(\"respect islands during delta compression\")),\n-\t\tOPT_STRING_LIST(0, \"uri-protocol\", &uri_protocols,\n-\t\t\t\tN_(\"protocol\"),\n-\t\t\t\tN_(\"exclude any configured uploadpack.blobpackfileuri with this protocol\")),\n+\t\tOPT_CALLBACK_F(0, \"uri-protocol\", NULL, N_(\"protocol\"),\n+\t\t\t N_(\"exclude any configured uploadpack.blobpackfileuri with this protocol\"),\n+\t\t\t PARSE_OPT_OPTARG, option_parse_uri_protocol),\n \t\tOPT_END(),\n \t};\n \ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex c135635e34..511a892d4c 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1755,11 +1755,14 @@ static void fetch_pack_config(void)\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n \tif (!uri_protocols.nr) {\n-\t\tchar *str;\n-\n-\t\tif (!git_config_get_string(\"fetch.uriprotocols\", &str) && str) {\n-\t\t\tstring_list_split(&uri_protocols, str, ',', -1);\n-\t\t\tfree(str);\n+\t\tconst char *value;\n+\n+\t\tif (!git_config_get_value(\"fetch.uriprotocols\", &value)) {\n+\t\t\tif (!value || !strcmp(value, \"\")) {\n+\t\t\t\tstring_list_append(&uri_protocols, \"http\");\n+\t\t\t\tstring_list_append(&uri_protocols, \"https\");\n+\t\t\t} else\n+\t\t\t\tstring_list_split(&uri_protocols, value, ',', -1);\n \t\t}\n \t}\n \n-- \n2.32.0.1.g0de8fe24d1.dirty\n\n"},{"id":"432665","messageId":"307a1837196a1508963a70bf4c0c4ac5727e4d19.1628845748.git.dyroneteng@gmail.com","threadId":"56275","inReplyTo":"cover.1628845748.git.dyroneteng@gmail.com","subject":"[PATCH v1 2/3] git-pack-objects.txt: introduce `--uri-protocol` option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2021-08-13T09:14:55Z","receivedAt":"2021-08-13T09:15:22Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Signed-off-by: Teng Long \n---\n Documentation/git-pack-objects.txt | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex 25d9fbe37a..16057b8c7d 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -313,6 +313,14 @@ raise an error.\n --unpack-unreachable::\n \tKeep unreachable objects in loose form. This implies `--revs`.\n \n+--uri-protocol::\n+\tExclude objects which hit the KEY (the object hash) of\n+\t`uploadpack.excludeobject` or `uploadpack.blobpackfileuri`\n+\tconfig (See linkgit:packfile-uri[1]) in repository and the\n+\tVALUE (the packfile uri) matches the given option parameter.\n+\tIf this option is set, but without a value, it defaults to\n+\tthe value will be \"http\" and \"https\".\n+\n --delta-islands::\n \tRestrict delta matches based on \"islands\". See DELTA ISLANDS\n \tbelow.\n@@ -426,6 +434,7 @@ SEE ALSO\n linkgit:git-rev-list[1]\n linkgit:git-repack[1]\n linkgit:git-prune-packed[1]\n+linkgit:packfile-uri[1]\n \n GIT\n ---\n-- \n2.32.0.1.g0de8fe24d1.dirty\n\n"},{"id":"432666","messageId":"3ee54255440fcc5cf549ab5e98fa51bbcd5df2e1.1628845748.git.dyroneteng@gmail.com","threadId":"56275","inReplyTo":"cover.1628845748.git.dyroneteng@gmail.com","subject":"[PATCH v1 3/3] t5702: `fetch.uriprotocols` is configured without value","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2021-08-13T09:14:56Z","receivedAt":"2021-08-13T09:15:30Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Signed-off-by: Teng Long \n---\n t/t5702-protocol-v2.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 66af411057..cf3bc89775 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -945,6 +945,29 @@ test_expect_success 'packfile URIs with fetch instead of clone' '\n \t\tfetch \"$HTTPD_URL/smart/http_parent\"\n '\n \n+test_expect_success 'packfile URIs with fetch by default `fetch.uriprotocols` config' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tconfigure_exclusion \"$P\" my-blob >h &&\n+\n+\tgit init http_child &&\n+\n+\tGIT_TEST_SIDEBAND_ALL=1 GIT_TRACE_PACKET=`pwd`/log \\\n+\tgit -C http_child \\\n+\t\t-c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols \\\n+\t\tfetch \"$HTTPD_URL/smart/http_parent\" &&\n+\tgrep \"git< packfile-uris http,https\" log\n+'\n+\n test_expect_success 'fetching with valid packfile URI but invalid hash fails' '\n \tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n \trm -rf \"$P\" http_child log &&\n-- \n2.32.0.1.g0de8fe24d1.dirty\n\n"},{"id":"433304","messageId":"87ilzzgr32.fsf@evledraar.gmail.com","threadId":"56275","inReplyTo":"307a1837196a1508963a70bf4c0c4ac5727e4d19.1628845748.git.dyroneteng@gmail.com","subject":"Re: [PATCH v1 2/3] git-pack-objects.txt: introduce `--uri-protocol` option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-21T08:05:08Z","receivedAt":"2021-08-21T08:06:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 13 2021, Teng Long wrote:\n\n> Signed-off-by: Teng Long \n> ---\n> Documentation/git-pack-objects.txt | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n>\n> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\n> index 25d9fbe37a..16057b8c7d 100644\n> --- a/Documentation/git-pack-objects.txt\n> +++ b/Documentation/git-pack-objects.txt\n> @@ -313,6 +313,14 @@ raise an error.\n> --unpack-unreachable::\n> \tKeep unreachable objects in loose form. This implies `--revs`.\n> \n> +--uri-protocol::\n> +\tExclude objects which hit the KEY (the object hash) of\n> +\t`uploadpack.excludeobject` or `uploadpack.blobpackfileuri`\n> +\tconfig (See linkgit:packfile-uri[1]) in repository and the\n> +\tVALUE (the packfile uri) matches the given option parameter.\n> +\tIf this option is set, but without a value, it defaults to\n> +\tthe value will be \"http\" and \"https\".\n> +\n> --delta-islands::\n> \tRestrict delta matches based on \"islands\". See DELTA ISLANDS\n> \tbelow.\n> @@ -426,6 +434,7 @@ SEE ALSO\n> linkgit:git-rev-list[1]\n> linkgit:git-repack[1]\n> linkgit:git-prune-packed[1]\n> +linkgit:packfile-uri[1]\n\nThis sort of link to technical/* doesn't work, see greeping for other\ntechnical/ resources, unfortunately we link to the generated HTML.\n\n(I have an unsubmitted series to clean that up and move them to the main\nmanpage space, but in the mentime...).\n"},{"id":"433305","messageId":"87fsv3gqv3.fsf@evledraar.gmail.com","threadId":"56275","inReplyTo":"0754ea6472f2665a5fe77de080434d3734c67259.1628845748.git.dyroneteng@gmail.com","subject":"Re: [PATCH v1 1/3] packfile-uri: http and https as default value of `--uri-protocol`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-21T08:08:33Z","receivedAt":"2021-08-21T08:10:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 13 2021, Teng Long wrote:\n\n> HTTP(S) is usually one of the most common protocols, using \"http\" and\n> \"https\" as the default parameter values of `--uri-protocol`, which can\n> simplify the use of packfile-uri feature on client side when run a\n> git-clone or git-fetch.\n>\n> Signed-off-by: Teng Long \n> ---\n> builtin/pack-objects.c | 18 +++++++++++++++---\n> fetch-pack.c | 13 ++++++++-----\n> 2 files changed, 23 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index de00adbb9e..5f6db92a4c 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3892,6 +3892,18 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n> \treturn 0;\n> }\n> \n> +static int option_parse_uri_protocol(const struct option *opt,\n> +\t\t\t\t const char *arg, int unset)\n> +{\n> +\tif (!arg) {\n> +\t\tstring_list_append(&uri_protocols, \"http\");\n> +\t\tstring_list_append(&uri_protocols, \"https\");\n> +\t} else {\n> +\t\tstring_list_append(&uri_protocols, arg);\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> {\n> \tint use_internal_rev_list = 0;\n> @@ -3995,9 +4007,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"do not pack objects in promisor packfiles\")),\n> \t\tOPT_BOOL(0, \"delta-islands\", &use_delta_islands,\n> \t\t\t N_(\"respect islands during delta compression\")),\n> -\t\tOPT_STRING_LIST(0, \"uri-protocol\", &uri_protocols,\n> -\t\t\t\tN_(\"protocol\"),\n> -\t\t\t\tN_(\"exclude any configured uploadpack.blobpackfileuri with this protocol\")),\n> +\t\tOPT_CALLBACK_F(0, \"uri-protocol\", NULL, N_(\"protocol\"),\n> +\t\t\t N_(\"exclude any configured uploadpack.blobpackfileuri with this protocol\"),\n> +\t\t\t PARSE_OPT_OPTARG, option_parse_uri_protocol),\n> \t\tOPT_END(),\n> \t};\n> \n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index c135635e34..511a892d4c 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1755,11 +1755,14 @@ static void fetch_pack_config(void)\n> \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n> \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n> \tif (!uri_protocols.nr) {\n> -\t\tchar *str;\n> -\n> -\t\tif (!git_config_get_string(\"fetch.uriprotocols\", &str) && str) {\n> -\t\t\tstring_list_split(&uri_protocols, str, ',', -1);\n> -\t\t\tfree(str);\n> +\t\tconst char *value;\n> +\n> +\t\tif (!git_config_get_value(\"fetch.uriprotocols\", &value)) {\n> +\t\t\tif (!value || !strcmp(value, \"\")) {\n> +\t\t\t\tstring_list_append(&uri_protocols, \"http\");\n> +\t\t\t\tstring_list_append(&uri_protocols, \"https\");\n> +\t\t\t} else\n> +\t\t\t\tstring_list_split(&uri_protocols, value, ',', -1);\n> \t\t}\n> \t}\n\nIsn't this adding support for:\n\n [fetch]\n uriProtocols\n\nI.e. the case where it's an existing key that we'll get a NULL from,\nditto the =\"\" case.\n\nPartially this issue pre-dates your series, but I think code here is\nrather unclear because we keep using the same comma-delimited\nrepresentation we want in the protocol for too long in fetch-pack.c et\nal.\n\nBut also, if we're going to make http/https the default, wouldn't\nparsing config/options it, and then interpreting the empty list as that\ndefault, as opposed to injecting it? Or have I misunderstood the intent\nhere...?\n\n"},{"id":"433306","messageId":"87czq7gqli.fsf@evledraar.gmail.com","threadId":"56275","inReplyTo":"cover.1628845748.git.dyroneteng@gmail.com","subject":"Re: [PATCH v1 0/3] packfile-uri: http and https as default value of `--uri-protocol`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-21T08:10:42Z","receivedAt":"2021-08-21T08:16:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 13 2021, Teng Long wrote:\n\n> The origin is from the reply by Ævar Arnfjörð Bjarmason:\n>\n> https://public-inbox.org/git/87a6m9ru85.fsf@evledraar.gmail.com\n\nThanks, I left some comments, see in particular the one on 1/3,\ni.e. maybe I'm confused about the goals here.\n\nI think the goal here makes sense, but that a better way to do this is\nto simply add a transfer.packfileURI setting, a boolean that defaults to\ntrue. See a similar transfer.bundleURI in a related series of mine:\nhttps://lore.kernel.org/git/RFC-patch-07.13-f0e4052de4-20210805T150534Z-avarab@gmail.com/\n\nI.e. in that series I also make use of fetch.uriProtocols to configure\nbundle-uri, so if it lands it'll be confusing to have a know to tweak\nbundle-uri, but not packfile-uri.\n\nBut even without it I think it's a lot less confusing to start\nintrodudcing client-specific settings for things we enable/disable in\nthe protocol explicitly, rather than implicitly by (in this case)\nsetting the allowed list of protocols.\n\nSo that transfer.{bundleURI,packfileURI}=[bool] suggestion, or perhaps\neven a more generic setting for directly turning on or off capabilities\nwithout having to introduce config handling for each\none. E.g. transferCapability.{bundle,packfile}-uri.disable=true. I.e. have\nserve.c and friends loop through transferCapability.*.disable and handle\nthings accordingly on the client.\n\n"}]}