Re: [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function
From: Elijah Newren <hidden>
Date: 2021-10-04 13:55:10
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT macro, but "merge-recursive.c" could not be converted to it since it (re-)initializes a "struct unpack_trees_options" on the heap. In order to convert use the macro as the source of truth for initialization we need to not only convert the initialization in unpack_trees_start(), but also call the new unpack_trees_options_init() just after the CALLOC_ARRAY() in merge_start().
Um...why?
When we call merge_trees() we'll call merge_start() followed by merge_trees_internal(), and it will call unpack_trees_start() before it does much of anything. So it's covered by an initialization in unpack_trees_start(). But when merge_recursive() is called it will call merge_start() followed by merge_recursive_internal(), which won't call unpack_trees_start() until its own call call to merge_trees_internal(), but in the meantime it might end up using that calloc'd "struct unpack_trees_options".
Nothing in merge-recursive.c usings unpack_opts before unpack_trees_start() or after unpack_trees_finish(). If anyone attempts to use it elsewhere, that would itself be a bug. So, I'd replace the above three paragraphs with: "Change the initialization of opt->priv_unpack_opts from using memset to 0, with unpack_trees_options_init()." or something like that, and then drop your change to merge_start().
This was OK before, as setup_unpack_trees_porcelain() would call strvec_init(), and our "struct dir_struct" in turn is OK with being NULL'd. Let's convert the former to macro initialization, we'll deal with the latter in a subsequent commit.
This is quite confusing; it's really hard to understand how this relates to the rest of the commit message. I have to read the code to see what you're doing, and then write my own commit message in my head because the wording in this paragraph still doesn't parse. I'd make the change strvec_init/STRVEC_INIT changes be a separate patch. I suspect it'll be easier to write the commit message for it as a standalone commit as well.
quoted hunk ↗ jump to hunk
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted> --- merge-recursive.c | 3 ++- unpack-trees.c | 8 ++++++-- unpack-trees.h | 5 ++++- 3 files changed, 12 insertions(+), 4 deletions(-)diff --git a/merge-recursive.c b/merge-recursive.c index e594d4c3fa1..d24a4903f1d 100644 --- a/merge-recursive.c +++ b/merge-recursive.c@@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt, struct tree_desc t[3]; struct index_state tmp_index = { NULL }; - memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts)); + unpack_trees_options_init(&opt->priv->unpack_opts); if (opt->priv->call_depth) opt->priv->unpack_opts.index_only = 1; else@@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head) CALLOC_ARRAY(opt->priv, 1); string_list_init_dup(&opt->priv->df_conflict_file_set); + unpack_trees_options_init(&opt->priv->unpack_opts);
Drop this hunk.
quoted hunk ↗ jump to hunk
return 0; }diff --git a/unpack-trees.c b/unpack-trees.c index 8ea0a542da8..393c1f35a5d 100644 --- a/unpack-trees.c +++ b/unpack-trees.c@@ -108,8 +108,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, const char **msgs = opts->msgs; const char *msg; - strvec_init(&opts->msgs_to_free); - if (!strcmp(cmd, "checkout")) msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE) ? _("Your local changes to the following files would be overwritten by checkout:\n%%s"@@ -189,6 +187,12 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, opts->unpack_rejects[i].strdup_strings = 1; } +void unpack_trees_options_init(struct unpack_trees_options *o) +{ + struct unpack_trees_options blank = UNPACK_TREES_OPTIONS_INIT; + memcpy(o, &blank, sizeof(*o)); +} + void clear_unpack_trees_porcelain(struct unpack_trees_options *opts) { strvec_clear(&opts->msgs_to_free);diff --git a/unpack-trees.h b/unpack-trees.h index ecf256cbcea..892b65ea623 100644 --- a/unpack-trees.h +++ b/unpack-trees.h@@ -91,7 +91,10 @@ struct unpack_trees_options { struct checkout_metadata meta; }; -#define UNPACK_TREES_OPTIONS_INIT { 0 } +#define UNPACK_TREES_OPTIONS_INIT { \ + .msgs_to_free = STRVEC_INIT, \ +} +void unpack_trees_options_init(struct unpack_trees_options *o); int unpack_trees(unsigned n, struct tree_desc *t, struct unpack_trees_options *options); --2.33.0.1404.g83021034c5d