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

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 22, 2018, 18:44 UTC
Message-ID: <9aa18ceb-672b-31e5-ebfc-0e369ef9bc80@gmail.com>
In-Reply-To: <CACsJy8BHgqqrDNvU_oy0+_QmDW-wt-jBvWT9oJ9H1pA_tmuVFg@mail.gmail.com>

On 6/9/2018 2:13 PM, Duy Nguyen wrote:

Show 7 quoted lines

> 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.

The other places where we call reprepare_packed_git() are for when we may have added a packfile, such as in fetch-pack.c, or sha1_file.c. The other candidate to consider is 'git gc', but the packfile deletion is handled by a call to 'git repack'.

Show 79 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.
>
>> +                               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_
>
>> +{
>> +       char *midx = get_midx_filename(object_dir);
>> +
>> +       if (remove_path(midx))
>> +               die(_("failed to clear multi-pack-index at %s"), midx);
> die_errno()
>
>> +}
>> 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
>>
>