Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Oct 4, 2021, 15:20 UTC
- Message-ID
- <87lf38n6e4.fsf@evledraar.gmail.com>
- In-Reply-To
- <CABPp-BH4ubjJ98Nvgp2iyKxmU9X+ypw4m1o=iL9Z4vSNZ-QTDw@mail.gmail.com>
On Mon, Oct 04 2021, Elijah Newren wrote:
Show 29 quoted lines
> On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> Change the clear_unpack_trees_porcelain() to be like a *_release()
>> function, not a *_reset() (in strbuf.c terms). Let's move the only API
>> user that relied on the latter to doing its own
>> unpack_trees_options_init(). See the commit that introduced
>> unpack_trees_options_init() for details on the control flow involved
>> here.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> merge-recursive.c | 1 +
>> unpack-trees.c | 1 -
>> 2 files changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index d24a4903f1d..a77f66b006c 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)
>> {
>> discard_index(&opt->priv->orig_index);
>> clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
>> + unpack_trees_options_init(&opt->priv->unpack_opts);
>
> This is wrong. It suggests that unpack_opts is used after
> unpack_trees_finish() (other than an outer merge first calling
> unpack_trees_start() again), which can only serve to greatly confuse
> future readers. Drop this hunk.Sure, but (and also re: https://lore.kernel.org/git/CABPp-BEA2myh2Np_YpFWnE+jqmT5vz7ohigZ0=2tL-wizgYQmg@mail.gmail.com/) if you'd like not initialize things in merge_start() just for good measure wouldn't the diff-at-the-end on top of your 5bf7e5779ec (merge-recursive: split internal fields into a separate struct, 2019-08-17) also make sense?
I.e. the reason I entered this particular rabbit hole was in looking at existing members of "struct merge_options_internal" & past commits and seeing how we did its initialization. That canary on top passes all our tests, and per my reading we also don't use "df_conflict_file_set" until as late as the things we setup in unpack_trees_start(). Should those be moved to do the post-merge_start() setup at the same time?
Show 15 quoted lines
>> }
>>
>> static int save_files_dirs(const struct object_id *oid,
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index 94767d3f96f..e7365322e82 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -197,7 +197,6 @@ 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));
>
> This seems like a very dangerous change. You want to leave opts->msgs
> pointing at freed memory?Yes, as argued in http://lore.kernel.org/git/87bl45niqs.fsf@evledraar.gmail.com; In this series we can see that nothing re-uses it, so it's as safe as our strbuf_release(), or a plain free().
Maybe I'm misunderstanding what you're getting at, and I could understand a "let's just reset it for good measure" POV. But I can't square your view that we shouldn't do setup in merge_start() for good measure in case some new future code accidentally uses the data earlier (which I'm fine with), but then also not finding it OK to skip the memset() here ...
diff --git a/merge-recursive.c b/merge-recursive.c index e594d4c3fa1..6d2b8e78896 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -601,6 +601,7 @@ static void record_df_conflict_files(struct merge_options *opt, df_sorted_entries.cmp = string_list_df_name_compare; string_list_sort(&df_sorted_entries); + assert(opt->priv->df_conflict_file_set.nr != 123456); string_list_clear(&opt->priv->df_conflict_file_set, 1); for (i = 0; i < df_sorted_entries.nr; i++) { const char *path = df_sorted_entries.items[i].string; @@ -869,6 +870,7 @@ static int make_room_for_path(struct merge_options *opt, const char *path) const char *msg = _("failed to create path '%s'%s"); /* Unlink any D/F conflict files that are in the way */ + assert(opt->priv->df_conflict_file_set.nr != 123456); for (i = 0; i < opt->priv->df_conflict_file_set.nr; i++) { const char *df_path = opt->priv->df_conflict_file_set.items[i].string; size_t pathlen = strlen(path); @@ -3467,6 +3469,7 @@ static int merge_trees_internal(struct merge_options *opt, return 1; } + string_list_init_dup(&opt->priv->df_conflict_file_set); code = unpack_trees_start(opt, merge_base, head, merge); if (code != 0) { @@ -3703,7 +3706,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); + opt->priv->df_conflict_file_set.nr = 123456; return 0; }