* [RFC PATCH v5 0/3] push: add support for pushing to remote groups
From: Usman Akinyemi @ 2026-05-18 18:27 UTC (permalink / raw)
To: usmanakinyemi202, git; +Cc: christian.couder, gitster, me, phillip.wood123, ps
In-Reply-To: <20260503153402.1333220-4-usmanakinyemi202@gmail.com>
This RFC series adds support for `git push` to accept a remote group
name (as configured via `remotes.<name>` in config) in addition to a
single remote name, mirroring the behaviour that `git fetch` has
supported for some time.
A user with multiple remotes configured as a group can now do:
git push all-remotes
instead of pushing to each remote individually, in the same way that:
git fetch all-remotes
already works.
The series is split into three patches:
- Patch 1 fix sign-compare warnings in push_cas_option
- Patch 2 moves `get_remote_group`, `add_remote_or_group`, and the
`remote_group_data` struct out of builtin/fetch.c and into
remote.c/remote.h, making them part of the public remote API.
- Patch 2 extends builtin/push.c to use the newly public
`add_remote_or_group()` to resolve the repository argument as
either a single remote or a group, and pushes to each member of
the group in turn.
Changes in v6:
- fix docs formating
Range-diff v5 -> v6:
1: e01126890c = 1: e01126890c remote: fix sign-compare warnings in push_cas_option
2: adbce652e6 = 2: adbce652e6 remote: move remote group resolution to remote.c
3: a8d5f4b7bd ! 3: 62a4499be6 push: support pushing to a remote group
@@ Documentation/git-push.adoc: further recursion will occur. In this case, `only`
+ ...
+ git push <options> rN <args>
+
-+where r1, r2, ..., rN are the members of `all-remotes`. No special
++where `r1`, `r2`, ..., `rN` are the members of `all-remotes`. No special
+behaviour is added or removed — the group is purely a shorthand for
+running the same push command against each member remote individually.
+
@@ Documentation/git-push.adoc: further recursion will occur. In this case, `only`
+any member push fails.
+
+This means the user is responsible for ensuring that the sequence of
-+individual pushes makes sense. If `git push r1`` would fail for a given
++individual pushes makes sense. If `git push r1` would fail for a given
+set of options and arguments, then `git push all-remotes` will fail in
-+the same way when it reaches r1. The group push does not do anything
++the same way when it reaches `r1`. The group push does not do anything
+special to make a failing individual push succeed.
+
OUTPUT
Usman Akinyemi (3):
remote: fix sign-compare warnings in push_cas_option
remote: move remote group resolution to remote.c
push: support pushing to a remote group
Documentation/git-push.adoc | 80 ++++++++++--
builtin/fetch.c | 42 ------
builtin/push.c | 251 +++++++++++++++++++++++++++++++-----
remote.c | 37 ++++++
remote.h | 16 ++-
t/meson.build | 1 +
t/t5566-push-group.sh | 160 +++++++++++++++++++++++
7 files changed, 502 insertions(+), 85 deletions(-)
create mode 100755 t/t5566-push-group.sh
--
2.54.0
^ permalink raw reply
* Re: [PATCH v4 0/8] fetch: rework negotiation tip options
From: Matthew John Cheetham @ 2026-05-18 17:24 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <pull.2085.v4.git.1778762495.gitgitgadget@gmail.com>
On 2026-05-14 13:41, Derrick Stolee via GitGitGadget wrote:
> Updates in v4
> =============
>
> Thanks, Matthew, for the detailed review! There are some big changes in this
> version.
>
> * Expanded commit message to cite the commit that introduced the bug
> (3f763ddf28).
> * Renamed --negotiation-tip to --negotiation-restrict throughout docs/code
> (including send-pack.c, transport-helper.c, builtin/pull.c). Added
> OPT_ALIAS in git-pull.
> * Switched config parsing to use parse_transport_option() helper. Removed
> git push from docs (not implemented yet). Restructured --negotiate-only
> validation flow.
> * NEW Patch 5: Added have_sent() interface to negotiators, so included
> haves can be de-duplicated properly by the negotiation algorithm.
> * Replaced COMMON flag hack with negotiator->have_sent() calls. Moved
> ref-pattern resolution into builtin/fetch.c (add_negotiation_tips()) so
> fetch-pack receives pre-resolved oid_array instead of string_list. Added
> test for --negotiation-tip ignoring missing refs. Added
> duplicate-avoidance test for v0. Accepts commit hashes in addition to ref
> names/globs.
> * Use parse_transport_option() for config. Updated docs to mention commit
> hashes. Removed git push from config docs. Fixed test to use correct
> restrict/include combinations.
> * In the last patch, add doc notes that remote config values also apply
> during git push with push.negotiate, now that they are integrated by that
> change.
>
Thank you for going through the comments on v3 in detail. This is a nice
improvement overall.
The main thing flagged (the COMMON bit confusion) is resolved by adding
the new have_sent() API on the negotiator interface, which is much
clearer and cleaner. The hoisting of the ref resolution to the same
layer and reuse of add_negotiate_tips() is also done and appreciated!
I've left replies on each patch, with only a small number of easily
addressed comments.
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH v4 8/8] send-pack: pass negotiation config in push
From: Matthew John Cheetham @ 2026-05-18 17:21 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <5b968245ebaf14c05233195f5c806c4e94fd3ff7.1778762495.git.gitgitgadget@gmail.com>
On 2026-05-14 13:41, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> When push.negotiate is enabled, 'git push' spawns a child 'git fetch
> --negotiate-only' process to find common commits. Pass
> --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 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
> --negotiation-restrict now that the new synonym exists.
Is this paragraph stale? Didn't we rename the option in patch 2?
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> Documentation/config/remote.adoc | 6 ++++++
> send-pack.c | 37 ++++++++++++++++++++++++++------
> send-pack.h | 2 ++
> t/t5516-fetch-push.sh | 30 ++++++++++++++++++++++++++
> transport.c | 2 ++
> 5 files changed, 70 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
> index 9ae20e4379..460b4e7952 100644
> --- a/Documentation/config/remote.adoc
> +++ b/Documentation/config/remote.adoc
> @@ -122,6 +122,9 @@ command-line option. If `--negotiation-restrict` (or its synonym
> `--negotiation-tip`) is specified on the command line, then the config
> values are not used.
> +
> +These values also influence negotiation during `git push` if
> +`push.negotiate` is enabled.
> ++
> Blank values signal to ignore all previous values, allowing a reset of
> the list from broader config scenarios.
>
Nice. We're now only mentioning `git push` behaviour once we've wired
it up in this patch.
> @@ -149,6 +152,9 @@ unconditionally on top of those heuristically selected commits. This
> option is also used during push negotiation when `push.negotiate` is
> enabled.
> +
> +These values also influence negotiation during `git push` if
> +`push.negotiate` is enabled.
> ++
> Blank values signal to ignore all previous values, allowing a reset of
> the list from broader config scenarios.
>
> diff --git a/send-pack.c b/send-pack.c
> index 3d5d36ba3b..d18e030ce8 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -433,28 +433,48 @@ 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_include,
> + const struct string_list *negotiation_restrict,
> const struct ref *remote_refs,
> struct oid_array *commons)
> {
> 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);
> - for (ref = remote_refs; ref; ref = ref->next) {
> - if (!is_null_oid(&ref->new_oid)) {
> +
> + if (negotiation_restrict && negotiation_restrict->nr) {
> + struct string_list_item *item;
> + for_each_string_list_item(item, negotiation_restrict)
> strvec_pushf(&child.args, "--negotiation-restrict=%s",
> - oid_to_hex(&ref->new_oid));
> - nr_negotiation_tip++;
> + item->string);
> + 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-restrict=%s",
> + oid_to_hex(&ref->new_oid));
> + nr_negotiation++;
> + }
> }
> }
> +
> + if (negotiation_include && negotiation_include->nr) {
> + struct string_list_item *item;
> + 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) {
> child_process_clear(&child);
> return;
> }
> @@ -528,7 +548,10 @@ 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_include,
> + args->negotiation_restrict,
> + remote_refs, &commons);
> trace2_region_leave("send_pack", "push_negotiate", r);
> }
>
> diff --git a/send-pack.h b/send-pack.h
> index c5ded2d200..13850c98bb 100644
> --- a/send-pack.h
> +++ b/send-pack.h
> @@ -18,6 +18,8 @@ struct repository;
>
> struct send_pack_args {
> const char *url;
> + const struct string_list *negotiation_include;
> + const struct string_list *negotiation_restrict;
> unsigned verbose:1,
> quiet:1,
> porcelain:1,
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index ac8447f21e..177cbc6c75 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -254,6 +254,36 @@ test_expect_success 'push with negotiation does not attempt to fetch submodules'
> ! grep "Fetching submodule" err
> '
>
> +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_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
> +'
> +
> +test_expect_success 'push with negotiation and remote.<name>.negotiationRestrict' '
> + test_when_finished rm -rf negotiation_restrict &&
> + mk_empty negotiation_restrict &&
> + git push negotiation_restrict $the_first_commit:refs/remotes/origin/first_commit &&
> + test_commit -C negotiation_restrict unrelated_commit &&
> + git -C negotiation_restrict 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_restrict.negotiationRestrict=refs/heads/main \
> + push negotiation_restrict refs/heads/main:refs/remotes/origin/main &&
> + test_grep \"key\":\"total_rounds\" event &&
> + grep_wrote 2 event # 1 commit, 1 tree
> +'
> +
> test_expect_success 'push without wildcard' '
> mk_empty testrepo &&
>
> diff --git a/transport.c b/transport.c
> index fa54928966..a2d8958cb8 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -921,6 +921,8 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
> args.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);
> args.push_options = transport->push_options;
> args.url = transport->url;
> + args.negotiation_include = &transport->remote->negotiation_include;
> + args.negotiation_restrict = &transport->remote->negotiation_restrict;
>
> if (flags & TRANSPORT_PUSH_CERT_ALWAYS)
> args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
Again v4 of this patch LGTM.
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH v4 7/8] remote: add remote.*.negotiationInclude config
From: Matthew John Cheetham @ 2026-05-18 17:18 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <7bd70a970b819c2d856bf8663e26797498526399.1778762495.git.gitgitgadget@gmail.com>
On 2026-05-14 13:41, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> 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_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.
>
> 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 | 27 ++++++++++++++++++
> Documentation/fetch-options.adoc | 4 +++
> builtin/fetch.c | 11 +++++++
> remote.c | 5 ++++
> remote.h | 1 +
> t/t5510-fetch.sh | 49 ++++++++++++++++++++++++++++++++
> 6 files changed, 97 insertions(+)
>
> diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
> index 4dcf81fbce..9ae20e4379 100644
> --- a/Documentation/config/remote.adoc
> +++ b/Documentation/config/remote.adoc
> @@ -125,6 +125,33 @@ values are not used.
> Blank values signal to ignore all previous values, allowing a reset of
> the list from broader config scenarios.
>
> +remote.<name>.negotiationInclude::
> + When negotiating with this remote during `git fetch`, the client
> + advertises a list of commits that exist locally. In repos with
> + many references, this list of "haves" can be truncated. Depending
> + on data shape, dropping certain references may be expensive. This
> + multi-valued config option specifies references, commit hashes,
> + or ref pattern globs whose tips should always be sent as "have"
> + commits during fetch negotiation with this remote.
> ++
> +Each value is either an exact ref name (e.g. `refs/heads/release`), a
> +commit hash, or a glob pattern (e.g. `refs/heads/release/*`). The
> +pattern syntax is the same as for `--negotiation-include`.
Thanks - references the correct cross-referenced option I raised in v3.
Commit hashes are also explicitly mentioned - good.
> +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>.negotiationInclude` are sent
> +unconditionally on top of those heuristically selected commits. This
> +option is also used during push negotiation when `push.negotiate` is
> +enabled.
One thing to mention: the "also used during push negotiation" sentence
is added here, and then I see in the next patch (patch 8) we're adding
another sentence "these values also influence negotiation during git
push" to this same block.
Perhaps consider dropping the additional 'push' sentence here (patch 7)
and let patch 8 add just its own sentence about push? Not too pressing.
> ++
> +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/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
> index 7b897a7202..8074004377 100644
> --- a/Documentation/fetch-options.adoc
> +++ b/Documentation/fetch-options.adoc
> @@ -91,6 +91,10 @@ 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`.
> ++
> +If this option is not specified on the command line, then any
> +`remote.<name>.negotiationInclude` config values for the current remote
> +are used instead.
>
> `--negotiate-only`::
> Do not fetch anything from the server, and instead print the
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 6b456b3689..2308cab377 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1630,6 +1630,17 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
> else
> warning(_("ignoring %s because the protocol does not support it"),
> "--negotiation-include");
> + } else if (remote->negotiation_include.nr) {
> + if (transport->smart_options) {
> + add_negotiation_tips(&remote->negotiation_include,
> + &transport->smart_options->negotiation_include_tips);
> + } else {
> + struct strbuf config_name = STRBUF_INIT;
> + 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);
> + }
> }
> return transport;
> }
> diff --git a/remote.c b/remote.c
> index 620086e16e..6fb5758820 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -153,6 +153,7 @@ 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_include);
>
> ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1,
> remote_state->remotes_alloc);
> @@ -181,6 +182,7 @@ 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_include, 0);
> }
>
> static void add_merge(struct branch *branch, const char *name)
> @@ -567,6 +569,9 @@ static int handle_config(const char *key, const char *value,
> } else if (!strcmp(subkey, "negotiationrestrict")) {
> return parse_transport_option(key, value,
> &remote->negotiation_restrict);
> + } else if (!strcmp(subkey, "negotiationinclude")) {
> + return parse_transport_option(key, value,
> + &remote->negotiation_include);
> } else if (!strcmp(subkey, "followremotehead")) {
> const char *no_warn_branch;
> if (!strcmp(value, "never"))
Good - uses the parse_transport_options() like with the earlier patch.
> diff --git a/remote.h b/remote.h
> index e6ec37c393..d8809b6991 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -118,6 +118,7 @@ struct remote {
>
> struct string_list server_options;
> struct string_list negotiation_restrict;
> + struct string_list negotiation_include;
>
> 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 bc2e2af959..33f61ac12a 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1587,6 +1587,55 @@ test_expect_success '--negotiation-include avoids duplicates with negotiator' '
> test_line_count = 1 matches
> '
>
> +test_expect_success 'remote.<name>.negotiationInclude used as default for --negotiation-include' '
> + test_when_finished rm -f trace &&
> + setup_negotiation_tip server server 0 &&
> +
> + # 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=beta_2 \
> + 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
> +'
Great! Now this test will catch failures to the reset-list behaviour
by using a different CLI option ref name than is in the config list.
> +
> +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.negotiationInclude "refs/tags/beta_*" &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> + --negotiation-restrict=alpha_1 \
> + 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 '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.negotiationInclude refs/tags/beta_2 &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> + --negotiation-restrict=alpha_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 &&
> + BETA_2=$(git -C client rev-parse beta_2) &&
> + test_grep ! "fetch> have $BETA_2" trace
> +'
> +
> test_expect_success '--negotiation-include avoids duplicates with v0' '
> test_when_finished rm -f trace &&
> setup_negotiation_tip server server 0 &&
Overall this patch addresses my comments from v3's review. Just the
minor issue with forward reference to 'git push' behaviour in the docs.
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH v4 6/8] fetch: add --negotiation-include option for negotiation
From: Matthew John Cheetham @ 2026-05-18 17:12 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <b4cd458fe0b7625736348655c3aa704affc541c9.1778762495.git.gitgitgadget@gmail.com>
On 2026-05-14 13:41, Derrick Stolee via GitGitGadget wrote:
> 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 branches. 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.
brances/branches from v3 fixed - thanks!
> 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
> negotiator performs its own algorithm to choose the next haves. Use the new
> have_sent() interface to signal these have commits were sent before engaging
> with the negotiator's next() iterator.
Now references the new have_sent() API - good!
> 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 | 32 ++++++++---
> builtin/pull.c | 3 ++
> fetch-pack.c | 81 +++++++++++++++++++++++++---
> fetch-pack.h | 6 ++-
> t/t5510-fetch.sh | 91 ++++++++++++++++++++++++++++++++
> transport.c | 8 ++-
> transport.h | 5 +-
> 8 files changed, 227 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
> index d39cecb446..7b897a7202 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=(<commit>|<glob>)`::
> + Ensure that the commits at the given tips are always sent as "have"
> + lines 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 commit is sent
> +unconditionally.
> ++
> +The argument may be an exact ref name (e.g. `refs/heads/release`), an
> +object hash, 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`.
> +
Good - the syntax placeholder matches --negotiate-restrict, and we
mention object hashes being valid input.
> `--negotiate-only`::
> Do not fetch anything from the server, and instead print the
> ancestors of the provided `--negotiation-restrict=` arguments,
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index a957739f37..6b456b3689 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;
> @@ -1534,23 +1535,28 @@ static int add_oid(const struct reference *ref, void *cb_data)
> return 0;
> }
>
> -static void add_negotiation_restrict_tips(struct git_transport_options *smart_options)
> +static void add_negotiation_tips(struct string_list *input_list,
> + struct oid_array **output_list)
> {
> struct oid_array *oids = xcalloc(1, sizeof(*oids));
> int i;
>
> - for (i = 0; i < negotiation_restrict.nr; i++) {
> - const char *s = negotiation_restrict.items[i].string;
> + for (i = 0; i < input_list->nr; i++) {
> + const char *s = input_list->items[i].string;
> struct refs_for_each_ref_options opts = {
> .pattern = s,
> };
> 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;
> }
> @@ -1561,7 +1567,7 @@ static void add_negotiation_restrict_tips(struct git_transport_options *smart_op
> warning(_("ignoring %s=%s because it does not match any refs"),
> "--negotiation-restrict", s);
> }
> - smart_options->negotiation_restrict_tips = oids;
> + *output_list = oids;
> }
>
> static struct transport *prepare_transport(struct remote *remote, int deepen,
Great! Now we have a unified implementation that takes a string_list
and ouputs an oid_array, used for both restrict and include lists.
We pre-resolve so that fetch-pack gets the oid_array so the ref
resolution happens in the same layer for both.
Just one small issue I see - the function emits a warning for
"--negotiate-restrict" when we don't match a ref, even if this is
handling a --negotiate-include value. Perhaps we should pass the
option name in as a parameter?
add_negotiation_tips(&negotiation_include,
"--negotiation-include",
&transport->smart_opt->negotiation_include_tips);
> @@ -1597,7 +1603,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
> }
> if (negotiation_restrict.nr) {
> if (transport->smart_options)
> - add_negotiation_restrict_tips(transport->smart_options);
> + add_negotiation_tips(&negotiation_restrict,
> + &transport->smart_options->negotiation_restrict_tips);
> else
> warning(_("ignoring %s because the protocol does not support it"),
> "--negotiation-restrict");
> @@ -1606,7 +1613,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
> 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);
> + add_negotiation_tips(&negotiation_restrict,
> + &transport->smart_options->negotiation_restrict_tips);
> else {
> struct strbuf config_name = STRBUF_INIT;
> strbuf_addf(&config_name, "remote.%s.negotiationRestrict", remote->name);
> @@ -1615,6 +1623,14 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
> strbuf_release(&config_name);
> }
> }
> + if (negotiation_include.nr) {
> + if (transport->smart_options)
> + add_negotiation_tips(&negotiation_include,
> + &transport->smart_options->negotiation_include_tips);
> + else
> + warning(_("ignoring %s because the protocol does not support it"),
> + "--negotiation-include");
> + }
> return transport;
> }
Having the shared helper makes this much nicer. Good!
> @@ -2582,6 +2598,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 cc6ce485fc..d49b09114a 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1000,6 +1000,9 @@ int cmd_pull(int argc,
> N_("report that we have only objects reachable from this object"),
> 0),
> OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
> + 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,
LGTM
> diff --git a/fetch-pack.c b/fetch-pack.c
> index baf239adf9..96071434b8 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,21 @@ static void send_filter(struct fetch_pack_args *args,
> }
> }
>
> +static void add_oids_to_set(const struct oid_array *array,
> + struct oidset *set)
> +{
> + if (!array)
> + return;
> +
> + for (size_t i = 0; i < array->nr; i++) {
> + struct object_id *oid = &array->oid[i];
> + if (!odb_has_object(the_repository->objects, oid, 0))
> + die(_("the object %s does not exist"), oid_to_hex(oid));
> +
> + oidset_insert(set, oid);
> + }
> +}
> +
Nice - this is much simpler since ref resolution has been hoisted up
and we only deal with pre-resolved OIDs.
> static int find_common(struct fetch_negotiator *negotiator,
> struct fetch_pack_args *args,
> int fd[2], struct object_id *result_oid,
> @@ -347,6 +363,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 +491,27 @@ 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 */
> + add_oids_to_set(args->negotiation_include_tips,
> + &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++;
> +
> + commit = lookup_commit(the_repository, oid);
> + if (commit)
> + negotiator->have_sent(negotiator, commit);
> + }
> + }
> +
> 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 +622,7 @@ done:
> flushes++;
> }
> strbuf_release(&req_buf);
> + oidset_clear(&negotiation_include_oids);
>
> if (!got_ready || !no_done)
> consume_shallow_list(args, &reader);
> @@ -1305,11 +1344,27 @@ 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))) {
> + struct commit *commit = lookup_commit(the_repository, oid);
> + if (commit) {
> + packet_buf_write(req_buf, "have %s\n",
> + oid_to_hex(oid));
> + negotiator->have_sent(negotiator, commit);
> + }
> + }
> + }
> +
> while ((oid = negotiator->next(negotiator))) {
> packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
> if (++haves_added >= *haves_to_send)
Here we're using the new have_sent() API and replaces the manual COMMON
bit flipping.
> @@ -1358,7 +1413,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 +1469,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 +1714,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 +1787,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> state = FETCH_SEND_REQUEST;
>
> mark_tips(negotiator, args->negotiation_restrict_tips);
> + add_oids_to_set(args->negotiation_include_tips,
> + &negotiation_include_oids);
> for_each_cached_alternate(negotiator,
> insert_one_alternate_object);
> break;
> @@ -1747,7 +1807,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 +1944,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;
> }
The v2 wiring looks correct.
> @@ -2181,12 +2243,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 oid_array *negotiation_include_tips)
> {
> 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 +2261,9 @@ void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
> fetch_negotiator_init(the_repository, &negotiator);
> mark_tips(&negotiator, negotiation_restrict_tips);
>
> + add_oids_to_set(negotiation_include_tips,
> + &negotiation_include_oids);
> +
> packet_reader_init(&reader, fd[0], NULL, 0,
> PACKET_READ_CHOMP_NEWLINE |
> PACKET_READ_DIE_ON_ERR_PACKET);
> @@ -2221,7 +2288,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 +2341,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..6d0dec7f41 100644
> --- a/fetch-pack.h
> +++ b/fetch-pack.h
> @@ -19,9 +19,10 @@ struct fetch_pack_args {
>
> /*
> * If not NULL, during packfile negotiation, fetch-pack will send "have"
> - * lines only with these tips and their ancestors.
> + * lines for all _include_ tips and then a subset of the _restrict_ tips.
> */
> const struct oid_array *negotiation_restrict_tips;
> + const struct oid_array *negotiation_include_tips;
>
> unsigned deepen_relative:1;
> unsigned quiet:1;
> @@ -93,7 +94,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 oid_array *negotiation_include_tips);
>
> /*
> * 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..bc2e2af959 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1460,6 +1460,16 @@ EOF
> test_cmp fatal-expect fatal-actual
> '
>
> +test_expect_success '--negotiation-tip ignores missing refs and invalid hashes' '
> + setup_negotiation_tip server server 0 &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> + --negotiation-tip=alpha_1 --negotiation-tip=beta_1 \
> + --negotiation-tip=no-such-ref \
> + --negotiation-tip=invalid-hash \
> + origin alpha_s beta_s &&
> + check_negotiation_tip
> +'
> +
Good! This checks for missing refs (not just invalid ones with all
zeros).
> test_expect_success '--negotiation-restrict limits "have" lines sent' '
> setup_negotiation_tip server server 0 &&
> GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> @@ -1511,6 +1521,87 @@ 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 '--negotiation-include avoids duplicates with v0' '
> + 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 \
> + -c protocol.version=0 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..fa54928966 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_tips = data->options.negotiation_include_tips;
> 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_tips);
> ret = 0;
> }
> goto cleanup;
> @@ -983,6 +985,10 @@ static int disconnect_git(struct transport *transport)
> oid_array_clear(data->options.negotiation_restrict_tips);
> free(data->options.negotiation_restrict_tips);
> }
> + if (data->options.negotiation_include_tips) {
> + oid_array_clear(data->options.negotiation_include_tips);
> + free(data->options.negotiation_include_tips);
> + }
> list_objects_filter_release(&data->options.filter_options);
> oid_array_clear(&data->extra_have);
> oid_array_clear(&data->shallow);
> diff --git a/transport.h b/transport.h
> index cdeb33c16f..97d905ecc0 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -40,13 +40,14 @@ struct git_transport_options {
>
> /*
> * This is only used during fetch. See the documentation of
> - * negotiation_restrict_tips in struct fetch_pack_args.
> + * these member names in struct fetch_pack_args.
> *
> - * This field is only supported by transports that support connect or
> + * These fields are only supported by transports that support connect or
> * stateless_connect. Set this field directly instead of using
> * transport_set_option().
> */
> struct oid_array *negotiation_restrict_tips;
> + struct oid_array *negotiation_include_tips;
>
> /*
> * If allocated, whenever transport_fetch_refs() is called, add known
Overall this version of the patch is much better IMO! Just that one
comment about the incorrect option name in the warning message.
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH v4 5/8] negotiator: add have_sent() interface
From: Matthew John Cheetham @ 2026-05-18 16:57 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <94b79784fe6a4f22dba32a2e1d44316b3e84da48.1778762495.git.gitgitgadget@gmail.com>
On 2026-05-14 13:41, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> In a future change, we will introduce a capability to choose specific commit
> OIDs as 'have's in fetch negotiation, with the ability to have the
> negotiator choose more 'have's to increase coverage beyond that required
> core set. The negotiator works to avoid emitting 'have's that can reach each
> other, but that logic is hidden beneath the negotiator's iterator function
> pointer ('next'). We need a way to communicate to the negotiator that we
> have picked a 'have' so it could incorporate that into its logic.
>
> Add a have_sent() method to the fetch_negotiator interface. This is the
> signal that allows the negotiator to track the commit as already shown and
> can perform the proper bookkeeping to avoid emitting those objects or
> anything they can reach.
>
> For our non-trivial negotiators, it is sufficient to mark these commits as
> common, so the implementation is quite simple. This logic will be exercised
> in the next change.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
This is a new patch in v4, and is in response to the COMMON (bit 2) vs
COMMON (bit 6) issue of v3.
Having a defined have_sent() API on the negotiator, and letting the
negotiator handle its own book-keeping is much nicer and safer.
> ---
> fetch-negotiator.h | 9 +++++++++
> negotiator/default.c | 8 ++++++++
> negotiator/noop.c | 7 +++++++
> negotiator/skipping.c | 8 ++++++++
> 4 files changed, 32 insertions(+)
>
> diff --git a/fetch-negotiator.h b/fetch-negotiator.h
> index e348905a1f..6ca422a064 100644
> --- a/fetch-negotiator.h
> +++ b/fetch-negotiator.h
> @@ -47,6 +47,15 @@ struct fetch_negotiator {
> */
> int (*ack)(struct fetch_negotiator *, struct commit *);
>
> + /*
> + * Inform the negotiator that this commit has already been sent as
> + * a "have" line outside of the negotiator's control. The negotiator
> + * should avoid outputting it from next() and may use it to optimize
> + * further negotiation (e.g., by treating it and its ancestors as
> + * common).
> + */
> + void (*have_sent)(struct fetch_negotiator *, struct commit *);
> +
Doc comment captures the contract well.
> void (*release)(struct fetch_negotiator *);
>
> /* internal use */
> diff --git a/negotiator/default.c b/negotiator/default.c
> index 116dedcf83..05ab616f39 100644
> --- a/negotiator/default.c
> +++ b/negotiator/default.c
> @@ -175,6 +175,13 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
> return known_to_be_common;
> }
>
> +static void have_sent(struct fetch_negotiator *n, struct commit *c)
> +{
> + if (repo_parse_commit(the_repository, c))
> + return;
> + mark_common(n->data, c, 0, 0);
> +}
> +
> static void release(struct fetch_negotiator *n)
> {
> clear_prio_queue(&((struct negotiation_state *)n->data)->rev_list);
> @@ -188,6 +195,7 @@ void default_negotiator_init(struct fetch_negotiator *negotiator)
> negotiator->add_tip = add_tip;
> negotiator->next = next;
> negotiator->ack = ack;
> + negotiator->have_sent = have_sent;
> negotiator->release = release;
> negotiator->data = CALLOC_ARRAY(ns, 1);
> ns->rev_list.compare = compare_commits_by_commit_date;
I traced this through default.c's mark_common() to confirm this does
what we want.. sets the COMMON bit (bit 2) and propogates COMMON to
the ancestors correctly. Good!
> diff --git a/negotiator/noop.c b/negotiator/noop.c
> index 65e3c20008..edf1b456f3 100644
> --- a/negotiator/noop.c
> +++ b/negotiator/noop.c
> @@ -29,6 +29,12 @@ static int ack(struct fetch_negotiator *n UNUSED, struct commit *c UNUSED)
> return 0;
> }
>
> +static void have_sent(struct fetch_negotiator *n UNUSED,
> + struct commit *c UNUSED)
> +{
> + /* nothing to do */
> +}
> +
> static void release(struct fetch_negotiator *n UNUSED)
> {
> /* nothing to release */
> @@ -40,6 +46,7 @@ void noop_negotiator_init(struct fetch_negotiator *negotiator)
> negotiator->add_tip = add_tip;
> negotiator->next = next;
> negotiator->ack = ack;
> + negotiator->have_sent = have_sent;
> negotiator->release = release;
> negotiator->data = NULL;
> }
Trivial implementation of the noop negotiator. Good.
> diff --git a/negotiator/skipping.c b/negotiator/skipping.c
> index 0a272130fb..69472c58e1 100644
> --- a/negotiator/skipping.c
> +++ b/negotiator/skipping.c
> @@ -243,6 +243,13 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
> return known_to_be_common;
> }
>
> +static void have_sent(struct fetch_negotiator *n, struct commit *c)
> +{
> + if (repo_parse_commit(the_repository, c))
> + return;
> + mark_common(n->data, c);
> +}
> +
> static void release(struct fetch_negotiator *n)
> {
> struct data *data = n->data;
> @@ -259,6 +266,7 @@ void skipping_negotiator_init(struct fetch_negotiator *negotiator)
> negotiator->add_tip = add_tip;
> negotiator->next = next;
> negotiator->ack = ack;
> + negotiator->have_sent = have_sent;
> negotiator->release = release;
> negotiator->data = CALLOC_ARRAY(data, 1);
> data->rev_list.compare = compare;
I traced this too, to skipping.c's mark_common(), and it sets the COMMON
bit correctly.
This patch is a welcome addition and LGTM!
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH v3 0/4] Batch prefetching
From: Elijah Newren @ 2026-05-18 16:55 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Elijah Newren via GitGitGadget, git, Phillip Wood
In-Reply-To: <0da4f159-8d4b-49e2-93c1-25aa0bf69371@gmail.com>
On Mon, May 18, 2026 at 5:17 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 5/14/2026 12:25 PM, Elijah Newren via GitGitGadget wrote:
> > Changes since v2:
> >
> > * Modified the final patch as suggested by Stolee to include pathspec usage
> > in the testcase
> > * Modified the last two patches to not re-download blobs we already have
> > locally, and adjusted the tests to verify
> > * Inserted a new first patch, containing a documentation addition that
> > would have helped me avoid making the above mistake in the first place.
>
> Thank you for these changes. I reviewed the updates and documentation and
> think this version is good to go.
As always, thanks for reviewing! Your comments on patch 3 in
particular led me to what would have been a rather annoying bug, so
thanks for calling out an improvement to the testcase that alerted me
to that issue.
> > Note: Stolee also suggest some code sharing or code movement in his review
> > of v2 2/3, but possibly based on a misunderstanding of v2 2/3 (that patch
> > isn't about a diff) and it's not clear to me what could be shared or moved,
> > so that's not part of this round.
>
> Your detailed responses in the v2 thread helped me understand that my thought
> was misguided. Thanks for giving me extra confidence in your approach here.
I can totally see where the comments came from; they did seem logical
on the surface. I knew the changes were cherry-specific in a few
ways, but trying to figure out how to explain that and dig further
into the details to find a good angle (and try to make sure I wasn't
just missing something about how part of the logic could be shared)
took a bunch of additional time, so I'm happy to hear that others
consider it enlightening. That makes it time well spent.
^ permalink raw reply
* Re: [PATCH v4 4/8] remote: add remote.*.negotiationRestrict config
From: Matthew John Cheetham @ 2026-05-18 16:46 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <a14c568a1fd3680e40b70cf9ca14fd2a5305df5e.1778762495.git.gitgitgadget@gmail.com>
On 2026-05-14 13:41, Derrick Stolee via GitGitGadget wrote:
> 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-tip. 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 | 18 ++++++++++++++++++
> builtin/fetch.c | 28 +++++++++++++++++++++-------
> remote.c | 5 +++++
> remote.h | 1 +
> t/t5510-fetch.sh | 26 ++++++++++++++++++++++++++
> 5 files changed, 71 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
> index 91e46f66f5..4dcf81fbce 100644
> --- a/Documentation/config/remote.adoc
> +++ b/Documentation/config/remote.adoc
> @@ -107,6 +107,24 @@ 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`, 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.
Good - now we don't talk about 'git push' before we have wired that
functionality up.
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 2ba0051d52..a957739f37 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;
> }
Unchanged from v3 and still reads cleanly. Good.
> @@ -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,14 +2758,19 @@ 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->acked_commits = &acked_commits;
> - } else {
> +
> + if (!gtransport->smart_options) {
> warning(_("protocol does not support --negotiate-only, exiting"));
> result = 1;
> trace2_region_leave("fetch", "negotiate-only", the_repository);
> goto cleanup;
> }
> + if (!gtransport->smart_options->negotiation_restrict_tips)
> + die(_("%s needs one or more %s"), "--negotiate-only",
> + "--negotiation-restrict=*");
> +
> + gtransport->smart_options->acked_commits = &acked_commits;
> +
> if (server_options.nr)
> gtransport->server_options = &server_options;
> result = transport_fetch_refs(gtransport, NULL);
This is nice! Addressed my concerns in v3 about the wrong message being
shown, and makes it clearer with an up-front check for the protocol
support.
> diff --git a/remote.c b/remote.c
> index 7ca2a6501b..620086e16e 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,9 @@ 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")) {
> + return parse_transport_option(key, value,
> + &remote->negotiation_restrict);
> } else if (!strcmp(subkey, "followremotehead")) {
> const char *no_warn_branch;
> if (!strcmp(value, "never"))
Good - reusing the parse_transport_option() as suggested makes this a
smaller diff and consistent with 'serveroption' above.
> 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 &&
> (
Again, this patch looks good to me and addresses my previous comments.
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH v4 3/8] transport: rename negotiation_tips
From: Matthew John Cheetham @ 2026-05-18 16:42 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <401bdaff7c88e18204cb66feb6f8f895c26aedfa.1778762495.git.gitgitgadget@gmail.com>
On 2026-05-14 13:41, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> The previous change added the --negotiation-restrict synonym for the
> --negotiation-tip 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 typo (plural vs singular 'tips') is fixed - thanks!
> 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;
> }
Same as in v3, this function will be refactored in a later patch, so
just doing the rename now keeps things reviewable - thanks!
> 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=*");
Simple rename - looks good!
> 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 dd78d40668..f4388da766 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 %s because the protocol does not support it."),
> "--negotiation-restrict");
>
> 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
All just simple renames - looks good to me! This patch is good.
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH v4 2/8] fetch: add --negotiation-restrict option
From: Matthew John Cheetham @ 2026-05-18 16:37 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <7836a2d6a537cdee419625e4eb43b94d599590c2.1778762495.git.gitgitgadget@gmail.com>
On 2026-05-14 13:41, Derrick Stolee via GitGitGadget wrote:
> 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/config/fetch.adoc | 2 +-
> Documentation/fetch-options.adoc | 6 +++++-
> builtin/fetch.c | 13 ++++++++-----
> builtin/pull.c | 3 ++-
> send-pack.c | 2 +-
> t/t5510-fetch.sh | 25 +++++++++++++++++++++++++
> t/t5702-protocol-v2.sh | 4 ++--
> transport-helper.c | 3 ++-
> 8 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/config/fetch.adoc b/Documentation/config/fetch.adoc
> index cd40db0cad..04ac90912d 100644
> --- a/Documentation/config/fetch.adoc
> +++ b/Documentation/config/fetch.adoc
> @@ -76,7 +76,7 @@
> default is `skipping`. Unknown values will cause `git fetch` to
> error out.
> +
> -See also the `--negotiate-only` and `--negotiation-tip` options to
> +See also the `--negotiate-only` and `--negotiation-restrict` options to
> linkgit:git-fetch[1].
Good - this addressed my nit in v4 about mentioning the old name here.
> `fetch.showForcedUpdates`::
> diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
> index 81a9d7f9bb..d39cecb446 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.
> +
> @@ -71,7 +75,7 @@ configuration variables documented in linkgit:git-config[1], and the
>
> `--negotiate-only`::
> Do not fetch anything from the server, and instead print the
> - ancestors of the provided `--negotiation-tip=` arguments,
> + ancestors of the provided `--negotiation-restrict=` arguments,
> which we have in common with the server.
> +
> This is incompatible with `--recurse-submodules=(yes|on-demand)`.
Good! The --negotiate-only paragraph now uses --negotiate-restrict too.
> 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)
Unchanged from v3: OPT_ALIAS keeps back-compat and messages are i18n
friendly. Nice.
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 7e67fdce97..cc6ce485fc 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -996,9 +996,10 @@ int cmd_pull(int argc,
> OPT_PASSTHRU('6', "ipv6", &opt_ipv6, NULL,
> N_("use IPv6 addresses only"),
> PARSE_OPT_NOARG),
> - OPT_PASSTHRU_ARGV(0, "negotiation-tip", &opt_fetch, N_("revision"),
> + OPT_PASSTHRU_ARGV(0, "negotiation-restrict", &opt_fetch, N_("revision"),
> N_("report that we have only objects reachable from this object"),
> 0),
> + OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
> 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,
Oh nice! v3 had two OPT_PASSTHRU_ARGV (one for each name). Now we're
using OPT_ALIAS here too for the old name which avoids duplicated
description strings - yay!
I had to convince myself this works (does OPT_ALIAS forward to the
passthru option?). Looking at parse-options.c, preprocess_options()
substitutes the alias with a copy of the source option keeping only
the alias's long_name. So really the child proc will get the old
--negotiate-tip name but the child has an alias anyway.. works fine.
> diff --git a/send-pack.c b/send-pack.c
> index 67d6987b1c..3d5d36ba3b 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -447,7 +447,7 @@ static void get_commons_through_negotiation(struct repository *r,
> strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
> 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++;
> }
Good. v3 was using the old name when shelling out (just a nit), but
nicer to have the rename fully consistent.
> 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 \
Unchanged from v3 - still good!
> diff --git a/transport-helper.c b/transport-helper.c
> index 4d95d84f9e..dd78d40668 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -755,7 +755,8 @@ static int fetch_refs(struct transport *transport,
> }
>
> if (data->transport_options.negotiation_tips)
> - warning("Ignoring --negotiation-tip because the protocol does not support it.");
> + warning(_("ignoring %s because the protocol does not support it."),
> + "--negotiation-restrict");
Good - picks up a missed rename from v3.
> if (data->fetch)
> return fetch_with_fetch(transport, nr_heads, to_fetch);
Overall this patch LGTM! All my v3 issues were addressed.
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH v4 1/8] t5516: fix test order flakiness
From: Matthew John Cheetham @ 2026-05-18 16:19 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <7409a479d67eefed4b8958be83c55f8636233b4f.1778762495.git.gitgitgadget@gmail.com>
On 2026-05-14 13:41, Derrick Stolee via GitGitGadget wrote:
> 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. This appears to be an accident added
> in 3f763ddf28 (fetch: set remote/HEAD if it does not exist, 2024-11-22).
>
> Use 'sort -k 3' to match the actual number of columns in the output.
Expanding the message to back reference 3f763ddf28 is a nice change.
Makes it easier for future people reading the history to follow back.
> 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 &&
Unchanged from v3 and still LGTM!
Thanks,
Matthew
^ permalink raw reply
* Git
From: aleksisrose @ 2026-05-18 16:15 UTC (permalink / raw)
To: git
Йоу, как настроение?
^ permalink raw reply
* Re: What's cooking in git.git (May 2026, #04)
From: Mirko Faina @ 2026-05-18 15:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Mirko Faina
In-Reply-To: <xmqqv7clbizy.fsf@gitster.g>
On Mon, May 18, 2026 at 10:32:01AM +0900, Junio C Hamano wrote:
> * mf/revision-max-count-oldest (2026-05-15) 1 commit
> - revision.c: implement --max-count-oldest
>
> "git rev-list" (and "git log" family of commands) learned a new "--max-count-oldest"
> that picks oldest N commits in the range instead of the usual newest.
>
> Will merge to 'next'?
> source: <463cc8e2764edb7de3d379f615f5cfbd0919bfa3.1778887662.git.mroik@delayed.space>
Sorry, I realized it's still a bit buggy when coupled with --boundary.
There will be a v8.
^ permalink raw reply
* Re: [PATCH 1/1] commit: allow -m/-F with --fixup=amend: or reword:
From: Phillip Wood @ 2026-05-18 15:27 UTC (permalink / raw)
To: Junio C Hamano, erik; +Cc: git, charvi077
In-Reply-To: <xmqqik8kc2nj.fsf@gitster.g>
On 18/05/2026 13:39, Junio C Hamano wrote:
> erik@cervined.in writes:
>
>> From: Erik Cervin-Edin <erik@cervined.in>
>
> The name on this overriding in-body From: line, and the name on
> Signed-off-by: line below, must match. Please pick a name with or
> without hyphen and stick to it.
>
>> --fixup=amend: and --fixup=reword: require an editor to supply the
>> replacement commit message. The -m and -F flags are rejected: -m is
>> caught by a die() in prepare_to_commit(), and -F is caught by
>> die_for_incompatible_opt4() which groups -F with --fixup as mutually
>> exclusive. This makes these modes unusable in non-interactive
>> workflows -- notably AI coding agents.
>
> "Unusable" may be stronger than reality, as you can make creatie use
> of GIT_EDITOR to achieve what you want. "awkward" or "poorly suited"
> would be more fitting.
Indeed
>> Plain --fixup (without amend: or reword:) continues to reject -F but
>> still accepts -m (even though it's practically a no-op).
>
> Is it "practically a no-op"? Wouldn't
>
> $ git commit --fixup <commit> -m "message body"
>
> be useful to leave a message in the resulting commit, which is later
> to be squashed into the named <commit>? Actually squashing with "fixup!"
> may lose the message supplied here, but wouldn't people use this
> facility to more easily identify what each of the fixups are about?
Yes, I find it quite useful to make a note of what the fixup is doing if
I know I'm not going to squash it for a while.
> For the same reason, "-F" would be just as useful as "-m" in this context,
> and it feels a bit inconsistent to allow one while rejecting the other.
Yes, looking at the way the code is structured I wonder if these options
were made incompatible to simplify the implementation, or maybe the
implementation merely reflects those restrictions.
>> @@ -1821,6 +1824,22 @@ int cmd_commit(int argc,
>> argc = parse_and_validate_options(argc, argv, builtin_commit_options,
>> builtin_commit_usage,
>> prefix, current_head, &s);
>> +
>> + if (logfile && fixup_message && !strcmp(fixup_prefix, "amend")) {
>> + if (!strcmp(logfile, "-")) {
>> + if (isatty(0))
>> + fprintf(stderr, _("(reading log message from standard input)\n"));
>> + if (strbuf_read(&message, 0, 0) < 0)
>> + die_errno(_("could not read log from standard input"));
>> + } else {
>> + if (strbuf_read_file(&message, logfile, 0) < 0)
>> + die_errno(_("could not read log file '%s'"), logfile);
>> + }
>> + strbuf_complete_line(&message);
>> + have_option_m = 1;
>> + FREE_AND_NULL(logfile);
>> + }
>> +
>
> It is curious that for this new feature alone, but not the other
> existing code paths, "-m" and "-F" options reads from file in the
> new code here, instead of letting the existing code for "-F" to read
> (which happens inside prepare_to_commit(), I presume?).
>
> A potential problem of the above code is if we find something wrong
> in message and complain later in the control flow, we have long lost
> where the message came from, as the point of the above code is
> exactly to pretend that "--fixup:amend/reword -F" message did *not*
> come from a file with the "-F" option, but from the command line via
> the "-m" option.
It is indeed unfortunate that we end up duplicating the code to read the
logfile here. I wonder how hard it would be to refactor
prepare_to_commit() so that it can accommodate "--fixup=amend:<commit> -F"
>> +test_expect_success '--fixup=amend: with -m option' '
>> commit_for_rebase_autosquash_setup &&
>> - echo "fatal: options '\''-m'\'' and '\''--fixup:reword'\'' cannot be used together" >expect &&
>> - test_must_fail git commit --fixup=reword:HEAD~ -m "reword commit message" 2>actual &&
>> - test_cmp expect actual
>> + cat >expected <<-EOF &&
>
> This comment is not about the added logic, but I notice that among
> 86 hits with string "expect" in this file in today's "master", only
> 14 hits are with string "expected", i.e., the prevalent name for the
> "golden copy result" that is compared with the actula result (called
> "actual") is "expect", not "expected". Please do not make the
> situation worse.
In this case it would be better to use
test_commit_message HEAD <<-EOF
amend! $(git log -1 --format=%s HEAD~)
amend commit message
EOF
and avoid creating actual and expect all together.
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH v9 5/5] branch: add --all-remotes flag
From: Phillip Wood @ 2026-05-18 15:27 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Kristoffer Haugsbakk, Johannes Sixt, Harald Nordgren
In-Reply-To: <6ae95d3f98212ca449cb81d3cfe332e78b8011ea.1778700883.git.gitgitgadget@gmail.com>
Hi Harald
On 13/05/2026 20:34, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> +`--all-remotes`::
> + With `--forked` or `--prune-merged`, act on every
> + configured remote in addition to any explicit _<remote>_
> + arguments.
Why do we except additional arguments if this option includes all the
remotes?
Thanks
Phillip
> `-v`::
> `-vv`::
> `--verbose`::
> diff --git a/builtin/branch.c b/builtin/branch.c
> index bc4f4a4a18..7d45bada45 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -687,6 +687,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> free_worktrees(worktrees);
> }
>
> +static int collect_remote_name(struct remote *remote, void *cb_data)
> +{
> + struct string_list *remote_names = cb_data;
> + string_list_insert(remote_names, remote->name);
> + return 0;
> +}
> +
> static void parse_forked_args(int argc, const char **argv,
> struct string_list *remote_names,
> struct string_list *tracking_refs)
> @@ -776,7 +783,7 @@ static void collect_default_branch_refs(const struct string_list *remote_names,
> }
> }
>
> -static void collect_forked_set(int argc, const char **argv,
> +static void collect_forked_set(int argc, const char **argv, int all_remotes,
> struct string_list *protected_default_refs,
> struct string_list *out)
> {
> @@ -789,6 +796,8 @@ static void collect_forked_set(int argc, const char **argv,
> };
>
> parse_forked_args(argc, argv, &remote_names, &tracking_refs);
> + if (all_remotes)
> + for_each_remote(collect_remote_name, &remote_names);
>
> refs_for_each_branch_ref(get_main_ref_store(the_repository),
> collect_forked_branch, &cb);
> @@ -802,15 +811,15 @@ static void collect_forked_set(int argc, const char **argv,
> string_list_clear(&tracking_refs, 0);
> }
>
> -static int list_forked_branches(int argc, const char **argv)
> +static int list_forked_branches(int argc, const char **argv, int all_remotes)
> {
> struct string_list out = STRING_LIST_INIT_DUP;
> struct string_list_item *item;
>
> - if (!argc)
> - die(_("--forked requires at least one <remote>"));
> + if (!argc && !all_remotes)
> + die(_("--forked requires at least one <remote> or --all-remotes"));
>
> - collect_forked_set(argc, argv, NULL, &out);
> + collect_forked_set(argc, argv, all_remotes, NULL, &out);
> for_each_string_list_item(item, &out)
> puts(item->string);
>
> @@ -818,7 +827,8 @@ static int list_forked_branches(int argc, const char **argv)
> return 0;
> }
>
> -static int prune_merged_branches(int argc, const char **argv, int quiet)
> +static int prune_merged_branches(int argc, const char **argv,
> + int all_remotes, int quiet)
> {
> struct string_list candidates = STRING_LIST_INIT_DUP;
> struct string_list protected_default_refs = STRING_LIST_INIT_DUP;
> @@ -827,10 +837,11 @@ static int prune_merged_branches(int argc, const char **argv, int quiet)
> int n_not_merged = 0;
> int ret = 0;
>
> - if (!argc)
> - die(_("--prune-merged requires at least one <remote>"));
> + if (!argc && !all_remotes)
> + die(_("--prune-merged requires at least one <remote> or --all-remotes"));
>
> - collect_forked_set(argc, argv, &protected_default_refs, &candidates);
> + collect_forked_set(argc, argv, all_remotes, &protected_default_refs,
> + &candidates);
>
> for_each_string_list_item(item, &candidates) {
> const char *short_name = item->string;
> @@ -943,6 +954,7 @@ int cmd_branch(int argc,
> unset_upstream = 0, show_current = 0, edit_description = 0;
> int forked = 0;
> int prune_merged = 0;
> + int all_remotes = 0;
> const char *new_upstream = NULL;
> int noncreate_actions = 0;
> /* possible options */
> @@ -1000,6 +1012,9 @@ int cmd_branch(int argc,
> N_("list local branches forked from the given <remote>s")),
> OPT_BOOL(0, "prune-merged", &prune_merged,
> N_("delete local branches forked from the given <remote>s that are merged into their upstream")),
> + OPT_BOOL_F(0, "all-remotes", &all_remotes,
> + N_("with --forked or --prune-merged, act on every configured remote"),
> + PARSE_OPT_NONEG),
> OPT__FORCE(&force, N_("force creation, move/rename, deletion"), PARSE_OPT_NOCOMPLETE),
> OPT_MERGED(&filter, N_("print only branches that are merged")),
> OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
> @@ -1043,6 +1058,10 @@ int cmd_branch(int argc,
> argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
> 0);
>
> + if (all_remotes && !forked && !prune_merged)
> + die(_("--all-remotes requires --forked or --prune-merged"));
> +
> +
> if (!delete && !rename && !copy && !edit_description && !new_upstream &&
> !show_current && !unset_upstream && !forked && !prune_merged &&
> argc == 0)
> @@ -1096,10 +1115,10 @@ int cmd_branch(int argc,
> quiet, 0, NULL);
> goto out;
> } else if (forked) {
> - ret = list_forked_branches(argc, argv);
> + ret = list_forked_branches(argc, argv, all_remotes);
> goto out;
> } else if (prune_merged) {
> - ret = prune_merged_branches(argc, argv, quiet);
> + ret = prune_merged_branches(argc, argv, all_remotes, quiet);
> goto out;
> } else if (show_current) {
> print_current_branch_name();
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 885a275e36..a36e5ee80a 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1771,6 +1771,27 @@ test_expect_success '--forked requires at least one <remote>' '
> test_grep "at least one <remote>" err
> '
>
> +test_expect_success '--forked --all-remotes covers every configured remote' '
> + git -C forked branch --forked --all-remotes >actual &&
> + cat >expect <<-\EOF &&
> + local-foreign
> + local-one
> + local-two
> + main
> + EOF
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--forked --all-remotes still validates explicit <remote>' '
> + test_must_fail git -C forked branch --forked nope --all-remotes 2>err &&
> + test_grep "neither a configured remote nor a remote-tracking branch" err
> +'
> +
> +test_expect_success '--all-remotes alone is rejected' '
> + test_must_fail git -C forked branch --all-remotes 2>err &&
> + test_grep "requires --forked or --prune-merged" err
> +'
> +
> test_expect_success '--prune-merged: setup' '
> test_create_repo pm-upstream &&
> test_commit -C pm-upstream base &&
> @@ -1881,4 +1902,27 @@ test_expect_success 'branch -d still deletes a pruneMerged=false branch' '
> test_must_fail git -C pm-optout-d rev-parse --verify refs/heads/one
> '
>
> +test_expect_success '--prune-merged --all-remotes covers every configured remote' '
> + test_when_finished "rm -rf pm-allremotes pm-other" &&
> + git clone pm-upstream pm-allremotes &&
> + test_create_repo pm-other &&
> + test_commit -C pm-other other-base &&
> + git -C pm-other checkout -b stable &&
> + test_commit -C pm-other foreign-commit &&
> + git -C pm-other branch foreign HEAD &&
> + git -C pm-other checkout main &&
> +
> + git -C pm-allremotes remote add other ../pm-other &&
> + git -C pm-allremotes fetch other &&
> + git -C pm-allremotes branch one one-commit &&
> + git -C pm-allremotes branch --set-upstream-to=origin/next one &&
> + git -C pm-allremotes branch foreign other/foreign &&
> + git -C pm-allremotes branch --set-upstream-to=other/stable foreign &&
> +
> + git -C pm-allremotes branch --prune-merged --all-remotes &&
> +
> + test_must_fail git -C pm-allremotes rev-parse --verify refs/heads/one &&
> + test_must_fail git -C pm-allremotes rev-parse --verify refs/heads/foreign
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH v9 3/5] branch: add --prune-merged <remote>
From: Phillip Wood @ 2026-05-18 15:27 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Kristoffer Haugsbakk, Johannes Sixt, Harald Nordgren
In-Reply-To: <f87e96e99d64c48bd92afecf3a6a819d36e56f6c.1778700883.git.gitgitgadget@gmail.com>
Hi Harald
On 13/05/2026 20:34, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Delete the local branches that --forked <remote> would list, but
> only those whose tip is reachable from their configured upstream
> remote-tracking branch (branch.<name>.merge): the work has already
> landed on the upstream it tracks, so the local copy is no longer
> needed.
I think being able to prune branches that have been merged into their
upstream is a good idea. However, I find the focus on remotes rather
than upstream branches in the UI a bit confusing. While the upstream of
a branch is often a remote tracking branch it doesn't have to be. For my
personal projects I often do
git checkout -b topic master
and it would be nice to be able to run
git branch --prune-merged master
to clean up those topics that have been merged. Similarly I think it is
confusing that
git checkout -b topic origin
starts a branch from the default branch on origin, but if I run
git branch --prune-merged origin
to clean it up, it will clean up all the branches with an upstream on
origin, not just those whose upstream matches origin/HEAD.
So I like the idea, but would prefer the arguments to --prune-merged to
be upstream branches, not remotes. We could support globs so that
git branch --prune-merges 'origin/*'
would clean up all the branches whose upstream is on origin if that is
useful.
Thanks
Phillip
> A branch whose upstream no longer resolves locally is left alone --
> its disappearance is not, on its own, evidence that the work was
> integrated. With --force, skip the reachability check and delete
> every branch in the candidate set. The currently checked-out
> branch in any worktree is always preserved, as is the local branch
> that mirrors <remote>'s default branch.
>
> Reachability is read from whatever the remote-tracking refs say
> locally, so the natural workflow is
>
> git fetch <remote>
> git branch --prune-merged <remote>
>
> with no implicit cleanup driven by fetch itself.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
> Documentation/git-branch.adoc | 19 +++++
> builtin/branch.c | 143 +++++++++++++++++++++++++++++-----
> t/t3200-branch.sh | 83 ++++++++++++++++++++
> 3 files changed, 226 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/git-branch.adoc b/Documentation/git-branch.adoc
> index 5773104cd3..375a0a68da 100644
> --- a/Documentation/git-branch.adoc
> +++ b/Documentation/git-branch.adoc
> @@ -25,6 +25,7 @@ git branch (-c|-C) [<old-branch>] <new-branch>
> git branch (-d|-D) [-r] <branch-name>...
> git branch --edit-description [<branch-name>]
> git branch --forked <remote>...
> +git branch --prune-merged <remote>...
>
> DESCRIPTION
> -----------
> @@ -211,6 +212,24 @@ Each _<remote>_ may be either the name of a configured remote
> `refs/remotes/origin/*` ref) or a specific remote-tracking branch
> (e.g. `origin/master`). Multiple _<remote>_ arguments are unioned.
>
> +`--prune-merged`::
> + Delete the local branches that `--forked` would list for
> + the same _<remote>_ arguments, but only those whose tip is
> + reachable from their configured upstream remote-tracking
> + branch (`branch.<name>.merge`). In other words: the work on
> + the branch has already landed on the upstream it tracks, so
> + the local copy is no longer needed.
> ++
> +Run `git fetch` first so the upstream remote-tracking branches
> +reflect the current state of _<remote>_; reachability is checked
> +against whatever the remote-tracking refs say locally.
> ++
> +A branch whose upstream no longer resolves locally is left alone
> +(its disappearance is not, on its own, evidence that the work was
> +integrated). The currently checked-out branch in any worktree is
> +always preserved, as is the local branch that mirrors _<remote>_'s
> +default branch.
> +
> `-v`::
> `-vv`::
> `--verbose`::
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 1941f8a9ad..6fe2ffd7e8 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -21,6 +21,7 @@
> #include "branch.h"
> #include "path.h"
> #include "string-list.h"
> +#include "strvec.h"
> #include "column.h"
> #include "utf8.h"
> #include "ref-filter.h"
> @@ -171,8 +172,8 @@ static int branch_merged(int kind, const char *name,
> * any of the following code, but during the transition period,
> * a gentle reminder is in order.
> */
> - if (head_rev != reference_rev) {
> - int expect = head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0;
> + if (head_rev && head_rev != reference_rev) {
> + int expect = repo_in_merge_bases(the_repository, rev, head_rev);
> if (expect < 0)
> exit(128);
> if (expect == merged)
> @@ -227,7 +228,9 @@ static void delete_branch_config(const char *branchname)
> strbuf_release(&buf);
> }
>
> -static int delete_branches(int argc, const char **argv, int force, int kinds,
> +static int delete_branches(int argc, const char **argv,
> + int no_head_fallback,
> + int force, int kinds,
> int quiet, int warn_only, int *n_not_merged)
> {
> struct commit *head_rev = NULL;
> @@ -262,7 +265,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> }
> branch_name_pos = strcspn(fmt, "%");
>
> - if (!force)
> + if (!force && !no_head_fallback)
> head_rev = lookup_commit_reference(the_repository, &head_oid);
>
> for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
> @@ -317,8 +320,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> }
>
> if (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
> - check_branch_commit(bname.buf, name, &oid, head_rev, kinds,
> - force, warn_only, n_not_merged)) {
> + check_branch_commit(bname.buf, name, &oid, head_rev,
> + kinds, force, warn_only, n_not_merged)) {
> if (!warn_only)
> ret = 1;
> goto next;
> @@ -753,36 +756,131 @@ static int collect_forked_branch(const struct reference *ref, void *cb_data)
> return 0;
> }
>
> -static int list_forked_branches(int argc, const char **argv)
> +static void collect_default_branch_refs(const struct string_list *remote_names,
> + struct string_list *out)
> +{
> + struct ref_store *refs = get_main_ref_store(the_repository);
> + struct string_list_item *item;
> +
> + for_each_string_list_item(item, remote_names) {
> + struct strbuf head = STRBUF_INIT;
> + const char *target;
> +
> + strbuf_addf(&head, "refs/remotes/%s/HEAD", item->string);
> + target = refs_resolve_ref_unsafe(refs, head.buf,
> + RESOLVE_REF_NO_RECURSE,
> + NULL, NULL);
> + if (target && starts_with(target, "refs/remotes/"))
> + string_list_insert(out, target);
> + strbuf_release(&head);
> + }
> +}
> +
> +static void collect_forked_set(int argc, const char **argv,
> + struct string_list *protected_default_refs,
> + struct string_list *out)
> {
> struct string_list remote_names = STRING_LIST_INIT_NODUP;
> struct string_list tracking_refs = STRING_LIST_INIT_DUP;
> - struct string_list out = STRING_LIST_INIT_DUP;
> - struct string_list_item *item;
> struct forked_cb cb = {
> .remote_names = &remote_names,
> .tracking_refs = &tracking_refs,
> - .out = &out,
> + .out = out,
> };
>
> - if (!argc)
> - die(_("--forked requires at least one <remote>"));
> -
> parse_forked_args(argc, argv, &remote_names, &tracking_refs);
>
> refs_for_each_branch_ref(get_main_ref_store(the_repository),
> collect_forked_branch, &cb);
>
> - string_list_sort(&out);
> - for_each_string_list_item(item, &out)
> - puts(item->string);
> + string_list_sort(out);
> +
> + if (protected_default_refs)
> + collect_default_branch_refs(&remote_names, protected_default_refs);
>
> string_list_clear(&remote_names, 0);
> string_list_clear(&tracking_refs, 0);
> +}
> +
> +static int list_forked_branches(int argc, const char **argv)
> +{
> + struct string_list out = STRING_LIST_INIT_DUP;
> + struct string_list_item *item;
> +
> + if (!argc)
> + die(_("--forked requires at least one <remote>"));
> +
> + collect_forked_set(argc, argv, NULL, &out);
> + for_each_string_list_item(item, &out)
> + puts(item->string);
> +
> string_list_clear(&out, 0);
> return 0;
> }
>
> +static int prune_merged_branches(int argc, const char **argv, int quiet)
> +{
> + struct string_list candidates = STRING_LIST_INIT_DUP;
> + struct string_list protected_default_refs = STRING_LIST_INIT_DUP;
> + struct strvec deletable = STRVEC_INIT;
> + struct string_list_item *item;
> + int n_not_merged = 0;
> + int ret = 0;
> +
> + if (!argc)
> + die(_("--prune-merged requires at least one <remote>"));
> +
> + collect_forked_set(argc, argv, &protected_default_refs, &candidates);
> +
> + for_each_string_list_item(item, &candidates) {
> + const char *short_name = item->string;
> + struct strbuf full = STRBUF_INIT;
> + struct branch *branch;
> + const char *upstream;
> +
> + strbuf_addf(&full, "refs/heads/%s", short_name);
> + if (branch_checked_out(full.buf)) {
> + strbuf_release(&full);
> + continue;
> + }
> + strbuf_release(&full);
> +
> + branch = branch_get(short_name);
> + upstream = branch ? branch_get_upstream(branch, NULL) : NULL;
> + if (!upstream ||
> + !refs_ref_exists(get_main_ref_store(the_repository),
> + upstream))
> + continue;
> + if (string_list_has_string(&protected_default_refs, upstream)) {
> + const char *leaf = strrchr(upstream, '/');
> + if (leaf && !strcmp(leaf + 1, short_name))
> + continue;
> + }
> +
> + strvec_push(&deletable, short_name);
> + }
> +
> + if (deletable.nr)
> + ret = delete_branches(deletable.nr, deletable.v,
> + 1, 0,
> + FILTER_REFS_BRANCHES, quiet,
> + 1, &n_not_merged);
> +
> + if (n_not_merged && !quiet)
> + fprintf(stderr,
> + Q_("Skipped %d branch that is not fully merged; "
> + "delete it with 'git branch -D' if you are sure.\n",
> + "Skipped %d branches that are not fully merged; "
> + "delete them with 'git branch -D' if you are sure.\n",
> + n_not_merged),
> + n_not_merged);
> +
> + strvec_clear(&deletable);
> + string_list_clear(&candidates, 0);
> + string_list_clear(&protected_default_refs, 0);
> + return ret;
> +}
> +
> static GIT_PATH_FUNC(edit_description, "EDIT_DESCRIPTION")
>
> static int edit_branch_description(const char *branch_name)
> @@ -825,6 +923,7 @@ int cmd_branch(int argc,
> int delete = 0, rename = 0, copy = 0, list = 0,
> unset_upstream = 0, show_current = 0, edit_description = 0;
> int forked = 0;
> + int prune_merged = 0;
> const char *new_upstream = NULL;
> int noncreate_actions = 0;
> /* possible options */
> @@ -880,6 +979,8 @@ int cmd_branch(int argc,
> N_("edit the description for the branch")),
> OPT_BOOL(0, "forked", &forked,
> N_("list local branches forked from the given <remote>s")),
> + OPT_BOOL(0, "prune-merged", &prune_merged,
> + N_("delete local branches forked from the given <remote>s that are merged into their upstream")),
> OPT__FORCE(&force, N_("force creation, move/rename, deletion"), PARSE_OPT_NOCOMPLETE),
> OPT_MERGED(&filter, N_("print only branches that are merged")),
> OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
> @@ -924,7 +1025,8 @@ int cmd_branch(int argc,
> 0);
>
> if (!delete && !rename && !copy && !edit_description && !new_upstream &&
> - !show_current && !unset_upstream && !forked && argc == 0)
> + !show_current && !unset_upstream && !forked && !prune_merged &&
> + argc == 0)
> list = 1;
>
> if (filter.with_commit || filter.no_commit ||
> @@ -933,7 +1035,7 @@ int cmd_branch(int argc,
>
> noncreate_actions = !!delete + !!rename + !!copy + !!new_upstream +
> !!show_current + !!list + !!edit_description +
> - !!unset_upstream + !!forked;
> + !!unset_upstream + !!forked + !!prune_merged;
> if (noncreate_actions > 1)
> usage_with_options(builtin_branch_usage, options);
>
> @@ -971,12 +1073,15 @@ int cmd_branch(int argc,
> if (delete) {
> if (!argc)
> die(_("branch name required"));
> - ret = delete_branches(argc, argv, delete > 1, filter.kind,
> + ret = delete_branches(argc, argv, 0, delete > 1, filter.kind,
> quiet, 0, NULL);
> goto out;
> } else if (forked) {
> ret = list_forked_branches(argc, argv);
> goto out;
> + } else if (prune_merged) {
> + ret = prune_merged_branches(argc, argv, quiet);
> + goto out;
> } else if (show_current) {
> print_current_branch_name();
> ret = 0;
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 24a3ec44ee..94ea493aee 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1771,4 +1771,87 @@ test_expect_success '--forked requires at least one <remote>' '
> test_grep "at least one <remote>" err
> '
>
> +test_expect_success '--prune-merged: setup' '
> + test_create_repo pm-upstream &&
> + test_commit -C pm-upstream base &&
> + git -C pm-upstream checkout -b next &&
> + test_commit -C pm-upstream one-commit &&
> + test_commit -C pm-upstream two-commit &&
> + git -C pm-upstream branch one HEAD~ &&
> + git -C pm-upstream branch two HEAD &&
> + git -C pm-upstream branch wip main &&
> + git -C pm-upstream checkout main
> +'
> +
> +test_expect_success '--prune-merged deletes branches integrated into upstream' '
> + test_when_finished "rm -rf pm-merged" &&
> + git clone pm-upstream pm-merged &&
> + git -C pm-merged branch one one-commit &&
> + git -C pm-merged branch --set-upstream-to=origin/next one &&
> + git -C pm-merged branch two two-commit &&
> + git -C pm-merged branch --set-upstream-to=origin/next two &&
> +
> + git -C pm-merged branch --prune-merged origin &&
> +
> + test_must_fail git -C pm-merged rev-parse --verify refs/heads/one &&
> + test_must_fail git -C pm-merged rev-parse --verify refs/heads/two
> +'
> +
> +test_expect_success '--prune-merged spares branches with un-integrated commits' '
> + test_when_finished "rm -rf pm-unmerged" &&
> + git clone pm-upstream pm-unmerged &&
> + git -C pm-unmerged checkout -b wip origin/wip &&
> + git -C pm-unmerged branch --set-upstream-to=origin/next wip &&
> + test_commit -C pm-unmerged local-only &&
> + git -C pm-unmerged checkout - &&
> +
> + git -C pm-unmerged branch --prune-merged origin 2>err &&
> + test_grep "not fully merged" err &&
> + test_grep "Skipped 1 branch" err &&
> + test_grep "git branch -D" err &&
> + test_grep ! "If you are sure you want to delete it" err &&
> + git -C pm-unmerged rev-parse --verify refs/heads/wip
> +'
> +
> +test_expect_success '--prune-merged skips branches whose upstream is gone' '
> + test_when_finished "rm -rf pm-upstream-gone" &&
> + git clone pm-upstream pm-upstream-gone &&
> + git -C pm-upstream-gone branch one one-commit &&
> + git -C pm-upstream-gone branch --set-upstream-to=origin/next one &&
> +
> + git -C pm-upstream-gone update-ref -d refs/remotes/origin/next &&
> + git -C pm-upstream-gone branch --prune-merged origin &&
> +
> + git -C pm-upstream-gone rev-parse --verify refs/heads/one
> +'
> +
> +test_expect_success '--prune-merged never deletes the checked-out branch' '
> + test_when_finished "rm -rf pm-head" &&
> + git clone pm-upstream pm-head &&
> + git -C pm-head checkout -b one one-commit &&
> + git -C pm-head branch --set-upstream-to=origin/next one &&
> +
> + git -C pm-head branch --prune-merged origin &&
> +
> + git -C pm-head rev-parse --verify refs/heads/one
> +'
> +
> +test_expect_success '--prune-merged spares the local default branch' '
> + test_when_finished "rm -rf pm-default" &&
> + git clone pm-upstream pm-default &&
> + git -C pm-default checkout --detach &&
> + git -C pm-default branch --prune-merged origin &&
> + git -C pm-default rev-parse --verify refs/heads/main
> +'
> +
> +test_expect_success '--prune-merged protects only the default branch by name, not by upstream' '
> + test_when_finished "rm -rf pm-default-alias" &&
> + git clone pm-upstream pm-default-alias &&
> + git -C pm-default-alias branch --track trunk origin/main &&
> + git -C pm-default-alias checkout --detach &&
> + git -C pm-default-alias branch --prune-merged origin &&
> + git -C pm-default-alias rev-parse --verify refs/heads/main &&
> + test_must_fail git -C pm-default-alias rev-parse --verify refs/heads/trunk
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH] merge: use repo_in_merge_bases for octopus up-to-date check
From: Kristofer Karlsson @ 2026-05-18 14:38 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <c5b333f1-0db6-4aec-a369-6503cb924e7f@gmail.com>
Good question! No, this was intended only as a code cleanup and
semantic simplification.
The code path seems like an edge case, so benchmarking it did not seem
worthwhile.
The main win as I see it is clearer semantics for what it's doing and
the optimization is just a bonus.
Checking if repo_get_merge_bases(HEAD, J) == J is better expressed as
repo_in_merge_bases(J, HEAD)
repo_in_merge_bases should probably be renamed to something like
repo_is_ancestor, since its comment says:
Is "commit" an ancestor of (i.e. reachable from) the "reference"?
but I can understand that it may be painful to rename it in practice.
If I do a mental rename, this becomes simpler to reason about:
Checking if repo_get_merge_bases(HEAD, J) == J is better expressed as
repo_is_ancestor(J, HEAD)
- Kristofer
On Mon, 18 May 2026 at 14:20, Derrick Stolee <stolee@gmail.com> wrote:
>
> On 5/12/2026 2:11 AM, Kristofer Karlsson via GitGitGadget wrote:
> > From: Kristofer Karlsson <krka@spotify.com>
> >
> > The octopus merge path checks whether each remote head is already
> > an ancestor of HEAD by computing all merge-bases via
> > repo_get_merge_bases() and comparing the first result's OID to
> > the remote head. This is more expensive than necessary:
> > repo_get_merge_bases() calls paint_down_to_common() with
> > min_generation=0, performs the full STALE drain, and may run
> > remove_redundant(), when all we need is a yes/no reachability
> > answer.
> >
> > Replace this with repo_in_merge_bases(), which answers the
> > is-ancestor question directly. When generation numbers are
> > available, repo_in_merge_bases() uses can_all_from_reach() -- a
> > DFS bounded by generation number that stops as soon as the target
> > is found or ruled out, without entering paint_down_to_common() at
> > all. Without generation numbers, it still benefits from a tighter
> > min_generation floor.
> >
> > Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> > ---
> > merge: use repo_in_merge_bases for octopus up-to-date check
> >
> > While reviewing callers of repo_get_merge_bases() for a different patch,
> > I noticed the octopus up-to-date loop in builtin/merge.c computes full
> > merge-bases only to check whether each remote head is an ancestor of
> > HEAD.
> >
> > The existing code calls repo_get_merge_bases(), takes the first result,
> > frees the list, and compares the OID to the remote head. This is
> > equivalent to an is-ancestor check, which repo_in_merge_bases() answers
> > directly.
> >
> > Using repo_in_merge_bases() simplifies the code (-14/+4 lines) and
> > avoids unnecessary work: with generation numbers it uses
> > can_all_from_reach() instead of paint_down_to_common(), and without
> > generation numbers it still benefits from a tighter min_generation
> > floor. In practice this only matters for octopus merges on repos with
> > deep history, so the main value here is the simplification.
>
> The code change looks right to me. Do you have any performance numbers
> to share? Or was this motivated mostly as an opportunity for code cleanup?
>
> Thanks,
> -Stolee
>
>
^ permalink raw reply
* Re: [GSoC RFC PATCH 0/1] graph: add indentation for commits preceded by a root
From: Pablo Sabater @ 2026-05-18 13:26 UTC (permalink / raw)
To: Chandra Pratap
Cc: phillip.wood, git, gitster, christian.couder, karthik.188,
jltobler, ayu.chandekar, siddharthasthana31
In-Reply-To: <CA+J6zkTGgeNuH0eusTy+t8LO3bjygSz4svJB=K4R5ASmBdd0uQ@mail.gmail.com>
Hi Chandra, Phillip,
> > >
> > > I have mixed feelings about which approach to choose.
> > > The idea of a blank line was thought at
> > > https://lore.kernel.org/git/xmqq8s8vvw9m.fsf@gitster.c.googlers.com/
> > > but Junio argued against it for having an extra row because the
> > > indentation he proposed didn't collapse, however I find indentation +
> > > no collapse the most confusing one.
> > > I'd say that I'm fine with both approaches, blank line or indentation
> > > + collapse.
> >
> > I'm afraid I don't understand this - what does it mean for the
> > indentation to collapse, or not collapse.
Collapsing would be when branches move to the left, eg:
*
|\ <- merge
| *
|/ <- collapse
*
> > Looking at the examples Junio
> > gave they look quite nice to me, though I'd find it clearer if
> >
> >
> > | | * 12345678 2021-01-14 merge xxxxx@xxxx into the history
> > | | |\
> > | | | \
> > | | * \ 23456789 2021-01-12 merge citest into the main history
> > | | |\ * 5505e019c2 2014-07-09 initial xxxxxx@xxxx
> > | | | * 3e658f4085 2019-09-10 (wiki/wip-citest, origin/wip-citest)
> > Added defau
> > | | | * ad148aafe6 2019-09-10 Added default CI/CD Jenkinsfile (from
> > f7daf088)
> >
> > was rendered as
> >
> >
> > | | * 12345678 2021-01-14 merge xxxxx@xxxx into the history
> > | | |\
> > | | | * 5505e019c2 2014-07-09 initial xxxxxx@xxxx
> > | | * 23456789 2021-01-12 merge citest into the main history
> > | | |\
> > | | | * 3e658f4085 2019-09-10 (wiki/wip-citest, origin/wip-citest)
> > Added defau
> > | | | * ad148aafe6 2019-09-10 Added default CI/CD Jenkinsfile (from
> > f7daf088)
>
> It probably *does* look clearer here, but I have the same reservations
> against this as Junio: the break won't be as noticeable when --graph is
> *not* used with --oneline.
>
> > >>> without the patch:
> > >>>
> > >>> * A root
> > >>> * B root
> > >>> * C root
> > >>> * D1 child
> > >>> * D root
> > >>>
> > >>> with the patch, the indentation cascades:
> > >>>
> > >>> * A root
> > >>> * B root
> > >>> * C root
> > >>> * D1 child
> > >>> _ /
> > >>> /
> > >>> /
> > >>> * D root
> > >
> > > * A root
> > >
> > > * B root
> > >
> > > * C root
> > >
> > > * D1 child
> > >
> > > * D root
> > >
> > > Here I think a blank line looks worse, too much space for just 5
> > > commits and becomes one extra line which if this were like up to 7 or
> > > more parentless commits one after the other would be more noticeable.
> >
> > But there shouldn't be a blank line between D and D1 so the two
> > alternatives take up the same amount of vertical space, the main
> > difference being whether D1 appears next to D
> >
> > * A root * A root
> > * B root
> > * B root * C root
> > * D1 child
> > * C root _/
> > /
> > * D1 child /
> > * D root * D root
> >
> > Of course if the indentation was smarter it would take up less room and
> > look better than having blank lines
> >
> > * A root
> > * B root
> > * C root
> > * D1 child
> > * D root
>
> Right, this would be ideal but that would require too much change to the
> existing graphing logic, and should be its own patch.
For the examples I'll use the term parentless instead of root, as
boundary commits are excluded even if they are roots.
By having is_parentless as a flag in 'git_graph' that every stage can
access we could modify the rendering and maybe completely drop the
commit placeholders, working on it for v4 but currently renders like
this
* A parentless
* B parentless
* C parentless
* D1 child
* D parentless
(A has indentation when it could not have, but that would require a
lookahead if the next commit is also parentless)
But definitely a step forward.
Do we want cascading or just a fixed indentation?
* A parentless
* B parentless
* C parentless
* D1 child
* D parentless
By being indented it indicates that it is parentless and that the one
below doesn't relate to it, but cascading looks clearer.
>
> > > But there are cases that blank line might be better:
> > >
> > > * 10_A2
> > > * 10_A1
> > > * 10_A
> > > * 10_M
> > > /|\
> > > | | * 10_D
> > > | * 10_C
> > > * 10_B
> > >
> > > Feels like a shower of commits instead of an indented merge.
> >
> > Yes, that is a bit confusing. I think the thing I find confusing with
> > this approach is that we're treating the commit rendered below the root
> > commit specially, rather than treating the root commit itself specially.
> > To me it is the root commit that's the odd one out because it does not
> > have any parents, but we treat the commit that's rendered below as the
> > odd one by indenting it relative to its parents.
>
> I guess that would make the examples look something like this:
>
> * A root
> * B root
> * C root
> * D1 child
> * D root
>
> No cascading, and no need for that massive _ / collapse line.
>
> * 10_A2
> * 10_A1
> \
> * 10_A
> * 10_M
> | \ \
> | | * 10_D
> | * 10_C
> * 10_B
>
> I say it looks better than the alternatives, but I'm not sure if this will
> be easy to implement. The diagonal connection line (\) will need to
> be printed before printing the actual root commit, which will require
> lookahead logic.
>
> I'd prefer to avoid major surgery on the codebase.
Octopus merges need a pre-commit phase where an additional row
increases the space around a commit with multiple parents to make room
for it.
A new phase can be created similarly to pre-commit as pre-root where
the connection edge (\) can be printed before the indented commit.
So far this is the comparison:
indentation at root:
* A parentless
* B1 child
\
* B parentless
* C1 child
* C parentless
indentation AFTER the root (current v3):
* A parentless
* B1 child
/
* B parentless
* C1 child
/
* C parentless
Karthik mentioned that by indenting the parentless, we lose the
consistency of having the roots on their real column and now some are
indented and some are not.
The biggest winner having the parentless indented are the merge commits:
* A child
* A child
\
* A parentless
*-. B child
| \ \
| | * C parentless
| * D parentless
* E parentless
which IMO looks clearer than the commit shower:
* A child
* A child
* A parentless
* B child
/|\
| | * C parentless
| * D parentless
* E parentless
>
>
> Thanks,
> Chandra.
Regards
--
Pablo
^ permalink raw reply
* Re: [PATCH v3 0/4] Batch prefetching
From: Junio C Hamano @ 2026-05-18 12:44 UTC (permalink / raw)
To: Derrick Stolee
Cc: Elijah Newren via GitGitGadget, git, Elijah Newren, Phillip Wood
In-Reply-To: <0da4f159-8d4b-49e2-93c1-25aa0bf69371@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
> On 5/14/2026 12:25 PM, Elijah Newren via GitGitGadget wrote:
>> Changes since v2:
>>
>> * Modified the final patch as suggested by Stolee to include pathspec usage
>> in the testcase
>> * Modified the last two patches to not re-download blobs we already have
>> locally, and adjusted the tests to verify
>> * Inserted a new first patch, containing a documentation addition that
>> would have helped me avoid making the above mistake in the first place.
>
> Thank you for these changes. I reviewed the updates and documentation and
> think this version is good to go.
>> Note: Stolee also suggest some code sharing or code movement in his review
>> of v2 2/3, but possibly based on a misunderstanding of v2 2/3 (that patch
>> isn't about a diff) and it's not clear to me what could be shared or moved,
>> so that's not part of this round.
>
> Your detailed responses in the v2 thread helped me understand that my thought
> was misguided. Thanks for giving me extra confidence in your approach here.
Thanks, both. I do agree that the series is in a good shape. Let
me mark the topic for 'next'.
^ permalink raw reply
* Re: [PATCH 1/1] commit: allow -m/-F with --fixup=amend: or reword:
From: Junio C Hamano @ 2026-05-18 12:39 UTC (permalink / raw)
To: erik; +Cc: git, charvi077
In-Reply-To: <20260518112225.73172-4-erik@cervined.in>
erik@cervined.in writes:
> From: Erik Cervin-Edin <erik@cervined.in>
The name on this overriding in-body From: line, and the name on
Signed-off-by: line below, must match. Please pick a name with or
without hyphen and stick to it.
> --fixup=amend: and --fixup=reword: require an editor to supply the
> replacement commit message. The -m and -F flags are rejected: -m is
> caught by a die() in prepare_to_commit(), and -F is caught by
> die_for_incompatible_opt4() which groups -F with --fixup as mutually
> exclusive. This makes these modes unusable in non-interactive
> workflows -- notably AI coding agents.
"Unusable" may be stronger than reality, as you can make creatie use
of GIT_EDITOR to achieve what you want. "awkward" or "poorly suited"
would be more fitting.
> Plain --fixup (without amend: or reword:) continues to reject -F but
> still accepts -m (even though it's practically a no-op).
Is it "practically a no-op"? Wouldn't
$ git commit --fixup <commit> -m "message body"
be useful to leave a message in the resulting commit, which is later
to be squashed into the named <commit>? Actually squashing with "fixup!"
may lose the message supplied here, but wouldn't people use this
facility to more easily identify what each of the fixups are about?
For the same reason, "-F" would be just as useful as "-m" in this context,
and it feels a bit inconsistent to allow one while rejecting the other.
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 28f6174503..269c2d782b 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -837,21 +837,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> hook_arg1 = "message";
>
> /*
> - * Only `-m` commit message option is checked here, as
> - * it supports `--fixup` to append the commit message.
> - *
> - * The other commit message options `-c`/`-C`/`-F` are
> - * incompatible with all the forms of `--fixup` and
> - * have already errored out while parsing the `git commit`
> - * options.
> + * `-m` (and `-F`, converted to `-m` earlier for
> + * amend/reword) appends the message body here.
> + * `-c`/`-C` are still incompatible with all forms
> + * of `--fixup`.
> */
> if (have_option_m && !strcmp(fixup_prefix, "fixup"))
> strbuf_addbuf(&sb, &message);
>
> if (!strcmp(fixup_prefix, "amend")) {
> if (have_option_m)
> - die(_("options '%s' and '%s:%s' cannot be used together"), "-m", "--fixup", fixup_message);
Good that you got rid of this overly long die() message line.
> - prepare_amend_commit(commit, &sb, &ctx);
> + strbuf_addbuf(&sb, &message);
> + else
> + prepare_amend_commit(commit, &sb, &ctx);
> }
> } else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
> size_t merge_msg_start;
> @@ -1338,10 +1336,12 @@ static int parse_and_validate_options(int argc, const char *argv[],
> }
> if (fixup_message && squash_message)
> die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup");
> - die_for_incompatible_opt4(!!use_message, "-C",
> + die_for_incompatible_opt3(!!use_message, "-C",
> !!edit_message, "-c",
> - !!logfile, "-F",
> !!fixup_message, "--fixup");
> + die_for_incompatible_opt3(!!use_message, "-C",
> + !!edit_message, "-c",
> + !!logfile, "-F");
> die_for_incompatible_opt4(have_option_m, "-m",
> !!edit_message, "-c",
> !!use_message, "-C",
> @@ -1410,6 +1410,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
> }
> }
>
> + if (logfile && fixup_message && !strcmp(fixup_prefix, "fixup"))
> + die(_("options '%s' and '%s' cannot be used together"), "-F", "--fixup");
> +
> if (0 <= edit_flag)
> use_editor = edit_flag;
>
> @@ -1821,6 +1824,22 @@ int cmd_commit(int argc,
> argc = parse_and_validate_options(argc, argv, builtin_commit_options,
> builtin_commit_usage,
> prefix, current_head, &s);
> +
> + if (logfile && fixup_message && !strcmp(fixup_prefix, "amend")) {
> + if (!strcmp(logfile, "-")) {
> + if (isatty(0))
> + fprintf(stderr, _("(reading log message from standard input)\n"));
> + if (strbuf_read(&message, 0, 0) < 0)
> + die_errno(_("could not read log from standard input"));
> + } else {
> + if (strbuf_read_file(&message, logfile, 0) < 0)
> + die_errno(_("could not read log file '%s'"), logfile);
> + }
> + strbuf_complete_line(&message);
> + have_option_m = 1;
> + FREE_AND_NULL(logfile);
> + }
> +
It is curious that for this new feature alone, but not the other
existing code paths, "-m" and "-F" options reads from file in the
new code here, instead of letting the existing code for "-F" to read
(which happens inside prepare_to_commit(), I presume?).
A potential problem of the above code is if we find something wrong
in message and complain later in the control flow, we have long lost
where the message came from, as the point of the above code is
exactly to pretend that "--fixup:amend/reword -F" message did *not*
come from a file with the "-F" option, but from the command line via
the "-m" option.
> +test_expect_success '--fixup=amend: with -m option' '
> commit_for_rebase_autosquash_setup &&
> - echo "fatal: options '\''-m'\'' and '\''--fixup:reword'\'' cannot be used together" >expect &&
> - test_must_fail git commit --fixup=reword:HEAD~ -m "reword commit message" 2>actual &&
> - test_cmp expect actual
> + cat >expected <<-EOF &&
This comment is not about the added logic, but I notice that among
86 hits with string "expect" in this file in today's "master", only
14 hits are with string "expected", i.e., the prevalent name for the
"golden copy result" that is compared with the actula result (called
"actual") is "expect", not "expected". Please do not make the
situation worse.
> - test_cmp expect actual
> + cat >expected <<-EOF &&
Ditto.
^ permalink raw reply
* Re: [PATCH] merge: use repo_in_merge_bases for octopus up-to-date check
From: Derrick Stolee @ 2026-05-18 12:20 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget, git; +Cc: Kristofer Karlsson
In-Reply-To: <pull.2110.git.1778566286543.gitgitgadget@gmail.com>
On 5/12/2026 2:11 AM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> The octopus merge path checks whether each remote head is already
> an ancestor of HEAD by computing all merge-bases via
> repo_get_merge_bases() and comparing the first result's OID to
> the remote head. This is more expensive than necessary:
> repo_get_merge_bases() calls paint_down_to_common() with
> min_generation=0, performs the full STALE drain, and may run
> remove_redundant(), when all we need is a yes/no reachability
> answer.
>
> Replace this with repo_in_merge_bases(), which answers the
> is-ancestor question directly. When generation numbers are
> available, repo_in_merge_bases() uses can_all_from_reach() -- a
> DFS bounded by generation number that stops as soon as the target
> is found or ruled out, without entering paint_down_to_common() at
> all. Without generation numbers, it still benefits from a tighter
> min_generation floor.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> merge: use repo_in_merge_bases for octopus up-to-date check
>
> While reviewing callers of repo_get_merge_bases() for a different patch,
> I noticed the octopus up-to-date loop in builtin/merge.c computes full
> merge-bases only to check whether each remote head is an ancestor of
> HEAD.
>
> The existing code calls repo_get_merge_bases(), takes the first result,
> frees the list, and compares the OID to the remote head. This is
> equivalent to an is-ancestor check, which repo_in_merge_bases() answers
> directly.
>
> Using repo_in_merge_bases() simplifies the code (-14/+4 lines) and
> avoids unnecessary work: with generation numbers it uses
> can_all_from_reach() instead of paint_down_to_common(), and without
> generation numbers it still benefits from a tighter min_generation
> floor. In practice this only matters for octopus merges on repos with
> deep history, so the main value here is the simplification.
The code change looks right to me. Do you have any performance numbers
to share? Or was this motivated mostly as an opportunity for code cleanup?
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v3 0/4] Batch prefetching
From: Derrick Stolee @ 2026-05-18 12:17 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Elijah Newren, Phillip Wood
In-Reply-To: <pull.2089.v3.git.1778775928.gitgitgadget@gmail.com>
On 5/14/2026 12:25 PM, Elijah Newren via GitGitGadget wrote:
> Changes since v2:
>
> * Modified the final patch as suggested by Stolee to include pathspec usage
> in the testcase
> * Modified the last two patches to not re-download blobs we already have
> locally, and adjusted the tests to verify
> * Inserted a new first patch, containing a documentation addition that
> would have helped me avoid making the above mistake in the first place.
Thank you for these changes. I reviewed the updates and documentation and
think this version is good to go.
> Note: Stolee also suggest some code sharing or code movement in his review
> of v2 2/3, but possibly based on a misunderstanding of v2 2/3 (that patch
> isn't about a diff) and it's not clear to me what could be shared or moved,
> so that's not part of this round.
Your detailed responses in the v2 thread helped me understand that my thought
was misguided. Thanks for giving me extra confidence in your approach here.
-Stolee
^ permalink raw reply
* Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`
From: Derrick Stolee @ 2026-05-18 12:14 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <CABPp-BGpXgDfJeDEB91U-h092-8L6Q_MLrzSLFg9HotPDZ-m-g@mail.gmail.com>
On 5/13/2026 7:17 PM, Elijah Newren wrote:
> On Mon, Apr 27, 2026 at 6:17 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 4/17/2026 8:32 PM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>> +static void collect_diff_blob_oids(struct commit *commit,
>>> + struct diff_options *opts,
>>> + struct oidset *blobs)
>>
>> I think that this is generally a good idea, though I worry that
>> having this hidden in builtin/log.c may not be the right long-
>> term home.
>>
>> I expect that we'll find more and more examples where we want to
>> prefetch blobs in different operations, those that exist now and
>> those that may be created in the future. It would be preferred if
>> they could automatically take advantage of the logic already in
>> diff_queued_diff_prefetch() within diffcore_std() in diff.c.
>>
>> Ultimately, _this_ patch cares about a diff.
>
> I read this patch a bit differently -- could you say more about what
> you have in mind?
>
> The body of collect_diff_blob_oids() really is just diff_tree_oid() +
> diffcore_std() + process each pair, so at the per-commit level I am
> already leaning on the diff library. One of the things this patch
> adds is accumulation across many commits: the containing loop (in
> prefetch_cherry_blobs) is over a commit range, not over a single diff.
>
> Concretely, the motivating case was a patch touching a few files where
> upstream had tens of thousands of commits in <limit>..<head>, several
> hundred of which modified the same set of files. A per-diff prefetch
> like diff.c uses would turn that into hundreds of small fetches of 1-3
> blobs each; what this series gives you is one fetch. So the win
> really does live above the diff library, not inside it.
My initial thought was about finding what we can abstract into the
diff API for later reuse. Upon rereading, it's clear that this is
tied very closely with the --cherry feature and wouldn't make a lot
of sense in the API layer.
> There are two further wrinkles in cherry that are filters layered on
> top of the cross-commit accumulation, and they're cherry-specific in a
> way that I don't think belongs in the diff library:
>
> 1. For most commits in <limit>..<head>, cherry doesn't care about
> the diff at all -- if the list of files modified doesn't exactly match
> the commit of interest, the commit is skipped before patch-id is even
> computed. Prefetching for those would be wasted.
>
> 2. We skip prefetching content for binary files (because patch-id
> uses oid_to_hex() for such files instead of the diff contents).
>
>> Could we compute a
>> "diff prep" computation using the core diff library instead of
>> inventing a second queue of results for diffing?
>
> To check this concretely I looked at each of the existing
> promisor_remote_get_direct() callsites for a similar producer. The
> closest cousin of collect_diff_blob_oids() (the only part of this
> patch that looks like it might be close to the right shape to put in a
> core diff library) is diff.c's diff_queued_diff_prefetch() -- but it
> operates on the already-populated global diff_queued_diff and fetches
> immediately, rather than setting up the diff itself and returning an
> oidset for the caller to accumulate. Reshaping it to match cherry's
> needs would either break its current caller in diffcore_std() or
> introduce a parallel function whose only consumer is cherry. None of
> the other sites (path-walk in backfill, index walk in read-cache,
> three-way state in merge-ort, etc.) do anything resembling "diff two
> trees and harvest oids."
>
> And even if we did factor a helper out, cherry's filter is
> patch-id-specific: commit_patch_id() substitutes oid_to_hex() for
> files marked binary by their userdiff driver, so we deliberately skip
> prefetching those. That isn't a generic "diff prep" consideration --
> it only makes sense because the caller is patch-id. We could express
> it as a predicate parameter, but with one caller that would feel to me
> like it's just pushing cherry's policy across an API boundary for no
> gain.
Thanks for the additional context. I agree with your assessment.
>> Patch 3 cares about a "scan prep" which cares about loading all
>> blobs for a given tree with respect to a pathspec. This is very
>> similar to what a checkout would do, though it ultimately uses
>> a form of diff to find out what change should be applied to the
>> working directory. Perhaps 'git archive' is a better matching
>> example.
>
> Agreed that archive is the closer analog -- both grep and archive do a
> pathspec-filtered single-tree walk, whereas checkout's prefetch is
> tied to the index and optimizes to the subset of paths that are
> different since the previous version checked out. Retrofitting that
> to grep would mean materializing an index for the target revision just
> to throw it away, which feels like more machinery to bridge the
> abstractions than the walk itself would take.
Makes sense.
>> By implementing things in a
>> common location, then we can have later integrations add to the
>> confidence in the feature through tests covering each user-facing
>> use.
>
> Sounds great...but what common user-facing uses exist?
>
> Looking at the existing 11 callsites of promisor_remote_get_direct()
> after this series [1], each has pretty specialized data needs --
> index-driven (read-cache), index-pack & pack-objects internals,
> path-walk batches (backfill), merge-ort's three-way logic,
> diffcore-rename's two independent rename-detection paths, plain old
> diffs, collection across a subset of commits (cherry),
> pathspec-filtered tree walk (grep), and
> on-demand-single-blob-at-a-time (odb.c) -- so I don't see a natural
> shared layer above the primitive itself (which is already
> promisor_remote_get_direct).
>
> archive, if it had prefetch logic, would be the first match. But it's
> not clear where the shared logic between grep and archive would live,
> if archive even had any prefetch logic to share.
>
> So I'm inclined to leave both new producers local to their builtins
> for now, and factor a tree-walk helper when archive (or a third
> caller) actually wants one. But I'm happy to be told I've missed the
> boat.
No, clearly I missed the boat. Thanks for giving me insight to your
deep understanding of this area. You executed on a good design based
on the right amount of specialization required for this need.
Thanks,
-Stolee
^ permalink raw reply
* [PATCH 1/1] commit: allow -m/-F with --fixup=amend: or reword:
From: erik @ 2026-05-18 11:22 UTC (permalink / raw)
To: git; +Cc: gitster, charvi077, Erik Cervin-Edin
In-Reply-To: <20260518112225.73172-2-erik@cervined.in>
From: Erik Cervin-Edin <erik@cervined.in>
--fixup=amend: and --fixup=reword: require an editor to supply the
replacement commit message. The -m and -F flags are rejected: -m is
caught by a die() in prepare_to_commit(), and -F is caught by
die_for_incompatible_opt4() which groups -F with --fixup as mutually
exclusive. This makes these modes unusable in non-interactive
workflows -- notably AI coding agents.
When the amend suboption was introduced in 494d314a05 (commit: add
amend suboption to --fixup to create amend! commit, 2021-03-15),
-m support for amend fixups was discussed but not pursued, and -F
was already caught by the higher-layer incompatibility check grouping
it with --fixup.
Allow -m and -F to supply the replacement message body for amend and
reword fixups. When provided, bypass the editor and directly use the
user's message as the body, replacing the original commit's message. For
-F, the file contents are read into the message strbuf and then handled
identically to -m.
Plain --fixup (without amend: or reword:) continues to reject -F but
still accepts -m (even though it's practically a no-op).
Signed-off-by: Erik Cervin Edin <erik@cervined.in>
---
Documentation/git-commit.adoc | 13 +++--
builtin/commit.c | 41 ++++++++++----
t/t7500-commit-template-squash-signoff.sh | 67 +++++++++++++++++++----
3 files changed, 92 insertions(+), 29 deletions(-)
diff --git a/Documentation/git-commit.adoc b/Documentation/git-commit.adoc
index 8329c1034b..9478d5d265 100644
--- a/Documentation/git-commit.adoc
+++ b/Documentation/git-commit.adoc
@@ -111,12 +111,13 @@ commit, but the additional commentary will be thrown away once the
The commit created by `--fixup=amend:<commit>` is similar but its
title is instead prefixed with "amend!". The log message of
_<commit>_ is copied into the log message of the "amend!" commit and
-opened in an editor so it can be refined. When `git rebase
---autosquash` squashes the "amend!" commit into _<commit>_, the
-log message of _<commit>_ is replaced by the refined log message
-from the "amend!" commit. It is an error for the "amend!" commit's
-log message to be empty unless `--allow-empty-message` is
-specified.
+opened in an editor so it can be refined. The replacement message may
+also be supplied directly using `-m` or `-F`, bypassing the need to open
+an editor. When `git rebase --autosquash` squashes the "amend!" commit
+into _<commit>_, the log message of _<commit>_ is replaced by the
+refined log message from the "amend!" commit. It is an error for the
+"amend!" commit's log message to be empty unless `--allow-empty-message`
+is specified.
+
`--fixup=reword:<commit>` is shorthand for `--fixup=amend:<commit>
--only`. It creates an "amend!" commit with only a log message
diff --git a/builtin/commit.c b/builtin/commit.c
index 28f6174503..269c2d782b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -837,21 +837,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
hook_arg1 = "message";
/*
- * Only `-m` commit message option is checked here, as
- * it supports `--fixup` to append the commit message.
- *
- * The other commit message options `-c`/`-C`/`-F` are
- * incompatible with all the forms of `--fixup` and
- * have already errored out while parsing the `git commit`
- * options.
+ * `-m` (and `-F`, converted to `-m` earlier for
+ * amend/reword) appends the message body here.
+ * `-c`/`-C` are still incompatible with all forms
+ * of `--fixup`.
*/
if (have_option_m && !strcmp(fixup_prefix, "fixup"))
strbuf_addbuf(&sb, &message);
if (!strcmp(fixup_prefix, "amend")) {
if (have_option_m)
- die(_("options '%s' and '%s:%s' cannot be used together"), "-m", "--fixup", fixup_message);
- prepare_amend_commit(commit, &sb, &ctx);
+ strbuf_addbuf(&sb, &message);
+ else
+ prepare_amend_commit(commit, &sb, &ctx);
}
} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
size_t merge_msg_start;
@@ -1338,10 +1336,12 @@ static int parse_and_validate_options(int argc, const char *argv[],
}
if (fixup_message && squash_message)
die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup");
- die_for_incompatible_opt4(!!use_message, "-C",
+ die_for_incompatible_opt3(!!use_message, "-C",
!!edit_message, "-c",
- !!logfile, "-F",
!!fixup_message, "--fixup");
+ die_for_incompatible_opt3(!!use_message, "-C",
+ !!edit_message, "-c",
+ !!logfile, "-F");
die_for_incompatible_opt4(have_option_m, "-m",
!!edit_message, "-c",
!!use_message, "-C",
@@ -1410,6 +1410,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
}
}
+ if (logfile && fixup_message && !strcmp(fixup_prefix, "fixup"))
+ die(_("options '%s' and '%s' cannot be used together"), "-F", "--fixup");
+
if (0 <= edit_flag)
use_editor = edit_flag;
@@ -1821,6 +1824,22 @@ int cmd_commit(int argc,
argc = parse_and_validate_options(argc, argv, builtin_commit_options,
builtin_commit_usage,
prefix, current_head, &s);
+
+ if (logfile && fixup_message && !strcmp(fixup_prefix, "amend")) {
+ if (!strcmp(logfile, "-")) {
+ if (isatty(0))
+ fprintf(stderr, _("(reading log message from standard input)\n"));
+ if (strbuf_read(&message, 0, 0) < 0)
+ die_errno(_("could not read log from standard input"));
+ } else {
+ if (strbuf_read_file(&message, logfile, 0) < 0)
+ die_errno(_("could not read log file '%s'"), logfile);
+ }
+ strbuf_complete_line(&message);
+ have_option_m = 1;
+ FREE_AND_NULL(logfile);
+ }
+
if (trailer_args.nr)
trailer_config_init();
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 66aff8e097..b7579ad789 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -384,18 +384,28 @@ test_expect_success '--fixup=reword: ignores staged changes' '
test_cmp foo actual
'
-test_expect_success '--fixup=reword: error out with -m option' '
+test_expect_success '--fixup=amend: with -m option' '
commit_for_rebase_autosquash_setup &&
- echo "fatal: options '\''-m'\'' and '\''--fixup:reword'\'' cannot be used together" >expect &&
- test_must_fail git commit --fixup=reword:HEAD~ -m "reword commit message" 2>actual &&
- test_cmp expect actual
+ cat >expected <<-EOF &&
+ amend! $(git log -1 --format=%s HEAD~)
+
+ amend commit message
+ EOF
+ git commit --fixup=amend:HEAD~ -m "amend commit message" &&
+ get_commit_msg HEAD >actual &&
+ test_cmp expected actual
'
-test_expect_success '--fixup=amend: error out with -m option' '
+test_expect_success '--fixup=reword: with -m option' '
commit_for_rebase_autosquash_setup &&
- echo "fatal: options '\''-m'\'' and '\''--fixup:amend'\'' cannot be used together" >expect &&
- test_must_fail git commit --fixup=amend:HEAD~ -m "amend commit message" 2>actual &&
- test_cmp expect actual
+ cat >expected <<-EOF &&
+ amend! $(git log -1 --format=%s HEAD~)
+
+ reword commit message
+ EOF
+ git commit --fixup=reword:HEAD~ -m "reword commit message" &&
+ get_commit_msg HEAD >actual &&
+ test_cmp expected actual
'
test_expect_success 'consecutive amend! commits remove amend! line from commit msg body' '
@@ -432,6 +442,12 @@ test_expect_success 'deny to create amend! commit if its commit msg body is empt
test_cmp expected actual
'
+test_expect_success '--fixup=amend: -m with empty message aborts' '
+ commit_for_rebase_autosquash_setup &&
+ test_must_fail git commit --fixup=amend:HEAD~ -m "" 2>err &&
+ test_grep "empty commit message body" err
+'
+
test_expect_success 'amend! commit allows empty commit msg body with --allow-empty-message' '
commit_for_rebase_autosquash_setup &&
cat >expected <<-EOF &&
@@ -468,10 +484,37 @@ test_expect_success '--fixup=reword: give error with pathsec' '
test_cmp expect actual
'
-test_expect_success '--fixup=reword: -F give error message' '
- echo "fatal: options '\''-F'\'' and '\''--fixup'\'' cannot be used together" >expect &&
- test_must_fail git commit --fixup=reword:HEAD~ -F msg 2>actual &&
- test_cmp expect actual
+test_expect_success '--fixup=reword: with -F option' '
+ commit_for_rebase_autosquash_setup &&
+ echo "message from file" >msgfile &&
+ cat >expected <<-EOF &&
+ amend! $(git log -1 --format=%s HEAD~)
+
+ message from file
+ EOF
+ git commit --fixup=reword:HEAD~ -F msgfile &&
+ get_commit_msg HEAD >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success '--fixup=amend: with -F option' '
+ commit_for_rebase_autosquash_setup &&
+ echo "amend message from file" >msgfile &&
+ cat >expected <<-EOF &&
+ amend! $(git log -1 --format=%s HEAD~)
+
+ amend message from file
+ EOF
+ git commit --fixup=amend:HEAD~ -F msgfile &&
+ get_commit_msg HEAD >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success '-F with plain --fixup still errors' '
+ commit_for_rebase_autosquash_setup &&
+ echo "message" >msgfile &&
+ test_must_fail git commit --fixup HEAD~ -F msgfile 2>err &&
+ test_grep "cannot be used together" err
'
test_expect_success 'commit --squash works with -F' '
--
2.54.0.772.g683d7313b1
^ permalink raw reply related
* [PATCH 0/1] commit: allow -m/-F with --fixup=amend: or reword:
From: erik @ 2026-05-18 11:22 UTC (permalink / raw)
To: git; +Cc: gitster, charvi077, Erik Cervin-Edin
From: Erik Cervin-Edin <erik@cervined.in>
The commit --fixup=reword: (and --fixup:amend) options are powerful but
currently not well-suited for non-interactive workflows.
I often find myself hacking away on a branch and the last thing I do is
finalize and formulate the commit messages. One of the current ways of
doing this is running an interactive rebase and picking the commits in
your branch to reword. However, doing this requires you to linearly go
through the messages and edit them one by one. The other options which
allows more flexible editing is to generate linear patches -- but this
trades editing freedom for branch topology freedom and has its own
drawbacks.
The --fixup=reword: flag introduced in 494d314a05 (commit: add
amend suboption to --fixup to create amend! commit, 2021-03-15),
adds a third workflow which allows rewording commits without initiating
a rebase and from the comfort of the HEAD of the branch. However, doing
such editing is only possible using $EDITOR, which restricts its use in
some workflows.
When amend:/reword: were introduced in Charvi's series, -m support
for amend fixups was discussed but not pursued
(xmqqwnuvsw0d.fsf@gitster.g and xmqqczwmsjzl.fsf@gitster.g):
On Fri, 26 Feb 2021 11:32:30 -0800, Junio C Hamano wrote:
> >> > + if (have_option_m)
> >> > + die(_("cannot combine -m with --fixup:%s"), fixup_message);
> >> > + else
> >> > + prepare_amend_commit(commit, &sb, &ctx);
> >>
> >> Hmph, why is -m so special? Should we allow --fixup=amend:<cmd>
> >> with -F (or -c/-C for that matter), or are these other options
> >> caught at a lot higher layer already and we do not have to check
> >> them here?
> >
> > yes, those options are caught earlier and give the error as below:
> > "Only one of -c/-C/-F/--fixup can be used."
> > and only `-m` is checked over here.
>
> And the reason why -m cannot be checked early is because we do not
> recognize which kind of "fixup" we are doing when "only one of
> -c/-C/-F/--fixup" check is made before this function is called?
>
> OK. I wonder if we can tell which kind of fixup we are doing much
> earlier, though. Then we could extend it to say "Only one of
> -c/-C/-F/-m/--fixup=amend:<commit> can be used", etc., and we do not
> have to have this "only -m is checked here, everything else is
> checked earlier" curiosity. But I do not know if such a change is
> necessarily an improvement. I guess a better "fix" would probably
> be to add a comment to this function where it only checks for "-m"
> and tell readers why -c/-C/-F do not have to be checked here.
This patch picks up that thread by allowing both -m and -F for
amend/reword fixups, bypassing the need for an interactive editor.
This makes it practical to, for example, write replacement messages in
files and batch-apply them as reword fixups without stepping through
each one interactively. It's also friendly to AI agents who have a hard time
editing text using a non-interactive $EDITOR.
Allowing -c/-C was also considered but left out of this patch -- it can
be added in a re-roll if reviewers think it's worthwhile. I could see it
being useful, for example if you want to use git notes as a re-write
commit message channel. Since this is my first patch I intentionally
thought it best to start small.
Erik Cervin-Edin (1):
commit: allow -m/-F with --fixup=amend: or reword:
Documentation/git-commit.adoc | 13 +++--
builtin/commit.c | 41 ++++++++++----
t/t7500-commit-template-squash-signoff.sh | 67 +++++++++++++++++++----
3 files changed, 92 insertions(+), 29 deletions(-)
--
2.54.0.772.g683d7313b1
^ permalink raw reply
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