From: Jeff Hostetler <git@jeffhostetler.com>
Date: Tue, 07 Nov 2017 18:54:20 GMT
Subject: Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list
Message-ID: <f698d5a8-bf31-cea1-a8da-88b755b0b7af@jeffhostetler.com>
In-Reply-To: <20171102123245.0f768968703ec4e35d3d1f81@google.com>



On 11/2/2017 3:32 PM, Jonathan Tan wrote:
> On Thu,  2 Nov 2017 17:50:11 +0000
> Jeff Hostetler <git@jeffhostetler.com> wrote:
> 
>> +		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.


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


>> 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]
> 
>> +struct frame {
>> +	int defval;
> 
> Document this variable?
> 
>> +	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.


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

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