Re: [PATCH 00/26] git-log: implement new --diff-merge options

From: Elijah Newren <newren@gmail.com>
Date: Dec 10, 2020, 07:26 UTC
Message-ID: <CABPp-BE7r4iUc6VD60Bdi+fF2bBn6_ZwVPrz_niFpY=LTZMXzw@mail.gmail.com>
In-Reply-To: <87k0tqdasa.fsf@osv.gnss.ru>

On Wed, Dec 9, 2020 at 11:44 AM Sergey Organov <sorganov@gmail.com> wrote:

Show 17 quoted lines

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> [...]
>
> > By the time the change to make "--cc" imply "-p" was introduced, it
> > was pretty much given that "-m -p" was useful to anybody, unless you
> > are consuming these individual patches in a script or something like
> > that.  So simply I didn't even think of making "-m" imply "-p".  It
> > would be logical to make it so, but it would not add much practical
> > value, I would have to say.
>
> I need some help here.
>
> Looking at the code and trying to follow the flow, I can't figure what
> rev->diff flag is for? Why rev->diffopt.output_format, that actually
> affects the output, is not enough?

I'm not a revision walking expert, but to the best of my understanding...

Showing a diff is not the only reason you might need to compute one. You may also need to compute them if you are filtering commits by pathspec (-- $filename), using the pickaxe (-S foo), checking if commits are cherry-picks (--cherry-mark), checking for commits with certain type of file changes (--diff-filter=A), selecting commits that modified a certain function (-L :funcname:filename --no-patch), or others I've overlooked. None of these cause a diff to be shown. I don't know if all these set rev->diff to 1 or if they special case some other way, but I suspect that rev->diff exists as a shorthand for "need a diff", so that the code can check for it without having to check a half dozen special conditions.

Show 7 quoted lines

>> My confusion originates from the fact that the code in revision.c sets
> rev->diff to 1 for -c/--cc , while it doesn't set it for -m, and this
> was the case *before* -c/--cc started to imply -p and -m.
>
> It seems that the only place where rev->diff is tested is at the start
> of log_tree_diff(), and even there its absence could be ignored when
> rev->diffopt.flags.exit_with_status is set.

rev->diffopt.flags.exit_with_status seems quite unlikely to be set, though. That setting was added with the --exit-code flag to git log in 2008 (in the pm/log-exit-code topic), but was never documented (other than to say it's incompatible with --check), the commit message adding it doesn't say what behavior was intended, and the commit message which added it added no regression tests either. I know what diff --exit-code does, but I'm really not sure what git-log's --exit-code does (random guess: sets the exit code to an OR of what git diff would have shown for any one of the commits shown?). Since I don't know and can't even figure it out looking at the commits in question, I suspect there aren't too many users out there using it. As such, I suspect rev->diffopt.flags.exit_with_status will be 0 most of the time and that the relevant check at the top of log_tree_diff() really is the "if (!opt->diff)" part of it.

> Is rev->diff an optimization, does it play another significant role, or
> is it a remnant?