Re: [PATCH v2 0/7] making log --first-parent imply -m
- From
Jeff King <peff@peff.net>
- Date
- Aug 4, 2020, 19:58 UTC
- Message-ID
- <20200804195830.GA2014743@coredump.intra.peff.net>
- In-Reply-To
- <874kpi47xj.fsf@osv.gnss.ru>
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).
Show 15 quoted lines
> 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?
Show 11 quoted lines
> + } 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