Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
From: Junio C Hamano <hidden>
Date: 2018-11-29 02:59:44
Ævar Arnfjörð Bjarmason [off-list ref] writes:
+ [--range-diff<common diff option>]]
Let's make sure a random string thrown at this mechanism will properly get noticed and diagnosed.
quoted hunk
@@ -257,6 +258,13 @@ feeding the result to `git send-email`. creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) for details. +--range-diff<common diff option>:: + Other options prefixed with `--range-diff` are stripped of + that prefix and passed as-is to the diff machinery used to + generate the range-diff, e.g. `--range-diff-U0` and + `--range-diff--no-color`. This allows for adjusting the format + of the range-diff independently from the patch itself.
Taking anything is of course the most general, but I am afraid if this backfires if there are some options that do not make sense to be different between the invocations of range-diff and format-patch.
quoted hunk
@@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) rev.preserve_subject = keep_subject; argc = setup_revisions(argc, argv, &rev, &s_r_opt); - if (argc > 1) - die(_("unrecognized argument: %s"), argv[1]); + if (argc > 1) { + struct argv_array args = ARGV_ARRAY_INIT; + const char *prefix = "--range-diff";
Please call that anything but "prefix" that hides the parameter to the function. const char *range_diff_opt = "--range-diff"; might work OK, or it might not. Let's read on.
+ int have_prefix = 0;
+
+ for (i = 0; i < argc; i++) {
+ struct strbuf sb = STRBUF_INIT;
+ char *str;
+
+ strbuf_addstr(&sb, argv[i]);
+ if (starts_with(argv[i], prefix)) {
+ have_prefix = 1;
+ strbuf_remove(&sb, 0, strlen(prefix));
+ }
+ str = strbuf_detach(&sb, NULL);
+ strbuf_release(&sb);
+
+ argv_array_push(&args, str);
+ }
+Is the body of the loop essentially this? char *passopt = argv[i]; if (!skip_prefix(passopt, range_diff_opt, &passopt)) saw_range_diff_opt = 1; argv_array_push(&args, xstrdup(passopt)); We only use that "prefix" thing once, so we may not even need the variable.
+ if (!have_prefix)
+ die(_("unrecognized argument: %s"), argv[1]);So we take normal options and check the leftover args; if there is no --range-diff<whatever> among the leftover bits, we pretend that we stumbled while reading the first such leftover arg.
+ argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
+ if (argc > 1)
+ die(_("unrecognized argument: %s"), argv[1]);
+ }Otherwise, we pass all the leftover bits, which is a random mixture but guaranteed to have at least one meant for range-diff, to another setup_revisions(). If it leaves a leftover arg, then that is diagnosed here, so we'd be OK (iow, this is not a new "attack vector" to inject random string to command line parser). One minor glitch I can see is "format-patch --range-diffSilly" would report "unrecognised arg: Silly". As we are pretending to be and reporting errors as format-patch, it would be better if we report that --range-diffSilly was what we did not understand.
Junio: I know it's late, but unless Eric has objections to this UI change I'd really like to have this in 2.20 since this is a change to a new command-line UI that's newly added in 2.20.
Quite honestly, I'd rather document "driving range-diff from format-patch is experimental and does silly things when given non-standard options in this release" and not touch the code at this late stage in the game. Would it be less intrusive a change to *not* support the --range-diff<whatever> option, still use rd_rev that is separate from the main rev, and use a reasonable hardcoded default settings when preparing rd_rev?