Git development
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Cc: Patrick Steinhardt <ps@pks.im>,
	 Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH v2] history: streamline message preparation and plug file stream leak
Date: Mon, 29 Jun 2026 09:08:42 -0700	[thread overview]
Message-ID: <xmqqmrwdxrat.fsf@gitster.g> (raw)
In-Reply-To: <xmqqecht8df1.fsf@gitster.g> (Junio C. Hamano's message of "Fri, 26 Jun 2026 09:38:42 -0700")

An early part of fill_commit_message() function uses write_file_buf()
to write out what was prepared in a strbuf, which is primarily meant
for use by callers that have their own message prepared fully and
called as the last thing to flush it to the destination file.

However, the function then opens a file stream in append mode to
further write into it.  It may have been understandable if this was
a later addition, but it seems it came from a single commit,
d205234c (builtin/history: implement "reword" subcommand,
2026-01-13), which is somewhat puzzling, but anyway...

Just open the file stream upfront for writing, write the message
the function has in the strbuf, and then keep writing whatever it
wants to write to the same open file stream.

And do not forget to close the stream.  We are about to pass the
resulting file to an external editor, and on some systems, notably
Windows, you are not supposed to keep a file open while expecting
another program to access it.

Diagnosed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Changes from v1 are two additional error checks to notice failure
   from fwrite() and fclose() to die.  Interdiff appears at the end.

 builtin/history.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin/history.c b/builtin/history.c
index 8dcb9a6046..365e81379b 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -41,11 +41,6 @@ static int fill_commit_message(struct repository *repo,
 		  " empty message aborts the commit.\n");
 	struct wt_status s;
 
-	strbuf_addstr(out, default_message);
-	strbuf_addch(out, '\n');
-	strbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);
-	write_file_buf(path, out->buf, out->len);
-
 	wt_status_prepare(repo, &s);
 	FREE_AND_NULL(s.branch);
 	s.ahead_behind_flags = AHEAD_BEHIND_QUICK;
@@ -57,14 +52,22 @@ static int fill_commit_message(struct repository *repo,
 	s.whence = FROM_COMMIT;
 	s.committable = 1;
 
-	s.fp = fopen(git_path_commit_editmsg(), "a");
+	s.fp = fopen(path, "w");
 	if (!s.fp)
-		return error_errno(_("could not open '%s'"), git_path_commit_editmsg());
+		return error_errno(_("could not open '%s'"), path);
+
+	strbuf_addstr(out, default_message);
+	strbuf_addch(out, '\n');
+	strbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);
+	if (fwrite(out->buf, 1, out->len, s.fp) != out->len)
+		die_errno(_("could not write to '%s'"), path);
 
 	wt_status_collect_changes_trees(&s, old_tree, new_tree);
 	wt_status_print(&s);
 	wt_status_collect_free_buffers(&s);
 	string_list_clear_func(&s.change, change_data_free);
+	if (fclose(s.fp))
+		die_errno(_("could not write to '%s'"), path);
 
 	strbuf_reset(out);
 	if (launch_editor(path, out, NULL)) {

Interdiff against v1:
  diff --git a/builtin/history.c b/builtin/history.c
  index f17ec049c0..365e81379b 100644
  --- a/builtin/history.c
  +++ b/builtin/history.c
  @@ -59,13 +59,15 @@ static int fill_commit_message(struct repository *repo,
   	strbuf_addstr(out, default_message);
   	strbuf_addch(out, '\n');
   	strbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);
  -	fwrite(out->buf, 1, out->len, s.fp);
  +	if (fwrite(out->buf, 1, out->len, s.fp) != out->len)
  +		die_errno(_("could not write to '%s'"), path);
   
   	wt_status_collect_changes_trees(&s, old_tree, new_tree);
   	wt_status_print(&s);
   	wt_status_collect_free_buffers(&s);
   	string_list_clear_func(&s.change, change_data_free);
  -	fclose(s.fp);
  +	if (fclose(s.fp))
  +		die_errno(_("could not write to '%s'"), path);
   
   	strbuf_reset(out);
   	if (launch_editor(path, out, NULL)) {
-- 
2.55.0-180-gf61bfe2e0b



      parent reply	other threads:[~2026-06-29 16:08 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-26 16:38 [PATCH] history: streamline message preparation and plug file stream leak Junio C Hamano
2026-06-29  6:33 ` Patrick Steinhardt
2026-06-29 15:21   ` Junio C Hamano
2026-06-29 16:04     ` Patrick Steinhardt
2026-06-29 16:08 ` Junio C Hamano [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

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

  git send-email \
    --in-reply-to=xmqqmrwdxrat.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    --cc=ps@pks.im \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox