Re: [PATCH 2/5] revision.c: group ref selection options together
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
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
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
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
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