diff mbox series

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

Message ID 20181128201852.9782-3-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/2] format-patch: add test for --range-diff diff output | expand

Commit Message

Ævar Arnfjörð Bjarmason Nov. 28, 2018, 8:18 p.m. UTC
Change the semantics of the "--range-diff" option so that the regular

Comments

Junio C Hamano Nov. 29, 2018, 2:59 a.m. UTC | #1
Æ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?
Johannes Schindelin Nov. 29, 2018, 10:07 a.m. UTC | #2
Hi Ævar,

On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> Change the semantics of the "--range-diff" option so that the regular
> diff options can be provided separately for the range-diff and the
> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> "format-patch" to provide different context for the range-diff and the
> patch. This wasn't possible before.

I really, really dislike the `--range-diff-<random-thing>`. We have
precedent for passing optional arguments that are passed to some other
command, so a much more logical and consistent convention would be to use
`--range-diff[=<diff-option>..]`, allowing all of the diff options that
you might want to pass to the outer diff in one go rather than having a
lengthy string of `--range-diff-this` and `--range-diff-that` options.

I only had time to skim the patch, and I have to wonder why you pass
around full-blown `rev_info` structs for range diff (and with that really
awful name `rd_rev`) rather than just the `diff_options` that you
*actually* care about?

Ciao,
Dscho

