On Fri, Dec 18, 2020 at 6:12 AM Sergey Organov <sorganov@xxxxxxxxx> wrote:
>
> Elijah Newren <newren@xxxxxxxxx> writes:
>
> > On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov <sorganov@xxxxxxxxx> wrote:
> >>
> >> We first implement new options as exact synonyms for their original
> >> counterparts, to get all the infrastructure right, and keep functional
> >> improvements for later commits.
> >>
> >> The following values are implemented:
> >>
> >> --diff-merges= old equivalent
> >> first|first-parent = --first-parent (only format implications)
> >> sep|separate = -m
> >> comb|combined = -c
> >> dense| dense-combined = --cc
> >>
> >> Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx>
> >> ---
> >> diff-merges.c | 19 ++++++++++++++++---
> >> 1 file changed, 16 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/diff-merges.c b/diff-merges.c
> >> index 6446e2093661..cba391604ac7 100644
> >> --- a/diff-merges.c
> >> +++ b/diff-merges.c
> >> @@ -15,6 +15,11 @@ static void set_separate(struct rev_info *revs) {
> >> revs->separate_merges = 1;
> >> }
> >>
> >> +static void set_first_parent(struct rev_info *revs) {
> >> + set_separate(revs);
> >> + revs->first_parent_merges = 1;
> >> +}
> >> +
> >> static void set_m(struct rev_info *revs) {
> >> /*
> >> * To "diff-index", "-m" means "match missing", and to the "log"
> >> @@ -38,11 +43,19 @@ static void set_dense_combined(struct rev_info *revs) {
> >> }
> >>
> >> static void set_diff_merges(struct rev_info *revs, const char *optarg) {
> >> - if (!strcmp(optarg, "off")) {
> >> + if (0) ;
> >
> > Leftover cruft from some intermediate changes or something?
>
> No. It's just an idiom for if-switch, making all the actual cases look
> the same. Without this the first one looks special when in fact it
> isn't. I won't die for it though.
>
> >
> >> + else if (!strcmp(optarg, "off") || !strcmp(optarg, "none"))
> >> suppress(revs);
> >> - } else {
> >> + else if (!strcmp(optarg, "first") || !strcmp(optarg, "first-parent"))
> >> + set_first_parent(revs);
> >> + else if (!strcmp(optarg, "sep") || !strcmp(optarg, "separate"))
> >> + set_separate(revs);
> >> + else if (!strcmp(optarg, "comb") || !strcmp(optarg, "combined"))
> >> + set_combined(revs);
> >> + else if (!strcmp(optarg, "dense") || !strcmp(optarg,
> >> "dense-combined"))
> >> + set_dense_combined(revs);
> >> + else
> >
> > Not sure I like the special-casing for "sep" and "comb". "dense"
> > seems okay since it's a real word.
>
> That was a poor-man attempt at unique shortcuts made by hand, as well as
> a reminder to consider to re-write this using generic options framework
> that will do it automagically. They are just a few first letters,
> nothing more. That's why I didn't even document them.
Seems like a
/* TODO: use generic options framework to allow abbreviations */
would make that intent much more clear.
> > Since you're adding short versions of m, c, and cc later in the
> > series, do we need these other special-case forms?
>
> No, I don't think we do necessarily need them, but then they do no harm
> either, so I didn't remove them when I added m, c, and cc. Neither was I
> sure we do need these m, c, and cc in the first place.
Well, I'd say it certainly adds clutter for reviewers, so there is at
least a little harm. ;-) It was on the patch that added m, c, and cc
that it particularly stood out to me, so I looked up the relevant
commit so that I could add the comment at the right place to make it
easier for you to rip out.
