{"thread":{"id":"30348","subject":"ref-format checking regression","startedAt":"2012-04-27T11:50:01Z","lastAt":"2012-04-28T09:24:50Z","messageCount":5,"participants":["Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"190208","messageId":"20120427115000.GA8126@sigill.intra.peff.net","threadId":"30348","inReplyTo":null,"subject":"ref-format checking regression","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-27T11:50:01Z","receivedAt":"2012-04-27T11:50:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I upgraded git on a machine recently, and it created problems for a repo\nwith a bogus character in a ref name. Older versions of git never\ncomplained about it. Newer ones, containing your dce4bab (\"add_ref():\nverify that the refname is formatted correctly\") do. That's fine; it's\nbogus and git _should_ complain about it.\n\nHowever, recovering from the situation is unnecessarily hard, as basic\nthings like fsck stop working, you can't access the ref via rev-parse,\netc. You can reproduce with something like this (in some repo with\nactual commits):\n\n $ evil=$(printf 'foo\\177bar')\n $ git rev-parse HEAD >\".git/refs/tags/$evil\"\n\n $ git fsck\n fatal: Reference has invalid format: 'refs/tags/foo^?bar'\n\n [does not even warn about the real reason for the error]\n $ git rev-parse --verify \"$evil\"\n fatal: Needed a single revision\n\n [does not let you find broken refs]\n $ git for-each-ref\n fatal: Reference has invalid format: 'refs/tags/foo^?bar'\n $ git tag -l\n fatal: Reference has invalid format: 'refs/tags/foo^?bar'\n\n [no way to rename or delete the bogus tag]\n $ git tag fixed \"$evil\"\n fatal: Failed to resolve 'foo^?bar' as a valid ref.\n\nI seem to recall discussing this format-tightening and trying to be sure\nthat users were left with a way forward for fixing their repos. But I\ncan't find the discussion, and I don't recall any conclusion we came to.\nSo maybe we decided not to worry about it. But I thought I'd mention it\nas a data point.\n\n-Peff\n"},{"id":"190211","messageId":"CACBZZX6i9C-RyB9-jtJkHjYOpqgDmDcBFwe10RP+bu5PGTmP9Q@mail.gmail.com","threadId":"30348","inReplyTo":"20120427115000.GA8126@sigill.intra.peff.net","subject":"Re: ref-format checking regression","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2012-04-27T12:57:53Z","receivedAt":"2012-04-27T12:57:53Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Apr 27, 2012 at 13:50, Jeff King wrote:\n> I seem to recall discussing this format-tightening and trying to be sure\n> that users were left with a way forward for fixing their repos. But I\n> can't find the discussion, and I don't recall any conclusion we came to.\n> So maybe we decided not to worry about it. But I thought I'd mention it\n> as a data point.\n\nWasn't that the one about the tightening of the ref checks breaking\ngit clone --mirror?\n\nIn any case we should be strict on input and loose when going over\nexisting data. It seems to me that we're using one function now to\ncheck the validity of refs when we should be using two, one for new\nrefs, and one for reading existing data.\n"},{"id":"190217","messageId":"xmqqr4v99pu5.fsf@junio.mtv.corp.google.com","threadId":"30348","inReplyTo":"20120427115000.GA8126@sigill.intra.peff.net","subject":"Re: ref-format checking regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-27T15:06:26Z","receivedAt":"2012-04-27T15:06:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I upgraded git on a machine recently, and it created problems for a repo\n> with a bogus character in a ref name. Older versions of git never\n> complained about it. Newer ones, containing your dce4bab (\"add_ref():\n> verify that the refname is formatted correctly\") do. That's fine; it's\n> bogus and git _should_ complain about it.\n> \n> However, recovering from the situation is unnecessarily hard, ...\n> ...\n> I seem to recall discussing this format-tightening and trying to be sure\n> that users were left with a way forward for fixing their repos. But I\n> can't find the discussion, and I don't recall any conclusion we came to.\n\nI haven't dug the archive but I do recall pointing many issues out\naround the theme \"be liberal in what you accept and strict in what you\nproduce\" on this topic, and loosening one or two showstoppers during the\nreview cycle, but obviously we did not catch all of them.\n\nMichael?\n"},{"id":"190219","messageId":"xmqqipgl9oz3.fsf@junio.mtv.corp.google.com","threadId":"30348","inReplyTo":"xmqqr4v99pu5.fsf@junio.mtv.corp.google.com","subject":"Re: ref-format checking regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-27T15:25:04Z","receivedAt":"2012-04-27T15:25:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> I upgraded git on a machine recently, and it created problems for a repo\n>> with a bogus character in a ref name. Older versions of git never\n>> complained about it. Newer ones, containing your dce4bab (\"add_ref():\n>> verify that the refname is formatted correctly\") do. That's fine; it's\n>> bogus and git _should_ complain about it.\n>> \n>> However, recovering from the situation is unnecessarily hard, ...\n>> ...\n>> I seem to recall discussing this format-tightening and trying to be sure\n>> that users were left with a way forward for fixing their repos. But I\n>> can't find the discussion, and I don't recall any conclusion we came to.\n>\n> I haven't dug the archive but I do recall ...\n\nA few...\n\n http://thread.gmane.org/gmane.comp.version-control.git/185564\n http://thread.gmane.org/gmane.comp.version-control.git/184382/focus=185483\n"},{"id":"190233","messageId":"20120428092450.GA2850@sigill.intra.peff.net","threadId":"30348","inReplyTo":"xmqqr4v99pu5.fsf@junio.mtv.corp.google.com","subject":"Re: ref-format checking regression","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-28T09:24:50Z","receivedAt":"2012-04-28T09:24:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 27, 2012 at 08:06:26AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I upgraded git on a machine recently, and it created problems for a repo\n> > with a bogus character in a ref name. Older versions of git never\n> > complained about it. Newer ones, containing your dce4bab (\"add_ref():\n> > verify that the refname is formatted correctly\") do. That's fine; it's\n> > bogus and git _should_ complain about it.\n> > \n> > However, recovering from the situation is unnecessarily hard, ...\n> > ...\n> > I seem to recall discussing this format-tightening and trying to be sure\n> > that users were left with a way forward for fixing their repos. But I\n> > can't find the discussion, and I don't recall any conclusion we came to.\n> \n> I haven't dug the archive but I do recall pointing many issues out\n> around the theme \"be liberal in what you accept and strict in what you\n> produce\" on this topic, and loosening one or two showstoppers during the\n> review cycle, but obviously we did not catch all of them.\n\nI should point out that this was due to GitHub recently upgrading the\nversion of git on our backend servers. And out of the bazillion repos we\nhost, I have so far seen only one actual bug report. So while it might\nbe nice to be more friendly, it may simply not be all that common an\nissue (and in this case, we were able to resolve it manually). If it's\neasy to fix, I think we should, but if the fix ends up being very\ncomplex, it might not be worth the trouble.\n\n-Peff\n"}]}