Re: [PATCH v3 09/10] fetch: fetch unpopulated, changed submodules

From: Glen Choo <chooglen@google.com>
Date: Feb 25, 2022, 03:04 UTC
Message-ID: <kl6lbkyv4pew.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqy220j6kf.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 28 quoted lines

> Glen Choo <chooglen@google.com> writes:
>
>> +	char *path;
>>  	/* The submodule commits that have changed in the rev walk. */
>>  	struct oid_array new_commits;
>>  };
>> @@ -818,6 +828,7 @@ struct changed_submodule_data {
>>  static void changed_submodule_data_clear(struct changed_submodule_data *cs_data)
>>  {
>>  	oid_array_clear(&cs_data->new_commits);
>> +	free(cs_data->path);
>
> OK.
>
>>  }
>>  
>>  static void collect_changed_submodules_cb(struct diff_queue_struct *q,
>> @@ -865,6 +876,8 @@ static void collect_changed_submodules_cb(struct diff_queue_struct *q,
>>  		if (!item->util)
>>  			item->util = xcalloc(1, sizeof(struct changed_submodule_data));
>>  		cs_data = item->util;
>> +		cs_data->super_oid = commit_oid;
>> +		cs_data->path = xstrdup(p->two->path);
>
> Iffy.  If item->util were populated already, wouldn't cs_data
> already have its .path member pointing at an allocated piece of
> memory?  Can we safely free it before assigning a new value, or does
> somebody else still have a copy of .path and we cannot free it?

Great catch! This is a silly mistake, it looks like this because I copied the pattern that we used to _append_ new commit oids, but .super_oid and .path aren't appended, they're replaced.

But we don't even need to replace .super_oid and .path, we can use the first values we encounter and ignore subsequent ones.