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

[Jeff King]

On Thu, Jan 26, 2017 at 04:18:06PM +0700, Duy Nguyen wrote:

> > 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 agree that would be ugly. But the current structure, which is:

  if (strcmp(arg, "--remotes")) {
  handle_refs(...);
  cleanup();
  } else if(...) {
  handle_refs(...);
  cleanup();
  }

does not seem so bad, and pushes those conditionals into the
handle_refs() function, where they only need to be expressed once (I
didn't look, but I wonder if you could push the cleanup steps in there,
too, or if there is a caller who wants to handle() multiple times before
cleaning up).

-Peff

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

[Duy Nguyen]

On Thu, Jan 26, 2017 at 3:50 AM, Jeff King  wrote:
> 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 
>> ---
>>  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

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

[Duy Nguyen]

On Thu, Jan 26, 2017 at 4:11 AM, Junio C Hamano  wrote:
>  * 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.

Yep. The "new one" is demonstrated in 5/5.

>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 switch/case is to deal with new REF_SELECT_* (at least it's how I
imagine it). What I was worried about was, when a user adds
--select-notes, they may not be aware that it's in the same
all/branches/tags/remotes group that's supposed to work with
--decorate-reflog as well, and as a result "--decorate-reflog
--select-notes" is the same as "--select-notes".

With the switch/case, when you add a new enum item, at the least the
compiler should warn about unhandled cases. And we can have a new
"case REF_SELECT_NOTES:" for both --exclude and --decorate-reflog.
Without the switch/case, I guess it's still possible to do something
like

if (!strcmp(arg, "--select-notes")) {
if (preceded_by_exclude())
does_one_thing();
else if (preceded_by_decorate_reflog())
   does_another_thing();
}

It's probably easier to maintain though, if all
decorate-reflog-related things are grouped together, rather than
spread out per option like the above.
-- 
Duy

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

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

one thing.

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

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

[Jeff King]

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

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

[Nguyễn Thái Ngọc Duy]

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 
---
 revision.c | 134 +
 1 file changed, 100 insertions(+), 34 deletions(-)

diff --git a/revision.c b/revision.c
index b37dbec378..6ebd38d1c8 100644
--- a/revision.c
+++ b/revision.c
@@ -2059,6 +2059,103 @@ static int for_each_good_bisect_ref(const char 
*submodule, each_ref_fn fn, void
return for_each_bisect_ref(submodule, fn, cb_data, term_good);
 }
 
+enum ref_selector {
+   REF_SELECT_NONE,
+   REF_SELECT_ALL,
+   REF_SELECT_BRANCHES,
+   REF_SELECT_TAGS,
+   REF_SELECT_REMOTES,
+   REF_SELECT_BY_GLOB
+};
+
+static enum ref_selector parse_ref_selector_option(int argc, const char **argv,
+  const char **optarg,
+  int *argcount)
+{
+   const char *arg = argv[0];
+
+   *optarg = NULL;
+
+   if (!strcmp(arg, "--all"))
+   return REF_SELECT_ALL;
+   else if (!strcmp(arg, "--branches") ||
+skip_prefix(arg, "--branches=", optarg))
+   return REF_SELECT_BRANCHES;
+   else if (!strcmp(arg, "--tags") ||
+skip_prefix(arg, "--tags=", optarg))
+   return REF_SELECT_TAGS;
+   else if (!strcmp(arg, "--remotes") ||
+skip_prefix(arg, "--remotes=", optarg))
+   return REF_SELECT_REMOTES;
+   else if ((*argcount = parse_long_opt("glob", argv, optarg)))
+   return REF_SELECT_BY_GLOB;
+
+   return REF_SELECT_NONE;
+}
+
+static int handle_refs_pseudo_opt(const char *submodule,
+ struct rev_info *revs,
+ int argc, const char **argv,
+ int *flags, int *ret)
+{
+   struct all_refs_cb cb;
+   const char *optarg = NULL;
+   int argcount;
+   enum ref_selector selector;
+
+   selector = parse_ref_selector_option(argc, argv, , );
+
+   if (optarg)
+   init_all_refs_cb(, revs, *flags);
+   *ret = 1;
+
+   switch (selector) {
+   case REF_SELECT_ALL:
+   handle_refs(submodule, revs, *flags, for_each_ref_submodule);
+   handle_refs(submodule, revs, *flags, head_ref_submodule);
+   break;
+
+   case REF_SELECT_BRANCHES:
+   if (optarg)
+   for_each_glob_ref_in(handle_one_ref, optarg,
+"refs/heads/", );
+   else
+   handle_refs(submodule, revs, *flags,
+   for_each_branch_ref_submodule);
+   break;
+
+   case REF_SELECT_TAGS:
+   if (optarg)
+   for_each_glob_ref_in(handle_one_ref, optarg,
+"refs/tags/", );
+   else
+   handle_refs(submodule, revs, *flags,
+   for_each_tag_ref_submodule);
+   break;
+
+   case REF_SELECT_REMOTES:
+   if (optarg)
+   for_each_glob_ref_in(handle_one_ref, optarg,
+"refs/remotes/", );
+   else
+   handle_refs(submodule, revs, *flags,
+   for_each_remote_ref_submodule);
+   break;
+
+   case REF_SELECT_BY_GLOB:
+   init_all_refs_cb(, revs, *flags);
+   for_each_glob_ref(handle_one_ref, optarg, );
+   *ret = argcount;
+   break;
+
+   case REF_SELECT_NONE:
+   return 0;
+   }
+
+   clear_ref_exclusion(>ref_excludes);
+   return 1;
+}
+
 static int handle_revision_pseudo_opt(const char *submodule,
struct rev_info *revs,
int argc, const char **argv, int *flags)
@@ -2066,6 +2163,7 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
const char *arg = argv[0];
const char *optarg;
int argcount;
+   int ret;
 
/*
 * NOTE!
@@ -2077,48 +2175,16 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
 * When implementing your new pseudo-option, remember to
 * register it in the list at the top