Elijah Newren <newren@xxxxxxxxx> writes:
> 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.
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
