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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2017, 21:11 UTC
Message-ID: <xmqqo9yualqn.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170125125054.7422-3-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> These options have on thing in common: when specified right after

one thing.

Show 11 quoted lines

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

I am not sure about these two refactorings, for at least two reasons.

 * Naming.  The function is all about handling "refs options" and I
   do not see anything "pseudo" about the options handled by the
   handle_refs_pseudo_opt() function.  This is minor.

 * I am expecting that the new one yet to be introduced will not
   share the huge "switch (selector)" part, but does its own things
   in a separate function with a similar structure.  The only thing
   common between these two functions would be the structure
   (i.e. it has a big "switch(selector)" that does different things
   depending on REF_SELECT_*) and a call to clear_* function.

   If we were to add a new kind of REF_SELECT_* (say
   REF_SELECT_NOTES just for the sake of being concrete), what
   changes will be needed to the code if the addition of "use reflog
   from this class of refs for decoration" feature was done with or
   without this step?  I have a suspicion that the change will be
   simpler without this step.

   The above comments will be invalid if the new "use reflog for
   decoration" feature will be done by extending this pseudo_opt()
   function by extending each of the switch/case arms.  If that is
   the case, please disregard the above.  I just didn't get that
   impression from the above paragraph.