Re: [PATCH v2 22/33] diff-merges: implement new values for --diff-merges
- From
Sergey Organov <sorganov@gmail.com>
- Date
- Dec 18, 2020, 14:12 UTC
- Message-ID
- <871rfncie5.fsf@osv.gnss.ru>
- In-Reply-To
- <CABPp-BG1S83EUbqtqqjeLT4=Je44BpRMrf0MB+DNQK2wjRDbqQ@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
Show 43 quoted lines
> On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov <sorganov@gmail.com> 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@gmail.com>
>> ---
>> 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.
Show 17 quoted lines
>
>> + 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.
> 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.
Thanks, -- Sergey