Re: [PATCH 1/5] multi-pack-index: prepare for 'expire' verb

From: Stefan Beller <sbeller@google.com>
Date: Dec 11, 2018, 01:35 UTC
Message-ID: <CAGZ79kYeHgCdzNNXUnp1NaokT=Vpj4mUVt1bZb5c+EkgO2z-4w@mail.gmail.com>
In-Reply-To: <1e34b48a2051089052c0b53b0059576b5ace45de.1544465177.git.gitgitgadget@gmail.com>

On Mon, Dec 10, 2018 at 10:06 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 38 quoted lines

>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The multi-pack-index tracks objects in a collection of pack-files.
> Only one copy of each object is indexed, using the modified time
> of the pack-files to determine tie-breakers. It is possible to
> have a pack-file with no referenced objects because all objects
> have a duplicate in a newer pack-file.
>
> Introduce a new 'expire' verb to the multi-pack-index builtin.
> This verb will delete these unused pack-files and rewrite the
> multi-pack-index to no longer refer to those files. More details
> about the specifics will follow as the method is implemented.
>
> Add a test that verifies the 'expire' verb is correctly wired,
> but will still be valid when the verb is implemented. Specifically,
> create a set of packs that should all have referenced objects and
> should not be removed during an 'expire' operation.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-multi-pack-index.txt |  8 +++++
>  builtin/multi-pack-index.c             |  4 ++-
>  midx.c                                 |  5 +++
>  midx.h                                 |  1 +
>  t/t5319-multi-pack-index.sh            | 47 ++++++++++++++++++++++++++
>  5 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt
> index f7778a2c85..822d83c845 100644
> --- a/Documentation/git-multi-pack-index.txt
> +++ b/Documentation/git-multi-pack-index.txt
> @@ -31,6 +31,14 @@ verify::
>         When given as the verb, verify the contents of the MIDX file
>         at `<dir>/packs/multi-pack-index`.
>
> +expire::
> +       When given as the verb,

Can it be given in another way? Or rather "if the verb is expire", then ... (I just checked the current man page, and both write and verify use this pattern as well. I find it strange as this first part of the sentence conveys little information, but is repeated 3 times now (once for each verb)).

Maybe we can restructure the man page to have it more like

    The following verbs are available:
    +write::
    +    create a new MIDX file, writing to <dir>/packs/multi-pack-index.
    +
    +verify::
    +    verify the contents ...

>               delete the pack-files that are tracked
> +       by the MIDX file at `<dir>/packs/multi-pack-index`

We're mentioning the location a lot. Could we keep a more detailed note in --object-dir and not go into such detail in the verbs section? (Then the paragraph here would be more concise. That makes it easier to understand)

>              but have
> +       no objects referenced by the MIDX. All objects in these pack-
> +       files have another copy in a more-recently modified pack-file.

The second sentence reads like a reason on why the first is a good thing to have, so maybe use some subordinating conjunction adverb ("because") to make tell the reader

> +       Rewrite the MIDX file afterward to remove all references to
> +       these pack-files.

Makes sense.

Show 47 quoted lines

>
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 70926b5bc0..948effc1ee 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -348,4 +348,51 @@ test_expect_success 'verify incorrect 64-bit offset' '
>                 "incorrect object offset"
>  '
>
> +test_expect_success 'setup expire tests' '
> +       mkdir dup &&
> +       (
> +               cd dup &&
> +               git init &&
> +               for i in $(test_seq 1 20)
> +               do
> +                       test_commit $i
> +               done &&
> +               git branch A HEAD &&
> +               git branch B HEAD~8 &&
> +               git branch C HEAD~13 &&
> +               git branch D HEAD~16 &&
> +               git branch E HEAD~18 &&
> +               git pack-objects --revs .git/objects/pack/pack-E <<-EOF &&
> +               refs/heads/E
> +               EOF
> +               git pack-objects --revs .git/objects/pack/pack-D <<-EOF &&
> +               refs/heads/D
> +               ^refs/heads/E
> +               EOF
> +               git pack-objects --revs .git/objects/pack/pack-C <<-EOF &&
> +               refs/heads/C
> +               ^refs/heads/D
> +               EOF
> +               git pack-objects --revs .git/objects/pack/pack-B <<-EOF &&
> +               refs/heads/B
> +               ^refs/heads/C
> +               EOF
> +               git pack-objects --revs .git/objects/pack/pack-A <<-EOF &&
> +               refs/heads/A
> +               ^refs/heads/B
> +               EOF
> +               git multi-pack-index write
> +       )
> +'
> +
> +test_expect_success 'expire does not remove any packs' '

With the clever setup, this test is already correctly testing what the docs claims it should do, despite having no implementation. Nice. Although the core issue is that the packs are disjunct sets of objects, so maybe /s/any packs/required packs/ or such?