Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list

On 11/2/2017 3:32 PM, Jonathan Tan wrote:

On Thu,  2 Nov 2017 17:50:11 +0000
Jeff Hostetler [off-list ref] wrote:

quoted

+		if (skip_prefix(v0, "oid=", &v1)) {
+			filter_options->choice = LOFC_SPARSE_OID;
+			if (!get_oid_with_context(v1, GET_OID_BLOB,
+						  &sparse_oid, &oc)) {
+				/*
+				 * We successfully converted the <oid-expr>
+				 * 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.)
+				 */

I think this would lead to confusing behavior - for example, a fetch
with "--filter=oid=mybranch:sparseconfig" would have different results
depending on whether "mybranch" refers to a valid object locally.

The way I see it, this should either (i) only accept full 40-character
OIDs or (ii) retain the raw string to be interpreted only when the
filtering is done. (i) is simpler and safer, but is not so useful. In
both cases, if the user really wants client-side interpretation, they
can still use "$(git rev-parse foo)" to make it explicit.

Good point. I'll change it to always pass the original expression
so that it is evaluated wherever the filtering is actually performed.

quoted

+				free((char *)filter_options->raw_value);
+				filter_options->raw_value =
+					xstrfmt("sparse:oid=%s",
+						oid_to_hex(&sparse_oid));
+				filter_options->sparse_oid_value =
+					oiddup(&sparse_oid);
+			} else {
+				/*
+				 * We could not turn the <oid-expr> 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=", &v1)) {
+			filter_options->choice = LOFC_SPARSE_PATH;
+			filter_options->sparse_path_value = strdup(v1);
+			return 0;
+		}
+	}
+
+	die(_("invalid filter expression '%s'"), arg);
+	return 0;
+}

[snip]

quoted

+void arg_format_list_objects_filter(
+	struct argv_array *argv_array,
+	const struct list_objects_filter_options *filter_options)

Is this function used anywhere (in this patch or subsequent patches)?

It is used in upload-pack.c in part 3.  I'll remove it from part 1
and revisit in part 3.
  

quoted

diff --git a/list-objects-filter.c b/list-objects-filter.c
+/* See object.h and revision.h */
+#define FILTER_REVISIT (1<<25)

Looking later in the code, this flag indicates that a tree has been
SHOWN, so it might be better to just call this FILTER_SHOWN.

I'll amend this. There are already several SHOWN bits that behave
slightly differently.  I'll update and document this better.  Thanks.

[snip]

quoted

+struct frame {
+	int defval;

Document this variable?

quoted

+	int child_prov_omit : 1;

I think it's clearer if we use "unsigned" here. Also, document this
(e.g. "1 if any descendant of this tree object was provisionally
omitted").

got it. thanks.

quoted

+enum list_objects_filter_type {
+	LOFT_BEGIN_TREE,
+	LOFT_END_TREE,
+	LOFT_BLOB
+};

Optional: probably a better name would be list_objects_filter_situation.

got it. thanks.

  

quoted

+void traverse_commit_list_filtered(
+	struct list_objects_filter_options *filter_options,
+	struct rev_info *revs,
+	show_commit_fn show_commit,
+	show_object_fn show_object,
+	void *show_data,
+	struct oidset *omitted)
+{
+	filter_object_fn filter_fn = NULL;
+	filter_free_fn filter_free_fn = NULL;
+	void *filter_data = NULL;
+
+	filter_data = list_objects_filter__init(omitted, filter_options,
+						&filter_fn, &filter_free_fn);
+	do_traverse(revs, show_commit, show_object, show_data,
+		    filter_fn, filter_data);
+	if (filter_data && filter_free_fn)
+		filter_free_fn(filter_data);
+}

This function traverse_commit_list_filtered() is in list-objects.c but
in list-objects-filter.h, if I'm reading the diff correctly?

oops.  thanks.

Overall, this looks like a good change. Object traversal was upgraded
with the behaviors of MARK_SEEN and SHOW independently controllable and
with the ability to do things post-tree (in addition to pre-tree and
blob), and this was used to support a few types of filtering, which
subsequent patches will allow the user to invoke through "--filter=".

thanks
Jeff