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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 7, 2017, 23:20 UTC
Message-ID: <20171107152034.47686f6ece72ea3d43005b12@google.com>
In-Reply-To: <20171107193546.10017-5-git@jeffhostetler.com>

On Tue, 7 Nov 2017 19:35:44 +0000 Jeff Hostetler <git@jeffhostetler.com> wrote:

Show 6 quoted lines

> +/*
> + * Reject the arg if it contains any characters that might
> + * require quoting or escaping when handing to a sub-command.
> + */
> +static int reject_injection_chars(const char *arg)
> +{

[snip]

> +}

Someone pointed me to quote.{c,h}, which is probably sufficient to ensure shell safety if we do invoke subcommands through the shell. If that is so, we probably don't need a blacklist.

Having said that, though, it might be safer to still introduce one, and relax it later if necessary - it is much easier to relax a constraint than to increase one.

Show 12 quoted lines

> +	} else if (skip_prefix(arg, "sparse:", &v0)) {
> +
> +		if (skip_prefix(v0, "oid=", &v1)) {
> +			struct object_context oc;
> +			struct object_id sparse_oid;
> +			filter_options->choice = LOFC_SPARSE_OID;
> +			if (!get_oid_with_context(v1, GET_OID_BLOB,
> +						  &sparse_oid, &oc))
> +				filter_options->sparse_oid_value =
> +					oiddup(&sparse_oid);
> +			return 0;
> +		}

In your recent e-mail [1], you said that you will change it to always pass the original expression - is that still the plan?

[1] https://public-inbox.org/git/f698d5a8-bf31-cea1-a8da-88b755b0b7af@jeffhostetler.com/

> +/* Remember to update object flag allocation in object.h */

You probably can delete this line.

Show 5 quoted lines

> +/*
> + * FILTER_SHOWN_BUT_REVISIT -- we set this bit on tree objects
> + * that have been shown, but should be revisited if they appear
> + * in the traversal (until we mark it SEEN).  This is a way to
> + * let us silently de-dup calls to show() in the caller.

This is unclear to me at first reading. Maybe something like:

  FILTER_SHOWN_BUT_REVISIT -- we set this bit on tree objects that have
  been shown, but should not be skipped over if they reappear in the
  traversal. This ensures that the tree's descendants are re-processed
  if the tree reappears subsequently, and that the tree is not shown
  twice.

Show 5 quoted lines

> + * This
> + * is subtly different from the "revision.h:SHOWN" and the
> + * "sha1_name.c:ONELINE_SEEN" bits.  And also different from
> + * the non-de-dup usage in pack-bitmap.c
> + */

Optional: I'm not sure if this comparison is useful. (Maybe it is useful
to others, though.)

Show 17 quoted lines

> +/*
> + * A filter driven by a sparse-checkout specification to only
> + * include blobs that a sparse checkout would populate.
> + *
> + * The sparse-checkout spec can be loaded from a blob with the
> + * given OID or from a local pathname.  We allow an OID because
> + * the repo may be bare or we may be doing the filtering on the
> + * server.
> + */
> +struct frame {
> +	/*
> +	 * defval is the usual default include/exclude value that
> +	 * should be inherited as we recurse into directories based
> +	 * upon pattern matching of the directory itself or of a
> +	 * containing directory.
> +	 */
> +	int defval;

Can this be an "unsigned defval : 1" as well? In the function below, I see that you assign to an "int val" first (which can take -1, 0, and 1) before assigning to this, so that is fine.

Also, maybe a better name would be "exclude", with the documentation:

  1 if the directory is excluded, 0 otherwise. Excluded directories will
  still be recursed through, because an "include" rule for an object
  might override an "exclude" rule for one of its ancestors.