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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 12/12/2011 09:33 AM, Jeff King wrote:
> On Mon, Dec 12, 2011 at 06:38:15AM +0100, mhagger@alum.mit.edu wrote:
> 
>> +/*
>> + * 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;
>> +	}
>> +}
> 
> As a user, I'm not sure what this message means. If I understand the
> context right, this happens when you have duplicate entries in your
> packed-refs file?

It could be caused by dups in any of the three ref caches: packed refs,
loose refs, or extra refs.

Duplicates in the packed-refs file could be caused by any garden-variety
data corruption external to git.  Or they could be caused by a bug in
git that caused it to write duplicates.  But the code that writes packed
refs iterates over the refs right after they are sorted, and sorting
eliminates duplicates, so this would be an unlikely bug the way the code
is currently organized.  I suppose they could also be caused by the
simultaneous writing of the packed-refs file by another process that
doesn't respect git's lock file; perhaps an ill-timed rsync or something.

Normally there should be no duplicate loose refs, because their names
are taken directly from the filesystem and duplicate filenames are not
allowed.  (Though I don't know if readdir() guarantees an atomic
snapshot of a directory; if not, then a dup could perhaps be created by
having another process add and remove files while git is reading a loose
ref directory.)

Extra refs are created locally by other git code that I am not familiar
with, so duplicate extra refs could only be created by a bug in git.

So in summary, duplicates could be caused by a git bug, by a corrupt
packed-refs file, or conceivably by a race condition with another
process that is mutating the packed-refs file or the loose reference
directories.

> This would indicate a bug in git, so should this perhaps say "BUG:" in
> front, or maybe give some more context so that a user understands it is
> not their error, or even a random error on this run, but that git has
> accidentally corrupted the packed-refs file (and their best bet is
> probably to report the bug to us).
> 
> This is obviously not an issue introduced by your patch, as you are
> just moving these error messages around. But maybe while the topic is
> fresh in your mind it is a good time to improve it. I dunno. AFAICT
> nobody has ever actually hit this message, so maybe it doesn't matter.
> :)

The first of Google search results for the fatal error message only
shows one instance where the error was observed [1].  This was a case of
cloning using the rsync protocol, which I believe is now deprecated
(probably for exactly this reason).

I think that the gold-plated way to improve the error message would be
to pass the error back to the caller, who would have more context to
decide the most likely cause of the duplicate and give an appropriate
warning or error message.  But I think we can afford to wait until more
error reports appear before bothering with this.

Michael

[1] http://kerneltrap.org/mailarchive/git/2008/12/17/4438764

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