> 
> Ever since the "--range-diff" option was added in
> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
> machinery has been the one we get from "format-patch"'s own
> setup_revisions().
> 
> This sort of thing is unique among the log-like commands in
> builtin/log.c, no command than format-patch will embed the output of
> another log-like command. Since the "rev->diffopt" is reused we need
> to munge it before we pass it to show_range_diff(). See
> 43dafc4172 ("format-patch: don't include --stat with --range-diff
> output", 2018-11-22) for a related regression fix which is being
> mostly reverted here.
> 
> Implementation notes: 1) We're not bothering with the full teardown
> around die() and will leak memory, but it's too much boilerplate to do
> all the frees with/without the die() and not worth it. 2) We call
> repo_init_revisions() for "rd_rev" even though we could get away with
> a shallow copy like the code we're replacing (and which
> show_range_diff() itself does). This is to make this code more easily
> understood.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/git-format-patch.txt | 10 ++++++-
>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index aba4c5febe..6c048f415f 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -24,7 +24,8 @@ SYNOPSIS
>  		   [--to=<email>] [--cc=<email>]
>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
>  		   [--interdiff=<previous>]
> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
> +		   [--range-diff=<previous> [--creation-factor=<percent>]
> +		      [--range-diff<common diff option>]]
>  		   [--progress]
>  		   [<common diff options>]
>  		   [ <since> | <revision range> ]
> @@ -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.
> +
>  --notes[=<ref>]::
>  	Append the notes (see linkgit:git-notes[1]) for the commit
>  	after the three-dash line.
> diff --git a/builtin/log.c b/builtin/log.c
> index 02d88fa233..7658e56ecc 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
>  	fprintf(rev->diffopt.file, "\n");
>  }
>  
> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
> +			      int use_stdout,
>  			      struct commit *origin,
>  			      int nr, struct commit **list,
>  			      const char *branch_name,
> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	}
>  
>  	if (rev->rdiff1) {
> -		struct diff_options opts;
> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
> -
>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>  		show_range_diff(rev->rdiff1, rev->rdiff2,
> -				rev->creation_factor, 1, &opts);
> +				rev->creation_factor, 1, &rd_rev->diffopt);
>  	}
>  }
>  
> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	struct commit *commit;
>  	struct commit **list = NULL;
>  	struct rev_info rev;
> +	struct rev_info rd_rev;
>  	struct setup_revision_opt s_r_opt;
>  	int nr = 0, total, i;
>  	int use_stdout = 0;
> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	init_log_defaults();
>  	git_config(git_format_config, NULL);
>  	repo_init_revisions(the_repository, &rev, prefix);
> +	repo_init_revisions(the_repository, &rd_rev, prefix);
>  	rev.commit_format = CMIT_FMT_EMAIL;
>  	rev.expand_tabs_in_log_default = 0;
>  	rev.verbose_header = 1;
> @@ -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";
> +		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);
> +		}
> +
> +		if (!have_prefix)
> +			die(_("unrecognized argument: %s"), argv[1]);
> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
> +		if (argc > 1)
> +			die(_("unrecognized argument: %s"), argv[1]);
> +	}
>  
>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
>  		die(_("--name-only does not make sense"));
> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (!use_patch_format &&
>  		(!rev.diffopt.output_format ||
>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> -		/* Needs to be mirrored in show_range_diff() invocation */
>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
>  	if (!rev.diffopt.stat_width)
>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (cover_letter) {
>  		if (thread)
>  			gen_message_id(&rev, "cover");
> -		make_cover_letter(&rev, use_stdout,
> +		make_cover_letter(&rev, &rd_rev, use_stdout,
>  				  origin, nr, list, branch_name, quiet);
>  		print_bases(&bases, rev.diffopt.file);
>  		print_signature(rev.diffopt.file);
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index bc5facc1cd..6916103888 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>  	sed -e "s|:$||" >expect <<-\EOF &&
> +	1:  a63e992 ! 1:  d966c5c s/12/B/
> +	    @@ -8,7 +8,7 @@
> +	     @@
> +	      9
> +	      10
> +	    - B
> +	    + BB
> +	     -12
> +	     +B
> +	      13
> +	-- :
> +	EOF
> +	test_cmp expect actual.range-diff &&
> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
> +	sed -e "s|:$||" >expect <<-\EOF &&
> +	--- a/file
> +	+++ b/file
> +	@@ -12 +12 @@ BB
> +	-12
> +	+B
> +	-- :
> +	EOF
> +	test_cmp expect actual.diff &&
> +
> +	# -U0 & --range-diff-U0
> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> +	sed -e "s|:$||" >expect <<-\EOF &&
>  	1:  a63e992 ! 1:  d966c5c s/12/B/
>  	    @@ -11 +11 @@
>  	    - B
> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
>  	test_cmp expect actual.diff
>  '
>  
> +test_expect_success 'format-patch option parsing with --range-diff-*' '
> +	test_must_fail git format-patch --stdout --unknown \
> +		master..unmodified 2>stderr &&
> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
> +	test_must_fail git format-patch --stdout --range-diff-unknown \
> +		master..unmodified 2>stderr &&
> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
> +		master..unmodified 2>stderr &&
> +	test_i18ngrep "unrecognized argument: --unknown" stderr
> +'
> +
>  test_done
> -- 
> 2.20.0.rc1.387.gf8505762e3
> 
>
Ævar Arnfjörð Bjarmason Nov. 29, 2018, 10:30 a.m. UTC | #3
On Thu, Nov 29 2018, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> Change the semantics of the "--range-diff" option so that the regular
>> diff options can be provided separately for the range-diff and the
>> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
>> "format-patch" to provide different context for the range-diff and the
>> patch. This wasn't possible before.
>
> I really, really dislike the `--range-diff-<random-thing>`. We have
> precedent for passing optional arguments that are passed to some other
> command, so a much more logical and consistent convention would be to use
> `--range-diff[=<diff-option>..]`, allowing all of the diff options that
> you might want to pass to the outer diff in one go rather than having a
> lengthy string of `--range-diff-this` and `--range-diff-that` options.

Where do we pass those sorts of arguments?

Reasons I did it this way:

 a) Passing it as one option will require the user to double-quote those
    options that take quoted arguments (e.g. --word-diff-regex), which I
    thought sucked more than the prefix. On the implementation side we
    couldn't leave the parsing of the command-line to the shell anymore.

 b) I think people will want to tweak this very rarely, much more rarely
    than e.g. -U10 in format-patch itself, so having something long-ish
    doesn't sound bad.

> I only had time to skim the patch, and I have to wonder why you pass
> around full-blown `rev_info` structs for range diff (and with that really
> awful name `rd_rev`) rather than just the `diff_options` that you
> *actually* care about?

Because setup_revisions() which does all the command-line parsing needs
a rev_info, so even if we only need the diffopt in the end we need to
initiate the whole thing.

Suggestions for a better varibale name most welcome.

> Ciao,
> Dscho
>
>>
>> Ever since the "--range-diff" option was added in
>> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
>> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
>> machinery has been the one we get from "format-patch"'s own
>> setup_revisions().
>>
>> This sort of thing is unique among the log-like commands in
>> builtin/log.c, no command than format-patch will embed the output of
>> another log-like command. Since the "rev->diffopt" is reused we need
>> to munge it before we pass it to show_range_diff(). See
>> 43dafc4172 ("format-patch: don't include --stat with --range-diff
>> output", 2018-11-22) for a related regression fix which is being
>> mostly reverted here.
>>
>> Implementation notes: 1) We're not bothering with the full teardown
>> around die() and will leak memory, but it's too much boilerplate to do
>> all the frees with/without the die() and not worth it. 2) We call
>> repo_init_revisions() for "rd_rev" even though we could get away with
>> a shallow copy like the code we're replacing (and which
>> show_range_diff() itself does). This is to make this code more easily
>> understood.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  Documentation/git-format-patch.txt | 10 ++++++-
>>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
>>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
>>  3 files changed, 82 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
>> index aba4c5febe..6c048f415f 100644
>> --- a/Documentation/git-format-patch.txt
>> +++ b/Documentation/git-format-patch.txt
>> @@ -24,7 +24,8 @@ SYNOPSIS
>>  		   [--to=<email>] [--cc=<email>]
>>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
>>  		   [--interdiff=<previous>]
>> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
>> +		   [--range-diff=<previous> [--creation-factor=<percent>]
>> +		      [--range-diff<common diff option>]]
>>  		   [--progress]
>>  		   [<common diff options>]
>>  		   [ <since> | <revision range> ]
>> @@ -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.
>> +
>>  --notes[=<ref>]::
>>  	Append the notes (see linkgit:git-notes[1]) for the commit
>>  	after the three-dash line.
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 02d88fa233..7658e56ecc 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
>>  	fprintf(rev->diffopt.file, "\n");
>>  }
>>
>> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
>> +			      int use_stdout,
>>  			      struct commit *origin,
>>  			      int nr, struct commit **list,
>>  			      const char *branch_name,
>> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>>  	}
>>
>>  	if (rev->rdiff1) {
>> -		struct diff_options opts;
>> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
>> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>> -
>>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>>  		show_range_diff(rev->rdiff1, rev->rdiff2,
>> -				rev->creation_factor, 1, &opts);
>> +				rev->creation_factor, 1, &rd_rev->diffopt);
>>  	}
>>  }
>>
>> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	struct commit *commit;
>>  	struct commit **list = NULL;
>>  	struct rev_info rev;
>> +	struct rev_info rd_rev;
>>  	struct setup_revision_opt s_r_opt;
>>  	int nr = 0, total, i;
>>  	int use_stdout = 0;
>> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	init_log_defaults();
>>  	git_config(git_format_config, NULL);
>>  	repo_init_revisions(the_repository, &rev, prefix);
>> +	repo_init_revisions(the_repository, &rd_rev, prefix);
>>  	rev.commit_format = CMIT_FMT_EMAIL;
>>  	rev.expand_tabs_in_log_default = 0;
>>  	rev.verbose_header = 1;
>> @@ -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";
>> +		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);
>> +		}
>> +
>> +		if (!have_prefix)
>> +			die(_("unrecognized argument: %s"), argv[1]);
>> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
>> +		if (argc > 1)
>> +			die(_("unrecognized argument: %s"), argv[1]);
>> +	}
>>
>>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
>>  		die(_("--name-only does not make sense"));
>> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	if (!use_patch_format &&
>>  		(!rev.diffopt.output_format ||
>>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
>> -		/* Needs to be mirrored in show_range_diff() invocation */
>>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
>>  	if (!rev.diffopt.stat_width)
>>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
>> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	if (cover_letter) {
>>  		if (thread)
>>  			gen_message_id(&rev, "cover");
>> -		make_cover_letter(&rev, use_stdout,
>> +		make_cover_letter(&rev, &rd_rev, use_stdout,
>>  				  origin, nr, list, branch_name, quiet);
>>  		print_bases(&bases, rev.diffopt.file);
>>  		print_signature(rev.diffopt.file);
>> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> index bc5facc1cd..6916103888 100755
>> --- a/t/t3206-range-diff.sh
>> +++ b/t/t3206-range-diff.sh
>> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
>>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
>>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>>  	sed -e "s|:$||" >expect <<-\EOF &&
>> +	1:  a63e992 ! 1:  d966c5c s/12/B/
>> +	    @@ -8,7 +8,7 @@
>> +	     @@
>> +	      9
>> +	      10
>> +	    - B
>> +	    + BB
>> +	     -12
>> +	     +B
>> +	      13
>> +	-- :
>> +	EOF
>> +	test_cmp expect actual.range-diff &&
>> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
>> +	sed -e "s|:$||" >expect <<-\EOF &&
>> +	--- a/file
>> +	+++ b/file
>> +	@@ -12 +12 @@ BB
>> +	-12
>> +	+B
>> +	-- :
>> +	EOF
>> +	test_cmp expect actual.diff &&
>> +
>> +	# -U0 & --range-diff-U0
>> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
>> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
>> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>> +	sed -e "s|:$||" >expect <<-\EOF &&
>>  	1:  a63e992 ! 1:  d966c5c s/12/B/
>>  	    @@ -11 +11 @@
>>  	    - B
>> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
>>  	test_cmp expect actual.diff
>>  '
>>
>> +test_expect_success 'format-patch option parsing with --range-diff-*' '
>> +	test_must_fail git format-patch --stdout --unknown \
>> +		master..unmodified 2>stderr &&
>> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
>> +	test_must_fail git format-patch --stdout --range-diff-unknown \
>> +		master..unmodified 2>stderr &&
>> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
>> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
>> +		master..unmodified 2>stderr &&
>> +	test_i18ngrep "unrecognized argument: --unknown" stderr
>> +'
>> +
>>  test_done
>> --
>> 2.20.0.rc1.387.gf8505762e3
>>
>>
Johannes Schindelin Nov. 29, 2018, 12:12 p.m. UTC | #4
Hi Ævar,

On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> 
> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> Change the semantics of the "--range-diff" option so that the regular
> >> diff options can be provided separately for the range-diff and the
> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> >> "format-patch" to provide different context for the range-diff and the
> >> patch. This wasn't possible before.
> >
> > I really, really dislike the `--range-diff-<random-thing>`. We have
> > precedent for passing optional arguments that are passed to some other
> > command, so a much more logical and consistent convention would be to use
> > `--range-diff[=<diff-option>..]`, allowing all of the diff options that
> > you might want to pass to the outer diff in one go rather than having a
> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
> 
> Where do we pass those sorts of arguments?
> 
> Reasons I did it this way:
> 
>  a) Passing it as one option will require the user to double-quote those
>     options that take quoted arguments (e.g. --word-diff-regex), which I
>     thought sucked more than the prefix. On the implementation side we
>     couldn't leave the parsing of the command-line to the shell anymore.
> 
>  b) I think people will want to tweak this very rarely, much more rarely
>     than e.g. -U10 in format-patch itself, so having something long-ish
>     doesn't sound bad.

Hmm. I still don't like it. It sets a precedent, and we simply do not do
it that way in other circumstances (most obvious would be the -X merge
options). The more divergent user interfaces for the same sort of thing
are, the more brain cycles you force users to spend on navigating said
interfaces.

> > I only had time to skim the patch, and I have to wonder why you pass
> > around full-blown `rev_info` structs for range diff (and with that really
> > awful name `rd_rev`) rather than just the `diff_options` that you
> > *actually* care about?
> 
> Because setup_revisions() which does all the command-line parsing needs
> a rev_info, so even if we only need the diffopt in the end we need to
> initiate the whole thing.
> 
> Suggestions for a better varibale name most welcome.

`range_diff_revs`

And you do not need to pass around the whole thing. You can easily pass
`&range_diff_revs.diffopt`.

Don't pass around what you do not need to pass around.

Ciao,
Dscho

> 
> > Ciao,
> > Dscho
> >
> >>
> >> Ever since the "--range-diff" option was added in
> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
> >> machinery has been the one we get from "format-patch"'s own
> >> setup_revisions().
> >>
> >> This sort of thing is unique among the log-like commands in
> >> builtin/log.c, no command than format-patch will embed the output of
> >> another log-like command. Since the "rev->diffopt" is reused we need
> >> to munge it before we pass it to show_range_diff(). See
> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff
> >> output", 2018-11-22) for a related regression fix which is being
> >> mostly reverted here.
> >>
> >> Implementation notes: 1) We're not bothering with the full teardown
> >> around die() and will leak memory, but it's too much boilerplate to do
> >> all the frees with/without the die() and not worth it. 2) We call
> >> repo_init_revisions() for "rd_rev" even though we could get away with
> >> a shallow copy like the code we're replacing (and which
> >> show_range_diff() itself does). This is to make this code more easily
> >> understood.
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >>  Documentation/git-format-patch.txt | 10 ++++++-
> >>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
> >>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
> >>  3 files changed, 82 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> >> index aba4c5febe..6c048f415f 100644
> >> --- a/Documentation/git-format-patch.txt
> >> +++ b/Documentation/git-format-patch.txt
> >> @@ -24,7 +24,8 @@ SYNOPSIS
> >>  		   [--to=<email>] [--cc=<email>]
> >>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
> >>  		   [--interdiff=<previous>]
> >> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
> >> +		   [--range-diff=<previous> [--creation-factor=<percent>]
> >> +		      [--range-diff<common diff option>]]
> >>  		   [--progress]
> >>  		   [<common diff options>]
> >>  		   [ <since> | <revision range> ]
> >> @@ -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.
> >> +
> >>  --notes[=<ref>]::
> >>  	Append the notes (see linkgit:git-notes[1]) for the commit
> >>  	after the three-dash line.
> >> diff --git a/builtin/log.c b/builtin/log.c
> >> index 02d88fa233..7658e56ecc 100644
> >> --- a/builtin/log.c
> >> +++ b/builtin/log.c
> >> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
> >>  	fprintf(rev->diffopt.file, "\n");
> >>  }
> >>
> >> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
> >> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
> >> +			      int use_stdout,
> >>  			      struct commit *origin,
> >>  			      int nr, struct commit **list,
> >>  			      const char *branch_name,
> >> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
> >>  	}
> >>
> >>  	if (rev->rdiff1) {
> >> -		struct diff_options opts;
> >> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
> >> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
> >> -
> >>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
> >>  		show_range_diff(rev->rdiff1, rev->rdiff2,
> >> -				rev->creation_factor, 1, &opts);
> >> +				rev->creation_factor, 1, &rd_rev->diffopt);
> >>  	}
> >>  }
> >>
> >> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >>  	struct commit *commit;
> >>  	struct commit **list = NULL;
> >>  	struct rev_info rev;
> >> +	struct rev_info rd_rev;
> >>  	struct setup_revision_opt s_r_opt;
> >>  	int nr = 0, total, i;
> >>  	int use_stdout = 0;
> >> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >>  	init_log_defaults();
> >>  	git_config(git_format_config, NULL);
> >>  	repo_init_revisions(the_repository, &rev, prefix);
> >> +	repo_init_revisions(the_repository, &rd_rev, prefix);
> >>  	rev.commit_format = CMIT_FMT_EMAIL;
> >>  	rev.expand_tabs_in_log_default = 0;
> >>  	rev.verbose_header = 1;
> >> @@ -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";
> >> +		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);
> >> +		}
> >> +
> >> +		if (!have_prefix)
> >> +			die(_("unrecognized argument: %s"), argv[1]);
> >> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
> >> +		if (argc > 1)
> >> +			die(_("unrecognized argument: %s"), argv[1]);
> >> +	}
> >>
> >>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
> >>  		die(_("--name-only does not make sense"));
> >> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >>  	if (!use_patch_format &&
> >>  		(!rev.diffopt.output_format ||
> >>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> >> -		/* Needs to be mirrored in show_range_diff() invocation */
> >>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
> >>  	if (!rev.diffopt.stat_width)
> >>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
> >> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >>  	if (cover_letter) {
> >>  		if (thread)
> >>  			gen_message_id(&rev, "cover");
> >> -		make_cover_letter(&rev, use_stdout,
> >> +		make_cover_letter(&rev, &rd_rev, use_stdout,
> >>  				  origin, nr, list, branch_name, quiet);
> >>  		print_bases(&bases, rev.diffopt.file);
> >>  		print_signature(rev.diffopt.file);
> >> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> >> index bc5facc1cd..6916103888 100755
> >> --- a/t/t3206-range-diff.sh
> >> +++ b/t/t3206-range-diff.sh
> >> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
> >>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
> >>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> >>  	sed -e "s|:$||" >expect <<-\EOF &&
> >> +	1:  a63e992 ! 1:  d966c5c s/12/B/
> >> +	    @@ -8,7 +8,7 @@
> >> +	     @@
> >> +	      9
> >> +	      10
> >> +	    - B
> >> +	    + BB
> >> +	     -12
> >> +	     +B
> >> +	      13
> >> +	-- :
> >> +	EOF
> >> +	test_cmp expect actual.range-diff &&
> >> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
> >> +	sed -e "s|:$||" >expect <<-\EOF &&
> >> +	--- a/file
> >> +	+++ b/file
> >> +	@@ -12 +12 @@ BB
> >> +	-12
> >> +	+B
> >> +	-- :
> >> +	EOF
> >> +	test_cmp expect actual.diff &&
> >> +
> >> +	# -U0 & --range-diff-U0
> >> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
> >> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
> >> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> >> +	sed -e "s|:$||" >expect <<-\EOF &&
> >>  	1:  a63e992 ! 1:  d966c5c s/12/B/
> >>  	    @@ -11 +11 @@
> >>  	    - B
> >> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
> >>  	test_cmp expect actual.diff
> >>  '
> >>
> >> +test_expect_success 'format-patch option parsing with --range-diff-*' '
> >> +	test_must_fail git format-patch --stdout --unknown \
> >> +		master..unmodified 2>stderr &&
> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
> >> +	test_must_fail git format-patch --stdout --range-diff-unknown \
> >> +		master..unmodified 2>stderr &&
> >> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
> >> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
> >> +		master..unmodified 2>stderr &&
> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr
> >> +'
> >> +
> >>  test_done
> >> --
> >> 2.20.0.rc1.387.gf8505762e3
> >>
> >>
>
Ævar Arnfjörð Bjarmason Nov. 29, 2018, 2:35 p.m. UTC | #5
On Thu, Nov 29 2018, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> On Thu, Nov 29 2018, Johannes Schindelin wrote:
>>
>> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> Change the semantics of the "--range-diff" option so that the regular
>> >> diff options can be provided separately for the range-diff and the
>> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
>> >> "format-patch" to provide different context for the range-diff and the
>> >> patch. This wasn't possible before.
>> >
>> > I really, really dislike the `--range-diff-<random-thing>`. We have
>> > precedent for passing optional arguments that are passed to some other
>> > command, so a much more logical and consistent convention would be to use
>> > `--range-diff[=<diff-option>..]`, allowing all of the diff options that
>> > you might want to pass to the outer diff in one go rather than having a
>> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
>>
>> Where do we pass those sorts of arguments?
>>
>> Reasons I did it this way:
>>
>>  a) Passing it as one option will require the user to double-quote those
>>     options that take quoted arguments (e.g. --word-diff-regex), which I
>>     thought sucked more than the prefix. On the implementation side we
>>     couldn't leave the parsing of the command-line to the shell anymore.
>>
>>  b) I think people will want to tweak this very rarely, much more rarely
>>     than e.g. -U10 in format-patch itself, so having something long-ish
>>     doesn't sound bad.
>
> Hmm. I still don't like it. It sets a precedent, and we simply do not do
> it that way in other circumstances (most obvious would be the -X merge
> options). The more divergent user interfaces for the same sort of thing
> are, the more brain cycles you force users to spend on navigating said
> interfaces.

Yeah it sucks, I just think it sucks less than the alternative :)
I.e. I'm not picky about --range-diff-* prefix the name, but I think
doing our own shell parsing would be nasty.

>> > I only had time to skim the patch, and I have to wonder why you pass
>> > around full-blown `rev_info` structs for range diff (and with that really
>> > awful name `rd_rev`) rather than just the `diff_options` that you
>> > *actually* care about?
>>
>> Because setup_revisions() which does all the command-line parsing needs
>> a rev_info, so even if we only need the diffopt in the end we need to
>> initiate the whole thing.
>>
>> Suggestions for a better varibale name most welcome.
>
> `range_diff_revs`
>
> And you do not need to pass around the whole thing. You can easily pass
> `&range_diff_revs.diffopt`.
>
> Don't pass around what you do not need to pass around.

Ah, you mean internally in log.c, yes that makes sense. I thought you
meant just pass diffopt to setup_revisions() (which needs the containing
struct). Willdo.

> Ciao,
> Dscho
>
>>
>> > Ciao,
>> > Dscho
>> >
>> >>
>> >> Ever since the "--range-diff" option was added in
>> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
>> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
>> >> machinery has been the one we get from "format-patch"'s own
>> >> setup_revisions().
>> >>
>> >> This sort of thing is unique among the log-like commands in
>> >> builtin/log.c, no command than format-patch will embed the output of
>> >> another log-like command. Since the "rev->diffopt" is reused we need
>> >> to munge it before we pass it to show_range_diff(). See
>> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff
>> >> output", 2018-11-22) for a related regression fix which is being
>> >> mostly reverted here.
>> >>
>> >> Implementation notes: 1) We're not bothering with the full teardown
>> >> around die() and will leak memory, but it's too much boilerplate to do
>> >> all the frees with/without the die() and not worth it. 2) We call
>> >> repo_init_revisions() for "rd_rev" even though we could get away with
>> >> a shallow copy like the code we're replacing (and which
>> >> show_range_diff() itself does). This is to make this code more easily
>> >> understood.
>> >>
>> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >> ---
>> >>  Documentation/git-format-patch.txt | 10 ++++++-
>> >>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
>> >>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
>> >>  3 files changed, 82 insertions(+), 11 deletions(-)
>> >>
>> >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
>> >> index aba4c5febe..6c048f415f 100644
>> >> --- a/Documentation/git-format-patch.txt
>> >> +++ b/Documentation/git-format-patch.txt
>> >> @@ -24,7 +24,8 @@ SYNOPSIS
>> >>  		   [--to=<email>] [--cc=<email>]
>> >>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
>> >>  		   [--interdiff=<previous>]
>> >> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
>> >> +		   [--range-diff=<previous> [--creation-factor=<percent>]
>> >> +		      [--range-diff<common diff option>]]
>> >>  		   [--progress]
>> >>  		   [<common diff options>]
>> >>  		   [ <since> | <revision range> ]
>> >> @@ -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.
>> >> +
>> >>  --notes[=<ref>]::
>> >>  	Append the notes (see linkgit:git-notes[1]) for the commit
>> >>  	after the three-dash line.
>> >> diff --git a/builtin/log.c b/builtin/log.c
>> >> index 02d88fa233..7658e56ecc 100644
>> >> --- a/builtin/log.c
>> >> +++ b/builtin/log.c
>> >> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
>> >>  	fprintf(rev->diffopt.file, "\n");
>> >>  }
>> >>
>> >> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> >> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
>> >> +			      int use_stdout,
>> >>  			      struct commit *origin,
>> >>  			      int nr, struct commit **list,
>> >>  			      const char *branch_name,
>> >> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> >>  	}
>> >>
>> >>  	if (rev->rdiff1) {
>> >> -		struct diff_options opts;
>> >> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
>> >> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>> >> -
>> >>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>> >>  		show_range_diff(rev->rdiff1, rev->rdiff2,
>> >> -				rev->creation_factor, 1, &opts);
>> >> +				rev->creation_factor, 1, &rd_rev->diffopt);
>> >>  	}
>> >>  }
>> >>
>> >> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >>  	struct commit *commit;
>> >>  	struct commit **list = NULL;
>> >>  	struct rev_info rev;
>> >> +	struct rev_info rd_rev;
>> >>  	struct setup_revision_opt s_r_opt;
>> >>  	int nr = 0, total, i;
>> >>  	int use_stdout = 0;
>> >> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >>  	init_log_defaults();
>> >>  	git_config(git_format_config, NULL);
>> >>  	repo_init_revisions(the_repository, &rev, prefix);
>> >> +	repo_init_revisions(the_repository, &rd_rev, prefix);
>> >>  	rev.commit_format = CMIT_FMT_EMAIL;
>> >>  	rev.expand_tabs_in_log_default = 0;
>> >>  	rev.verbose_header = 1;
>> >> @@ -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";
>> >> +		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);
>> >> +		}
>> >> +
>> >> +		if (!have_prefix)
>> >> +			die(_("unrecognized argument: %s"), argv[1]);
>> >> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
>> >> +		if (argc > 1)
>> >> +			die(_("unrecognized argument: %s"), argv[1]);
>> >> +	}
>> >>
>> >>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
>> >>  		die(_("--name-only does not make sense"));
>> >> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >>  	if (!use_patch_format &&
>> >>  		(!rev.diffopt.output_format ||
>> >>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
>> >> -		/* Needs to be mirrored in show_range_diff() invocation */
>> >>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
>> >>  	if (!rev.diffopt.stat_width)
>> >>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
>> >> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >>  	if (cover_letter) {
>> >>  		if (thread)
>> >>  			gen_message_id(&rev, "cover");
>> >> -		make_cover_letter(&rev, use_stdout,
>> >> +		make_cover_letter(&rev, &rd_rev, use_stdout,
>> >>  				  origin, nr, list, branch_name, quiet);
>> >>  		print_bases(&bases, rev.diffopt.file);
>> >>  		print_signature(rev.diffopt.file);
>> >> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> >> index bc5facc1cd..6916103888 100755
>> >> --- a/t/t3206-range-diff.sh
>> >> +++ b/t/t3206-range-diff.sh
>> >> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
>> >>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
>> >>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>> >>  	sed -e "s|:$||" >expect <<-\EOF &&
>> >> +	1:  a63e992 ! 1:  d966c5c s/12/B/
>> >> +	    @@ -8,7 +8,7 @@
>> >> +	     @@
>> >> +	      9
>> >> +	      10
>> >> +	    - B
>> >> +	    + BB
>> >> +	     -12
>> >> +	     +B
>> >> +	      13
>> >> +	-- :
>> >> +	EOF
>> >> +	test_cmp expect actual.range-diff &&
>> >> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
>> >> +	sed -e "s|:$||" >expect <<-\EOF &&
>> >> +	--- a/file
>> >> +	+++ b/file
>> >> +	@@ -12 +12 @@ BB
>> >> +	-12
>> >> +	+B
>> >> +	-- :
>> >> +	EOF
>> >> +	test_cmp expect actual.diff &&
>> >> +
>> >> +	# -U0 & --range-diff-U0
>> >> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
>> >> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
>> >> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>> >> +	sed -e "s|:$||" >expect <<-\EOF &&
>> >>  	1:  a63e992 ! 1:  d966c5c s/12/B/
>> >>  	    @@ -11 +11 @@
>> >>  	    - B
>> >> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
>> >>  	test_cmp expect actual.diff
>> >>  '
>> >>
>> >> +test_expect_success 'format-patch option parsing with --range-diff-*' '
>> >> +	test_must_fail git format-patch --stdout --unknown \
>> >> +		master..unmodified 2>stderr &&
>> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
>> >> +	test_must_fail git format-patch --stdout --range-diff-unknown \
>> >> +		master..unmodified 2>stderr &&
>> >> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
>> >> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
>> >> +		master..unmodified 2>stderr &&
>> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr
>> >> +'
>> >> +
>> >>  test_done
>> >> --
>> >> 2.20.0.rc1.387.gf8505762e3
>> >>
>> >>
>>
Johannes Schindelin Nov. 29, 2018, 3:41 p.m. UTC | #6
Hi Ævar,

On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> 
> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> >>
> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >> >
> >> >> Change the semantics of the "--range-diff" option so that the regular
> >> >> diff options can be provided separately for the range-diff and the
> >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> >> >> "format-patch" to provide different context for the range-diff and the
> >> >> patch. This wasn't possible before.
> >> >
> >> > I really, really dislike the `--range-diff-<random-thing>`. We have
> >> > precedent for passing optional arguments that are passed to some other
> >> > command, so a much more logical and consistent convention would be to use
> >> > `--range-diff[=<diff-option>..]`, allowing all of the diff options that
> >> > you might want to pass to the outer diff in one go rather than having a
> >> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
> >>
> >> Where do we pass those sorts of arguments?
> >>
> >> Reasons I did it this way:
> >>
> >>  a) Passing it as one option will require the user to double-quote those
> >>     options that take quoted arguments (e.g. --word-diff-regex), which I
> >>     thought sucked more than the prefix. On the implementation side we
> >>     couldn't leave the parsing of the command-line to the shell anymore.
> >>
> >>  b) I think people will want to tweak this very rarely, much more rarely
> >>     than e.g. -U10 in format-patch itself, so having something long-ish
> >>     doesn't sound bad.
> >
> > Hmm. I still don't like it. It sets a precedent, and we simply do not do
> > it that way in other circumstances (most obvious would be the -X merge
> > options). The more divergent user interfaces for the same sort of thing
> > are, the more brain cycles you force users to spend on navigating said
> > interfaces.
> 
> Yeah it sucks, I just think it sucks less than the alternative :)
> I.e. I'm not picky about --range-diff-* prefix the name, but I think
> doing our own shell parsing would be nasty.

What prevents you from using `sq_dequote_to_argv()`?

> >> > I only had time to skim the patch, and I have to wonder why you pass
> >> > around full-blown `rev_info` structs for range diff (and with that really
> >> > awful name `rd_rev`) rather than just the `diff_options` that you
> >> > *actually* care about?
> >>
> >> Because setup_revisions() which does all the command-line parsing needs
> >> a rev_info, so even if we only need the diffopt in the end we need to
> >> initiate the whole thing.
> >>
> >> Suggestions for a better varibale name most welcome.
> >
> > `range_diff_revs`
> >
> > And you do not need to pass around the whole thing. You can easily pass
> > `&range_diff_revs.diffopt`.
> >
> > Don't pass around what you do not need to pass around.
> 
> Ah, you mean internally in log.c, yes that makes sense. I thought you
> meant just pass diffopt to setup_revisions() (which needs the containing
> struct). Willdo.

Thanks,
Dscho

> 
> > Ciao,
> > Dscho
> >
> >>
> >> > Ciao,
> >> > Dscho
> >> >
> >> >>
> >> >> Ever since the "--range-diff" option was added in
> >> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
> >> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
> >> >> machinery has been the one we get from "format-patch"'s own
> >> >> setup_revisions().
> >> >>
> >> >> This sort of thing is unique among the log-like commands in
> >> >> builtin/log.c, no command than format-patch will embed the output of
> >> >> another log-like command. Since the "rev->diffopt" is reused we need
> >> >> to munge it before we pass it to show_range_diff(). See
> >> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff
> >> >> output", 2018-11-22) for a related regression fix which is being
> >> >> mostly reverted here.
> >> >>
> >> >> Implementation notes: 1) We're not bothering with the full teardown
> >> >> around die() and will leak memory, but it's too much boilerplate to do
> >> >> all the frees with/without the die() and not worth it. 2) We call
> >> >> repo_init_revisions() for "rd_rev" even though we could get away with
> >> >> a shallow copy like the code we're replacing (and which
> >> >> show_range_diff() itself does). This is to make this code more easily
> >> >> understood.
> >> >>
> >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> >> ---
> >> >>  Documentation/git-format-patch.txt | 10 ++++++-
> >> >>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
> >> >>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
> >> >>  3 files changed, 82 insertions(+), 11 deletions(-)
> >> >>
> >> >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> >> >> index aba4c5febe..6c048f415f 100644
> >> >> --- a/Documentation/git-format-patch.txt
> >> >> +++ b/Documentation/git-format-patch.txt
> >> >> @@ -24,7 +24,8 @@ SYNOPSIS
> >> >>  		   [--to=<email>] [--cc=<email>]
> >> >>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
> >> >>  		   [--interdiff=<previous>]
> >> >> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
> >> >> +		   [--range-diff=<previous> [--creation-factor=<percent>]
> >> >> +		      [--range-diff<common diff option>]]
> >> >>  		   [--progress]
> >> >>  		   [<common diff options>]
> >> >>  		   [ <since> | <revision range> ]
> >> >> @@ -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.
> >> >> +
> >> >>  --notes[=<ref>]::
> >> >>  	Append the notes (see linkgit:git-notes[1]) for the commit
> >> >>  	after the three-dash line.
> >> >> diff --git a/builtin/log.c b/builtin/log.c
> >> >> index 02d88fa233..7658e56ecc 100644
> >> >> --- a/builtin/log.c
> >> >> +++ b/builtin/log.c
> >> >> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
> >> >>  	fprintf(rev->diffopt.file, "\n");
> >> >>  }
> >> >>
> >> >> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
> >> >> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
> >> >> +			      int use_stdout,
> >> >>  			      struct commit *origin,
> >> >>  			      int nr, struct commit **list,
> >> >>  			      const char *branch_name,
> >> >> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
> >> >>  	}
> >> >>
> >> >>  	if (rev->rdiff1) {
> >> >> -		struct diff_options opts;
> >> >> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
> >> >> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
> >> >> -
> >> >>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
> >> >>  		show_range_diff(rev->rdiff1, rev->rdiff2,
> >> >> -				rev->creation_factor, 1, &opts);
> >> >> +				rev->creation_factor, 1, &rd_rev->diffopt);
> >> >>  	}
> >> >>  }
> >> >>
> >> >> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >> >>  	struct commit *commit;
> >> >>  	struct commit **list = NULL;
> >> >>  	struct rev_info rev;
> >> >> +	struct rev_info rd_rev;
> >> >>  	struct setup_revision_opt s_r_opt;
> >> >>  	int nr = 0, total, i;
> >> >>  	int use_stdout = 0;
> >> >> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >> >>  	init_log_defaults();
> >> >>  	git_config(git_format_config, NULL);
> >> >>  	repo_init_revisions(the_repository, &rev, prefix);
> >> >> +	repo_init_revisions(the_repository, &rd_rev, prefix);
> >> >>  	rev.commit_format = CMIT_FMT_EMAIL;
> >> >>  	rev.expand_tabs_in_log_default = 0;
> >> >>  	rev.verbose_header = 1;
> >> >> @@ -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";
> >> >> +		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);
> >> >> +		}
> >> >> +
> >> >> +		if (!have_prefix)
> >> >> +			die(_("unrecognized argument: %s"), argv[1]);
> >> >> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
> >> >> +		if (argc > 1)
> >> >> +			die(_("unrecognized argument: %s"), argv[1]);
> >> >> +	}
> >> >>
> >> >>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
> >> >>  		die(_("--name-only does not make sense"));
> >> >> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >> >>  	if (!use_patch_format &&
> >> >>  		(!rev.diffopt.output_format ||
> >> >>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> >> >> -		/* Needs to be mirrored in show_range_diff() invocation */
> >> >>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
> >> >>  	if (!rev.diffopt.stat_width)
> >> >>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
> >> >> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >> >>  	if (cover_letter) {
> >> >>  		if (thread)
> >> >>  			gen_message_id(&rev, "cover");
> >> >> -		make_cover_letter(&rev, use_stdout,
> >> >> +		make_cover_letter(&rev, &rd_rev, use_stdout,
> >> >>  				  origin, nr, list, branch_name, quiet);
> >> >>  		print_bases(&bases, rev.diffopt.file);
> >> >>  		print_signature(rev.diffopt.file);
> >> >> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> >> >> index bc5facc1cd..6916103888 100755
> >> >> --- a/t/t3206-range-diff.sh
> >> >> +++ b/t/t3206-range-diff.sh
> >> >> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
> >> >>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
> >> >>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> >> >>  	sed -e "s|:$||" >expect <<-\EOF &&
> >> >> +	1:  a63e992 ! 1:  d966c5c s/12/B/
> >> >> +	    @@ -8,7 +8,7 @@
> >> >> +	     @@
> >> >> +	      9
> >> >> +	      10
> >> >> +	    - B
> >> >> +	    + BB
> >> >> +	     -12
> >> >> +	     +B
> >> >> +	      13
> >> >> +	-- :
> >> >> +	EOF
> >> >> +	test_cmp expect actual.range-diff &&
> >> >> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
> >> >> +	sed -e "s|:$||" >expect <<-\EOF &&
> >> >> +	--- a/file
> >> >> +	+++ b/file
> >> >> +	@@ -12 +12 @@ BB
> >> >> +	-12
> >> >> +	+B
> >> >> +	-- :
> >> >> +	EOF
> >> >> +	test_cmp expect actual.diff &&
> >> >> +
> >> >> +	# -U0 & --range-diff-U0
> >> >> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
> >> >> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
> >> >> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> >> >> +	sed -e "s|:$||" >expect <<-\EOF &&
> >> >>  	1:  a63e992 ! 1:  d966c5c s/12/B/
> >> >>  	    @@ -11 +11 @@
> >> >>  	    - B
> >> >> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
> >> >>  	test_cmp expect actual.diff
> >> >>  '
> >> >>
> >> >> +test_expect_success 'format-patch option parsing with --range-diff-*' '
> >> >> +	test_must_fail git format-patch --stdout --unknown \
> >> >> +		master..unmodified 2>stderr &&
> >> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
> >> >> +	test_must_fail git format-patch --stdout --range-diff-unknown \
> >> >> +		master..unmodified 2>stderr &&
> >> >> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
> >> >> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
> >> >> +		master..unmodified 2>stderr &&
> >> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr
> >> >> +'
> >> >> +
> >> >>  test_done
> >> >> --
> >> >> 2.20.0.rc1.387.gf8505762e3
> >> >>
> >> >>
> >>
>
Ævar Arnfjörð Bjarmason Nov. 29, 2018, 4:03 p.m. UTC | #7
On Thu, Nov 29 2018, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> On Thu, Nov 29 2018, Johannes Schindelin wrote:
>>
>> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> On Thu, Nov 29 2018, Johannes Schindelin wrote:
>> >>
>> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>> >> >
>> >> >> Change the semantics of the "--range-diff" option so that the regular
>> >> >> diff options can be provided separately for the range-diff and the
>> >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
>> >> >> "format-patch" to provide different context for the range-diff and the
>> >> >> patch. This wasn't possible before.
>> >> >
>> >> > I really, really dislike the `--range-diff-<random-thing>`. We have
>> >> > precedent for passing optional arguments that are passed to some other
>> >> > command, so a much more logical and consistent convention would be to use
>> >> > `--range-diff[=<diff-option>..]`, allowing all of the diff options that
>> >> > you might want to pass to the outer diff in one go rather than having a
>> >> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
>> >>
>> >> Where do we pass those sorts of arguments?
>> >>
>> >> Reasons I did it this way:
>> >>
>> >>  a) Passing it as one option will require the user to double-quote those
>> >>     options that take quoted arguments (e.g. --word-diff-regex), which I
>> >>     thought sucked more than the prefix. On the implementation side we
>> >>     couldn't leave the parsing of the command-line to the shell anymore.
>> >>
>> >>  b) I think people will want to tweak this very rarely, much more rarely
>> >>     than e.g. -U10 in format-patch itself, so having something long-ish
>> >>     doesn't sound bad.
>> >
>> > Hmm. I still don't like it. It sets a precedent, and we simply do not do
>> > it that way in other circumstances (most obvious would be the -X merge
>> > options). The more divergent user interfaces for the same sort of thing
>> > are, the more brain cycles you force users to spend on navigating said
>> > interfaces.
>>
>> Yeah it sucks, I just think it sucks less than the alternative :)
>> I.e. I'm not picky about --range-diff-* prefix the name, but I think
>> doing our own shell parsing would be nasty.
>
> What prevents you from using `sq_dequote_to_argv()`?

I mean not just nasty in terms of implementation, yeah we could do it,
but also a nasty UX for things like --word-diff-regex. I.e. instead of:

    --range-diff-word-diff-regex='[0-9"]'

You need:

    --range-diff-opts="--word-diff-regex='[0-9\"]'"

Now admittedly that in itself isn't very painful *in this case*, but in
terms of precedent I really dislike that option, i.e. git having some
mode where I need to work to escape input to pass to another command.

Not saying that this --range-diff-* thing is what we should go for, but
surely we can find some way to do deal with this that doesn't involve
the user needing to escape stuff like this.

It also has other downstream effects in the UI, e.g. it's presumably
easy to teach the bash completion that a --foo=XYZ option is also called
--some-prefix--foo=XYZ and to enable completion for that, less so for
making it smart enough to complete "--some-prefix-opts="--foo=<TAB>".

>> >> > I only had time to skim the patch, and I have to wonder why you pass
>> >> > around full-blown `rev_info` structs for range diff (and with that really
>> >> > awful name `rd_rev`) rather than just the `diff_options` that you
>> >> > *actually* care about?
>> >>
>> >> Because setup_revisions() which does all the command-line parsing needs
>> >> a rev_info, so even if we only need the diffopt in the end we need to
>> >> initiate the whole thing.
>> >>
>> >> Suggestions for a better varibale name most welcome.
>> >
>> > `range_diff_revs`
>> >
>> > And you do not need to pass around the whole thing. You can easily pass
>> > `&range_diff_revs.diffopt`.
>> >
>> > Don't pass around what you do not need to pass around.
>>
>> Ah, you mean internally in log.c, yes that makes sense. I thought you
>> meant just pass diffopt to setup_revisions() (which needs the containing
>> struct). Willdo.
>
> Thanks,
> Dscho
>
>>
>> > Ciao,
>> > Dscho
>> >
>> >>
>> >> > Ciao,
>> >> > Dscho
>> >> >
>> >> >>
>> >> >> Ever since the "--range-diff" option was added in
>> >> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
>> >> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
>> >> >> machinery has been the one we get from "format-patch"'s own
>> >> >> setup_revisions().
>> >> >>
>> >> >> This sort of thing is unique among the log-like commands in
>> >> >> builtin/log.c, no command than format-patch will embed the output of
>> >> >> another log-like command. Since the "rev->diffopt" is reused we need
>> >> >> to munge it before we pass it to show_range_diff(). See
>> >> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff
>> >> >> output", 2018-11-22) for a related regression fix which is being
>> >> >> mostly reverted here.
>> >> >>
>> >> >> Implementation notes: 1) We're not bothering with the full teardown
>> >> >> around die() and will leak memory, but it's too much boilerplate to do
>> >> >> all the frees with/without the die() and not worth it. 2) We call
>> >> >> repo_init_revisions() for "rd_rev" even though we could get away with
>> >> >> a shallow copy like the code we're replacing (and which
>> >> >> show_range_diff() itself does). This is to make this code more easily
>> >> >> understood.
>> >> >>
>> >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >> >> ---
>> >> >>  Documentation/git-format-patch.txt | 10 ++++++-
>> >> >>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
>> >> >>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
>> >> >>  3 files changed, 82 insertions(+), 11 deletions(-)
>> >> >>
>> >> >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
>> >> >> index aba4c5febe..6c048f415f 100644
>> >> >> --- a/Documentation/git-format-patch.txt
>> >> >> +++ b/Documentation/git-format-patch.txt
>> >> >> @@ -24,7 +24,8 @@ SYNOPSIS
>> >> >>  		   [--to=<email>] [--cc=<email>]
>> >> >>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
>> >> >>  		   [--interdiff=<previous>]
>> >> >> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
>> >> >> +		   [--range-diff=<previous> [--creation-factor=<percent>]
>> >> >> +		      [--range-diff<common diff option>]]
>> >> >>  		   [--progress]
>> >> >>  		   [<common diff options>]
>> >> >>  		   [ <since> | <revision range> ]
>> >> >> @@ -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.
>> >> >> +
>> >> >>  --notes[=<ref>]::
>> >> >>  	Append the notes (see linkgit:git-notes[1]) for the commit
>> >> >>  	after the three-dash line.
>> >> >> diff --git a/builtin/log.c b/builtin/log.c
>> >> >> index 02d88fa233..7658e56ecc 100644
>> >> >> --- a/builtin/log.c
>> >> >> +++ b/builtin/log.c
>> >> >> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
>> >> >>  	fprintf(rev->diffopt.file, "\n");
>> >> >>  }
>> >> >>
>> >> >> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> >> >> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
>> >> >> +			      int use_stdout,
>> >> >>  			      struct commit *origin,
>> >> >>  			      int nr, struct commit **list,
>> >> >>  			      const char *branch_name,
>> >> >> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> >> >>  	}
>> >> >>
>> >> >>  	if (rev->rdiff1) {
>> >> >> -		struct diff_options opts;
>> >> >> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
>> >> >> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>> >> >> -
>> >> >>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>> >> >>  		show_range_diff(rev->rdiff1, rev->rdiff2,
>> >> >> -				rev->creation_factor, 1, &opts);
>> >> >> +				rev->creation_factor, 1, &rd_rev->diffopt);
>> >> >>  	}
>> >> >>  }
>> >> >>
>> >> >> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >> >>  	struct commit *commit;
>> >> >>  	struct commit **list = NULL;
>> >> >>  	struct rev_info rev;
>> >> >> +	struct rev_info rd_rev;
>> >> >>  	struct setup_revision_opt s_r_opt;
>> >> >>  	int nr = 0, total, i;
>> >> >>  	int use_stdout = 0;
>> >> >> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >> >>  	init_log_defaults();
>> >> >>  	git_config(git_format_config, NULL);
>> >> >>  	repo_init_revisions(the_repository, &rev, prefix);
>> >> >> +	repo_init_revisions(the_repository, &rd_rev, prefix);
>> >> >>  	rev.commit_format = CMIT_FMT_EMAIL;
>> >> >>  	rev.expand_tabs_in_log_default = 0;
>> >> >>  	rev.verbose_header = 1;
>> >> >> @@ -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";
>> >> >> +		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);
>> >> >> +		}
>> >> >> +
>> >> >> +		if (!have_prefix)
>> >> >> +			die(_("unrecognized argument: %s"), argv[1]);
>> >> >> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
>> >> >> +		if (argc > 1)
>> >> >> +			die(_("unrecognized argument: %s"), argv[1]);
>> >> >> +	}
>> >> >>
>> >> >>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
>> >> >>  		die(_("--name-only does not make sense"));
>> >> >> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >> >>  	if (!use_patch_format &&
>> >> >>  		(!rev.diffopt.output_format ||
>> >> >>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
>> >> >> -		/* Needs to be mirrored in show_range_diff() invocation */
>> >> >>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
>> >> >>  	if (!rev.diffopt.stat_width)
>> >> >>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
>> >> >> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >> >>  	if (cover_letter) {
>> >> >>  		if (thread)
>> >> >>  			gen_message_id(&rev, "cover");
>> >> >> -		make_cover_letter(&rev, use_stdout,
>> >> >> +		make_cover_letter(&rev, &rd_rev, use_stdout,
>> >> >>  				  origin, nr, list, branch_name, quiet);
>> >> >>  		print_bases(&bases, rev.diffopt.file);
>> >> >>  		print_signature(rev.diffopt.file);
>> >> >> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> >> >> index bc5facc1cd..6916103888 100755
>> >> >> --- a/t/t3206-range-diff.sh
>> >> >> +++ b/t/t3206-range-diff.sh
>> >> >> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
>> >> >>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
>> >> >>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>> >> >>  	sed -e "s|:$||" >expect <<-\EOF &&
>> >> >> +	1:  a63e992 ! 1:  d966c5c s/12/B/
>> >> >> +	    @@ -8,7 +8,7 @@
>> >> >> +	     @@
>> >> >> +	      9
>> >> >> +	      10
>> >> >> +	    - B
>> >> >> +	    + BB
>> >> >> +	     -12
>> >> >> +	     +B
>> >> >> +	      13
>> >> >> +	-- :
>> >> >> +	EOF
>> >> >> +	test_cmp expect actual.range-diff &&
>> >> >> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
>> >> >> +	sed -e "s|:$||" >expect <<-\EOF &&
>> >> >> +	--- a/file
>> >> >> +	+++ b/file
>> >> >> +	@@ -12 +12 @@ BB
>> >> >> +	-12
>> >> >> +	+B
>> >> >> +	-- :
>> >> >> +	EOF
>> >> >> +	test_cmp expect actual.diff &&
>> >> >> +
>> >> >> +	# -U0 & --range-diff-U0
>> >> >> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
>> >> >> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
>> >> >> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>> >> >> +	sed -e "s|:$||" >expect <<-\EOF &&
>> >> >>  	1:  a63e992 ! 1:  d966c5c s/12/B/
>> >> >>  	    @@ -11 +11 @@
>> >> >>  	    - B
>> >> >> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
>> >> >>  	test_cmp expect actual.diff
>> >> >>  '
>> >> >>
>> >> >> +test_expect_success 'format-patch option parsing with --range-diff-*' '
>> >> >> +	test_must_fail git format-patch --stdout --unknown \
>> >> >> +		master..unmodified 2>stderr &&
>> >> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
>> >> >> +	test_must_fail git format-patch --stdout --range-diff-unknown \
>> >> >> +		master..unmodified 2>stderr &&
>> >> >> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
>> >> >> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
>> >> >> +		master..unmodified 2>stderr &&
>> >> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr
>> >> >> +'
>> >> >> +
>> >> >>  test_done
>> >> >> --
>> >> >> 2.20.0.rc1.387.gf8505762e3
>> >> >>
>> >> >>
>> >>
>>
Johannes Schindelin Nov. 29, 2018, 7:03 p.m. UTC | #8
Hi Ævar,

On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> 
> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> >>
> >> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >> >
> >> >> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> >> >>
> >> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >> >> >
> >> >> >> Change the semantics of the "--range-diff" option so that the regular
> >> >> >> diff options can be provided separately for the range-diff and the
> >> >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> >> >> >> "format-patch" to provide different context for the range-diff and the
> >> >> >> patch. This wasn't possible before.
> >> >> >
> >> >> > I really, really dislike the `--range-diff-<random-thing>`. We have
> >> >> > precedent for passing optional arguments that are passed to some other
> >> >> > command, so a much more logical and consistent convention would be to use
> >> >> > `--range-diff[=<diff-option>..]`, allowing all of the diff options that
> >> >> > you might want to pass to the outer diff in one go rather than having a
> >> >> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
> >> >>
> >> >> Where do we pass those sorts of arguments?
> >> >>
> >> >> Reasons I did it this way:
> >> >>
> >> >>  a) Passing it as one option will require the user to double-quote those
> >> >>     options that take quoted arguments (e.g. --word-diff-regex), which I
> >> >>     thought sucked more than the prefix. On the implementation side we
> >> >>     couldn't leave the parsing of the command-line to the shell anymore.
> >> >>
> >> >>  b) I think people will want to tweak this very rarely, much more rarely
> >> >>     than e.g. -U10 in format-patch itself, so having something long-ish
> >> >>     doesn't sound bad.
> >> >
> >> > Hmm. I still don't like it. It sets a precedent, and we simply do not do
> >> > it that way in other circumstances (most obvious would be the -X merge
> >> > options). The more divergent user interfaces for the same sort of thing
> >> > are, the more brain cycles you force users to spend on navigating said
> >> > interfaces.
> >>
> >> Yeah it sucks, I just think it sucks less than the alternative :)
> >> I.e. I'm not picky about --range-diff-* prefix the name, but I think
> >> doing our own shell parsing would be nasty.
> >
> > What prevents you from using `sq_dequote_to_argv()`?
> 
> I mean not just nasty in terms of implementation, yeah we could do it,
> but also a nasty UX for things like --word-diff-regex. I.e. instead of:
> 
>     --range-diff-word-diff-regex='[0-9"]'
> 
> You need:
> 
>     --range-diff-opts="--word-diff-regex='[0-9\"]'"

Really? I think that would not work. It would pass the single quotes as
part of the regex to the diff machinery.

Or maybe not. But the extra quotes do not strike me as necessary, as there
is no shell script involved (thank deity!) after `git range-diff` parsed
the options.

> Now admittedly that in itself isn't very painful *in this case*, but in
> terms of precedent I really dislike that option, i.e. git having some
> mode where I need to work to escape input to pass to another command.
> 
> Not saying that this --range-diff-* thing is what we should go for, but
> surely we can find some way to do deal with this that doesn't involve
> the user needing to escape stuff like this.
> 
> It also has other downstream effects in the UI, e.g. it's presumably
> easy to teach the bash completion that a --foo=XYZ option is also called
> --some-prefix--foo=XYZ and to enable completion for that, less so for
> making it smart enough to complete "--some-prefix-opts="--foo=<TAB>".

These are all good points, and need proper discussion.

Sadly, all that time needed for a proper discussion is not left before
v2.20.0 is supposed to come out.

Quite honestly, I think what we will have to do is to describe in the
documentation of `format-patch`'s `--range-diff` option that the exact
user interface how to pass diff options down to `range-diff` is in flux
and not final.

That way, we can give your design the proper treatment, and work together
on making a user interface we all can be happy with.

Ciao,
Dscho
Junio C Hamano Nov. 30, 2018, 2:30 a.m. UTC | #9
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> What prevents you from using `sq_dequote_to_argv()`?
>
> I mean not just nasty in terms of implementation, yeah we could do it,
> but also a nasty UX for things like --word-diff-regex. I.e. instead of:
>
>     --range-diff-word-diff-regex='[0-9"]'
>
> You need:
>
>     --range-diff-opts="--word-diff-regex='[0-9\"]'"
>
> Now admittedly that in itself isn't very painful *in this case*, but in
> terms of precedent I really dislike that option, i.e. git having some
> mode where I need to work to escape input to pass to another command.

In addition, sq_dequote are meant to be used on quoted string we
internally produce; I do not think we want to promise that it is
safe to use on a random string that comes from end users.

In any case, I tend to agree with the conclusion in the downthread
by Dscho that we should just clearly mark that invocations of the
"format-patch --range-diff" command with additional diff options is
an experimental feature that may not do anything sensible in the
upcoming release, and declare that the UI to pass diff options to
affect only the range-diff part may later be invented.  IOW, I am
coming a bit stronger than Dscho's suggestion in that we should not
even pretend that we aimed to make the options used for range-diff
customizable when driven from format-patch in the upcoming release,
or aimed to make --range-diff option compatible with other diff
options given to the format-patch command.

I had to delay -rc2 to see these last minute tweaks come to some
reasonable place to stop at, and I do not think we want to delay the
final any longer or destablizing it further by piling last minute
undercooked changes on top.
Eric Sunshine Nov. 30, 2018, 9:58 a.m. UTC | #10
On Thu, Nov 29, 2018 at 11:03 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> I mean not just nasty in terms of implementation, yeah we could do it,
> but also a nasty UX for things like --word-diff-regex. I.e. instead of:
>
>     --range-diff-word-diff-regex='[0-9"]'
>
> You need:
>
>     --range-diff-opts="--word-diff-regex='[0-9\"]'"
>
> Now admittedly that in itself isn't very painful *in this case*, but in
> terms of precedent I really dislike that option, i.e. git having some
> mode where I need to work to escape input to pass to another command.
>
> Not saying that this --range-diff-* thing is what we should go for, but
> surely we can find some way to do deal with this that doesn't involve
> the user needing to escape stuff like this.

I should mention that it was never the intention that
git-format-patch's --range-diff option would allow passing _all_
possible options to git-range-diff, but only those most likely to be
tweaked by the user (such as --creation-factor). It was understood
from the start (and stated by me either in the cover letter to that
series or in discussion) that the user always has the escape hatch of
running git-range-diff manually and copy/pasting its output into the
cover letter.

So, I'm not convinced that we need this sort of flexibility in
git-format-patch's --range-diff option, but we certainly can add
convenience options (such as I did with --creation-factor) as people
complain that their favorite option is missing. For a UI, I think we
already have sufficient precedent for
"--range-diff-opts=nopatch,creation-factor:60" as a possibility.
diff mbox series

Patch

diff options can be provided separately for the range-diff and the
patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
"format-patch" to provide different context for the range-diff and the
patch. This wasn't possible before.

Ever since the "--range-diff" option was added in
31e2617a5f ("format-patch: add --range-diff option to embed diff in
cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
machinery has been the one we get from "format-patch"'s own
setup_revisions().

This sort of thing is unique among the log-like commands in
builtin/log.c, no command than format-patch will embed the output of
another log-like command. Since the "rev->diffopt" is reused we need
to munge it before we pass it to show_range_diff(). See
43dafc4172 ("format-patch: don't include --stat with --range-diff
output", 2018-11-22) for a related regression fix which is being
mostly reverted here.

Implementation notes: 1) We're not bothering with the full teardown
around die() and will leak memory, but it's too much boilerplate to do
all the frees with/without the die() and not worth it. 2) We call
repo_init_revisions() for "rd_rev" even though we could get away with
a shallow copy like the code we're replacing (and which
show_range_diff() itself does). This is to make this code more easily
understood.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-format-patch.txt | 10 ++++++-
 builtin/log.c                      | 42 +++++++++++++++++++++++-------
 t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index aba4c5febe..6c048f415f 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -24,7 +24,8 @@  SYNOPSIS
 		   [--to=<email>] [--cc=<email>]
 		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
 		   [--interdiff=<previous>]
