threads / patch / 54269

patch, 2 partsFix overriding of partial clone filter when lazy-fetching

Subject: [PATCH 0/2] Fix overriding of partial clone filter when lazy-fetching

## tl;dr

11 messages between Sep 22, 2020 and Sep 28, 2020. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Jonathan Tan· Sep 22, 2020, 03:03 UTC · lore

This was noticed at $DAYJOB when in a partial clone with a filter other than blob:none. The 1st patch is a code cleanup (so that the 2nd patch does not need to take into account the unused variable), and the 2nd patch contains the bug fix.

Jonathan Tan (2):
  promisor-remote: remove unused variable
  fetch: do not override partial clone filter

 builtin/fetch.c               |  2 +-
 list-objects-filter-options.c | 10 +++++++++-
 promisor-remote.c             |  5 -----
 promisor-remote.h             |  2 +-
 t/t5601-clone.sh              |  5 +++++
 5 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.28.0.681.g6f77f65b4e-goog

Jonathan Tan· Sep 22, 2020, 03:03 UTC · re: Jonathan Tan · lore

[PATCH 2/2] fetch: do not override partial clone filter

When a fetch with the --filter argument is made, the configured default filter is set even if one already exists. This change was made in 5e46139376 ("builtin/fetch: remove unique promisor remote limitation", 2019-06-25) - in particular, changing from:

 * If this is the FIRST partial-fetch request, we enable partial
 * on this repo and remember the given filter-spec as the default
 * for subsequent fetches to this remote.

to:

 * If this is a partial-fetch request, we enable partial on
 * this repo if not already enabled and remember the given
 * filter-spec as the default for subsequent fetches to this
 * remote.

(The given filter-spec is "remembered" even if there is already an existing one.)

This is problematic whenever a lazy fetch is made, because lazy fetches are made using "git fetch --filter=blob:none", but this will also happen if the user invokes "git fetch --filter=<filter>" manually. Therefore, restore the behavior prior to 5e46139376, which writes a filter-spec only if the current fetch request is the first partial-fetch one (for that remote).

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/fetch.c               |  2 +-
 list-objects-filter-options.c | 10 +++++++++-
 t/t5601-clone.sh              |  5 +++++
 3 files changed, 15 insertions(+), 2 deletions(-)

Show changes to 3 files +15 −2

builtin/fetch.c, list-objects-filter-options.c, t/t5601-clone.sh

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a6d3268661..752148eec5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1677,7 +1677,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)
 	 * If this is a partial-fetch request, we enable partial on
 	 * this repo if not already enabled and remember the given
 	 * filter-spec as the default for subsequent fetches to this
