From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 22 Mar 2009 19:06:41 GMT
Subject: Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs
Message-ID: <7viqm15qda.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7vbpru9nh9.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

>> +static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)
>> +{
>> +	static struct packed_git *last_found = (void *)1;
>> +	struct packed_git *p;
>> +
>> +	p = (last_found == (void *)1) ? packed_git : last_found;
>
> Why (void *)1, not like:
>
> 	static struct packed_git *last_found;
> 	struct packed_git *p = last_found ? last_found : packed_git;
>
> Am I missing something?
>
>> +	while (p) {
>> +		if ((!p->pack_local || p->pack_keep) &&
>> +			find_pack_entry_one(sha1, p)) {
>> +			last_found = p;
>> +			return 1;
>> +		}
>> +		if (p == last_found)
>> +			p = packed_git;
>> +		else
>> +			p = p->next;
>> +		if (p == last_found)
>> +			p = p->next;
>> +	}
>> +	return 0;
>> +}

Yes I was.

If you allow (last_found == NULL), the loop iterates over packed_git list,
and when it has its last element in p after discovering nothing matches
the criteria, this happens:

	if (p == last_found) /* false, p is the last element on the list */
        	p = packed_git; /* not taken */
	else
        	p = p->next; /* taken, p == NULL now */
	if (p == last_found) /* true! */
		p = p->next; /* OOPS */