[RFC PATCH 0/1] Expand abbreviated filters

[RFC PATCH 0/1] Expand abbreviated filters

[Josh Steadmon]

NOTE: this patch is based on top of md/list-objects-filter-by-depth

Currently, git clients pass filter specs verbatim over the network and
to subprocesses. We support various numeric abbreviations for parameters
on these limits (via git_parse_ulong()), but other implementations may
not support the same abbreviations (or may support them differently;
e.g., should "1k" == 1000 or 1024?). It would be better to only pass
fully-expanded numbers in this case, and keep the expansion logic
completely on the client side.

This patch updates the protocol-v2 doc to specify that clients MUST
expand abbreviations in filter specifications before passing them to
other processes. It adds a new function
"expand_list_objects_filter_spec()" in list-objects-filter-options.c
that implements the expansion logic, and updates users of the
filter_spec field to instead expand the spec first.

Josh Steadmon (1):
  filter-options: Expand abbreviated numbers

 Documentation/technical/protocol-v2.txt |  5 ++++-
 builtin/clone.c                         |  6 +++++-
 builtin/fetch.c                         |  7 ++++++-
 fetch-pack.c                            | 15 ++++++++++++---
 list-objects-filter-options.c           | 20 ++++++++++++++++++--
 list-objects-filter-options.h           | 17 +++++++++++++++--
 t/t6112-rev-list-filters-objects.sh     | 17 +++++++++++++++++
 transport-helper.c                      | 13 +++++++++----
 upload-pack.c                           |  7 +++++--
 9 files changed, 91 insertions(+), 16 deletions(-)

-- 
2.20.0.405.gbc1bbc6f85-goog

[RFC PATCH 1/1] filter-options: Expand abbreviated numbers

[Josh Steadmon]

When communicating with a remote server or a subprocess, use expanded
numbers rather than abbreviated numbers in the object filter spec (e.g.
"limit:blob=1k" becomes "limit:blob=1024").

Update the protocol docs to note that clients should always perform this
expansion, to allow for more compatibility between server
implementations.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/technical/protocol-v2.txt |  5 ++++-
 builtin/clone.c                         |  6 +++++-
 builtin/fetch.c                         |  7 ++++++-
 fetch-pack.c                            | 15 ++++++++++++---
 list-objects-filter-options.c           | 20 ++++++++++++++++++--
 list-objects-filter-options.h           | 17 +++++++++++++++--
 t/t6112-rev-list-filters-objects.sh     | 17 +++++++++++++++++
 transport-helper.c                      | 13 +++++++++----
 upload-pack.c                           |  7 +++++--
 9 files changed, 91 insertions(+), 16 deletions(-)

diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 09e4e0273f..d001372404 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -296,7 +296,10 @@ included in the client's request:
 	Request that various objects from the packfile be omitted
 	using one of several filtering techniques. These are intended
 	for use with partial clone and partial fetch operations. See
-	`rev-list` for possible "filter-spec" values.
+	`rev-list` for possible "filter-spec" values. Clients MUST
+	translate abbreviated numbers (e.g. "1k") into fully-expanded
+	numbers (e.g. "1024") on the client side, so that the server
+	does not need to implement unit parsing.
 
 If the 'ref-in-want' feature is advertised, the following argument can
 be included in the client's request as well as the potential addition of
diff --git a/builtin/clone.c b/builtin/clone.c
index 15b142d646..8e05e8ad6c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1130,9 +1130,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 				     option_upload_pack);
 
 	if (filter_options.choice) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
+		expand_list_objects_filter_spec(&filter_options,
+						&expanded_filter_spec);
 		transport_set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
-				     filter_options.filter_spec);
+				     expanded_filter_spec.buf);
 		transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
+		strbuf_release(&expanded_filter_spec);
 	}
 
 	if (transport->smart_options && !deepen && !filter_options.choice)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e0140327aa..8b8bb64921 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1172,6 +1172,7 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
 static struct transport *prepare_transport(struct remote *remote, int deepen)
 {
 	struct transport *transport;
+
 	transport = transport_get(remote, NULL);
 	transport_set_verbosity(transport, verbosity, progress);
 	transport->family = family;
@@ -1191,9 +1192,13 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 	if (update_shallow)
 		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
 	if (filter_options.choice) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
+		expand_list_objects_filter_spec(&filter_options,
+						&expanded_filter_spec);
 		set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
-			   filter_options.filter_spec);
+			   expanded_filter_spec.buf);
 		set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
+		strbuf_release(&expanded_filter_spec);
 	}
 	if (negotiation_tip.nr) {
 		if (transport->smart_options)
diff --git a/fetch-pack.c b/fetch-pack.c
index 9691046e64..485632fabe 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -329,9 +329,14 @@ static int find_common(struct fetch_negotiator *negotiator,
 			packet_buf_write(&req_buf, "deepen-not %s", s->string);
 		}
 	}
