Possible regression in ref advertisement

Possible regression in ref advertisement

[Carlos Martín Nieto]

Hi all,

When testing to see if a different implementation was in shape, I came
across something odd where newer git doesn't advertise one of the refs
in the git repo.

Running `git ls-remote .` or `git-upload-pack` in my git repo, newer git
versions omit peeling the v1.8.0-rc3 tag.

The diff between the command above when ran with 1.7.10.4 (from Debian)
and current 'master'

--- old 2013-02-25 17:31:29.583526606 +0100
+++ new 2013-02-25 17:31:36.783526559 +0100
@@ -1379,7 +1379,6 @@
 c15295d7477ccec489953299bd03a8e62f86e611       refs/tags/v1.8.0-rc2
 cd46259ebf2e624bcee2aaae05c36663d414e1a2       refs/tags/v1.8.0-rc2^{}
 22ed067acc84eac8a0a72d20478a18aee4e25571       refs/tags/v1.8.0-rc3
-87a5461fa7b30f7b7baf27204f10219d61500fbf       refs/tags/v1.8.0-rc3^{}
 bfeb8b9ae0012cb61e026cbcd29664876abf5389       refs/tags/v1.8.0.1
 ed9fe755130891fc878bb2433204faffb534697b       refs/tags/v1.8.0.1^{}
 63add1fb45e1ab7a76bb38bbb9467c91fdfaaa7e       refs/tags/v1.8.0.2

Diffing with the output from next, diff tells me it's binary for some
reason, but looking manually, the peeled v1.8.0-rc3 tag isn't there
either. I haven't had time to bisect this, so I'm putting it out there
in case anybody wants to investigate before I have time to dig into it.


   cmn

Re: Possible regression in ref advertisement

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> Hi all,
>
> When testing to see if a different implementation was in shape, I came
> across something odd where newer git doesn't advertise one of the refs
> in the git repo.
>
> Running `git ls-remote .` or `git-upload-pack` in my git repo, newer git
> versions omit peeling the v1.8.0-rc3 tag.
>
> The diff between the command above when ran with 1.7.10.4 (from Debian)
> and current 'master'
>
> --- old 2013-02-25 17:31:29.583526606 +0100
> +++ new 2013-02-25 17:31:36.783526559 +0100
> @@ -1379,7 +1379,6 @@
>  c15295d7477ccec489953299bd03a8e62f86e611       refs/tags/v1.8.0-rc2
>  cd46259ebf2e624bcee2aaae05c36663d414e1a2       refs/tags/v1.8.0-rc2^{}
>  22ed067acc84eac8a0a72d20478a18aee4e25571       refs/tags/v1.8.0-rc3
> -87a5461fa7b30f7b7baf27204f10219d61500fbf       refs/tags/v1.8.0-rc3^{}
>  bfeb8b9ae0012cb61e026cbcd29664876abf5389       refs/tags/v1.8.0.1
>  ed9fe755130891fc878bb2433204faffb534697b       refs/tags/v1.8.0.1^{}
>  63add1fb45e1ab7a76bb38bbb9467c91fdfaaa7e       refs/tags/v1.8.0.2
>
> Diffing with the output from next, diff tells me it's binary for some
> reason, but looking manually, the peeled v1.8.0-rc3 tag isn't there
> either. I haven't had time to bisect this, so I'm putting it out there
> in case anybody wants to investigate before I have time to dig into it.

Interesting.  "git ls-remote . | grep 1.8.0-" for maint, master,
next and pu produce identical results for me, all showing peeled
ones correctly.

Re: Possible regression in ref advertisement

[Carlos Martín Nieto]

On Mon, 2013-02-25 at 10:31 -0800, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > Hi all,
> >
> > When testing to see if a different implementation was in shape, I came
> > across something odd where newer git doesn't advertise one of the refs
> > in the git repo.
> >
> > Running `git ls-remote .` or `git-upload-pack` in my git repo, newer git
> > versions omit peeling the v1.8.0-rc3 tag.
> >
> > The diff between the command above when ran with 1.7.10.4 (from Debian)
> > and current 'master'
> >
> > --- old 2013-02-25 17:31:29.583526606 +0100
> > +++ new 2013-02-25 17:31:36.783526559 +0100
> > @@ -1379,7 +1379,6 @@
> >  c15295d7477ccec489953299bd03a8e62f86e611       refs/tags/v1.8.0-rc2
> >  cd46259ebf2e624bcee2aaae05c36663d414e1a2       refs/tags/v1.8.0-rc2^{}
> >  22ed067acc84eac8a0a72d20478a18aee4e25571       refs/tags/v1.8.0-rc3
> > -87a5461fa7b30f7b7baf27204f10219d61500fbf       refs/tags/v1.8.0-rc3^{}
> >  bfeb8b9ae0012cb61e026cbcd29664876abf5389       refs/tags/v1.8.0.1
> >  ed9fe755130891fc878bb2433204faffb534697b       refs/tags/v1.8.0.1^{}
> >  63add1fb45e1ab7a76bb38bbb9467c91fdfaaa7e       refs/tags/v1.8.0.2
> >
> > Diffing with the output from next, diff tells me it's binary for some
> > reason, but looking manually, the peeled v1.8.0-rc3 tag isn't there
> > either. I haven't had time to bisect this, so I'm putting it out there
> > in case anybody wants to investigate before I have time to dig into it.
> 
> Interesting.  "git ls-remote . | grep 1.8.0-" for maint, master,
> next and pu produce identical results for me, all showing peeled
> ones correctly.