-		   [--range-diff=<previous> [--creation-factor=<percent>]]
+		   [--range-diff=<previous> [--creation-factor=<percent>]
+		      [--range-diff<common diff option>]]
 		   [--progress]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
@@ -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.
+
 --notes[=<ref>]::
 	Append the notes (see linkgit:git-notes[1]) for the commit
 	after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index 02d88fa233..7658e56ecc 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1023,7 +1023,8 @@  static void show_diffstat(struct rev_info *rev,
 	fprintf(rev->diffopt.file, "\n");
 }
 
-static void make_cover_letter(struct rev_info *rev, int use_stdout,
+static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
+			      int use_stdout,
 			      struct commit *origin,
 			      int nr, struct commit **list,
 			      const char *branch_name,
@@ -1095,13 +1096,9 @@  static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	}
 
 	if (rev->rdiff1) {
-		struct diff_options opts;
-		memcpy(&opts, &rev->diffopt, sizeof(opts));
-		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
-
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, &opts);
+				rev->creation_factor, 1, &rd_rev->diffopt);
 	}
 }
 
@@ -1485,6 +1482,7 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct commit *commit;
 	struct commit **list = NULL;
 	struct rev_info rev;
+	struct rev_info rd_rev;
 	struct setup_revision_opt s_r_opt;
 	int nr = 0, total, i;
 	int use_stdout = 0;
