Re: [PATCH 03/23] midx: add midx builtin
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Jun 7, 2018, 17:20 UTC
- Message-ID
- <CACsJy8Ca4B_xjpuTU=sFS+fOEin5fFiSmH+Z7JKjoUA6ZMnv4A@mail.gmail.com>
- In-Reply-To
- <20180607140338.32440-4-dstolee@microsoft.com>
On Thu, Jun 7, 2018 at 4:03 PM, Derrick Stolee <stolee@gmail.com> wrote:
Show 12 quoted lines
> diff --git a/Documentation/git-midx.txt b/Documentation/git-midx.txt > new file mode 100644 > index 0000000000..2bd886f1a2 > --- /dev/null > +++ b/Documentation/git-midx.txt > @@ -0,0 +1,29 @@ > +git-midx(1) > +============ > + > +NAME > +---- > +git-midx - Write and verify multi-pack-indexes (MIDX files).
No full stop. This head line is collected automatically with others and its having a full stop while the rest does not looks strange/
Show 10 quoted lines
> diff --git a/builtin/midx.c b/builtin/midx.c > new file mode 100644 > index 0000000000..59ea92178f > --- /dev/null > +++ b/builtin/midx.c > @@ -0,0 +1,38 @@ > +#include "builtin.h" > +#include "cache.h" > +#include "config.h" > +#include "git-compat-util.h"
You only need either cache.h or git-compat-util.h. If cache.h is here, git-compat-util can be removed.
Show 15 quoted lines
> +#include "parse-options.h"
> +
> +static char const * const builtin_midx_usage[] ={
> + N_("git midx [--object-dir <dir>]"),
> + NULL
> +};
> +
> +static struct opts_midx {
> + const char *object_dir;
> +} opts;
> +
> +int cmd_midx(int argc, const char **argv, const char *prefix)
> +{
> + static struct option builtin_midx_options[] = {
> + { OPTION_STRING, 0, "object-dir", &opts.object_dir,For paths (including dir), OPTION_FILENAME may be a better option to handle correctly when the command is run in a subdir. See df217ed643 (parse-opts: add OPT_FILENAME and transition builtins - 2009-05-23) for more info.
> + N_("dir"),
> + N_("The object directory containing set of packfile and pack-index pairs.") },Other help strings do not have full stop either (I only checked a couple commands though)
Also, doesn't OPT_STRING() work here too (if you avoid OPTION_FILENAME for some reason)?
Show 17 quoted lines
> + OPT_END(), > + }; > + > + if (argc == 2 && !strcmp(argv[1], "-h")) > + usage_with_options(builtin_midx_usage, builtin_midx_options); > + > + git_config(git_default_config, NULL); > + > + argc = parse_options(argc, argv, prefix, > + builtin_midx_options, > + builtin_midx_usage, 0); > + > + if (!opts.object_dir) > + opts.object_dir = get_object_directory(); > + > + return 0; > +}
Show 9 quoted lines
> diff --git a/git.c b/git.c
> index c2f48d53dd..400fadd677 100644
> --- a/git.c
> +++ b/git.c
> @@ -503,6 +503,7 @@ static struct cmd_struct commands[] = {
> { "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> { "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> { "merge-tree", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },
> + { "midx", cmd_midx, RUN_SETUP },If it's a plumbing and can take an --object-dir, then I don't think you should require it to run in a repo (with RUN_SETUP). RUN_SETUP_GENTLY may be better. You could even leave it empty here and only call setup_git_directory() only when --object-dir is not set.
> { "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
> { "mktree", cmd_mktree, RUN_SETUP },
> { "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },-- Duy