Bisection leads me to Peff's 435c8332 (2012-10-04; upload-pack: use
peel_ref for ref advertisements) and reverting that commit brings the
-rc3^{} back.

   cmn

Re: Possible regression in ref advertisement

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> On Mon, 2013-02-25 at 10:31 -0800, Junio C Hamano wrote:
> ...
>> Interesting.  "git ls-remote . | grep 1.8.0-" for maint, master,
>> next and pu produce identical results for me, all showing peeled
>> ones correctly.
>
> Bisection leads me to Peff's 435c8332 (2012-10-04; upload-pack: use
> peel_ref for ref advertisements) and reverting that commit brings the
> -rc3^{} back.

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?

Re: Possible regression in ref advertisement

[Carlos Martín Nieto]

On Mon, 2013-02-25 at 11:27 -0800, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > On Mon, 2013-02-25 at 10:31 -0800, Junio C Hamano wrote:
> > ...
> >> Interesting.  "git ls-remote . | grep 1.8.0-" for maint, master,
> >> next and pu produce identical results for me, all showing peeled
> >> ones correctly.
> >
> > Bisection leads me to Peff's 435c8332 (2012-10-04; upload-pack: use
> > peel_ref for ref advertisements) and reverting that commit brings the
> > -rc3^{} back.
> 
> 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?)

   cmn

Re: Possible regression in ref advertisement

[Junio C Hamano]

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.

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 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'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?

Re: Possible regression in ref advertisement

[Carlos Martín Nieto]

On Mon, 2013-02-25 at 12:07 -0800, Junio C Hamano wrote:
> 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).

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

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

Re: Possible regression in ref advertisement

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

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

I used that word in the POSIX sense, i.e.

  http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap03.html#tag_21_03_00_67

Unless the user edited the file, an incomplete line may indicate
that the file has been truncated when (or after) it was written, and
we have to suspect not just that the last "line" may have been
truncated (in this case, not having the full 40-hex object name),
but other records that should have been after that line were lost.

We may want to detect such corruption at runtime, at least at
strategic places; making it a hard runtime error will make it
difficult to use Git itself to recover from such an corruption
(e.g. you may have a healty mirror from which you can recover refs
with "fetch --mirror").

We should at least refrain from running repack/gc to make things
worse, for example.

Re: Possible regression in ref advertisement

[Carlos Martín Nieto]

On Mon, 2013-02-25 at 13:16 -0800, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> >> 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.
> 
> I used that word in the POSIX sense, i.e.
> 
>   http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap03.html#tag_21_03_00_67

Huh, I must revise my POSIX. Sure, in that sense it's incomplete.

> 
> Unless the user edited the file, an incomplete line may indicate
> that the file has been truncated when (or after) it was written, and
> we have to suspect not just that the last "line" may have been
> truncated (in this case, not having the full 40-hex object name),
> but other records that should have been after that line were lost.
> 
> We may want to detect such corruption at runtime, at least at
> strategic places; making it a hard runtime error will make it
> difficult to use Git itself to recover from such an corruption
> (e.g. you may have a healty mirror from which you can recover refs
> with "fetch --mirror").

Since the libgit2 parser seems to work with it, it's perfectly possible
I did mess about with the file and then promptly forgot. An error would
definitely not help here, but I do think a warning should be issued if
the file isn't quite as it should be. It seems the parser can already
detect this, so it could be as easy as adding a fprintf(stderr, "...").

> 
> We should at least refrain from running repack/gc to make things
> worse, for example.
> 

Sounds sensible, yep.

   cmn

Re: Possible regression in ref advertisement

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> Since the libgit2 parser seems to work with it, it's perfectly possible
> I did mess about with the file and then promptly forgot. An error would
> definitely not help here, but I do think a warning should be issued if
> the file isn't quite as it should be. It seems the parser can already
> detect this, so it could be as easy as adding a fprintf(stderr, "...").

Yeah, another thing that may be worth checking is if this was
written/corrupted by hand, in which case there is no need to worry
about it further, or by something else, e.g. reimplementations such
as jgit and libgit2, or a third-party tool that wants to be
compatible with Git, in which case we do want to see that get fixed.