git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] Packfile-uris support excluding commit objects
@ 2021-05-07  2:11 Teng Long
  2021-05-10 11:14 ` Ævar Arnfjörð Bjarmason
  2021-05-18  8:49 ` [PATCH v2 0/3] packfile-uris: commit objects exclusion Teng Long
  0 siblings, 2 replies; 18+ messages in thread
From: Teng Long @ 2021-05-07  2:11 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, Teng Long

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.

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.

Similarly, there are related documents that will be included in
subsequent patches.

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")) {
 		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);
 		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);
 		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;
+        }
+
+        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
+            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 &&
+            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
+    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 &&
+
+	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 &&
+
+
+	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 &&
+	othercommit=$(git -C "$P" rev-parse HEAD) &&
+	configure_exclusion commit "$P" "$mycommit" >h &&
+	configure_exclusion commit "$P" "$othercommit" >h2 &&
+
+	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);
-- 
2.31.1.442.g7e39198978.dirty


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] Packfile-uris support excluding commit objects
  2021-05-07  2:11 [PATCH] Packfile-uris support excluding commit objects Teng Long
@ 2021-05-10 11:14 ` Ævar Arnfjörð Bjarmason
  2021-05-18  8:49 ` [PATCH v2 0/3] packfile-uris: commit objects exclusion Teng Long
  1 sibling, 0 replies; 18+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-10 11:14 UTC (permalink / raw)
  To: Teng Long; +Cc: git, jonathantanmy


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:

> 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.

>  		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.

>  		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.

> +
> +        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...

> +            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".

> +            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...

> +    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"

> +
> +	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?

> +
> +
> +	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...

> +
> +	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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 0/3] packfile-uris: commit objects exclusion
  2021-05-07  2:11 [PATCH] Packfile-uris support excluding commit objects Teng Long
  2021-05-10 11:14 ` Ævar Arnfjörð Bjarmason
@ 2021-05-18  8:49 ` Teng Long
  2021-05-18  8:49   ` [PATCH v2 1/3] packfile-uris: support for excluding commit object Teng Long
                     ` (3 more replies)
  1 sibling, 4 replies; 18+ messages in thread
From: Teng Long @ 2021-05-18  8:49 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, avarab, Teng Long

Changes since v2:

* Rename the "uploadpack.blobpackfileuri" configuration to
  "uploadpack.excludeobject".
* In addition to blobs, packfile-uris now supports the exclusion of
  commit objects (recursive and non-recursive).
* Added a patch to modify the packfile-uris.txt file.
* Added a patch for related tests in t5702.

About renaming, I do not know whether it will bring some compatibility
impact, packfile-uris now is an experimental feature, how to deal
with this situation, hoping to get some advice.

Also, I did not consider implementing packfile-uri support for tree
objects, because in the design scenario of packfile-uris, it seems to
be of little use.

Teng Long (3):
  packfile-uris: support for excluding commit object
  packfile-uris.txt: excluding commit object
  t5702: excluding commits with packfile-uris

 Documentation/technical/packfile-uri.txt |  20 ++--
 builtin/pack-objects.c                   |  53 ++++++---
 fetch-pack.c                             |   5 +
 t/t5702-protocol-v2.sh                   | 145 +++++++++++++++++------
 upload-pack.c                            |   5 +-
 5 files changed, 166 insertions(+), 62 deletions(-)

Range-diff against v1:
1:  1f2fb5c85f < -:  ---------- Packfile-uris support excluding commit objects
-:  ---------- > 1:  73e64147b1 packfile-uris: support for excluding commit object
-:  ---------- > 2:  4abab98a76 packfile-uris.txt: excluding commit object
-:  ---------- > 3:  e824cc26a7 t5702: excluding commits with packfile-uris
-- 
2.31.1.442.g7e39198978.dirty


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 1/3] packfile-uris: support for excluding commit object
  2021-05-18  8:49 ` [PATCH v2 0/3] packfile-uris: commit objects exclusion Teng Long
@ 2021-05-18  8:49   ` Teng Long
  2021-05-19  4:28     ` Junio C Hamano
  2021-05-20  4:46     ` Junio C Hamano
  2021-05-18  8:49   ` [PATCH v2 2/3] packfile-uris.txt: " Teng Long
                     ` (2 subsequent siblings)
  3 siblings, 2 replies; 18+ messages in thread
From: Teng Long @ 2021-05-18  8:49 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, avarab, Teng Long

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

The old configuration `uploadpack.blobpackfileuri` is only support to
exclude blobs and the name has no abstract meaning, so the configruation
name changes, to support more object types. Compatibility issues will
not be considered because packfile-uris now is an experimental feature.

In addition to the configuration name, the format of the configuration
value has also been expanded. When excluding the commits (or trees in
the future) objects, the old format `<object-hash> <pack-hash> <uri>`
can not express the meaning of recursion. So, the format is expanded,
the new format `<object-hash> <recursively> <pack-hash> <uri>` should
deal with this scenario (When processing commit objects, whether they
are absolutely recursively excluded).

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/pack-objects.c | 53 ++++++++++++++++++++++++++++++------------
 fetch-pack.c           |  5 ++++
 upload-pack.c          |  5 ++--
 3 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6d13cd3e1a..e687061420 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -99,6 +99,8 @@ static struct bitmap *reuse_packfile_bitmap;
 static int use_bitmap_index_default = 1;
 static int use_bitmap_index = -1;
 static int allow_pack_reuse = 1;
+static int in_commit_order;
+static int exclude_until_next_commit;
 static enum {
 	WRITE_BITMAP_FALSE = 0,
 	WRITE_BITMAP_QUIET,
@@ -132,6 +134,7 @@ struct configured_exclusion {
 	struct oidmap_entry e;
 	char *pack_hash_hex;
 	char *uri;
+	int recursively:1;
 };
 static struct oidmap configured_exclusions;
 
@@ -1291,10 +1294,16 @@ static int want_object_in_pack_one(struct packed_git *p,
  * and its offset in these variables.
  */
 static int want_object_in_pack(const struct object_id *oid,
+			       enum object_type type,
 			       int exclude,
 			       struct packed_git **found_pack,
 			       off_t *found_offset)
 {
+	if (exclude_until_next_commit && type != OBJ_COMMIT)
+		return 0;
+	if (type == OBJ_COMMIT)
+		exclude_until_next_commit = 0 ;
+
 	int want;
 	struct list_head *pos;
 	struct multi_pack_index *m;
@@ -1345,6 +1354,8 @@ static int want_object_in_pack(const struct object_id *oid,
 						&p) &&
 				    *p == ':') {
 					oidset_insert(&excluded_by_config, oid);
+					if(ex->recursively && type == OBJ_COMMIT)
+						exclude_until_next_commit = 1;
 					return 0;
 				}
 			}
@@ -1394,7 +1405,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
 	if (have_duplicate_entry(oid, exclude))
 		return 0;
 
-	if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {
+	if (!want_object_in_pack(oid, type, exclude, &found_pack, &found_offset)) {
 		/* The pack is missing an object, so it will not have closure */
 		if (write_bitmap_index) {
 			if (write_bitmap_index != WRITE_BITMAP_QUIET)
@@ -1420,7 +1431,7 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,
 	if (have_duplicate_entry(oid, 0))
 		return 0;
 
-	if (!want_object_in_pack(oid, 0, &pack, &offset))
+	if (!want_object_in_pack(oid, type, 0, &pack, &offset))
 		return 0;
 
 	create_object_entry(oid, type, name_hash, 0, 0, pack, offset);
@@ -2985,27 +2996,33 @@ 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.excludeobject")) {
 		struct configured_exclusion *ex = xmalloc(sizeof(*ex));
-		const char *oid_end, *pack_end;
+		const char *oid_end, *pack_end, *recursively_end;
 		/*
 		 * Stores the pack hash. This is not a true object ID, but is
 		 * of the same form.
 		 */
 		struct object_id pack_hash;
-
+		char recursively[2];
 		if (parse_oid_hex(v, &ex->e.oid, &oid_end) ||
 		    *oid_end != ' ' ||
-		    parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||
+		    !strlcpy(recursively, oid_end + 1, sizeof(recursively)) ||
+		    parse_oid_hex(oid_end + 3, &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.excludeobject must be "
+                              "of the form '<object-hash> <recursively> <pack-hash> <uri>' (got '%s')"), v);
 		if (oidmap_get(&configured_exclusions, &ex->e.oid))
-			die(_("object already configured in another "
-			      "uploadpack.blobpackfileuri (got '%s')"), v);
-		ex->pack_hash_hex = xcalloc(1, pack_end - oid_end);
-		memcpy(ex->pack_hash_hex, oid_end + 1, pack_end - oid_end - 1);
+                        die(_("object already configured by an earlier "
+                              "uploadpack.excludeobject (got '%s')"), v);
+		recursively_end = oid_end + 2;
+		ex->pack_hash_hex = xcalloc(1, pack_end - recursively_end);
+		memcpy(ex->pack_hash_hex, recursively_end + 1, pack_end - recursively_end - 1);
 		ex->uri = xstrdup(pack_end + 1);
+		if (atoi(recursively)) {
+			ex->recursively = 1;
+			in_commit_order = 1;
+                }
 		oidmap_put(&configured_exclusions, ex);
 	}
 	return git_default_config(k, v, cb);
@@ -3023,7 +3040,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 	struct rev_info *revs = _data;
 	struct object_info oi = OBJECT_INFO_INIT;
 	off_t ofs;
-	enum object_type type;
+	static enum object_type type;
 
 	display_progress(progress_state, ++nr_seen);
 
@@ -3031,7 +3048,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		return 0;
 
 	ofs = nth_packed_object_offset(p, pos);
-	if (!want_object_in_pack(oid, 0, &p, &ofs))
+	if (!want_object_in_pack(oid, type, 0, &p, &ofs))
 		return 0;
 
 	oi.typep = &type;
@@ -3831,7 +3848,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			 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")),
+				N_("exclude any configured uploadpack.excludeobject with this protocol")),
 		OPT_END(),
 	};
 
@@ -3903,6 +3920,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		fetch_if_missing = 0;
 		strvec_push(&rp, "--exclude-promisor-objects");
 	}
+
+	if (in_commit_order){
+		use_internal_rev_list = 1;
+		strvec_push(&rp, "--in-commit-order");
+	}
+
 	if (unpack_unreachable || keep_unreachable || pack_loose_unreachable)
 		use_internal_rev_list = 1;
 
diff --git a/fetch-pack.c b/fetch-pack.c
index 2318ebe680..cdf8777907 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,8 @@ 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_add(&uris, uri))
+			continue;
 		strvec_push(&cmd.args, "http-fetch");
 		strvec_pushf(&cmd.args, "--packfile=%.*s",
 			     (int) the_hash_algo->hexsz,
diff --git a/upload-pack.c b/upload-pack.c
index 5c1cd19612..4d994658d2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1745,9 +1745,8 @@ int upload_pack_advertise(struct repository *r,
 			strbuf_addstr(value, " sideband-all");
 
 		if (!repo_config_get_string(the_repository,
-					    "uploadpack.blobpackfileuri",
-					    &str) &&
-		    str) {
+					 "uploadpack.excludeobject",
+					 &str) && str) {
 			strbuf_addstr(value, " packfile-uris");
 			free(str);
 		}
-- 
2.31.1.442.g7e39198978.dirty


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 2/3] packfile-uris.txt: excluding commit object
  2021-05-18  8:49 ` [PATCH v2 0/3] packfile-uris: commit objects exclusion Teng Long
  2021-05-18  8:49   ` [PATCH v2 1/3] packfile-uris: support for excluding commit object Teng Long
@ 2021-05-18  8:49   ` Teng Long
  2021-05-18  8:49   ` [PATCH v2 3/3] t5702: excluding commits with packfile-uris Teng Long
  2021-07-26  9:46   ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Teng Long
  3 siblings, 0 replies; 18+ messages in thread
From: Teng Long @ 2021-05-18  8:49 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, avarab, Teng Long

Modified the content related to the configuration of packfile-uris, that
is, the modification of the configuration format and the support for
excluding commit objects, and cut some descriptions about future work.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/technical/packfile-uri.txt | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
index f7eabc6c76..6ed850930f 100644
--- a/Documentation/technical/packfile-uri.txt
+++ b/Documentation/technical/packfile-uri.txt
@@ -35,13 +35,16 @@ include some sort of non-trivial implementation in the Minimum Viable Product,
 at least so that we can test the client.
 
 This is the implementation: a feature, marked experimental, that allows the
-server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
-<uri>` entries. Whenever the list of objects to be sent is assembled, all such
-blobs are excluded, replaced with URIs. As noted in "Future work" below, the
-server can evolve in the future to support excluding other objects (or other
-implementations of servers could be made that support excluding other objects)
-without needing a protocol change, so clients should not expect that packfiles
-downloaded in this way only contain single blobs.
+server to be configured by one or more entries with the format:
+
+    uploadpack.excludeobject=<object-hash> <recursively> <pack-hash> <uri>
+
+Value <object-hash> is the key of entry, and the object type can be a blob
+or commit. Value <recursively> works for commit object, if <recursively>
+is configured as '1', then the commit and all the referenced objects by
+the commit will be recursively excluded. Otherwise, only the commit itself
+will be excluded. Whenever the list of objects to be sent is assembled, all
+such objects are excluded, replaced with URIs.
 
 Client design
 -------------
@@ -65,9 +68,6 @@ The protocol design allows some evolution of the server and client without any
 need for protocol changes, so only a small-scoped design is included here to
 form the MVP. For example, the following can be done:
 
- * On the server, more sophisticated means of excluding objects (e.g. by
-   specifying a commit to represent that commit and all objects that it
-   references).
  * On the client, resumption of clone. If a clone is interrupted, information
    could be recorded in the repository's config and a "clone-resume" command
    can resume the clone in progress. (Resumption of subsequent fetches is more
-- 
2.31.1.442.g7e39198978.dirty


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 3/3] t5702: excluding commits with packfile-uris
  2021-05-18  8:49 ` [PATCH v2 0/3] packfile-uris: commit objects exclusion Teng Long
  2021-05-18  8:49   ` [PATCH v2 1/3] packfile-uris: support for excluding commit object Teng Long
  2021-05-18  8:49   ` [PATCH v2 2/3] packfile-uris.txt: " Teng Long