-	if (server_supports_filtering && args->filter_options.choice)
+	if (server_supports_filtering && args->filter_options.choice) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
+		expand_list_objects_filter_spec(&args->filter_options,
+						&expanded_filter_spec);
 		packet_buf_write(&req_buf, "filter %s",
-				 args->filter_options.filter_spec);
+				 expanded_filter_spec.buf);
+		strbuf_release(&expanded_filter_spec);
+	}
 	packet_buf_flush(&req_buf);
 	state_len = req_buf.len;
 
@@ -1155,9 +1160,13 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 	/* Add filter */
 	if (server_supports_feature("fetch", "filter", 0) &&
 	    args->filter_options.choice) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
 		print_verbose(args, _("Server supports filter"));
+		expand_list_objects_filter_spec(&args->filter_options,
+						&expanded_filter_spec);
 		packet_buf_write(&req_buf, "filter %s",
-				 args->filter_options.filter_spec);
+				 expanded_filter_spec.buf);
+		strbuf_release(&expanded_filter_spec);
 	} else if (args->filter_options.choice) {
 		warning("filtering not recognized by server, ignoring");
 	}
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 5285e7674d..9efb3e9902 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -18,8 +18,9 @@
  * See Documentation/rev-list-options.txt for allowed values for <arg>.
  *
  * Capture the given arg as the "filter_spec".  This can be forwarded to
