git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] Packfile-uris support excluding commit objects

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
May 10, 2021, 11:14 UTC
Message-ID
<87wns6u8pc.fsf@evledraar.gmail.com>
In-Reply-To
<20210507021140.31372-1-dyroneteng@gmail.com>
On Fri, May 07 2021, Teng Long wrote:

It seems like this and your http://lore.kernel.org/git/20210506073354.27833-1-dyroneteng@gmail.com should be part of one series, not split up.

> On the server, more sophisticated means of excluding objects should be
> supported, such as commit object. This commit introduces a new
> configuration `uploadpack.commitpackfileuri` for this.

Per my understanding in https://lore.kernel.org/git/87o8hk820f.fsf@evledraar.gmail.com/ this + Jonathan's earlier bfc2a36ff2a (Doc: clarify contents of packfile sent as URI, 2021-01-20) still makes this whole thing more confusing that it needs to be.

I think we should just have a new uploadpack.excludeObject, and document that uploadpack.blobpackfileuri is an (unfortunately named) synonym for it. I.e. the actual implementation doesn't care about the objec type it just excludes any object listed via an oidmap. No?

As for some comments on the implementation:
> This patch only pack the commit object, not including the that commit
> and all objects that it references. This work will be done in a further
> patch recently.

I realize you're probably not a native English speaker (neither am I), but I honestly can't understand that "This work will be done in a further patch recently.". Do you mean something like:

    This change does not add support for recursively excluding things
    referenced by container objects such as "commit", "tag", and
    "tree". We'll still just dumbly exclude that specific object (this
    was originally meant for specific "blobs"). Smartly excluding things
    recursively might be implemented by a future change.
> Similarly, there are related documents that will be included in
> subsequent patches.

Please send the earlier doc cleanup + the spec change for this + any doc updates as one series.

