[v2,1/1] filter-options: Expand abbreviated numbers
diff mbox series

Message ID d35827de35d2a158cd5325569eaaf355563bf028.1546906008.git.steadmon@google.com
State New
Headers show
Series
  • Expand abbreviated filters
Related show

Commit Message

Josh Steadmon Jan. 8, 2019, 12:17 a.m. UTC
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(-)

Comments

SZEDER Gábor Jan. 9, 2019, 12:23 p.m. UTC | #1
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
Josh Steadmon Jan. 9, 2019, 6:55 p.m. UTC | #2
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.
SZEDER Gábor Jan. 9, 2019, 7:20 p.m. UTC | #3
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'.

Patch
diff mbox series

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);
 		}
 	}