- * subordinate commands when necessary.  We also "intern" the arg for
- * the convenience of the current command.
+ * subordinate commands when necessary (although it's better to pass it through
+ * expand_list_objects_filter_spec() first).  We also "intern" the arg for the
+ * convenience of the current command.
  */
 static int gently_parse_list_objects_filter(
 	struct list_objects_filter_options *filter_options,
@@ -111,6 +112,21 @@ int opt_parse_list_objects_filter(const struct option *opt,
 	return parse_list_objects_filter(filter_options, arg);
 }
 
+void expand_list_objects_filter_spec(
+	const struct list_objects_filter_options *filter,
+	struct strbuf *expanded_spec)
+{
+	strbuf_init(expanded_spec, strlen(filter->filter_spec));
+	if (filter->choice == LOFC_BLOB_LIMIT)
+		strbuf_addf(expanded_spec, "blob:limit=%lu",
+			    filter->blob_limit_value);
+	else if (filter->choice == LOFC_TREE_DEPTH)
+		strbuf_addf(expanded_spec, "tree:%lu",
+			    filter->tree_exclude_depth);
+	else
+		strbuf_addstr(expanded_spec, filter->filter_spec);
+}
+
 void list_objects_filter_release(
 	struct list_objects_filter_options *filter_options)
 {
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index 477cd97029..e3adc78ebf 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -2,6 +2,7 @@
 #define LIST_OBJECTS_FILTER_OPTIONS_H
 
 #include "parse-options.h"
+#include "strbuf.h"
 
 /*
  * The list of defined filters for list-objects.
@@ -20,8 +21,9 @@ struct list_objects_filter_options {
 	/*
 	 * 'filter_spec' is the raw argument value given on the command line
 	 * or protocol request.  (The part after the "--keyword=".)  For
-	 * commands that launch filtering sub-processes, this value should be
-	 * passed to them as received by the current process.
+	 * commands that launch filtering sub-processes, or for communication
+	 * over the network, don't use this value; use the result of
+	 * expand_list_objects_filter_spec() instead.
 	 */
 	char *filter_spec;
 
@@ -62,6 +64,17 @@ int opt_parse_list_objects_filter(const struct option *opt,
 	  N_("object filtering"), 0, \
 	  opt_parse_list_objects_filter }
 
+/*
+ * Translates abbreviated numbers in the filter's filter_spec into their
+ * fully-expanded forms (e.g., "limit:blob=1k" becomes "limit:blob=1024").
+ *
+ * This form should be used instead of the raw filter_spec field when
+ * communicating with a remote process or subprocess.
+ */
+void expand_list_objects_filter_spec(
+	const struct list_objects_filter_options *filter,
+	struct strbuf *expanded_spec);
+
 void list_objects_filter_release(
 	struct list_objects_filter_options *filter_options);
 
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 18b0b14d5a..f96f551fb5 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -437,4 +437,21 @@ test_expect_success 'rev-list W/ missing=allow-any' '
 	git -C r1 rev-list --quiet --missing=allow-any --objects HEAD
 '
 
+# Test expansion of filter specs.
+
+test_expect_success 'expand blob limit in protocol' '
+	git -C r2 config --local uploadpack.allowfilter 1 &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -c protocol.version=2 clone \
+		--filter=blob:limit=1k "file://$(pwd)/r2" limit &&
+	! grep "blob:limit=1k" trace &&
+	grep "blob:limit=1024" trace
+'
+
+test_expect_success 'expand tree depth limit in protocol' '
+	GIT_TRACE_PACKET="$(pwd)/tree_trace" git -c protocol.version=2 clone \
+		--filter=tree:0k "file://$(pwd)/r2" tree &&
+	! grep "tree:0k" tree_trace &&
+	grep "tree:0" tree_trace
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index bf225c698f..01404bfac5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -679,10 +679,15 @@ static int fetch(struct transport *transport,
 	if (data->transport_options.update_shallow)
 		set_helper_option(transport, "update-shallow", "true");
 
-	if (data->transport_options.filter_options.choice)
-		set_helper_option(
-			transport, "filter",
-			data->transport_options.filter_options.filter_spec);
+	if (data->transport_options.filter_options.choice) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
+		expand_list_objects_filter_spec(
+			&data->transport_options.filter_options,
+			&expanded_filter_spec);
+		set_helper_option(transport, "filter",
+				  expanded_filter_spec.buf);
+		strbuf_release(&expanded_filter_spec);
+	}
 
 	if (data->transport_options.negotiation_tips)
 		warning("Ignoring --negotiation-tip because the protocol does not support it.");
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24..1c6d73e5a2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -140,14 +140,17 @@ static void create_pack_file(const struct object_array *have_obj,
 	if (use_include_tag)
 		argv_array_push(&pack_objects.args, "--include-tag");
 	if (filter_options.filter_spec) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
+		expand_list_objects_filter_spec(&filter_options,
+						&expanded_filter_spec);
 		if (pack_objects.use_shell) {
 			struct strbuf buf = STRBUF_INIT;
-			sq_quote_buf(&buf, filter_options.filter_spec);
+			sq_quote_buf(&buf, expanded_filter_spec.buf);
 			argv_array_pushf(&pack_objects.args, "--filter=%s", buf.buf);
 			strbuf_release(&buf);
 		} else {
 			argv_array_pushf(&pack_objects.args, "--filter=%s",
-					 filter_options.filter_spec);
+					 expanded_filter_spec.buf);
 		}
 	}
 
-- 
2.20.0.405.gbc1bbc6f85-goog

Re: [RFC PATCH 1/1] filter-options: Expand abbreviated numbers

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> When communicating with a remote server or a subprocess, use expanded
> numbers rather than abbreviated numbers in the object filter spec (e.g.
> "limit:blob=1k" becomes "limit:blob=1024").
>
> Update the protocol docs to note that clients should always perform this
> expansion, to allow for more compatibility between server
> implementations.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  Documentation/technical/protocol-v2.txt |  5 ++++-
>  builtin/clone.c                         |  6 +++++-
>  builtin/fetch.c                         |  7 ++++++-
>  fetch-pack.c                            | 15 ++++++++++++---
>  list-objects-filter-options.c           | 20 ++++++++++++++++++--
>  list-objects-filter-options.h           | 17 +++++++++++++++--
>  t/t6112-rev-list-filters-objects.sh     | 17 +++++++++++++++++
>  transport-helper.c                      | 13 +++++++++----
>  upload-pack.c                           |  7 +++++--
>  9 files changed, 91 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
> index 09e4e0273f..d001372404 100644
> --- a/Documentation/technical/protocol-v2.txt
> +++ b/Documentation/technical/protocol-v2.txt
> @@ -296,7 +296,10 @@ included in the client's request:
>  	Request that various objects from the packfile be omitted
>  	using one of several filtering techniques. These are intended
>  	for use with partial clone and partial fetch operations. See
> -	`rev-list` for possible "filter-spec" values.
> +	`rev-list` for possible "filter-spec" values. Clients MUST
> +	translate abbreviated numbers (e.g. "1k") into fully-expanded
> +	numbers (e.g. "1024") on the client side, so that the server
> +	does not need to implement unit parsing.

I suspect that it is too late now to retroactively say "MUST" here.
The best we may be able to do is to say "The sender SHOULD send a
plain integer without unit and the receiver SHOULD be prepared to
scale an integer with unit".

Re: [RFC PATCH 1/1] filter-options: Expand abbreviated numbers

[Josh Steadmon]

On 2019.01.02 15:15, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > When communicating with a remote server or a subprocess, use expanded
> > numbers rather than abbreviated numbers in the object filter spec (e.g.
> > "limit:blob=1k" becomes "limit:blob=1024").
> >
> > Update the protocol docs to note that clients should always perform this
> > expansion, to allow for more compatibility between server
> > implementations.
> >
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> >  Documentation/technical/protocol-v2.txt |  5 ++++-
> >  builtin/clone.c                         |  6 +++++-
> >  builtin/fetch.c                         |  7 ++++++-
> >  fetch-pack.c                            | 15 ++++++++++++---
> >  list-objects-filter-options.c           | 20 ++++++++++++++++++--
> >  list-objects-filter-options.h           | 17 +++++++++++++++--
> >  t/t6112-rev-list-filters-objects.sh     | 17 +++++++++++++++++
> >  transport-helper.c                      | 13 +++++++++----
> >  upload-pack.c                           |  7 +++++--
> >  9 files changed, 91 insertions(+), 16 deletions(-)
> >
> > diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
> > index 09e4e0273f..d001372404 100644
> > --- a/Documentation/technical/protocol-v2.txt
> > +++ b/Documentation/technical/protocol-v2.txt
> > @@ -296,7 +296,10 @@ included in the client's request:
> >  	Request that various objects from the packfile be omitted
> >  	using one of several filtering techniques. These are intended
> >  	for use with partial clone and partial fetch operations. See
> > -	`rev-list` for possible "filter-spec" values.
> > +	`rev-list` for possible "filter-spec" values. Clients MUST
> > +	translate abbreviated numbers (e.g. "1k") into fully-expanded
> > +	numbers (e.g. "1024") on the client side, so that the server
> > +	does not need to implement unit parsing.
> 
> I suspect that it is too late now to retroactively say "MUST" here.
> The best we may be able to do is to say "The sender SHOULD send a
> plain integer without unit and the receiver SHOULD be prepared to
> scale an integer with unit".
> 

In that case, do you think we should also specify that units should be
interpreted as powers-of-2 rather than powers-of-10?

Re: [RFC PATCH 1/1] filter-options: Expand abbreviated numbers

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

>> > -	`rev-list` for possible "filter-spec" values.
>> > +	`rev-list` for possible "filter-spec" values. Clients MUST
>> > +	translate abbreviated numbers (e.g. "1k") into fully-expanded
>> > +	numbers (e.g. "1024") on the client side, so that the server
>> > +	does not need to implement unit parsing.
>> 
>> I suspect that it is too late now to retroactively say "MUST" here.
>> The best we may be able to do is to say "The sender SHOULD send a
>> plain integer without unit and the receiver SHOULD be prepared to
>> scale an integer with unit".
>> 
>
> In that case, do you think we should also specify that units should be
> interpreted as powers-of-2 rather than powers-of-10?

If we do not document the number scaling elsewhere, then we
certainly should, but I somehow doubt it.

Documentation/git-config.txt does list these explicitly where it
talks about --type=int, but these human-readable scaled integers are
also used in command line arguments as well, so we may want to find
a central place that is clear to the readers that the description
applies to all of them and move the description there.

Re: [RFC PATCH 1/1] filter-options: Expand abbreviated numbers

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Josh Steadmon <steadmon@google.com> writes:
>
>>> > -	`rev-list` for possible "filter-spec" values.
>>> > +	`rev-list` for possible "filter-spec" values. Clients MUST
>>> > +	translate abbreviated numbers (e.g. "1k") into fully-expanded
>>> > +	numbers (e.g. "1024") on the client side, so that the server
>>> > +	does not need to implement unit parsing.
>>> 
>>> I suspect that it is too late now to retroactively say "MUST" here.
>>> The best we may be able to do is to say "The sender SHOULD send a
>>> plain integer without unit and the receiver SHOULD be prepared to
>>> scale an integer with unit".
>>> 
>>
>> In that case, do you think we should also specify that units should be
>> interpreted as powers-of-2 rather than powers-of-10?
>
> If we do not document the number scaling elsewhere, then we
> certainly should, but I somehow doubt it.
>
> Documentation/git-config.txt does list these explicitly where it
> talks about --type=int, but these human-readable scaled integers are
> also used in command line arguments as well, so we may want to find
> a central place that is clear to the readers that the description
> applies to all of them and move the description there.

Another thing.  We may probably end up adding more scaling factors,
but going forward we would not want to require any and all Git
reimplementations to adopt them in lock-step, so perhaps

    `rev-list` for possible "filter-spec" values.  Senders SHOULD
    translate a scaled integer (e.g. "1k") into a full expanded form
    (e.g. "1024") so that an older receiver that does not know newly
    invented scaling units can still interoperate with it, but
    receivers SHOULD accept the following scalilng units: 'k', 'm'
    and 'g' for 1024, 1048576 and 1073741824 respectively.

or something like that.

[PATCH v2 0/1] Expand abbreviated filters

[Josh Steadmon]

NOTE: this patch is based on top of md/list-objects-filter-by-depth

Currently, git clients pass filter specs verbatim over the network and
to subprocesses. We support various scaling suffixes for parameters on
these limits (via git_parse_ulong()), but other implementations may not
support the same suffixes. It would be better to only pass fully-
expanded numbers in this case, and keep the expansion logic completely
on the client side.

This patch updates the protocol-v2 doc to specify that clients SHOULD
expand abbreviations in filter specifications before passing them to
other processes. It adds a new function
"expand_list_objects_filter_spec()" in list-objects-filter-options.c
that implements the expansion logic, and updates users of the
filter_spec field to instead expand the spec first.

Changes since V1:
  * Changed "MUST" to "SHOULD" in protocol-v2.txt
  * Noted specific suffixes that receivers SHOULD accept

Josh Steadmon (1):
  filter-options: Expand abbreviated numbers

 Documentation/technical/protocol-v2.txt |  8 +++++++-
 builtin/clone.c                         |  6 +++++-
 builtin/fetch.c                         |  7 ++++++-
 fetch-pack.c                            | 15 ++++++++++++---
 list-objects-filter-options.c           | 20 ++++++++++++++++++--
 list-objects-filter-options.h           | 17 +++++++++++++++--
 t/t6112-rev-list-filters-objects.sh     | 17 +++++++++++++++++
 transport-helper.c                      | 13 +++++++++----
 upload-pack.c                           |  7 +++++--
 9 files changed, 94 insertions(+), 16 deletions(-)

Range-diff against v1:
1:  d324e78369 ! 1:  d35827de35 filter-options: Expand abbreviated numbers
    @@ -20,10 +20,13 @@
      	using one of several filtering techniques. These are intended
      	for use with partial clone and partial fetch operations. See
     -	`rev-list` for possible "filter-spec" values.
    -+	`rev-list` for possible "filter-spec" values. Clients MUST
    -+	translate abbreviated numbers (e.g. "1k") into fully-expanded
    -+	numbers (e.g. "1024") on the client side, so that the server
    -+	does not need to implement unit parsing.
    ++	`rev-list` for possible "filter-spec" values. When communicating
    ++	with other processes, senders SHOULD translate scaled integers
    ++	(e.g. "1k") into a fully-expanded form (e.g. "1024") to aid
    ++	interoperability with older receivers that may not understand
    ++	newly-invented scaling suffixes. However, receivers SHOULD
    ++	accept the following suffixes: 'k', 'm', and 'g' for 1024,
    ++	1048576, and 1073741824, respectively.
      
      If the 'ref-in-want' feature is advertised, the following argument can
      be included in the client's request as well as the potential addition of
-- 
2.20.1.97.g81188d93c3-goog

[PATCH v2 1/1] filter-options: Expand abbreviated numbers

[Josh Steadmon]

When communicating with a remote server or a subprocess, use expanded
numbers rather than abbreviated numbers in the object filter spec (e.g.
"limit:blob=1k" becomes "limit:blob=1024").

Update the protocol docs to note that clients should always perform this
expansion, to allow for more compatibility between server
implementations.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/technical/protocol-v2.txt |  8 +++++++-
 builtin/clone.c                         |  6 +++++-
 builtin/fetch.c                         |  7 ++++++-
 fetch-pack.c                            | 15 ++++++++++++---
 list-objects-filter-options.c           | 20 ++++++++++++++++++--
 list-objects-filter-options.h           | 17 +++++++++++++++--
 t/t6112-rev-list-filters-objects.sh     | 17 +++++++++++++++++
 transport-helper.c                      | 13 +++++++++----
 upload-pack.c                           |  7 +++++--
 9 files changed, 94 insertions(+), 16 deletions(-)

diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 09e4e0273f..292060a9dc 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -296,7 +296,13 @@ included in the client's request:
 	Request that various objects from the packfile be omitted
 	using one of several filtering techniques. These are intended
 	for use with partial clone and partial fetch operations. See
-	`rev-list` for possible "filter-spec" values.
+	`rev-list` for possible "filter-spec" values. When communicating
+	with other processes, senders SHOULD translate scaled integers
+	(e.g. "1k") into a fully-expanded form (e.g. "1024") to aid
+	interoperability with older receivers that may not understand
+	newly-invented scaling suffixes. However, receivers SHOULD
+	accept the following suffixes: 'k', 'm', and 'g' for 1024,
+	1048576, and 1073741824, respectively.
 
 If the 'ref-in-want' feature is advertised, the following argument can
 be included in the client's request as well as the potential addition of
diff --git a/builtin/clone.c b/builtin/clone.c
index 15b142d646..8e05e8ad6c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1130,9 +1130,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 				     option_upload_pack);
 
 	if (filter_options.choice) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
