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

From: Duy Nguyen <pclouds@gmail.com>
Date: Jan 26, 2017, 09:18 UTC
Message-ID: <CACsJy8CeUWD9ux3Q-VRhwR4-qbSD69CzH2mmztVH_-cx=31h8w@mail.gmail.com>
In-Reply-To: <20170125205037.cg3aebh5rsx4zb2l@sigill.intra.peff.net>

On Thu, Jan 26, 2017 at 3:50 AM, Jeff King <peff@peff.net> wrote:

Show 24 quoted lines

> On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> 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.

It's not simplification, but hopefully for better maintainability. This

if (strcmp(arg, "--remotes")) {
   if (preceded_by_exclide())
      does_something();
   else if (preceded_by_decorate())
      does_another()
} else if (strcmp(arg, "--branches")) {
   if (preceded_by_exclide())
      does_something();
   else if (preceded_by_decorate())
      does_another()
}

starts to look ugly especially when the third "preceded_by_" comes into picture. Putting all "does_something" in one group and "does_another" in another, I think, gives us a better view how ref selection is handled for a specific operation like --exclude or --decorate-ref.

> 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.

It may go that way, depending on how we handle these options for decorate-reflog. The current load_ref_decorations() is not really suited for fine-grained ref selection yet. -- Duy