Re: [PATCH v2 08/51] is_dup_ref(): extract function from sort_ref_array()

From: Michael Haggerty <mhagger@alum.mit.edu>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Drew Northup <drew.northup@maine.edu>,
	Jakub Narebski <jnareb@gmail.com>,
	Heiko Voigt <hvoigt@hvoigt.net>,
	Johan Herland <johan@herland.net>,
	Julian Phillips <julian@quantumfyre.co.uk>
Subject: Re: [PATCH v2 08/51] is_dup_ref(): extract function from sort_ref_array()
Date: Tue, 13 Dec 2011 05:35:38 +0100	[thread overview]
Message-ID: <4EE6D61A.2030300@alum.mit.edu> (raw)
In-Reply-To: <7vvcplxq4r.fsf@alter.siamese.dyndns.org>

On 12/12/2011 11:33 PM, Junio C Hamano wrote:
> mhagger@alum.mit.edu writes:
> 
>> +/*
>> + * Emit a warning and return true iff ref1 and ref2 have the same name
>> + * and the same sha1.  Die if they have the same name but different
>> + * sha1s.
>> + */
>> +static int is_dup_ref(const struct ref_entry *ref1, const struct ref_entry *ref2)
>> +{
>> +	if (!strcmp(ref1->name, ref2->name)) {
>> +		/* Duplicate name; make sure that the SHA1s match: */
>> +		if (hashcmp(ref1->sha1, ref2->sha1))
>> +			die("Duplicated ref, and SHA1s don't match: %s",
>> +			    ref1->name);
>> +		warning("Duplicated ref: %s", ref1->name);
>> +		return 1;
>> +	} else {
>> +		return 0;
>> +	}
>> +}
> 
> At this step, is_dup_ref() is only called from sort_ref_array() which in
> turn is only called on either a collection of loose or packed refs, so
> warning is warranted. IOW I do not see anything wrong with _this_ patch.

I agree.

> Later in the series, however, the collection of extra refs seems to be
> shoved into a phoney "direntry" and made to go through the same add_ref()
> machinery that uses find_containing_direntry() which in turn calls
> search_ref_dir() that smells like a definite no-no.

Correct.

I had remembered your explanation of the purpose of extra refs and also
that they have unusual names, but I hadn't allowed for the possibility
that multiple extra_refs can have the same name.  The old code never
sorted the extra refs and therefore never checked or eliminated
duplicates.  The new code does, as sorting is an intrinsic part of
looking up references and reference subdirectories in the hierarchical
cache.  It is introduced along with the meat of the change in

    [PATCH v2 29/51] refs: store references hierarchically

So...the new code would die if two extra refs have the same name but
different SHA1s, and emit a warning if they have the same name and the
same SHA1s.  (However, it is no problem for an extra ref to have the
same name as a packed or loose ref.)

Is it *really* possible to have multiple extra_refs with the same name,
or is this more of a hypothetical problem that requires more careful
analysis?

If the former, then I will have to do some work.  My approach would
probably be to allow sorting without de-duplication; that way extra_refs
can continue to share most of the code used for the other ref caches.
It would be relatively easy to add such a fix on top of the patch
series, where every ref_entry knows the ref_cache with which it is
associated.  It would be quite a bit more painful to massage such a fix
through the whole patch series.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/