+		expand_list_objects_filter_spec(&filter_options,
+						&expanded_filter_spec);
 		transport_set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
-				     filter_options.filter_spec);
+				     expanded_filter_spec.buf);
 		transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
+		strbuf_release(&expanded_filter_spec);
 	}
 
 	if (transport->smart_options && !deepen && !filter_options.choice)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e0140327aa..8b8bb64921 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1172,6 +1172,7 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
 static struct transport *prepare_transport(struct remote *remote, int deepen)
 {
 	struct transport *transport;
+
 	transport = transport_get(remote, NULL);
 	transport_set_verbosity(transport, verbosity, progress);
 	transport->family = family;
@@ -1191,9 +1192,13 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 	if (update_shallow)
 		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
 	if (filter_options.choice) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
+		expand_list_objects_filter_spec(&filter_options,
+						&expanded_filter_spec);
 		set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
-			   filter_options.filter_spec);
+			   expanded_filter_spec.buf);
 		set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
+		strbuf_release(&expanded_filter_spec);
 	}
 	if (negotiation_tip.nr) {
 		if (transport->smart_options)
diff --git a/fetch-pack.c b/fetch-pack.c
index 9691046e64..485632fabe 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -329,9 +329,14 @@ static int find_common(struct fetch_negotiator *negotiator,
 			packet_buf_write(&req_buf, "deepen-not %s", s->string);
 		}
 	}
