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

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Nov 16, 2017, 21:49 UTC
Message-ID: <bdf75d90-570d-8cf0-886a-2c5c9cdba1c1@jeffhostetler.com>
In-Reply-To: <20171116122133.4cc718414579c1a5a682174b@google.com>

On 11/16/2017 3:21 PM, Jonathan Tan wrote:

Show 29 quoted lines

> On Thu, 16 Nov 2017 18:07:41 +0000
> Jeff Hostetler <git@jeffhostetler.com> wrote:
> 
>> +/*
>> + * Return 1 if the given string needs armoring because of "special"
>> + * characters that may cause injection problems when a command passes
>> + * the argument to a subordinate command (such as when upload-pack
>> + * launches pack-objects).
>> + *
>> + * The usual alphanumeric and key punctuation do not trigger it.
>> + */
>> +static int arg_needs_armor(const char *arg)
> 
> First of all, about the injection problem, replying to your previous e-mail
> [1]:
> 
> https://public-inbox.org/git/61855872-221b-0e97-abaa-24a011ad899e@jeffhostetler.com/
> 
>> I couldn't use quote.[ch] because it is more concerned with
>> quoting pathnames because of LF and CR characters within
>> them -- rather than semicolons and quotes and the like which
>> I was concerned about.
> 
> sq_quote_buf() (or one of the other similarly-named functions) should
> solve this problem, right? The single quotes around the argument takes
> care of LF, CR, and semicolons, and things like backslashes and quotes
> are taken care of as documented.
> 
> I don't think we need to invent another encoding to solve this.

I'll take another look, sq_quote_buf() looks like it might work. I was looking at quote_c_style() and that didn't seem right for my needs. Thanks.

Show 15 quoted lines

> 
>> +{
>> +	const unsigned char *p;
>> +
>> +	for (p = (const unsigned char *)arg; *p; p++) {
>> +		if (*p >= 'a' && *p <= 'z')
>> +			continue;
>> +		if (*p >= 'A' && *p <= 'Z')
>> +			continue;
>> +		if (*p >= '0' && *p <= '9')
>> +			continue;
>> +		if (*p == '-' || *p == '_' || *p == '.' || *p == '/')
>> +			continue;
> 
> If we do take this approach, can ':' also be included?

Sure.  I just picked the common ones.

Show 30 quoted lines

> 
>> +	if (skip_prefix(arg, "sparse:oid=", &v0)) {
>> +		struct object_context oc;
>> +		struct object_id sparse_oid;
>> +
>> +		/*
>> +		 * Try to parse <oid-expression> into an OID for the current
>> +		 * command, but DO NOT complain if we don't have the blob or
>> +		 * ref locally.
>> +		 */
>> +		if (!get_oid_with_context(v0, GET_OID_BLOB,
>> +					  &sparse_oid, &oc))
>> +			filter_options->sparse_oid_value = oiddup(&sparse_oid);
>> +		filter_options->choice = LOFC_SPARSE_OID;
>> +		if (arg_needs_armor(v0))
>> +			filter_options->requires_armor = v0 - arg;
>> +		return 0;
>> +	}
> 
> In your previous e-mail, you mentioned:
> 
>> yes.  I always pass filter_options.raw_value over the wire.
>> The code above tries to parse it and put it in an OID for
>> private use by the current process -- just like the size limit
>> value in the blob:limit filter.
> 
> So I think this function should complain if you don't have the blob or
> ref locally. (I envision that if a filter string is to be directly sent
> to a server, it should be stored as a string, not processed by this
> function first.)

The whole point was for clone to be able to ask the server to use a known sparse-checkout spec, based on a branch name and a pathname within the tree. That's more usable. My expectation was that users could keep one or more sparse-checkout specs in the tree based upon the area/subset of the tree they want to work on. Then do a partial clone based upon the desired subset. Granted, we make them publish OIDs (outside of git) and then make the user request the partial clone by OID, but that's awkward.

There are 2 uses of the "--filter=..." parser -- one to actually use the data within the current process (rev-list and pack-objects) and one to capture the value and pass it on to another process (fetch-pack and upload-pack). The former want the fields fully decoded into actual variables and the latter want the raw string to pass on. The parser routine provides both values for its callers. (In an earlier draft, the parser would replace the raw_value of a sparse spec with the actual OID when it had it locally, but I took that out based on your earlier comments. Now, the raw_value is always passed by the second type of usages.)

Jeff