All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Andrzej Hunt" <ajrhunt@google.com>, "Jeff King" <peff@peff.net>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH 03/10] unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options"
Date: Mon,  4 Oct 2021 02:46:04 +0200	[thread overview]
Message-ID: <patch-03.10-aaad6fce194-20211004T002226Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com>

Change the "struct dir" in "struct unpack_trees_options" away from a
pointer to being embedded in the struct itself, this mean that we can
initialize it with our new UNPACK_TREES_OPTIONS_INIT macro.

As it turned out nothing actually needed to provide its own "struct
dir" to this API, there's another patch to "hide" the struct instead,
see the discussion at [1], but I think just allocating it on the stack
along with the rest makes more sense.

This fixes a memory leak in "builtin/checkout.c" that's been there
since the clear_unpack_trees_porcelain() function was added in
1c41d2805e4 (unpack_trees_options: free messages when done,
2018-05-21), i.e. we still had a memory leak from allocating the "dir"
member here. That "dir" member had in turn been with us ever since
782c2d65c24 (Build in checkout, 2008-02-07).

This fixes that memory leak, and allows us to remove the boilerplate
dir_clear() elsewhere in favor of just using
clear_unpack_trees_porcelain().

1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/checkout.c  |  5 ++---
 builtin/read-tree.c | 11 ++++-------
 merge-ort.c         |  7 ++-----
 merge.c             |  7 ++-----
 unpack-trees.c      | 10 ++++------
 unpack-trees.h      |  4 +++-
 6 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index fea4533dbec..d4b88affba7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -745,9 +745,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
 				       &new_branch_info->commit->object.oid :
 				       &new_branch_info->oid, NULL);
 		if (opts->overwrite_ignore) {
-			topts.dir = xcalloc(1, sizeof(*topts.dir));
-			topts.dir->flags |= DIR_SHOW_IGNORED;
-			setup_standard_excludes(topts.dir);
+			topts.dir.flags |= DIR_SHOW_IGNORED;
+			setup_standard_excludes(&topts.dir);
 		}
 		tree = parse_tree_indirect(old_branch_info->commit ?
 					   &old_branch_info->commit->object.oid :
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 847182fdad6..06f3b97ac05 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -53,20 +53,17 @@ static int index_output_cb(const struct option *opt, const char *arg,
 static int exclude_per_directory_cb(const struct option *opt, const char *arg,
 				    int unset)
 {
-	struct dir_struct *dir;
 	struct unpack_trees_options *opts;
 
 	BUG_ON_OPT_NEG(unset);
 
 	opts = (struct unpack_trees_options *)opt->value;
 
-	if (opts->dir)
+	if (opts->dir.exclude_per_dir)
 		die("more than one --exclude-per-directory given.");
 
-	dir = xcalloc(1, sizeof(*opts->dir));
-	dir->flags |= DIR_SHOW_IGNORED;
-	dir->exclude_per_dir = arg;
-	opts->dir = dir;
+	opts->dir.flags |= DIR_SHOW_IGNORED;
+	opts->dir.exclude_per_dir = arg;
 	/* We do not need to nor want to do read-directory
 	 * here; we are merely interested in reusing the
 	 * per directory ignore stack mechanism.
@@ -208,7 +205,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	if ((opts.update || opts.index_only) && !opts.merge)
 		die("%s is meaningless without -m, --reset, or --prefix",
 		    opts.update ? "-u" : "-i");
-	if ((opts.dir && !opts.update))
+	if ((opts.dir.exclude_per_dir && !opts.update))
 		die("--exclude-per-directory is meaningless unless -u");
 	if (opts.merge && !opts.index_only)
 		setup_work_tree();
diff --git a/merge-ort.c b/merge-ort.c
index 75d2b8e4b99..e526b78b88d 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4045,9 +4045,8 @@ static int checkout(struct merge_options *opt,
 	unpack_opts.verbose_update = (opt->verbosity > 2);
 	unpack_opts.fn = twoway_merge;
 	if (1/* FIXME: opts->overwrite_ignore*/) {
-		CALLOC_ARRAY(unpack_opts.dir, 1);
-		unpack_opts.dir->flags |= DIR_SHOW_IGNORED;
-		setup_standard_excludes(unpack_opts.dir);
+		unpack_opts.dir.flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(&unpack_opts.dir);
 	}
 	parse_tree(prev);
 	init_tree_desc(&trees[0], prev->buffer, prev->size);
@@ -4056,8 +4055,6 @@ static int checkout(struct merge_options *opt,
 
 	ret = unpack_trees(2, trees, &unpack_opts);
 	clear_unpack_trees_porcelain(&unpack_opts);
-	dir_clear(unpack_opts.dir);
-	FREE_AND_NULL(unpack_opts.dir);
 	return ret;
 }
 
diff --git a/merge.c b/merge.c
index 086f04b0f97..9cb32990dd9 100644
--- a/merge.c
+++ b/merge.c
@@ -53,7 +53,6 @@ int checkout_fast_forward(struct repository *r,
 	struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
 	struct tree_desc t[MAX_UNPACK_TREES];
 	int i, nr_trees = 0;
-	struct dir_struct dir = DIR_INIT;
 	struct lock_file lock_file = LOCK_INIT;
 
 	refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
@@ -80,9 +79,8 @@ int checkout_fast_forward(struct repository *r,
 	}
 
 	if (overwrite_ignore) {
-		dir.flags |= DIR_SHOW_IGNORED;
-		setup_standard_excludes(&dir);
-		opts.dir = &dir;
+		opts.dir.flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(&opts.dir);
 	}
 
 	opts.head_idx = 1;
@@ -100,7 +98,6 @@ int checkout_fast_forward(struct repository *r,
 		clear_unpack_trees_porcelain(&opts);
 		return -1;
 	}
-	dir_clear(&dir);
 	clear_unpack_trees_porcelain(&opts);
 
 	if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
diff --git a/unpack-trees.c b/unpack-trees.c
index 393c1f35a5d..94767d3f96f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -196,6 +196,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)
 void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
 {
 	strvec_clear(&opts->msgs_to_free);
+	dir_clear(&opts->dir);
 	memset(opts->msgs, 0, sizeof(opts->msgs));
 }
 
@@ -2085,7 +2086,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
 	 */
 	int namelen;
 	int i;
-	struct dir_struct d;
+	struct dir_struct d = DIR_INIT;
 	char *pathbuf;
 	int cnt = 0;
 
@@ -2136,9 +2137,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
 	 */
 	pathbuf = xstrfmt("%.*s/", namelen, ce->name);
 
-	memset(&d, 0, sizeof(d));
-	if (o->dir)
-		d.exclude_per_dir = o->dir->exclude_per_dir;
+	d.exclude_per_dir = o->dir.exclude_per_dir;
 	i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
 	if (i)
 		return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
@@ -2179,8 +2178,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
 	if (ignore_case && icase_exists(o, name, len, st))
 		return 0;
 
-	if (o->dir &&
-	    is_excluded(o->dir, o->src_index, name, &dtype))
+	if (is_excluded(&o->dir, o->src_index, name, &dtype))
 		/*
 		 * ce->name is explicitly excluded, so it is Ok to
 		 * overwrite it.
diff --git a/unpack-trees.h b/unpack-trees.h
index 892b65ea623..40c4841748d 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -5,6 +5,7 @@
 #include "strvec.h"
 #include "string-list.h"
 #include "tree-walk.h"
+#include "dir.h"
 
 #define MAX_UNPACK_TREES MAX_TRAVERSE_TREES
 
@@ -66,7 +67,7 @@ struct unpack_trees_options {
 		     dry_run;
 	const char *prefix;
 	int cache_bottom;
-	struct dir_struct *dir;
+	struct dir_struct dir;
 	struct pathspec *pathspec;
 	merge_fn_t fn;
 	const char *msgs[NB_UNPACK_TREES_WARNING_TYPES];
@@ -93,6 +94,7 @@ struct unpack_trees_options {
 
 #define UNPACK_TREES_OPTIONS_INIT { \
 	.msgs_to_free = STRVEC_INIT, \
+	.dir = DIR_INIT, \
 }
 void unpack_trees_options_init(struct unpack_trees_options *o);
 
-- 
2.33.0.1404.g83021034c5d


  parent reply	other threads:[~2021-10-04  0:46 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-04  0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
2021-10-04  0:46 ` [PATCH 01/10] unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INIT Ævar Arnfjörð Bjarmason
2021-10-04  0:46 ` [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function Ævar Arnfjörð Bjarmason
2021-10-04 13:45   ` Elijah Newren
2021-10-04 14:41     ` Ævar Arnfjörð Bjarmason
2021-10-04 15:04       ` Elijah Newren
2021-10-04  0:46 ` Ævar Arnfjörð Bjarmason [this message]
2021-10-04 13:45   ` [PATCH 03/10] unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options" Elijah Newren
2021-10-04  0:46 ` [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset Ævar Arnfjörð Bjarmason
2021-10-04  9:31   ` Phillip Wood
2021-10-04 11:12     ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45   ` Elijah Newren
2021-10-04 15:20     ` Ævar Arnfjörð Bjarmason
2021-10-04 16:28       ` Elijah Newren
2021-10-04  0:46 ` [PATCH 05/10] dir.[ch]: make DIR_INIT mandatory Ævar Arnfjörð Bjarmason
2021-10-04 13:45   ` Elijah Newren
2021-10-04  0:46 ` [PATCH 06/10] dir.c: get rid of lazy initialization Ævar Arnfjörð Bjarmason
2021-10-04 13:45   ` Elijah Newren
2021-10-04  0:46 ` [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain() Ævar Arnfjörð Bjarmason
2021-10-04  9:38   ` Phillip Wood
2021-10-04 11:10     ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45   ` Elijah Newren
2021-10-04  0:46 ` [PATCH 08/10] unpack-trees: don't leak memory in verify_clean_subdirectory() Ævar Arnfjörð Bjarmason
2021-10-04 13:45   ` Elijah Newren
2021-10-04  0:46 ` [PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code Ævar Arnfjörð Bjarmason
2021-10-04 13:45   ` Elijah Newren
2021-10-04 14:50     ` Ævar Arnfjörð Bjarmason
2021-10-04  0:46 ` [PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release() Ævar Arnfjörð Bjarmason
2021-10-04 13:45   ` Elijah Newren
2021-10-04 14:54     ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Elijah Newren

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=patch-03.10-aaad6fce194-20211004T002226Z-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=ajrhunt@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=martin.agren@gmail.com \
    --cc=newren@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.