| Message ID | 20201216184929.3924-1-sorganov@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | git-log: implement new --diff-merge options | expand |
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 >
Hi Elijah, Thanks a lot for reviewing the series! Elijah Newren <newren@gmail.com> writes: > 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. Well, documentation on contributing doesn't seem to mention this. I'll try to learn it for the future though, thanks! > > 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). Yeah, I hoped to address everything that has been discussed. [...] Thanks, -- Sergey
These patch series implement new set of options governing diff output of merge commits, all under the umbrella of single --diff-merges=<mode> option. 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