@ 2021-05-18  8:49   ` Teng Long
  2021-07-26  9:46   ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Teng Long
  3 siblings, 0 replies; 18+ messages in thread
From: Teng Long @ 2021-05-18  8:49 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, avarab, Teng Long

Modify the logic of configure_exclusion to support the processing of
commits objects and add test cases for excluding commit objects
(recursive and non-recursive).

Replace "rm..." in the original test with "test_when_finished...".
Replace "git commit..." in the original test with "test_commit...".

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 t/t5702-protocol-v2.sh | 145 +++++++++++++++++++++++++++++++----------
 1 file changed, 111 insertions(+), 34 deletions(-)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 2e1243ca40..2148bfcda1 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -753,7 +753,7 @@ test_expect_success 'ls-remote with v2 http sends only one POST' '
 '
 
 test_expect_success 'push with http:// and a config of v2 does not request v2' '
-	test_when_finished "rm -f log" &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child log" &&
 	# Till v2 for push is designed, make sure that if a client has
 	# protocol.version configured to use v2, that the client instead falls
 	# back and uses v0.
@@ -776,7 +776,7 @@ test_expect_success 'push with http:// and a config of v2 does not request v2' '
 '
 
 test_expect_success 'when server sends "ready", expect DELIM' '
-	rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child" &&
 
 	git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
 	test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one &&
@@ -796,7 +796,7 @@ test_expect_success 'when server sends "ready", expect DELIM' '
 '
 
 test_expect_success 'when server does not send "ready", expect FLUSH' '
-	rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child log &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child log" &&
 
 	git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
 	test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one &&
@@ -824,17 +824,39 @@ 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
+    objt="$1"
+    P="$2"
+    recursive="$4"
+
+    if test "$objt" = "blob"
+    then
+        git -C "$P" hash-object "$3" >objh &&
+        git -C "$P" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
+        git -C "$P" config --add \
+                "uploadpack.excludeobject" \
+                "$(cat objh) $recursive $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
+        cat objh
+    elif test "$objt" = "commit"
+    then
+        echo "$3" >objh
+        if test "$recursive" = 0
+            then
+                git -C "$P" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh
+        else
+            git -C "$2" pack-objects --revs "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh
+        fi
+        git -C "$P" config --add \
+                "uploadpack.excludeobject" \
+                "$(cat objh) $recursive $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
+        cat objh
+    else
+        echo "unsupported object type in configure_exclusion (got $objt)"
+    fi
 }
 
 test_expect_success 'part of packfile response provided as URI' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child log &&
+	test_when_finished "rm -rf \"$P\" http_child log" &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -843,10 +865,10 @@ test_expect_success 'part of packfile response provided as URI' '
 	git -C "$P" add my-blob &&
 	echo other-blob >"$P/other-blob" &&
 	git -C "$P" add other-blob &&
-	git -C "$P" commit -m x &&
+	test_commit -C "$P" A &&
 
-	configure_exclusion "$P" my-blob >h &&
-	configure_exclusion "$P" other-blob >h2 &&
+	configure_exclusion blob "$P" my-blob 0 >h &&
+	configure_exclusion blob "$P" other-blob 0 >h2 &&
 
 	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
 	git -c protocol.version=2 \
@@ -881,18 +903,18 @@ 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 &&
+	test_when_finished "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 &&
+	test_commit -C "$P" A &&
 
-	configure_exclusion "$P" my-blob >h &&
+	configure_exclusion blob "$P" my-blob 0 >h &&
 
 	git init http_child &&
 
@@ -902,9 +924,65 @@ test_expect_success 'packfile URIs with fetch instead of clone' '
 		fetch "$HTTPD_URL/smart/http_parent"
 '
 
+test_expect_success 'commits(not recursively) packfile URIs with fetch instead of clone' '
+	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+	test_when_finished "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 &&
+	test_commit -C "$P" A &&
+
+	mycommit=$(git -C "$P" rev-parse A) &&
+	echo other-blob >"$P/other-blob" &&
+	git -C "$P" add other-blob &&
+	test_commit -C "$P" B &&
+	othercommit=$(git -C "$P" rev-parse B) &&
+
+	configure_exclusion commit "$P" "$mycommit" 0 >h &&
+	configure_exclusion commit "$P" "$othercommit" 0 >h2 &&
+
+	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"
+'
+
+test_expect_success 'commits(recursively) packfile URIs with fetch instead of clone' '
+	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+	test_when_finished "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 &&
+	test_commit -C "$P" A &&
+
+	mycommit=$(git -C "$P" rev-parse A) &&
+	echo other-blob >"$P/other-blob" &&
+    git -C "$P" add other-blob &&
+	test_commit -C "$P" B &&
+	othercommit=$(git -C "$P" rev-parse B) &&
+
+	configure_exclusion commit "$P" "$mycommit" 1 >h2 &&
+	configure_exclusion commit "$P" "$othercommit" 1 >h2 &&
+
+	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"
+'
+
 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 &&
+	test_when_finished "rm -rf \"$P\" http_child log" &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -913,9 +991,9 @@ test_expect_success 'fetching with valid packfile URI but invalid hash fails' '
 	git -C "$P" add my-blob &&
 	echo other-blob >"$P/other-blob" &&
 	git -C "$P" add other-blob &&
-	git -C "$P" commit -m x &&
+	test_commit -C "$P" A &&
 
-	configure_exclusion "$P" my-blob >h &&
+	configure_exclusion blob "$P" my-blob 0 >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
@@ -923,8 +1001,8 @@ test_expect_success 'fetching with valid packfile URI but invalid hash fails' '
 	git -C "$P" hash-object other-blob >objh &&
 	git -C "$P" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
 	git -C "$P" config --add \
-		"uploadpack.blobpackfileuri" \
-		"$(cat objh) $(cat objh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
+		"uploadpack.excludeobject" \
+		"$(cat objh) 0 $(cat objh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
 
 	test_must_fail env GIT_TEST_SIDEBAND_ALL=1 \
 		git -c protocol.version=2 \
@@ -935,16 +1013,15 @@ test_expect_success 'fetching with valid packfile URI but invalid hash fails' '
 
 test_expect_success 'packfile-uri with transfer.fsckobjects' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child log &&
+	test_when_finished "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 &&
+	test_commit -C "$P" A &&
+	configure_exclusion blob "$P" my-blob 0 >h &&
 
 	sane_unset GIT_TEST_SIDEBAND_ALL &&
 	git -c protocol.version=2 -c transfer.fsckobjects=1 \
@@ -959,7 +1036,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects' '
 
 test_expect_success 'packfile-uri with transfer.fsckobjects fails on bad object' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child log &&
+	test_when_finished "rm -rf \"$P\" http_child log" &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -976,9 +1053,9 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails on bad object'
 
 	echo my-blob >"$P/my-blob" &&
 	git -C "$P" add my-blob &&
-	git -C "$P" commit -m x &&
+	test_commit -C "$P" A &&
 
-	configure_exclusion "$P" my-blob >h &&
+	configure_exclusion blob "$P" my-blob 0 >h &&
 
 	sane_unset GIT_TEST_SIDEBAND_ALL &&
 	test_must_fail git -c protocol.version=2 -c transfer.fsckobjects=1 \
@@ -989,7 +1066,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails on bad object'
 
 test_expect_success 'packfile-uri with transfer.fsckobjects succeeds when .gitmodules is separate from tree' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child &&
+	test_when_finished "rm -rf \"$P\" http_child" &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -1000,7 +1077,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 0 >h &&
 
 	sane_unset GIT_TEST_SIDEBAND_ALL &&
 	git -c protocol.version=2 -c transfer.fsckobjects=1 \
@@ -1015,7 +1092,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects succeeds when .gitmo
 
 test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodules separate from tree is invalid' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child err &&
+	test_when_finished "rm -rf \"$P\" http_child err" &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -1024,9 +1101,9 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul
 	echo "path = include/foo" >>"$P/.gitmodules" &&
 	echo "url = git://example.com/git/lib.git" >>"$P/.gitmodules" &&
 	git -C "$P" add .gitmodules &&
-	git -C "$P" commit -m x &&
+	test_commit -C "$P" A &&
 
-	configure_exclusion "$P" .gitmodules >h &&
+	configure_exclusion blob "$P" .gitmodules 0 >h &&
 
 	sane_unset GIT_TEST_SIDEBAND_ALL &&
 	test_must_fail git -c protocol.version=2 -c transfer.fsckobjects=1 \
-- 
2.31.1.442.g7e39198978.dirty


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/3] packfile-uris: support for excluding commit object
  2021-05-18  8:49   ` [PATCH v2 1/3] packfile-uris: support for excluding commit object Teng Long
@ 2021-05-19  4:28     ` Junio C Hamano
  2021-05-20  4:46     ` Junio C Hamano
  1 sibling, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2021-05-19  4:28 UTC (permalink / raw)
  To: Teng Long; +Cc: git, jonathantanmy, avarab

Teng Long <dyroneteng@gmail.com> writes:

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

This "should" is not justfied at all, it seems?  What is lacking in
what we already have?  What new things does it all us to do by
adding a new configuration variable?

> The old configuration `uploadpack.blobpackfileuri` is only support to
> exclude blobs and the name has no abstract meaning, so the configruation
> name changes, to support more object types. Compatibility issues will
> not be considered because packfile-uris now is an experimental feature.

I'll let Jonathan speak up, but even for an experimental feature,
whatever new and incompatible way to do things should have a clear
advantage compared to the old way.  Sell the backward incomptibility
along that line---"it is an experimental so I'll trash it" is not,
but "by doing this it gets this much better, and migrating existing
users won't be too taxing (it is just this simple thing)" is an
acceptable way to justify such a change.

Note that I am not opposed to the proposed change (and I am not
supporting it, either).  I do have a problem with the way the change
is sold, though.

>  builtin/pack-objects.c | 53 ++++++++++++++++++++++++++++++------------
>  fetch-pack.c           |  5 ++++
>  upload-pack.c          |  5 ++--
>  3 files changed, 45 insertions(+), 18 deletions(-)

Even though the name of the configuration variable changed, and the
semantics of the value of it changed, there is no documentation
change, because...?

Because the original didn't even document the variable properly?  It
may be another reason why changing it may not impact the existing
users too much.

