Re: [PATCH 07/13] list-objects-filter-options: common argument parsing

[Jeff Hostetler]

On 10/25/2017 12:14 AM, Jonathan Tan wrote:

On Tue, Oct 24, 2017 at 11:53 AM, Jeff Hostetler  wrote:

+ *  ::= blob:none
+ *   blob:limit:[kmg]
+ *   sparse:oid:
+ *   sparse:path:


I notice in the code below that there are some usages of "=" instead
of ":" - could you clarify which one it is? (Ideally this would point
to one point of documentation which serves as both user and technical
documentation.)


good catch.  thanks!
 

+ */
+int parse_list_objects_filter(struct list_objects_filter_options 
*filter_options,
+ const char *arg)
+{
+   struct object_context oc;
+   struct object_id sparse_oid;
+   const char *v0;
+   const char *v1;
+
+   if (filter_options->choice)
+   die(_("multiple object filter types cannot be combined"));
+
+   /*
+* TODO consider rejecting 'arg' if it contains any
+* TODO injection characters (since we might send this
+* TODO to a sub-command or to the server and we don't
+* TODO want to deal with legacy quoting/escaping for
+* TODO a new feature).
+*/
+
+   filter_options->raw_value = strdup(arg);
+
+   if (skip_prefix(arg, "blob:", ) || skip_prefix(arg, "blobs:", )) {


I know that some people prefer leniency, but I think it's better to
standardize on one form ("blob" instead of both "blob" and "blobs").


I could go either way on this.  (I kept mistyping it during interactive testing,
so I added both cases...)




+   if (!strcmp(v0, "none")) {
+   filter_options->choice = LOFC_BLOB_NONE;
+   return 0;
+   }
+
+   if (skip_prefix(v0, "limit=", ) &&
+   git_parse_ulong(v1, _options->blob_limit_value)) {
+   filter_options->choice = LOFC_BLOB_LIMIT;
+   return 0;
+   }
+   }
+   else if (skip_prefix(arg, "sparse:", )) {
+   if (skip_prefix(v0, "oid=", )) {
+   filter_options->choice = LOFC_SPARSE_OID;
+   if (!get_oid_with_context(v1, GET_OID_BLOB,
+ _oid, )) {
+   /*
+* We successfully converted the 
+* into an actual OID.  Rewrite the raw_value
+* in canonoical form with just the OID.
+* (If we send this request to the server, we
+* want an absolute expression rather than a
+* local-ref-relative expression.)
+*/
+   free((char *)filter_options->raw_value);
+   filter_options->raw_value =
+   xstrfmt("sparse:oid=%s",
+   oid_to_hex(_oid));
+   filter_options->sparse_oid_value =
+   oiddup(_oid);
+   } else {
+   /*
+* We could not turn the  into an
+* OID.  Leave the raw_value as is in case
+* the server can parse it.  (It may refer to
+* a branch, commit, or blob we don't have.)
+*/
+   }
+   return 0;
+   }
+
+   if (skip_prefix(v0, "path=", )) {
+   filter_options->choice = LOFC_SPARSE_PATH;
+   filter_options->sparse_path_value = strdup(v1);
+   return 0;
+   }
+   }
+
+   die(_("invalid filter expression '%s'"), arg);
+   return 0;
+}
+
+int opt_parse_list_objects_filter(const struct option *opt,
+ const char *arg, int unset)
+{
+   struct list_objects_filter_options *filter_options = opt->value;
+
+   assert(arg);
+   assert(!unset);
+
+   return parse_list_objects_filter(filter_options, arg);
+}
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
new file mode 100644
index 000..23bd68e
--- /dev/null
+++ b/list-objects-filter-options.h
@@ -0,0 +1,50 @@
+#ifndef LIST_OBJECTS_FILTER_OPTIONS_H
+#define LIST_OBJECTS_FILTER_OPTIONS_H
+
+#include "parse-options.h"
+
+/*
+ * Common declarations and utilities for filtering objects (such as omitting
+ * large blobs) in list_objects:traverse_commit_list() and git-rev-list.
+ */
+
+enum list_objects_filter_choice {
+   LOFC_DISABLED = 0,
+   LOFC_BLOB_NONE,
+   LOFC_BLOB_LIMIT,
+   LOFC_SPARSE_OID,
+   LOFC_SPARSE_PATH,
+};
+
+struct list_objects_filter_options {
+   /*
+* The raw argument value given on the 

Re: [PATCH 07/13] list-objects-filter-options: common argument parsing

[Jonathan Tan]

On Tue, Oct 24, 2017 at 11:53 AM, Jeff Hostetler  wrote:
> + *  ::= blob:none
> + *   blob:limit:[kmg]
> + *   sparse:oid:
> + *   sparse:path:

I notice in the code below that there are some usages of "=" instead
of ":" - could you clarify which one it is? (Ideally this would point
to one point of documentation which serves as both user and technical
documentation.)

> + */
> +int parse_list_objects_filter(struct list_objects_filter_options 
> *filter_options,
> + const char *arg)
> +{
> +   struct object_context oc;
> +   struct object_id sparse_oid;
> +   const char *v0;
> +   const char *v1;
> +
> +   if (filter_options->choice)
> +   die(_("multiple object filter types cannot be combined"));
> +
> +   /*
> +* TODO consider rejecting 'arg' if it contains any
> +* TODO injection characters (since we might send this
> +* TODO to a sub-command or to the server and we don't
> +* TODO want to deal with legacy quoting/escaping for
> +* TODO a new feature).
> +*/
> +
> +   filter_options->raw_value = strdup(arg);
> +
> +   if (skip_prefix(arg, "blob:", ) || skip_prefix(arg, "blobs:", 
> )) {

I know that some people prefer leniency, but I think it's better to
standardize on one form ("blob" instead of both "blob" and "blobs").

> +   if (!strcmp(v0, "none")) {
> +   filter_options->choice = LOFC_BLOB_NONE;
> +   return 0;
> +   }
> +
> +   if (skip_prefix(v0, "limit=", ) &&
> +   git_parse_ulong(v1, _options->blob_limit_value)) {
> +   filter_options->choice = LOFC_BLOB_LIMIT;
> +   return 0;
> +   }
> +   }
> +   else if (skip_prefix(arg, "sparse:", )) {
> +   if (skip_prefix(v0, "oid=", )) {
> +   filter_options->choice = LOFC_SPARSE_OID;
> +   if (!get_oid_with_context(v1, GET_OID_BLOB,
> + _oid, )) {
> +   /*
> +* We successfully converted the 
> +* into an actual OID.  Rewrite the raw_value
> +* in canonoical form with just the OID.
> +* (If we send this request to the server, we
> +* want an absolute expression rather than a
> +* local-ref-relative expression.)
> +*/
> +   free((char *)filter_options->raw_value);
> +   filter_options->raw_value =
> +   xstrfmt("sparse:oid=%s",
> +   oid_to_hex(_oid));
> +   filter_options->sparse_oid_value =
> +   oiddup(_oid);
> +   } else {
> +   /*
> +* We could not turn the  into an
> +* OID.  Leave the raw_value as is in case
> +* the server can parse it.  (It may refer to
> +* a branch, commit, or blob we don't have.)
> +*/
> +   }
> +   return 0;
> +   }
> +
> +   if (skip_prefix(v0, "path=", )) {
> +   filter_options->choice = LOFC_SPARSE_PATH;
> +   filter_options->sparse_path_value = strdup(v1);
> +   return 0;
> +   }
> +   }
> +
> +   die(_("invalid filter expression '%s'"), arg);
> +   return 0;
> +}
> +
> +int opt_parse_list_objects_filter(const struct option *opt,
> + const char *arg, int unset)
> +{
> +   struct list_objects_filter_options *filter_options = opt->value;
> +
> +   assert(arg);
> +   assert(!unset);
> +
> +   return parse_list_objects_filter(filter_options, arg);
> +}
> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
> new file mode 100644
> index 000..23bd68e
> --- /dev/null
> +++ b/list-objects-filter-options.h
> @@ -0,0 +1,50 @@
> +#ifndef LIST_OBJECTS_FILTER_OPTIONS_H
> +#define LIST_OBJECTS_FILTER_OPTIONS_H
> +
> +#include "parse-options.h"
> +
> +/*
> + * Common declarations and utilities for filtering objects (such as omitting
> + * large blobs) in list_objects:traverse_commit_list() and git-rev-list.
> + */
> +
> +enum list_objects_filter_choice {
> +   LOFC_DISABLED = 0,
> +   LOFC_BLOB_NONE,
> +   LOFC_BLOB_LIMIT,
> +   LOFC_SPARSE_OID,
> +   LOFC_SPARSE_PATH,
> +};
> +
> +struct list_objects_filter_options {
> +   /*
> +

[PATCH 07/13] list-objects-filter-options: common argument parsing

[Jeff Hostetler]

From: Jeff Hostetler 

Create common routines and defines for parsing
list-objects-filter-related command line arguments and
pack-protocol fields.

Signed-off-by: Jeff Hostetler 
---
 Makefile  |   1 +
 list-objects-filter-options.c | 101 ++
 list-objects-filter-options.h |  50 +
 3 files changed, 152 insertions(+)
 create mode 100644 list-objects-filter-options.c
 create mode 100644 list-objects-filter-options.h

diff --git a/Makefile b/Makefile
index fc82664..b9ff0b4 100644
--- a/Makefile
+++ b/Makefile
@@ -810,6 +810,7 @@ LIB_OBJS += list-objects.o
 LIB_OBJS += list-objects-filter-blobs-limit.o
 LIB_OBJS += list-objects-filter-blobs-none.o
 LIB_OBJS += list-objects-filter-map.o
+LIB_OBJS += list-objects-filter-options.o
 LIB_OBJS += list-objects-filter-sparse.o
 LIB_OBJS += ll-merge.o
 LIB_OBJS += lockfile.o
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
new file mode 100644
index 000..40f48ac
--- /dev/null
+++ b/list-objects-filter-options.c
@@ -0,0 +1,101 @@
+#include "cache.h"
+#include "commit.h"
+#include "config.h"
+#include "revision.h"
+#include "list-objects.h"
+#include "list-objects-filter-options.h"
+
+/*
+ * Parse value of the argument to the "filter" keword.
+ * On the command line this looks like: --filter=
+ * and in the pack protocol as: filter 
+ *
+ *  ::= blob:none
+ *   blob:limit:[kmg]
+ *   sparse:oid:
+ *   sparse:path:
+ */
+int parse_list_objects_filter(struct list_objects_filter_options 
*filter_options,
+ const char *arg)
+{
+   struct object_context oc;
+   struct object_id sparse_oid;
+   const char *v0;
+   const char *v1;
+
+   if (filter_options->choice)
+   die(_("multiple object filter types cannot be combined"));
+
+   /*
+* TODO consider rejecting 'arg' if it contains any
+* TODO injection characters (since we might send this
+* TODO to a sub-command or to the server and we don't
+* TODO want to deal with legacy quoting/escaping for
+* TODO a new feature).
+*/
+
+   filter_options->raw_value = strdup(arg);
+
+   if (skip_prefix(arg, "blob:", ) || skip_prefix(arg, "blobs:", )) {
+   if (!strcmp(v0, "none")) {
+   filter_options->choice = LOFC_BLOB_NONE;
+   return 0;
+   }
+
+   if (skip_prefix(v0, "limit=", ) &&
+   git_parse_ulong(v1, _options->blob_limit_value)) {
+   filter_options->choice = LOFC_BLOB_LIMIT;
+   return 0;
+   }
+   }
+   else if (skip_prefix(arg, "sparse:", )) {
+   if (skip_prefix(v0, "oid=", )) {
+   filter_options->choice = LOFC_SPARSE_OID;
+   if (!get_oid_with_context(v1, GET_OID_BLOB,
+ _oid, )) {
+   /*
+* We successfully converted the 
+* into an actual OID.  Rewrite the raw_value
+* in canonoical form with just the OID.
+* (If we send this request to the server, we
+* want an absolute expression rather than a
+* local-ref-relative expression.)
+*/
+   free((char *)filter_options->raw_value);
+   filter_options->raw_value =
+   xstrfmt("sparse:oid=%s",
+   oid_to_hex(_oid));
+   filter_options->sparse_oid_value =
+   oiddup(_oid);
+   } else {
+   /*
+* We could not turn the  into an
+* OID.  Leave the raw_value as is in case
+* the server can parse it.  (It may refer to
+* a branch, commit, or blob we don't have.)
+*/
+   }
+   return 0;
+   }
+
+   if (skip_prefix(v0, "path=", )) {
+   filter_options->choice = LOFC_SPARSE_PATH;
+   filter_options->sparse_path_value = strdup(v1);
+   return 0;
+   }
+   }
+
+   die(_("invalid filter expression '%s'"), arg);
+   return 0;
+}
+
+int opt_parse_list_objects_filter(const struct option *opt,
+ const char *arg, int unset)
+{
+   struct list_objects_filter_options *filter_options = opt->value;
+
+   assert(arg);
+