On Wed, Nov 04, 2020 at 08:24:28AM -0500, Jeff King wrote:
> The issue is that "--output" was never supposed to work with
> format-patch. But a subtle change in the option parsing a while back
> caused it to be respected. And as you noticed, the documentation
> mistakenly mentions the option, since format-patch includes the standard
> diff-options text.
>
> So one obvious fix would be to forbid it and adjust the documentation.
> But because of the way the option parsers interact, it's surprisingly
> hard to do so cleanly. It's actually easier to just make it do something
> useful (i.e., behave like --stdout but sent to a file). So I did that
> instead.
>
> [1/3]: format-patch: refactor output selection
> [2/3]: format-patch: tie file-opening logic to output_directory
> [3/3]: format-patch: support --output option
Here's a re-roll taking into account the comments from Eric. The only
thing I didn't do is rewrite --output as a format-patch option. As I
said in the thread, I'd rather keep parity with how git-log works here
(though I don't mind if somebody wants to do further clean up on top).
[1/3]: format-patch: refactor output selection
[2/3]: format-patch: tie file-opening logic to output_directory
[3/3]: format-patch: support --output option
builtin/log.c | 37 ++++++++++++++++++++++---------------
t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+), 15 deletions(-)
Range diff from v1:
1: 49e8b54549 ! 1: 9206d6852b format-patch: refactor output selection
@@ Commit message
slightly easier to follow now, and also will keep things sane when we
add another output mode in a future patch.
+ We'll add a few tests as well, covering the mutual exclusion and the
+ fact that we are not confused by a configured output directory.
+
Signed-off-by: Jeff King <peff@xxxxxxxx>
## builtin/log.c ##
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
- if (!output_directory && !use_stdout)
- output_directory = config_output_directory;
+ if (use_stdout + !!output_directory > 1)
-+ die(_("specify only one of --stdout, --output, and --output-directory"));
++ die(_("--stdout and --output-directory are mutually exclusive"));
- if (!use_stdout)
- output_directory = set_outdir(prefix, output_directory);
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
/*
* We consider <outdir> as 'outside of gitdir', therefore avoid
* applying adjust_shared_perm in s-c-l-d.
+
+ ## t/t4014-format-patch.sh ##
+@@ t/t4014-format-patch.sh: test_expect_success 'format-patch -o overrides format.outputDirectory' '
+ test_path_is_dir patchset
+ '
+
++test_expect_success 'format-patch forbids multiple outputs' '
++ rm -fr outdir &&
++ test_must_fail \
++ git format-patch --stdout --output-directory=outdir
++'
++
++test_expect_success 'configured outdir does not conflict with output options' '
++ rm -fr outfile outdir &&
++ test_config format.outputDirectory outdir &&
++ git format-patch --stdout &&
++ test_path_is_missing outdir
++'
++
+ test_expect_success 'format-patch --base' '
+ git checkout patchid &&
+
2: 884c06861d ! 2: 9dc30924b2 format-patch: tie file-opening logic to output_directory
@@ Commit message
format-patch: tie file-opening logic to output_directory
In format-patch we're either outputting to stdout or to individual files
- in an output directory (which maybe just "./"). Our logic for whether to
- open a new file for each patch is checked with "!use_stdout", but it is
- equally correct to check for a non-NULL output_directory.
+ in an output directory (which may be just "./"). Our logic for whether
+ to open a new file for each patch is checked with "!use_stdout", but it
+ is equally correct to check for a non-NULL output_directory.
The distinction will matter when we add a new single-stream output in a
future patch, when only one of the three methods will want individual
3: 8befceb150 ! 3: 2b0fab9b50 format-patch: support --output option
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
load_display_notes(&rev.notes_opt);
- if (use_stdout + !!output_directory > 1)
+- die(_("--stdout and --output-directory are mutually exclusive"));
+ if (use_stdout + rev.diffopt.close_file + !!output_directory > 1)
- die(_("specify only one of --stdout, --output, and --output-directory"));
++ die(_("--stdout, --output, and --output-directory are mutually exclusive"));
if (use_stdout) {
setup_pager();
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
## t/t4014-format-patch.sh ##
@@ t/t4014-format-patch.sh: test_expect_success 'format-patch -o overrides format.outputDirectory' '
- test_path_is_dir patchset
- '
-
-+test_expect_success 'format-patch forbids multiple outputs' '
-+ rm -fr outfile outdir &&
+ test_expect_success 'format-patch forbids multiple outputs' '
+ rm -fr outdir &&
+ test_must_fail \
+- git format-patch --stdout --output-directory=outdir
++ git format-patch --stdout --output-directory=outdir &&
+ test_must_fail \
+ git format-patch --stdout --output=outfile &&
+ test_must_fail \
-+ git format-patch --stdout --output-directory=outdir &&
-+ test_must_fail \
+ git format-patch --output=outfile --output-directory=outdir
-+'
-+
-+test_expect_success 'configured outdir does not conflict with output options' '
-+ rm -fr outfile outdir &&
-+ test_config format.outputDirectory outdir &&
-+ git format-patch --stdout &&
+ '
+
+ test_expect_success 'configured outdir does not conflict with output options' '
+ rm -fr outfile outdir &&
+ test_config format.outputDirectory outdir &&
+ git format-patch --stdout &&
+ test_path_is_missing outdir &&
+ git format-patch --output=outfile &&
-+ test_path_is_missing outdir
-+'
-+
+ test_path_is_missing outdir
+ '
+
+test_expect_success 'format-patch --output' '
+ rm -fr outfile &&
+ git format-patch -3 --stdout HEAD >expect &&
