{"thread":{"id":"33008","subject":"Possible regression in ref advertisement","startedAt":"2013-02-25T16:58:36Z","lastAt":"2013-02-26T15:46:37Z","messageCount":10,"participants":["Carlos Martín Nieto","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"210253","messageId":"1361811516.3212.14.camel@centaur.cmartin.tk","threadId":"33008","inReplyTo":null,"subject":"Possible regression in ref advertisement","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-02-25T16:58:36Z","receivedAt":"2013-02-25T16:58:36Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Hi all,\n\nWhen testing to see if a different implementation was in shape, I came\nacross something odd where newer git doesn't advertise one of the refs\nin the git repo.\n\nRunning `git ls-remote .` or `git-upload-pack` in my git repo, newer git\nversions omit peeling the v1.8.0-rc3 tag.\n\nThe diff between the command above when ran with 1.7.10.4 (from Debian)\nand current 'master'\n\n--- old 2013-02-25 17:31:29.583526606 +0100\n+++ new 2013-02-25 17:31:36.783526559 +0100\n@@ -1379,7 +1379,6 @@\n c15295d7477ccec489953299bd03a8e62f86e611 refs/tags/v1.8.0-rc2\n cd46259ebf2e624bcee2aaae05c36663d414e1a2 refs/tags/v1.8.0-rc2^{}\n 22ed067acc84eac8a0a72d20478a18aee4e25571 refs/tags/v1.8.0-rc3\n-87a5461fa7b30f7b7baf27204f10219d61500fbf refs/tags/v1.8.0-rc3^{}\n bfeb8b9ae0012cb61e026cbcd29664876abf5389 refs/tags/v1.8.0.1\n ed9fe755130891fc878bb2433204faffb534697b refs/tags/v1.8.0.1^{}\n 63add1fb45e1ab7a76bb38bbb9467c91fdfaaa7e refs/tags/v1.8.0.2\n\nDiffing with the output from next, diff tells me it's binary for some\nreason, but looking manually, the peeled v1.8.0-rc3 tag isn't there\neither. I haven't had time to bisect this, so I'm putting it out there\nin case anybody wants to investigate before I have time to dig into it.\n\n\n cmn\n"},{"id":"210263","messageId":"7vvc9gxn2y.fsf@alter.siamese.dyndns.org","threadId":"33008","inReplyTo":"1361811516.3212.14.camel@centaur.cmartin.tk","subject":"Re: Possible regression in ref advertisement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T18:31:01Z","receivedAt":"2013-02-25T18:31:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> Hi all,\n>\n> When testing to see if a different implementation was in shape, I came\n> across something odd where newer git doesn't advertise one of the refs\n> in the git repo.\n>\n> Running `git ls-remote .` or `git-upload-pack` in my git repo, newer git\n> versions omit peeling the v1.8.0-rc3 tag.\n>\n> The diff between the command above when ran with 1.7.10.4 (from Debian)\n> and current 'master'\n>\n> --- old 2013-02-25 17:31:29.583526606 +0100\n> +++ new 2013-02-25 17:31:36.783526559 +0100\n> @@ -1379,7 +1379,6 @@\n> c15295d7477ccec489953299bd03a8e62f86e611 refs/tags/v1.8.0-rc2\n> cd46259ebf2e624bcee2aaae05c36663d414e1a2 refs/tags/v1.8.0-rc2^{}\n> 22ed067acc84eac8a0a72d20478a18aee4e25571 refs/tags/v1.8.0-rc3\n> -87a5461fa7b30f7b7baf27204f10219d61500fbf refs/tags/v1.8.0-rc3^{}\n> bfeb8b9ae0012cb61e026cbcd29664876abf5389 refs/tags/v1.8.0.1\n> ed9fe755130891fc878bb2433204faffb534697b refs/tags/v1.8.0.1^{}\n> 63add1fb45e1ab7a76bb38bbb9467c91fdfaaa7e refs/tags/v1.8.0.2\n>\n> Diffing with the output from next, diff tells me it's binary for some\n> reason, but looking manually, the peeled v1.8.0-rc3 tag isn't there\n> either. I haven't had time to bisect this, so I'm putting it out there\n> in case anybody wants to investigate before I have time to dig into it.\n\nInteresting. \"git ls-remote . | grep 1.8.0-\" for maint, master,\nnext and pu produce identical results for me, all showing peeled\nones correctly.\n"},{"id":"210276","messageId":"1361819916.24515.5.camel@centaur.cmartin.tk","threadId":"33008","inReplyTo":"7vvc9gxn2y.fsf@alter.siamese.dyndns.org","subject":"Re: Possible regression in ref advertisement","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-02-25T19:18:36Z","receivedAt":"2013-02-25T19:18:36Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Mon, 2013-02-25 at 10:31 -0800, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> > Hi all,\n> >\n> > When testing to see if a different implementation was in shape, I came\n> > across something odd where newer git doesn't advertise one of the refs\n> > in the git repo.\n> >\n> > Running `git ls-remote .` or `git-upload-pack` in my git repo, newer git\n> > versions omit peeling the v1.8.0-rc3 tag.\n> >\n> > The diff between the command above when ran with 1.7.10.4 (from Debian)\n> > and current 'master'\n> >\n> > --- old 2013-02-25 17:31:29.583526606 +0100\n> > +++ new 2013-02-25 17:31:36.783526559 +0100\n> > @@ -1379,7 +1379,6 @@\n> > c15295d7477ccec489953299bd03a8e62f86e611 refs/tags/v1.8.0-rc2\n> > cd46259ebf2e624bcee2aaae05c36663d414e1a2 refs/tags/v1.8.0-rc2^{}\n> > 22ed067acc84eac8a0a72d20478a18aee4e25571 refs/tags/v1.8.0-rc3\n> > -87a5461fa7b30f7b7baf27204f10219d61500fbf refs/tags/v1.8.0-rc3^{}\n> > bfeb8b9ae0012cb61e026cbcd29664876abf5389 refs/tags/v1.8.0.1\n> > ed9fe755130891fc878bb2433204faffb534697b refs/tags/v1.8.0.1^{}\n> > 63add1fb45e1ab7a76bb38bbb9467c91fdfaaa7e refs/tags/v1.8.0.2\n> >\n> > Diffing with the output from next, diff tells me it's binary for some\n> > reason, but looking manually, the peeled v1.8.0-rc3 tag isn't there\n> > either. I haven't had time to bisect this, so I'm putting it out there\n> > in case anybody wants to investigate before I have time to dig into it.\n> \n> Interesting. \"git ls-remote . | grep 1.8.0-\" for maint, master,\n> next and pu produce identical results for me, all showing peeled\n> ones correctly.\n\nBisection leads me to Peff's 435c8332 (2012-10-04; upload-pack: use\npeel_ref for ref advertisements) and reverting that commit brings the\n-rc3^{} back.\n\n cmn\n"},{"id":"210277","messageId":"7vip5gxkgy.fsf@alter.siamese.dyndns.org","threadId":"33008","inReplyTo":"1361819916.24515.5.camel@centaur.cmartin.tk","subject":"Re: Possible regression in ref advertisement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T19:27:25Z","receivedAt":"2013-02-25T19:27:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> On Mon, 2013-02-25 at 10:31 -0800, Junio C Hamano wrote:\n> ...\n>> Interesting. \"git ls-remote . | grep 1.8.0-\" for maint, master,\n>> next and pu produce identical results for me, all showing peeled\n>> ones correctly.\n>\n> Bisection leads me to Peff's 435c8332 (2012-10-04; upload-pack: use\n> peel_ref for ref advertisements) and reverting that commit brings the\n> -rc3^{} back.\n\nA shot in the dark, as I do not seem to be able to reproduce the issue\nwith anything that contains the commit. Perhaps your .git/packed-refs\nis corrupt?\n"},{"id":"210287","messageId":"1361822092.30765.12.camel@centaur.cmartin.tk","threadId":"33008","inReplyTo":"7vip5gxkgy.fsf@alter.siamese.dyndns.org","subject":"Re: Possible regression in ref advertisement","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-02-25T19:54:52Z","receivedAt":"2013-02-25T19:54:52Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Mon, 2013-02-25 at 11:27 -0800, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> > On Mon, 2013-02-25 at 10:31 -0800, Junio C Hamano wrote:\n> > ...\n> >> Interesting. \"git ls-remote . | grep 1.8.0-\" for maint, master,\n> >> next and pu produce identical results for me, all showing peeled\n> >> ones correctly.\n> >\n> > Bisection leads me to Peff's 435c8332 (2012-10-04; upload-pack: use\n> > peel_ref for ref advertisements) and reverting that commit brings the\n> > -rc3^{} back.\n> \n> A shot in the dark, as I do not seem to be able to reproduce the issue\n> with anything that contains the commit. Perhaps your .git/packed-refs\n> is corrupt?\n\nMy packed-refs file did not end with LF. It seems it must or the parser\nwon't consider the last tag in the file to have a peeled target and mine\ndidn't. I don't how the ended up this way but it looks like there's is a\nbug in the parser (or does the format force you to have LF at the end?)\n\n cmn\n"},{"id":"210288","messageId":"7v1uc4ximl.fsf@alter.siamese.dyndns.org","threadId":"33008","inReplyTo":"1361822092.30765.12.camel@centaur.cmartin.tk","subject":"Re: Possible regression in ref advertisement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T20:07:14Z","receivedAt":"2013-02-25T20:07:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n>> A shot in the dark, as I do not seem to be able to reproduce the issue\n>> with anything that contains the commit. Perhaps your .git/packed-refs\n>> is corrupt?\n>\n> My packed-refs file did not end with LF. It seems it must or the parser\n> won't consider the last tag in the file to have a peeled target and mine\n> didn't. I don't how the ended up this way but it looks like there's is a\n> bug in the parser (or does the format force you to have LF at the end?)\n\nIt is unclear what kind of corruption your packed-refs file had, as\nthere are multiple ways for a file not to \"end with LF\", but anyway.\n\nAs packed-refs file is expected to be a text file, it is not\nsurprising to get an undefined result if the it ends with an\nincomplete line.\n\nI do not offhand recall if we tolerate lines with CRLF endings; as\nfar as I know we do not _write_ CRLF out, and because we do not\nexpect people to muck directly with editor bypassing \"pack-refs\", I\nwould not be surprised if we didn't do anything special for people\nwho make the lines end with with CRLF the file, either.\n\nI'd be more worried about the possibly lost entries after that\nincomplete line (and also possibly truncated end part of the tagname\non the last, incomplete line). Perhaps fsck should diagnose such an\nanomaly as repository corruption? Perhaps we should enhance the\nfile format a bit (right now, the first \"capability\" line should say\nsomething like \"# pack-refs with: peeled\" to say it was created with\nthe version of pack-refs that can record peeled tags, but we can add\nother capabilities to extend the format) to add checksums to detect\ncorruption?\n"},{"id":"210291","messageId":"1361824519.31692.12.camel@centaur.cmartin.tk","threadId":"33008","inReplyTo":"7v1uc4ximl.fsf@alter.siamese.dyndns.org","subject":"Re: Possible regression in ref advertisement","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-02-25T20:35:19Z","receivedAt":"2013-02-25T20:35:19Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Mon, 2013-02-25 at 12:07 -0800, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> >> A shot in the dark, as I do not seem to be able to reproduce the issue\n> >> with anything that contains the commit. Perhaps your .git/packed-refs\n> >> is corrupt?\n> >\n> > My packed-refs file did not end with LF. It seems it must or the parser\n> > won't consider the last tag in the file to have a peeled target and mine\n> > didn't. I don't how the ended up this way but it looks like there's is a\n> > bug in the parser (or does the format force you to have LF at the end?)\n> \n> It is unclear what kind of corruption your packed-refs file had, as\n> there are multiple ways for a file not to \"end with LF\", but anyway.\n\nI mean that the file ended at the end of the peeled id. It was missing\nthe last empty line.\n\n> \n> As packed-refs file is expected to be a text file, it is not\n> surprising to get an undefined result if the it ends with an\n> incomplete line.\n\nI guess that depends on what you mean by incomplete. All the data is\nthere, just that it had\n\n SP LF ^\n\nas the last entry instead of\n\n SP LF ^ LF\n\nThe parser skips the last entry if there is no trailing LF (the same\nhappens for lightweight tags, though here it simply doesn't report\nthem).\n\n> \n> I do not offhand recall if we tolerate lines with CRLF endings; as\n> far as I know we do not _write_ CRLF out, and because we do not\n> expect people to muck directly with editor bypassing \"pack-refs\", I\n> would not be surprised if we didn't do anything special for people\n> who make the lines end with with CRLF the file, either.\n\nI wouldn't expect it to work. It would probably skip over those entries.\n\n> \n> I'd be more worried about the possibly lost entries after that\n> incomplete line (and also possibly truncated end part of the tagname\n> on the last, incomplete line). Perhaps fsck should diagnose such an\n> anomaly as repository corruption? Perhaps we should enhance the\n> file format a bit (right now, the first \"capability\" line should say\n> something like \"# pack-refs with: peeled\" to say it was created with\n> the version of pack-refs that can record peeled tags, but we can add\n> other capabilities to extend the format) to add checksums to detect\n> corruption?\n> \n\nI just tested and the parser ignores any malformed lines. The ones after\nit are fine. Nothing complains though, ls-remote just shows the next\nentry. I'd expect at least fsck to complain, but it doesn't say\nanything. Presumably they all use the same parser that just ignores\nanything it doesn't like.\n\n cmn\n"},{"id":"210295","messageId":"7vfw0kt7qc.fsf@alter.siamese.dyndns.org","threadId":"33008","inReplyTo":"1361824519.31692.12.camel@centaur.cmartin.tk","subject":"Re: Possible regression in ref advertisement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T21:16:11Z","receivedAt":"2013-02-25T21:16:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n>> As packed-refs file is expected to be a text file, it is not\n>> surprising to get an undefined result if the it ends with an\n>> incomplete line.\n>\n> I guess that depends on what you mean by incomplete.\n\nI used that word in the POSIX sense, i.e.\n\n http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap03.html#tag_21_03_00_67\n\nUnless the user edited the file, an incomplete line may indicate\nthat the file has been truncated when (or after) it was written, and\nwe have to suspect not just that the last \"line\" may have been\ntruncated (in this case, not having the full 40-hex object name),\nbut other records that should have been after that line were lost.\n\nWe may want to detect such corruption at runtime, at least at\nstrategic places; making it a hard runtime error will make it\ndifficult to use Git itself to recover from such an corruption\n(e.g. you may have a healty mirror from which you can recover refs\nwith \"fetch --mirror\").\n\nWe should at least refrain from running repack/gc to make things\nworse, for example.\n"},{"id":"210324","messageId":"1361891069.9005.11.camel@flaca.cmartin.tk","threadId":"33008","inReplyTo":"7vfw0kt7qc.fsf@alter.siamese.dyndns.org","subject":"Re: Possible regression in ref advertisement","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-02-26T15:04:29Z","receivedAt":"2013-02-26T15:04:29Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Mon, 2013-02-25 at 13:16 -0800, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> >> As packed-refs file is expected to be a text file, it is not\n> >> surprising to get an undefined result if the it ends with an\n> >> incomplete line.\n> >\n> > I guess that depends on what you mean by incomplete.\n> \n> I used that word in the POSIX sense, i.e.\n> \n> http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap03.html#tag_21_03_00_67\n\nHuh, I must revise my POSIX. Sure, in that sense it's incomplete.\n\n> \n> Unless the user edited the file, an incomplete line may indicate\n> that the file has been truncated when (or after) it was written, and\n> we have to suspect not just that the last \"line\" may have been\n> truncated (in this case, not having the full 40-hex object name),\n> but other records that should have been after that line were lost.\n> \n> We may want to detect such corruption at runtime, at least at\n> strategic places; making it a hard runtime error will make it\n> difficult to use Git itself to recover from such an corruption\n> (e.g. you may have a healty mirror from which you can recover refs\n> with \"fetch --mirror\").\n\nSince the libgit2 parser seems to work with it, it's perfectly possible\nI did mess about with the file and then promptly forgot. An error would\ndefinitely not help here, but I do think a warning should be issued if\nthe file isn't quite as it should be. It seems the parser can already\ndetect this, so it could be as easy as adding a fprintf(stderr, \"...\").\n\n> \n> We should at least refrain from running repack/gc to make things\n> worse, for example.\n> \n\nSounds sensible, yep.\n\n cmn\n"},{"id":"210326","messageId":"7v621fnkma.fsf@alter.siamese.dyndns.org","threadId":"33008","inReplyTo":"1361891069.9005.11.camel@flaca.cmartin.tk","subject":"Re: Possible regression in ref advertisement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-26T15:46:37Z","receivedAt":"2013-02-26T15:46:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> Since the libgit2 parser seems to work with it, it's perfectly possible\n> I did mess about with the file and then promptly forgot. An error would\n> definitely not help here, but I do think a warning should be issued if\n> the file isn't quite as it should be. It seems the parser can already\n> detect this, so it could be as easy as adding a fprintf(stderr, \"...\").\n\nYeah, another thing that may be worth checking is if this was\nwritten/corrupted by hand, in which case there is no need to worry\nabout it further, or by something else, e.g. reimplementations such\nas jgit and libgit2, or a third-party tool that wants to be\ncompatible with Git, in which case we do want to see that get fixed.\n"}]}