Re: [PATCH v3 06/24] multi-pack-index: load into memory

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 6, 2018, 04:19 UTC
Message-ID: <CAPig+cSntb3=YTBm67+4T1FgLCi585CnUuN__DE2mdo4y2n98A@mail.gmail.com>
In-Reply-To: <20180706005321.124643-7-dstolee@microsoft.com>

On Thu, Jul 5, 2018 at 8:53 PM Derrick Stolee <stolee@gmail.com> wrote:

Show 11 quoted lines

> Create a new multi_pack_index struct for loading multi-pack-indexes into
> memory. Create a test-tool builtin for reading basic information about
> that multi-pack-index to verify the correct data is written.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
> @@ -0,0 +1,34 @@
> +/*
> + * test-mktemp.c: code to exercise the creation of temporary files
> + */

Meh. Copy/paste botch.

Show 6 quoted lines

> +static int read_midx_file(const char *object_dir)
> +{
> +       struct multi_pack_index *m = load_multi_pack_index(object_dir);
> +
> +       if (!m)
> +               return 0;

Should this 'return 0' be a die() or BUG() or something?

Show 14 quoted lines

> +       printf("header: %08x %d %d %d\n",
> +              m->signature,
> +              m->version,
> +              m->num_chunks,
> +              m->num_packs);
> +
> +       printf("object_dir: %s\n", m->object_dir);
> +
> +       return 0;
> +}
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> @@ -3,9 +3,19 @@
> +midx_read_expect () {
> +       cat >expect <<-EOF

I guess you're planning on interpolating some variables here in a later patch, which is why you used -EOF rather than -\EOF?

Show 13 quoted lines

> +       header: 4d494458 1 0 0
> +       object_dir: .
> +       EOF
> +       test-tool read-midx . >actual &&
> +       test_cmp expect actual
> +}
> +
>  test_expect_success 'write midx with no packs' '
>         git multi-pack-index --object-dir=. write &&
> -       test_path_is_file pack/multi-pack-index
> +       test_path_is_file pack/multi-pack-index &&
> +       midx_read_expect
>  '

Kind of a do-nothing change. I wonder if this step would better be delayed until a later patch. (Not necessarily a big deal.)