Re: [PATCH v2 22/33] diff-merges: implement new values for --diff-merges

Elijah Newren [off-list ref] writes:

On Fri, Dec 18, 2020 at 6:12 AM Sergey Organov [off-list ref] wrote:

quoted

Elijah Newren [off-list ref] writes:

quoted

On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov [off-list ref] wrote:

quoted

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

quoted

quoted

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

quoted

quoted

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.

May that's because they did make sense before the patch that added m, c,
and cc, where they stood out for you.

Overall, if we do agree m, c, and cc mnemonics are good to have, I'd opt
for removing these hand-made abbrevs indeed.

Thanks,
-- Sergey