Re: [PATCH v5 08/10] sha1_file: support lazily fetching missing objects

[Jeff Hostetler <git@jeffhostetler.com>]

On 12/2/2017 1:29 PM, Christian Couder wrote:
> On Tue, Nov 21, 2017 at 10:07 PM, Jeff Hostetler <git@jeffhostetler.com> wrote:
>> From: Jonathan Tan <jonathantanmy@google.com>
[...]
>>   int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)
>>   {
[...]
>>
>> -       if (!find_pack_entry(real, &e)) {
>> -               /* Most likely it's a loose object. */
>> -               if (!sha1_loose_object_info(real, oi, flags))
>> -                       return 0;
>> +retry:
>> +       if (find_pack_entry(real, &e))
>> +               goto found_packed;
>>
>> -               /* Not a loose object; someone else may have just packed it. */
>> -               if (flags & OBJECT_INFO_QUICK) {
>> -                       return -1;
>> -               } else {
>> -                       reprepare_packed_git();
>> -                       if (!find_pack_entry(real, &e))
>> -                               return -1;
>> -               }
>> +       /* Most likely it's a loose object. */
>> +       if (!sha1_loose_object_info(real, oi, flags))
>> +               return 0;
>> +
>> +       /* Not a loose object; someone else may have just packed it. */
>> +       reprepare_packed_git();
>> +       if (find_pack_entry(real, &e))
>> +               goto found_packed;
>> +
>> +       /* Check if it is a missing object */
>> +       if (fetch_if_missing && repository_format_partial_clone &&
>> +           !already_retried) {
>> +               fetch_object(repository_format_partial_clone, real);
>> +               already_retried = 1;
>> +               goto retry;
>>          }
>>
>> +       return -1;
>> +
>> +found_packed:
>>          if (oi == &blank_oi)
[...]
> 
> The above is adding 2 different gotos into this function while there
> are quite simple ways to avoid them. See
> https://public-inbox.org/git/CAP8UFD2THRj7+RXmismUtUOpXQv4wM7aZsejpd_FHEOycP+ZJA@mail.gmail.com/
> and the follow up email in the thread.

Personally, I'm OK with limited goto's like these.  Yes, they are ugly,
but not that complicated.  Alternatively, we could put everything between
the 2 labels in a while (1) {...} and replace the goto's with break's and
continue's.  I think it would be better to revisit this later, as I'd
rather not start refactoring sha1_file.c in the middle of what is already
a 30+ commit patch series (for the 3 parts of partial clone).

Thoughts?
Jeff