* [PATCH v3 14/18] setup: stop using `the_repository` in `upgrade_repository_format()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Stop using `the_repository` in `upgrade_repository_format()` and instead
accept the repository as a parameter. The injection of `the_repository`
is thus bumped one level higher, where callers now pass it in
explicitly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
list-objects-filter-options.c | 2 +-
repository.h | 2 +-
setup.c | 6 +++---
worktree.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index cef67e5919..bc5d98f9e6 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -378,7 +378,7 @@ void partial_clone_register(
*/
return;
} else {
- if (upgrade_repository_format(1) < 0)
+ if (upgrade_repository_format(the_repository, 1) < 0)
die(_("unable to upgrade repository format to support partial clone"));
/* Add promisor config for the remote */
diff --git a/repository.h b/repository.h
index d391aff8ab..c3ec0f4b79 100644
--- a/repository.h
+++ b/repository.h
@@ -281,6 +281,6 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);
* Return 1 if upgrade repository format to target_version succeeded,
* 0 if no upgrade is necessary, and -1 when upgrade is not possible.
*/
-int upgrade_repository_format(int target_version);
+int upgrade_repository_format(struct repository *repo, int target_version);
#endif /* REPOSITORY_H */
diff --git a/setup.c b/setup.c
index 5dc27caf15..ed0c14e98e 100644
--- a/setup.c
+++ b/setup.c
@@ -811,7 +811,7 @@ static int check_repository_format_gently(struct repository *repo,
return 0;
}
-int upgrade_repository_format(int target_version)
+int upgrade_repository_format(struct repository *repo, int target_version)
{
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
@@ -819,7 +819,7 @@ int upgrade_repository_format(int target_version)
struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
int ret;
- repo_common_path_append(the_repository, &sb, "config");
+ repo_common_path_append(repo, &sb, "config");
read_repository_format(&repo_fmt, sb.buf);
strbuf_release(&sb);
@@ -841,7 +841,7 @@ int upgrade_repository_format(int target_version)
}
strbuf_addf(&repo_version, "%d", target_version);
- repo_config_set(the_repository, "core.repositoryformatversion", repo_version.buf);
+ repo_config_set(repo, "core.repositoryformatversion", repo_version.buf);
ret = 1;
diff --git a/worktree.c b/worktree.c
index d874e23b4e..988be84a30 100644
--- a/worktree.c
+++ b/worktree.c
@@ -1104,7 +1104,7 @@ void write_worktree_linking_files(const char *dotgit, const char *gitdir,
strbuf_realpath(&repo, repo.buf, 1);
if (use_relative_paths && !the_repository->repository_format_relative_worktrees) {
- if (upgrade_repository_format(1) < 0)
+ if (upgrade_repository_format(the_repository, 1) < 0)
die(_("unable to upgrade repository format to support relative worktrees"));
if (repo_config_set_gently(the_repository, "extensions.relativeWorktrees", "true"))
die(_("unable to set extensions.relativeWorktrees setting"));
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 13/18] setup: stop using `the_repository` in `setup_git_directory()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Stop using `the_repository` in `setup_git_directory()` and instead
accept the repository as a parameter. The injection of `the_repository`
is thus bumped one level higher, where callers now pass it in
explicitly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
archive.c | 2 +-
builtin/grep.c | 2 +-
builtin/hash-object.c | 2 +-
builtin/merge-file.c | 2 +-
builtin/rev-parse.c | 4 ++--
git.c | 2 +-
http-push.c | 2 +-
scalar.c | 4 ++--
setup.c | 4 ++--
setup.h | 2 +-
t/helper/test-advise.c | 2 +-
t/helper/test-bitmap.c | 2 +-
t/helper/test-bloom.c | 2 +-
t/helper/test-cache-tree.c | 2 +-
t/helper/test-config.c | 2 +-
t/helper/test-dump-cache-tree.c | 2 +-
t/helper/test-dump-fsmonitor.c | 2 +-
t/helper/test-dump-split-index.c | 2 +-
t/helper/test-dump-untracked-cache.c | 2 +-
t/helper/test-find-pack.c | 2 +-
t/helper/test-fsmonitor-client.c | 2 +-
t/helper/test-lazy-init-name-hash.c | 2 +-
t/helper/test-match-trees.c | 2 +-
t/helper/test-pack-deltas.c | 2 +-
t/helper/test-pack-mtimes.c | 2 +-
t/helper/test-partial-clone.c | 4 +++-
t/helper/test-path-walk.c | 2 +-
t/helper/test-reach.c | 2 +-
t/helper/test-read-cache.c | 2 +-
t/helper/test-read-graph.c | 2 +-
t/helper/test-read-midx.c | 2 +-
t/helper/test-ref-store.c | 2 +-
t/helper/test-revision-walking.c | 2 +-
t/helper/test-scrap-cache-tree.c | 2 +-
t/helper/test-serve-v2.c | 2 +-
t/helper/test-submodule-config.c | 2 +-
t/helper/test-submodule-nested-repo-config.c | 2 +-
t/helper/test-submodule.c | 10 +++++-----
t/helper/test-userdiff.c | 2 +-
t/helper/test-write-cache.c | 2 +-
40 files changed, 49 insertions(+), 47 deletions(-)
diff --git a/archive.c b/archive.c
index fcd474c682..51229107a5 100644
--- a/archive.c
+++ b/archive.c
@@ -786,7 +786,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
* die ourselves; but its error message will be more specific
* than what we could write here.
*/
- setup_git_directory();
+ setup_git_directory(the_repository);
}
parse_treeish_arg(argv, &args, remote);
diff --git a/builtin/grep.c b/builtin/grep.c
index 679f8b567a..560133feb8 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1064,7 +1064,7 @@ int cmd_grep(int argc,
use_index = 0;
else
/* die the same way as if we did it at the beginning */
- setup_git_directory();
+ setup_git_directory(the_repository);
}
/* Ignore --recurse-submodules if --no-index is given or implied */
if (!use_index)
diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index d7905bedc2..f306b0643f 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -100,7 +100,7 @@ int cmd_hash_object(int argc,
hash_object_usage, 0);
if (flags & INDEX_WRITE_OBJECT)
- prefix = setup_git_directory();
+ prefix = setup_git_directory(the_repository);
else
prefix = setup_git_directory_gently(the_repository, &nongit);
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 59a9792208..8fa5765239 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -110,7 +110,7 @@ int cmd_merge_file(int argc,
if (!repo && object_id)
/* emit the correct "not a git repo" error in this case */
- setup_git_directory();
+ setup_git_directory(the_repository);
for (i = 0; i < 3; i++) {
char *fname;
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 8fdb75413d..0a01ff7a75 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -739,7 +739,7 @@ int cmd_rev_parse(int argc,
/* No options; just report on whether we're in a git repo or not. */
if (argc == 1) {
- setup_git_directory();
+ setup_git_directory(the_repository);
repo_config(the_repository, git_default_config, NULL);
return 0;
}
@@ -774,7 +774,7 @@ int cmd_rev_parse(int argc,
/* The rest of the options require a git repository. */
if (!did_repo_setup) {
- prefix = setup_git_directory();
+ prefix = setup_git_directory(the_repository);
repo_config(the_repository, git_default_config, NULL);
did_repo_setup = 1;
diff --git a/git.c b/git.c
index 2cc018fc5c..42cd5f5b3c 100644
--- a/git.c
+++ b/git.c
@@ -477,7 +477,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
run_setup = RUN_SETUP_GENTLY;
if (run_setup & RUN_SETUP) {
- prefix = setup_git_directory();
+ prefix = setup_git_directory(the_repository);
no_repo = 0;
} else if (run_setup & RUN_SETUP_GENTLY) {
prefix = setup_git_directory_gently(the_repository, &no_repo);
diff --git a/http-push.c b/http-push.c
index d143fe2845..520d6c3b6a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1788,7 +1788,7 @@ int cmd_main(int argc, const char **argv)
if (delete_branch && rs.nr != 1)
die("You must specify only one branch name when deleting a remote branch");
- gitdir = setup_git_directory();
+ gitdir = setup_git_directory(the_repository);
memset(remote_dir_exists, -1, 256);
diff --git a/scalar.c b/scalar.c
index 4efb6ac36d..a80d8ee3ff 100644
--- a/scalar.c
+++ b/scalar.c
@@ -58,7 +58,7 @@ static void setup_enlistment_directory(int argc, const char **argv,
}
strbuf_setlen(&path, len);
- setup_git_directory();
+ setup_git_directory(the_repository);
if (!the_repository->worktree)
die(_("Scalar enlistments require a worktree"));
@@ -514,7 +514,7 @@ static int cmd_clone(int argc, const char **argv)
goto cleanup;
}
- setup_git_directory();
+ setup_git_directory(the_repository);
/* common-main already logs `argv` */
trace2_def_repo(the_repository);
diff --git a/setup.c b/setup.c
index c12c6cbda2..5dc27caf15 100644
--- a/setup.c
+++ b/setup.c
@@ -2136,9 +2136,9 @@ void check_repository_format(struct repository_format *fmt)
* directory is not a strict subdirectory of the work tree root. The
* prefix always ends with a '/' character.
*/
-const char *setup_git_directory(void)
+const char *setup_git_directory(struct repository *repo)
{
- return setup_git_directory_gently(the_repository, NULL);
+ return setup_git_directory_gently(repo, NULL);
}
const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)
diff --git a/setup.h b/setup.h
index 18092fbf16..b779661ce7 100644
--- a/setup.h
+++ b/setup.h
@@ -137,7 +137,7 @@ enum {
const char *enter_repo(struct repository *repo, const char *path, unsigned flags);
const char *setup_git_directory_gently(struct repository *repo, int *);
-const char *setup_git_directory(void);
+const char *setup_git_directory(struct repository *repo);
char *prefix_path(struct repository *repo, const char *prefix, int len, const char *path);
char *prefix_path_gently(struct repository *repo, const char *prefix, int len, int *remaining, const char *path);
diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
index 81ed93a05c..8f9db2693e 100644
--- a/t/helper/test-advise.c
+++ b/t/helper/test-advise.c
@@ -11,7 +11,7 @@ int cmd__advise_if_enabled(int argc, const char **argv)
if (argc != 2)
die("usage: %s <advice>", argv[0]);
- setup_git_directory();
+ setup_git_directory(the_repository);
repo_config(the_repository, git_default_config, NULL);
/*
diff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c
index 16a01669e4..d9b9a83b8f 100644
--- a/t/helper/test-bitmap.c
+++ b/t/helper/test-bitmap.c
@@ -37,7 +37,7 @@ static int bitmap_dump_pseudo_merge_objects(uint32_t n)
int cmd__bitmap(int argc, const char **argv)
{
- setup_git_directory();
+ setup_git_directory(the_repository);
if (argc == 2 && !strcmp(argv[1], "list-commits"))
return bitmap_list_commits();
diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c
index 3283544bd3..0c65befbf0 100644
--- a/t/helper/test-bloom.c
+++ b/t/helper/test-bloom.c
@@ -52,7 +52,7 @@ static const char *const bloom_usage = "\n"
int cmd__bloom(int argc, const char **argv)
{
- setup_git_directory();
+ setup_git_directory(the_repository);
if (argc < 2)
usage(bloom_usage);
diff --git a/t/helper/test-cache-tree.c b/t/helper/test-cache-tree.c
index ff61d0ca7e..d42e260092 100644
--- a/t/helper/test-cache-tree.c
+++ b/t/helper/test-cache-tree.c
@@ -33,7 +33,7 @@ int cmd__cache_tree(int argc, const char **argv)
OPT_END()
};
- setup_git_directory();
+ setup_git_directory(the_repository);
argc = parse_options(argc, argv, NULL, options, test_cache_tree_usage, 0);
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 9f8cca7c48..cfb3f4b111 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -102,7 +102,7 @@ int cmd__config(int argc, const char **argv)
return 0;
}
- setup_git_directory();
+ setup_git_directory(the_repository);
git_configset_init(&cs);
diff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c
index 3f0c7d0ed0..ccb41a4239 100644
--- a/t/helper/test-dump-cache-tree.c
+++ b/t/helper/test-dump-cache-tree.c
@@ -66,7 +66,7 @@ int cmd__dump_cache_tree(int ac UNUSED, const char **av UNUSED)
struct cache_tree *another = cache_tree();
int ret;
- setup_git_directory();
+ setup_git_directory(the_repository);
if (repo_read_index(the_repository) < 0)
die("unable to read index file");
istate = *the_repository->index;
diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
index efd017ca35..c991cbbb8a 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -9,7 +9,7 @@ int cmd__dump_fsmonitor(int ac UNUSED, const char **av UNUSED)
{
struct index_state *istate = the_repository->index;
- setup_git_directory();
+ setup_git_directory(the_repository);
if (do_read_index(istate, the_repository->index_file, 0) < 0)
die("unable to read index file");
if (!istate->fsmonitor_last_update) {
diff --git a/t/helper/test-dump-split-index.c b/t/helper/test-dump-split-index.c
index f855a3862c..aae0a40a74 100644
--- a/t/helper/test-dump-split-index.c
+++ b/t/helper/test-dump-split-index.c
@@ -17,7 +17,7 @@ int cmd__dump_split_index(int ac UNUSED, const char **av)
{
struct split_index *si;
- setup_git_directory();
+ setup_git_directory(the_repository);
do_read_index(the_repository->index, av[1], 1);
printf("own %s\n", oid_to_hex(&the_repository->index->oid));
diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index 01a109496b..24308bd371 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -54,7 +54,7 @@ int cmd__dump_untracked_cache(int ac UNUSED, const char **av UNUSED)
xsetenv("GIT_CONFIG_KEY_0", "core.untrackedCache", 1);
xsetenv("GIT_CONFIG_VALUE_0", "keep", 1);
- setup_git_directory();
+ setup_git_directory(the_repository);
if (repo_read_index(the_repository) < 0)
die("unable to read index file");
uc = the_repository->index->untracked;
diff --git a/t/helper/test-find-pack.c b/t/helper/test-find-pack.c
index fc4b8a77b3..28d5b1fe09 100644
--- a/t/helper/test-find-pack.c
+++ b/t/helper/test-find-pack.c
@@ -25,7 +25,7 @@ int cmd__find_pack(int argc, const char **argv)
struct object_id oid;
struct packed_git *p;
int count = -1, actual_count = 0;
- const char *prefix = setup_git_directory();
+ const char *prefix = setup_git_directory(the_repository);
struct option options[] = {
OPT_INTEGER('c', "check-count", &count, "expected number of packs"),
diff --git a/t/helper/test-fsmonitor-client.c b/t/helper/test-fsmonitor-client.c
index 02bfe92e8d..dc1dff23fb 100644
--- a/t/helper/test-fsmonitor-client.c
+++ b/t/helper/test-fsmonitor-client.c
@@ -210,7 +210,7 @@ int cmd__fsmonitor_client(int argc, const char **argv)
subcmd = argv[0];
- setup_git_directory();
+ setup_git_directory(the_repository);
if (!strcmp(subcmd, "query"))
return !!do_send_query(token);
diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c
index 40f5df4412..e542985c94 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -211,7 +211,7 @@ int cmd__lazy_init_name_hash(int argc, const char **argv)
const char *prefix;
uint64_t avg_single, avg_multi;
- prefix = setup_git_directory();
+ prefix = setup_git_directory(the_repository);
argc = parse_options(argc, argv, prefix, options, usage, 0);
diff --git a/t/helper/test-match-trees.c b/t/helper/test-match-trees.c
index 2ed064b971..006ce5278e 100644
--- a/t/helper/test-match-trees.c
+++ b/t/helper/test-match-trees.c
@@ -13,7 +13,7 @@ int cmd__match_trees(int ac UNUSED, const char **av)
struct object_id hash1, hash2, shifted;
struct tree *one, *two;
- setup_git_directory();
+ setup_git_directory(the_repository);
if (repo_get_oid(the_repository, av[1], &hash1))
die("cannot parse %s as an object name", av[1]);
diff --git a/t/helper/test-pack-deltas.c b/t/helper/test-pack-deltas.c
index 4981401eaa..c493b75e02 100644
--- a/t/helper/test-pack-deltas.c
+++ b/t/helper/test-pack-deltas.c
@@ -95,7 +95,7 @@ int cmd__pack_deltas(int argc, const char **argv)
if (argc || num_objects < 0)
usage_with_options(usage_str, options);
- setup_git_directory();
+ setup_git_directory(the_repository);
f = hashfd(the_repository->hash_algo, 1, "<stdout>");
write_pack_header(f, num_objects);
diff --git a/t/helper/test-pack-mtimes.c b/t/helper/test-pack-mtimes.c
index 7a8ee1de24..b774056799 100644
--- a/t/helper/test-pack-mtimes.c
+++ b/t/helper/test-pack-mtimes.c
@@ -32,7 +32,7 @@ int cmd__pack_mtimes(int argc, const char **argv)
struct strbuf buf = STRBUF_INIT;
struct packed_git *p;
- setup_git_directory();
+ setup_git_directory(the_repository);
if (argc != 2)
usage(pack_mtimes_usage);
diff --git a/t/helper/test-partial-clone.c b/t/helper/test-partial-clone.c
index d848800749..a7aab426d0 100644
--- a/t/helper/test-partial-clone.c
+++ b/t/helper/test-partial-clone.c
@@ -1,3 +1,5 @@
+#define USE_THE_REPOSITORY_VARIABLE
+
#include "test-tool.h"
#include "hex.h"
#include "repository.h"
@@ -32,7 +34,7 @@ static void object_info(const char *gitdir, const char *oid_hex)
int cmd__partial_clone(int argc, const char **argv)
{
- setup_git_directory();
+ setup_git_directory(the_repository);
if (argc < 4)
die("too few arguments");
diff --git a/t/helper/test-path-walk.c b/t/helper/test-path-walk.c
index fe63002c2b..69676b15a5 100644
--- a/t/helper/test-path-walk.c
+++ b/t/helper/test-path-walk.c
@@ -89,7 +89,7 @@ int cmd__path_walk(int argc, const char **argv)
OPT_END(),
};
- setup_git_directory();
+ setup_git_directory(the_repository);
revs.repo = the_repository;
argc = parse_options(argc, argv, NULL,
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index 3131b54a87..5d86a96c17 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -39,7 +39,7 @@ int cmd__reach(int ac, const char **av)
struct strbuf buf = STRBUF_INIT;
struct repository *r = the_repository;
- setup_git_directory();
+ setup_git_directory(the_repository);
if (ac < 2)
exit(1);
diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index 9ae71cefb3..6b08ba8f07 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -19,7 +19,7 @@ int cmd__read_cache(int argc, const char **argv)
if (argc == 2)
cnt = strtol(argv[1], NULL, 0);
- setup_git_directory();
+ setup_git_directory(the_repository);
repo_config(the_repository, git_default_config, NULL);
for (i = 0; i < cnt; i++) {
diff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c
index 6a5f64e473..9f07b9c25a 100644
--- a/t/helper/test-read-graph.c
+++ b/t/helper/test-read-graph.c
@@ -76,7 +76,7 @@ int cmd__read_graph(int argc, const char **argv)
struct odb_source *source;
int ret = 0;
- setup_git_directory();
+ setup_git_directory(the_repository);
source = the_repository->objects->sources;
prepare_repo_settings(the_repository);
diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
index 388d29e2b5..790000fb26 100644
--- a/t/helper/test-read-midx.c
+++ b/t/helper/test-read-midx.c
@@ -14,7 +14,7 @@
static struct multi_pack_index *setup_midx(const char *object_dir)
{
struct odb_source *source;
- setup_git_directory();
+ setup_git_directory(the_repository);
source = odb_find_source(the_repository->objects, object_dir);
if (!source)
source = odb_add_to_alternates_memory(the_repository->objects,
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 74edf2029a..3866d0aca4 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -340,7 +340,7 @@ int cmd__ref_store(int argc UNUSED, const char **argv)
const char *func;
struct command *cmd;
- setup_git_directory();
+ setup_git_directory(the_repository);
argv = get_store(argv + 1, &refs);
diff --git a/t/helper/test-revision-walking.c b/t/helper/test-revision-walking.c
index 071f5bd1e2..70051eeaf8 100644
--- a/t/helper/test-revision-walking.c
+++ b/t/helper/test-revision-walking.c
@@ -56,7 +56,7 @@ int cmd__revision_walking(int argc, const char **argv)
if (argc < 2)
return 1;
- setup_git_directory();
+ setup_git_directory(the_repository);
if (!strcmp(argv[1], "run-twice")) {
printf("1st\n");
diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index 64fff6e9e3..7b5ce501d9 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -12,7 +12,7 @@ int cmd__scrap_cache_tree(int ac UNUSED, const char **av UNUSED)
{
struct lock_file index_lock = LOCK_INIT;
- setup_git_directory();
+ setup_git_directory(the_repository);
repo_hold_locked_index(the_repository, &index_lock, LOCK_DIE_ON_ERROR);
if (repo_read_index(the_repository) < 0)
die("unable to read index file");
diff --git a/t/helper/test-serve-v2.c b/t/helper/test-serve-v2.c
index 63a200b8d4..27f3ed8947 100644
--- a/t/helper/test-serve-v2.c
+++ b/t/helper/test-serve-v2.c
@@ -23,7 +23,7 @@ int cmd__serve_v2(int argc, const char **argv)
N_("exit immediately after advertising capabilities")),
OPT_END()
};
- const char *prefix = setup_git_directory();
+ const char *prefix = setup_git_directory(the_repository);
/* ignore all unknown cmdline switches for now */
argc = parse_options(argc, argv, prefix, options, serve_usage,
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index cbe93f2f9e..3f30292179 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -34,7 +34,7 @@ int cmd__submodule_config(int argc, const char **argv)
if (my_argc % 2 != 0)
die_usage(argc, argv, "Wrong number of arguments.");
- setup_git_directory();
+ setup_git_directory(the_repository);
while (*arg) {
struct object_id commit_oid;
diff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c
index 2710341cd5..7e31d3fe47 100644
--- a/t/helper/test-submodule-nested-repo-config.c
+++ b/t/helper/test-submodule-nested-repo-config.c
@@ -19,7 +19,7 @@ int cmd__submodule_nested_repo_config(int argc, const char **argv)
if (argc < 3)
die_usage(argv, "Wrong number of arguments.");
- setup_git_directory();
+ setup_git_directory(the_repository);
if (repo_submodule_init(&subrepo, the_repository, argv[1], null_oid(the_hash_algo))) {
die_usage(argv, "Submodule not found.");
diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
index 0133852e1e..3c5c4c4a09 100644
--- a/t/helper/test-submodule.c
+++ b/t/helper/test-submodule.c
@@ -99,7 +99,7 @@ static int cmd__submodule_is_active(int argc, const char **argv)
if (argc != 1)
usage_with_options(submodule_is_active_usage, options);
- setup_git_directory();
+ setup_git_directory(the_repository);
return !is_submodule_active(the_repository, argv[0]);
}
@@ -142,7 +142,7 @@ static int cmd__submodule_config_list(int argc, const char **argv)
argc = parse_options(argc, argv, "test-tools", options, usage,
PARSE_OPT_KEEP_ARGV0);
- setup_git_directory();
+ setup_git_directory(the_repository);
if (argc == 2)
return print_config_from_gitmodules(the_repository, argv[1]);
@@ -161,7 +161,7 @@ static int cmd__submodule_config_set(int argc, const char **argv)
argc = parse_options(argc, argv, "test-tools", options, usage,
PARSE_OPT_KEEP_ARGV0);
- setup_git_directory();
+ setup_git_directory(the_repository);
/* Equivalent to ACTION_SET in builtin/config.c */
if (argc == 3) {
@@ -183,7 +183,7 @@ static int cmd__submodule_config_unset(int argc, const char **argv)
NULL
};
- setup_git_directory();
+ setup_git_directory(the_repository);
if (argc == 2) {
if (!is_writing_gitmodules_ok())
@@ -202,7 +202,7 @@ static int cmd__submodule_config_writeable(int argc, const char **argv UNUSED)
"test-tool submodule config-writeable",
NULL
};
- setup_git_directory();
+ setup_git_directory(the_repository);
if (argc == 1)
return is_writing_gitmodules_ok() ? 0 : -1;
diff --git a/t/helper/test-userdiff.c b/t/helper/test-userdiff.c
index aa3a9894d2..fc34c589b3 100644
--- a/t/helper/test-userdiff.c
+++ b/t/helper/test-userdiff.c
@@ -40,7 +40,7 @@ int cmd__userdiff(int argc, const char **argv)
return error("unknown argument %s", argv[1]);
if (want & USERDIFF_DRIVER_TYPE_CUSTOM) {
- setup_git_directory();
+ setup_git_directory(the_repository);
repo_config(the_repository, cmd__userdiff_config, NULL);
}
diff --git a/t/helper/test-write-cache.c b/t/helper/test-write-cache.c
index b37dd2c5d6..98e1477c98 100644
--- a/t/helper/test-write-cache.c
+++ b/t/helper/test-write-cache.c
@@ -12,7 +12,7 @@ int cmd__write_cache(int argc, const char **argv)
int i, cnt = 1;
if (argc == 2)
cnt = strtol(argv[1], NULL, 0);
- setup_git_directory();
+ setup_git_directory(the_repository);
repo_read_index(the_repository);
for (i = 0; i < cnt; i++) {
repo_hold_locked_index(the_repository, &index_lock,
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 15/18] setup: stop using `the_repository` in `check_repository_format()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Stop using `the_repository` in `check_repository_format()` and instead
accept the repository as a parameter. The injection of `the_repository`
is thus bumped one level higher, where callers now pass it in
explicitly.
Furthermore, the function is never used outside "setup.c". Drop its
declaration in "setup.h" and make it static. Note that this requires us
to reorder the function.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 58 +++++++++++++++++++++++++++++++++-------------------------
setup.h | 10 ----------
2 files changed, 33 insertions(+), 35 deletions(-)
diff --git a/setup.c b/setup.c
index ed0c14e98e..406984b62c 100644
--- a/setup.c
+++ b/setup.c
@@ -1758,6 +1758,37 @@ enum discovery_result discover_git_directory_reason(struct strbuf *commondir,
return result;
}
+/*
+ * Check the repository format version in the path found in repo_get_git_dir(repo),
+ * and die if it is a version we don't understand. Generally one would
+ * set_git_dir() before calling this, and use it only for "are we in a valid
+ * repo?".
+ *
+ * If successful and fmt is not NULL, fill fmt with data.
+ */
+static void check_repository_format(struct repository *repo, struct repository_format *fmt)
+{
+ struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
+ if (!fmt)
+ fmt = &repo_fmt;
+ check_repository_format_gently(repo, repo_get_git_dir(repo), fmt, NULL);
+ startup_info->have_repository = 1;
+ repo_set_hash_algo(repo, fmt->hash_algo);
+ repo_set_compat_hash_algo(repo, fmt->compat_hash_algo);
+ repo_set_ref_storage_format(repo,
+ fmt->ref_storage_format,
+ fmt->ref_storage_payload);
+ repo->repository_format_worktree_config =
+ fmt->worktree_config;
+ repo->repository_format_submodule_path_cfg =
+ fmt->submodule_path_cfg;
+ repo->repository_format_relative_worktrees =
+ fmt->relative_worktrees;
+ repo->repository_format_partial_clone =
+ xstrdup_or_null(fmt->partial_clone);
+ clear_repository_format(&repo_fmt);
+}
+
const char *enter_repo(struct repository *repo, const char *path, unsigned flags)
{
static struct strbuf validated_path = STRBUF_INIT;
@@ -1832,7 +1863,7 @@ const char *enter_repo(struct repository *repo, const char *path, unsigned flags
if (is_git_directory(".")) {
set_git_dir(repo, ".", 0);
- check_repository_format(NULL);
+ check_repository_format(repo, NULL);
return path;
}
@@ -2107,29 +2138,6 @@ int git_config_perm(const char *var, const char *value)
return -(i & 0666);
}
-void check_repository_format(struct repository_format *fmt)
-{
- struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
- if (!fmt)
- fmt = &repo_fmt;
- check_repository_format_gently(the_repository, repo_get_git_dir(the_repository), fmt, NULL);
- startup_info->have_repository = 1;
- repo_set_hash_algo(the_repository, fmt->hash_algo);
- repo_set_compat_hash_algo(the_repository, fmt->compat_hash_algo);
- repo_set_ref_storage_format(the_repository,
- fmt->ref_storage_format,
- fmt->ref_storage_payload);
- the_repository->repository_format_worktree_config =
- fmt->worktree_config;
- the_repository->repository_format_submodule_path_cfg =
- fmt->submodule_path_cfg;
- the_repository->repository_format_relative_worktrees =
- fmt->relative_worktrees;
- the_repository->repository_format_partial_clone =
- xstrdup_or_null(fmt->partial_clone);
- clear_repository_format(&repo_fmt);
-}
-
/*
* Returns the "prefix", a path to the current working directory
* relative to the work tree root, or NULL, if the current working
@@ -2804,7 +2812,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
* config file, so this will not fail. What we are catching
* is an attempt to reinitialize new repository with an old tool.
*/
- check_repository_format(&repo_fmt);
+ check_repository_format(the_repository, &repo_fmt);
repository_format_configure(the_repository, &repo_fmt, hash, ref_storage_format);
diff --git a/setup.h b/setup.h
index b779661ce7..a820041af0 100644
--- a/setup.h
+++ b/setup.h
@@ -221,16 +221,6 @@ void clear_repository_format(struct repository_format *format);
int verify_repository_format(const struct repository_format *format,
struct strbuf *err);
-/*
- * Check the repository format version in the path found in repo_get_git_dir(the_repository),
- * and die if it is a version we don't understand. Generally one would
- * set_git_dir() before calling this, and use it only for "are we in a valid
- * repo?".
- *
- * If successful and fmt is not NULL, fill fmt with data.
- */
-void check_repository_format(struct repository_format *fmt);
-
const char *get_template_dir(const char *option_template);
#define INIT_DB_QUIET (1 << 0)
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 16/18] setup: stop using `the_repository` in `initialize_repository_version()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Stop using `the_repository` in `initialize_repository_version()` and
instead accept the repository as a parameter. The injection of
`the_repository` is thus bumped one level higher, where callers now pass
it in explicitly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/clone.c | 4 ++--
refs.c | 2 +-
setup.c | 29 +++++++++++++++--------------
setup.h | 3 ++-
4 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 8844e3d481..24fe0eead5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1229,7 +1229,7 @@ int cmd_clone(int argc,
*
* This is sufficient for Git commands to discover the Git directory.
*/
- initialize_repository_version(GIT_HASH_UNKNOWN,
+ initialize_repository_version(the_repository, GIT_HASH_UNKNOWN,
the_repository->ref_storage_format, 1);
refs_create_refdir_stubs(the_repository, git_dir, NULL);
@@ -1442,7 +1442,7 @@ int cmd_clone(int argc,
* ours to the same thing.
*/
hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
- initialize_repository_version(hash_algo, the_repository->ref_storage_format, 1);
+ initialize_repository_version(the_repository, hash_algo, the_repository->ref_storage_format, 1);
repo_set_hash_algo(the_repository, hash_algo);
create_reference_database(NULL, 1);
diff --git a/refs.c b/refs.c
index 844785219d..c36a322f4c 100644
--- a/refs.c
+++ b/refs.c
@@ -3453,7 +3453,7 @@ int repo_migrate_ref_storage_format(struct repository *repo,
* repository format so that clients will use the new ref store.
* We also need to swap out the repository's main ref store.
*/
- initialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1);
+ initialize_repository_version(the_repository, hash_algo_by_ptr(repo->hash_algo), format, 1);
/*
* Unset the old ref store and release it. `get_main_ref_store()` will
diff --git a/setup.c b/setup.c
index 406984b62c..e09483ba34 100644
--- a/setup.c
+++ b/setup.c
@@ -2385,7 +2385,8 @@ static int needs_work_tree_config(const char *git_dir, const char *work_tree)
return 1;
}
-void initialize_repository_version(int hash_algo,
+void initialize_repository_version(struct repository *repo,
+ int hash_algo,
enum ref_storage_format ref_storage_format,
int reinit)
{
@@ -2402,35 +2403,35 @@ void initialize_repository_version(int hash_algo,
*/
if (hash_algo != GIT_HASH_SHA1_LEGACY ||
ref_storage_format != REF_STORAGE_FORMAT_FILES ||
- the_repository->ref_storage_payload)
+ repo->ref_storage_payload)
target_version = GIT_REPO_VERSION_READ;
if (hash_algo != GIT_HASH_SHA1_LEGACY && hash_algo != GIT_HASH_UNKNOWN)
- repo_config_set(the_repository, "extensions.objectformat",
+ repo_config_set(repo, "extensions.objectformat",
hash_algos[hash_algo].name);
else if (reinit)
- repo_config_set_gently(the_repository, "extensions.objectformat", NULL);
+ repo_config_set_gently(repo, "extensions.objectformat", NULL);
- if (the_repository->ref_storage_payload) {
+ if (repo->ref_storage_payload) {
struct strbuf ref_uri = STRBUF_INIT;
strbuf_addf(&ref_uri, "%s://%s",
ref_storage_format_to_name(ref_storage_format),
- the_repository->ref_storage_payload);
- repo_config_set(the_repository, "extensions.refstorage", ref_uri.buf);
+ repo->ref_storage_payload);
+ repo_config_set(repo, "extensions.refstorage", ref_uri.buf);
strbuf_release(&ref_uri);
} else if (ref_storage_format != REF_STORAGE_FORMAT_FILES) {
- repo_config_set(the_repository, "extensions.refstorage",
+ repo_config_set(repo, "extensions.refstorage",
ref_storage_format_to_name(ref_storage_format));
} else if (reinit) {
- repo_config_set_gently(the_repository, "extensions.refstorage", NULL);
+ repo_config_set_gently(repo, "extensions.refstorage", NULL);
}
if (reinit) {
struct strbuf config = STRBUF_INIT;
struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
- repo_common_path_append(the_repository, &config, "config");
+ repo_common_path_append(repo, &config, "config");
read_repository_format(&repo_fmt, config.buf);
if (repo_fmt.v1_only_extensions.nr)
@@ -2440,17 +2441,17 @@ void initialize_repository_version(int hash_algo,
clear_repository_format(&repo_fmt);
}
- repo_config_get_bool(the_repository, "init.defaultSubmodulePathConfig",
+ repo_config_get_bool(repo, "init.defaultSubmodulePathConfig",
&default_submodule_path_config);
if (default_submodule_path_config) {
/* extensions.submodulepathconfig requires at least version 1 */
if (target_version == 0)
target_version = 1;
- repo_config_set(the_repository, "extensions.submodulepathconfig", "true");
+ repo_config_set(repo, "extensions.submodulepathconfig", "true");
}
strbuf_addf(&repo_version, "%d", target_version);
- repo_config_set(the_repository, "core.repositoryformatversion", repo_version.buf);
+ repo_config_set(repo, "core.repositoryformatversion", repo_version.buf);
strbuf_release(&repo_version);
}
@@ -2551,7 +2552,7 @@ static int create_default_files(struct repository *repo,
adjust_shared_perm(repo, repo_get_git_dir(repo));
}
- initialize_repository_version(fmt->hash_algo, fmt->ref_storage_format, reinit);
+ initialize_repository_version(repo, fmt->hash_algo, fmt->ref_storage_format, reinit);
/* Check filemode trustability */
repo_git_path_replace(repo, &path, "config");
diff --git a/setup.h b/setup.h
index a820041af0..c33b675ccf 100644
--- a/setup.h
+++ b/setup.h
@@ -232,7 +232,8 @@ int init_db(const char *git_dir, const char *real_git_dir,
enum ref_storage_format ref_storage_format,
const char *initial_branch, int init_shared_repository,
unsigned int flags);
-void initialize_repository_version(int hash_algo,
+void initialize_repository_version(struct repository *repo,
+ int hash_algo,
enum ref_storage_format ref_storage_format,
int reinit);
void create_reference_database(const char *initial_branch, int quiet);
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 17/18] setup: stop using `the_repository` in `create_reference_database()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Stop using `the_repository` in `create_reference_database()` and instead
accept the repository as a parameter. The injection of `the_repository`
is thus bumped one level higher, where callers now pass it in
explicitly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/clone.c | 2 +-
setup.c | 13 +++++++------
setup.h | 2 +-
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 24fe0eead5..53a41629e6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1444,7 +1444,7 @@ int cmd_clone(int argc,
hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
initialize_repository_version(the_repository, hash_algo, the_repository->ref_storage_format, 1);
repo_set_hash_algo(the_repository, hash_algo);
- create_reference_database(NULL, 1);
+ create_reference_database(the_repository, NULL, 1);
/*
* Before fetching from the remote, download and install bundle
diff --git a/setup.c b/setup.c
index e09483ba34..9c49319568 100644
--- a/setup.c
+++ b/setup.c
@@ -2468,13 +2468,14 @@ static int is_reinit(struct repository *repo)
return ret;
}
-void create_reference_database(const char *initial_branch, int quiet)
+void create_reference_database(struct repository *repo,
+ const char *initial_branch, int quiet)
{
struct strbuf err = STRBUF_INIT;
char *to_free = NULL;
- int reinit = is_reinit(the_repository);
+ int reinit = is_reinit(repo);
- if (ref_store_create_on_disk(get_main_ref_store(the_repository), 0, &err))
+ if (ref_store_create_on_disk(get_main_ref_store(repo), 0, &err))
die("failed to set up refs db: %s", err.buf);
/*
@@ -2486,14 +2487,14 @@ void create_reference_database(const char *initial_branch, int quiet)
if (!initial_branch)
initial_branch = to_free =
- repo_default_branch_name(the_repository, quiet);
+ repo_default_branch_name(repo, quiet);
ref = xstrfmt("refs/heads/%s", initial_branch);
if (check_refname_format(ref, 0) < 0)
die(_("invalid initial branch name: '%s'"),
initial_branch);
- if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", ref, NULL) < 0)
+ if (refs_update_symref(get_main_ref_store(repo), "HEAD", ref, NULL) < 0)
exit(1);
free(ref);
}
@@ -2830,7 +2831,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
&repo_fmt, init_shared_repository);
if (!(flags & INIT_DB_SKIP_REFDB))
- create_reference_database(initial_branch, flags & INIT_DB_QUIET);
+ create_reference_database(the_repository, initial_branch, flags & INIT_DB_QUIET);
create_object_directory(the_repository);
if (repo_settings_get_shared_repository(the_repository)) {
diff --git a/setup.h b/setup.h
index c33b675ccf..21737e9bd6 100644
--- a/setup.h
+++ b/setup.h
@@ -236,7 +236,7 @@ void initialize_repository_version(struct repository *repo,
int hash_algo,
enum ref_storage_format ref_storage_format,
int reinit);
-void create_reference_database(const char *initial_branch, int quiet);
+void create_reference_database(struct repository *repo, const char *initial_branch, int quiet);
/*
* NOTE NOTE NOTE!!
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 18/18] setup: stop using `the_repository` in `init_db()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Stop using `the_repository` in `init_db()` and instead accept
the repository as a parameter. The injection of `the_repository` is thus
bumped one level higher, where callers now pass it in explicitly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/clone.c | 2 +-
builtin/init-db.c | 2 +-
setup.c | 43 ++++++++++++++++++++++---------------------
setup.h | 3 ++-
4 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 53a41629e6..d60d1b60bc 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1186,7 +1186,7 @@ int cmd_clone(int argc,
* repository, and reference backends may persist that information into
* their on-disk data structures.
*/
- init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN,
+ init_db(the_repository, git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN,
ref_storage_format, NULL,
do_not_override_repo_unix_permissions, INIT_DB_QUIET | INIT_DB_SKIP_REFDB);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index e626b0d8b7..c55517ad94 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -252,7 +252,7 @@ int cmd_init_db(int argc,
}
flags |= INIT_DB_EXIST_OK;
- ret = init_db(git_dir, real_git_dir, template_dir, hash_algo,
+ ret = init_db(the_repository, git_dir, real_git_dir, template_dir, hash_algo,
ref_storage_format, initial_branch,
init_shared_repository, flags);
diff --git a/setup.c b/setup.c
index 9c49319568..6aee839d8c 100644
--- a/setup.c
+++ b/setup.c
@@ -2778,7 +2778,8 @@ static void repository_format_configure(struct repository *repo,
repo_fmt->ref_storage_payload);
}
-int init_db(const char *git_dir, const char *real_git_dir,
+int init_db(struct repository *repo,
+ const char *git_dir, const char *real_git_dir,
const char *template_dir, int hash,
enum ref_storage_format ref_storage_format,
const char *initial_branch,
@@ -2798,13 +2799,13 @@ int init_db(const char *git_dir, const char *real_git_dir,
if (!exist_ok && !stat(real_git_dir, &st))
die(_("%s already exists"), real_git_dir);
- set_git_dir(the_repository, real_git_dir, 1);
- git_dir = repo_get_git_dir(the_repository);
+ set_git_dir(repo, real_git_dir, 1);
+ git_dir = repo_get_git_dir(repo);
separate_git_dir(git_dir, original_git_dir);
}
else {
- set_git_dir(the_repository, git_dir, 1);
- git_dir = repo_get_git_dir(the_repository);
+ set_git_dir(repo, git_dir, 1);
+ git_dir = repo_get_git_dir(repo);
}
startup_info->have_repository = 1;
@@ -2814,27 +2815,27 @@ int init_db(const char *git_dir, const char *real_git_dir,
* config file, so this will not fail. What we are catching
* is an attempt to reinitialize new repository with an old tool.
*/
- check_repository_format(the_repository, &repo_fmt);
+ check_repository_format(repo, &repo_fmt);
- repository_format_configure(the_repository, &repo_fmt, hash, ref_storage_format);
+ repository_format_configure(repo, &repo_fmt, hash, ref_storage_format);
/*
* Ensure `core.hidedotfiles` is processed. This must happen after we
* have set up the repository format such that we can evaluate
* includeIf conditions correctly in the case of re-initialization.
*/
- repo_config(the_repository, git_default_core_config, NULL);
+ repo_config(repo, git_default_core_config, NULL);
- safe_create_dir(the_repository, git_dir, 0);
+ safe_create_dir(repo, git_dir, 0);
- reinit = create_default_files(the_repository, template_dir, original_git_dir,
+ reinit = create_default_files(repo, template_dir, original_git_dir,
&repo_fmt, init_shared_repository);
if (!(flags & INIT_DB_SKIP_REFDB))
- create_reference_database(the_repository, initial_branch, flags & INIT_DB_QUIET);
- create_object_directory(the_repository);
+ create_reference_database(repo, initial_branch, flags & INIT_DB_QUIET);
+ create_object_directory(repo);
- if (repo_settings_get_shared_repository(the_repository)) {
+ if (repo_settings_get_shared_repository(repo)) {
char buf[10];
/* We do not spell "group" and such, so that
* the configuration can be read by older version
@@ -2842,29 +2843,29 @@ int init_db(const char *git_dir, const char *real_git_dir,
* and compatibility values for PERM_GROUP and
* PERM_EVERYBODY.
*/
- if (repo_settings_get_shared_repository(the_repository) < 0)
+ if (repo_settings_get_shared_repository(repo) < 0)
/* force to the mode value */
- xsnprintf(buf, sizeof(buf), "0%o", -repo_settings_get_shared_repository(the_repository));
- else if (repo_settings_get_shared_repository(the_repository) == PERM_GROUP)
+ xsnprintf(buf, sizeof(buf), "0%o", -repo_settings_get_shared_repository(repo));
+ else if (repo_settings_get_shared_repository(repo) == PERM_GROUP)
xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_GROUP);
- else if (repo_settings_get_shared_repository(the_repository) == PERM_EVERYBODY)
+ else if (repo_settings_get_shared_repository(repo) == PERM_EVERYBODY)
xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_EVERYBODY);
else
BUG("invalid value for shared_repository");
- repo_config_set(the_repository, "core.sharedrepository", buf);
- repo_config_set(the_repository, "receive.denyNonFastforwards", "true");
+ repo_config_set(repo, "core.sharedrepository", buf);
+ repo_config_set(repo, "receive.denyNonFastforwards", "true");
}
if (!(flags & INIT_DB_QUIET)) {
int len = strlen(git_dir);
if (reinit)
- printf(repo_settings_get_shared_repository(the_repository)
+ printf(repo_settings_get_shared_repository(repo)
? _("Reinitialized existing shared Git repository in %s%s\n")
: _("Reinitialized existing Git repository in %s%s\n"),
git_dir, len && git_dir[len-1] != '/' ? "/" : "");
else
- printf(repo_settings_get_shared_repository(the_repository)
+ printf(repo_settings_get_shared_repository(repo)
? _("Initialized empty shared Git repository in %s%s\n")
: _("Initialized empty Git repository in %s%s\n"),
git_dir, len && git_dir[len-1] != '/' ? "/" : "");
diff --git a/setup.h b/setup.h
index 21737e9bd6..9409326fe4 100644
--- a/setup.h
+++ b/setup.h
@@ -227,7 +227,8 @@ const char *get_template_dir(const char *option_template);
#define INIT_DB_EXIST_OK (1 << 1)
#define INIT_DB_SKIP_REFDB (1 << 2)
-int init_db(const char *git_dir, const char *real_git_dir,
+int init_db(struct repository *repo,
+ const char *git_dir, const char *real_git_dir,
const char *template_dir, int hash_algo,
enum ref_storage_format ref_storage_format,
const char *initial_branch, int init_shared_repository,
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* Re: [PATCH v11] checkout: extend --track with a "fetch" mode to refresh start-point
From: Junio C Hamano @ 2026-05-19 10:34 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget
Cc: git, Ramsay Jones, D. Ben Knoble, Kristoffer Haugsbakk,
Marc Branchaud, Phillip Wood, Harald Nordgren
In-Reply-To: <pull.2281.v11.git.git.1779177508772.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> checkout: --track=fetch
>
> * Find the right remote by checking which remote's fetch refspec maps
> to the user's start-point, instead of assuming the start-point begins
> with the remote's name. This fixes cases where the user has a custom
> refspec mapping into a namespace whose name differs from the remote
> (e.g. fetching from origin into refs/remotes/upstream/*).
This comment is even before looking at the patch text. After
getting one issue pointed out, I'd expect you to think about related
issues before sending a new round out.
One. Have you considered the case where the remote-tracking refs
are overlapping, e.g., where "origin" and "upstream" point at
different URLs but they both store in "refs/remotes/upstream/*"?
Perhaps their URLs may textually be different but are pointing
logically at the same place (e.g., one ssh:// the other https:// for
example).
What should happen? What does happen after you apply this patch?
> * For a bare namespace name, follow <namespace>/HEAD first to figure
> out which branch to fetch.
What should happen if HEAD does not exist? What does happen after
you apply this patch?
Thanks.
^ permalink raw reply
* Re: [GSoC RFC PATCH 0/1] graph: add indentation for commits preceded by a root
From: Chandra Pratap @ 2026-05-19 10:39 UTC (permalink / raw)
To: Pablo Sabater
Cc: phillip.wood, git, gitster, christian.couder, karthik.188,
jltobler, ayu.chandekar, siddharthasthana31
In-Reply-To: <CAN5EUNQoKRqt3FGLmzRGpPU1nO5jCAogP8Wm9gBZXuPbMNbQAw@mail.gmail.com>
On Mon, 18 May 2026 at 18:57, Pablo Sabater <pabloosabaterr@gmail.com> wrote:
>
> Hi Chandra, Phillip,
>
> > > >
> > > > I have mixed feelings about which approach to choose.
> > > > The idea of a blank line was thought at
> > > > https://lore.kernel.org/git/xmqq8s8vvw9m.fsf@gitster.c.googlers.com/
> > > > but Junio argued against it for having an extra row because the
> > > > indentation he proposed didn't collapse, however I find indentation +
> > > > no collapse the most confusing one.
> > > > I'd say that I'm fine with both approaches, blank line or indentation
> > > > + collapse.
> > >
> > > I'm afraid I don't understand this - what does it mean for the
> > > indentation to collapse, or not collapse.
>
> Collapsing would be when branches move to the left, eg:
>
> *
> |\ <- merge
> | *
> |/ <- collapse
> *
> > > Looking at the examples Junio
> > > gave they look quite nice to me, though I'd find it clearer if
> > >
> > >
> > > | | * 12345678 2021-01-14 merge xxxxx@xxxx into the history
> > > | | |\
> > > | | | \
> > > | | * \ 23456789 2021-01-12 merge citest into the main history
> > > | | |\ * 5505e019c2 2014-07-09 initial xxxxxx@xxxx
> > > | | | * 3e658f4085 2019-09-10 (wiki/wip-citest, origin/wip-citest)
> > > Added defau
> > > | | | * ad148aafe6 2019-09-10 Added default CI/CD Jenkinsfile (from
> > > f7daf088)
> > >
> > > was rendered as
> > >
> > >
> > > | | * 12345678 2021-01-14 merge xxxxx@xxxx into the history
> > > | | |\
> > > | | | * 5505e019c2 2014-07-09 initial xxxxxx@xxxx
> > > | | * 23456789 2021-01-12 merge citest into the main history
> > > | | |\
> > > | | | * 3e658f4085 2019-09-10 (wiki/wip-citest, origin/wip-citest)
> > > Added defau
> > > | | | * ad148aafe6 2019-09-10 Added default CI/CD Jenkinsfile (from
> > > f7daf088)
> >
> > It probably *does* look clearer here, but I have the same reservations
> > against this as Junio: the break won't be as noticeable when --graph is
> > *not* used with --oneline.
> >
> > > >>> without the patch:
> > > >>>
> > > >>> * A root
> > > >>> * B root
> > > >>> * C root
> > > >>> * D1 child
> > > >>> * D root
> > > >>>
> > > >>> with the patch, the indentation cascades:
> > > >>>
> > > >>> * A root
> > > >>> * B root
> > > >>> * C root
> > > >>> * D1 child
> > > >>> _ /
> > > >>> /
> > > >>> /
> > > >>> * D root
> > > >
> > > > * A root
> > > >
> > > > * B root
> > > >
> > > > * C root
> > > >
> > > > * D1 child
> > > >
> > > > * D root
> > > >
> > > > Here I think a blank line looks worse, too much space for just 5
> > > > commits and becomes one extra line which if this were like up to 7 or
> > > > more parentless commits one after the other would be more noticeable.
> > >
> > > But there shouldn't be a blank line between D and D1 so the two
> > > alternatives take up the same amount of vertical space, the main
> > > difference being whether D1 appears next to D
> > >
> > > * A root * A root
> > > * B root
> > > * B root * C root
> > > * D1 child
> > > * C root _/
> > > /
> > > * D1 child /
> > > * D root * D root
> > >
> > > Of course if the indentation was smarter it would take up less room and
> > > look better than having blank lines
> > >
> > > * A root
> > > * B root
> > > * C root
> > > * D1 child
> > > * D root
> >
> > Right, this would be ideal but that would require too much change to the
> > existing graphing logic, and should be its own patch.
>
> For the examples I'll use the term parentless instead of root, as
> boundary commits are excluded even if they are roots.
> By having is_parentless as a flag in 'git_graph' that every stage can
> access we could modify the rendering and maybe completely drop the
> commit placeholders, working on it for v4 but currently renders like
> this
>
> * A parentless
> * B parentless
> * C parentless
> * D1 child
> * D parentless
>
> (A has indentation when it could not have, but that would require a
> lookahead if the next commit is also parentless)
> But definitely a step forward.
>
> Do we want cascading or just a fixed indentation?
>
> * A parentless
> * B parentless
> * C parentless
> * D1 child
> * D parentless
>
> By being indented it indicates that it is parentless and that the one
> below doesn't relate to it, but cascading looks clearer.
Agreed, let's keep the cascading.
> >
> > > > But there are cases that blank line might be better:
> > > >
> > > > * 10_A2
> > > > * 10_A1
> > > > * 10_A
> > > > * 10_M
> > > > /|\
> > > > | | * 10_D
> > > > | * 10_C
> > > > * 10_B
> > > >
> > > > Feels like a shower of commits instead of an indented merge.
> > >
> > > Yes, that is a bit confusing. I think the thing I find confusing with
> > > this approach is that we're treating the commit rendered below the root
> > > commit specially, rather than treating the root commit itself specially.
> > > To me it is the root commit that's the odd one out because it does not
> > > have any parents, but we treat the commit that's rendered below as the
> > > odd one by indenting it relative to its parents.
> >
> > I guess that would make the examples look something like this:
> >
> > * A root
> > * B root
> > * C root
> > * D1 child
> > * D root
> >
> > No cascading, and no need for that massive _ / collapse line.
> >
> > * 10_A2
> > * 10_A1
> > \
> > * 10_A
> > * 10_M
> > | \ \
> > | | * 10_D
> > | * 10_C
> > * 10_B
> >
> > I say it looks better than the alternatives, but I'm not sure if this will
> > be easy to implement. The diagonal connection line (\) will need to
> > be printed before printing the actual root commit, which will require
> > lookahead logic.
> >
> > I'd prefer to avoid major surgery on the codebase.
>
> Octopus merges need a pre-commit phase where an additional row
> increases the space around a commit with multiple parents to make room
> for it.
> A new phase can be created similarly to pre-commit as pre-root where
> the connection edge (\) can be printed before the indented commit.
>
> So far this is the comparison:
>
> indentation at root:
>
> * A parentless
> * B1 child
> \
> * B parentless
> * C1 child
> * C parentless
>
> indentation AFTER the root (current v3):
>
> * A parentless
> * B1 child
> /
> * B parentless
> * C1 child
> /
> * C parentless
>
> Karthik mentioned that by indenting the parentless, we lose the
> consistency of having the roots on their real column and now some are
> indented and some are not.
> The biggest winner having the parentless indented are the merge commits:
>
> * A child
> * A child
> \
> * A parentless
> *-. B child
> | \ \
> | | * C parentless
> | * D parentless
> * E parentless
>
> which IMO looks clearer than the commit shower:
>
> * A child
> * A child
> * A parentless
> * B child
> /|\
> | | * C parentless
> | * D parentless
> * E parentless
I guess we're deciding between cleaner root (parentless) commits and
cleaner merge commits. I favour merge commits because they appear
much more frequently in most codebases.
> >
> >
> > Thanks,
> > Chandra.
>
> Regards
>
> --
> Pablo
^ permalink raw reply
* Re: [PATCH v7] revision.c: implement --max-count-oldest
From: Mirko Faina @ 2026-05-19 10:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Jean-Noël Avila, Patrick Steinhardt,
Tian Yuchen, Ben Knoble, Johannes Sixt, Chris Torek, Mirko Faina
In-Reply-To: <xmqqse7n7w2i.fsf@gitster.g>
On Tue, May 19, 2026 at 03:27:49PM +0900, Junio C Hamano wrote:
> Mirko Faina <mroik@delayed.space> writes:
>
> > + * graph_update() as it doesn't do the actualy printing, we'd
>
> "actually"?
Whoops, it should be "actual".
Thank you
^ permalink raw reply
* [PATCH] stash: reuse cached index entries in --patch temporary index
From: Adam Johnson via GitGitGadget @ 2026-05-19 12:43 UTC (permalink / raw)
To: git
Cc: Thomas Gummerer, Elijah Newren, Phillip Wood, Victoria Dye,
Adam Johnson, Adam Johnson
From: Adam Johnson <me@adamj.eu>
`git stash -p` prepares the interactive selection by creating a
temporary index at HEAD, switching `GIT_INDEX_FILE` to it, and then
running the `add -p` machinery.
That temporary index was created by running `git read-tree HEAD`. The
resulting index had no useful cached stat data or fsmonitor-valid bits
from the real index. When `run_add_p()` refreshed that temporary index
before showing the first prompt, it could end up lstat(2)-ing every
tracked file, even in a repository where `git diff` and `git restore -p`
can use fsmonitor to avoid that work.
Create the temporary index in-process instead. Use `unpack_trees()` to
reset the real index contents to HEAD while writing the result to the
temporary index path. For paths whose index entries already match HEAD,
`oneway_merge()` reuses the existing cache entries, preserving their
cached stat data and `CE_FSMONITOR_VALID` state.
This makes the refresh performed by `run_add_p()` behave like the one
used by `git restore -p`: unchanged paths can be skipped via fsmonitor
instead of being scanned again.
In a 206k file repository with `core.fsmonitor` enabled and a one-line
change in one file, time to first prompt dropped from 34.774 seconds to
0.659 seconds.
Signed-off-by: Adam Johnson <me@adamj.eu>
---
stash: reuse cached index entries in --patch temporary index
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2306%2Fadamchainz%2Faj%2Foptimize-stash-patch-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2306/adamchainz/aj/optimize-stash-patch-v1
Pull-Request: https://github.com/git/git/pull/2306
builtin/stash.c | 71 ++++++++++++++++++++++++++++++++++++++----
t/t3904-stash-patch.sh | 18 +++++++++++
2 files changed, 83 insertions(+), 6 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index 32dbc97b47..48189cb9f7 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -372,6 +372,57 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
return 0;
}
+static int create_index_from_tree(const struct object_id *tree_id,
+ const char *index_path)
+{
+ int nr_trees = 1;
+ int ret = 0;
+ struct unpack_trees_options opts;
+ struct tree_desc t[MAX_UNPACK_TREES];
+ struct tree *tree;
+ struct index_state dst_istate = INDEX_STATE_INIT(the_repository);
+ struct lock_file lock_file = LOCK_INIT;
+
+ repo_read_index_preload(the_repository, NULL, 0);
+ if (refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL))
+ return -1;
+
+ hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
+
+ memset(&opts, 0, sizeof(opts));
+
+ tree = repo_parse_tree_indirect(the_repository, tree_id);
+ if (!tree || repo_parse_tree(the_repository, tree)) {
+ ret = -1;
+ goto done;
+ }
+
+ init_tree_desc(t, &tree->object.oid, tree->buffer, tree->size);
+
+ opts.head_idx = 1;
+ opts.src_index = the_repository->index;
+ opts.dst_index = &dst_istate;
+ opts.merge = 1;
+ opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
+ opts.fn = oneway_merge;
+
+ if (unpack_trees(nr_trees, t, &opts)) {
+ ret = -1;
+ goto done;
+ }
+
+ if (write_locked_index(&dst_istate, &lock_file, COMMIT_LOCK)) {
+ ret = error(_("unable to write new index file"));
+ goto done;
+ }
+
+done:
+ release_index(&dst_istate);
+ if (ret)
+ rollback_lock_file(&lock_file);
+ return ret;
+}
+
static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
{
struct child_process cp = CHILD_PROCESS_INIT;
@@ -1321,18 +1372,26 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
struct interactive_options *interactive_opts)
{
int ret = 0;
- struct child_process cp_read_tree = CHILD_PROCESS_INIT;
struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
+ struct commit *head_commit;
+ const struct object_id *head_tree;
struct index_state istate = INDEX_STATE_INIT(the_repository);
char *old_index_env = NULL, *old_repo_index_file;
remove_path(stash_index_path.buf);
- cp_read_tree.git_cmd = 1;
- strvec_pushl(&cp_read_tree.args, "read-tree", "HEAD", NULL);
- strvec_pushf(&cp_read_tree.env, "GIT_INDEX_FILE=%s",
- stash_index_path.buf);
- if (run_command(&cp_read_tree)) {
+ head_commit = lookup_commit(the_repository, &info->b_commit);
+ if (!head_commit || repo_parse_commit(the_repository, head_commit)) {
+ ret = -1;
+ goto done;
+ }
+ head_tree = get_commit_tree_oid(head_commit);
+ if (!head_tree) {
+ ret = -1;
+ goto done;
+ }
+
+ if (create_index_from_tree(head_tree, stash_index_path.buf)) {
ret = -1;
goto done;
}
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index 90a4ff2c10..4b3241c8cd 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -84,6 +84,24 @@ test_expect_success 'none of this moved HEAD' '
verify_saved_head
'
+test_expect_success 'stash -p with unmodified tracked files present' '
+ git reset --hard &&
+ echo line1 >alpha &&
+ echo line1 >beta &&
+ git add alpha beta &&
+ git commit -m "add alpha and beta" &&
+ echo line2 >>alpha &&
+ echo y | git stash -p &&
+ echo line1 >expect &&
+ test_cmp expect alpha &&
+ test_cmp expect beta &&
+ git stash pop &&
+ printf "line1\nline2\n" >expect &&
+ test_cmp expect alpha &&
+ echo line1 >expect &&
+ test_cmp expect beta
+'
+
test_expect_success 'stash -p with split hunk' '
git reset --hard &&
cat >test <<-\EOF &&
base-commit: 7bcaabddcf68bd0702697da5904c3b68c52f94cf
--
gitgitgadget
^ permalink raw reply related
* [PATCH] http: fix memory leak in fetch_and_setup_pack_index()
From: LorenzoPegorari @ 2026-05-19 14:54 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox, Jeff King
Inside the function `fetch_and_setup_pack_index()`, when the pack
obtained using `fetch_pack_index()` fails to be verified by
`parse_pack_index()`, the function returns without closing and freeing
said pack.
Fix this by calling `close_pack_index()` to munmap the index file for
the leaking pack (which might have been mmapped by `fetch_pack_index()`
or `verify_pack_index()`), and then free it.
Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
---
http.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/http.c b/http.c
index 67c9c6fc60..c28be26ad9 100644
--- a/http.c
+++ b/http.c
@@ -2545,11 +2545,13 @@ static int fetch_and_setup_pack_index(struct packfile_list *packs,
}
ret = verify_pack_index(new_pack);
- if (!ret)
- close_pack_index(new_pack);
+
+ close_pack_index(new_pack);
free(tmp_idx);
- if (ret)
+ if (ret) {
+ free(new_pack);
return -1;
+ }
packfile_list_prepend(packs, new_pack);
return 0;
--
2.54.0.dirty
^ permalink raw reply related
* Re: [PATCH v5 0/8] fetch: rework negotiation tip options
From: Derrick Stolee @ 2026-05-19 15:08 UTC (permalink / raw)
To: Matthew John Cheetham, Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps
In-Reply-To: <VI0PR03MB11634DBCD7ED97E3358AC9339C0002@VI0PR03MB11634.eurprd03.prod.outlook.com>
On 5/19/2026 5:13 AM, Matthew John Cheetham wrote:
> On 2026-05-18 21:19, Derrick Stolee via GitGitGadget wrote:
>
>> Range-diff vs v4:
>>
>> 1: 7409a479d6 ! 1: 538913a327 t5516: fix test order flakiness
>> @@ Commit message
>> Use 'sort -k 3' to match the actual number of columns in the output.
>> + Reviewed-by: Matthew John Cheetham <mcheetham@outlook.com>
>> Signed-off-by: Derrick Stolee <stolee@gmail.com>
>
> mjcheetham@outlook.com, not mcheetham@outlook.com.
OH NO! Sorry for this typo that I copied into every commit.
> LGTM
>
> Thanks,
> Matthew
I'll fix this reviewed-by tag and resend. Sorry :(
Thanks for the review!
^ permalink raw reply
* Re: [GSoC PATCH v6 0/6] preserve promisor files content after repack
From: Lorenzo Pegorari @ 2026-05-19 15:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Christian Couder, Taylor Blau, Derrick Stolee,
Patrick Steinhardt, Tian Yuchen, Eric Sunshine, Elijah Newren
In-Reply-To: <xmqqse7xm8av.fsf@gitster.g>
On Tue, May 12, 2026 at 03:49:28PM +0900, Junio C Hamano wrote:
> Lorenzo, it seems that not many people are reviewing this final
> round, and then I noticed that the list of CC addresses lacks a big
> name in the promisor remote topic, so I added Christian to the To:
> line of this message. Christian, you have no obligation to review
> these patches if they do not interest you, but just in case you
> weren't aware of this effort, I thought it might interest you; I am
> sure we all would benefit from your expertise.
>
> Thanks.
Yeah, it does seem that way. This patch series is really messy, and
generated way too much noise, so I don't blame them, and I'm sorry about
that.
I still learned a ton, so I'm happy about that. In the mean time I will
stick to solving simpler issues.
Thanks for your patience,
Lorenzo
^ permalink raw reply
* Re: [PATCH v2 6/8] promisor-remote: trust known remotes matching acceptFromServerUrl
From: Christian Couder @ 2026-05-19 15:24 UTC (permalink / raw)
To: Toon Claes
Cc: git, Junio C Hamano, Patrick Steinhardt, Taylor Blau,
Karthik Nayak, Elijah Newren, Christian Couder
In-Reply-To: <875x4yoys5.fsf@toon--20250203-5JQV3.mail-host-address-is-not-set>
On Fri, May 8, 2026 at 3:45 PM Toon Claes <toon@iotcl.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
> > +static bool match_one_url(const struct url_info *pi, const struct url_info *ui)
> > +{
> > + const char *pat = pi->url;
> > + const char *url = ui->url;
> > + char *p_str, *u_str;
> > + bool res;
> > +
> > + /*
> > + * Schemes must match exactly. They are case-folded by
> > + * url_normalize(), so strncmp() suffices.
> > + */
> > + if (pi->scheme_len != ui->scheme_len || strncmp(pat, url, pi->scheme_len))
> > + return false;
> > +
> > + /*
> > + * Ports must match exactly. url_normalize() strips default
> > + * ports (like 443 for https), so length and content
> > + * comparisons are sufficient.
> > + */
> > + if (pi->port_len != ui->port_len ||
> > + strncmp(pat + pi->port_off, url + ui->port_off, pi->port_len))
> > + return false;
> > +
> > + /*
> > + * Match host and path separately to prevent a '*' in the host
> > + * portion of the pattern from matching across the '/'
> > + * boundary into the path. Use WM_PATHNAME for the host so '*'
> > + * cannot cross '/' there, and 0 for the path so '*' can still
> > + * match multi-level paths.
> > + */
>
> Do we actually need WM_PATHNAME, because we only xstrndup() the host
> part anyway?
Yeah, it's not really needed.
On one hand it doesn't hurt either, and it conveys the intent, which
is that no / boundary should be crossed.
But on the other hand I agree it could be confusing and it's simpler
to just remove it, so I have removed it in the v3 I will send very
soon.
> > +
> > + p_str = xstrndup(pat + pi->host_off, pi->host_len);
> > + u_str = xstrndup(url + ui->host_off, ui->host_len);
> > + res = !wildmatch(p_str, u_str, WM_PATHNAME);
> > + free(p_str);
> > + free(u_str);
> > +
> > + if (!res)
> > + return false;
> > +
> > + p_str = xstrndup(pat + pi->path_off, pi->path_len);
> > + u_str = xstrndup(url + ui->path_off, ui->path_len);
> > + res = !wildmatch(p_str, u_str, 0);
> > + free(p_str);
> > + free(u_str);
>
> Is it correct we intentionally do not compare the user and pass (at
> `user_off` and `passwd_off`)? I assume so, because this allows the
> server to update those?
Yes, we ignore them intentionally. Using the existing `token` field
should be prefered, but maybe some need a user and password part of
the URL.
Anyway I have documented that in v3.
> > static int should_accept_remote(enum accept_promisor accept,
> > struct promisor_info *advertised,
> > + struct string_list *accept_urls,
> > struct string_list *config_info)
> > {
> > struct promisor_info *p;
> > @@ -771,9 +846,6 @@ static int should_accept_remote(enum accept_promisor accept,
> > if (accept == ACCEPT_KNOWN_NAME)
> > return all_fields_match(advertised, config_info, p);
> >
> > - if (accept != ACCEPT_KNOWN_URL)
> > - BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
> > -
> > if (strcmp(p->url, remote_url)) {
> > warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
> > "ignoring this remote"),
> > @@ -781,7 +853,21 @@ static int should_accept_remote(enum accept_promisor accept,
> > return 0;
> > }
> >
> > - return all_fields_match(advertised, config_info, p);
> > + if (accept == ACCEPT_KNOWN_URL)
> > + return all_fields_match(advertised, config_info, p);
> > +
> > + if (accept != ACCEPT_NONE)
> > + BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
> > +
> > + /*
> > + * Even if accept == ACCEPT_NONE, we MUST trust this known
> > + * remote to update its token or other such fields if its URL
> > + * matches the acceptFromServerUrl allowlist!
> > + */
> > + if (url_matches_accept_list(accept_urls, remote_url))
> > + return all_fields_match(advertised, config_info, p);
>
> I should verify in the following patches, but it seems to me only when
> promisor.AcceptFromServer is set to None it will store the advertised
> servers to the local .git/config, or not?
Right, it's better to check if the URL is in the allowlist as soon as
we can. So in the v3 I have moved as much as possible the
`promisor.acceptFromServerUrl` related checks before the other checks.
The idea is that `promisor.acceptFromServerUrl` takes precedence over
`promisor.acceptFromServer`, so having the
`promisor.acceptFromServerUrl` checks first makes sense.
Note that we should still not accept an advertised remote with an URL
that matches a pattern in `promisor.acceptFromServerUrl` if a remote
with the same name but a different URL exist on the client, unless the
user has explicitly set `promisor.AcceptFromServer` to either 'All' or
'knownName'.
In the v3 I have also added some documentation to be explicit about this.
> > +test_expect_success "clone with 'None' but URL allowlisted" '
> > + git -C server config promisor.advertise true &&
> > + test_when_finished "rm -rf client" &&
> > +
> > + GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \
> > + -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
> > + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
> > + -c promisor.acceptfromserver=None \
> > + -c promisor.acceptFromServerUrl="$ENCODED_TRASH_DIRECTORY_URL/*" \
> > + --no-local --filter="blob:limit=5k" server client &&
> > +
> > + # Check that the largest object is still missing on the server
> > + check_missing_objects server 1 "$oid"
> > +'
>
> Why do some tests end with `initialize_server 1 "$oid"` and this one
> not? Isn't it weird tests prepare for the next test?
It's more cleaning up after themselves than preparing for the next test.
Initializing the server with `initialize_server 1 "$oid"` is only
needed when some large objects end up on the server where they should
not be.
If we wanted to be sure that the state of the server is always clean
for the next test, then we should initialize the server at the end of
every test, using something like:
test_when_finished "initialize_server 1 \"$oid\""
just in case something went wrong and a large file was transferred to
the server. But I think that's quite expensive for not much gain.
It's also clearer for readers to have `initialize_server 1 "$oid"`
only where we think it's really needed.
So in the end I prefer to leave this as-is for now. We can still
address this later in a separate series for all the tests in
"t5710-promisor-remote-capability.sh" if we really think it's worth
addressing.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 6/8] promisor-remote: trust known remotes matching acceptFromServerUrl
From: Christian Couder @ 2026-05-19 15:25 UTC (permalink / raw)
To: Toon Claes
Cc: git, Junio C Hamano, Patrick Steinhardt, Taylor Blau,
Karthik Nayak, Elijah Newren, Christian Couder
In-Reply-To: <87qzninlb4.fsf@toon--20250203-5JQV3.mail-host-address-is-not-set>
On Mon, May 11, 2026 at 3:11 PM Toon Claes <toon@iotcl.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > +static bool match_one_url(const struct url_info *pi, const struct url_info *ui)
> > +{
> > + const char *pat = pi->url;
> > + const char *url = ui->url;
> > + char *p_str, *u_str;
> > + bool res;
> > +
> > + /*
> > + * Schemes must match exactly. They are case-folded by
> > + * url_normalize(), so strncmp() suffices.
> > + */
> > + if (pi->scheme_len != ui->scheme_len || strncmp(pat, url, pi->scheme_len))
> > + return false;
> > +
> > + /*
> > + * Ports must match exactly. url_normalize() strips default
> > + * ports (like 443 for https), so length and content
> > + * comparisons are sufficient.
> > + */
> > + if (pi->port_len != ui->port_len ||
> > + strncmp(pat + pi->port_off, url + ui->port_off, pi->port_len))
> > + return false;
> > +
> > + /*
> > + * Match host and path separately to prevent a '*' in the host
> > + * portion of the pattern from matching across the '/'
> > + * boundary into the path. Use WM_PATHNAME for the host so '*'
> > + * cannot cross '/' there, and 0 for the path so '*' can still
> > + * match multi-level paths.
> > + */
> > +
> > + p_str = xstrndup(pat + pi->host_off, pi->host_len);
> > + u_str = xstrndup(url + ui->host_off, ui->host_len);
> > + res = !wildmatch(p_str, u_str, WM_PATHNAME);
> > + free(p_str);
> > + free(u_str);
> > +
> > + if (!res)
>
> I feel it's a bit confusing your negating the result from wildmatch()
> to negate it here again? Maybe keep using the int return value, or
> rename the variable to 'matches' ?
I have simplified this in the v3.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 7/8] promisor-remote: auto-configure unknown remotes
From: Christian Couder @ 2026-05-19 15:25 UTC (permalink / raw)
To: Toon Claes
Cc: git, Junio C Hamano, Patrick Steinhardt, Taylor Blau,
Karthik Nayak, Elijah Newren, Christian Couder
In-Reply-To: <87v7cunlid.fsf@toon--20250203-5JQV3.mail-host-address-is-not-set>
On Mon, May 11, 2026 at 3:06 PM Toon Claes <toon@iotcl.com> wrote:
> > +static char *promisor_remote_name_from_url(const char *url)
> > +{
> > + struct url_info url_info = { 0 };
> > + char *normalized = url_normalize(url, &url_info);
> > + struct strbuf buf = STRBUF_INIT;
> > +
> > + if (!normalized) {
> > + warning(_("couldn't normalize advertised url '%s', "
> > + "ignoring this remote"), url);
> > + return NULL;
> > + }
> > +
> > + if (url_info.host_len) {
> > + strbuf_add(&buf, normalized + url_info.host_off, url_info.host_len);
> > + strbuf_addch(&buf, '-');
> > + }
> > +
> > + if (url_info.port_len) {
> > + strbuf_add(&buf, normalized + url_info.port_off, url_info.port_len);
> > + strbuf_addch(&buf, '-');
>
> If the url doesn't have a path, this could lead to the name being
> `example-com-8443`. But we have a MAX_REMOTES_WITH_SIMILAR_NAMES at 20,
> would this be an issue for a second remote without configured name?
>
> As far as I can tell from handle_matching_allowed_url(), it's no issue,
> because the numeric `-%d` suffix is added and we never atoi() the number
> from existing remotes in the config.
Right.
[...]
> > +test_expect_success "clone with URL allowlisted and no remote already configured" '
> > + git -C server config promisor.advertise true &&
> > + test_when_finished "rm -rf client" &&
> > + test_when_finished "rm -f full_names" &&
> > +
> > + GIT_NO_LAZY_FETCH=0 git clone \
> > + -c promisor.acceptfromserver=None \
> > + -c promisor.acceptFromServerUrl="$ENCODED_TRASH_DIRECTORY_URL/*" \
> > + --no-local --filter="blob:limit=5k" server client &&
>
> So promisor.acceptFromServerUrl only works if promisor.acceptFromServer
> is "none"? I mean which one should precedence? If
> promisor.acceptFromServer is set to "all", the promisor remote is
> accepted by the client, but not saved to the config. Is that
> intentional? Should we document that?
Yeah, it was buggy in v2 for some values of
`promisor.acceptfromserver`, and things were not properly documented.
But I think it's correct and much clearer now in v3 as discussed in my
reply to your comments on the previous patch.
Thanks.
^ permalink raw reply
* [PATCH v3 0/8] Auto-configure advertised remotes via URL allowlist
From: Christian Couder @ 2026-05-19 15:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Toon Claes, Christian Couder
In-Reply-To: <20260427124108.3524129-1-christian.couder@gmail.com>
Currently, the "promisor-remote" protocol capability allows a server
to advertise promisor remotes (and their tokens/filters), but the
client's `promisor.acceptFromServer` mechanism requires these remotes
to already exist in the config.
This is a significant burden for users and administrators who have to
pre-configure remotes.
This patch series improves on this by introducing a new
`promisor.acceptFromServerUrl` config option, which provides an
additive, URL-based security allowlist.
Multiple `promisor.acceptFromServerUrl` config options can be provided
in different config files. Each one should contain a URL glob pattern
which can optionally be prefixed with a remote name in the
"[<name>=]<pattern>" format.
The goal is for something like a simple:
git config set --global promisor.acceptFromServerUrl "https://my-org.com/*"
to be all that is needed for internal work in many organizations.
With this new config option:
- The server can update fields (like tokens) for known remotes,
provided their URL matches the allowlist, even if
`acceptFromServer` is set to `None`.
- Unknown remotes advertised by the server can be automatically
configured on the client if their URL matches the allowlist.
- If there is no `<name>` prefix before the glob pattern matched, the
auto-configured remote is named using the
"promisor-auto-<sanitized-url>" format. So the same auto-configured
remote config entry will be reused for the same URL.
- If a `<name>` prefix is provided, it will be used for the
auto-configured remote config entry.
- If the chosen name (auto-generated or prefixed) already exists but
points to a different URL, overwriting the existing config is
prevented by appending a numeric suffix (e.g., -1, -2) to the name
and auto-configuring using that name.
- The server's originally advertised name is always saved in the
`remote.<name>.advertisedAs` config variable of the auto-configured
remote for tracing and debugging.
Security considerations:
- Advertised URLs and glob patterns are routed through
url_normalize() / url_normalize_pattern() before matching, to
prevent percent-encoding, case variation, or path-traversal (..)
bypasses.
- URL matching is done component by component: scheme and port
must match exactly (no wildcards), the host is matched with
WM_PATHNAME so a '*' cannot cross the '/' boundary into the
path, and the path is matched without WM_PATHNAME so '*' can
still span multi-level paths.
- Auto-generated remote names are sanitized (non-alphanumeric
characters are replaced with '-', runs of '-' are collapsed)
and prefixed with 'promisor-auto-'. User-supplied names (from
the 'name=<pattern>' syntax) are validated with
valid_remote_name(). Together, these prevent a server from
maliciously overwriting standard remotes (like 'origin').
- If the auto-generated or user-supplied name collides with an
existing remote configured to a different URL, a numeric
suffix ('-1', '-2', ...) is appended, up to a bounded limit,
so a server cannot hijack an existing remote by name.
- Known remotes are still subject to URL consistency checks:
even if an advertised URL matches the allowlist, it is only
accepted for a known remote if it matches the URL already
configured locally for that remote.
- The documentation explains in detail how to write secure glob
patterns in `promisor.acceptFromServerUrl`, and highlights the
risks of overly broad patterns on shared hosting platforms.
High level description of the patches
=====================================
- Patch 1/8 is a very small preparatory patch that simplifies some
tests a bit.
- Patches 2/8 and 3/8 expose and adapt a url_normalize_pattern()
helper function in the urlmatch API.
- Patch 4/8 adapts `struct promisor_info` by adding a new
`local_name` member to it to prepare for the next patches.
- Patches 5/8 to 7/8 implement the core feature. They introduce the
parsing machinery, add the additive allowlist for known remotes
(with url_normalize() security), and finally implement the
auto-creation and collision resolution for unknown remotes.
- Patch 8/8 cleans up and modernizes the existing
`promisor.acceptFromServer` documentation.
Changes compared to v2
======================
Thanks to Toon, Patrick and Junio for reviewing the previous versions
of this series and of the preparatory series.
This series has been rebased on top of master now that the preparatory
series has been merged in a19de4d24a (Merge branch
'cc/promisor-auto-config-url', 2026-05-11).
Only the following patches changed:
- Patch 6/8 (promisor-remote: trust known remotes matching acceptFromServerUrl)
- The WM_PATHNAME flag is not used anymore when calling wildmatch().
- The match_one_url() function has been refactored using a new
match_pattern_url() helper function. There is no double negation
anymore.
- The call to url_matches_accept_list() in should_accept_remote()
has been moved up to make sure `promisor.acceptFromServerUrl`
takes precedence over `promisor.acceptFromServer`.
- It's documented that the username and password components of the
URL are intentionally ignored during matching.
- The documentation now clarifies how
`promisor.acceptFromServerUrl` interacts with
`promisor.acceptFromServer`.
- Patch 7/8 (promisor-remote: auto-configure unknown remotes)
- The call to should_accept_new_remote_url() is now before the
`accept == ACCEPT_ALL` check.
- The documentation continues to clarify how
`promisor.acceptFromServerUrl` interacts with
`promisor.acceptFromServer`.
CI tests
========
They all pass, see:
https://github.com/chriscool/git/actions/runs/26103407562
Range diff since v2
===================
1: 44e9a16455 = 1: ab231c0896 t5710: simplify 'mkdir X' followed by 'git -C X init'
2: 42f174910c = 2: b3e66f329f urlmatch: change 'allow_globs' arg to bool
3: 8088374458 = 3: 813d748dd6 urlmatch: add url_normalize_pattern() helper
4: 6bfda89a79 = 4: e92863bee8 promisor-remote: add 'local_name' to 'struct promisor_info'
5: fefa17e6dd = 5: 7e1b106404 promisor-remote: introduce promisor.acceptFromServerUrl
6: 2f238d0a7a ! 6: f00eed4bf2 promisor-remote: trust known remotes matching acceptFromServerUrl
@@ Commit message
returns the first matching allowed_url entry (or NULL).
The URL matching is done component by component: scheme and port are
- compared exactly, the host is matched with wildmatch() using the
- WM_PATHNAME flag (so '*' cannot cross the '/' boundary into the path),
- and the path is matched with wildmatch() without WM_PATHNAME (so '*'
- can still match multi-level paths). Before matching, the advertised
- URL is passed through url_normalize() so that case variations in the
- scheme/host, percent-encoding tricks, and ".." path segments cannot
- bypass the allowlist.
+ compared exactly, the host and path are matched with wildmatch().
+ Before matching, the advertised URL is passed through url_normalize()
+ so that case variations in the scheme/host, percent-encoding tricks,
+ and ".." path segments cannot bypass the allowlist.
- Let's then use this helper at the tail of should_accept_remote() so
- that, when `accept == ACCEPT_NONE`, a known remote whose URL matches
- the allowlist is still accepted.
+ The username and password components of the URL are intentionally
+ ignored during matching to allow servers to rotate them, though using
+ the 'token' field of the capability is preferred over embedding
+ credentials in the URL.
+
+ Let's then use this helper in should_accept_remote() so that, a known
+ remote whose URL matches the allowlist is accepted.
To prepare for this new logic, let's also:
@@ Commit message
- Replace the BUG() guard in the ACCEPT_KNOWN_URL case with an
explicit 'if (accept == ACCEPT_KNOWN_URL) return' and a new
- BUG() guard in the ACCEPT_NONE case, so url_matches_accept_list()
- is only called in the ACCEPT_NONE case.
+ BUG() guard in the ACCEPT_NONE case.
- Call accept_from_server_url() from filter_promisor_remote()
and relax its early return so that the function is entered when
@@ Commit message
including the URL normalization behavior and the component-wise
matching, and let's mention it in "gitprotocol-v2.adoc".
+ Also let's clarify in the documentation how
+ `promisor.acceptFromServerUrl` interacts with
+ `promisor.acceptFromServer`:
+
+ - Precedence: when both options are set,
+ `promisor.acceptFromServerUrl` is consulted first. If a matching
+ pattern leads to acceptance, the remote is accepted regardless of
+ `promisor.acceptFromServer`. Otherwise the decision is left to
+ `promisor.acceptFromServer`.
+
+ - URL-mismatch guard: even when the advertised URL matches the
+ allowlist, an already-existing client-side remote whose configured
+ URL differs from the advertised one is not accepted through
+ `promisor.acceptFromServerUrl`. `promisor.acceptFromServer=all` and
+ `=knownName` keep their pre-existing, looser semantics.
+
+ The precedence paragraph is intentionally scoped here to known remotes
+ only (field updates). A following commit that introduces auto-creation
+ of unknown remotes will extend it to cover that case as well.
+
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
## Documentation/config/promisor.adoc ##
@@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
+URL will be accepted if it matches _ANY_ glob pattern specified by
+this option in _ANY_ config file read by Git.
++
++When both `promisor.acceptFromServer` and `promisor.acceptFromServerUrl`
++are set, `promisor.acceptFromServerUrl` is consulted first and takes
++precedence: if a matching pattern leads to acceptance (by accepting
++field updates for a known remote whose URL matches both the local
++configuration and the allowlist), the advertised remote is accepted
++regardless of the `promisor.acceptFromServer` setting. If no pattern
++in `promisor.acceptFromServerUrl` triggers acceptance, the decision
++is left to `promisor.acceptFromServer`.
+++
++Note however that, even when an advertised URL matches a pattern in
++`promisor.acceptFromServerUrl`, an already-existing remote on the
++client whose name matches the advertised name but whose configured URL
++differs from the advertised one will _NOT_ be accepted through
++`promisor.acceptFromServerUrl`. This prevents a server from silently
++re-pointing an existing client-side remote at a different URL. (Such a
++remote may still be accepted through `promisor.acceptFromServer=all`
++or `=knownName`, which have their own, looser semantics; see the
++documentation of that option.)
+++
+Be _VERY_ careful with these patterns: `*` matches any sequence of
+characters within the 'host' and 'path' parts of a URL (but cannot
+cross part boundaries). An overly broad pattern is a major security
@@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
+characters are decoded where possible, and path segments like `..`
+are resolved. The port must also match exactly (e.g.,
+`https://example.com:8080/*` will not match a URL advertised on
-+port 9999).
++port 9999). The username and password components of the URL are
++ignored during matching. Note that embedding credentials in URLs is
++discouraged. Passing authentication tokens via the `token` field of
++the `promisor-remote` capability is strongly preferred.
++
+For the security implications of accepting a promisor remote, see the
+documentation of `promisor.acceptFromServer`. For details on the
@@ promisor-remote.c: static void load_accept_from_server_url(struct repository *re
}
}
++static bool match_pattern_url(const char *pat, size_t pat_len,
++ const char *url, size_t url_len)
++{
++ char *p_str = xstrndup(pat, pat_len);
++ char *u_str = xstrndup(url, url_len);
++ bool res = !wildmatch(p_str, u_str, 0);
++
++ free(p_str);
++ free(u_str);
++
++ return res;
++}
++
+static bool match_one_url(const struct url_info *pi, const struct url_info *ui)
+{
+ const char *pat = pi->url;
+ const char *url = ui->url;
-+ char *p_str, *u_str;
-+ bool res;
+
+ /*
+ * Schemes must match exactly. They are case-folded by
@@ promisor-remote.c: static void load_accept_from_server_url(struct repository *re
+ /*
+ * Match host and path separately to prevent a '*' in the host
+ * portion of the pattern from matching across the '/'
-+ * boundary into the path. Use WM_PATHNAME for the host so '*'
-+ * cannot cross '/' there, and 0 for the path so '*' can still
-+ * match multi-level paths.
++ * boundary into the path.
+ */
+
-+ p_str = xstrndup(pat + pi->host_off, pi->host_len);
-+ u_str = xstrndup(url + ui->host_off, ui->host_len);
-+ res = !wildmatch(p_str, u_str, WM_PATHNAME);
-+ free(p_str);
-+ free(u_str);
-+
-+ if (!res)
-+ return false;
-+
-+ p_str = xstrndup(pat + pi->path_off, pi->path_len);
-+ u_str = xstrndup(url + ui->path_off, ui->path_len);
-+ res = !wildmatch(p_str, u_str, 0);
-+ free(p_str);
-+ free(u_str);
-+
-+ return res;
++ return match_pattern_url(pat + pi->host_off, pi->host_len,
++ url + ui->host_off, ui->host_len) &&
++ match_pattern_url(pat + pi->path_off, pi->path_len,
++ url + ui->path_off, ui->path_len);
+}
+
+static struct allowed_url *url_matches_accept_list(
@@ promisor-remote.c: static void load_accept_from_server_url(struct repository *re
{
struct promisor_info *p;
@@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
- if (accept == ACCEPT_KNOWN_NAME)
+ "this remote should have been rejected earlier",
+ remote_name);
+
+- if (accept == ACCEPT_ALL)
+- return all_fields_match(advertised, config_info, NULL);
+-
+ /* Get config info for that promisor remote */
+ item = string_list_lookup(config_info, remote_name);
+
+- if (!item)
++ if (!item) {
+ /* We don't know about that remote */
++ if (accept == ACCEPT_ALL)
++ return all_fields_match(advertised, config_info, NULL);
+ return 0;
++ }
+
+ p = item->util;
+
+- if (accept == ACCEPT_KNOWN_NAME)
++ /* Known remote in the allowlist? */
++ if (!strcmp(p->url, remote_url) && url_matches_accept_list(accept_urls, remote_url))
return all_fields_match(advertised, config_info, p);
- if (accept != ACCEPT_KNOWN_URL)
- BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
--
++ if (accept == ACCEPT_ALL)
++ return all_fields_match(advertised, config_info, NULL);
++
++ if (accept == ACCEPT_KNOWN_NAME)
++ return all_fields_match(advertised, config_info, p);
+
if (strcmp(p->url, remote_url)) {
warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
- "ignoring this remote"),
@@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
return 0;
}
@@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
+ if (accept != ACCEPT_NONE)
+ BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
+
-+ /*
-+ * Even if accept == ACCEPT_NONE, we MUST trust this known
-+ * remote to update its token or other such fields if its URL
-+ * matches the acceptFromServerUrl allowlist!
-+ */
-+ if (url_matches_accept_list(accept_urls, remote_url))
-+ return all_fields_match(advertised, config_info, p);
-+
+ return 0;
}
7: a077f33df4 ! 7: af06fb31db promisor-remote: auto-configure unknown remotes
@@ Commit message
handling, and by
- adding a "remote.<name>.advertisedAs" entry to "remote.adoc".
+ Also let's extend the precedence paragraph added by a previous commit
+ to mention this new acceptance path: until now, the only way for
+ `promisor.acceptFromServerUrl` to trigger acceptance was to allow
+ field updates for a known remote. With this commit, it can also trigger
+ auto-creation of a previously-unknown remote whose advertised URL
+ matches the allowlist.
+
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
## Documentation/config/promisor.adoc ##
@@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
tokens) from the server, even if `promisor.acceptFromServer`
is set to `none` (the default).
@@ Documentation/config/promisor.adoc: this option in _ANY_ config file read by Git.
+ +
+ When both `promisor.acceptFromServer` and `promisor.acceptFromServerUrl`
+ are set, `promisor.acceptFromServerUrl` is consulted first and takes
+-precedence: if a matching pattern leads to acceptance (by accepting
+-field updates for a known remote whose URL matches both the local
+-configuration and the allowlist), the advertised remote is accepted
+-regardless of the `promisor.acceptFromServer` setting. If no pattern
+-in `promisor.acceptFromServerUrl` triggers acceptance, the decision
+-is left to `promisor.acceptFromServer`.
++precedence: if a matching pattern leads to acceptance (either by
++auto-configuring an unknown remote or by accepting field updates for
++a known remote whose URL matches both the local configuration and the
++allowlist), the advertised remote is accepted regardless of the
++`promisor.acceptFromServer` setting. If no pattern in
++`promisor.acceptFromServerUrl` triggers acceptance, the decision is
++left to `promisor.acceptFromServer`.
+ +
+ Note however that, even when an advertised URL matches a pattern in
+ `promisor.acceptFromServerUrl`, an already-existing remote on the
+@@ Documentation/config/promisor.adoc: documentation of that option.)
Be _VERY_ careful with these patterns: `*` matches any sequence of
characters within the 'host' and 'path' parts of a URL (but cannot
cross part boundaries). An overly broad pattern is a major security
@@ Documentation/config/promisor.adoc: this option in _ANY_ config file read by Git
+
1. Start with a secure protocol scheme, like `https://` or `ssh://`.
+
-@@ Documentation/config/promisor.adoc: are resolved. The port must also match exactly (e.g.,
- `https://example.com:8080/*` will not match a URL advertised on
- port 9999).
+@@ Documentation/config/promisor.adoc: ignored during matching. Note that embedding credentials in URLs is
+ discouraged. Passing authentication tokens via the `token` field of
+ the `promisor-remote` capability is strongly preferred.
+
+The glob pattern can optionally be prefixed with a remote name and an
+equals sign (e.g., `cdn=https://cdn.example.com/*`). If such a prefix
@@ promisor-remote.c: static struct allowed_url *url_matches_accept_list(
struct promisor_info *p;
struct string_list_item *item;
@@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
- /* Get config info for that promisor remote */
- item = string_list_lookup(config_info, remote_name);
-- if (!item)
-+ if (!item) {
+ if (!item) {
/* We don't know about that remote */
-- return 0;
++
+ int res = should_accept_new_remote_url(repo, accept_urls, advertised);
-+ if (res)
++ if (res) {
+ *reload_config = true;
-+ return res;
-+ }
-
- p = item->util;
-
++ return res;
++ }
++
+ if (accept == ACCEPT_ALL)
+ return all_fields_match(advertised, config_info, NULL);
+ return 0;
@@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
string_list_sort(&config_info);
}
8: b68b9497aa = 8: 92075d88d8 doc: promisor: improve acceptFromServer entry
Christian Couder (8):
t5710: simplify 'mkdir X' followed by 'git -C X init'
urlmatch: change 'allow_globs' arg to bool
urlmatch: add url_normalize_pattern() helper
promisor-remote: add 'local_name' to 'struct promisor_info'
promisor-remote: introduce promisor.acceptFromServerUrl
promisor-remote: trust known remotes matching acceptFromServerUrl
promisor-remote: auto-configure unknown remotes
doc: promisor: improve acceptFromServer entry
Documentation/config/promisor.adoc | 146 +++++++--
Documentation/config/remote.adoc | 9 +
Documentation/gitprotocol-v2.adoc | 9 +-
promisor-remote.c | 413 ++++++++++++++++++++++++--
t/t5710-promisor-remote-capability.sh | 202 ++++++++++++-
urlmatch.c | 11 +-
urlmatch.h | 12 +
7 files changed, 754 insertions(+), 48 deletions(-)
--
2.54.0.134.gbbe8e27878.dirty
^ permalink raw reply
* [PATCH v3 1/8] t5710: simplify 'mkdir X' followed by 'git -C X init'
From: Christian Couder @ 2026-05-19 15:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Toon Claes, Christian Couder, Christian Couder
In-Reply-To: <20260519153808.494105-1-christian.couder@gmail.com>
It's simpler and more efficient to just use `git init client` instead
of `mkdir client && git -C client init`.
So let's replace the latter with the former.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/t5710-promisor-remote-capability.sh | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index b404ad9f0a..bf1cc54605 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -177,8 +177,7 @@ test_expect_success "init + fetch with promisor.advertise set to 'true'" '
git -C server config promisor.advertise true &&
test_when_finished "rm -rf client" &&
- mkdir client &&
- git -C client init &&
+ git init client &&
git -C client config remote.lop.promisor true &&
git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
git -C client config remote.lop.url "$TRASH_DIRECTORY_URL/lop" &&
@@ -231,8 +230,7 @@ test_expect_success "init + fetch two promisors but only one advertised" '
# Create a promisor that will be configured but not be used
git init --bare unused_lop &&
- mkdir client &&
- git -C client init &&
+ git init client &&
git -C client config remote.unused_lop.promisor true &&
git -C client config remote.unused_lop.fetch "+refs/heads/*:refs/remotes/unused_lop/*" &&
git -C client config remote.unused_lop.url "$TRASH_DIRECTORY_URL/unused_lop" &&
--
2.54.0.134.gbbe8e27878.dirty
^ permalink raw reply related
* [PATCH v3 2/8] urlmatch: change 'allow_globs' arg to bool
From: Christian Couder @ 2026-05-19 15:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Toon Claes, Christian Couder, Christian Couder
In-Reply-To: <20260519153808.494105-1-christian.couder@gmail.com>
The last argument of url_normalize_1() is `char allow_globs` but it is
used as a boolean, not as a char.
Let's convert it to a `bool`, and while at it convert the two calls to
url_normalize_1() so they pass 'true' or 'false' instead of '1' or '0'.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
urlmatch.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/urlmatch.c b/urlmatch.c
index eea8300489..989bc7eb8b 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -111,7 +111,7 @@ static int match_host(const struct url_info *url_info,
return (!url_len && !pat_len);
}
-static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)
+static char *url_normalize_1(const char *url, struct url_info *out_info, bool allow_globs)
{
/*
* Normalize NUL-terminated url using the following rules:
@@ -437,7 +437,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al
char *url_normalize(const char *url, struct url_info *out_info)
{
- return url_normalize_1(url, out_info, 0);
+ return url_normalize_1(url, out_info, false);
}
static size_t url_match_prefix(const char *url,
@@ -577,7 +577,7 @@ int urlmatch_config_entry(const char *var, const char *value,
struct url_info norm_info;
config_url = xmemdupz(key, dot - key);
- norm_url = url_normalize_1(config_url, &norm_info, 1);
+ norm_url = url_normalize_1(config_url, &norm_info, true);
if (norm_url)
retval = match_urls(url, &norm_info, &matched);
else if (collect->fallback_match_fn)
--
2.54.0.134.gbbe8e27878.dirty
^ permalink raw reply related
* [PATCH v3 3/8] urlmatch: add url_normalize_pattern() helper
From: Christian Couder @ 2026-05-19 15:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Toon Claes, Christian Couder, Christian Couder
In-Reply-To: <20260519153808.494105-1-christian.couder@gmail.com>
In a following commit, we will need to normalize a URL glob pattern
(which may contain '*' in the host portion) and extract its component
offsets (host, path, etc.) for separate matching. Let's export a
dedicated helper function url_normalize_pattern() for that purpose.
It works like url_normalize(), but passes allow_globs=true to the
internal url_normalize_1(), so that '*' characters in the host are
accepted rather than rejected.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
urlmatch.c | 5 +++++
urlmatch.h | 12 ++++++++++++
2 files changed, 17 insertions(+)
diff --git a/urlmatch.c b/urlmatch.c
index 989bc7eb8b..7e734e2660 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -440,6 +440,11 @@ char *url_normalize(const char *url, struct url_info *out_info)
return url_normalize_1(url, out_info, false);
}
+char *url_normalize_pattern(const char *url, struct url_info *out_info)
+{
+ return url_normalize_1(url, out_info, true);
+}
+
static size_t url_match_prefix(const char *url,
const char *url_prefix,
size_t url_prefix_len)
diff --git a/urlmatch.h b/urlmatch.h
index 5ba85cea13..32c5067f9b 100644
--- a/urlmatch.h
+++ b/urlmatch.h
@@ -36,6 +36,18 @@ struct url_info {
char *url_normalize(const char *, struct url_info *);
+/*
+ * Like url_normalize(), but also allows '*' glob characters in the host
+ * portion. Use this when normalizing URL patterns from user configuration.
+ *
+ * Note that '*' is a valid path character per RFC 3986 (as a sub-delim),
+ * so glob patterns using '*' in the path are also accepted.
+ *
+ * Returns a newly allocated normalized string and fills out_info if
+ * non-NULL, or NULL if the pattern is invalid.
+ */
+char *url_normalize_pattern(const char *url, struct url_info *out_info);
+
struct urlmatch_item {
size_t hostmatch_len;
size_t pathmatch_len;
--
2.54.0.134.gbbe8e27878.dirty
^ permalink raw reply related
* [PATCH v3 4/8] promisor-remote: add 'local_name' to 'struct promisor_info'
From: Christian Couder @ 2026-05-19 15:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Toon Claes, Christian Couder, Christian Couder
In-Reply-To: <20260519153808.494105-1-christian.couder@gmail.com>
In a following commit, we will store promisor remote information under
a remote name different than the one the server advertised.
To prepare for this change, let's add a new 'char *local_name' member
to 'struct promisor_info', and let's update the related functions.
While at it, let's also add a small promisor_info_internal_name()
helper that returns `local_name` when set, `name` otherwise, and let's
use this small helper in promisor_store_advertised_fields() and in the
post-loop of filter_promisor_remote() so that lookups against the local
repo configuration use the right name.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
promisor-remote.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/promisor-remote.c b/promisor-remote.c
index 38fa050542..7699e259eb 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -434,13 +434,14 @@ static struct string_list *fields_stored(void)
* Struct for promisor remotes involved in the "promisor-remote"
* protocol capability.
*
- * Except for "name", each <member> in this struct and its <value>
- * should correspond (either on the client side or on the server side)
- * to a "remote.<name>.<member>" config variable set to <value> where
- * "<name>" is a promisor remote name.
+ * Except for "name" and "local_name", each <member> in this struct
+ * and its <value> should correspond (either on the client side or on
+ * the server side) to a "remote.<name>.<member>" config variable set
+ * to <value> where "<name>" is a promisor remote name.
*/
struct promisor_info {
- const char *name;
+ const char *name; /* name the server advertised */
+ const char *local_name; /* name used locally (may be auto-generated) */
const char *url;
const char *filter;
const char *token;
@@ -449,6 +450,7 @@ struct promisor_info {
static void promisor_info_free(struct promisor_info *p)
{
free((char *)p->name);
+ free((char *)p->local_name);
free((char *)p->url);
free((char *)p->filter);
free((char *)p->token);
@@ -462,6 +464,11 @@ static void promisor_info_list_clear(struct string_list *list)
string_list_clear(list, 0);
}
+static const char *promisor_info_internal_name(struct promisor_info *p)
+{
+ return p->local_name ? p->local_name : p->name;
+}
+
static void set_one_field(struct promisor_info *p,
const char *field, const char *value)
{
@@ -829,7 +836,7 @@ static bool promisor_store_advertised_fields(struct promisor_info *advertised,
{
struct promisor_info *p;
struct string_list_item *item;
- const char *remote_name = advertised->name;
+ const char *remote_name = promisor_info_internal_name(advertised);
bool reload_config = false;
if (!(store_info->store_filter || store_info->store_token))
@@ -937,7 +944,8 @@ static void filter_promisor_remote(struct repository *repo,
/* Apply accepted remotes to the stable repo state */
for_each_string_list_item(item, accepted_remotes) {
struct promisor_info *info = item->util;
- struct promisor_remote *r = repo_promisor_remote_find(repo, info->name);
+ const char *local = promisor_info_internal_name(info);
+ struct promisor_remote *r = repo_promisor_remote_find(repo, local);
if (r) {
r->accepted = 1;
--
2.54.0.134.gbbe8e27878.dirty
^ permalink raw reply related
* [PATCH v3 5/8] promisor-remote: introduce promisor.acceptFromServerUrl
From: Christian Couder @ 2026-05-19 15:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Toon Claes, Christian Couder, Christian Couder
In-Reply-To: <20260519153808.494105-1-christian.couder@gmail.com>
The "promisor-remote" protocol capability allows servers to advertise
promisor remotes, but doesn't allow these remotes to be automatically
configured on the client.
Let's introduce a new `promisor.acceptFromServerUrl` config variable
which contains a glob pattern, so that advertised remotes with a URL
matching that pattern will be automatically configured.
The glob pattern can optionally be prefixed with a remote name which
will be used as the name of the new local remote.
For now though, let's only introduce the functions to read and validate
the glob patterns and the optional prefixes.
Checking if the URLs of the advertised remotes match the glob patterns
and taking the appropriate action is left for a following commit.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
promisor-remote.c | 90 +++++++++++++++++++++++++++
t/t5710-promisor-remote-capability.sh | 21 +++++++
2 files changed, 111 insertions(+)
diff --git a/promisor-remote.c b/promisor-remote.c
index 7699e259eb..3f3924f587 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -12,6 +12,7 @@
#include "packfile.h"
#include "environment.h"
#include "url.h"
+#include "urlmatch.h"
#include "version.h"
struct promisor_remote_config {
@@ -657,6 +658,90 @@ static bool has_control_char(const char *s)
return false;
}
+struct allowed_url {
+ char *remote_name;
+ char *url_pattern;
+ struct url_info pattern_info;
+};
+
+static void allowed_url_free(void *util, const char *str UNUSED)
+{
+ struct allowed_url *allowed = util;
+
+ if (!allowed)
+ return;
+
+ /* Depending on prefix, free either remote_name or url_pattern */
+ free(allowed->remote_name ? allowed->remote_name : allowed->url_pattern);
+ free(allowed->pattern_info.url);
+ free(allowed);
+}
+
+static struct allowed_url *valid_accept_url(const char *url)
+{
+ char *dup, *p;
+ struct allowed_url *allowed;
+
+ if (!url)
+ return NULL;
+
+ dup = xstrdup(url);
+ p = strchr(dup, '=');
+ if (p) {
+ *p = '\0';
+ if (!valid_remote_name(dup)) {
+ warning(_("invalid remote name '%s' before '=' sign "
+ "in '%s' from promisor.acceptFromServerUrl config"),
+ dup, url);
+ free(dup);
+ return NULL;
+ }
+ p++;
+ } else {
+ p = dup;
+ }
+
+ if (has_control_char(p)) {
+ warning(_("invalid url pattern '%s' "
+ "in '%s' from promisor.acceptFromServerUrl config"), p, url);
+ free(dup);
+ return NULL;
+ }
+
+ allowed = xmalloc(sizeof(*allowed));
+ allowed->remote_name = (p == dup) ? NULL : dup;
+ allowed->url_pattern = p;
+ allowed->pattern_info.url = url_normalize_pattern(p, &allowed->pattern_info);
+ if (!allowed->pattern_info.url) {
+ warning(_("invalid url pattern '%s' "
+ "in '%s' from promisor.acceptFromServerUrl config"), p, url);
+ free(dup);
+ free(allowed);
+ return NULL;
+ }
+
+ return allowed;
+}
+
+static void load_accept_from_server_url(struct repository *repo,
+ struct string_list *accept_urls)
+{
+ const struct string_list *config_urls;
+
+ if (!repo_config_get_string_multi(repo, "promisor.acceptfromserverurl", &config_urls)) {
+ struct string_list_item *item;
+
+ for_each_string_list_item(item, config_urls) {
+ struct allowed_url *allowed = valid_accept_url(item->string);
+ if (allowed) {
+ struct string_list_item *new;
+ new = string_list_append(accept_urls, item->string);
+ new->util = allowed;
+ }
+ }
+ }
+}
+
static int should_accept_remote(enum accept_promisor accept,
struct promisor_info *advertised,
struct string_list *config_info)
@@ -901,6 +986,10 @@ static void filter_promisor_remote(struct repository *repo,
struct string_list_item *item;
bool reload_config = false;
enum accept_promisor accept = accept_from_server(repo);
+ struct string_list accept_urls = STRING_LIST_INIT_DUP;
+
+ /* Load and validate the acceptFromServerUrl config */
+ load_accept_from_server_url(repo, &accept_urls);
if (accept == ACCEPT_NONE)
return;
@@ -934,6 +1023,7 @@ static void filter_promisor_remote(struct repository *repo,
}
}
+ string_list_clear_func(&accept_urls, allowed_url_free);
promisor_info_list_clear(&config_info);
string_list_clear(&remote_info, 0);
store_info_free(store_info);
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index bf1cc54605..3b39505380 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -387,6 +387,27 @@ test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" '
check_missing_objects server 1 "$oid"
'
+test_expect_success "clone with invalid promisor.acceptFromServerUrl" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ # As "bad name" contains a space, which is not a valid remote name,
+ # the pattern should be rejected with a warning and no remote created.
+ GIT_NO_LAZY_FETCH=0 git clone \
+ -c promisor.acceptfromserver=None \
+ -c "promisor.acceptFromServerUrl=bad name=https://example.com/*" \
+ --no-local --filter="blob:limit=5k" server client 2>err &&
+
+ # Check that a warning was emitted
+ test_grep "invalid remote name '\''bad name'\''" err &&
+
+ # Check that the largest object is not missing on the server
+ check_missing_objects server 0 "" &&
+
+ # Reinitialize server so that the largest object is missing again
+ initialize_server 1 "$oid"
+'
+
test_expect_success "clone with promisor.sendFields" '
git -C server config promisor.advertise true &&
test_when_finished "rm -rf client" &&
--
2.54.0.134.gbbe8e27878.dirty
^ permalink raw reply related
* [PATCH v3 6/8] promisor-remote: trust known remotes matching acceptFromServerUrl
From: Christian Couder @ 2026-05-19 15:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Toon Claes, Christian Couder, Christian Couder
In-Reply-To: <20260519153808.494105-1-christian.couder@gmail.com>
A previous commit introduced the `promisor.acceptFromServerUrl` config
variable along with the machinery to parse and validate the URL glob
patterns and optional remote name prefixes it contains. However, these
URL patterns are not yet tied into the client's acceptance logic.
When a promisor remote is already configured locally, its fields (like
authentication tokens) may occasionally need to be refreshed by the
server. If `promisor.acceptFromServer` is set to the secure default
("None"), these updates are rejected, potentially causing future
fetches to fail.
To enable such targeted updates for trusted URLs, let's use the URL
patterns from `promisor.acceptFromServerUrl` as an additional URL
based allowlist.
Concretely, let's check the advertised URLs against the URL glob
patterns by introducing a new small helper function called
url_matches_accept_list(), which iterates over the glob patterns and
returns the first matching allowed_url entry (or NULL).
The URL matching is done component by component: scheme and port are
compared exactly, the host and path are matched with wildmatch().
Before matching, the advertised URL is passed through url_normalize()
so that case variations in the scheme/host, percent-encoding tricks,
and ".." path segments cannot bypass the allowlist.
The username and password components of the URL are intentionally
ignored during matching to allow servers to rotate them, though using
the 'token' field of the capability is preferred over embedding
credentials in the URL.
Let's then use this helper in should_accept_remote() so that, a known
remote whose URL matches the allowlist is accepted.
To prepare for this new logic, let's also:
- Add an 'accept_urls' parameter to should_accept_remote().
- Replace the BUG() guard in the ACCEPT_KNOWN_URL case with an
explicit 'if (accept == ACCEPT_KNOWN_URL) return' and a new
BUG() guard in the ACCEPT_NONE case.
- Call accept_from_server_url() from filter_promisor_remote()
and relax its early return so that the function is entered when
`accept_urls` has entries even if `accept == ACCEPT_NONE`.
With this, many organizations may only need something like:
git config set --global \
promisor.acceptFromServerUrl "https://my-org.com/*"
to accept only their own remotes. And if they need to accept additional
remotes in some specific repos, they can also set:
git config set promisor.acceptFromServer knownUrl
and configure the additional remote manually only in the repos where
they are needed.
Let's then properly document `promisor.acceptFromServerUrl` in
"promisor.adoc" as an additive security allowlist for known remotes,
including the URL normalization behavior and the component-wise
matching, and let's mention it in "gitprotocol-v2.adoc".
Also let's clarify in the documentation how
`promisor.acceptFromServerUrl` interacts with
`promisor.acceptFromServer`:
- Precedence: when both options are set,
`promisor.acceptFromServerUrl` is consulted first. If a matching
pattern leads to acceptance, the remote is accepted regardless of
`promisor.acceptFromServer`. Otherwise the decision is left to
`promisor.acceptFromServer`.
- URL-mismatch guard: even when the advertised URL matches the
allowlist, an already-existing client-side remote whose configured
URL differs from the advertised one is not accepted through
`promisor.acceptFromServerUrl`. `promisor.acceptFromServer=all` and
`=knownName` keep their pre-existing, looser semantics.
The precedence paragraph is intentionally scoped here to known remotes
only (field updates). A following commit that introduces auto-creation
of unknown remotes will extend it to cover that case as well.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/config/promisor.adoc | 74 +++++++++++++++++++
Documentation/gitprotocol-v2.adoc | 9 ++-
promisor-remote.c | 102 +++++++++++++++++++++++---
t/t5710-promisor-remote-capability.sh | 71 ++++++++++++++++++
4 files changed, 242 insertions(+), 14 deletions(-)
diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
index b0fa43b839..4a1ecb4425 100644
--- a/Documentation/config/promisor.adoc
+++ b/Documentation/config/promisor.adoc
@@ -51,6 +51,80 @@ promisor.acceptFromServer::
to "fetch" and "clone" requests from the client. Name and URL
comparisons are case sensitive. See linkgit:gitprotocol-v2[5].
+promisor.acceptFromServerUrl::
+ A glob pattern to specify which server-advertised URLs a
+ client is allowed to act on. When a URL matches, the client
+ will accept the advertised remote as a promisor remote and may
+ automatically accept field updates (such as authentication
+ tokens) from the server, even if `promisor.acceptFromServer`
+ is set to `none` (the default).
++
+This option can appear multiple times in config files. An advertised
+URL will be accepted if it matches _ANY_ glob pattern specified by
+this option in _ANY_ config file read by Git.
++
+When both `promisor.acceptFromServer` and `promisor.acceptFromServerUrl`
+are set, `promisor.acceptFromServerUrl` is consulted first and takes
+precedence: if a matching pattern leads to acceptance (by accepting
+field updates for a known remote whose URL matches both the local
+configuration and the allowlist), the advertised remote is accepted
+regardless of the `promisor.acceptFromServer` setting. If no pattern
+in `promisor.acceptFromServerUrl` triggers acceptance, the decision
+is left to `promisor.acceptFromServer`.
++
+Note however that, even when an advertised URL matches a pattern in
+`promisor.acceptFromServerUrl`, an already-existing remote on the
+client whose name matches the advertised name but whose configured URL
+differs from the advertised one will _NOT_ be accepted through
+`promisor.acceptFromServerUrl`. This prevents a server from silently
+re-pointing an existing client-side remote at a different URL. (Such a
+remote may still be accepted through `promisor.acceptFromServer=all`
+or `=knownName`, which have their own, looser semantics; see the
+documentation of that option.)
++
+Be _VERY_ careful with these patterns: `*` matches any sequence of
+characters within the 'host' and 'path' parts of a URL (but cannot
+cross part boundaries). An overly broad pattern is a major security
+risk, as a matching URL allows a server to update fields (such as
+authentication tokens) on known remotes without further confirmation.
+To minimize security risks, follow these guidelines:
++
+1. Start with a secure protocol scheme, like `https://` or `ssh://`.
++
+2. Only allow domain names or paths where you control and trust _ALL_
+ the content. Be especially careful with shared hosting platforms
+ like `github.com` or `gitlab.com`. A broad pattern like
+ `https://gitlab.com/*` is dangerous because it trusts every
+ repository on the entire platform. Always restrict such patterns to
+ your specific organization or namespace (e.g.,
+ `https://gitlab.com/your-org/*`).
++
+3. Never use globs at the end of domain names. For example,
+ `https://cdn.your-org.com/*` might be safe, but
+ `https://cdn.your-org.com*/*` is a major security risk because
+ the latter matches `https://cdn.your-org.com.hacker.net/repo`.
++
+4. Be careful using globs at the beginning of domain names. While the
+ code ensures a `*` in the host cannot cross into the path, a
+ pattern like `https://*.example.com/*` will still match any
+ subdomain. This is extremely dangerous on shared hosting platforms
+ (e.g., `https://*.github.io/*` trusts every user's site on the
+ entire platform).
++
+Before matching, both the advertised URL and the pattern are
+normalized: the scheme and host are lowercased, percent-encoded
+characters are decoded where possible, and path segments like `..`
+are resolved. The port must also match exactly (e.g.,
+`https://example.com:8080/*` will not match a URL advertised on
+port 9999). The username and password components of the URL are
+ignored during matching. Note that embedding credentials in URLs is
+discouraged. Passing authentication tokens via the `token` field of
+the `promisor-remote` capability is strongly preferred.
++
+For the security implications of accepting a promisor remote, see the
+documentation of `promisor.acceptFromServer`. For details on the
+protocol, see linkgit:gitprotocol-v2[5].
+
promisor.checkFields::
A comma or space separated list of additional remote related
field names. A client checks if the values of these fields
diff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc
index befa697d21..2beb70595f 100644
--- a/Documentation/gitprotocol-v2.adoc
+++ b/Documentation/gitprotocol-v2.adoc
@@ -866,10 +866,11 @@ the server advertised, the client shouldn't advertise the
On the server side, the "promisor.advertise" and "promisor.sendFields"
configuration options can be used to control what it advertises. On
-the client side, the "promisor.acceptFromServer" configuration option
-can be used to control what it accepts, and the "promisor.storeFields"
-option, to control what it stores. See the documentation of these
-configuration options in linkgit:git-config[1] for more information.
+the client side, the "promisor.acceptFromServer" and
+"promisor.acceptFromServerUrl" configuration options can be used to
+control what it accepts, and the "promisor.storeFields" option, to
+control what it stores. See the documentation of these configuration
+options in linkgit:git-config[1] for more information.
Note that in the future it would be nice if the "promisor-remote"
protocol capability could be used by the server, when responding to
diff --git a/promisor-remote.c b/promisor-remote.c
index 3f3924f587..ac4f54c590 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -14,6 +14,7 @@
#include "url.h"
#include "urlmatch.h"
#include "version.h"
+#include "wildmatch.h"
struct promisor_remote_config {
struct promisor_remote *promisors;
@@ -742,8 +743,79 @@ static void load_accept_from_server_url(struct repository *repo,
}
}
+static bool match_pattern_url(const char *pat, size_t pat_len,
+ const char *url, size_t url_len)
+{
+ char *p_str = xstrndup(pat, pat_len);
+ char *u_str = xstrndup(url, url_len);
+ bool res = !wildmatch(p_str, u_str, 0);
+
+ free(p_str);
+ free(u_str);
+
+ return res;
+}
+
+static bool match_one_url(const struct url_info *pi, const struct url_info *ui)
+{
+ const char *pat = pi->url;
+ const char *url = ui->url;
+
+ /*
+ * Schemes must match exactly. They are case-folded by
+ * url_normalize(), so strncmp() suffices.
+ */
+ if (pi->scheme_len != ui->scheme_len || strncmp(pat, url, pi->scheme_len))
+ return false;
+
+ /*
+ * Ports must match exactly. url_normalize() strips default
+ * ports (like 443 for https), so length and content
+ * comparisons are sufficient.
+ */
+ if (pi->port_len != ui->port_len ||
+ strncmp(pat + pi->port_off, url + ui->port_off, pi->port_len))
+ return false;
+
+ /*
+ * Match host and path separately to prevent a '*' in the host
+ * portion of the pattern from matching across the '/'
+ * boundary into the path.
+ */
+
+ return match_pattern_url(pat + pi->host_off, pi->host_len,
+ url + ui->host_off, ui->host_len) &&
+ match_pattern_url(pat + pi->path_off, pi->path_len,
+ url + ui->path_off, ui->path_len);
+}
+
+static struct allowed_url *url_matches_accept_list(
+ struct string_list *accept_urls, const char *url)
+{
+ struct string_list_item *item;
+ struct url_info url_info;
+
+ url_info.url = url_normalize(url, &url_info);
+
+ if (!url_info.url)
+ return NULL;
+
+ for_each_string_list_item(item, accept_urls) {
+ struct allowed_url *allowed = item->util;
+
+ if (match_one_url(&allowed->pattern_info, &url_info)) {
+ free(url_info.url);
+ return allowed;
+ }
+ }
+
+ free(url_info.url);
+ return NULL;
+}
+
static int should_accept_remote(enum accept_promisor accept,
struct promisor_info *advertised,
+ struct string_list *accept_urls,
struct string_list *config_info)
{
struct promisor_info *p;
@@ -756,23 +828,27 @@ static int should_accept_remote(enum accept_promisor accept,
"this remote should have been rejected earlier",
remote_name);
- if (accept == ACCEPT_ALL)
- return all_fields_match(advertised, config_info, NULL);
-
/* Get config info for that promisor remote */
item = string_list_lookup(config_info, remote_name);
- if (!item)
+ if (!item) {
/* We don't know about that remote */
+ if (accept == ACCEPT_ALL)
+ return all_fields_match(advertised, config_info, NULL);
return 0;
+ }
p = item->util;
- if (accept == ACCEPT_KNOWN_NAME)
+ /* Known remote in the allowlist? */
+ if (!strcmp(p->url, remote_url) && url_matches_accept_list(accept_urls, remote_url))
return all_fields_match(advertised, config_info, p);
- if (accept != ACCEPT_KNOWN_URL)
- BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
+ if (accept == ACCEPT_ALL)
+ return all_fields_match(advertised, config_info, NULL);
+
+ if (accept == ACCEPT_KNOWN_NAME)
+ return all_fields_match(advertised, config_info, p);
if (strcmp(p->url, remote_url)) {
warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
@@ -781,7 +857,13 @@ static int should_accept_remote(enum accept_promisor accept,
return 0;
}
- return all_fields_match(advertised, config_info, p);
+ if (accept == ACCEPT_KNOWN_URL)
+ return all_fields_match(advertised, config_info, p);
+
+ if (accept != ACCEPT_NONE)
+ BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
+
+ return 0;
}
static int skip_field_name_prefix(const char *elem, const char *field_name, const char **value)
@@ -991,7 +1073,7 @@ static void filter_promisor_remote(struct repository *repo,
/* Load and validate the acceptFromServerUrl config */
load_accept_from_server_url(repo, &accept_urls);
- if (accept == ACCEPT_NONE)
+ if (accept == ACCEPT_NONE && !accept_urls.nr)
return;
/* Parse remote info received */
@@ -1011,7 +1093,7 @@ static void filter_promisor_remote(struct repository *repo,
string_list_sort(&config_info);
}
- if (should_accept_remote(accept, advertised, &config_info)) {
+ if (should_accept_remote(accept, advertised, &accept_urls, &config_info)) {
if (!store_info)
store_info = store_info_new(repo);
if (promisor_store_advertised_fields(advertised, store_info))
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 3b39505380..0659b2ac15 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -387,6 +387,77 @@ test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" '
check_missing_objects server 1 "$oid"
'
+test_expect_success "clone with 'None' but URL allowlisted" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=None \
+ -c promisor.acceptFromServerUrl="$ENCODED_TRASH_DIRECTORY_URL/*" \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone with 'None' but URL not in allowlist" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=None \
+ -c promisor.acceptFromServerUrl="https://example.com/*" \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that the largest object is not missing on the server
+ check_missing_objects server 0 "" &&
+
+ # Reinitialize server so that the largest object is missing again
+ initialize_server 1 "$oid"
+'
+
+test_expect_success "clone with 'None' but URL allowlisted in one pattern out of two" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=None \
+ -c promisor.acceptFromServerUrl="https://example.com/*" \
+ -c promisor.acceptFromServerUrl="$ENCODED_TRASH_DIRECTORY_URL/*" \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone with 'None', URL allowlisted, but client has different URL" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ # The client configures "lop" with a different URL (serverTwo) than
+ # what the server advertises (lop). Even though the advertised URL
+ # matches the allowlist, the remote is rejected because the
+ # configured URL does not match the advertised one.
+ GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="$TRASH_DIRECTORY_URL/serverTwo" \
+ -c promisor.acceptfromserver=None \
+ -c promisor.acceptFromServerUrl="$ENCODED_TRASH_DIRECTORY_URL/*" \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that the largest object is not missing on the server
+ check_missing_objects server 0 "" &&
+
+ # Reinitialize server so that the largest object is missing again
+ initialize_server 1 "$oid"
+'
+
test_expect_success "clone with invalid promisor.acceptFromServerUrl" '
git -C server config promisor.advertise true &&
test_when_finished "rm -rf client" &&
--
2.54.0.134.gbbe8e27878.dirty
^ permalink raw reply related
* [PATCH v3 7/8] promisor-remote: auto-configure unknown remotes
From: Christian Couder @ 2026-05-19 15:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Toon Claes, Christian Couder, Christian Couder
In-Reply-To: <20260519153808.494105-1-christian.couder@gmail.com>
Previous commits have introduced the `promisor.acceptFromServerUrl`
config variable to allowlist some URLs advertised by a server through
the "promisor-remote" protocol capability.
However the new `promisor.acceptFromServerUrl` mechanism, like the old
`promisor.acceptFromServer` mechanism, still requires a remote to
already exist in the client's local configuration before it can be
accepted. This places a significant manual burden on users to
pre-configure these remotes, and creates friction for administrators
who have to troubleshoot or manually provision these setups for their
teams.
To eliminate this burden, let's automatically create a new `[remote]`
section in the client's config when a server advertises an unknown
remote whose URL matches a `promisor.acceptFromServerUrl` glob pattern.
Concretely, let's add four helpers:
- sanitize_remote_name(): turn an arbitrary URL-derived string into a
valid remote name by replacing non-alphanumeric characters,
collapsing runs of '-', and prepending "promisor-auto-".
- promisor_remote_name_from_url(): normalize the URL and extract
host+port+path to build a human-readable base name, then pass it
through sanitize_remote_name().
- configure_auto_promisor_remote(): write the remote.*.url,
remote.*.promisor and remote.*.advertisedAs keys to the repo
config.
- handle_matching_allowed_url(): pick the final name (user-supplied
alias or auto-generated), handle collisions by appending "-1",
"-2", etc., then call configure_auto_promisor_remote().
Let's also add should_accept_new_remote_url() which reuses the
url_matches_accept_list() helper introduced in a previous commit to
find a matching pattern, then delegates to handle_matching_allowed_url()
to create the remote.
And then let's call should_accept_new_remote_url() from the '!item'
(unknown remote) branch of should_accept_remote(), setting
`reload_config` so that the newly-written config is picked up.
Finally let's document all that by:
- expanding the `promisor.acceptFromServerUrl` entry to describe
auto-creation, the optional "name=" prefix syntax, the
"promisor-auto-*" generation rules, and numeric-suffix collision
handling, and by
- adding a "remote.<name>.advertisedAs" entry to "remote.adoc".
Also let's extend the precedence paragraph added by a previous commit
to mention this new acceptance path: until now, the only way for
`promisor.acceptFromServerUrl` to trigger acceptance was to allow
field updates for a known remote. With this commit, it can also trigger
auto-creation of a previously-unknown remote whose advertised URL
matches the allowlist.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/config/promisor.adoc | 39 +++--
Documentation/config/remote.adoc | 9 ++
promisor-remote.c | 201 +++++++++++++++++++++++++-
t/t5710-promisor-remote-capability.sh | 104 +++++++++++++
4 files changed, 340 insertions(+), 13 deletions(-)
diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
index 4a1ecb4425..11b5716294 100644
--- a/Documentation/config/promisor.adoc
+++ b/Documentation/config/promisor.adoc
@@ -54,7 +54,8 @@ promisor.acceptFromServer::
promisor.acceptFromServerUrl::
A glob pattern to specify which server-advertised URLs a
client is allowed to act on. When a URL matches, the client
- will accept the advertised remote as a promisor remote and may
+ will accept the advertised remote as a promisor remote, may
+ automatically create a new remote configuration for it and may
automatically accept field updates (such as authentication
tokens) from the server, even if `promisor.acceptFromServer`
is set to `none` (the default).
@@ -65,12 +66,13 @@ this option in _ANY_ config file read by Git.
+
When both `promisor.acceptFromServer` and `promisor.acceptFromServerUrl`
are set, `promisor.acceptFromServerUrl` is consulted first and takes
-precedence: if a matching pattern leads to acceptance (by accepting
-field updates for a known remote whose URL matches both the local
-configuration and the allowlist), the advertised remote is accepted
-regardless of the `promisor.acceptFromServer` setting. If no pattern
-in `promisor.acceptFromServerUrl` triggers acceptance, the decision
-is left to `promisor.acceptFromServer`.
+precedence: if a matching pattern leads to acceptance (either by
+auto-configuring an unknown remote or by accepting field updates for
+a known remote whose URL matches both the local configuration and the
+allowlist), the advertised remote is accepted regardless of the
+`promisor.acceptFromServer` setting. If no pattern in
+`promisor.acceptFromServerUrl` triggers acceptance, the decision is
+left to `promisor.acceptFromServer`.
+
Note however that, even when an advertised URL matches a pattern in
`promisor.acceptFromServerUrl`, an already-existing remote on the
@@ -85,9 +87,10 @@ documentation of that option.)
Be _VERY_ careful with these patterns: `*` matches any sequence of
characters within the 'host' and 'path' parts of a URL (but cannot
cross part boundaries). An overly broad pattern is a major security
-risk, as a matching URL allows a server to update fields (such as
-authentication tokens) on known remotes without further confirmation.
-To minimize security risks, follow these guidelines:
+risk, as a matching URL allows a server to auto-configure new remotes
+and to update fields (such as authentication tokens) on known remotes
+without further confirmation. To minimize security risks, follow these
+guidelines:
+
1. Start with a secure protocol scheme, like `https://` or `ssh://`.
+
@@ -121,6 +124,22 @@ ignored during matching. Note that embedding credentials in URLs is
discouraged. Passing authentication tokens via the `token` field of
the `promisor-remote` capability is strongly preferred.
+
+The glob pattern can optionally be prefixed with a remote name and an
+equals sign (e.g., `cdn=https://cdn.example.com/*`). If such a prefix
+is provided, accepted remotes will be saved under that name. If no
+such prefix is provided, a safe remote name will be automatically
+generated by sanitizing the URL and prefixing it with
+`promisor-auto-`.
++
+If a remote with the chosen name already exists but points to a
+different URL, Git will append a numeric suffix (e.g., `-1`, `-2`) to
+the name to prevent overwriting existing configurations. You should
+make sure that this doesn't happen often though, as remotes will be
+rejected if the numeric suffix increases too much. In all cases, the
+original name advertised by the server is recorded in the
+`remote.<name>.advertisedAs` configuration variable for tracing and
+debugging purposes.
++
For the security implications of accepting a promisor remote, see the
documentation of `promisor.acceptFromServer`. For details on the
protocol, see linkgit:gitprotocol-v2[5].
diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
index 91e46f66f5..6e2bbdf457 100644
--- a/Documentation/config/remote.adoc
+++ b/Documentation/config/remote.adoc
@@ -91,6 +91,15 @@ remote.<name>.promisor::
When set to true, this remote will be used to fetch promisor
objects.
+remote.<name>.advertisedAs::
+ When a promisor remote is automatically configured using
+ information advertised by a server through the
+ `promisor-remote` protocol capability (see
+ `promisor.acceptFromServerUrl`), the server's originally
+ advertised name is saved in this variable. This is for
+ information, tracing and debugging purposes. Users should not
+ typically modify or create such configuration entries.
+
remote.<name>.partialclonefilter::
The filter that will be applied when fetching from this promisor remote.
Changing or clearing this value will only affect fetches for new commits.
diff --git a/promisor-remote.c b/promisor-remote.c
index ac4f54c590..cbfe6672a3 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -813,10 +813,197 @@ static struct allowed_url *url_matches_accept_list(
return NULL;
}
-static int should_accept_remote(enum accept_promisor accept,
+/*
+ * Sanitize the buffer to make it a valid remote name coming from the
+ * server by:
+ *
+ * - replacing any non alphanumeric character with a '-'
+ * - stripping any leading '-',
+ * - condensing multiple '-' into one,
+ * - prepending "promisor-auto-",
+ * - validating the result.
+ */
+static int sanitize_remote_name(struct strbuf *buf, const char *url)
+{
+ char prev = '-';
+ for (size_t i = 0; i < buf->len; ) {
+ if (!isalnum(buf->buf[i]))
+ buf->buf[i] = '-';
+ if (prev == '-' && buf->buf[i] == '-') {
+ strbuf_remove(buf, i, 1);
+ } else {
+ prev = buf->buf[i];
+ i++;
+ }
+ }
+
+ strbuf_strip_suffix(buf, "-");
+
+ if (!buf->len) {
+ warning(_("couldn't generate a valid remote name from "
+ "advertised url '%s', ignoring this remote"), url);
+ return -1;
+ }
+
+ strbuf_insertstr(buf, 0, "promisor-auto-");
+
+ if (!valid_remote_name(buf->buf)) {
+ warning(_("generated remote name '%s' from advertised url '%s' "
+ "is invalid, ignoring this remote"), buf->buf, url);
+ return -1;
+ }
+
+ return 0;
+}
+
+static char *promisor_remote_name_from_url(const char *url)
+{
+ struct url_info url_info = { 0 };
+ char *normalized = url_normalize(url, &url_info);
+ struct strbuf buf = STRBUF_INIT;
+
+ if (!normalized) {
+ warning(_("couldn't normalize advertised url '%s', "
+ "ignoring this remote"), url);
+ return NULL;
+ }
+
+ if (url_info.host_len) {
+ strbuf_add(&buf, normalized + url_info.host_off, url_info.host_len);
+ strbuf_addch(&buf, '-');
+ }
+
+ if (url_info.port_len) {
+ strbuf_add(&buf, normalized + url_info.port_off, url_info.port_len);
+ strbuf_addch(&buf, '-');
+ }
+
+ if (url_info.path_len) {
+ strbuf_add(&buf, normalized + url_info.path_off, url_info.path_len);
+ strbuf_trim_trailing_dir_sep(&buf);
+ strbuf_strip_suffix(&buf, ".git");
+ }
+
+ free(normalized);
+
+ if (sanitize_remote_name(&buf, url)) {
+ strbuf_release(&buf);
+ return NULL;
+ }
+
+ return strbuf_detach(&buf, NULL);
+}
+
+static void configure_auto_promisor_remote(struct repository *repo,
+ const char *name,
+ const char *url,
+ const char *advertised_as,
+ bool reuse)
+{
+ char *key;
+
+ if (!reuse) {
+ fprintf(stderr, _("Auto-creating promisor remote '%s' for URL '%s'\n"),
+ name, url);
+
+ key = xstrfmt("remote.%s.url", name);
+ repo_config_set_gently(repo, key, url);
+ free(key);
+ }
+
+ /* NB: when reusing, this promotes an existing non-promisor remote */
+ key = xstrfmt("remote.%s.promisor", name);
+ repo_config_set_gently(repo, key, "true");
+ free(key);
+
+ if (advertised_as) {
+ key = xstrfmt("remote.%s.advertisedAs", name);
+ repo_config_set_gently(repo, key, advertised_as);
+ free(key);
+ }
+}
+
+#define MAX_REMOTES_WITH_SIMILAR_NAMES 20
+
+/* Return the allocated local name, or NULL on failure */
+static char *handle_matching_allowed_url(struct repository *repo,
+ char *allowed_name,
+ const char *remote_url,
+ const char *remote_name)
+{
+ char *name;
+ char *basename = allowed_name ?
+ xstrdup(allowed_name) :
+ promisor_remote_name_from_url(remote_url);
+ int i = 0;
+ bool reuse = false;
+
+ if (!basename)
+ return NULL;
+
+ name = xstrdup(basename);
+
+ while (i < MAX_REMOTES_WITH_SIMILAR_NAMES) {
+ char *url_key = xstrfmt("remote.%s.url", name);
+ const char *existing_url;
+ int exists = !repo_config_get_string_tmp(repo, url_key, &existing_url);
+
+ free(url_key);
+
+ if (!exists)
+ break; /* Free to use */
+
+ if (!strcmp(existing_url, remote_url)) {
+ reuse = true;
+ break; /* Same URL, so safe to reuse */
+ }
+
+ i++;
+ free(name);
+ name = xstrfmt("%s-%d", basename, i);
+ }
+
+ if (i < MAX_REMOTES_WITH_SIMILAR_NAMES) {
+ configure_auto_promisor_remote(repo, name,
+ remote_url, remote_name,
+ reuse);
+ } else {
+ warning(_("too many remotes accepted with name like '%s-X', "
+ "ignoring this remote"), basename);
+ FREE_AND_NULL(name);
+ }
+
+ free(basename);
+ return name;
+}
+
+static int should_accept_new_remote_url(struct repository *repo,
+ struct string_list *accept_urls,
+ struct promisor_info *advertised)
+{
+ struct allowed_url *allowed = url_matches_accept_list(accept_urls,
+ advertised->url);
+ if (allowed) {
+ char *name = handle_matching_allowed_url(repo,
+ allowed->remote_name,
+ advertised->url,
+ advertised->name);
+ if (name) {
+ free((char *)advertised->local_name);
+ advertised->local_name = name;
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
+static int should_accept_remote(struct repository *repo,
+ enum accept_promisor accept,
struct promisor_info *advertised,
struct string_list *accept_urls,
- struct string_list *config_info)
+ struct string_list *config_info,
+ bool *reload_config)
{
struct promisor_info *p;
struct string_list_item *item;
@@ -833,6 +1020,13 @@ static int should_accept_remote(enum accept_promisor accept,
if (!item) {
/* We don't know about that remote */
+
+ int res = should_accept_new_remote_url(repo, accept_urls, advertised);
+ if (res) {
+ *reload_config = true;
+ return res;
+ }
+
if (accept == ACCEPT_ALL)
return all_fields_match(advertised, config_info, NULL);
return 0;
@@ -1093,7 +1287,8 @@ static void filter_promisor_remote(struct repository *repo,
string_list_sort(&config_info);
}
- if (should_accept_remote(accept, advertised, &accept_urls, &config_info)) {
+ if (should_accept_remote(repo, accept, advertised, &accept_urls,
+ &config_info, &reload_config)) {
if (!store_info)
store_info = store_info_new(repo);
if (promisor_store_advertised_fields(advertised, store_info))
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 0659b2ac15..549acff23f 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -458,6 +458,107 @@ test_expect_success "clone with 'None', URL allowlisted, but client has differen
initialize_server 1 "$oid"
'
+test_expect_success "clone with URL allowlisted and no remote already configured" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+ test_when_finished "rm -f full_names" &&
+
+ GIT_NO_LAZY_FETCH=0 git clone \
+ -c promisor.acceptfromserver=None \
+ -c promisor.acceptFromServerUrl="$ENCODED_TRASH_DIRECTORY_URL/*" \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that exactly one remote has been auto-created, identified
+ # by "remote.<name>.advertisedAs" == "lop".
+ git -C client config get --all --show-names --regexp \
+ "remote\..*\.advertisedas" >full_names &&
+ test_line_count = 1 full_names &&
+ REMOTE_NAME=$(sed "s/^remote\.\(.*\)\.advertisedas .*$/\1/" full_names) &&
+
+ # Check ".url" and ".promisor" values
+ printf "%s\n" "$TRASH_DIRECTORY_URL/lop" "true" >expect &&
+ git -C client config "remote.$REMOTE_NAME.url" >actual &&
+ git -C client config "remote.$REMOTE_NAME.promisor" >>actual &&
+ test_cmp expect actual &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone with named URL allowlisted and no pre-configured remote" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ GIT_NO_LAZY_FETCH=0 git clone \
+ -c promisor.acceptfromserver=None \
+ -c promisor.acceptFromServerUrl="cdn=$ENCODED_TRASH_DIRECTORY_URL/*" \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that a remote has been auto-created with the right "cdn" name and fields.
+ printf "%s\n" "$TRASH_DIRECTORY_URL/lop" "true" "lop" >expect &&
+ git -C client config "remote.cdn.url" >actual &&
+ git -C client config "remote.cdn.promisor" >>actual &&
+ git -C client config "remote.cdn.advertisedAs" >>actual &&
+ test_cmp expect actual &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone with URL allowlisted but colliding name" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ GIT_NO_LAZY_FETCH=0 git clone -c remote.cdn.promisor=true \
+ -c remote.cdn.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.cdn.url="https://example.com/cdn" \
+ -c promisor.acceptfromserver=None \
+ -c promisor.acceptFromServerUrl="cdn=$ENCODED_TRASH_DIRECTORY_URL/*" \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that a remote has been auto-created with the right "cdn-1" name and fields.
+ printf "%s\n" "$TRASH_DIRECTORY_URL/lop" "true" "lop" >expect &&
+ git -C client config "remote.cdn-1.url" >actual &&
+ git -C client config "remote.cdn-1.promisor" >>actual &&
+ git -C client config "remote.cdn-1.advertisedAs" >>actual &&
+ test_cmp expect actual &&
+
+ # Check that the original "cdn" remote was not overwritten.
+ printf "%s\n" "https://example.com/cdn" "true" >expect &&
+ git -C client config "remote.cdn.url" >actual &&
+ git -C client config "remote.cdn.promisor" >>actual &&
+ test_cmp expect actual &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone with URL allowlisted and reusable remote" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ GIT_NO_LAZY_FETCH=0 git clone \
+ -c remote.cdn.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.cdn.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=None \
+ -c promisor.acceptFromServerUrl="cdn=$ENCODED_TRASH_DIRECTORY_URL/*" \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that the existing "cdn" remote has been properly updated.
+ printf "%s\n" "$TRASH_DIRECTORY_URL/lop" "true" "lop" "+refs/heads/*:refs/remotes/lop/*" >expect &&
+ git -C client config "remote.cdn.url" >actual &&
+ git -C client config "remote.cdn.promisor" >>actual &&
+ git -C client config "remote.cdn.advertisedAs" >>actual &&
+ git -C client config "remote.cdn.fetch" >>actual &&
+ test_cmp expect actual &&
+
+ # Check that no new "cdn-1" remote has been created.
+ test_must_fail git -C client config "remote.cdn-1.url" &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
test_expect_success "clone with invalid promisor.acceptFromServerUrl" '
git -C server config promisor.advertise true &&
test_when_finished "rm -rf client" &&
@@ -472,6 +573,9 @@ test_expect_success "clone with invalid promisor.acceptFromServerUrl" '
# Check that a warning was emitted
test_grep "invalid remote name '\''bad name'\''" err &&
+ # Check that no remote was auto-created
+ test_must_fail git -C client config get --regexp "remote\..*\.advertisedas" &&
+
# Check that the largest object is not missing on the server
check_missing_objects server 0 "" &&
--
2.54.0.134.gbbe8e27878.dirty
^ permalink raw reply related
* [PATCH v3 8/8] doc: promisor: improve acceptFromServer entry
From: Christian Couder @ 2026-05-19 15:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Toon Claes, Christian Couder, Christian Couder
In-Reply-To: <20260519153808.494105-1-christian.couder@gmail.com>
The entry for the `promisor.acceptFromServer` in
"Documentation/config/promisor.adoc" has a number of issues:
- it's not clear if new remotes and URLs can be created,
- it looks like a big block of text,
- it's not easy to see all the options,
- it's not easy to see which option is the default one,
- for "knownName", it says "advertised by the client" instead of
"advertised by the server",
- it doesn't refer to the new related `acceptFromServerUrl`
option.
Let's address all these issues by rewording large parts of it
and using bullet points for the different options.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/config/promisor.adoc | 53 ++++++++++++++++++++----------
1 file changed, 35 insertions(+), 18 deletions(-)
diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
index 11b5716294..cc728bb0b5 100644
--- a/Documentation/config/promisor.adoc
+++ b/Documentation/config/promisor.adoc
@@ -32,24 +32,41 @@ variable is set to "true", and the "name" and "url" fields are always
advertised regardless of this setting.
promisor.acceptFromServer::
- If set to "all", a client will accept all the promisor remotes
- a server might advertise using the "promisor-remote"
- capability. If set to "knownName" the client will accept
- promisor remotes which are already configured on the client
- and have the same name as those advertised by the client. This
- is not very secure, but could be used in a corporate setup
- where servers and clients are trusted to not switch name and
- URLs. If set to "knownUrl", the client will accept promisor
- remotes which have both the same name and the same URL
- configured on the client as the name and URL advertised by the
- server. This is more secure than "all" or "knownName", so it
- should be used if possible instead of those options. Default
- is "none", which means no promisor remote advertised by a
- server will be accepted. By accepting a promisor remote, the
- client agrees that the server might omit objects that are
- lazily fetchable from this promisor remote from its responses
- to "fetch" and "clone" requests from the client. Name and URL
- comparisons are case sensitive. See linkgit:gitprotocol-v2[5].
+ Controls which promisor remotes advertised by a server (using the
+ "promisor-remote" protocol capability) a client will accept. By
+ accepting a promisor remote, the client agrees that the server
+ might omit objects that are lazily fetchable from this promisor
+ remote from its responses to "fetch" and "clone" requests.
++
+Note that this option does not cause new remotes to be automatically
+created in the client's configuration. It only allows remotes which
+are somehow already configured to be trusted for the current
+operation, or their fields to be updated (if `promisor.storeFields` is
+set and the remote already exists locally). To allow Git to
+automatically create and persist new remotes from server
+advertisements, use `promisor.acceptFromServerUrl`.
++
+The available options are:
++
+* `none` (default): No promisor remote advertised by a server will be
+ accepted.
++
+* `knownUrl`: The client will accept promisor remotes that are already
+ configured on the client and have both the same name and the same URL
+ as advertised by the server. This is more secure than `all` or
+ `knownName`, and should be used if possible instead of those options.
++
+* `knownName`: The client will accept promisor remotes that are already
+ configured on the client and have the same name as those advertised
+ by the server. This is not very secure, but could be used in a corporate
+ setup where servers and clients are trusted to not switch names and URLs.
++
+* `all`: The client will accept all the promisor remotes a server might
+ advertise. This is the least secure option and should only be used in
+ fully trusted environments.
++
+Name and URL comparisons are case-sensitive. See linkgit:gitprotocol-v2[5]
+for protocol details.
promisor.acceptFromServerUrl::
A glob pattern to specify which server-advertised URLs a
--
2.54.0.134.gbbe8e27878.dirty
^ 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