* [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
* [PATCH v4 00/16] repack: incremental MIDX/bitmap-based repacking
From: Taylor Blau @ 2026-05-19 15:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1774820449.git.me@ttaylorr.com>
Here is another small reroll of my series to implement the last
remaining component of the incremental MIDX/bitmap-based repacking
strategy [1].
The changes since v3 are fairly small:
- `repack_prepare_midx_command()` now calls its third parameter
"subcommand" instead of "verb".
- `clear_incremental_midx_files()` now avoids a redundant conditional
around `r->objects` and reuses a local `struct odb_source *` when
closing any open incremental MIDX handles.
As usual, a range-diff is included below for convenience. Thanks in
advance for reviewing!
[1]: https://lore.kernel.org/git/cover.1777507303.git.me@ttaylorr.com/
Taylor Blau (16):
midx-write: handle noop writes when converting incremental chains
midx: use `strset` for retained MIDX files
midx: build `keep_hashes` array in order
midx: use `strvec` for `keep_hashes`
midx: introduce `--no-write-chain-file` for incremental MIDX writes
midx: support custom `--base` for incremental MIDX writes
repack: track the ODB source via existing_packs
midx: expose `midx_layer_contains_pack()`
repack-midx: factor out `repack_prepare_midx_command()`
repack-midx: extract `repack_fill_midx_stdin_packs()`
repack-geometry: prepare for incremental MIDX repacking
builtin/repack.c: convert `--write-midx` to an `OPT_CALLBACK`
packfile: ensure `close_pack_revindex()` frees in-memory revindex
repack: implement incremental MIDX repacking
repack: introduce `--write-midx=incremental`
repack: allow `--write-midx=incremental` without `--geometric`
Documentation/config/repack.adoc | 18 +
Documentation/git-multi-pack-index.adoc | 32 +-
Documentation/git-repack.adoc | 44 +-
builtin/multi-pack-index.c | 48 +-
builtin/repack.c | 102 +++-
midx-write.c | 206 ++++---
midx.c | 102 ++--
midx.h | 11 +-
packfile.c | 2 +
repack-geometry.c | 48 +-
repack-midx.c | 710 +++++++++++++++++++++++-
repack.c | 58 +-
repack.h | 26 +-
t/meson.build | 1 +
t/t5334-incremental-multi-pack-index.sh | 63 +++
t/t5335-compact-multi-pack-index.sh | 113 ++++
t/t7705-repack-incremental-midx.sh | 525 ++++++++++++++++++
17 files changed, 1909 insertions(+), 200 deletions(-)
create mode 100755 t/t7705-repack-incremental-midx.sh
Range-diff against v3:
1: d6c27317c25 = 1: ead11e610c8 midx-write: handle noop writes when converting incremental chains
2: 629c8d23116 = 2: ece55bf2957 midx: use `strset` for retained MIDX files
3: e303bf6a4ac = 3: 5609d1941e6 midx: build `keep_hashes` array in order
4: 42d76c70060 = 4: 13b7c808860 midx: use `strvec` for `keep_hashes`
5: 2c80aa34fac = 5: cac3fd54bf0 midx: introduce `--no-write-chain-file` for incremental MIDX writes
6: 2a05f4b86f3 = 6: 1bbb387d6b6 midx: support custom `--base` for incremental MIDX writes
7: 92aba3d366f = 7: 4a93adb3ad3 repack: track the ODB source via existing_packs
8: d3ac65c1f11 = 8: 8d1b8b1d301 midx: expose `midx_layer_contains_pack()`
9: 1bd2f194c6f ! 9: 42111e5f75d repack-midx: factor out `repack_prepare_midx_command()`
@@ Commit message
subcommands), so extract the common portions of the command setup into a
reusable `repack_prepare_midx_command()` helper.
- The extracted helper sets `git_cmd`, pushes the `multi-pack-index`
- subcommand and verb, and handles `--progress`/`--no-progress` and
- `--bitmap` flags. The remaining arguments that are specific to the
- `write` subcommand (such as `--stdin-packs`) are left to the caller.
+ The extracted helper sets `git_cmd`, pushes `multi-pack-index` and a
+ subcommand, and handles `--progress`/`--no-progress` and `--bitmap`
+ flags. The remaining arguments that are specific to the `write`
+ subcommand (such as `--stdin-packs`) are left to the caller.
No functional changes are included in this patch.
@@ repack-midx.c: static void remove_redundant_bitmaps(struct string_list *include,
+static void repack_prepare_midx_command(struct child_process *cmd,
+ struct repack_write_midx_opts *opts,
-+ const char *verb)
++ const char *subcommand)
+{
+ cmd->git_cmd = 1;
+
-+ strvec_pushl(&cmd->args, "multi-pack-index", verb, NULL);
++ strvec_pushl(&cmd->args, "multi-pack-index", subcommand, NULL);
+
+ if (opts->show_progress)
+ strvec_push(&cmd->args, "--progress");
10: 2a87a1e4561 = 10: ed76e6efd1c repack-midx: extract `repack_fill_midx_stdin_packs()`
11: 3d32b9c88da = 11: 9665f1b3a64 repack-geometry: prepare for incremental MIDX repacking
12: 1f7a5479bb8 = 12: e0db62b9f10 builtin/repack.c: convert `--write-midx` to an `OPT_CALLBACK`
13: b155f25d53c = 13: c8c846b1ac1 packfile: ensure `close_pack_revindex()` frees in-memory revindex
14: ef012314930 = 14: 78f1a98c1ea repack: implement incremental MIDX repacking
15: 04cfecd5136 ! 15: 95bf8b21fff repack: introduce `--write-midx=incremental`
@@ midx.c: void clear_midx_file(struct repository *r)
+void clear_incremental_midx_files(struct repository *r,
+ const struct strvec *keep_hashes)
+{
++ struct odb_source *source = r->objects->sources;
+ struct strbuf chain = STRBUF_INIT;
+
-+ get_midx_chain_filename(r->objects->sources, &chain);
++ get_midx_chain_filename(source, &chain);
+
-+ if (r->objects) {
-+ struct odb_source *source = r->objects->sources;
-+ for (source = r->objects->sources; source; source = source->next) {
-+ struct odb_source_files *files = odb_source_files_downcast(source);
-+ if (files->packed->midx)
-+ close_midx(files->packed->midx);
-+ files->packed->midx = NULL;
-+ }
++ for (; source; source = source->next) {
++ struct odb_source_files *files = odb_source_files_downcast(source);
++ if (files->packed->midx)
++ close_midx(files->packed->midx);
++ files->packed->midx = NULL;
+ }
+
+ if (!keep_hashes && remove_path(chain.buf))
16: 1c05dfce579 = 16: 8bd0ec98dc3 repack: allow `--write-midx=incremental` without `--geometric`
base-commit: 7bcaabddcf68bd0702697da5904c3b68c52f94cf
--
2.54.0.175.g8bd0ec98dc3
^ permalink raw reply
* [PATCH v4 01/16] midx-write: handle noop writes when converting incremental chains
From: Taylor Blau @ 2026-05-19 15:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1779206239.git.me@ttaylorr.com>
When updating a MIDX, we optimize out writes that will result in an
identical MIDX as the one we already have on disk. See b3bab9d2729
(midx-write: extract function to test whether MIDX needs updating,
2025-12-10) for more details on exactly which writes are optimized out.
If `midx_needs_update()` can't rule out any of the obvious cases (e.g.,
the checksum is invalid, we're requesting a different version, or
performing compaction which always requires an update), then we compare
the packs we're writing to the packs we already know about. If there are
an equal number of packs being written as there are in any existing
MIDX layer(s), then we compare the packs by their name.
This comparison fails when we have an incremental MIDX chain with
at least two layers, since we do not recursively peel through earlier
layers, instead treating the `->pack_names` array of the tip MIDX layer
as containing all `m->num_packs + m->num_packs_in_base` packs.
Adjust this to instead look through the MIDX layers one by one when
comparing pack names. While we're at it, fix a typo above in the same
function.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx-write.c | 18 ++++++++++--------
t/t5334-incremental-multi-pack-index.sh | 16 ++++++++++++++++
2 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/midx-write.c b/midx-write.c
index a25cab75aba..9328f65a201 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1152,7 +1152,7 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c
/*
* Ensure that we have a valid checksum before consulting the
- * exisiting MIDX in order to determine if we can avoid an
+ * existing MIDX in order to determine if we can avoid an
* update.
*
* This is necessary because the given MIDX is loaded directly
@@ -1208,14 +1208,16 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c
BUG("same pack added twice?");
}
- for (uint32_t i = 0; i < ctx->nr; i++) {
- strbuf_reset(&buf);
- strbuf_addstr(&buf, midx->pack_names[i]);
- strbuf_strip_suffix(&buf, ".idx");
+ for (struct multi_pack_index *m = midx; m; m = m->base_midx) {
+ for (uint32_t i = 0; i < m->num_packs; i++) {
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, m->pack_names[i]);
+ strbuf_strip_suffix(&buf, ".idx");
- if (!strset_contains(&packs, buf.buf))
- goto out;
- strset_remove(&packs, buf.buf);
+ if (!strset_contains(&packs, buf.buf))
+ goto out;
+ strset_remove(&packs, buf.buf);
+ }
}
needed = false;
diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
index 99c7d44d8e9..c9f5b4e87aa 100755
--- a/t/t5334-incremental-multi-pack-index.sh
+++ b/t/t5334-incremental-multi-pack-index.sh
@@ -132,4 +132,20 @@ test_expect_success 'relink existing MIDX layer' '
'
+test_expect_success 'non-incremental write with existing incremental chain' '
+ git init non-incremental-write-with-existing &&
+ test_when_finished "rm -fr non-incremental-write-with-existing" &&
+
+ (
+ cd non-incremental-write-with-existing &&
+
+ git config set maintenance.auto false &&
+
+ write_midx_layer &&
+ write_midx_layer &&
+
+ git multi-pack-index write
+ )
+'
+
test_done
--
2.54.0.175.g8bd0ec98dc3
^ permalink raw reply related
* [PATCH v4 02/16] midx: use `strset` for retained MIDX files
From: Taylor Blau @ 2026-05-19 15:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1779206239.git.me@ttaylorr.com>
Both `clear_midx_files_ext()` and `clear_incremental_midx_files_ext()`
build a list of filenames to keep while pruning stale MIDX files. Today
they hand-roll an array instead of using a `strset`, thus requiring us
to pass an additional length parameter, and makes lookups linear.
Replace the bare array with a `strset` which can be passed around as a
single parameter. Though it improves lookup performance, the difference
is likely immeasurable given how small the keep_hashes array typically
is.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx.c | 57 +++++++++++++++++++++++++++------------------------------
1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/midx.c b/midx.c
index 81d6ab11e6e..f75e3c9fa6d 100644
--- a/midx.c
+++ b/midx.c
@@ -758,8 +758,7 @@ int midx_checksum_valid(struct multi_pack_index *m)
}
struct clear_midx_data {
- char **keep;
- uint32_t keep_nr;
+ struct strset keep;
const char *ext;
};
@@ -767,15 +766,12 @@ static void clear_midx_file_ext(const char *full_path, size_t full_path_len UNUS
const char *file_name, void *_data)
{
struct clear_midx_data *data = _data;
- uint32_t i;
if (!(starts_with(file_name, "multi-pack-index-") &&
ends_with(file_name, data->ext)))
return;
- for (i = 0; i < data->keep_nr; i++) {
- if (!strcmp(data->keep[i], file_name))
- return;
- }
+ if (strset_contains(&data->keep, file_name))
+ return;
if (unlink(full_path))
die_errno(_("failed to remove %s"), full_path);
}
@@ -783,48 +779,49 @@ static void clear_midx_file_ext(const char *full_path, size_t full_path_len UNUS
void clear_midx_files_ext(struct odb_source *source, const char *ext,
const char *keep_hash)
{
- struct clear_midx_data data;
- memset(&data, 0, sizeof(struct clear_midx_data));
+ struct clear_midx_data data = {
+ .keep = STRSET_INIT,
+ .ext = ext,
+ };
if (keep_hash) {
- ALLOC_ARRAY(data.keep, 1);
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addf(&buf, "multi-pack-index-%s.%s", keep_hash, ext);
- data.keep[0] = xstrfmt("multi-pack-index-%s.%s", keep_hash, ext);
- data.keep_nr = 1;
+ strset_add(&data.keep, buf.buf);
+
+ strbuf_release(&buf);
}
- data.ext = ext;
- for_each_file_in_pack_dir(source->path,
- clear_midx_file_ext,
- &data);
+ for_each_file_in_pack_dir(source->path, clear_midx_file_ext, &data);
- if (keep_hash)
- free(data.keep[0]);
- free(data.keep);
+ strset_clear(&data.keep);
}
void clear_incremental_midx_files_ext(struct odb_source *source, const char *ext,
char **keep_hashes,
uint32_t hashes_nr)
{
- struct clear_midx_data data;
+ struct clear_midx_data data = {
+ .keep = STRSET_INIT,
+ .ext = ext,
+ };
+ struct strbuf buf = STRBUF_INIT;
uint32_t i;
- memset(&data, 0, sizeof(struct clear_midx_data));
+ for (i = 0; i < hashes_nr; i++) {
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "multi-pack-index-%s.%s", keep_hashes[i],
+ ext);
- ALLOC_ARRAY(data.keep, hashes_nr);
- for (i = 0; i < hashes_nr; i++)
- data.keep[i] = xstrfmt("multi-pack-index-%s.%s", keep_hashes[i],
- ext);
- data.keep_nr = hashes_nr;
- data.ext = ext;
+ strset_add(&data.keep, buf.buf);
+ }
for_each_file_in_pack_subdir(source->path, "multi-pack-index.d",
clear_midx_file_ext, &data);
- for (i = 0; i < hashes_nr; i++)
- free(data.keep[i]);
- free(data.keep);
+ strbuf_release(&buf);
+ strset_clear(&data.keep);
}
void clear_midx_file(struct repository *r)
--
2.54.0.175.g8bd0ec98dc3
^ 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