Re: [PATCH 1/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 8, 2016, 16:08 UTC
Message-ID: <xmqqk2frgs4s.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160808135020.yggthtfekdiwq3lz@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 34 quoted lines

> 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.

Show 19 quoted lines

>>  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.