From patchwork Wed Nov 4 13:25:22 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11880583 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C227DC2D0A3 for ; Wed, 4 Nov 2020 13:25:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5B1F520780 for ; Wed, 4 Nov 2020 13:25:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729909AbgKDNZX (ORCPT ); Wed, 4 Nov 2020 08:25:23 -0500 Received: from cloud.peff.net ([104.130.231.41]:47140 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729718AbgKDNZX (ORCPT ); Wed, 4 Nov 2020 08:25:23 -0500 Received: (qmail 9711 invoked by uid 109); 4 Nov 2020 13:25:22 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 04 Nov 2020 13:25:22 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 10143 invoked by uid 111); 4 Nov 2020 13:25:22 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 04 Nov 2020 08:25:22 -0500 Authentication-Results: peff.net; auth=none Date: Wed, 4 Nov 2020 08:25:22 -0500 From: Jeff King To: Johannes Postler Cc: git@vger.kernel.org Subject: [PATCH 1/3] format-patch: refactor output selection Message-ID: <20201104132522.GA3030146@coredump.intra.peff.net> References: <20201104132428.GA2491189@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201104132428.GA2491189@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org The --stdout and --output-directory options are mutually exclusive, but it's hard to tell from reading the code. We have three separate conditionals that check for use_stdout, and it's only after we've set up the output_directory fully that we check whether the user also specified --stdout. Instead, let's check the exclusion explicitly first, then have a single conditional that handles stdout versus an output directory. This is slightly easier to follow now, and also will keep things sane when we add another output mode in a future patch. Signed-off-by: Jeff King --- builtin/log.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 9f939e6cdf..4c391ba3ca 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1942,20 +1942,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (rev.show_notes) load_display_notes(&rev.notes_opt); - 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")); - if (!use_stdout) - output_directory = set_outdir(prefix, output_directory); - else + if (use_stdout) { setup_pager(); - - if (output_directory) { + } else { int saved; + + if (!output_directory) + output_directory = config_output_directory; + output_directory = set_outdir(prefix, output_directory); + if (rev.diffopt.use_color != GIT_COLOR_ALWAYS) rev.diffopt.use_color = GIT_COLOR_NEVER; - if (use_stdout) - die(_("standard output, or directory, which one?")); /* * We consider as 'outside of gitdir', therefore avoid * applying adjust_shared_perm in s-c-l-d. From patchwork Wed Nov 4 13:27:37 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11880587 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8B87CC2D0A3 for ; Wed, 4 Nov 2020 13:27:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 420602222C for ; Wed, 4 Nov 2020 13:27:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729677AbgKDN1j (ORCPT ); Wed, 4 Nov 2020 08:27:39 -0500 Received: from cloud.peff.net ([104.130.231.41]:47148 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729787AbgKDN1i (ORCPT ); Wed, 4 Nov 2020 08:27:38 -0500 Received: (qmail 9722 invoked by uid 109); 4 Nov 2020 13:27:38 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 04 Nov 2020 13:27:38 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 10178 invoked by uid 111); 4 Nov 2020 13:27:37 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 04 Nov 2020 08:27:37 -0500 Authentication-Results: peff.net; auth=none Date: Wed, 4 Nov 2020 08:27:37 -0500 From: Jeff King To: Johannes Postler Cc: git@vger.kernel.org Subject: [PATCH 2/3] format-patch: tie file-opening logic to output_directory Message-ID: <20201104132737.GB3030146@coredump.intra.peff.net> References: <20201104132428.GA2491189@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201104132428.GA2491189@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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. 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 files. Let's swap the logic here in preparation. Signed-off-by: Jeff King --- builtin/log.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 4c391ba3ca..27c6d612e4 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1153,7 +1153,7 @@ static void get_notes_args(struct strvec *arg, struct rev_info *rev) } } -static void make_cover_letter(struct rev_info *rev, int use_stdout, +static void make_cover_letter(struct rev_info *rev, int use_separate_file, struct commit *origin, int nr, struct commit **list, const char *branch_name, @@ -1173,7 +1173,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, committer = git_committer_info(0); - if (!use_stdout && + if (use_separate_file && open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet)) die(_("failed to create cover-letter file")); @@ -2117,7 +2117,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, !!output_directory, origin, nr, list, branch_name, quiet); print_bases(&bases, rev.diffopt.file); print_signature(rev.diffopt.file); @@ -2172,7 +2172,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) gen_message_id(&rev, oid_to_hex(&commit->object.oid)); } - if (!use_stdout && + if (output_directory && open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet)) die(_("failed to create output files")); shown = log_tree_commit(&rev, commit); @@ -2185,7 +2185,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) * the log; when using one file per patch, we do * not want the extra blank line. */ - if (!use_stdout) + if (output_directory) rev.shown_one = 0; if (shown) { print_bases(&bases, rev.diffopt.file); @@ -2196,7 +2196,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) else print_signature(rev.diffopt.file); } - if (!use_stdout) + if (output_directory) fclose(rev.diffopt.file); } stop_progress(&progress); From patchwork Wed Nov 4 13:29:07 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11880591 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CF07AC2D0A3 for ; Wed, 4 Nov 2020 13:29:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7162120867 for ; Wed, 4 Nov 2020 13:29:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729198AbgKDN3I (ORCPT ); Wed, 4 Nov 2020 08:29:08 -0500 Received: from cloud.peff.net ([104.130.231.41]:47156 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728508AbgKDN3I (ORCPT ); Wed, 4 Nov 2020 08:29:08 -0500 Received: (qmail 9731 invoked by uid 109); 4 Nov 2020 13:29:07 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 04 Nov 2020 13:29:07 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 10195 invoked by uid 111); 4 Nov 2020 13:29:07 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 04 Nov 2020 08:29:07 -0500 Authentication-Results: peff.net; auth=none Date: Wed, 4 Nov 2020 08:29:07 -0500 From: Jeff King To: Johannes Postler Cc: git@vger.kernel.org Subject: [PATCH 3/3] format-patch: support --output option Message-ID: <20201104132907.GC3030146@coredump.intra.peff.net> References: <20201104132428.GA2491189@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201104132428.GA2491189@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 Signed-off-by: Jeff King --- 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 &&