[PATCH v2] format-patch --output

[Jeff King <peff@peff.net>]

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@peff.net>
     
      ## 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 &&