On Mon, Feb 5, 2018 at 5:19 PM, brian m. carlson
<sand...@crustytoothpaste.net> wrote:
> On Mon, Feb 05, 2018 at 03:54:46PM -0800, Stefan Beller wrote:
>> @@ -434,12 +433,12 @@ static int link_alt_odb_entry_the_repository(const
>> char *entry,
>> ent = alloc_alt_odb(pathbuf.buf);
>>
>> /* add the alternate entry */
>> - *the_repository->objects.alt_odb_tail = ent;
>> - the_repository->objects.alt_odb_tail = &(ent->next);
>> + *r->objects.alt_odb_tail = ent;
>> + r->objects.alt_odb_tail = &(ent->next);
>> ent->next = NULL;
>
> I'm sure I'm missing something obvious, but it's not clear to me that
> this transformation is correct. Could you perhaps say a few words about
> why it is?
This is a pretty open ended question, so I'll give it a try:
* ent is a local variable that is newly allocated using `alloc_alt_odb`.
`alloc_alt_odb` has no hidden dependencies on a specific repository,
it uses FLEX_ALLOC_STR, which is defined in cache.h as a wrapper
around xcalloc/memcpy
* Before this patch we always used the_repository->objects.alt_odb_tail,
but with this patch there is no reference "alt_odb.tail" of the_repository,
but only of a given arbitrary repository.
Usually we convert only one function at a time. (Chose that function,
which calls only already converted functions, because then passing in
r instead of the_repository still compiles correctly)
The additional messiness comes from a cycle:
read_info_alternates
-> link_alt_odb_entries
-> link_alt_odb_entry
-> read_info_alternates
That is why we cannot just convert one function, but we have to convert
the whole strongly connected component in this graph of functions.
This is why this patch is so messy and touches multiple functions at once.
* While I hope this helps, I assume you want me to repeat this or other
hints in the commit message, too.
Thanks,
Stefan
