Re: [PATCH v2 0/7] making log --first-parent imply -m

[Jeff King <peff@peff.net>]

On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:

> Attached is rather minimal incompatible change to --diff-merges that'd
> allow extensions in the future, to get out of urge for the discussed
> changes. I'm going to follow-up with actual improvements and I'm aware
> it lacks documentation changes.

Thanks, I like the direction here. Definitely it would need
documentation, but also tests (probably in t4013 alongside the ones my
series added; in fact you'd probably need to adjust my tests for the
non-optional argument).

> diff --git a/revision.c b/revision.c
> index 669bc856694f..dcdff59bc36a 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		revs->diff = 1;
>  		revs->diffopt.flags.recursive = 1;
>  		revs->diffopt.flags.tree_in_recursive = 1;
> -	} else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) {
> +	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
>  		revs->ignore_merges = 0;
> +		if (!strcmp(optarg, "off")) {
> +			revs->ignore_merges = 1;
> +		} else if (!strcmp(optarg, "all")) {
> +			revs->diff = 0;

Should this be revs->ignore_merges = 0?

> +		} else if (!strcmp(optarg, "c")) {
> +			revs->diff = 1;
> +			revs->dense_combined_merges = 0;
> +			revs->combine_merges = 1;
> +		} else if (!strcmp(optarg, "cc")) {
> +			revs->diff = 1;
> +			revs->dense_combined_merges = 1;
> +			revs->combine_merges = 1;
> +		} else if (!strcmp(optarg, "combined-all-paths")) {
> +			revs->diff = 1;
> +			revs->combined_all_paths = 1;

I think Junio's suggestion to push these out to a separate patch is a
good one.

It's unfortunate that we have to duplicate all of the various options
that get set (from the "--cc", etc, blocks). But I think the boilerplate
for pushing it into a helper would make it even harder to read.

> +		} else {
> +			die("--diff-merges: unknown value '%s'.", optarg);
> +		}

A few nits:

  - we usually don't have a period at the end of our error messages

  - this should probably be marked for translation, i.e.,
    die(_("translated message"), optarg)

  - I think other similar messages are more like:

      unknown value for --diff-merges: %s

> +		return argcount;
>  	} else if (!strcmp(arg, "--no-diff-merges")) {
>  		revs->ignore_merges = 1;

I thought at first that the --no- form would be handled by
parse_long_opt() via the parseopt code, but it is not using parseopt at
all. :) So it is correct to keep this.

-Peff