{"thread":{"id":"48153","subject":"[RFC 0/1] Tolerate broken headers in `packed-refs` files","startedAt":"2018-03-26T12:43:15Z","lastAt":"2018-03-26T14:34:00Z","messageCount":5,"participants":["Michael Haggerty","Derrick Stolee","Jeff King","Edward Thomson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"342993","messageId":"cover.1522062649.git.mhagger@alum.mit.edu","threadId":"48153","inReplyTo":null,"subject":"[RFC 0/1] Tolerate broken headers in `packed-refs` files","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2018-03-26T12:42:58Z","receivedAt":"2018-03-26T12:43:15Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Prior to\n\n    9308b7f3ca read_packed_refs(): die if `packed-refs` contains bogus data, 2017-07-01\n\nwe silently ignored pretty much any bogus data in a `packed-refs`\nfile. I think that was pretty clearly a bad policy. The above commit\nmade parsing quite a bit stricter, calling `die()` if it found any\nlines that it didn't understand.\n\nBut there's one situation that is maybe not quite so clear-cut. The\nfirst line of a `packed-refs` file can be a header that enumerates\nsome traits of the file containing it; for example,\n\n    # pack-refs with: peeled fully-peeled \n\nThe old code would have tolerated lots of breakage in that line. For\nexample, any of the following headers would have just been ignored:\n\n    # arbitrary data that looks like a comment\n    # pack-refs with peeled fully-peeled          ← note: missing colon\n    # pack-refs\n\nNow, any of the above lines are considered errors and cause git to\ndie.\n\nIn my opinion, that is a good thing and we *shouldn't* tolerate broken\nheader lines; i.e., the status quo is good and we *shouldn't* apply\nthis patch.\n\nBut there might be some tools out in the wild that have been writing\nbroken headers. In that case, users who upgrade Git might suddenly\nfind that they can't read repositories that they could read before. In\nfact, a tool that we wrote and use internally at GitHub was doing\nexactly that, which is how we discovered this \"problem\".\n\nThis patch shows what it would look like to relax the parsing again,\nalbeit *only* for the first line of the file, and *only* for lines\nthat start with '#'.\n\nThe problem with this patch is that it would make it harder for people\nwho implement broken tools in the future to discover their mistakes.\nThe only result of the error would be that it is slower to work with\nthe `packed-refs` files that they wrote. Such an error could go\nundiscovered for a long time.\n\nThe tighter check was released quite a while ago, and AFAIK we haven't\nhad any bug reports from people tripped up by this consistency check.\nSo I'm inclined to believe that the existing tools are OK and this\npatch would be counterproductive. But I wanted to share it with the\nlist anyway.\n\nMichael\n\nMichael Haggerty (1):\n  packed-backend: ignore broken headers\n\n refs/packed-backend.c | 21 +++++++++------------\n t/t3210-pack-refs.sh  | 33 ++++++++++++++++++++++++++++++++-\n 2 files changed, 41 insertions(+), 13 deletions(-)\n\n-- \n2.14.2\n\n"},{"id":"342994","messageId":"fca2318b35cfd19f8fa2278919306ba4c6475382.1522062649.git.mhagger@alum.mit.edu","threadId":"48153","inReplyTo":"cover.1522062649.git.mhagger@alum.mit.edu","subject":"[RFC 1/1] packed-backend: ignore broken headers","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2018-03-26T12:42:59Z","receivedAt":"2018-03-26T12:43:18Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Prior to\n\n    9308b7f3ca read_packed_refs(): die if `packed-refs` contains bogus data, 2017-07-01\n\nwe silently ignored any lines in a `packed-refs` file that we didn't\nunderstand. That policy was clearly wrong.\n\nBut at the time, unrecognized header lines were processed by the same\ncode as reference and peeled lines. This means that they were also\nsubject to the same liberal treatment. For example, any of the\nfollowing \"header\" lines would have been ignored:\n\n* \"# arbitrary data that looks like a comment\"\n* \"# pack-refs with peeled fully-peeled\" ← note: missing colon\n* \"# pack-refs\"\n\nLoosen up the parser to ignore any first line that begins with `#` but\ndoesn't start with the exact character sequence \"# pack-refs with:\".\n\n(In fact, the old liberal policy meant that \"comment\" lines would have\nbeen ignored anywhere in the file. But the file format isn't actually\ndocumented to allow comments, and comments make little sense in this\nfile, so we won't go that far.)\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nI *don't* think we actually want to merge this patch. See the cover\nletter for my reasoning.\n\n refs/packed-backend.c | 21 +++++++++------------\n t/t3210-pack-refs.sh  | 33 ++++++++++++++++++++++++++++++++-\n 2 files changed, 41 insertions(+), 13 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 65288c6472..f9d71bb60d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -635,21 +635,18 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n \n \t\ttmp = xmemdupz(snapshot->buf, eol - snapshot->buf);\n \n-\t\tif (!skip_prefix(tmp, \"# pack-refs with:\", (const char **)&p))\n-\t\t\tdie_invalid_line(refs->path,\n-\t\t\t\t\t snapshot->buf,\n-\t\t\t\t\t snapshot->eof - snapshot->buf);\n+\t\tif (skip_prefix(tmp, \"# pack-refs with:\", (const char **)&p)) {\n+\t\t\tstring_list_split_in_place(&traits, p, ' ', -1);\n \n-\t\tstring_list_split_in_place(&traits, p, ' ', -1);\n+\t\t\tif (unsorted_string_list_has_string(&traits, \"fully-peeled\"))\n+\t\t\t\tsnapshot->peeled = PEELED_FULLY;\n+\t\t\telse if (unsorted_string_list_has_string(&traits, \"peeled\"))\n+\t\t\t\tsnapshot->peeled = PEELED_TAGS;\n \n-\t\tif (unsorted_string_list_has_string(&traits, \"fully-peeled\"))\n-\t\t\tsnapshot->peeled = PEELED_FULLY;\n-\t\telse if (unsorted_string_list_has_string(&traits, \"peeled\"))\n-\t\t\tsnapshot->peeled = PEELED_TAGS;\n+\t\t\tsorted = unsorted_string_list_has_string(&traits, \"sorted\");\n \n-\t\tsorted = unsorted_string_list_has_string(&traits, \"sorted\");\n-\n-\t\t/* perhaps other traits later as well */\n+\t\t\t/* perhaps other traits later as well */\n+\t\t}\n \n \t\t/* The \"+ 1\" is for the LF character. */\n \t\tsnapshot->start = eol + 1;\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex afa27ffe2d..353ef3e655 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -20,7 +20,8 @@ test_expect_success \\\n     'echo Hello > A &&\n      git update-index --add A &&\n      git commit -m \"Initial commit.\" &&\n-     HEAD=$(git rev-parse --verify HEAD)'\n+     HEAD=$(git rev-parse --verify HEAD) &&\n+     git tag -m \"A tag\" annotated-tag'\n \n SHA1=\n \n@@ -221,6 +222,36 @@ test_expect_success 'reject packed-refs with a short SHA-1' '\n \ttest_cmp expected_err err\n '\n \n+test_expect_success 'handle packed-refs with a bogus header' '\n+\tgit show-ref -d >expected &&\n+\tmv .git/packed-refs .git/packed-refs.bak &&\n+\ttest_when_finished \"mv .git/packed-refs.bak .git/packed-refs\" &&\n+\tsed -e \"s/^#.*/# whacked-refs/\" <.git/packed-refs.bak >.git/packed-refs &&\n+\tgit show-ref -d >actual 2>err &&\n+\ttest_cmp expected actual &&\n+\ttest_must_be_empty err\n+'\n+\n+test_expect_success 'handle packed-refs with a truncated header' '\n+\tgit show-ref -d >expected &&\n+\tmv .git/packed-refs .git/packed-refs.bak &&\n+\ttest_when_finished \"mv .git/packed-refs.bak .git/packed-refs\" &&\n+\tsed -e \"s/^#.*/# pack-refs/\" <.git/packed-refs.bak >.git/packed-refs &&\n+\tgit show-ref -d >actual 2>err &&\n+\ttest_cmp expected actual &&\n+\ttest_must_be_empty err\n+'\n+\n+test_expect_success 'handle packed-refs with no traits in header' '\n+\tgit show-ref -d >expected &&\n+\tmv .git/packed-refs .git/packed-refs.bak &&\n+\ttest_when_finished \"mv .git/packed-refs.bak .git/packed-refs\" &&\n+\tsed -e \"s/^#.*/# pack-refs with:/\" <.git/packed-refs.bak >.git/packed-refs &&\n+\tgit show-ref -d >actual 2>err &&\n+\ttest_cmp expected actual &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'timeout if packed-refs.lock exists' '\n \tLOCK=.git/packed-refs.lock &&\n \t>\"$LOCK\" &&\n-- \n2.14.2\n\n"},{"id":"343000","messageId":"f8d0f3b6-69b3-ba42-c39c-551814caf335@gmail.com","threadId":"48153","inReplyTo":"cover.1522062649.git.mhagger@alum.mit.edu","subject":"Re: [RFC 0/1] Tolerate broken headers in `packed-refs` files","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-03-26T13:08:04Z","receivedAt":"2018-03-26T13:08:10Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/26/2018 8:42 AM, Michael Haggerty wrote:\n> [...]\n>\n> But there might be some tools out in the wild that have been writing\n> broken headers. In that case, users who upgrade Git might suddenly\n> find that they can't read repositories that they could read before. In\n> fact, a tool that we wrote and use internally at GitHub was doing\n> exactly that, which is how we discovered this \"problem\".\n>\n> This patch shows what it would look like to relax the parsing again,\n> albeit *only* for the first line of the file, and *only* for lines\n> that start with '#'.\n>\n> The problem with this patch is that it would make it harder for people\n> who implement broken tools in the future to discover their mistakes.\n> The only result of the error would be that it is slower to work with\n> the `packed-refs` files that they wrote. Such an error could go\n> undiscovered for a long time.\n\nMy opinion is that we shouldn't maintain back-compat with formats that \nmay have been written by another tool because Git wasn't strict about \nit. As long as Git never wrote files with these formats, then they \nshouldn't be supported.\n\nYou are absolutely right that staying strict will help discover the \ntools that are writing an incorrect format.\n\nSince most heavily-used tools that didn't spawn Git processes use \nLibGit2 to interact with Git repos, I added Ed Thomson to CC to see if \nlibgit2 could ever write these bad header comments.\n\nThanks for writing this RFC so we can have the discussion and more \nquickly identify this issue if/when users are broken.\n\nThanks,\n-Stolee\n"},{"id":"343010","messageId":"20180326143340.GA23926@sigill.intra.peff.net","threadId":"48153","inReplyTo":"f8d0f3b6-69b3-ba42-c39c-551814caf335@gmail.com","subject":"Re: [RFC 0/1] Tolerate broken headers in `packed-refs` files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-26T14:33:40Z","receivedAt":"2018-03-26T14:33:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 26, 2018 at 09:08:04AM -0400, Derrick Stolee wrote:\n\n> Since most heavily-used tools that didn't spawn Git processes use LibGit2 to\n> interact with Git repos, I added Ed Thomson to CC to see if libgit2 could\n> ever write these bad header comments.\n\nEd can probably answer more definitively, but I dug in the libgit2\nhistory a little, and I think it has only ever generated correct\n\"pack-refs with:\" lines.\n\nDitto for JGit, though there it blames down to 1a6964c82 (Initial JGit\ncontribution to eclipse.org, 2009-09-29). I didn't dig further into the\nhistorical JGit repository, but I think that's probably far enough to\nfeel good about it.\n\n-Peff\n"},{"id":"343011","messageId":"CA+WKDT1LMuwuXar9i-1RJ15+o5Rxp013FL9fqfAfMydZJ9cQ6g@mail.gmail.com","threadId":"48153","inReplyTo":"f8d0f3b6-69b3-ba42-c39c-551814caf335@gmail.com","subject":"Re: [RFC 0/1] Tolerate broken headers in `packed-refs` files","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2018-03-26T14:33:54Z","receivedAt":"2018-03-26T14:34:00Z","isPatch":false,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"On Mon, Mar 26, 2018 at 2:08 PM, Derrick Stolee <stolee@gmail.com> wrote:\n> Since most heavily-used tools that didn't spawn Git processes use\n> LibGit2 to interact with Git repos, I added Ed Thomson to CC to see\n> if libgit2 could ever write these bad header comments.\n\nWe added the `sorted` capability to our `packed-refs` header relatively\nrecently (approximately two months ago, v0.27.0 will be the first release\nto include it as of today).  So, at the moment, libgit2 writes:\n\n  \"# pack-refs with: peeled fully-peeled sorted \"\n\nPrior to this change, libgit2's header was stable for the last five years\nas:\n\n  \"# pack-refs with: peeled fully-peeled \"\n\nAnd prior to that, we advertised only `peeled`:\n\n  \"# pack-refs with: peeled \"\n\nThanks for thinking of us!\n\n-ed\n"}]}