git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Rubén Justo" <rjusto@gmail.com>
Subject: [PATCH 11/11] blame: use "dup" string_list for ignore-revs files
Date: Sat, 6 Apr 2024 21:07:04 -0400	[thread overview]
Message-ID: <20240407010704.GK868358@coredump.intra.peff.net> (raw)
In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net>

We feed items into ignore_revs_file_list in two ways: from config and
from the command-line. Items from the command-line point to argv entries
that we don't own, but items from config are hea-allocated strings whose
ownership is handed to the string list when we insert them.

Because the string_list is declared with "NODUP", our string_list_clear()
call will not free the entries themselves. This is the right thing for
the argv entries, but means that we leak the allocated config entries.

Let's declare the string list as owning its own strings, which means
that the argv entries will be copied.

For the config entries we would ideally use string_list_append_nodup()
to just hand off ownership of our strings. But we insert them into the
sorted list with string_list_insert(), which doesn't have a nodup
variant. Since this isn't a hot path, we can accept that the path
interpolation will produce a temporary string which is then freed.

Signed-off-by: Jeff King <peff@peff.net>
---
Not strictly related to this series, but something I noticed while
converting this spot in an earlier patch. I had thought originally we
could switch to avoid the extra copy altogether:

  if (!value)
	return config_error_nonbool();
  string_list_insert(&ignore_revs_file_list, value);

but of course that is missing the call to interpolate_path().

I imagine that it could also be further refactored to append while
reading the config, and then sort after. That's more efficient overall,
and would let us use append_nodup().

 builtin/blame.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 0b07ceb110..fa7f8fce09 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -67,7 +67,7 @@ static int no_whole_file_rename;
 static int show_progress;
 static char repeated_meta_color[COLOR_MAXLEN];
 static int coloring_mode;
-static struct string_list ignore_revs_file_list = STRING_LIST_INIT_NODUP;
+static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP;
 static int mark_unblamable_lines;
 static int mark_ignored_lines;
 
@@ -725,6 +725,7 @@ static int git_blame_config(const char *var, const char *value,
 		if (ret)
 			return ret;
 		string_list_insert(&ignore_revs_file_list, str);
+		free(str);
 		return 0;
 	}
 	if (!strcmp(var, "blame.markunblamablelines")) {
-- 
2.44.0.872.g288abe5b5b


  parent reply	other threads:[~2024-04-07  1:07 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-06 18:11 [PATCH] config: do not leak excludes_file Junio C Hamano
2024-04-06 19:17 ` [WIP] git_config_pathname() leakfix Junio C Hamano
2024-04-07  0:56 ` [PATCH 0/12] git_config_string() considered harmful Jeff King
2024-04-07  0:58   ` [PATCH 01/11] config: make sequencer.c's git_config_string_dup() public Jeff King
2024-04-07  1:00   ` [PATCH 02/11] config: add git_config_pathname_dup() Jeff King
2024-04-07  1:00   ` [PATCH 03/11] config: prefer git_config_string_dup() for temp variables Jeff King
2024-04-07  1:50     ` Eric Sunshine
2024-04-07  2:45       ` Jeff King
2024-04-07  1:01   ` [PATCH 04/11] config: use git_config_string_dup() for open-coded equivalents Jeff King
2024-04-07  1:01   ` [PATCH 05/11] config: use git_config_string_dup() to fix leaky open coding Jeff King
2024-04-07  1:02   ` [PATCH 06/11] config: use git_config_string() in easy cases Jeff King
2024-04-07  2:05     ` Eric Sunshine
2024-04-07  2:47       ` Jeff King
2024-04-07  1:03   ` [PATCH 07/11] config: use git_config_pathname_dup() " Jeff King
2024-04-07  1:03   ` [PATCH 08/11] http: use git_config_string_dup() Jeff King
2024-04-07  1:04   ` [PATCH 09/11] merge: use git_config_string_dup() for pull strategies Jeff King
2024-04-07  1:04   ` [PATCH 10/11] userdiff: use git_config_string_dup() when we can Jeff King
2024-04-07  1:07   ` Jeff King [this message]
2024-04-07  2:42     ` [PATCH 11/11] blame: use "dup" string_list for ignore-revs files Eric Sunshine
2024-04-07  1:08   ` [PATCH 0/12] git_config_string() considered harmful Jeff King
2024-04-07 17:58   ` Rubén Justo
2024-04-08 20:55     ` Jeff King
2024-04-11 23:36       ` Rubén Justo

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=20240407010704.GK868358@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=rjusto@gmail.com \
    /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.
Code repositories for project(s) associated with this public 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).