{"thread":{"id":"65870","subject":"[PATCH] history: close COMMIT_EDITMSG before launching the editor","startedAt":"2026-06-25T18:33:49Z","lastAt":"2026-06-25T20:12:43Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"546425","messageId":"pull.2158.git.1782412427801.gitgitgadget@gmail.com","threadId":"65870","inReplyTo":null,"subject":"[PATCH] history: close COMMIT_EDITMSG before launching the editor","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-06-25T18:33:46Z","receivedAt":"2026-06-25T18:33:49Z","isPatch":true,"body":"From: Johannes Schindelin \n\nThe `git history reword` and `git history fixup` subcommands prepare the\ncommit message by writing it to COMMIT_EDITMSG and then opening that same\nfile a second time, in append mode, through `wt_status`'s `fp` field to\nappend the status information. That second handle is never closed before\n`launch_editor()` runs, so the editor is started while git still holds\nthe file open.\n\nEverywhere this leaks a file descriptor, but on Windows it is outright\nbroken: a process cannot replace a file that another process keeps open,\nso an editor that rewrites COMMIT_EDITMSG by creating a fresh file in its\nplace fails. This surfaced while running Git for Windows' test suite with\nBusyBox' `ash` as the POSIX shell: the fake editor's `cp message \"$1\"`\naborts with \"cp: can't create '.../COMMIT_EDITMSG': File exists\" (MSYS2's\ncoreutils `cp` hides the problem via its POSIX unlink emulation, BusyBox'\nnative `cp` does not), making t3451-history-reword and t3453-history-fixup\nfail wholesale.\n\nClose the handle once the status has been written, before handing the\nfile off to the editor.\n\nAssisted-by: Opus 4.8\nSigned-off-by: Johannes Schindelin \n---\n history: close COMMIT_EDITMSG before launching the editor\n \n I noticed this problem while trying to whip MinGit-BusyBox into a better\n shape during the -rc phase. Technically, this is not a fix for a\n regression during the v2.55.0 period, but I figured it'd be better to\n send it now anyway than to forget about sending it after v2.55.0 is\n released.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2158%2Fdscho%2Ffix-fd-leak-in-history-reword-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2158/dscho/fix-fd-leak-in-history-reword-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2158\n\n builtin/history.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/builtin/history.c b/builtin/history.c\nindex 9526938085..4a5d9192f3 100644\n--- a/builtin/history.c\n+++ b/builtin/history.c\n@@ -74,6 +74,14 @@ static int fill_commit_message(struct repository *repo,\n \twt_status_collect_free_buffers(&s);\n \tstring_list_clear_func(&s.change, change_data_free);\n \n+\t/*\n+\t * Close the handle before launching the editor: on Windows an open\n+\t * handle would prevent the editor from replacing the file (e.g.\n+\t * BusyBox' `ash` cannot overwrite a file that another process keeps\n+\t * open), and leaving it open leaks the descriptor everywhere else.\n+\t */\n+\tfclose(s.fp);\n+\n \tstrbuf_reset(out);\n \tif (launch_editor(path, out, NULL)) {\n \t\tfprintf(stderr, _(\"Aborting commit as launching the editor failed.\\n\"));\n\nbase-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0\n-- \ngitgitgadget\n"},{"id":"546428","messageId":"xmqqh5mqfkpv.fsf@gitster.g","threadId":"65870","inReplyTo":"pull.2158.git.1782412427801.gitgitgadget@gmail.com","subject":"Re* [PATCH] history: close COMMIT_EDITMSG before launching the editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-25T20:06:52Z","receivedAt":"2026-06-25T20:06:55Z","isPatch":true,"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Close the handle once the status has been written, before handing the\n> file off to the editor.\n>\n> Assisted-by: Opus 4.8\n\nDo we even need this?\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> history: close COMMIT_EDITMSG before launching the editor\n> \n> I noticed this problem while trying to whip MinGit-BusyBox into a better\n> shape during the -rc phase. Technically, this is not a fix for a\n> regression during the v2.55.0 period, but I figured it'd be better to\n> send it now anyway than to forget about sending it after v2.55.0 is\n> released.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2158%2Fdscho%2Ffix-fd-leak-in-history-reword-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2158/dscho/fix-fd-leak-in-history-reword-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/2158\n>\n> builtin/history.c | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n>\n> diff --git a/builtin/history.c b/builtin/history.c\n> index 9526938085..4a5d9192f3 100644\n> --- a/builtin/history.c\n> +++ b/builtin/history.c\n> @@ -74,6 +74,14 @@ static int fill_commit_message(struct repository *repo,\n> \twt_status_collect_free_buffers(&s);\n> \tstring_list_clear_func(&s.change, change_data_free);\n> \n> +\t/*\n> +\t * Close the handle before launching the editor: on Windows an open\n> +\t * handle would prevent the editor from replacing the file (e.g.\n> +\t * BusyBox' `ash` cannot overwrite a file that another process keeps\n> +\t * open), and leaving it open leaks the descriptor everywhere else.\n> +\t */\n> +\tfclose(s.fp);\n> +\n> \tstrbuf_reset(out);\n> \tif (launch_editor(path, out, NULL)) {\n> \t\tfprintf(stderr, _(\"Aborting commit as launching the editor failed.\\n\"));\n\nThe function is extremely sloppy beyond words X-<. Thanks for\ntaking the first step to clean it up.\n\n * It calls git_path_commit_editmsg() to obtain a constant pathname\n into \"const char *path\", but then the part that leads to this\n file stream leak does not even use that \"path\" constant. It\n makes two independent calls to git_path_commit_editmsg()!\n\n * The function first calls write_file_buf() to the file, which is a\n convenience function when you have something you need to write\n upfront and just want to write it and be done with it.\n\n * And then the unclosed file stream you just fixed.\n\nWhat is surprising is that all of this was created in a single\ncommit. I suspected that this part that does fopen() to leak the\nfile stream was a later addition than the initial write_file_buf(),\nwhich should have been critiqued with \"once you want to do your\ncustom writing that is more than \"I have this block of memory, write\nit into file\", you should rewrite write_file_buf() call and roll it\ninto your own custom writing\", but that is not the case.\n\nSo taking the opportunity to clean things up, how about doing it\nthis way intead?\n\n----- >8 ---------- >8 ---------- >8 -----\n\nSubject: [PATCH] history: streamline message preparation and plug file stream leak\n\nAn early part of fill_commit_mmessage() function uses write_file_buf()\nto write out what was prepared in a strbuf, which is primarily meant\nfor use by callers that have their own message prepared fully and\ncalled as the last thing to flush it to the destination file.\n\nHowever, the function then opens a file stream in append mode to\nfurther write into it. It may have been understandable if this was\na later addition, but it seems it came from a single commit,\nd205234c (builtin/history: implement \"reword\" subcommand,\n2026-01-13), which is somewhat puzzling, but anyway...\n\nJust open the file stream upfront for writing, write the message\nthe function has in the strbuf, and then keep writing whatever it\nwants to write to the same open file stream.\n\nAnd do not forget to close the stream. We are about to pass the\nresulting file to an external editor, and on some systems, notably\nWindows, you are not supposed to keep a file open while expecting\nanother program to access it.\n\nDiagnosed-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n builtin/history.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/history.c b/builtin/history.c\nindex 8dcb9a6046..a882ad82e5 100644\n--- a/builtin/history.c\n+++ b/builtin/history.c\n@@ -41,11 +41,6 @@ static int fill_commit_message(struct repository *repo,\n \t\t \" empty message aborts the commit.\\n\");\n \tstruct wt_status s;\n \n-\tstrbuf_addstr(out, default_message);\n-\tstrbuf_addch(out, '\\n');\n-\tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n-\twrite_file_buf(path, out->buf, out->len);\n-\n \twt_status_prepare(repo, &s);\n \tFREE_AND_NULL(s.branch);\n \ts.ahead_behind_flags = AHEAD_BEHIND_QUICK;\n@@ -57,14 +52,20 @@ static int fill_commit_message(struct repository *repo,\n \ts.whence = FROM_COMMIT;\n \ts.committable = 1;\n \n-\ts.fp = fopen(git_path_commit_editmsg(), \"a\");\n+\ts.fp = fopen(path, \"w\");\n \tif (!s.fp)\n-\t\treturn error_errno(_(\"could not open '%s'\"), git_path_commit_editmsg());\n+\t\treturn error_errno(_(\"could not open '%s'\"), path);\n+\n+\tstrbuf_addstr(out, default_message);\n+\tstrbuf_addch(out, '\\n');\n+\tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n+\tfwrite(out.buf, 1, out.len, s.fp);\n \n \twt_status_collect_changes_trees(&s, old_tree, new_tree);\n \twt_status_print(&s);\n \twt_status_collect_free_buffers(&s);\n \tstring_list_clear_func(&s.change, change_data_free);\n+\tfclose(s.fp);\n \n \tstrbuf_reset(out);\n \tif (launch_editor(path, out, NULL)) {\n-- \n2.55.0-rc2-165-g3249676ba5\n\n"},{"id":"546429","messageId":"xmqqcxxefkg6.fsf@gitster.g","threadId":"65870","inReplyTo":"xmqqh5mqfkpv.fsf@gitster.g","subject":"Re: Re* [PATCH] history: close COMMIT_EDITMSG before launching the editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-25T20:12:41Z","receivedAt":"2026-06-25T20:12:43Z","isPatch":true,"body":"Junio C Hamano writes:\n\nOf course, this should be ...\n\n> +\tfwrite(out.buf, 1, out.len, s.fp);\n\n...\n\n\tfwrite(out->buf, 1, out->len, s.fp);\n\n"}]}