> @@ -132,6 +134,7 @@ struct configured_exclusion {
>  	struct oidmap_entry e;
>  	char *pack_hash_hex;
>  	char *uri;
> +	int recursively:1;
>  };
>  static struct oidmap configured_exclusions;
>  
> @@ -1291,10 +1294,16 @@ static int want_object_in_pack_one(struct packed_git *p,
>   * and its offset in these variables.
>   */
>  static int want_object_in_pack(const struct object_id *oid,
> +			       enum object_type type,
>  			       int exclude,
>  			       struct packed_git **found_pack,
>  			       off_t *found_offset)
>  {
> +	if (exclude_until_next_commit && type != OBJ_COMMIT)
> +		return 0;
> +	if (type == OBJ_COMMIT)
> +		exclude_until_next_commit = 0 ;

Lose SP before the semicolon.

Our codebase does not allow statements before declarations.  Move
all of the above down to be below the block of decls at the
beginning of the function.

>  	int want;
>  	struct list_head *pos;
>  	struct multi_pack_index *m;

> @@ -1345,6 +1354,8 @@ static int want_object_in_pack(const struct object_id *oid,
>  						&p) &&
>  				    *p == ':') {
>  					oidset_insert(&excluded_by_config, oid);
> +					if(ex->recursively && type == OBJ_COMMIT)
> +						exclude_until_next_commit = 1;

This depends on a new file-scope global variable, which means two
things.

 * if two or more threads are deciding which object to pack and not
   to pack, this code will horribly break, as they are traversing
   totally different parts of the object DAG to find out which
   objects to pack, but one thread hitting a commit to be excluded
   and setting this flag will cause other thread skip unrelated
   blobs and trees that it discovers, doesn't it?

 * even if we assume there is no concurrency and reentrancy issues
   (e.g. by forcing single-threaded operation when this feature is
   in use), the code _assumes_ a concrete order in which this helper
   function gets called, namely, non-commit objects fed to this
   helper after the helper gets a single commit object *all* belong
   to that commit.  With the current code that feeds objects as they
   are discovered during depth first traversal of the top-level tree
   starting at each commit, that assumption might hold, but it feels
   that the assumption is too much to be healty.  For example, would
   it be possible for the bitmap code to cause this helper to be
   called in different order (i.e. it might find it more convenent
   to feed a tree, a blob or a tag that is unrelated to the commit
   that was last fed to the helper)?  If so, the logic in this code
   will constrain the caller too much.

I'll stop reading for now at this place; review of the remainder may
come at a later time, but not now.

Thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/3] packfile-uris: support for excluding commit object
  2021-05-18  8:49   ` [PATCH v2 1/3] packfile-uris: support for excluding commit object Teng Long
  2021-05-19  4:28     ` Junio C Hamano
@ 2021-05-20  4:46     ` Junio C Hamano
  1 sibling, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2021-05-20  4:46 UTC (permalink / raw)
  To: Teng Long; +Cc: git, jonathantanmy, avarab

(continuing from yesterday's review)

> @@ -3023,7 +3040,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,
>  	struct rev_info *revs = _data;
>  	struct object_info oi = OBJECT_INFO_INIT;
>  	off_t ofs;
> -	enum object_type type;
> +	static enum object_type type;
>  
>  	display_progress(progress_state, ++nr_seen);
>  
> @@ -3031,7 +3048,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,
>  		return 0;
>  
>  	ofs = nth_packed_object_offset(p, pos);
> -	if (!want_object_in_pack(oid, 0, &p, &ofs))
> +	if (!want_object_in_pack(oid, type, 0, &p, &ofs))
>  		return 0;
>  
>  	oi.typep = &type;

This change is puzzling.

The first call to this helper will use BSS initialized type to call
want_object_in_pack(), and then after that call says "yes, we want
that object", packed_object_info() gets called to learn what type it
is.  And the second call would use the type of the object we
previously handled, because type is a function scope static.  Or if
we are not that lucky and multiple threads are involved, the type we
pass is from a random and unrelated object some other thread has
called this helper with.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 0/3] packfile-uris: commit objects exclusio
  2021-05-18  8:49 ` [PATCH v2 0/3] packfile-uris: commit objects exclusion Teng Long
                     ` (2 preceding siblings ...)
  2021-05-18  8:49   ` [PATCH v2 3/3] t5702: excluding commits with packfile-uris Teng Long
@ 2021-07-26  9:46   ` Teng Long
  2021-07-26  9:46     ` [PATCH v3 1/3] packfile-uris: support for excluding commit objects Teng Long
                       ` (3 more replies)
  3 siblings, 4 replies; 18+ messages in thread
From: Teng Long @ 2021-07-26  9:46 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, avarab, Teng Long

Changes since v3:

* Ensure the forward compatibility of the old
  configuration (uploadpack.blobpackfileuri).
* Reimplementation of the commit object exclusion method (without rely
  on the "--in-commit-order" arg).
* Extend `show_object` function.
* Remove `exclude_until_next_commit` var in pack-objects.c (Concurrency
  issues).
* Restore the definition of want_object_in_pack method (problems caused
  by the new "type" parameter)

Teng Long (3):
  packfile-uris: support for excluding commit objects
  t5702: support for excluding commit objects
  packfile-uri.txt: support for excluding commit objects

 Documentation/technical/packfile-uri.txt |  20 +--
 builtin/describe.c                       |   4 +-
 builtin/pack-objects.c                   |  97 +++++++------
 builtin/rev-list.c                       |   2 +-
 fetch-pack.c                             |   6 +
 list-objects.c                           |  37 ++---
 list-objects.h                           |   2 +-
 object.c                                 |  15 +-
 object.h                                 |   4 +
 pack-bitmap.c                            |   8 +-
 reachable.c                              |   8 +-
 revision.c                               |  36 +++--
 revision.h                               |   4 +
 t/t5702-protocol-v2.sh                   | 166 ++++++++++++++++++-----
 upload-pack.c                            |   7 +
 15 files changed, 291 insertions(+), 125 deletions(-)

Range-diff against v2:
-:  ---------- > 1:  91dce385f6 packfile-uris: support for excluding commit objects
-:  ---------- > 2:  92def8c72b t5702: support for excluding commit objects
-:  ---------- > 3:  01ab2cbb34 packfile-uri.txt: support for excluding commit objects
-- 
2.31.1.443.g55c63af4c9.dirty


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/3] packfile-uris: support for excluding commit objects
  2021-07-26  9:46   ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Teng Long
@ 2021-07-26  9:46     ` Teng Long
  2021-07-26 18:15       ` Junio C Hamano
  2021-07-26  9:46     ` [PATCH v3 2/3] t5702: " Teng Long
                       ` (2 subsequent siblings)
  3 siblings, 1 reply; 18+ messages in thread
From: Teng Long @ 2021-07-26  9:46 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, avarab, Teng Long

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

The reason for bringing a new configuration is for two considerations.
First, the old configuration supports a single object type (blob), which
limits the use of this feature. Secondly, the name of the old
configuration is not abstract enough, this make extension difficult. If
different object types use different configuration names, the
configuration items will be bloated and difficult to maintain, so the
new configuration is more abstract in name and easy to extend.

Although a new configuration has been introduced, the old one is
still available and compatible with the new configuration. The old
configuration `uploadpack.blobpackfileuri` only supports excluding
blobs. The new configuration `uploadpack.excludeobject` not only
supports excluding blob objects, but also supports excluding commit
objects, as well as recursively excluding tree objects and blob objects
they contain.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/describe.c     |  4 +-
 builtin/pack-objects.c | 97 ++++++++++++++++++++++++------------------
 builtin/rev-list.c     |  2 +-
 fetch-pack.c           |  6 +++
 list-objects.c         | 37 +++++++++-------
 list-objects.h         |  2 +-
 object.c               | 15 +++++--
 object.h               |  4 ++
 pack-bitmap.c          |  8 ++--
 reachable.c            |  8 ++--
 revision.c             | 36 +++++++++++-----
 revision.h             |  4 ++
 upload-pack.c          |  7 +++
 13 files changed, 148 insertions(+), 82 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 40482d8e9f..045da79b5c 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -485,9 +485,9 @@ static void process_commit(struct commit *commit, void *data)
 	pcd->current_commit = commit->object.oid;
 }
 
-static void process_object(struct object *obj, const char *path, void *data)
+static void process_object(struct object *obj, const char *path, void *show_data, void *carry_data)
 {
-	struct process_commit_data *pcd = data;
+	struct process_commit_data *pcd = show_data;
 
 	if (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {
 		reset_revision_walk();
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6d13cd3e1a..154c98bcb6 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1188,6 +1188,24 @@ static int have_duplicate_entry(const struct object_id *oid,
 	return 1;
 }
 
+static int match_packfile_uri_exclusions(struct configured_exclusion *ex)
+{
+	int i;
+	const char *p;
+
+	if (ex) {
+		for (i = 0; i < uri_protocols.nr; i++) {
+			if (skip_prefix(ex->uri,
+					uri_protocols.items[i].string,
+					&p) &&
+			    *p == ':')
+				return 1;
+
+		}
+	}
+	return 0;
+}
+
 static int want_found_object(const struct object_id *oid, int exclude,
 			     struct packed_git *p)
 {
@@ -1293,7 +1311,8 @@ static int want_object_in_pack_one(struct packed_git *p,
 static int want_object_in_pack(const struct object_id *oid,
 			       int exclude,
 			       struct packed_git **found_pack,
-			       off_t *found_offset)
+			       off_t *found_offset,
+			       struct object *referred_commit)
 {
 	int want;
 	struct list_head *pos;
@@ -1333,21 +1352,13 @@ static int want_object_in_pack(const struct object_id *oid,
 	}
 
 	if (uri_protocols.nr) {
-		struct configured_exclusion *ex =
-			oidmap_get(&configured_exclusions, oid);
-		int i;
-		const char *p;
-
-		if (ex) {
-			for (i = 0; i < uri_protocols.nr; i++) {
-				if (skip_prefix(ex->uri,
-						uri_protocols.items[i].string,
-						&p) &&
-				    *p == ':') {
-					oidset_insert(&excluded_by_config, oid);
-					return 0;
-				}
-			}
+		if (referred_commit) {
+			if (oidmap_get(&configured_exclusions, &referred_commit->oid) && match_packfile_uri_exclusions(referred_ex))
+				return 0;
+		}
+		if (oidmap_get(&configured_exclusions, oid) && match_packfile_uri_exclusions(ex)) {
+			oidset_insert(&excluded_by_config, oid);
+			return 0;
 		}
 	}
 
@@ -1384,7 +1395,8 @@ static const char no_closure_warning[] = N_(
 );
 
 static int add_object_entry(const struct object_id *oid, enum object_type type,
-			    const char *name, int exclude)
+			    const char *name, int exclude,
+			    struct object *referred_commit)
 {
 	struct packed_git *found_pack = NULL;
 	off_t found_offset = 0;
@@ -1394,7 +1406,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
 	if (have_duplicate_entry(oid, exclude))
 		return 0;
 
-	if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {
+	if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset, referred_commit)) {
 		/* The pack is missing an object, so it will not have closure */
 		if (write_bitmap_index) {
 			if (write_bitmap_index != WRITE_BITMAP_QUIET)
@@ -1420,7 +1432,7 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,
 	if (have_duplicate_entry(oid, 0))
 		return 0;
 
-	if (!want_object_in_pack(oid, 0, &pack, &offset))
+	if (!want_object_in_pack(oid, 0, &pack, &offset, NULL))
 		return 0;
 
 	create_object_entry(oid, type, name_hash, 0, 0, pack, offset);
@@ -1560,7 +1572,7 @@ static void add_pbase_object(struct tree_desc *tree,
 		if (name[cmplen] != '/') {
 			add_object_entry(&entry.oid,
 					 object_type(entry.mode),
-					 fullname, 1);
+					 fullname, 1, NULL);
 			return;
 		}
 		if (S_ISDIR(entry.mode)) {
@@ -1628,7 +1640,7 @@ static void add_preferred_base_object(const char *name)
 	cmplen = name_cmp_len(name);
 	for (it = pbase_tree; it; it = it->next) {
 		if (cmplen == 0) {
-			add_object_entry(&it->pcache.oid, OBJ_TREE, NULL, 1);
+			add_object_entry(&it->pcache.oid, OBJ_TREE, NULL, 1, NULL);
 		}
 		else {
 			struct tree_desc tree;
@@ -2830,7 +2842,7 @@ static void add_tag_chain(const struct object_id *oid)
 			die(_("unable to pack objects reachable from tag %s"),
 			    oid_to_hex(oid));
 
-		add_object_entry(&tag->object.oid, OBJ_TAG, NULL, 0);
+		add_object_entry(&tag->object.oid, OBJ_TAG, NULL, 0, NULL);
 
 		if (tag->tagged->type != OBJ_TAG)
 			return;
@@ -2985,7 +2997,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.excludeobject") || !strcmp(k, "uploadpack.blobpackfileuri")) {
 		struct configured_exclusion *ex = xmalloc(sizeof(*ex));
 		const char *oid_end, *pack_end;
 		/*
@@ -2998,11 +3010,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.excludeobject or uploadpack.blobpackfileuri must be "
+                              "of the form '<object-hash> <pack-hash> <uri>' (got '%s')"), v);
 		if (oidmap_get(&configured_exclusions, &ex->e.oid))
-			die(_("object already configured in another "
-			      "uploadpack.blobpackfileuri (got '%s')"), v);
+                        die(_("object already configured by an earlier "
+                              "uploadpack.excludeobject or uploadpack.blobpackfileuri (got '%s')"), v);
 		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);
@@ -3031,7 +3043,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		return 0;
 
 	ofs = nth_packed_object_offset(p, pos);
-	if (!want_object_in_pack(oid, 0, &p, &ofs))
+	if (!want_object_in_pack(oid, 0, &p, &ofs, NULL))
 		return 0;
 
 	oi.typep = &type;
@@ -3059,7 +3071,7 @@ static void show_commit_pack_hint(struct commit *commit, void *_data)
 }
 
 static void show_object_pack_hint(struct object *object, const char *name,
-				  void *_data)
+				  void *show_data, void *carry_data)
 {
 	struct object_entry *oe = packlist_find(&to_pack, &object->oid);
 	if (!oe)
@@ -3224,7 +3236,7 @@ static void read_object_list_from_stdin(void)
 			die(_("expected object ID, got garbage:\n %s"), line);
 
 		add_preferred_base_object(p + 1);
-		add_object_entry(&oid, OBJ_NONE, p + 1, 0);
+		add_object_entry(&oid, OBJ_NONE, p + 1, 0, NULL);
 	}
 }
 
@@ -3233,7 +3245,7 @@ static void read_object_list_from_stdin(void)
 
 static void show_commit(struct commit *commit, void *data)
 {
-	add_object_entry(&commit->object.oid, OBJ_COMMIT, NULL, 0);
+	add_object_entry(&commit->object.oid, OBJ_COMMIT, NULL, 0, NULL);
 	commit->object.flags |= OBJECT_ADDED;
 
 	if (write_bitmap_index)
@@ -3243,10 +3255,11 @@ static void show_commit(struct commit *commit, void *data)
 		propagate_island_marks(commit);
 }
 
-static void show_object(struct object *obj, const char *name, void *data)
+static void show_object(struct object *obj, const char *name, void *show_data, void *carry_data)
 {
+	struct object *referred_commit = carry_data;
 	add_preferred_base_object(name);
-	add_object_entry(&obj->oid, obj->type, name, 0);
+	add_object_entry(&obj->oid, obj->type, name, 0, referred_commit);
 	obj->flags |= OBJECT_ADDED;
 
 	if (use_delta_islands) {
@@ -3265,7 +3278,7 @@ static void show_object(struct object *obj, const char *name, void *data)
 	}
 }
 
-static void show_object__ma_allow_any(struct object *obj, const char *name, void *data)
+static void show_object__ma_allow_any(struct object *obj, const char *name, void *show_data, void *carry_data)
 {
 	assert(arg_missing_action == MA_ALLOW_ANY);
 
@@ -3276,10 +3289,10 @@ static void show_object__ma_allow_any(struct object *obj, const char *name, void
 	if (!has_object(the_repository, &obj->oid, 0))
 		return;
 
-	show_object(obj, name, data);
+	show_object(obj, name, show_data, carry_data);
 }
 
-static void show_object__ma_allow_promisor(struct object *obj, const char *name, void *data)
+static void show_object__ma_allow_promisor(struct object *obj, const char *name, void *show_data, void *carry_data)
 {
 	assert(arg_missing_action == MA_ALLOW_PROMISOR);
 
@@ -3290,7 +3303,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
 	if (!has_object(the_repository, &obj->oid, 0) && is_promisor_object(&obj->oid))
 		return;
 
-	show_object(obj, name, data);
+	show_object(obj, name, show_data, carry_data);
 }
 
 static int option_parse_missing_action(const struct option *opt,
@@ -3397,7 +3410,7 @@ static void add_objects_in_unpacked_packs(void)
 		QSORT(in_pack.array, in_pack.nr, ofscmp);
 		for (i = 0; i < in_pack.nr; i++) {
 			struct object *o = in_pack.array[i].object;
-			add_object_entry(&o->oid, o->type, "", 0);
+			add_object_entry(&o->oid, o->type, "", 0, NULL);
 		}
 	}
 	free(in_pack.array);
@@ -3413,7 +3426,7 @@ static int add_loose_object(const struct object_id *oid, const char *path,
 		return 0;
 	}
 
-	add_object_entry(oid, type, "", 0);
+	add_object_entry(oid, type, "", 0, NULL);
 	return 0;
 }
 
@@ -3538,7 +3551,8 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
 
 static void record_recent_object(struct object *obj,
 				 const char *name,
-				 void *data)
+				 void *show_data,
+				 void *carry_data)
 {
 	oid_array_append(&recent_objects, &obj->oid);
 }
@@ -3831,7 +3845,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			 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")),
+				N_("exclude any configured uploadpack.excludeobject or "
+				   	"uploadpack.blobpackfileuri with this protocol")),
 		OPT_END(),
 	};
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index b4d8ea0a35..1cad33d9e8 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -266,7 +266,7 @@ static int finish_object(struct object *obj, const char *name, void *cb_data)
 	return 0;
 }
 
-static void show_object(struct object *obj, const char *name, void *cb_data)
+static void show_object(struct object *obj, const char *name, void *cb_data, void *carry_data)
 {
 	struct rev_list_info *info = cb_data;
 	struct rev_info *revs = info->revs;
diff --git a/fetch-pack.c b/fetch-pack.c
index 2318ebe680..39bb449586 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;
@@ -1576,6 +1577,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	struct string_list packfile_uris = STRING_LIST_INIT_DUP;
 	int i;
 	struct strvec index_pack_args = STRVEC_INIT;
+	struct strset uris;
 
 	negotiator = &negotiator_alloc;
 	fetch_negotiator_init(r, negotiator);
@@ -1677,6 +1679,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		}
 	}
 
+	strset_init(&uris);
 	for (i = 0; i < packfile_uris.nr; i++) {
 		int j;
 		struct child_process cmd = CHILD_PROCESS_INIT;
@@ -1684,6 +1687,8 @@ 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_add(&uris, uri))
+			continue;
 		strvec_push(&cmd.args, "http-fetch");
 		strvec_pushf(&cmd.args, "--packfile=%.*s",
 			     (int) the_hash_algo->hexsz,
@@ -1727,6 +1732,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 						 get_object_directory(),
 						 packname));
 	}
+	strset_clear(&uris);
 	string_list_clear(&packfile_uris, 0);
 	strvec_clear(&index_pack_args);
 
diff --git a/list-objects.c b/list-objects.c
index e19589baa0..fa3156dc89 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -24,7 +24,8 @@ struct traversal_context {
 static void process_blob(struct traversal_context *ctx,
 			 struct blob *blob,
 			 struct strbuf *path,
-			 const char *name)
+			 const char *name,
+			 struct object *referred_commit)
 {
 	struct object *obj = &blob->object;
 	size_t pathlen;
@@ -60,7 +61,7 @@ static void process_blob(struct traversal_context *ctx,
 	if (r & LOFR_MARK_SEEN)
 		obj->flags |= SEEN;
 	if (r & LOFR_DO_SHOW)
-		ctx->show_object(obj, path->buf, ctx->show_data);
+		ctx->show_object(obj, path->buf, ctx->show_data, referred_commit);
 	strbuf_setlen(path, pathlen);
 }
 
@@ -97,11 +98,13 @@ static void process_gitlink(struct traversal_context *ctx,
 static void process_tree(struct traversal_context *ctx,
 			 struct tree *tree,
 			 struct strbuf *base,
-			 const char *name);
+			 const char *name,
+			 struct object *referred_commit);
 
 static void process_tree_contents(struct traversal_context *ctx,
 				  struct tree *tree,
-				  struct strbuf *base)
+				  struct strbuf *base,
+				  struct object *referred_commit)
 {
 	struct tree_desc desc;
 	struct name_entry entry;
@@ -129,7 +132,7 @@ static void process_tree_contents(struct traversal_context *ctx,
 				    entry.path, oid_to_hex(&tree->object.oid));
 			}
 			t->object.flags |= NOT_USER_GIVEN;
-			process_tree(ctx, t, base, entry.path);
+			process_tree(ctx, t, base, entry.path, referred_commit);
 		}
 		else if (S_ISGITLINK(entry.mode))
 			process_gitlink(ctx, entry.oid.hash,
@@ -142,7 +145,7 @@ static void process_tree_contents(struct traversal_context *ctx,
 				    entry.path, oid_to_hex(&tree->object.oid));
 			}
 			b->object.flags |= NOT_USER_GIVEN;
-			process_blob(ctx, b, base, entry.path);
+			process_blob(ctx, b, base, entry.path, referred_commit);
 		}
 	}
 }
@@ -150,7 +153,8 @@ static void process_tree_contents(struct traversal_context *ctx,
 static void process_tree(struct traversal_context *ctx,
 			 struct tree *tree,
 			 struct strbuf *base,
-			 const char *name)
+			 const char *name,
+			 struct object *referred_commit)
 {
 	struct object *obj = &tree->object;
 	struct rev_info *revs = ctx->revs;
@@ -191,14 +195,14 @@ static void process_tree(struct traversal_context *ctx,
 	if (r & LOFR_MARK_SEEN)
 		obj->flags |= SEEN;
 	if (r & LOFR_DO_SHOW)
-		ctx->show_object(obj, base->buf, ctx->show_data);
+		ctx->show_object(obj, base->buf, ctx->show_data, referred_commit);
 	if (base->len)
 		strbuf_addch(base, '/');
 
 	if (r & LOFR_SKIP_TREE)
 		trace_printf("Skipping contents of tree %s...\n", base->buf);
 	else if (!failed_parse)
-		process_tree_contents(ctx, tree, base);
+		process_tree_contents(ctx, tree, base, referred_commit);
 
 	r = list_objects_filter__filter_object(ctx->revs->repo,
 					       LOFS_END_TREE, obj,
@@ -207,7 +211,7 @@ static void process_tree(struct traversal_context *ctx,
 	if (r & LOFR_MARK_SEEN)
 		obj->flags |= SEEN;
 	if (r & LOFR_DO_SHOW)
-		ctx->show_object(obj, base->buf, ctx->show_data);
+		ctx->show_object(obj, base->buf, ctx->show_data, referred_commit);
 
 	strbuf_setlen(base, baselen);
 	free_tree_buffer(tree);
@@ -314,9 +318,9 @@ void mark_edges_uninteresting(struct rev_info *revs,
 	}
 }
 
-static void add_pending_tree(struct rev_info *revs, struct tree *tree)
+static void add_pending_tree(struct rev_info *revs,  struct tree *tree, struct object *referred_commit)
 {
-	add_pending_object(revs, &tree->object, "");
+	add_pending_object_with_referred_commit(revs, &tree->object, "", referred_commit);
 }
 
 static void traverse_trees_and_blobs(struct traversal_context *ctx,
@@ -329,23 +333,24 @@ static void traverse_trees_and_blobs(struct traversal_context *ctx,
 	for (i = 0; i < ctx->revs->pending.nr; i++) {
 		struct object_array_entry *pending = ctx->revs->pending.objects + i;
 		struct object *obj = pending->item;
+		struct object *referred_commit = pending->referred_commit;
 		const char *name = pending->name;
 		const char *path = pending->path;
 		if (obj->flags & (UNINTERESTING | SEEN))
 			continue;
 		if (obj->type == OBJ_TAG) {
 			obj->flags |= SEEN;
-			ctx->show_object(obj, name, ctx->show_data);
+			ctx->show_object(obj, name, ctx->show_data, referred_commit);
 			continue;
 		}
 		if (!path)
 			path = "";
 		if (obj->type == OBJ_TREE) {
-			process_tree(ctx, (struct tree *)obj, base, path);
+			process_tree(ctx, (struct tree *)obj, base, path, referred_commit);
 			continue;
 		}
 		if (obj->type == OBJ_BLOB) {
-			process_blob(ctx, (struct blob *)obj, base, path);
+			process_blob(ctx, (struct blob *)obj, base, path, referred_commit);
 			continue;
 		}
 		die("unknown pending object %s (%s)",
@@ -370,7 +375,7 @@ static void do_traverse(struct traversal_context *ctx)
 		else if (get_commit_tree(commit)) {
 			struct tree *tree = get_commit_tree(commit);
 			tree->object.flags |= NOT_USER_GIVEN;
-			add_pending_tree(ctx->revs, tree);
+			add_pending_tree(ctx->revs, tree, &commit->object);
 		} else if (commit->object.parsed) {
 			die(_("unable to load root tree for commit %s"),
 			      oid_to_hex(&commit->object.oid));
diff --git a/list-objects.h b/list-objects.h
index a952680e46..ab946d34db 100644
--- a/list-objects.h
+++ b/list-objects.h
@@ -6,7 +6,7 @@ struct object;
 struct rev_info;
 
 typedef void (*show_commit_fn)(struct commit *, void *);
-typedef void (*show_object_fn)(struct object *, const char *, void *);
+typedef void (*show_object_fn)(struct object *, const char *, void *, void *);
 void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, void *);
 
 typedef void (*show_edge_fn)(struct commit *);
diff --git a/object.c b/object.c
index 14188453c5..6b1ce2fcde 100644
--- a/object.c
+++ b/object.c
@@ -322,9 +322,10 @@ void object_list_free(struct object_list **list)
  */
 static char object_array_slopbuf[1];
 
-void add_object_array_with_path(struct object *obj, const char *name,
-				struct object_array *array,
-				unsigned mode, const char *path)
+void add_object_array_with_path_and_referred_commit(struct object *obj, const char *name,
+						    struct object_array *array,
+						    unsigned mode, const char *path,
+						    struct object *referred_commit)
 {
 	unsigned nr = array->nr;
 	unsigned alloc = array->alloc;
@@ -339,6 +340,7 @@ void add_object_array_with_path(struct object *obj, const char *name,
 	}
 	entry = &objects[nr];
 	entry->item = obj;
+	entry->referred_commit = referred_commit;
 	if (!name)
 		entry->name = NULL;
 	else if (!*name)
@@ -354,6 +356,13 @@ void add_object_array_with_path(struct object *obj, const char *name,
 	array->nr = ++nr;
 }
 
+void add_object_array_with_path(struct object *obj, const char *name,
+				struct object_array *array,
+				unsigned mode, const char *path)
+{
+	add_object_array_with_path_and_referred_commit(obj, name, array, mode, path, NULL);
+}
+
 void add_object_array(struct object *obj, const char *name, struct object_array *array)
 {
 	add_object_array_with_path(obj, name, array, S_IFINVALID, NULL);
diff --git a/object.h b/object.h
index 87a6da47c8..de9f15b97d 100644
--- a/object.h
+++ b/object.h
@@ -43,6 +43,7 @@ struct object_array {
 	unsigned int alloc;
 	struct object_array_entry {
 		struct object *item;
+		struct object *referred_commit;
 		/*
 		 * name or NULL.  If non-NULL, the memory pointed to
 		 * is owned by this object *except* if it points at
@@ -157,6 +158,9 @@ void object_list_free(struct object_list **list);
 /* Object array handling .. */
 void add_object_array(struct object *obj, const char *name, struct object_array *array);
 void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);
+void add_object_array_with_path_and_referred_commit(struct object *obj, const char *name, struct object_array *array,
+						    unsigned mode, const char *path,
+						    struct object *referred_commit);
 
 /*
  * Returns NULL if the array is empty. Otherwise, returns the last object
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 3ed15431cd..516eb235da 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -459,9 +459,9 @@ struct bitmap_show_data {
 	struct bitmap *base;
 };
 
-static void show_object(struct object *object, const char *name, void *data_)
+static void show_object(struct object *object, const char *name, void *show_data, void *carry_data)
 {
-	struct bitmap_show_data *data = data_;
+	struct bitmap_show_data *data = show_data;
 	int bitmap_pos;
 
 	bitmap_pos = bitmap_position(data->bitmap_git, &object->oid);
@@ -1268,9 +1268,9 @@ struct bitmap_test_data {
 };
 
 static void test_show_object(struct object *object, const char *name,
-			     void *data)
+			     void *show_data, void *carry_data)
 {
-	struct bitmap_test_data *tdata = data;
+	struct bitmap_test_data *tdata = show_data;
 	int bitmap_pos;
 
 	bitmap_pos = bitmap_position(tdata->bitmap_git, &object->oid);
diff --git a/reachable.c b/reachable.c
index 77a60c70a5..ebd817c446 100644
--- a/reachable.c
+++ b/reachable.c
@@ -47,14 +47,14 @@ static int add_one_ref(const char *path, const struct object_id *oid,
  * The traversal will have already marked us as SEEN, so we
  * only need to handle any progress reporting here.
  */
-static void mark_object(struct object *obj, const char *name, void *data)
+static void mark_object(struct object *obj, const char *name, void *show_data, void *carry_data)
 {
-	update_progress(data);
+	update_progress(show_data);
 }
 
-static void mark_commit(struct commit *c, void *data)
+static void mark_commit(struct commit *c, void *show_data)
 {
-	mark_object(&c->object, NULL, data);
+	mark_object(&c->object, NULL, show_data,  NULL);
 }
 
 struct recent_data {
diff --git a/revision.c b/revision.c
index 4853c85d0b..da0ce0e3f2 100644
--- a/revision.c
+++ b/revision.c
@@ -304,10 +304,11 @@ void mark_parents_uninteresting(struct commit *commit)
 	commit_stack_clear(&pending);
 }
 
-static void add_pending_object_with_path(struct rev_info *revs,
-					 struct object *obj,
-					 const char *name, unsigned mode,
-					 const char *path)
+static void add_pending_object_with_path_and_referred_commit(struct rev_info *revs,
+							     struct object *obj,
+							     const char *name, unsigned mode,
+							     const char *path,
+							     struct object *referred_commit)
 {
 	struct interpret_branch_name_options options = { 0 };
 	if (!obj)
@@ -326,20 +327,36 @@ static void add_pending_object_with_path(struct rev_info *revs,
 		strbuf_release(&buf);
 		return; /* do not add the commit itself */
 	}
-	add_object_array_with_path(obj, name, &revs->pending, mode, path);
+	add_object_array_with_path_and_referred_commit(obj, name, &revs->pending, mode, path, referred_commit);
+}
+
+static void add_pending_object_with_path(struct rev_info *revs,
+					 struct object *obj,
+					 const char *name, unsigned mode,
+					 const char *path) {
+	add_pending_object_with_path_and_referred_commit(revs, obj, name, mode, path, NULL);
 }
 
 static void add_pending_object_with_mode(struct rev_info *revs,
 					 struct object *obj,
-					 const char *name, unsigned mode)
+					 const char *name, unsigned mode,
+					 struct object *referred_commit)
 {
-	add_pending_object_with_path(revs, obj, name, mode, NULL);
+
+	add_pending_object_with_path_and_referred_commit(revs, obj, name, mode, NULL, referred_commit);
+}
+
+void add_pending_object_with_referred_commit(struct rev_info *revs,
+					     struct object *obj, const char *name,
+					     struct object *referred_commit)
+{
+	add_pending_object_with_mode(revs, obj, name, S_IFINVALID, referred_commit);
 }
 
 void add_pending_object(struct rev_info *revs,
 			struct object *obj, const char *name)
 {
-	add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
+	add_pending_object_with_mode(revs, obj, name, S_IFINVALID, NULL);
 }
 
 void add_head_to_pending(struct rev_info *revs)
@@ -2764,7 +2781,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			continue;
 		}
 
-
 		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
 			int j;
 			if (seen_dashdash || *arg == '^')
@@ -2817,7 +2833,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		if (get_oid_with_context(revs->repo, revs->def, 0, &oid, &oc))
 			diagnose_missing_default(revs->def);
 		object = get_reference(revs, revs->def, &oid, 0);
-		add_pending_object_with_mode(revs, object, revs->def, oc.mode);
+		add_pending_object_with_mode(revs, object, revs->def, oc.mode, NULL);
 	}
 
 	/* Did the user ask for any diff output? Run the diff! */
diff --git a/revision.h b/revision.h
index a24f72dcd1..8a632e3587 100644
--- a/revision.h
+++ b/revision.h
@@ -424,6 +424,10 @@ void show_object_with_name(FILE *, struct object *, const char *);
 void add_pending_object(struct rev_info *revs,
 			struct object *obj, const char *name);
 
+void add_pending_object_with_referred_commit(struct rev_info *revs,
+					     struct object *obj, const char *name,
+					     struct object *referred_commit);
+
 void add_pending_oid(struct rev_info *revs,
 		     const char *name, const struct object_id *oid,
 		     unsigned int flags);
diff --git a/upload-pack.c b/upload-pack.c
index 5c1cd19612..d26fb351a3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1751,6 +1751,13 @@ int upload_pack_advertise(struct repository *r,
 			strbuf_addstr(value, " packfile-uris");
 			free(str);
 		}
+
+		if (!repo_config_get_string(the_repository,
+					    "uploadpack.excludeobject",
+					    &str) && str) {
+			strbuf_addstr(value, " packfile-uris");
+			free(str);
+		}
 	}
 
 	return 1;
-- 
2.31.1.443.g55c63af4c9.dirty


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 2/3] t5702: support for excluding commit objects
  2021-07-26  9:46   ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Teng Long
  2021-07-26  9:46     ` [PATCH v3 1/3] packfile-uris: support for excluding commit objects Teng Long
@ 2021-07-26  9:46     ` Teng Long
  2021-07-26 15:03       ` Ævar Arnfjörð Bjarmason
  2021-07-26  9:46     ` [PATCH v3 3/3] packfile-uri.txt: " Teng Long
  2021-07-26 12:34     ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Ævar Arnfjörð Bjarmason
  3 siblings, 1 reply; 18+ messages in thread
From: Teng Long @ 2021-07-26  9:46 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, avarab, Teng Long

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 t/t5702-protocol-v2.sh | 166 +++++++++++++++++++++++++++++++++--------
 1 file changed, 133 insertions(+), 33 deletions(-)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 2e1243ca40..bcf21e1445 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -753,7 +753,7 @@ test_expect_success 'ls-remote with v2 http sends only one POST' '
 '
 
 test_expect_success 'push with http:// and a config of v2 does not request v2' '
-	test_when_finished "rm -f log" &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child log" &&
 	# Till v2 for push is designed, make sure that if a client has
 	# protocol.version configured to use v2, that the client instead falls
 	# back and uses v0.
@@ -776,7 +776,7 @@ test_expect_success 'push with http:// and a config of v2 does not request v2' '
 '
 
 test_expect_success 'when server sends "ready", expect DELIM' '
-	rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child" &&
 
 	git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
 	test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one &&
@@ -796,7 +796,7 @@ test_expect_success 'when server sends "ready", expect DELIM' '
 '
 
 test_expect_success 'when server does not send "ready", expect FLUSH' '
-	rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child log &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child log" &&
 
 	git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
 	test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one &&
@@ -824,17 +824,44 @@ 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
+    objt="$1"
+    P="$2"
+	version="$3"
+
+    oldc="uploadpack.blobpackfileuri"
+    newc="uploadpack.excludeobject"
+	configkey=""
+	if test "$version" = "0"
+    then
+    	configkey="$oldc"
+    else
+    	configkey="$newc"
+	fi
+
+    if test "$objt" = "blob"
+    then
+        git -C "$P" hash-object "$3" >objh &&
+        git -C "$P" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
+        git -C "$P" config --add \
+                "$configkey" \
+                "$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
+        cat objh
+    elif test "$objt" = "commit" || test "$objt" = "tag"
+    then
+        echo "$3" >objh
+		git -C "$2" pack-objects --revs "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh
+        git -C "$P" config --add \
+                "$configkey" \
+                "$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
+        cat objh
+    else
+        echo "unsupported object type in configure_exclusion (got $objt)"
+    fi
 }
 
 test_expect_success 'part of packfile response provided as URI' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child log &&
+	test_when_finished "rm -rf \"$P\" http_child log" &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -843,10 +870,10 @@ test_expect_success 'part of packfile response provided as URI' '
 	git -C "$P" add my-blob &&
 	echo other-blob >"$P/other-blob" &&
 	git -C "$P" add other-blob &&
-	git -C "$P" commit -m x &&
+	test_commit -C "$P" A &&
 
-	configure_exclusion "$P" my-blob >h &&
-	configure_exclusion "$P" other-blob >h2 &&
+	configure_exclusion blob "$P" my-blob 0 >h &&
+	configure_exclusion blob "$P" other-blob 0 >h2 &&
 
 	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
 	git -c protocol.version=2 \
@@ -881,18 +908,40 @@ 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 &&
+	test_when_finished "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 &&
+	test_commit -C "$P" A &&
+
+	configure_exclusion blob "$P" my-blob >h &&
+
+	git init http_child &&
+
+	GIT_TEST_SIDEBAND_ALL=1 \
+	git -C http_child -c protocol.version=2 \
+		-c fetch.uriprotocols=http,https \
+		fetch "$HTTPD_URL/smart/http_parent"
+'
+
+test_expect_success 'blobs packfile URIs(Compatible with the old) with fetch instead of clone' '
+	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+	test_when_finished "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 &&
+	test_commit -C "$P" A &&
 
-	configure_exclusion "$P" my-blob >h &&
+	# with the old "uploadpack.blobpackfileuri" configure
+	configure_exclusion blob "$P" my-blob 0 >h &&
 
 	git init http_child &&
 
@@ -902,9 +951,60 @@ 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" &&
+	test_when_finished "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 &&
+	test_commit -C "$P" A &&
+
+	mycommit=$(git -C "$P" rev-parse A) &&
+	echo other-blob >"$P/other-blob" &&
+    git -C "$P" add other-blob &&
+	test_commit -C "$P" B &&
+	othercommit=$(git -C "$P" rev-parse B) &&
+
+	configure_exclusion commit "$P" "$mycommit" >h2 &&
+	configure_exclusion commit "$P" "$othercommit" >h2 &&
+
+	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"
+'
+
+test_expect_success 'tags packfile URIs with fetch instead of clone' '
+	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+ 	test_when_finished "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 &&
+	test_commit -C "$P" A &&
+	git -C "$P" tag -a -m "annotated_tag" tagA &&
+	tagObj=$(git -C "$P" rev-parse tagA) &&
+
+	configure_exclusion tag "$P" "$tagObj" >h2 &&
+
+	git init http_child &&
+
+	GIT_TRACE=1 GIT_TRACE_PACKET=1 GIT_TEST_SIDEBAND_ALL=1 \
+	git -C http_child -c protocol.version=2 \
+		-c fetch.uriprotocols=http,https \
+		fetch --tags "$HTTPD_URL/smart/http_parent"
+'
+
 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 &&
+	test_when_finished "rm -rf \"$P\" http_child log" &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -913,9 +1013,9 @@ test_expect_success 'fetching with valid packfile URI but invalid hash fails' '
 	git -C "$P" add my-blob &&
 	echo other-blob >"$P/other-blob" &&
 	git -C "$P" add other-blob &&
-	git -C "$P" commit -m x &&
+	test_commit -C "$P" A &&
 
-	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
@@ -923,7 +1023,7 @@ test_expect_success 'fetching with valid packfile URI but invalid hash fails' '
 	git -C "$P" hash-object other-blob >objh &&
 	git -C "$P" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
 	git -C "$P" config --add \
-		"uploadpack.blobpackfileuri" \
+		"uploadpack.excludeobject" \
 		"$(cat objh) $(cat objh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
 
 	test_must_fail env GIT_TEST_SIDEBAND_ALL=1 \
@@ -933,18 +1033,18 @@ test_expect_success 'fetching with valid packfile URI but invalid hash fails' '
 	test_i18ngrep "pack downloaded from.*does not match expected hash" err
 '
 
+
 test_expect_success 'packfile-uri with transfer.fsckobjects' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child log &&
+	test_when_finished "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 &&
+	test_commit -C "$P" A &&
+	configure_exclusion blob "$P" my-blob >h &&
 
 	sane_unset GIT_TEST_SIDEBAND_ALL &&
 	git -c protocol.version=2 -c transfer.fsckobjects=1 \
@@ -959,7 +1059,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects' '
 
 test_expect_success 'packfile-uri with transfer.fsckobjects fails on bad object' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child log &&
+	test_when_finished "rm -rf \"$P\" http_child log" &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -976,9 +1076,9 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails on bad object'
 
 	echo my-blob >"$P/my-blob" &&
 	git -C "$P" add my-blob &&
-	git -C "$P" commit -m x &&
+	test_commit -C "$P" A &&
 
-	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 \
@@ -989,7 +1089,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails on bad object'
 
 test_expect_success 'packfile-uri with transfer.fsckobjects succeeds when .gitmodules is separate from tree' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child &&
+	test_when_finished "rm -rf \"$P\" http_child" &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -1000,7 +1100,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 \
@@ -1015,7 +1115,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects succeeds when .gitmo
 
 test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodules separate from tree is invalid' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child err &&
+	test_when_finished "rm -rf \"$P\" http_child err" &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -1024,9 +1124,9 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul
 	echo "path = include/foo" >>"$P/.gitmodules" &&
 	echo "url = git://example.com/git/lib.git" >>"$P/.gitmodules" &&
 	git -C "$P" add .gitmodules &&
-	git -C "$P" commit -m x &&
+	test_commit -C "$P" A &&
 
-	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 \
-- 
2.31.1.443.g55c63af4c9.dirty


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 3/3] packfile-uri.txt: support for excluding commit objects
  2021-07-26  9:46   ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Teng Long
  2021-07-26  9:46     ` [PATCH v3 1/3] packfile-uris: support for excluding commit objects Teng Long
  2021-07-26  9:46     ` [PATCH v3 2/3] t5702: " Teng Long
@ 2021-07-26  9:46     ` Teng Long
  2021-07-26 20:52       ` Junio C Hamano
  2021-07-26 12:34     ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Ævar Arnfjörð Bjarmason
  3 siblings, 1 reply; 18+ messages in thread
From: Teng Long @ 2021-07-26  9:46 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, avarab, Teng Long

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/technical/packfile-uri.txt | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
index f7eabc6c76..2532db0e99 100644
--- a/Documentation/technical/packfile-uri.txt
+++ b/Documentation/technical/packfile-uri.txt
@@ -35,13 +35,16 @@ include some sort of non-trivial implementation in the Minimum Viable Product,
 at least so that we can test the client.
 
 This is the implementation: a feature, marked experimental, that allows the
-server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
-<uri>` entries. Whenever the list of objects to be sent is assembled, all such
-blobs are excluded, replaced with URIs. As noted in "Future work" below, the
-server can evolve in the future to support excluding other objects (or other
-implementations of servers could be made that support excluding other objects)
-without needing a protocol change, so clients should not expect that packfiles
-downloaded in this way only contain single blobs.
+server to be configured by one or more entries with the format:
+
+    uploadpack.excludeobject=<object-hash> <recursively> <pack-hash> <uri>
+
+Value <object-hash> is the key of entry, and the object type can be a blob
+or commit. Whenever the list of objects to be sent is assembled, all such
+objects are excluded, replaced with URIs. At the same time, for the old
+configuration `uploadpack.blobPackfileUri=<sha1> <pack-hash> <uri>` is
+still compatible for now, but this configuration only supports the
+exclusion of blob objects.
 
 Client design
 -------------
@@ -65,9 +68,6 @@ The protocol design allows some evolution of the server and client without any
 need for protocol changes, so only a small-scoped design is included here to
 form the MVP. For example, the following can be done:
 
- * On the server, more sophisticated means of excluding objects (e.g. by
-   specifying a commit to represent that commit and all objects that it
-   references).
  * On the client, resumption of clone. If a clone is interrupted, information
    could be recorded in the repository's config and a "clone-resume" command
    can resume the clone in progress. (Resumption of subsequent fetches is more
-- 
2.31.1.443.g55c63af4c9.dirty


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 0/3] packfile-uris: commit objects exclusio
  2021-07-26  9:46   ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Teng Long
                       ` (2 preceding siblings ...)
  2021-07-26  9:46     ` [PATCH v3 3/3] packfile-uri.txt: " Teng Long
@ 2021-07-26 12:34     ` Ævar Arnfjörð Bjarmason
  3 siblings, 0 replies; 18+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-26 12:34 UTC (permalink / raw)
  To: Teng Long; +Cc: git, jonathantanmy


On Mon, Jul 26 2021, Teng Long wrote:

> Range-diff against v2:
> -:  ---------- > 1:  91dce385f6 packfile-uris: support for excluding commit objects
> -:  ---------- > 2:  92def8c72b t5702: support for excluding commit objects
> -:  ---------- > 3:  01ab2cbb34 packfile-uri.txt: support for excluding commit objects

It looks like you provided the wrong base for the --range-diff (likely
master?), so it's not a diff against v2, just whatever you used as a
base.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/3] t5702: support for excluding commit objects
  2021-07-26  9:46     ` [PATCH v3 2/3] t5702: " Teng Long
@ 2021-07-26 15:03       ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 18+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-26 15:03 UTC (permalink / raw)
  To: Teng Long; +Cc: git, jonathantanmy


On Mon, Jul 26 2021, Teng Long wrote:

> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  t/t5702-protocol-v2.sh | 166 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 133 insertions(+), 33 deletions(-)
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 2e1243ca40..bcf21e1445 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -753,7 +753,7 @@ test_expect_success 'ls-remote with v2 http sends only one POST' '
>  '
>  
>  test_expect_success 'push with http:// and a config of v2 does not request v2' '
> -	test_when_finished "rm -f log" &&
> +	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child log" &&
>  	# Till v2 for push is designed, make sure that if a client has
>  	# protocol.version configured to use v2, that the client instead falls
>  	# back and uses v0.
> @@ -776,7 +776,7 @@ test_expect_success 'push with http:// and a config of v2 does not request v2' '
>  '
>  
>  test_expect_success 'when server sends "ready", expect DELIM' '
> -	rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child &&
> +	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child" &&
>  
>  	git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
>  	test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one &&
> @@ -796,7 +796,7 @@ test_expect_success 'when server sends "ready", expect DELIM' '
>  '
>  
>  test_expect_success 'when server does not send "ready", expect FLUSH' '
> -	rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child log &&
> +	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child log" &&
>  
>  	git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
>  	test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one &&
> @@ -824,17 +824,44 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
>  '

This looks like a good cleanup, but should be split into another cleanup
commit. It looks unrelated.

>  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
> +    objt="$1"
> +    P="$2"
> +	version="$3"
> +
> +    oldc="uploadpack.blobpackfileuri"
> +    newc="uploadpack.excludeobject"
> +	configkey=""
> +	if test "$version" = "0"
> +    then
> +    	configkey="$oldc"
> +    else
> +    	configkey="$newc"
> +	fi

You've got all sorts of mixed space/tab indent here.

> +    if test "$objt" = "blob"
> +    then
> +        git -C "$P" hash-object "$3" >objh &&
> +        git -C "$P" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
> +        git -C "$P" config --add \
> +                "$configkey" \
> +                "$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
> +        cat objh
> +    elif test "$objt" = "commit" || test "$objt" = "tag"
> +    then
> +        echo "$3" >objh
> +		git -C "$2" pack-objects --revs "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh
> +        git -C "$P" config --add \
> +                "$configkey" \
> +                "$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
> +        cat objh
> +    else
> +        echo "unsupported object type in configure_exclusion (got $objt)"
> +    fi
>  }
>  
>  test_expect_success 'part of packfile response provided as URI' '
>  	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
> -	rm -rf "$P" http_child log &&
> +	test_when_finished "rm -rf \"$P\" http_child log" &&
>  
>  	git init "$P" &&
>  	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
> @@ -843,10 +870,10 @@ test_expect_success 'part of packfile response provided as URI' '
>  	git -C "$P" add my-blob &&
>  	echo other-blob >"$P/other-blob" &&
>  	git -C "$P" add other-blob &&
> -	git -C "$P" commit -m x &&
> +	test_commit -C "$P" A &&
>  
> -	configure_exclusion "$P" my-blob >h &&
> -	configure_exclusion "$P" other-blob >h2 &&
> +	configure_exclusion blob "$P" my-blob 0 >h &&
> +	configure_exclusion blob "$P" other-blob 0 >h2 &&
>  
>  	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
>  	git -c protocol.version=2 \
> @@ -881,18 +908,40 @@ 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 &&
> +	test_when_finished "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 &&
> +	test_commit -C "$P" A &&
> +
> +	configure_exclusion blob "$P" my-blob >h &&
> +
> +	git init http_child &&
> +
> +	GIT_TEST_SIDEBAND_ALL=1 \
> +	git -C http_child -c protocol.version=2 \
> +		-c fetch.uriprotocols=http,https \