-	if (server_supports_filtering && args->filter_options.choice)
+	if (server_supports_filtering && args->filter_options.choice) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
+		expand_list_objects_filter_spec(&args->filter_options,
+						&expanded_filter_spec);
 		packet_buf_write(&req_buf, "filter %s",
-				 args->filter_options.filter_spec);
+				 expanded_filter_spec.buf);
+		strbuf_release(&expanded_filter_spec);
+	}
 	packet_buf_flush(&req_buf);
 	state_len = req_buf.len;
 
@@ -1155,9 +1160,13 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 	/* Add filter */
 	if (server_supports_feature("fetch", "filter", 0) &&
 	    args->filter_options.choice) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
 		print_verbose(args, _("Server supports filter"));
+		expand_list_objects_filter_spec(&args->filter_options,
+						&expanded_filter_spec);
 		packet_buf_write(&req_buf, "filter %s",
-				 args->filter_options.filter_spec);
+				 expanded_filter_spec.buf);
+		strbuf_release(&expanded_filter_spec);
 	} else if (args->filter_options.choice) {
 		warning("filtering not recognized by server, ignoring");
 	}
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 5285e7674d..9efb3e9902 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -18,8 +18,9 @@
  * See Documentation/rev-list-options.txt for allowed values for <arg>.
  *
  * Capture the given arg as the "filter_spec".  This can be forwarded to
