git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Johannes Postler <johannes.postler@txture.io>
Cc: git@vger.kernel.org
Subject: [PATCH 3/3] format-patch: support --output option
Date: Wed, 4 Nov 2020 08:29:07 -0500	[thread overview]
Message-ID: <20201104132907.GC3030146@coredump.intra.peff.net> (raw)
In-Reply-To: <20201104132428.GA2491189@coredump.intra.peff.net>

We've never intended to support diff's --output option in format-patch.
And until baa4adc66a (parse-options: disable option abbreviation with
PARSE_OPT_KEEP_UNKNOWN, 2019-01-27), it was impossible to trigger. We
first parse the format-patch options before handing the remainder off to
setup_revisions(). Before that commit, we'd accept "--output=foo" as an
abbreviation for "--output-directory=foo". But afterwards, we don't
check abbreviations, and --output gets passed to the diff code.

This results in nonsense behavior and bugs. The diff code will have
opened a filehandle at rev.diffopt.file, but we'll overwrite that with
our own handles that we open for each individual patch file. So the
--output file will always just be empty. But worse, the diff code also
sets rev.diffopt.close_file, so log_tree_commit() will close the
filehandle itself. And then the main loop in cmd_format_patch() will try
to close it again, resulting in a double-free.

The simplest solution would be to just disallow --output with
format-patch, as nobody ever intended it to work. However, we have
accidentally documented it (because format-patch includes diff-options).
And it does work with "git log", which writes the whole output to the
specified file. It's easy enough to make that work for format-patch,
too: it's really the same as --stdout, but pointed at a specific file.

We can detect the use of the --output option by the "close_file" flag
(note that we can't use rev.diffopt.file, since the diff setup will
otherwise set it to stdout). So we just need to unset that flag, but
don't have to do anything else. Our situation is otherwise exactly like
--stdout (note that we don't fclose() the file, but nor does the stdout
case; exiting the program takes care of that for us).

Reported-by: Johannes Postler <johannes.postler@txture.io>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c           |  9 ++++++++-
 t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 27c6d612e4..ddb3ea7326 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (rev.show_notes)
 		load_display_notes(&rev.notes_opt);
 
-	if (use_stdout + !!output_directory > 1)
+	if (use_stdout + rev.diffopt.close_file + !!output_directory > 1)
 		die(_("specify only one of --stdout, --output, and --output-directory"));
 
 	if (use_stdout) {
 		setup_pager();
+	} else if (rev.diffopt.close_file) {
+		/*
+		 * The diff code parsed --output; it has already opened the
+		 * file, but but we must instruct it not to close after each
+		 * diff.
+		 */
+		rev.diffopt.close_file = 0;
 	} else {
 		int saved;
 
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 294e76c860..64574c51c1 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1919,6 +1919,39 @@ 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_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_path_is_missing outdir &&
+	git format-patch --output=outfile &&
+	test_path_is_missing outdir
+'
+
+test_expect_success 'format-patch --output' '
+	rm -fr outfile &&
+	git format-patch -3 --stdout HEAD >expect &&
+	git format-patch -3 --output=outfile HEAD &&
+	test_cmp expect outfile
+'
+
+test_expect_success 'format-patch --cover-letter --output' '
+	rm -fr outfile &&
+	git format-patch --cover-letter -3 --stdout HEAD >expect &&
+	git format-patch --cover-letter -3 --output=outfile HEAD &&
+	test_cmp expect outfile
+'
+
 test_expect_success 'format-patch --base' '
 	git checkout patchid &&
 
-- 
2.29.2.559.g8ec94df761

  parent reply	other threads:[~2020-11-04 13:29 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   ` Jeff King [this message]
2020-11-04 17:27     ` [PATCH 3/3] format-patch: support --output option 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     ` [PATCH v2] format-patch --output Johannes Postler

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=20201104132907.GC3030146@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=johannes.postler@txture.io \
    --subject='Re: [PATCH 3/3] format-patch: support --output option' \
    /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).