Re: Possible regression in ref advertisement

From: Carlos Martín Nieto <cmn@elego.de>
Date: Feb 25, 2013, 20:35 UTC
Message-ID: <1361824519.31692.12.camel@centaur.cmartin.tk>
In-Reply-To: <7v1uc4ximl.fsf@alter.siamese.dyndns.org>

On Mon, 2013-02-25 at 12:07 -0800, Junio C Hamano wrote:

Show 13 quoted lines

> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> >> A shot in the dark, as I do not seem to be able to reproduce the issue
> >> with anything that contains the commit.  Perhaps your .git/packed-refs
> >> is corrupt?
> >
> > My packed-refs file did not end with LF. It seems it must or the parser
> > won't consider the last tag in the file to have a peeled target and mine
> > didn't. I don't how the ended up this way but it looks like there's is a
> > bug in the parser (or does the format force you to have LF at the end?)
> 
> It is unclear what kind of corruption your packed-refs file had, as
> there are multiple ways for a file not to "end with LF", but anyway.

I mean that the file ended at the end of the peeled id. It was missing the last empty line.

> 
> As packed-refs file is expected to be a text file, it is not
> surprising to get an undefined result if the it ends with an
> incomplete line.

I guess that depends on what you mean by incomplete. All the data is there, just that it had

    <tag id> SP <refname> LF ^<peeled id>

as the last entry instead of

    <tag id> SP <refname> LF ^<peeled id> LF

The parser skips the last entry if there is no trailing LF (the same happens for lightweight tags, though here it simply doesn't report them).

Show 6 quoted lines

> 
> I do not offhand recall if we tolerate lines with CRLF endings; as
> far as I know we do not _write_ CRLF out, and because we do not
> expect people to muck directly with editor bypassing "pack-refs", I
> would not be surprised if we didn't do anything special for people
> who make the lines end with with CRLF the file, either.

I wouldn't expect it to work. It would probably skip over those entries.

Show 11 quoted lines

> 
> I'd be more worried about the possibly lost entries after that
> incomplete line (and also possibly truncated end part of the tagname
> on the last, incomplete line).  Perhaps fsck should diagnose such an
> anomaly as repository corruption?  Perhaps we should enhance the
> file format a bit (right now, the first "capability" line should say
> something like "# pack-refs with: peeled" to say it was created with
> the version of pack-refs that can record peeled tags, but we can add
> other capabilities to extend the format) to add checksums to detect
> corruption?
>

I just tested and the parser ignores any malformed lines. The ones after it are fine. Nothing complains though, ls-remote just shows the next entry. I'd expect at least fsck to complain, but it doesn't say anything. Presumably they all use the same parser that just ignores anything it doesn't like.

cmn