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