- * subordinate commands when necessary.  We also "intern" the arg for
- * the convenience of the current command.
+ * subordinate commands when necessary (although it's better to pass it through
+ * expand_list_objects_filter_spec() first).  We also "intern" the arg for the
+ * convenience of the current command.
  */
 static int gently_parse_list_objects_filter(
 	struct list_objects_filter_options *filter_options,
@@ -111,6 +112,21 @@ int opt_parse_list_objects_filter(const struct option *opt,
 	return parse_list_objects_filter(filter_options, arg);
 }
 
+void expand_list_objects_filter_spec(
+	const struct list_objects_filter_options *filter,
+	struct strbuf *expanded_spec)
+{
+	strbuf_init(expanded_spec, strlen(filter->filter_spec));
+	if (filter->choice == LOFC_BLOB_LIMIT)
+		strbuf_addf(expanded_spec, "blob:limit=%lu",
+			    filter->blob_limit_value);
+	else if (filter->choice == LOFC_TREE_DEPTH)
+		strbuf_addf(expanded_spec, "tree:%lu",
+			    filter->tree_exclude_depth);
+	else
+		strbuf_addstr(expanded_spec, filter->filter_spec);
+}
+
 void list_objects_filter_release(
 	struct list_objects_filter_options *filter_options)
 {
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index 477cd97029..e3adc78ebf 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -2,6 +2,7 @@
 #define LIST_OBJECTS_FILTER_OPTIONS_H
 
 #include "parse-options.h"
+#include "strbuf.h"
 
 /*
  * The list of defined filters for list-objects.
@@ -20,8 +21,9 @@ struct list_objects_filter_options {
 	/*
 	 * 'filter_spec' is the raw argument value given on the command line
 	 * or protocol request.  (The part after the "--keyword=".)  For
-	 * commands that launch filtering sub-processes, this value should be
-	 * passed to them as received by the current process.
+	 * commands that launch filtering sub-processes, or for communication
+	 * over the network, don't use this value; use the result of
+	 * expand_list_objects_filter_spec() instead.
 	 */
 	char *filter_spec;
 
@@ -62,6 +64,17 @@ int opt_parse_list_objects_filter(const struct option *opt,
 	  N_("object filtering"), 0, \
 	  opt_parse_list_objects_filter }
 
+/*
+ * Translates abbreviated numbers in the filter's filter_spec into their
+ * fully-expanded forms (e.g., "limit:blob=1k" becomes "limit:blob=1024").
+ *
+ * This form should be used instead of the raw filter_spec field when
+ * communicating with a remote process or subprocess.
+ */
+void expand_list_objects_filter_spec(
+	const struct list_objects_filter_options *filter,
+	struct strbuf *expanded_spec);
+
 void list_objects_filter_release(
 	struct list_objects_filter_options *filter_options);
 
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 18b0b14d5a..f96f551fb5 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -437,4 +437,21 @@ test_expect_success 'rev-list W/ missing=allow-any' '
 	git -C r1 rev-list --quiet --missing=allow-any --objects HEAD
 '
 
+# Test expansion of filter specs.
+
+test_expect_success 'expand blob limit in protocol' '
+	git -C r2 config --local uploadpack.allowfilter 1 &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -c protocol.version=2 clone \
+		--filter=blob:limit=1k "file://$(pwd)/r2" limit &&
+	! grep "blob:limit=1k" trace &&
+	grep "blob:limit=1024" trace
+'
+
+test_expect_success 'expand tree depth limit in protocol' '
+	GIT_TRACE_PACKET="$(pwd)/tree_trace" git -c protocol.version=2 clone \
+		--filter=tree:0k "file://$(pwd)/r2" tree &&
+	! grep "tree:0k" tree_trace &&
+	grep "tree:0" tree_trace
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index bf225c698f..01404bfac5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -679,10 +679,15 @@ static int fetch(struct transport *transport,
 	if (data->transport_options.update_shallow)
 		set_helper_option(transport, "update-shallow", "true");
 
-	if (data->transport_options.filter_options.choice)
-		set_helper_option(
-			transport, "filter",
-			data->transport_options.filter_options.filter_spec);
+	if (data->transport_options.filter_options.choice) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
+		expand_list_objects_filter_spec(
+			&data->transport_options.filter_options,
+			&expanded_filter_spec);
+		set_helper_option(transport, "filter",
+				  expanded_filter_spec.buf);
+		strbuf_release(&expanded_filter_spec);
+	}
 
 	if (data->transport_options.negotiation_tips)
 		warning("Ignoring --negotiation-tip because the protocol does not support it.");
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24..1c6d73e5a2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -140,14 +140,17 @@ static void create_pack_file(const struct object_array *have_obj,
 	if (use_include_tag)
 		argv_array_push(&pack_objects.args, "--include-tag");
 	if (filter_options.filter_spec) {
+		struct strbuf expanded_filter_spec = STRBUF_INIT;
+		expand_list_objects_filter_spec(&filter_options,
+						&expanded_filter_spec);
 		if (pack_objects.use_shell) {
 			struct strbuf buf = STRBUF_INIT;
-			sq_quote_buf(&buf, filter_options.filter_spec);
+			sq_quote_buf(&buf, expanded_filter_spec.buf);
 			argv_array_pushf(&pack_objects.args, "--filter=%s", buf.buf);
 			strbuf_release(&buf);
 		} else {
 			argv_array_pushf(&pack_objects.args, "--filter=%s",
-					 filter_options.filter_spec);
+					 expanded_filter_spec.buf);
 		}
 	}
 
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH v2 1/1] filter-options: Expand abbreviated numbers

[SZEDER Gábor]

On Mon, Jan 07, 2019 at 04:17:09PM -0800, Josh Steadmon wrote:
> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index 5285e7674d..9efb3e9902 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c

> @@ -111,6 +112,21 @@ int opt_parse_list_objects_filter(const struct option *opt,
>  	return parse_list_objects_filter(filter_options, arg);
>  }
>  
> +void expand_list_objects_filter_spec(
> +	const struct list_objects_filter_options *filter,
> +	struct strbuf *expanded_spec)
> +{
> +	strbuf_init(expanded_spec, strlen(filter->filter_spec));
> +	if (filter->choice == LOFC_BLOB_LIMIT)
> +		strbuf_addf(expanded_spec, "blob:limit=%lu",
> +			    filter->blob_limit_value);
> +	else if (filter->choice == LOFC_TREE_DEPTH)
> +		strbuf_addf(expanded_spec, "tree:%lu",
> +			    filter->tree_exclude_depth);
> +	else
> +		strbuf_addstr(expanded_spec, filter->filter_spec);
> +}
> +

All compilers error out with something like this:

  list-objects-filter-options.c: In function
  ‘expand_list_objects_filter_spec’:
  list-objects-filter-options.c:124:29: error: ‘LOFC_TREE_DEPTH’ undeclared (first use in this function); did you mean ‘LOFC_TREE_NONE’?
    else if (filter->choice == LOFC_TREE_DEPTH)
                               ^~~~~~~~~~~~~~~
                               LOFC_TREE_NONE
  list-objects-filter-options.c:124:29: note: each undeclared identifier is reported only once for each function it appears in
  list-objects-filter-options.c:126:14: error: ‘const struct list_objects_filter_options’ has no member named ‘tree_exclude_depth’
          filter->tree_exclude_depth);
                ^~
  make: *** [list-objects-filter-options.o] Error 1

