Jeff King <peff@xxxxxxxx> writes:
> On Mon, Aug 08, 2016 at 03:37:35PM +0300, Kirill Smelkov wrote:
> ...
> static int want_object_in_pack(...)
> {
> ...
> if (!exclude && local && has_loose_object_nonlocal(sha1))
> return 0;
>
> if (*found_pack) {
> int ret = want_found_object(exclude, *found_pack);
> if (ret != -1)
> return ret;
> }
>
> for (p = packed_git; p; p = p->next) {
> off_t offset;
>
> if (p == *found_pack)
> offset = *found_offset;
> else
> offset = find_pack_entry(sha1, p);
> if (offset) {
> ... fill in *found_pack ...
> int ret = want_found_object(exclude, p);
> if (ret != -1)
> return ret;
> }
> }
> return 1;
> }
>
> That's a little more verbose, but IMHO the flow is a lot easier to
> follow (especially as the later re-rolls of that series actually muck
> with the loop order more, but with this approach there's no conflict).
I agree; Kirill's version was so confusing that I couldn't see what
it was trying to do with "pack1_seen" flag that is reset every time
loop repeats (at least, before got my coffee ;-). A helper function
like the above makes the logic a lot easier to grasp.
>> static int add_object_entry(const unsigned char *sha1, enum object_type type,
>> const char *name, int exclude)
>> {
>> - struct packed_git *found_pack;
>> - off_t found_offset;
>> + struct packed_git *found_pack = NULL;
>> + off_t found_offset = 0;
>> uint32_t index_pos;
>>
>> if (have_duplicate_entry(sha1, exclude, &index_pos))
>> @@ -1073,6 +1088,9 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,
>> if (have_duplicate_entry(sha1, 0, &index_pos))
>> return 0;
>>
>> + if (!want_object_in_pack(sha1, 0, &pack, &offset))
>> + return 0;
>> +
>
> This part looks correct and easy to understand.
Yes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html