Re: [PATCH v2 0/7] making log --first-parent imply -m
- From
Sergey Organov <sorganov@gmail.com>
- Date
- Aug 4, 2020, 21:41 UTC
- Message-ID
- <87y2muxf5m.fsf@osv.gnss.ru>
- In-Reply-To
- <20200804212518.GB2020725@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 28 quoted lines
> On Tue, Aug 04, 2020 at 11:56:25PM +0300, Sergey Organov wrote:
>
>> >> diff --git a/revision.c b/revision.c
>> >> index 669bc856694f..dcdff59bc36a 100644
>> >> --- a/revision.c
>> >> +++ b/revision.c
>> >> @@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct
>> >> rev_info *revs, int argc, const char **arg
>> >> revs->diff = 1;
>> >> revs->diffopt.flags.recursive = 1;
>> >> revs->diffopt.flags.tree_in_recursive = 1;
>> >> - } else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) {
>> >> + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
>> >> revs->ignore_merges = 0;
>> >> + if (!strcmp(optarg, "off")) {
>> >> + revs->ignore_merges = 1;
>> >> + } else if (!strcmp(optarg, "all")) {
>> >> + revs->diff = 0;
>> >
>> > Should this be revs->ignore_merges = 0?
>>
>> It's 4 lines above, as it's in fact common for all the cases but the
>> first one.
>
> Ah, I missed that. That raises more questions, though. ;)
>
> For "-m" we do not need to set revs->diff; why do we need to do so
> here?Good question!
I believe --diff-merges=all should reset revs->diff back to 0 to entirely undo all the effects of '-c' and '--cc', provided those appeared before on the command-line, that '-m' fails to do.
Admittedly, I didn't yet check what is the outcome of, say, "git log -c -m". Is it = "-c", ="-m", or what?
Also, this makes 'all' not the entire synonym for '-m', and that's why I removed 'all' from the second version of the patch ;-)
Show 5 quoted lines
> > For "--cc", we do not need to set revs->ignore_merges. Why do we need to > do so here? We do need it set eventually, but I think setup_revisions() > later handles that, and wants ignore_merges untouched to decide whether > the user asked for it or not.
OK, I'll take further look at this for further changes, -- thanks for telling!
My general approach though is that every of mutually exclusive options should better explicitly set all the involved variables appropriately.
Thanks, -- Sergey