Re: [PATCH v2 1/1] filter-options: Expand abbreviated numbers

[Josh Steadmon]

On 2019.01.09 13:23, SZEDER Gábor wrote:
> On Mon, Jan 07, 2019 at 04:17:09PM -0800, Josh Steadmon wrote:
> > diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> > index 5285e7674d..9efb3e9902 100644
> > --- a/list-objects-filter-options.c
> > +++ b/list-objects-filter-options.c
> 
> > @@ -111,6 +112,21 @@ int opt_parse_list_objects_filter(const struct option *opt,
> >  	return parse_list_objects_filter(filter_options, arg);
> >  }
> >  
> > +void expand_list_objects_filter_spec(
> > +	const struct list_objects_filter_options *filter,
> > +	struct strbuf *expanded_spec)
> > +{
> > +	strbuf_init(expanded_spec, strlen(filter->filter_spec));
> > +	if (filter->choice == LOFC_BLOB_LIMIT)
> > +		strbuf_addf(expanded_spec, "blob:limit=%lu",
> > +			    filter->blob_limit_value);
> > +	else if (filter->choice == LOFC_TREE_DEPTH)
> > +		strbuf_addf(expanded_spec, "tree:%lu",
> > +			    filter->tree_exclude_depth);
> > +	else
> > +		strbuf_addstr(expanded_spec, filter->filter_spec);
> > +}
> > +
> 
> All compilers error out with something like this:
> 
>   list-objects-filter-options.c: In function
>   ‘expand_list_objects_filter_spec’:
>   list-objects-filter-options.c:124:29: error: ‘LOFC_TREE_DEPTH’ undeclared (first use in this function); did you mean ‘LOFC_TREE_NONE’?
>     else if (filter->choice == LOFC_TREE_DEPTH)
>                                ^~~~~~~~~~~~~~~
>                                LOFC_TREE_NONE
>   list-objects-filter-options.c:124:29: note: each undeclared identifier is reported only once for each function it appears in
>   list-objects-filter-options.c:126:14: error: ‘const struct list_objects_filter_options’ has no member named ‘tree_exclude_depth’
>           filter->tree_exclude_depth);
>                 ^~
>   make: *** [list-objects-filter-options.o] Error 1
> 

