* [PATCH v3 5/7] fetch: add --negotiation-include option for negotiation
From: Derrick Stolee via GitGitGadget @ 2026-04-22 15:25 UTC (permalink / raw)
To: git; +Cc: gitster, ps, Derrick Stolee, Derrick Stolee
In-Reply-To: <pull.2085.v3.git.1776871546.gitgitgadget@gmail.com>
From: Derrick Stolee <stolee@gmail.com>
Add a new --negotiation-include option to 'git fetch', which ensures
that certain ref tips are always sent as 'have' lines during fetch
negotiation, regardless of what the negotiation algorithm selects.
This is useful when the repository has a large number of references, so
the normal negotiation algorithm truncates the list. This is especially
important in repositories with long parallel commit histories. For
example, a repo could have a 'dev' branch for development and a
'release' branch for released versions. If the 'dev' branch isn't
selected for negotiation, then it's not a big deal because there are
many in-progress development branches with a shared history. However, if
'release' is not selected for negotiation, then the server may think
that this is the first time the client has asked for that reference,
causing a full download of its parallel commit history (and any extra
data that may be unique to that branch). This is based on a real example
where certain fetches would grow to 60+ GB when a release branch
updated.
This option is a complement to --negotiation-restrict, which reduces the
negotiation ref set to a specific list. In the earlier example, using
--negotiation-restrict to focus the negotiation to 'dev' and 'release'
would avoid those problematic downloads, but would still not allow
advertising potentially-relevant user brances. In this way, the
'include' version solves the problem I mention while allowing
negotiation to pick other references opportunistically. The two options
can also be combined to allow the best of both worlds.
The argument may be an exact ref name or a glob pattern. Non-existent
refs are silently ignored. This behavior is also updated in the ref matching
logic for the related --negotiation-restrict option to match.
The implementation outputs the requested objects as haves before the
negotiation algorithm kicks in and performs a priority-queue walk from the
tip commits. In order to avoid duplicates, we mark the requested objects as
COMMON so they (and their descendants) are not output by the negotiator. The
negotiator still outputs at least one have before a round is flushed, when
the server could ACK to stop the negotiation.
Also add --negotiation-include to 'git pull' passthrough options.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/fetch-options.adoc | 19 ++++++
builtin/fetch.c | 16 ++++-
builtin/pull.c | 3 +
fetch-pack.c | 112 +++++++++++++++++++++++++++++--
fetch-pack.h | 10 ++-
t/t5510-fetch.sh | 66 ++++++++++++++++++
transport.c | 4 +-
transport.h | 6 ++
8 files changed, 227 insertions(+), 9 deletions(-)
diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
index c07b85499f..decc7f6abd 100644
--- a/Documentation/fetch-options.adoc
+++ b/Documentation/fetch-options.adoc
@@ -73,6 +73,25 @@ See also the `fetch.negotiationAlgorithm` and `push.negotiate`
configuration variables documented in linkgit:git-config[1], and the
`--negotiate-only` option below.
+`--negotiation-include=<revision>`::
+ Ensure that the given ref tip is always sent as a "have" line
+ during fetch negotiation, regardless of what the negotiation
+ algorithm selects. This is useful to guarantee that common
+ history reachable from specific refs is always considered, even
+ when `--negotiation-restrict` restricts the set of tips or when
+ the negotiation algorithm would otherwise skip them.
++
+This option may be specified more than once; if so, each ref is sent
+unconditionally.
++
+The argument may be an exact ref name (e.g. `refs/heads/release`) or a
+glob pattern (e.g. `refs/heads/release/{asterisk}`). The pattern syntax
+is the same as for `--negotiation-restrict`.
++
+If `--negotiation-restrict` is used, the have set is first restricted by
+that option and then increased to include the tips specified by
+`--negotiation-include`.
+
`--negotiate-only`::
Do not fetch anything from the server, and instead print the
ancestors of the provided `--negotiation-tip=` arguments,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a1960e3e0c..ef50e2fbe9 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -99,6 +99,7 @@ static struct transport *gsecondary;
static struct refspec refmap = REFSPEC_INIT_FETCH;
static struct string_list server_options = STRING_LIST_INIT_DUP;
static struct string_list negotiation_restrict = STRING_LIST_INIT_NODUP;
+static struct string_list negotiation_include = STRING_LIST_INIT_NODUP;
struct fetch_config {
enum display_format display_format;
@@ -1547,10 +1548,14 @@ static void add_negotiation_restrict_tips(struct git_transport_options *smart_op
int old_nr;
if (!has_glob_specials(s)) {
struct object_id oid;
+
+ /* Ignore missing reference. */
if (repo_get_oid(the_repository, s, &oid))
- die(_("%s is not a valid object"), s);
+ continue;
+ /* Fail on missing object pointed by ref. */
if (!odb_has_object(the_repository->objects, &oid, 0))
die(_("the object %s does not exist"), s);
+
oid_array_append(oids, &oid);
continue;
}
@@ -1615,6 +1620,13 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
strbuf_release(&config_name);
}
}
+ if (negotiation_include.nr) {
+ if (transport->smart_options)
+ transport->smart_options->negotiation_include = &negotiation_include;
+ else
+ warning(_("ignoring %s because the protocol does not support it"),
+ "--negotiation-include");
+ }
return transport;
}
@@ -2582,6 +2594,8 @@ int cmd_fetch(int argc,
OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_restrict, N_("revision"),
N_("report that we have only objects reachable from this object")),
OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
+ OPT_STRING_LIST(0, "negotiation-include", &negotiation_include, N_("revision"),
+ N_("ensure this ref is always sent as a negotiation have")),
OPT_BOOL(0, "negotiate-only", &negotiate_only,
N_("do not fetch a packfile; instead, print ancestors of negotiation tips")),
OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
diff --git a/builtin/pull.c b/builtin/pull.c
index 821cc6699a..86c85b60ef 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1002,6 +1002,9 @@ int cmd_pull(int argc,
OPT_PASSTHRU_ARGV(0, "negotiation-restrict", &opt_fetch, N_("revision"),
N_("report that we have only objects reachable from this object"),
0),
+ OPT_PASSTHRU_ARGV(0, "negotiation-include", &opt_fetch, N_("revision"),
+ N_("ensure this ref is always sent as a negotiation have"),
+ 0),
OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
N_("check for forced-updates on all updated branches")),
OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
diff --git a/fetch-pack.c b/fetch-pack.c
index baf239adf9..8b080b0080 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -25,6 +25,7 @@
#include "oidset.h"
#include "packfile.h"
#include "odb.h"
+#include "object-name.h"
#include "path.h"
#include "connected.h"
#include "fetch-negotiator.h"
@@ -332,6 +333,48 @@ static void send_filter(struct fetch_pack_args *args,
}
}
+static int add_oid_to_oidset(const struct reference *ref, void *cb_data)
+{
+ struct oidset *set = cb_data;
+ if (!odb_has_object(the_repository->objects, ref->oid, 0))
+ die(_("the object %s does not exist"), oid_to_hex(ref->oid));
+ oidset_insert(set, ref->oid);
+ return 0;
+}
+
+static void resolve_negotiation_include(const struct string_list *negotiation_include,
+ struct oidset *result)
+{
+ struct string_list_item *item;
+
+ if (!negotiation_include || !negotiation_include->nr)
+ return;
+
+ for_each_string_list_item(item, negotiation_include) {
+ if (!has_glob_specials(item->string)) {
+ struct object_id oid;
+
+ /* Ignore missing reference. */
+ if (repo_get_oid(the_repository, item->string, &oid))
+ continue;
+
+ /* Fail on missing object pointed by ref. */
+ if (!odb_has_object(the_repository->objects, &oid, 0))
+ die(_("the object %s does not exist"),
+ item->string);
+
+ oidset_insert(result, &oid);
+ } else {
+ struct refs_for_each_ref_options opts = {
+ .pattern = item->string,
+ };
+ refs_for_each_ref_ext(
+ get_main_ref_store(the_repository),
+ add_oid_to_oidset, result, &opts);
+ }
+ }
+}
+
static int find_common(struct fetch_negotiator *negotiator,
struct fetch_pack_args *args,
int fd[2], struct object_id *result_oid,
@@ -347,6 +390,7 @@ static int find_common(struct fetch_negotiator *negotiator,
struct strbuf req_buf = STRBUF_INIT;
size_t state_len = 0;
struct packet_reader reader;
+ struct oidset negotiation_include_oids = OIDSET_INIT;
if (args->stateless_rpc && multi_ack == 1)
die(_("the option '%s' requires '%s'"), "--stateless-rpc", "multi_ack_detailed");
@@ -474,6 +518,33 @@ static int find_common(struct fetch_negotiator *negotiator,
trace2_region_enter("fetch-pack", "negotiation_v0_v1", the_repository);
flushes = 0;
retval = -1;
+
+ /* Send unconditional haves from --negotiation-include */
+ resolve_negotiation_include(args->negotiation_include,
+ &negotiation_include_oids);
+ if (oidset_size(&negotiation_include_oids)) {
+ struct oidset_iter iter;
+ oidset_iter_init(&negotiation_include_oids, &iter);
+
+ while ((oid = oidset_iter_next(&iter))) {
+ struct commit *commit;
+ packet_buf_write(&req_buf, "have %s\n",
+ oid_to_hex(oid));
+ print_verbose(args, "have %s", oid_to_hex(oid));
+ count++;
+
+ /*
+ * If this is a commit, then mark as COMMON to
+ * avoid the negotiator also outputting it as
+ * a have.
+ */
+ commit = lookup_commit(the_repository, oid);
+ if (commit &&
+ !repo_parse_commit(the_repository, commit))
+ commit->object.flags |= COMMON;
+ }
+ }
+
while ((oid = negotiator->next(negotiator))) {
packet_buf_write(&req_buf, "have %s\n", oid_to_hex(oid));
print_verbose(args, "have %s", oid_to_hex(oid));
@@ -584,6 +655,7 @@ done:
flushes++;
}
strbuf_release(&req_buf);
+ oidset_clear(&negotiation_include_oids);
if (!got_ready || !no_done)
consume_shallow_list(args, &reader);
@@ -1305,12 +1377,26 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)
static int add_haves(struct fetch_negotiator *negotiator,
struct strbuf *req_buf,
- int *haves_to_send)
+ int *haves_to_send,
+ struct oidset *negotiation_include_oids)
{
int haves_added = 0;
const struct object_id *oid;
+ /* Send unconditional haves from --negotiation-include */
+ if (negotiation_include_oids) {
+ struct oidset_iter iter;
+ oidset_iter_init(negotiation_include_oids, &iter);
+
+ while ((oid = oidset_iter_next(&iter)))
+ packet_buf_write(req_buf, "have %s\n",
+ oid_to_hex(oid));
+ }
+
while ((oid = negotiator->next(negotiator))) {
+ if (negotiation_include_oids &&
+ oidset_contains(negotiation_include_oids, oid))
+ continue;
packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
if (++haves_added >= *haves_to_send)
break;
@@ -1358,7 +1444,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
struct fetch_pack_args *args,
const struct ref *wants, struct oidset *common,
int *haves_to_send, int *in_vain,
- int sideband_all, int seen_ack)
+ int sideband_all, int seen_ack,
+ struct oidset *negotiation_include_oids)
{
int haves_added;
int done_sent = 0;
@@ -1413,7 +1500,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
/* Add all of the common commits we've found in previous rounds */
add_common(&req_buf, common);
- haves_added = add_haves(negotiator, &req_buf, haves_to_send);
+ haves_added = add_haves(negotiator, &req_buf, haves_to_send,
+ negotiation_include_oids);
*in_vain += haves_added;
trace2_data_intmax("negotiation_v2", the_repository, "haves_added", haves_added);
trace2_data_intmax("negotiation_v2", the_repository, "in_vain", *in_vain);
@@ -1657,6 +1745,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
struct ref *ref = copy_ref_list(orig_ref);
enum fetch_state state = FETCH_CHECK_LOCAL;
struct oidset common = OIDSET_INIT;
+ struct oidset negotiation_include_oids = OIDSET_INIT;
struct packet_reader reader;
int in_vain = 0, negotiation_started = 0;
int negotiation_round = 0;
@@ -1729,6 +1818,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
state = FETCH_SEND_REQUEST;
mark_tips(negotiator, args->negotiation_restrict_tips);
+ resolve_negotiation_include(args->negotiation_include,
+ &negotiation_include_oids);
for_each_cached_alternate(negotiator,
insert_one_alternate_object);
break;
@@ -1747,7 +1838,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
&common,
&haves_to_send, &in_vain,
reader.use_sideband,
- seen_ack)) {
+ seen_ack,
+ &negotiation_include_oids)) {
trace2_region_leave_printf("negotiation_v2", "round",
the_repository, "%d",
negotiation_round);
@@ -1883,6 +1975,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
negotiator->release(negotiator);
oidset_clear(&common);
+ oidset_clear(&negotiation_include_oids);
return ref;
}
@@ -2181,12 +2274,14 @@ void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
const struct string_list *server_options,
int stateless_rpc,
int fd[],
- struct oidset *acked_commits)
+ struct oidset *acked_commits,
+ const struct string_list *negotiation_include)
{
struct fetch_negotiator negotiator;
struct packet_reader reader;
struct object_array nt_object_array = OBJECT_ARRAY_INIT;
struct strbuf req_buf = STRBUF_INIT;
+ struct oidset negotiation_include_oids = OIDSET_INIT;
int haves_to_send = INITIAL_FLUSH;
int in_vain = 0;
int seen_ack = 0;
@@ -2197,6 +2292,9 @@ void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
fetch_negotiator_init(the_repository, &negotiator);
mark_tips(&negotiator, negotiation_restrict_tips);
+ resolve_negotiation_include(negotiation_include,
+ &negotiation_include_oids);
+
packet_reader_init(&reader, fd[0], NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_DIE_ON_ERR_PACKET);
@@ -2221,7 +2319,8 @@ void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
packet_buf_write(&req_buf, "wait-for-done");
- haves_added = add_haves(&negotiator, &req_buf, &haves_to_send);
+ haves_added = add_haves(&negotiator, &req_buf, &haves_to_send,
+ &negotiation_include_oids);
in_vain += haves_added;
if (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN))
last_iteration = 1;
@@ -2273,6 +2372,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
clear_common_flag(acked_commits);
object_array_clear(&nt_object_array);
+ oidset_clear(&negotiation_include_oids);
negotiator.release(&negotiator);
strbuf_release(&req_buf);
}
diff --git a/fetch-pack.h b/fetch-pack.h
index 6c70c942c2..32ae94d0b4 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -23,6 +23,13 @@ struct fetch_pack_args {
*/
const struct oid_array *negotiation_restrict_tips;
+ /*
+ * If non-empty, ref patterns whose tips should always be sent
+ * as "have" lines during negotiation, regardless of what the
+ * negotiation algorithm selects.
+ */
+ const struct string_list *negotiation_include;
+
unsigned deepen_relative:1;
unsigned quiet:1;
unsigned keep_pack:1;
@@ -93,7 +100,8 @@ void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
const struct string_list *server_options,
int stateless_rpc,
int fd[],
- struct oidset *acked_commits);
+ struct oidset *acked_commits,
+ const struct string_list *negotiation_include);
/*
* Print an appropriate error message for each sought ref that wasn't
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index eff3ce8e2d..4316f8d4ea 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1511,6 +1511,72 @@ test_expect_success 'CLI --negotiation-restrict overrides remote config' '
test_grep ! "fetch> have $BETA_1" trace
'
+test_expect_success '--negotiation-include includes configured refs as haves' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
+ --negotiation-include=refs/tags/beta_1 \
+ origin alpha_s beta_s &&
+
+ ALPHA_1=$(git -C client rev-parse alpha_1) &&
+ test_grep "fetch> have $ALPHA_1" trace &&
+ BETA_1=$(git -C client rev-parse beta_1) &&
+ test_grep "fetch> have $BETA_1" trace
+'
+
+test_expect_success '--negotiation-include works with glob patterns' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
+ --negotiation-include="refs/tags/beta_*" \
+ origin alpha_s beta_s &&
+
+ BETA_1=$(git -C client rev-parse beta_1) &&
+ test_grep "fetch> have $BETA_1" trace &&
+ BETA_2=$(git -C client rev-parse beta_2) &&
+ test_grep "fetch> have $BETA_2" trace
+'
+
+test_expect_success '--negotiation-include is additive with negotiation' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-include=refs/tags/beta_1 \
+ origin alpha_s beta_s &&
+
+ BETA_1=$(git -C client rev-parse beta_1) &&
+ test_grep "fetch> have $BETA_1" trace
+'
+
+test_expect_success '--negotiation-include ignores non-existent refs silently' '
+ setup_negotiation_tip server server 0 &&
+
+ git -C client fetch --quiet \
+ --negotiation-restrict=alpha_1 \
+ --negotiation-include=refs/tags/nonexistent \
+ origin alpha_s beta_s 2>err &&
+ test_must_be_empty err
+'
+
+test_expect_success '--negotiation-include avoids duplicates with negotiator' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
+ ALPHA_1=$(git -C client rev-parse alpha_1) &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
+ --negotiation-include=refs/tags/alpha_1 \
+ origin alpha_s beta_s &&
+
+ test_grep "fetch> have $ALPHA_1" trace >matches &&
+ test_line_count = 1 matches
+'
+
test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' '
git init df-conflict &&
(
diff --git a/transport.c b/transport.c
index a3051f6733..8a2d8adffc 100644
--- a/transport.c
+++ b/transport.c
@@ -464,6 +464,7 @@ static int fetch_refs_via_pack(struct transport *transport,
args.stateless_rpc = transport->stateless_rpc;
args.server_options = transport->server_options;
args.negotiation_restrict_tips = data->options.negotiation_restrict_tips;
+ args.negotiation_include = data->options.negotiation_include;
args.reject_shallow_remote = transport->smart_options->reject_shallow;
if (!data->finished_handshake) {
@@ -495,7 +496,8 @@ static int fetch_refs_via_pack(struct transport *transport,
transport->server_options,
transport->stateless_rpc,
data->fd,
- data->options.acked_commits);
+ data->options.acked_commits,
+ data->options.negotiation_include);
ret = 0;
}
goto cleanup;
diff --git a/transport.h b/transport.h
index cdeb33c16f..6092775a27 100644
--- a/transport.h
+++ b/transport.h
@@ -48,6 +48,12 @@ struct git_transport_options {
*/
struct oid_array *negotiation_restrict_tips;
+ /*
+ * If non-empty, ref patterns whose tips should always be sent
+ * as "have" lines during negotiation.
+ */
+ const struct string_list *negotiation_include;
+
/*
* If allocated, whenever transport_fetch_refs() is called, add known
* common commits to this oidset instead of fetching any packfiles.
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 4/7] remote: add remote.*.negotiationRestrict config
From: Derrick Stolee via GitGitGadget @ 2026-04-22 15:25 UTC (permalink / raw)
To: git; +Cc: gitster, ps, Derrick Stolee, Derrick Stolee
In-Reply-To: <pull.2085.v3.git.1776871546.gitgitgadget@gmail.com>
From: Derrick Stolee <stolee@gmail.com>
In a previous change, the --negotiation-restrict command-line option of
'git fetch' was added as a synonym of --negotiation-tips. Both of these
options restrict the set of 'haves' the client can send as part of
negotiation.
This was previously not available via a configuration option. Add a new
'remote.<name>.negotiationRestrict' multi-valued config option that
updates 'git fetch <name>' to use these restrictions by default.
If the user provides even one --negotiation-restrict argument, then the
config is ignored.
An empty value resets the value list to allow ignoring earlier config
values, such as those that might be set in system or global config.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/config/remote.adoc | 19 +++++++++++++++++++
builtin/fetch.c | 21 +++++++++++++++++----
remote.c | 8 ++++++++
remote.h | 1 +
t/t5510-fetch.sh | 26 ++++++++++++++++++++++++++
5 files changed, 71 insertions(+), 4 deletions(-)
diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
index 91e46f66f5..f1d889d03e 100644
--- a/Documentation/config/remote.adoc
+++ b/Documentation/config/remote.adoc
@@ -107,6 +107,25 @@ priority configuration file (e.g. `.git/config` in a repository) to clear
the values inherited from a lower priority configuration files (e.g.
`$HOME/.gitconfig`).
+remote.<name>.negotiationRestrict::
+ When negotiating with this remote during `git fetch` and `git push`,
+ restrict the commits advertised as "have" lines to only those
+ reachable from refs matching the given patterns. This multi-valued
+ config option behaves like `--negotiation-restrict` on the command
+ line.
++
+Each value is either an exact ref name (e.g. `refs/heads/release`) or a
+glob pattern (e.g. `refs/heads/release/*`). The pattern syntax is the
+same as for `--negotiation-restrict`.
++
+These config values are used as defaults for the `--negotiation-restrict`
+command-line option. If `--negotiation-restrict` (or its synonym
+`--negotiation-tip`) is specified on the command line, then the config
+values are not used.
++
+Blank values signal to ignore all previous values, allowing a reset of
+the list from broader config scenarios.
+
remote.<name>.followRemoteHEAD::
How linkgit:git-fetch[1] should handle updates to `remotes/<name>/HEAD`
when fetching using the configured refspecs of a remote.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 2ba0051d52..a1960e3e0c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1601,6 +1601,19 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
else
warning(_("ignoring %s because the protocol does not support it"),
"--negotiation-restrict");
+ } else if (remote->negotiation_restrict.nr) {
+ struct string_list_item *item;
+ for_each_string_list_item(item, &remote->negotiation_restrict)
+ string_list_append(&negotiation_restrict, item->string);
+ if (transport->smart_options)
+ add_negotiation_restrict_tips(transport->smart_options);
+ else {
+ struct strbuf config_name = STRBUF_INIT;
+ strbuf_addf(&config_name, "remote.%s.negotiationRestrict", remote->name);
+ warning(_("ignoring %s because the protocol does not support it"),
+ config_name.buf);
+ strbuf_release(&config_name);
+ }
}
return transport;
}
@@ -2658,10 +2671,6 @@ int cmd_fetch(int argc,
config.display_format = DISPLAY_FORMAT_PORCELAIN;
}
- if (negotiate_only && !negotiation_restrict.nr)
- die(_("%s needs one or more %s"), "--negotiate-only",
- "--negotiation-restrict=*");
-
if (deepen_relative) {
if (deepen_relative < 0)
die(_("negative depth in --deepen is not supported"));
@@ -2749,6 +2758,10 @@ int cmd_fetch(int argc,
if (!remote)
die(_("must supply remote when using --negotiate-only"));
gtransport = prepare_transport(remote, 1, &filter_options);
+ if (!gtransport->smart_options ||
+ !gtransport->smart_options->negotiation_restrict_tips)
+ die(_("%s needs one or more %s"), "--negotiate-only",
+ "--negotiation-restrict=*");
if (gtransport->smart_options) {
gtransport->smart_options->acked_commits = &acked_commits;
} else {
diff --git a/remote.c b/remote.c
index 7ca2a6501b..166a56408a 100644
--- a/remote.c
+++ b/remote.c
@@ -152,6 +152,7 @@ static struct remote *make_remote(struct remote_state *remote_state,
refspec_init_push(&ret->push);
refspec_init_fetch(&ret->fetch);
string_list_init_dup(&ret->server_options);
+ string_list_init_dup(&ret->negotiation_restrict);
ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1,
remote_state->remotes_alloc);
@@ -179,6 +180,7 @@ static void remote_clear(struct remote *remote)
FREE_AND_NULL(remote->http_proxy);
FREE_AND_NULL(remote->http_proxy_authmethod);
string_list_clear(&remote->server_options, 0);
+ string_list_clear(&remote->negotiation_restrict, 0);
}
static void add_merge(struct branch *branch, const char *name)
@@ -562,6 +564,12 @@ static int handle_config(const char *key, const char *value,
} else if (!strcmp(subkey, "serveroption")) {
return parse_transport_option(key, value,
&remote->server_options);
+ } else if (!strcmp(subkey, "negotiationrestrict")) {
+ /* reset list on empty value. */
+ if (!value || !*value)
+ string_list_clear(&remote->negotiation_restrict, 0);
+ else
+ string_list_append(&remote->negotiation_restrict, value);
} else if (!strcmp(subkey, "followremotehead")) {
const char *no_warn_branch;
if (!strcmp(value, "never"))
diff --git a/remote.h b/remote.h
index fc052945ee..e6ec37c393 100644
--- a/remote.h
+++ b/remote.h
@@ -117,6 +117,7 @@ struct remote {
char *http_proxy_authmethod;
struct string_list server_options;
+ struct string_list negotiation_restrict;
enum follow_remote_head_settings follow_remote_head;
const char *no_warn_branch;
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index dc3ce56d84..eff3ce8e2d 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1485,6 +1485,32 @@ test_expect_success '--negotiation-restrict and --negotiation-tip can be mixed'
check_negotiation_tip
'
+test_expect_success 'remote.<name>.negotiationRestrict used as default' '
+ setup_negotiation_tip server server 0 &&
+
+ # test the reset of the list on an empty value
+ git -C client config --add remote.origin.negotiationRestrict alpha_2 &&
+ git -C client config --add remote.origin.negotiationRestrict "" &&
+ git -C client config --add remote.origin.negotiationRestrict alpha_1 &&
+ git -C client config --add remote.origin.negotiationRestrict beta_1 &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ origin alpha_s beta_s &&
+ check_negotiation_tip
+'
+
+test_expect_success 'CLI --negotiation-restrict overrides remote config' '
+ setup_negotiation_tip server server 0 &&
+ git -C client config --add remote.origin.negotiationRestrict alpha_1 &&
+ git -C client config --add remote.origin.negotiationRestrict beta_1 &&
+ ALPHA_1=$(git -C client rev-parse alpha_1) &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
+ origin alpha_s beta_s &&
+ test_grep "fetch> have $ALPHA_1" trace &&
+ BETA_1=$(git -C client rev-parse beta_1) &&
+ test_grep ! "fetch> have $BETA_1" trace
+'
+
test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' '
git init df-conflict &&
(
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 3/7] transport: rename negotiation_tips
From: Derrick Stolee via GitGitGadget @ 2026-04-22 15:25 UTC (permalink / raw)
To: git; +Cc: gitster, ps, Derrick Stolee, Derrick Stolee
In-Reply-To: <pull.2085.v3.git.1776871546.gitgitgadget@gmail.com>
From: Derrick Stolee <stolee@gmail.com>
The previous change added the --negotiation-restrict synonym for the
--negotiation-tips option for 'git fetch'. In anticipation of adding a
new option that behaves similarly but with distinct changes to its
behavior, rename the internal representation of this data from
'negotiation_tips' to 'negotiation_restrict_tips'.
The 'tips' part is kept because this is an oid_array in the transport
layer. This requires the builtin to handle parsing refs into collections
of oids so the transport layer can handle this cleaner form of the data.
Also update the string_list used to store the inputs from command-line
options.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/fetch.c | 18 +++++++++---------
fetch-pack.c | 18 +++++++++---------
fetch-pack.h | 4 ++--
transport-helper.c | 2 +-
transport.c | 10 +++++-----
transport.h | 4 ++--
6 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index fc950fe35b..2ba0051d52 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -98,7 +98,7 @@ static struct transport *gtransport;
static struct transport *gsecondary;
static struct refspec refmap = REFSPEC_INIT_FETCH;
static struct string_list server_options = STRING_LIST_INIT_DUP;
-static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
+static struct string_list negotiation_restrict = STRING_LIST_INIT_NODUP;
struct fetch_config {
enum display_format display_format;
@@ -1534,13 +1534,13 @@ static int add_oid(const struct reference *ref, void *cb_data)
return 0;
}
-static void add_negotiation_tips(struct git_transport_options *smart_options)
+static void add_negotiation_restrict_tips(struct git_transport_options *smart_options)
{
struct oid_array *oids = xcalloc(1, sizeof(*oids));
int i;
- for (i = 0; i < negotiation_tip.nr; i++) {
- const char *s = negotiation_tip.items[i].string;
+ for (i = 0; i < negotiation_restrict.nr; i++) {
+ const char *s = negotiation_restrict.items[i].string;
struct refs_for_each_ref_options opts = {
.pattern = s,
};
@@ -1561,7 +1561,7 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
warning(_("ignoring %s=%s because it does not match any refs"),
"--negotiation-restrict", s);
}
- smart_options->negotiation_tips = oids;
+ smart_options->negotiation_restrict_tips = oids;
}
static struct transport *prepare_transport(struct remote *remote, int deepen,
@@ -1595,9 +1595,9 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER, spec);
set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
}
- if (negotiation_tip.nr) {
+ if (negotiation_restrict.nr) {
if (transport->smart_options)
- add_negotiation_tips(transport->smart_options);
+ add_negotiation_restrict_tips(transport->smart_options);
else
warning(_("ignoring %s because the protocol does not support it"),
"--negotiation-restrict");
@@ -2566,7 +2566,7 @@ int cmd_fetch(int argc,
N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
OPT_IPVERSION(&family),
- OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, N_("revision"),
+ OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_restrict, N_("revision"),
N_("report that we have only objects reachable from this object")),
OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
OPT_BOOL(0, "negotiate-only", &negotiate_only,
@@ -2658,7 +2658,7 @@ int cmd_fetch(int argc,
config.display_format = DISPLAY_FORMAT_PORCELAIN;
}
- if (negotiate_only && !negotiation_tip.nr)
+ if (negotiate_only && !negotiation_restrict.nr)
die(_("%s needs one or more %s"), "--negotiate-only",
"--negotiation-restrict=*");
diff --git a/fetch-pack.c b/fetch-pack.c
index 6ecd468ef7..baf239adf9 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -291,21 +291,21 @@ static int next_flush(int stateless_rpc, int count)
}
static void mark_tips(struct fetch_negotiator *negotiator,
- const struct oid_array *negotiation_tips)
+ const struct oid_array *negotiation_restrict_tips)
{
struct refs_for_each_ref_options opts = {
.flags = REFS_FOR_EACH_INCLUDE_BROKEN,
};
int i;
- if (!negotiation_tips) {
+ if (!negotiation_restrict_tips) {
refs_for_each_ref_ext(get_main_ref_store(the_repository),
rev_list_insert_ref_oid, negotiator, &opts);
return;
}
- for (i = 0; i < negotiation_tips->nr; i++)
- rev_list_insert_ref(negotiator, &negotiation_tips->oid[i]);
+ for (i = 0; i < negotiation_restrict_tips->nr; i++)
+ rev_list_insert_ref(negotiator, &negotiation_restrict_tips->oid[i]);
return;
}
@@ -355,7 +355,7 @@ static int find_common(struct fetch_negotiator *negotiator,
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_DIE_ON_ERR_PACKET);
- mark_tips(negotiator, args->negotiation_tips);
+ mark_tips(negotiator, args->negotiation_restrict_tips);
for_each_cached_alternate(negotiator, insert_one_alternate_object);
fetching = 0;
@@ -1728,7 +1728,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
else
state = FETCH_SEND_REQUEST;
- mark_tips(negotiator, args->negotiation_tips);
+ mark_tips(negotiator, args->negotiation_restrict_tips);
for_each_cached_alternate(negotiator,
insert_one_alternate_object);
break;
@@ -2177,7 +2177,7 @@ static void clear_common_flag(struct oidset *s)
}
}
-void negotiate_using_fetch(const struct oid_array *negotiation_tips,
+void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
const struct string_list *server_options,
int stateless_rpc,
int fd[],
@@ -2195,13 +2195,13 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
timestamp_t min_generation = GENERATION_NUMBER_INFINITY;
fetch_negotiator_init(the_repository, &negotiator);
- mark_tips(&negotiator, negotiation_tips);
+ mark_tips(&negotiator, negotiation_restrict_tips);
packet_reader_init(&reader, fd[0], NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_DIE_ON_ERR_PACKET);
- oid_array_for_each((struct oid_array *) negotiation_tips,
+ oid_array_for_each((struct oid_array *) negotiation_restrict_tips,
add_to_object_array,
&nt_object_array);
diff --git a/fetch-pack.h b/fetch-pack.h
index 9d3470366f..6c70c942c2 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -21,7 +21,7 @@ struct fetch_pack_args {
* If not NULL, during packfile negotiation, fetch-pack will send "have"
* lines only with these tips and their ancestors.
*/
- const struct oid_array *negotiation_tips;
+ const struct oid_array *negotiation_restrict_tips;
unsigned deepen_relative:1;
unsigned quiet:1;
@@ -89,7 +89,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
* In the capability advertisement that has happened prior to invoking this
* function, the "wait-for-done" capability must be present.
*/
-void negotiate_using_fetch(const struct oid_array *negotiation_tips,
+void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
const struct string_list *server_options,
int stateless_rpc,
int fd[],
diff --git a/transport-helper.c b/transport-helper.c
index 4d95d84f9e..0e5b3b7202 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -754,7 +754,7 @@ static int fetch_refs(struct transport *transport,
set_helper_option(transport, "filter", spec);
}
- if (data->transport_options.negotiation_tips)
+ if (data->transport_options.negotiation_restrict_tips)
warning("Ignoring --negotiation-tip because the protocol does not support it.");
if (data->fetch)
diff --git a/transport.c b/transport.c
index 107f4fa5dc..a3051f6733 100644
--- a/transport.c
+++ b/transport.c
@@ -463,7 +463,7 @@ static int fetch_refs_via_pack(struct transport *transport,
args.refetch = data->options.refetch;
args.stateless_rpc = transport->stateless_rpc;
args.server_options = transport->server_options;
- args.negotiation_tips = data->options.negotiation_tips;
+ args.negotiation_restrict_tips = data->options.negotiation_restrict_tips;
args.reject_shallow_remote = transport->smart_options->reject_shallow;
if (!data->finished_handshake) {
@@ -491,7 +491,7 @@ static int fetch_refs_via_pack(struct transport *transport,
warning(_("server does not support wait-for-done"));
ret = -1;
} else {
- negotiate_using_fetch(data->options.negotiation_tips,
+ negotiate_using_fetch(data->options.negotiation_restrict_tips,
transport->server_options,
transport->stateless_rpc,
data->fd,
@@ -979,9 +979,9 @@ static int disconnect_git(struct transport *transport)
finish_connect(data->conn);
}
- if (data->options.negotiation_tips) {
- oid_array_clear(data->options.negotiation_tips);
- free(data->options.negotiation_tips);
+ if (data->options.negotiation_restrict_tips) {
+ oid_array_clear(data->options.negotiation_restrict_tips);
+ free(data->options.negotiation_restrict_tips);
}
list_objects_filter_release(&data->options.filter_options);
oid_array_clear(&data->extra_have);
diff --git a/transport.h b/transport.h
index 892f19454a..cdeb33c16f 100644
--- a/transport.h
+++ b/transport.h
@@ -40,13 +40,13 @@ struct git_transport_options {
/*
* This is only used during fetch. See the documentation of
- * negotiation_tips in struct fetch_pack_args.
+ * negotiation_restrict_tips in struct fetch_pack_args.
*
* This field is only supported by transports that support connect or
* stateless_connect. Set this field directly instead of using
* transport_set_option().
*/
- struct oid_array *negotiation_tips;
+ struct oid_array *negotiation_restrict_tips;
/*
* If allocated, whenever transport_fetch_refs() is called, add known
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 2/7] fetch: add --negotiation-restrict option
From: Derrick Stolee via GitGitGadget @ 2026-04-22 15:25 UTC (permalink / raw)
To: git; +Cc: gitster, ps, Derrick Stolee, Derrick Stolee
In-Reply-To: <pull.2085.v3.git.1776871546.gitgitgadget@gmail.com>
From: Derrick Stolee <stolee@gmail.com>
The --negotiation-tip option to 'git fetch' and 'git pull' allows users
to specify that they want to focus negotiation on a small set of
references. This is a _restriction_ on the negotiation set, helping to
focus the negotiation when the ref count is high. However, it doesn't
allow for the ability to opportunistically select references beyond that
list.
This subtle detail that this is a 'maximum set' and not a 'minimum set'
is not immediately clear from the option name. This makes it more
complicated to add a new option that provides the complementary behavior
of a minimum set.
For now, create a new synonym option, --negotiation-restrict, that
behaves identically to --negotiation-tip. Update the documentation to
make it clear that this new name is the preferred option, but we keep
the old name for compatibility. Mark --negotiation-tip as an alias of the
new, preferred option.
Update a few warning messages with the new option, but also make them
translatable with the option name inserted by formatting. At least one
of these messages will be reused later for a new option.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/fetch-options.adoc | 4 ++++
builtin/fetch.c | 13 ++++++++-----
builtin/pull.c | 3 +++
t/t5510-fetch.sh | 25 +++++++++++++++++++++++++
t/t5702-protocol-v2.sh | 4 ++--
5 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
index 81a9d7f9bb..c07b85499f 100644
--- a/Documentation/fetch-options.adoc
+++ b/Documentation/fetch-options.adoc
@@ -49,6 +49,7 @@ the current repository has the same history as the source repository.
`.git/shallow`. This option updates `.git/shallow` and accepts such
refs.
+`--negotiation-restrict=(<commit>|<glob>)`::
`--negotiation-tip=(<commit>|<glob>)`::
By default, Git will report, to the server, commits reachable
from all local refs to find common commits in an attempt to
@@ -58,6 +59,9 @@ the current repository has the same history as the source repository.
local ref is likely to have commits in common with the
upstream ref being fetched.
+
+`--negotiation-restrict` is the preferred name for this option;
+`--negotiation-tip` is accepted as a synonym.
++
This option may be specified more than once; if so, Git will report
commits reachable from any of the given commits.
+
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4795b2a13c..fc950fe35b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1558,8 +1558,8 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
refs_for_each_ref_ext(get_main_ref_store(the_repository),
add_oid, oids, &opts);
if (old_nr == oids->nr)
- warning("ignoring --negotiation-tip=%s because it does not match any refs",
- s);
+ warning(_("ignoring %s=%s because it does not match any refs"),
+ "--negotiation-restrict", s);
}
smart_options->negotiation_tips = oids;
}
@@ -1599,7 +1599,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
if (transport->smart_options)
add_negotiation_tips(transport->smart_options);
else
- warning("ignoring --negotiation-tip because the protocol does not support it");
+ warning(_("ignoring %s because the protocol does not support it"),
+ "--negotiation-restrict");
}
return transport;
}
@@ -2565,8 +2566,9 @@ int cmd_fetch(int argc,
N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
OPT_IPVERSION(&family),
- OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
+ OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, N_("revision"),
N_("report that we have only objects reachable from this object")),
+ OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
OPT_BOOL(0, "negotiate-only", &negotiate_only,
N_("do not fetch a packfile; instead, print ancestors of negotiation tips")),
OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
@@ -2657,7 +2659,8 @@ int cmd_fetch(int argc,
}
if (negotiate_only && !negotiation_tip.nr)
- die(_("--negotiate-only needs one or more --negotiation-tip=*"));
+ die(_("%s needs one or more %s"), "--negotiate-only",
+ "--negotiation-restrict=*");
if (deepen_relative) {
if (deepen_relative < 0)
diff --git a/builtin/pull.c b/builtin/pull.c
index 7e67fdce97..821cc6699a 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -999,6 +999,9 @@ int cmd_pull(int argc,
OPT_PASSTHRU_ARGV(0, "negotiation-tip", &opt_fetch, N_("revision"),
N_("report that we have only objects reachable from this object"),
0),
+ OPT_PASSTHRU_ARGV(0, "negotiation-restrict", &opt_fetch, N_("revision"),
+ N_("report that we have only objects reachable from this object"),
+ 0),
OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
N_("check for forced-updates on all updated branches")),
OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 5dcb4b51a4..dc3ce56d84 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1460,6 +1460,31 @@ EOF
test_cmp fatal-expect fatal-actual
'
+test_expect_success '--negotiation-restrict limits "have" lines sent' '
+ setup_negotiation_tip server server 0 &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 --negotiation-restrict=beta_1 \
+ origin alpha_s beta_s &&
+ check_negotiation_tip
+'
+
+test_expect_success '--negotiation-restrict understands globs' '
+ setup_negotiation_tip server server 0 &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=*_1 \
+ origin alpha_s beta_s &&
+ check_negotiation_tip
+'
+
+test_expect_success '--negotiation-restrict and --negotiation-tip can be mixed' '
+ setup_negotiation_tip server server 0 &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
+ --negotiation-tip=beta_1 \
+ origin alpha_s beta_s &&
+ check_negotiation_tip
+'
+
test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' '
git init df-conflict &&
(
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index f826ac46a5..9f6cf4142d 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -869,14 +869,14 @@ setup_negotiate_only () {
test_commit -C client three
}
-test_expect_success 'usage: --negotiate-only without --negotiation-tip' '
+test_expect_success 'usage: --negotiate-only without --negotiation-restrict' '
SERVER="server" &&
URI="file://$(pwd)/server" &&
setup_negotiate_only "$SERVER" "$URI" &&
cat >err.expect <<-\EOF &&
- fatal: --negotiate-only needs one or more --negotiation-tip=*
+ fatal: --negotiate-only needs one or more --negotiation-restrict=*
EOF
test_must_fail git -c protocol.version=2 -C client fetch \
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 0/7] fetch: rework negotiation tip options
From: Derrick Stolee via GitGitGadget @ 2026-04-22 15:25 UTC (permalink / raw)
To: git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <pull.2085.v2.git.1776266066.gitgitgadget@gmail.com>
Fetch negotiation aims to find enough information from haves and wants such
that the server can be reasonably confident that it will send all necessary
objects and not too many "extra" objects that the client already has.
However, this can break down if there are too many references, since Git
truncates the list of haves based on a few factors (a 256 count limit or the
server sending an ACK at the right time).
We already have the --negotiation-tip feature to focus the set of references
that are used in negotiation, but I feel like this is designed backwards.
I'd rather that we have a way to say "this is an important set of refs, but
feel free to add more refs if needed" than "only use these refs for
negotiation".
Here's an example that demonstrates the problem. In an internal monorepo,
developers work off of the 'main' branch so there are thousands of user
branches that each add a few commits different from the 'main' branch.
However, there is also a long-lived 'release' branch. This branch has a
first-parent history that is parallel to 'main' and each of those commits is
a merge whose second parent is a commit from 'main' that had a successful CI
run. There are additional changes in the 'release' branch merge commits that
add some changelog data, so there is a nontrivial set of novel blob content
in that branch and not just a different set of commits.
The problem we had was that our georeplication system was regularly fetching
from the origin and trying to get all data from all reachable branches. When
the 'release' branch updated, the client would run out of haves before
advertising its copy of the 'release' branch, but it would still list the
new 'release' tip as a want. The server would then think that the client had
never fetched that branch before and would send all of the changelog data
from the whole history of the repo. (This led to a lot of downstream
problems; we mitigated by setting a refspec that stopped fetching the
'release' branch, but this is not ideal.)
What I'd like is a mechanism to say "always advertise the client's version
of 'main' and 'release' but also opportunistically include some user
branches".
Based on my understanding, the '--negotiation-tip' option is close but not
quite what I want. I could have the client only advertise 'release' and
'main' and never advertise any user branches. But then we'd download all
content from each user branch every time it updates. Perhaps this would
happen even with opportunistic inclusion of more haves, but I'd like to
explore this area more.
There's also an issue that the '--negotiation-tip' feature doesn't seem to
have a config key that enables it without CLI arguments. This is something
that we could consider independently.
This patch series adds a new '--negotiation-include' option that does what I
want: it makes sure that these references are included as 'have's during
negotiation. In order to help clarify the difference between this and
'--negotiation-tip', I first create a synonym called
'--negotiation-restrict'.
Both of these options get 'remote.*.negotiation(Include|Restrict)' config
options that enable their behavior by default.
During development, I had briefly considered only using config values, but
that required some strange changes to care about the remote name in the
transport layer. This was most different in the 'git push' integration. When
I discovered the '--negotiation-tip' feature during the process, that gave
me a clear pattern to follow with the addition of a config on top.
Updates in v2
=============
This version is a near-complete rewrite based on feedback around the names
of the previous option and config. The --negotiation-restrict option is new
and the ability to set it via config is also new.
I did try to be more careful around translatable error messages, too.
Updates in v3
=============
* --negotiation-tip is now an alias of --negotiation-restrict.
* More translatable strings use %s to isolate non-translatable options from
translatable words.
* The string_list named negotiation_tip is now renamed to
negotiation_restrict.
* The config options now allow an empty value to reset the list.
* The --negotiation-require option is now called --negotiation-include.
* Similarly, the config option is renamed and all code references.
* The included haves now mark their commits as COMMON so commits that they
can reach are not included in the negotiation walk if they are reached
from the restricted commits.
* The ref iterators are more careful about failing on bad references (ref
exists but object doesn't) and ignoring missing references (perhaps
config is erroneous?).
* When sending tips during push negotiation, use the --negotiation-restrict
option instead of -tip.
Thanks, -Stolee
Derrick Stolee (7):
t5516: fix test order flakiness
fetch: add --negotiation-restrict option
transport: rename negotiation_tips
remote: add remote.*.negotiationRestrict config
fetch: add --negotiation-include option for negotiation
remote: add remote.*.negotiationInclude config
send-pack: pass negotiation config in push
Documentation/config/remote.adoc | 46 +++++++++
Documentation/fetch-options.adoc | 27 +++++
builtin/fetch.c | 70 ++++++++++---
builtin/pull.c | 6 ++
fetch-pack.c | 130 +++++++++++++++++++++---
fetch-pack.h | 14 ++-
remote.c | 16 +++
remote.h | 2 +
send-pack.c | 39 ++++++--
send-pack.h | 2 +
t/t5510-fetch.sh | 166 +++++++++++++++++++++++++++++++
t/t5516-fetch-push.sh | 32 +++++-
t/t5702-protocol-v2.sh | 4 +-
transport-helper.c | 2 +-
transport.c | 16 +--
transport.h | 10 +-
16 files changed, 529 insertions(+), 53 deletions(-)
base-commit: 6e8d538aab8fe4dd07ba9fb87b5c7edcfa5706ad
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2085%2Fderrickstolee%2Fmust-have-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2085/derrickstolee/must-have-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2085
Range-diff vs v2:
1: 466c56abe0 = 1: 466c56abe0 t5516: fix test order flakiness
2: 9a25b0fade ! 2: fe875399a8 fetch: add --negotiation-restrict option
@@ Commit message
For now, create a new synonym option, --negotiation-restrict, that
behaves identically to --negotiation-tip. Update the documentation to
make it clear that this new name is the preferred option, but we keep
- the old name for compatibility.
+ the old name for compatibility. Mark --negotiation-tip as an alias of the
+ new, preferred option.
Update a few warning messages with the new option, but also make them
translatable with the option name inserted by formatting. At least one
@@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
return transport;
}
@@ builtin/fetch.c: int cmd_fetch(int argc,
+ N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
+ OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
OPT_IPVERSION(&family),
- OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
- N_("report that we have only objects reachable from this object")),
+- OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
+ OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, N_("revision"),
-+ N_("report that we have only objects reachable from this object")),
+ N_("report that we have only objects reachable from this object")),
++ OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
OPT_BOOL(0, "negotiate-only", &negotiate_only,
N_("do not fetch a packfile; instead, print ancestors of negotiation tips")),
OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
@@ builtin/fetch.c: int cmd_fetch(int argc,
if (negotiate_only && !negotiation_tip.nr)
- die(_("--negotiate-only needs one or more --negotiation-tip=*"));
-+ die(_("--negotiate-only needs one or more --negotiation-restrict=*"));
++ die(_("%s needs one or more %s"), "--negotiate-only",
++ "--negotiation-restrict=*");
if (deepen_relative) {
if (deepen_relative < 0)
3: 0f89665aee ! 3: 4332cbf266 transport: rename negotiation_tips
@@ Commit message
layer. This requires the builtin to handle parsing refs into collections
of oids so the transport layer can handle this cleaner form of the data.
+ Also update the string_list used to store the inputs from command-line
+ options.
+
Signed-off-by: Derrick Stolee <stolee@gmail.com>
## builtin/fetch.c ##
+@@ builtin/fetch.c: static struct transport *gtransport;
+ static struct transport *gsecondary;
+ static struct refspec refmap = REFSPEC_INIT_FETCH;
+ static struct string_list server_options = STRING_LIST_INIT_DUP;
+-static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
++static struct string_list negotiation_restrict = STRING_LIST_INIT_NODUP;
+
+ struct fetch_config {
+ enum display_format display_format;
@@ builtin/fetch.c: static int add_oid(const struct reference *ref, void *cb_data)
return 0;
}
@@ builtin/fetch.c: static int add_oid(const struct reference *ref, void *cb_data)
{
struct oid_array *oids = xcalloc(1, sizeof(*oids));
int i;
+
+- for (i = 0; i < negotiation_tip.nr; i++) {
+- const char *s = negotiation_tip.items[i].string;
++ for (i = 0; i < negotiation_restrict.nr; i++) {
++ const char *s = negotiation_restrict.items[i].string;
+ struct refs_for_each_ref_options opts = {
+ .pattern = s,
+ };
@@ builtin/fetch.c: static void add_negotiation_tips(struct git_transport_options *smart_options)
warning(_("ignoring %s=%s because it does not match any refs"),
"--negotiation-restrict", s);
@@ builtin/fetch.c: static void add_negotiation_tips(struct git_transport_options *
static struct transport *prepare_transport(struct remote *remote, int deepen,
@@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remote, int deepen,
+ set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER, spec);
+ set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
}
- if (negotiation_tip.nr) {
+- if (negotiation_tip.nr) {
++ if (negotiation_restrict.nr) {
if (transport->smart_options)
- add_negotiation_tips(transport->smart_options);
+ add_negotiation_restrict_tips(transport->smart_options);
else
warning(_("ignoring %s because the protocol does not support it"),
"--negotiation-restrict");
+@@ builtin/fetch.c: int cmd_fetch(int argc,
+ N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
+ OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
+ OPT_IPVERSION(&family),
+- OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, N_("revision"),
++ OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_restrict, N_("revision"),
+ N_("report that we have only objects reachable from this object")),
+ OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
+ OPT_BOOL(0, "negotiate-only", &negotiate_only,
+@@ builtin/fetch.c: int cmd_fetch(int argc,
+ config.display_format = DISPLAY_FORMAT_PORCELAIN;
+ }
+
+- if (negotiate_only && !negotiation_tip.nr)
++ if (negotiate_only && !negotiation_restrict.nr)
+ die(_("%s needs one or more %s"), "--negotiate-only",
+ "--negotiation-restrict=*");
+
## fetch-pack.c ##
@@ fetch-pack.c: static int next_flush(int stateless_rpc, int count)
4: a731f4fc87 ! 4: d2f48b78b5 remote: add remote.*.negotiationRestrict config
@@ Commit message
If the user provides even one --negotiation-restrict argument, then the
config is ignored.
+ An empty value resets the value list to allow ignoring earlier config
+ values, such as those that might be set in system or global config.
+
Signed-off-by: Derrick Stolee <stolee@gmail.com>
## Documentation/config/remote.adoc ##
@@ Documentation/config/remote.adoc: priority configuration file (e.g. `.git/config
+command-line option. If `--negotiation-restrict` (or its synonym
+`--negotiation-tip`) is specified on the command line, then the config
+values are not used.
+++
++Blank values signal to ignore all previous values, allowing a reset of
++the list from broader config scenarios.
+
remote.<name>.followRemoteHEAD::
How linkgit:git-fetch[1] should handle updates to `remotes/<name>/HEAD`
@@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
+ } else if (remote->negotiation_restrict.nr) {
+ struct string_list_item *item;
+ for_each_string_list_item(item, &remote->negotiation_restrict)
-+ string_list_append(&negotiation_tip, item->string);
++ string_list_append(&negotiation_restrict, item->string);
+ if (transport->smart_options)
+ add_negotiation_restrict_tips(transport->smart_options);
+ else {
@@ builtin/fetch.c: int cmd_fetch(int argc,
config.display_format = DISPLAY_FORMAT_PORCELAIN;
}
-- if (negotiate_only && !negotiation_tip.nr)
-- die(_("--negotiate-only needs one or more --negotiation-restrict=*"));
-+ if (negotiate_only && !negotiation_tip.nr) {
-+ /*
-+ * Defer this check: remote.<name>.negotiationRestrict may
-+ * provide defaults in prepare_transport().
-+ */
-+ }
-
+- if (negotiate_only && !negotiation_restrict.nr)
+- die(_("%s needs one or more %s"), "--negotiate-only",
+- "--negotiation-restrict=*");
+-
if (deepen_relative) {
if (deepen_relative < 0)
+ die(_("negative depth in --deepen is not supported"));
@@ builtin/fetch.c: int cmd_fetch(int argc,
if (!remote)
die(_("must supply remote when using --negotiate-only"));
gtransport = prepare_transport(remote, 1, &filter_options);
+ if (!gtransport->smart_options ||
+ !gtransport->smart_options->negotiation_restrict_tips)
-+ die(_("--negotiate-only needs one or more --negotiation-restrict=*"));
++ die(_("%s needs one or more %s"), "--negotiate-only",
++ "--negotiation-restrict=*");
if (gtransport->smart_options) {
gtransport->smart_options->acked_commits = &acked_commits;
} else {
@@ remote.c: static int handle_config(const char *key, const char *value,
return parse_transport_option(key, value,
&remote->server_options);
+ } else if (!strcmp(subkey, "negotiationrestrict")) {
-+ if (!value)
-+ return config_error_nonbool(key);
-+ string_list_append(&remote->negotiation_restrict, value);
++ /* reset list on empty value. */
++ if (!value || !*value)
++ string_list_clear(&remote->negotiation_restrict, 0);
++ else
++ string_list_append(&remote->negotiation_restrict, value);
} else if (!strcmp(subkey, "followremotehead")) {
const char *no_warn_branch;
if (!strcmp(value, "never"))
@@ t/t5510-fetch.sh: test_expect_success '--negotiation-restrict and --negotiation-
+test_expect_success 'remote.<name>.negotiationRestrict used as default' '
+ setup_negotiation_tip server server 0 &&
++
++ # test the reset of the list on an empty value
++ git -C client config --add remote.origin.negotiationRestrict alpha_2 &&
++ git -C client config --add remote.origin.negotiationRestrict "" &&
+ git -C client config --add remote.origin.negotiationRestrict alpha_1 &&
+ git -C client config --add remote.origin.negotiationRestrict beta_1 &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
5: 49c80cef2e ! 5: ae81ef36a1 fetch: add --negotiation-require option for negotiation
@@ Metadata
Author: Derrick Stolee <stolee@gmail.com>
## Commit message ##
- fetch: add --negotiation-require option for negotiation
+ fetch: add --negotiation-include option for negotiation
- Add a new --negotiation-require option to 'git fetch', which ensures
+ Add a new --negotiation-include option to 'git fetch', which ensures
that certain ref tips are always sent as 'have' lines during fetch
negotiation, regardless of what the negotiation algorithm selects.
@@ Commit message
--negotiation-restrict to focus the negotiation to 'dev' and 'release'
would avoid those problematic downloads, but would still not allow
advertising potentially-relevant user brances. In this way, the
- 'require' version solves the problem I mention while allowing
+ 'include' version solves the problem I mention while allowing
negotiation to pick other references opportunistically. The two options
can also be combined to allow the best of both worlds.
The argument may be an exact ref name or a glob pattern. Non-existent
- refs are silently ignored.
+ refs are silently ignored. This behavior is also updated in the ref matching
+ logic for the related --negotiation-restrict option to match.
- Also add --negotiation-require to 'git pull' passthrough options.
+ The implementation outputs the requested objects as haves before the
+ negotiation algorithm kicks in and performs a priority-queue walk from the
+ tip commits. In order to avoid duplicates, we mark the requested objects as
+ COMMON so they (and their descendants) are not output by the negotiator. The
+ negotiator still outputs at least one have before a round is flushed, when
+ the server could ACK to stop the negotiation.
+
+ Also add --negotiation-include to 'git pull' passthrough options.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
@@ Documentation/fetch-options.adoc: See also the `fetch.negotiationAlgorithm` and
configuration variables documented in linkgit:git-config[1], and the
`--negotiate-only` option below.
-+`--negotiation-require=<revision>`::
++`--negotiation-include=<revision>`::
+ Ensure that the given ref tip is always sent as a "have" line
+ during fetch negotiation, regardless of what the negotiation
+ algorithm selects. This is useful to guarantee that common
@@ Documentation/fetch-options.adoc: See also the `fetch.negotiationAlgorithm` and
++
+If `--negotiation-restrict` is used, the have set is first restricted by
+that option and then increased to include the tips specified by
-+`--negotiation-require`.
++`--negotiation-include`.
+
`--negotiate-only`::
Do not fetch anything from the server, and instead print the
@@ builtin/fetch.c
@@ builtin/fetch.c: static struct transport *gsecondary;
static struct refspec refmap = REFSPEC_INIT_FETCH;
static struct string_list server_options = STRING_LIST_INIT_DUP;
- static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
-+static struct string_list negotiation_require = STRING_LIST_INIT_NODUP;
+ static struct string_list negotiation_restrict = STRING_LIST_INIT_NODUP;
++static struct string_list negotiation_include = STRING_LIST_INIT_NODUP;
struct fetch_config {
enum display_format display_format;
+@@ builtin/fetch.c: static void add_negotiation_restrict_tips(struct git_transport_options *smart_op
+ int old_nr;
+ if (!has_glob_specials(s)) {
+ struct object_id oid;
++
++ /* Ignore missing reference. */
+ if (repo_get_oid(the_repository, s, &oid))
+- die(_("%s is not a valid object"), s);
++ continue;
++ /* Fail on missing object pointed by ref. */
+ if (!odb_has_object(the_repository->objects, &oid, 0))
+ die(_("the object %s does not exist"), s);
++
+ oid_array_append(oids, &oid);
+ continue;
+ }
@@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remote, int deepen,
strbuf_release(&config_name);
}
}
-+ if (negotiation_require.nr) {
++ if (negotiation_include.nr) {
+ if (transport->smart_options)
-+ transport->smart_options->negotiation_require = &negotiation_require;
++ transport->smart_options->negotiation_include = &negotiation_include;
+ else
+ warning(_("ignoring %s because the protocol does not support it"),
-+ "--negotiation-require");
++ "--negotiation-include");
+ }
return transport;
}
@@ builtin/fetch.c: int cmd_fetch(int argc,
+ OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_restrict, N_("revision"),
N_("report that we have only objects reachable from this object")),
- OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, N_("revision"),
- N_("report that we have only objects reachable from this object")),
-+ OPT_STRING_LIST(0, "negotiation-require", &negotiation_require, N_("revision"),
+ OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
++ OPT_STRING_LIST(0, "negotiation-include", &negotiation_include, N_("revision"),
+ N_("ensure this ref is always sent as a negotiation have")),
OPT_BOOL(0, "negotiate-only", &negotiate_only,
N_("do not fetch a packfile; instead, print ancestors of negotiation tips")),
@@ builtin/pull.c: int cmd_pull(int argc,
OPT_PASSTHRU_ARGV(0, "negotiation-restrict", &opt_fetch, N_("revision"),
N_("report that we have only objects reachable from this object"),
0),
-+ OPT_PASSTHRU_ARGV(0, "negotiation-require", &opt_fetch, N_("revision"),
++ OPT_PASSTHRU_ARGV(0, "negotiation-include", &opt_fetch, N_("revision"),
+ N_("ensure this ref is always sent as a negotiation have"),
+ 0),
OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
@@ fetch-pack.c: static void send_filter(struct fetch_pack_args *args,
+static int add_oid_to_oidset(const struct reference *ref, void *cb_data)
+{
+ struct oidset *set = cb_data;
-+ if (odb_has_object(the_repository->objects, ref->oid, 0))
-+ oidset_insert(set, ref->oid);
++ if (!odb_has_object(the_repository->objects, ref->oid, 0))
++ die(_("the object %s does not exist"), oid_to_hex(ref->oid));
++ oidset_insert(set, ref->oid);
+ return 0;
+}
+
-+static void resolve_negotiation_require(const struct string_list *negotiation_require,
++static void resolve_negotiation_include(const struct string_list *negotiation_include,
+ struct oidset *result)
+{
+ struct string_list_item *item;
+
-+ if (!negotiation_require || !negotiation_require->nr)
++ if (!negotiation_include || !negotiation_include->nr)
+ return;
+
-+ for_each_string_list_item(item, negotiation_require) {
++ for_each_string_list_item(item, negotiation_include) {
+ if (!has_glob_specials(item->string)) {
+ struct object_id oid;
++
++ /* Ignore missing reference. */
+ if (repo_get_oid(the_repository, item->string, &oid))
+ continue;
++
++ /* Fail on missing object pointed by ref. */
+ if (!odb_has_object(the_repository->objects, &oid, 0))
-+ continue;
++ die(_("the object %s does not exist"),
++ item->string);
++
+ oidset_insert(result, &oid);
+ } else {
+ struct refs_for_each_ref_options opts = {
@@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,
struct strbuf req_buf = STRBUF_INIT;
size_t state_len = 0;
struct packet_reader reader;
-+ struct oidset negotiation_require_oids = OIDSET_INIT;
++ struct oidset negotiation_include_oids = OIDSET_INIT;
if (args->stateless_rpc && multi_ack == 1)
die(_("the option '%s' requires '%s'"), "--stateless-rpc", "multi_ack_detailed");
@@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,
flushes = 0;
retval = -1;
+
-+ /* Send unconditional haves from --negotiation-require */
-+ resolve_negotiation_require(args->negotiation_require,
-+ &negotiation_require_oids);
-+ if (oidset_size(&negotiation_require_oids)) {
++ /* Send unconditional haves from --negotiation-include */
++ resolve_negotiation_include(args->negotiation_include,
++ &negotiation_include_oids);
++ if (oidset_size(&negotiation_include_oids)) {
+ struct oidset_iter iter;
-+ oidset_iter_init(&negotiation_require_oids, &iter);
++ oidset_iter_init(&negotiation_include_oids, &iter);
+
+ while ((oid = oidset_iter_next(&iter))) {
++ struct commit *commit;
+ packet_buf_write(&req_buf, "have %s\n",
+ oid_to_hex(oid));
+ print_verbose(args, "have %s", oid_to_hex(oid));
++ count++;
++
++ /*
++ * If this is a commit, then mark as COMMON to
++ * avoid the negotiator also outputting it as
++ * a have.
++ */
++ commit = lookup_commit(the_repository, oid);
++ if (commit &&
++ !repo_parse_commit(the_repository, commit))
++ commit->object.flags |= COMMON;
+ }
+ }
+
while ((oid = negotiator->next(negotiator))) {
-+ /* avoid duplicate oids from --negotiation-require */
-+ if (oidset_contains(&negotiation_require_oids, oid))
-+ continue;
packet_buf_write(&req_buf, "have %s\n", oid_to_hex(oid));
print_verbose(args, "have %s", oid_to_hex(oid));
- in_vain++;
@@ fetch-pack.c: done:
flushes++;
}
strbuf_release(&req_buf);
-+ oidset_clear(&negotiation_require_oids);
++ oidset_clear(&negotiation_include_oids);
if (!got_ready || !no_done)
consume_shallow_list(args, &reader);
@@ fetch-pack.c: static void add_common(struct strbuf *req_buf, struct oidset *comm
struct strbuf *req_buf,
- int *haves_to_send)
+ int *haves_to_send,
-+ struct oidset *negotiation_require_oids)
++ struct oidset *negotiation_include_oids)
{
int haves_added = 0;
const struct object_id *oid;
-+ /* Send unconditional haves from --negotiation-require */
-+ if (negotiation_require_oids) {
++ /* Send unconditional haves from --negotiation-include */
++ if (negotiation_include_oids) {
+ struct oidset_iter iter;
-+ oidset_iter_init(negotiation_require_oids, &iter);
++ oidset_iter_init(negotiation_include_oids, &iter);
+
+ while ((oid = oidset_iter_next(&iter)))
+ packet_buf_write(req_buf, "have %s\n",
@@ fetch-pack.c: static void add_common(struct strbuf *req_buf, struct oidset *comm
+ }
+
while ((oid = negotiator->next(negotiator))) {
-+ if (negotiation_require_oids &&
-+ oidset_contains(negotiation_require_oids, oid))
++ if (negotiation_include_oids &&
++ oidset_contains(negotiation_include_oids, oid))
+ continue;
packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
if (++haves_added >= *haves_to_send)
@@ fetch-pack.c: static int send_fetch_request(struct fetch_negotiator *negotiator,
int *haves_to_send, int *in_vain,
- int sideband_all, int seen_ack)
+ int sideband_all, int seen_ack,
-+ struct oidset *negotiation_require_oids)
++ struct oidset *negotiation_include_oids)
{
int haves_added;
int done_sent = 0;
@@ fetch-pack.c: static int send_fetch_request(struct fetch_negotiator *negotiator,
- haves_added = add_haves(negotiator, &req_buf, haves_to_send);
+ haves_added = add_haves(negotiator, &req_buf, haves_to_send,
-+ negotiation_require_oids);
++ negotiation_include_oids);
*in_vain += haves_added;
trace2_data_intmax("negotiation_v2", the_repository, "haves_added", haves_added);
trace2_data_intmax("negotiation_v2", the_repository, "in_vain", *in_vain);
@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
struct ref *ref = copy_ref_list(orig_ref);
enum fetch_state state = FETCH_CHECK_LOCAL;
struct oidset common = OIDSET_INIT;
-+ struct oidset negotiation_require_oids = OIDSET_INIT;
++ struct oidset negotiation_include_oids = OIDSET_INIT;
struct packet_reader reader;
int in_vain = 0, negotiation_started = 0;
int negotiation_round = 0;
@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
state = FETCH_SEND_REQUEST;
mark_tips(negotiator, args->negotiation_restrict_tips);
-+ resolve_negotiation_require(args->negotiation_require,
-+ &negotiation_require_oids);
++ resolve_negotiation_include(args->negotiation_include,
++ &negotiation_include_oids);
for_each_cached_alternate(negotiator,
insert_one_alternate_object);
break;
@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
reader.use_sideband,
- seen_ack)) {
+ seen_ack,
-+ &negotiation_require_oids)) {
++ &negotiation_include_oids)) {
trace2_region_leave_printf("negotiation_v2", "round",
the_repository, "%d",
negotiation_round);
@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
negotiator->release(negotiator);
oidset_clear(&common);
-+ oidset_clear(&negotiation_require_oids);
++ oidset_clear(&negotiation_include_oids);
return ref;
}
@@ fetch-pack.c: void negotiate_using_fetch(const struct oid_array *negotiation_res
int fd[],
- struct oidset *acked_commits)
+ struct oidset *acked_commits,
-+ const struct string_list *negotiation_require)
++ const struct string_list *negotiation_include)
{
struct fetch_negotiator negotiator;
struct packet_reader reader;
struct object_array nt_object_array = OBJECT_ARRAY_INIT;
struct strbuf req_buf = STRBUF_INIT;
-+ struct oidset negotiation_require_oids = OIDSET_INIT;
++ struct oidset negotiation_include_oids = OIDSET_INIT;
int haves_to_send = INITIAL_FLUSH;
int in_vain = 0;
int seen_ack = 0;
@@ fetch-pack.c: void negotiate_using_fetch(const struct oid_array *negotiation_res
fetch_negotiator_init(the_repository, &negotiator);
mark_tips(&negotiator, negotiation_restrict_tips);
-+ resolve_negotiation_require(negotiation_require,
-+ &negotiation_require_oids);
++ resolve_negotiation_include(negotiation_include,
++ &negotiation_include_oids);
+
packet_reader_init(&reader, fd[0], NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
@@ fetch-pack.c: void negotiate_using_fetch(const struct oid_array *negotiation_res
- haves_added = add_haves(&negotiator, &req_buf, &haves_to_send);
+ haves_added = add_haves(&negotiator, &req_buf, &haves_to_send,
-+ &negotiation_require_oids);
++ &negotiation_include_oids);
in_vain += haves_added;
if (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN))
last_iteration = 1;
@@ fetch-pack.c: void negotiate_using_fetch(const struct oid_array *negotiation_res
clear_common_flag(acked_commits);
object_array_clear(&nt_object_array);
-+ oidset_clear(&negotiation_require_oids);
++ oidset_clear(&negotiation_include_oids);
negotiator.release(&negotiator);
strbuf_release(&req_buf);
}
@@ fetch-pack.h: struct fetch_pack_args {
+ * as "have" lines during negotiation, regardless of what the
+ * negotiation algorithm selects.
+ */
-+ const struct string_list *negotiation_require;
++ const struct string_list *negotiation_include;
+
unsigned deepen_relative:1;
unsigned quiet:1;
@@ fetch-pack.h: void negotiate_using_fetch(const struct oid_array *negotiation_res
int fd[],
- struct oidset *acked_commits);
+ struct oidset *acked_commits,
-+ const struct string_list *negotiation_require);
++ const struct string_list *negotiation_include);
/*
* Print an appropriate error message for each sought ref that wasn't
@@ t/t5510-fetch.sh: test_expect_success 'CLI --negotiation-restrict overrides remo
test_grep ! "fetch> have $BETA_1" trace
'
-+test_expect_success '--negotiation-require includes configured refs as haves' '
++test_expect_success '--negotiation-include includes configured refs as haves' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
-+ --negotiation-require=refs/tags/beta_1 \
++ --negotiation-include=refs/tags/beta_1 \
+ origin alpha_s beta_s &&
+
+ ALPHA_1=$(git -C client rev-parse alpha_1) &&
@@ t/t5510-fetch.sh: test_expect_success 'CLI --negotiation-restrict overrides remo
+ test_grep "fetch> have $BETA_1" trace
+'
+
-+test_expect_success '--negotiation-require works with glob patterns' '
++test_expect_success '--negotiation-include works with glob patterns' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
-+ --negotiation-require="refs/tags/beta_*" \
++ --negotiation-include="refs/tags/beta_*" \
+ origin alpha_s beta_s &&
+
+ BETA_1=$(git -C client rev-parse beta_1) &&
@@ t/t5510-fetch.sh: test_expect_success 'CLI --negotiation-restrict overrides remo
+ test_grep "fetch> have $BETA_2" trace
+'
+
-+test_expect_success '--negotiation-require is additive with negotiation' '
++test_expect_success '--negotiation-include is additive with negotiation' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
-+ --negotiation-require=refs/tags/beta_1 \
++ --negotiation-include=refs/tags/beta_1 \
+ origin alpha_s beta_s &&
+
+ BETA_1=$(git -C client rev-parse beta_1) &&
+ test_grep "fetch> have $BETA_1" trace
+'
+
-+test_expect_success '--negotiation-require ignores non-existent refs silently' '
++test_expect_success '--negotiation-include ignores non-existent refs silently' '
+ setup_negotiation_tip server server 0 &&
+
+ git -C client fetch --quiet \
+ --negotiation-restrict=alpha_1 \
-+ --negotiation-require=refs/tags/nonexistent \
++ --negotiation-include=refs/tags/nonexistent \
+ origin alpha_s beta_s 2>err &&
+ test_must_be_empty err
+'
+
-+test_expect_success '--negotiation-require avoids duplicates with negotiator' '
++test_expect_success '--negotiation-include avoids duplicates with negotiator' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
+ ALPHA_1=$(git -C client rev-parse alpha_1) &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
-+ --negotiation-require=refs/tags/alpha_1 \
++ --negotiation-include=refs/tags/alpha_1 \
+ origin alpha_s beta_s &&
+
+ test_grep "fetch> have $ALPHA_1" trace >matches &&
@@ transport.c: static int fetch_refs_via_pack(struct transport *transport,
args.stateless_rpc = transport->stateless_rpc;
args.server_options = transport->server_options;
args.negotiation_restrict_tips = data->options.negotiation_restrict_tips;
-+ args.negotiation_require = data->options.negotiation_require;
++ args.negotiation_include = data->options.negotiation_include;
args.reject_shallow_remote = transport->smart_options->reject_shallow;
if (!data->finished_handshake) {
@@ transport.c: static int fetch_refs_via_pack(struct transport *transport,
data->fd,
- data->options.acked_commits);
+ data->options.acked_commits,
-+ data->options.negotiation_require);
++ data->options.negotiation_include);
ret = 0;
}
goto cleanup;
@@ transport.h: struct git_transport_options {
+ * If non-empty, ref patterns whose tips should always be sent
+ * as "have" lines during negotiation.
+ */
-+ const struct string_list *negotiation_require;
++ const struct string_list *negotiation_include;
+
/*
* If allocated, whenever transport_fetch_refs() is called, add known
6: 081f904c07 ! 6: a2d15fa12a remote: add negotiationRequire config as default for --negotiation-require
@@ Metadata
Author: Derrick Stolee <stolee@gmail.com>
## Commit message ##
- remote: add negotiationRequire config as default for --negotiation-require
+ remote: add remote.*.negotiationInclude config
- Add a new 'remote.<name>.negotiationRequire' multi-valued config option
- that provides default values for --negotiation-require when no
- --negotiation-require arguments are specified over the command line.
- This is a mirror of how 'remote.<name>.negotiationRestrict' specifies
- defaults for the --negotiation-restrict arguments.
+ Add a new 'remote.<name>.negotiationInclude' multi-valued config option that
+ provides default values for --negotiation-include when no
+ --negotiation-include arguments are specified over the command line. This
+ is a mirror of how 'remote.<name>.negotiationRestrict' specifies defaults
+ for the --negotiation-restrict arguments.
- Each value is either an exact ref name or a glob pattern whose tips
- should always be sent as 'have' lines during negotiation. The config
- values are resolved through the same resolve_negotiation_require()
- codepath as the CLI options.
+ Each value is either an exact ref name or a glob pattern whose tips should
+ always be sent as 'have' lines during negotiation. The config values are
+ resolved through the same resolve_negotiation_include() codepath as the CLI
+ options.
- This option is additive with the normal negotiation process: the
- negotiation algorithm still runs and advertises its own selected
- commits, but the refs matching the config are sent unconditionally
- on top of those heuristically selected commits.
+ This option is additive with the normal negotiation process: the negotiation
+ algorithm still runs and advertises its own selected commits, but the refs
+ matching the config are sent unconditionally on top of those heuristically
+ selected commits.
+
+ Similar to the negotiationRestrict config, an empty value resets the value
+ list to allow ignoring earlier config values, such as those that might be
+ set in system or global config.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
## Documentation/config/remote.adoc ##
-@@ Documentation/config/remote.adoc: command-line option. If `--negotiation-restrict` (or its synonym
- `--negotiation-tip`) is specified on the command line, then the config
- values are not used.
+@@ Documentation/config/remote.adoc: values are not used.
+ Blank values signal to ignore all previous values, allowing a reset of
+ the list from broader config scenarios.
-+remote.<name>.negotiationRequire::
++remote.<name>.negotiationInclude::
+ When negotiating with this remote during `git fetch` and `git push`,
+ the client advertises a list of commits that exist locally. In
+ repos with many references, this list of "haves" can be truncated.
@@ Documentation/config/remote.adoc: command-line option. If `--negotiation-restri
+glob pattern (e.g. `refs/heads/release/*`). The pattern syntax is the same
+as for `--negotiation-restrict`.
++
-+These config values are used as defaults for the `--negotiation-require`
-+command-line option. If `--negotiation-require` is specified on the
++These config values are used as defaults for the `--negotiation-include`
++command-line option. If `--negotiation-include` is specified on the
+command line, then the config values are not used.
++
+This option is additive with the normal negotiation process: the
+negotiation algorithm still runs and advertises its own selected commits,
-+but the refs matching `remote.<name>.negotiationRequire` are sent
++but the refs matching `remote.<name>.negotiationInclude` are sent
+unconditionally on top of those heuristically selected commits. This
+option is also used during push negotiation when `push.negotiate` is
+enabled.
+++
++Blank values signal to ignore all previous values, allowing a reset of
++the list from broader config scenarios.
+
remote.<name>.followRemoteHEAD::
How linkgit:git-fetch[1] should handle updates to `remotes/<name>/HEAD`
@@ Documentation/fetch-options.adoc
@@ Documentation/fetch-options.adoc: is the same as for `--negotiation-restrict`.
If `--negotiation-restrict` is used, the have set is first restricted by
that option and then increased to include the tips specified by
- `--negotiation-require`.
+ `--negotiation-include`.
++
+If this option is not specified on the command line, then any
-+`remote.<name>.negotiationRequire` config values for the current remote
++`remote.<name>.negotiationInclude` config values for the current remote
+are used instead.
`--negotiate-only`::
@@ builtin/fetch.c
@@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remote, int deepen,
else
warning(_("ignoring %s because the protocol does not support it"),
- "--negotiation-require");
-+ } else if (remote->negotiation_require.nr) {
+ "--negotiation-include");
++ } else if (remote->negotiation_include.nr) {
+ if (transport->smart_options) {
-+ transport->smart_options->negotiation_require = &remote->negotiation_require;
++ transport->smart_options->negotiation_include = &remote->negotiation_include;
+ } else {
+ struct strbuf config_name = STRBUF_INIT;
-+ strbuf_addf(&config_name, "remote.%s.negotiationRequire", remote->name);
++ strbuf_addf(&config_name, "remote.%s.negotiationInclude", remote->name);
+ warning(_("ignoring %s because the protocol does not support it"),
+ config_name.buf);
+ strbuf_release(&config_name);
@@ remote.c: static struct remote *make_remote(struct remote_state *remote_state,
refspec_init_fetch(&ret->fetch);
string_list_init_dup(&ret->server_options);
string_list_init_dup(&ret->negotiation_restrict);
-+ string_list_init_dup(&ret->negotiation_require);
++ string_list_init_dup(&ret->negotiation_include);
ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1,
remote_state->remotes_alloc);
@@ remote.c: static void remote_clear(struct remote *remote)
FREE_AND_NULL(remote->http_proxy_authmethod);
string_list_clear(&remote->server_options, 0);
string_list_clear(&remote->negotiation_restrict, 0);
-+ string_list_clear(&remote->negotiation_require, 0);
++ string_list_clear(&remote->negotiation_include, 0);
}
static void add_merge(struct branch *branch, const char *name)
@@ remote.c: static int handle_config(const char *key, const char *value,
- if (!value)
- return config_error_nonbool(key);
- string_list_append(&remote->negotiation_restrict, value);
-+ } else if (!strcmp(subkey, "negotiationrequire")) {
-+ if (!value)
-+ return config_error_nonbool(key);
-+ string_list_append(&remote->negotiation_require, value);
+ string_list_clear(&remote->negotiation_restrict, 0);
+ else
+ string_list_append(&remote->negotiation_restrict, value);
++ } else if (!strcmp(subkey, "negotiationinclude")) {
++ /* reset list on empty value. */
++ if (!value || !*value)
++ string_list_clear(&remote->negotiation_include, 0);
++ else
++ string_list_append(&remote->negotiation_include, value);
} else if (!strcmp(subkey, "followremotehead")) {
const char *no_warn_branch;
if (!strcmp(value, "never"))
@@ remote.h: struct remote {
struct string_list server_options;
struct string_list negotiation_restrict;
-+ struct string_list negotiation_require;
++ struct string_list negotiation_include;
enum follow_remote_head_settings follow_remote_head;
const char *no_warn_branch;
## t/t5510-fetch.sh ##
-@@ t/t5510-fetch.sh: test_expect_success '--negotiation-require avoids duplicates with negotiator' '
+@@ t/t5510-fetch.sh: test_expect_success '--negotiation-include avoids duplicates with negotiator' '
test_line_count = 1 matches
'
-+test_expect_success 'remote.<name>.negotiationRequire used as default for --negotiation-require' '
++test_expect_success 'remote.<name>.negotiationInclude used as default for --negotiation-include' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
-+ git -C client config --add remote.origin.negotiationRequire refs/tags/beta_1 &&
++ # test the reset of the list on an empty value
++ git -C client config --add remote.origin.negotiationInclude refs/tags/alpha_1 &&
++ git -C client config --add remote.origin.negotiationInclude "" &&
++ git -C client config --add remote.origin.negotiationInclude refs/tags/beta_1 &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
+ origin alpha_s beta_s &&
@@ t/t5510-fetch.sh: test_expect_success '--negotiation-require avoids duplicates w
+ test_grep "fetch> have $BETA_1" trace
+'
+
-+test_expect_success 'remote.<name>.negotiationRequire works with glob patterns' '
++test_expect_success 'remote.<name>.negotiationInclude works with glob patterns' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
-+ git -C client config --add remote.origin.negotiationRequire "refs/tags/beta_*" &&
++ git -C client config --add remote.origin.negotiationInclude "refs/tags/beta_*" &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
+ origin alpha_s beta_s &&
@@ t/t5510-fetch.sh: test_expect_success '--negotiation-require avoids duplicates w
+ test_grep "fetch> have $BETA_2" trace
+'
+
-+test_expect_success 'CLI --negotiation-require overrides remote.<name>.negotiationRequire' '
++test_expect_success 'CLI --negotiation-include overrides remote.<name>.negotiationInclude' '
+ test_when_finished rm -f trace &&
+ setup_negotiation_tip server server 0 &&
+
-+ git -C client config --add remote.origin.negotiationRequire refs/tags/beta_2 &&
++ git -C client config --add remote.origin.negotiationInclude refs/tags/beta_2 &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+ --negotiation-restrict=alpha_1 \
-+ --negotiation-require=refs/tags/beta_1 \
++ --negotiation-include=refs/tags/beta_1 \
+ origin alpha_s beta_s &&
+
+ BETA_1=$(git -C client rev-parse beta_1) &&
7: 7cccf59beb ! 7: e6c79f0661 send-pack: pass negotiation config in push
@@ Commit message
When push.negotiate is enabled, 'git push' spawns a child 'git fetch
--negotiate-only' process to find common commits. Pass
- --negotiation-require and --negotiation-restrict options from the
- 'remote.<name>.negotiationRequire' and
+ --negotiation-include and --negotiation-restrict options from the
+ 'remote.<name>.negotiationInclude' and
'remote.<name>.negotiationRestrict' config keys to this child process.
When negotiationRestrict is configured, it replaces the default
behavior of using all remote refs as negotiation tips. This allows
the user to control which local refs are used for push negotiation.
- When negotiationRequire is configured, the specified ref patterns
- are passed as --negotiation-require to ensure their tips are always
+ When negotiationInclude is configured, the specified ref patterns
+ are passed as --negotiation-include to ensure their tips are always
sent as 'have' lines during push negotiation.
This change also updates the use of --negotiation-tip into
@@ send-pack.c: static void reject_invalid_nonce(const char *nonce, int len)
static void get_commons_through_negotiation(struct repository *r,
const char *url,
-+ const struct string_list *negotiation_require,
++ const struct string_list *negotiation_include,
+ const struct string_list *negotiation_restrict,
const struct ref *remote_refs,
struct oid_array *commons)
{
-@@ send-pack.c: static void get_commons_through_negotiation(struct repository *r,
+ struct child_process child = CHILD_PROCESS_INIT;
+ const struct ref *ref;
+ int len = r->hash_algo->hexsz + 1; /* hash + NL */
+- int nr_negotiation_tip = 0;
++ int nr_negotiation = 0;
+
+ child.git_cmd = 1;
child.no_stdin = 1;
child.out = -1;
strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
@@ send-pack.c: static void get_commons_through_negotiation(struct repository *r,
+ for_each_string_list_item(item, negotiation_restrict)
+ strvec_pushf(&child.args, "--negotiation-restrict=%s",
+ item->string);
-+ nr_negotiation_tip = negotiation_restrict->nr;
++ nr_negotiation = negotiation_restrict->nr;
+ } else {
+ for (ref = remote_refs; ref; ref = ref->next) {
+ if (!is_null_oid(&ref->new_oid)) {
-+ strvec_pushf(&child.args, "--negotiation-tip=%s",
++ strvec_pushf(&child.args, "--negotiation-restrict=%s",
+ oid_to_hex(&ref->new_oid));
-+ nr_negotiation_tip++;
++ nr_negotiation++;
+ }
}
}
+
-+ if (negotiation_require && negotiation_require->nr) {
++ if (negotiation_include && negotiation_include->nr) {
+ struct string_list_item *item;
-+ for_each_string_list_item(item, negotiation_require)
-+ strvec_pushf(&child.args, "--negotiation-require=%s",
++ for_each_string_list_item(item, negotiation_include)
++ strvec_pushf(&child.args, "--negotiation-include=%s",
+ item->string);
++ nr_negotiation += negotiation_include->nr;
+ }
+
strvec_push(&child.args, url);
- if (!nr_negotiation_tip) {
+- if (!nr_negotiation_tip) {
++ if (!nr_negotiation) {
+ child_process_clear(&child);
+ return;
+ }
@@ send-pack.c: int send_pack(struct repository *r,
repo_config_get_bool(r, "push.negotiate", &push_negotiate);
if (push_negotiate) {
trace2_region_enter("send_pack", "push_negotiate", r);
- get_commons_through_negotiation(r, args->url, remote_refs, &commons);
+ get_commons_through_negotiation(r, args->url,
-+ args->negotiation_require,
++ args->negotiation_include,
+ args->negotiation_restrict,
+ remote_refs, &commons);
trace2_region_leave("send_pack", "push_negotiate", r);
@@ send-pack.h: struct repository;
struct send_pack_args {
const char *url;
-+ const struct string_list *negotiation_require;
++ const struct string_list *negotiation_include;
+ const struct string_list *negotiation_restrict;
unsigned verbose:1,
quiet:1,
@@ t/t5516-fetch-push.sh: test_expect_success 'push with negotiation does not attem
! grep "Fetching submodule" err
'
-+test_expect_success 'push with negotiation and remote.<name>.negotiationRequire' '
-+ test_when_finished rm -rf negotiation_require &&
-+ mk_empty negotiation_require &&
-+ git push negotiation_require $the_first_commit:refs/remotes/origin/first_commit &&
-+ test_commit -C negotiation_require unrelated_commit &&
-+ git -C negotiation_require config receive.hideRefs refs/remotes/origin/first_commit &&
++test_expect_success 'push with negotiation and remote.<name>.negotiationInclude' '
++ test_when_finished rm -rf negotiation_include &&
++ mk_empty negotiation_include &&
++ git push negotiation_include $the_first_commit:refs/remotes/origin/first_commit &&
++ test_commit -C negotiation_include unrelated_commit &&
++ git -C negotiation_include config receive.hideRefs refs/remotes/origin/first_commit &&
+ test_when_finished "rm event" &&
+ GIT_TRACE2_EVENT="$(pwd)/event" \
+ git -c protocol.version=2 -c push.negotiate=1 \
-+ -c remote.negotiation_require.negotiationRequire=refs/heads/main \
-+ push negotiation_require refs/heads/main:refs/remotes/origin/main &&
++ -c remote.negotiation_include.negotiationInclude=refs/heads/main \
++ push negotiation_include refs/heads/main:refs/remotes/origin/main &&
+ test_grep \"key\":\"total_rounds\" event &&
+ grep_wrote 2 event # 1 commit, 1 tree
+'
@@ transport.c: static int git_transport_push(struct transport *transport, struct r
args.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);
args.push_options = transport->push_options;
args.url = transport->url;
-+ args.negotiation_require = &transport->remote->negotiation_require;
++ args.negotiation_include = &transport->remote->negotiation_include;
+ args.negotiation_restrict = &transport->remote->negotiation_restrict;
if (flags & TRANSPORT_PUSH_CERT_ALWAYS)
--
gitgitgadget
^ permalink raw reply
* [PATCH v3 1/7] t5516: fix test order flakiness
From: Derrick Stolee via GitGitGadget @ 2026-04-22 15:25 UTC (permalink / raw)
To: git; +Cc: gitster, ps, Derrick Stolee, Derrick Stolee
In-Reply-To: <pull.2085.v3.git.1776871546.gitgitgadget@gmail.com>
From: Derrick Stolee <stolee@gmail.com>
The 'fetch follows tags by default' test sorts using 'sort -k 4', but
for-each-ref output only has 3 columns. This relies on sort treating
records with fewer fields as having an empty fourth field, which may
produce unstable results depending on locale. Use 'sort -k 3' to match
the actual number of columns in the output.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
t/t5516-fetch-push.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 29e2f17608..ac8447f21e 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1349,7 +1349,7 @@ test_expect_success 'fetch follows tags by default' '
git for-each-ref >tmp1 &&
sed -n "p; s|refs/heads/main$|refs/remotes/origin/main|p" tmp1 |
sed -n "p; s|refs/heads/main$|refs/remotes/origin/HEAD|p" |
- sort -k 4 >../expect
+ sort -k 3 >../expect
) &&
test_when_finished "rm -rf dst" &&
git init dst &&
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/2] commit: sign commit after mutating buffer
From: Elijah Newren @ 2026-04-22 15:10 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Junio C Hamano, Kushal Das
In-Reply-To: <20260420221425.2763661-2-sandals@crustytoothpaste.net>
On Mon, Apr 20, 2026 at 3:14 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> The ensure_utf8 function can mutate the buffer to change its encoding,
> so we must call it before signing the buffer so that we do not
> invalidate the signature, which is made over raw bytes. Add a test for
> this case as well using 0xfe and 0xff, which are never valid in UTF-8.
>
> Reported-by: Kushal Das <kushal@sunet.se>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> commit.c | 12 ++++++++----
> t/t7510-signed-commit.sh | 8 ++++++++
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 790dd2faed..bc41859be1 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1747,6 +1747,11 @@ int commit_tree_extended(const char *msg, size_t msg_len,
> oidcpy(&parent_buf[i++], &p->item->object.oid);
>
> write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra);
> +
> + /* And check the encoding. */
> + if (encoding_is_utf8 && !ensure_utf8(&buffer))
> + fprintf(stderr, _(commit_utf8_warn));
> +
> if (sign_commit && sign_buffer(&buffer, &sig, sign_commit,
> SIGN_BUFFER_USE_DEFAULT_KEY)) {
> result = -1;
> @@ -1780,6 +1785,9 @@ int commit_tree_extended(const char *msg, size_t msg_len,
> free_commit_extra_headers(compat_extra);
> free(mapped_parents);
>
> + if (encoding_is_utf8 && !ensure_utf8(&compat_buffer))
> + fprintf(stderr, _(commit_utf8_warn));
> +
So the users might see "commit message did not conform to UTF-8..."
twice? (Isn't compat_buffer likely to have invalid UTF-8 whenever
buffer does?) Do we want to avoid that double printing?
> if (sign_commit && sign_buffer(&compat_buffer, &compat_sig,
> sign_commit,
> SIGN_BUFFER_USE_DEFAULT_KEY)) {
> @@ -1818,10 +1826,6 @@ int commit_tree_extended(const char *msg, size_t msg_len,
> }
> }
>
> - /* And check the encoding. */
> - if (encoding_is_utf8 && (!ensure_utf8(&buffer) || !ensure_utf8(&compat_buffer)))
> - fprintf(stderr, _(commit_utf8_warn));
> -
Did the change in this patch also fix a short-circuiting error?
Previously, when both buffers had invalid UTF-8, we'd only call
ensure_utf8() on the first one and fix it, and then short-circuit and
not handle compat_buffer, right?
> if (r->compat_hash_algo) {
> hash_object_file(r->compat_hash_algo, compat_buffer.buf, compat_buffer.len,
> OBJ_COMMIT, &compat_oid_buf);
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 1201c85ba6..071dbb3d39 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -462,4 +462,12 @@ test_expect_success 'custom `gpg.program`' '
> git commit -S --allow-empty -m signed-commit
> '
>
> +test_expect_success GPG 'commit verifies with non-UTF-8 commit message' '
> + printf "I hate\\376\\377UTF-8\\n" >message &&
> + echo unusual-message >file &&
> + git add file &&
> + test_tick && git commit -S -F message &&
> + git verify-commit HEAD
> +'
Nice test.
^ permalink raw reply
* Re: [PATCH 1/2] commit: name UTF-8 function appropriately
From: Elijah Newren @ 2026-04-22 15:10 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Junio C Hamano, Kushal Das
In-Reply-To: <20260420221425.2763661-1-sandals@crustytoothpaste.net>
On Mon, Apr 20, 2026 at 3:14 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> We have a function named verify_utf8, but it does more than verify, it
> modifies the buffer if it is not UTF-8. This is different from what
> most people would expect, so call the function ensure_utf8, since it
> mutates the buffer in some cases.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> commit.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 80d8d07875..790dd2faed 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1637,12 +1637,12 @@ static int find_invalid_utf8(const char *buf, int len)
> }
>
> /*
> - * This verifies that the buffer is in proper utf8 format.
> + * This ensures that the buffer is in proper utf8 format.
> *
> * If it isn't, it assumes any non-utf8 characters are Latin1,
> * and does the conversion.
> */
> -static int verify_utf8(struct strbuf *buf)
> +static int ensure_utf8(struct strbuf *buf)
> {
> int ok = 1;
> long pos = 0;
> @@ -1819,7 +1819,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,
> }
>
> /* And check the encoding. */
> - if (encoding_is_utf8 && (!verify_utf8(&buffer) || !verify_utf8(&compat_buffer)))
> + if (encoding_is_utf8 && (!ensure_utf8(&buffer) || !ensure_utf8(&compat_buffer)))
> fprintf(stderr, _(commit_utf8_warn));
>
> if (r->compat_hash_algo) {
Makes sense.
^ permalink raw reply
* Re: [PATCH 6/8] refs: move object parsing to the generic layer
From: Karthik Nayak @ 2026-04-22 15:03 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aeit7X0Q_MlxvPas@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Mon, Apr 20, 2026 at 12:12:04PM +0200, Karthik Nayak wrote:
>> Regular reference updates made via reference transactions validate that
>> the provided object ID exists in the object database, this is done by
>
> s/this/which/
>
>> calling 'parse_object()'. This check is done independently by the
>> backends.
>
> ..., which leads to duplicated logic.
Will add in.
>
>> 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.
>
> Right, this is what the preceding commits have allowed us to do.
>
> I think this is a good step. Being less entangled with the object
> database in the ref backends is a good thing.
>
> Patrick
Agreed. Thanks for the review.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v2 00/16] repack: incremental MIDX/bitmap-based repacking
From: Elijah Newren @ 2026-04-22 14:45 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1776803827.git.me@ttaylorr.com>
On Tue, Apr 21, 2026 at 1:37 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> [Note to the maintainer, this is rebased onto current 'master', which is
> 94f057755b7 (Git 2.54, 2026-04-19) at the time of writing.]
>
> This a modest-sized reroll of my series to implement the last remaining
> component of the incremental MIDX/bitmap-based repacking strategy that I
> have been working on.
>
> The main changes since last time are described in [1], which I sent to
> the list last week just before the 2.54 release was tagged. For
> convenience, a copy of the main changes are below:
>
> - Use a strset instead of a string_list for keeping track of the MIDX
> layers to retain when calling `either clear_midx_files_ext()` or
> `clear_incremental_midx_files_ext()`.
>
> - A new patch to rewrite the logic for determining which MIDX layers
> comprise the new chain via keep_hashes to build the array in order.
> The subsequent patch converts that into a strvec, which no longer
> requires direct manipulation.
>
> - The new "--checksum-only" option has been renamed to
> "--no-write-chain-file", and various small implementation tweaks
> (e.g., relying on `is_lock_file_locked()` to determine whether we
> should update the chain file as opposed to reading the flags).
>
> As usual, a range-diff is included below as well for convenience. Thanks
> in advance for reviewing!
[...]
> Range-diff against v1:
[...]
This round addresses all the (admittedly minor) pieces of feedback I
had on v1, and I didn't spot anything new to comment on in looking
over the changes in this round.
^ permalink raw reply
* Re: [PATCH 2/2] status: improve rebase todo list parsing
From: Phillip Wood @ 2026-04-22 14:15 UTC (permalink / raw)
To: Elijah Newren, Phillip Wood; +Cc: git
In-Reply-To: <CABPp-BFziRXjuMKqf=RHgCwuCcujXSSrz0f+BS4pvE6EUbk-WQ@mail.gmail.com>
Hi Elijah
Thanks for the review, all you suggestions look sensible to me, I'll
send a re-roll.
Phillip
On 22/04/2026 01:32, Elijah Newren wrote:
> On Mon, Apr 20, 2026 at 8:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When there is rebase in progress "git status" displays the last couple
>> of completed and the next couple of pending commands from the todo
>> list. When it does this is tries to abbreviate the object ids of
>
> is tries => it tries ?
>
> [...]
>> @@ -1363,6 +1363,51 @@ static int split_commit_in_progress(struct wt_status *s)
>> free(rebase_orig_head);
>>
>> return split_in_progress;
>> +}
>> +
>> +static void abbrev_oid_in_line(struct repository *r,
>> + struct strbuf *line, char **pp)
>> +{
>> + char *p = *pp;
>> + char *end_of_object_name, saved;
>> + const char *abbrev;
>> + struct object_id oid;
>> + bool have_oid;
>
> I'll put "thinking out loud" text in square brackets below...
>
>> +
>> + p += strspn(p, " \t");
>> + end_of_object_name = p + strcspn(p, " \t");
>
> [Advances p after whitespace, marks the end of the object with the
> next whitespace after that.]
>
>> + /*
>> + * The for "merge" and "reset" the object name may be a label or
>
> The for => For ?
>
>> + * ref rather than a hex object id. Only abbreviate the object
>> + * name if it is a hex object id.
>> + */
>> + for (const char *q = p; q < end_of_object_name; q++) {
>> + if (!isxdigit(*q))
>> + goto out;
>> + }
>
>
>
>> + saved = *end_of_object_name;
>> + *end_of_object_name = '\0';
>> + have_oid = !repo_get_oid(r, p, &oid);
>> + *end_of_object_name = saved;
>
> [Tries to resolve the token, doing NUL-termination and restore dance.]
>
>> + if (!have_oid)
>> + goto out; /* object name was a label */
>
>
>> + abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
>> + if (!starts_with(p, abbrev))
>> + goto out; /* object name was a refname containing only xdigits */
>
> [Ensures what we have is an oid rather than a branch name that can be
> resolved to an oid]
>
>> + p += strlen(abbrev);
>> + strbuf_remove(line, p - line->buf, end_of_object_name - p);
>> + end_of_object_name = p;
>
> [Splice out a bunch of characters in the middle?]
>
>> +out:
>> + *pp = end_of_object_name;
>> +}
>
> I had a hard time following the logic in the function and trying to
> figure out what it was doing. I went line by line but had no mental
> model to follow. When I got to the comment that is now above
> format_todo_line(), I suddenly understood, but without it, all the
> code was hard to follow. Maybe a small comment at the beginning of
> the function along the lines of
>
> /*
> * If the whitespace-delimited token starting at or just after *pp is a
> * full hex object id that resolves uniquely, rewrite it in place to
> * its default abbreviation, shrinking `line` accordingly. On return
> * *pp points one past the (possibly abbreviated) token. Leaves both
> * `line` and *pp-advanced-past-the-token unchanged in all other cases
> * (non-hex token, unresolvable, or a refname that happens to consist
> * only of hex digits).
> */
>
> ? (Assuming I'm understanding correctly, of course.)
>
>> +
>> +static void skip_dash_c(char **pp) {
>
> Move the brace to the next line?
>
>> + char *p = *pp;
>> +
>> + p += strspn(p, " \t");
>> + /* The (void) cast is required to silence -Wunused_value */
>
> -Wunused_value => -Wunused-value ?
>
>> + (void)(skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p));
>> + *pp = p;
>> }
>>
>> /*
>> @@ -1371,29 +1416,57 @@ static int split_commit_in_progress(struct wt_status *s)
>> * into
>> * "pick d6a2f03 some message"
>> *
>> - * The function assumes that the line does not contain useless spaces
>> - * before or after the command.
>> + * Returns false on comment lines, true otherwise
>> */
>> -static void abbrev_oid_in_line(struct repository *r, struct strbuf *line)
>> +static bool format_todo_line(struct repository *r, struct strbuf *line)
>> {
>> - struct string_list split = STRING_LIST_INIT_DUP;
>> - struct object_id oid;
>> -
>> - if (starts_with(line->buf, "exec ") ||
>> - starts_with(line->buf, "x ") ||
>> - starts_with(line->buf, "label ") ||
>> - starts_with(line->buf, "l "))
>> - return;
>> -
>> - if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
>> - !repo_get_oid(r, split.items[1].string, &oid)) {
>> - strbuf_reset(line);
>> - strbuf_addf(line, "%s ", split.items[0].string);
>> - strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
>> - for (size_t i = 2; i < split.nr; i++)
>> - strbuf_addf(line, " %s", split.items[i].string);
>> + enum todo_command cmd;
>> + char *p = line->buf;
>> +
>> + if (!sequencer_parse_todo_command((const char**)&p, &cmd))
>> + return true; /* keep invalid lines */
>> +
>> + switch (cmd) {
>> + case TODO_COMMENT:
>> + return false;
>> +
>> + case TODO_MERGE:
>> + skip_dash_c(&p);
>> + while (true) {
>> + p += strspn(p, " \t");
>> + if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
>> + break;
>> + abbrev_oid_in_line(r, line, &p);
>> + }
>> + break;
>> +
>> + case TODO_FIXUP:
>> + skip_dash_c(&p);
>> + /* fallthrough */
>> + case TODO_DROP:
>> + case TODO_EDIT:
>> + case TODO_PICK:
>> + case TODO_RESET:
>> + case TODO_REVERT:
>> + case TODO_REWORD:
>> + case TODO_SQUASH:
>> + abbrev_oid_in_line(r, line, &p);
>> + break;
>> +
>> + /*
>> + * Avoid "default" and instead list all the other commands so
>> + * that -Wswitch warns if a new command is added without handling
>> + * it in this function.
>> + */
>
> Nice. :-)
>
>> + case TODO_BREAK:
>> + case TODO_EXEC:
>> + case TODO_LABEL:
>> + case TODO_NOOP:
>> + case TODO_UPDATE_REF:
>> + break;
>> }
>> - string_list_clear(&split, 0);
>> +
>> + return true;
>> }
>>
>> static int read_rebase_todolist(struct repository *r, const char *fname, struct string_list *lines)
>> @@ -1411,13 +1484,9 @@ static int read_rebase_todolist(struct repository *r, const char *fname, struct
>> repo_git_path_replace(r, &buf, "%s", fname));
>> }
>> while (!strbuf_getline_lf(&buf, f)) {
>> - if (starts_with(buf.buf, comment_line_str))
>> - continue;
>> strbuf_trim(&buf);
>> - if (!buf.len)
>> - continue;
>> - abbrev_oid_in_line(r, &buf);
>> - string_list_append(lines, buf.buf);
>> + if (format_todo_line(r, &buf))
>> + string_list_append(lines, buf.buf);
>> }
>> fclose(f);
>>
>> --
>> 2.54.0.rc1.174.gd833f386ac5.dirty
>
> Other than the minor comments above, this looks like a nice cleanup.
^ permalink raw reply
* Re: [PATCH 2/2] status: improve rebase todo list parsing
From: Phillip Wood @ 2026-04-22 14:14 UTC (permalink / raw)
To: Patrick Steinhardt, Elijah Newren; +Cc: Phillip Wood, git
In-Reply-To: <aejM4EY29MGht5or@pks.im>
On 22/04/2026 14:28, Patrick Steinhardt wrote:
> On Tue, Apr 21, 2026 at 05:32:21PM -0700, Elijah Newren wrote:
>> On Mon, Apr 20, 2026 at 8:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>> + /*
>>> + * Avoid "default" and instead list all the other commands so
>>> + * that -Wswitch warns if a new command is added without handling
>>> + * it in this function.
>>> + */
>>
>> Nice. :-)
>
> Do we actually use -Wswitch anywhere? A quick grep in our code base
> didn't surface it, so I'm a bit sceptical that we would actually detect
> any missing cases via CI.
I've just double checked the gcc docs and it's included in -Wall (I'm
pretty sure I tried deleting one of the case statements to check before
I submitted the patch)
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH 5/8] update-ref: handle rejections while adding updates
From: Karthik Nayak @ 2026-04-22 14:13 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aeit6Ic4fUa3Xx1Z@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4307 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Mon, Apr 20, 2026 at 12:12:03PM +0200, Karthik Nayak wrote:
>> 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 committing of the
>
> Nit: s/committing/commit phase/
>
Will do.
>> transaction.
>>
>> In the following commits, we'll also extend `ref_transaction_update()`
>> to reject updates before even a transaction is prepared/committed. In
>
> The "even" reads like it's at the wrong position.
>
Yeah seems like a filler, will remove.
>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
>> index 5259cc7226..d1980c60c4 100644
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -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)
>
> Here you follow my earlier suggestion of using `_options` for the struct
> name, but `opts` for the variable :)
>
Haha, indeed, seems like I learn very quickly.
>> @@ -289,11 +300,18 @@ 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);
>> +
>> + if (tx_err && tx_err != REF_TRANSACTION_ERROR_GENERIC &&
>> + opts->allow_update_failures)
>> + print_rejected_refs(refname, have_old ? &old_oid : NULL,
>> + &new_oid, NULL, NULL, tx_err, err.buf,
>> + NULL);
>> + else if (tx_err)
>> die("%s", err.buf);
>>
>> update_flags = default_flags;
>
> I think this could use some curly braces to become easier to read, even
> if it's only single-line statements.
That's fair.
> Does this change have an impact on the ordering the user sees for
> printed errors? If so, it might make sense to point that out in the
> commit message.
>
I didn't consider that, it will be different since we already output to
the user before the transaction is prepared/committed.
Will add.
>> @@ -341,12 +361,19 @@ 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);
>> +
>> + if (tx_err && tx_err != REF_TRANSACTION_ERROR_GENERIC &&
>> + 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);
>> + else if (tx_err)
>> die("%s", err.buf);
>>
>> update_flags = default_flags;
>
> Same here, curly braces might help readability.
>
Will do.
>> @@ -644,6 +680,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)
>
> Suggestion, please feel free to ignore: we could also pass the flags
> directly instead.
>
> Patrick
Yeah we could, I deliberately avoided it to make the easily extensible
in the future. If you feel strongly, happy to make the change.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v2] generate-configlist: collapse depfile for older Ninja
From: Phillip Wood @ 2026-04-22 14:12 UTC (permalink / raw)
To: Toon Claes, git; +Cc: D. Ben Knoble, Patrick Steinhardt
In-Reply-To: <0557838b-214d-4e8f-9cbd-bc342563e9ba@gmail.com>
On 22/04/2026 14:45, Phillip Wood wrote:
>
> sed -e "s/second/second \\\n foo/" patch1 >patchnl &&
Of course the extra backslashes would supress any special meaning of
'\n' so that's not a good example. However the freebsd man page [1] says
The escape sequence \n matches a newline character embedded in the
pattern space. You cannot, however, use a literal newline character
in an address or in the substitute command.
Thanks
Phillip
[1]
https://man.freebsd.org/cgi/man.cgi?query=sed&apropos=0&sektion=0&manpath=FreeBSD+16.0-CURRENT&format=html
>
> However if I add "cat patchnl" it shows the subject line is
>
> Subject: [PATCH] second \n foo
>
> so sed has inserted "\n" rather than a newline. Indeed looking at the
> commit message for that test it is testing a fix that c escapes are
> printed verbatim introduced by 4b7cc26a74 (git-am: use printf instead of
> echo on user-supplied strings, 2007-05-25).
>
> I've not tested it but I think
>
> sed 's/ $/\
> /'
>
> will insert a newline. Alternatively we could do
>
> printf '%s' "$QUOTED_OUTPUT: "
> printf '%s\n' "$SOURCE_DIR"/Documentation/*config.adoc \
> "$SOURCE_DIR"/Documentation/config/*.adoc |
> sed -e 's/[# ]/\\&/g' |
> tr '\n' ' '
> printf '\n'
>
> That leaves a trailing space at the end of the line but I don't think
> that should matter.
>
> As I recall, the depfiles created by gcc have all the dependencies on a
> single line so this should be widely supported and I agree with Patrick
> that we should do this unconditionally.
>
> Thanks
>
> Phillip
>
> [1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
>
>> printf '%s:\n' "$SOURCE_DIR"/Documentation/*config.adoc \
>> "$SOURCE_DIR"/Documentation/config/*.adoc |
>> sed -e 's/[# ]/\\&/g'
>>
>> ---
>> base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
>> change-id: 20260421-toon-fix-almalinux8-102de9138294
>>
>>
>
^ permalink raw reply
* Re: [PATCH v2] generate-configlist: collapse depfile for older Ninja
From: Phillip Wood @ 2026-04-22 13:45 UTC (permalink / raw)
To: Toon Claes, git; +Cc: D. Ben Knoble, Patrick Steinhardt
In-Reply-To: <20260422-toon-fix-almalinux8-v2-1-45d8471ed0e9@iotcl.com>
Hi Toon
On 22/04/2026 08:21, Toon Claes wrote:
Thanks for the excellent commit message which I've trimmed.
> diff --git a/tools/generate-configlist.sh b/tools/generate-configlist.sh
> index e28054f9e0..f5f42492c6 100755
> --- a/tools/generate-configlist.sh
> +++ b/tools/generate-configlist.sh
> @@ -44,7 +44,9 @@ then
> {
> printf '%s\n' "$SOURCE_DIR"/Documentation/*config.adoc \
> "$SOURCE_DIR"/Documentation/config/*.adoc |
> - sed -e 's/[# ]/\\&/g' -e "s/^/$QUOTED_OUTPUT: /"
> + sed -e 's/[# ]/\\&/g' |
> + tr '\n' ' ' |
> + sed -e "s/^/$QUOTED_OUTPUT: /" -e 's/ $/\n/'
I don't think this use of '\n' portable. The sed man page [1] says that
'\n' matches a newline in the pattern space, but does not mention it
being supported in the replacement string. We do have an existing use in
t4150-am.sh:"am newline in subject" which does
sed -e "s/second/second \\\n foo/" patch1 >patchnl &&
However if I add "cat patchnl" it shows the subject line is
Subject: [PATCH] second \n foo
so sed has inserted "\n" rather than a newline. Indeed looking at the
commit message for that test it is testing a fix that c escapes are
printed verbatim introduced by 4b7cc26a74 (git-am: use printf instead of
echo on user-supplied strings, 2007-05-25).
I've not tested it but I think
sed 's/ $/\
/'
will insert a newline. Alternatively we could do
printf '%s' "$QUOTED_OUTPUT: "
printf '%s\n' "$SOURCE_DIR"/Documentation/*config.adoc \
"$SOURCE_DIR"/Documentation/config/*.adoc |
sed -e 's/[# ]/\\&/g' |
tr '\n' ' '
printf '\n'
That leaves a trailing space at the end of the line but I don't think
that should matter.
As I recall, the depfiles created by gcc have all the dependencies on a
single line so this should be widely supported and I agree with Patrick
that we should do this unconditionally.
Thanks
Phillip
[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
> printf '%s:\n' "$SOURCE_DIR"/Documentation/*config.adoc \
> "$SOURCE_DIR"/Documentation/config/*.adoc |
> sed -e 's/[# ]/\\&/g'
>
> ---
> base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
> change-id: 20260421-toon-fix-almalinux8-102de9138294
>
>
^ permalink raw reply
* Re: [PATCH 2/2] status: improve rebase todo list parsing
From: Patrick Steinhardt @ 2026-04-22 13:28 UTC (permalink / raw)
To: Elijah Newren; +Cc: Phillip Wood, git, Phillip Wood
In-Reply-To: <CABPp-BFziRXjuMKqf=RHgCwuCcujXSSrz0f+BS4pvE6EUbk-WQ@mail.gmail.com>
On Tue, Apr 21, 2026 at 05:32:21PM -0700, Elijah Newren wrote:
> On Mon, Apr 20, 2026 at 8:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > + /*
> > + * Avoid "default" and instead list all the other commands so
> > + * that -Wswitch warns if a new command is added without handling
> > + * it in this function.
> > + */
>
> Nice. :-)
Do we actually use -Wswitch anywhere? A quick grep in our code base
didn't surface it, so I'm a bit sceptical that we would actually detect
any missing cases via CI.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH 4/8] update-ref: move `print_rejected_refs()` up
From: Karthik Nayak @ 2026-04-22 13:16 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aeit3RVFhX680NfZ@pks.im>
[-- Attachment #1: Type: text/plain, Size: 320 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Mon, Apr 20, 2026 at 12:12:02PM +0200, Karthik Nayak wrote:
>> The `print_rejected_refs()` is used to print any rejected refs when
>
> Seems like a word is missing here. Maybe "The `print_rejected_refs()`
> function"?
>
> Patrick
Yeah adding 'function' reads better. Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 3/8] refs: return `ref_transaction_error` from `ref_transaction_update()`
From: Karthik Nayak @ 2026-04-22 13:14 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aeit12h5l34Wkon-@pks.im>
[-- Attachment #1: Type: text/plain, Size: 897 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Mon, Apr 20, 2026 at 12:12:01PM +0200, Karthik Nayak wrote:
>> The `ref_transaction_update()` function is used to add updates to a
>> given reference transactions. In the following commit, we'll add more
>> validation to this function. As such, it would be more beneficial if the
>
> s/more beneficial/beneficial/
>
Will do.
>> function returns specific error types, so callers can differentiate
>> between different errors.
>>
>> To facilitate this, return `enum ref_transaction_error` from the
>> function and covert the existing '-1' returns to
>> 'REF_TRANSACTION_ERROR_GENERIC'. Since this retains the existing
>> behavior, no changes are made to any of the callers but this sets the
>> necessary infrastructure for introduction of other errors.
>
> Yup, makes sense. This doesn't buy us anything yet, but will eventually.
>
> Patrick
Exactly!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [BUG] git-credential-libsecret writes secret to stdout on store
From: Phillip Wood @ 2026-04-22 13:13 UTC (permalink / raw)
To: Mantas Mikulėnas, Lutz-Christian Quander; +Cc: git
In-Reply-To: <0b2370ed-f3e1-4011-8a2c-8da539759881@gmail.com>
On 22/04/2026 06:49, Mantas Mikulėnas wrote:
>> 2. The direct-invocation pattern shows up widely in distro docs,
>> StackOverflow answers, and automation scripts -- empirically the
>> "internal protocol" boundary is porous. Fixing the helper is one
>> line; documenting the internal boundary across the ecosystem is
>> not.
>>
>> If the preferred answer is instead "users should only use
>> `git credential approve`", that would also work for me, but it may
>> deserve a note in gitcredentials(7) to steer people away from the
>> direct pattern -- the current docs don't actively discourage it.
Yes, users should be using "git credential", not be running the helpers
directly. That's why the helpers are installed in a directory that is
not in $PATH. gitcredentials(7) shows how to set the config setting used
by "git credential", as far as I can see it does not suggest that users
should be running the helpers directly.
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH 2/8] refs: extract out reflog config to generic layer
From: Karthik Nayak @ 2026-04-22 13:13 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aeit0pw44IxBfc2J@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Mon, Apr 20, 2026 at 12:12:00PM +0200, Karthik Nayak wrote:
>> The reference backends need to know when to create reflog entries, this
>
> s/this/which/
>
Better.
>> is dictated by the 'core.logallrefupdates' config. Instead of relying on
>
> s/dictated/controlled/
>
Both of those mean the same. I guess there is a negative connotation to
using the word 'dictated', but that's present with 'controlled' too.
Perhaps 'determined'?
>> the backends to call `repo_settings_get_log_all_ref_updates()` to obtain
>> this config value, let's do this in the generic layer and pass down the
>> value to the backends.
>>
>> Instead of passing this in as a new argument, let's create a new
>> `ref_init_options` structure which will house information required to
>> initialize a reference backend. Move the access flags here as well.
>
> I agree with this direction. It's also something that I'm doing for many
> callbacks in the ODB layer, and I'm moving more and more into that
> direction.
>
>> diff --git a/refs.c b/refs.c
>> index bfcb9c7ac3..aa66c6b28e 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2295,6 +2295,10 @@ static struct ref_store *ref_store_init(struct repository *repo,
>> {
>> const struct ref_storage_be *be;
>> struct ref_store *refs;
>> + struct ref_store_init_options options = {
>> + .access_flags = flags,
>> + .log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo),
>> + };
>>
>> be = find_ref_storage_backend(format);
>> if (!be)
>
> Tiniest nit, please feel free to ignore: we often call the structure
> itself `_options`, but the variables just `opts`. May just be my own
> preference though.
>
Let's make it consistent, I'll also start using `opts`.
>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> index 2d963cc4f4..eed13af4eb 100644
>> --- a/refs/refs-internal.h
>> +++ b/refs/refs-internal.h
>> @@ -385,6 +385,21 @@ struct ref_store;
>> REF_STORE_ODB | \
>> REF_STORE_MAIN)
>>
>> +/*
>> + * Options for initializing the ref backend. All backend-agnostic information
>> + * which backends required will be held here.
>> + */
>> +struct ref_store_init_options {
>> + /* The kind of operations that the ref_store is allowed to perform. */
>> + unsigned int access_flags;
>> +
>> + /*
>> + * Denotes under what conditions reflogs should be created when updating
>> + * references.
>> + */
>> + enum log_refs_config log_all_ref_updates;
>> +};
>
> Nit: it might've made sense to split this up into two steps: the
> introduction of the struct, and then moving the config in there.
>
> Patrick
Yeah that might be better. I'll go ahead and do that.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 1/8] refs: remove unused typedef 'ref_transaction_commit_fn'
From: Karthik Nayak @ 2026-04-22 12:20 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aeityL_05KDRZF98@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Mon, Apr 20, 2026 at 12:11:59PM +0200, Karthik Nayak wrote:
>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> index d79e35fd26..2d963cc4f4 100644
>> --- a/refs/refs-internal.h
>> +++ b/refs/refs-internal.h
>> @@ -421,10 +421,6 @@ typedef int ref_transaction_abort_fn(struct ref_store *refs,
>> struct ref_transaction *transaction,
>> struct strbuf *err);
>>
>> -typedef int ref_transaction_commit_fn(struct ref_store *refs,
>> - struct ref_transaction *transaction,
>> - struct strbuf *err);
>> -
>> typedef int optimize_fn(struct ref_store *ref_store,
>> struct refs_optimize_opts *opts);
>>
>
> I'm in general not much of a fan of these typedefs -- there's not really
> much of a point why we'd need them in the first place. We don't use them
> as a type anywhere but in the struct definition for the ref backend. So
> we could just as well move them in there, which would also ensure that
> they cannot become stale in the first place.
>
> Patrick
I do like them, when reading `struct ref_storage_be {}` with these
typedefs, the interface is simplified and I only need to know what are
the different functions which need to be implemented without being
exposed to the details of each of the functions. The details are
available in the typedefs.
I'll leave it out of this series though.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* [PATCH v5 10/10] doc: document autocorrect API
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Explain behaviors for autocorrect_resolve(), autocorrect_confirm(), and
struct autocorrect.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
autocorrect.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/autocorrect.h b/autocorrect.h
index 14ee7c4548d3..5bb67cf6debd 100644
--- a/autocorrect.h
+++ b/autocorrect.h
@@ -13,13 +13,24 @@ enum autocorrect_mode {
AUTOCORRECT_DELAY,
};
+/**
+ * `mode` indicates which action will be performed by autocorrect_confirm().
+ * `delay` is the timeout before autocorrect_confirm() returns, in tenths of a
+ * second. Use it only with AUTOCORRECT_DELAY.
+ */
struct autocorrect {
enum autocorrect_mode mode;
int delay;
};
+/**
+ * Resolve the autocorrect configuration into `conf`.
+ */
void autocorrect_resolve(struct autocorrect *conf);
+/**
+ * Interact with the user in different ways depending on `conf->mode`.
+ */
void autocorrect_confirm(struct autocorrect *conf, const char *assumed);
#endif /* AUTOCORRECT_H */
--
2.53.0
^ permalink raw reply related
* [PATCH v5 09/10] parseopt: add tests for subcommand autocorrection
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
These tests cover default behavior (help.autocorrect is unset), no
correction, immediate correction, delayed correction, and rejection
when the typo is too dissimilar.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
t/meson.build | 1 +
t/t9004-autocorrect-subcommand.sh | 58 +++++++++++++++++++++++++++++++
2 files changed, 59 insertions(+)
create mode 100755 t/t9004-autocorrect-subcommand.sh
diff --git a/t/meson.build b/t/meson.build
index 7528e5cda5fe..871cd7ab0a08 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -980,6 +980,7 @@ integration_tests = [
't9001-send-email.sh',
't9002-column.sh',
't9003-help-autocorrect.sh',
+ 't9004-autocorrect-subcommand.sh',
't9100-git-svn-basic.sh',
't9101-git-svn-props.sh',
't9102-git-svn-deep-rmdir.sh',
diff --git a/t/t9004-autocorrect-subcommand.sh b/t/t9004-autocorrect-subcommand.sh
new file mode 100755
index 000000000000..09d281bd105b
--- /dev/null
+++ b/t/t9004-autocorrect-subcommand.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='subcommand auto-correction test
+
+Test autocorrection for subcommands with different
+help.autocorrect mode.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' "
+ echo '^error: unknown subcommand: ' >grep_unknown
+"
+
+test_expect_success 'default is show hint only' '
+ test_must_fail git worktree lsit 2>actual &&
+ test_grep "most similar subcommand" actual
+'
+
+test_expect_success "'never' disables autocorrection" '
+ test_config help.autocorrect never &&
+
+ test_must_fail git worktree lsit 2>actual &&
+ head -n1 actual >first && test_grep -f grep_unknown first
+'
+
+for mode in false no off 0 show
+do
+ test_expect_success "'$mode' disables autocorrection and shows hints" "
+ test_config help.autocorrect $mode &&
+
+ test_must_fail git worktree lsit 2>actual &&
+ test_grep 'most similar subcommand' actual
+ "
+done
+
+for mode in -39 immediate 1
+do
+ test_expect_success "autocorrect immediately with '$mode'" - <<-EOT
+ test_config help.autocorrect $mode &&
+
+ git worktree lsit 2>actual &&
+ test_grep "you meant 'list'\.$" actual
+ EOT
+done
+
+test_expect_success 'delay path is executed' - <<-\EOT
+ test_config help.autocorrect 2 &&
+
+ git worktree lsit 2>actual &&
+ test_grep '^Continuing in 0.2 seconds, ' actual
+EOT
+
+test_expect_success 'deny if too dissimilar' - <<-\EOT
+ test_must_fail git remote rensnr 2>actual &&
+ head -n1 actual >first && test_grep -f grep_unknown first
+EOT
+
+test_done
--
2.53.0
^ permalink raw reply related
* [PATCH v5 08/10] parseopt: enable subcommand autocorrection for git-remote and git-notes
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Add PARSE_OPT_SUBCOMMAND_AUTOCORRECT to enable autocorrection for
subcommands parsed with PARSE_OPT_SUBCOMMAND_OPTIONAL.
Apply this to git-remote and git-notes, so mistyped subcommands can be
automatically corrected, and builtin entry points no longer need to
handle the unknown subcommand error path themselves.
This is safe. Both builtins either resolve to a single subcommand or
take no subcommand at all, meaning any unknown argument encountered by
the parser must be a mistyped subcommand.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
builtin/notes.c | 10 +++-------
builtin/remote.c | 12 ++++--------
parse-options.c | 16 +++++++++-------
parse-options.h | 1 +
4 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 9af602bdd7b4..f9bf350df4e8 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -1149,14 +1149,10 @@ int cmd_notes(int argc,
repo_config(the_repository, git_default_config, NULL);
argc = parse_options(argc, argv, prefix, options, git_notes_usage,
- PARSE_OPT_SUBCOMMAND_OPTIONAL);
- if (!fn) {
- if (argc) {
- error(_("unknown subcommand: `%s'"), argv[0]);
- usage_with_options(git_notes_usage, options);
- }
+ PARSE_OPT_SUBCOMMAND_OPTIONAL |
+ PARSE_OPT_SUBCOMMAND_AUTOCORRECT);
+ if (!fn)
fn = list;
- }
if (override_notes_ref) {
struct strbuf sb = STRBUF_INIT;
diff --git a/builtin/remote.c b/builtin/remote.c
index de989ea3ba96..6a78ab8f4cd2 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1953,15 +1953,11 @@ int cmd_remote(int argc,
};
argc = parse_options(argc, argv, prefix, options, builtin_remote_usage,
- PARSE_OPT_SUBCOMMAND_OPTIONAL);
+ PARSE_OPT_SUBCOMMAND_OPTIONAL |
+ PARSE_OPT_SUBCOMMAND_AUTOCORRECT);
- if (fn) {
+ if (fn)
return !!fn(argc, argv, prefix, repo);
- } else {
- if (argc) {
- error(_("unknown subcommand: `%s'"), argv[0]);
- usage_with_options(builtin_remote_usage, options);
- }
+ else
return !!show_all();
- }
}
diff --git a/parse-options.c b/parse-options.c
index a488f9a41df8..820683e982c1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -720,14 +720,16 @@ static enum parse_opt_result handle_subcommand(struct parse_opt_ctx_t *ctx,
if (!err)
return PARSE_OPT_SUBCOMMAND;
- /*
- * arg is neither a short or long option nor a subcommand. Since this
- * command has a default operation mode, we have to treat this arg and
- * all remaining args as args meant to that default operation mode.
- * So we are done parsing.
- */
- if (ctx->flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)
+ if (ctx->flags & PARSE_OPT_SUBCOMMAND_OPTIONAL &&
+ !(ctx->flags & PARSE_OPT_SUBCOMMAND_AUTOCORRECT)) {
+ /*
+ * arg is neither a short or long option nor a subcommand.
+ * Since this command has a default operation mode, we have to
+ * treat this arg and all remaining args as args meant to that
+ * default operation mode. So we are done parsing.
+ */
return PARSE_OPT_DONE;
+ }
find_subcommands(&cmds, options);
assumed = autocorrect_subcommand(arg, &cmds);
diff --git a/parse-options.h b/parse-options.h
index 706de9729f6b..e5fd4da4055b 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -40,6 +40,7 @@ enum parse_opt_flags {
PARSE_OPT_ONE_SHOT = 1 << 5,
PARSE_OPT_SHELL_EVAL = 1 << 6,
PARSE_OPT_SUBCOMMAND_OPTIONAL = 1 << 7,
+ PARSE_OPT_SUBCOMMAND_AUTOCORRECT = 1 << 8,
};
enum parse_opt_option_flags {
--
2.53.0
^ permalink raw reply related
* [PATCH v5 07/10] parseopt: autocorrect mistyped subcommands
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Try to autocorrect the mistyped mandatory subcommand before showing an
error and exiting. Subcommands parsed with PARSE_OPT_SUBCOMMAND_OPTIONAL
are skipped.
The subcommand autocorrection behaves the same as the command
autocorrection.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
autocorrect.h | 4 ++
help.c | 13 ++----
parse-options.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 107 insertions(+), 12 deletions(-)
diff --git a/autocorrect.h b/autocorrect.h
index 0d3e819262ed..14ee7c4548d3 100644
--- a/autocorrect.h
+++ b/autocorrect.h
@@ -1,6 +1,10 @@
#ifndef AUTOCORRECT_H
#define AUTOCORRECT_H
+/* An empirically derived magic number */
+#define AUTOCORRECT_SIMILARITY_FLOOR 7
+#define AUTOCORRECT_SIMILAR_ENOUGH(x) ((x) < AUTOCORRECT_SIMILARITY_FLOOR)
+
enum autocorrect_mode {
AUTOCORRECT_HINT,
AUTOCORRECT_NEVER,
diff --git a/help.c b/help.c
index 0fff43545cc1..ecc5673b7243 100644
--- a/help.c
+++ b/help.c
@@ -580,10 +580,6 @@ static void add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
old->cnt = 0;
}
-/* An empirically derived magic number */
-#define SIMILARITY_FLOOR 7
-#define SIMILAR_ENOUGH(x) ((x) < SIMILARITY_FLOOR)
-
static const char bad_interpreter_advice[] =
N_("'%s' appears to be a git command, but we were not\n"
"able to execute it. Maybe git-%s is broken?");
@@ -659,7 +655,7 @@ char *help_unknown_cmd(const char *cmd)
if (main_cmds.cnt <= n) {
/* prefix matches with everything? that is too ambiguous */
- best_similarity = SIMILARITY_FLOOR + 1;
+ best_similarity = AUTOCORRECT_SIMILARITY_FLOOR + 1;
} else {
/* count all the most similar ones */
for (best_similarity = main_cmds.names[n++]->len;
@@ -670,7 +666,7 @@ char *help_unknown_cmd(const char *cmd)
}
if (autocorrect.mode != AUTOCORRECT_HINT && n == 1 &&
- SIMILAR_ENOUGH(best_similarity)) {
+ AUTOCORRECT_SIMILAR_ENOUGH(best_similarity)) {
char *assumed = xstrdup(main_cmds.names[0]->name);
fprintf_ln(stderr,
@@ -687,11 +683,10 @@ char *help_unknown_cmd(const char *cmd)
fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);
- if (SIMILAR_ENOUGH(best_similarity)) {
+ if (AUTOCORRECT_SIMILAR_ENOUGH(best_similarity)) {
fprintf_ln(stderr,
Q_("\nThe most similar command is",
- "\nThe most similar commands are",
- n));
+ "\nThe most similar commands are", n));
for (i = 0; i < n; i++)
fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
diff --git a/parse-options.c b/parse-options.c
index 803ce2ba4443..a488f9a41df8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -7,6 +7,8 @@
#include "string-list.h"
#include "strmap.h"
#include "utf8.h"
+#include "autocorrect.h"
+#include "levenshtein.h"
static int disallow_abbreviated_options;
@@ -623,13 +625,98 @@ static int parse_subcommand(const char *arg, const struct option *options)
return -1;
}
+static void find_subcommands(struct string_list *list,
+ const struct option *options)
+{
+ for (; options->type != OPTION_END; options++) {
+ if (options->type == OPTION_SUBCOMMAND)
+ string_list_append(list, options->long_name);
+ }
+}
+
+static int levenshtein_compare(const void *p1, const void *p2)
+{
+ const struct string_list_item *i1 = p1, *i2 = p2;
+ const char *s1 = i1->string, *s2 = i2->string;
+ int l1 = (intptr_t)i1->util;
+ int l2 = (intptr_t)i2->util;
+
+ return l1 != l2 ? l1 - l2 : strcmp(s1, s2);
+}
+
+static const char *autocorrect_subcommand(const char *cmd,
+ struct string_list *cmds)
+{
+ struct autocorrect autocorrect = { 0 };
+ unsigned int n = 0, best = 0;
+ struct string_list_item *cand;
+
+ autocorrect_resolve(&autocorrect);
+
+ if (autocorrect.mode == AUTOCORRECT_NEVER)
+ return NULL;
+
+ for_each_string_list_item(cand, cmds) {
+ if (starts_with(cand->string, cmd)) {
+ cand->util = 0;
+ } else {
+ int edit = levenshtein(cmd, cand->string,
+ 0, 2, 1, 3) + 1;
+
+ cand->util = (void *)(intptr_t)edit;
+ }
+ }
+
+ QSORT(cmds->items, cmds->nr, levenshtein_compare);
+
+ /* Match help.c:help_unknown_cmd */
+ for (; n < cmds->nr && !cmds->items[n].util; n++);
+
+ if (n == cmds->nr)
+ /* prefix matches with every subcommands */
+ best = AUTOCORRECT_SIMILARITY_FLOOR + 1;
+ else
+ for (best = (intptr_t)cmds->items[n++].util;
+ (n < cmds->nr && best == (intptr_t)cmds->items[n].util);
+ n++);
+
+ if (autocorrect.mode != AUTOCORRECT_HINT && n == 1 &&
+ AUTOCORRECT_SIMILAR_ENOUGH(best)) {
+ fprintf_ln(stderr,
+ _("WARNING: You called a subcommand named '%s', which does not exist."),
+ cmd);
+
+ autocorrect_confirm(&autocorrect, cmds->items[0].string);
+ return cmds->items[0].string;
+ }
+
+ if (AUTOCORRECT_SIMILAR_ENOUGH(best)) {
+ error(_("'%s' is not a subcommand."), cmd);
+
+ fprintf_ln(stderr,
+ Q_("\nThe most similar subcommand is",
+ "\nThe most similar subcommands are",
+ n));
+
+ for (unsigned int i = 0; i < n; i++)
+ fprintf(stderr, "\t%s\n", cmds->items[i].string);
+
+ exit(1);
+ }
+
+ return NULL;
+}
+
static enum parse_opt_result handle_subcommand(struct parse_opt_ctx_t *ctx,
const char *arg,
const struct option *options,
const char * const usagestr[])
{
- int err = parse_subcommand(arg, options);
+ int err;
+ const char *assumed;
+ struct string_list cmds = STRING_LIST_INIT_NODUP;
+ err = parse_subcommand(arg, options);
if (!err)
return PARSE_OPT_SUBCOMMAND;
@@ -642,8 +729,17 @@ static enum parse_opt_result handle_subcommand(struct parse_opt_ctx_t *ctx,
if (ctx->flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)
return PARSE_OPT_DONE;
- error(_("unknown subcommand: `%s'"), arg);
- usage_with_options(usagestr, options);
+ find_subcommands(&cmds, options);
+ assumed = autocorrect_subcommand(arg, &cmds);
+
+ if (!assumed) {
+ error(_("unknown subcommand: `%s'"), arg);
+ usage_with_options(usagestr, options);
+ }
+
+ string_list_clear(&cmds, 0);
+ parse_subcommand(assumed, options);
+ return PARSE_OPT_SUBCOMMAND;
}
static void check_typos(const char *arg, const struct option *options)
--
2.53.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox