Re: [PATCH 2/5] revision.c: group ref selection options together

From: Jeff King <peff@peff.net>
Date: Jan 25, 2017, 20:50 UTC
Message-ID: <20170125205037.cg3aebh5rsx4zb2l@sigill.intra.peff.net>
In-Reply-To: <20170125125054.7422-3-pclouds@gmail.com>

On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:

Show 17 quoted lines

> These options have on thing in common: when specified right after
> --exclude, they will de-select refs instead of selecting them by
> default.
> 
> This change makes it possible to introduce new options that use these
> options in the same way as --exclude. Such an option would just
> implement something like handle_refs_pseudo_opt().
> 
> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
> that similar functions like handle_refs_pseudo_opt() are forced to
> handle all ref selector options, not skipping some by mistake, which may
> revert the option back to default behavior (rev selection).
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 100 insertions(+), 34 deletions(-)

Hmm. I see what you're trying to do here, and abstract the repeated bits. But I'm not sure the line-count reflects a real simplification. Everything ends up converted to an enum, and then that enum just expands to similar C code.

I kind of expected that clear_ref_exclusion() would just become a more abstract clear_ref_selection(). For now it would clear exclusions, and then later learn to clear the decoration flags.

Maybe I am missing something in the later patches, though.

-Peff