From: Jeff Hostetler <git@jeffhostetler.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, gitster@pobox.com, peff@peff.net,
Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list
Date: Tue, 7 Nov 2017 13:54:20 -0500 [thread overview]
Message-ID: <f698d5a8-bf31-cea1-a8da-88b755b0b7af@jeffhostetler.com> (raw)
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
next prev parent reply other threads:[~2017-11-07 18:54 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-02 17:50 [PATCH v2 0/6] Partial clone part 1: object filtering Jeff Hostetler
2017-11-02 17:50 ` [PATCH v2 1/6] dir: allow exclusions from blob in addition to file Jeff Hostetler
2017-11-02 17:50 ` [PATCH v2 2/6] oidmap: add oidmap iterator methods Jeff Hostetler
2017-11-02 17:50 ` [PATCH v2 3/6] oidset: add iterator methods to oidset Jeff Hostetler
2017-11-02 17:50 ` [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list Jeff Hostetler
2017-11-02 19:32 ` Jonathan Tan
2017-11-03 11:54 ` Johannes Schindelin
2017-11-03 13:37 ` Jeff Hostetler
2017-11-07 18:54 ` Jeff Hostetler [this message]
2017-11-06 17:51 ` Jeff Hostetler
2017-11-06 18:08 ` Jonathan Tan
2017-11-02 17:50 ` [PATCH v2 5/6] rev-list: add list-objects filtering support Jeff Hostetler
2017-11-02 17:50 ` [PATCH v2 6/6] pack-objects: add list-objects filtering Jeff Hostetler
2017-11-02 19:44 ` [PATCH v2 0/6] Partial clone part 1: object filtering Jonathan Tan
2017-11-03 13:43 ` Jeff Hostetler
2017-11-03 15:05 ` Junio C Hamano
2017-11-03 18:34 ` Jeff Hostetler
2017-11-08 0:41 ` Jonathan Tan
2017-11-08 0:54 ` Junio C Hamano
2017-11-08 14:39 ` Jeff Hostetler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f698d5a8-bf31-cea1-a8da-88b755b0b7af@jeffhostetler.com \
--to=git@jeffhostetler.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jeffhost@microsoft.com \
--cc=jonathantanmy@google.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).