git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Johannes Postler <johannes.postler@txture.io>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
	Eric Sunshine <sunshine@sunshineco.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2] format-patch --output
Date: Thu, 5 Nov 2020 07:30:28 +0100	[thread overview]
Message-ID: <CAO9fQ5rAjO0Q+AUA_-BqQqhunOhjadUB5YAU7+W2Pwi9OpUyGQ@mail.gmail.com> (raw)
In-Reply-To: <20201104192645.GA3059114@coredump.intra.peff.net>

Thanks a lot everyone for fixing this so quickly!
Please let me know if there is anything else I can do.
Best,
johannes

Johannes Postler
DevOps & Consulting


Txture - The Cloud Transformation Platform
Grabenweg 68
A-6020 Innsbruck
johannes.postler@txture.io
www.txture.io



Johannes Postler
DevOps & Consulting



Txture - The Cloud Transformation Platform
Grabenweg 68
A-6020 Innsbruck
johannes.postler@txture.io
www.txture.io

Follow us:


On Wed, Nov 4, 2020 at 8:26 PM Jeff King <peff@peff.net> wrote:
>
> 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 &&

      parent reply	other threads:[~2020-11-05  6:30 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-04 10:18 [Bug report] Crash when creating patch Johannes Postler
2020-11-04 13:24 ` Jeff King
2020-11-04 13:25   ` [PATCH 1/3] format-patch: refactor output selection Jeff King
2020-11-04 17:01     ` Eric Sunshine
2020-11-04 17:13       ` Jeff King
2020-11-04 13:27   ` [PATCH 2/3] format-patch: tie file-opening logic to output_directory Jeff King
2020-11-04 17:03     ` Eric Sunshine
2020-11-04 13:29   ` [PATCH 3/3] format-patch: support --output option Jeff King
2020-11-04 17:27     ` Eric Sunshine
2020-11-04 19:15       ` Jeff King
2020-11-04 20:16         ` Eric Sunshine
2020-11-04 18:00     ` Junio C Hamano
2020-11-04 19:26   ` [PATCH v2] format-patch --output Jeff King
2020-11-04 19:28     ` [PATCH v2 1/3] format-patch: refactor output selection Jeff King
2020-11-04 19:28     ` [PATCH v2 2/3] format-patch: tie file-opening logic to output_directory Jeff King
2020-11-04 19:28     ` [PATCH v2 3/3] format-patch: support --output option Jeff King
2020-11-05  6:30     ` Johannes Postler [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAO9fQ5rAjO0Q+AUA_-BqQqhunOhjadUB5YAU7+W2Pwi9OpUyGQ@mail.gmail.com \
    --to=johannes.postler@txture.io \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH v2] format-patch --output' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).