Web lists-archives.com

Re: [PATCH 025/194] object-store: allow prepare_alt_odb to handle arbitrary repositories




On Mon, Feb 5, 2018 at 5:19 PM, brian m. carlson
<sandals@xxxxxxxxxxxxxxxxxxxx> 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