Re: [RFC PATCH] pack-refs: fail on falsely sorted packed-refs

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jan 30, 2019 at 6:21 PM Max Kirillov <max@max630.net> wrote:
> If packed-refs is marked as sorted but not really sorted it causes
> very hard to comprehend misbehavior of reference resolving - a reference
> is reported as not found.
>
> As the scope of the issue is not clear, make it visible by failing
> pack-refs command - the one which would not suffer performance penalty
> to verify the sortedness - when it encounters not really sorted existing
> data.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> @@ -1088,6 +1088,7 @@ static int write_with_updates(struct packed_ref_store *refs,
> +       struct strbuf prev_ref = STRBUF_INIT;
> @@ -1137,6 +1138,20 @@ static int write_with_updates(struct packed_ref_store *refs,
> +               if (iter)
> +               {
> +                       if (prev_ref.len &&  strcmp(prev_ref.buf, iter->refname) > 0)
> +                       {
> +                               strbuf_addf(err, "broken sorting in packed-refs: '%s' > '%s'",
> +                                           prev_ref.buf,
> +                                           iter->refname);

strbuf_release(&prev_ref) either here or after the "error" label.

> +                               goto error;
> +                       }
> +
> +                       strbuf_init(&prev_ref, 0);
> +                       strbuf_addstr(&prev_ref, iter->refname);
> +               }
> diff --git a/t/t3212-pack-refs-broken-sorting.sh b/t/t3212-pack-refs-broken-sorting.sh
> @@ -0,0 +1,26 @@
> +test_expect_success 'setup' '
> +       git commit --allow-empty -m commit &&
> +       for num in $(test_seq 10)
> +       do
> +               git branch b$(printf "%02d" $num) || break

This should probably be "|| return 1" rather than "|| break" in order
to fail the test immediately.

> +       done &&
> +       git pack-refs --all &&
> +       head_object=$(git rev-parse HEAD) &&
> +       printf "$head_object refs/heads/b00\\n" >>.git/packed-refs &&
> +       git branch b11
> +'
> +
> +test_expect_success 'off-order branch not found' '
> +       ! git show-ref --verify --quiet refs/heads/b00
> +'

Use test_must_fail() rather than '!' when expecting a Git command to fail.

> +test_expect_success 'subsequent pack-refs fails' '
> +       ! git pack-refs --all
> +'

Ditto.