Re: [PATCH] revisions.c: put promisor option in specialized struct
- From
Jeff King <peff@peff.net>
- Date
- Dec 3, 2018, 21:24 UTC
- Message-ID
- <20181203212431.GB8700@sigill.intra.peff.net>
- In-Reply-To
- <20181203192356.51432-1-matvore@google.com>
On Mon, Dec 03, 2018 at 11:23:56AM -0800, Matthew DeVore wrote:
Show 13 quoted lines
> 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. > > Signed-off-by: Matthew DeVore <matvore@google.com> > --- > builtin/pack-objects.c | 7 +++++-- > builtin/prune.c | 1 - > builtin/rev-list.c | 6 ++++-- > revision.c | 10 ++++++---- > revision.h | 4 ++-- > 5 files changed, 17 insertions(+), 11 deletions(-)
Thanks, this mostly looks good to me (with or without tweaking the initializers as discussed in the other part of the thread).
One thing I noticed:
Show 9 quoted lines
> @@ -297,7 +296,8 @@ struct setup_revision_opt {
> const char *def;
> void (*tweak)(struct rev_info *, struct setup_revision_opt *);
> const char *submodule; /* TODO: drop this and use rev_info->repo */
> - int assume_dashdash;
> + int assume_dashdash : 1;
> + int allow_exclude_promisor_objects : 1;
> unsigned revarg_opt;
> };I don't know that we need to penny-pinch bytes in this struct, but in general it shouldn't hurt either awy. However, a signed bit-field with 1 bit is funny. I'm not even sure what the standard has to say, but in twos-complement that would store "-1" and "0" (gcc -Wpedantic also complains about overflow in assigning "1" to it).
So this probably ought to be "unsigned".
-Peff