Narrow comments on the patch:
Show 18 quoted lines
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  builtin/pack-objects.c |  8 ++---
>  fetch-pack.c           |  8 +++++
>  t/t5702-protocol-v2.sh | 71 +++++++++++++++++++++++++++++++++---------
>  upload-pack.c          |  7 +++--
>  4 files changed, 73 insertions(+), 21 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 6d13cd3e1a..2f1817fe28 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2985,7 +2985,7 @@ static int git_pack_config(const char *k, const char *v, void *cb)
>  			pack_idx_opts.flags &= ~WRITE_REV;
>  		return 0;
>  	}
> -	if (!strcmp(k, "uploadpack.blobpackfileuri")) {
> +    if (!strcmp(k, "uploadpack.blobpackfileuri") || !strcmp(k, "uploadpack.commitpackfileuri")) {
Nit: Split this across two lines.
Show 11 quoted lines
>  		struct configured_exclusion *ex = xmalloc(sizeof(*ex));
>  		const char *oid_end, *pack_end;
>  		/*
> @@ -2998,11 +2998,11 @@ static int git_pack_config(const char *k, const char *v, void *cb)
>  		    *oid_end != ' ' ||
>  		    parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||
>  		    *pack_end != ' ')
> -			die(_("value of uploadpack.blobpackfileuri must be "
> -			      "of the form '<object-hash> <pack-hash> <uri>' (got '%s')"), v);
> +            die(_("value of uploadpack.blobpackfileuri or upload.commitpackfileuri must be "
> +                  "of the form '<object-hash> <pack-hash> <uri>' (got '%s')"), v);
Indending with spaces.
>  		if (oidmap_get(&configured_exclusions, &ex->e.oid))
>  			die(_("object already configured in another "
> -			      "uploadpack.blobpackfileuri (got '%s')"), v);
> +			      "uploadpack.blobpackfileuri or uploadpack.commitpackfileuri (got '%s')"), v);

I think by having a uploadpack.excludeObject documented as the primary interface to this we could just say "object already listed by an earlier exclusion" or something like that.

Show 31 quoted lines
>  		ex->pack_hash_hex = xcalloc(1, pack_end - oid_end);
>  		memcpy(ex->pack_hash_hex, oid_end + 1, pack_end - oid_end - 1);
>  		ex->uri = xstrdup(pack_end + 1);
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 2318ebe680..24a947835b 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -23,6 +23,7 @@
>  #include "fetch-negotiator.h"
>  #include "fsck.h"
>  #include "shallow.h"
> +#include "strmap.h"
>  
>  static int transfer_unpack_limit = -1;
>  static int fetch_unpack_limit = -1;
> @@ -1677,6 +1678,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  		}
>  	}
>  
> +	struct strset uris;
> +	strset_init(&uris);
>  	for (i = 0; i < packfile_uris.nr; i++) {
>  		int j;
>  		struct child_process cmd = CHILD_PROCESS_INIT;
> @@ -1684,6 +1687,11 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  		const char *uri = packfile_uris.items[i].string +
>  			the_hash_algo->hexsz + 1;
>  
> +        if (strset_contains(&uris, uri)) {
> +            continue;
> +        }
More indenting with spaces, also don't need the {} here.
Show 21 quoted lines
> +
> +        strset_add(&uris, uri);
>  		strvec_push(&cmd.args, "http-fetch");
>  		strvec_pushf(&cmd.args, "--packfile=%.*s",
>  			     (int) the_hash_algo->hexsz,
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 2e1243ca40..d444177fb5 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -824,12 +824,22 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
>  '
>  
>  configure_exclusion () {
> -	git -C "$1" hash-object "$2" >objh &&
> -	git -C "$1" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
> -	git -C "$1" config --add \
> -		"uploadpack.blobpackfileuri" \
> -		"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
> -	cat objh
> +    if test "$1" = "blob"
> +        then
Don't indent the "then", also spaces...
Show 8 quoted lines
> +            git -C "$2" hash-object "$3" >objh &&
> +            git -C "$2" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
> +            git -C "$2" config --add \
> +            		"uploadpack.blobpackfileuri" \
> +            		"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
> +            cat objh
> +        else
> +            echo "$3" > objh &&
Use ">objh" not "> objh".
Show 5 quoted lines
> +            git -C "$2" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
> +            git -C "$2" config --add \
> +            		"uploadpack.commitpackfileuri" \
> +            		"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
> +            cat objh

This whole if/else seems like it could be better split up by discovering the variable first, using that as a variable, and then avoiding the duplication. But if we just used uploadpack.excludeObject...

Show 40 quoted lines
> +    fi
>  }
>  
>  test_expect_success 'part of packfile response provided as URI' '
> @@ -845,8 +855,8 @@ test_expect_success 'part of packfile response provided as URI' '
>  	git -C "$P" add other-blob &&
>  	git -C "$P" commit -m x &&
>  
> -	configure_exclusion "$P" my-blob >h &&
> -	configure_exclusion "$P" other-blob >h2 &&
> +	configure_exclusion blob "$P" my-blob >h &&
> +	configure_exclusion blob "$P" other-blob >h2 &&
>  
>  	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
>  	git -c protocol.version=2 \
> @@ -881,7 +891,7 @@ test_expect_success 'part of packfile response provided as URI' '
>  	test_line_count = 6 filelist
>  '
>  
> -test_expect_success 'packfile URIs with fetch instead of clone' '
> +test_expect_success 'blobs packfile URIs with fetch instead of clone' '
>  	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
>  	rm -rf "$P" http_child log &&
>  
> @@ -892,7 +902,7 @@ test_expect_success 'packfile URIs with fetch instead of clone' '
>  	git -C "$P" add my-blob &&
>  	git -C "$P" commit -m x &&
>  
> -	configure_exclusion "$P" my-blob >h &&
> +	configure_exclusion blob "$P" my-blob >h &&
>  
>  	git init http_child &&
>  
> @@ -902,6 +912,37 @@ test_expect_success 'packfile URIs with fetch instead of clone' '
>  		fetch "$HTTPD_URL/smart/http_parent"
>  '
>  
> +test_expect_success 'commits packfile URIs with fetch instead of clone' '
> +	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
> +	rm -rf "$P" http_child log &&
Put stuff like this in "test_when_finished"
Show 7 quoted lines
> +
> +	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 &&
You can just use test_commit here, no?
Show 6 quoted lines
> +
> +
> +	mycommit=$(git -C "$P" rev-parse HEAD) &&
> +    echo other-blob >"$P/other-blob" &&
> +    git -C "$P" add other-blob &&
> +    git -C "$P" commit -m x &&
ditto test_commit.
> +	othercommit=$(git -C "$P" rev-parse HEAD) &&
> +	configure_exclusion commit "$P" "$mycommit" >h &&
> +	configure_exclusion commit "$P" "$othercommit" >h2 &&

Personally I'd just skip this whole "rev-parse HEAD" etc. and just pass the tag name(s) created by earlier test_commit, then have configure_exclusion ust always do a rev-parse...

Show 79 quoted lines
> +
> +	git init http_child &&
> +
> +	GIT_TRACE=1 GIT_TEST_SIDEBAND_ALL=1 \
> +	git -C http_child -c protocol.version=2 \
> +		-c fetch.uriprotocols=http,https \
> +		fetch "$HTTPD_URL/smart/http_parent" &&
> +	ls http_child/.git/objects/pack/*.pack \
> +    	    http_child/.git/objects/pack/*.idx >filelist &&
> +    	test_line_count = 6 filelist
> +'
> +
>  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 &&
> @@ -915,7 +956,7 @@ test_expect_success 'fetching with valid packfile URI but invalid hash fails' '
>  	git -C "$P" add other-blob &&
>  	git -C "$P" commit -m x &&
>  
> -	configure_exclusion "$P" my-blob >h &&
> +	configure_exclusion blob "$P" my-blob >h &&
>  	# Configure a URL for other-blob. Just reuse the hash of the object as
>  	# the hash of the packfile, since the hash does not matter for this
>  	# test as long as it is not the hash of the pack, and it is of the
> @@ -944,7 +985,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects' '
>  	git -C "$P" add my-blob &&
>  	git -C "$P" commit -m x &&
>  
> -	configure_exclusion "$P" my-blob >h &&
> +	configure_exclusion blob "$P" my-blob >h &&
>  
>  	sane_unset GIT_TEST_SIDEBAND_ALL &&
>  	git -c protocol.version=2 -c transfer.fsckobjects=1 \
> @@ -978,7 +1019,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails on bad object'
>  	git -C "$P" add my-blob &&
>  	git -C "$P" commit -m x &&
>  
> -	configure_exclusion "$P" my-blob >h &&
> +	configure_exclusion blob "$P" my-blob >h &&
>  
>  	sane_unset GIT_TEST_SIDEBAND_ALL &&
>  	test_must_fail git -c protocol.version=2 -c transfer.fsckobjects=1 \
> @@ -1000,7 +1041,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects succeeds when .gitmo
>  	git -C "$P" add .gitmodules &&
>  	git -C "$P" commit -m x &&
>  
> -	configure_exclusion "$P" .gitmodules >h &&
> +	configure_exclusion blob "$P" .gitmodules >h &&
>  
>  	sane_unset GIT_TEST_SIDEBAND_ALL &&
>  	git -c protocol.version=2 -c transfer.fsckobjects=1 \
> @@ -1026,7 +1067,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul
>  	git -C "$P" add .gitmodules &&
>  	git -C "$P" commit -m x &&
>  
> -	configure_exclusion "$P" .gitmodules >h &&
> +	configure_exclusion blob "$P" .gitmodules >h &&
>  
>  	sane_unset GIT_TEST_SIDEBAND_ALL &&
>  	test_must_fail git -c protocol.version=2 -c transfer.fsckobjects=1 \
> diff --git a/upload-pack.c b/upload-pack.c
> index 5c1cd19612..34f8bb81a8 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1744,9 +1744,12 @@ int upload_pack_advertise(struct repository *r,
>  		     allow_sideband_all_value))
>  			strbuf_addstr(value, " sideband-all");
>  
> -		if (!repo_config_get_string(the_repository,
> +		if ((!repo_config_get_string(the_repository,
>  					    "uploadpack.blobpackfileuri",
> -					    &str) &&
> +					    &str) ||
> +            !repo_config_get_string(the_repository,
> +                        "uploadpack.commitpackfileuri",
> +                        &str)) &&
>  		    str) {
>  			strbuf_addstr(value, " packfile-uris");
>  			free(str);

Not a new issue, but I wonder if we shouldn't just export configset_find_element(). This is at least 2 stackframes down the chain of the "does this key exist?" we actually care about here.

Previous: Teng LongNext: Teng Long
Message 2 of 72 in “Packfile-uris support excluding commit objects”
  1. Packfile-uris support excluding commit objectsTeng Long, May 7, 2021
  2. Ævar Arnfjörð BjarmasonMay 10, 2021
  3. 0/3 packfile-uris: commit objects exclusionTeng Long, May 18, 2021
  4. 1/3 packfile-uris: support for excluding commit objectTeng Long, May 18, 2021
  5. Junio C HamanoMay 19, 2021
  6. Junio C HamanoMay 20, 2021
  7. 2/3 packfile-uris.txt: excluding commit objectTeng Long, May 18, 2021
  8. 3/3 t5702: excluding commits with packfile-urisTeng Long, May 18, 2021
  9. 0/3 packfile-uris: commit objects exclusioTeng Long, Jul 26, 2021
  10. 1/3 packfile-uris: support for excluding commit objectsTeng Long, Jul 26, 2021
  11. Junio C HamanoJul 26, 2021
  12. Felipe ContrerasJul 26, 2021
  13. Teng LongAug 11, 2021
  14. 2/3 t5702: support for excluding commit objectsTeng Long, Jul 26, 2021
  15. Ævar Arnfjörð BjarmasonJul 26, 2021
  16. Teng LongAug 11, 2021
  17. 3/3 packfile-uri.txt: support for excluding commit objectsTeng Long, Jul 26, 2021
  18. Junio C HamanoJul 26, 2021
  19. Teng LongAug 11, 2021
  20. Ævar Arnfjörð BjarmasonJul 26, 2021
  21. Teng LongAug 11, 2021
  22. 0/7 packfile-uris: commits and trees exclusionTeng Long, Aug 11, 2021
  23. 1/7 pack-objects.c: introduce new method `match_packfile_uri_exclusions`Teng Long, Aug 11, 2021
  24. 2/7 Add new parameter "carry_data" for "show_object" functionTeng Long, Aug 11, 2021
  25. 3/7 packfile-uri: support for excluding commit objectsTeng Long, Aug 11, 2021
  26. 4/7 packfile-uri: support for excluding tree objectsTeng Long, Aug 11, 2021
  27. 5/7 packfile-uri.txt: support for excluding commits and treesTeng Long, Aug 11, 2021
  28. Bagas SanjayaAug 11, 2021
  29. 6/7 t5702: replace with "test_when_finished" for cleanupTeng Long, Aug 11, 2021
  30. 7/7 t5702: support for excluding commit objectsTeng Long, Aug 11, 2021
  31. 00/14 packfile-uris: commits, trees and tags exclusionTeng Long, Aug 25, 2021
  32. 01/14 pack-objects.c: introduce new method `match_packfile_uri_exclusions`Teng Long, Aug 25, 2021
  33. 02/14 Add new parameter "carry_data" for "show_object" functionTeng Long, Aug 25, 2021
  34. Junio C HamanoAug 26, 2021
  35. Teng LongSep 2, 2021
  36. 03/14 packfile-uri: support for excluding commit objectsTeng Long, Aug 25, 2021
  37. Ævar Arnfjörð BjarmasonAug 25, 2021
  38. Teng LongSep 2, 2021
  39. Junio C HamanoAug 26, 2021
  40. Teng LongSep 2, 2021
  41. 04/14 packfile-uri: support for excluding tree objectsTeng Long, Aug 25, 2021
  42. 05/14 packfile-uri.txt: support for excluding commits and treesTeng Long, Aug 25, 2021
  43. Ævar Arnfjörð BjarmasonAug 25, 2021
  44. Teng LongSep 2, 2021
  45. 06/14 t5702: replace with "test_when_finished" for cleanupTeng Long, Aug 25, 2021
  46. Ævar Arnfjörð BjarmasonAug 25, 2021
  47. Teng LongSep 2, 2021
  48. 07/14 t5702: support for excluding commit objectsTeng Long, Aug 25, 2021
  49. 08/14 Add new parameter "carry_data" for "show_commit functionTeng Long, Aug 25, 2021
  50. 09/14 commit.h: add wrapped tags in commit structTeng Long, Aug 25, 2021
  51. Ævar Arnfjörð BjarmasonAug 25, 2021
  52. Teng LongSep 2, 2021
  53. ZheNing HuSep 2, 2021
  54. Teng LongSep 2, 2021
  55. 10/14 object.h: add referred tags in `referred_objects` structTeng Long, Aug 25, 2021
  56. 11/14 packfile-uri: support for excluding tag objectsTeng Long, Aug 25, 2021
  57. 12/14 packfile-uri.txt: support for excluding tag objectsTeng Long, Aug 25, 2021
  58. 13/14 t5702: add tag exclusion test caseTeng Long, Aug 25, 2021
  59. 14/14 pack-objects.c: introduce `want_exclude_object` functionTeng Long, Aug 25, 2021
  60. 00/12 packfile-uri: support excluding multiple object typesTeng Long, Oct 19, 2021
  61. 01/12 objects.c: introduce `exclude_level` enumTeng Long, Oct 19, 2021
  62. 02/12 Introduce function `match_packfile_uri_exclusions`Teng Long, Oct 19, 2021
  63. 03/12 Replace `show_data` with structure `show_info`Teng Long, Oct 19, 2021
  64. 04/12 Introduce `uploadpack.excludeobject` configurationTeng Long, Oct 19, 2021
  65. 05/12 t5702: test cases for `uploadpack.excludeobject`Teng Long, Oct 19, 2021
  66. 06/12 packfile-uri: support for excluding commitsTeng Long, Oct 19, 2021
  67. 07/12 t5702: test cases for excluding commitsTeng Long, Oct 19, 2021
  68. 08/12 packfile-uri: support for excluding treesTeng Long, Oct 19, 2021
  69. 09/12 t5702: test cases for excluding treesTeng Long, Oct 19, 2021
  70. 10/12 packfile-uri: support for excluding tagsTeng Long, Oct 19, 2021
  71. 11/12 t5702: test cases for excluding tagsTeng Long, Oct 19, 2021
  72. 12/12 packfile-uri.txt: support multiple object typesTeng Long, Oct 19, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.