Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Date: Thu, 29 Nov 2018 11:59:37 +0900	[thread overview]
Message-ID: <xmqq8t1cr3l2.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20181128201852.9782-3-avarab@gmail.com> (=?utf-8?B?IsOGdmFy?= =?utf-8?B?IEFybmZqw7Zyw7A=?= Bjarmason"'s message of "Wed, 28 Nov 2018 21:18:52 +0100")

Æ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.

> @@ -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.

> @@ -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?