Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
- From
Jeff King <peff@peff.net>
- Date
- Dec 1, 2018, 19:44 UTC
- Message-ID
- <20181201194424.GB28918@sigill.intra.peff.net>
- In-Reply-To
- <19c82fb0-e0d6-0b15-06ab-cfba4d699d94@comcast.net>
On Fri, Nov 30, 2018 at 05:32:47PM -0800, Matthew DeVore wrote:
Show 20 quoted lines
> > Speaking of which, would this flag work better as a field in > > setup_revision_opt, which is passed to setup_revisions()? The intent > > seem to be to influence how we parse command-line arguments, and that's > > where other similar flags are (e.g., assume_dashdash). > > Good idea. This would solve the problem of mistakenly believing the flag > matters when it doesn't, since it is in the struct which is used as the > arguments of the exact function that cares about it. Here's a new patch - > I'm tweaking e-mail client settings so hopefully this makes it to the list > without mangling - if not I'll resend it with `git-send-email` later. > > From 941c89fe1e226ed4d210ce35d0d906526b8277ed Mon Sep 17 00:00:00 2001 > From: Matthew DeVore <matvore@google.com> > Date: Fri, 30 Nov 2018 16:43:32 -0800 > Subject: [PATCH] revisions.c: put promisor option in specialized struct > > Put the allow_exclude_promisor_objects flag in setup_revision_opt. When > it was in rev_info, it was unclear when it was used, since rev_info is > passed to functions that don't use the flag. This resulted in > unnecessary setting of the flag in prune.c, so fix that as well.
Thanks, this looks pretty reasonable overall. Two comments:
Show 8 quoted lines
> 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.
Show 5 quoted lines
> 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.
-Peff