range-diff: add --matched-only to skip one-sided commits - #2401

"Harald Nordgren via GitGitGadget" <[email protected]> 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.

> 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.

Seems like a big change, and deprecated options are a pain in the neck
because we can never actually remove them.

If we decide to go this way, we might name them "--hide-{left,right}"
and just not introduce a condition that makes them incompatible. Then
"--matched-only" would be pure syntactic sugar and wouldn't even be
100% necessary to have to achieve this.


Harald

Harald Nordgren <[email protected]> writes:

>> 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.
>
> Seems like a big change, and deprecated options are a pain in the neck
> because we can never actually remove them.
>
> If we decide to go this way, we might name them "--hide-{left,right}"
> and just not introduce a condition that makes them incompatible. Then
> "--matched-only" would be pure syntactic sugar and wouldn't even be
> 100% necessary to have to achieve this.

Or we can just keep the code and fix the documentation.  I think
that would be much less impact.

> > Seems like a big change, and deprecated options are a pain in the neck
> > because we can never actually remove them.
> >
> > If we decide to go this way, we might name them "--hide-{left,right}"
> > and just not introduce a condition that makes them incompatible. Then
> > "--matched-only" would be pure syntactic sugar and wouldn't even be
> > 100% necessary to have to achieve this.
>
> Or we can just keep the code and fix the documentation.  I think
> that would be much less impact.

I agree.


Harald

Harald Nordgren <[email protected]> writes:

>> > Seems like a big change, and deprecated options are a pain in the neck
>> > because we can never actually remove them.
>> >
>> > If we decide to go this way, we might name them "--hide-{left,right}"
>> > and just not introduce a condition that makes them incompatible. Then
>> > "--matched-only" would be pure syntactic sugar and wouldn't even be
>> > 100% necessary to have to achieve this.
>>
>> Or we can just keep the code and fix the documentation.  I think
>> that would be much less impact.
>
> I agree.
>
>
> Harald

I thought I'd try my own version, but it seems that we can simply
remove the misleading sentence and the remainder already is very
easy to read and understand ;-)

`--matched-only`::
	Only emit commits that have a corresponding commit in the other
	range, suppressing any commit that exists on only one side.  Useful
	to skip added or removed commits when reviewing how the commits
	that survived a rebase changed.

"Harald Nordgren via GitGitGadget" <[email protected]> writes:

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

Sorry for suggesting this change during the review.  After thinking
about it a bit more, I think it is a bad change.

Top-level range-diff.c should be kept reusable from different
front-ends, some of which may invent different ways to deal with
errors coming from incorrect end-user settings.  It would rob them
of an opportunity to recover if we called die() from here like this.
Calling error() and signaling an error to the caller, as the original
code did, was the right thing to do.

In other words, this adds an unnecessary layering violation, and the
original code was much better from that point of view.

We might eventually want to have error_for_incompatible_optN() so
that this hunk becomes something like:

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

but I think we should leave that outside this topic.

In general, whenever I need to say "something like" (as opposed to
"exactly this") in a suggestion, it is a sign that the overall idea
might be OK, but the details need to be worked out and would take a
lot of time.  I think this is one of those cases that are better
handled as a separate topic without holding this change hostage.

Thanks.