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

From: Matthew DeVore <matvore@comcast.net>
To: Jeff King <peff@peff.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 11:10:49 -0800	[thread overview]
Message-ID: <80a08b99-14cb-e398-e6c2-2aa94a5fdda3@comcast.net> (raw)
In-Reply-To: <20181201194424.GB28918@sigill.intra.peff.net>

On 12/01/2018 11:44 AM, Jeff King wrote:
>>   	repo_init_revisions(the_repository, &revs, NULL);
>>   	save_commit_buffer = 0;
>> -	revs.allow_exclude_promisor_objects_opt = 1;
>> -	setup_revisions(ac, av, &revs, NULL);
>> +
>> +	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.

> 
>>   static int handle_revision_opt(struct rev_info *revs, int argc, const char
>> **argv,
>> -			       int *unkc, const char **unkv)
>> +			       int *unkc, const char **unkv,
>> +			       int allow_exclude_promisor_objects)
> 
> Why not pass in the whole setup_revision_opt struct? We don't need
> anything else from it yet, but it seems like the point of that struct is
> to pass around preferences like this.
OK, the code reads better if I do that, so I agree.

> 
> -Peff
> 