packfile-uri: http and https as default value of `--uri-protocol`
cover packfile-uri: http and https as default value of `--uri-protocol`
The origin is from the reply by Ævar Arnfjörð Bjarmason:
https://public-inbox.org/git/[email protected]
Teng Long (3):
packfile-uri: http and https as default value of --uri-protocol git-pack-objects.txt: introduce --uri-protocol option
t5702: fetch.uriprotocols is configured without value
Documentation/git-pack-objects.txt | 9 +++++++++ builtin/pack-objects.c | 18 +++++++++++++++--- fetch-pack.c | 13 ++++++++----- t/t5702-protocol-v2.sh | 23 +++++++++++++++++++++++ 4 files changed, 55 insertions(+), 8 deletions(-)
Signature · 1 line
2.32.0.1.g0de8fe24d1.dirty
PATCH 1/3 packfile-uri: http and https as default value of `--uri-protocol`
HTTP(S) is usually one of the most common protocols, using "http" and "https" as the default parameter values of --uri-protocol, which can simplify the use of packfile-uri feature on client side when run a git-clone or git-fetch.
--- builtin/pack-objects.c | 18 +++++++++++++++--- fetch-pack.c | 13 ++++++++----- 2 files changed, 23 insertions(+), 8 deletions(-)
builtin/pack-objects.c +15 -3
@@ -3892,6 +3892,18 @@ static int option_parse_unpack_unreachable(const struct option *opt,return 0 ; } +static int option_parse_uri_protocol (const struct option *opt, +const char *arg,int unset) +{ +if (!arg) { +string_list_append (&uri_protocols,"http" ); +string_list_append (&uri_protocols,"https" ); + }else { +string_list_append (&uri_protocols, arg); + } +return 0 ; +} +int cmd_pack_objects (int argc,const char **argv,const char *prefix) {int use_internal_rev_list =0 ;
@@ -3995,9 +4007,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) N_("do not pack objects in promisor packfiles")),OPT_BOOL (0 ,"delta-islands" , &use_delta_islands,N_ ("respect islands during delta compression" )), -OPT_STRING_LIST (0 ,"uri-protocol" , &uri_protocols, -N_ ("protocol" ), -N_ ("exclude any configured uploadpack.blobpackfileuri with this protocol" )), +OPT_CALLBACK_F (0 ,"uri-protocol" ,NULL ,N_ ("protocol" ), +N_ ("exclude any configured uploadpack.blobpackfileuri with this protocol" ), +PARSE_OPT_OPTARG , option_parse_uri_protocol), OPT_END(), };
fetch-pack.c +8 -5
@@ -1755,11 +1755,14 @@ static void fetch_pack_config(void)git_config_get_bool ("transfer.fsckobjects" , &transfer_fsck_objects);git_config_get_bool ("transfer.advertisesid" , &advertise_sid);if (!uri_protocols.nr) { -char *str; - -if (!git_config_get_string ("fetch.uriprotocols" , &str) && str) { -string_list_split (&uri_protocols, str,',' ,-1 ); -free (str); +const char *value; + +if (!git_config_get_value ("fetch.uriprotocols" , &value)) { +if (!value || !strcmp (value,"" )) { +string_list_append (&uri_protocols,"http" ); +string_list_append (&uri_protocols,"https" ); + }else +string_list_split (&uri_protocols, value,',' ,-1 ); } }
Signature · 1 line
2.32.0.1.g0de8fe24d1.dirty
Quoted 60 lines · Teng Long
HTTP(S) is usually one of the most common protocols, using "http" and "https" as the default parameter values of
--uri-protocol, which can simplify the use of packfile-uri feature on client side when run a git-clone or git-fetch.Signed-off-by Teng Long <[email protected]>--- builtin/pack-objects.c | 18 +++++++++++++++--- fetch-pack.c | 13 ++++++++----- 2 files changed, 23 insertions(+), 8 deletions(-)builtin/pack-objects.c +15
-3@@ -3892,6 +3892,18 @@ static int option_parse_unpack_unreachable(const struct option *opt,return 0 ; } +static int option_parse_uri_protocol (const struct option *opt, +const char *arg,int unset) +{ +if (!arg) { +string_list_append (&uri_protocols,"http" ); +string_list_append (&uri_protocols,"https" ); + }else { +string_list_append (&uri_protocols, arg); + } +return 0 ; +} +int cmd_pack_objects (int argc,const char **argv,const char *prefix) {int use_internal_rev_list =0 ;@@ -3995,9 +4007,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) N_("do not pack objects in promisor packfiles")),OPT_BOOL (0 ,"delta-islands" , &use_delta_islands,N_ ("respect islands during delta compression" )), -OPT_STRING_LIST (0 ,"uri-protocol" , &uri_protocols, -N_ ("protocol" ), -N_ ("exclude any configured uploadpack.blobpackfileuri with this protocol" )), +OPT_CALLBACK_F (0 ,"uri-protocol" ,NULL ,N_ ("protocol" ), +N_ ("exclude any configured uploadpack.blobpackfileuri with this protocol" ), +PARSE_OPT_OPTARG , option_parse_uri_protocol), OPT_END(), };fetch-pack.c +0
-5@@ -1755,11 +1755,14 @@ static void fetch_pack_config(void)git_config_get_bool ("transfer.fsckobjects" , &transfer_fsck_objects);git_config_get_bool ("transfer.advertisesid" , &advertise_sid);if (!uri_protocols.nr) { -char *str; - -if (!git_config_get_string ("fetch.uriprotocols" , &str) && str) { -string_list_split (&uri_protocols, str,',' ,-1 ); -free (str);
fetch-pack.c static void fetch_pack_config(void) · line 1767
Quoted 10 lines
fetch-pack.c +8
-0+const char *value; + +if (!git_config_get_value ("fetch.uriprotocols" , &value)) { +if (!value || !strcmp (value,"" )) { +string_list_append (&uri_protocols,"http" ); +string_list_append (&uri_protocols,"https" ); + }else +string_list_split (&uri_protocols, value,',' ,-1 ); } }
Isn't this adding support for:
[fetch]
uriProtocolsI.e. the case where it's an existing key that we'll get a NULL from, ditto the ="" case.
Partially this issue pre-dates your series, but I think code here is rather unclear because we keep using the same comma-delimited representation we want in the protocol for too long in fetch-pack.c et al.
But also, if we're going to make http/https the default, wouldn't parsing config/options it, and then interpreting the empty list as that default, as opposed to injecting it? Or have I misunderstood the intent here...?
PATCH 2/3 git-pack-objects.txt: introduce `--uri-protocol` option
In reply to Teng Long--- Documentation/git-pack-objects.txt | 9 +++++++++ 1 file changed, 9 insertions(+)
Documentation/git-pack-objects.txt +9 -0
@@ -313,6 +313,14 @@ raise an error. --unpack-unreachable:: Keep unreachable objects in loose form. This implies `--revs`. +--uri-protocol:: + Exclude objects which hit the KEY (the object hash) of + `uploadpack.excludeobject` or `uploadpack.blobpackfileuri` + config (See linkgit:packfile-uri[1]) in repository and the + VALUE (the packfile uri) matches the given option parameter. + If this option is set, but without a value, it defaults to + the value will be "http" and "https". + --delta-islands:: Restrict delta matches based on "islands". See DELTA ISLANDS below.
@@ -426,6 +434,7 @@ SEE ALSO linkgit:git-rev-list[1] linkgit:git-repack[1] linkgit:git-prune-packed[1] +linkgit:packfile-uri[1] GIT ---
Signature · 1 line
2.32.0.1.g0de8fe24d1.dirty
Re: [PATCH v1 2/3] git-pack-objects.txt: introduce `--uri-protocol` option
Quoted 25 lines · Teng Long
Signed-off-by Teng Long <[email protected]>--- Documentation/git-pack-objects.txt | 9 +++++++++ 1 file changed, 9 insertions(+)Documentation/git-pack-objects.txt +8
-0@@ -313,6 +313,14 @@ raise an error. --unpack-unreachable:: Keep unreachable objects in loose form. This implies `--revs`. +--uri-protocol:: + Exclude objects which hit the KEY (the object hash) of + `uploadpack.excludeobject` or `uploadpack.blobpackfileuri` + config (See linkgit:packfile-uri[1]) in repository and the + VALUE (the packfile uri) matches the given option parameter. + If this option is set, but without a value, it defaults to + the value will be "http" and "https". + --delta-islands:: Restrict delta matches based on "islands". See DELTA ISLANDS below.
Documentation/git-pack-objects.txt SEE ALSO · line 437
Quoted 4 lines
Documentation/git-pack-objects.txt +1
-0@@ -426,6 +434,7 @@ SEE ALSO linkgit:git-rev-list[1] linkgit:git-repack[1] linkgit:git-prune-packed[1] +linkgit:packfile-uri[1]
This sort of link to technical/* doesn't work, see greeping for other technical/ resources, unfortunately we link to the generated HTML.
(I have an unsubmitted series to clean that up and move them to the main manpage space, but in the mentime...).
PATCH 3/3 t5702: `fetch.uriprotocols` is configured without value
In reply to Teng Long--- t/t5702-protocol-v2.sh | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
t/t5702-protocol-v2.sh +23 -0
@@ -945,6 +945,29 @@ test_expect_success 'packfile URIs with fetch instead of clone' ' fetch "$HTTPD_URL/smart/http_parent" ' +test_expect_success 'packfile URIs with fetch by default `fetch.uriprotocols` config' ' + P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" && + rm -rf "$P" http_child log && + + git init "$P" && + git -C "$P" config "uploadpack.allowsidebandall" "true" && + + echo my-blob >"$P/my-blob" && + git -C "$P" add my-blob && + git -C "$P" commit -m x && + + configure_exclusion "$P" my-blob >h && + + git init http_child && + + GIT_TEST_SIDEBAND_ALL=1 GIT_TRACE_PACKET=`pwd`/log \ + git -C http_child \ + -c protocol.version=2 \ + -c fetch.uriprotocols \ + fetch "$HTTPD_URL/smart/http_parent" && + grep "git< packfile-uris http,https" log +' + test_expect_success 'fetching with valid packfile URI but invalid hash fails' ' P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" && rm -rf "$P" http_child log &&
Signature · 1 line
2.32.0.1.g0de8fe24d1.dirty
Quoted 3 lines · Teng Long
The origin is from the reply by Ævar Arnfjörð Bjarmason:
https://public-inbox.org/git/[email protected]
Thanks, I left some comments, see in particular the one on 1/3, i.e. maybe I'm confused about the goals here.
I think the goal here makes sense, but that a better way to do this is to simply add a transfer.packfileURI setting, a boolean that defaults to true. See a similar transfer.bundleURI in a related series of mine:
https://lore.kernel.org/git/[email protected]/
I.e. in that series I also make use of fetch.uriProtocols to configure bundle-uri, so if it lands it'll be confusing to have a know to tweak bundle-uri, but not packfile-uri.
But even without it I think it's a lot less confusing to start introdudcing client-specific settings for things we enable/disable in the protocol explicitly, rather than implicitly by (in this case) setting the allowed list of protocols.
So that transfer.{bundleURI,packfileURI}=[bool] suggestion, or perhaps even a more generic setting for directly turning on or off capabilities without having to introduce config handling for each
one. E.g. transferCapability.{bundle,packfile}-uri.disable=true. I.e. have serve.c and friends loop through transferCapability.*.disable and handle things accordingly on the client.