Isn't accepting http and https the default? Is this guarding against
config leak from a previous test? Ditto some later changes.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 1/3] packfile-uris: support for excluding commit objects
  2021-07-26  9:46     ` [PATCH v3 1/3] packfile-uris: support for excluding commit objects Teng Long
@ 2021-07-26 18:15       ` Junio C Hamano
  2021-07-26 19:45         ` Felipe Contreras
  0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2021-07-26 18:15 UTC (permalink / raw)
  To: Teng Long; +Cc: git, jonathantanmy, avarab

Teng Long <dyroneteng@gmail.com> writes:

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

Please avoid adjectives that express subjective values, like
"sophisticated".  Readers will expect a lot more sophistication than
your code actually offers and will be disappointed ("wow, that would
be wonderful if we can say 'exclude commits made by bots, and those
older than 3 months'---eh, you cannot do that?  where is your
sophistication then?").

Please avoid "should" without first describing the background for
"why it should".  It would help if you briefly describe what we
currently have and its limitation before this first paragraph
(i.e. your "we can already exclude only blob objects" would become
major part of the explanation, but you'd need to present in what
situations it would help to be able to exclude other types).

This commit is probalby doing too many things at once.  For example,
refactoring like creation of match_packfile_uri_exclusions() helper
function out of existing code (there probably are others) can and
should be done as separate preparatory steps before the API gets
modified (e.g. process-object callbacks gain an extra parameter) in
tree-wide way.

And by slimming the primary step that introduces the new feature,
there will be a space to also add documentation and test in the same
step, which would help reviewers.  With the current structure of the
series, with a code dump in the first step with only a vague promiss
of "sophistication" without documentation updates, reviewers cannot
even tell how the "commit object" is used easily.

Thanks.

>  builtin/describe.c     |  4 +-
>  builtin/pack-objects.c | 97 ++++++++++++++++++++++++------------------
>  builtin/rev-list.c     |  2 +-
>  fetch-pack.c           |  6 +++
>  list-objects.c         | 37 +++++++++-------
>  list-objects.h         |  2 +-
>  object.c               | 15 +++++--
>  object.h               |  4 ++
>  pack-bitmap.c          |  8 ++--
>  reachable.c            |  8 ++--
>  revision.c             | 36 +++++++++++-----
>  revision.h             |  4 ++
>  upload-pack.c          |  7 +++
>  13 files changed, 148 insertions(+), 82 deletions(-)
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 40482d8e9f..045da79b5c 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -485,9 +485,9 @@ static void process_commit(struct commit *commit, void *data)
>  	pcd->current_commit = commit->object.oid;
>  }
>  
> -static void process_object(struct object *obj, const char *path, void *data)
> +static void process_object(struct object *obj, const char *path, void *show_data, void *carry_data)
>  {
> -	struct process_commit_data *pcd = data;
> +	struct process_commit_data *pcd = show_data;
>  
>  	if (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {
>  		reset_revision_walk();
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 6d13cd3e1a..154c98bcb6 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1188,6 +1188,24 @@ static int have_duplicate_entry(const struct object_id *oid,
>  	return 1;
>  }
>  
> +static int match_packfile_uri_exclusions(struct configured_exclusion *ex)
> +{
> +	int i;
> +	const char *p;
> +
> +	if (ex) {
> +		for (i = 0; i < uri_protocols.nr; i++) {
> +			if (skip_prefix(ex->uri,
> +					uri_protocols.items[i].string,
> +					&p) &&
> +			    *p == ':')
> +				return 1;
> +
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int want_found_object(const struct object_id *oid, int exclude,
>  			     struct packed_git *p)
>  {
> @@ -1293,7 +1311,8 @@ static int want_object_in_pack_one(struct packed_git *p,
>  static int want_object_in_pack(const struct object_id *oid,
>  			       int exclude,
>  			       struct packed_git **found_pack,
> -			       off_t *found_offset)
> +			       off_t *found_offset,
> +			       struct object *referred_commit)
>  {
>  	int want;
>  	struct list_head *pos;
> @@ -1333,21 +1352,13 @@ static int want_object_in_pack(const struct object_id *oid,
>  	}
>  
>  	if (uri_protocols.nr) {
> -		struct configured_exclusion *ex =
> -			oidmap_get(&configured_exclusions, oid);
> -		int i;
> -		const char *p;
> -
> -		if (ex) {
> -			for (i = 0; i < uri_protocols.nr; i++) {
> -				if (skip_prefix(ex->uri,
> -						uri_protocols.items[i].string,
> -						&p) &&
> -				    *p == ':') {
> -					oidset_insert(&excluded_by_config, oid);
> -					return 0;
> -				}
> -			}
> +		if (referred_commit) {
> +			if (oidmap_get(&configured_exclusions, &referred_commit->oid) && match_packfile_uri_exclusions(referred_ex))
> +				return 0;
> +		}
> +		if (oidmap_get(&configured_exclusions, oid) && match_packfile_uri_exclusions(ex)) {
> +			oidset_insert(&excluded_by_config, oid);
> +			return 0;
>  		}
>  	}
>  
> @@ -1384,7 +1395,8 @@ static const char no_closure_warning[] = N_(
>  );
>  
>  static int add_object_entry(const struct object_id *oid, enum object_type type,
> -			    const char *name, int exclude)
> +			    const char *name, int exclude,
> +			    struct object *referred_commit)
>  {
>  	struct packed_git *found_pack = NULL;
>  	off_t found_offset = 0;
> @@ -1394,7 +1406,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
>  	if (have_duplicate_entry(oid, exclude))
>  		return 0;
>  
> -	if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {
> +	if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset, referred_commit)) {
>  		/* The pack is missing an object, so it will not have closure */
>  		if (write_bitmap_index) {
>  			if (write_bitmap_index != WRITE_BITMAP_QUIET)
> @@ -1420,7 +1432,7 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,
>  	if (have_duplicate_entry(oid, 0))
>  		return 0;
>  
> -	if (!want_object_in_pack(oid, 0, &pack, &offset))
> +	if (!want_object_in_pack(oid, 0, &pack, &offset, NULL))
>  		return 0;
>  
>  	create_object_entry(oid, type, name_hash, 0, 0, pack, offset);
> @@ -1560,7 +1572,7 @@ static void add_pbase_object(struct tree_desc *tree,
>  		if (name[cmplen] != '/') {
>  			add_object_entry(&entry.oid,
>  					 object_type(entry.mode),
> -					 fullname, 1);
> +					 fullname, 1, NULL);
>  			return;
>  		}
>  		if (S_ISDIR(entry.mode)) {
> @@ -1628,7 +1640,7 @@ static void add_preferred_base_object(const char *name)
>  	cmplen = name_cmp_len(name);
>  	for (it = pbase_tree; it; it = it->next) {
>  		if (cmplen == 0) {
> -			add_object_entry(&it->pcache.oid, OBJ_TREE, NULL, 1);
> +			add_object_entry(&it->pcache.oid, OBJ_TREE, NULL, 1, NULL);
>  		}
>  		else {
>  			struct tree_desc tree;
> @@ -2830,7 +2842,7 @@ static void add_tag_chain(const struct object_id *oid)
>  			die(_("unable to pack objects reachable from tag %s"),
>  			    oid_to_hex(oid));
>  
> -		add_object_entry(&tag->object.oid, OBJ_TAG, NULL, 0);
> +		add_object_entry(&tag->object.oid, OBJ_TAG, NULL, 0, NULL);
>  
>  		if (tag->tagged->type != OBJ_TAG)
>  			return;
> @@ -2985,7 +2997,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.excludeobject") || !strcmp(k, "uploadpack.blobpackfileuri")) {
>  		struct configured_exclusion *ex = xmalloc(sizeof(*ex));
>  		const char *oid_end, *pack_end;
>  		/*
> @@ -2998,11 +3010,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.excludeobject or uploadpack.blobpackfileuri must be "
> +                              "of the form '<object-hash> <pack-hash> <uri>' (got '%s')"), v);
>  		if (oidmap_get(&configured_exclusions, &ex->e.oid))
> -			die(_("object already configured in another "
> -			      "uploadpack.blobpackfileuri (got '%s')"), v);
> +                        die(_("object already configured by an earlier "
> +                              "uploadpack.excludeobject or uploadpack.blobpackfileuri (got '%s')"), v);
>  		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);
> @@ -3031,7 +3043,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,
>  		return 0;
>  
>  	ofs = nth_packed_object_offset(p, pos);
> -	if (!want_object_in_pack(oid, 0, &p, &ofs))
> +	if (!want_object_in_pack(oid, 0, &p, &ofs, NULL))
>  		return 0;
>  
>  	oi.typep = &type;
> @@ -3059,7 +3071,7 @@ static void show_commit_pack_hint(struct commit *commit, void *_data)
>  }
>  
>  static void show_object_pack_hint(struct object *object, const char *name,
> -				  void *_data)
> +				  void *show_data, void *carry_data)
>  {
>  	struct object_entry *oe = packlist_find(&to_pack, &object->oid);
>  	if (!oe)
> @@ -3224,7 +3236,7 @@ static void read_object_list_from_stdin(void)
>  			die(_("expected object ID, got garbage:\n %s"), line);
>  
>  		add_preferred_base_object(p + 1);
> -		add_object_entry(&oid, OBJ_NONE, p + 1, 0);
> +		add_object_entry(&oid, OBJ_NONE, p + 1, 0, NULL);
>  	}
>  }
>  
> @@ -3233,7 +3245,7 @@ static void read_object_list_from_stdin(void)
>  
>  static void show_commit(struct commit *commit, void *data)
>  {
> -	add_object_entry(&commit->object.oid, OBJ_COMMIT, NULL, 0);
> +	add_object_entry(&commit->object.oid, OBJ_COMMIT, NULL, 0, NULL);
>  	commit->object.flags |= OBJECT_ADDED;
>  
>  	if (write_bitmap_index)
> @@ -3243,10 +3255,11 @@ static void show_commit(struct commit *commit, void *data)
>  		propagate_island_marks(commit);
>  }
>  
> -static void show_object(struct object *obj, const char *name, void *data)
> +static void show_object(struct object *obj, const char *name, void *show_data, void *carry_data)
>  {
> +	struct object *referred_commit = carry_data;
>  	add_preferred_base_object(name);
> -	add_object_entry(&obj->oid, obj->type, name, 0);
> +	add_object_entry(&obj->oid, obj->type, name, 0, referred_commit);
>  	obj->flags |= OBJECT_ADDED;
>  
>  	if (use_delta_islands) {
> @@ -3265,7 +3278,7 @@ static void show_object(struct object *obj, const char *name, void *data)
>  	}
>  }
>  
> -static void show_object__ma_allow_any(struct object *obj, const char *name, void *data)
> +static void show_object__ma_allow_any(struct object *obj, const char *name, void *show_data, void *carry_data)
>  {
>  	assert(arg_missing_action == MA_ALLOW_ANY);
>  
> @@ -3276,10 +3289,10 @@ static void show_object__ma_allow_any(struct object *obj, const char *name, void
>  	if (!has_object(the_repository, &obj->oid, 0))
>  		return;
>  
> -	show_object(obj, name, data);
> +	show_object(obj, name, show_data, carry_data);
>  }
>  
> -static void show_object__ma_allow_promisor(struct object *obj, const char *name, void *data)
> +static void show_object__ma_allow_promisor(struct object *obj, const char *name, void *show_data, void *carry_data)
>  {
>  	assert(arg_missing_action == MA_ALLOW_PROMISOR);
>  
> @@ -3290,7 +3303,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
>  	if (!has_object(the_repository, &obj->oid, 0) && is_promisor_object(&obj->oid))
>  		return;
>  
> -	show_object(obj, name, data);
> +	show_object(obj, name, show_data, carry_data);
>  }
>  
>  static int option_parse_missing_action(const struct option *opt,
> @@ -3397,7 +3410,7 @@ static void add_objects_in_unpacked_packs(void)
>  		QSORT(in_pack.array, in_pack.nr, ofscmp);
>  		for (i = 0; i < in_pack.nr; i++) {
>  			struct object *o = in_pack.array[i].object;
> -			add_object_entry(&o->oid, o->type, "", 0);
> +			add_object_entry(&o->oid, o->type, "", 0, NULL);
>  		}
>  	}
>  	free(in_pack.array);
> @@ -3413,7 +3426,7 @@ static int add_loose_object(const struct object_id *oid, const char *path,
>  		return 0;
>  	}
>  
> -	add_object_entry(oid, type, "", 0);
> +	add_object_entry(oid, type, "", 0, NULL);
>  	return 0;
>  }
>  
> @@ -3538,7 +3551,8 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
>  
>  static void record_recent_object(struct object *obj,
>  				 const char *name,
> -				 void *data)
> +				 void *show_data,
> +				 void *carry_data)
>  {
>  	oid_array_append(&recent_objects, &obj->oid);
>  }
> @@ -3831,7 +3845,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  			 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")),
> +				N_("exclude any configured uploadpack.excludeobject or "
> +				   	"uploadpack.blobpackfileuri with this protocol")),
>  		OPT_END(),
>  	};
>  
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index b4d8ea0a35..1cad33d9e8 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -266,7 +266,7 @@ static int finish_object(struct object *obj, const char *name, void *cb_data)
>  	return 0;
>  }
>  
> -static void show_object(struct object *obj, const char *name, void *cb_data)
> +static void show_object(struct object *obj, const char *name, void *cb_data, void *carry_data)
>  {
>  	struct rev_list_info *info = cb_data;
>  	struct rev_info *revs = info->revs;
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 2318ebe680..39bb449586 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;
> @@ -1576,6 +1577,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  	struct string_list packfile_uris = STRING_LIST_INIT_DUP;
>  	int i;
>  	struct strvec index_pack_args = STRVEC_INIT;
> +	struct strset uris;
>  
>  	negotiator = &negotiator_alloc;
>  	fetch_negotiator_init(r, negotiator);
> @@ -1677,6 +1679,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  		}
>  	}
>  
> +	strset_init(&uris);
>  	for (i = 0; i < packfile_uris.nr; i++) {
>  		int j;
>  		struct child_process cmd = CHILD_PROCESS_INIT;
> @@ -1684,6 +1687,8 @@ 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_add(&uris, uri))
> +			continue;
>  		strvec_push(&cmd.args, "http-fetch");
>  		strvec_pushf(&cmd.args, "--packfile=%.*s",
>  			     (int) the_hash_algo->hexsz,
> @@ -1727,6 +1732,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  						 get_object_directory(),
>  						 packname));
>  	}
> +	strset_clear(&uris);
>  	string_list_clear(&packfile_uris, 0);
>  	strvec_clear(&index_pack_args);
>  
> diff --git a/list-objects.c b/list-objects.c
> index e19589baa0..fa3156dc89 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -24,7 +24,8 @@ struct traversal_context {
>  static void process_blob(struct traversal_context *ctx,
>  			 struct blob *blob,
>  			 struct strbuf *path,
> -			 const char *name)
> +			 const char *name,
> +			 struct object *referred_commit)
>  {
>  	struct object *obj = &blob->object;
>  	size_t pathlen;
> @@ -60,7 +61,7 @@ static void process_blob(struct traversal_context *ctx,
>  	if (r & LOFR_MARK_SEEN)
>  		obj->flags |= SEEN;
>  	if (r & LOFR_DO_SHOW)
> -		ctx->show_object(obj, path->buf, ctx->show_data);
> +		ctx->show_object(obj, path->buf, ctx->show_data, referred_commit);
>  	strbuf_setlen(path, pathlen);
>  }
>  
> @@ -97,11 +98,13 @@ static void process_gitlink(struct traversal_context *ctx,
>  static void process_tree(struct traversal_context *ctx,
>  			 struct tree *tree,
>  			 struct strbuf *base,
> -			 const char *name);
> +			 const char *name,
> +			 struct object *referred_commit);
>  
>  static void process_tree_contents(struct traversal_context *ctx,
>  				  struct tree *tree,
> -				  struct strbuf *base)
> +				  struct strbuf *base,
> +				  struct object *referred_commit)
>  {
>  	struct tree_desc desc;
>  	struct name_entry entry;
> @@ -129,7 +132,7 @@ static void process_tree_contents(struct traversal_context *ctx,
>  				    entry.path, oid_to_hex(&tree->object.oid));
>  			}
>  			t->object.flags |= NOT_USER_GIVEN;
> -			process_tree(ctx, t, base, entry.path);
> +			process_tree(ctx, t, base, entry.path, referred_commit);
>  		}
>  		else if (S_ISGITLINK(entry.mode))
>  			process_gitlink(ctx, entry.oid.hash,
> @@ -142,7 +145,7 @@ static void process_tree_contents(struct traversal_context *ctx,
>  				    entry.path, oid_to_hex(&tree->object.oid));
>  			}
>  			b->object.flags |= NOT_USER_GIVEN;
> -			process_blob(ctx, b, base, entry.path);
> +			process_blob(ctx, b, base, entry.path, referred_commit);
>  		}
>  	}
>  }
> @@ -150,7 +153,8 @@ static void process_tree_contents(struct traversal_context *ctx,
>  static void process_tree(struct traversal_context *ctx,
>  			 struct tree *tree,
>  			 struct strbuf *base,
> -			 const char *name)
> +			 const char *name,
> +			 struct object *referred_commit)
>  {
>  	struct object *obj = &tree->object;
>  	struct rev_info *revs = ctx->revs;
> @@ -191,14 +195,14 @@ static void process_tree(struct traversal_context *ctx,
>  	if (r & LOFR_MARK_SEEN)
>  		obj->flags |= SEEN;
>  	if (r & LOFR_DO_SHOW)
> -		ctx->show_object(obj, base->buf, ctx->show_data);
> +		ctx->show_object(obj, base->buf, ctx->show_data, referred_commit);
>  	if (base->len)
>  		strbuf_addch(base, '/');
>  
>  	if (r & LOFR_SKIP_TREE)
>  		trace_printf("Skipping contents of tree %s...\n", base->buf);
>  	else if (!failed_parse)
> -		process_tree_contents(ctx, tree, base);
> +		process_tree_contents(ctx, tree, base, referred_commit);
>  
>  	r = list_objects_filter__filter_object(ctx->revs->repo,
>  					       LOFS_END_TREE, obj,
> @@ -207,7 +211,7 @@ static void process_tree(struct traversal_context *ctx,
>  	if (r & LOFR_MARK_SEEN)
>  		obj->flags |= SEEN;
>  	if (r & LOFR_DO_SHOW)
> -		ctx->show_object(obj, base->buf, ctx->show_data);
> +		ctx->show_object(obj, base->buf, ctx->show_data, referred_commit);
>  
>  	strbuf_setlen(base, baselen);
>  	free_tree_buffer(tree);
> @@ -314,9 +318,9 @@ void mark_edges_uninteresting(struct rev_info *revs,
>  	}
>  }
>  
> -static void add_pending_tree(struct rev_info *revs, struct tree *tree)
> +static void add_pending_tree(struct rev_info *revs,  struct tree *tree, struct object *referred_commit)
>  {
> -	add_pending_object(revs, &tree->object, "");
> +	add_pending_object_with_referred_commit(revs, &tree->object, "", referred_commit);
>  }
>  
>  static void traverse_trees_and_blobs(struct traversal_context *ctx,
> @@ -329,23 +333,24 @@ static void traverse_trees_and_blobs(struct traversal_context *ctx,
>  	for (i = 0; i < ctx->revs->pending.nr; i++) {
>  		struct object_array_entry *pending = ctx->revs->pending.objects + i;
>  		struct object *obj = pending->item;
> +		struct object *referred_commit = pending->referred_commit;
>  		const char *name = pending->name;
>  		const char *path = pending->path;
>  		if (obj->flags & (UNINTERESTING | SEEN))
>  			continue;
>  		if (obj->type == OBJ_TAG) {
>  			obj->flags |= SEEN;
> -			ctx->show_object(obj, name, ctx->show_data);
> +			ctx->show_object(obj, name, ctx->show_data, referred_commit);
>  			continue;
>  		}
>  		if (!path)
>  			path = "";
>  		if (obj->type == OBJ_TREE) {
> -			process_tree(ctx, (struct tree *)obj, base, path);
> +			process_tree(ctx, (struct tree *)obj, base, path, referred_commit);
>  			continue;
>  		}
>  		if (obj->type == OBJ_BLOB) {
> -			process_blob(ctx, (struct blob *)obj, base, path);
> +			process_blob(ctx, (struct blob *)obj, base, path, referred_commit);
>  			continue;
>  		}
>  		die("unknown pending object %s (%s)",
> @@ -370,7 +375,7 @@ static void do_traverse(struct traversal_context *ctx)
>  		else if (get_commit_tree(commit)) {
>  			struct tree *tree = get_commit_tree(commit);
>  			tree->object.flags |= NOT_USER_GIVEN;
> -			add_pending_tree(ctx->revs, tree);
> +			add_pending_tree(ctx->revs, tree, &commit->object);
>  		} else if (commit->object.parsed) {
>  			die(_("unable to load root tree for commit %s"),
>  			      oid_to_hex(&commit->object.oid));
> diff --git a/list-objects.h b/list-objects.h
> index a952680e46..ab946d34db 100644
> --- a/list-objects.h
> +++ b/list-objects.h
> @@ -6,7 +6,7 @@ struct object;
>  struct rev_info;
>  
>  typedef void (*show_commit_fn)(struct commit *, void *);
> -typedef void (*show_object_fn)(struct object *, const char *, void *);
> +typedef void (*show_object_fn)(struct object *, const char *, void *, void *);
>  void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, void *);
>  
>  typedef void (*show_edge_fn)(struct commit *);
> diff --git a/object.c b/object.c
> index 14188453c5..6b1ce2fcde 100644
> --- a/object.c
> +++ b/object.c
> @@ -322,9 +322,10 @@ void object_list_free(struct object_list **list)
>   */
>  static char object_array_slopbuf[1];
>  
> -void add_object_array_with_path(struct object *obj, const char *name,
> -				struct object_array *array,
> -				unsigned mode, const char *path)
> +void add_object_array_with_path_and_referred_commit(struct object *obj, const char *name,
> +						    struct object_array *array,
> +						    unsigned mode, const char *path,
> +						    struct object *referred_commit)
>  {
>  	unsigned nr = array->nr;
>  	unsigned alloc = array->alloc;
> @@ -339,6 +340,7 @@ void add_object_array_with_path(struct object *obj, const char *name,
>  	}
>  	entry = &objects[nr];
>  	entry->item = obj;
> +	entry->referred_commit = referred_commit;
>  	if (!name)
>  		entry->name = NULL;
>  	else if (!*name)
> @@ -354,6 +356,13 @@ void add_object_array_with_path(struct object *obj, const char *name,
>  	array->nr = ++nr;
>  }
>  
> +void add_object_array_with_path(struct object *obj, const char *name,
> +				struct object_array *array,
> +				unsigned mode, const char *path)
> +{
> +	add_object_array_with_path_and_referred_commit(obj, name, array, mode, path, NULL);
> +}
> +
>  void add_object_array(struct object *obj, const char *name, struct object_array *array)
>  {
>  	add_object_array_with_path(obj, name, array, S_IFINVALID, NULL);
> diff --git a/object.h b/object.h
> index 87a6da47c8..de9f15b97d 100644
> --- a/object.h
> +++ b/object.h
> @@ -43,6 +43,7 @@ struct object_array {
>  	unsigned int alloc;
>  	struct object_array_entry {
>  		struct object *item;
> +		struct object *referred_commit;
>  		/*
>  		 * name or NULL.  If non-NULL, the memory pointed to
>  		 * is owned by this object *except* if it points at
> @@ -157,6 +158,9 @@ void object_list_free(struct object_list **list);
>  /* Object array handling .. */
>  void add_object_array(struct object *obj, const char *name, struct object_array *array);
>  void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);
> +void add_object_array_with_path_and_referred_commit(struct object *obj, const char *name, struct object_array *array,
> +						    unsigned mode, const char *path,
> +						    struct object *referred_commit);
>  
>  /*
>   * Returns NULL if the array is empty. Otherwise, returns the last object
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 3ed15431cd..516eb235da 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -459,9 +459,9 @@ struct bitmap_show_data {
>  	struct bitmap *base;
>  };
>  
> -static void show_object(struct object *object, const char *name, void *data_)
> +static void show_object(struct object *object, const char *name, void *show_data, void *carry_data)
>  {
> -	struct bitmap_show_data *data = data_;
> +	struct bitmap_show_data *data = show_data;
>  	int bitmap_pos;
>  
>  	bitmap_pos = bitmap_position(data->bitmap_git, &object->oid);
> @@ -1268,9 +1268,9 @@ struct bitmap_test_data {
>  };
>  
>  static void test_show_object(struct object *object, const char *name,
> -			     void *data)
> +			     void *show_data, void *carry_data)
>  {
> -	struct bitmap_test_data *tdata = data;
> +	struct bitmap_test_data *tdata = show_data;
>  	int bitmap_pos;
>  
>  	bitmap_pos = bitmap_position(tdata->bitmap_git, &object->oid);
> diff --git a/reachable.c b/reachable.c
> index 77a60c70a5..ebd817c446 100644
> --- a/reachable.c
> +++ b/reachable.c
> @@ -47,14 +47,14 @@ static int add_one_ref(const char *path, const struct object_id *oid,
>   * The traversal will have already marked us as SEEN, so we
>   * only need to handle any progress reporting here.
>   */
> -static void mark_object(struct object *obj, const char *name, void *data)
> +static void mark_object(struct object *obj, const char *name, void *show_data, void *carry_data)
>  {
> -	update_progress(data);
> +	update_progress(show_data);
>  }
>  
> -static void mark_commit(struct commit *c, void *data)
> +static void mark_commit(struct commit *c, void *show_data)
>  {
> -	mark_object(&c->object, NULL, data);
> +	mark_object(&c->object, NULL, show_data,  NULL);
>  }
>  
>  struct recent_data {
> diff --git a/revision.c b/revision.c
> index 4853c85d0b..da0ce0e3f2 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -304,10 +304,11 @@ void mark_parents_uninteresting(struct commit *commit)
>  	commit_stack_clear(&pending);
>  }
>  
> -static void add_pending_object_with_path(struct rev_info *revs,
> -					 struct object *obj,
> -					 const char *name, unsigned mode,
> -					 const char *path)
> +static void add_pending_object_with_path_and_referred_commit(struct rev_info *revs,
> +							     struct object *obj,
> +							     const char *name, unsigned mode,
> +							     const char *path,
> +							     struct object *referred_commit)
>  {
>  	struct interpret_branch_name_options options = { 0 };
>  	if (!obj)
> @@ -326,20 +327,36 @@ static void add_pending_object_with_path(struct rev_info *revs,
>  		strbuf_release(&buf);
>  		return; /* do not add the commit itself */
>  	}
> -	add_object_array_with_path(obj, name, &revs->pending, mode, path);
> +	add_object_array_with_path_and_referred_commit(obj, name, &revs->pending, mode, path, referred_commit);
> +}
> +
> +static void add_pending_object_with_path(struct rev_info *revs,
> +					 struct object *obj,
> +					 const char *name, unsigned mode,
> +					 const char *path) {
> +	add_pending_object_with_path_and_referred_commit(revs, obj, name, mode, path, NULL);
>  }
>  
>  static void add_pending_object_with_mode(struct rev_info *revs,
>  					 struct object *obj,
> -					 const char *name, unsigned mode)
> +					 const char *name, unsigned mode,
> +					 struct object *referred_commit)
>  {
> -	add_pending_object_with_path(revs, obj, name, mode, NULL);
> +
> +	add_pending_object_with_path_and_referred_commit(revs, obj, name, mode, NULL, referred_commit);
> +}
> +
> +void add_pending_object_with_referred_commit(struct rev_info *revs,
> +					     struct object *obj, const char *name,
> +					     struct object *referred_commit)
> +{
> +	add_pending_object_with_mode(revs, obj, name, S_IFINVALID, referred_commit);
>  }
>  
>  void add_pending_object(struct rev_info *revs,
>  			struct object *obj, const char *name)
>  {
> -	add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
> +	add_pending_object_with_mode(revs, obj, name, S_IFINVALID, NULL);
>  }
>  
>  void add_head_to_pending(struct rev_info *revs)
> @@ -2764,7 +2781,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  			continue;
>  		}
>  
> -
>  		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
>  			int j;
>  			if (seen_dashdash || *arg == '^')
> @@ -2817,7 +2833,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  		if (get_oid_with_context(revs->repo, revs->def, 0, &oid, &oc))
>  			diagnose_missing_default(revs->def);
>  		object = get_reference(revs, revs->def, &oid, 0);
> -		add_pending_object_with_mode(revs, object, revs->def, oc.mode);
> +		add_pending_object_with_mode(revs, object, revs->def, oc.mode, NULL);
>  	}
>  
>  	/* Did the user ask for any diff output? Run the diff! */
> diff --git a/revision.h b/revision.h
> index a24f72dcd1..8a632e3587 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -424,6 +424,10 @@ void show_object_with_name(FILE *, struct object *, const char *);
>  void add_pending_object(struct rev_info *revs,
>  			struct object *obj, const char *name);
>  
> +void add_pending_object_with_referred_commit(struct rev_info *revs,
> +					     struct object *obj, const char *name,
> +					     struct object *referred_commit);
> +
>  void add_pending_oid(struct rev_info *revs,
>  		     const char *name, const struct object_id *oid,
>  		     unsigned int flags);
> diff --git a/upload-pack.c b/upload-pack.c
> index 5c1cd19612..d26fb351a3 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1751,6 +1751,13 @@ int upload_pack_advertise(struct repository *r,
>  			strbuf_addstr(value, " packfile-uris");
>  			free(str);
>  		}
> +
> +		if (!repo_config_get_string(the_repository,
> +					    "uploadpack.excludeobject",
> +					    &str) && str) {
> +			strbuf_addstr(value, " packfile-uris");
> +			free(str);
> +		}
>  	}
>  
>  	return 1;

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 1/3] packfile-uris: support for excluding commit objects
  2021-07-26 18:15       ` Junio C Hamano