Hmm, looks like you may not have applied this on top of
md/list-objects-filter-by-depth? However, the most recent version of
that branch has its own compilation errors at the moment.

Re: [PATCH v2 1/1] filter-options: Expand abbreviated numbers

[SZEDER Gábor]

On Wed, Jan 09, 2019 at 10:55:43AM -0800, Josh Steadmon wrote:
> On 2019.01.09 13:23, SZEDER Gábor wrote:
> > On Mon, Jan 07, 2019 at 04:17:09PM -0800, Josh Steadmon wrote:
> > > diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> > > index 5285e7674d..9efb3e9902 100644
> > > --- a/list-objects-filter-options.c
> > > +++ b/list-objects-filter-options.c
> > 
> > > @@ -111,6 +112,21 @@ int opt_parse_list_objects_filter(const struct option *opt,
> > >  	return parse_list_objects_filter(filter_options, arg);
> > >  }
> > >  
> > > +void expand_list_objects_filter_spec(
> > > +	const struct list_objects_filter_options *filter,
> > > +	struct strbuf *expanded_spec)
> > > +{
> > > +	strbuf_init(expanded_spec, strlen(filter->filter_spec));
> > > +	if (filter->choice == LOFC_BLOB_LIMIT)
> > > +		strbuf_addf(expanded_spec, "blob:limit=%lu",
> > > +			    filter->blob_limit_value);
> > > +	else if (filter->choice == LOFC_TREE_DEPTH)
> > > +		strbuf_addf(expanded_spec, "tree:%lu",
> > > +			    filter->tree_exclude_depth);
> > > +	else
> > > +		strbuf_addstr(expanded_spec, filter->filter_spec);
> > > +}
> > > +
> > 
> > All compilers error out with something like this:
> > 
> >   list-objects-filter-options.c: In function
> >   ‘expand_list_objects_filter_spec’:
> >   list-objects-filter-options.c:124:29: error: ‘LOFC_TREE_DEPTH’ undeclared (first use in this function); did you mean ‘LOFC_TREE_NONE’?
> >     else if (filter->choice == LOFC_TREE_DEPTH)
> >                                ^~~~~~~~~~~~~~~
> >                                LOFC_TREE_NONE
> >   list-objects-filter-options.c:124:29: note: each undeclared identifier is reported only once for each function it appears in
> >   list-objects-filter-options.c:126:14: error: ‘const struct list_objects_filter_options’ has no member named ‘tree_exclude_depth’
> >           filter->tree_exclude_depth);
> >                 ^~
> >   make: *** [list-objects-filter-options.o] Error 1
> > 
> 
> Hmm, looks like you may not have applied this on top of
> md/list-objects-filter-by-depth? However, the most recent version of
> that branch has its own compilation errors at the moment.

Ah, OK.  I didn't actually apply this patch anywhere, I just tried to
test this topic as it is in 'pu', where it branches off from current
'master'.