Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

From: Jeff King <peff@peff.net>
To: Matthew DeVore <matvore@comcast.net>
Cc: Matthew DeVore <matvore@google.com>,
	git@vger.kernel.org, gitster@pobox.com, pclouds@gmail.com,
	jonathantanmy@google.com, jeffhost@microsoft.com
Subject: Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
Date: Mon, 3 Dec 2018 16:15:55 -0500	[thread overview]
Message-ID: <20181203211555.GA8700@sigill.intra.peff.net> (raw)
In-Reply-To: <80a08b99-14cb-e398-e6c2-2aa94a5fdda3@comcast.net>

On Mon, Dec 03, 2018 at 11:10:49AM -0800, Matthew DeVore wrote:

> > > +	memset(&s_r_opt, 0, sizeof(s_r_opt));
> > > +	s_r_opt.allow_exclude_promisor_objects = 1;
> > > +	setup_revisions(ac, av, &revs, &s_r_opt);
> > 
> > I wonder if a static initializer for setup_revision_opt is worth it. It
> > would remove the need for this memset. Probably not a big deal either
> > way, though.
> I think you mean something like this:
> 
> static struct setup_revision_opt s_r_opt = {NULL, NULL, NULL, 0, 1, 0};
> 
> This is a bit cryptic (I have to read the struct declaration in order to
> know what is being set to 1) and if the struct ever gets a new field before
> allow_exclude_promisor_objects, this initializer has to be updated.

I agree that's pretty awful.  I meant something like this:

  struct setup_revision_opt s_r_opt = { NULL };
  ...

  s_r_opt.allow_exclude_promisor_objects = 1;
  setup_revisions(...);

It's functionally equivalent to the memset(), but you don't have to
wonder about whether we peek at the uninitialized state in between.

That said, our C99 designated initializer weather-balloons haven't
gotten any complaints yet. So I think you could actually do:

  struct setup_revision_opt s_r_opt = {
	.allow_exclude_promisor_objects = 1,
  };
  ...
  setup_revisions(...);

which is pretty nice.

-Peff