Re: [PATCH] range-diff: add --matched-only to skip one-sided commits

[Junio C Hamano <gitster@pobox.com>]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/Documentation/git-range-diff.adoc b/Documentation/git-range-diff.adoc
> index 5cc5e2ed56..58e59e8e3b 100644
> --- a/Documentation/git-range-diff.adoc
> +++ b/Documentation/git-range-diff.adoc
> @@ -10,7 +10,8 @@ SYNOPSIS
>  [synopsis]
>  git range-diff [--color=[<when>]] [--no-color] [<diff-options>]
>  	[--no-dual-color] [--creation-factor=<factor>]
> -	[--left-only | --right-only] [--diff-merges=<format>]
> +	[--left-only | --right-only | --matched-only]
> +	[--diff-merges=<format>]
>  	[--remerge-diff] [--no-notes | --notes[=<ref>]]
>  	( <range1> <range2> | <rev1>...<rev2> | <base> <rev1> <rev2> )
>  	[[--] <path>...]
> @@ -82,6 +83,13 @@ to revert to color all lines according to the outer diff markers
>  	Suppress commits that are missing from the second specified range
>  	(or the "right range" when using the `<rev1>...<rev2>` form).
>  
> +`--matched-only`::
> +	Only emit commits that have a corresponding commit in the other
> +	range, suppressing any commit that exists on only one side. This is
> +	the same as using `--left-only` and `--right-only` together. Useful
> +	to skip added or removed commits when reviewing how the commits
> +	that survived a rebase changed.

While conceptually it is the same as giving "--hide-right-only"
(which would have hidden the right-only entry) and
"--hide-left-only" at the same time, because the existing two
options are not defined in terms of "hiding" entries that have only
one side (which would have logically allowed combining) but instead
showing "only" one side (which makes it impossible to give them
together, and indeed that is the first thing
range-diff.c:show_range_diff() checks and yields an error), this
description is not accurate.

I wonder if the implementation actually can be more like

 - give "--hide-left-only" and "--hide-right-only" as synonyms to
   "--right-only" and "--left-only", and deprecate the original;

 - allow them to be given together, which will give the new
   behaviour you are introducing, i.e., skip steps without both
   sides from the output;

 - give a short-hand synonym, "--matched-only", to truly behave the
   same as giving "--hide-{left,right}-only" together.

which would allow the above explanation to be more accurate?  I
dunno.

> +	if (range_diff_opts->left_only + range_diff_opts->right_only +
> +	    range_diff_opts->matched_only > 1)
> +		res = error(_("options '%s', '%s', or '%s' cannot be used together"),
> +			    "--left-only", "--right-only", "--matched-only");

Don't we have die_for_incompatible_opt3() to do this?

The basic idea sounds good.  The unmatched entries do serve as a
strong hint that a greater --creation-factor may help.  For example,

> +		git range-diff -s --abbrev=7 combined-old...combined-new >actual &&
> +		cat >expect <<-EOF &&
> +		1:  $old_only_oid < -:  ------- c-old-only
> +		-:  ------- > 1:  $new_only_oid c-new-only
> +		2:  $common_old_oid = 2:  $common_new_oid c-common
> +		EOF
> +		test_cmp expect actual &&

the above clearly shows that the command might compare c-old-only
and c-new-only with a better creation factor settings.

But because the entries are numbered, gaps in the numbers, like this
output

> +		git range-diff -s --abbrev=7 --matched-only combined-old...combined-new \
> +			>actual &&
> +		echo "2:  $common_old_oid = 2:  $common_new_oid c-common" >expect &&
> +		test_cmp expect actual

may be sufficient (we can tell that 1 was omitted), except that
somehow we at least need to be aware that there were only 2 commits
on both sides (it may be hiding commits 3 thru 99 as unmatching
pairs and we lose that hint from the new output), which is not a
huge downside.

Thanks.