On Fri, Nov 30, 2018 at 05:32:47PM -0800, Matthew DeVore wrote: > > 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@xxxxxxxxxx> > 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: > 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. > 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
