From 1878b5edc03c4bf685c7278fc4ced4704c876984 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:35 +0200 Subject: revision.[ch]: provide and start using a release_revisions() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The users of the revision.[ch] API's "struct rev_info" are a major source of memory leaks in the test suite under SANITIZE=leak, which in turn adds a lot of noise when trying to mark up tests with "TEST_PASSES_SANITIZE_LEAK=true". The users of that API are largely one-shot, e.g. "git rev-list" or "git log", or the "git checkout" and "git stash" being modified here For these callers freeing the memory is arguably a waste of time, but in many cases they've actually been trying to free the memory, and just doing that in a buggy manner. Let's provide a release_revisions() function for these users, and start migrating them over per the plan outlined in [1]. Right now this only handles the "pending" member of the struct, but more will be added in subsequent commits. Even though we only clear the "pending" member now, let's not leave a trap in code like the pre-image of index_differs_from(), where we'd start doing the wrong thing as soon as the release_revisions() learned to clear its "diffopt". I.e. we need to call release_revisions() after we've inspected any state in "struct rev_info". This leaves in place e.g. clear_pathspec(&rev.prune_data) in stash_working_tree() in builtin/stash.c, subsequent commits will teach release_revisions() to free "prune_data" and other members that in some cases are individually cleared by users of "struct rev_info" by reaching into its members. Those subsequent commits will remove the relevant calls to e.g. clear_pathspec(). We avoid amending code in index_differs_from() in diff-lib.c as well as wt_status_collect_changes_index(), has_unstaged_changes() and has_uncommitted_changes() in wt-status.c in a way that assumes that we are already clearing the "diffopt" member. That will be handled in a subsequent commit. 1. https://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.h | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'revision.h') diff --git a/revision.h b/revision.h index 5bc59c7bfe..61c780fc4c 100644 --- a/revision.h +++ b/revision.h @@ -377,6 +377,12 @@ void repo_init_revisions(struct repository *r, int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *); +/** + * Free data allocated in a "struct rev_info" after it's been + * initialized with repo_init_revisions(). + */ +void release_revisions(struct rev_info *revs); + void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx, const struct option *options, const char * const usagestr[]); -- cgit v1.2.3 From 296a1438455d366475570ac49afb466f591417dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:37 +0200 Subject: revision.[ch]: document and move code declared around "init" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A subsequent commit will add "REV_INFO_INIT" macro adjacent to repo_init_revisions(), unfortunately between the "struct rev_info" itself and that function we've added various miscellaneous code between the two over the years. Let's move that code either lower in revision.h, giving it API docs while we're at it, or in cases where it wasn't public API at all move it into revision.c No lines of code are changed here, only moved around. The only changes are the addition of new API comments. The "tree_difference" variable could also be declared like this, which I think would be a lot clearer, but let's leave that for now to keep this a move-only change: static enum { REV_TREE_SAME, REV_TREE_NEW, /* Only new files */ REV_TREE_OLD, /* Only files removed */ REV_TREE_DIFFERENT, /* Mixed changes */ } tree_difference = REV_TREE_SAME; Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.h | 50 ++++++++++++++++++++++++-------------------------- 1 file changed, 24 insertions(+), 26 deletions(-) (limited to 'revision.h') diff --git a/revision.h b/revision.h index 61c780fc4c..b9070e4342 100644 --- a/revision.h +++ b/revision.h @@ -329,32 +329,6 @@ struct rev_info { struct tmp_objdir *remerge_objdir; }; -int ref_excluded(struct string_list *, const char *path); -void clear_ref_exclusion(struct string_list **); -void add_ref_exclusion(struct string_list **, const char *exclude); - - -#define REV_TREE_SAME 0 -#define REV_TREE_NEW 1 /* Only new files */ -#define REV_TREE_OLD 2 /* Only files removed */ -#define REV_TREE_DIFFERENT 3 /* Mixed changes */ - -/* revision.c */ -typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *); -extern volatile show_early_output_fn_t show_early_output; - -struct setup_revision_opt { - const char *def; - void (*tweak)(struct rev_info *, struct setup_revision_opt *); - unsigned int assume_dashdash:1, - allow_exclude_promisor_objects:1; - unsigned revarg_opt; -}; - -#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS -#define init_revisions(revs, prefix) repo_init_revisions(the_repository, revs, prefix) -#endif - /** * Initialize a rev_info structure with default values. The third parameter may * be NULL or can be prefix path, and then the `.prefix` variable will be set @@ -366,6 +340,9 @@ struct setup_revision_opt { void repo_init_revisions(struct repository *r, struct rev_info *revs, const char *prefix); +#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS +#define init_revisions(revs, prefix) repo_init_revisions(the_repository, revs, prefix) +#endif /** * Parse revision information, filling in the `rev_info` structure, and @@ -374,6 +351,13 @@ void repo_init_revisions(struct repository *r, * head of the argument list. The last parameter is used in case no * parameter given by the first two arguments. */ +struct setup_revision_opt { + const char *def; + void (*tweak)(struct rev_info *, struct setup_revision_opt *); + unsigned int assume_dashdash:1, + allow_exclude_promisor_objects:1; + unsigned revarg_opt; +}; int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *); @@ -423,6 +407,14 @@ void mark_trees_uninteresting_sparse(struct repository *r, struct oidset *trees) void show_object_with_name(FILE *, struct object *, const char *); +/** + * Helpers to check if a "struct string_list" item matches with + * wildmatch(). + */ +int ref_excluded(struct string_list *, const char *path); +void clear_ref_exclusion(struct string_list **); +void add_ref_exclusion(struct string_list **, const char *exclude); + /** * This function can be used if you want to add commit objects as revision * information. You can use the `UNINTERESTING` object flag to indicate if @@ -478,4 +470,10 @@ int rewrite_parents(struct rev_info *revs, */ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit); +/** + * Global for the (undocumented) "--early-output" flag for "git log". + */ +typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *); +extern volatile show_early_output_fn_t show_early_output; + #endif -- cgit v1.2.3 From f196c1e908db9242206e1fc9c64f33baf92676bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:38 +0200 Subject: revisions API users: use release_revisions() needing REV_INFO_INIT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use release_revisions() to various users of "struct rev_list" which need to have their "struct rev_info" zero-initialized before we can start using it. For the bundle.c code see the early exit case added in 3bbbe467f29 (bundle verify: error out if called without an object database, 2019-05-27). For the relevant bisect.c code see 45b6370812c (bisect: libify `check_good_are_ancestors_of_bad` and its dependents, 2020-02-17). For the submodule.c code see the "goto" on "(!left || !right || !sub)" added in 8e6df65015f (submodule: refactor show_submodule_summary with helper function, 2016-08-31). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.h | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) (limited to 'revision.h') diff --git a/revision.h b/revision.h index b9070e4342..2621eb6d65 100644 --- a/revision.h +++ b/revision.h @@ -329,6 +329,25 @@ struct rev_info { struct tmp_objdir *remerge_objdir; }; +/** + * Initialize the "struct rev_info" structure with a macro. + * + * This will not fully initialize a "struct rev_info", the + * repo_init_revisions() function needs to be called before + * setup_revisions() and any revision walking takes place. + * + * Use REV_INFO_INIT to make the "struct rev_info" safe for passing to + * release_revisions() when it's inconvenient (e.g. due to a "goto + * cleanup" pattern) to arrange for repo_init_revisions() to be called + * before release_revisions() is called. + * + * Initializing with this REV_INFO_INIT is redundant to invoking + * repo_init_revisions(). If repo_init_revisions() is guaranteed to be + * called before release_revisions() the "struct rev_info" can be left + * uninitialized. + */ +#define REV_INFO_INIT { 0 } + /** * Initialize a rev_info structure with default values. The third parameter may * be NULL or can be prefix path, and then the `.prefix` variable will be set @@ -363,7 +382,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, /** * Free data allocated in a "struct rev_info" after it's been - * initialized with repo_init_revisions(). + * initialized with repo_init_revisions() or REV_INFO_INIT. */ void release_revisions(struct rev_info *revs); -- cgit v1.2.3