Re: [PATCH 23/23] midx: clear midx on repack

From: Duy Nguyen <pclouds@gmail.com>
Date: Jun 9, 2018, 18:13 UTC
Message-ID: <CACsJy8BHgqqrDNvU_oy0+_QmDW-wt-jBvWT9oJ9H1pA_tmuVFg@mail.gmail.com>
In-Reply-To: <20180607140338.32440-24-dstolee@microsoft.com>

On Thu, Jun 7, 2018 at 4:07 PM Derrick Stolee <stolee@gmail.com> wrote:

>
> If a 'git repack' command replaces existing packfiles, then we must
> clear the existing multi-pack-index before moving the packfiles it
> references.

I think there are other places where we add or remove pack files and need to reprepare_packed_git(). Any midx invalidation should be part of that as well.

Show 34 quoted lines

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/repack.c | 8 ++++++++
>  midx.c           | 8 ++++++++
>  midx.h           | 1 +
>  3 files changed, 17 insertions(+)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 6c636e159e..66a7d8e8ea 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -8,6 +8,7 @@
>  #include "strbuf.h"
>  #include "string-list.h"
>  #include "argv-array.h"
> +#include "midx.h"
>
>  static int delta_base_offset = 1;
>  static int pack_kept_objects = -1;
> @@ -174,6 +175,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>         int no_update_server_info = 0;
>         int quiet = 0;
>         int local = 0;
> +       int midx_cleared = 0;
>
>         struct option builtin_repack_options[] = {
>                 OPT_BIT('a', NULL, &pack_everything,
> @@ -340,6 +342,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>                                 continue;
>                         }
>
> +                       if (!midx_cleared) {
> +                               /* if we move a packfile, it will invalidated the midx */

What about removing packs, which also happens in repack? If the removed pack is part of midx, then midx becomes invalid as well.

Show 17 quoted lines

> +                               clear_midx_file(get_object_directory());
> +                               midx_cleared = 1;
> +                       }
> +
>                         fname_old = mkpathdup("%s/old-%s%s", packdir,
>                                                 item->string, exts[ext].name);
>                         if (file_exists(fname_old))
> diff --git a/midx.c b/midx.c
> index e46f392fa4..1043c01fa7 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -913,3 +913,11 @@ int write_midx_file(const char *object_dir)
>         FREE_AND_NULL(pack_names);
>         return 0;
>  }
> +
> +void clear_midx_file(const char *object_dir)

delete_ may be more obvious than clear_

Show 5 quoted lines

> +{
> +       char *midx = get_midx_filename(object_dir);
> +
> +       if (remove_path(midx))
> +               die(_("failed to clear multi-pack-index at %s"), midx);

die_errno()

Show 15 quoted lines

> +}
> diff --git a/midx.h b/midx.h
> index 6996b5ff6b..46f9f44c94 100644
> --- a/midx.h
> +++ b/midx.h
> @@ -18,5 +18,6 @@ int midx_contains_pack(struct midxed_git *m, const char *idx_name);
>  int prepare_midxed_git_one(struct repository *r, const char *object_dir);
>
>  int write_midx_file(const char *object_dir);
> +void clear_midx_file(const char *object_dir);
>
>  #endif
> --
> 2.18.0.rc1
>

-- 
Duy