Re: [PATCH 04/23] midx: add 'write' subcommand and basic wiring

From: Duy Nguyen <pclouds@gmail.com>
Date: Jun 7, 2018, 17:27 UTC
Message-ID: <CACsJy8DwNeYQUdJPnD694v3cEanq00f=zWz_bx53syJ47=ekyQ@mail.gmail.com>
In-Reply-To: <20180607140338.32440-5-dstolee@microsoft.com>

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

Show 22 quoted lines

> diff --git a/builtin/midx.c b/builtin/midx.c
> index 59ea92178f..dc0a5acd3f 100644
> --- a/builtin/midx.c
> +++ b/builtin/midx.c
> @@ -3,9 +3,10 @@
>  #include "config.h"
>  #include "git-compat-util.h"
>  #include "parse-options.h"
> +#include "midx.h"
>
>  static char const * const builtin_midx_usage[] ={
> -       N_("git midx [--object-dir <dir>]"),
> +       N_("git midx [--object-dir <dir>] [write]"),
>         NULL
>  };
>
> @@ -34,5 +35,11 @@ int cmd_midx(int argc, const char **argv, const char *prefix)
>         if (!opts.object_dir)
>                 opts.object_dir = get_object_directory();
>
> +       if (argc == 0)
> +               return 0;

Isn't it better to die here when no verb is given? I don't see any good use case for running a no-op "git midx" without verbs. It's more likely a mistake (e.g. "git midx $foo" where foo happens to be empty)

Show 14 quoted lines

> +
> +       if (!strcmp(argv[0], "write"))
> +               return write_midx_file(opts.object_dir);
> +
>         return 0;
>  }
> diff --git a/midx.c b/midx.c
> new file mode 100644
> index 0000000000..616af66b13
> --- /dev/null
> +++ b/midx.c
> @@ -0,0 +1,9 @@
> +#include "git-compat-util.h"
> +#include "cache.h"

Only one of the two is needed

> +#include "dir.h"

Not needed yet. It's better to include it in the patch that actually needs it.

Show 14 quoted lines

> +#include "midx.h"
> +
> +int write_midx_file(const char *object_dir)
> +{
> +       return 0;
> +}
> diff --git a/midx.h b/midx.h
> new file mode 100644
> index 0000000000..3a63673952
> --- /dev/null
> +++ b/midx.h
> @@ -0,0 +1,4 @@
> +#include "cache.h"
> +#include "packfile.h"

These includes are not needed, at least not now. And please protect the header file with #ifndef __MINDX_H__ .. #endif.

Show 14 quoted lines

> +
> +int write_midx_file(const char *object_dir);
> diff --git a/t/t5319-midx.sh b/t/t5319-midx.sh
> new file mode 100755
> index 0000000000..a590137af7
> --- /dev/null
> +++ b/t/t5319-midx.sh
> @@ -0,0 +1,10 @@
> +#!/bin/sh
> +
> +test_description='multi-pack-indexes'
> +. ./test-lib.sh
> +
> +test_expect_success 'write midx with no pakcs' '

no packs

Show 7 quoted lines

> +       git midx --object-dir=. write
> +'
> +
> +test_done
> --
> 2.18.0.rc1
>

-- 
Duy