Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Oct 04 2021, Elijah Newren wrote:

> 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?

>>  }
>>
>>  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;
 }