Re: [PATCH v2] Make the global packed_git variable static to sha1_file.c.

From: Stefan Zager <szager@chromium.org>
Date: Feb 13, 2014, 23:15 UTC
Message-ID: <CAHOQ7J_06UBEgitY7G8DTwvWToq2qfVyRYrzdBos4bsL7i7oew@mail.gmail.com>
In-Reply-To: <xmqqioskm3l6.fsf@gitster.dls.corp.google.com>

I uploaded a new patch; a few comments inline below...

On Wed, Feb 12, 2014 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> szager@chromium.org writes:
>
> Also I'd suggest s/pack_data_fn/collect_pack_data/ or something.
> "_fn" may be a good suffix for typedef'ed typename used in a
> callback function, but for a concrete function, it only adds noise
> without giving us anything new.

Fixed this everywhere.

Show 9 quoted lines

>>  static struct cached_object *find_cached_object(const unsigned char *sha1)
>> @@ -468,7 +469,6 @@ static unsigned int pack_open_fds;
>>  static unsigned int pack_max_fds;
>>  static size_t peak_pack_mapped;
>>  static size_t pack_mapped;
>> -struct packed_git *packed_git;
>
> Hmm, any particular reason why only this variable and not others are
> moved up?

No, just need packed_git declared before use. I moved all the static variables up, for clarity.

Show 12 quoted lines

>> +     foreach_packed_git(find_pack_fn, NULL, &fpd);
>> +     if (fpd.found_pack && !exclude &&
>> +         (incremental ||
>> +          (local && fpd.found_non_local_pack) ||
>> +          (ignore_packed_keep && fpd.found_pack_keep)))
>> +             return 0;
>
> When told to do --incremental, we used to return 0 from this
> function immediately once we find the object in one pack, without
> going thru the list of packs.  Now we let foreach to loop thru all
> of them and then return 0.  Does this difference matter?  A similar
> difference may exist for local/keep but I did not think it through.

Fixed.

Thanks,

Stefan