* Re: [PATCH 3/3] bisect: add --auto-reset to leave when done
From: Junio C Hamano @ 2026-07-16 17:22 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Harald Nordgren
In-Reply-To: <a9194b1d00b260a7a7852eccec54c872618b5fdf.1784180159.git.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> When a bisection finished, "git bisect" reported the first bad commit
> but left the session active until "git bisect reset" was run by hand.
If this gives an observation of the behavior of the current code,
please write it in the present tense.
> Add an "--auto-reset" option, accepted by both "git bisect start" and
> "git bisect run", that resets as soon as the first bad commit is found,
> returning to the commit checked out before "git bisect start". The flag
> is persisted in a BISECT_AUTO_RESET state file and the restoring
> checkout is done quietly.
I often find myself, after the culprit is found, running 'git
reset --hard' or 'git bisect reset' to jump to the problematic
commit to investigate further. If '--auto-reset' leaves me
checked out on that bad commit, that would be a very welcome
change. If it only returns me to where I started before the
bisection, well, 'Meh'.
^ permalink raw reply
* [PATCH 7/7] fast-import: use struct option for usage string
From: Christian Couder @ 2026-07-16 16:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Elijah Newren, Jeff King,
brian m . carlson, Johannes Schindelin, Justin Tobler,
Christian Couder, Christian Couder
In-Reply-To: <20260716165517.433849-1-christian.couder@gmail.com>
Currently `git fast-import -h` shows the following on a single line:
usage : git fast-import [--date-format=<f>] [--max-pack-size=<n>] \
[--big-file-threshold=<n>] [--depth=<n>] \
[--active-branches=<n>] \
[--export-marks=<marks.file>]
This output has a number of issues like:
- It's missing a lot of options.
- It's not consistent with the SYNOPSIS section of the doc.
- With `--help-all` instead of `-h` additional hidden options should
be shown, but that's not the case.
- It's not standard style anymore.
- Most other Git commands show additional lines for most of the
options they support.
Also while most commands use the parse-options API to handle their
options, "builtin/fast-import.c" still doesn't use it.
Let's improve on that by using the parse-options API to display the
options when `-h` and `--help-all` are used.
While at it, let's make the SYNOPSIS section of
"Documentation/git-fast-import.adoc" consistent with the new usage
string.
This deliberately leaves it to future work to also use the
parse-options API to actually parse the options.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-fast-import.adoc | 2 +-
builtin/fast-import.c | 86 +++++++++++++++++++++++++++---
t/t0450/adoc-help-mismatches | 1 -
3 files changed, 81 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc
index d68bc52b7e..7c5900e048 100644
--- a/Documentation/git-fast-import.adoc
+++ b/Documentation/git-fast-import.adoc
@@ -9,7 +9,7 @@ git-fast-import - Backend for fast Git data importers
SYNOPSIS
--------
[verse]
-frontend | 'git fast-import' [<options>]
+'git fast-import' [<options>]
DESCRIPTION
-----------
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 53f5d39173..a2952b273f 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -30,6 +30,7 @@
#include "khash.h"
#include "date.h"
#include "gpg-interface.h"
+#include "parse-options.h"
#define PACK_ID_BITS 16
#define MAX_PACK_ID ((1<<PACK_ID_BITS)-1)
@@ -277,16 +278,18 @@ struct fast_import_state {
const char *prefix;
int seen_data_command;
int allow_unsafe_features;
+ struct option *option;
};
static void fast_import_state_init(struct fast_import_state *state,
int argc, const char **argv,
- const char *prefix)
+ const char *prefix, struct option *option)
{
memset(state, 0, sizeof(*state));
state->argc = argc;
state->argv = argv;
state->prefix = prefix;
+ state->option = option;
}
static void parse_argv(struct fast_import_state *state);
@@ -3907,8 +3910,10 @@ static void git_pack_config(void)
repo_config(the_repository, git_default_config, NULL);
}
-static const char fast_import_usage[] =
-"git fast-import [--date-format=<f>] [--max-pack-size=<n>] [--big-file-threshold=<n>] [--depth=<n>] [--active-branches=<n>] [--export-marks=<marks.file>]";
+static const char *const fast_import_usage[] = {
+ N_("git fast-import [<options>]"),
+ NULL
+};
static void parse_argv(struct fast_import_state *state)
{
@@ -3937,7 +3942,7 @@ static void parse_argv(struct fast_import_state *state)
die(_("unknown option --%s"), a);
}
if (i != state->argc)
- usage(fast_import_usage);
+ usage_with_options(fast_import_usage, state->option);
state->seen_data_command = 1;
if (import_marks_file)
@@ -3952,9 +3957,78 @@ int cmd_fast_import(int argc,
{
struct fast_import_state state;
- show_usage_if_asked(argc, argv, fast_import_usage);
+ unsigned long pack_size_limit, big_file_threshold, depth, active_branches;
+ char *edges, *signed_commits, *signed_tags, *date_format, *import_marks;
+ char *import_marks_if_exists, *export_marks, *submodules_from, *submodules_to;
+ int opt_quiet, opt_show_stats, opt_relative_marks, opt_force, opt_done;
+ int opt_allow_unsafe;
+ int cat_blob;
- fast_import_state_init(&state, argc, argv, prefix);
+ /*
+ * NEEDSWORK: For now this is used only to render
+ * `-h`/`--help-all` usage messages. The actual parsing is
+ * done by parse_one_option()/parse_one_feature().
+ */
+ struct option fast_import_options[] = {
+ OPT_GROUP(N_("Common")),
+ OPT_STRING_F(0, "date-format", &date_format, N_("fmt"),
+ N_("format of the commit/tag dates"), PARSE_OPT_NONEG),
+ OPT_BOOL_F(0, "stats", &opt_show_stats,
+ N_("display some basic statistics (objects, packfiles and memory)"),
+ PARSE_OPT_NONEG),
+ OPT_BOOL_F(0, "quiet", &opt_quiet,
+ N_("disable the output shown by --stats"), PARSE_OPT_NONEG),
+ OPT_BOOL_F(0, "force", &opt_force,
+ N_("force updating modified existing branches"), PARSE_OPT_NONEG),
+ OPT_BOOL_F(0, "done", &opt_done,
+ N_("require a terminating 'done' command"), PARSE_OPT_NONEG),
+ OPT_UNSIGNED(0, "max-pack-size", &pack_size_limit,
+ N_("maximum size of each output pack file")),
+ OPT_UNSIGNED(0, "big-file-threshold", &big_file_threshold,
+ N_("maximum size of a blob that will be deltified")),
+ OPT_UNSIGNED(0, "depth", &depth,
+ N_("maximum delta depth")),
+ OPT_UNSIGNED(0, "active-branches", &active_branches,
+ N_("maximum number of branches to maintain active")),
+ OPT_GROUP(N_("Marks")),
+ OPT_STRING_F(0, "import-marks", &import_marks, N_("file"),
+ N_("import marks from <file>"), PARSE_OPT_NONEG),
+ OPT_STRING_F(0, "import-marks-if-exists", &import_marks_if_exists, N_("file"),
+ N_("import marks from <file> if it exists"), PARSE_OPT_NONEG),
+ OPT_STRING_F(0, "export-marks", &export_marks, N_("file"),
+ N_("dump marks to <file>"), PARSE_OPT_NONEG),
+ OPT_BOOL(0, "relative-marks", &opt_relative_marks,
+ N_("are --(import|export)-marks= paths relative to '.git/info/fast-import'?")),
+ OPT_GROUP(N_("Submodule rewrite")),
+ OPT_STRING_F(0, "rewrite-submodules-from", &submodules_from, N_("name:filename"),
+ N_("rewrite object IDs for submodule <name> from <filename>"),
+ PARSE_OPT_NONEG),
+ OPT_STRING_F(0, "rewrite-submodules-to", &submodules_to, N_("name:filename"),
+ N_("rewrite object IDs for submodule <name> to <filename>"),
+ PARSE_OPT_NONEG),
+ OPT_GROUP(N_("Signing")),
+ OPT_STRING_F(0, "signed-commits", &signed_commits, N_("mode"),
+ N_("how to handle signed commits"),
+ PARSE_OPT_NONEG),
+ OPT_STRING_F(0, "signed-tags", &signed_tags, N_("mode"),
+ N_("how to handle signed tags"),
+ PARSE_OPT_NONEG),
+ OPT_HIDDEN_GROUP(N_("Advanced")),
+ OPT_BOOL_F(0, "allow-unsafe-features", &opt_allow_unsafe,
+ N_("allow unsafe mark commands from the stream"),
+ PARSE_OPT_HIDDEN | PARSE_OPT_NONEG),
+ OPT_STRING_F(0, "export-pack-edges", &edges, N_("file"),
+ N_("dump edge commits to <file>"),
+ PARSE_OPT_HIDDEN | PARSE_OPT_NONEG),
+ OPT_INTEGER_F(0, "cat-blob-fd", &cat_blob,
+ N_("write some responses to <fd> instead of stdout"),
+ PARSE_OPT_HIDDEN | PARSE_OPT_NONEG),
+ OPT_END()
+ };
+
+ show_usage_with_options_if_asked(argc, argv, fast_import_usage, fast_import_options);
+
+ fast_import_state_init(&state, argc, argv, prefix, fast_import_options);
reset_pack_idx_option(&pack_idx_opts);
git_pack_config();
diff --git a/t/t0450/adoc-help-mismatches b/t/t0450/adoc-help-mismatches
index e8d6c13ccd..85b039a4be 100644
--- a/t/t0450/adoc-help-mismatches
+++ b/t/t0450/adoc-help-mismatches
@@ -13,7 +13,6 @@ credential
credential-cache
credential-store
fast-export
-fast-import
fetch-pack
fmt-merge-msg
format-patch
--
2.55.0.185.g9120d2b5c0
^ permalink raw reply related
* [PATCH 6/7] fast-import: move command state globals into 'struct fast_import_state'
From: Christian Couder @ 2026-07-16 16:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Elijah Newren, Jeff King,
brian m . carlson, Johannes Schindelin, Justin Tobler,
Christian Couder, Christian Couder
In-Reply-To: <20260716165517.433849-1-christian.couder@gmail.com>
A previous commit introduced 'struct fast_import_state' to hold some
command state, and reduce the need for global variables.
Let's continue in the same direction and move two more global variables
that describe the command state into it: 'seen_data_command' and
'allow_unsafe_features'.
All the sites accessing these variables are already in functions that
receive the 'state' parameter (or in cmd_fast_import() which owns the
struct), so no additional threading is needed.
As 'state->allow_unsafe_features' is now dereferenced in
check_unsafe_feature(), its 'state' parameter is no longer unused, so
the UNUSED marker is removed.
The fast_import_state_init() call is moved up before the early
command-line scan for '--allow-unsafe-features', so that this option
can be recorded directly into the struct without being clobbered by
the memset() in fast_import_state_init().
This is a mechanical refactoring with no intended behavior change.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin/fast-import.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index d20353679a..53f5d39173 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -257,9 +257,7 @@ static struct recent_command *rc_free;
static unsigned int cmd_save = 100;
static uintmax_t next_mark;
static struct strbuf new_data = STRBUF_INIT;
-static int seen_data_command;
static int require_explicit_termination;
-static int allow_unsafe_features;
/* Signal handling */
static volatile sig_atomic_t checkpoint_requested;
@@ -277,6 +275,8 @@ struct fast_import_state {
int argc;
const char **argv;
const char *prefix;
+ int seen_data_command;
+ int allow_unsafe_features;
};
static void fast_import_state_init(struct fast_import_state *state,
@@ -1876,7 +1876,7 @@ static int read_next_command(struct fast_import_state *state)
if (stdin_eof)
return EOF;
- if (!seen_data_command
+ if (!state->seen_data_command
&& !starts_with(command_buf.buf, "feature ")
&& !starts_with(command_buf.buf, "option ")) {
parse_argv(state);
@@ -3809,9 +3809,9 @@ 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 UNUSED, const char *feature, int from_stream)
+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);
}
@@ -3860,7 +3860,7 @@ static int parse_one_feature(struct fast_import_state *state, const char *featur
static void parse_feature(struct fast_import_state *state, const char *feature)
{
- if (seen_data_command)
+ if (state->seen_data_command)
die(_("got feature command '%s' after data command"), feature);
if (parse_one_feature(state, feature, 1))
@@ -3871,7 +3871,7 @@ static void parse_feature(struct fast_import_state *state, const char *feature)
static void parse_option(struct fast_import_state *state, const char *option)
{
- if (seen_data_command)
+ if (state->seen_data_command)
die(_("got option command '%s' after data command"), option);
if (parse_one_option(state, option))
@@ -3939,7 +3939,7 @@ static void parse_argv(struct fast_import_state *state)
if (i != state->argc)
usage(fast_import_usage);
- seen_data_command = 1;
+ state->seen_data_command = 1;
if (import_marks_file)
read_marks();
build_mark_map(&sub_marks_from, &sub_marks_to);
@@ -3954,6 +3954,8 @@ int cmd_fast_import(int argc,
show_usage_if_asked(argc, argv, fast_import_usage);
+ fast_import_state_init(&state, argc, argv, prefix);
+
reset_pack_idx_option(&pack_idx_opts);
git_pack_config();
@@ -3977,11 +3979,9 @@ int cmd_fast_import(int argc,
if (*arg != '-' || !strcmp(arg, "--"))
break;
if (!strcmp(arg, "--allow-unsafe-features"))
- allow_unsafe_features = 1;
+ state.allow_unsafe_features = 1;
}
- fast_import_state_init(&state, argc, argv, prefix);
-
rc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free));
for (unsigned int i = 0; i < (cmd_save - 1); i++)
rc_free[i].next = &rc_free[i + 1];
@@ -4028,7 +4028,7 @@ int cmd_fast_import(int argc,
}
/* argv hasn't been parsed yet, do so */
- if (!seen_data_command)
+ if (!state.seen_data_command)
parse_argv(&state);
if (require_explicit_termination && feof(stdin))
--
2.55.0.185.g9120d2b5c0
^ permalink raw reply related
* [PATCH 5/7] fast-import: introduce 'struct fast_import_state'
From: Christian Couder @ 2026-07-16 16:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Elijah Newren, Jeff King,
brian m . carlson, Johannes Schindelin, Justin Tobler,
Christian Couder, Christian Couder
In-Reply-To: <20260716165517.433849-1-christian.couder@gmail.com>
"builtin/fast-import.c" uses a large number of global variables. This
makes it harder than necessary to reason about and improve. Especially
adding new features requires adding more global variables, while
modernizing and eventually libifying the code becomes more and more
difficult.
To start reverting the sad trend to more and more globals and to start
cleaning things up, let's introduce a 'struct fast_import_state' and
pass an instance of it as the first argument to many functions.
This is similar to what was done for "builtin/apply.c" by introducing a
'struct apply_state', see 07d7e290ff (apply: move 'struct apply_state'
to a header file, 2016-08-11) and related commits.
As a first step only the 'global_argc', 'global_argv' and
'global_prefix' variables are moved into the new struct. More variables
will be moved into it in the following commits.
Some functions receive the new 'state' parameter only to pass it
along or for future use, so they are marked with UNUSED for now to
satisfy '-Werror=unused-parameter'.
This is a mostly mechanical refactoring with no intended behavior
change.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin/fast-import.c | 262 ++++++++++++++++++++++--------------------
1 file changed, 138 insertions(+), 124 deletions(-)
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index fd4e13b7ca..d20353679a 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -184,10 +184,6 @@ static int failure;
static FILE *pack_edges;
static unsigned int show_stats = 1;
static unsigned int quiet;
-static int global_argc;
-static const char **global_argv;
-static const char *global_prefix;
-
static enum sign_mode signed_tag_mode = SIGN_VERBATIM;
static enum sign_mode signed_commit_mode = SIGN_VERBATIM;
static const char *signed_commit_keyid;
@@ -276,10 +272,27 @@ static kh_oid_map_t *sub_oid_map;
/* Where to write output of cat-blob commands */
static int cat_blob_fd = STDOUT_FILENO;
-static void parse_argv(void);
-static void parse_get_mark(const char *p);
-static void parse_cat_blob(const char *p);
-static void parse_ls(const char *p, struct branch *b);
+/* Command state */
+struct fast_import_state {
+ int argc;
+ const char **argv;
+ const char *prefix;
+};
+
+static void fast_import_state_init(struct fast_import_state *state,
+ int argc, const char **argv,
+ const char *prefix)
+{
+ memset(state, 0, sizeof(*state));
+ state->argc = argc;
+ state->argv = argv;
+ state->prefix = prefix;
+}
+
+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 for_each_mark(struct mark_set *m, uintmax_t base, each_mark_fn_t callback, void *p)
{
@@ -1844,7 +1857,7 @@ static void read_marks(void)
}
-static int read_next_command(void)
+static int read_next_command(struct fast_import_state *state)
{
static int stdin_eof = 0;
@@ -1866,7 +1879,7 @@ static int read_next_command(void)
if (!seen_data_command
&& !starts_with(command_buf.buf, "feature ")
&& !starts_with(command_buf.buf, "option ")) {
- parse_argv();
+ parse_argv(state);
}
rc = rc_free;
@@ -1898,22 +1911,22 @@ static void skip_optional_lf(void)
ungetc(term_char, stdin);
}
-static void parse_mark(void)
+static void parse_mark(struct fast_import_state *state)
{
const char *v;
if (skip_prefix(command_buf.buf, "mark :", &v)) {
next_mark = strtoumax(v, NULL, 10);
- read_next_command();
+ read_next_command(state);
}
else
next_mark = 0;
}
-static void parse_original_identifier(void)
+static void parse_original_identifier(struct fast_import_state *state)
{
const char *v;
if (skip_prefix(command_buf.buf, "original-oid ", &v))
- read_next_command();
+ read_next_command(state);
}
static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)
@@ -2067,11 +2080,11 @@ static void parse_and_store_blob(
}
}
-static void parse_new_blob(void)
+static void parse_new_blob(struct fast_import_state *state)
{
- read_next_command();
- parse_mark();
- parse_original_identifier();
+ read_next_command(state);
+ parse_mark(state);
+ parse_original_identifier(state);
parse_and_store_blob(&last_blob, NULL, next_mark);
}
@@ -2367,7 +2380,7 @@ static void parse_path_space(struct strbuf *sb, const char *p,
(*endp)++;
}
-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 struct strbuf path = STRBUF_INIT;
struct object_entry *oe;
@@ -2434,10 +2447,10 @@ static void file_change_m(const char *p, struct branch *b)
if (S_ISDIR(mode))
die(_("directories cannot be specified 'inline': %s"),
command_buf.buf);
- while (read_next_command() != EOF) {
+ while (read_next_command(state) != EOF) {
const char *v;
if (skip_prefix(command_buf.buf, "cat-blob ", &v))
- parse_cat_blob(v);
+ parse_cat_blob(state, v);
else {
parse_and_store_blob(&last_blob, &oid, 0);
break;
@@ -2511,7 +2524,7 @@ static void file_change_cr(const char *p, struct branch *b, int rename)
leaf.tree);
}
-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)
{
struct object_entry *oe;
struct branch *s;
@@ -2576,7 +2589,7 @@ static void note_change_n(const char *p, struct branch *b, unsigned char *old_fa
die(_("invalid ref name or SHA1 expression: %s"), p);
if (inline_data) {
- read_next_command();
+ read_next_command(state);
parse_and_store_blob(&last_blob, &oid, 0);
} else if (oe) {
if (oe->type != OBJ_BLOB)
@@ -2643,7 +2656,7 @@ 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)
{
struct branch *s;
struct object_id oid;
@@ -2686,31 +2699,31 @@ static int parse_objectish(struct branch *b, const char *objectish)
b->branch_tree.tree = NULL;
}
- read_next_command();
+ read_next_command(state);
return 1;
}
-static int parse_from(struct branch *b)
+static int parse_from(struct fast_import_state *state, struct branch *b)
{
const char *from;
if (!skip_prefix(command_buf.buf, "from ", &from))
return 0;
- return parse_objectish(b, from);
+ return parse_objectish(state, b, from);
}
-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)
{
const char *base;
if (!skip_prefix(command_buf.buf, prefix, &base))
return 0;
- return parse_objectish(b, base);
+ return parse_objectish(state, b, base);
}
-static struct hash_list *parse_merge(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;
@@ -2744,7 +2757,7 @@ static struct hash_list *parse_merge(unsigned int *count)
tail = &n->next;
(*count)++;
- read_next_command();
+ read_next_command(state);
}
return list;
}
@@ -2755,7 +2768,7 @@ struct signature_data {
struct strbuf data; /* The actual 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)
{
char *args = xstrdup(v); /* Will be freed when sig->hash_algo is freed */
char *space = strchr(args, ' ');
@@ -2780,15 +2793,15 @@ static void parse_one_signature(struct signature_data *sig, const char *v)
warning(_("'unknown' signature format in gpgsig"));
/* Read signature data */
- read_next_command();
+ read_next_command(state);
parse_data(&sig->data, 0, NULL);
}
-static void discard_one_signature(void)
+static void discard_one_signature(struct fast_import_state *state)
{
struct strbuf data = STRBUF_INIT;
- read_next_command();
+ read_next_command(state);
parse_data(&data, 0, NULL);
strbuf_release(&data);
}
@@ -2826,13 +2839,14 @@ static void store_signature(struct signature_data *stored_sig,
}
}
-static void import_one_signature(struct signature_data *sig_sha1,
+static void import_one_signature(struct fast_import_state *state,
+ struct signature_data *sig_sha1,
struct signature_data *sig_sha256,
const char *v)
{
struct signature_data sig = { NULL, NULL, STRBUF_INIT };
- parse_one_signature(&sig, v);
+ parse_one_signature(state, &sig, v);
if (!strcmp(sig.hash_algo, "sha1"))
store_signature(sig_sha1, &sig, "SHA-1");
@@ -2946,7 +2960,7 @@ static void handle_signature_if_invalid(struct strbuf *new_data,
strbuf_release(&tmp_buf);
}
-static void parse_new_commit(const char *arg)
+static void parse_new_commit(struct fast_import_state *state, const char *arg)
{
static struct strbuf msg = STRBUF_INIT;
struct signature_data sig_sha1 = { NULL, NULL, STRBUF_INIT };
@@ -2964,16 +2978,16 @@ static void parse_new_commit(const char *arg)
if (!b)
b = new_branch(arg);
- read_next_command();
- parse_mark();
- parse_original_identifier();
+ read_next_command(state);
+ parse_mark(state);
+ parse_original_identifier(state);
if (skip_prefix(command_buf.buf, "author ", &v)) {
author = parse_ident(v);
- read_next_command();
+ read_next_command(state);
}
if (skip_prefix(command_buf.buf, "committer ", &v)) {
committer = parse_ident(v);
- read_next_command();
+ read_next_command(state);
}
if (!committer)
die(_("expected committer but didn't get one"));
@@ -2989,7 +3003,7 @@ static void parse_new_commit(const char *arg)
warning(_("stripping a commit signature"));
/* fallthru */
case SIGN_STRIP:
- discard_one_signature();
+ discard_one_signature(state);
break;
/* Second, modes that parse the signature */
@@ -3000,24 +3014,24 @@ static void parse_new_commit(const char *arg)
case SIGN_STRIP_IF_INVALID:
case SIGN_SIGN_IF_INVALID:
case SIGN_ABORT_IF_INVALID:
- import_one_signature(&sig_sha1, &sig_sha256, v);
+ import_one_signature(state, &sig_sha1, &sig_sha256, v);
break;
/* Third, BUG */
default:
BUG("invalid signed_commit_mode value %d", signed_commit_mode);
}
- read_next_command();
+ read_next_command(state);
}
if (skip_prefix(command_buf.buf, "encoding ", &v)) {
encoding = xstrdup(v);
- read_next_command();
+ read_next_command(state);
}
parse_data(&msg, 0, NULL);
- read_next_command();
- parse_from(b);
- merge_list = parse_merge(&merge_count);
+ read_next_command(state);
+ parse_from(state, b);
+ merge_list = parse_merge(state, &merge_count);
/* ensure the branch is active/loaded */
if (!b->branch_tree.tree || !max_active_branches) {
@@ -3030,7 +3044,7 @@ static void parse_new_commit(const char *arg)
/* file_change* */
while (command_buf.len > 0) {
if (skip_prefix(command_buf.buf, "M ", &v))
- file_change_m(v, b);
+ file_change_m(state, v, b);
else if (skip_prefix(command_buf.buf, "D ", &v))
file_change_d(v, b);
else if (skip_prefix(command_buf.buf, "R ", &v))
@@ -3038,18 +3052,18 @@ static void parse_new_commit(const char *arg)
else if (skip_prefix(command_buf.buf, "C ", &v))
file_change_cr(v, b, 0);
else if (skip_prefix(command_buf.buf, "N ", &v))
- note_change_n(v, b, &prev_fanout);
+ note_change_n(state, v, b, &prev_fanout);
else if (!strcmp("deleteall", command_buf.buf))
file_change_deleteall(b);
else if (skip_prefix(command_buf.buf, "ls ", &v))
- parse_ls(v, b);
+ parse_ls(state, v, b);
else if (skip_prefix(command_buf.buf, "cat-blob ", &v))
- parse_cat_blob(v);
+ parse_cat_blob(state, v);
else {
unread_command_buf = 1;
break;
}
- if (read_next_command() == EOF)
+ if (read_next_command(state) == EOF)
break;
}
@@ -3187,7 +3201,7 @@ static void handle_tag_signature(struct strbuf *buf, struct strbuf *msg, const c
}
}
-static void parse_new_tag(const char *arg)
+static void parse_new_tag(struct fast_import_state *state, const char *arg)
{
static struct strbuf msg = STRBUF_INIT;
const char *from;
@@ -3206,8 +3220,8 @@ static void parse_new_tag(const char *arg)
else
first_tag = t;
last_tag = t;
- read_next_command();
- parse_mark();
+ read_next_command(state);
+ parse_mark(state);
/* from ... */
if (!skip_prefix(command_buf.buf, "from ", &from))
@@ -3235,15 +3249,15 @@ static void parse_new_tag(const char *arg)
type = oe->type;
} else
die(_("invalid ref name or SHA1 expression: %s"), from);
- read_next_command();
+ read_next_command(state);
/* original-oid ... */
- parse_original_identifier();
+ parse_original_identifier(state);
/* tagger ... */
if (skip_prefix(command_buf.buf, "tagger ", &v)) {
tagger = parse_ident(v);
- read_next_command();
+ read_next_command(state);
} else
tagger = NULL;
@@ -3274,7 +3288,7 @@ static void parse_new_tag(const char *arg)
t->pack_id = pack_id;
}
-static void parse_reset_branch(const char *arg)
+static void parse_reset_branch(struct fast_import_state *state, const char *arg)
{
struct branch *b;
const char *tag_name;
@@ -3291,8 +3305,8 @@ static void parse_reset_branch(const char *arg)
}
else
b = new_branch(arg);
- read_next_command();
- parse_from(b);
+ read_next_command(state);
+ parse_from(state, b);
if (b->delete && skip_prefix(b->name, "refs/tags/", &tag_name)) {
/*
* Elsewhere, we call dump_branches() before dump_tags(),
@@ -3377,7 +3391,7 @@ static void cat_blob(struct object_entry *oe, struct object_id *oid)
free(buf);
}
-static void parse_get_mark(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];
@@ -3394,7 +3408,7 @@ static void parse_get_mark(const char *p)
cat_blob_write(output, the_hash_algo->hexsz + 1);
}
-static void parse_cat_blob(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;
@@ -3559,7 +3573,7 @@ static void print_ls(int mode, const unsigned char *hash, const char *path)
cat_blob_write(line.buf, line.len);
}
-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 struct strbuf path = STRBUF_INIT;
struct tree_entry *root = NULL;
@@ -3606,13 +3620,13 @@ static void checkpoint(void)
dump_marks();
}
-static void parse_checkpoint(void)
+static void parse_checkpoint(struct fast_import_state *state UNUSED)
{
checkpoint_requested = 1;
skip_optional_lf();
}
-static void parse_progress(void)
+static void parse_progress(struct fast_import_state *state UNUSED)
{
fwrite(command_buf.buf, 1, command_buf.len, stdout);
fputc('\n', stdout);
@@ -3620,36 +3634,36 @@ static void parse_progress(void)
skip_optional_lf();
}
-static void parse_alias(void)
+static void parse_alias(struct fast_import_state *state)
{
struct object_entry *e;
struct branch b;
skip_optional_lf();
- read_next_command();
+ read_next_command(state);
/* mark ... */
- parse_mark();
+ parse_mark(state);
if (!next_mark)
die(_("expected 'mark' command, got %s"), command_buf.buf);
/* to ... */
memset(&b, 0, sizeof(b));
- if (!parse_objectish_with_prefix(&b, "to "))
+ if (!parse_objectish_with_prefix(state, &b, "to "))
die(_("expected 'to' command, got %s"), command_buf.buf);
e = find_object(&b.oid);
assert(e);
insert_mark(&marks, next_mark, e);
}
-static char* make_fast_import_path(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);
+ return prefix_filename(state->prefix, path);
return repo_git_path(the_repository, "info/fast-import/%s", path);
}
-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)
{
if (import_marks_file) {
@@ -3662,7 +3676,7 @@ static void option_import_marks(const char *marks,
}
free(import_marks_file);
- import_marks_file = make_fast_import_path(marks);
+ import_marks_file = make_fast_import_path(state, marks);
import_marks_file_from_stream = from_stream;
import_marks_file_ignore_missing = ignore_missing;
}
@@ -3702,13 +3716,13 @@ static void option_active_branches(const char *branches)
max_active_branches = ulong_arg("--active-branches", branches);
}
-static void option_export_marks(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);
+ export_marks_file = make_fast_import_path(state, marks);
}
-static void option_cat_blob_fd(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)
@@ -3716,16 +3730,16 @@ static void option_cat_blob_fd(const char *fd)
cat_blob_fd = (int) n;
}
-static void option_export_pack_edges(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);
if (pack_edges)
fclose(pack_edges);
pack_edges = xfopen(fn, "a");
free(fn);
}
-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)
{
struct mark_set *ms;
FILE *fp;
@@ -3737,7 +3751,7 @@ static void option_rewrite_submodules(const char *arg, struct string_list *list)
f++;
CALLOC_ARRAY(ms, 1);
- f = prefix_filename(global_prefix, f);
+ f = prefix_filename(state->prefix, f);
fp = fopen(f, "r");
if (!fp)
die_errno(_("cannot read '%s'"), f);
@@ -3750,7 +3764,7 @@ static void option_rewrite_submodules(const char *arg, struct string_list *list)
free(s);
}
-static int parse_one_option(const char *option)
+static int parse_one_option(struct fast_import_state *state, const char *option)
{
if (skip_prefix(option, "max-pack-size=", &option)) {
unsigned long v;
@@ -3774,7 +3788,7 @@ static int parse_one_option(const char *option)
} else if (skip_prefix(option, "active-branches=", &option)) {
option_active_branches(option);
} else if (skip_prefix(option, "export-pack-edges=", &option)) {
- option_export_pack_edges(option);
+ option_export_pack_edges(state, option);
} else if (skip_prefix(option, "signed-commits=", &option)) {
if (parse_sign_mode(option, &signed_commit_mode, &signed_commit_keyid))
usagef(_("unknown --signed-commits mode '%s'"), option);
@@ -3795,34 +3809,34 @@ static int parse_one_option(const char *option)
return 1;
}
-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)
{
if (from_stream && !allow_unsafe_features)
die(_("feature '%s' forbidden in input without --allow-unsafe-features"),
feature);
}
-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)
{
const char *arg;
if (skip_prefix(feature, "date-format=", &arg)) {
option_date_format(arg);
} else if (skip_prefix(feature, "import-marks=", &arg)) {
- check_unsafe_feature("import-marks", from_stream);
- option_import_marks(arg, from_stream, 0);
+ check_unsafe_feature(state, "import-marks", from_stream);
+ option_import_marks(state, arg, from_stream, 0);
} else if (skip_prefix(feature, "import-marks-if-exists=", &arg)) {
- check_unsafe_feature("import-marks-if-exists", from_stream);
- option_import_marks(arg, from_stream, 1);
+ check_unsafe_feature(state, "import-marks-if-exists", from_stream);
+ option_import_marks(state, arg, from_stream, 1);
} else if (skip_prefix(feature, "export-marks=", &arg)) {
- check_unsafe_feature(feature, from_stream);
- option_export_marks(arg);
+ check_unsafe_feature(state, feature, from_stream);
+ option_export_marks(state, arg);
} else if (!strcmp(feature, "alias")) {
; /* Don't die - this feature is supported */
} else if (skip_prefix(feature, "rewrite-submodules-to=", &arg)) {
- option_rewrite_submodules(arg, &sub_marks_to);
+ option_rewrite_submodules(state, arg, &sub_marks_to);
} else if (skip_prefix(feature, "rewrite-submodules-from=", &arg)) {
- option_rewrite_submodules(arg, &sub_marks_from);
+ option_rewrite_submodules(state, arg, &sub_marks_from);
} else if (!strcmp(feature, "get-mark")) {
; /* Don't die - this feature is supported */
} else if (!strcmp(feature, "cat-blob")) {
@@ -3844,23 +3858,23 @@ static int parse_one_feature(const char *feature, int from_stream)
return 1;
}
-static void parse_feature(const char *feature)
+static void parse_feature(struct fast_import_state *state, const char *feature)
{
if (seen_data_command)
die(_("got feature command '%s' after data command"), feature);
- if (parse_one_feature(feature, 1))
+ if (parse_one_feature(state, feature, 1))
return;
die(_("this version of fast-import does not support feature %s."), feature);
}
-static void parse_option(const char *option)
+static void parse_option(struct fast_import_state *state, const char *option)
{
if (seen_data_command)
die(_("got option command '%s' after data command"), option);
- if (parse_one_option(option))
+ if (parse_one_option(state, option))
return;
die(_("this version of fast-import does not support option: %s"), option);
@@ -3896,12 +3910,12 @@ static void git_pack_config(void)
static const char fast_import_usage[] =
"git fast-import [--date-format=<f>] [--max-pack-size=<n>] [--big-file-threshold=<n>] [--depth=<n>] [--active-branches=<n>] [--export-marks=<marks.file>]";
-static void parse_argv(void)
+static void parse_argv(struct fast_import_state *state)
{
unsigned int i;
- for (i = 1; i < global_argc; i++) {
- const char *a = global_argv[i];
+ for (i = 1; i < state->argc; i++) {
+ const char *a = state->argv[i];
if (*a != '-' || !strcmp(a, "--"))
break;
@@ -3909,20 +3923,20 @@ static void parse_argv(void)
if (!skip_prefix(a, "--", &a))
die(_("unknown option %s"), a);
- if (parse_one_option(a))
+ if (parse_one_option(state, a))
continue;
- if (parse_one_feature(a, 0))
+ if (parse_one_feature(state, a, 0))
continue;
if (skip_prefix(a, "cat-blob-fd=", &a)) {
- option_cat_blob_fd(a);
+ option_cat_blob_fd(state, a);
continue;
}
die(_("unknown option --%s"), a);
}
- if (i != global_argc)
+ if (i != state->argc)
usage(fast_import_usage);
seen_data_command = 1;
@@ -3936,6 +3950,8 @@ int cmd_fast_import(int argc,
const char *prefix,
struct repository *repo)
{
+ struct fast_import_state state;
+
show_usage_if_asked(argc, argv, fast_import_usage);
reset_pack_idx_option(&pack_idx_opts);
@@ -3964,9 +3980,7 @@ int cmd_fast_import(int argc,
allow_unsafe_features = 1;
}
- global_argc = argc;
- global_argv = argv;
- global_prefix = prefix;
+ fast_import_state_init(&state, argc, argv, prefix);
rc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free));
for (unsigned int i = 0; i < (cmd_save - 1); i++)
@@ -3976,34 +3990,34 @@ int cmd_fast_import(int argc,
start_packfile();
set_die_routine(die_nicely);
set_checkpoint_signal();
- while (read_next_command() != EOF) {
+ while (read_next_command(&state) != EOF) {
const char *v;
if (!strcmp("blob", command_buf.buf))
- parse_new_blob();
+ parse_new_blob(&state);
else if (skip_prefix(command_buf.buf, "commit ", &v))
- parse_new_commit(v);
+ parse_new_commit(&state, v);
else if (skip_prefix(command_buf.buf, "tag ", &v))
- parse_new_tag(v);
+ parse_new_tag(&state, v);
else if (skip_prefix(command_buf.buf, "reset ", &v))
- parse_reset_branch(v);
+ parse_reset_branch(&state, v);
else if (skip_prefix(command_buf.buf, "ls ", &v))
- parse_ls(v, NULL);
+ parse_ls(&state, v, NULL);
else if (skip_prefix(command_buf.buf, "cat-blob ", &v))
- parse_cat_blob(v);
+ parse_cat_blob(&state, v);
else if (skip_prefix(command_buf.buf, "get-mark ", &v))
- parse_get_mark(v);
+ parse_get_mark(&state, v);
else if (!strcmp("checkpoint", command_buf.buf))
- parse_checkpoint();
+ parse_checkpoint(&state);
else if (!strcmp("done", command_buf.buf))
break;
else if (!strcmp("alias", command_buf.buf))
- parse_alias();
+ parse_alias(&state);
else if (starts_with(command_buf.buf, "progress "))
- parse_progress();
+ parse_progress(&state);
else if (skip_prefix(command_buf.buf, "feature ", &v))
- parse_feature(v);
+ parse_feature(&state, v);
else if (skip_prefix(command_buf.buf, "option git ", &v))
- parse_option(v);
+ parse_option(&state, v);
else if (starts_with(command_buf.buf, "option "))
/* ignore non-git options*/;
else
@@ -4015,7 +4029,7 @@ int cmd_fast_import(int argc,
/* argv hasn't been parsed yet, do so */
if (!seen_data_command)
- parse_argv();
+ parse_argv(&state);
if (require_explicit_termination && feof(stdin))
die(_("stream ends early"));
--
2.55.0.185.g9120d2b5c0
^ permalink raw reply related
* [PATCH 4/7] fast-import: localize 'i' into the 'for' loops using it
From: Christian Couder @ 2026-07-16 16:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Elijah Newren, Jeff King,
brian m . carlson, Johannes Schindelin, Justin Tobler,
Christian Couder, Christian Couder
In-Reply-To: <20260716165517.433849-1-christian.couder@gmail.com>
In cmd_fast_import(), a local variable 'i' is defined as an
`unsigned int` and then used as a loop counter in four different
`for (i = ...; i < ...; i++) { ... }` loops.
But in three out of the four cases, `unsigned int` isn't the best type
to use.
To give each loop counter the type matching its bound
(int/unsigned/size_t), let's localize 'i' into each loop that uses it.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin/fast-import.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index aa656c5195..fd4e13b7ca 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -3936,8 +3936,6 @@ int cmd_fast_import(int argc,
const char *prefix,
struct repository *repo)
{
- unsigned int i;
-
show_usage_if_asked(argc, argv, fast_import_usage);
reset_pack_idx_option(&pack_idx_opts);
@@ -3958,7 +3956,7 @@ int cmd_fast_import(int argc,
* 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.
*/
- for (i = 1; i < argc; i++) {
+ for (int i = 1; i < argc; i++) {
const char *arg = argv[i];
if (*arg != '-' || !strcmp(arg, "--"))
break;
@@ -3971,7 +3969,7 @@ int cmd_fast_import(int argc,
global_prefix = prefix;
rc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free));
- for (i = 0; i < (cmd_save - 1); i++)
+ for (unsigned int i = 0; i < (cmd_save - 1); i++)
rc_free[i].next = &rc_free[i + 1];
rc_free[cmd_save - 1].next = NULL;
@@ -4034,9 +4032,9 @@ int cmd_fast_import(int argc,
if (show_stats) {
uintmax_t total_count = 0, duplicate_count = 0;
- for (i = 0; i < ARRAY_SIZE(object_count_by_type); i++)
+ for (size_t i = 0; i < ARRAY_SIZE(object_count_by_type); i++)
total_count += object_count_by_type[i];
- for (i = 0; i < ARRAY_SIZE(duplicate_count_by_type); i++)
+ for (size_t i = 0; i < ARRAY_SIZE(duplicate_count_by_type); i++)
duplicate_count += duplicate_count_by_type[i];
fprintf(stderr, "%s statistics:\n", argv[0]);
--
2.55.0.185.g9120d2b5c0
^ permalink raw reply related
* [PATCH 3/7] api-parse-options.adoc: document hidden and OPT_*_F option macros
From: Christian Couder @ 2026-07-16 16:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Elijah Newren, Jeff King,
brian m . carlson, Johannes Schindelin, Justin Tobler,
Christian Couder, Christian Couder
In-Reply-To: <20260716165517.433849-1-christian.couder@gmail.com>
In "Documentation/technical/api-parse-options.adoc", the list of option
macros does not mention the `OPT_*_F()` macro variants that take a
trailing `flags` argument, nor the `OPT_HIDDEN_GROUP()` and
`OPT_HIDDEN_BOOL()` convenience macros.
Now that a previous commit documents the per-option flags, let's
document these macros too:
- Add a paragraph explaining the `OPT_*_F` convention and how it
relates to the per-option flags.
- Document `OPT_HIDDEN_GROUP()`, introduced in a previous commit,
right after `OPT_GROUP()`.
- Document `OPT_HIDDEN_BOOL()` right after `OPT_BOOL()`.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/technical/api-parse-options.adoc | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/Documentation/technical/api-parse-options.adoc b/Documentation/technical/api-parse-options.adoc
index fb4580e755..0e10327d07 100644
--- a/Documentation/technical/api-parse-options.adoc
+++ b/Documentation/technical/api-parse-options.adoc
@@ -213,6 +213,13 @@ Macros
There are some macros to easily define options:
+Many of the macros below have an `_F` variant (for example `OPT_BOOL_F`,
+`OPT_STRING_F`, `OPT_INTEGER_F`, `OPT_SET_INT_F`, `OPT_BIT_F` and
+`OPT_CALLBACK_F`) that takes an additional trailing `flags` argument.
+That argument is the bitwise-or of the per-option flags described in the
+"Option flags" section above; the non-`_F` macros are simply defined
+with `flags` set to `0`.
+
`OPT__ABBREV(&int_var)`::
Add `--abbrev[=<n>]`.
@@ -236,10 +243,21 @@ There are some macros to easily define options:
describes the group or an empty string.
Start the description with an upper-case letter.
+`OPT_HIDDEN_GROUP(description)`::
+ Like `OPT_GROUP()`, but the group header carries
+ `PARSE_OPT_HIDDEN`, so it is only shown by `--help-all` and not
+ by `-h`. Use it to label a group that contains only hidden
+ options, which would otherwise show an empty header under `-h`.
+
`OPT_BOOL(short, long, &int_var, description)`::
Introduce a boolean option. `int_var` is set to one with
`--option` and set to zero with `--no-option`.
+`OPT_HIDDEN_BOOL(short, long, &int_var, description)`::
+ Like `OPT_BOOL()`, but the option carries `PARSE_OPT_HIDDEN`,
+ so it is hidden from `-h` while still being shown by
+ `--help-all`.
+
`OPT_COUNTUP(short, long, &int_var, description)`::
Introduce a count-up option.
Each use of `--option` increments `int_var`, starting from zero
--
2.55.0.185.g9120d2b5c0
^ permalink raw reply related
* [PATCH 2/7] api-parse-options.adoc: document per-option flags
From: Christian Couder @ 2026-07-16 16:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Elijah Newren, Jeff King,
brian m . carlson, Johannes Schindelin, Justin Tobler,
Christian Couder, Christian Couder
In-Reply-To: <20260716165517.433849-1-christian.couder@gmail.com>
The "Flags" section in "Documentation/technical/api-parse-options.adoc"
documents the flags that can be passed to parse_options() itself. It
does not, however, document the flags that can be set on individual
options through the `flags` member of `struct option` (and through the
`OPT_*_F()` macro variants).
These per-option flags are used throughout the codebase (for example
`PARSE_OPT_HIDDEN` is used to hide an option from `-h` while still
showing it with `--help-all`), but a reader currently has to dig into
"parse-options.h" to find them.
To remediate that, let's add an "Option flags" subsection to the
"Data Structure" section, just before the list of option macros.
Let's also make it explicit that these are distinct from the
parse_options() flags described earlier, and let's describe the `-h`
versus `--help-all` behavior for `PARSE_OPT_HIDDEN`.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
.../technical/api-parse-options.adoc | 61 +++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/Documentation/technical/api-parse-options.adoc b/Documentation/technical/api-parse-options.adoc
index 880eb94642..fb4580e755 100644
--- a/Documentation/technical/api-parse-options.adoc
+++ b/Documentation/technical/api-parse-options.adoc
@@ -150,6 +150,67 @@ Data Structure
The main data structure is an array of the `option` struct,
say `static struct option builtin_add_options[]`.
+
+Option flags
+~~~~~~~~~~~~
+
+Each option can carry flags in the `flags` field of its `option`
+struct. These are per-option flags and are distinct from the
+`parse_options()` flags described above; they are usually set through
+the `OPT_*_F()` macro variants (see below) rather than by hand. They
+are the bitwise-or of:
+
+`PARSE_OPT_OPTARG`::
+ The option's argument is optional, i.e. both `--option` and
+ `--option=<value>` are accepted.
+
+`PARSE_OPT_NOARG`::
+ The option takes no argument at all. Using `--option=<value>`
+ is rejected.
+
+`PARSE_OPT_NONEG`::
+ Disable the automatically generated negated `--no-option`
+ form.
+
+`PARSE_OPT_HIDDEN`::
+ Hide the option: it is omitted from the usage shown by
+ `git <cmd> -h`, but is still shown by `git <cmd> --help-all`.
+ The option is parsed as usual either way. This is meant for
+ 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.
+
+`PARSE_OPT_NODASH`::
+ The option is a single character without a leading dash, such
+ as the `+` used by some commands.
+
+`PARSE_OPT_LITERAL_ARGHELP`::
+ Use the argument help string (`argh`) verbatim in the usage
+ output instead of surrounding it with `<>` or `[]`. Useful when
+ `argh` already contains a hand-formatted description.
+
+`PARSE_OPT_FROM_ALIAS`::
+ Internal flag, set on options that were expanded from a
+ configured alias. It should not be set by callers.
+
+`PARSE_OPT_NOCOMPLETE`::
+ Do not offer this option for completion.
+
+`PARSE_OPT_COMP_ARG`::
+ The option's argument, rather than the option itself, is what
+ should be completed.
+
+`PARSE_OPT_CMDMODE`::
+ The option is one of several mutually exclusive "command mode"
+ options that share the same variable. Using more than one of
+ them at once is rejected.
+
+Macros
+~~~~~~
+
There are some macros to easily define options:
`OPT__ABBREV(&int_var)`::
--
2.55.0.185.g9120d2b5c0
^ permalink raw reply related
* [PATCH 1/7] parse-options: introduce OPT_HIDDEN_GROUP
From: Christian Couder @ 2026-07-16 16:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Elijah Newren, Jeff King,
brian m . carlson, Johannes Schindelin, Justin Tobler,
Christian Couder, Christian Couder
In-Reply-To: <20260716165517.433849-1-christian.couder@gmail.com>
Hidden options are not shown by `git <cmd> -h`, but are still shown by
`git <cmd> --help-all`. If there are a lot of hidden options or if they
don't belong to the same categories as other options, there is
currently no way to properly group them.
Using `OPT_GROUP("Foo")` means that "Foo" will always be shown which we
don't want if that group contains only hidden options.
To provide a way to have groups shown only when hidden options are
shown, let's implement an OPT_HIDDEN_GROUP macro.
To test this new macro, let's also improve `test-tool parse-options`
and test its output with `--help-all`.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
parse-options.c | 4 ++--
parse-options.h | 5 +++++
t/helper/test-parse-options.c | 4 ++++
t/t0040-parse-options.sh | 25 ++++++++++++++++++++++++-
4 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index f4647e0099..640e600de8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1404,6 +1404,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
if (opts->type == OPTION_SUBCOMMAND)
continue;
+ if (!full && (opts->flags & PARSE_OPT_HIDDEN))
+ continue;
if (opts->type == OPTION_GROUP) {
fputc('\n', outfile);
need_newline = 0;
@@ -1411,8 +1413,6 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
fprintf(outfile, "%s\n", _(opts->help));
continue;
}
- if (!full && (opts->flags & PARSE_OPT_HIDDEN))
- continue;
if (need_newline) {
fputc('\n', outfile);
diff --git a/parse-options.h b/parse-options.h
index 0d1f738f8d..a28b3cd942 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -236,6 +236,11 @@ struct option {
.type = OPTION_GROUP, \
.help = (h), \
}
+#define OPT_HIDDEN_GROUP(h) { \
+ .type = OPTION_GROUP, \
+ .help = (h), \
+ .flags = PARSE_OPT_HIDDEN, \
+}
#define OPT_BIT(s, l, v, h, b) OPT_BIT_F(s, l, v, h, b, 0)
#define OPT_BITOP(s, l, v, h, set, clear) { \
.type = OPTION_BITOP, \
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 68579d83f3..f181f0c02d 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -209,6 +209,10 @@ int cmd__parse_options(int argc, const char **argv)
OPT_GROUP("Alias"),
OPT_STRING('A', "alias-source", &string, "string", "get a string"),
OPT_ALIAS('Z', "alias-target", "alias-source"),
+ OPT_HIDDEN_GROUP("Hidden options"),
+ OPT_HIDDEN_BOOL(0, "hidden-bool", &boolean, "get a boolean"),
+ OPT_INTEGER_F('k', "hidden-integer", &integer, "get a integer",
+ PARSE_OPT_HIDDEN),
OPT_END(),
};
int ret = 0;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ca55ea8228..4040333185 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -7,7 +7,7 @@ test_description='our own option parser'
. ./test-lib.sh
-cat >expect <<\EOF
+cat >expect-part1 <<\EOF
usage: test-tool parse-options <options>
A helper function for the parse-options API.
@@ -41,6 +41,9 @@ String options
--[no-]string2 <str> get another string
--[no-]st <st> get another string (pervert ordering)
-o <str> get another string
+EOF
+
+cat >expect-part2 <<\EOF
--longhelp help text of this entry
spans multiple lines
--[no-]list <str> add str to list
@@ -67,12 +70,32 @@ Alias
EOF
+cat >expect-noop <<\EOF
+ --[no-]obsolete no-op (backward compatibility)
+EOF
+
+cat >expect-hidden <<\EOF
+Hidden options
+ --[no-]hidden-bool get a boolean
+ -k, --[no-]hidden-integer <n>
+ get a integer
+
+EOF
+
test_expect_success 'test help' '
+ cat expect-part1 expect-part2 >expect &&
test_must_fail test-tool parse-options -h >output 2>output.err &&
test_must_be_empty output.err &&
test_cmp expect output
'
+test_expect_success 'test --help-all shows hidden group and options' '
+ cat expect-part1 expect-noop expect-part2 expect-hidden >expect-help-all &&
+ test_must_fail test-tool parse-options --help-all >output 2>output.err &&
+ test_must_be_empty output.err &&
+ test_cmp expect-help-all output
+'
+
mv expect expect.err
check () {
--
2.55.0.185.g9120d2b5c0
^ permalink raw reply related
* [PATCH 0/7] fast-import: standardize usage string and SYNOPSIS
From: Christian Couder @ 2026-07-16 16:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Elijah Newren, Jeff King,
brian m . carlson, Johannes Schindelin, Justin Tobler,
Christian Couder
The goal of this series is to improve on `git fast-import`'s usage
string as it is obsolete in many ways.
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.
`struct option` is used only for the usage string for now and there
are still many global variables left, so it's left to future work to
finish on these directions.
But this is already enough to standardize the usage string and make it
consistent with the SYNOPSIS in the docs, so that the command can be
removed from "t/t0450/adoc-help-mismatches".
Overview of the patches
=======================
- Patch 1/7: Introduces OPT_HIDDEN_GROUP and improves on the hidden
option tests.
- Patches 2/7 and 3/7: Improves on the parse-options API docs.
- Patch 4/7: Cleans up an 'i' loop counter variable in
cmd_fast_import().
- Patches 5/7 and 6/7: Starts libifying "builtin/fast-import.c" by
introducing a 'struct fast_import_state' and using it to store
some global variables.
- Patch 7/7: Improves the usage string and SYNOPSIS by introducing
`struct option`.
CI tests
========
They all pass, see: https://github.com/chriscool/git/actions/runs/29513714019
Christian Couder (7):
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: introduce 'struct fast_import_state'
fast-import: move command state globals into 'struct
fast_import_state'
fast-import: use struct option for usage string
Documentation/git-fast-import.adoc | 2 +-
.../technical/api-parse-options.adoc | 79 ++++
builtin/fast-import.c | 372 +++++++++++-------
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 -
8 files changed, 344 insertions(+), 148 deletions(-)
--
2.55.0.185.g9120d2b5c0
^ permalink raw reply
* Vagrant + GitBash Issues
From: Moksh Goyal @ 2026-07-16 16:40 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 406 bytes --]
Hi Team,
I am currently using Vagrant and logging into my virtual machine via SSH. I
have encountered an issue where pressing Ctrl+C terminates my SSH session
entirely instead of just force-stopping the active command.
This setup works correctly when using PowerShell, but the issue
consistently occurs when using Git Bash.
Do you have any suggestions on how to resolve this?
Best regards,
Moksh Goyal
[-- Attachment #1.2: Type: text/html, Size: 463 bytes --]
[-- Attachment #2: Screenshot 2026-07-16 220913.png --]
[-- Type: image/png, Size: 65119 bytes --]
^ permalink raw reply
* Re: [PATCH v5 0/9] [RFC] diff: add diff.<driver>.process for external hunk providers
From: Junio C Hamano @ 2026-07-16 16:40 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget
Cc: git, Johannes Schindelin, Michael Montalbo
In-Reply-To: <pull.2120.v5.git.1784149323.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> A "Which features consult the diff process" section in gitattributes(5) lays
> out, per feature, why each does or does not consult the process (patch
> output, blame, summary formats, and the -L line-range view do; pickaxe -G,
> patch-id, merge, range-diff, --check, and --raw do not, with reasons).
> Combined diffs (--cc) remain on the builtin algorithm and are noted as
> future work.
>
> Changes since v4:
This round does not play well with the mm/line-log-limited-ops
topic, unfortunately, it seems.
^ permalink raw reply
* Re: [PATCH GSoC v18 10/13] transport: add client support for object-info
From: Junio C Hamano @ 2026-07-16 16:37 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, chandrapratap3519, chriscool, eric.peijian, jltobler,
karthik.188, peff, toon, Calvin Wan, Jonathan Tan
In-Reply-To: <20260715-ps-eric-work-rebase-v18-10-34d7adb051bb@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
> @@ -1159,6 +1159,7 @@ LIB_OBJS += ewah/ewah_rlw.o
> LIB_OBJS += exec-cmd.o
> LIB_OBJS += fetch-negotiator.o
> LIB_OBJS += fetch-pack.o
> +LIB_OBJS += fetch-object-info.o
> LIB_OBJS += fmt-merge-msg.o
> LIB_OBJS += fsck.o
> LIB_OBJS += fsmonitor.o
Noticed while preparing evil merge for Patrick's patch to move
everything under a new lib/ directory, but you inserted the new
entry in a wrong place, i.e. 'o' < 'p'. Keep the list sorted.
^ permalink raw reply
* Re: [PATCH v3 0/6] refs: remove use of `the_repository`
From: Toon Claes @ 2026-07-16 15:36 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Junio C Hamano
In-Reply-To: <20260716-pks-refs-wo-the-repository-v3-0-db0a804e0224@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Changes in v3:
> - Merge the patch that removes `USE_THE_REPOSITORY_VARIABLE` from the
> "packed" backend into the patch that removes the last use of
> `the_repository`.
Makes sense to me.
And the range-diff seems to confirm that.
--
Cheers,
Toon
^ permalink raw reply
* Re: [GSoC Patch 7/7] repo: add path.git-prefix path key validation
From: K Jayatheerth @ 2026-07-16 15:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jltobler, lucasseikioshiro
In-Reply-To: <xmqqech3k47u.fsf@gitster.g>
Hey Junio,
On Thu, Jul 16, 2026 at 8:53 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>
> > diff --git a/builtin/repo.c b/builtin/repo.c
> > index a97ad71649..00d5064281 100644
> > --- a/builtin/repo.c
> > +++ b/builtin/repo.c
> > @@ -1,3 +1,4 @@
> > +#include "compat/posix.h"
> > #define USE_THE_REPOSITORY_VARIABLE
> > #include "builtin.h"
>
> The first include must be <git-compat-util.h> or common include
> files that include <git-compat-util.h> as the first thing, like
> <builtin.h>.
>
> As the file already includes <builtin.h>, extra inclusion of
> <compat/posix.h> before everything else is an absolute no-no.
>
Oh no, that's just my editor adding headers automatically.
Thanks for pointing this out, I will fix it.
> By the way, I do not see any "validation" in the patch as the title
> claims. Perhaps retitle it to "repo: add path.git-prefix key" or
> something simpler like that?
>
True, I will correct that as well.
Regards,
- K Jayatheerth
^ permalink raw reply
* Re: [PATCH v1] repository: move fetch_if_missing into struct repository
From: Junio C Hamano @ 2026-07-16 15:28 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Tian Yuchen, git, five231003, hariom18599, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <alcqQp0lkwRIIE1t@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
>> /*
>> * index-pack never needs to fetch missing objects except when
>> * REF_DELTA bases are missing (which are explicitly handled). It only
>> * accesses the repo to do hash collision checks and to check which
>> * REF_DELTA bases need to be fetched.
>> */
>> - fetch_if_missing = 0;
>> -
>> - show_usage_if_asked(argc, argv, index_pack_usage);
>> + if (repo)
>> + repo->fetch_if_missing = 0;
>>
>> disable_replace_refs();
>>
>
> Okay. This command can run without a repository, in which case we'll end
> up just indexing the pack. My assumption is that we'll probably end up
> using `the_repository` if so, as we still use `the_repository` in this
> file. So could this here cause a change in behaviour?
Meaning that even outside a repository, we could have read the
setting from ~/.gitconfig or some other places other than the
per-repository .git/config file?
^ permalink raw reply
* [PATCH v2] copy: drop dependency on `the_repository`
From: Patrick Steinhardt @ 2026-07-16 15:28 UTC (permalink / raw)
To: git; +Cc: Phillip Wood
In-Reply-To: <20260716-pks-copy-wo-the-repository-v1-1-8f1e078bb82f@pks.im>
When copying a file we need to potentially adapt permissions of the new
file based on whether or not "core.shared" is enabled. Parsing this
configuration makes us implicitly depend on `the_repository`.
Refactor the code to instead require the caller to pass in a repository
so that we can remove `USE_THE_REPOSITORY_VARIABLE`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Hi,
I guess the title says it all: this small patch removes the dependency
on `the_repository` in "copy.c". Thanks!
Changes in v2:
- Adapt a couple more sites to use a repository from the context.
- Link to v1: https://patch.msgid.link/20260716-pks-copy-wo-the-repository-v1-1-8f1e078bb82f@pks.im
Patrick
---
builtin/clone.c | 2 +-
builtin/difftool.c | 4 ++--
builtin/worktree.c | 4 ++--
bundle-uri.c | 2 +-
copy.c | 12 ++++++------
copy.h | 8 ++++++--
refs/files-backend.c | 2 +-
rerere.c | 2 +-
sequencer.c | 6 +++---
setup.c | 2 +-
10 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index d60d1b60bc..18603dd4ce 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -335,7 +335,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
die_errno(_("failed to create link '%s'"), dest->buf);
option_no_hardlinks = 1;
}
- if (copy_file_with_time(dest->buf, src->buf, 0666))
+ if (copy_file_with_time(the_repository, dest->buf, src->buf, 0666))
die_errno(_("failed to copy file to '%s'"), dest->buf);
}
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 26778f8515..5e7777fbe4 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -552,7 +552,7 @@ static int run_dir_diff(struct repository *repo,
struct stat st;
if (stat(wtdir.buf, &st))
st.st_mode = 0644;
- if (copy_file(rdir.buf, wtdir.buf,
+ if (copy_file(repo, rdir.buf, wtdir.buf,
st.st_mode)) {
ret = error("could not copy '%s' to '%s'", wtdir.buf, rdir.buf);
goto finish;
@@ -658,7 +658,7 @@ static int run_dir_diff(struct repository *repo,
warning("%s", "");
err = 1;
} else if (unlink(wtdir.buf) ||
- copy_file(wtdir.buf, rdir.buf, st.st_mode))
+ copy_file(repo, wtdir.buf, rdir.buf, st.st_mode))
warning_errno(_("could not copy '%s' to '%s'"),
rdir.buf, wtdir.buf);
}
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d21c43fde3..84b01960fb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -349,7 +349,7 @@ static void copy_sparse_checkout(const char *worktree_git_dir)
if (file_exists(from_file)) {
if (safe_create_leading_directories(the_repository, to_file) ||
- copy_file(to_file, from_file, 0666))
+ copy_file(the_repository, to_file, from_file, 0666))
error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
from_file, to_file);
}
@@ -368,7 +368,7 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)
int bare;
if (safe_create_leading_directories(the_repository, to_file) ||
- copy_file(to_file, from_file, 0666)) {
+ copy_file(the_repository, to_file, from_file, 0666)) {
error(_("failed to copy worktree config from '%s' to '%s'"),
from_file, to_file);
goto worktree_copy_cleanup;
diff --git a/bundle-uri.c b/bundle-uri.c
index 3b2e347288..ef37aebf30 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -396,7 +396,7 @@ static int copy_uri_to_file(const char *filename, const char *uri)
uri = out;
/* Copy as a file */
- return copy_file(filename, uri, 0);
+ return copy_file(the_repository, filename, uri, 0);
}
static int unbundle_from_file(struct repository *r, const char *file)
diff --git a/copy.c b/copy.c
index b668209b6c..6074132050 100644
--- a/copy.c
+++ b/copy.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "git-compat-util.h"
#include "copy.h"
#include "path.h"
@@ -35,7 +33,8 @@ static int copy_times(const char *dst, const char *src)
return 0;
}
-int copy_file(const char *dst, const char *src, int mode)
+int copy_file(struct repository *repo,
+ const char *dst, const char *src, int mode)
{
int fdi, fdo, status;
@@ -59,15 +58,16 @@ int copy_file(const char *dst, const char *src, int mode)
if (close(fdo) != 0)
return error_errno("%s: close error", dst);
- if (!status && adjust_shared_perm(the_repository, dst))
+ if (!status && adjust_shared_perm(repo, dst))
return -1;
return status;
}
-int copy_file_with_time(const char *dst, const char *src, int mode)
+int copy_file_with_time(struct repository *repo,
+ const char *dst, const char *src, int mode)
{
- int status = copy_file(dst, src, mode);
+ int status = copy_file(repo, dst, src, mode);
if (!status)
return copy_times(dst, src);
return status;
diff --git a/copy.h b/copy.h
index 2af77cba86..1059b118d6 100644
--- a/copy.h
+++ b/copy.h
@@ -1,10 +1,14 @@
#ifndef COPY_H
#define COPY_H
+struct repository;
+
#define COPY_READ_ERROR (-2)
#define COPY_WRITE_ERROR (-3)
int copy_fd(int ifd, int ofd);
-int copy_file(const char *dst, const char *src, int mode);
-int copy_file_with_time(const char *dst, const char *src, int mode);
+int copy_file(struct repository *repo,
+ const char *dst, const char *src, int mode);
+int copy_file_with_time(struct repository *repo,
+ const char *dst, const char *src, int mode);
#endif /* COPY_H */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3df56c25c8..442c98414e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1736,7 +1736,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
goto out;
}
- if (copy && log && copy_file(tmp_renamed_log.buf, sb_oldref.buf, 0644)) {
+ if (copy && log && copy_file(refs->base.repo, tmp_renamed_log.buf, sb_oldref.buf, 0644)) {
ret = error("unable to copy logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
goto out;
diff --git a/rerere.c b/rerere.c
index 8232542585..bf5cfc6e51 100644
--- a/rerere.c
+++ b/rerere.c
@@ -756,7 +756,7 @@ static void do_rerere_one_path(struct index_state *istate,
/* Has the user resolved it already? */
if (variant >= 0) {
if (!handle_file(istate, path, NULL, NULL)) {
- copy_file(rerere_path(&buf, id, "postimage"), path, 0666);
+ copy_file(the_repository, rerere_path(&buf, id, "postimage"), path, 0666);
id->collection->status[variant] |= RR_HAS_POSTIMAGE;
fprintf_ln(stderr, _("Recorded resolution for '%s'."), path);
free_rerere_id(rr_item);
diff --git a/sequencer.c b/sequencer.c
index 1355a99a09..63bc1ef215 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2419,7 +2419,7 @@ static int do_pick_commit(struct repository *r,
} else {
const char *dest = git_path_squash_msg(r);
unlink(dest);
- if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
+ if (copy_file(r, dest, rebase_path_squash_msg(), 0666)) {
res = error(_("could not copy '%s' to '%s'"),
rebase_path_squash_msg(), dest);
goto leave;
@@ -3864,11 +3864,11 @@ static int error_failed_squash(struct repository *r,
int subject_len,
const char *subject)
{
- if (copy_file(rebase_path_message(), rebase_path_squash_msg(), 0666))
+ if (copy_file(r, rebase_path_message(), rebase_path_squash_msg(), 0666))
return error(_("could not copy '%s' to '%s'"),
rebase_path_squash_msg(), rebase_path_message());
unlink(git_path_merge_msg(r));
- if (copy_file(git_path_merge_msg(r), rebase_path_message(), 0666))
+ if (copy_file(r, git_path_merge_msg(r), rebase_path_message(), 0666))
return error(_("could not copy '%s' to '%s'"),
rebase_path_message(),
git_path_merge_msg(r));
diff --git a/setup.c b/setup.c
index 0de56a074f..91d61a5939 100644
--- a/setup.c
+++ b/setup.c
@@ -2331,7 +2331,7 @@ static void copy_templates_1(struct repository *repo,
strbuf_release(&lnk);
}
else if (S_ISREG(st_template.st_mode)) {
- if (copy_file(path->buf, template_path->buf, st_template.st_mode))
+ if (copy_file(repo, path->buf, template_path->buf, st_template.st_mode))
die_errno(_("cannot copy '%s' to '%s'"),
template_path->buf, path->buf);
}
---
base-commit: d35c5399e3e54ac277bb391fc2f6be3e816d312b
change-id: 20260716-pks-copy-wo-the-repository-aa01ccdbed76
^ permalink raw reply related
* Re: [PATCH] copy: drop dependency on `the_repository`
From: Patrick Steinhardt @ 2026-07-16 15:06 UTC (permalink / raw)
To: phillip.wood; +Cc: git
In-Reply-To: <27d4d72f-1ab5-4dc3-8cf6-1a9b6883a631@gmail.com>
On Thu, Jul 16, 2026 at 02:41:44PM +0100, Phillip Wood wrote:
> > diff --git a/sequencer.c b/sequencer.c
> > index 1355a99a09..c9ede9c02d 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2419,7 +2419,7 @@ static int do_pick_commit(struct repository *r,
> > } else {
> > const char *dest = git_path_squash_msg(r);
> > unlink(dest);
> > - if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
> > + if (copy_file(the_repository, dest, rebase_path_squash_msg(), 0666)) {
>
> The path for "dest" is obtained using a local repository instance "r", but
> we're using "the_repository" to set the permissions on that path. While that
> matches the current behavior it is clearly better to use the same repository
> instance to obtain both the path and and permissions for that path. In the
> hunk below we even have "the_repository" and "r" on the same line which
> seems confusing. This patch uses a local repository instance in
> refs/files-backend.c and setup.c, lets do the same here.
Makes sense. In that case though I'll also adapt the two uses of
`the_repository` below. Thanks!
Patrick
> > @@ -3864,11 +3864,11 @@ static int error_failed_squash(struct repository *r,
> > int subject_len,
> > const char *subject)
> > {
> > - if (copy_file(rebase_path_message(), rebase_path_squash_msg(), 0666))
> > + if (copy_file(the_repository, rebase_path_message(), rebase_path_squash_msg(), 0666))
> > return error(_("could not copy '%s' to '%s'"),
> > rebase_path_squash_msg(), rebase_path_message());
> > unlink(git_path_merge_msg(r));
> > - if (copy_file(git_path_merge_msg(r), rebase_path_message(), 0666))
> > + if (copy_file(the_repository, git_path_merge_msg(r), rebase_path_message(), 0666))
> > return error(_("could not copy '%s' to '%s'"),
> > rebase_path_message(),
> > git_path_merge_msg(r));
^ permalink raw reply
* Re: [PATCH GSoC v18 05/13] fetch-pack: drop static advertise_sid variable
From: Pablo Sabater @ 2026-07-16 14:52 UTC (permalink / raw)
To: Karthik Nayak, Pablo Sabater, git
Cc: chandrapratap3519, chriscool, eric.peijian, gitster, jltobler,
peff, toon, Jonathan Tan, Calvin Wan
In-Reply-To: <CAOLa=ZSy1Z-R38cqFiz-Ejj9CNJkp4x_6rFk_wdfhyBytYH9fw@mail.gmail.com>
On Wed Jul 15, 2026 at 11:48 PM CEST, Karthik Nayak wrote:
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
>> write_fetch_command_and_capabilities() is moved to 'connect.c' in a
>> subsequent commit. To prepare for that, drop the static variable usage
>> of advertise_sid. Currently advertise_sid is used in two places:
>>
>> 1. In function do_fetch_pack():
>> if (!server_supports("session-id"))
>> advertise_sid = 0;
>>
>> 2. In function fetch_pack_config():
>> repo_config_get_bool("transfer.advertisesid", &advertise_sid);
>>
>
> Nit: But #2 isn't a usecase, it's where it is set no? Looking at the
> usecase, it seems like we have two:
>
> #1 like you stated
> #2 within `write_fetch_command_and_capabilities()`. But the flow is
> that, the variable is set in `fetch_pack_config()`, right?
Completly true, fetch_pack_config() is where it's set, but not where
it's used.
I'll fix #2 to be where it is used.
Thanks.
>
>> About 1, it is only relevant for v0/v1 protocol, move it into
>> find_common().
>>
>> About 2, call repo_config_get_bool() inside of
>> write_fetch_command_and_capabilities() and find_common() replacing the
>> static variable.
>>
>> Because repo_config_get_bool() leaves advertise_sid as is if it is not
>> set, initialize it to 0 matching its default.
>>
>> Helped-by: Jonathan Tan <jonathantanmy@google.com>
>> Helped-by: Christian Couder <chriscool@tuxfamily.org>
>> Signed-off-by: Calvin Wan <calvinwan@google.com>
>> Signed-off-by: Eric Ju <eric.peijian@gmail.com>
>> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
>> ---
>> fetch-pack.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index eea72b2500..8e04db8640 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -49,7 +49,6 @@ static int fetch_fsck_objects = -1;
>> static int transfer_fsck_objects = -1;
>> static int agent_supported;
>> static int server_supports_filtering;
>> -static int advertise_sid;
>> static struct shallow_lock shallow_lock;
>> static const char *alternate_shallow_file;
>> static struct strbuf fsck_msg_types = STRBUF_INIT;
>> @@ -363,6 +362,9 @@ static int find_common(struct fetch_negotiator *negotiator,
>> size_t state_len = 0;
>> struct packet_reader reader;
>> struct oidset negotiation_include_oids = OIDSET_INIT;
>> + int advertise_sid = 0;
>> +
>> + repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
>>
>> if (args->stateless_rpc && multi_ack == 1)
>> die(_("the option '%s' requires '%s'"), "--stateless-rpc", "multi_ack_detailed");
>> @@ -414,7 +416,7 @@ static int find_common(struct fetch_negotiator *negotiator,
>> if (deepen_not_ok) strbuf_addstr(&c, " deepen-not");
>> if (agent_supported) strbuf_addf(&c, " agent=%s",
>> git_user_agent_sanitized());
>> - if (advertise_sid)
>> + if (advertise_sid && server_supports("session-id"))
>> strbuf_addf(&c, " session-id=%s", trace2_session_id());
>> if (args->filter_options.choice)
>> strbuf_addstr(&c, " filter");
>> @@ -1160,9 +1162,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>> (int)agent_len, agent_feature);
>> }
>>
>> - if (!server_supports("session-id"))
>> - advertise_sid = 0;
>> -
>
> So earlier we'd set the `advertise_sid` to 0 if the server didn't support
> 'session-id'. But we could directly check where it is needed, which is
> `find_common()`. Is `find_common()` only called in v0/v1 as stated in
> the commit message?
Yes, find_common() is only called from do_fetch_pack() which is the v0/v1
path, if we take a look at fetch_pack():
if (version == protocol_v2) {
[snip]
ref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,
&shallows_scratch, &si,
pack_lockfiles);
} else {
[snip]
ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
&si, pack_lockfiles);
}
>
>> if (server_supports("shallow"))
>> print_verbose(args, _("Server supports %s"), "shallow");
>> else if (args->depth > 0 || is_repository_shallow(r))
>> @@ -1380,6 +1379,9 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
>> const struct string_list *server_options)
>> {
>> const char *hash_name;
>> + int advertise_sid = 0;
>> +
>> + repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
>>
>> ensure_server_supports_v2("fetch");
>> packet_buf_write(req_buf, "command=fetch");
>> @@ -1998,7 +2000,6 @@ static void fetch_pack_config(void)
>> repo_config_get_bool(the_repository, "repack.usedeltabaseoffset", &prefer_ofs_delta);
>> repo_config_get_bool(the_repository, "fetch.fsckobjects", &fetch_fsck_objects);
>> repo_config_get_bool(the_repository, "transfer.fsckobjects", &transfer_fsck_objects);
>> - repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
>> if (!uri_protocols.nr) {
>> char *str;
>>
>>
>> --
>> 2.54.0
Thanks for the feedback, will make it clear next reroll,
Pablo
^ permalink raw reply
* Re: [PATCH v3 0/6] refs: remove use of `the_repository`
From: Junio C Hamano @ 2026-07-16 14:36 UTC (permalink / raw)
To: Christian Couder; +Cc: Patrick Steinhardt, git, Toon Claes
In-Reply-To: <CAP8UFD2e15P19_XCVyf-NQHz8Dj8R4UshxzWL-i6R8c6prmc5A@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Thu, Jul 16, 2026 at 7:33 AM Patrick Steinhardt <ps@pks.im> wrote:
>>
>> Hi,
>>
>> this patch series refactors the ref subsystem to drop uses of
>> `the_repository`. These patches were part of a discarded attempt to
>> make the initialization of the refdb eager. I guess they make sense by
>> themselves though, so here we go.
>>
>> Note that these patches contain a slight tangent to also adapt
>> "worktree.c". This is one of the subsystems that caused problems with
>> eager refdb initialization because of `has_worktrees()`, so I refactored
>> this subsystem while at it.
>
> The changes in this series look good to me too.
Thanks, all, for helping this topic. Let's merge it down to 'next',
then.
^ permalink raw reply
* Re: [PATCH] remote-curl: simplify passing of push specs
From: Junio C Hamano @ 2026-07-16 14:28 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: René Scharfe, Git List
In-Reply-To: <alhr2bb0lUTHtvjO@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> On Wed, Jul 15, 2026 at 05:39:51PM +0200, René Scharfe wrote:
>> >
>> We could add one. Not sure it would make a measurable difference; if
>> the number of specs is huge there are probably other costs that dwarf
>> pushing them to a strvec.
>
> Yeah, I don't expect it to make a difference here, either. But by having
> it we could use it in more places going forward, and that might lead to
> tiny savings here and there that ultimately add up. So it'd be nudging
> folks to "do the right thing".
That's a sensible thought.
>> I have to admit that the simplicity of strvec_pushv() nudged me towards
>> using a NULL-terminated array here, though. So just having a
>> strvec_pushvec() available could guide towards using the length-limited
>> strvec instead of a simpler NULL-terminated array (which explodes if
>> left unterminated).
>
> And that's not a huge issue by itself. I think the version you have here
> is totally fine, and I won't insist on a reroll. But I think it gives us
> a good opportunity to improve the status quo, if we want to take it.
Yeah, strvec_pushvec() might be a worthwhile thing to do, but that
can come independent of this topic. The output from:
$ git grep -n -e strvec_pushv\(
is easy enough to look through to find which callers pass a strvec
as the second parameter. It should be quite straightforward to find
conversion candidates once the helper is actually implemented. It
might even be possible to use Coccinelle for such a conversion.
^ permalink raw reply
* [PATCH 2/2] wincred: prevent silent credential loss when storing OAuth tokens
From: Johannes Schindelin via GitGitGadget @ 2026-07-16 14:27 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2182.git.1784212072.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `git credential approve` hands the wincred helper a password
together with an `oauth_refresh_token`, the OAuth branch of
`store_credential()` writes one WCHAR past the allocation while
formatting both fields into a single `CredentialBlob`. On Windows
this trips heap verification and tears the helper down with status
`0xC0000374`; `approve` masks the failure, so the credential the
user meant to save never reaches `CredWriteW()` and the next
session prompts for it again.
The bug has the same shape as the one fixed in the previous commit:
the allocation leaves no room for the terminating NUL, and the
`sizeOfBuffer` argument to `_snwprintf_s()` is a byte count where
the API expects a WCHAR count, which lets the safe-CRT runtime
write the terminator out of bounds.
Apply the same remedy d22a488482 (wincred: avoid memory corruption,
2025-11-17) applied in `get_credential()`: allocate `(wlen + 1) *
sizeof(WCHAR)` bytes and pass `wlen + 1` as the destination
capacity in WCHARs.
This closes the second of the two heap writes tracked under
GHSA-rxqw-wxqg-g7hw.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/credential/wincred/git-credential-wincred.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 190bbccdf9..22eb27ca31 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -208,8 +208,8 @@ static void store_credential(void)
if (oauth_refresh_token) {
wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
- secret = xmalloc(sizeof(WCHAR) * wlen);
- _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
+ secret = xmalloc((wlen + 1) * sizeof(WCHAR));
+ _snwprintf_s(secret, wlen + 1, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
} else {
secret = _wcsdup(password);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/2] wincred: avoid memory corruption when erasing a credential
From: Johannes Schindelin via GitGitGadget @ 2026-07-16 14:27 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2182.git.1784212072.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The earlier d22a488482 (wincred: avoid memory corruption, 2025-11-17)
repaired only get_credential(); match_cred_password() has the same
defect and is reached on `git credential reject`. When Git asks the
helper to erase a stored credential whose password was supplied by
the caller, the helper copies the candidate's password into a freshly
allocated buffer for comparison. That copy overruns the allocation
by one WCHAR of NUL, which on uninstrumented Windows manifests as
process termination with status 0xC0000374. Because the helper can
die before reaching CredDeleteW(), `git credential reject` masks the
failure and the rejected credential remains stored.
CredentialBlobSize is documented as a byte count, so for an N-WCHAR
blob it equals N * sizeof(WCHAR). The pre-fix code allocated that
many bytes and asked wcsncpy_s to copy N wide characters, but
wcsncpy_s always appends a terminating NUL WCHAR, writing one WCHAR
past the allocation. The destination-capacity argument was also
passed in bytes rather than in WCHAR elements as the API requires,
so the safe-CRT runtime never rejected the copy.
See GHSA-rxqw-wxqg-g7hw.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/credential/wincred/git-credential-wincred.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 73c2b9b72a..190bbccdf9 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -121,10 +121,10 @@ static int match_part_last(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR delim)
static int match_cred_password(const CREDENTIALW *cred) {
int ret;
- WCHAR *cred_password = xmalloc(cred->CredentialBlobSize);
- wcsncpy_s(cred_password, cred->CredentialBlobSize,
- (LPCWSTR)cred->CredentialBlob,
- cred->CredentialBlobSize / sizeof(WCHAR));
+ size_t wlen = cred->CredentialBlobSize / sizeof(WCHAR);
+ WCHAR *cred_password = xmalloc((wlen + 1) * sizeof(WCHAR));
+ wcsncpy_s(cred_password, wlen + 1,
+ (LPCWSTR)cred->CredentialBlob, wlen);
ret = !wcscmp(cred_password, password);
free(cred_password);
return ret;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/2] Some wincred fixes
From: Johannes Schindelin via GitGitGadget @ 2026-07-16 14:27 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
These were rolled out as part of the security fix release Git for Windows
v2.55.0(3).
Johannes Schindelin (2):
wincred: avoid memory corruption when erasing a credential
wincred: prevent silent credential loss when storing OAuth tokens
contrib/credential/wincred/git-credential-wincred.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2182%2Fdscho%2Fwincred-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2182/dscho/wincred-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2182
--
gitgitgadget
^ permalink raw reply
* [PATCH v6 2/2] fetch: add fetch.submoduleErrors to make submodule fetch errors non-fatal
From: Paulius Zaleckas @ 2026-07-16 14:09 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ramsay Jones, Paulius Zaleckas,
Jean-Noël Avila, Ævar Arnfjörð Bjarmason,
Glen Choo, Patrick Steinhardt
In-Reply-To: <20260716140956.1023740-1-paulius.zaleckas@gmail.com>
When fetching with --recurse-submodules, a submodule commit that is not
yet reachable from any of the submodule's remote refs causes the entire
fetch to fail. This is overly strict when the missing commit belongs to
an upstream branch that is still being prepared (e.g. an in-progress
merge topic): the local branch does not need that commit, so there is no
reason to treat its absence as fatal.
Add a new config key fetch.submoduleErrors (values: fail/warn) and a
corresponding --submodule-errors=(fail|warn) command-line option that
control this behaviour. The default remains fail (existing behaviour);
setting the value to warn causes submodule fetch failures to be reported
on stderr without affecting the overall exit status of git fetch / git
pull.
Forward the option to child fetches in add_options_to_argv() so that it
also takes effect for `git fetch --all` / `--multiple` (where per-remote
child processes handle the submodule recursion themselves) and for
nested submodule recursion. The resolved value is forwarded whenever it
was set explicitly, in either direction: the per-remote children re-read
the repository configuration, so a command-line --submodule-errors=fail
must be passed down to them to override fetch.submoduleErrors=warn from
the configuration. When neither the configuration nor the command line
sets a value, nothing is forwarded and the child processes fall back to
their own configuration.
Helped-by: Jean-Noël Avila <avila.jn@gmail.com>
Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Paulius Zaleckas <paulius.zaleckas@gmail.com>
---
Documentation/config/fetch.adoc | 14 +++++
Documentation/fetch-options.adoc | 8 +++
builtin/fetch.c | 70 ++++++++++++++++++++++++-
submodule.c | 8 ++-
submodule.h | 7 ++-
t/t5526-fetch-submodules.sh | 89 ++++++++++++++++++++++++++++++++
6 files changed, 192 insertions(+), 4 deletions(-)
diff --git a/Documentation/config/fetch.adoc b/Documentation/config/fetch.adoc
index 04ac90912d..5c9c942a70 100644
--- a/Documentation/config/fetch.adoc
+++ b/Documentation/config/fetch.adoc
@@ -10,6 +10,20 @@
reference.
Defaults to `on-demand`, or to the value of `submodule.recurse` if set.
+`fetch.submoduleErrors`::
+ Controls how errors from submodule fetches are handled when
+ `--recurse-submodules` is in effect. When set to `fail` (the default),
+ any submodule fetch error causes the overall `git fetch` or `git pull`
+ to exit with a non-zero status. When set to `warn`, submodule fetch
+ errors are reported to standard error but do not affect the exit
+ status of the command. This is useful when working in repositories
+ where some branches reference submodule commits that are not yet
+ available on the submodule remote, but those commits are not needed
+ for the currently checked-out branch.
++
+The value of this option can be overridden by the `--submodule-errors`
+option of linkgit:git-fetch[1].
+
`fetch.fsckObjects`::
If it is set to true, git-fetch-pack will check all fetched
objects. See `transfer.fsckObjects` for what's
diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
index 035f780e58..78525f6848 100644
--- a/Documentation/fetch-options.adoc
+++ b/Documentation/fetch-options.adoc
@@ -294,6 +294,14 @@ ifndef::git-pull[]
`--no-recurse-submodules`::
Disable recursive fetching of submodules (this has the same effect as
using the `--recurse-submodules=no` option).
+
+`--submodule-errors=(fail|warn)`::
+ Control how errors from submodule fetches are handled when
+ `--recurse-submodules` is in effect. When set to `fail` (the default),
+ any submodule fetch error causes the overall `git fetch` to exit with a
+ non-zero status. When set to `warn`, submodule fetch errors are reported
+ to standard error but do not affect the exit status of the command. Can
+ also be configured via `fetch.submoduleErrors`. See linkgit:git-config[1].
endif::git-pull[]
`--set-upstream`::
diff --git a/builtin/fetch.c b/builtin/fetch.c
index c1d7c672f4..2c583ed0cc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -110,8 +110,30 @@ struct fetch_config {
int recurse_submodules;
int parallel;
int submodule_fetch_jobs;
+ int submodule_errors;
};
+/* really private - use accessors below to parse and format */
+static const char *submodule_error_name[] = {
+ [SUBMODULE_ERRORS_FAIL] = "fail",
+ [SUBMODULE_ERRORS_WARN] = "warn",
+};
+
+static const char *submodule_error(unsigned num)
+{
+ if (ARRAY_SIZE(submodule_error_name) <= num)
+ BUG("invalid submodule errors mode %u", num);
+ return submodule_error_name[num];
+}
+
+static int parse_submodule_error(const char *name)
+{
+ for (unsigned num = 0; num < ARRAY_SIZE(submodule_error_name); num++)
+ if (!strcmp(submodule_error_name[num], name))
+ return num;
+ return -1;
+}
+
static int git_fetch_config(const char *k, const char *v,
const struct config_context *ctx, void *cb)
{
@@ -152,6 +174,19 @@ static int git_fetch_config(const char *k, const char *v,
return 0;
}
+ if (!strcmp(k, "fetch.submoduleerrors")) {
+ int mode;
+
+ if (!v)
+ return config_error_nonbool(k);
+ mode = parse_submodule_error(v);
+ if (mode < 0)
+ die(_("invalid value for '%s': '%s'"),
+ "fetch.submoduleErrors", v);
+ fetch_config->submodule_errors = mode;
+ return 0;
+ }
+
if (!strcmp(k, "fetch.parallel")) {
fetch_config->parallel = git_config_int(k, v, ctx->kvi);
if (fetch_config->parallel < 0)
@@ -2205,6 +2240,9 @@ static void add_options_to_argv(struct strvec *argv,
strvec_push(argv, "--no-recurse-submodules");
else if (config->recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
strvec_push(argv, "--recurse-submodules=on-demand");
+ if (config->submodule_errors != -1)
+ strvec_pushf(argv, "--submodule-errors=%s",
+ submodule_error(config->submodule_errors));
if (tags == TAGS_SET)
strvec_push(argv, "--tags");
else if (tags == TAGS_UNSET)
@@ -2464,6 +2502,23 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
return exit_code;
}
+static int option_parse_submodule_errors(const struct option *opt,
+ const char *arg, int unset)
+{
+ int *v = opt->value;
+ int mode;
+
+ if (unset) {
+ *v = SUBMODULE_ERRORS_FAIL;
+ return 0;
+ }
+ mode = parse_submodule_error(arg);
+ if (mode < 0)
+ die(_("invalid value for '%s': '%s'"), "--submodule-errors", arg);
+ *v = mode;
+ return 0;
+}
+
int cmd_fetch(int argc,
const char **argv,
const char *prefix,
@@ -2477,6 +2532,7 @@ int cmd_fetch(int argc,
.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,
.parallel = 1,
.submodule_fetch_jobs = -1,
+ .submodule_errors = -1, /* unset */
};
const char *submodule_prefix = "";
const char *bundle_uri;
@@ -2491,6 +2547,7 @@ int cmd_fetch(int argc,
int max_jobs = -1;
int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
+ int submodule_errors_cli = -1; /* -1: not set on command line */
int fetch_write_commit_graph = -1;
int stdin_refspecs = 0;
int negotiate_only = 0;
@@ -2527,6 +2584,10 @@ int cmd_fetch(int argc,
OPT_CALLBACK_F(0, "recurse-submodules", &recurse_submodules_cli, N_("on-demand"),
N_("control recursive fetching of submodules"),
PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
+ OPT_CALLBACK_F(0, "submodule-errors", &submodule_errors_cli,
+ N_("(fail|warn)"),
+ N_("control how submodule fetch errors are handled"),
+ 0, option_parse_submodule_errors),
OPT_BOOL(0, "dry-run", &dry_run,
N_("dry run")),
OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
@@ -2616,6 +2677,9 @@ int cmd_fetch(int argc,
if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
config.recurse_submodules = recurse_submodules_cli;
+ if (submodule_errors_cli != -1)
+ config.submodule_errors = submodule_errors_cli;
+
if (negotiate_only) {
switch (recurse_submodules_cli) {
case RECURSE_SUBMODULES_OFF:
@@ -2819,11 +2883,14 @@ int cmd_fetch(int argc,
if (!result && remote && (config.recurse_submodules != RECURSE_SUBMODULES_OFF)) {
struct strvec options = STRVEC_INIT;
int max_children = max_jobs;
+ int submodule_errors = config.submodule_errors;
if (max_children < 0)
max_children = config.submodule_fetch_jobs;
if (max_children < 0)
max_children = config.parallel;
+ if (submodule_errors < 0)
+ submodule_errors = SUBMODULE_ERRORS_FAIL;
add_options_to_argv(&options, &config);
trace2_region_enter_printf("fetch", "recurse-submodule", the_repository, "%s", submodule_prefix);
@@ -2833,7 +2900,8 @@ int cmd_fetch(int argc,
config.recurse_submodules,
recurse_submodules_default,
verbosity < 0,
- max_children);
+ max_children,
+ submodule_errors);
trace2_region_leave_printf("fetch", "recurse-submodule", the_repository, "%s", submodule_prefix);
strvec_clear(&options);
}
diff --git a/submodule.c b/submodule.c
index 8bcef68a42..da4ace751f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1409,6 +1409,7 @@ struct submodule_parallel_fetch {
int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
struct strbuf submodules_with_errors;
+ int submodule_errors;
};
#define SPF_INIT { \
.args = STRVEC_INIT, \
@@ -1565,7 +1566,8 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
static void record_fetch_error(struct submodule_parallel_fetch *spf,
const char *name)
{
- spf->result = 1;
+ if (spf->submodule_errors == SUBMODULE_ERRORS_FAIL)
+ spf->result = 1;
strbuf_addf(&spf->submodules_with_errors, "\t%s\n", name);
}
@@ -1851,7 +1853,8 @@ int fetch_submodules(struct repository *r,
const struct strvec *options,
const char *prefix, int command_line_option,
int default_option,
- int quiet, int max_parallel_jobs)
+ int quiet, int max_parallel_jobs,
+ int submodule_errors)
{
struct submodule_parallel_fetch spf = SPF_INIT;
const struct run_process_parallel_opts opts = {
@@ -1871,6 +1874,7 @@ int fetch_submodules(struct repository *r,
spf.default_option = default_option;
spf.quiet = quiet;
spf.prefix = prefix;
+ spf.submodule_errors = submodule_errors;
if (!r->worktree)
goto out;
diff --git a/submodule.h b/submodule.h
index b10e16e6c0..c80b687d2a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -90,12 +90,17 @@ int should_update_submodules(void);
*/
const struct submodule *submodule_from_ce(const struct cache_entry *ce);
void check_for_new_submodule_commits(struct object_id *oid);
+/* Values for the submodule_errors parameter of fetch_submodules(). */
+#define SUBMODULE_ERRORS_FAIL 0 /* submodule fetch errors are fatal (default) */
+#define SUBMODULE_ERRORS_WARN 1 /* submodule fetch errors are non-fatal warnings */
+
int fetch_submodules(struct repository *r,
const struct strvec *options,
const char *prefix,
int command_line_option,
int default_option,
- int quiet, int max_parallel_jobs);
+ int quiet, int max_parallel_jobs,
+ int submodule_errors);
unsigned is_submodule_modified(const char *path, int ignore_untracked);
int submodule_uses_gitfile(const char *path);
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 7ad274ce04..19d17440cf 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -1307,6 +1307,57 @@ test_expect_success 'setup for submodule fetch error tests' '
git config --global protocol.file.allow always
'
+test_expect_success 'fetch --recurse-submodules fails when submodule commit is unreachable (default)' '
+ test_when_finished "rm -fr env_default" &&
+ create_err_env env_default &&
+ push_unreachable_commit env_default &&
+ test_must_fail git -C env_default/clone fetch --recurse-submodules 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success 'fetch.submoduleErrors=warn: unreachable submodule commit is non-fatal' '
+ test_when_finished "rm -fr env_warn_cfg" &&
+ create_err_env env_warn_cfg &&
+ push_unreachable_commit env_warn_cfg &&
+ git -C env_warn_cfg/clone -c fetch.submoduleErrors=warn \
+ fetch --recurse-submodules 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success '--submodule-errors=warn: unreachable submodule commit is non-fatal' '
+ test_when_finished "rm -fr env_warn_cli" &&
+ create_err_env env_warn_cli &&
+ push_unreachable_commit env_warn_cli &&
+ git -C env_warn_cli/clone fetch --recurse-submodules \
+ --submodule-errors=warn 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success '--submodule-errors=fail: unreachable submodule commit is fatal' '
+ test_when_finished "rm -fr env_fail_cli" &&
+ create_err_env env_fail_cli &&
+ push_unreachable_commit env_fail_cli &&
+ test_must_fail git -C env_fail_cli/clone fetch --recurse-submodules \
+ --submodule-errors=fail 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success 'fetch.submoduleErrors=warn does not suppress successful fetch' '
+ # A new reachable submodule commit (pushed to sub_bare) should be
+ # fetched without any error summary.
+ test_when_finished "rm -fr env_ok" &&
+ create_err_env env_ok &&
+ test_commit -C env_ok/sub_work reachable_ok &&
+ git -C env_ok/sub_work push &&
+ git -C env_ok/super_work submodule update --remote &&
+ git -C env_ok/super_work add sub &&
+ git -C env_ok/super_work commit -m "point sub to reachable commit" &&
+ git -C env_ok/super_work push &&
+ git -C env_ok/clone -c fetch.submoduleErrors=warn \
+ fetch --recurse-submodules 2>err &&
+ test_grep ! "Errors during submodule fetch" err
+'
+
test_expect_success 'failed submodule fetch is fatal even when its commits are present locally' '
# Create the same commit (unreferenced, via commit-tree with fixed
# dates) in both super_work/sub and clone/sub, point the gitlink at
@@ -1334,4 +1385,42 @@ test_expect_success 'failed submodule fetch is fatal even when its commits are p
test_grep "Errors during submodule fetch" err
'
+test_expect_success '--submodule-errors=warn is honored by fetch --all' '
+ # A second remote forces fetch_multiple(), which hands the submodule
+ # recursion off to per-remote child processes; the option must be
+ # forwarded to them.
+ test_when_finished "rm -fr env_all" &&
+ create_err_env env_all &&
+ push_unreachable_commit env_all &&
+ git -C env_all/clone remote add second "$pwd/env_all/super_bare" &&
+ git -C env_all/clone fetch --all --recurse-submodules \
+ --submodule-errors=warn 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success '--submodule-errors=fail overrides warn config for fetch --all' '
+ # The per-remote child processes re-read the repository config, so
+ # the command-line override must be forwarded to them explicitly.
+ test_when_finished "rm -fr env_override" &&
+ create_err_env env_override &&
+ push_unreachable_commit env_override &&
+ git -C env_override/clone remote add second "$pwd/env_override/super_bare" &&
+ git -C env_override/clone config fetch.submoduleErrors warn &&
+ test_must_fail git -C env_override/clone fetch --all --recurse-submodules \
+ --submodule-errors=fail 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success 'fetch.submoduleErrors=warn: inaccessible submodule is non-fatal' '
+ test_when_finished "rm -fr env_access" &&
+ create_err_env env_access &&
+ rm env_access/clone/sub/.git &&
+ rm -r env_access/clone/.git/modules/sub &&
+ git -C env_access/clone -c fetch.submoduleErrors=warn \
+ fetch --recurse-submodules 2>err &&
+ test_grep "Could not access submodule" err &&
+ test_must_fail git -C env_access/clone fetch --recurse-submodules 2>err &&
+ test_grep "Could not access submodule" err
+'
+
test_done
--
2.54.0
^ permalink raw reply related
* [PATCH v6 1/2] submodule: fix premature failure in recursive submodule fetch
From: Paulius Zaleckas @ 2026-07-16 14:09 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ramsay Jones, Paulius Zaleckas, Elijah Newren,
Patrick Steinhardt, Jonathan Tan, Glen Choo
In-Reply-To: <20260716140956.1023740-1-paulius.zaleckas@gmail.com>
When git fetch --recurse-submodules encounters a failure fetching a
submodule's refs (phase 1), it immediately marks the overall operation
as failed, even though a subsequent OID-based fetch (phase 2) is about
to be attempted for any missing commits. If phase 2 succeeds, the
overall result should be success, but the prematurely set failure flag
makes it look like an error.
Restructure fetch_finish() so that a phase-1 failure does not record an
error immediately. Instead, the decision is deferred:
- If missing commits trigger a phase-2 (OID-based) retry and that
retry succeeds, no error is recorded.
- If the phase-2 retry also fails, the error is recorded then.
- If the submodule was fetched unconditionally (RECURSE_SUBMODULES_ON)
and is not in the changed list, a phase-1 failure is recorded right
away since there is no OID retry to fall back on.
- If phase 1 fails but all required commits are already present
locally, there is no retry to defer to; the failure is still
recorded, since the fetch itself went wrong (e.g. a transport
error) even though the wanted commits happen to be available.
This resolves the NEEDSWORK comment added by bd5e567dc7 (submodule:
explain first attempt failure clearly, 2019-03-13).
Extract the common error-recording logic into a helper
record_fetch_error() and use it in fetch_start_failure() and for the
"Could not access submodule" error in get_fetch_task_from_index() as
well; the latter now also lists the submodule in the final error
summary.
Add a test ensuring a failed submodule fetch is still reported when
the gitlinked commits happen to be present locally.
Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Paulius Zaleckas <paulius.zaleckas@gmail.com>
---
submodule.c | 52 +++++++++++++++++++--------
t/t5526-fetch-submodules.sh | 72 +++++++++++++++++++++++++++++++++++++
2 files changed, 110 insertions(+), 14 deletions(-)
diff --git a/submodule.c b/submodule.c
index fd91201a92..8bcef68a42 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1562,6 +1562,13 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
return NULL;
}
+static void record_fetch_error(struct submodule_parallel_fetch *spf,
+ const char *name)
+{
+ spf->result = 1;
+ strbuf_addf(&spf->submodules_with_errors, "\t%s\n", name);
+}
+
static struct fetch_task *
get_fetch_task_from_index(struct submodule_parallel_fetch *spf,
struct strbuf *err)
@@ -1599,7 +1606,7 @@ get_fetch_task_from_index(struct submodule_parallel_fetch *spf,
ce->name);
if (S_ISGITLINK(ce->ce_mode) &&
!is_empty_dir(empty_submodule_path.buf)) {
- spf->result = 1;
+ record_fetch_error(spf, ce->name);
strbuf_addf(err,
_("Could not access submodule '%s'\n"),
ce->name);
@@ -1753,7 +1760,7 @@ static int fetch_start_failure(struct strbuf *err UNUSED,
struct submodule_parallel_fetch *spf = cb;
struct fetch_task *task = task_cb;
- spf->result = 1;
+ record_fetch_error(spf, task->sub->name);
fetch_task_free(task);
return 0;
@@ -1779,18 +1786,12 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
if (!task || !task->sub)
BUG("callback cookie bogus");
- if (retvalue) {
+ if (retvalue && task->commits) {
/*
- * NEEDSWORK: This indicates that the overall fetch
- * failed, even though there may be a subsequent fetch
- * by commit hash that might work. It may be a good
- * idea to not indicate failure in this case, and only
- * indicate failure if the subsequent fetch fails.
+ * This is the second pass (OID-based fetch) and it failed.
+ * The commits are genuinely unavailable from the remote.
*/
- spf->result = 1;
-
- strbuf_addf(&spf->submodules_with_errors, "\t%s\n",
- task->sub->name);
+ record_fetch_error(spf, task->sub->name);
}
/* Is this the second time we process this submodule? */
@@ -1798,9 +1799,17 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
goto out;
it = string_list_lookup(&spf->changed_submodule_names, task->sub->name);
- if (!it)
- /* Could be an unchanged submodule, not contained in the list */
+ if (!it) {
+ /*
+ * This submodule is not in the changed list (e.g. it was
+ * fetched because RECURSE_SUBMODULES_ON fetches all populated
+ * submodules). A phase 1 failure here has no OID-based retry
+ * to fall back on, so it is a genuine error.
+ */
+ if (retvalue)
+ record_fetch_error(spf, task->sub->name);
goto out;
+ }
cs_data = it->util;
oid_array_filter(&cs_data->new_commits,
@@ -1809,6 +1818,11 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
/* Are there commits we want, but do not exist? */
if (cs_data->new_commits.nr) {
+ /*
+ * Schedule an OID-based phase 2 fetch to retrieve the missing
+ * commits directly. Defer any error from phase 1: if phase 2
+ * succeeds, the overall operation should still succeed.
+ */
task->commits = &cs_data->new_commits;
ALLOC_GROW(spf->oid_fetch_tasks,
spf->oid_fetch_tasks_nr + 1,
@@ -1818,6 +1832,16 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
return 0;
}
+ /*
+ * All required commits are already present locally (they were either
+ * fetched by phase 1 or existed beforehand), so there is no phase 2
+ * retry to defer to. If phase 1 failed, the fetch itself went wrong
+ * (e.g. a transport error) and must still be reported, even though
+ * the gitlinked commits are available.
+ */
+ if (retvalue)
+ record_fetch_error(spf, task->sub->name);
+
out:
fetch_task_free(task);
return 0;
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1242ee9185..7ad274ce04 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -1262,4 +1262,76 @@ test_expect_success "fetch --all with --no-recurse-submodules only fetches super
! grep "Fetching submodule" fetch-log
'
+# Create an isolated environment for submodule fetch error tests.
+#
+# Sets up sub_bare (the submodule upstream), super_bare (the superproject
+# upstream), super_work (a working clone of super_bare with an initialized
+# submodule), and clone (a clone of super_bare with an initialized submodule
+# at a reachable commit). The caller can then create an unreachable commit
+# and push the superproject to put the clone one commit behind a state it
+# cannot fully fetch.
+#
+# Usage: create_err_env <envdir>
+create_err_env () {
+ local envdir="$1" &&
+ mkdir "$envdir" &&
+
+ git init --bare "$envdir/sub_bare" &&
+ git clone "$envdir/sub_bare" "$envdir/sub_work" &&
+ test_commit -C "$envdir/sub_work" "${envdir}_base" &&
+ git -C "$envdir/sub_work" push &&
+
+ git init --bare "$envdir/super_bare" &&
+ git clone "$envdir/super_bare" "$envdir/super_work" &&
+ git -C "$envdir/super_work" submodule add \
+ "$pwd/$envdir/sub_bare" sub &&
+ git -C "$envdir/super_work" commit -m "add submodule" &&
+ git -C "$envdir/super_work" push &&
+
+ git clone "$envdir/super_bare" "$envdir/clone" &&
+ git -C "$envdir/clone" submodule update --init
+}
+
+# Push a commit to <envdir>/super_bare that records a submodule SHA that is
+# present locally in super_work/sub but NOT pushed to sub_bare, making the
+# submodule commit unreachable from clone's sub remote.
+push_unreachable_commit () {
+ local envdir="$1" &&
+ git -C "$envdir/super_work/sub" commit --allow-empty -m "unreachable" &&
+ git -C "$envdir/super_work" add sub &&
+ git -C "$envdir/super_work" commit -m "point sub to unreachable commit" &&
+ git -C "$envdir/super_work" push
+}
+
+test_expect_success 'setup for submodule fetch error tests' '
+ git config --global protocol.file.allow always
+'
+
+test_expect_success 'failed submodule fetch is fatal even when its commits are present locally' '
+ # Create the same commit (unreferenced, via commit-tree with fixed
+ # dates) in both super_work/sub and clone/sub, point the gitlink at
+ # it, and break clone/sub'\''s remote. The commit exists in clone/sub
+ # but is unreachable, so the submodule stays in the changed list; the
+ # fetch failure must still be reported even though there is nothing
+ # left to fetch by commit hash.
+ test_when_finished "rm -fr env_phase1" &&
+ create_err_env env_phase1 &&
+ commit=$(GIT_AUTHOR_DATE="1234567890 +0000" \
+ GIT_COMMITTER_DATE="1234567890 +0000" \
+ git -C env_phase1/super_work/sub commit-tree \
+ "HEAD^{tree}" -p HEAD -m present) &&
+ present=$(GIT_AUTHOR_DATE="1234567890 +0000" \
+ GIT_COMMITTER_DATE="1234567890 +0000" \
+ git -C env_phase1/clone/sub commit-tree \
+ "HEAD^{tree}" -p HEAD -m present) &&
+ test "$commit" = "$present" &&
+ git -C env_phase1/super_work/sub checkout "$commit" &&
+ git -C env_phase1/super_work add sub &&
+ git -C env_phase1/super_work commit -m "gitlink to locally-present commit" &&
+ git -C env_phase1/super_work push &&
+ git -C env_phase1/clone/sub remote set-url origin "$pwd/env_phase1/missing" &&
+ test_must_fail git -C env_phase1/clone fetch --recurse-submodules 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
test_done
--
2.54.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox