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

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

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> 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.