@ 2021-07-26 19:45         ` Felipe Contreras
  0 siblings, 0 replies; 18+ messages in thread
From: Felipe Contreras @ 2021-07-26 19:45 UTC (permalink / raw)
  To: Junio C Hamano, Teng Long; +Cc: git, jonathantanmy, avarab

Junio C Hamano wrote:
> Teng Long <dyroneteng@gmail.com> writes:
> 
> > On the server, more sophisticated means of excluding objects should be
> > supported, such as commit object. This commit introduces a new
> > configuration `uploadpack.excludeobject` for this.
> 
> Please avoid adjectives that express subjective values, like
> "sophisticated".

The word "sophisticated" is not necessarily subjective, it can easily
mean "complex".

https://www.merriam-webster.com/dictionary/sophisticated

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 3/3] packfile-uri.txt: support for excluding commit objects
  2021-07-26  9:46     ` [PATCH v3 3/3] packfile-uri.txt: " Teng Long
@ 2021-07-26 20:52       ` Junio C Hamano
  0 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2021-07-26 20:52 UTC (permalink / raw)
  To: Teng Long; +Cc: git, jonathantanmy, avarab

Teng Long <dyroneteng@gmail.com> writes:

> +++ b/Documentation/technical/packfile-uri.txt
> @@ -35,13 +35,16 @@ include some sort of non-trivial implementation in the Minimum Viable Product,
>  at least so that we can test the client.
>  
>  This is the implementation: a feature, marked experimental, that allows the
> +server to be configured by one or more entries with the format:
> +
> +    uploadpack.excludeobject=<object-hash> <recursively> <pack-hash> <uri>
> +
> +Value <object-hash> is the key of entry, and the object type can be a blob
> +or commit. Whenever the list of objects to be sent is assembled, all such
> +objects are excluded, replaced with URIs. At the same time, for the old
> +configuration `uploadpack.blobPackfileUri=<sha1> <pack-hash> <uri>` is
> +still compatible for now, but this configuration only supports the
> +exclusion of blob objects.

Do not hint deprecation and future removal with "still" and "for
now", before seeing a concensus that it should be deprecated and
removed.

The new thing, <recursively>, deserves some explanation.  What are
the acceptable values (yes/no? spatial/time/both? infinitely/limited?)
and what do these values mean?

Why is this limited to only <blob> and <commit>?

There isn't a fundamental reason why I shouldn't be able to say
"v2.32.0" instead of ebf3c04b262aa27fbb97f8a0156c2347fecafafb (or
"v2.32.0~0") to say "I want anything reachable from v2.32.0 (in
other words, that version and everything before it)", is there?

For that matter, "everything reachable from this tree object" may
also be a reasonable way to specify which set of objects are
offloaded to an out-of-band URI.

Thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] Packfile-uris support excluding commit objects
@ 2021-05-18  8:08 Teng Long
  0 siblings, 0 replies; 18+ messages in thread
From: Teng Long @ 2021-05-18  8:08 UTC (permalink / raw)
  To: avarab; +Cc: dyroneteng, git, jonathantanmy

To Ævar Arnfjörð Bjarmaso 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.

Obviously, the current series of patches will be longer, and the patches
of the separate documents you mentioned can be repaired and released in
advance. The latter, Junio said, will be added to the queue.

> 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?

Regarding the naming of "uploadpack.blobpackfileuri" and future
scalability issues, I have similar feelings. In next round, I will follow
your naming suggestions about "uploadpack.excludeObject". In addition,
the naming modification may cause a compatible question, although I know
packfile-uris is an experimental feature, I still hope to get some
compatibility-related suggestions.

> 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:
>        ......

Thanks for correcting, I may rewrite the commit message in the next
patch. I'm not a native English speaker, improving :)

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

The reason for splitting the two patches is mentioned above, and the
corresponding document modification to support the exclusion of commit
will be added in the next patch series.

> Nit: Split this across two lines.
> Indending with spaces.
> More indenting with spaces, also don't need the {} here.
> Don't indent the "then", also spaces...
> Use ">objh" not "> objh".

Thanks, will correct them in the next round.

> 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.

Thanks, will refer to your suggestions.

> 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...

I will try to modify it, but I am not sure to fully understand what you
mean. If this problem persists in the next patch, please help to point
out the problem.

> Put stuff like this in "test_when_finished"
> You can just use test_commit here, no?

Thanks, now I know about "test_when_finished" and "test_commit".
In addition, there are some problems with using "rm" directly in t5702,
I will replace them in the next patch series.

> 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...

Thanks, will use tag name(s) instead of "HEAD".

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-07-26 20:53 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07  2:11 [PATCH] Packfile-uris support excluding commit objects Teng Long
2021-05-10 11:14 ` Ævar Arnfjörð Bjarmason
2021-05-18  8:49 ` [PATCH v2 0/3] packfile-uris: commit objects exclusion Teng Long
2021-05-18  8:49   ` [PATCH v2 1/3] packfile-uris: support for excluding commit object Teng Long
2021-05-19  4:28     ` Junio C Hamano
2021-05-20  4:46     ` Junio C Hamano
2021-05-18  8:49   ` [PATCH v2 2/3] packfile-uris.txt: " Teng Long
2021-05-18  8:49   ` [PATCH v2 3/3] t5702: excluding commits with packfile-uris Teng Long
2021-07-26  9:46   ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Teng Long
2021-07-26  9:46     ` [PATCH v3 1/3] packfile-uris: support for excluding commit objects Teng Long
2021-07-26 18:15       ` Junio C Hamano
2021-07-26 19:45         ` Felipe Contreras
2021-07-26  9:46     ` [PATCH v3 2/3] t5702: " Teng Long
2021-07-26 15:03       ` Ævar Arnfjörð Bjarmason
2021-07-26  9:46     ` [PATCH v3 3/3] packfile-uri.txt: " Teng Long
2021-07-26 20:52       ` Junio C Hamano
2021-07-26 12:34     ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Ævar Arnfjörð Bjarmason
2021-05-18  8:08 [PATCH] Packfile-uris support excluding commit objects Teng Long

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git