[PATCH v3 00/12] fast-import: standardize usage string and SYNOPSIS
Christian Couder <[email protected]>
| Newsgroups | org.kernel.vger.git |
|---|---|
| Message-ID | <[email protected]> |
The goal of this series is to improve on `git fast-import`'s usage
string as it is obsolete in many ways.
As it appeared that a good way to reach that goal was to make
`git fast-import` use the parse-options API, this series also achieves
this secondary goal.
Along the way it modernizes "builtin/fast-import.c" mostly by using
`struct option`, by starting to remove global variables and libify
that command, and by introducing a new `OPT_HIDDEN_GROUP` macro.
There are still many global variables left, so it's left to future
work to finish on that direction.
Anyway the usage string is standardized and consistent with the
SYNOPSIS in the docs, so that the command can be removed from
"t/t0450/adoc-help-mismatches".
Using the parse-options API also enabled some code standardization and
simplification.
Overview of the patches
=======================
- Patch 1/12: Introduces OPT_HIDDEN_GROUP and improves on the hidden
option tests.
- Patches 2/12 and 3/12: Improve on the parse-options API docs.
- Patch 4/12: Cleans up an 'i' loop counter variable in
cmd_fast_import().
- Patches 5/12 and 6/12: Prepare for using the parse-option API to
parse command line options.
- Patches 7/12 and 8/12: Start libifying "builtin/fast-import.c" by
introducing a 'struct fast_import_state' and using it to store
some global variables.
- Patch 9/12: Improves the usage string and SYNOPSIS by introducing
`struct option`.
- Patch 10/12: Further prepares for using the parse-option API to
actually parse command line options by using OPT_CALLBACK.
- Patch 11/12: Actually parses the command line options using the
parse-option API.
- Patch 12/12: Performs a cleanup of some functions arguments allowed
by the previous commit.
Changes since v2
================
Thanks to Elijah for reviewing v2 and Junio for reviewing v1.
The series has been rebased on top of recent master at 010afd3166 (The
12th batch, 2026-08-07).
- Patch 2/12 now describes `PARSE_OPT_LASTARG_DEFAULT` the way Elijah
suggested, which matches what "parse-options.h" says, and mentions
that it should not be combined with `PARSE_OPT_OPTARG`.
- Patch 7/12 now wraps the function signatures that grew too long
when they gained a 'struct fast_import_state *state' argument, with
one parameter per line. Patches 8/12 and 12/12 have been adjusted
accordingly, as they touch two of these signatures again.
- Patch 11/12 now also lists two other behavior changes that Elijah
spotted: value-taking options accept the space-separated
`--opt value` form, and a bare or trailing `--` is now accepted.
- Patch 11/12 also documents, both in a NEEDSWORK code comment and
in "Documentation/git-fast-import.adoc", the third behavior change
Elijah spotted about `--allow-unsafe-features`: as the early scan
for that option only matches its exact spelling and stops at the
first non-option argument, it disagrees with parse_options() for
command lines like `--allow-unsafe` or
`--depth 5 --allow-unsafe-features`. This errs on the safe side,
as unsafe `feature` commands from the stream are refused in that
case.
I plan to work on a follow-up series that will improve on this by
teaching the early scan about the options that take a value. It
looks like the parse-options API could provide some helpers for
this, which might benefit other commands like `git` itself (see
handle_options() in "git.c") that need to look at some options
before the actual option parsing.
CI tests
========
They all pass, see:
https://github.com/chriscool/git/actions/runs/31469672131
Christian Couder (12):
parse-options: introduce OPT_HIDDEN_GROUP
api-parse-options.adoc: document per-option flags
api-parse-options.adoc: document hidden and OPT_*_F option macros
fast-import: localize 'i' into the 'for' loops using it
fast-import: use int for some bool flags
fast-import: factor out option_*() functions
fast-import: introduce 'struct fast_import_state'
fast-import: move command state globals into 'struct
fast_import_state'
fast-import: use struct option for usage string
fast-import: use callbacks to parse some options
fast-import: use parse_options() for command line options
fast-import: remove useless from_stream argument
Documentation/git-fast-import.adoc | 9 +-
.../technical/api-parse-options.adoc | 80 +++
builtin/fast-import.c | 617 ++++++++++++------
parse-options.c | 4 +-
parse-options.h | 5 +
t/helper/test-parse-options.c | 4 +
t/t0040-parse-options.sh | 25 +-
t/t0450/adoc-help-mismatches | 1 -
t/t9300-fast-import.sh | 7 +
9 files changed, 564 insertions(+), 188 deletions(-)
Range-diff against v2:
1: b09d727e71 = 1: 06e37fe78b parse-options: introduce OPT_HIDDEN_GROUP
2: fe21471420 ! 2: 184837437c api-parse-options.adoc: document per-option flags
@@ Documentation/technical/api-parse-options.adoc: Data Structure
+ deprecated, advanced or otherwise uncommon options.
+
+`PARSE_OPT_LASTARG_DEFAULT`::
-+ Use the default value (`defval`) when the option is used
-+ without an argument, even for an option that normally requires
-+ one. Only the last argument on the command line takes effect.
++ The no-argument form is only accepted when the option is the
++ last token on the command line; used earlier, it still
++ requires an argument. Should not be combined with
++ `PARSE_OPT_OPTARG`.
+
+`PARSE_OPT_NODASH`::
+ The option is a single character without a leading dash, such
3: f43d4cefa4 = 3: 3e3655c6d8 api-parse-options.adoc: document hidden and OPT_*_F option macros
4: 3eb8106ed6 = 4: bb0dc9b12e fast-import: localize 'i' into the 'for' loops using it
5: 34514eb2ac = 5: e653d2a43f fast-import: use int for some bool flags
6: 336395c70a = 6: 2f809b3a01 fast-import: factor out option_*() functions
7: 45cc2107e2 ! 7: 3c913f0034 fast-import: introduce 'struct fast_import_state'
@@ builtin/fast-import.c: static kh_oid_map_t *sub_oid_map;
+static void parse_argv(struct fast_import_state *state);
+static void parse_get_mark(struct fast_import_state *state, const char *p);
+static void parse_cat_blob(struct fast_import_state *state, const char *p);
-+static void parse_ls(struct fast_import_state *state, const char *p, struct branch *b);
++static void parse_ls(struct fast_import_state *state,
++ const char *p,
++ struct branch *b);
static void for_each_mark(struct mark_set *m, uintmax_t base, each_mark_fn_t callback, void *p)
{
@@ builtin/fast-import.c: static void parse_path_space(struct strbuf *sb, const cha
}
-static void file_change_m(const char *p, struct branch *b)
-+static void file_change_m(struct fast_import_state *state, const char *p, struct branch *b)
++static void file_change_m(struct fast_import_state *state,
++ const char *p,
++ struct branch *b)
{
static struct strbuf path = STRBUF_INIT;
struct object_entry *oe;
@@ builtin/fast-import.c: static void file_change_cr(const char *p, struct branch *
}
-static void note_change_n(const char *p, struct branch *b, unsigned char *old_fanout)
-+static void note_change_n(struct fast_import_state *state, const char *p, struct branch *b, unsigned char *old_fanout)
++static void note_change_n(struct fast_import_state *state,
++ const char *p,
++ struct branch *b,
++ unsigned char *old_fanout)
{
struct object_entry *oe;
struct branch *s;
@@ builtin/fast-import.c: static void parse_from_existing(struct branch *b)
}
-static int parse_objectish(struct branch *b, const char *objectish)
-+static int parse_objectish(struct fast_import_state *state, struct branch *b, const char *objectish)
++static int parse_objectish(struct fast_import_state *state,
++ struct branch *b,
++ const char *objectish)
{
struct branch *s;
struct object_id oid;
@@ builtin/fast-import.c: static int parse_objectish(struct branch *b, const char *
}
-static int parse_objectish_with_prefix(struct branch *b, const char *prefix)
-+static int parse_objectish_with_prefix(struct fast_import_state *state, struct branch *b, const char *prefix)
++static int parse_objectish_with_prefix(struct fast_import_state *state,
++ struct branch *b,
++ const char *prefix)
{
const char *base;
@@ builtin/fast-import.c: static int parse_objectish(struct branch *b, const char *
}
-static struct hash_list *parse_merge(unsigned int *count)
-+static struct hash_list *parse_merge(struct fast_import_state *state, unsigned int *count)
++static struct hash_list *parse_merge(struct fast_import_state *state,
++ unsigned int *count)
{
struct hash_list *list = NULL, **tail = &list, *n;
const char *from;
@@ builtin/fast-import.c: struct signature_data {
};
-static void parse_one_signature(struct signature_data *sig, const char *v)
-+static void parse_one_signature(struct fast_import_state *state, struct signature_data *sig, const char *v)
++static void parse_one_signature(struct fast_import_state *state,
++ struct signature_data *sig,
++ const char *v)
{
char *args = xstrdup(v); /* Will be freed when sig->hash_algo is freed */
char *space = strchr(args, ' ');
@@ builtin/fast-import.c: static void cat_blob(struct object_entry *oe, struct obje
}
-static void parse_get_mark(const char *p)
-+static void parse_get_mark(struct fast_import_state *state UNUSED, const char *p)
++static void parse_get_mark(struct fast_import_state *state UNUSED,
++ const char *p)
{
struct object_entry *oe;
char output[GIT_MAX_HEXSZ + 2];
@@ builtin/fast-import.c: static void parse_get_mark(const char *p)
}
-static void parse_cat_blob(const char *p)
-+static void parse_cat_blob(struct fast_import_state *state UNUSED, const char *p)
++static void parse_cat_blob(struct fast_import_state *state UNUSED,
++ const char *p)
{
struct object_entry *oe;
struct object_id oid;
@@ builtin/fast-import.c: static void print_ls(int mode, const unsigned char *hash,
}
-static void parse_ls(const char *p, struct branch *b)
-+static void parse_ls(struct fast_import_state *state UNUSED, const char *p, struct branch *b)
++static void parse_ls(struct fast_import_state *state UNUSED,
++ const char *p,
++ struct branch *b)
{
static struct strbuf path = STRBUF_INIT;
struct tree_entry *root = NULL;
@@ builtin/fast-import.c: static void parse_progress(void)
}
-static char* make_fast_import_path(const char *path)
-+static char* make_fast_import_path(struct fast_import_state *state, const char *path)
++static char* make_fast_import_path(struct fast_import_state *state,
++ const char *path)
{
if (!relative_marks_paths || is_absolute_path(path))
- return prefix_filename(global_prefix, path);
@@ builtin/fast-import.c: static void parse_progress(void)
}
-static void option_import_marks(const char *marks,
-+static void option_import_marks(struct fast_import_state *state, const char *marks,
- int from_stream, int ignore_missing)
+- int from_stream, int ignore_missing)
++static void option_import_marks(struct fast_import_state *state,
++ const char *marks,
++ int from_stream,
++ int ignore_missing)
{
if (import_marks_file) {
+ if (from_stream)
@@ builtin/fast-import.c: static void option_import_marks(const char *marks,
}
@@ builtin/fast-import.c: static void option_active_branches(const char *branches)
}
-static void option_export_marks(const char *marks)
-+static void option_export_marks(struct fast_import_state *state, const char *marks)
++static void option_export_marks(struct fast_import_state *state,
++ const char *marks)
{
free(export_marks_file);
- export_marks_file = make_fast_import_path(marks);
@@ builtin/fast-import.c: static void option_active_branches(const char *branches)
}
-static void option_cat_blob_fd(const char *fd)
-+static void option_cat_blob_fd(struct fast_import_state *state UNUSED, const char *fd)
++static void option_cat_blob_fd(struct fast_import_state *state UNUSED,
++ const char *fd)
{
unsigned long n = ulong_arg("--cat-blob-fd", fd);
if (n > (unsigned long) INT_MAX)
@@ builtin/fast-import.c: static void option_cat_blob_fd(const char *fd)
}
-static void option_export_pack_edges(const char *edges)
-+static void option_export_pack_edges(struct fast_import_state *state, const char *edges)
++static void option_export_pack_edges(struct fast_import_state *state,
++ const char *edges)
{
- char *fn = prefix_filename(global_prefix, edges);
+ char *fn = prefix_filename(state->prefix, edges);
@@ builtin/fast-import.c: static void option_cat_blob_fd(const char *fd)
}
-static void option_rewrite_submodules(const char *arg, struct string_list *list)
-+static void option_rewrite_submodules(struct fast_import_state *state, const char *arg, struct string_list *list)
++static void option_rewrite_submodules(struct fast_import_state *state,
++ const char *arg,
++ struct string_list *list)
{
struct mark_set *ms;
FILE *fp;
@@ builtin/fast-import.c: static int parse_one_option(const char *option)
}
-static void check_unsafe_feature(const char *feature, int from_stream)
-+static void check_unsafe_feature(struct fast_import_state *state UNUSED, const char *feature, int from_stream)
++static void check_unsafe_feature(struct fast_import_state *state UNUSED,
++ const char *feature,
++ int from_stream)
{
if (from_stream && !allow_unsafe_features)
die(_("feature '%s' forbidden in input without --allow-unsafe-features"),
@@ builtin/fast-import.c: static int parse_one_option(const char *option)
}
-static int parse_one_feature(const char *feature, int from_stream)
-+static int parse_one_feature(struct fast_import_state *state, const char *feature, int from_stream)
++static int parse_one_feature(struct fast_import_state *state,
++ const char *feature,
++ int from_stream)
{
const char *arg;
8: 7f068facc2 ! 8: 5d0b07148e fast-import: move command state globals into 'struct fast_import_state'
@@ builtin/fast-import.c: static int parse_one_option(struct fast_import_state *sta
return 1;
}
--static void check_unsafe_feature(struct fast_import_state *state UNUSED, const char *feature, int from_stream)
-+static void check_unsafe_feature(struct fast_import_state *state, const char *feature, int from_stream)
+-static void check_unsafe_feature(struct fast_import_state *state UNUSED,
++static void check_unsafe_feature(struct fast_import_state *state,
+ const char *feature,
+ int from_stream)
{
- if (from_stream && !allow_unsafe_features)
+ if (from_stream && !state->allow_unsafe_features)
die(_("feature '%s' forbidden in input without --allow-unsafe-features"),
feature);
}
-@@ builtin/fast-import.c: static int parse_one_feature(struct fast_import_state *state, const char *featur
+@@ builtin/fast-import.c: static int parse_one_feature(struct fast_import_state *state,
static void parse_feature(struct fast_import_state *state, const char *feature)
{
9: 3d6ab86518 = 9: efcd1b9ac4 fast-import: use struct option for usage string
10: 3208937f13 = 10: d109b8c622 fast-import: use callbacks to parse some options
11: 202a50beec ! 11: 95919e8319 fast-import: use parse_options() for command line options
@@ Commit message
requires a value" unlike the other four options that are not
accepted anymore on the command line (see above).
+ - Value-taking options now also accept the space-separated
+ `--opt value` form, like `--depth 5`, in addition to the
+ `--opt=value` form.
+
+ - A bare or trailing `--` is now accepted and the stream is read
+ normally, while it used to be a usage error.
+
- The error messages for some options might differ a bit.
- The code is shorter and more standard.
@@ Commit message
Signed-off-by: Christian Couder <[email protected]>
+ ## Documentation/git-fast-import.adoc ##
+@@ Documentation/git-fast-import.adoc: Only enable this option if you trust the program generating the
+ fast-import stream! This option is enabled automatically for
+ remote-helpers that use the `import` capability, as they are
+ already trusted to run their own code.
+++
++Note that this option has to be spelled in full, and has to appear
++before any option whose value is separated from it by a space, for
++the unsafe `feature` commands in the stream to be allowed. So
++`--allow-unsafe` or `--depth 5 --allow-unsafe-features` still refuse
++them, while `--allow-unsafe-features --depth 5` and
++`--depth=5 --allow-unsafe-features` allow them.
+
+ `--signed-tags=<mode>`::
+ Specify how to handle signed tags. Behaves in the same way as
+
## builtin/fast-import.c ##
@@ builtin/fast-import.c: static const char *const fast_import_usage[] = {
@@ builtin/fast-import.c: static const char *const fast_import_usage[] = {
-
- if (*a != '-' || !strcmp(a, "--"))
- break;
-+ int argc = parse_options(state->argc, state->argv, state->prefix,
-+ state->option, fast_import_usage,
-+ PARSE_OPT_KEEP_ARGV0);
-
+-
- if (!skip_prefix(a, "--", &a))
- die(_("unknown option %s"), a);
-
@@ builtin/fast-import.c: static const char *const fast_import_usage[] = {
- option_cat_blob_fd(state, a);
- continue;
- }
--
++ int argc = parse_options(state->argc, state->argv, state->prefix,
++ state->option, fast_import_usage,
++ PARSE_OPT_KEEP_ARGV0);
+
- die(_("unknown option --%s"), a);
- }
- if (i != state->argc)
@@ builtin/fast-import.c: int cmd_fast_import(int argc,
struct option fast_import_options[] = {
OPT_GROUP(N_("Common")),
OPT_CALLBACK_F(0, "date-format", NULL, N_("fmt"),
+@@ builtin/fast-import.c: int cmd_fast_import(int argc,
+ * "feature" lines at the start of the stream (which allows the command
+ * line to override stream data). But we must do an early parse of any
+ * command-line options that impact how we interpret the feature lines.
++ *
++ * NEEDSWORK: This scan only matches the exact "--allow-unsafe-features"
++ * spelling and stops at the first argument that doesn't start with a
++ * dash. As parse_options() below also accepts unambiguous abbreviations
++ * and values separated by a space from their option, the two disagree
++ * for command lines like "--allow-unsafe" or "--depth 5
++ * --allow-unsafe-features": parse_options() accepts the option, but
++ * this scan doesn't see it, so unsafe features from the stream are
++ * still refused. This errs on the safe side, but should be fixed by
++ * teaching this scan about the options that take a value.
+ */
+ for (int i = 1; i < argc; i++) {
+ const char *arg = argv[i];
## t/t9300-fast-import.sh ##
@@ t/t9300-fast-import.sh: test_expect_success 'R: unknown commandline options are rejected' '\
12: 99ff791a62 ! 12: 96f17c83d2 fast-import: remove useless from_stream argument
@@ Commit message
## builtin/fast-import.c ##
@@ builtin/fast-import.c: static int parse_one_option(struct fast_import_state *state, const char *option)
- return 1;
}
--static void check_unsafe_feature(struct fast_import_state *state, const char *feature, int from_stream)
-+static void check_unsafe_feature(struct fast_import_state *state, const char *feature)
+ static void check_unsafe_feature(struct fast_import_state *state,
+- const char *feature,
+- int from_stream)
++ const char *feature)
{
- if (from_stream && !state->allow_unsafe_features)
+ if (!state->allow_unsafe_features)
@@ builtin/fast-import.c: static int parse_one_option(struct fast_import_state *sta
feature);
}
--static int parse_one_feature(struct fast_import_state *state, const char *feature, int from_stream)
-+static int parse_one_feature(struct fast_import_state *state, const char *feature)
+ static int parse_one_feature(struct fast_import_state *state,
+- const char *feature,
+- int from_stream)
++ const char *feature)
{
const char *arg;
--
2.55.0.530.gdb3615d990.dirty