Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 29, 2018, 02:59 UTC
- Message-ID
- <xmqq8t1cr3l2.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20181128201852.9782-3-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> + [--range-diff<common diff option>]]
Let's make sure a random string thrown at this mechanism will properly get noticed and diagnosed.
Show 10 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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.
Show 17 quoted lines
> + 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?