{"thread":{"id":"54570","subject":"[Bug report] Crash when creating patch","startedAt":"2020-11-04T10:18:41Z","lastAt":"2020-11-05T06:30:59Z","messageCount":17,"participants":["Johannes Postler","Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"409066","messageId":"CAO9fQ5pm2vCGrnj59QZg-fJ8VpnrsNoaVXRqd_Lsuw6rEaN3XQ@mail.gmail.com","threadId":"54570","inReplyTo":null,"subject":"[Bug report] Crash when creating patch","fromName":"Johannes Postler","fromEmail":"johannes.postler@txture.io","sentAt":"2020-11-04T10:18:10Z","receivedAt":"2020-11-04T10:18:41Z","isPatch":false,"sender":{"key":"johannes.postler@txture.io","avatar":null},"body":"Dear git-team,\nI was trying to create a patch file of a commit to another directory.\nHowever, git crashed like that:\n\n[josi@localhost typescript]$ git format-patch -1 236a9\n--output=/home/josi/tmp/patches/txt-6117.patch\n0001-Txture-Core-Implemented-whitelisting-for-groovy-scri.patch\nfree(): invalid pointer\nAborted (core dumped)\n\nWhile I'm not entirely sure I interpreted the man page correctly, I\nthink the crash shouldn't happen anyway.\n\nI'm using git 2.28.0, but my colleague is experiencing the same issue\nwith git 2.25.1.\nPlease find attached the output of git bugreport and some more debug\nfiles that were collected by Fedora's ABRT. Please let me know if you\nneed any other output of ABRT.\n\nBest regards,\njohannes\n\nJohannes Postler\nDevOps & Consulting\n\nTxture - The Cloud Transformation Platform\nGrabenweg 68\nA-6020 Innsbruck\njohannes.postler@txture.io\nwww.txture.io\n\n\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\nI tried to create a patch file of a commit in another directory using the following command:\n[josi@localhost typescript]$ git format-patch -1 236a9 --output=/home/josi/tmp/patches/txt-6117.patch\n\nWhat did you expect to happen? (Expected behavior)\nI expected a patch file at the location specified in --output\n\nWhat happened instead? (Actual behavior)\ngit crashed:\n0001-Txture-Core-Implemented-whitelisting-for-groovy-scri.patch\nfree(): invalid pointer\nAborted (core dumped)\n\nWhat's different between what you expected and what actually happened?\nI didn't expect the crash :D\n\nAnything else you want to add:\nI do have a full core dump recorded by ABRT (Fedora). Please let me know at johannes.postler@txture.io if you need thaThe following is the backtrace according to ABRT:\n{   \"signal\": 6\n,   \"executable\": \"/usr/bin/git\"\n,   \"stacktrace\":\n      [ {   \"crash_thread\": true\n        ,   \"frames\":\n              [ {   \"address\": 139721002875845\n                ,   \"build_id\": \"2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea\"\n                ,   \"build_id_offset\": 252869\n                ,   \"function_name\": \"raise\"\n                ,   \"file_name\": \"/lib64/libc.so.6\"\n                }\n              , {   \"address\": 139721002780836\n                ,   \"build_id\": \"2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea\"\n                ,   \"build_id_offset\": 157860\n                ,   \"function_name\": \"abort\"\n                ,   \"file_name\": \"/lib64/libc.so.6\"\n                }\n              , {   \"address\": 139721003147559\n                ,   \"build_id\": \"2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea\"\n                ,   \"build_id_offset\": 524583\n                ,   \"function_name\": \"__libc_message\"\n                ,   \"file_name\": \"/lib64/libc.so.6\"\n                }\n              , {   \"address\": 139721003179548\n                ,   \"build_id\": \"2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea\"\n                ,   \"build_id_offset\": 556572\n                ,   \"function_name\": \".annobin_top_check.start\"\n                ,   \"file_name\": \"/lib64/libc.so.6\"\n                }\n              , {   \"address\": 139721003186412\n                ,   \"build_id\": \"2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea\"\n                ,   \"build_id_offset\": 563436\n                ,   \"function_name\": \"_int_free\"\n                ,   \"file_name\": \"/lib64/libc.so.6\"\n                }\n              , {   \"address\": 139721003104931\n                ,   \"build_id\": \"2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea\"\n                ,   \"build_id_offset\": 481955\n                ,   \"function_name\": \"fclose@@GLIBC_2.2.5\"\n                ,   \"file_name\": \"/lib64/libc.so.6\"\n                }\n              , {   \"address\": 94420595460270\n                ,   \"build_id\": \"ba2b8ad417de884d751cd7503b17e9ce357b2dcf\"\n                ,   \"build_id_offset\": 538798\n                ,   \"function_name\": \"cmd_format_patch\"\n                ,   \"file_name\": \"/usr/bin/git\"\n                }\n              , {   \"address\": 94420595060875\n                ,   \"build_id\": \"ba2b8ad417de884d751cd7503b17e9ce357b2dcf\"\n                ,   \"build_id_offset\": 139403\n                ,   \"function_name\": \"handle_builtin.lto_priv.0\"\n                ,   \"file_name\": \"/usr/bin/git\"\n                }\n              , {   \"address\": 94420595041051\n                ,   \"build_id\": \"ba2b8ad417de884d751cd7503b17e9ce357b2dcf\"\n                ,   \"build_id_offset\": 119579\n                ,   \"function_name\": \"main\"\n                ,   \"file_name\": \"/usr/bin/git\"\n                } ]\n        } ]\n}\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.28.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.8.16-300.fc33.x86_64 #1 SMP Mon Oct 19 13:18:33 UTC 2020 x86_64\ncompiler info: gnuc: 10.2\nlibc info: glibc: 2.32\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\napplypatch-msg\npre-applypatch\npost-applypatch\npre-commit\npre-merge-commit\nprepare-commit-msg\ncommit-msg\npost-commit\npre-rebase\npost-checkout\npost-merge\npre-push\npre-receive\nupdate\npost-receive\npost-update\npush-to-checkout\npre-auto-gc\npost-rewrite\nsendemail-validate\n"},{"id":"409071","messageId":"20201104132428.GA2491189@coredump.intra.peff.net","threadId":"54570","inReplyTo":"CAO9fQ5pm2vCGrnj59QZg-fJ8VpnrsNoaVXRqd_Lsuw6rEaN3XQ@mail.gmail.com","subject":"Re: [Bug report] Crash when creating patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T13:24:28Z","receivedAt":"2020-11-04T13:24:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2020 at 11:18:10AM +0100, Johannes Postler wrote:\n\n> [josi@localhost typescript]$ git format-patch -1 236a9\n> --output=/home/josi/tmp/patches/txt-6117.patch\n> 0001-Txture-Core-Implemented-whitelisting-for-groovy-scri.patch\n> free(): invalid pointer\n> Aborted (core dumped)\n\nThanks for the report. I was able to reproduce the problem here.\n\nThe issue is that \"--output\" was never supposed to work with\nformat-patch. But a subtle change in the option parsing a while back\ncaused it to be respected. And as you noticed, the documentation\nmistakenly mentions the option, since format-patch includes the standard\ndiff-options text.\n\nSo one obvious fix would be to forbid it and adjust the documentation.\nBut because of the way the option parsers interact, it's surprisingly\nhard to do so cleanly. It's actually easier to just make it do something\nuseful (i.e., behave like --stdout but sent to a file). So I did that\ninstead.\n\n  [1/3]: format-patch: refactor output selection\n  [2/3]: format-patch: tie file-opening logic to output_directory\n  [3/3]: format-patch: support --output option\n\n builtin/log.c           | 37 ++++++++++++++++++++++---------------\n t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 15 deletions(-)\n\n-Peff\n"},{"id":"409073","messageId":"20201104132522.GA3030146@coredump.intra.peff.net","threadId":"54570","inReplyTo":"20201104132428.GA2491189@coredump.intra.peff.net","subject":"[PATCH 1/3] format-patch: refactor output selection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T13:25:22Z","receivedAt":"2020-11-04T13:25:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The --stdout and --output-directory options are mutually exclusive, but\nit's hard to tell from reading the code. We have three separate\nconditionals that check for use_stdout, and it's only after we've set up\nthe output_directory fully that we check whether the user also specified\n--stdout.\n\nInstead, let's check the exclusion explicitly first, then have a single\nconditional that handles stdout versus an output directory. This is\nslightly easier to follow now, and also will keep things sane when we\nadd another output mode in a future patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 9f939e6cdf..4c391ba3ca 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1942,20 +1942,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (rev.show_notes)\n \t\tload_display_notes(&rev.notes_opt);\n \n-\tif (!output_directory && !use_stdout)\n-\t\toutput_directory = config_output_directory;\n+\tif (use_stdout + !!output_directory > 1)\n+\t\tdie(_(\"specify only one of --stdout, --output, and --output-directory\"));\n \n-\tif (!use_stdout)\n-\t\toutput_directory = set_outdir(prefix, output_directory);\n-\telse\n+\tif (use_stdout) {\n \t\tsetup_pager();\n-\n-\tif (output_directory) {\n+\t} else {\n \t\tint saved;\n+\n+\t\tif (!output_directory)\n+\t\t\toutput_directory = config_output_directory;\n+\t\toutput_directory = set_outdir(prefix, output_directory);\n+\n \t\tif (rev.diffopt.use_color != GIT_COLOR_ALWAYS)\n \t\t\trev.diffopt.use_color = GIT_COLOR_NEVER;\n-\t\tif (use_stdout)\n-\t\t\tdie(_(\"standard output, or directory, which one?\"));\n \t\t/*\n \t\t * We consider <outdir> as 'outside of gitdir', therefore avoid\n \t\t * applying adjust_shared_perm in s-c-l-d.\n-- \n2.29.2.559.g8ec94df761\n\n"},{"id":"409074","messageId":"20201104132737.GB3030146@coredump.intra.peff.net","threadId":"54570","inReplyTo":"20201104132428.GA2491189@coredump.intra.peff.net","subject":"[PATCH 2/3] format-patch: tie file-opening logic to output_directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T13:27:37Z","receivedAt":"2020-11-04T13:27:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In format-patch we're either outputting to stdout or to individual files\nin an output directory (which maybe just \"./\"). Our logic for whether to\nopen a new file for each patch is checked with \"!use_stdout\", but it is\nequally correct to check for a non-NULL output_directory.\n\nThe distinction will matter when we add a new single-stream output in a\nfuture patch, when only one of the three methods will want individual\nfiles. Let's swap the logic here in preparation.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 4c391ba3ca..27c6d612e4 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1153,7 +1153,7 @@ static void get_notes_args(struct strvec *arg, struct rev_info *rev)\n \t}\n }\n \n-static void make_cover_letter(struct rev_info *rev, int use_stdout,\n+static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\t\t      struct commit *origin,\n \t\t\t      int nr, struct commit **list,\n \t\t\t      const char *branch_name,\n@@ -1173,7 +1173,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \n \tcommitter = git_committer_info(0);\n \n-\tif (!use_stdout &&\n+\tif (use_separate_file &&\n \t    open_next_file(NULL, rev->numbered_files ? NULL : \"cover-letter\", rev, quiet))\n \t\tdie(_(\"failed to create cover-letter file\"));\n \n@@ -2117,7 +2117,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (cover_letter) {\n \t\tif (thread)\n \t\t\tgen_message_id(&rev, \"cover\");\n-\t\tmake_cover_letter(&rev, use_stdout,\n+\t\tmake_cover_letter(&rev, !!output_directory,\n \t\t\t\t  origin, nr, list, branch_name, quiet);\n \t\tprint_bases(&bases, rev.diffopt.file);\n \t\tprint_signature(rev.diffopt.file);\n@@ -2172,7 +2172,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\tgen_message_id(&rev, oid_to_hex(&commit->object.oid));\n \t\t}\n \n-\t\tif (!use_stdout &&\n+\t\tif (output_directory &&\n \t\t    open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))\n \t\t\tdie(_(\"failed to create output files\"));\n \t\tshown = log_tree_commit(&rev, commit);\n@@ -2185,7 +2185,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t * the log; when using one file per patch, we do\n \t\t * not want the extra blank line.\n \t\t */\n-\t\tif (!use_stdout)\n+\t\tif (output_directory)\n \t\t\trev.shown_one = 0;\n \t\tif (shown) {\n \t\t\tprint_bases(&bases, rev.diffopt.file);\n@@ -2196,7 +2196,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\telse\n \t\t\t\tprint_signature(rev.diffopt.file);\n \t\t}\n-\t\tif (!use_stdout)\n+\t\tif (output_directory)\n \t\t\tfclose(rev.diffopt.file);\n \t}\n \tstop_progress(&progress);\n-- \n2.29.2.559.g8ec94df761\n\n"},{"id":"409075","messageId":"20201104132907.GC3030146@coredump.intra.peff.net","threadId":"54570","inReplyTo":"20201104132428.GA2491189@coredump.intra.peff.net","subject":"[PATCH 3/3] format-patch: support --output option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T13:29:07Z","receivedAt":"2020-11-04T13:29:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We've never intended to support diff's --output option in format-patch.\nAnd until baa4adc66a (parse-options: disable option abbreviation with\nPARSE_OPT_KEEP_UNKNOWN, 2019-01-27), it was impossible to trigger. We\nfirst parse the format-patch options before handing the remainder off to\nsetup_revisions(). Before that commit, we'd accept \"--output=foo\" as an\nabbreviation for \"--output-directory=foo\". But afterwards, we don't\ncheck abbreviations, and --output gets passed to the diff code.\n\nThis results in nonsense behavior and bugs. The diff code will have\nopened a filehandle at rev.diffopt.file, but we'll overwrite that with\nour own handles that we open for each individual patch file. So the\n--output file will always just be empty. But worse, the diff code also\nsets rev.diffopt.close_file, so log_tree_commit() will close the\nfilehandle itself. And then the main loop in cmd_format_patch() will try\nto close it again, resulting in a double-free.\n\nThe simplest solution would be to just disallow --output with\nformat-patch, as nobody ever intended it to work. However, we have\naccidentally documented it (because format-patch includes diff-options).\nAnd it does work with \"git log\", which writes the whole output to the\nspecified file. It's easy enough to make that work for format-patch,\ntoo: it's really the same as --stdout, but pointed at a specific file.\n\nWe can detect the use of the --output option by the \"close_file\" flag\n(note that we can't use rev.diffopt.file, since the diff setup will\notherwise set it to stdout). So we just need to unset that flag, but\ndon't have to do anything else. Our situation is otherwise exactly like\n--stdout (note that we don't fclose() the file, but nor does the stdout\ncase; exiting the program takes care of that for us).\n\nReported-by: Johannes Postler <johannes.postler@txture.io>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c           |  9 ++++++++-\n t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 27c6d612e4..ddb3ea7326 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (rev.show_notes)\n \t\tload_display_notes(&rev.notes_opt);\n \n-\tif (use_stdout + !!output_directory > 1)\n+\tif (use_stdout + rev.diffopt.close_file + !!output_directory > 1)\n \t\tdie(_(\"specify only one of --stdout, --output, and --output-directory\"));\n \n \tif (use_stdout) {\n \t\tsetup_pager();\n+\t} else if (rev.diffopt.close_file) {\n+\t\t/*\n+\t\t * The diff code parsed --output; it has already opened the\n+\t\t * file, but but we must instruct it not to close after each\n+\t\t * diff.\n+\t\t */\n+\t\trev.diffopt.close_file = 0;\n \t} else {\n \t\tint saved;\n \ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 294e76c860..64574c51c1 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1919,6 +1919,39 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n \ttest_path_is_dir patchset\n '\n \n+test_expect_success 'format-patch forbids multiple outputs' '\n+\trm -fr outfile outdir &&\n+\ttest_must_fail \\\n+\t\tgit format-patch --stdout --output=outfile &&\n+\ttest_must_fail \\\n+\t\tgit format-patch --stdout --output-directory=outdir &&\n+\ttest_must_fail \\\n+\t\tgit format-patch --output=outfile --output-directory=outdir\n+'\n+\n+test_expect_success 'configured outdir does not conflict with output options' '\n+\trm -fr outfile outdir &&\n+\ttest_config format.outputDirectory outdir &&\n+\tgit format-patch --stdout &&\n+\ttest_path_is_missing outdir &&\n+\tgit format-patch --output=outfile &&\n+\ttest_path_is_missing outdir\n+'\n+\n+test_expect_success 'format-patch --output' '\n+\trm -fr outfile &&\n+\tgit format-patch -3 --stdout HEAD >expect &&\n+\tgit format-patch -3 --output=outfile HEAD &&\n+\ttest_cmp expect outfile\n+'\n+\n+test_expect_success 'format-patch --cover-letter --output' '\n+\trm -fr outfile &&\n+\tgit format-patch --cover-letter -3 --stdout HEAD >expect &&\n+\tgit format-patch --cover-letter -3 --output=outfile HEAD &&\n+\ttest_cmp expect outfile\n+'\n+\n test_expect_success 'format-patch --base' '\n \tgit checkout patchid &&\n \n-- \n2.29.2.559.g8ec94df761\n"},{"id":"409099","messageId":"CAPig+cRG4uPL1TDpEOgOZwSEJfyt-d0XmsVz3hcCebgwbO_v_A@mail.gmail.com","threadId":"54570","inReplyTo":"20201104132522.GA3030146@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] format-patch: refactor output selection","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-04T17:01:53Z","receivedAt":"2020-11-04T17:02:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 4, 2020 at 8:25 AM Jeff King <peff@peff.net> wrote:\n> The --stdout and --output-directory options are mutually exclusive, but\n> it's hard to tell from reading the code. We have three separate\n> conditionals that check for use_stdout, and it's only after we've set up\n> the output_directory fully that we check whether the user also specified\n> --stdout.\n>\n> Instead, let's check the exclusion explicitly first, then have a single\n> conditional that handles stdout versus an output directory. This is\n> slightly easier to follow now, and also will keep things sane when we\n> add another output mode in a future patch.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/builtin/log.c b/builtin/log.c\n> @@ -1942,20 +1942,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> +       if (use_stdout + !!output_directory > 1)\n> +               die(_(\"specify only one of --stdout, --output, and --output-directory\"));\n\nIs mention of --output intentional here? The commit message only talks\nabout --stdout and --output-directory.\n\nIt's subjective, but \"mutually exclusive\" sounds a bit more consistent\nwith other similar error messages elsewhere:\n\n    --stdout, --output, and --output-directory are mutually exclusive\n"},{"id":"409101","messageId":"CAPig+cT8BTTwcORWB1zNWFy8DaohyibWFxojDiU-Na933abRqA@mail.gmail.com","threadId":"54570","inReplyTo":"20201104132737.GB3030146@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] format-patch: tie file-opening logic to output_directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-04T17:03:22Z","receivedAt":"2020-11-04T17:03:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 4, 2020 at 8:27 AM Jeff King <peff@peff.net> wrote:\n> In format-patch we're either outputting to stdout or to individual files\n> in an output directory (which maybe just \"./\"). Our logic for whether to\n\ns/maybe/may be/\n\n> open a new file for each patch is checked with \"!use_stdout\", but it is\n> equally correct to check for a non-NULL output_directory.\n>\n> The distinction will matter when we add a new single-stream output in a\n> future patch, when only one of the three methods will want individual\n> files. Let's swap the logic here in preparation.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"409102","messageId":"20201104171344.GA3047090@coredump.intra.peff.net","threadId":"54570","inReplyTo":"CAPig+cRG4uPL1TDpEOgOZwSEJfyt-d0XmsVz3hcCebgwbO_v_A@mail.gmail.com","subject":"Re: [PATCH 1/3] format-patch: refactor output selection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T17:13:44Z","receivedAt":"2020-11-04T17:13:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2020 at 12:01:53PM -0500, Eric Sunshine wrote:\n\n> > diff --git a/builtin/log.c b/builtin/log.c\n> > @@ -1942,20 +1942,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> > +       if (use_stdout + !!output_directory > 1)\n> > +               die(_(\"specify only one of --stdout, --output, and --output-directory\"));\n> \n> Is mention of --output intentional here? The commit message only talks\n> about --stdout and --output-directory.\n\nWhoops, thanks. I wrote this line after adding the new feature, but\nforgot to revise it when I rebased.\n\n> It's subjective, but \"mutually exclusive\" sounds a bit more consistent\n> with other similar error messages elsewhere:\n> \n>     --stdout, --output, and --output-directory are mutually exclusive\n\nYeah, that reads better. I remember I reworded it a few times to try to\nget it not-awkward, but I'm not sure how I failed to come up with that\nquite obvious wording. ;)\n\n-Peff\n"},{"id":"409104","messageId":"CAPig+cQ7t1by2X0xwddyo40sQdb9BPBKmERpqKR6oMD82kUwgg@mail.gmail.com","threadId":"54570","inReplyTo":"20201104132907.GC3030146@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] format-patch: support --output option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-04T17:27:30Z","receivedAt":"2020-11-04T17:27:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 4, 2020 at 8:29 AM Jeff King <peff@peff.net> wrote:\n> [...]\n> The simplest solution would be to just disallow --output with\n> format-patch, as nobody ever intended it to work. However, we have\n> accidentally documented it (because format-patch includes diff-options).\n> And it does work with \"git log\", which writes the whole output to the\n> specified file. It's easy enough to make that work for format-patch,\n> too: it's really the same as --stdout, but pointed at a specific file.\n> [...]\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/builtin/log.c b/builtin/log.c\n> @@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> +       } else if (rev.diffopt.close_file) {\n> +               /*\n> +                * The diff code parsed --output; it has already opened the\n> +                * file, but but we must instruct it not to close after each\n> +                * diff.\n> +                */\n> +               rev.diffopt.close_file = 0;\n>         } else {\n\nThe commit message's justification for supporting --output seems\nreasonable. However, my knee-jerk reaction to the implementation was\nthat it feels overly magical and a bit too hacky. I can see the logic\nin it but it also leaves a bad taste when the implementation has to\n\"undo\" a side-effect of some other piece of code, which makes it feel\nunplanned and fragile. The question which popped into my mind\nimmediately was \"why not handle --output explicitly via\nbuiltin_format_patch_options[] along with other first-class options?\".\nThis review comment may or may not be actionable; it's just expressing\nsurprise and a bit of nose-wrinkling I experienced.\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> @@ -1919,6 +1919,39 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n> +test_expect_success 'format-patch forbids multiple outputs' '\n> +       rm -fr outfile outdir &&\n> +       test_must_fail \\\n> +               git format-patch --stdout --output=outfile &&\n> +       test_must_fail \\\n> +               git format-patch --stdout --output-directory=outdir &&\n> +       test_must_fail \\\n> +               git format-patch --output=outfile --output-directory=outdir\n> +'\n\nI would have expected to see this test added in patch [1/3], and then\nextended by this patch with the addition of the --output case(s).\n"},{"id":"409118","messageId":"xmqq361pngs4.fsf@gitster.c.googlers.com","threadId":"54570","inReplyTo":"20201104132907.GC3030146@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] format-patch: support --output option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T18:00:43Z","receivedAt":"2020-11-04T18:00:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +test_expect_success 'format-patch --output' '\n> +\trm -fr outfile &&\n> +\tgit format-patch -3 --stdout HEAD >expect &&\n> +\tgit format-patch -3 --output=outfile HEAD &&\n> +\ttest_cmp expect outfile\n> +'\n> +\n> +test_expect_success 'format-patch --cover-letter --output' '\n> +\trm -fr outfile &&\n> +\tgit format-patch --cover-letter -3 --stdout HEAD >expect &&\n> +\tgit format-patch --cover-letter -3 --output=outfile HEAD &&\n> +\ttest_cmp expect outfile\n> +'\n\nIt is pleasing to see an obvious and clear demonstration that\n\"--output=X\" is equivalent to \"--stdout >X\".\n"},{"id":"409123","messageId":"20201104191559.GA3053889@coredump.intra.peff.net","threadId":"54570","inReplyTo":"CAPig+cQ7t1by2X0xwddyo40sQdb9BPBKmERpqKR6oMD82kUwgg@mail.gmail.com","subject":"Re: [PATCH 3/3] format-patch: support --output option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T19:15:59Z","receivedAt":"2020-11-04T19:16:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2020 at 12:27:30PM -0500, Eric Sunshine wrote:\n\n> > diff --git a/builtin/log.c b/builtin/log.c\n> > @@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> > +       } else if (rev.diffopt.close_file) {\n> > +               /*\n> > +                * The diff code parsed --output; it has already opened the\n> > +                * file, but but we must instruct it not to close after each\n> > +                * diff.\n> > +                */\n> > +               rev.diffopt.close_file = 0;\n> >         } else {\n> \n> The commit message's justification for supporting --output seems\n> reasonable. However, my knee-jerk reaction to the implementation was\n> that it feels overly magical and a bit too hacky. I can see the logic\n> in it but it also leaves a bad taste when the implementation has to\n> \"undo\" a side-effect of some other piece of code, which makes it feel\n> unplanned and fragile. The question which popped into my mind\n> immediately was \"why not handle --output explicitly via\n> builtin_format_patch_options[] along with other first-class options?\".\n> This review comment may or may not be actionable; it's just expressing\n> surprise and a bit of nose-wrinkling I experienced.\n\nI agree it's a bit magical. I didn't really consider writing a new\n\"--output\" option for format-patch, because I came at it from the angle\nof \"let's unbreak the existing diff --output option\". Which isn't\nnecessarily a defense, but just an explanation.\n\nFWIW, that unsetting of rev.diffopt.close_file is exactly how git-log\ndoes it. So while I agree it's a bit ugly, this is the intended way for\n--output to be used across multiple diffs, and with log_tree_commit().\nI'd prefer to go this way for now, and if somebody wants to make it less\nugly, they can clean up all of the callers in one go.\n\nAmusingly, 6ea57703f6 (log: prepare log/log-tree to reuse the\ndiffopt.close_file attribute, 2016-06-22) which introduced this code\nforesaw the format-patch bug here.\n\n-Peff\n"},{"id":"409124","messageId":"20201104192645.GA3059114@coredump.intra.peff.net","threadId":"54570","inReplyTo":"20201104132428.GA2491189@coredump.intra.peff.net","subject":"[PATCH v2] format-patch --output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T19:26:45Z","receivedAt":"2020-11-04T19:26:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2020 at 08:24:28AM -0500, Jeff King wrote:\n\n> The issue is that \"--output\" was never supposed to work with\n> format-patch. But a subtle change in the option parsing a while back\n> caused it to be respected. And as you noticed, the documentation\n> mistakenly mentions the option, since format-patch includes the standard\n> diff-options text.\n> \n> So one obvious fix would be to forbid it and adjust the documentation.\n> But because of the way the option parsers interact, it's surprisingly\n> hard to do so cleanly. It's actually easier to just make it do something\n> useful (i.e., behave like --stdout but sent to a file). So I did that\n> instead.\n> \n>   [1/3]: format-patch: refactor output selection\n>   [2/3]: format-patch: tie file-opening logic to output_directory\n>   [3/3]: format-patch: support --output option\n\nHere's a re-roll taking into account the comments from Eric. The only\nthing I didn't do is rewrite --output as a format-patch option. As I\nsaid in the thread, I'd rather keep parity with how git-log works here\n(though I don't mind if somebody wants to do further clean up on top).\n\n  [1/3]: format-patch: refactor output selection\n  [2/3]: format-patch: tie file-opening logic to output_directory\n  [3/3]: format-patch: support --output option\n\n builtin/log.c           | 37 ++++++++++++++++++++++---------------\n t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 15 deletions(-)\n\n\nRange diff from v1:\n\n1:  49e8b54549 ! 1:  9206d6852b format-patch: refactor output selection\n    @@ Commit message\n         slightly easier to follow now, and also will keep things sane when we\n         add another output mode in a future patch.\n     \n    +    We'll add a few tests as well, covering the mutual exclusion and the\n    +    fact that we are not confused by a configured output directory.\n    +\n         Signed-off-by: Jeff King <peff@peff.net>\n     \n      ## builtin/log.c ##\n    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n     -\tif (!output_directory && !use_stdout)\n     -\t\toutput_directory = config_output_directory;\n     +\tif (use_stdout + !!output_directory > 1)\n    -+\t\tdie(_(\"specify only one of --stdout, --output, and --output-directory\"));\n    ++\t\tdie(_(\"--stdout and --output-directory are mutually exclusive\"));\n      \n     -\tif (!use_stdout)\n     -\t\toutput_directory = set_outdir(prefix, output_directory);\n    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n      \t\t/*\n      \t\t * We consider <outdir> as 'outside of gitdir', therefore avoid\n      \t\t * applying adjust_shared_perm in s-c-l-d.\n    +\n    + ## t/t4014-format-patch.sh ##\n    +@@ t/t4014-format-patch.sh: test_expect_success 'format-patch -o overrides format.outputDirectory' '\n    + \ttest_path_is_dir patchset\n    + '\n    + \n    ++test_expect_success 'format-patch forbids multiple outputs' '\n    ++\trm -fr outdir &&\n    ++\ttest_must_fail \\\n    ++\t\tgit format-patch --stdout --output-directory=outdir\n    ++'\n    ++\n    ++test_expect_success 'configured outdir does not conflict with output options' '\n    ++\trm -fr outfile outdir &&\n    ++\ttest_config format.outputDirectory outdir &&\n    ++\tgit format-patch --stdout &&\n    ++\ttest_path_is_missing outdir\n    ++'\n    ++\n    + test_expect_success 'format-patch --base' '\n    + \tgit checkout patchid &&\n    + \n2:  884c06861d ! 2:  9dc30924b2 format-patch: tie file-opening logic to output_directory\n    @@ Commit message\n         format-patch: tie file-opening logic to output_directory\n     \n         In format-patch we're either outputting to stdout or to individual files\n    -    in an output directory (which maybe just \"./\"). Our logic for whether to\n    -    open a new file for each patch is checked with \"!use_stdout\", but it is\n    -    equally correct to check for a non-NULL output_directory.\n    +    in an output directory (which may be just \"./\"). Our logic for whether\n    +    to open a new file for each patch is checked with \"!use_stdout\", but it\n    +    is equally correct to check for a non-NULL output_directory.\n     \n         The distinction will matter when we add a new single-stream output in a\n         future patch, when only one of the three methods will want individual\n3:  8befceb150 ! 3:  2b0fab9b50 format-patch: support --output option\n    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n      \t\tload_display_notes(&rev.notes_opt);\n      \n     -\tif (use_stdout + !!output_directory > 1)\n    +-\t\tdie(_(\"--stdout and --output-directory are mutually exclusive\"));\n     +\tif (use_stdout + rev.diffopt.close_file + !!output_directory > 1)\n    - \t\tdie(_(\"specify only one of --stdout, --output, and --output-directory\"));\n    ++\t\tdie(_(\"--stdout, --output, and --output-directory are mutually exclusive\"));\n      \n      \tif (use_stdout) {\n      \t\tsetup_pager();\n    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n     \n      ## t/t4014-format-patch.sh ##\n     @@ t/t4014-format-patch.sh: test_expect_success 'format-patch -o overrides format.outputDirectory' '\n    - \ttest_path_is_dir patchset\n    - '\n    - \n    -+test_expect_success 'format-patch forbids multiple outputs' '\n    -+\trm -fr outfile outdir &&\n    + test_expect_success 'format-patch forbids multiple outputs' '\n    + \trm -fr outdir &&\n    + \ttest_must_fail \\\n    +-\t\tgit format-patch --stdout --output-directory=outdir\n    ++\t\tgit format-patch --stdout --output-directory=outdir &&\n     +\ttest_must_fail \\\n     +\t\tgit format-patch --stdout --output=outfile &&\n     +\ttest_must_fail \\\n    -+\t\tgit format-patch --stdout --output-directory=outdir &&\n    -+\ttest_must_fail \\\n     +\t\tgit format-patch --output=outfile --output-directory=outdir\n    -+'\n    -+\n    -+test_expect_success 'configured outdir does not conflict with output options' '\n    -+\trm -fr outfile outdir &&\n    -+\ttest_config format.outputDirectory outdir &&\n    -+\tgit format-patch --stdout &&\n    + '\n    + \n    + test_expect_success 'configured outdir does not conflict with output options' '\n    + \trm -fr outfile outdir &&\n    + \ttest_config format.outputDirectory outdir &&\n    + \tgit format-patch --stdout &&\n     +\ttest_path_is_missing outdir &&\n     +\tgit format-patch --output=outfile &&\n    -+\ttest_path_is_missing outdir\n    -+'\n    -+\n    + \ttest_path_is_missing outdir\n    + '\n    + \n     +test_expect_success 'format-patch --output' '\n     +\trm -fr outfile &&\n     +\tgit format-patch -3 --stdout HEAD >expect &&\n"},{"id":"409125","messageId":"20201104192831.GA3060275@coredump.intra.peff.net","threadId":"54570","inReplyTo":"20201104192645.GA3059114@coredump.intra.peff.net","subject":"[PATCH v2 1/3] format-patch: refactor output selection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T19:28:31Z","receivedAt":"2020-11-04T19:28:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The --stdout and --output-directory options are mutually exclusive, but\nit's hard to tell from reading the code. We have three separate\nconditionals that check for use_stdout, and it's only after we've set up\nthe output_directory fully that we check whether the user also specified\n--stdout.\n\nInstead, let's check the exclusion explicitly first, then have a single\nconditional that handles stdout versus an output directory. This is\nslightly easier to follow now, and also will keep things sane when we\nadd another output mode in a future patch.\n\nWe'll add a few tests as well, covering the mutual exclusion and the\nfact that we are not confused by a configured output directory.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c           | 18 +++++++++---------\n t/t4014-format-patch.sh | 13 +++++++++++++\n 2 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 9f939e6cdf..fbff5493d2 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1942,20 +1942,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (rev.show_notes)\n \t\tload_display_notes(&rev.notes_opt);\n \n-\tif (!output_directory && !use_stdout)\n-\t\toutput_directory = config_output_directory;\n+\tif (use_stdout + !!output_directory > 1)\n+\t\tdie(_(\"--stdout and --output-directory are mutually exclusive\"));\n \n-\tif (!use_stdout)\n-\t\toutput_directory = set_outdir(prefix, output_directory);\n-\telse\n+\tif (use_stdout) {\n \t\tsetup_pager();\n-\n-\tif (output_directory) {\n+\t} else {\n \t\tint saved;\n+\n+\t\tif (!output_directory)\n+\t\t\toutput_directory = config_output_directory;\n+\t\toutput_directory = set_outdir(prefix, output_directory);\n+\n \t\tif (rev.diffopt.use_color != GIT_COLOR_ALWAYS)\n \t\t\trev.diffopt.use_color = GIT_COLOR_NEVER;\n-\t\tif (use_stdout)\n-\t\t\tdie(_(\"standard output, or directory, which one?\"));\n \t\t/*\n \t\t * We consider <outdir> as 'outside of gitdir', therefore avoid\n \t\t * applying adjust_shared_perm in s-c-l-d.\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 294e76c860..e8d6156a6a 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1919,6 +1919,19 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n \ttest_path_is_dir patchset\n '\n \n+test_expect_success 'format-patch forbids multiple outputs' '\n+\trm -fr outdir &&\n+\ttest_must_fail \\\n+\t\tgit format-patch --stdout --output-directory=outdir\n+'\n+\n+test_expect_success 'configured outdir does not conflict with output options' '\n+\trm -fr outdir &&\n+\ttest_config format.outputDirectory outdir &&\n+\tgit format-patch --stdout &&\n+\ttest_path_is_missing outdir\n+'\n+\n test_expect_success 'format-patch --base' '\n \tgit checkout patchid &&\n \n-- \n2.29.2.559.g8ec94df761\n\n"},{"id":"409126","messageId":"20201104192834.GB3060275@coredump.intra.peff.net","threadId":"54570","inReplyTo":"20201104192645.GA3059114@coredump.intra.peff.net","subject":"[PATCH v2 2/3] format-patch: tie file-opening logic to output_directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T19:28:34Z","receivedAt":"2020-11-04T19:28:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In format-patch we're either outputting to stdout or to individual files\nin an output directory (which may be just \"./\"). Our logic for whether\nto open a new file for each patch is checked with \"!use_stdout\", but it\nis equally correct to check for a non-NULL output_directory.\n\nThe distinction will matter when we add a new single-stream output in a\nfuture patch, when only one of the three methods will want individual\nfiles. Let's swap the logic here in preparation.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex fbff5493d2..927156fb85 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1153,7 +1153,7 @@ static void get_notes_args(struct strvec *arg, struct rev_info *rev)\n \t}\n }\n \n-static void make_cover_letter(struct rev_info *rev, int use_stdout,\n+static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\t\t      struct commit *origin,\n \t\t\t      int nr, struct commit **list,\n \t\t\t      const char *branch_name,\n@@ -1173,7 +1173,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \n \tcommitter = git_committer_info(0);\n \n-\tif (!use_stdout &&\n+\tif (use_separate_file &&\n \t    open_next_file(NULL, rev->numbered_files ? NULL : \"cover-letter\", rev, quiet))\n \t\tdie(_(\"failed to create cover-letter file\"));\n \n@@ -2117,7 +2117,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (cover_letter) {\n \t\tif (thread)\n \t\t\tgen_message_id(&rev, \"cover\");\n-\t\tmake_cover_letter(&rev, use_stdout,\n+\t\tmake_cover_letter(&rev, !!output_directory,\n \t\t\t\t  origin, nr, list, branch_name, quiet);\n \t\tprint_bases(&bases, rev.diffopt.file);\n \t\tprint_signature(rev.diffopt.file);\n@@ -2172,7 +2172,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\tgen_message_id(&rev, oid_to_hex(&commit->object.oid));\n \t\t}\n \n-\t\tif (!use_stdout &&\n+\t\tif (output_directory &&\n \t\t    open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))\n \t\t\tdie(_(\"failed to create output files\"));\n \t\tshown = log_tree_commit(&rev, commit);\n@@ -2185,7 +2185,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t * the log; when using one file per patch, we do\n \t\t * not want the extra blank line.\n \t\t */\n-\t\tif (!use_stdout)\n+\t\tif (output_directory)\n \t\t\trev.shown_one = 0;\n \t\tif (shown) {\n \t\t\tprint_bases(&bases, rev.diffopt.file);\n@@ -2196,7 +2196,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\telse\n \t\t\t\tprint_signature(rev.diffopt.file);\n \t\t}\n-\t\tif (!use_stdout)\n+\t\tif (output_directory)\n \t\t\tfclose(rev.diffopt.file);\n \t}\n \tstop_progress(&progress);\n-- \n2.29.2.559.g8ec94df761\n\n"},{"id":"409127","messageId":"20201104192836.GC3060275@coredump.intra.peff.net","threadId":"54570","inReplyTo":"20201104192645.GA3059114@coredump.intra.peff.net","subject":"[PATCH v2 3/3] format-patch: support --output option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T19:28:36Z","receivedAt":"2020-11-04T19:28:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We've never intended to support diff's --output option in format-patch.\nAnd until baa4adc66a (parse-options: disable option abbreviation with\nPARSE_OPT_KEEP_UNKNOWN, 2019-01-27), it was impossible to trigger. We\nfirst parse the format-patch options before handing the remainder off to\nsetup_revisions(). Before that commit, we'd accept \"--output=foo\" as an\nabbreviation for \"--output-directory=foo\". But afterwards, we don't\ncheck abbreviations, and --output gets passed to the diff code.\n\nThis results in nonsense behavior and bugs. The diff code will have\nopened a filehandle at rev.diffopt.file, but we'll overwrite that with\nour own handles that we open for each individual patch file. So the\n--output file will always just be empty. But worse, the diff code also\nsets rev.diffopt.close_file, so log_tree_commit() will close the\nfilehandle itself. And then the main loop in cmd_format_patch() will try\nto close it again, resulting in a double-free.\n\nThe simplest solution would be to just disallow --output with\nformat-patch, as nobody ever intended it to work. However, we have\naccidentally documented it (because format-patch includes diff-options).\nAnd it does work with \"git log\", which writes the whole output to the\nspecified file. It's easy enough to make that work for format-patch,\ntoo: it's really the same as --stdout, but pointed at a specific file.\n\nWe can detect the use of the --output option by the \"close_file\" flag\n(note that we can't use rev.diffopt.file, since the diff setup will\notherwise set it to stdout). So we just need to unset that flag, but\ndon't have to do anything else. Our situation is otherwise exactly like\n--stdout (note that we don't fclose() the file, but nor does the stdout\ncase; exiting the program takes care of that for us).\n\nReported-by: Johannes Postler <johannes.postler@txture.io>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c           | 11 +++++++++--\n t/t4014-format-patch.sh | 26 +++++++++++++++++++++++---\n 2 files changed, 32 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 927156fb85..662c041de2 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (rev.show_notes)\n \t\tload_display_notes(&rev.notes_opt);\n \n-\tif (use_stdout + !!output_directory > 1)\n-\t\tdie(_(\"--stdout and --output-directory are mutually exclusive\"));\n+\tif (use_stdout + rev.diffopt.close_file + !!output_directory > 1)\n+\t\tdie(_(\"--stdout, --output, and --output-directory are mutually exclusive\"));\n \n \tif (use_stdout) {\n \t\tsetup_pager();\n+\t} else if (rev.diffopt.close_file) {\n+\t\t/*\n+\t\t * The diff code parsed --output; it has already opened the\n+\t\t * file, but but we must instruct it not to close after each\n+\t\t * diff.\n+\t\t */\n+\t\trev.diffopt.close_file = 0;\n \t} else {\n \t\tint saved;\n \ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex e8d6156a6a..42588bf6e1 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1920,18 +1920,38 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n '\n \n test_expect_success 'format-patch forbids multiple outputs' '\n-\trm -fr outdir &&\n+\trm -fr outfile outdir &&\n \ttest_must_fail \\\n-\t\tgit format-patch --stdout --output-directory=outdir\n+\t\tgit format-patch --stdout --output-directory=outdir &&\n+\ttest_must_fail \\\n+\t\tgit format-patch --stdout --output=outfile &&\n+\ttest_must_fail \\\n+\t\tgit format-patch --output=outfile --output-directory=outdir\n '\n \n test_expect_success 'configured outdir does not conflict with output options' '\n-\trm -fr outdir &&\n+\trm -fr outfile outdir &&\n \ttest_config format.outputDirectory outdir &&\n \tgit format-patch --stdout &&\n+\ttest_path_is_missing outdir &&\n+\tgit format-patch --output=outfile &&\n \ttest_path_is_missing outdir\n '\n \n+test_expect_success 'format-patch --output' '\n+\trm -fr outfile &&\n+\tgit format-patch -3 --stdout HEAD >expect &&\n+\tgit format-patch -3 --output=outfile HEAD &&\n+\ttest_cmp expect outfile\n+'\n+\n+test_expect_success 'format-patch --cover-letter --output' '\n+\trm -fr outfile &&\n+\tgit format-patch --cover-letter -3 --stdout HEAD >expect &&\n+\tgit format-patch --cover-letter -3 --output=outfile HEAD &&\n+\ttest_cmp expect outfile\n+'\n+\n test_expect_success 'format-patch --base' '\n \tgit checkout patchid &&\n \n-- \n2.29.2.559.g8ec94df761\n"},{"id":"409137","messageId":"CAPig+cSOB+raZHEpSeqnZZ-VHNqLqVyrfVUA8fCr97mv8oSBFw@mail.gmail.com","threadId":"54570","inReplyTo":"20201104191559.GA3053889@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] format-patch: support --output option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-04T20:16:04Z","receivedAt":"2020-11-04T20:16:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 4, 2020 at 2:16 PM Jeff King <peff@peff.net> wrote:\n> On Wed, Nov 04, 2020 at 12:27:30PM -0500, Eric Sunshine wrote:\n> > The commit message's justification for supporting --output seems\n> > reasonable. However, my knee-jerk reaction to the implementation was\n> > that it feels overly magical and a bit too hacky. I can see the logic\n> > in it but it also leaves a bad taste when the implementation has to\n> > \"undo\" a side-effect of some other piece of code, which makes it feel\n> > unplanned and fragile. [...]\n>\n> FWIW, that unsetting of rev.diffopt.close_file is exactly how git-log\n> does it. So while I agree it's a bit ugly, this is the intended way for\n> --output to be used across multiple diffs, and with log_tree_commit().\n> I'd prefer to go this way for now, and if somebody wants to make it less\n> ugly, they can clean up all of the callers in one go.\n\nIf you intend to re-roll, it might make sense to include this\nexplanation in the commit message since existing precedent helps sell\nthe ugliness (bad taste), making for a less negative knee-jerk\nreaction.\n"},{"id":"409196","messageId":"CAO9fQ5rAjO0Q+AUA_-BqQqhunOhjadUB5YAU7+W2Pwi9OpUyGQ@mail.gmail.com","threadId":"54570","inReplyTo":"20201104192645.GA3059114@coredump.intra.peff.net","subject":"Re: [PATCH v2] format-patch --output","fromName":"Johannes Postler","fromEmail":"johannes.postler@txture.io","sentAt":"2020-11-05T06:30:28Z","receivedAt":"2020-11-05T06:30:59Z","isPatch":true,"sender":{"key":"johannes.postler@txture.io","avatar":null},"body":"Thanks a lot everyone for fixing this so quickly!\nPlease let me know if there is anything else I can do.\nBest,\njohannes\n\nJohannes Postler\nDevOps & Consulting\n\n\nTxture - The Cloud Transformation Platform\nGrabenweg 68\nA-6020 Innsbruck\njohannes.postler@txture.io\nwww.txture.io\n\n\n\nJohannes Postler\nDevOps & Consulting\n\n\n\nTxture - The Cloud Transformation Platform\nGrabenweg 68\nA-6020 Innsbruck\njohannes.postler@txture.io\nwww.txture.io\n\nFollow us:\n\n\nOn Wed, Nov 4, 2020 at 8:26 PM Jeff King <peff@peff.net> wrote:\n>\n> On Wed, Nov 04, 2020 at 08:24:28AM -0500, Jeff King wrote:\n>\n> > The issue is that \"--output\" was never supposed to work with\n> > format-patch. But a subtle change in the option parsing a while back\n> > caused it to be respected. And as you noticed, the documentation\n> > mistakenly mentions the option, since format-patch includes the standard\n> > diff-options text.\n> >\n> > So one obvious fix would be to forbid it and adjust the documentation.\n> > But because of the way the option parsers interact, it's surprisingly\n> > hard to do so cleanly. It's actually easier to just make it do something\n> > useful (i.e., behave like --stdout but sent to a file). So I did that\n> > instead.\n> >\n> >   [1/3]: format-patch: refactor output selection\n> >   [2/3]: format-patch: tie file-opening logic to output_directory\n> >   [3/3]: format-patch: support --output option\n>\n> Here's a re-roll taking into account the comments from Eric. The only\n> thing I didn't do is rewrite --output as a format-patch option. As I\n> said in the thread, I'd rather keep parity with how git-log works here\n> (though I don't mind if somebody wants to do further clean up on top).\n>\n>   [1/3]: format-patch: refactor output selection\n>   [2/3]: format-patch: tie file-opening logic to output_directory\n>   [3/3]: format-patch: support --output option\n>\n>  builtin/log.c           | 37 ++++++++++++++++++++++---------------\n>  t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++\n>  2 files changed, 55 insertions(+), 15 deletions(-)\n>\n>\n> Range diff from v1:\n>\n> 1:  49e8b54549 ! 1:  9206d6852b format-patch: refactor output selection\n>     @@ Commit message\n>          slightly easier to follow now, and also will keep things sane when we\n>          add another output mode in a future patch.\n>\n>     +    We'll add a few tests as well, covering the mutual exclusion and the\n>     +    fact that we are not confused by a configured output directory.\n>     +\n>          Signed-off-by: Jeff King <peff@peff.net>\n>\n>       ## builtin/log.c ##\n>     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n>      -  if (!output_directory && !use_stdout)\n>      -          output_directory = config_output_directory;\n>      +  if (use_stdout + !!output_directory > 1)\n>     -+          die(_(\"specify only one of --stdout, --output, and --output-directory\"));\n>     ++          die(_(\"--stdout and --output-directory are mutually exclusive\"));\n>\n>      -  if (!use_stdout)\n>      -          output_directory = set_outdir(prefix, output_directory);\n>     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n>                 /*\n>                  * We consider <outdir> as 'outside of gitdir', therefore avoid\n>                  * applying adjust_shared_perm in s-c-l-d.\n>     +\n>     + ## t/t4014-format-patch.sh ##\n>     +@@ t/t4014-format-patch.sh: test_expect_success 'format-patch -o overrides format.outputDirectory' '\n>     +   test_path_is_dir patchset\n>     + '\n>     +\n>     ++test_expect_success 'format-patch forbids multiple outputs' '\n>     ++  rm -fr outdir &&\n>     ++  test_must_fail \\\n>     ++          git format-patch --stdout --output-directory=outdir\n>     ++'\n>     ++\n>     ++test_expect_success 'configured outdir does not conflict with output options' '\n>     ++  rm -fr outfile outdir &&\n>     ++  test_config format.outputDirectory outdir &&\n>     ++  git format-patch --stdout &&\n>     ++  test_path_is_missing outdir\n>     ++'\n>     ++\n>     + test_expect_success 'format-patch --base' '\n>     +   git checkout patchid &&\n>     +\n> 2:  884c06861d ! 2:  9dc30924b2 format-patch: tie file-opening logic to output_directory\n>     @@ Commit message\n>          format-patch: tie file-opening logic to output_directory\n>\n>          In format-patch we're either outputting to stdout or to individual files\n>     -    in an output directory (which maybe just \"./\"). Our logic for whether to\n>     -    open a new file for each patch is checked with \"!use_stdout\", but it is\n>     -    equally correct to check for a non-NULL output_directory.\n>     +    in an output directory (which may be just \"./\"). Our logic for whether\n>     +    to open a new file for each patch is checked with \"!use_stdout\", but it\n>     +    is equally correct to check for a non-NULL output_directory.\n>\n>          The distinction will matter when we add a new single-stream output in a\n>          future patch, when only one of the three methods will want individual\n> 3:  8befceb150 ! 3:  2b0fab9b50 format-patch: support --output option\n>     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n>                 load_display_notes(&rev.notes_opt);\n>\n>      -  if (use_stdout + !!output_directory > 1)\n>     +-          die(_(\"--stdout and --output-directory are mutually exclusive\"));\n>      +  if (use_stdout + rev.diffopt.close_file + !!output_directory > 1)\n>     -           die(_(\"specify only one of --stdout, --output, and --output-directory\"));\n>     ++          die(_(\"--stdout, --output, and --output-directory are mutually exclusive\"));\n>\n>         if (use_stdout) {\n>                 setup_pager();\n>     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n>\n>       ## t/t4014-format-patch.sh ##\n>      @@ t/t4014-format-patch.sh: test_expect_success 'format-patch -o overrides format.outputDirectory' '\n>     -   test_path_is_dir patchset\n>     - '\n>     -\n>     -+test_expect_success 'format-patch forbids multiple outputs' '\n>     -+  rm -fr outfile outdir &&\n>     + test_expect_success 'format-patch forbids multiple outputs' '\n>     +   rm -fr outdir &&\n>     +   test_must_fail \\\n>     +-          git format-patch --stdout --output-directory=outdir\n>     ++          git format-patch --stdout --output-directory=outdir &&\n>      +  test_must_fail \\\n>      +          git format-patch --stdout --output=outfile &&\n>      +  test_must_fail \\\n>     -+          git format-patch --stdout --output-directory=outdir &&\n>     -+  test_must_fail \\\n>      +          git format-patch --output=outfile --output-directory=outdir\n>     -+'\n>     -+\n>     -+test_expect_success 'configured outdir does not conflict with output options' '\n>     -+  rm -fr outfile outdir &&\n>     -+  test_config format.outputDirectory outdir &&\n>     -+  git format-patch --stdout &&\n>     + '\n>     +\n>     + test_expect_success 'configured outdir does not conflict with output options' '\n>     +   rm -fr outfile outdir &&\n>     +   test_config format.outputDirectory outdir &&\n>     +   git format-patch --stdout &&\n>      +  test_path_is_missing outdir &&\n>      +  git format-patch --output=outfile &&\n>     -+  test_path_is_missing outdir\n>     -+'\n>     -+\n>     +   test_path_is_missing outdir\n>     + '\n>     +\n>      +test_expect_success 'format-patch --output' '\n>      +  rm -fr outfile &&\n>      +  git format-patch -3 --stdout HEAD >expect &&\n"}]}