@@ -1603,6 +1601,7 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	init_log_defaults();
 	git_config(git_format_config, NULL);
 	repo_init_revisions(the_repository, &rev, prefix);
+	repo_init_revisions(the_repository, &rd_rev, prefix);
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.expand_tabs_in_log_default = 0;
 	rev.verbose_header = 1;
@@ -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";
+		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);
+		}
+
+		if (!have_prefix)
+			die(_("unrecognized argument: %s"), argv[1]);
+		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
+		if (argc > 1)
+			die(_("unrecognized argument: %s"), argv[1]);
+	}
 
 	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
 		die(_("--name-only does not make sense"));
@@ -1702,7 +1725,6 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (!use_patch_format &&
 		(!rev.diffopt.output_format ||
 		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
-		/* Needs to be mirrored in show_range_diff() invocation */
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
 	if (!rev.diffopt.stat_width)
 		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
@@ -1877,7 +1899,7 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
-		make_cover_letter(&rev, use_stdout,
+		make_cover_letter(&rev, &rd_rev, use_stdout,
 				  origin, nr, list, branch_name, quiet);
 		print_bases(&bases, rev.diffopt.file);
 		print_signature(rev.diffopt.file);
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index bc5facc1cd..6916103888 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -308,6 +308,35 @@  test_expect_success 'format-patch with <common diff option>' '
 		--range-diff=topic~..topic changed~..changed >actual.raw &&
 	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
 	sed -e "s|:$||" >expect <<-\EOF &&
+	1:  a63e992 ! 1:  d966c5c s/12/B/
+	    @@ -8,7 +8,7 @@
+	     @@
+	      9
+	      10
+	    - B
+	    + BB
+	     -12
+	     +B
+	      13
+	-- :
+	EOF
+	test_cmp expect actual.range-diff &&
+	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
+	sed -e "s|:$||" >expect <<-\EOF &&
+	--- a/file
+	+++ b/file
+	@@ -12 +12 @@ BB
+	-12
+	+B
+	-- :
+	EOF
+	test_cmp expect actual.diff &&
+
+	# -U0 & --range-diff-U0
+	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
+		--range-diff=topic~..topic changed~..changed >actual.raw &&
+	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
+	sed -e "s|:$||" >expect <<-\EOF &&
 	1:  a63e992 ! 1:  d966c5c s/12/B/
 	    @@ -11 +11 @@
 	    - B
@@ -327,4 +356,16 @@  test_expect_success 'format-patch with <common diff option>' '
 	test_cmp expect actual.diff
 '
 
+test_expect_success 'format-patch option parsing with --range-diff-*' '
+	test_must_fail git format-patch --stdout --unknown \
+		master..unmodified 2>stderr &&
+	test_i18ngrep "unrecognized argument: --unknown" stderr &&
+	test_must_fail git format-patch --stdout --range-diff-unknown \
+		master..unmodified 2>stderr &&
+	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
+	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
+		master..unmodified 2>stderr &&
+	test_i18ngrep "unrecognized argument: --unknown" stderr
+'
+
 test_done