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

[Elijah Newren <newren@gmail.com>]

On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> These patch series implement new set of options governing diff output
> of merge commits, all under the umbrella of single
> --diff-merges=<mode> option.

With this round, I was hoping to get a range-diff (using the
--range-diff option to format-patch), so I could more easily see what
was new.  Since I've been updating frequently enough, I was able to
generate this locally using Junio's published so/log-diff-merge topic,
but it'd be helpful if you could include it in any future rounds.

With your previous patch series, I scanned most the patches somewhat
briefly but looked at the final patches more closely (the series is
kind of long, and I noticed Junio had started reviewing the early part
of the series, so I figured it might be most helpful to jump in and
cover the end in case he didn't get that far).  With this round, I
read through the range-diff, and then looked at all the new patches
and have left a number of comments.  I think Junio reviewed the first
8 or so patches of an earlier round, so patches 9-21 probably could
benefit from someone reviewing more closely.

Overall, I like the direction of the series.  I think it'll make it
easier to add --remerge-diff later since it simplifies getting the
interaction between it and -m/-c/--cc/--first-parent right.  It also
adds some new capabilities you want (--diff-merges=first-parent, to
show merges as diff against first parent without only traversing first
parents), and that Junio wants (--diff-merges=dense-combined, to only
show merges for diffs without showing diffs for non-merge commits).

> Unlike original -c/--cc options, these new options do not imply -p,
> thus allowing for getting diffs for merge commits without provoking of
> diff output for regular, non-merge commits. E.g.:
>
>   git log --diff-merges=cc
>
> will output diffs (in dense-combined format) only for merge commits,
> whereas:
>
>   git log --cc
>
> enables diffs for all the commits being output, either merges or
> simple ones.
>
> There is also another additional functionality provided, allowing to
> get the format of "-p --first-parent" without change in history
> traversal that --first-parent option causes, like this:
>
>   git log --diff-merges=first-parent
>
> The net result of these series are the following new options:
>
> --diff-merges=    |  rough original equivalent
> ------------------+----------------
> 1|first-parent    | --first-parent (only diff format implications)
> m|separate        | -m and enable diff for merges
> c|combined        | -c and enable diff for merges, but not for regulars
> cc|dense-combined | --cc and enable diff for merges, but not for
>                   |      regulars
>
> The series also cleanup logic of handling of diff merges options and
> fix an issue found in the original implementation where logically
> mutually exclusive options -m/-c/--cc failed to actually override each
> other. Neither semantics of these old options nor their interactions
> with other options, such as --first-parent and -p, is supposed to be
> changed.
>
> The series start with the set of pure refactoring commits that are expected
> to introduce no functional changes. These are all commits up to and
> including:
>
> "diff-merges: revise revs->diff flag handling"
>
> The aim of these commits is to isolate options handling for diff merges so
> that it could be easily understood and tweaked to ease introduction of the
> new options.
>
> Then the fix of -m/-c/-cc overriding issue follows, starting with a failing
> test and followed by the fix.
>
> Then follows a little bit of additional refactoring in order to
> prepare for introduction of the new options, and finally the series
> are finished by the implementation, documentation updates, and
> some testing for the new options.
>
> Updates in v2:
>
>   * Move logic of "-c/--cc imply -p" to this module and do not imply
>     -p by new --diff-merges options. Instead enable corresponding diff
>     output without affecting non-merge commits. This is the most
>     significant change with respect to v1 and it starts at 24/33.
>
>   * Add support for old mnemonics: --diff-merges=(m|c|cc) to help
>     those who are used to them, and add --diff-merges=1 to cover all
>     variants with short mnemonics.
>
>   * Fixed functions definitions style to have open curly brace on its
>     own line, pointed to by Junio C Hamano.
>
>   * Tweak --diff-merges=first-parent description, requested by Elijah
>     Newren.
>
>   * Fixed git-show documentation not to include description chunk
>     relevant to git-log only, noticed by Elijah Newren.
>
>   * Fixed documentation mistake claiming that -p is needed for
>     diff-merges options to take effect, noticed by Elijah Newren.
>
>   * Fixed a case where a change was put into wrong commit. The change
>     moved to 11/27 form 10/27. Didn't affect end-result in any way.
>
>   * Added short module description to diff-merges.h, as suggested by
>     Junio C Hamano.
>
>   * Fixed not returning "argcount" from diff_merges_parse_opts(),
>     noticed by Junio C Hamano.
>
> Updates in v1:
>
>   * Added documentation fix for git-show to include --diff-merges.
>
>   * Fixed typos in commit messages noticed by Philip Oakley.
>
> Sergey Organov (33):
>   revision: factor out parsing of diff-merge related options
>   revision: factor out setup of diff-merge related settings
>   revision: factor out initialization of diff-merge related settings
>   revision: provide implementation for diff merges tweaks
>   revision: move diff merges functions to its own diff-merges.c
>   diff-merges: rename all functions to have common prefix
>   diff-merges: move checks for first_parent_only out of the module
>   diff-merges: rename diff_merges_default_to_enable() to match semantics
>   diff-merges: re-arrange functions to match the order they are called
>     in
>   diff-merges: new function diff_merges_suppress()
>   diff-merges: new function diff_merges_set_dense_combined_if_unset()
>   diff-merges: introduce revs->first_parent_merges flag
>   diff-merges: revise revs->diff flag handling
>   t4013: support test_expect_failure through ':failure' magic
>   t4013: add tests for -m failing to override -c/--cc
>   diff-merges: fix -m to properly override -c/--cc
>   diff-merges: split 'ignore_merges' field
>   diff-merges: group diff-merge flags next to each other inside
>     'rev_info'
>   diff-merges: get rid of now empty diff_merges_init_revs()
>   diff-merges: refactor opt settings into separate functions
>   diff-merges: make -m/-c/--cc explicitly mutually exclusive
>   diff-merges: implement new values for --diff-merges
>   diff-merges: fix style of functions definitions
>   diff-merges: handle imply -p on -c/--cc logic for log.c
>   diff-merges: do not imply -p for new options
>   diff-merges: let new options enable diff without -p
>   diff-merges: add old mnemonic counterparts to --diff-merges
>   diff-merges: add '--diff-merges=1' as synonym for 'first-parent'
>   doc/git-log: describe new --diff-merges options
>   doc/diff-generate-patch: mention new --diff-merges option
>   doc/rev-list-options: document --first-parent changes merges format
>   doc/git-show: include --diff-merges description
>   t4013: add tests for --diff-merges=first-parent
>
>  Documentation/diff-generate-patch.txt         |   6 +-
>  Documentation/diff-options.txt                |  53 +++++
>  Documentation/git-log.txt                     |  46 +---
>  Documentation/git-show.txt                    |   7 +-
>  Documentation/rev-list-options.txt            |   5 +
>  Makefile                                      |   1 +
>  builtin/diff-files.c                          |   5 +-
>  builtin/diff.c                                |   9 +-
>  builtin/log.c                                 |  22 +-
>  builtin/merge.c                               |   3 +-
>  diff-merges.c                                 | 151 +++++++++++++
>  diff-merges.h                                 |  24 +++
>  fmt-merge-msg.c                               |   3 +-
>  log-tree.c                                    |  30 +--
>  revision.c                                    |  38 +---
>  revision.h                                    |   9 +-
>  t/t4013-diff-various.sh                       |  11 +-
>  t/t4013/diff.log_--cc_-m_-p_master            | 200 ++++++++++++++++++
>  ...diff.log_--diff-merges=first-parent_master |  56 +++++
>  t/t4013/diff.log_-c_-m_-p_master              | 200 ++++++++++++++++++
>  ...f.log_-p_--diff-merges=first-parent_master | 137 ++++++++++++
>  21 files changed, 900 insertions(+), 116 deletions(-)
>  create mode 100644 diff-merges.c
>  create mode 100644 diff-merges.h
>  create mode 100644 t/t4013/diff.log_--cc_-m_-p_master
>  create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_master
>  create mode 100644 t/t4013/diff.log_-c_-m_-p_master
>  create mode 100644 t/t4013/diff.log_-p_--diff-merges=first-parent_master
>
> --
> 2.25.1
>