* Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`
From: Derrick Stolee @ 2026-04-27 13:16 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Elijah Newren
In-Reply-To: <a705852723fbe88e94ad3de1daba548dbce32211.1776472347.git.gitgitgadget@gmail.com>
On 4/17/2026 8:32 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
(I'm sorry that I'm reviewing out of order. This reply includes my
feelings about patch 3 after reading both.)
> +/*
> + * Enumerate blob OIDs from a single commit's diff, inserting them into blobs.
> + * Skips files whose userdiff driver explicitly declares binary status
> + * (drv->binary > 0), since patch-ID uses oid_to_hex() for those and
> + * never reads blob content. Use userdiff_find_by_path() since
> + * diff_filespec_load_driver() is static in diff.c.
> + *
> + * Clean up with diff_queue_clear() (from diffcore.h).
> + */
> +static void collect_diff_blob_oids(struct commit *commit,
> + struct diff_options *opts,
> + struct oidset *blobs)
I think that this is generally a good idea, though I worry that
having this hidden in builtin/log.c may not be the right long-
term home.
I expect that we'll find more and more examples where we want to
prefetch blobs in different operations, those that exist now and
those that may be created in the future. It would be preferred if
they could automatically take advantage of the logic already in
diff_queued_diff_prefetch() within diffcore_std() in diff.c.
Ultimately, _this_ patch cares about a diff. Could we compute a
"diff prep" computation using the core diff library instead of
inventing a second queue of results for diffing?
Patch 3 cares about a "scan prep" which cares about loading all
blobs for a given tree with respect to a pathspec. This is very
similar to what a checkout would do, though it ultimately uses
a form of diff to find out what change should be applied to the
working directory. Perhaps 'git archive' is a better matching
example.
I don't mean to make your series more complicated. I value what
you're doing and can see how your current attention can be used
to make further improvements later. By implementing things in a
common location, then we can have later integrations add to the
confidence in the feature through tests covering each user-facing
use.
I'm not sure if it makes sense to attempt to create a universal
library method that would be used by builtin/log.c _and_ diff.c,
at least not right now. I'm most interested in having this logic
be more reusable in the future without needing to move code
across files.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2 0/8] Auto-configure advertised remotes via URL allowlist
From: Christian Couder @ 2026-04-27 13:00 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren
In-Reply-To: <20260427124108.3524129-1-christian.couder@gmail.com>
Sorry, it looks like I sent this series in reply to:
https://lore.kernel.org/git/20251223111113.47473-1-christian.couder@gmail.com/
instead of:
https://lore.kernel.org/git/20260323080520.887550-1-christian.couder@gmail.com/
I will try to do better next time.
Also I forgot to say that this series is based on a merge of 'master'
@ v2.54.0 and 'cc/promisor-auto-config-url' (which is in 'next' but is
marked with "Will merge to master" in the last "What's cooking..."
email).
On Mon, Apr 27, 2026 at 2:41 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> 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 new. It 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 v1
> ======================
>
> Thanks to Patrick and Junio for reviewing the previous versions of
> this series and of the preparatory series.
>
> - A lot of preparatory patches have been moved to a preparatory series
> that has already been merged. See:
>
> https://lore.kernel.org/git/20260407115243.358642-1-christian.couder@gmail.com/
>
> This is why this v2 contains only 8 patches compared to 16 patches
> in v1.
>
> - Everywhere in this series "whitelist" as been replaced with
> "allowlist".
>
> - In the tests added in this series, the new $TRASH_DIRECTORY_URL and
> $ENCODED_TRASH_DIRECTORY_URL introduced by the preparatory series
> are used instead of the previous $PWD_URL and $ENCODED_PWD_URL.
>
> - Patch 1/8 ("t5710: simplify 'mkdir X' followed by 'git -C X init'")
> is new.
>
> - Patch 3/8 ("urlmatch: add url_normalize_pattern() helper") replaces
> patch 3/16 ("urlmatch: add url_is_valid_pattern() helper") because
> in subsequent patches we now normalize patterns to validate them
> and match them component by component against URLs.
>
> - In patch 5/8, previously 13/16, ("promisor-remote: introduce
> promisor.acceptFromServerUrl"):
>
> - We add a `struct url_info pattern_info;` to `struct allowed_url`,
> so we can validate patterns using url_normalize_pattern() and, in
> a subsequent patch, match URLs component by component. This
> requires a new allowed_url_free() function that is passed to
> string_list_clear_func() to clear the `struct allowed_url`
> instances.
>
> - We don't use a `static struct string_list` to store the URL
> patterns we accept. Instead we load them from the config into a
> `struct string_list` passed as argument. The function doing this
> is renamed accordingly from accept_from_server_url() to
> load_accept_from_server_url().
>
> - A "clone with invalid promisor.acceptFromServerUrl" test is moved
> from patch 15/16 to this patch as it's more relevant in this
> patch (where we validate the content of the
> `promisor.acceptFromServerUrl` environment variable).
>
> - In patch 6/8, previously 14/16, ("promisor-remote: trust known
> remotes matching acceptFromServerUrl"):
>
> - In the commit message, an example, which shows how the new
> "acceptFromServerUrl" config option can be useful, is added.
>
> - The matching of URLs advertised by the server to URLs patterns
> from the config, is now performed component by component. This is
> reflected in the commit message, the documentation and the
> code. This ensures a `*` in the host pattern cannot cross into
> the path.
>
> - In the code, we add a new match_one_url() function to perform the
> matching.
>
> - In patch 7/8, previously 15/16 ("promisor-remote: auto-configure
> unknown remotes"):
>
> - In the doc, the unclear "considered trusted by the client" is
> clarified using "a client is allowed to act on" and subsequent
> explanations. In general the doc is also improved a bit.
>
> - In the tests, parsing the "remote.<name>.advertisedAs" config
> option is now more careful about the possibility that more than
> one such options exist.
>
> - The test that was moved to patch 5/8 is still enhanced a bit in
> this commit by checking that no "remote.<name>.advertisedAs"
> config option has been added.
>
> CI tests
> ========
>
> They all pass, see:
>
> https://github.com/chriscool/git/actions/runs/24992478331
>
> Range diff since v1
> ===================
>
> 1: b2894eb33a < -: ---------- promisor-remote: try accepted remotes before others in get_direct()
> -: ---------- > 1: 44e9a16455 t5710: simplify 'mkdir X' followed by 'git -C X init'
> 2: a3206a6ae9 = 2: 42f174910c urlmatch: change 'allow_globs' arg to bool
> 3: 51bbf65c52 < -: ---------- urlmatch: add url_is_valid_pattern() helper
> 4: f367beef72 < -: ---------- promisor-remote: clarify that a remote is ignored
> 5: 1faf74cb3f < -: ---------- promisor-remote: refactor has_control_char()
> 6: 40cf0af639 < -: ---------- promisor-remote: refactor accept_from_server()
> 7: b75dca8037 < -: ---------- promisor-remote: keep accepted promisor_info structs alive
> 8: f5e55dc407 < -: ---------- promisor-remote: remove the 'accepted' strvec
> -: ---------- > 3: 8088374458 urlmatch: add url_normalize_pattern() helper
> 9: 63c1db30de ! 4: 6bfda89a79 promisor-remote: add 'local_name' to 'struct promisor_info'
> @@ Commit message
> 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 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()
> @@ Commit message
>
> ## promisor-remote.c ##
> @@ promisor-remote.c: static struct string_list *fields_stored(void)
> -
> - /*
> * Struct for promisor remotes involved in the "promisor-remote"
> -- * protocol capability.
> -+ * protocol capability:
> + * 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.
> -+ * - "name" is the name the server advertised.
> -+ * - "local_name" is the name we use locally (may be auto-generated).
> -+ *
> + * 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 *local_name;
> +- 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;
> 10: e9b8a64ab8 < -: ---------- promisor-remote: pass config entry to all_fields_match() directly
> 11: 2e1260190a < -: ---------- promisor-remote: refactor should_accept_remote() control flow
> 12: b33f06173a < -: ---------- t5710: use proper file:// URIs for absolute paths
> 13: 681b03e248 ! 5: fefa17e6dd promisor-remote: introduce promisor.acceptFromServerUrl
> @@ promisor-remote.c: static bool has_control_char(const char *s)
> +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;
> @@ promisor-remote.c: static bool has_control_char(const char *s)
> + p = dup;
> + }
> +
> -+ if (has_control_char(p) || !url_is_valid_pattern(p)) {
> ++ if (has_control_char(p)) {
> + warning(_("invalid url pattern '%s' "
> + "in '%s' from promisor.acceptFromServerUrl config"), p, url);
> + free(dup);
> @@ promisor-remote.c: static bool has_control_char(const char *s)
> + 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 struct string_list *accept_from_server_url(struct repository *repo)
> ++static void load_accept_from_server_url(struct repository *repo,
> ++ struct string_list *accept_urls)
> +{
> -+ static struct string_list accept_urls = STRING_LIST_INIT_DUP;
> -+ static int initialized;
> + const struct string_list *config_urls;
> +
> -+ if (initialized)
> -+ return &accept_urls;
> -+
> -+ initialized = 1;
> -+
> + if (!repo_config_get_string_multi(repo, "promisor.acceptfromserverurl", &config_urls)) {
> + struct string_list_item *item;
> +
> @@ promisor-remote.c: static bool has_control_char(const char *s)
> + 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 = string_list_append(accept_urls, item->string);
> + new->util = allowed;
> + }
> + }
> + }
> -+
> -+ return &accept_urls;
> +}
> +
> static int should_accept_remote(enum accept_promisor accept,
> @@ promisor-remote.c: 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);
> -+ /* Pre-load and validate the acceptFromServerUrl config */
> -+ (void)accept_from_server_url(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;
> +@@ promisor-remote.c: 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);
> +
> + ## t/t5710-promisor-remote-capability.sh ##
> +@@ t/t5710-promisor-remote-capability.sh: 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" &&
> 14: 8c04e48d66 ! 6: 2f238d0a7a promisor-remote: trust known remotes matching acceptFromServerUrl
> @@ Commit message
>
> To enable such targeted updates for trusted URLs, let's use the URL
> patterns from `promisor.acceptFromServerUrl` as an additional URL
> - based whitelist.
> + 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).
>
> - (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 whitelist.)
> + 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.
>
> 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 whitelist is still accepted.
> + the allowlist is still accepted.
>
> To prepare for this new logic, let's also:
>
> @@ Commit message
> 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 whitelist for known remotes,
> - including the URL normalization behavior, and let's mention it in
> - "gitprotocol-v2.adoc".
> + "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".
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>
> @@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
> comparisons are case sensitive. See linkgit:gitprotocol-v2[5].
>
> +promisor.acceptFromServerUrl::
> -+ A glob pattern to specify which URLs advertised by a server
> -+ are considered trusted by the client. This option acts as an
> -+ additive security whitelist that works in conjunction with
> -+ `promisor.acceptFromServer`.
> ++ 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.
> ++
> -+Be _VERY_ careful with these glob patterns, as it can be a big
> -+security hole to allow any advertised remote to be auto-configured!
> ++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://`.
> @@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
> + your specific organization or namespace (e.g.,
> + `https://gitlab.com/your-org/*`).
> ++
> -+3. Don't use globs (`*`) in the domain name. For example
> -+ `https://cdn.example.com/*` is much safer than
> -+ `https://*.example.com/*`, because the latter matches
> -+ `https://evil-hacker.net/fake.example.com/repo`.
> ++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. Make sure to have a `/` at the end of the domain name (or the end
> -+ of specific directories). For example `https://cdn.example.com/*`
> -+ is much safer than `https://cdn.example.com*`, because the latter
> -+ matches `https://cdn.example.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, the advertised URL is normalized: the scheme and
> -+host are lowercased, percent-encoded characters are decoded where
> -+possible, and path segments like `..` are resolved. Glob patterns
> -+are matched against this normalized URL as-is, so patterns should
> -+be written in normalized form (e.g., lowercase scheme and host).
> ++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).
> ++
> -+Even if `promisor.acceptFromServer` is set to `None` (the default),
> -+Git will still accept field updates (like tokens) for known remotes,
> -+provided their URLs match a pattern in
> -+`promisor.acceptFromServerUrl`. See linkgit:gitprotocol-v2[5] for
> -+details on the protocol.
> ++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
> @@ promisor-remote.c
>
> struct promisor_remote_config {
> struct promisor_remote *promisors;
> -@@ promisor-remote.c: static struct string_list *accept_from_server_url(struct repository *repo)
> - return &accept_urls;
> +@@ promisor-remote.c: static void load_accept_from_server_url(struct repository *repo,
> + }
> }
>
> ++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)
> ++ 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;
> ++}
> ++
> +static struct allowed_url *url_matches_accept_list(
> + struct string_list *accept_urls, const char *url)
> +{
> + struct string_list_item *item;
> -+ char *normalized = url_normalize(url, NULL);
> ++ struct url_info url_info;
> ++
> ++ url_info.url = url_normalize(url, &url_info);
> +
> -+ if (!normalized)
> ++ if (!url_info.url)
> + return NULL;
> +
> + for_each_string_list_item(item, accept_urls) {
> + struct allowed_url *allowed = item->util;
> +
> -+ if (!wildmatch(allowed->url_pattern, normalized, 0)) {
> -+ free(normalized);
> ++ if (match_one_url(&allowed->pattern_info, &url_info)) {
> ++ free(url_info.url);
> + return allowed;
> + }
> + }
> +
> -+ free(normalized);
> ++ free(url_info.url);
> + return NULL;
> +}
> +
> @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor 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 whitelist!
> ++ * matches the acceptFromServerUrl allowlist!
> + */
> + if (url_matches_accept_list(accept_urls, remote_url))
> + return all_fields_match(advertised, config_info, p);
> @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
>
> static int skip_field_name_prefix(const char *elem, const char *field_name, const char **value)
> @@ promisor-remote.c: 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);
> -- /* Pre-load and validate the acceptFromServerUrl config */
> -- (void)accept_from_server_url(repo);
> -+ struct string_list *accept_urls = accept_from_server_url(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)
> ++ if (accept == ACCEPT_NONE && !accept_urls.nr)
> return;
>
> /* Parse remote info received */
> @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
> }
>
> - if (should_accept_remote(accept, advertised, &config_info)) {
> -+ if (should_accept_remote(accept, advertised, accept_urls, &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))
> @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
> check_missing_objects server 1 "$oid"
> '
>
> -+test_expect_success "clone with 'None' but URL whitelisted" '
> ++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="$PWD_URL/lop" \
> ++ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
> + -c promisor.acceptfromserver=None \
> -+ -c promisor.acceptFromServerUrl="$ENCODED_PWD_URL/*" \
> ++ -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 whitelist" '
> ++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="$PWD_URL/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 &&
> @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
> + initialize_server 1 "$oid"
> +'
> +
> -+test_expect_success "clone with 'None' but URL whitelisted in one pattern out of two" '
> ++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="$PWD_URL/lop" \
> ++ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
> + -c promisor.acceptfromserver=None \
> + -c promisor.acceptFromServerUrl="https://example.com/*" \
> -+ -c promisor.acceptFromServerUrl="$ENCODED_PWD_URL/*" \
> ++ -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 whitelisted, but client has different URL" '
> ++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 whitelist, the remote is rejected because the
> ++ # 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="$PWD_URL/serverTwo" \
> ++ -c remote.lop.url="$TRASH_DIRECTORY_URL/serverTwo" \
> + -c promisor.acceptfromserver=None \
> -+ -c promisor.acceptFromServerUrl="$ENCODED_PWD_URL/*" \
> ++ -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
> @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
> + initialize_server 1 "$oid"
> +'
> +
> - test_expect_success "clone with promisor.sendFields" '
> + test_expect_success "clone with invalid promisor.acceptFromServerUrl" '
> git -C server config promisor.advertise true &&
> test_when_finished "rm -rf client" &&
> -@@ t/t5710-promisor-remote-capability.sh: test_expect_success "subsequent fetch from a client when promisor.advertise is f
> - check_missing_objects server 1 "$oid"
> - '
> -
> -+
> -+
> - test_done
> 15: 314150a860 ! 7: a077f33df4 promisor-remote: auto-configure unknown remotes
> @@ Commit message
> promisor-remote: auto-configure unknown remotes
>
> Previous commits have introduced the `promisor.acceptFromServerUrl`
> - config variable to whitelist some URLs advertised by a server through
> + 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
> @@ Commit message
>
> ## Documentation/config/promisor.adoc ##
> @@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
> -
> promisor.acceptFromServerUrl::
> - A glob pattern to specify which URLs advertised by a server
> -- are considered trusted by the client. This option acts as an
> -- additive security whitelist that works in conjunction with
> -- `promisor.acceptFromServer`.
> -+ are allowed to be auto-configured (created and persisted) on
> -+ the client side. Unlike `promisor.acceptFromServer`, which
> -+ only accepts already configured remotes, a match against this
> -+ option instructs Git to write a new `[remote "<name>"]`
> -+ section to the client's configuration.
> + 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).
> +@@ Documentation/config/promisor.adoc: this option in _ANY_ config file read by Git.
> + 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://`.
> +
> - This option can appear multiple times in config files. An advertised
> - URL will be accepted if it matches _ANY_ glob pattern specified by
> -@@ Documentation/config/promisor.adoc: possible, and path segments like `..` are resolved. Glob patterns
> - are matched against this normalized URL as-is, so patterns should
> - be written in normalized form (e.g., lowercase scheme and host).
> +@@ 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).
> +
> --Even if `promisor.acceptFromServer` is set to `None` (the default),
> --Git will still accept field updates (like tokens) for known remotes,
> --provided their URLs match a pattern in
> --`promisor.acceptFromServerUrl`. See linkgit:gitprotocol-v2[5] for
> --details on the protocol.
> +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.
> ++`promisor-auto-`.
> ++
> -+Note that this option acts as an additive security whitelist. It works
> -+in conjunction with `promisor.acceptFromServer` (see the documentation
> -+of that option for the implications of accepting a promisor
> -+remote). Even if `promisor.acceptFromServer` is set to `None` (the
> -+default), Git will still automatically configure new remotes, and
> -+accept field updates (like tokens) for known remotes, provided their
> -+URLs match a pattern in `promisor.acceptFromServerUrl`. See
> -+linkgit:gitprotocol-v2[5] for details on the protocol.
> -
> - promisor.checkFields::
> - A comma or space separated list of additional remote related
> ++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].
>
> ## Documentation/config/remote.adoc ##
> @@ Documentation/config/remote.adoc: remote.<name>.promisor::
> @@ promisor-remote.c: 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,
> +- 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))
>
> ## t/t5710-promisor-remote-capability.sh ##
> -@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', URL whitelisted, but client has differen
> +@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', URL allowlisted, but client has differen
> initialize_server 1 "$oid"
> '
>
> -+test_expect_success "clone with URL whitelisted and no remote already configured" '
> ++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_PWD_URL/*" \
> ++ -c promisor.acceptFromServerUrl="$ENCODED_TRASH_DIRECTORY_URL/*" \
> + --no-local --filter="blob:limit=5k" server client &&
> +
> -+ # Check that a remote has been auto-created with the right fields.
> -+ # The remote is identified by "remote.<name>.advertisedAs" == "lop".
> -+ FULL_NAME=$(git -C client config --name-only --get-regexp "remote\..*\.advertisedas" "^lop$") &&
> -+ REMOTE_NAME=$(echo "$FULL_NAME" | sed "s/remote\.\(.*\)\.advertisedas/\1/") &&
> ++ # 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" "$PWD_URL/lop" "true" >expect &&
> ++ 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 &&
> @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', U
> + check_missing_objects server 1 "$oid"
> +'
> +
> -+test_expect_success "clone with named URL whitelisted and no pre-configured remote" '
> ++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_PWD_URL/*" \
> ++ -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" "$PWD_URL/lop" "true" "lop" >expect &&
> ++ 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 &&
> @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', U
> + check_missing_objects server 1 "$oid"
> +'
> +
> -+test_expect_success "clone with URL whitelisted but colliding name" '
> ++test_expect_success "clone with URL allowlisted but colliding name" '
> + git -C server config promisor.advertise true &&
> + test_when_finished "rm -rf client" &&
> +
> @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', U
> + -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_PWD_URL/*" \
> ++ -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" "$PWD_URL/lop" "true" "lop" >expect &&
> ++ 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 &&
> @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', U
> + check_missing_objects server 1 "$oid"
> +'
> +
> -+test_expect_success "clone with URL whitelisted and reusable remote" '
> ++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="$PWD_URL/lop" \
> ++ -c remote.cdn.url="$TRASH_DIRECTORY_URL/lop" \
> + -c promisor.acceptfromserver=None \
> -+ -c promisor.acceptFromServerUrl="cdn=$ENCODED_PWD_URL/*" \
> ++ -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" "$PWD_URL/lop" "true" "lop" "+refs/heads/*:refs/remotes/lop/*" >expect &&
> ++ 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 &&
> @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', U
> + 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 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 "" &&
> -+
> -+ # Reinitialize server so that the largest object is missing again
> -+ initialize_server 1 "$oid"
> -+'
> -+
> - test_expect_success "clone with promisor.sendFields" '
> + test_expect_success "clone with invalid promisor.acceptFromServerUrl" '
> git -C server config promisor.advertise true &&
> test_when_finished "rm -rf client" &&
> +@@ t/t5710-promisor-remote-capability.sh: 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 "" &&
> +
> 16: 20f70b52bb ! 8: b68b9497aa doc: promisor: improve acceptFromServer entry
> @@ Documentation/config/promisor.adoc: variable is set to "true", and the "name" an
> +for protocol details.
>
> promisor.acceptFromServerUrl::
> - A glob pattern to specify which URLs advertised by a server
> + A glob pattern to specify which server-advertised URLs a
>
>
> 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 | 123 ++++++--
> Documentation/config/remote.adoc | 9 +
> Documentation/gitprotocol-v2.adoc | 9 +-
> promisor-remote.c | 410 ++++++++++++++++++++++++--
> t/t5710-promisor-remote-capability.sh | 202 ++++++++++++-
> urlmatch.c | 11 +-
> urlmatch.h | 12 +
> 7 files changed, 730 insertions(+), 46 deletions(-)
>
> --
> 2.54.0.19.gb68b9497aa
>
^ permalink raw reply
* Re: [PATCH v2 3/3] grep: prefetch necessary blobs
From: Derrick Stolee @ 2026-04-27 12:59 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Elijah Newren
In-Reply-To: <8fbfe69bc4d0c6166967986f24861ffa393ed7cf.1776472347.git.gitgitgadget@gmail.com>
On 4/17/2026 8:32 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> In partial clones, `git grep` fetches necessary blobs on-demand one
> at a time, which can be very slow. In partial clones, add an extra
> preliminary walk over the tree similar to grep_tree() which collects
> the blobs of interest, and then prefetches them.
A log of the code is about walking trees to find blobs matching
the input pathspec, with this being the core method:
> +static void collect_blob_oids_for_tree(struct repository *repo,
> + const struct pathspec *pathspec,
> + struct tree_desc *tree,
> + struct strbuf *base,
> + int tn_len,
> + struct oidset *blob_oids)
And in your test, you set up a repo to have three blobs with
matches in two of the files:
> +test_expect_success 'grep of revision in partial clone does bulk prefetch' '
> + test_when_finished "rm -rf grep-partial-src grep-partial" &&
> +
> + git init grep-partial-src &&
> + (
> + cd grep-partial-src &&
> + git config uploadpack.allowfilter 1 &&
> + git config uploadpack.allowanysha1inwant 1 &&
> + echo "needle in haystack" >searchme &&
> + echo "no match here" >other &&
> + mkdir subdir &&
> + echo "needle again" >subdir/deep &&
> + git add . &&
> + git commit -m "initial"
> + ) &&
But then the command downloads all of the blobs, not using a
pathspec:
> + # grep HEAD should batch-prefetch all blobs in one request.
> + GIT_TRACE2_EVENT="$(pwd)/grep-trace" \
> + git -C grep-partial grep -c "needle" HEAD >result &&
> +
> + # Should find matches in two files.
> + test_line_count = 2 result &&
> +
> + # Should have prefetched all 3 objects at once
> + test_trace2_data promisor fetch_count 3 <grep-trace
> +'
I think your code is correct, but I'd like to see a test
here that demonstrates a pathspec filter on the 'grep'
command to help filter out a blob that has a matching string.
Perhaps something like:
* matches.txt (has needle)
* nomatch.txt (does not have needle)
* matches.md (has needle)
and then 'git grep -c "needle" HEAD -- *.txt' would
download two blobs and find one match. A second run without
the pathspec would download one blob and find two matches.
Does that make sense as a test?
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH 14/16] promisor-remote: trust known remotes matching acceptFromServerUrl
From: Christian Couder @ 2026-04-27 12:45 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Taylor Blau, Karthik Nayak, Elijah Newren,
Christian Couder
In-Reply-To: <acUk0vTuj8COlvgf@pks.im>
On Thu, Mar 26, 2026 at 1:21 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Mar 23, 2026 at 09:05:17AM +0100, Christian Couder wrote:
> > To enable such targeted updates for trusted URLs, let's use the URL
> > patterns from `promisor.acceptFromServerUrl` as an additional URL
> > based whitelist.
> >
> > 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).
> >
> > (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 whitelist.)
> >
> > 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 whitelist is still 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, so url_matches_accept_list()
> > is only called 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`.
> >
> > Let's then properly document `promisor.acceptFromServerUrl` in
> > "promisor.adoc" as an additive security whitelist for known remotes,
> > including the URL normalization behavior, and let's mention it in
> > "gitprotocol-v2.adoc".
>
> I feel like the description is steering a bit too strongly into the
> direction of a step-by-step instruction of how to implement the change
> rather than explaining what's done and why it's done this way.
I have added the following:
"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."
> > +promisor.acceptFromServerUrl::
> > + A glob pattern to specify which URLs advertised by a server
> > + are considered trusted by the client. This option acts as an
> > + additive security whitelist that works in conjunction with
> > + `promisor.acceptFromServer`.
> > ++
> > +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.
> > ++
> > +Be _VERY_ careful with these glob patterns, as it can be a big
> > +security hole to allow any advertised remote to be auto-configured!
> > +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. Don't use globs (`*`) in the domain name. For example
> > + `https://cdn.example.com/*` is much safer than
> > + `https://*.example.com/*`, because the latter matches
> > + `https://evil-hacker.net/fake.example.com/repo`.
> > ++
> > +4. Make sure to have a `/` at the end of the domain name (or the end
> > + of specific directories). For example `https://cdn.example.com/*`
> > + is much safer than `https://cdn.example.com*`, because the latter
> > + matches `https://cdn.example.com.hacker.net/repo`.
> > ++
> > +Before matching, the advertised URL is normalized: the scheme and
> > +host are lowercased, percent-encoded characters are decoded where
> > +possible, and path segments like `..` are resolved. Glob patterns
> > +are matched against this normalized URL as-is, so patterns should
> > +be written in normalized form (e.g., lowercase scheme and host).
> > ++
> > +Even if `promisor.acceptFromServer` is set to `None` (the default),
> > +Git will still accept field updates (like tokens) for known remotes,
> > +provided their URLs match a pattern in
> > +`promisor.acceptFromServerUrl`. See linkgit:gitprotocol-v2[5] for
> > +details on the protocol.
>
> Given that there's a bunch to process here, would it make sense to give
> users an example for how to do it properly?
I can give an example like the one I added to the commit message (see
above), but it might be too lax for some use cases. Perhaps in many
organizations only a single repo will ever require to accept promisor
remotes, so giving an example with the `--global` flag like:
git config set --global promisor.acceptFromServerUrl "https://my-org.com/*"
could make everyone's config a bit more vulnerable than necessary.
I think it's better to nudge people to think through the four steps
above, rather than to encourage them to copy-paste something that
might not be very well suited to their needs.
> I also wonder why we require a new config entry instead of extending
> `promisor.acceptFromRemote` to have for example a new "url:https://..."
> setting. Are there cases where you would ever want to use the new
> URL-based schema with a different setting than "all"?
Yes, I think the example in the commit message shows why having both:
- `promisor.acceptFromRemote` that you can set for example to
"knownUrl" only in some specific repos where you accept external
promisor remotes that you configure manually, and
- `promisor.acceptFromRemoteUrl` that you can set for example to
"https://my-org.com/*" globally
can be relatively simple and quite powerful:
- all internal remotes (with URLs in https://my-org.com/) are
automatically accepted in all the repos,
- in certain specific repos, some external remotes (with names and
URLs that are manually configured in the repos) are also accepted.
> I guess the case with "none" is exactly that, where you may auto-update
> configured remotes. But I wonder whether it would be more sensible to
> split out behaviour of accepting and updating promisors into separate
> configuration variables. These are ultimately different concerns, and
> the interaction as layed out in this commit is somewhat non-obvious to
> me.
Let me know if there are things I could clarify more in the above
explanations. I am also open to concrete suggestions.
Thanks.
^ permalink raw reply
* Re: [PATCH 13/16] promisor-remote: introduce promisor.acceptFromServerUrl
From: Christian Couder @ 2026-04-27 12:44 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Taylor Blau, Karthik Nayak, Elijah Newren,
Christian Couder
In-Reply-To: <acUkzY7f5302uWD8@pks.im>
On Thu, Mar 26, 2026 at 1:21 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Mar 23, 2026 at 09:05:16AM +0100, Christian Couder wrote:
> > diff --git a/promisor-remote.c b/promisor-remote.c
> > index c2f0eb7223..4cb18e1a6a 100644
> > --- a/promisor-remote.c
> > +++ b/promisor-remote.c
> [snip]
> > +static struct string_list *accept_from_server_url(struct repository *repo)
> > +{
> > + static struct string_list accept_urls = STRING_LIST_INIT_DUP;
> > + static int initialized;
> > + const struct string_list *config_urls;
> > +
> > + if (initialized)
> > + return &accept_urls;
> > +
> > + initialized = 1;
> > +
> > + 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;
> > + }
> > + }
> > + }
> > +
> > + return &accept_urls;
> > +}
>
> I'm still not much of a fan of us getting more and more function-local
> static variables. It just feels wrong to me, and like we're accruing
> technical debt. I also doubt that the performance overhead of storing
> this on the stack with proper lifecycle management will matter at all
> given that we're in a context where we talk with a remote anyway. The
> handful of allocations really shouldn't matter in that context.
OK, I have removed the static variables and it looks like the following in v2:
+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;
^ permalink raw reply
* Re: [PATCH 15/16] promisor-remote: auto-configure unknown remotes
From: Christian Couder @ 2026-04-27 12:44 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Taylor Blau, Karthik Nayak, Elijah Newren,
Christian Couder
In-Reply-To: <acUk11mt06GJZaur@pks.im>
On Thu, Mar 26, 2026 at 1:21 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Mar 23, 2026 at 09:05:18AM +0100, Christian Couder wrote:
> > Previous commits have introduced the `promisor.acceptFromServerUrl`
> > config variable to whitelist 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.
>
> Would it make sense to extend git-clone(1) to have a command line option
> that basically does this as a one-shot? Something like `git clone
> --accept-promisors=url:https://gitlab.com/*`? I assume that many users
> may not want to keep on updating their configured promisors all the
> time.
I don't understand why you say "many users may not want to keep on
updating their configured promisors all the time". It seems to me that
what I propose requires even less effort from users than what you
suggest.
If users set up something like:
git config set --global promisor.acceptFromServerUrl "https://my-org.com/*"
or:
git config set --global promisor.acceptFromServerUrl
"https://gitlab.com/my-org/*"
they would then automatically accept the promisor remotes with an URL
matching the pattern when they make a partial clone.
So it's a one time setup instead of having to use
`--accept-promisors=url:...` each time they clone.
Also `git -c promisor.acceptFromServerUrl="..." clone` can basically
be used to get the same thing as the `--accept-promisors=url:...` flag
you suggest.
> Furthermore, this here reconfirms my thought on the previous commit that
> it would make sense to detangle accepting promisors, storing them in the
> configuration and updating them automatically. These are all different
> things:
>
> - Accepting promisors is basically an ongoing runtime thing where you
> start to use announced promisors even though they are not configured
> at all.
Why an "ongoing runtime thing"? If users think it's fine to accept
promisors from their own domain, why should they have to confirm that
every time they clone?
> - Storing promisors is typically a one-time thing that you'd want to
> do when creating a new repository.
Except that some fields and maybe sometimes URLs might change on the
server side and it would be nice if this didn't require manual updates
on the client side.
> - Updating promisors automatically is probably something you want to
> do on an ongoing basis when you have stored promisors.
Yeah, so it's similar in many ways to storing promisors.
> We're currently putting all of these use cases into the same bag, but
> they have very different characteristics.
I don't think all use cases are put in the same bag. There are a
number of config options already that allow a lot of customization.
Adding "promisor.acceptFromServerUrl" as a separate option from
"promisor.acceptFromServer" also only increases the possibilities for
users.
> I guess the most common use
> case will eventually be to never auto-accept promisors, store them at
> clone time, and keep them updated whenever they change.
I am not sure at all this is the most common use case.
If you require a `--accept-promisors=url:https://my-org.com/*` each
time, people might just copy-paste it or create an alias for that and
then use it all the time and you won't get much more security than
something like:
git config set --global promisor.acceptFromServerUrl "https://my-org.com/*"
once and then regular `git clone ...`
When users have to often pass parameters manually, the typos and
misconfiguration risks also increase compared to admins setting things
up globally for everyone, or even users doing it once for themselves.
> This cannot be
> expressed with "promisor.acceptFromServerUrl" as far as I understand.
`git -c promisor.acceptFromServerUrl="..." clone` is basically the
same as the option you suggest.
^ permalink raw reply
* Re: [PATCH 09/16] promisor-remote: add 'local_name' to 'struct promisor_info'
From: Christian Couder @ 2026-04-27 12:42 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Taylor Blau, Karthik Nayak, Elijah Newren,
Christian Couder
In-Reply-To: <acUkuD6iuq6nTeHn@pks.im>
On Thu, Mar 26, 2026 at 1:21 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Mar 23, 2026 at 09:05:12AM +0100, Christian Couder wrote:
> > 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
>
> Micronit: s/char* local_name/char *local_name/
Fixed in the v2 I just sent.
> > diff --git a/promisor-remote.c b/promisor-remote.c
> > index bdfc5e7608..da347fa2dc 100644
> > --- a/promisor-remote.c
> > +++ b/promisor-remote.c
> > @@ -434,15 +434,19 @@ static struct string_list *fields_stored(void)
> >
> > /*
> > * Struct for promisor remotes involved in the "promisor-remote"
> > - * protocol capability.
> > + * 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.
> > + * - "name" is the name the server advertised.
> > + * - "local_name" is the name we use locally (may be auto-generated).
> > + *
> > + * 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 *local_name;
> > const char *url;
> > const char *filter;
> > const char *token;
>
> I think it would be easier to follow if the struct-level comment applied
> to the general description of the struct, and individual members would
> then have their own comments describing their intent.
Right, the diff looks like the following in the v2:
@@ -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;
Thanks.
^ permalink raw reply
* Re: [PATCH 03/16] urlmatch: add url_is_valid_pattern() helper
From: Christian Couder @ 2026-04-27 12:42 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Taylor Blau, Karthik Nayak, Elijah Newren,
Christian Couder
In-Reply-To: <acUkpJjHgYs0jqX4@pks.im>
On Thu, Mar 26, 2026 at 1:20 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Mar 23, 2026 at 09:05:06AM +0100, Christian Couder wrote:
> > +bool url_is_valid_pattern(const char *url)
> > +{
> > + char *normalized = url_normalize_1(url, NULL, true);
> > +
> > + if (normalized) {
> > + free(normalized);
> > + return true;
> > + }
> > +
> > + return false;
> > +}
>
> We could simplify this implementation to:
>
> bool url_is_valid_pattern(const char *url)
> {
> char *normalized = url_normalize_1(url, NULL, true);
> free(normalized);
> return !!normalized;
> }
Thanks for the suggestion but this patch has been replaced by a patch
adding url_normalize_pattern() in the v2 I just sent.
^ permalink raw reply
* [PATCH v2 8/8] doc: promisor: improve acceptFromServer entry
From: Christian Couder @ 2026-04-27 12:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260427124108.3524129-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 ae1686a6e0..095c1693ac 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.19.gb68b9497aa
^ permalink raw reply related
* [PATCH v2 7/8] promisor-remote: auto-configure unknown remotes
From: Christian Couder @ 2026-04-27 12:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260427124108.3524129-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".
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/config/promisor.adoc | 26 +++-
Documentation/config/remote.adoc | 9 ++
promisor-remote.c | 202 +++++++++++++++++++++++++-
t/t5710-promisor-remote-capability.sh | 104 +++++++++++++
4 files changed, 332 insertions(+), 9 deletions(-)
diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
index efc066c3f2..ae1686a6e0 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).
@@ -66,9 +67,10 @@ this option in _ANY_ config file read by Git.
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://`.
+
@@ -99,6 +101,22 @@ are resolved. The port must also match exactly (e.g.,
`https://example.com:8080/*` will not match a URL advertised on
port 9999).
+
+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 72d5b94bf7..8c8a798fdb 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -816,10 +816,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;
@@ -837,9 +1024,13 @@ 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) {
/* We don't know about that remote */
- return 0;
+ int res = should_accept_new_remote_url(repo, accept_urls, advertised);
+ if (res)
+ *reload_config = true;
+ return res;
+ }
p = item->util;
@@ -1097,7 +1288,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.19.gb68b9497aa
^ permalink raw reply related
* [PATCH v2 6/8] promisor-remote: trust known remotes matching acceptFromServerUrl
From: Christian Couder @ 2026-04-27 12:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260427124108.3524129-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 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.
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.
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, so url_matches_accept_list()
is only called 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".
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/config/promisor.adoc | 52 ++++++++++++++
Documentation/gitprotocol-v2.adoc | 9 +--
promisor-remote.c | 98 +++++++++++++++++++++++++--
t/t5710-promisor-remote-capability.sh | 71 +++++++++++++++++++
4 files changed, 220 insertions(+), 10 deletions(-)
diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
index b0fa43b839..efc066c3f2 100644
--- a/Documentation/config/promisor.adoc
+++ b/Documentation/config/promisor.adoc
@@ -51,6 +51,58 @@ 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.
++
+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).
++
+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..72d5b94bf7 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,82 @@ static void load_accept_from_server_url(struct repository *repo,
}
}
+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)
+ 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;
+}
+
+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;
@@ -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);
+
+ return 0;
}
static int skip_field_name_prefix(const char *elem, const char *field_name, const char **value)
@@ -991,7 +1077,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 +1097,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.19.gb68b9497aa
^ permalink raw reply related
* [PATCH v2 5/8] promisor-remote: introduce promisor.acceptFromServerUrl
From: Christian Couder @ 2026-04-27 12:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260427124108.3524129-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.19.gb68b9497aa
^ permalink raw reply related
* [PATCH v2 4/8] promisor-remote: add 'local_name' to 'struct promisor_info'
From: Christian Couder @ 2026-04-27 12:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260427124108.3524129-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.19.gb68b9497aa
^ permalink raw reply related
* [PATCH v2 3/8] urlmatch: add url_normalize_pattern() helper
From: Christian Couder @ 2026-04-27 12:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260427124108.3524129-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.19.gb68b9497aa
^ permalink raw reply related
* [PATCH v2 2/8] urlmatch: change 'allow_globs' arg to bool
From: Christian Couder @ 2026-04-27 12:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260427124108.3524129-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.19.gb68b9497aa
^ permalink raw reply related
* [PATCH v2 1/8] t5710: simplify 'mkdir X' followed by 'git -C X init'
From: Christian Couder @ 2026-04-27 12:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260427124108.3524129-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.19.gb68b9497aa
^ permalink raw reply related
* [PATCH v2 0/8] Auto-configure advertised remotes via URL allowlist
From: Christian Couder @ 2026-04-27 12:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Taylor Blau, Karthik Nayak,
Elijah Newren, Christian Couder
In-Reply-To: <20251223111113.47473-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 new. It 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 v1
======================
Thanks to Patrick and Junio for reviewing the previous versions of
this series and of the preparatory series.
- A lot of preparatory patches have been moved to a preparatory series
that has already been merged. See:
https://lore.kernel.org/git/20260407115243.358642-1-christian.couder@gmail.com/
This is why this v2 contains only 8 patches compared to 16 patches
in v1.
- Everywhere in this series "whitelist" as been replaced with
"allowlist".
- In the tests added in this series, the new $TRASH_DIRECTORY_URL and
$ENCODED_TRASH_DIRECTORY_URL introduced by the preparatory series
are used instead of the previous $PWD_URL and $ENCODED_PWD_URL.
- Patch 1/8 ("t5710: simplify 'mkdir X' followed by 'git -C X init'")
is new.
- Patch 3/8 ("urlmatch: add url_normalize_pattern() helper") replaces
patch 3/16 ("urlmatch: add url_is_valid_pattern() helper") because
in subsequent patches we now normalize patterns to validate them
and match them component by component against URLs.
- In patch 5/8, previously 13/16, ("promisor-remote: introduce
promisor.acceptFromServerUrl"):
- We add a `struct url_info pattern_info;` to `struct allowed_url`,
so we can validate patterns using url_normalize_pattern() and, in
a subsequent patch, match URLs component by component. This
requires a new allowed_url_free() function that is passed to
string_list_clear_func() to clear the `struct allowed_url`
instances.
- We don't use a `static struct string_list` to store the URL
patterns we accept. Instead we load them from the config into a
`struct string_list` passed as argument. The function doing this
is renamed accordingly from accept_from_server_url() to
load_accept_from_server_url().
- A "clone with invalid promisor.acceptFromServerUrl" test is moved
from patch 15/16 to this patch as it's more relevant in this
patch (where we validate the content of the
`promisor.acceptFromServerUrl` environment variable).
- In patch 6/8, previously 14/16, ("promisor-remote: trust known
remotes matching acceptFromServerUrl"):
- In the commit message, an example, which shows how the new
"acceptFromServerUrl" config option can be useful, is added.
- The matching of URLs advertised by the server to URLs patterns
from the config, is now performed component by component. This is
reflected in the commit message, the documentation and the
code. This ensures a `*` in the host pattern cannot cross into
the path.
- In the code, we add a new match_one_url() function to perform the
matching.
- In patch 7/8, previously 15/16 ("promisor-remote: auto-configure
unknown remotes"):
- In the doc, the unclear "considered trusted by the client" is
clarified using "a client is allowed to act on" and subsequent
explanations. In general the doc is also improved a bit.
- In the tests, parsing the "remote.<name>.advertisedAs" config
option is now more careful about the possibility that more than
one such options exist.
- The test that was moved to patch 5/8 is still enhanced a bit in
this commit by checking that no "remote.<name>.advertisedAs"
config option has been added.
CI tests
========
They all pass, see:
https://github.com/chriscool/git/actions/runs/24992478331
Range diff since v1
===================
1: b2894eb33a < -: ---------- promisor-remote: try accepted remotes before others in get_direct()
-: ---------- > 1: 44e9a16455 t5710: simplify 'mkdir X' followed by 'git -C X init'
2: a3206a6ae9 = 2: 42f174910c urlmatch: change 'allow_globs' arg to bool
3: 51bbf65c52 < -: ---------- urlmatch: add url_is_valid_pattern() helper
4: f367beef72 < -: ---------- promisor-remote: clarify that a remote is ignored
5: 1faf74cb3f < -: ---------- promisor-remote: refactor has_control_char()
6: 40cf0af639 < -: ---------- promisor-remote: refactor accept_from_server()
7: b75dca8037 < -: ---------- promisor-remote: keep accepted promisor_info structs alive
8: f5e55dc407 < -: ---------- promisor-remote: remove the 'accepted' strvec
-: ---------- > 3: 8088374458 urlmatch: add url_normalize_pattern() helper
9: 63c1db30de ! 4: 6bfda89a79 promisor-remote: add 'local_name' to 'struct promisor_info'
@@ Commit message
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 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()
@@ Commit message
## promisor-remote.c ##
@@ promisor-remote.c: static struct string_list *fields_stored(void)
-
- /*
* Struct for promisor remotes involved in the "promisor-remote"
-- * protocol capability.
-+ * protocol capability:
+ * 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.
-+ * - "name" is the name the server advertised.
-+ * - "local_name" is the name we use locally (may be auto-generated).
-+ *
+ * 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 *local_name;
+- 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;
10: e9b8a64ab8 < -: ---------- promisor-remote: pass config entry to all_fields_match() directly
11: 2e1260190a < -: ---------- promisor-remote: refactor should_accept_remote() control flow
12: b33f06173a < -: ---------- t5710: use proper file:// URIs for absolute paths
13: 681b03e248 ! 5: fefa17e6dd promisor-remote: introduce promisor.acceptFromServerUrl
@@ promisor-remote.c: static bool has_control_char(const char *s)
+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;
@@ promisor-remote.c: static bool has_control_char(const char *s)
+ p = dup;
+ }
+
-+ if (has_control_char(p) || !url_is_valid_pattern(p)) {
++ if (has_control_char(p)) {
+ warning(_("invalid url pattern '%s' "
+ "in '%s' from promisor.acceptFromServerUrl config"), p, url);
+ free(dup);
@@ promisor-remote.c: static bool has_control_char(const char *s)
+ 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 struct string_list *accept_from_server_url(struct repository *repo)
++static void load_accept_from_server_url(struct repository *repo,
++ struct string_list *accept_urls)
+{
-+ static struct string_list accept_urls = STRING_LIST_INIT_DUP;
-+ static int initialized;
+ const struct string_list *config_urls;
+
-+ if (initialized)
-+ return &accept_urls;
-+
-+ initialized = 1;
-+
+ if (!repo_config_get_string_multi(repo, "promisor.acceptfromserverurl", &config_urls)) {
+ struct string_list_item *item;
+
@@ promisor-remote.c: static bool has_control_char(const char *s)
+ 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 = string_list_append(accept_urls, item->string);
+ new->util = allowed;
+ }
+ }
+ }
-+
-+ return &accept_urls;
+}
+
static int should_accept_remote(enum accept_promisor accept,
@@ promisor-remote.c: 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);
-+ /* Pre-load and validate the acceptFromServerUrl config */
-+ (void)accept_from_server_url(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;
+@@ promisor-remote.c: 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);
+
+ ## t/t5710-promisor-remote-capability.sh ##
+@@ t/t5710-promisor-remote-capability.sh: 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" &&
14: 8c04e48d66 ! 6: 2f238d0a7a promisor-remote: trust known remotes matching acceptFromServerUrl
@@ Commit message
To enable such targeted updates for trusted URLs, let's use the URL
patterns from `promisor.acceptFromServerUrl` as an additional URL
- based whitelist.
+ 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).
- (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 whitelist.)
+ 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.
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 whitelist is still accepted.
+ the allowlist is still accepted.
To prepare for this new logic, let's also:
@@ Commit message
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 whitelist for known remotes,
- including the URL normalization behavior, and let's mention it in
- "gitprotocol-v2.adoc".
+ "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".
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
comparisons are case sensitive. See linkgit:gitprotocol-v2[5].
+promisor.acceptFromServerUrl::
-+ A glob pattern to specify which URLs advertised by a server
-+ are considered trusted by the client. This option acts as an
-+ additive security whitelist that works in conjunction with
-+ `promisor.acceptFromServer`.
++ 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.
++
-+Be _VERY_ careful with these glob patterns, as it can be a big
-+security hole to allow any advertised remote to be auto-configured!
++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://`.
@@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
+ your specific organization or namespace (e.g.,
+ `https://gitlab.com/your-org/*`).
++
-+3. Don't use globs (`*`) in the domain name. For example
-+ `https://cdn.example.com/*` is much safer than
-+ `https://*.example.com/*`, because the latter matches
-+ `https://evil-hacker.net/fake.example.com/repo`.
++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. Make sure to have a `/` at the end of the domain name (or the end
-+ of specific directories). For example `https://cdn.example.com/*`
-+ is much safer than `https://cdn.example.com*`, because the latter
-+ matches `https://cdn.example.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, the advertised URL is normalized: the scheme and
-+host are lowercased, percent-encoded characters are decoded where
-+possible, and path segments like `..` are resolved. Glob patterns
-+are matched against this normalized URL as-is, so patterns should
-+be written in normalized form (e.g., lowercase scheme and host).
++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).
++
-+Even if `promisor.acceptFromServer` is set to `None` (the default),
-+Git will still accept field updates (like tokens) for known remotes,
-+provided their URLs match a pattern in
-+`promisor.acceptFromServerUrl`. See linkgit:gitprotocol-v2[5] for
-+details on the protocol.
++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
@@ promisor-remote.c
struct promisor_remote_config {
struct promisor_remote *promisors;
-@@ promisor-remote.c: static struct string_list *accept_from_server_url(struct repository *repo)
- return &accept_urls;
+@@ promisor-remote.c: static void load_accept_from_server_url(struct repository *repo,
+ }
}
++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)
++ 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;
++}
++
+static struct allowed_url *url_matches_accept_list(
+ struct string_list *accept_urls, const char *url)
+{
+ struct string_list_item *item;
-+ char *normalized = url_normalize(url, NULL);
++ struct url_info url_info;
++
++ url_info.url = url_normalize(url, &url_info);
+
-+ if (!normalized)
++ if (!url_info.url)
+ return NULL;
+
+ for_each_string_list_item(item, accept_urls) {
+ struct allowed_url *allowed = item->util;
+
-+ if (!wildmatch(allowed->url_pattern, normalized, 0)) {
-+ free(normalized);
++ if (match_one_url(&allowed->pattern_info, &url_info)) {
++ free(url_info.url);
+ return allowed;
+ }
+ }
+
-+ free(normalized);
++ free(url_info.url);
+ return NULL;
+}
+
@@ promisor-remote.c: static int should_accept_remote(enum accept_promisor 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 whitelist!
++ * matches the acceptFromServerUrl allowlist!
+ */
+ if (url_matches_accept_list(accept_urls, remote_url))
+ return all_fields_match(advertised, config_info, p);
@@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
static int skip_field_name_prefix(const char *elem, const char *field_name, const char **value)
@@ promisor-remote.c: 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);
-- /* Pre-load and validate the acceptFromServerUrl config */
-- (void)accept_from_server_url(repo);
-+ struct string_list *accept_urls = accept_from_server_url(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)
++ if (accept == ACCEPT_NONE && !accept_urls.nr)
return;
/* Parse remote info received */
@@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
}
- if (should_accept_remote(accept, advertised, &config_info)) {
-+ if (should_accept_remote(accept, advertised, accept_urls, &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))
@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
check_missing_objects server 1 "$oid"
'
-+test_expect_success "clone with 'None' but URL whitelisted" '
++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="$PWD_URL/lop" \
++ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=None \
-+ -c promisor.acceptFromServerUrl="$ENCODED_PWD_URL/*" \
++ -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 whitelist" '
++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="$PWD_URL/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 &&
@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
+ initialize_server 1 "$oid"
+'
+
-+test_expect_success "clone with 'None' but URL whitelisted in one pattern out of two" '
++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="$PWD_URL/lop" \
++ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=None \
+ -c promisor.acceptFromServerUrl="https://example.com/*" \
-+ -c promisor.acceptFromServerUrl="$ENCODED_PWD_URL/*" \
++ -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 whitelisted, but client has different URL" '
++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 whitelist, the remote is rejected because the
++ # 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="$PWD_URL/serverTwo" \
++ -c remote.lop.url="$TRASH_DIRECTORY_URL/serverTwo" \
+ -c promisor.acceptfromserver=None \
-+ -c promisor.acceptFromServerUrl="$ENCODED_PWD_URL/*" \
++ -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
@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
+ initialize_server 1 "$oid"
+'
+
- test_expect_success "clone with promisor.sendFields" '
+ test_expect_success "clone with invalid promisor.acceptFromServerUrl" '
git -C server config promisor.advertise true &&
test_when_finished "rm -rf client" &&
-@@ t/t5710-promisor-remote-capability.sh: test_expect_success "subsequent fetch from a client when promisor.advertise is f
- check_missing_objects server 1 "$oid"
- '
-
-+
-+
- test_done
15: 314150a860 ! 7: a077f33df4 promisor-remote: auto-configure unknown remotes
@@ Commit message
promisor-remote: auto-configure unknown remotes
Previous commits have introduced the `promisor.acceptFromServerUrl`
- config variable to whitelist some URLs advertised by a server through
+ 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
@@ Commit message
## Documentation/config/promisor.adoc ##
@@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
-
promisor.acceptFromServerUrl::
- A glob pattern to specify which URLs advertised by a server
-- are considered trusted by the client. This option acts as an
-- additive security whitelist that works in conjunction with
-- `promisor.acceptFromServer`.
-+ are allowed to be auto-configured (created and persisted) on
-+ the client side. Unlike `promisor.acceptFromServer`, which
-+ only accepts already configured remotes, a match against this
-+ option instructs Git to write a new `[remote "<name>"]`
-+ section to the client's configuration.
+ 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).
+@@ Documentation/config/promisor.adoc: this option in _ANY_ config file read by Git.
+ 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://`.
+
- This option can appear multiple times in config files. An advertised
- URL will be accepted if it matches _ANY_ glob pattern specified by
-@@ Documentation/config/promisor.adoc: possible, and path segments like `..` are resolved. Glob patterns
- are matched against this normalized URL as-is, so patterns should
- be written in normalized form (e.g., lowercase scheme and host).
+@@ 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).
+
--Even if `promisor.acceptFromServer` is set to `None` (the default),
--Git will still accept field updates (like tokens) for known remotes,
--provided their URLs match a pattern in
--`promisor.acceptFromServerUrl`. See linkgit:gitprotocol-v2[5] for
--details on the protocol.
+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.
++`promisor-auto-`.
++
-+Note that this option acts as an additive security whitelist. It works
-+in conjunction with `promisor.acceptFromServer` (see the documentation
-+of that option for the implications of accepting a promisor
-+remote). Even if `promisor.acceptFromServer` is set to `None` (the
-+default), Git will still automatically configure new remotes, and
-+accept field updates (like tokens) for known remotes, provided their
-+URLs match a pattern in `promisor.acceptFromServerUrl`. See
-+linkgit:gitprotocol-v2[5] for details on the protocol.
-
- promisor.checkFields::
- A comma or space separated list of additional remote related
++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].
## Documentation/config/remote.adoc ##
@@ Documentation/config/remote.adoc: remote.<name>.promisor::
@@ promisor-remote.c: 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,
+- 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))
## t/t5710-promisor-remote-capability.sh ##
-@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', URL whitelisted, but client has differen
+@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', URL allowlisted, but client has differen
initialize_server 1 "$oid"
'
-+test_expect_success "clone with URL whitelisted and no remote already configured" '
++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_PWD_URL/*" \
++ -c promisor.acceptFromServerUrl="$ENCODED_TRASH_DIRECTORY_URL/*" \
+ --no-local --filter="blob:limit=5k" server client &&
+
-+ # Check that a remote has been auto-created with the right fields.
-+ # The remote is identified by "remote.<name>.advertisedAs" == "lop".
-+ FULL_NAME=$(git -C client config --name-only --get-regexp "remote\..*\.advertisedas" "^lop$") &&
-+ REMOTE_NAME=$(echo "$FULL_NAME" | sed "s/remote\.\(.*\)\.advertisedas/\1/") &&
++ # 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" "$PWD_URL/lop" "true" >expect &&
++ 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 &&
@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', U
+ check_missing_objects server 1 "$oid"
+'
+
-+test_expect_success "clone with named URL whitelisted and no pre-configured remote" '
++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_PWD_URL/*" \
++ -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" "$PWD_URL/lop" "true" "lop" >expect &&
++ 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 &&
@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', U
+ check_missing_objects server 1 "$oid"
+'
+
-+test_expect_success "clone with URL whitelisted but colliding name" '
++test_expect_success "clone with URL allowlisted but colliding name" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', U
+ -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_PWD_URL/*" \
++ -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" "$PWD_URL/lop" "true" "lop" >expect &&
++ 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 &&
@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', U
+ check_missing_objects server 1 "$oid"
+'
+
-+test_expect_success "clone with URL whitelisted and reusable remote" '
++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="$PWD_URL/lop" \
++ -c remote.cdn.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=None \
-+ -c promisor.acceptFromServerUrl="cdn=$ENCODED_PWD_URL/*" \
++ -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" "$PWD_URL/lop" "true" "lop" "+refs/heads/*:refs/remotes/lop/*" >expect &&
++ 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 &&
@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'None', U
+ 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 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 "" &&
-+
-+ # Reinitialize server so that the largest object is missing again
-+ initialize_server 1 "$oid"
-+'
-+
- test_expect_success "clone with promisor.sendFields" '
+ test_expect_success "clone with invalid promisor.acceptFromServerUrl" '
git -C server config promisor.advertise true &&
test_when_finished "rm -rf client" &&
+@@ t/t5710-promisor-remote-capability.sh: 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 "" &&
+
16: 20f70b52bb ! 8: b68b9497aa doc: promisor: improve acceptFromServer entry
@@ Documentation/config/promisor.adoc: variable is set to "true", and the "name" an
+for protocol details.
promisor.acceptFromServerUrl::
- A glob pattern to specify which URLs advertised by a server
+ A glob pattern to specify which server-advertised URLs a
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 | 123 ++++++--
Documentation/config/remote.adoc | 9 +
Documentation/gitprotocol-v2.adoc | 9 +-
promisor-remote.c | 410 ++++++++++++++++++++++++--
t/t5710-promisor-remote-capability.sh | 202 ++++++++++++-
urlmatch.c | 11 +-
urlmatch.h | 12 +
7 files changed, 730 insertions(+), 46 deletions(-)
--
2.54.0.19.gb68b9497aa
^ permalink raw reply
* Re: [PATCH] index-pack, unpack-objects: increase input buffer from 4 KiB to 128 KiB
From: Derrick Stolee @ 2026-04-27 12:36 UTC (permalink / raw)
To: Junio C Hamano, Scott Bauersfeld via GitGitGadget; +Cc: git, Scott Bauersfeld
In-Reply-To: <xmqqldeb9w8e.fsf@gitster.g>
On 4/25/2026 6:21 AM, Junio C Hamano wrote:
> "Scott Bauersfeld via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Scott Bauersfeld <sbauersfeld@g.ucla.edu>
>>
>> On FUSE-backed filesystems every write(2) is a synchronous round
>> trip through the FUSE protocol (userspace -> kernel -> userspace ->
>> back), so the 4 KiB buffer turns a clone into many unnecessary tiny
>> writes with noticeable latency overhead.
>>
>> Increase the buffer from 4 KiB to 128 KiB, matching the default
>> already used by the hashfile layer in csum-file.c.
>
> Quite sensible reasoning presented very nicely.
>
> It may probably be a #leftoverbit but these three instances of (128
> * 1024) may want to have a common symbolic constant, like
>
> #define DEFAULT_IOBUFFER_SIZE_IN_BYTES (128 * 1024)
>
> in a bit more central header file. Especially for the one in
> csum-file.c where there is no symbolic constant used for that
> purpose.
I also had this thought. Would environment.h be the best place?
>> Testing with strace on HTTPS clones of git/git (~296 MB pack, 5 runs
>> per variant, isolated builds from the same v2.54.0 source) shows:
>>
>> index-pack pack file writes: 72,465 -> 24,943 avg (66% reduction)
>> total write() syscalls: 310,192 -> 259,530 avg (17% reduction)
>> writes of exactly 4096 bytes: ~40,077 -> 0 (eliminated)
>
> Hmph, I would have expected more like (1 - 4/128) ~ 97% reduction.
> The difference between that and 66% is coming from where? There are
> inherently short writes that do not utilize the new larger buffer
> beyond 4kB? If so, another number of interest might be the number
> of writes smaller than 4096 bytes, perhaps?
One way to reword what you're asking is to measure "number of writes
not using the whole buffer" which is basically going to be "the
number of flush events from the application layer". Every time the
application intends to flush, the current buffer is likely to not
be exactly full. I would expect this number to not change between
implementations in real experiments.
The improvement here comes from the reduced number of flushes due
to buffer limits. I see that this can be measured in the number of
system-level events, but what impact does this have on the end-to-
end time of 'git index-pack' or 'git unpack-objects'? Is there a
t/perf/ test that can demonstrate this improvement for a variety
of real repos using GIT_PERF_REPO?
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v4 1/2] revision.c: implement --reverse=before for walks
From: Junio C Hamano @ 2026-04-27 12:30 UTC (permalink / raw)
To: Johannes Sixt
Cc: git, Jeff King, Jean-Noël Avila, Patrick Steinhardt,
Tian Yuchen, Ben Knoble, Mirko Faina
In-Reply-To: <971f19db-eb10-4c88-8d5d-3f4f7f92db73@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>> I wonder --reverse=oldest and --reverse=newest is easier to teach
>> and explain? I dunno.
> What does it mean to "revert the oldest"? Or "the newest"?
I do not quite understand where the "revert" comes from, though.
> If at all,
> then this "newest" and "oldest" must be a restriction that applies to
> --max-count in some way. Perhaps we need a --max-count-oldest option,
> then --reverse does not have to be touched at all, because it is still
> applied only after the set of commits to show has been determined.
That makes two of us to suspect that this is more about --max-count
than --reverse.
cf. https://lore.kernel.org/git/xmqqv7dlr4yz.fsf@gitster.g/
"git log --max-count-oldest=3" will give us three oldest commit in
reverse chronological order, the set of commits shown are the same
with or without "--reverse", which makes tons of sense.
^ permalink raw reply
* [PATCH] Reintegrate: send "Huh?" warnings to stderr, not stdout
From: Erik Cervin-Edin @ 2026-04-27 10:47 UTC (permalink / raw)
To: git; +Cc: gitster
The "Huh?: $msg" warning in show_merge(), emitted when a first-parent
merge subject does not match either "Merge branch '...'" or "Merge
remote branch '...'", uses
echo 2>&1 "Huh?: $msg"
The "2>&1" redirect dupes stderr onto stdout's destination; it does
not change where stdout itself points. Since echo writes to stdout,
the "Huh?:" message lands on stdout regardless -- as would any
command's normal output. The intent appears to have been ">&2",
which dupes stdout onto stderr.
In the common Reintegrate invocation that captures stdout, e.g.
Meta/Reintegrate next..seen >Meta/redo-seen.sh
this means the warning is silently embedded in the generated heredoc
body instead of being printed to the maintainer's terminal. The
resulting redo-* script is corrupted with a "Huh?:..." line and the
maintainer has no diagnostic that something went wrong.
Every other diagnostic in this script already uses ">&2"; this line
is the lone outlier.
Use ">&2" so the warning reaches stderr as intended.
Signed-off-by: Erik Cervin-Edin <erik@cervined.in>
---
Reintegrate | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Reintegrate b/Reintegrate
index a1e67a0330..6fdc7c5f41 100755
--- a/Reintegrate
+++ b/Reintegrate
@@ -327,7 +327,7 @@ show_merge () {
merge_hier=
;;
*)
- echo 2>&1 "Huh?: $msg"
+ echo >&2 "Huh?: $msg"
return
;;
esac &&
--
2.53.0
^ permalink raw reply related
* [PATCH v3 9/9] refs: use peeled tag values in reference backends
From: Karthik Nayak @ 2026-04-27 10:42 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, toon
In-Reply-To: <20260427-refs-move-to-generic-layer-v3-0-e4638dfb7897@gmail.com>
The reference backends peel tag objects when storing references to them.
This is to provide optimized reads which avoids hitting the odb. The
previous commits ensures that the peeled value is now propagated via the
generic layer. So modify the packed and reftable backend to directly use
this value instead of calling `peel_object()` independently.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/packed-backend.c | 6 ++----
refs/reftable-backend.c | 9 ++-------
2 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 35a0f32e1c..0acde48c45 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1531,13 +1531,11 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
*/
i++;
} else {
- struct object_id peeled;
- int peel_error = peel_object(refs->base.repo, &update->new_oid,
- &peeled, PEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE);
+ bool peeled = update->flags & REF_HAVE_PEELED;
if (write_packed_entry(out, update->refname,
&update->new_oid,
- peel_error ? NULL : &peeled))
+ peeled ? &update->peeled : NULL))
goto write_error;
i++;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index b0c010387d..8b4ac2e618 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -12,7 +12,6 @@
#include "../hex.h"
#include "../ident.h"
#include "../iterator.h"
-#include "../object.h"
#include "../parse.h"
#include "../path.h"
#include "../refs.h"
@@ -1584,17 +1583,13 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
goto done;
} else if (u->flags & REF_HAVE_NEW) {
struct reftable_ref_record ref = {0};
- struct object_id peeled;
- int peel_error;
ref.refname = (char *)u->refname;
ref.update_index = ts;
- peel_error = peel_object(arg->refs->base.repo, &u->new_oid, &peeled,
- PEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE);
- if (!peel_error) {
+ if (u->flags & REF_HAVE_PEELED) {
ref.value_type = REFTABLE_REF_VAL2;
- memcpy(ref.value.val2.target_value, peeled.hash, GIT_MAX_RAWSZ);
+ memcpy(ref.value.val2.target_value, u->peeled.hash, GIT_MAX_RAWSZ);
memcpy(ref.value.val2.value, u->new_oid.hash, GIT_MAX_RAWSZ);
} else if (!is_null_oid(&u->new_oid)) {
ref.value_type = REFTABLE_REF_VAL1;
--
2.53.GIT
^ permalink raw reply related
* [PATCH v3 8/9] refs: add peeled object ID to the `ref_update` struct
From: Karthik Nayak @ 2026-04-27 10:42 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, toon
In-Reply-To: <20260427-refs-move-to-generic-layer-v3-0-e4638dfb7897@gmail.com>
Certain reference backends {packed, reftable}, have the ability to also
store the peeled object ID for a reference pointing to a tag object.
This has the added benefit that during retrieval of such references, we
also obtain the peeled object ID without having to use the ODB.
To provide this functionality, each backend independently calls the ODB
to obtain the peeled OID. To move this functionality to the generic
layer, there must be support infrastructure to pass in a peeled OID for
reference updates.
Add a `peeled` field to the `ref_update` structure and modify
`ref_transaction_add_update()` to receive and copy this object ID to the
`ref_update` structure. Finally, modify `ref_transaction_update()` to
peel tag objects and pass the peeled OID to
`ref_transaction_add_update()`.
Update all callers of these functions with the new function parameters.
Callers which only add reflog updates, need to only pass in NULL, since
for reflogs, we don't store peeled OIDs. Reference deletions also only
need to pass in NULL. For others, pass along the peeled OID if
available.
In a following commit, we'll modify the backends to use this peeled OID
instead of parsing it themselves.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 15 +++++++++++++--
refs/files-backend.c | 20 ++++++++++++--------
refs/refs-internal.h | 14 ++++++++++++++
refs/reftable-backend.c | 6 +++---
4 files changed, 42 insertions(+), 13 deletions(-)
diff --git a/refs.c b/refs.c
index 662a9e6f9e..0648df2b6c 100644
--- a/refs.c
+++ b/refs.c
@@ -1307,6 +1307,7 @@ struct ref_update *ref_transaction_add_update(
const char *refname, unsigned int flags,
const struct object_id *new_oid,
const struct object_id *old_oid,
+ const struct object_id *peeled,
const char *new_target, const char *old_target,
const char *committer_info,
const char *msg)
@@ -1339,6 +1340,8 @@ struct ref_update *ref_transaction_add_update(
update->committer_info = xstrdup_or_null(committer_info);
update->msg = normalize_reflog_message(msg);
}
+ if (flags & REF_HAVE_PEELED)
+ oidcpy(&update->peeled, peeled);
/*
* This list is generally used by the backends to avoid duplicates.
@@ -1392,6 +1395,8 @@ enum ref_transaction_error ref_transaction_update(struct ref_transaction *transa
unsigned int flags, const char *msg,
struct strbuf *err)
{
+ struct object_id peeled;
+
assert(err);
if ((flags & REF_FORCE_CREATE_REFLOG) &&
@@ -1432,10 +1437,16 @@ enum ref_transaction_error ref_transaction_update(struct ref_transaction *transa
oid_to_hex(new_oid), refname);
return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
}
+
+ if (o->type == OBJ_TAG) {
+ if (!peel_object(transaction->ref_store->repo, new_oid, &peeled,
+ PEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE))
+ flags |= REF_HAVE_PEELED;
+ }
}
ref_transaction_add_update(transaction, refname, flags,
- new_oid, old_oid, new_target,
+ new_oid, old_oid, &peeled, new_target,
old_target, NULL, msg);
return 0;
@@ -1462,7 +1473,7 @@ int ref_transaction_update_reflog(struct ref_transaction *transaction,
return -1;
update = ref_transaction_add_update(transaction, refname, flags,
- new_oid, old_oid, NULL, NULL,
+ new_oid, old_oid, NULL, NULL, NULL,
committer_info, msg);
update->index = index;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f20f580fbc..d0896d0e37 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1325,7 +1325,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
ref_transaction_add_update(
transaction, r->name,
REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_IS_PRUNING,
- null_oid(the_hash_algo), &r->oid, NULL, NULL, NULL, NULL);
+ null_oid(the_hash_algo), &r->oid, NULL, NULL, NULL,
+ NULL, NULL);
if (ref_transaction_commit(transaction, &err))
goto cleanup;
@@ -2468,7 +2469,7 @@ static enum ref_transaction_error split_head_update(struct ref_update *update,
new_update = ref_transaction_add_update(
transaction, "HEAD",
update->flags | REF_LOG_ONLY | REF_NO_DEREF | REF_LOG_VIA_SPLIT,
- &update->new_oid, &update->old_oid,
+ &update->new_oid, &update->old_oid, &update->peeled,
NULL, NULL, update->committer_info, update->msg);
new_update->parent_update = update;
@@ -2530,8 +2531,8 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,
transaction, referent, new_flags,
update->new_target ? NULL : &update->new_oid,
update->old_target ? NULL : &update->old_oid,
- update->new_target, update->old_target, NULL,
- update->msg);
+ &update->peeled, update->new_target, update->old_target,
+ NULL, update->msg);
new_update->parent_update = update;
@@ -2994,7 +2995,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
ref_transaction_add_update(
packed_transaction, update->refname,
REF_HAVE_NEW | REF_NO_DEREF,
- &update->new_oid, NULL,
+ &update->new_oid, NULL, NULL,
NULL, NULL, NULL, NULL);
}
}
@@ -3200,19 +3201,22 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
if (update->flags & REF_LOG_ONLY)
ref_transaction_add_update(loose_transaction, update->refname,
update->flags, &update->new_oid,
- &update->old_oid, NULL, NULL,
+ &update->old_oid, &update->peeled,
+ NULL, NULL,
update->committer_info, update->msg);
else
ref_transaction_add_update(loose_transaction, update->refname,
update->flags & ~REF_HAVE_OLD,
update->new_target ? NULL : &update->new_oid, NULL,
- update->new_target, NULL, update->committer_info,
+ &update->peeled, update->new_target,
+ NULL, update->committer_info,
NULL);
} else {
ref_transaction_add_update(packed_transaction, update->refname,
update->flags & ~REF_HAVE_OLD,
&update->new_oid, &update->old_oid,
- NULL, NULL, update->committer_info, NULL);
+ &update->peeled, NULL, NULL,
+ update->committer_info, NULL);
}
}
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index d103387ebf..307dcb277b 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -39,6 +39,13 @@ struct ref_transaction;
*/
#define REF_LOG_ONLY (1 << 7)
+/*
+ * The reference contains a peeled object ID. This is used when the
+ * new_oid is pointing to a tag object and the reference backend
+ * wants to also store the peeled value for optimized retrieval.
+ */
+#define REF_HAVE_PEELED (1 << 15)
+
/*
* Return the length of time to retry acquiring a loose reference lock
* before giving up, in milliseconds:
@@ -92,6 +99,12 @@ struct ref_update {
*/
struct object_id old_oid;
+ /*
+ * If the new_oid points to a tag object, set this to the peeled
+ * object ID for optimized retrieval without needed to hit the odb.
+ */
+ struct object_id peeled;
+
/*
* If set, point the reference to this value. This can also be
* used to convert regular references to become symbolic refs.
@@ -169,6 +182,7 @@ struct ref_update *ref_transaction_add_update(
const char *refname, unsigned int flags,
const struct object_id *new_oid,
const struct object_id *old_oid,
+ const struct object_id *peeled,
const char *new_target, const char *old_target,
const char *committer_info,
const char *msg);
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 444b0c24e5..b0c010387d 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1107,8 +1107,8 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
ref_transaction_add_update(
transaction, "HEAD",
u->flags | REF_LOG_ONLY | REF_NO_DEREF,
- &u->new_oid, &u->old_oid, NULL, NULL, NULL,
- u->msg);
+ &u->new_oid, &u->old_oid, &u->peeled, NULL, NULL,
+ NULL, u->msg);
}
ret = reftable_backend_read_ref(be, rewritten_ref,
@@ -1194,7 +1194,7 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
transaction, referent->buf, new_flags,
u->new_target ? NULL : &u->new_oid,
u->old_target ? NULL : &u->old_oid,
- u->new_target, u->old_target,
+ &u->peeled, u->new_target, u->old_target,
u->committer_info, u->msg);
new_update->parent_update = u;
--
2.53.GIT
^ permalink raw reply related
* [PATCH v3 7/9] refs: move object parsing to the generic layer
From: Karthik Nayak @ 2026-04-27 10:42 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, toon
In-Reply-To: <20260427-refs-move-to-generic-layer-v3-0-e4638dfb7897@gmail.com>
Regular reference updates made via reference transactions validate that
the provided object ID exists in the object database, which is done by
calling 'parse_object()'. This check is done independently by the
backends which leads to duplicated logic.
Let's move this to the generic layer, ensuring the backends only have to
care about reference storage and not about validation of the object IDs.
With this also remove the 'REF_TRANSACTION_ERROR_INVALID_NEW_VALUE'
error type as its no longer used.
Since we don't iterate over individual references in
`ref_transaction_prepare()`, we add this check to
`ref_transaction_update()`. This means that the validation is done as
soon as an update is queued, without needing to prepare the
transaction. It can be argued that this is more ideal, since this
validation has no dependency on the reference transaction being
prepared.
It must be noted that the change in behavior means that this error
cannot be ignored even with usage of batched updates, since this happens
when the update is being added to the transaction. But since the caller
gets specific error codes, they can either abort the transaction or
continue adding other updates to the transaction.
Modify 'builtin/receive-pack.c' to now capture the error type so that
the error propagated to the client stays the same. Also remove two of
the tests which validates batch-updates with invalid new_oid.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/receive-pack.c | 22 +++++++++++++---------
refs.c | 18 ++++++++++++++++++
refs/files-backend.c | 28 ++--------------------------
refs/reftable-backend.c | 19 -------------------
4 files changed, 33 insertions(+), 54 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 878aa7f0ed..376e755e97 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1641,8 +1641,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
ret = NULL; /* good */
}
strbuf_release(&err);
- }
- else {
+ } else {
+ enum ref_transaction_error tx_err;
struct strbuf err = STRBUF_INIT;
if (shallow_update && si->shallow_ref[cmd->index] &&
update_shallow_ref(cmd, si)) {
@@ -1650,14 +1650,18 @@ static const char *update(struct command *cmd, struct shallow_info *si)
goto out;
}
- if (ref_transaction_update(transaction,
- namespaced_name,
- new_oid, old_oid,
- NULL, NULL,
- 0, "push",
- &err)) {
+ tx_err = ref_transaction_update(transaction,
+ namespaced_name,
+ new_oid, old_oid,
+ NULL, NULL,
+ 0, "push",
+ &err);
+ if (tx_err) {
rp_error("%s", err.buf);
- ret = "failed to update ref";
+ if (tx_err == REF_TRANSACTION_ERROR_GENERIC)
+ ret = "failed to update ref";
+ else
+ ret = ref_transaction_error_msg(tx_err);
} else {
ret = NULL; /* good */
}
diff --git a/refs.c b/refs.c
index efa16b739d..662a9e6f9e 100644
--- a/refs.c
+++ b/refs.c
@@ -1416,6 +1416,24 @@ enum ref_transaction_error ref_transaction_update(struct ref_transaction *transa
flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
flags |= (new_target ? REF_HAVE_NEW : 0) | (old_target ? REF_HAVE_OLD : 0);
+ if ((flags & REF_HAVE_NEW) && !new_target && !is_null_oid(new_oid) &&
+ !(flags & REF_SKIP_OID_VERIFICATION) && !(flags & REF_LOG_ONLY)) {
+ struct object *o = parse_object(transaction->ref_store->repo, new_oid);
+
+ if (!o) {
+ strbuf_addf(err,
+ _("trying to write ref '%s' with nonexistent object %s"),
+ refname, oid_to_hex(new_oid));
+ return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
+ }
+
+ if (o->type != OBJ_COMMIT && is_branch(refname)) {
+ strbuf_addf(err, _("trying to write non-commit object %s to branch '%s'"),
+ oid_to_hex(new_oid), refname);
+ return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
+ }
+ }
+
ref_transaction_add_update(transaction, refname, flags,
new_oid, old_oid, new_target,
old_target, NULL, msg);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4b2faf4777..f20f580fbc 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -19,7 +19,6 @@
#include "../iterator.h"
#include "../dir-iterator.h"
#include "../lockfile.h"
-#include "../object.h"
#include "../path.h"
#include "../dir.h"
#include "../chdir-notify.h"
@@ -1589,7 +1588,6 @@ static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)
static enum ref_transaction_error write_ref_to_lockfile(struct files_ref_store *refs,
struct ref_lock *lock,
const struct object_id *oid,
- int skip_oid_verification,
struct strbuf *err);
static int commit_ref_update(struct files_ref_store *refs,
struct ref_lock *lock,
@@ -1737,7 +1735,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
}
oidcpy(&lock->old_oid, &orig_oid);
- if (write_ref_to_lockfile(refs, lock, &orig_oid, 0, &err) ||
+ if (write_ref_to_lockfile(refs, lock, &orig_oid, &err) ||
commit_ref_update(refs, lock, &orig_oid, logmsg, 0, &err)) {
error("unable to write current sha1 into %s: %s", newrefname, err.buf);
strbuf_release(&err);
@@ -1755,7 +1753,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
goto rollbacklog;
}
- if (write_ref_to_lockfile(refs, lock, &orig_oid, 0, &err) ||
+ if (write_ref_to_lockfile(refs, lock, &orig_oid, &err) ||
commit_ref_update(refs, lock, &orig_oid, NULL, REF_SKIP_CREATE_REFLOG, &err)) {
error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
strbuf_release(&err);
@@ -1999,32 +1997,11 @@ static int files_log_ref_write(struct files_ref_store *refs,
static enum ref_transaction_error write_ref_to_lockfile(struct files_ref_store *refs,
struct ref_lock *lock,
const struct object_id *oid,
- int skip_oid_verification,
struct strbuf *err)
{
static char term = '\n';
- struct object *o;
int fd;
- if (!skip_oid_verification) {
- o = parse_object(refs->base.repo, oid);
- if (!o) {
- strbuf_addf(
- err,
- "trying to write ref '%s' with nonexistent object %s",
- lock->ref_name, oid_to_hex(oid));
- unlock_ref(lock);
- return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
- }
- if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
- strbuf_addf(
- err,
- "trying to write non-commit object %s to branch '%s'",
- oid_to_hex(oid), lock->ref_name);
- unlock_ref(lock);
- return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
- }
- }
fd = get_lock_file_fd(&lock->lk);
if (write_in_full(fd, oid_to_hex(oid), refs->base.repo->hash_algo->hexsz) < 0 ||
write_in_full(fd, &term, 1) < 0 ||
@@ -2828,7 +2805,6 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
} else {
ret = write_ref_to_lockfile(
refs, lock, &update->new_oid,
- update->flags & REF_SKIP_OID_VERIFICATION,
err);
if (ret) {
char *write_err = strbuf_detach(err, NULL);
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 93374d25c2..444b0c24e5 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1081,25 +1081,6 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
return 0;
}
- /* Verify that the new object ID is valid. */
- if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) &&
- !(u->flags & REF_SKIP_OID_VERIFICATION) &&
- !(u->flags & REF_LOG_ONLY)) {
- struct object *o = parse_object(refs->base.repo, &u->new_oid);
- if (!o) {
- strbuf_addf(err,
- _("trying to write ref '%s' with nonexistent object %s"),
- u->refname, oid_to_hex(&u->new_oid));
- return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
- }
-
- if (o->type != OBJ_COMMIT && is_branch(u->refname)) {
- strbuf_addf(err, _("trying to write non-commit object %s to branch '%s'"),
- oid_to_hex(&u->new_oid), u->refname);
- return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
- }
- }
-
/*
* When we update the reference that HEAD points to we enqueue
* a second log-only update for HEAD so that its reflog is
--
2.53.GIT
^ permalink raw reply related
* [PATCH v3 6/9] update-ref: handle rejections while adding updates
From: Karthik Nayak @ 2026-04-27 10:42 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, toon
In-Reply-To: <20260427-refs-move-to-generic-layer-v3-0-e4638dfb7897@gmail.com>
When using git-update-ref(1) with the '--batch-updates' flag, updates
rejected by the reference backend are displayed to the user while other
updates are applied. This only applies during the commit phase of the
transaction.
In the following commits, we'll also extend `ref_transaction_update()`
to reject updates before a transaction is prepared/committed. In
preparation, modify the code in update-ref to also handle non-generic
rejections from `ref_transaction_update()`. This involves propagating
information to each of the commands on whether updates are allowed to be
rejected, and also checking for rejections and only dying for generic
failures.
Errors encountered during updates will be shown to the user immediately
unlike other errors encountered only when the transaction is
prepared/committed. As the verification of object IDs and peeled tag
objects will move into `ref_transaction_update()` in the following
commit, this means that those errors will be shown to the user before
other errors, this changes the order of errors, but the functionality
remains the same.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/update-ref.c | 106 +++++++++++++++++++++++++++++++++++++--------------
1 file changed, 77 insertions(+), 29 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 5259cc7226..348b7fec94 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -25,6 +25,15 @@ static unsigned int default_flags;
static unsigned create_reflog_flag;
static const char *msg;
+struct command_options {
+ /*
+ * Individual updates are allowed to fail without causing
+ * update-ref to exit. This is set when using the
+ * '--batch-updates' flag.
+ */
+ bool allow_update_failures;
+};
+
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
* and append the result to arg. Return a pointer to the terminator.
@@ -268,11 +277,13 @@ static void print_rejected_refs(const char *refname,
*/
static void parse_cmd_update(struct ref_transaction *transaction,
- const char *next, const char *end)
+ const char *next, const char *end,
+ struct command_options *opts)
{
struct strbuf err = STRBUF_INIT;
char *refname;
struct object_id new_oid, old_oid;
+ enum ref_transaction_error tx_err;
int have_old;
refname = parse_refname(&next);
@@ -289,22 +300,35 @@ static void parse_cmd_update(struct ref_transaction *transaction,
if (*next != line_termination)
die("update %s: extra input: %s", refname, next);
- if (ref_transaction_update(transaction, refname,
- &new_oid, have_old ? &old_oid : NULL,
- NULL, NULL,
- update_flags | create_reflog_flag,
- msg, &err))
+ tx_err = ref_transaction_update(transaction, refname,
+ &new_oid, have_old ? &old_oid : NULL,
+ NULL, NULL,
+ update_flags | create_reflog_flag,
+ msg, &err);
+
+ /*
+ * Generic errors are non-recoverable, so we cannot skip the update
+ * or mark it as rejected.
+ */
+ if (tx_err == REF_TRANSACTION_ERROR_GENERIC)
die("%s", err.buf);
+ if (tx_err && opts->allow_update_failures)
+ print_rejected_refs(refname, have_old ? &old_oid : NULL,
+ &new_oid, NULL, NULL, tx_err, err.buf,
+ NULL);
+
update_flags = default_flags;
free(refname);
strbuf_release(&err);
}
static void parse_cmd_symref_update(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts)
{
char *refname, *new_target, *old_arg;
+ enum ref_transaction_error tx_err;
char *old_target = NULL;
struct strbuf err = STRBUF_INIT;
struct object_id old_oid;
@@ -341,14 +365,25 @@ static void parse_cmd_symref_update(struct ref_transaction *transaction,
if (*next != line_termination)
die("symref-update %s: extra input: %s", refname, next);
- if (ref_transaction_update(transaction, refname, NULL,
- have_old_oid ? &old_oid : NULL,
- new_target,
- have_old_oid ? NULL : old_target,
- update_flags | create_reflog_flag,
- msg, &err))
+ tx_err = ref_transaction_update(transaction, refname, NULL,
+ have_old_oid ? &old_oid : NULL,
+ new_target,
+ have_old_oid ? NULL : old_target,
+ update_flags | create_reflog_flag,
+ msg, &err);
+
+ /*
+ * Generic errors are non-recoverable, so we cannot skip the update
+ * or mark it as rejected.
+ */
+ if (tx_err == REF_TRANSACTION_ERROR_GENERIC)
die("%s", err.buf);
+ if (tx_err && opts->allow_update_failures)
+ print_rejected_refs(refname, have_old_oid ? &old_oid : NULL,
+ NULL, have_old_oid ? NULL : old_target,
+ new_target, tx_err, err.buf, NULL);
+
update_flags = default_flags;
free(refname);
free(old_arg);
@@ -358,7 +393,8 @@ static void parse_cmd_symref_update(struct ref_transaction *transaction,
}
static void parse_cmd_create(struct ref_transaction *transaction,
- const char *next, const char *end)
+ const char *next, const char *end,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
char *refname;
@@ -387,9 +423,9 @@ static void parse_cmd_create(struct ref_transaction *transaction,
strbuf_release(&err);
}
-
static void parse_cmd_symref_create(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
char *refname, *new_target;
@@ -417,7 +453,8 @@ static void parse_cmd_symref_create(struct ref_transaction *transaction,
}
static void parse_cmd_delete(struct ref_transaction *transaction,
- const char *next, const char *end)
+ const char *next, const char *end,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
char *refname;
@@ -450,9 +487,9 @@ static void parse_cmd_delete(struct ref_transaction *transaction,
strbuf_release(&err);
}
-
static void parse_cmd_symref_delete(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
char *refname, *old_target;
@@ -479,9 +516,9 @@ static void parse_cmd_symref_delete(struct ref_transaction *transaction,
strbuf_release(&err);
}
-
static void parse_cmd_verify(struct ref_transaction *transaction,
- const char *next, const char *end)
+ const char *next, const char *end,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
char *refname;
@@ -508,7 +545,8 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
}
static void parse_cmd_symref_verify(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
struct object_id old_oid;
@@ -550,7 +588,8 @@ static void report_ok(const char *command)
}
static void parse_cmd_option(struct ref_transaction *transaction UNUSED,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
const char *rest;
if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
@@ -560,7 +599,8 @@ static void parse_cmd_option(struct ref_transaction *transaction UNUSED,
}
static void parse_cmd_start(struct ref_transaction *transaction UNUSED,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
if (*next != line_termination)
die("start: extra input: %s", next);
@@ -568,7 +608,8 @@ static void parse_cmd_start(struct ref_transaction *transaction UNUSED,
}
static void parse_cmd_prepare(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf error = STRBUF_INIT;
if (*next != line_termination)
@@ -579,7 +620,8 @@ static void parse_cmd_prepare(struct ref_transaction *transaction,
}
static void parse_cmd_abort(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf error = STRBUF_INIT;
if (*next != line_termination)
@@ -590,7 +632,8 @@ static void parse_cmd_abort(struct ref_transaction *transaction,
}
static void parse_cmd_commit(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf error = STRBUF_INIT;
if (*next != line_termination)
@@ -618,7 +661,8 @@ enum update_refs_state {
static const struct parse_cmd {
const char *prefix;
- void (*fn)(struct ref_transaction *, const char *, const char *);
+ void (*fn)(struct ref_transaction *, const char *, const char *,
+ struct command_options *);
unsigned args;
enum update_refs_state state;
} command[] = {
@@ -644,6 +688,10 @@ static void update_refs_stdin(unsigned int flags)
struct ref_transaction *transaction;
int i, j;
+ struct command_options opts = {
+ .allow_update_failures = flags & REF_TRANSACTION_ALLOW_FAILURE,
+ };
+
transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
flags, &err);
if (!transaction)
@@ -721,7 +769,7 @@ static void update_refs_stdin(unsigned int flags)
}
cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,
- input.buf + input.len);
+ input.buf + input.len, &opts);
}
switch (state) {
--
2.53.GIT
^ permalink raw reply related
* [PATCH v3 5/9] update-ref: move `print_rejected_refs()` up
From: Karthik Nayak @ 2026-04-27 10:42 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, toon
In-Reply-To: <20260427-refs-move-to-generic-layer-v3-0-e4638dfb7897@gmail.com>
The `print_rejected_refs()` function is used to print any rejected refs
when using git-updated-ref(1) with the '--batch-updates' option. In the
following commit, we'll need to use this function in another place, so
move the function up to avoid a separate forward declaration.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/update-ref.c | 45 ++++++++++++++++++++++-----------------------
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2d68c40ecb..5259cc7226 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -234,6 +234,28 @@ static int parse_next_oid(const char **next, const char *end,
command, refname);
}
+static void print_rejected_refs(const char *refname,
+ const struct object_id *old_oid,
+ const struct object_id *new_oid,
+ const char *old_target,
+ const char *new_target,
+ enum ref_transaction_error err,
+ const char *details,
+ void *cb_data UNUSED)
+{
+ struct strbuf sb = STRBUF_INIT;
+
+ if (details && *details)
+ error("%s", details);
+
+ strbuf_addf(&sb, "rejected %s %s %s %s\n", refname,
+ new_oid ? oid_to_hex(new_oid) : new_target,
+ old_oid ? oid_to_hex(old_oid) : old_target,
+ ref_transaction_error_msg(err));
+
+ fwrite(sb.buf, sb.len, 1, stdout);
+ strbuf_release(&sb);
+}
/*
* The following five parse_cmd_*() functions parse the corresponding
@@ -567,29 +589,6 @@ static void parse_cmd_abort(struct ref_transaction *transaction,
report_ok("abort");
}
-static void print_rejected_refs(const char *refname,
- const struct object_id *old_oid,
- const struct object_id *new_oid,
- const char *old_target,
- const char *new_target,
- enum ref_transaction_error err,
- const char *details,
- void *cb_data UNUSED)
-{
- struct strbuf sb = STRBUF_INIT;
-
- if (details && *details)
- error("%s", details);
-
- strbuf_addf(&sb, "rejected %s %s %s %s\n", refname,
- new_oid ? oid_to_hex(new_oid) : new_target,
- old_oid ? oid_to_hex(old_oid) : old_target,
- ref_transaction_error_msg(err));
-
- fwrite(sb.buf, sb.len, 1, stdout);
- strbuf_release(&sb);
-}
-
static void parse_cmd_commit(struct ref_transaction *transaction,
const char *next, const char *end UNUSED)
{
--
2.53.GIT
^ 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