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

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

[Jonathan Tan]

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

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

[Jonathan Tan]

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(-)

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

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

[Jonathan Tan]

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(-)

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

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

[Junio C Hamano]

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.

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

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

[Derrick Stolee]

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.

Thanks,
-Stolee

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

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

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

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

[Junio C Hamano]

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

>
> 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(-)

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

[Jonathan Tan]

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

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

[Jonathan Tan]

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

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

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

[Jonathan Tan]

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>
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(-)

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

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

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