{"thread":{"id":"50366","subject":"[RFC PATCH] pack-refs: fail on falsely sorted packed-refs","startedAt":"2019-01-30T23:21:28Z","lastAt":"2019-02-23T07:11:04Z","messageCount":12,"participants":["Max Kirillov","Eric Sunshine","Ævar Arnfjörð Bjarmason","SZEDER Gábor","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"368207","messageId":"20190130231359.23978-1-max@max630.net","threadId":"50366","inReplyTo":null,"subject":"[RFC PATCH] pack-refs: fail on falsely sorted packed-refs","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2019-01-30T23:13:59Z","receivedAt":"2019-01-30T23:21:28Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"If packed-refs is marked as sorted but not really sorted it causes\nvery hard to comprehend misbehavior of reference resolving - a reference\nis reported as not found.\n\nAs the scope of the issue is not clear, make it visible by failing\npack-refs command - the one which would not suffer performance penalty\nto verify the sortedness - when it encounters not really sorted existing\ndata.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nI happened to have a not really sorted packed-refs file. As you might guess,\nit was quite wtf-ing experience. It worked, mostly, but there was one branch\nwhich just did not resolve, regardless of existing and being presented in\nfor-each-refs output.\n\nI don't know where the corruption came from. I should admit it could even be a manual\nediting but last time I did it (in that reporitory) was several years ago so it is unlikely.\n\nI am not sure what should be the proper fix. I did a minimal detection, so that\nit does not go unnoticed. Probably next step would be either fixing in `git fsck` call.\n\n refs/packed-backend.c               | 15 +++++++++++++++\n t/t3212-pack-refs-broken-sorting.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+)\n create mode 100755 t/t3212-pack-refs-broken-sorting.sh\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex c01c7f5901..505f4535b5 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1088,6 +1088,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n \tFILE *out;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *packed_refs_path;\n+\tstruct strbuf prev_ref = STRBUF_INIT;\n \n \tif (!is_lock_file_locked(&refs->lock))\n \t\tBUG(\"write_with_updates() called while unlocked\");\n@@ -1137,6 +1138,20 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\tstruct ref_update *update = NULL;\n \t\tint cmp;\n \n+\t\tif (iter)\n+\t\t{\n+\t\t\tif (prev_ref.len &&  strcmp(prev_ref.buf, iter->refname) > 0)\n+\t\t\t{\n+\t\t\t\tstrbuf_addf(err, \"broken sorting in packed-refs: '%s' > '%s'\",\n+\t\t\t\t\t    prev_ref.buf,\n+\t\t\t\t\t    iter->refname);\n+\t\t\t\tgoto error;\n+\t\t\t}\n+\n+\t\t\tstrbuf_init(&prev_ref, 0);\n+\t\t\tstrbuf_addstr(&prev_ref, iter->refname);\n+\t\t}\n+\n \t\tif (i >= updates->nr) {\n \t\t\tcmp = -1;\n \t\t} else {\ndiff --git a/t/t3212-pack-refs-broken-sorting.sh b/t/t3212-pack-refs-broken-sorting.sh\nnew file mode 100755\nindex 0000000000..37a98a6fb1\n--- /dev/null\n+++ b/t/t3212-pack-refs-broken-sorting.sh\n@@ -0,0 +1,26 @@\n+#!/bin/sh\n+\n+test_description='tests for the falsely sorted refs'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit commit --allow-empty -m commit &&\n+\tfor num in $(test_seq 10)\n+\tdo\n+\t\tgit branch b$(printf \"%02d\" $num) || break\n+\tdone &&\n+\tgit pack-refs --all &&\n+\thead_object=$(git rev-parse HEAD) &&\n+\tprintf \"$head_object refs/heads/b00\\\\n\" >>.git/packed-refs &&\n+\tgit branch b11\n+'\n+\n+test_expect_success 'off-order branch not found' '\n+\t! git show-ref --verify --quiet refs/heads/b00\n+'\n+\n+test_expect_success 'subsequent pack-refs fails' '\n+\t! git pack-refs --all\n+'\n+\n+test_done\n-- \n2.19.0.1202.g68e1e8f04e\n\n"},{"id":"368208","messageId":"CAPig+cTn2gURyQgWHZQMNf2cZ+zwFhbH1Q4iPmbwuvYjMrPZPg@mail.gmail.com","threadId":"50366","inReplyTo":"20190130231359.23978-1-max@max630.net","subject":"Re: [RFC PATCH] pack-refs: fail on falsely sorted packed-refs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-01-30T23:31:34Z","receivedAt":"2019-01-30T23:31:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 30, 2019 at 6:21 PM Max Kirillov <max@max630.net> wrote:\n> If packed-refs is marked as sorted but not really sorted it causes\n> very hard to comprehend misbehavior of reference resolving - a reference\n> is reported as not found.\n>\n> As the scope of the issue is not clear, make it visible by failing\n> pack-refs command - the one which would not suffer performance penalty\n> to verify the sortedness - when it encounters not really sorted existing\n> data.\n>\n> Signed-off-by: Max Kirillov <max@max630.net>\n> ---\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> @@ -1088,6 +1088,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n> +       struct strbuf prev_ref = STRBUF_INIT;\n> @@ -1137,6 +1138,20 @@ static int write_with_updates(struct packed_ref_store *refs,\n> +               if (iter)\n> +               {\n> +                       if (prev_ref.len &&  strcmp(prev_ref.buf, iter->refname) > 0)\n> +                       {\n> +                               strbuf_addf(err, \"broken sorting in packed-refs: '%s' > '%s'\",\n> +                                           prev_ref.buf,\n> +                                           iter->refname);\n\nstrbuf_release(&prev_ref) either here or after the \"error\" label.\n\n> +                               goto error;\n> +                       }\n> +\n> +                       strbuf_init(&prev_ref, 0);\n> +                       strbuf_addstr(&prev_ref, iter->refname);\n> +               }\n> diff --git a/t/t3212-pack-refs-broken-sorting.sh b/t/t3212-pack-refs-broken-sorting.sh\n> @@ -0,0 +1,26 @@\n> +test_expect_success 'setup' '\n> +       git commit --allow-empty -m commit &&\n> +       for num in $(test_seq 10)\n> +       do\n> +               git branch b$(printf \"%02d\" $num) || break\n\nThis should probably be \"|| return 1\" rather than \"|| break\" in order\nto fail the test immediately.\n\n> +       done &&\n> +       git pack-refs --all &&\n> +       head_object=$(git rev-parse HEAD) &&\n> +       printf \"$head_object refs/heads/b00\\\\n\" >>.git/packed-refs &&\n> +       git branch b11\n> +'\n> +\n> +test_expect_success 'off-order branch not found' '\n> +       ! git show-ref --verify --quiet refs/heads/b00\n> +'\n\nUse test_must_fail() rather than '!' when expecting a Git command to fail.\n\n> +test_expect_success 'subsequent pack-refs fails' '\n> +       ! git pack-refs --all\n> +'\n\nDitto.\n"},{"id":"368226","messageId":"20190131082140.GA24787@jessie.local","threadId":"50366","inReplyTo":"CAPig+cTn2gURyQgWHZQMNf2cZ+zwFhbH1Q4iPmbwuvYjMrPZPg@mail.gmail.com","subject":"Re: [RFC PATCH] pack-refs: fail on falsely sorted packed-refs","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2019-01-31T08:21:41Z","receivedAt":"2019-01-31T08:29:07Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Jan 30, 2019 at 06:31:34PM -0500, Eric Sunshine wrote:\n> On Wed, Jan 30, 2019 at 6:21 PM Max Kirillov <max@max630.net> wrote:\n>> +                               strbuf_addf(err, \"broken sorting in packed-refs: '%s' > '%s'\",\n>> +                                           prev_ref.buf,\n>> +                                           iter->refname);\n\n> strbuf_release(&prev_ref) either here or after the \"error\" label.\n\nThanks! I seem to forget about it.\n\n> > +               git branch b$(printf \"%02d\" $num) || break\n\n> This should probably be \"|| return 1\" rather than \"|| break\" in order\n> to fail the test immediately.\n\nI've been looking for the correct way, and have seen the\nbreak somewhere. Now I see the \"return 1\" is mostly user.\nThanks, will fix.\n\n> Use test_must_fail() rather than '!' when expecting a Git command to fail.\n\nWill fix in both places\n"},{"id":"368884","messageId":"20190208212221.31670-1-max@max630.net","threadId":"50366","inReplyTo":"CAPig+cTn2gURyQgWHZQMNf2cZ+zwFhbH1Q4iPmbwuvYjMrPZPg@mail.gmail.com","subject":"[PATCH v2] pack-refs: fail on falsely sorted packed-refs","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2019-02-08T21:22:21Z","receivedAt":"2019-02-08T21:22:36Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"If packed-refs is marked as sorted but not really sorted it causes\nvery hard to comprehend misbehavior of reference resolving - a reference\nis reported as not found, though it is listed by commands which output\nthe references list.\n\nAs the scope of the issue is not clear, make it visible by failing\npack-refs command - the one which would not suffer performance penalty\nto verify the sortedness - when it encounters not really sorted existing\ndata.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nFixed the notes\n refs/packed-backend.c               | 18 ++++++++++++++++++\n t/t3212-pack-refs-broken-sorting.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+)\n create mode 100755 t/t3212-pack-refs-broken-sorting.sh\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex c01c7f5901..c89a5eb899 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1088,6 +1088,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n \tFILE *out;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *packed_refs_path;\n+\tstruct strbuf prev_ref = STRBUF_INIT;\n \n \tif (!is_lock_file_locked(&refs->lock))\n \t\tBUG(\"write_with_updates() called while unlocked\");\n@@ -1137,6 +1138,21 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\tstruct ref_update *update = NULL;\n \t\tint cmp;\n \n+\t\tif (iter)\n+\t\t{\n+\t\t\tif (prev_ref.len &&  strcmp(prev_ref.buf, iter->refname) > 0)\n+\t\t\t{\n+\t\t\t\tstrbuf_addf(err, \"broken sorting in packed-refs: '%s' > '%s'\",\n+\t\t\t\t\t    prev_ref.buf,\n+\t\t\t\t\t    iter->refname);\n+\t\t\t\tstrbuf_release(&prev_ref);\n+\t\t\t\tgoto error;\n+\t\t\t}\n+\n+\t\t\tstrbuf_init(&prev_ref, 0);\n+\t\t\tstrbuf_addstr(&prev_ref, iter->refname);\n+\t\t}\n+\n \t\tif (i >= updates->nr) {\n \t\t\tcmp = -1;\n \t\t} else {\n@@ -1240,6 +1256,8 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\t}\n \t}\n \n+\tstrbuf_release(&prev_ref);\n+\n \tif (ok != ITER_DONE) {\n \t\tstrbuf_addstr(err, \"unable to write packed-refs file: \"\n \t\t\t      \"error iterating over old contents\");\ndiff --git a/t/t3212-pack-refs-broken-sorting.sh b/t/t3212-pack-refs-broken-sorting.sh\nnew file mode 100755\nindex 0000000000..a44785c8fc\n--- /dev/null\n+++ b/t/t3212-pack-refs-broken-sorting.sh\n@@ -0,0 +1,26 @@\n+#!/bin/sh\n+\n+test_description='tests for the falsely sorted refs'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit commit --allow-empty -m commit &&\n+\tfor num in $(test_seq 10)\n+\tdo\n+\t\tgit branch b$(printf \"%02d\" $num) || return 1\n+\tdone &&\n+\tgit pack-refs --all &&\n+\thead_object=$(git rev-parse HEAD) &&\n+\tprintf \"$head_object refs/heads/b00\\\\n\" >>.git/packed-refs &&\n+\tgit branch b11\n+'\n+\n+test_expect_success 'off-order branch not found' '\n+\ttest_must_fail git show-ref --verify --quiet refs/heads/b00\n+'\n+\n+test_expect_success 'subsequent pack-refs fails' '\n+\ttest_must_fail git pack-refs --all\n+'\n+\n+test_done\n-- \n2.19.0.1202.g68e1e8f04e\n\n"},{"id":"368885","messageId":"CAPig+cSNoXQQrDDXt6yN-gbYc3P4ZEiwJL1nwQWBomyLdVm_Vg@mail.gmail.com","threadId":"50366","inReplyTo":"20190208212221.31670-1-max@max630.net","subject":"Re: [PATCH v2] pack-refs: fail on falsely sorted packed-refs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-08T21:40:28Z","receivedAt":"2019-02-08T21:40:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 8, 2019 at 4:22 PM Max Kirillov <max@max630.net> wrote:\n> If packed-refs is marked as sorted but not really sorted it causes\n> very hard to comprehend misbehavior of reference resolving - a reference\n> is reported as not found, though it is listed by commands which output\n> the references list.\n>\n> As the scope of the issue is not clear, make it visible by failing\n> pack-refs command - the one which would not suffer performance penalty\n> to verify the sortedness - when it encounters not really sorted existing\n> data.\n>\n> Signed-off-by: Max Kirillov <max@max630.net>\n> ---\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> @@ -1137,6 +1138,21 @@ static int write_with_updates(struct packed_ref_store *refs,\n> +               if (iter)\n> +               {\n> +                       if (prev_ref.len &&  strcmp(prev_ref.buf, iter->refname) > 0)\n> +                       {\n> +                               [...]\n> +                               strbuf_release(&prev_ref);\n> +                               goto error;\n> +                       }\n> +\n> +                       strbuf_init(&prev_ref, 0);\n> +                       strbuf_addstr(&prev_ref, iter->refname);\n> +               }\n\nThe call to strbuf_init() is leaking the allocated strbuf buffer each\ntime through the loop. The typical way to re-use a strbuf, and the way\nyou should do it here, is strbuf_reset().\n"},{"id":"369204","messageId":"20190213042307.GA3064@jessie.local","threadId":"50366","inReplyTo":"CAMy9T_EX_L80-V4zD626nFCxw6qa90+pZwcbd6wHw9ZHcj2rNA@mail.gmail.com","subject":"Re: [PATCH v2] pack-refs: fail on falsely sorted packed-refs","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2019-02-13T04:23:07Z","receivedAt":"2019-02-13T04:23:16Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Feb 11, 2019 at 08:24:46PM +0100, Michael Haggerty wrote:\n> The change to `write_with_updates()` doesn't only affect `pack-refs`.\n> That function is also called when the `packed-refs` file has to be\n> rewritten when a packed reference is deleted. This is another thing\n> that you could test.\n\nOk, I'll check ti and add to the tests.\n\n> But that also means that fairly common commands like `git branch -d`\n> could be slowed down by this change. I doubt that the slowdown is\n> prohibitive, but it would be great to see numbers to prove it. For\n> example, create a repository with a lot (say 10000) references, pack\n> them, then run `git branch -d` to delete one of them. Benchmark that\n> once with master and once with your modification and document the\n> difference.\n\nAt my hardware, with 1M references, \"branch -d\" takes 0.31s\nof user time before change vs 0.38 after change. Should I\nmention it in the commit message?\n\n>> +test_expect_success 'off-order branch not found' '\n>> +       test_must_fail git show-ref --verify --quiet refs/heads/b00\n>> +'\n> \n> I don't think that the above test makes sense. We don't *guarantee*\n> that an out-of-order reference won't be found. That is an\n> implementation detail that we are free to change. I think that it\n> would be OK to just omit this test.\n\nThanks, will remove this one\n"},{"id":"369205","messageId":"20190213042404.GB3064@jessie.local","threadId":"50366","inReplyTo":"CAPig+cSNoXQQrDDXt6yN-gbYc3P4ZEiwJL1nwQWBomyLdVm_Vg@mail.gmail.com","subject":"Re: [PATCH v2] pack-refs: fail on falsely sorted packed-refs","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2019-02-13T04:24:04Z","receivedAt":"2019-02-13T04:24:09Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Fri, Feb 08, 2019 at 04:40:28PM -0500, Eric Sunshine wrote:\n> The call to strbuf_init() is leaking the allocated strbuf buffer each\n> time through the loop. The typical way to re-use a strbuf, and the way\n> you should do it here, is strbuf_reset().\n\nThank you! Will fix it\n"},{"id":"369219","messageId":"87lg2kj91a.fsf@evledraar.gmail.com","threadId":"50366","inReplyTo":"20190130231359.23978-1-max@max630.net","subject":"Re: [RFC PATCH] pack-refs: fail on falsely sorted packed-refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-13T10:08:01Z","receivedAt":"2019-02-13T10:08:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 31 2019, Max Kirillov wrote:\n\n> If packed-refs is marked as sorted but not really sorted it causes\n> very hard to comprehend misbehavior of reference resolving - a reference\n> is reported as not found.\n>\n> As the scope of the issue is not clear, make it visible by failing\n> pack-refs command - the one which would not suffer performance penalty\n> to verify the sortedness - when it encounters not really sorted existing\n> data.\n>\n> Signed-off-by: Max Kirillov <max@max630.net>\n> ---\n> I happened to have a not really sorted packed-refs file. As you might guess,\n> it was quite wtf-ing experience. It worked, mostly, but there was one branch\n> which just did not resolve, regardless of existing and being presented in\n> for-each-refs output.\n>\n> I don't know where the corruption came from. I should admit it could even be a manual\n> editing but last time I did it (in that reporitory) was several years ago so it is unlikely.\n>\n> I am not sure what should be the proper fix. I did a minimal detection, so that\n> it does not go unnoticed. Probably next step would be either fixing in `git fsck` call.\n>\n>  refs/packed-backend.c               | 15 +++++++++++++++\n>  t/t3212-pack-refs-broken-sorting.sh | 26 ++++++++++++++++++++++++++\n>  2 files changed, 41 insertions(+)\n>  create mode 100755 t/t3212-pack-refs-broken-sorting.sh\n\nThis is not an area I'm very familiar with. So mostly commeting on\ncosmetic issues with the patch. FWIW the \"years back\" issue you had\ncould be that an issue didn't manifest until now, i.e. in a sorted file\nformat you can get lucky and not see corruption for a while with a\nrandom insert.\n\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index c01c7f5901..505f4535b5 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1088,6 +1088,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n>  \tFILE *out;\n>  \tstruct strbuf sb = STRBUF_INIT;\n>  \tchar *packed_refs_path;\n> +\tstruct strbuf prev_ref = STRBUF_INIT;\n>\n>  \tif (!is_lock_file_locked(&refs->lock))\n>  \t\tBUG(\"write_with_updates() called while unlocked\");\n> @@ -1137,6 +1138,20 @@ static int write_with_updates(struct packed_ref_store *refs,\n>  \t\tstruct ref_update *update = NULL;\n>  \t\tint cmp;\n>\n> +\t\tif (iter)\n> +\t\t{\n> +\t\t\tif (prev_ref.len &&  strcmp(prev_ref.buf, iter->refname) > 0)\n\nYou have an extra two whitespaces after \"&&\" there.\n\n> +\t\t\t{\n> +\t\t\t\tstrbuf_addf(err, \"broken sorting in packed-refs: '%s' > '%s'\",\n> +\t\t\t\t\t    prev_ref.buf,\n> +\t\t\t\t\t    iter->refname);\n> +\t\t\t\tgoto error;\n> +\t\t\t}\n> +\n> +\t\t\tstrbuf_init(&prev_ref, 0);\n> +\t\t\tstrbuf_addstr(&prev_ref, iter->refname);\n> +\t\t}\n> +\n>  \t\tif (i >= updates->nr) {\n>  \t\t\tcmp = -1;\n>  \t\t} else {\n> diff --git a/t/t3212-pack-refs-broken-sorting.sh b/t/t3212-pack-refs-broken-sorting.sh\n> new file mode 100755\n> index 0000000000..37a98a6fb1\n> --- /dev/null\n> +++ b/t/t3212-pack-refs-broken-sorting.sh\n> @@ -0,0 +1,26 @@\n> +#!/bin/sh\n> +\n> +test_description='tests for the falsely sorted refs'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\tgit commit --allow-empty -m commit &&\n\nLooks like just \"test_commit A\" would do here.\n\n> +\tfor num in $(test_seq 10)\n> +\tdo\n> +\t\tgit branch b$(printf \"%02d\" $num) || break\n> +\tdone &&\n\nWe can fail in these sorts of loops. There's a few ways to deal with\nthat. Doing it like this with \"break\" will still silently hide errors:\n\n    $ for i in $(seq 1 3); do if test $i = 2; then false || break; else echo $i; fi; done && echo success\n    1\n    success\n\nOne way to deal with that is to e.g. before the loop say \"had_fail=\",\nthen set \"had_fail=t\" in that \"||\" case, and test for it after the loop.\n\nBut perhaps in this case we're better off e.g. running for-each-ref\nafter and either using test_cmp or test_line_count to see that we\ncreated the refs successfully?\n\n> +\tgit pack-refs --all &&\n> +\thead_object=$(git rev-parse HEAD) &&\n> +\tprintf \"$head_object refs/heads/b00\\\\n\" >>.git/packed-refs &&\n\nLooks like just \"echo\" here would be simpler since we only use printf to\nadd a newline.\n\n> +\tgit branch b11\n> +'\n> +\n> +test_expect_success 'off-order branch not found' '\n> +\t! git show-ref --verify --quiet refs/heads/b00\n> +'\n> +\n> +test_expect_success 'subsequent pack-refs fails' '\n> +\t! git pack-refs --all\n> +'\n\nInstead of \"! git ...\" use \"test_must_fail git ...\". See t/README. This\nwill hide e.g. segfaults.\n\nAlso, perhaps:\n\n    test_must_fail git ... 2>stderr &&\n    grep \"broken sorting in packed-refs\" stderr\n\nWould make this more obvious/self-documenting so we know we failed due\nto that issue in particular.\n"},{"id":"369226","messageId":"20190213105616.GH1622@szeder.dev","threadId":"50366","inReplyTo":"87lg2kj91a.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] pack-refs: fail on falsely sorted packed-refs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-13T10:56:16Z","receivedAt":"2019-02-13T10:56:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Feb 13, 2019 at 11:08:01AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Jan 31 2019, Max Kirillov wrote:\n\n> >  refs/packed-backend.c               | 15 +++++++++++++++\n> >  t/t3212-pack-refs-broken-sorting.sh | 26 ++++++++++++++++++++++++++\n> >  2 files changed, 41 insertions(+)\n> >  create mode 100755 t/t3212-pack-refs-broken-sorting.sh\n> \n> This is not an area I'm very familiar with. So mostly commeting on\n> cosmetic issues with the patch. \n\nJust two quick comments in addition to Ævar's:\n\n> > @@ -1137,6 +1138,20 @@ static int write_with_updates(struct packed_ref_store *refs,\n> >  \t\tstruct ref_update *update = NULL;\n> >  \t\tint cmp;\n> >\n> > +\t\tif (iter)\n> > +\t\t{\n\nAccording to our CodingGuidelines, the opening bracket should go on\nthe same line as the condition, i.e.\n\n  if (iter) {\n\n> > diff --git a/t/t3212-pack-refs-broken-sorting.sh b/t/t3212-pack-refs-broken-sorting.sh\n> > new file mode 100755\n> > index 0000000000..37a98a6fb1\n> > --- /dev/null\n> > +++ b/t/t3212-pack-refs-broken-sorting.sh\n> > @@ -0,0 +1,26 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='tests for the falsely sorted refs'\n> > +. ./test-lib.sh\n> > +\n> > +test_expect_success 'setup' '\n> > +\tgit commit --allow-empty -m commit &&\n> \n> Looks like just \"test_commit A\" would do here.\n> \n> > +\tfor num in $(test_seq 10)\n> > +\tdo\n> > +\t\tgit branch b$(printf \"%02d\" $num) || break\n> > +\tdone &&\n> \n> We can fail in these sorts of loops. There's a few ways to deal with\n> that. Doing it like this with \"break\" will still silently hide errors:\n> \n>     $ for i in $(seq 1 3); do if test $i = 2; then false || break; else echo $i; fi; done && echo success\n>     1\n>     success\n> \n> One way to deal with that is to e.g. before the loop say \"had_fail=\",\n> then set \"had_fail=t\" in that \"||\" case, and test for it after the loop.\n\nNo, you can simply do 'cmd1 && cmd2 || return 1' in the body of the\nfor loop; that's why we have a separate test_eval_inner() helper\nfunction in test-lib.\n\n"},{"id":"369293","messageId":"20190214060657.GA20997@sigill.intra.peff.net","threadId":"50366","inReplyTo":"87lg2kj91a.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] pack-refs: fail on falsely sorted packed-refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-14T06:06:57Z","receivedAt":"2019-02-14T06:07:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 13, 2019 at 11:08:01AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > I happened to have a not really sorted packed-refs file. As you might guess,\n> > it was quite wtf-ing experience. It worked, mostly, but there was one branch\n> > which just did not resolve, regardless of existing and being presented in\n> > for-each-refs output.\n> >\n> > I don't know where the corruption came from. I should admit it could even be a manual\n> > editing but last time I did it (in that reporitory) was several years ago so it is unlikely.\n> >\n> > I am not sure what should be the proper fix. I did a minimal detection, so that\n> > it does not go unnoticed. Probably next step would be either fixing in `git fsck` call.\n> >\n> >  refs/packed-backend.c               | 15 +++++++++++++++\n> >  t/t3212-pack-refs-broken-sorting.sh | 26 ++++++++++++++++++++++++++\n> >  2 files changed, 41 insertions(+)\n> >  create mode 100755 t/t3212-pack-refs-broken-sorting.sh\n> \n> This is not an area I'm very familiar with. So mostly commeting on\n> cosmetic issues with the patch. FWIW the \"years back\" issue you had\n> could be that an issue didn't manifest until now, i.e. in a sorted file\n> format you can get lucky and not see corruption for a while with a\n> random insert.\n\nIt actually shouldn't be that old a breakage. Until 02b920f3f7\n(read_packed_refs(): ensure that references are ordered when read,\n2017-09-25), we did not assume the file was sorted (even though we\nalways wrote it out sorted). And we continue to not assume the file is\nsorted unless it is written out with an explicit \"sorted\" trait in the\nheader (which we started doing in that commit, too).\n\nSo a years-old manual edit would not have the \"sorted\" trait, and should\nnot have manifested as a problem, even now.  Likewise for a years-old\nbug. It would have to be a bug in a _new_ writer which writes out the\nsorted trait.  If there is such a bug in our implementation, this would\nbe the first report we've seen. Given the number of times pack-refs has\nbeen run, without further evidence I'm inclined to think it was some\nweird manual edit, or maybe an alternate implementation (though one\nwould _hope_ they would not write out the sorted trait without actually\nsorting!).\n\nI agree with all of the cosmetic issues you mentioned. As far as what\nthe patch itself does, I think it's OK. We could probably go further and\nactually sort it (or even just write it out without a \"sorted\" trait,\nwhich means the next read would load it all into memory and sort it).\nThat's a little friendlier, since just dying leaves the user to fix it\nup themselves. But given that we expect this code to trigger\napproximately never, it's probably not worth spending much time on a\nfancy solution.\n\n-Peff\n"},{"id":"370005","messageId":"20190223070914.GC2354@jessie.local","threadId":"50366","inReplyTo":"87lg2kj91a.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] pack-refs: fail on falsely sorted packed-refs","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2019-02-23T07:09:14Z","receivedAt":"2019-02-23T07:09:20Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Feb 13, 2019 at 11:08:01AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> You have an extra two whitespaces after \"&&\" there.\n\nThanks, will check it.\n\n>> +\tgit commit --allow-empty -m commit &&\n> Looks like just \"test_commit A\" would do here.\n\nAbout this I'm not sure. AFAIK test_commit does lots of stuff,\nso can it be considered \"just\" compared to \"commit\n--allow-empty\" or the opposite? I could replace it with\ntest_commit for uniformity reason though.\n\n> We can fail in these sorts of loops. There's a few ways to deal with\n> that. Doing it like this with \"break\" will still silently hide errors:\n\nThanks, this was pointed point\n\n>> +\tprintf \"$head_object refs/heads/b00\\\\n\" >>.git/packed-refs &&\n> \n> Looks like just \"echo\" here would be simpler since we only use printf to\n> add a newline.\n\nCould it happen so that \"echo\" adds '\\r\\n' at Windows? I\ncould use echo.\n\n> Instead of \"! git ...\" use \"test_must_fail git ...\". See t/README. This\n> will hide e.g. segfaults.\n\nThanks, this was pointed point\n\n> Also, perhaps:\n> \n>     test_must_fail git ... 2>stderr &&\n>     grep \"broken sorting in packed-refs\" stderr\n> \n> Would make this more obvious/self-documenting so we know we failed due\n> to that issue in particular.\n\nThanks, will change it\n"},{"id":"370006","messageId":"20190223071059.GD2354@jessie.local","threadId":"50366","inReplyTo":"20190213105616.GH1622@szeder.dev","subject":"Re: [RFC PATCH] pack-refs: fail on falsely sorted packed-refs","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2019-02-23T07:10:59Z","receivedAt":"2019-02-23T07:11:04Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Feb 13, 2019 at 11:56:16AM +0100, SZEDER Gábor wrote:\n>>> +\t\tif (iter)\n>>> +\t\t{\n> \n> According to our CodingGuidelines, the opening bracket should go on\n> the same line as the condition, i.e.\n> \n>   if (iter) {\n\nOh, thanks. I must have been professionally deformed.\n"}]}