From 57af9cc2e628165bef849576e2d42d8b200717ee Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 8 Sep 2025 07:30:47 +0200 Subject: [PATCH 01/14] promisor-remote: refactor to get rid of 'struct strvec' In a following commit, we will use the new 'promisor-remote' protocol capability introduced by d460267613 (Add 'promisor-remote' capability to protocol v2, 2025-02-18) to pass and process more information about promisor remotes than just their name and url. For that purpose, we will need to store information about other fields, especially information that might or might not be available for different promisor remotes. Unfortunately using 'struct strvec', as we currently do, to store information about the promisor remotes with one 'struct strvec' for each field like "name" or "url" does not scale easily in that case. We would need one 'struct strvec' for each new field, and then we would have to pass all these 'struct strvec' around. Let's refactor this and introduce a new 'struct promisor_info'. It will only store promisor remote information in its members. For now it has only a 'name' member for the promisor remote name and an 'url' member for its URL. We will use a 'struct string_list' to store the instances of 'struct promisor_info'. For each 'item' in the string_list, 'item->string' will point to the promisor remote name and 'item->util' will point to the corresponding 'struct promisor_info' instance. Explicit members are used within 'struct promisor_info' for type safety and clarity regarding the specific information being handled, rather than a generic key-value store. We want to specify and document each field and its content, so adding new members to the struct as more fields are supported is fine. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 107 ++++++++++++++++++++++++++++------------------ 1 file changed, 66 insertions(+), 41 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index 08b0da896227c8..c3df8f071ef9bc 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -314,9 +314,35 @@ static int allow_unsanitized(char ch) return ch > 32 && ch < 127; } -static void promisor_info_vecs(struct repository *repo, - struct strvec *names, - struct strvec *urls) +/* + * Struct for promisor remotes involved in the "promisor-remote" + * protocol capability. + * + * Except for "name", each in this struct and its + * should correspond (either on the client side or on the server side) + * to a "remote.." config variable set to where + * "" is a promisor remote name. + */ +struct promisor_info { + const char *name; + const char *url; +}; + +static void promisor_info_list_clear(struct string_list *list) +{ + for (size_t i = 0; i < list->nr; i++) { + struct promisor_info *p = list->items[i].util; + free((char *)p->name); + free((char *)p->url); + } + string_list_clear(list, 1); +} + +/* + * Populate 'list' with promisor remote information from the config. + * The 'util' pointer of each list item will hold a 'struct promisor_info'. + */ +static void promisor_config_info_list(struct repository *repo, struct string_list *list) { struct promisor_remote *r; @@ -328,8 +354,14 @@ static void promisor_info_vecs(struct repository *repo, /* Only add remotes with a non empty URL */ if (!repo_config_get_string_tmp(the_repository, url_key, &url) && *url) { - strvec_push(names, r->name); - strvec_push(urls, url); + struct promisor_info *new_info = xcalloc(1, sizeof(*new_info)); + struct string_list_item *item; + + new_info->name = xstrdup(r->name); + new_info->url = xstrdup(url); + + item = string_list_append(list, new_info->name); + item->util = new_info; } free(url_key); @@ -340,47 +372,36 @@ char *promisor_remote_info(struct repository *repo) { struct strbuf sb = STRBUF_INIT; int advertise_promisors = 0; - struct strvec names = STRVEC_INIT; - struct strvec urls = STRVEC_INIT; + struct string_list config_info = STRING_LIST_INIT_NODUP; + struct string_list_item *item; repo_config_get_bool(the_repository, "promisor.advertise", &advertise_promisors); if (!advertise_promisors) return NULL; - promisor_info_vecs(repo, &names, &urls); + promisor_config_info_list(repo, &config_info); - if (!names.nr) + if (!config_info.nr) return NULL; - for (size_t i = 0; i < names.nr; i++) { - if (i) + for_each_string_list_item(item, &config_info) { + struct promisor_info *p = item->util; + + if (item != config_info.items) strbuf_addch(&sb, ';'); + strbuf_addstr(&sb, "name="); - strbuf_addstr_urlencode(&sb, names.v[i], allow_unsanitized); + strbuf_addstr_urlencode(&sb, p->name, allow_unsanitized); strbuf_addstr(&sb, ",url="); - strbuf_addstr_urlencode(&sb, urls.v[i], allow_unsanitized); + strbuf_addstr_urlencode(&sb, p->url, allow_unsanitized); } - strvec_clear(&names); - strvec_clear(&urls); + promisor_info_list_clear(&config_info); return strbuf_detach(&sb, NULL); } -/* - * Find first index of 'nicks' where there is 'nick'. 'nick' is - * compared case sensitively to the strings in 'nicks'. If not found - * 'nicks->nr' is returned. - */ -static size_t remote_nick_find(struct strvec *nicks, const char *nick) -{ - for (size_t i = 0; i < nicks->nr; i++) - if (!strcmp(nicks->v[i], nick)) - return i; - return nicks->nr; -} - enum accept_promisor { ACCEPT_NONE = 0, ACCEPT_KNOWN_URL, @@ -390,19 +411,23 @@ enum accept_promisor { static int should_accept_remote(enum accept_promisor accept, const char *remote_name, const char *remote_url, - struct strvec *names, struct strvec *urls) + struct string_list *config_info) { - size_t i; + struct promisor_info *p; + struct string_list_item *item; if (accept == ACCEPT_ALL) return 1; - i = remote_nick_find(names, remote_name); + /* Get config info for that promisor remote */ + item = string_list_lookup(config_info, remote_name); - if (i >= names->nr) + if (!item) /* We don't know about that remote */ return 0; + p = item->util; + if (accept == ACCEPT_KNOWN_NAME) return 1; @@ -414,11 +439,11 @@ static int should_accept_remote(enum accept_promisor accept, return 0; } - if (!strcmp(urls->v[i], remote_url)) + if (!strcmp(p->url, remote_url)) return 1; warning(_("known remote named '%s' but with URL '%s' instead of '%s'"), - remote_name, urls->v[i], remote_url); + remote_name, p->url, remote_url); return 0; } @@ -430,8 +455,7 @@ static void filter_promisor_remote(struct repository *repo, struct strbuf **remotes; const char *accept_str; enum accept_promisor accept = ACCEPT_NONE; - struct strvec names = STRVEC_INIT; - struct strvec urls = STRVEC_INIT; + struct string_list config_info = STRING_LIST_INIT_NODUP; if (!repo_config_get_string_tmp(the_repository, "promisor.acceptfromserver", &accept_str)) { if (!*accept_str || !strcasecmp("None", accept_str)) @@ -450,8 +474,10 @@ static void filter_promisor_remote(struct repository *repo, if (accept == ACCEPT_NONE) return; - if (accept != ACCEPT_ALL) - promisor_info_vecs(repo, &names, &urls); + if (accept != ACCEPT_ALL) { + promisor_config_info_list(repo, &config_info); + string_list_sort(&config_info); + } /* Parse remote info received */ @@ -482,7 +508,7 @@ static void filter_promisor_remote(struct repository *repo, if (remote_url) decoded_url = url_percent_decode(remote_url); - if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, &names, &urls)) + if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, &config_info)) strvec_push(accepted, decoded_name); strbuf_list_free(elems); @@ -490,8 +516,7 @@ static void filter_promisor_remote(struct repository *repo, free(decoded_url); } - strvec_clear(&names); - strvec_clear(&urls); + promisor_info_list_clear(&config_info); strbuf_list_free(remotes); } From 4bf7ae3123b2d2a2b0656af31c16401407664a9f Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 8 Sep 2025 07:30:48 +0200 Subject: [PATCH 02/14] promisor-remote: allow a server to advertise more fields For now the "promisor-remote" protocol capability can only pass "name" and "url" information from a server to a client in the form "name=,url=". To allow clients to make more informed decisions about which promisor remotes they accept, let's make it possible to pass more information by introducing a new "promisor.sendFields" configuration variable. On the server side, information about a remote `foo` is stored in configuration variables named `remote.foo.`. To make it clearer and simpler, we use `field` and `field name` like this: * `field name` refers to the part of such a configuration variable, and * `field` refers to both the `field name` and the value of such a configuration variable. The "promisor.sendFields" configuration variable should contain a comma or space separated list of field names that will be looked up in the configuration of the remote on the server to find the values that will be passed to the client. Only a set of predefined field names are allowed. The only field names in this set are "partialCloneFilter" and "token". The "partialCloneFilter" field name specifies the filter definition used by the promisor remote, and the "token" field name can provide an authentication credential for accessing it. For example, if "promisor.sendFields" is set to "partialCloneFilter", and the server has the "remote.foo.partialCloneFilter" config variable set to a value, then that value will be passed in the "partialCloneFilter" field in the form "partialCloneFilter=" after the "name" and "url" fields. A following commit will allow the client to use the information to decide if it accepts the remote or not. For now the client doesn't do anything with the additional information it receives. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- Documentation/config/promisor.adoc | 22 +++++ Documentation/gitprotocol-v2.adoc | 64 +++++++++---- promisor-remote.c | 129 ++++++++++++++++++++++++-- t/t5710-promisor-remote-capability.sh | 31 +++++++ 4 files changed, 221 insertions(+), 25 deletions(-) diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc index 2638b01f8308a1..b4a72c21521110 100644 --- a/Documentation/config/promisor.adoc +++ b/Documentation/config/promisor.adoc @@ -9,6 +9,28 @@ promisor.advertise:: "false", which means the "promisor-remote" capability is not advertised. +promisor.sendFields:: + A comma or space separated list of additional remote related + field names. A server sends these field names and the + associated field values from its configuration when + advertising its promisor remotes using the "promisor-remote" + capability, see linkgit:gitprotocol-v2[5]. Currently, only the + "partialCloneFilter" and "token" field names are supported. ++ +`partialCloneFilter`:: contains the partial clone filter +used for the remote. ++ +`token`:: contains an authentication token for the remote. ++ +When a field name is part of this list and a corresponding +"remote.foo." config variable is set on the server to a +non-empty value, then the field name and value are sent when +advertising the promisor remote "foo". ++ +This list has no effect unless the "promisor.advertise" config +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" diff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc index 9a57005d77773b..c7db103299ae54 100644 --- a/Documentation/gitprotocol-v2.adoc +++ b/Documentation/gitprotocol-v2.adoc @@ -785,33 +785,64 @@ retrieving the header from a bundle at the indicated URI, and thus save themselves and the server(s) the request(s) needed to inspect the headers of that bundle or bundles. -promisor-remote= -~~~~~~~~~~~~~~~~~~~~~~~~~~ +promisor-remote= +~~~~~~~~~~~~~~~~~~~~~~~~~ The server may advertise some promisor remotes it is using or knows about to a client which may want to use them as its promisor remotes, -instead of this repository. In this case should be of the +instead of this repository. In this case should be of the form: - pr-infos = pr-info | pr-infos ";" pr-info + pr-info = pr-fields | pr-info ";" pr-fields - pr-info = "name=" pr-name | "name=" pr-name "," "url=" pr-url + pr-fields = pr-field | pr-fields "," pr-field -where `pr-name` is the urlencoded name of a promisor remote, and -`pr-url` the urlencoded URL of that promisor remote. + pr-field = field-name "=" field-value -In this case, if the client decides to use one or more promisor -remotes the server advertised, it can reply with -"promisor-remote=" where should be of the form: +where all the `field-name` and `field-value` in a given `pr-fields` +are field names and values related to a single promisor remote. A +given `field-name` MUST NOT appear more than once in given +`pr-fields`. + +The server MUST advertise at least the "name" and "url" field names +along with the associated field values, which are the name of a valid +remote and its URL, in each `pr-fields`. The "name" and "url" fields +MUST appear first in each pr-fields, in that order. + +After these mandatory fields, the server MAY advertise the following +optional fields in any order: + +`partialCloneFilter`:: The filter specification used by the remote. +Clients can use this to determine if the remote's filtering strategy +is compatible with their needs (e.g., checking if both use "blob:none"). +It corresponds to the "remote..partialCloneFilter" config setting. + +`token`:: An authentication token that clients can use when +connecting to the remote. It corresponds to the "remote..token" +config setting. + +No other fields are defined by the protocol at this time. Field names +are case-sensitive and MUST be transmitted exactly as specified +above. Clients MUST ignore fields they don't recognize to allow for +future protocol extensions. + +For now, the client can only use information transmitted through these +fields to decide if it accepts the advertised promisor remote. In the +future that information might be used for other purposes though. + +Field values MUST be urlencoded. + +If the client decides to use one or more promisor remotes the server +advertised, it can reply with "promisor-remote=" where + should be of the form: pr-names = pr-name | pr-names ";" pr-name where `pr-name` is the urlencoded name of a promisor remote the server advertised and the client accepts. -Note that, everywhere in this document, `pr-name` MUST be a valid -remote name, and the ';' and ',' characters MUST be encoded if they -appear in `pr-name` or `pr-url`. +Note that, everywhere in this document, the ';' and ',' characters +MUST be encoded if they appear in `pr-name` or `field-value`. If the server doesn't know any promisor remote that could be good for a client to use, or prefers a client not to use any promisor remote it @@ -822,9 +853,10 @@ In this case, or if the client doesn't want to use any promisor remote the server advertised, the client shouldn't advertise the "promisor-remote" capability at all in its reply. -The "promisor.advertise" and "promisor.acceptFromServer" configuration -options can be used on the server and client side to control what they -advertise or accept respectively. See the documentation of these +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. See the documentation of these configuration options for more information. Note that in the future it would be nice if the "promisor-remote" diff --git a/promisor-remote.c b/promisor-remote.c index c3df8f071ef9bc..98ba59e9529332 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -314,6 +314,75 @@ static int allow_unsanitized(char ch) return ch > 32 && ch < 127; } +static const char promisor_field_filter[] = "partialCloneFilter"; +static const char promisor_field_token[] = "token"; + +/* + * List of optional field names that can be used in the + * "promisor-remote" protocol capability (others must be + * ignored). Each field should correspond to a configurable property + * of a remote that can be relevant for the client. + */ +static const char *known_fields[] = { + promisor_field_filter, /* Filter used for partial clone */ + promisor_field_token, /* Authentication token for the remote */ + NULL +}; + +/* + * Check if 'field' is in the list of the known field names for the + * "promisor-remote" protocol capability. + */ +static int is_known_field(const char *field) +{ + const char **p; + + for (p = known_fields; *p; p++) + if (!strcasecmp(*p, field)) + return 1; + return 0; +} + +static int is_valid_field(struct string_list_item *item, void *cb_data) +{ + const char *field = item->string; + const char *config_key = (const char *)cb_data; + + if (!is_known_field(field)) { + warning(_("unsupported field '%s' in '%s' config"), field, config_key); + return 0; + } + return 1; +} + +static char *fields_from_config(struct string_list *fields_list, const char *config_key) +{ + char *fields = NULL; + + if (!repo_config_get_string(the_repository, config_key, &fields) && *fields) { + string_list_split_in_place_f(fields_list, fields, ",", -1, + STRING_LIST_SPLIT_TRIM | + STRING_LIST_SPLIT_NONEMPTY); + filter_string_list(fields_list, 0, is_valid_field, (void *)config_key); + } + + return fields; +} + +static struct string_list *fields_sent(void) +{ + static struct string_list fields_list = STRING_LIST_INIT_NODUP; + static int initialized; + + if (!initialized) { + fields_list.cmp = strcasecmp; + fields_from_config(&fields_list, "promisor.sendFields"); + initialized = 1; + } + + return &fields_list; +} + /* * Struct for promisor remotes involved in the "promisor-remote" * protocol capability. @@ -326,6 +395,8 @@ static int allow_unsanitized(char ch) struct promisor_info { const char *name; const char *url; + const char *filter; + const char *token; }; static void promisor_info_list_clear(struct string_list *list) @@ -334,15 +405,47 @@ static void promisor_info_list_clear(struct string_list *list) struct promisor_info *p = list->items[i].util; free((char *)p->name); free((char *)p->url); + free((char *)p->filter); + free((char *)p->token); } string_list_clear(list, 1); } +static void set_one_field(struct promisor_info *p, + const char *field, const char *value) +{ + if (!strcasecmp(field, promisor_field_filter)) + p->filter = xstrdup(value); + else if (!strcasecmp(field, promisor_field_token)) + p->token = xstrdup(value); + else + BUG("invalid field '%s'", field); +} + +static void set_fields(struct promisor_info *p, + struct string_list *field_names) +{ + struct string_list_item *item; + + for_each_string_list_item(item, field_names) { + char *key = xstrfmt("remote.%s.%s", p->name, item->string); + const char *val; + if (!repo_config_get_string_tmp(the_repository, key, &val) && *val) + set_one_field(p, item->string, val); + free(key); + } +} + /* * Populate 'list' with promisor remote information from the config. - * The 'util' pointer of each list item will hold a 'struct promisor_info'. + * The 'util' pointer of each list item will hold a 'struct + * promisor_info'. Except "name" and "url", only members of that + * struct specified by the 'field_names' list are set (using values + * from the configuration). */ -static void promisor_config_info_list(struct repository *repo, struct string_list *list) +static void promisor_config_info_list(struct repository *repo, + struct string_list *list, + struct string_list *field_names) { struct promisor_remote *r; @@ -360,6 +463,9 @@ static void promisor_config_info_list(struct repository *repo, struct string_lis new_info->name = xstrdup(r->name); new_info->url = xstrdup(url); + if (field_names) + set_fields(new_info, field_names); + item = string_list_append(list, new_info->name); item->util = new_info; } @@ -380,7 +486,7 @@ char *promisor_remote_info(struct repository *repo) if (!advertise_promisors) return NULL; - promisor_config_info_list(repo, &config_info); + promisor_config_info_list(repo, &config_info, fields_sent()); if (!config_info.nr) return NULL; @@ -395,6 +501,15 @@ char *promisor_remote_info(struct repository *repo) strbuf_addstr_urlencode(&sb, p->name, allow_unsanitized); strbuf_addstr(&sb, ",url="); strbuf_addstr_urlencode(&sb, p->url, allow_unsanitized); + + if (p->filter) { + strbuf_addf(&sb, ",%s=", promisor_field_filter); + strbuf_addstr_urlencode(&sb, p->filter, allow_unsanitized); + } + if (p->token) { + strbuf_addf(&sb, ",%s=", promisor_field_token); + strbuf_addstr_urlencode(&sb, p->token, allow_unsanitized); + } } promisor_info_list_clear(&config_info); @@ -475,7 +590,7 @@ static void filter_promisor_remote(struct repository *repo, return; if (accept != ACCEPT_ALL) { - promisor_config_info_list(repo, &config_info); + promisor_config_info_list(repo, &config_info, NULL); string_list_sort(&config_info); } @@ -494,13 +609,9 @@ static void filter_promisor_remote(struct repository *repo, elems = strbuf_split(remotes[i], ','); for (size_t j = 0; elems[j]; j++) { - int res; strbuf_strip_suffix(elems[j], ","); - res = skip_prefix(elems[j]->buf, "name=", &remote_name) || + if (!skip_prefix(elems[j]->buf, "name=", &remote_name)) skip_prefix(elems[j]->buf, "url=", &remote_url); - if (!res) - warning(_("unknown element '%s' from remote info"), - elems[j]->buf); } if (remote_name) diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh index cb061b1f35efcc..204528b2e0cc69 100755 --- a/t/t5710-promisor-remote-capability.sh +++ b/t/t5710-promisor-remote-capability.sh @@ -295,6 +295,37 @@ test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" ' check_missing_objects server 1 "$oid" ' +test_expect_success "clone with promisor.sendFields" ' + git -C server config promisor.advertise true && + test_when_finished "rm -rf client" && + + git -C server remote add otherLop "https://invalid.invalid" && + git -C server config remote.otherLop.token "fooBar" && + git -C server config remote.otherLop.stuff "baz" && + git -C server config remote.otherLop.partialCloneFilter "blob:limit=10k" && + test_when_finished "git -C server remote remove otherLop" && + test_config -C server promisor.sendFields "partialCloneFilter, token" && + test_when_finished "rm trace" && + + # Clone from server to create a client + GIT_TRACE_PACKET="$(pwd)/trace" 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="file://$(pwd)/lop" \ + -c promisor.acceptfromserver=All \ + --no-local --filter="blob:limit=5k" server client && + + # Check that fields are properly transmitted + ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") && + PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" && + PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" && + test_grep "clone< promisor-remote=$PR1;$PR2" trace && + test_grep "clone> promisor-remote=lop;otherLop" trace && + + # Check that the largest object is still missing on the server + check_missing_objects server 1 "$oid" +' + test_expect_success "clone with promisor.advertise set to 'true' but don't delete the client" ' git -C server config promisor.advertise true && From 4e2139c9c52766f2853dd42c7ff76eee5ac86449 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 8 Sep 2025 07:30:49 +0200 Subject: [PATCH 03/14] promisor-remote: use string constants for 'name' and 'url' too A previous commit started to define `promisor_field_filter` and `promisor_field_token`, and used them instead of the "partialCloneFilter" and "token" string literals. Let's do the same for "name" and "url" to avoid repeating them several times and for consistency with the other fields. For skipping "name=" or "url=" in advertisements, let's introduce a skip_field_name_prefix() helper function to keep parsing clean and easy to understand. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index 98ba59e9529332..3913e32c1166cc 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -314,6 +314,12 @@ static int allow_unsanitized(char ch) return ch > 32 && ch < 127; } +/* + * All the fields used in "promisor-remote" protocol capability, + * including the mandatory "name" and "url" ones. + */ +static const char promisor_field_name[] = "name"; +static const char promisor_field_url[] = "url"; static const char promisor_field_filter[] = "partialCloneFilter"; static const char promisor_field_token[] = "token"; @@ -497,9 +503,9 @@ char *promisor_remote_info(struct repository *repo) if (item != config_info.items) strbuf_addch(&sb, ';'); - strbuf_addstr(&sb, "name="); + strbuf_addf(&sb, "%s=", promisor_field_name); strbuf_addstr_urlencode(&sb, p->name, allow_unsanitized); - strbuf_addstr(&sb, ",url="); + strbuf_addf(&sb, ",%s=", promisor_field_url); strbuf_addstr_urlencode(&sb, p->url, allow_unsanitized); if (p->filter) { @@ -563,6 +569,15 @@ static int should_accept_remote(enum accept_promisor accept, return 0; } +static int skip_field_name_prefix(const char *elem, const char *field_name, const char **value) +{ + const char *p; + if (!skip_prefix(elem, field_name, &p) || *p != '=') + return 0; + *value = p + 1; + return 1; +} + static void filter_promisor_remote(struct repository *repo, struct strvec *accepted, const char *info) @@ -610,8 +625,8 @@ static void filter_promisor_remote(struct repository *repo, for (size_t j = 0; elems[j]; j++) { strbuf_strip_suffix(elems[j], ","); - if (!skip_prefix(elems[j]->buf, "name=", &remote_name)) - skip_prefix(elems[j]->buf, "url=", &remote_url); + if (!skip_field_name_prefix(elems[j]->buf, promisor_field_name, &remote_name)) + skip_field_name_prefix(elems[j]->buf, promisor_field_url, &remote_url); } if (remote_name) From de1efeaf0cee5ca8947ead8b83235e84652c657f Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 8 Sep 2025 07:30:50 +0200 Subject: [PATCH 04/14] promisor-remote: refactor how we parse advertised fields In a follow up commit we are going to parse more fields, like a filter and a token, coming from the server when it advertises promisor remotes using the "promisor-remote" capability. To prepare for this, let's refactor the code that parses the advertised fields coming from the server into a new parse_one_advertised_remote() function that will populate a `struct promisor_info` with the content of the fields it parsed. While at it, let's also pass this `struct promisor_info` to the should_accept_remote() function, instead of passing it the parsed name and url. These changes will make it simpler to both parse more fields and access the content of these parsed fields in follow up commits. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 86 +++++++++++++++++++++++++++++++---------------- 1 file changed, 57 insertions(+), 29 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index 3913e32c1166cc..c22128d09e3425 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -405,16 +405,20 @@ struct promisor_info { const char *token; }; +static void promisor_info_free(struct promisor_info *p) +{ + free((char *)p->name); + free((char *)p->url); + free((char *)p->filter); + free((char *)p->token); + free(p); +} + static void promisor_info_list_clear(struct string_list *list) { - for (size_t i = 0; i < list->nr; i++) { - struct promisor_info *p = list->items[i].util; - free((char *)p->name); - free((char *)p->url); - free((char *)p->filter); - free((char *)p->token); - } - string_list_clear(list, 1); + for (size_t i = 0; i < list->nr; i++) + promisor_info_free(list->items[i].util); + string_list_clear(list, 0); } static void set_one_field(struct promisor_info *p, @@ -531,11 +535,13 @@ enum accept_promisor { }; static int should_accept_remote(enum accept_promisor accept, - const char *remote_name, const char *remote_url, + struct promisor_info *advertised, struct string_list *config_info) { struct promisor_info *p; struct string_list_item *item; + const char *remote_name = advertised->name; + const char *remote_url = advertised->url; if (accept == ACCEPT_ALL) return 1; @@ -578,6 +584,41 @@ static int skip_field_name_prefix(const char *elem, const char *field_name, cons return 1; } +static struct promisor_info *parse_one_advertised_remote(const char *remote_info) +{ + struct promisor_info *info = xcalloc(1, sizeof(*info)); + struct string_list elem_list = STRING_LIST_INIT_DUP; + struct string_list_item *item; + + string_list_split(&elem_list, remote_info, ",", -1); + + for_each_string_list_item(item, &elem_list) { + const char *elem = item->string; + const char *p = strchr(elem, '='); + + if (!p) { + warning(_("invalid element '%s' from remote info"), elem); + continue; + } + + if (skip_field_name_prefix(elem, promisor_field_name, &p)) + info->name = url_percent_decode(p); + else if (skip_field_name_prefix(elem, promisor_field_url, &p)) + info->url = url_percent_decode(p); + } + + string_list_clear(&elem_list, 0); + + if (!info->name || !info->url) { + warning(_("server advertised a promisor remote without a name or URL: %s"), + remote_info); + promisor_info_free(info); + return NULL; + } + + return info; +} + static void filter_promisor_remote(struct repository *repo, struct strvec *accepted, const char *info) @@ -614,32 +655,19 @@ static void filter_promisor_remote(struct repository *repo, remotes = strbuf_split_str(info, ';', 0); for (size_t i = 0; remotes[i]; i++) { - struct strbuf **elems; - const char *remote_name = NULL; - const char *remote_url = NULL; - char *decoded_name = NULL; - char *decoded_url = NULL; + struct promisor_info *advertised; strbuf_strip_suffix(remotes[i], ";"); - elems = strbuf_split(remotes[i], ','); - for (size_t j = 0; elems[j]; j++) { - strbuf_strip_suffix(elems[j], ","); - if (!skip_field_name_prefix(elems[j]->buf, promisor_field_name, &remote_name)) - skip_field_name_prefix(elems[j]->buf, promisor_field_url, &remote_url); - } + advertised = parse_one_advertised_remote(remotes[i]->buf); - if (remote_name) - decoded_name = url_percent_decode(remote_name); - if (remote_url) - decoded_url = url_percent_decode(remote_url); + if (!advertised) + continue; - if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, &config_info)) - strvec_push(accepted, decoded_name); + if (should_accept_remote(accept, advertised, &config_info)) + strvec_push(accepted, advertised->name); - strbuf_list_free(elems); - free(decoded_name); - free(decoded_url); + promisor_info_free(advertised); } promisor_info_list_clear(&config_info); From bcb08c837570f24a82d6484fc5f475372820e3f3 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 8 Sep 2025 07:30:51 +0200 Subject: [PATCH 05/14] promisor-remote: use string_list_split() in filter_promisor_remote() A previous commit introduced a new parse_one_advertised_remote() function that takes a `const char *` argument. This function is called from filter_promisor_remote() and parses all the fields for one remote. This means that in filter_promisor_remote() we no longer need to split the remote information that will be passed to parse_one_advertised_remote() into an array of relatively heavy and complex `struct strbuf`. To use something lighter, let's then replace strbuf_split_str() with string_list_split() in filter_promisor_remote() to parse the remote information that is passed to parse_one_advertised_remote(). Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index c22128d09e3425..afec0d081dd1da 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -623,10 +623,11 @@ static void filter_promisor_remote(struct repository *repo, struct strvec *accepted, const char *info) { - struct strbuf **remotes; const char *accept_str; enum accept_promisor accept = ACCEPT_NONE; struct string_list config_info = STRING_LIST_INIT_NODUP; + struct string_list remote_info = STRING_LIST_INIT_DUP; + struct string_list_item *item; if (!repo_config_get_string_tmp(the_repository, "promisor.acceptfromserver", &accept_str)) { if (!*accept_str || !strcasecmp("None", accept_str)) @@ -652,14 +653,12 @@ static void filter_promisor_remote(struct repository *repo, /* Parse remote info received */ - remotes = strbuf_split_str(info, ';', 0); + string_list_split(&remote_info, info, ";", -1); - for (size_t i = 0; remotes[i]; i++) { + for_each_string_list_item(item, &remote_info) { struct promisor_info *advertised; - strbuf_strip_suffix(remotes[i], ";"); - - advertised = parse_one_advertised_remote(remotes[i]->buf); + advertised = parse_one_advertised_remote(item->string); if (!advertised) continue; @@ -671,7 +670,7 @@ static void filter_promisor_remote(struct repository *repo, } promisor_info_list_clear(&config_info); - strbuf_list_free(remotes); + string_list_clear(&remote_info, 0); } char *promisor_remote_reply(const char *info) From c213820c512dc0a5cfe11a075e41f789f3225923 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 8 Sep 2025 07:30:52 +0200 Subject: [PATCH 06/14] promisor-remote: allow a client to check fields A previous commit allowed a server to pass additional fields through the "promisor-remote" protocol capability after the "name" and "url" fields, specifically the "partialCloneFilter" and "token" fields. Let's make it possible for a client to check if these fields match what it expects before accepting a promisor remote. We allow this by introducing a new "promisor.checkFields" configuration variable. It should contain a comma or space separated list of fields that will be checked. By limiting the protocol to specific well-defined fields, we ensure both server and client have a shared understanding of field semantics and usage. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- Documentation/config/promisor.adoc | 39 ++++++++++++ promisor-remote.c | 89 ++++++++++++++++++++++++--- t/t5710-promisor-remote-capability.sh | 34 ++++++++++ 3 files changed, 154 insertions(+), 8 deletions(-) diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc index b4a72c21521110..93e5e0d9b55eb4 100644 --- a/Documentation/config/promisor.adoc +++ b/Documentation/config/promisor.adoc @@ -50,3 +50,42 @@ promisor.acceptFromServer:: 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]. + +promisor.checkFields:: + A comma or space separated list of additional remote related + field names. A client checks if the values of these fields + transmitted by a server correspond to the values of these + fields in its own configuration before accepting a promisor + remote. Currently, "partialCloneFilter" and "token" are the + only supported field names. ++ +If one of these field names (e.g., "token") is being checked for an +advertised promisor remote (e.g., "foo"), three conditions must be met +for the check of this specific field to pass: ++ +1. The corresponding local configuration (e.g., `remote.foo.token`) + must be set. +2. The server must advertise the "token" field for remote "foo". +3. The value of the locally configured `remote.foo.token` must exactly + match the value advertised by the server for the "token" field. ++ +If any of these conditions is not met for any field name listed in +`promisor.checkFields`, the advertised remote "foo" is rejected. ++ +For the "partialCloneFilter" field, this allows the client to ensure +that the server's filter matches what it expects locally, preventing +inconsistencies in filtering behavior. For the "token" field, this can +be used to verify that authentication credentials match expected +values. ++ +Field values are compared case-sensitively. ++ +The "name" and "url" fields are always checked according to the +`promisor.acceptFromServer` policy, independently of this setting. ++ +The field names and values should be passed by the server through the +"promisor-remote" capability by using the `promisor.sendFields` config +variable. The fields are checked only if the +`promisor.acceptFromServer` config variable is not set to "None". If +set to "None", this config variable has no effect. See +linkgit:gitprotocol-v2[5]. diff --git a/promisor-remote.c b/promisor-remote.c index afec0d081dd1da..a6cfade22377f4 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -389,6 +389,20 @@ static struct string_list *fields_sent(void) return &fields_list; } +static struct string_list *fields_checked(void) +{ + static struct string_list fields_list = STRING_LIST_INIT_NODUP; + static int initialized; + + if (!initialized) { + fields_list.cmp = strcasecmp; + fields_from_config(&fields_list, "promisor.checkFields"); + initialized = 1; + } + + return &fields_list; +} + /* * Struct for promisor remotes involved in the "promisor-remote" * protocol capability. @@ -534,6 +548,61 @@ enum accept_promisor { ACCEPT_ALL }; +static int match_field_against_config(const char *field, const char *value, + struct promisor_info *config_info) +{ + if (config_info->filter && !strcasecmp(field, promisor_field_filter)) + return !strcmp(config_info->filter, value); + else if (config_info->token && !strcasecmp(field, promisor_field_token)) + return !strcmp(config_info->token, value); + + return 0; +} + +static int all_fields_match(struct promisor_info *advertised, + struct string_list *config_info, + int in_list) +{ + struct string_list *fields = fields_checked(); + struct string_list_item *item_checked; + + for_each_string_list_item(item_checked, fields) { + int match = 0; + const char *field = item_checked->string; + const char *value = NULL; + struct string_list_item *item; + + if (!strcasecmp(field, promisor_field_filter)) + value = advertised->filter; + else if (!strcasecmp(field, promisor_field_token)) + value = advertised->token; + + if (!value) + return 0; + + if (in_list) { + for_each_string_list_item(item, config_info) { + struct promisor_info *p = item->util; + if (match_field_against_config(field, value, p)) { + match = 1; + break; + } + } + } else { + item = string_list_lookup(config_info, advertised->name); + if (item) { + struct promisor_info *p = item->util; + match = match_field_against_config(field, value, p); + } + } + + if (!match) + return 0; + } + + return 1; +} + static int should_accept_remote(enum accept_promisor accept, struct promisor_info *advertised, struct string_list *config_info) @@ -544,7 +613,7 @@ static int should_accept_remote(enum accept_promisor accept, const char *remote_url = advertised->url; if (accept == ACCEPT_ALL) - return 1; + return all_fields_match(advertised, config_info, 1); /* Get config info for that promisor remote */ item = string_list_lookup(config_info, remote_name); @@ -556,7 +625,7 @@ static int should_accept_remote(enum accept_promisor accept, p = item->util; if (accept == ACCEPT_KNOWN_NAME) - return 1; + return all_fields_match(advertised, config_info, 0); if (accept != ACCEPT_KNOWN_URL) BUG("Unhandled 'enum accept_promisor' value '%d'", accept); @@ -567,7 +636,7 @@ static int should_accept_remote(enum accept_promisor accept, } if (!strcmp(p->url, remote_url)) - return 1; + return all_fields_match(advertised, config_info, 0); warning(_("known remote named '%s' but with URL '%s' instead of '%s'"), remote_name, p->url, remote_url); @@ -605,6 +674,10 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info info->name = url_percent_decode(p); else if (skip_field_name_prefix(elem, promisor_field_url, &p)) info->url = url_percent_decode(p); + else if (skip_field_name_prefix(elem, promisor_field_filter, &p)) + info->filter = url_percent_decode(p); + else if (skip_field_name_prefix(elem, promisor_field_token, &p)) + info->token = url_percent_decode(p); } string_list_clear(&elem_list, 0); @@ -646,11 +719,6 @@ static void filter_promisor_remote(struct repository *repo, if (accept == ACCEPT_NONE) return; - if (accept != ACCEPT_ALL) { - promisor_config_info_list(repo, &config_info, NULL); - string_list_sort(&config_info); - } - /* Parse remote info received */ string_list_split(&remote_info, info, ";", -1); @@ -663,6 +731,11 @@ static void filter_promisor_remote(struct repository *repo, if (!advertised) continue; + if (!config_info.nr) { + promisor_config_info_list(repo, &config_info, fields_checked()); + string_list_sort(&config_info); + } + if (should_accept_remote(accept, advertised, &config_info)) strvec_push(accepted, advertised->name); diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh index 204528b2e0cc69..023735d6a84ea8 100755 --- a/t/t5710-promisor-remote-capability.sh +++ b/t/t5710-promisor-remote-capability.sh @@ -326,6 +326,40 @@ test_expect_success "clone with promisor.sendFields" ' check_missing_objects server 1 "$oid" ' +test_expect_success "clone with promisor.checkFields" ' + git -C server config promisor.advertise true && + test_when_finished "rm -rf client" && + + git -C server remote add otherLop "https://invalid.invalid" && + git -C server config remote.otherLop.token "fooBar" && + git -C server config remote.otherLop.stuff "baz" && + git -C server config remote.otherLop.partialCloneFilter "blob:limit=10k" && + test_when_finished "git -C server remote remove otherLop" && + test_config -C server promisor.sendFields "partialCloneFilter, token" && + test_when_finished "rm trace" && + + # Clone from server to create a client + GIT_TRACE_PACKET="$(pwd)/trace" 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="file://$(pwd)/lop" \ + -c remote.lop.partialCloneFilter="blob:none" \ + -c promisor.acceptfromserver=All \ + -c promisor.checkFields=partialcloneFilter \ + --no-local --filter="blob:limit=5k" server client && + + # Check that fields are properly transmitted + ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") && + PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" && + PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" && + test_grep "clone< promisor-remote=$PR1;$PR2" trace && + test_grep "clone> promisor-remote=lop" trace && + test_grep ! "clone> promisor-remote=lop;otherLop" trace && + + # Check that the largest object is still missing on the server + check_missing_objects server 1 "$oid" +' + test_expect_success "clone with promisor.advertise set to 'true' but don't delete the client" ' git -C server config promisor.advertise true && From 68a746e9a892f8afa910cdf5c5360dae69193599 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 8 Sep 2025 07:30:53 +0200 Subject: [PATCH 07/14] promisor-remote: use string_list_split() in mark_remotes_as_accepted() Previous commits replaced some strbuf_split*() calls with calls to string_list_split*() in "promisor-remote.c". For consistency, let's also replace the strbuf_split_str() call in mark_remotes_as_accepted() with a call to string_list_split(), as we don't need the splitted strings to be managed by a `struct strbuf`. Using the lighter-weight `string_list` API is enough for our needs. While at it let's remove a useless call to `strbuf_strip_suffix()`. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index a6cfade22377f4..77ebf537e2b3ee 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -769,16 +769,15 @@ char *promisor_remote_reply(const char *info) void mark_promisor_remotes_as_accepted(struct repository *r, const char *remotes) { - struct strbuf **accepted_remotes = strbuf_split_str(remotes, ';', 0); + struct string_list accepted_remotes = STRING_LIST_INIT_DUP; + struct string_list_item *item; - for (size_t i = 0; accepted_remotes[i]; i++) { - struct promisor_remote *p; - char *decoded_remote; + string_list_split(&accepted_remotes, remotes, ";", -1); - strbuf_strip_suffix(accepted_remotes[i], ";"); - decoded_remote = url_percent_decode(accepted_remotes[i]->buf); + for_each_string_list_item(item, &accepted_remotes) { + char *decoded_remote = url_percent_decode(item->string); + struct promisor_remote *p = repo_promisor_remote_find(r, decoded_remote); - p = repo_promisor_remote_find(r, decoded_remote); if (p) p->accepted = 1; else @@ -788,5 +787,5 @@ void mark_promisor_remotes_as_accepted(struct repository *r, const char *remotes free(decoded_remote); } - strbuf_list_free(accepted_remotes); + string_list_clear(&accepted_remotes, 0); } From 89b4183efe14cc9bda1c8542b8e08dcc66a2b7d5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 8 Sep 2025 12:42:32 -0400 Subject: [PATCH 08/14] stash: pass --no-color to diff plumbing child processes After a partial stash, we may clear out the working tree by capturing the output of diff-tree and piping it into git-apply (and likewise we may use diff-index to restore the index). So we most definitely do not want color diff output from that diff-tree process. And it normally would not produce any, since its stdout is not going to a tty, and the default value of color.ui is "auto". However, if GIT_PAGER_IN_USE is set in the environment, that overrides the tty check, and we'll produce a colorized diff that chokes git-apply: $ echo y | GIT_PAGER_IN_USE=1 git stash -p [...] Saved working directory and index state WIP on main: 4f2e2bb foo error: No valid patches in input (allow with "--allow-empty") Cannot remove worktree changes Setting this variable is a relatively silly thing to do, and not something most users would run into. But we sometimes do it in our tests to stimulate color. And it is a user-visible bug, so let's fix it rather than work around it in the tests. The root issue here is that diff-tree (and other diff plumbing) should probably not ever produce color by default. It does so not by parsing color.ui, but because of the baked-in "auto" default from 4c7f1819b3 (make color.ui default to 'auto', 2013-06-10). But changing that is risky; we've had discussions back and forth on the topic over the years. E.g.: https://lore.kernel.org/git/86D0A377-8AFD-460D-A90E-6327C6934DFC@gmail.com/. So let's accept that as the status quo for now and protect ourselves by passing --no-color to the child processes. This is the same thing we did for add-interactive itself in 1c6ffb546b (add--interactive.perl: specify --no-color explicitly, 2020-09-07). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/stash.c | 5 ++++- t/t3904-stash-patch.sh | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index 1977e50df27fc5..063208b9b4834e 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -377,7 +377,7 @@ static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit) * however it should be done together with apply_cached. */ cp.git_cmd = 1; - strvec_pushl(&cp.args, "diff-tree", "--binary", NULL); + strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL); strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex); return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); @@ -1283,6 +1283,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch, cp_diff_tree.git_cmd = 1; strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "--binary", + "--no-color", "-U1", "HEAD", oid_to_hex(&info->w_tree), "--", NULL); if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) { ret = -1; @@ -1345,6 +1346,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, cp_diff_tree.git_cmd = 1; strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD", + "--no-color", oid_to_hex(&info->w_tree), "--", NULL); if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) { ret = -1; @@ -1719,6 +1721,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q cp_diff.git_cmd = 1; strvec_pushl(&cp_diff.args, "diff-index", "-p", + "--no-color", "--cached", "--binary", "HEAD", "--", NULL); add_pathspecs(&cp_diff.args, ps); diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh index ae313e3c705e46..90a4ff2c102cef 100755 --- a/t/t3904-stash-patch.sh +++ b/t/t3904-stash-patch.sh @@ -107,4 +107,23 @@ test_expect_success 'stash -p with split hunk' ' ! grep "added line 2" test ' +test_expect_success 'stash -p not confused by GIT_PAGER_IN_USE' ' + echo to-stash >test && + # Set both GIT_PAGER_IN_USE and TERM. Our goal is to entice any + # diff subprocesses into thinking that they could output + # color, even though their stdout is not going into a tty. + echo y | + GIT_PAGER_IN_USE=1 TERM=vt100 git stash -p && + git diff --exit-code +' + +test_expect_success 'index push not confused by GIT_PAGER_IN_USE' ' + echo index >test && + git add test && + echo working-tree >test && + # As above, we try to entice the child diff into using color. + GIT_PAGER_IN_USE=1 TERM=vt100 git stash push test && + git diff --exit-code +' + test_done From 8c78b5c8bc42728dcc8a527401955b7d1089e667 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 8 Sep 2025 12:42:36 -0400 Subject: [PATCH 09/14] add-interactive: respect color.diff for diff coloring The old perl git-add--interactive.perl script used the color.diff config option to decide whether to color diffs (and if not set, it fell back to the value of color.ui via git-config's --get-colorbool option). When we switched to the builtin version, this was lost: we respect only color.ui. So for example: git -c color.diff=false add -p would color the diff, even when it should not. The culprit is this line in add-interactive.c's parse_diff(): if (want_color_fd(1, -1)) That "-1" means "no config has been set", which causes it to fall back to the color.ui setting. We should instead be passing the value of color.diff. But the problem is that we never even parse that config option! Instead the builtin interactive code parses only the value of color.interactive, which is used for prompts and other messages. One could perhaps argue that this should cover interactive diff coloring, too, but historically it did not. The perl script treated color.interactive and color.diff separately. So we should grab the values for both, keeping separate fields in our add_i_state variable, rather than a single use_color field. We also load individual color slots (e.g., color.interactive.prompt), leaving them as the empty string when color is disabled. This happens via the init_color() helper in add-interactive, which checks that use_color field. Now that there are two such fields, we need to pass the appropriate one for each color. The colors are mostly easy to divide up; color.interactive.* follows color.interactive, and color.diff.* follows color.diff. But the "reset" color is tricky. It is used for both types of coloring, but the two can be configured independently. So we introduce two separate reset colors, and use each in the appropriate spot. There are two new tests. The first enables interactive prompt colors but disables color.diff. We should see a colored prompt but not a colored diff, showing that we are now respecting color.diff (and not color.interactive or color.ui). The second does the opposite. We disable color.interactive but turn on color.diff with a custom fragment color. When we split a hunk, the interactive code has to re-color the hunk header, which lets us check that we correctly loaded the color.diff.frag config based on color.diff, not color.interactive. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- add-interactive.c | 79 ++++++++++++++++++++++---------------- add-interactive.h | 7 +++- add-patch.c | 12 +++--- t/t3701-add-interactive.sh | 38 ++++++++++++++++++ 4 files changed, 95 insertions(+), 41 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 3e692b47eca0a3..877160d298556e 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -20,14 +20,14 @@ #include "prompt.h" #include "tree.h" -static void init_color(struct repository *r, struct add_i_state *s, +static void init_color(struct repository *r, int use_color, const char *section_and_slot, char *dst, const char *default_color) { char *key = xstrfmt("color.%s", section_and_slot); const char *value; - if (!s->use_color) + if (!use_color) dst[0] = '\0'; else if (repo_config_get_value(r, key, &value) || color_parse(value, dst)) @@ -36,42 +36,54 @@ static void init_color(struct repository *r, struct add_i_state *s, free(key); } -void init_add_i_state(struct add_i_state *s, struct repository *r, - struct add_p_opt *add_p_opt) +static int check_color_config(struct repository *r, const char *var) { const char *value; + int ret; + + if (repo_config_get_value(r, var, &value)) + ret = -1; + else + ret = git_config_colorbool(var, value); + return want_color(ret); +} +void init_add_i_state(struct add_i_state *s, struct repository *r, + struct add_p_opt *add_p_opt) +{ s->r = r; s->context = -1; s->interhunkcontext = -1; - if (repo_config_get_value(r, "color.interactive", &value)) - s->use_color = -1; - else - s->use_color = - git_config_colorbool("color.interactive", value); - s->use_color = want_color(s->use_color); - - init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD); - init_color(r, s, "interactive.help", s->help_color, GIT_COLOR_BOLD_RED); - init_color(r, s, "interactive.prompt", s->prompt_color, - GIT_COLOR_BOLD_BLUE); - init_color(r, s, "interactive.error", s->error_color, - GIT_COLOR_BOLD_RED); - - init_color(r, s, "diff.frag", s->fraginfo_color, - diff_get_color(s->use_color, DIFF_FRAGINFO)); - init_color(r, s, "diff.context", s->context_color, "fall back"); + s->use_color_interactive = check_color_config(r, "color.interactive"); + + init_color(r, s->use_color_interactive, "interactive.header", + s->header_color, GIT_COLOR_BOLD); + init_color(r, s->use_color_interactive, "interactive.help", + s->help_color, GIT_COLOR_BOLD_RED); + init_color(r, s->use_color_interactive, "interactive.prompt", + s->prompt_color, GIT_COLOR_BOLD_BLUE); + init_color(r, s->use_color_interactive, "interactive.error", + s->error_color, GIT_COLOR_BOLD_RED); + strlcpy(s->reset_color_interactive, + s->use_color_interactive ? GIT_COLOR_RESET : "", COLOR_MAXLEN); + + s->use_color_diff = check_color_config(r, "color.diff"); + + init_color(r, s->use_color_diff, "diff.frag", s->fraginfo_color, + diff_get_color(s->use_color_diff, DIFF_FRAGINFO)); + init_color(r, s->use_color_diff, "diff.context", s->context_color, + "fall back"); if (!strcmp(s->context_color, "fall back")) - init_color(r, s, "diff.plain", s->context_color, - diff_get_color(s->use_color, DIFF_CONTEXT)); - init_color(r, s, "diff.old", s->file_old_color, - diff_get_color(s->use_color, DIFF_FILE_OLD)); - init_color(r, s, "diff.new", s->file_new_color, - diff_get_color(s->use_color, DIFF_FILE_NEW)); - - strlcpy(s->reset_color, - s->use_color ? GIT_COLOR_RESET : "", COLOR_MAXLEN); + init_color(r, s->use_color_diff, "diff.plain", + s->context_color, + diff_get_color(s->use_color_diff, DIFF_CONTEXT)); + init_color(r, s->use_color_diff, "diff.old", s->file_old_color, + diff_get_color(s->use_color_diff, DIFF_FILE_OLD)); + init_color(r, s->use_color_diff, "diff.new", s->file_new_color, + diff_get_color(s->use_color_diff, DIFF_FILE_NEW)); + strlcpy(s->reset_color_diff, + s->use_color_diff ? GIT_COLOR_RESET : "", COLOR_MAXLEN); FREE_AND_NULL(s->interactive_diff_filter); repo_config_get_string(r, "interactive.difffilter", @@ -109,7 +121,8 @@ void clear_add_i_state(struct add_i_state *s) FREE_AND_NULL(s->interactive_diff_filter); FREE_AND_NULL(s->interactive_diff_algorithm); memset(s, 0, sizeof(*s)); - s->use_color = -1; + s->use_color_interactive = -1; + s->use_color_diff = -1; } /* @@ -1188,9 +1201,9 @@ int run_add_i(struct repository *r, const struct pathspec *ps, * When color was asked for, use the prompt color for * highlighting, otherwise use square brackets. */ - if (s.use_color) { + if (s.use_color_interactive) { data.color = s.prompt_color; - data.reset = s.reset_color; + data.reset = s.reset_color_interactive; } print_file_item_data.color = data.color; print_file_item_data.reset = data.reset; diff --git a/add-interactive.h b/add-interactive.h index 4213dcd67b9a8e..ceadfa6bb67812 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -12,16 +12,19 @@ struct add_p_opt { struct add_i_state { struct repository *r; - int use_color; + int use_color_interactive; + int use_color_diff; char header_color[COLOR_MAXLEN]; char help_color[COLOR_MAXLEN]; char prompt_color[COLOR_MAXLEN]; char error_color[COLOR_MAXLEN]; - char reset_color[COLOR_MAXLEN]; + char reset_color_interactive[COLOR_MAXLEN]; + char fraginfo_color[COLOR_MAXLEN]; char context_color[COLOR_MAXLEN]; char file_old_color[COLOR_MAXLEN]; char file_new_color[COLOR_MAXLEN]; + char reset_color_diff[COLOR_MAXLEN]; int use_single_key; char *interactive_diff_filter, *interactive_diff_algorithm; diff --git a/add-patch.c b/add-patch.c index 302e6ba7d9a353..b0389c5d5bad6d 100644 --- a/add-patch.c +++ b/add-patch.c @@ -300,7 +300,7 @@ static void err(struct add_p_state *s, const char *fmt, ...) va_start(args, fmt); fputs(s->s.error_color, stdout); vprintf(fmt, args); - puts(s->s.reset_color); + puts(s->s.reset_color_interactive); va_end(args); } @@ -457,7 +457,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) } strbuf_complete_line(plain); - if (want_color_fd(1, -1)) { + if (want_color_fd(1, s->s.use_color_diff)) { struct child_process colored_cp = CHILD_PROCESS_INIT; const char *diff_filter = s->s.interactive_diff_filter; @@ -714,7 +714,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk, if (len) strbuf_add(out, p, len); else if (colored) - strbuf_addf(out, "%s\n", s->s.reset_color); + strbuf_addf(out, "%s\n", s->s.reset_color_diff); else strbuf_addch(out, '\n'); } @@ -1107,7 +1107,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk) s->s.file_new_color : s->s.context_color); strbuf_add(&s->colored, plain + current, eol - current); - strbuf_addstr(&s->colored, s->s.reset_color); + strbuf_addstr(&s->colored, s->s.reset_color_diff); if (next > eol) strbuf_add(&s->colored, plain + eol, next - eol); current = next; @@ -1528,8 +1528,8 @@ static int patch_update_file(struct add_p_state *s, : 1)); printf(_(s->mode->prompt_mode[prompt_mode_type]), s->buf.buf); - if (*s->s.reset_color) - fputs(s->s.reset_color, stdout); + if (*s->s.reset_color_interactive) + fputs(s->s.reset_color_interactive, stdout); fflush(stdout); if (read_single_character(s) == EOF) break; diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 04d2a198352531..6b400ad9a3c6bc 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -866,6 +866,44 @@ test_expect_success 'colorized diffs respect diff.wsErrorHighlight' ' test_grep "old<" output ' +test_expect_success 'diff color respects color.diff' ' + git reset --hard && + + echo old >test && + git add test && + echo new >test && + + printf n >n && + force_color git \ + -c color.interactive=auto \ + -c color.interactive.prompt=blue \ + -c color.diff=false \ + -c color.diff.old=red \ + add -p >output.raw 2>&1 output && + test_grep "BLUE.*Stage this hunk" output && + test_grep ! "RED" output +' + +test_expect_success 're-coloring diff without color.interactive' ' + git reset --hard && + + test_write_lines 1 2 3 >test && + git add test && + test_write_lines one 2 three >test && + + test_write_lines s n n | + force_color git \ + -c color.interactive=false \ + -c color.interactive.prompt=blue \ + -c color.diff=true \ + -c color.diff.frag="bold magenta" \ + add -p >output.raw 2>&1 && + test_decode_color output && + test_grep "@@" output && + test_grep ! "BLUE" output +' + test_expect_success 'diffFilter filters diff' ' git reset --hard && From 776d6fbd45cfcb0a1287dc2a03c391164fbf6453 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 8 Sep 2025 12:42:39 -0400 Subject: [PATCH 10/14] add-interactive: manually fall back color config to color.ui Color options like color.interactive and color.diff should fall back to the value of color.ui if they aren't set. In add-interactive, we check the specific options (e.g., color.diff) via repo_config_get_value(), which does not depend on the main command having loaded any color config via the git_config() callback mechanism. But then we call want_color() on the result; if our specific config is unset then that function uses the value of git_use_color_default. That variable is typically set from color.ui by the git_color_config() callback, which is called by the main command in its own git_config() callback function. This works fine for "add -p", whose add_config() callback calls into git_color_config(). But it doesn't work for other commands like "checkout -p", which is otherwise unaware of color at all. People tend not to notice because the default is "auto", and that's what they'd set color.ui to as well. But something like: git -c color.ui=false checkout -p should disable color, and it doesn't. This regression goes back to 0527ccb1b5 (add -i: default to the built-in implementation, 2021-11-30). In the perl version we got the color config from "git config --get-colorbool", which did the full lookup for us. The obvious fix is for git-checkout to add a call to git_color_config() to its own config callback. But we'd have to do so for every command with this problem, which is error-prone. Let's see if we can fix it more centrally. It is tempting to teach want_color() to look up the value of repo_config_get_value("color.ui") itself. But I think that would have disastrous consequences. Plumbing commands, especially older ones, avoid porcelain config like "color.*" by simply not parsing it in their config callbacks. Looking up the value of color.ui under the hood would undermine that. Instead, let's do that lookup in the add-interactive setup code. We're already demand-loading other color config there, which is probably fine (even in a plumbing command like "git reset", the interactive mode is inherently porcelain-ish). That catches all commands that use the interactive code, whether they were calling git_color_config() themselves or not. Reported-by: Isaac Oscar Gariano Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- add-interactive.c | 9 +++++++++ t/t3701-add-interactive.sh | 15 +++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index 877160d298556e..4604c69140d62d 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -45,6 +45,15 @@ static int check_color_config(struct repository *r, const char *var) ret = -1; else ret = git_config_colorbool(var, value); + + /* + * Do not rely on want_color() to fall back to color.ui for us. It uses + * the value parsed by git_color_config(), which may not have been + * called by the main command. + */ + if (ret < 0 && !repo_config_get_value(r, "color.ui", &value)) + ret = git_config_colorbool("color.ui", value); + return want_color(ret); } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 6b400ad9a3c6bc..d9fe289a7ad13a 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -1321,6 +1321,12 @@ test_expect_success 'stash accepts -U and --inter-hunk-context' ' test_grep "@@ -2,20 +2,20 @@" actual ' +test_expect_success 'set up base for -p color tests' ' + echo commit >file && + git commit -am "commit state" && + git tag patch-base +' + for cmd in add checkout commit reset restore "stash save" "stash push" do test_expect_success "$cmd rejects invalid context options" ' @@ -1337,6 +1343,15 @@ do test_must_fail git $cmd --inter-hunk-context 2 2>actual && test_grep -E ".--inter-hunk-context. requires .(--interactive/)?--patch." actual ' + + test_expect_success "$cmd falls back to color.ui" ' + git reset --hard patch-base && + echo working-tree >file && + test_write_lines y | + force_color git -c color.ui=false $cmd -p >output.raw 2>&1 && + test_decode_color output && + test_cmp output.raw output + ' done test_done From 1092cd6435642808c3e921f0c3c4a7588cc455e6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 8 Sep 2025 12:42:42 -0400 Subject: [PATCH 11/14] contrib/diff-highlight: mention interactive.diffFilter When the README for diff-highlight was written, there was no way to trigger it for the `add -p` interactive patch mode. We've since grown a feature to support that, but it was documented only on the Git side. Let's also let people coming the other direction, from diff-highlight, know that it's an option. Suggested-by: Isaac Oscar Gariano Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- contrib/diff-highlight/README | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README index d4c23431752063..1db4440e68324d 100644 --- a/contrib/diff-highlight/README +++ b/contrib/diff-highlight/README @@ -58,6 +58,14 @@ following in your git configuration: diff = diff-highlight | less --------------------------------------------- +If you use the interactive patch mode of `git add -p`, `git checkout +-p`, etc, you may also want to configure it to be used there: + +--------------------------------------------- +[interactive] + diffFilter = diff-highlight +--------------------------------------------- + Color Config ------------ From a66fc22bf9b7f379fc68e23c54d42ac9b7eaa845 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Wed, 10 Sep 2025 19:16:30 +0200 Subject: [PATCH 12/14] use repo_get_oid_with_flags() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit get_oid_with_context() allows specifying flags and reports object details via a passed-in struct object_context. Some callers just want to specify flags, but don't need any details back. Convert them to repo_get_oid_with_flags(), which provides just that and frees them from dealing with the context structure. Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- builtin/ls-tree.c | 7 ++----- builtin/rev-parse.c | 7 ++----- builtin/stash.c | 14 +++++--------- list-objects-filter.c | 9 +++------ object-name.c | 30 +++++------------------------- 5 files changed, 17 insertions(+), 50 deletions(-) diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 5d55731ca35b7a..ec6940fc7c4b9c 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -373,7 +373,6 @@ int cmd_ls_tree(int argc, OPT_END() }; struct ls_tree_cmdmode_to_fmt *m2f = ls_tree_cmdmode_format; - struct object_context obj_context = {0}; int ret; repo_config(the_repository, git_default_config, NULL); @@ -405,9 +404,8 @@ int cmd_ls_tree(int argc, ls_tree_usage, ls_tree_options); if (argc < 1) usage_with_options(ls_tree_usage, ls_tree_options); - if (get_oid_with_context(the_repository, argv[0], - GET_OID_HASH_ANY, &oid, - &obj_context)) + if (repo_get_oid_with_flags(the_repository, argv[0], &oid, + GET_OID_HASH_ANY)) die("Not a valid object name %s", argv[0]); /* @@ -447,6 +445,5 @@ int cmd_ls_tree(int argc, ret = !!read_tree(the_repository, tree, &options.pathspec, fn, &options); clear_pathspec(&options.pathspec); - object_context_release(&obj_context); return ret; } diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 44ff1b8342acae..9da92b990d074b 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -708,7 +708,6 @@ int cmd_rev_parse(int argc, struct object_id oid; unsigned int flags = 0; const char *name = NULL; - struct object_context unused; struct strbuf buf = STRBUF_INIT; int seen_end_of_options = 0; enum format_type format = FORMAT_DEFAULT; @@ -1141,9 +1140,8 @@ int cmd_rev_parse(int argc, name++; type = REVERSED; } - if (!get_oid_with_context(the_repository, name, - flags, &oid, &unused)) { - object_context_release(&unused); + if (!repo_get_oid_with_flags(the_repository, name, &oid, + flags)) { if (output_algo) repo_oid_to_algop(the_repository, &oid, output_algo, &oid); @@ -1153,7 +1151,6 @@ int cmd_rev_parse(int argc, show_rev(type, &oid, name); continue; } - object_context_release(&unused); if (verify) die_no_single_rev(quiet); if (has_dashdash) diff --git a/builtin/stash.c b/builtin/stash.c index 1977e50df27fc5..e9aac85c76e558 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1088,7 +1088,6 @@ static int store_stash(int argc, const char **argv, const char *prefix, int quiet = 0; const char *stash_msg = NULL; struct object_id obj; - struct object_context dummy = {0}; struct option options[] = { OPT__QUIET(&quiet, N_("be quiet")), OPT_STRING('m', "message", &stash_msg, "message", @@ -1108,9 +1107,8 @@ static int store_stash(int argc, const char **argv, const char *prefix, return -1; } - if (get_oid_with_context(the_repository, - argv[0], quiet ? GET_OID_QUIETLY : 0, &obj, - &dummy)) { + if (repo_get_oid_with_flags(the_repository, argv[0], &obj, + quiet ? GET_OID_QUIETLY : 0)) { if (!quiet) fprintf_ln(stderr, _("Cannot update %s with %s"), ref_stash, argv[0]); @@ -1121,7 +1119,6 @@ static int store_stash(int argc, const char **argv, const char *prefix, ret = do_store_stash(&obj, stash_msg, quiet); out: - object_context_release(&dummy); return ret; } @@ -2233,7 +2230,6 @@ static int do_export_stash(struct repository *r, const char **argv) { struct object_id base; - struct object_context unused; struct commit *prev; struct commit_list *items = NULL, **iter = &items, *cur; int res = 0; @@ -2267,9 +2263,9 @@ static int do_export_stash(struct repository *r, struct commit *stash; if (parse_stash_revision(&revision, argv[i], 1) || - get_oid_with_context(r, revision.buf, - GET_OID_QUIETLY | GET_OID_GENTLY, - &oid, &unused)) { + repo_get_oid_with_flags(r, revision.buf, &oid, + GET_OID_QUIETLY | + GET_OID_GENTLY)) { res = error(_("unable to find stash entry %s"), argv[i]); goto out; } diff --git a/list-objects-filter.c b/list-objects-filter.c index 7ecd4d9ef50a8e..acd65ebb734523 100644 --- a/list-objects-filter.c +++ b/list-objects-filter.c @@ -524,12 +524,11 @@ static void filter_sparse_oid__init( struct filter *filter) { struct filter_sparse_data *d = xcalloc(1, sizeof(*d)); - struct object_context oc; struct object_id sparse_oid; - if (get_oid_with_context(the_repository, - filter_options->sparse_oid_name, - GET_OID_BLOB, &sparse_oid, &oc)) + if (repo_get_oid_with_flags(the_repository, + filter_options->sparse_oid_name, + &sparse_oid, GET_OID_BLOB)) die(_("unable to access sparse blob in '%s'"), filter_options->sparse_oid_name); if (add_patterns_from_blob_to_list(&sparse_oid, "", 0, &d->pl) < 0) @@ -544,8 +543,6 @@ static void filter_sparse_oid__init( filter->filter_data = d; filter->filter_object_fn = filter_sparse; filter->free_fn = filter_sparse_free; - - object_context_release(&oc); } /* diff --git a/object-name.c b/object-name.c index 11aa0e6afc565e..a37fbde5a0ce28 100644 --- a/object-name.c +++ b/object-name.c @@ -1857,55 +1857,35 @@ int repo_get_oid_committish(struct repository *r, const char *name, struct object_id *oid) { - struct object_context unused; - int ret = get_oid_with_context(r, name, GET_OID_COMMITTISH, - oid, &unused); - object_context_release(&unused); - return ret; + return repo_get_oid_with_flags(r, name, oid, GET_OID_COMMITTISH); } int repo_get_oid_treeish(struct repository *r, const char *name, struct object_id *oid) { - struct object_context unused; - int ret = get_oid_with_context(r, name, GET_OID_TREEISH, - oid, &unused); - object_context_release(&unused); - return ret; + return repo_get_oid_with_flags(r, name, oid, GET_OID_TREEISH); } int repo_get_oid_commit(struct repository *r, const char *name, struct object_id *oid) { - struct object_context unused; - int ret = get_oid_with_context(r, name, GET_OID_COMMIT, - oid, &unused); - object_context_release(&unused); - return ret; + return repo_get_oid_with_flags(r, name, oid, GET_OID_COMMIT); } int repo_get_oid_tree(struct repository *r, const char *name, struct object_id *oid) { - struct object_context unused; - int ret = get_oid_with_context(r, name, GET_OID_TREE, - oid, &unused); - object_context_release(&unused); - return ret; + return repo_get_oid_with_flags(r, name, oid, GET_OID_TREE); } int repo_get_oid_blob(struct repository *r, const char *name, struct object_id *oid) { - struct object_context unused; - int ret = get_oid_with_context(r, name, GET_OID_BLOB, - oid, &unused); - object_context_release(&unused); - return ret; + return repo_get_oid_with_flags(r, name, oid, GET_OID_BLOB); } /* Must be called only when object_name:filename doesn't exist. */ From 83f9dad7d6fb5988b68f80b25bd87c68693195dd Mon Sep 17 00:00:00 2001 From: Colin Stagner Date: Tue, 9 Sep 2025 22:11:24 -0500 Subject: [PATCH 13/14] contrib/subtree: fix split with squashed subtrees 98ba49ccc2 (subtree: fix split processing with multiple subtrees present, 2023-12-01) increases the performance of git subtree split --prefix=subA by ignoring subtree merges which are outside of `subA/`. It also introduces a regression. Subtree merges that should be retained are incorrectly ignored if they: 1. are nested under `subA/`; and 2. are merged with `--squash`. For example, a subtree merged like: git subtree merge --squash --prefix=subA/subB "$rev" # ^^^^^^^^ ^^^^ is erroneously ignored during a split of `subA`. This causes missing tree files and different commit hashes starting in git v2.44.0-rc0. The method: should_ignore_subtree_split_commit REV should test only a single commit REV, but the combination of git log -1 --grep=... actually searches all *parent* commits until a `--grep` match is discovered. Rewrite this method to test only one REV at a time. Extract commit information with a single `git` call as opposed to three. The `test` conditions for rejecting a commit remain unchanged. Unit tests now cover nested subtrees. Signed-off-by: Colin Stagner Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- contrib/subtree/git-subtree.sh | 36 +++++++++++---- contrib/subtree/t/t7900-subtree.sh | 71 ++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 8 deletions(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 3fddba797cb92c..17106d1a721519 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -785,20 +785,40 @@ ensure_valid_ref_format () { die "fatal: '$1' does not look like a ref" } -# Usage: check if a commit from another subtree should be +# Usage: should_ignore_subtree_split_commit REV +# +# Check if REV is a commit from another subtree and should be # ignored from processing for splits should_ignore_subtree_split_commit () { assert test $# = 1 - local rev="$1" - if test -n "$(git log -1 --grep="git-subtree-dir:" $rev)" + + git show \ + --no-patch \ + --no-show-signature \ + --format='%(trailers:key=git-subtree-dir,key=git-subtree-mainline)' \ + "$1" | + ( + have_mainline= + subtree_dir= + + while read -r trailer val + do + case "$trailer" in + git-subtree-dir:) + subtree_dir="${val%/}" ;; + git-subtree-mainline:) + have_mainline=y ;; + esac + done + + if test -n "${subtree_dir}" && + test -z "${have_mainline}" && + test "${subtree_dir}" != "$arg_prefix" then - if test -z "$(git log -1 --grep="git-subtree-mainline:" $rev)" && - test -z "$(git log -1 --grep="git-subtree-dir: $arg_prefix$" $rev)" - then - return 0 - fi + return 0 fi return 1 + ) } # Usage: process_split_commit REV PARENTS diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh index 3edbb33af46971..316dc5269e2b6f 100755 --- a/contrib/subtree/t/t7900-subtree.sh +++ b/contrib/subtree/t/t7900-subtree.sh @@ -9,6 +9,9 @@ This test verifies the basic operation of the add, merge, split, pull, and push subcommands of git subtree. ' +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + TEST_DIRECTORY=$(pwd)/../../../t . "$TEST_DIRECTORY"/test-lib.sh . "$TEST_DIRECTORY"/lib-gpg.sh @@ -68,6 +71,33 @@ test_create_pre2_32_repo () { git -C "$1-clone" replace HEAD^2 $new_commit } +# test_create_subtree_add REPO ORPHAN PREFIX FILENAME ... +# +# Create a simple subtree on a new branch named ORPHAN in REPO. +# The subtree is then merged into the current branch of REPO, +# under PREFIX. The generated subtree has has one commit +# with subject and tag FILENAME with a single file "FILENAME.t" +# +# When this method returns: +# - the current branch of REPO will have file PREFIX/FILENAME.t +# - REPO will have a branch named ORPHAN with subtree history +# +# additional arguments are forwarded to "subtree add" +test_create_subtree_add () { + ( + cd "$1" && + orphan="$2" && + prefix="$3" && + filename="$4" && + shift 4 && + last="$(git branch --show-current)" && + git switch --orphan "$orphan" && + test_commit "$filename" && + git checkout "$last" && + git subtree add --prefix="$prefix" "$@" "$orphan" + ) +} + test_expect_success 'shows short help text for -h' ' test_expect_code 129 git subtree -h >out 2>err && test_must_be_empty err && @@ -426,6 +456,47 @@ test_expect_success 'split with multiple subtrees' ' --squash --rejoin -d -m "Sub B Split 1" 2>&1 | grep -w "\[1\]")" = "" ' +# When subtree split-ing a directory that has other subtree +# *merges* underneath it, the split must include those subtrees. +# This test creates a nested subtree, `subA/subB`, and tests +# that the tree is correct after a subtree split of `subA/`. +# The test covers: +# - An initial `subtree add`; and +# - A follow-up `subtree merge` +# both with and without `--squashed`. +for is_squashed in '' 'y' +do + test_expect_success "split keeps nested ${is_squashed:+--squash }subtrees that are part of the split" ' + subtree_test_create_repo "$test_count" && + ( + cd "$test_count" && + mkdir subA && + test_commit subA/file1 && + test_create_subtree_add \ + . mksubtree subA/subB file2 ${is_squashed:+--squash} && + test_path_is_file subA/file1.t && + test_path_is_file subA/subB/file2.t && + git subtree split --prefix=subA --branch=bsplit && + git checkout bsplit && + test_path_is_file file1.t && + test_path_is_file subB/file2.t && + git checkout mksubtree && + git branch -D bsplit && + test_commit file3 && + git checkout main && + git subtree merge \ + ${is_squashed:+--squash} \ + --prefix=subA/subB mksubtree && + test_path_is_file subA/subB/file3.t && + git subtree split --prefix=subA --branch=bsplit && + git checkout bsplit && + test_path_is_file file1.t && + test_path_is_file subB/file2.t && + test_path_is_file subB/file3.t + ) + ' +done + test_expect_success 'split sub dir/ with --rejoin from scratch' ' subtree_test_create_repo "$test_count" && test_create_commit "$test_count" main1 && From bb69721404348ea2db0a081c41ab6ebfe75bdec8 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 23 Sep 2025 11:53:31 -0700 Subject: [PATCH 14/14] The twelfth batch Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.52.0.adoc | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Documentation/RelNotes/2.52.0.adoc b/Documentation/RelNotes/2.52.0.adoc index eae371f239a1fd..c4fc56163135d3 100644 --- a/Documentation/RelNotes/2.52.0.adoc +++ b/Documentation/RelNotes/2.52.0.adoc @@ -31,6 +31,10 @@ UI, Workflows & Features * "git send-email" learned to drive "git imap-send" to store already sent e-mails in an IMAP folder. + * The "promisor-remote" capability mechanism has been updated to + allow the "partialCloneFilter" settings and the "token" value to be + communicated from the server side. + Performance, Internal Implementation, Development Support etc. -------------------------------------------------------------- @@ -205,6 +209,14 @@ including security updates, are included in this release. characters, which has been fixed. (merge 8655908b9e jc/longer-disambiguation-fix later to maint). + * Some among "git add -p" and friends ignored color.diff and/or + color.ui configuration variables, which is an old regression, which + has been corrected. + (merge 1092cd6435 jk/add-i-color later to maint). + + * "git subtree" (in contrib/) did not work correctly when splitting + squashed subtrees, which has been improved. + * Other code cleanup, docfix, build fix, etc. (merge 823d537fa7 kh/doc-git-log-markup-fix later to maint). (merge cf7efa4f33 rj/t6137-cygwin-fix later to maint). @@ -229,3 +241,4 @@ including security updates, are included in this release. (merge 31397bc4f7 kh/doc-fast-import-markup-fix later to maint). (merge ac7096723b jc/doc-includeif-hasconfig-remote-url-fix later to maint). (merge fafc9b08b8 ag/doc-sendmail-gmail-example-update later to maint). + (merge a66fc22bf9 rs/get-oid-with-flags-cleanup later to maint).