-	 * remote.
+	 * remote if there is currently no default filter-spec.
 	 */
 	if (filter_options.choice) {
 		partial_clone_register(remote->name, &filter_options);
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index b66314560a..defd3dfd10 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -344,11 +344,19 @@ void partial_clone_register(
 	const char *remote,
 	struct list_objects_filter_options *filter_options)
 {
+	struct promisor_remote *promisor_remote;
 	char *cfg_name;
 	char *filter_name;
 
 	/* Check if it is already registered */
-	if (!promisor_remote_find(remote)) {
+	if ((promisor_remote = promisor_remote_find(remote))) {
+		if (promisor_remote->partial_clone_filter)
+			/*
+			 * Remote is already registered and a filter is already
+			 * set, so we don't need to do anything here.
+			 */
+			return;
+	} else {
 		if (upgrade_repository_format(1) < 0)
 			die(_("unable to upgrade repository format to support partial clone"));
 
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 15fb64c18d..7c5cf5a2ec 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -672,6 +672,11 @@ test_expect_success 'partial clone with -o' '
 	git clone -o blah --filter=blob:limit=0 "file://$(pwd)/server" client
 '
 
+test_expect_success 'ensure that filter is written to config' '
+	FILTER=$(git -C client config --get remote.blah.partialclonefilter) &&
+	test "$FILTER" == "blob:limit=0"
+'
+
 test_expect_success 'partial clone: warn if server does not support object filtering' '
 	rm -rf server client &&
 	test_create_repo server &&

-- 
2.28.0.681.g6f77f65b4e-goog

Junio C Hamano· Sep 22, 2020, 05:46 UTC · re: Jonathan Tan · lore

Re: [PATCH 2/2] fetch: do not override partial clone filter

Jonathan Tan <jonathantanmy@google.com> writes:

> +test_expect_success 'ensure that filter is written to config' '
> +	FILTER=$(git -C client config --get remote.blah.partialclonefilter) &&
> +	test "$FILTER" == "blob:limit=0"

I'll do s/==/=/ locally because otherwise the test-lint-shell-syntax will choke on this line. No need to resend only this line.

Thanks.

Show 5 quoted lines

> +'
> +
>  test_expect_success 'partial clone: warn if server does not support object filtering' '
>  	rm -rf server client &&
>  	test_create_repo server &&

Derrick Stolee· Sep 22, 2020, 11:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] fetch: do not override partial clone filter

On 9/22/2020 1:46 AM, Junio C Hamano wrote:

Show 8 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
>> +test_expect_success 'ensure that filter is written to config' '
>> +	FILTER=$(git -C client config --get remote.blah.partialclonefilter) &&
>> +	test "$FILTER" == "blob:limit=0"
> 
> I'll do s/==/=/ locally because otherwise the test-lint-shell-syntax
> will choke on this line.  No need to resend only this line.

Wouldn't "test_cmp_config" be a better here? It seems like this is testing the "git clone" behavior of the previous test, so it could be added to that test instead.

Thanks, -Stolee

Junio C Hamano· Sep 22, 2020, 15:51 UTC · re: Derrick Stolee · lore

Re: [PATCH 2/2] fetch: do not override partial clone filter

Derrick Stolee <stolee@gmail.com> writes:

Show 13 quoted lines

> On 9/22/2020 1:46 AM, Junio C Hamano wrote:
>> Jonathan Tan <jonathantanmy@google.com> writes:
>> 
>>> +test_expect_success 'ensure that filter is written to config' '
>>> +	FILTER=$(git -C client config --get remote.blah.partialclonefilter) &&
>>> +	test "$FILTER" == "blob:limit=0"
>> 
>> I'll do s/==/=/ locally because otherwise the test-lint-shell-syntax
>> will choke on this line.  No need to resend only this line.
>
> Wouldn't "test_cmp_config" be a better here? It seems like
> this is testing the "git clone" behavior of the previous test,
> so it could be added to that test instead.

TIL that one ;-) "git config --get" gives only a single instance, but in this particular case, shouldn't we be verifying not just that the expected one is found, but it is the only one, with "--get-all"?

FWIW, many uses of that test helper may want to be tightened the same way. In short, unless we are testing the last-one-wins behaviour, and especially when we are interested in what changes our tested operation makes to the config file's contents, we should be using "--get-all" instead of the default "--get", I suspect.

Thanks.

Jonathan Tan· Sep 22, 2020, 03:03 UTC · re: Jonathan Tan · lore

[PATCH 1/2] promisor-remote: remove unused variable

The variable core_partial_clone_filter_default has been unused since fa3d1b63e8 ("promisor-remote: parse remote.*.partialclonefilter", 2019-06-25), when Git was changed to refer to remote.*.partialclonefilter as the default filter when fetching in a partial clone, but (perhaps inadvertently) there was no fallback to core.partialclonefilter.

One alternative is to add the fallback, but the aforementioned change was made more than a year ago and I have not heard of any complaints regarding this matter. In addition, there is currently no mention of core.partialclonefilter in the user documentation. So it seems best to reaffirm that Git will only support remote.*.partialclonefilter.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 promisor-remote.c | 5 -----
 promisor-remote.h | 2 +-
 2 files changed, 1 insertion(+), 6 deletions(-)

Show changes to 2 files +1 −6

promisor-remote.c, promisor-remote.h

diff --git a/promisor-remote.c b/promisor-remote.c
index 6530e26f98..3c572b1c81 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -6,7 +6,6 @@
 #include "strvec.h"
 
 static char *repository_format_partial_clone;
-static const char *core_partial_clone_filter_default;
 
 void set_repository_format_partial_clone(char *partial_clone)
 {
@@ -100,10 +99,6 @@ static int promisor_remote_config(const char *var, const char *value, void *data
 	size_t namelen;
 	const char *subkey;
 
-	if (!strcmp(var, "core.partialclonefilter"))
-		return git_config_string(&core_partial_clone_filter_default,
-					 var, value);
-
 	if (parse_config_key(var, "remote", &name, &namelen, &subkey) < 0)
 		return 0;
 
diff --git a/promisor-remote.h b/promisor-remote.h
index 6343c47d18..c7a14063c5 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -9,7 +9,7 @@ struct object_id;
  * Promisor remote linked list
  *
  * Information in its fields come from remote.XXX config entries or
- * from extensions.partialclone or core.partialclonefilter.
+ * from extensions.partialclone.
  */
 struct promisor_remote {
 	struct promisor_remote *next;

-- 
2.28.0.681.g6f77f65b4e-goog

Junio C Hamano· Sep 22, 2020, 05:45 UTC · re: Jonathan Tan · lore

Re: [PATCH 0/2] Fix overriding of partial clone filter when lazy-fetching

Jonathan Tan <jonathantanmy@google.com> writes:

> This was noticed at $DAYJOB when in a partial clone with a filter other
> than blob:none. The 1st patch is a code cleanup (so that the 2nd patch
> does not need to take into account the unused variable), and the 2nd
> patch contains the bug fix.

What is "this" is not clear at all. This is one of those cover letters unfriendly to readers who do not know what the author did before being told X-<. We got a vague "we found some unspecified bug and fixed it, and we need to read them through if we want to find out what it is".

Show 11 quoted lines

>
> Jonathan Tan (2):
>   promisor-remote: remove unused variable
>   fetch: do not override partial clone filter
>
>  builtin/fetch.c               |  2 +-
>  list-objects-filter-options.c | 10 +++++++++-
>  promisor-remote.c             |  5 -----
>  promisor-remote.h             |  2 +-
>  t/t5601-clone.sh              |  5 +++++
>  5 files changed, 16 insertions(+), 8 deletions(-)

Jonathan Tan· Sep 28, 2020, 22:26 UTC · re: Jonathan Tan · lore

[PATCH v2 0/2] Fix overriding of partial clone filter when lazy-fetching

Thanks everyone for your comments. Patch 1 is exactly the same, and in patch 2, I've changed the config check to use test_cmp_config with "--get-all", as requested.

Comment noted about saying "this" without informing the reader what it's referring to - in this case, it was about the issue in patch 2.

Jonathan Tan (2):
  promisor-remote: remove unused variable
  fetch: do not override partial clone filter

 builtin/fetch.c               |  2 +-
 list-objects-filter-options.c | 10 +++++++++-
 promisor-remote.c             |  5 -----
 promisor-remote.h             |  2 +-
 t/t5601-clone.sh              |  3 ++-
 5 files changed, 13 insertions(+), 9 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  625e7f148e promisor-remote: remove unused variable
1:  af960f33b6 ! 2:  c3ced59875 fetch: do not override partial clone filter
    @@ list-objects-filter-options.c: void partial_clone_register(
      
     
      ## t/t5601-clone.sh ##
    -@@ t/t5601-clone.sh: test_expect_success 'partial clone with -o' '
    - 	git clone -o blah --filter=blob:limit=0 "file://$(pwd)/server" client
    +@@ t/t5601-clone.sh: test_expect_success 'partial clone' '
    + 
    + test_expect_success 'partial clone with -o' '
    + 	partial_clone_server server &&
    +-	git clone -o blah --filter=blob:limit=0 "file://$(pwd)/server" client
    ++	git clone -o blah --filter=blob:limit=0 "file://$(pwd)/server" client &&
    ++	test_cmp_config -C client "blob:limit=0" --get-all remote.blah.partialclonefilter
      '
      
    -+test_expect_success 'ensure that filter is written to config' '
    -+	FILTER=$(git -C client config --get remote.blah.partialclonefilter) &&
    -+	test "$FILTER" = "blob:limit=0"
    -+'
    -+
      test_expect_success 'partial clone: warn if server does not support object filtering' '
    - 	rm -rf server client &&
    - 	test_create_repo server &&

-- 
2.28.0.709.gb0816b6eb0-goog

Jonathan Tan· Sep 28, 2020, 22:26 UTC · re: Jonathan Tan · lore

[PATCH v2 2/2] fetch: do not override partial clone filter

 * If this is the FIRST partial-fetch request, we enable partial
 * on this repo and remember the given filter-spec as the default
 * for subsequent fetches to this remote.

to:

 * If this is a partial-fetch request, we enable partial on
 * this repo if not already enabled and remember the given
 * filter-spec as the default for subsequent fetches to this
 * remote.

(The given filter-spec is "remembered" even if there is already an existing one.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/fetch.c               |  2 +-
 list-objects-filter-options.c | 10 +++++++++-
 t/t5601-clone.sh              |  3 ++-
 3 files changed, 12 insertions(+), 3 deletions(-)

Show changes to 3 files +12 −3

builtin/fetch.c, list-objects-filter-options.c, t/t5601-clone.sh

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c555836937..97930f8201 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1677,7 +1677,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)
 	 * If this is a partial-fetch request, we enable partial on
 	 * this repo if not already enabled and remember the given
 	 * filter-spec as the default for subsequent fetches to this
-	 * remote.
+	 * remote if there is currently no default filter-spec.
 	 */
 	if (filter_options.choice) {
 		partial_clone_register(remote->name, &filter_options);
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index b66314560a..defd3dfd10 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -344,11 +344,19 @@ void partial_clone_register(
 	const char *remote,
 	struct list_objects_filter_options *filter_options)
 {
+	struct promisor_remote *promisor_remote;
 	char *cfg_name;
 	char *filter_name;
 
 	/* Check if it is already registered */
-	if (!promisor_remote_find(remote)) {
+	if ((promisor_remote = promisor_remote_find(remote))) {
+		if (promisor_remote->partial_clone_filter)
+			/*
+			 * Remote is already registered and a filter is already
+			 * set, so we don't need to do anything here.
+			 */
+			return;
+	} else {
 		if (upgrade_repository_format(1) < 0)
 			die(_("unable to upgrade repository format to support partial clone"));
 
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 15fb64c18d..4631f019fe 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -669,7 +669,8 @@ test_expect_success 'partial clone' '
 
 test_expect_success 'partial clone with -o' '
 	partial_clone_server server &&
-	git clone -o blah --filter=blob:limit=0 "file://$(pwd)/server" client
+	git clone -o blah --filter=blob:limit=0 "file://$(pwd)/server" client &&
+	test_cmp_config -C client "blob:limit=0" --get-all remote.blah.partialclonefilter
 '
 
 test_expect_success 'partial clone: warn if server does not support object filtering' '

-- 
2.28.0.709.gb0816b6eb0-goog

Junio C Hamano· Sep 28, 2020, 23:43 UTC · re: Jonathan Tan · lore

Re: [PATCH v2 2/2] fetch: do not override partial clone filter

Jonathan Tan <jonathantanmy@google.com> writes:

Show 6 quoted lines

>  test_expect_success 'partial clone with -o' '
>  	partial_clone_server server &&
> -	git clone -o blah --filter=blob:limit=0 "file://$(pwd)/server" client
> +	git clone -o blah --filter=blob:limit=0 "file://$(pwd)/server" client &&
> +	test_cmp_config -C client "blob:limit=0" --get-all remote.blah.partialclonefilter
>  '

Makes sense to piggyback on this existing test. It also makes sense to ensure that the original one is all and the only filter that is being configured.

Will queue.  Let's merge them to 'next'.

Jonathan Tan· Sep 28, 2020, 22:26 UTC · re: Jonathan Tan · lore

[PATCH v2 1/2] promisor-remote: remove unused variable

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 promisor-remote.c | 5 -----
 promisor-remote.h | 2 +-
 2 files changed, 1 insertion(+), 6 deletions(-)

Show changes to 2 files +1 −6

promisor-remote.c, promisor-remote.h

diff --git a/promisor-remote.c b/promisor-remote.c
index 6530e26f98..3c572b1c81 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -6,7 +6,6 @@
 #include "strvec.h"
 
 static char *repository_format_partial_clone;
-static const char *core_partial_clone_filter_default;
 
 void set_repository_format_partial_clone(char *partial_clone)
 {
@@ -100,10 +99,6 @@ static int promisor_remote_config(const char *var, const char *value, void *data
 	size_t namelen;
 	const char *subkey;
 
-	if (!strcmp(var, "core.partialclonefilter"))
-		return git_config_string(&core_partial_clone_filter_default,
-					 var, value);
-
 	if (parse_config_key(var, "remote", &name, &namelen, &subkey) < 0)
 		return 0;
 
diff --git a/promisor-remote.h b/promisor-remote.h
index 6343c47d18..c7a14063c5 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -9,7 +9,7 @@ struct object_id;
  * Promisor remote linked list
  *
  * Information in its fields come from remote.XXX config entries or
- * from extensions.partialclone or core.partialclonefilter.
+ * from extensions.partialclone.
  */
 struct promisor_remote {
 	struct promisor_remote *next;

-- 
2.28.0.709.gb0816b6eb0-goog