git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Matthew DeVore <matvore@comcast.net>
To: Jeff Hostetler <git@jeffhostetler.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Emily Shaffer <emilyshaffer@google.com>,
	Matthew DeVore <matvore@google.com>,
	jonathantanmy@google.com, jrn@google.com, git@vger.kernel.org,
	dstolee@microsoft.com, jeffhost@microsoft.com,
	jrnieder@gmail.com, pclouds@gmail.com
Subject: Re: [PATCH] list-objects-filter: merge filter data structs
Date: Wed, 29 May 2019 16:10:13 -0700	[thread overview]
Message-ID: <20190529231013.GD4700@comcast.net> (raw)
In-Reply-To: <e9147614-80f9-4c18-b431-539e2376295d@jeffhostetler.com>

> > I am hoping that I am not misreading the intention but you do not
> > plan to use the above so that you can say "apply 'tree:depth=4' and
> > 'blobs:limit=1G' at the same time" by filling the fields in a single
> > struct, do you?  For combined filter, you'll still have multiple
> > instances of filter_data struct, strung together in a list that says
> > "all of these must be satisfied" or something like that, right?

It is the latter and not the former. I don't want to populate multiple filters
in a single struct. My idea was basically that when the fields got too numerous
we could use unions or add NULLable pointers for the bigger filter data structs,
so the data would be properly "optional".

On Wed, May 29, 2019 at 04:57:23PM -0400, Jeff Hostetler wrote:
> I'm not sure I like the combined structure as proposed.
> But let's think about it.
> 
> I think part of problem with my original version was putting the
> filter_fn and filter_free_fn in the traversal_context rather than
> inside the filter_*_data structure.

Agreed. This cleanup I'm proposing is basically something I was itching to do in
the process of bundling up the filter_fn and filter_free_fn pointers in a single
pointer, which makes the LOFC_COMBINE-particular filter data more concise.

I can still bundle up the pointers into a single pointer and make this cleanup
less aggressive.

> I did a simple combined structure for the list_objects_filter_options
> and kind of regretted it because it wasn't obvious which data fields
> were defined or undefined in each filter constructor.  But it was
> convenient when parsing the command line.
> 
> I think having a combined structure with a union enclosing a structure
> for the data fields in each filter type would be worth considering.
> That way you have a somewhat self-documenting sub-structure for each
> filter type that indicates which fields are defined.

I'm OK with the union approach. The drawback is that the __free function now
needs a switch block to choose the correct union, but the union is also good for
the self-documenting aspect you mention.

> 
> I'd also suggest keeping the "oidset omits" inside each of the
> sub-structures, but that's just me.
> 
> 
> BTW, I don't see a free_fn.  That may collapse out with your proposal
> but I wanted to ask.

See the list_objects_filter__free function. It's trivial, but it inherits the
leakiness of the sparse filters' free function it subsumes.

Thank you for considering the patch.

  reply	other threads:[~2019-05-29 23:10 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-22  0:21 [PATCH v1 0/5] Filter combination Matthew DeVore
2019-05-22  0:21 ` [PATCH v1 1/5] list-objects-filter: refactor into a context struct Matthew DeVore
2019-05-24  0:49   ` Emily Shaffer
2019-05-28 18:48     ` Matthew DeVore
2019-05-28 22:40       ` [PATCH] list-objects-filter: merge filter data structs Matthew DeVore
2019-05-29 19:48         ` Junio C Hamano
2019-05-29 20:57           ` Jeff Hostetler
2019-05-29 23:10             ` Matthew DeVore [this message]
2019-05-30  1:56             ` [RFC PATCH v2] " Matthew DeVore
2019-05-30 16:12               ` Junio C Hamano
2019-05-30 18:29                 ` Matthew DeVore
2019-05-30 19:05             ` [PATCH] " Matthew DeVore
2019-05-22  0:21 ` [PATCH v1 2/5] list-objects-filter-options: error is localizeable Matthew DeVore
2019-05-24  0:55   ` Emily Shaffer
2019-05-28 23:01     ` Matthew DeVore
2019-05-22  0:21 ` [PATCH v1 3/5] list-objects-filter: implement composite filters Matthew DeVore
2019-05-24 21:01   ` Jeff Hostetler
2019-05-28 17:59     ` Junio C Hamano
2019-05-29 15:02       ` Matthew DeVore
2019-05-29 21:29         ` Jeff Hostetler
2019-05-29 23:27           ` Matthew DeVore
2019-05-30 14:01             ` Jeff Hostetler
2019-05-31 20:53               ` Matthew DeVore
2019-06-03 21:04                 ` Jeff Hostetler
2019-06-01  0:11     ` Matthew DeVore
2019-05-28 21:53   ` Emily Shaffer
2019-05-31 20:48     ` Matthew DeVore
2019-05-31 21:10       ` Jeff King
2019-06-01  0:12         ` Matthew DeVore
2019-06-03 12:34           ` Jeff King
2019-06-03 22:22             ` Matthew DeVore
2019-06-04 16:13               ` Jeff King
2019-06-04 17:19                 ` Matthew DeVore
2019-06-04 18:51                   ` Jeff King
2019-06-04 22:59                     ` Matthew DeVore
2019-06-04 23:14                       ` Jeff King
2019-06-04 23:49                         ` Matthew DeVore
2019-06-09 12:36                           ` Jeff King
2019-05-22  0:21 ` [PATCH v1 4/5] list-objects-filter-options: move error check up Matthew DeVore
2019-05-22  0:21 ` [PATCH v1 5/5] list-objects-filter-options: allow mult. --filter Matthew DeVore
2019-06-06 22:44 ` [PATCH v1 0/5] Filter combination Matthew DeVore

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=20190529231013.GD4700@comcast.net \
    --to=matvore@comcast.net \
    --cc=dstolee@microsoft.com \
    --cc=emilyshaffer@google.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=jonathantanmy@google.com \
    --cc=jrn@google.com \
    --cc=jrnieder@gmail.com \
    --cc=matvore@google.com \
    --cc=pclouds@gmail.com \
    /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).