* [PATCH GSoC v16 06/13] fetch-pack: move write_fetch_command_and_capabilities() to connect.c
From: Pablo Sabater @ 2026-07-10 16:41 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon
In-Reply-To: <20260710-ps-eric-work-rebase-v16-0-66e07b58a8fe@gmail.com>
write_fetch_command_and_capabilities() is refactored in a subsequent
commit where it becomes a more general-purpose function, making it
more accessible to additional commands in the future.
Move write_fetch_command_and_capabilities() to 'connect.c', where
there are similar purpose functions.
Because string_list is only used as a pointer, use a forward
declaration [1].
[1]: https://lore.kernel.org/git/Z0RIqUAoEob8lGfM@pks.im/
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
connect.c | 34 ++++++++++++++++++++++++++++++++++
connect.h | 4 ++++
fetch-pack.c | 34 ----------------------------------
3 files changed, 38 insertions(+), 34 deletions(-)
diff --git a/connect.c b/connect.c
index 47e39d2a73..c09947cc56 100644
--- a/connect.c
+++ b/connect.c
@@ -700,6 +700,40 @@ int server_supports(const char *feature)
return !!server_feature_value(feature, NULL);
}
+void write_fetch_command_and_capabilities(struct strbuf *req_buf,
+ const struct string_list *server_options)
+{
+ const char *hash_name;
+ int advertise_sid = 0;
+
+ repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
+
+ ensure_server_supports_v2("fetch");
+ packet_buf_write(req_buf, "command=fetch");
+ if (server_supports_v2("agent"))
+ packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
+ if (advertise_sid && server_supports_v2("session-id"))
+ packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
+ if (server_options && server_options->nr) {
+ ensure_server_supports_v2("server-option");
+ for (size_t i = 0; i < server_options->nr; i++)
+ packet_buf_write(req_buf, "server-option=%s",
+ server_options->items[i].string);
+ }
+
+ if (server_feature_v2("object-format", &hash_name)) {
+ const unsigned int hash_algo = hash_algo_by_name(hash_name);
+ if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
+ die(_("mismatched algorithms: client %s; server %s"),
+ the_hash_algo->name, hash_name);
+ packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name);
+ } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1_LEGACY) {
+ die(_("the server does not support algorithm '%s'"),
+ the_hash_algo->name);
+ }
+ packet_buf_delim(req_buf);
+}
+
static const char *url_scheme_name(enum url_scheme scheme)
{
switch (scheme) {
diff --git a/connect.h b/connect.h
index aa482a37fb..c4f6ea4b0a 100644
--- a/connect.h
+++ b/connect.h
@@ -34,4 +34,8 @@ void check_stateless_delimiter(int stateless_rpc,
struct packet_reader *reader,
const char *error);
+struct string_list;
+void write_fetch_command_and_capabilities(struct strbuf *req_buf,
+ const struct string_list *server_options);
+
#endif
diff --git a/fetch-pack.c b/fetch-pack.c
index 8e04db8640..5e7c4f1d46 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1375,40 +1375,6 @@ static int add_haves(struct fetch_negotiator *negotiator,
return haves_added;
}
-static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
- const struct string_list *server_options)
-{
- const char *hash_name;
- int advertise_sid = 0;
-
- repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
-
- ensure_server_supports_v2("fetch");
- packet_buf_write(req_buf, "command=fetch");
- if (server_supports_v2("agent"))
- packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
- if (advertise_sid && server_supports_v2("session-id"))
- packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
- if (server_options && server_options->nr) {
- ensure_server_supports_v2("server-option");
- for (size_t i = 0; i < server_options->nr; i++)
- packet_buf_write(req_buf, "server-option=%s",
- server_options->items[i].string);
- }
-
- if (server_feature_v2("object-format", &hash_name)) {
- const unsigned int hash_algo = hash_algo_by_name(hash_name);
- if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
- die(_("mismatched algorithms: client %s; server %s"),
- the_hash_algo->name, hash_name);
- packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name);
- } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1_LEGACY) {
- die(_("the server does not support algorithm '%s'"),
- the_hash_algo->name);
- }
- packet_buf_delim(req_buf);
-}
-
static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
struct fetch_pack_args *args,
const struct ref *wants, struct oidset *common,
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v16 05/13] fetch-pack: drop static advertise_sid variable
From: Pablo Sabater @ 2026-07-10 16:41 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon
In-Reply-To: <20260710-ps-eric-work-rebase-v16-0-66e07b58a8fe@gmail.com>
write_fetch_command_and_capabilities() is moved to 'connect.c' in a
subsequent commit. To prepare for that, drop the static variable usage
of advertise_sid. Currently advertise_sid is used in two places:
1. In function do_fetch_pack():
if (!server_supports("session-id"))
advertise_sid = 0;
2. In function fetch_pack_config():
repo_config_get_bool("transfer.advertisesid", &advertise_sid);
About 1, it is only relevant for v0/v1 protocol, move it into
find_common().
About 2, call repo_config_get_bool() inside of
write_fetch_command_and_capabilities() and find_common() replacing the
static variable.
Because repo_config_get_bool() leaves advertise_sid as is if it is not
set, initialize it to 0 matching its default.
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
fetch-pack.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index eea72b2500..8e04db8640 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -49,7 +49,6 @@ static int fetch_fsck_objects = -1;
static int transfer_fsck_objects = -1;
static int agent_supported;
static int server_supports_filtering;
-static int advertise_sid;
static struct shallow_lock shallow_lock;
static const char *alternate_shallow_file;
static struct strbuf fsck_msg_types = STRBUF_INIT;
@@ -363,6 +362,9 @@ static int find_common(struct fetch_negotiator *negotiator,
size_t state_len = 0;
struct packet_reader reader;
struct oidset negotiation_include_oids = OIDSET_INIT;
+ int advertise_sid = 0;
+
+ repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
if (args->stateless_rpc && multi_ack == 1)
die(_("the option '%s' requires '%s'"), "--stateless-rpc", "multi_ack_detailed");
@@ -414,7 +416,7 @@ static int find_common(struct fetch_negotiator *negotiator,
if (deepen_not_ok) strbuf_addstr(&c, " deepen-not");
if (agent_supported) strbuf_addf(&c, " agent=%s",
git_user_agent_sanitized());
- if (advertise_sid)
+ if (advertise_sid && server_supports("session-id"))
strbuf_addf(&c, " session-id=%s", trace2_session_id());
if (args->filter_options.choice)
strbuf_addstr(&c, " filter");
@@ -1160,9 +1162,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
(int)agent_len, agent_feature);
}
- if (!server_supports("session-id"))
- advertise_sid = 0;
-
if (server_supports("shallow"))
print_verbose(args, _("Server supports %s"), "shallow");
else if (args->depth > 0 || is_repository_shallow(r))
@@ -1380,6 +1379,9 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
const struct string_list *server_options)
{
const char *hash_name;
+ int advertise_sid = 0;
+
+ repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
ensure_server_supports_v2("fetch");
packet_buf_write(req_buf, "command=fetch");
@@ -1998,7 +2000,6 @@ static void fetch_pack_config(void)
repo_config_get_bool(the_repository, "repack.usedeltabaseoffset", &prefer_ofs_delta);
repo_config_get_bool(the_repository, "fetch.fsckobjects", &fetch_fsck_objects);
repo_config_get_bool(the_repository, "transfer.fsckobjects", &transfer_fsck_objects);
- repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
if (!uri_protocols.nr) {
char *str;
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v16 04/13] fetch-pack: fix hash_algo variable type
From: Pablo Sabater @ 2026-07-10 16:41 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon
In-Reply-To: <20260710-ps-eric-work-rebase-v16-0-66e07b58a8fe@gmail.com>
hash_algo_by_name() returns "unsigned int", but the variable that it is
assigned to is "int".
Change hash_algo variable type to match hash_algo_by_name() type, also
make it const because it is never modified.
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
fetch-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index f13951d154..eea72b2500 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1395,7 +1395,7 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
}
if (server_feature_v2("object-format", &hash_name)) {
- int hash_algo = hash_algo_by_name(hash_name);
+ const unsigned int hash_algo = hash_algo_by_name(hash_name);
if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
die(_("mismatched algorithms: client %s; server %s"),
the_hash_algo->name, hash_name);
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v16 03/13] t1006: split test utility functions into new 'lib-cat-file.sh'
From: Pablo Sabater @ 2026-07-10 16:41 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon
In-Reply-To: <20260710-ps-eric-work-rebase-v16-0-66e07b58a8fe@gmail.com>
From: Eric Ju <eric.peijian@gmail.com>
This refactor extracts utility functions from the cat-file's test
script 't1006-cat-file.sh' into a new 'lib-cat-file.sh' dedicated
library file.
A subsequent commit will need this functions, the goal is to improve
code reuse and readability,enabling future tests to leverage these
utilities without duplicating code.
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
t/lib-cat-file.sh | 16 ++++++++++++++++
t/t1006-cat-file.sh | 13 +------------
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/t/lib-cat-file.sh b/t/lib-cat-file.sh
new file mode 100644
index 0000000000..44af232d74
--- /dev/null
+++ b/t/lib-cat-file.sh
@@ -0,0 +1,16 @@
+# Library of git-cat-file related test functions.
+
+# Print a string without a trailing newline.
+echo_without_newline () {
+ printf '%s' "$*"
+}
+
+# Print a string without newlines and replace them with a NULL character (\0).
+echo_without_newline_nul () {
+ echo_without_newline "$@" | tr '\n' '\0'
+}
+
+# Calculate the length of a string.
+strlen () {
+ echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
+}
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 8e2c52652c..8360f3bbd9 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -4,6 +4,7 @@ test_description='git cat-file'
. ./test-lib.sh
. "$TEST_DIRECTORY/lib-loose.sh"
+. "$TEST_DIRECTORY"/lib-cat-file.sh
test_cmdmode_usage () {
test_expect_code 129 "$@" 2>err &&
@@ -99,18 +100,6 @@ do
'
done
-echo_without_newline () {
- printf '%s' "$*"
-}
-
-echo_without_newline_nul () {
- echo_without_newline "$@" | tr '\n' '\0'
-}
-
-strlen () {
- echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
-}
-
run_tests () {
type=$1
object_name="$2"
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v16 02/13] cat-file: declare loop counter inside for()
From: Pablo Sabater @ 2026-07-10 16:41 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon
In-Reply-To: <20260710-ps-eric-work-rebase-v16-0-66e07b58a8fe@gmail.com>
From: Eric Ju <eric.peijian@gmail.com>
Some code used in this series declares variable i and only uses it
in a for loop, not in any other logic outside the loop.
Change the declaration of i to be inside the for loop for readability.
While at it, we also change its type from int to size_t where the
latter makes more sense.
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
builtin/cat-file.c | 13 ++++---------
fetch-pack.c | 3 +--
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 60869b8b37..26ad07b62c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -721,14 +721,12 @@ static void dispatch_calls(struct batch_options *opt,
struct strbuf *output,
struct expand_data *data,
struct queued_cmd *cmd,
- int nr)
+ size_t nr)
{
- int i;
-
if (!opt->buffer_output)
die(_("flush is only for --buffer mode"));
- for (i = 0; i < nr; i++)
+ for (size_t i = 0; i < nr; i++)
cmd[i].fn(opt, cmd[i].line, output, data);
fflush(stdout);
@@ -736,9 +734,7 @@ static void dispatch_calls(struct batch_options *opt,
static void free_cmds(struct queued_cmd *cmd, size_t *nr)
{
- size_t i;
-
- for (i = 0; i < *nr; i++)
+ for (size_t i = 0; i < *nr; i++)
FREE_AND_NULL(cmd[i].line);
*nr = 0;
@@ -765,7 +761,6 @@ static void batch_objects_command(struct batch_options *opt,
size_t alloc = 0, nr = 0;
while (strbuf_getdelim_strip_crlf(&input, stdin, opt->input_delim) != EOF) {
- int i;
const struct parse_cmd *cmd = NULL;
const char *p = NULL, *cmd_end;
struct queued_cmd call = {0};
@@ -775,7 +770,7 @@ static void batch_objects_command(struct batch_options *opt,
if (isspace(*input.buf))
die(_("whitespace before command: '%s'"), input.buf);
- for (i = 0; i < ARRAY_SIZE(commands); i++) {
+ for (size_t i = 0; i < ARRAY_SIZE(commands); i++) {
if (!skip_prefix(input.buf, commands[i].name, &cmd_end))
continue;
diff --git a/fetch-pack.c b/fetch-pack.c
index 120e01f3cf..f13951d154 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1388,9 +1388,8 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
if (advertise_sid && server_supports_v2("session-id"))
packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
if (server_options && server_options->nr) {
- int i;
ensure_server_supports_v2("server-option");
- for (i = 0; i < server_options->nr; i++)
+ for (size_t i = 0; i < server_options->nr; i++)
packet_buf_write(req_buf, "server-option=%s",
server_options->items[i].string);
}
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v16 01/13] transport-helper: fix memory leak of helper on disconnect
From: Pablo Sabater @ 2026-07-10 16:41 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon
In-Reply-To: <20260710-ps-eric-work-rebase-v16-0-66e07b58a8fe@gmail.com>
disconnect_helper() only frees data inside of the if(data->helper) block
[1]. When the transport is disconnected without the helper being fully
started, data->name allocated in transport_helper_init()
is never freed.
Move FREE_AND_NULL(data->name) outside the conditional block so it's
always freed on disconnect.
[1]: https://lore.kernel.org/git/05fbadbae2184479c87c37675dde7bd79b3e32ab.1716465556.git.ps@pks.im/
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
transport-helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/transport-helper.c b/transport-helper.c
index 80f90eb7ba..f195070788 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -266,9 +266,9 @@ static int disconnect_helper(struct transport *transport)
close(data->helper->out);
fclose(data->out);
res = finish_command(data->helper);
- FREE_AND_NULL(data->name);
FREE_AND_NULL(data->helper);
}
+ FREE_AND_NULL(data->name);
return res;
}
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v16 00/13] cat-file: add remote-object-info to batch-command
From: Pablo Sabater @ 2026-07-10 16:41 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon
In-Reply-To: <20260701-ps-eric-work-rebase-v15-0-c88a43b63917@gmail.com>
This patch series is a continuation of Eric Ju's
(eric.peijian@gmail.com) and Calvin Wan's (calvinwan@google.com) patch
series [1] and [2] respectively.
Sometimes it is beneficial to retrieve information about an object
without having to download it completely. The server logic for
retrieving size has already been implemented and merged in a2ba162cda
(object-info: support for retrieving object info, 2021-04-20) [3].
This patch series implement the client option for it.
Eric's series adds the remote-object-info command to cat-file
--batch-command. This command allows the client to make an object-info
command request to a server that supports protocol v2.
If the server uses protocol v2 but does not support the object-info
capability, cat-file --batch-command will die.
If a user attempts to use remote-object-info with protocol v1, cat-file
--batch-command will die.
Currently, only the size (%(objectsize)) is supported end to end in this
implementation. The type (%(objecttype)) is known by the client's
allow-list and request path but is not supported on the server side
nor the response parsing. A follow up series will add full end-to-end
support for %(objecttype).
The default format for remote-object-info is set to "%(objectname)
%(objectsize)". Once %(objecttype) is supported, the default format will
be unified accordingly.
If the batch command format includes unsupported fields such as
%(objecttype), %(objectsize:disk), or %(deltabase), the command will
return empty strings for each unsupported field.
This series completes Eric's work mainly with the refactor of the
validation of the placeholder with an allow-list that filters what the
client asks with what the server is capable of provide following Jeff
King's idea [4].
GitHub CI: https://github.com/pabloosabaterr/git/actions/runs/29091116939
[1]: https://lore.kernel.org/git/20250221190451.12536-1-eric.peijian@gmail.com/
[2]: https://lore.kernel.org/git/20220728230210.2952731-1-calvinwan@google.com/#t
[3]: https://git.kernel.org/pub/scm/git/git.git/commit/?id=a2ba162cda2acc171c3e36acbbc854792b093cb7
[4]: https://lore.kernel.org/git/20250313060250.GH94015@coredump.intra.peff.net/
Changes since v15:
- Completely dropped the static advertise_sid variable at fetch-pack.c
- Split the hash_algo type change into its own commit.
- Removed strtoumax_szt() from git-compat-util.h (and its commit) into a
static parse_object_size() helper.
- Removed backquotes from commit message bodies and fixed typos.
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
Calvin Wan (3):
fetch-pack: move fetch initialization
serve: advertise object-info feature
transport: add client support for object-info
Eric Ju (3):
cat-file: declare loop counter inside for()
t1006: split test utility functions into new 'lib-cat-file.sh'
cat-file: add remote-object-info to batch-command
Pablo Sabater (7):
transport-helper: fix memory leak of helper on disconnect
fetch-pack: fix hash_algo variable type
fetch-pack: drop static advertise_sid variable
fetch-pack: move write_fetch_command_and_capabilities() to connect.c
connect: make write_fetch_command_and_capabilities() more generic
cat-file: validate remote atoms with an allow-list
cat-file: make remote-object-info allow-list dynamic
Documentation/git-cat-file.adoc | 29 +-
Documentation/gitprotocol-v2.adoc | 11 +-
Makefile | 1 +
builtin/cat-file.c | 221 ++++++++++-
connect.c | 34 ++
connect.h | 8 +
fetch-object-info.c | 129 ++++++
fetch-object-info.h | 22 ++
fetch-pack.c | 58 +--
fetch-pack.h | 1 +
meson.build | 1 +
object-file.c | 10 +
odb.h | 3 +
serve.c | 5 +-
t/lib-cat-file.sh | 16 +
t/meson.build | 1 +
t/t1006-cat-file.sh | 13 +-
t/t1017-cat-file-remote-object-info.sh | 699 +++++++++++++++++++++++++++++++++
transport-helper.c | 15 +-
transport-internal.h | 8 +
transport.c | 46 +++
transport.h | 10 +
22 files changed, 1255 insertions(+), 86 deletions(-)
base-commit: f60db8d575adb79761d363e026fb49bddf330c73
^ permalink raw reply
* [PATCH v4 11/11] builtin/receive-pack: stage incoming objects via ODB transactions
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
Objects received by git-receive-pack(1) are quarantined in a temporary
"incoming" directory and migrated into the object database prior to the
reference updates. The quarantine is currently managed through
`tmp_objdir` directly. In a pluggable ODB future, how exactly an object
gets written to a transaction may vary for a given ODB source. Refactor
git-receive-pack(1) to use the ODB transaction interfaces to manage the
object staging area in a more agnostic manner accordingly.
Note that the ODB transaction is now responsible for managing the
primary and alternate ODBs for the repository. One small change as a
result is that the temporary directory is now applied as the primary ODB
in the main process instead of an alternate. This does not change
anything for git-receive-pack(1) though because it only needs access to
the newly written objects and doesn't care how exactly it is set up.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
builtin/receive-pack.c | 68 ++++++++++++++++++++++--------------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 50bc05c70c..8b8c20dc1a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -37,7 +37,6 @@
#include "sigchain.h"
#include "string-list.h"
#include "strvec.h"
-#include "tmp-objdir.h"
#include "trace.h"
#include "trace2.h"
#include "version.h"
@@ -112,8 +111,6 @@ static enum {
} use_keepalive;
static int keepalive_in_sec = 5;
-static struct tmp_objdir *tmp_objdir;
-
static struct proc_receive_ref {
unsigned int want_add:1,
want_delete:1,
@@ -926,6 +923,7 @@ static void receive_hook_feed_state_free(void *data)
static int run_receive_hook(struct command *commands,
const char *hook_name,
int skip_broken,
+ struct odb_transaction *transaction,
const struct string_list *push_options)
{
struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
@@ -959,8 +957,8 @@ static int run_receive_hook(struct command *commands,
strvec_push(&opt.env, "GIT_PUSH_OPTION_COUNT");
}
- if (tmp_objdir)
- strvec_pushv(&opt.env, tmp_objdir_env(tmp_objdir));
+ if (transaction)
+ odb_transaction_env(transaction, &opt.env);
prepare_push_cert_sha1(&opt);
@@ -1789,24 +1787,30 @@ static const struct object_id *command_singleton_iterator(void *cb_data)
}
static void set_connectivity_errors(struct command *commands,
- struct shallow_info *si)
+ struct shallow_info *si,
+ struct odb_transaction *transaction)
{
struct command *cmd;
for (cmd = commands; cmd; cmd = cmd->next) {
struct command *singleton = cmd;
struct check_connected_options opt = CHECK_CONNECTED_INIT;
+ struct strvec env = STRVEC_INIT;
if (shallow_update && si->shallow_ref[cmd->index])
/* to be checked in update_shallow_ref() */
continue;
- opt.env = tmp_objdir_env(tmp_objdir);
+ odb_transaction_env(transaction, &env);
+ opt.env = env.v;
+
if (!check_connected(command_singleton_iterator, &singleton,
&opt))
continue;
cmd->error_string = "missing necessary objects";
+
+ strvec_clear(&env);
}
}
@@ -2027,6 +2031,7 @@ static void execute_commands_atomic(struct command *commands,
static void execute_commands(struct command *commands,
const char *unpacker_error,
struct shallow_info *si,
+ struct odb_transaction *transaction,
const struct string_list *push_options)
{
struct check_connected_options opt = CHECK_CONNECTED_INIT;
@@ -2043,6 +2048,8 @@ static void execute_commands(struct command *commands,
}
if (!skip_connectivity_check) {
+ struct strvec env = STRVEC_INIT;
+
if (use_sideband) {
memset(&muxer, 0, sizeof(muxer));
muxer.proc = copy_to_sideband;
@@ -2056,14 +2063,17 @@ static void execute_commands(struct command *commands,
data.si = si;
opt.err_fd = err_fd;
opt.progress = err_fd && !quiet;
- opt.env = tmp_objdir_env(tmp_objdir);
+ odb_transaction_env(transaction, &env);
+ opt.env = env.v;
opt.exclude_hidden_refs_section = "receive";
if (check_connected(iterate_receive_command_list, &data, &opt))
- set_connectivity_errors(commands, si);
+ set_connectivity_errors(commands, si, transaction);
if (use_sideband)
finish_async(&muxer);
+
+ strvec_clear(&env);
}
reject_updates_to_hidden(commands);
@@ -2084,7 +2094,7 @@ static void execute_commands(struct command *commands,
}
}
- if (run_receive_hook(commands, "pre-receive", 0, push_options)) {
+ if (run_receive_hook(commands, "pre-receive", 0, transaction, push_options)) {
for (cmd = commands; cmd; cmd = cmd->next) {
if (!cmd->error_string)
cmd->error_string = "pre-receive hook declined";
@@ -2105,14 +2115,13 @@ static void execute_commands(struct command *commands,
* Now we'll start writing out refs, which means the objects need
* to be in their final positions so that other processes can see them.
*/
- if (tmp_objdir_migrate(tmp_objdir) < 0) {
+ if (odb_transaction_commit(transaction)) {
for (cmd = commands; cmd; cmd = cmd->next) {
if (!cmd->error_string)
cmd->error_string = "unable to migrate objects to permanent storage";
}
return;
}
- tmp_objdir = NULL;
check_aliased_updates(commands);
@@ -2325,7 +2334,8 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
}
-static const char *unpack(int err_fd, struct shallow_info *si)
+static const char *unpack(int err_fd, struct shallow_info *si,
+ struct odb_transaction *transaction)
{
struct pack_header hdr;
const char *hdr_err;
@@ -2350,20 +2360,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
strvec_push(&child.args, alt_shallow_file);
}
- tmp_objdir = tmp_objdir_create(the_repository, "incoming");
- if (!tmp_objdir) {
- if (err_fd > 0)
- close(err_fd);
- return "unable to create temporary object directory";
- }
- strvec_pushv(&child.env, tmp_objdir_env(tmp_objdir));
-
- /*
- * Normally we just pass the tmp_objdir environment to the child
- * processes that do the heavy lifting, but we may need to see these
- * objects ourselves to set up shallow information.
- */
- tmp_objdir_add_as_alternate(tmp_objdir);
+ odb_transaction_env(transaction, &child.env);
if (ntohl(hdr.hdr_entries) < unpack_limit) {
strvec_push(&child.args, "unpack-objects");
@@ -2430,13 +2427,14 @@ static const char *unpack(int err_fd, struct shallow_info *si)
return NULL;
}
-static const char *unpack_with_sideband(struct shallow_info *si)
+static const char *unpack_with_sideband(struct shallow_info *si,
+ struct odb_transaction *transaction)
{
struct async muxer;
const char *ret;
if (!use_sideband)
- return unpack(0, si);
+ return unpack(0, si, transaction);
use_keepalive = KEEPALIVE_AFTER_NUL;
memset(&muxer, 0, sizeof(muxer));
@@ -2445,7 +2443,7 @@ static const char *unpack_with_sideband(struct shallow_info *si)
if (start_async(&muxer))
return NULL;
- ret = unpack(muxer.in, si);
+ ret = unpack(muxer.in, si, transaction);
finish_async(&muxer);
return ret;
@@ -2622,6 +2620,7 @@ int cmd_receive_pack(int argc,
struct oid_array ref = OID_ARRAY_INIT;
struct shallow_info si;
struct packet_reader reader;
+ struct odb_transaction *transaction = NULL;
struct option options[] = {
OPT__QUIET(&quiet, N_("quiet")),
@@ -2706,11 +2705,14 @@ int cmd_receive_pack(int argc,
if (!si.nr_ours && !si.nr_theirs)
shallow_update = 0;
if (!delete_only(commands)) {
- unpack_status = unpack_with_sideband(&si);
+ if (odb_transaction_begin(the_repository->objects, &transaction, ODB_TRANSACTION_RECEIVE))
+ unpack_status = "unable to start object transaction";
+ else
+ unpack_status = unpack_with_sideband(&si, transaction);
update_shallow_info(commands, &si, &ref);
}
use_keepalive = KEEPALIVE_ALWAYS;
- execute_commands(commands, unpack_status, &si,
+ execute_commands(commands, unpack_status, &si, transaction,
&push_options);
delete_tempfile(&pack_lockfile);
sigchain_push(SIGPIPE, SIG_IGN);
@@ -2719,7 +2721,7 @@ int cmd_receive_pack(int argc,
else if (report_status)
report(commands, unpack_status);
sigchain_pop(SIGPIPE);
- run_receive_hook(commands, "post-receive", 1,
+ run_receive_hook(commands, "post-receive", 1, NULL,
&push_options);
run_update_post_hook(commands);
free_commands(commands);
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* [PATCH v4 09/11] odb/transaction: introduce ODB transaction flags
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
The temporary directory used by git-receive-pack(1) to write objects is
managed slightly differently than how it is done via ODB transactions:
- The temporary directory is eagerly created upfront, instead of
waiting for the first object write.
- The prefix name of the temporary directory is "incoming" instead of
"bulk-fsync".
In a subsequent commit, git-receive-pack(1) will use ODB transactions
instead of `tmp_objdir` directly. To provide a means to configure the
same transaction behavior, introduce `enum odb_transaction_flags` and
the ODB_TRANSACTION_RECEIVE flag intended as a signal for ODB
transactions using the "files" backend to be set up for
git-receive-pack(1). Transaction call sites are updated accordingly to
provide the required flag parameter.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
builtin/add.c | 2 +-
builtin/unpack-objects.c | 2 +-
builtin/update-index.c | 2 +-
cache-tree.c | 2 +-
object-file.c | 29 ++++++++++++++++++++++++++---
object-file.h | 4 +++-
odb/source-files.c | 5 +++--
odb/source-inmemory.c | 3 ++-
odb/source-loose.c | 3 ++-
odb/source.h | 9 ++++++---
odb/transaction.c | 5 +++--
odb/transaction.h | 15 +++++++++++----
read-cache.c | 2 +-
13 files changed, 61 insertions(+), 22 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 3d5d9cfdb9..60ffbede2b 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -581,7 +581,7 @@ int cmd_add(int argc,
string_list_clear(&only_match_skip_worktree, 0);
}
- odb_transaction_begin_or_die(repo->objects, &transaction);
+ odb_transaction_begin_or_die(repo->objects, &transaction, 0);
ps_matched = xcalloc(pathspec.nr, 1);
if (add_renormalize)
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index d0136cdd99..c3d0fc7507 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -598,7 +598,7 @@ static void unpack_all(void)
progress = start_progress(the_repository,
_("Unpacking objects"), nr_objects);
CALLOC_ARRAY(obj_list, nr_objects);
- odb_transaction_begin_or_die(the_repository->objects, &transaction);
+ odb_transaction_begin_or_die(the_repository->objects, &transaction, 0);
for (i = 0; i < nr_objects; i++) {
unpack_one(i);
display_progress(progress, i + 1);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 17f3ea284c..bf6ea60ef4 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1124,7 +1124,7 @@ int cmd_update_index(int argc,
* Allow the object layer to optimize adding multiple objects in
* a batch.
*/
- odb_transaction_begin_or_die(the_repository->objects, &transaction);
+ odb_transaction_begin_or_die(the_repository->objects, &transaction, 0);
while (ctx.argc) {
if (parseopt_state != PARSE_OPT_DONE)
parseopt_state = parse_options_step(&ctx, options,
diff --git a/cache-tree.c b/cache-tree.c
index 8eec1d4d52..99c6a0a7d0 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -492,7 +492,7 @@ int cache_tree_update(struct index_state *istate, int flags)
trace_performance_enter();
trace2_region_enter("cache_tree", "update", istate->repo);
if (!inflight)
- odb_transaction_begin_or_die(the_repository->objects, &transaction);
+ odb_transaction_begin_or_die(the_repository->objects, &transaction, 0);
i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
"", 0, &skip, flags);
if (!inflight)
diff --git a/object-file.c b/object-file.c
index 39b92e275c..0640a22009 100644
--- a/object-file.c
+++ b/object-file.c
@@ -498,6 +498,7 @@ struct odb_transaction_files {
struct tmp_objdir *objdir;
struct transaction_packfile packfile;
+ const char *prefix;
};
static int odb_transaction_files_prepare(struct odb_transaction *base)
@@ -514,7 +515,7 @@ static int odb_transaction_files_prepare(struct odb_transaction *base)
if (!transaction || transaction->objdir)
return 0;
- transaction->objdir = tmp_objdir_create(base->source->odb->repo, "bulk-fsync");
+ transaction->objdir = tmp_objdir_create(base->source->odb->repo, transaction->prefix);
if (!transaction->objdir)
return error(_("unable to create temporary object directory"));
@@ -1359,7 +1360,7 @@ int index_fd(struct index_state *istate, struct object_id *oid,
int inflight = !!transaction;
if (!inflight)
- odb_transaction_begin_or_die(odb, &transaction);
+ odb_transaction_begin_or_die(odb, &transaction, 0);
ret = odb_transaction_write_object_stream(transaction,
&stream,
xsize_t(st->st_size),
@@ -1703,7 +1704,8 @@ static int odb_transaction_files_env(struct odb_transaction *base,
}
int odb_transaction_files_begin(struct odb_source *source,
- struct odb_transaction **out)
+ struct odb_transaction **out,
+ enum odb_transaction_flags flags)
{
struct odb_transaction_files *transaction;
@@ -1712,6 +1714,27 @@ int odb_transaction_files_begin(struct odb_source *source,
transaction->base.commit = odb_transaction_files_commit;
transaction->base.write_object_stream = odb_transaction_files_write_object_stream;
transaction->base.env = odb_transaction_files_env;
+
+ transaction->prefix = "bulk-fsync";
+ if (flags & ODB_TRANSACTION_RECEIVE) {
+ /*
+ * ODB transactions for git-receive-pack(1) eagerly create a
+ * temporary directory and use a different temporary directory
+ * prefix.
+ *
+ * NEEDSWORK: This transaction flag is only used by the "files"
+ * backend to special case temporary directory set up and
+ * handling. Ideally transaction users should not have to care
+ * though. To avoid this, we could eagerly create the temporary
+ * directory and use the same prefix name for all transactions.
+ */
+ transaction->prefix = "incoming";
+ if (odb_transaction_files_prepare(&transaction->base)) {
+ free(transaction);
+ return -1;
+ }
+ }
+
*out = &transaction->base;
return 0;
diff --git a/object-file.h b/object-file.h
index 1a023226ac..bdd2d67a2e 100644
--- a/object-file.h
+++ b/object-file.h
@@ -5,6 +5,7 @@
#include "object.h"
#include "odb.h"
#include "odb/source-loose.h"
+#include "odb/transaction.h"
/* The maximum size for an object header. */
#define MAX_HEADER_LEN 32
@@ -197,6 +198,7 @@ struct odb_transaction;
* to make new objects visible.
*/
int odb_transaction_files_begin(struct odb_source *source,
- struct odb_transaction **out);
+ struct odb_transaction **out,
+ enum odb_transaction_flags flags);
#endif /* OBJECT_FILE_H */
diff --git a/odb/source-files.c b/odb/source-files.c
index 2545bd81d4..534f48aad9 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -180,9 +180,10 @@ static int odb_source_files_write_object_stream(struct odb_source *source,
}
static int odb_source_files_begin_transaction(struct odb_source *source,
- struct odb_transaction **out)
+ struct odb_transaction **out,
+ enum odb_transaction_flags flags)
{
- return odb_transaction_files_begin(source, out);
+ return odb_transaction_files_begin(source, out, flags);
}
static int odb_source_files_read_alternates(struct odb_source *source,
diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
index e004566d76..9644d9d474 100644
--- a/odb/source-inmemory.c
+++ b/odb/source-inmemory.c
@@ -304,7 +304,8 @@ static int odb_source_inmemory_freshen_object(struct odb_source *source,
}
static int odb_source_inmemory_begin_transaction(struct odb_source *source UNUSED,
- struct odb_transaction **out UNUSED)
+ struct odb_transaction **out UNUSED,
+ enum odb_transaction_flags flags UNUSED)
{
return error("in-memory source does not support transactions");
}
diff --git a/odb/source-loose.c b/odb/source-loose.c
index 66e6bb8d3f..57c91986b4 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -638,7 +638,8 @@ static int odb_source_loose_write_object_stream(struct odb_source *source,
}
static int odb_source_loose_begin_transaction(struct odb_source *source UNUSED,
- struct odb_transaction **out UNUSED)
+ struct odb_transaction **out UNUSED,
+ enum odb_transaction_flags flags UNUSED)
{
/* TODO: this is a known omission that we'll want to address eventually. */
return error("loose source does not support transactions");
diff --git a/odb/source.h b/odb/source.h
index 2192a101b8..3790d03ff2 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -3,6 +3,7 @@
#include "object.h"
#include "odb.h"
+#include "odb/transaction.h"
enum odb_source_type {
/*
@@ -228,7 +229,8 @@ struct odb_source {
* negative error code otherwise.
*/
int (*begin_transaction)(struct odb_source *source,
- struct odb_transaction **out);
+ struct odb_transaction **out,
+ enum odb_transaction_flags flags);
/*
* This callback is expected to read the list of alternate object
@@ -467,9 +469,10 @@ static inline int odb_source_write_alternate(struct odb_source *source,
* Returns 0 on success, a negative error code otherwise.
*/
static inline int odb_source_begin_transaction(struct odb_source *source,
- struct odb_transaction **out)
+ struct odb_transaction **out,
+ enum odb_transaction_flags flags)
{
- return source->begin_transaction(source, out);
+ return source->begin_transaction(source, out, flags);
}
#endif
diff --git a/odb/transaction.c b/odb/transaction.c
index 92ec8786a1..dab7da6a9a 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -4,14 +4,15 @@
#include "odb/transaction.h"
int odb_transaction_begin(struct object_database *odb,
- struct odb_transaction **out)
+ struct odb_transaction **out,
+ enum odb_transaction_flags flags)
{
int ret;
if (odb->transaction)
return error(_("object database transaction already pending"));
- ret = odb_source_begin_transaction(odb->sources, out);
+ ret = odb_source_begin_transaction(odb->sources, out, flags);
if (!ret)
odb->transaction = *out;
diff --git a/odb/transaction.h b/odb/transaction.h
index 5e51ce5ca4..4cb2eafcbf 100644
--- a/odb/transaction.h
+++ b/odb/transaction.h
@@ -3,7 +3,6 @@
#include "gettext.h"
#include "odb.h"
-#include "odb/source.h"
/*
* A transaction may be started for an object database prior to writing new
@@ -44,6 +43,12 @@ struct odb_transaction {
int (*env)(struct odb_transaction *transaction, struct strvec *env);
};
+/* Flags used to configure an ODB transaction. */
+enum odb_transaction_flags {
+ /* Configures the transaction for use with git-receive-pack(1). */
+ ODB_TRANSACTION_RECEIVE = (1 << 0),
+};
+
/*
* Starts an ODB transaction and returns it via `out`. Subsequent objects are
* written to the transaction and not committed until odb_transaction_commit()
@@ -52,12 +57,14 @@ struct odb_transaction {
* ODB already has an inflight transaction pending.
*/
int odb_transaction_begin(struct object_database *odb,
- struct odb_transaction **out);
+ struct odb_transaction **out,
+ enum odb_transaction_flags flags);
static inline void odb_transaction_begin_or_die(struct object_database *odb,
- struct odb_transaction **out)
+ struct odb_transaction **out,
+ enum odb_transaction_flags flags)
{
- if (odb_transaction_begin(odb, out))
+ if (odb_transaction_begin(odb, out, flags))
die(_("failed to start ODB transaction"));
}
diff --git a/read-cache.c b/read-cache.c
index d511d25834..50e2320c8d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -4044,7 +4044,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,
* may not have their own transaction active.
*/
if (!inflight)
- odb_transaction_begin_or_die(repo->objects, &transaction);
+ odb_transaction_begin_or_die(repo->objects, &transaction, 0);
run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
if (!inflight)
odb_transaction_commit(transaction);
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* [PATCH v4 10/11] builtin/receive-pack: drop redundant tmpdir env
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
When performing the connectivity checks for a shallow ref in
`update_shallow_ref()`, the child process environment variables are
populated via `tmp_objdir_env()`. This is unnecessary though as
`update_shallow_ref()` is only reached after `tmp_objdir_migrate()` has
been performed which means there is no longer a temporary directory that
needs to be shared with child processes.
Drop the call to `tmp_objdir_env()` accordingly.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
builtin/receive-pack.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 19eb6a1b61..50bc05c70c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1363,7 +1363,6 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
!delayed_reachability_test(si, i))
oid_array_append(&extra, &si->shallow->oid[i]);
- opt.env = tmp_objdir_env(tmp_objdir);
setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);
if (check_connected(command_singleton_iterator, cmd, &opt)) {
rollback_shallow_file(the_repository, &shallow_lock);
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* [PATCH v4 08/11] odb/transaction: add transaction env interface
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
The ODB transaction backend is responsible for creating/managing its own
staging area for writing objects. Other child processes spawned by Git
may need access to uncommitted objects or write new objects in the
staging area though.
Introduce `odb_transaction_env()` which is expected to provide the set
of environment variables needed by a child process to access the
transaction's staging area.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
object-file.c | 16 ++++++++++++++++
odb/transaction.c | 8 ++++++++
odb/transaction.h | 17 +++++++++++++++++
3 files changed, 41 insertions(+)
diff --git a/object-file.c b/object-file.c
index 358684beae..39b92e275c 100644
--- a/object-file.c
+++ b/object-file.c
@@ -27,6 +27,7 @@
#include "path.h"
#include "read-cache-ll.h"
#include "setup.h"
+#include "strvec.h"
#include "tempfile.h"
#include "tmp-objdir.h"
@@ -1687,6 +1688,20 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
return 0;
}
+static int odb_transaction_files_env(struct odb_transaction *base,
+ struct strvec *env)
+{
+ struct odb_transaction_files *transaction =
+ container_of(base, struct odb_transaction_files, base);
+ int ret;
+
+ ret = odb_transaction_files_prepare(&transaction->base);
+ if (!ret)
+ strvec_pushv(env, tmp_objdir_env(transaction->objdir));
+
+ return ret;
+}
+
int odb_transaction_files_begin(struct odb_source *source,
struct odb_transaction **out)
{
@@ -1696,6 +1711,7 @@ int odb_transaction_files_begin(struct odb_source *source,
transaction->base.source = source;
transaction->base.commit = odb_transaction_files_commit;
transaction->base.write_object_stream = odb_transaction_files_write_object_stream;
+ transaction->base.env = odb_transaction_files_env;
*out = &transaction->base;
return 0;
diff --git a/odb/transaction.c b/odb/transaction.c
index 249ef4d9b7..92ec8786a1 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -43,3 +43,11 @@ int odb_transaction_write_object_stream(struct odb_transaction *transaction,
{
return transaction->write_object_stream(transaction, stream, len, oid);
}
+
+int odb_transaction_env(struct odb_transaction *transaction, struct strvec *env)
+{
+ if (!transaction)
+ return 0;
+
+ return transaction->env(transaction, env);
+}
diff --git a/odb/transaction.h b/odb/transaction.h
index 3b0a5a78e5..5e51ce5ca4 100644
--- a/odb/transaction.h
+++ b/odb/transaction.h
@@ -34,6 +34,14 @@ struct odb_transaction {
int (*write_object_stream)(struct odb_transaction *transaction,
struct odb_write_stream *stream, size_t len,
struct object_id *oid);
+
+ /*
+ * This callback is expected to populate the provided strvec with the
+ * environment variables that a child process should inherit so that its
+ * object writes participate in the transaction. Returns 0 on success, a
+ * negative error code otherwise.
+ */
+ int (*env)(struct odb_transaction *transaction, struct strvec *env);
};
/*
@@ -69,4 +77,13 @@ int odb_transaction_write_object_stream(struct odb_transaction *transaction,
struct odb_write_stream *stream,
size_t len, struct object_id *oid);
+/*
+ * Populates the provided strvec with the environment variables that a child
+ * process should inherit so that its object writes participate in the
+ * transaction, suitable for using via child_process.env. Returns 0 on success,
+ * a negative error code otherwise. Note that, if the specified transaction is
+ * NULL, the function is a no-op and no error is returned.
+ */
+int odb_transaction_env(struct odb_transaction *transaction, struct strvec *env);
+
#endif
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* [PATCH v4 07/11] odb/transaction: propagate commit errors
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
When `odb_transaction_commit()` is invoked, the return value of the
backend commit callback is silently discarded. A backend has no way
to signal that committing failed, such as when the "files" backend
cannot migrate its temporary object directory into the permanent
ODB.
In a subsequent commit, git-receive-pack(1) starts using ODB transaction
to stage objects and consequently cares about such failures so it can
handle the error appropriately. Change the commit callback signature to
return an int error code and have `odb_transaction_commit()` forward it
accordingly.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
odb/transaction.c | 10 +++++++---
odb/transaction.h | 7 ++++---
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/odb/transaction.c b/odb/transaction.c
index b6da4a3942..249ef4d9b7 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -18,19 +18,23 @@ int odb_transaction_begin(struct object_database *odb,
return ret;
}
-void odb_transaction_commit(struct odb_transaction *transaction)
+int odb_transaction_commit(struct odb_transaction *transaction)
{
+ int ret;
+
if (!transaction)
- return;
+ return 0;
/*
* Ensure the transaction ending matches the pending transaction.
*/
ASSERT(transaction == transaction->source->odb->transaction);
- transaction->commit(transaction);
+ ret = transaction->commit(transaction);
transaction->source->odb->transaction = NULL;
free(transaction);
+
+ return ret;
}
int odb_transaction_write_object_stream(struct odb_transaction *transaction,
diff --git a/odb/transaction.h b/odb/transaction.h
index f5c43187c9..3b0a5a78e5 100644
--- a/odb/transaction.h
+++ b/odb/transaction.h
@@ -54,10 +54,11 @@ static inline void odb_transaction_begin_or_die(struct object_database *odb,
}
/*
- * Commits an ODB transaction making the written objects visible. If the
- * specified transaction is NULL, the function is a no-op.
+ * Commits an ODB transaction making the written objects visible. Returns 0 on
+ * success, a negative error code otherwise. Note that, if the specified
+ * transaction is NULL, the function is a no-op and no error is returned.
*/
-void odb_transaction_commit(struct odb_transaction *transaction);
+int odb_transaction_commit(struct odb_transaction *transaction);
/*
* Writes the object in the provided stream into the transaction. The resulting
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* [PATCH v4 06/11] odb/transaction: propagate begin errors
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
When `odb_transaction_begin()` is invoked, the function returns the
transaction pointer directly. There is no way for the backend to
signal that it failed to set up its state, such as when creating the
temporary object directory backing the transaction.
In a subsequent commit, git-receive-pack(1) starts using ODB
transactions and needs to be able to report such failures rather
than silently ignore them. Refactor `odb_transaction_begin()` to
return an int error code and write the resulting transaction into an
out parameter. Also introduce `odb_transaction_begin_or_die()` as a
convenience for callsites that do not need to handle errors
explicitly.
Note that `odb_transaction_begin()` now returns an error when the ODB
already has an inflight transaction pending. ODB transaction call sites
that may encounter an inflight transaction are updated to explicitly
handle this case.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
builtin/add.c | 2 +-
builtin/unpack-objects.c | 2 +-
builtin/update-index.c | 2 +-
cache-tree.c | 7 +++++--
object-file.c | 10 +++++++---
odb/transaction.c | 14 ++++++++++----
odb/transaction.h | 19 +++++++++++++++----
read-cache.c | 7 +++++--
8 files changed, 45 insertions(+), 18 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c859f66519..3d5d9cfdb9 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -581,7 +581,7 @@ int cmd_add(int argc,
string_list_clear(&only_match_skip_worktree, 0);
}
- transaction = odb_transaction_begin(repo->objects);
+ odb_transaction_begin_or_die(repo->objects, &transaction);
ps_matched = xcalloc(pathspec.nr, 1);
if (add_renormalize)
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index f3849bb654..d0136cdd99 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -598,7 +598,7 @@ static void unpack_all(void)
progress = start_progress(the_repository,
_("Unpacking objects"), nr_objects);
CALLOC_ARRAY(obj_list, nr_objects);
- transaction = odb_transaction_begin(the_repository->objects);
+ odb_transaction_begin_or_die(the_repository->objects, &transaction);
for (i = 0; i < nr_objects; i++) {
unpack_one(i);
display_progress(progress, i + 1);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 3d6646c318..17f3ea284c 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1124,7 +1124,7 @@ int cmd_update_index(int argc,
* Allow the object layer to optimize adding multiple objects in
* a batch.
*/
- transaction = odb_transaction_begin(the_repository->objects);
+ odb_transaction_begin_or_die(the_repository->objects, &transaction);
while (ctx.argc) {
if (parseopt_state != PARSE_OPT_DONE)
parseopt_state = parse_options_step(&ctx, options,
diff --git a/cache-tree.c b/cache-tree.c
index 184f7e2635..8eec1d4d52 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -474,6 +474,7 @@ static int update_one(struct cache_tree *it,
int cache_tree_update(struct index_state *istate, int flags)
{
+ int inflight = !!the_repository->objects->transaction;
struct odb_transaction *transaction;
int skip, i;
@@ -490,10 +491,12 @@ int cache_tree_update(struct index_state *istate, int flags)
trace_performance_enter();
trace2_region_enter("cache_tree", "update", istate->repo);
- transaction = odb_transaction_begin(the_repository->objects);
+ if (!inflight)
+ odb_transaction_begin_or_die(the_repository->objects, &transaction);
i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
"", 0, &skip, flags);
- odb_transaction_commit(transaction);
+ if (!inflight)
+ odb_transaction_commit(transaction);
trace2_region_leave("cache_tree", "update", istate->repo);
trace_performance_leave("cache_tree_update");
if (i < 0)
diff --git a/object-file.c b/object-file.c
index 3651605ea2..358684beae 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1354,13 +1354,17 @@ int index_fd(struct index_state *istate, struct object_id *oid,
if (flags & INDEX_WRITE_OBJECT) {
struct object_database *odb = the_repository->objects;
- struct odb_transaction *transaction = odb_transaction_begin(odb);
+ struct odb_transaction *transaction = odb->transaction;
+ int inflight = !!transaction;
- ret = odb_transaction_write_object_stream(odb->transaction,
+ if (!inflight)
+ odb_transaction_begin_or_die(odb, &transaction);
+ ret = odb_transaction_write_object_stream(transaction,
&stream,
xsize_t(st->st_size),
oid);
- odb_transaction_commit(transaction);
+ if (!inflight)
+ odb_transaction_commit(transaction);
} else {
ret = hash_blob_stream(&stream,
the_repository->hash_algo, oid,
diff --git a/odb/transaction.c b/odb/transaction.c
index b16e07aebf..b6da4a3942 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -1,15 +1,21 @@
#include "git-compat-util.h"
+#include "gettext.h"
#include "odb/source.h"
#include "odb/transaction.h"
-struct odb_transaction *odb_transaction_begin(struct object_database *odb)
+int odb_transaction_begin(struct object_database *odb,
+ struct odb_transaction **out)
{
+ int ret;
+
if (odb->transaction)
- return NULL;
+ return error(_("object database transaction already pending"));
- odb_source_begin_transaction(odb->sources, &odb->transaction);
+ ret = odb_source_begin_transaction(odb->sources, out);
+ if (!ret)
+ odb->transaction = *out;
- return odb->transaction;
+ return ret;
}
void odb_transaction_commit(struct odb_transaction *transaction)
diff --git a/odb/transaction.h b/odb/transaction.h
index d52f0533ce..f5c43187c9 100644
--- a/odb/transaction.h
+++ b/odb/transaction.h
@@ -1,6 +1,7 @@
#ifndef ODB_TRANSACTION_H
#define ODB_TRANSACTION_H
+#include "gettext.h"
#include "odb.h"
#include "odb/source.h"
@@ -36,11 +37,21 @@ struct odb_transaction {
};
/*
- * Starts an ODB transaction. Subsequent objects are written to the transaction
- * and not committed until odb_transaction_commit() is invoked on the
- * transaction. If the ODB already has a pending transaction, NULL is returned.
+ * Starts an ODB transaction and returns it via `out`. Subsequent objects are
+ * written to the transaction and not committed until odb_transaction_commit()
+ * is invoked on the transaction. Returns 0 on success and a negative value on
+ * error. Note that it is considered an error to start a new transaction if the
+ * ODB already has an inflight transaction pending.
*/
-struct odb_transaction *odb_transaction_begin(struct object_database *odb);
+int odb_transaction_begin(struct object_database *odb,
+ struct odb_transaction **out);
+
+static inline void odb_transaction_begin_or_die(struct object_database *odb,
+ struct odb_transaction **out)
+{
+ if (odb_transaction_begin(odb, out))
+ die(_("failed to start ODB transaction"));
+}
/*
* Commits an ODB transaction making the written objects visible. If the
diff --git a/read-cache.c b/read-cache.c
index 21ca58beea..d511d25834 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -4012,6 +4012,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,
const struct pathspec *pathspec, char *ps_matched,
int include_sparse, int flags, int ignored_too )
{
+ int inflight = !!repo->objects->transaction;
struct odb_transaction *transaction;
struct update_callback_data data;
struct rev_info rev;
@@ -4042,9 +4043,11 @@ int add_files_to_cache(struct repository *repo, const char *prefix,
* This function is invoked from commands other than 'add', which
* may not have their own transaction active.
*/
- transaction = odb_transaction_begin(repo->objects);
+ if (!inflight)
+ odb_transaction_begin_or_die(repo->objects, &transaction);
run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
- odb_transaction_commit(transaction);
+ if (!inflight)
+ odb_transaction_commit(transaction);
release_revisions(&rev);
return !!data.add_errors;
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* [PATCH v4 05/11] object-file: propagate files transaction errors
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
The "files" transaction backend may encounter errors related to managing
the temporary directory used to stage objects, but silently ignores
these errors. Instead return errors encountered in the
`odb_transaction_files_{prepare,begin,commit}()` interfaces to allow
callers to handle them as needed.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
object-file.c | 26 ++++++++++++++++++--------
object-file.h | 3 ++-
odb/source-files.c | 6 +-----
odb/transaction.h | 7 +++++--
4 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/object-file.c b/object-file.c
index e51389833a..3651605ea2 100644
--- a/object-file.c
+++ b/object-file.c
@@ -499,7 +499,7 @@ struct odb_transaction_files {
struct transaction_packfile packfile;
};
-static void odb_transaction_files_prepare(struct odb_transaction *base)
+static int odb_transaction_files_prepare(struct odb_transaction *base)
{
struct odb_transaction_files *transaction =
container_of_or_null(base, struct odb_transaction_files, base);
@@ -511,11 +511,15 @@ static void odb_transaction_files_prepare(struct odb_transaction *base)
* added at the time they call odb_transaction_files_begin.
*/
if (!transaction || transaction->objdir)
- return;
+ return 0;
transaction->objdir = tmp_objdir_create(base->source->odb->repo, "bulk-fsync");
- if (transaction->objdir)
- tmp_objdir_replace_primary_odb(transaction->objdir, 0);
+ if (!transaction->objdir)
+ return error(_("unable to create temporary object directory"));
+
+ tmp_objdir_replace_primary_odb(transaction->objdir, 0);
+
+ return 0;
}
static void odb_transaction_files_fsync(struct odb_transaction *base,
@@ -1639,7 +1643,7 @@ int read_loose_object(struct repository *repo,
return ret;
}
-static void odb_transaction_files_commit(struct odb_transaction *base)
+static int odb_transaction_files_commit(struct odb_transaction *base)
{
struct odb_transaction_files *transaction =
container_of(base, struct odb_transaction_files, base);
@@ -1668,14 +1672,19 @@ static void odb_transaction_files_commit(struct odb_transaction *base)
* Make the object files visible in the primary ODB after their data is
* fully durable.
*/
- tmp_objdir_migrate(transaction->objdir);
+ if (tmp_objdir_migrate(transaction->objdir))
+ return error(_("unable to migrate temporary objects"));
+
transaction->objdir = NULL;
}
flush_packfile_transaction(transaction);
+
+ return 0;
}
-struct odb_transaction *odb_transaction_files_begin(struct odb_source *source)
+int odb_transaction_files_begin(struct odb_source *source,
+ struct odb_transaction **out)
{
struct odb_transaction_files *transaction;
@@ -1683,6 +1692,7 @@ struct odb_transaction *odb_transaction_files_begin(struct odb_source *source)
transaction->base.source = source;
transaction->base.commit = odb_transaction_files_commit;
transaction->base.write_object_stream = odb_transaction_files_write_object_stream;
+ *out = &transaction->base;
- return &transaction->base;
+ return 0;
}
diff --git a/object-file.h b/object-file.h
index ea43d818f0..1a023226ac 100644
--- a/object-file.h
+++ b/object-file.h
@@ -196,6 +196,7 @@ struct odb_transaction;
* multiple objects. odb_transaction_files_commit must be called
* to make new objects visible.
*/
-struct odb_transaction *odb_transaction_files_begin(struct odb_source *source);
+int odb_transaction_files_begin(struct odb_source *source,
+ struct odb_transaction **out);
#endif /* OBJECT_FILE_H */
diff --git a/odb/source-files.c b/odb/source-files.c
index 5bdd042922..2545bd81d4 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -182,11 +182,7 @@ static int odb_source_files_write_object_stream(struct odb_source *source,
static int odb_source_files_begin_transaction(struct odb_source *source,
struct odb_transaction **out)
{
- struct odb_transaction *tx = odb_transaction_files_begin(source);
- if (!tx)
- return -1;
- *out = tx;
- return 0;
+ return odb_transaction_files_begin(source, out);
}
static int odb_source_files_read_alternates(struct odb_source *source,
diff --git a/odb/transaction.h b/odb/transaction.h
index 854fda06f5..d52f0533ce 100644
--- a/odb/transaction.h
+++ b/odb/transaction.h
@@ -16,8 +16,11 @@ struct odb_transaction {
/* The ODB source the transaction is opened against. */
struct odb_source *source;
- /* The ODB source specific callback invoked to commit a transaction. */
- void (*commit)(struct odb_transaction *transaction);
+ /*
+ * The ODB source specific callback invoked to commit a transaction.
+ * Returns 0 on success, a negative error code otherwise.
+ */
+ int (*commit)(struct odb_transaction *transaction);
/*
* This callback is expected to write the given object stream into
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* Re: [PATCH v3 0/2] prio-queue: use bottom-up sift for extract-min
From: René Scharfe @ 2026-07-10 16:37 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget, git; +Cc: Kristofer Karlsson
In-Reply-To: <pull.2132.v3.git.1783532989.gitgitgadget@gmail.com>
On 7/8/26 7:49 PM, Kristofer Karlsson via GitGitGadget wrote:
> Note: sift_up() currently uses swap, matching the existing code style. It
> could be further optimized to use copy (hold the element in a temp, shift
> parents down, write once), but that would require changing compare() to
> accept element values instead of array indices. Left for a potential
> follow-up.
Same for sift_down_root(), I guess? It could almost halve the number of
writes, right? I wonder how much of that benefit will be eaten by
caching.
René
^ permalink raw reply
* [PATCH v4 04/11] object-file: drop check for inflight transactions
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
ODB transactions are started via `odb_transaction_begin()` and contain
validation to avoid starting multiple transactions at the same time. The
"files" backend also has the same logic, but is redundant due to the
generic layer already handling it. Drop this validation from the "files"
backend accordingly.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
object-file.c | 4 ----
object-file.h | 3 +--
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/object-file.c b/object-file.c
index 33bd6c6810..e51389833a 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1678,10 +1678,6 @@ static void odb_transaction_files_commit(struct odb_transaction *base)
struct odb_transaction *odb_transaction_files_begin(struct odb_source *source)
{
struct odb_transaction_files *transaction;
- struct object_database *odb = source->odb;
-
- if (odb->transaction)
- return NULL;
transaction = xcalloc(1, sizeof(*transaction));
transaction->base.source = source;
diff --git a/object-file.h b/object-file.h
index 528c4e6e69..ea43d818f0 100644
--- a/object-file.h
+++ b/object-file.h
@@ -194,8 +194,7 @@ struct odb_transaction;
/*
* Tell the object database to optimize for adding
* multiple objects. odb_transaction_files_commit must be called
- * to make new objects visible. If a transaction is already
- * pending, NULL is returned.
+ * to make new objects visible.
*/
struct odb_transaction *odb_transaction_files_begin(struct odb_source *source);
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* [PATCH v4 03/11] object-file: embed transaction flush logic in commit function
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
When a "files" transaction is committed,
`flush_loose_object_transaction()` is invoked to handle performing a
hardware flush along with migrating the temporary object directory into
the primary and configuring the repository ODB source accordingly. The
function name here is a bit misleading because the helper is doing a bit
more than just "flushing" the transaction contents. Also, in a
subsequent commit, the transaction temporary directory is used to stage
packfiles and not just loose objects anymore.
Lift the helper function logic into `odb_transaction_files_commit()` to
more accurately signal to readers the operation being performed.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
object-file.c | 64 ++++++++++++++++++++++-----------------------------
1 file changed, 28 insertions(+), 36 deletions(-)
diff --git a/object-file.c b/object-file.c
index d68824bb44..33bd6c6810 100644
--- a/object-file.c
+++ b/object-file.c
@@ -543,41 +543,6 @@ static void odb_transaction_files_fsync(struct odb_transaction *base,
}
}
-/*
- * Cleanup after batch-mode fsync_object_files.
- */
-static void flush_loose_object_transaction(struct odb_transaction_files *transaction)
-{
- struct strbuf temp_path = STRBUF_INIT;
- struct tempfile *temp;
-
- if (!transaction->objdir)
- return;
-
- /*
- * Issue a full hardware flush against a temporary file to ensure
- * that all objects are durable before any renames occur. The code in
- * odb_transaction_files_fsync has already issued a writeout
- * request, but it has not flushed any writeback cache in the storage
- * hardware or any filesystem logs. This fsync call acts as a barrier
- * to ensure that the data in each new object file is durable before
- * the final name is visible.
- */
- strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX",
- repo_get_object_directory(transaction->base.source->odb->repo));
- temp = xmks_tempfile(temp_path.buf);
- fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));
- delete_tempfile(&temp);
- strbuf_release(&temp_path);
-
- /*
- * Make the object files visible in the primary ODB after their data is
- * fully durable.
- */
- tmp_objdir_migrate(transaction->objdir);
- transaction->objdir = NULL;
-}
-
/* Finalize a file on disk, and close it. */
static void close_loose_object(struct odb_source_loose *loose,
int fd, const char *filename)
@@ -1679,7 +1644,34 @@ static void odb_transaction_files_commit(struct odb_transaction *base)
struct odb_transaction_files *transaction =
container_of(base, struct odb_transaction_files, base);
- flush_loose_object_transaction(transaction);
+ if (transaction->objdir) {
+ struct strbuf temp_path = STRBUF_INIT;
+ struct tempfile *temp;
+
+ /*
+ * Issue a full hardware flush against a temporary file to ensure
+ * that all objects are durable before any renames occur. The code in
+ * odb_transaction_files_fsync has already issued a writeout
+ * request, but it has not flushed any writeback cache in the storage
+ * hardware or any filesystem logs. This fsync call acts as a barrier
+ * to ensure that the data in each new object file is durable before
+ * the final name is visible.
+ */
+ strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX",
+ repo_get_object_directory(transaction->base.source->odb->repo));
+ temp = xmks_tempfile(temp_path.buf);
+ fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));
+ delete_tempfile(&temp);
+ strbuf_release(&temp_path);
+
+ /*
+ * Make the object files visible in the primary ODB after their data is
+ * fully durable.
+ */
+ tmp_objdir_migrate(transaction->objdir);
+ transaction->objdir = NULL;
+ }
+
flush_packfile_transaction(transaction);
}
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* [PATCH v4 02/11] object-file: rename files transaction fsync function
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
When writing an object to a "files" ODB transaction, a full hardware
flush is not initially performed during the fsync in
`fsync_loose_object_transaction()` and instead delayed until the
transaction is later committed.
To be more consistent with other "files" ODB transaction helpers, rename
the function to `odb_transaction_files_fsync()` accordingly. The
conditional in the helper is also slightly restructured to improve
clarity to readers.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
object-file.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/object-file.c b/object-file.c
index a3eb8d71dd..d68824bb44 100644
--- a/object-file.c
+++ b/object-file.c
@@ -518,12 +518,17 @@ static void odb_transaction_files_prepare(struct odb_transaction *base)
tmp_objdir_replace_primary_odb(transaction->objdir, 0);
}
-static void fsync_loose_object_transaction(struct odb_transaction *base,
- int fd, const char *filename)
+static void odb_transaction_files_fsync(struct odb_transaction *base,
+ int fd, const char *filename)
{
struct odb_transaction_files *transaction =
container_of_or_null(base, struct odb_transaction_files, base);
+ if (!transaction || !transaction->objdir) {
+ fsync_or_die(fd, filename);
+ return;
+ }
+
/*
* If we have an active ODB transaction, we issue a call that
* cleans the filesystem page cache but avoids a hardware flush
@@ -531,8 +536,7 @@ static void fsync_loose_object_transaction(struct odb_transaction *base,
* before renaming the objects to their final names as part of
* flush_batch_fsync.
*/
- if (!transaction || !transaction->objdir ||
- git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {
+ if (git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {
if (errno == ENOSYS)
warning(_("core.fsyncMethod = batch is unsupported on this platform"));
fsync_or_die(fd, filename);
@@ -553,7 +557,7 @@ static void flush_loose_object_transaction(struct odb_transaction_files *transac
/*
* Issue a full hardware flush against a temporary file to ensure
* that all objects are durable before any renames occur. The code in
- * fsync_loose_object_transaction has already issued a writeout
+ * odb_transaction_files_fsync has already issued a writeout
* request, but it has not flushed any writeback cache in the storage
* hardware or any filesystem logs. This fsync call acts as a barrier
* to ensure that the data in each new object file is durable before
@@ -582,7 +586,7 @@ static void close_loose_object(struct odb_source_loose *loose,
goto out;
if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
- fsync_loose_object_transaction(loose->base.odb->transaction, fd, filename);
+ odb_transaction_files_fsync(loose->base.odb->transaction, fd, filename);
else if (fsync_object_files > 0)
fsync_or_die(fd, filename);
else
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* Re: [PATCH v3 2/2] prio-queue: use cascade for unfused gets
From: René Scharfe @ 2026-07-10 16:37 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget, git; +Cc: Kristofer Karlsson
In-Reply-To: <89a22c6a7532afa530f1c04ee27177e141dd360c.1783532989.git.gitgitgadget@gmail.com>
On 7/8/26 7:49 PM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> When flush_get() removes the root without an immediate replacement,
> use a cascade-then-sift-up strategy instead of sift-down.
>
> Standard sift-down places the last element at the root and sifts it
> down. This needs two comparisons per level (pick the smaller child,
> then compare against the element), even though the displaced element
> almost always ends up near the bottom where it came from.
>
> cascade_down() instead moves the vacancy down by promoting the
> smaller child at each level (one comparison per level), leaving the
> vacancy at a leaf. The last element is then placed at the vacancy
> and sift_up() floats it to its correct position, which is typically
> very little work since it already belongs near the bottom.
>
> This is the well-known "bottom-up" variant of sift-down [1].
>
> [1] https://en.wikipedia.org/wiki/Heapsort#Bottom-up_heapsort
On an Apple M1 I get a 1% slowdown for bulk describe on Git's repo:
Benchmark 1: ./git_next describe $(git rev-list v2.41.0..v2.47.0)
Time (mean ± σ): 939.5 ms ± 3.6 ms [User: 576.8 ms, System: 65.0 ms]
Range (min … max): 935.0 ms … 946.2 ms 10 runs
Benchmark 2: ./git describe $(git rev-list v2.41.0..v2.47.0)
Time (mean ± σ): 945.5 ms ± 3.3 ms [User: 581.6 ms, System: 67.5 ms]
Range (min … max): 940.1 ms … 950.5 ms 10 runs
Summary
./git_next describe $(git rev-list v2.41.0..v2.47.0) ran
1.01 ± 0.01 times faster than ./git describe $(git rev-list v2.41.0..v2.47.0)
... and on Linux's repo:
Benchmark 1: ./git_next -C ../linux describe $(git -C ../linux rev-list v4.0..v4.1)
Time (mean ± σ): 4.880 s ± 0.014 s [User: 3.914 s, System: 0.252 s]
Range (min … max): 4.864 s … 4.905 s 10 runs
Benchmark 2: ./git -C ../linux describe $(git -C ../linux rev-list v4.0..v4.1)
Time (mean ± σ): 4.917 s ± 0.011 s [User: 3.948 s, System: 0.254 s]
Range (min … max): 4.902 s … 4.938 s 10 runs
Summary
./git_next -C ../linux describe $(git -C ../linux rev-list v4.0..v4.1) ran
1.01 ± 0.00 times faster than ./git -C ../linux describe $(git -C ../linux rev-list v4.0..v4.1)
I see a 1% slowdown on an Apple M5 as well in both cases. I can't
reproduce it on a Ryzen laptop, but that's too noisy to measure 1%
changes anyway.
Checked the total number of prio_queue comparisons with the crude patch
below, and as expected they go down, from 70386235 to 60682175 for Git
and from 473983445 to 439809087 for Linux. So there's less work to do,
still user time goes up -- no idea why.
Also this -- what's up with the system time here:
Benchmark 1: ./git_next rev-list --all --count
Time (mean ± σ): 115.2 ms ± 0.8 ms [User: 95.6 ms, System: 17.7 ms]
Range (min … max): 113.0 ms … 117.1 ms 24 runs
Benchmark 2: ./git rev-list --all --count
Time (mean ± σ): 116.5 ms ± 0.8 ms [User: 95.4 ms, System: 19.0 ms]
Range (min … max): 115.1 ms … 118.6 ms 24 runs
Summary
./git_next rev-list --all --count ran
1.01 ± 0.01 times faster than ./git rev-list --all --count
But:
Benchmark 1: ./git_next -C ../linux rev-list --all --count
Time (mean ± σ): 937.6 ms ± 2.2 ms [User: 887.2 ms, System: 45.5 ms]
Range (min … max): 933.2 ms … 939.9 ms 10 runs
Benchmark 2: ./git -C ../linux rev-list --all --count
Time (mean ± σ): 937.3 ms ± 1.7 ms [User: 887.8 ms, System: 45.0 ms]
Range (min … max): 934.6 ms … 940.3 ms 10 runs
Summary
./git -C ../linux rev-list --all --count ran
1.00 ± 0.00 times faster than ./git_next -C ../linux rev-list --all --count
:-?
> Helped-by: Rene Scharfe <l.s.r@web.de>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> prio-queue.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/prio-queue.c b/prio-queue.c
> index 926fc04e85..230d6f5e33 100644
> --- a/prio-queue.c
> +++ b/prio-queue.c
> @@ -66,13 +66,31 @@ static void sift_down_root(struct prio_queue *queue)
> }
> }
>
> +/* Cascade vacancy toward a leaf, promoting the smaller child at each level */
> +static size_t cascade_down(struct prio_queue *queue)
> +{
> + size_t ix, child;
> +
> + for (ix = 0; (child = ix * 2 + 1) < queue->nr_; ix = child) {
> + if (child + 1 < queue->nr_ &&
> + compare(queue, child, child + 1) >= 0)
> + child++;
> + queue->array[ix] = queue->array[child];
> + }
> + return ix;
> +}
> +
> static inline void flush_get(struct prio_queue *queue)
> {
> + size_t ix;
> +
> if (!queue->get_pending)
> return;
> queue->get_pending = 0;
> - queue->array[0] = queue->array[--queue->nr_];
> - sift_down_root(queue);
> + --queue->nr_;
> + ix = cascade_down(queue);
> + queue->array[ix] = queue->array[queue->nr_];
> + sift_up(queue, ix);
> }
>
> void prio_queue_put(struct prio_queue *queue, void *thing)
The patch looks fine, though. It introduces struct assignments, but
they should be OK. Tried replacing them with swap() instead (which
does a useless extra write), but that didn't change the performance
(still 1% slowdown). Odd.
René
diff --git a/builtin/describe.c b/builtin/describe.c
index c0abc931a59..4a6ad976d30 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -791,5 +791,6 @@ int cmd_describe(int argc,
while (argc-- > 0)
describe(*argv++, argc == 0);
}
+ print_compares();
return 0;
}
diff --git a/prio-queue.c b/prio-queue.c
index 199775d5afd..b0189bf80e6 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -1,6 +1,13 @@
#include "git-compat-util.h"
#include "prio-queue.h"
+static uintmax_t compares;
+
+void print_compares(void)
+{
+ fprintf(stderr, "compares: %lu\n", compares);
+}
+
static inline int compare(struct prio_queue *queue, size_t i, size_t j)
{
int cmp = queue->compare(queue->array[i].data, queue->array[j].data,
@@ -8,6 +15,7 @@ static inline int compare(struct prio_queue *queue, size_t i, size_t j)
if (!cmp)
cmp = (queue->array[i].ctr > queue->array[j].ctr) -
(queue->array[i].ctr < queue->array[j].ctr);
+ compares++;
return cmp;
}
diff --git a/prio-queue.h b/prio-queue.h
index 570b48e6485..e4cc0c4fb83 100644
--- a/prio-queue.h
+++ b/prio-queue.h
@@ -68,4 +68,6 @@ void clear_prio_queue(struct prio_queue *);
/* Reverse the LIFO elements */
void prio_queue_reverse(struct prio_queue *);
+void print_compares(void);
+
#endif /* PRIO_QUEUE_H */
^ permalink raw reply related
* [PATCH v4 01/11] object-file: rename files transaction prepare function
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260710163722.2962278-1-jltobler@gmail.com>
The "files" ODB transaction backend lazily creates a temporary object
directory when the first loose object is written to the transaction via
`prepare_loose_object_transaction()`. In a subsequent commit, the
temporary directory is used to also write packfiles to.
Rename the function to `odb_transaction_files_prepare()` accordingly.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
object-file.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/object-file.c b/object-file.c
index e3d92bbda2..a3eb8d71dd 100644
--- a/object-file.c
+++ b/object-file.c
@@ -499,7 +499,7 @@ struct odb_transaction_files {
struct transaction_packfile packfile;
};
-static void prepare_loose_object_transaction(struct odb_transaction *base)
+static void odb_transaction_files_prepare(struct odb_transaction *base)
{
struct odb_transaction_files *transaction =
container_of_or_null(base, struct odb_transaction_files, base);
@@ -761,7 +761,7 @@ int write_loose_object(struct odb_source_loose *loose,
static struct strbuf filename = STRBUF_INIT;
if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
- prepare_loose_object_transaction(loose->base.odb->transaction);
+ odb_transaction_files_prepare(loose->base.odb->transaction);
odb_loose_path(loose, &filename, oid);
@@ -825,7 +825,7 @@ int odb_source_loose_write_stream(struct odb_source_loose *loose,
int hdrlen;
if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
- prepare_loose_object_transaction(loose->base.odb->transaction);
+ odb_transaction_files_prepare(loose->base.odb->transaction);
/* Since oid is not determined, save tmp file to odb path. */
strbuf_addf(&filename, "%s/", loose->base.path);
--
2.55.0.122.gf85a7e6620
^ permalink raw reply related
* [PATCH v4 00/11] receive-pack: use ODB transactions to stage object writes
From: Justin Tobler @ 2026-07-10 16:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-1-jltobler@gmail.com>
Greetings,
This patch series replaces direct usage of the `tmp_objdir` interfaces
in git-receive-pack(1) to instead use the `odb_transaction` interfaces
to create/manage a staging area to write objects to. The purpose of this
change is to get git-receive-pack(1) one step closer to being ODB
backend agnostic. For now, the object writes themselves are still
"files" backend specific due to being handled by the git-index-pack(1)
and git-unpack-objects(1) child processes. This will be tackled in a
separate series though.
Changes since V3:
- Removed ugly line break in commit message to prevent eye strain.
- `odb_transaction_begin()` now only sets the repository transaction
on success.
- `odb_transaction_env()` now bubbles up error when failing to create
the temporary directory.
Changes since V2:
- Clarified commit log reasoning for embedding
`flush_loose_object_transaction()` logic in commit function.
- Started printed some error messages on transaction errors.
- Removed include statement.
- Fixed transaction leak on `odb_transaction_commit()` error.
Changes since V1:
- Adapted other "file" ODB transaction helpers to be more consistent
with current naming scheme.
- Removed redundant NULL transaction handling from
`odb_transaction_files_begin()`.
- `odb_transaction_begin()` now returns an error if there is already
an inflight transaction pending instead of setting the `out` pointer
to NULL.
- Updated `odb_transaction_env()` to return an error code and append
environment variables to a strvec provided as an argument.
- Removed redundant setting of tmpdir environment variables for child
processes after tmpdir has been migrated.
- Split changes adding ODB transaction flags into a separate commit.
- Consistently wire the ODB transaction throughout git-receive-pack
code instead of reading it from `the_repository`.
- Updated user facing error message.
- Updated some comments to better document functions/flags.
- Clarified some commit messages.
- Fixed typos.
Thanks,
-Justin
Justin Tobler (11):
object-file: rename files transaction prepare function
object-file: rename files transaction fsync function
object-file: embed transaction flush logic in commit function
object-file: drop check for inflight transactions
object-file: propagate files transaction errors
odb/transaction: propagate begin errors
odb/transaction: propagate commit errors
odb/transaction: add transaction env interface
odb/transaction: introduce ODB transaction flags
builtin/receive-pack: drop redundant tmpdir env
builtin/receive-pack: stage incoming objects via ODB transactions
builtin/add.c | 2 +-
builtin/receive-pack.c | 69 ++++++++---------
builtin/unpack-objects.c | 2 +-
builtin/update-index.c | 2 +-
cache-tree.c | 7 +-
object-file.c | 161 +++++++++++++++++++++++++--------------
object-file.h | 8 +-
odb/source-files.c | 9 +--
odb/source-inmemory.c | 3 +-
odb/source-loose.c | 3 +-
odb/source.h | 9 ++-
odb/transaction.c | 33 ++++++--
odb/transaction.h | 59 +++++++++++---
read-cache.c | 7 +-
14 files changed, 244 insertions(+), 130 deletions(-)
Range-diff against v3:
1: 9c14b219ad = 1: 9c14b219ad object-file: rename files transaction prepare function
2: 5703a9e93b = 2: 5703a9e93b object-file: rename files transaction fsync function
3: 76204847f2 ! 3: 70267741b0 object-file: embed transaction flush logic in commit function
@@ Commit message
subsequent commit, the transaction temporary directory is used to stage
packfiles and not just loose objects anymore.
- Lift the helper function logic directly into
- `odb_transaction_files_commit()` to more accurately signal to readers
- the operation being performed.
+ Lift the helper function logic into `odb_transaction_files_commit()` to
+ more accurately signal to readers the operation being performed.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
4: c97eb7763f = 4: 34cd3822c5 object-file: drop check for inflight transactions
5: 1f3a1f7714 = 5: 240aa3475f object-file: propagate files transaction errors
6: 09d13272d5 ! 6: 0d91310fac odb/transaction: propagate begin errors
@@ odb/transaction.c
- odb_source_begin_transaction(odb->sources, &odb->transaction);
+ ret = odb_source_begin_transaction(odb->sources, out);
-+ odb->transaction = *out;
++ if (!ret)
++ odb->transaction = *out;
- return odb->transaction;
+ return ret;
7: 12833d6773 = 7: 5e4680ed75 odb/transaction: propagate commit errors
8: f2586f2f34 ! 8: babcf6b156 odb/transaction: add transaction env interface
@@ object-file.c: static int odb_transaction_files_commit(struct odb_transaction *b
+{
+ struct odb_transaction_files *transaction =
+ container_of(base, struct odb_transaction_files, base);
++ int ret;
+
-+ odb_transaction_files_prepare(&transaction->base);
-+ strvec_pushv(env, tmp_objdir_env(transaction->objdir));
++ ret = odb_transaction_files_prepare(&transaction->base);
++ if (!ret)
++ strvec_pushv(env, tmp_objdir_env(transaction->objdir));
+
-+ return 0;
++ return ret;
+}
+
int odb_transaction_files_begin(struct odb_source *source,
9: 9d082b5e47 ! 9: 96f2a21eec odb/transaction: introduce ODB transaction flags
@@ odb/transaction.c
- ret = odb_source_begin_transaction(odb->sources, out);
+ ret = odb_source_begin_transaction(odb->sources, out, flags);
- odb->transaction = *out;
+ if (!ret)
+ odb->transaction = *out;
- return ret;
## odb/transaction.h ##
@@
10: e11d8a6676 = 10: 56718f1190 builtin/receive-pack: drop redundant tmpdir env
11: fee57c2817 = 11: 5197a19fbf builtin/receive-pack: stage incoming objects via ODB transactions
base-commit: ab776a62a78576513ee121424adb19597fbb7613
--
2.55.0.122.gf85a7e6620
^ permalink raw reply
* Re: [PATCH v2 00/12] coverity: avoid dereferencing NULL
From: Junio C Hamano @ 2026-07-10 15:46 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> This is a continuation of the effort I started in the patch series that
> became js/coverity-fixes. This next batch adds guards to avoid dereferencing
> NULL pointers and accessing NULL file descriptors.
>
> Changes since v1:
>
> * Calling remote_tracking() no longer returns -1 when remote is NULL, but
> instead BUG()s out.
> * bisect_successful() returns with BISECT_FAILED instead of the -1 that
> only worked by happenstance.
> * The commit "revision: avoid dereferencing NULL in add_parents_only()" now
> comes with a regression test.
> * The commit "bisect: ensure non-NULL head before using it" no longer
> claims that the fixed bug can be triggered with the current code base.
> * The missing shallow commit's OID is no longer computed twice.
> * A follow-up commit was folded into this patch series that lets
> write_one_shallow() avoid the rolling buffers of oid_to_hex(), as
> suggested by Junio. It technically does not fit the goal of this patch
> series (fixing issues pointed out by Coverity), but was asked for
> explicitly.
> ...
> Range-diff vs v1:
> ...
I found everything including the new patch good. Unless others find
more issues in this round in a few days, let's mark the topic for
'next'.
Thanks.
^ permalink raw reply
* Re: [PATCH v18 0/7] branch: delete-merged
From: Phillip Wood @ 2026-07-10 15:26 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Kristoffer Haugsbakk, Johannes Sixt, Harald Nordgren
In-Reply-To: <pull.2285.v18.git.git.1782338106.gitgitgadget@gmail.com>
Hi Harald
I've looked through the first five patches and left a few comments and
queries. I'll look at the last two at the start of next week so don't
re-roll just yet, but my all means reply to the comments, especially
where I've asked questions. So far I like what I've seen.
Thanks
Phillip
On 24/06/2026 22:54, Harald Nordgren via GitGitGadget wrote:
> Delete branches that have already been merged on upstream.
>
> Changes in v18:
>
> * Instead of keeping the whole chain of upstream branches, keep only the
> ones an unmerged branch still needs. When a kept (merged) branch in turn
> tracks a branch that is being deleted, clear its now-stale upstream
> config.
> * Rework spare_stacked_bases() to record the kept bases and, in a second
> pass, clear the upstream of any whose own base is going away. Build the
> to-delete list with strset_for_each_entry() instead of re-walking the
> candidate array.
>
> Changes in v17:
>
> * Keep a merged branch when another surviving branch still tracks it as its
> upstream, so --delete-merged no longer deletes a branch out from under
> one stacked on top of it.
> * Move the --dry-run and branch.<name>.deleteMerged opt-out fully into
> their own commits.
>
> Changes in v16:
>
> * Convert delete_merged_branches() to take an unsigned int flags argument
> instead of separate quiet/dry_run booleans, matching delete_branches()
> * Reuse the strbuf across the skip-config loop (strbuf_reset per iteration,
> single strbuf_release after) instead of allocating and freeing it each
> time
> * Rewrite the --delete-merged tests as integration tests: branches that
> land commits upstream, with deletion and the checked-out, upstream-gone,
> and push-equals-upstream safety cases exercised together in one run and
> output asserted via test_cmp
> * Collapse the many per-aspect test repos into a single reused repo set up
> by a setup_repo_for_delete_merged helper, and rename helpers off the old
> pm_/prune naming
> * Nest single-repo setup sequences in ( cd ... ) subshells instead of
> prefixing every command with -C
>
> Changes in v15:
>
> * Renamed --prune-merged to --delete-merged throughout. Not necessarily
> final, but something to advance the discussion.
> * --delete-merged now silently skips not-yet-merged branches instead of
> warning.
> * Initialized the delete_branches() flag locals where declared. Only force
> stays deferred.
> * delete_branches()/check_branch_commit() doc and code cleanups: redundant
> branch NULL checks dropped, ref_array candidates = { 0 }, a BUG() for the
> unreachable non-branch ref, and reworked --delete-merged doc wording.
> * Broadened the --forked tests (local commits for realism, remote add -f,
> --forked coverage), renamed the misleading trunk fixture, and replaced
> the misnamed detached branch with git checkout --detach.
>
> Changes in v14:
>
> * Fixed a git branch -d -r regression (broke t5404/t5505/t5514): the
> remotes path set a local force but not the DELETE_BRANCH_FORCE bit that
> check_branch_commit() reads, so it wrongly ran the merge check.
> * Made flags the single source of truth in delete_branches() so the bit and
> the derived locals can't disagree.
> * Works locally, but GitHub CI has problems that are there for other
> branches too, hopefully not related
> (https://github.com/git/git/pull/2285).
>
> Changes in v13:
>
> * Reworked --forked into a real ref-filter applied in apply_ref_filter()
> instead of a post-pass, so non-matching branches are never allocated.
> * Match exact --forked patterns on full refnames (only globs use the
> abbreviated upstream), and dropped the old helper machinery, forward
> declaration, and string_list in favor of a strvec.
> * Replaced the boolean parameters of
> delete_branches()/check_branch_commit() with a single unsigned int flags.
> * --prune-merged now collects candidates via filter_refs() rather than its
> own branch walk.
> * --prune-merged now takes its patterns as positional arguments (e.g. git
> branch --prune-merged origin/main 'feature*') instead of repeating the
> option.
>
> Changes in v12:
>
> * Reworked --forked from a standalone action into a --list-mode filter.
> * Switched --forked and --prune-merged to repeatable OPT_STRING_LIST
> options.
> * Dropped the bare-remote-name resolution for --forked, the argument is now
> a ref or a glob.
>
> Changes in v11:
>
> * The flags now take a branch, not a remote. --forked and --prune-merged
> accept a literal upstream short name like origin/main or a wildmatch
> pattern like origin/. The old --all-remotes flag is gone, since origin/
> covers that case.
> * The prune guard now compares @{push} against @{upstream}. A branch is
> spared when these are equal. That is the trunk like case, such as local
> main tracking and pushing to origin/main, where "fully merged to
> upstream" cannot be told apart from "just pulled". Only branches that
> push somewhere other than their upstream, typically fork based topics,
> are candidates. The earlier /HEAD by name guard that the reviewer
> rejected is gone.
> * New --dry-run for --prune-merged.
>
> Changes in v10:
>
> * --forked / --prune-merged now take a branch glob instead of a remote name
> — origin, origin/*, origin/release-- all work. This replaces the
> remote-only form and subsumes the old --all-remotes flag, which has been
> dropped.
> * New --dry-run for --prune-merged.
>
> Changes in v9:
>
> * --force no longer has special meaning with --prune-merged; reachability
> is always enforced. Use git branch -D to delete an unmerged branch.
> Matches how git branch's other read/safe actions treat --force.
> * Synopsis drops [-f]; "not fully merged" hint points at git branch -D.
> * Dropped the --prune-merged --force tests.
>
> Changes in v8:
>
> * Delete only when the branch's work is actually reachable from its
> upstream
> * Skip branches whose upstream is gone (even with --force)
> * Simplified the internal safety flag to live in one place
>
> Changes in v7:
>
> * --prune-merged now checks if a branch is merged into its own upstream
> first. If the upstream is gone, it checks against the remote's default
> branch instead. If neither exists, the branch is refused (use --force to
> delete anyway).
>
> Changes in v6:
>
> * --prune-merged now measures merged-ness against the remote's default
> branch instead of the candidate's upstream — so the decision no longer
> depends on which branch happens to be checked out locally.
> * delete_branches() / check_branch_commit() gained a per-candidate override
> that lets a caller substitute a different "what counts as merged"
> reference (or skip the check). branch -d callers pass NULL and keep their
> existing semantics.
> * prune_merged_branches() resolves each candidate's push-remote HEAD and
> threads it through, so --prune-merged --all-remotes measures each
> candidate against its own remote rather than a single global reference.
>
> Changes in v5:
>
> * Drop commit 'fetch: add --prune-merged'
>
> Changes in v4:
>
> * Resolve each remote's HEAD and collect the targets into a
> protected_default_refs set in collect_forked_set.
> * In prune_merged_branches, skip a candidate when its upstream is a
> protected default ref and the local branch name matches the default
> branch's leaf name (so a local main tracking origin/main is spared, but a
> renamed trunk tracking origin/main is not).
> * Also skip when the candidate's push ref points at a protected default
> ref, so a topic branch configured to push to origin/main is never pruned.
> * Tests: spare the local default branch; only protect by matching leaf name
> (not by upstream alone); spare a branch whose push ref is the remote
> default.
>
> Changes in v3:
>
> * s/remote-tracking refs/remote-tracking branches/g
>
> Changes in v2:
>
> * The whole feature moved out of git fetch and into git branch. git fetch
> --prune-merged now just calls git branch --prune-merged after fetching.
> * The fetch.pruneLocalBranches and remote..pruneLocalBranches config
> options are gone, replaced by per-branch opt-out via branch..pruneMerged.
> * New git branch --forked lists local branches whose upstream lives on the
> given remote (read-only building block).
> * New git branch --prune-merged deletes those branches, but only if their
> tip is reachable from the upstream tracking ref; --force skips that
> safety check.
> * New git branch --all-remotes lets --forked/--prune-merged operate across
> every configured remote at once.
> * The currently checked-out branch in any worktree is always preserved.
> * branch..pruneMerged=false lets you exempt a branch (e.g. a long-running
> topic branch) even with --force; doesn't affect explicit git branch -d.
> * delete_branches() got a warn_only mode so bulk deletion prints a one-line
> warning per skipped branch instead of the noisy four-line hint that git
> branch -d shows.
> * New section in git-branch docs; git-fetch docs trimmed to just mention
> --prune-merged.
> * New tests in t3200-branch.sh for the new branch flags; t5510-fetch.sh
> shrunk since most logic moved.
>
> Harald Nordgren (7):
> branch: add --forked filter for --list mode
> branch: convert delete_branches() to a flags argument
> branch: let delete_branches skip unmerged branches on bulk refusal
> branch: prepare delete_branches for a bulk caller
> branch: add --delete-merged <branch>
> branch: add branch.<name>.deleteMerged opt-out
> branch: add --dry-run for --delete-merged
>
> Documentation/config/branch.adoc | 7 +
> Documentation/git-branch.adoc | 48 ++++-
> builtin/branch.c | 266 +++++++++++++++++++++---
> ref-filter.c | 70 +++++++
> ref-filter.h | 10 +
> t/t3200-branch.sh | 342 +++++++++++++++++++++++++++++++
> 6 files changed, 715 insertions(+), 28 deletions(-)
>
>
> base-commit: ab776a62a78576513ee121424adb19597fbb7613
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2285%2FHaraldNordgren%2Ffetch-prune-local-branches-v18
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2285/HaraldNordgren/fetch-prune-local-branches-v18
> Pull-Request: https://github.com/git/git/pull/2285
>
> Range-diff vs v17:
>
> 1: d8cc17bd7f = 1: 3e29ff17bd branch: add --forked filter for --list mode
> 2: d14b0403f0 = 2: cdd4fea4a7 branch: convert delete_branches() to a flags argument
> 3: ef2719dac3 = 3: a0fd5b4a6c branch: let delete_branches skip unmerged branches on bulk refusal
> 4: 80518f5d11 = 4: a56d8fe93e branch: prepare delete_branches for a bulk caller
> 5: 46da7c8140 ! 5: a84c555d99 branch: add --delete-merged <branch>
> @@ Commit message
> upstream. The work has already landed on the upstream they track,
> so the local copy is no longer needed.
>
> - Three kinds of branches are not deleted:
> + A branch is not deleted when:
>
> - * any branch checked out in any worktree
> - * any branch whose upstream remote-tracking branch no longer
> - exists, since a missing upstream is not by itself a sign of
> - integration
> - * any branch whose push destination equals its upstream
> - (<branch>@{push} is the same as <branch>@{upstream}), such as
> - a local "main" that tracks and pushes to "origin/main". Right
> - after a pull it just looks "fully merged", so it is kept. Only
> - branches that push somewhere other than their upstream,
> - typically topics in a fork workflow, are candidates.
> + * it is checked out in any worktree
> + * its upstream remote-tracking branch no longer exists, since a
> + missing upstream is not by itself a sign of integration
> + * its push destination equals its upstream (<branch>@{push} is
> + the same as <branch>@{upstream}), such as a local "main" that
> + tracks and pushes to "origin/main". Right after a pull it just
> + looks "fully merged", so it is kept. Only branches that push
> + somewhere other than their upstream, typically topics in a fork
> + workflow, are candidates.
>
> A branch whose work is not yet merged into its upstream is silently
> skipped, so one unmerged topic does not abort the whole sweep.
>
> A branch that another, surviving branch tracks as its upstream is
> also kept, so a branch is never deleted out from under one stacked
> - on top of it. Sparing such a base can in turn protect its own
> - upstream, so the check repeats until the set stops changing.
> + on top of it. Such a kept branch is itself merged, so when its own
> + upstream is being deleted, clear its now-stale upstream config.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
>
> @@ Documentation/git-branch.adoc: This option is only applicable in non-verbose mod
> +silently skipped. Delete it with `git branch -D` if you want to
> +remove it anyway.
> ++
> -+A branch that another, surviving branch still tracks as its upstream
> -+is kept, so a branch is never deleted out from under one stacked on
> -+top of it.
> ++A branch that another, surviving branch tracks as its upstream is
> ++kept, so a branch is never deleted out from under one stacked on top
> ++of it. If that kept branch in turn tracks a branch that is being
> ++deleted, its now-stale upstream configuration is cleared.
> +
> `-v`::
> `-vv`::
> @@ builtin/branch.c: static int parse_opt_forked(const struct option *opt, const ch
> return 0;
> }
>
> -+static int collect_upstream(const struct reference *ref, void *cb_data)
> -+{
> -+ struct string_list *upstreams = cb_data;
> -+ struct branch *branch = branch_get(ref->name);
> -+ const char *upstream = branch_get_upstream(branch, NULL);
> ++struct spare_data {
> ++ struct strset *deletable;
> ++ struct strset *spared;
> ++};
> +
> -+ string_list_append(upstreams, ref->name)->util =
> -+ xstrdup_or_null(upstream);
> ++/*
> ++ * A surviving branch stacked on a deletion candidate would lose its
> ++ * upstream, so drop that candidate from the delete set and remember it
> ++ * in "spared" so its own upstream can be tidied up afterwards.
> ++ */
> ++static int spare_stacked_base(const struct reference *ref, void *cb_data)
> ++{
> ++ struct spare_data *data = cb_data;
> ++ struct branch *branch;
> ++ const char *upstream, *up_short;
> ++
> ++ if (strset_contains(data->deletable, ref->name))
> ++ return 0;
> ++ branch = branch_get(ref->name);
> ++ upstream = branch_get_upstream(branch, NULL);
> ++ if (!upstream || !skip_prefix(upstream, "refs/heads/", &up_short) ||
> ++ !strset_contains(data->deletable, up_short))
> ++ return 0;
> ++
> ++ strset_remove(data->deletable, up_short);
> ++ strset_add(data->spared, up_short);
> + return 0;
> +}
> +
> +/*
> -+ * Keep any branch that another, surviving branch tracks as its
> -+ * upstream, so we never delete a branch out from under one stacked on
> -+ * top of it. Sparing a branch makes it a survivor whose own upstream
> -+ * then needs the same protection, so repeat until nothing changes.
> ++ * Keep any branch that a surviving branch tracks as its upstream, so we
> ++ * never delete a branch out from under one stacked on top of it. Such a
> ++ * base is itself merged, so when its own upstream is also going away
> ++ * (no surviving branch tracks it), clear the base's now-stale upstream.
> + */
> +static void spare_stacked_bases(struct ref_store *refs, struct strset *deletable)
> +{
> -+ struct string_list upstreams = STRING_LIST_INIT_DUP;
> -+ struct string_list_item *item;
> -+ bool spared;
> -+
> -+ refs_for_each_branch_ref(refs, collect_upstream, &upstreams);
> -+ do {
> -+ spared = false;
> -+ for_each_string_list_item(item, &upstreams) {
> -+ const char *up = item->util, *up_short;
> -+
> -+ if (!up || strset_contains(deletable, item->string))
> -+ continue;
> -+ if (!skip_prefix(up, "refs/heads/", &up_short) ||
> -+ !strset_contains(deletable, up_short))
> -+ continue;
> -+
> -+ strset_remove(deletable, up_short);
> -+ spared = true;
> -+ }
> -+ } while (spared);
> -+
> -+ string_list_clear(&upstreams, 1);
> ++ struct strset spared = STRSET_INIT;
> ++ struct spare_data data = { .deletable = deletable, .spared = &spared };
> ++ struct strbuf key = STRBUF_INIT;
> ++ struct hashmap_iter iter;
> ++ struct strmap_entry *entry;
> ++
> ++ refs_for_each_branch_ref(refs, spare_stacked_base, &data);
> ++
> ++ strset_for_each_entry(&spared, &iter, entry) {
> ++ struct branch *branch = branch_get(entry->key);
> ++ const char *upstream = branch_get_upstream(branch, NULL);
> ++ const char *up_short;
> ++
> ++ if (!upstream || !skip_prefix(upstream, "refs/heads/", &up_short) ||
> ++ !strset_contains(deletable, up_short))
> ++ continue;
> ++
> ++ strbuf_reset(&key);
> ++ strbuf_addf(&key, "branch.%s.merge", branch->name);
> ++ repo_config_set_gently(the_repository, key.buf, NULL);
> ++ strbuf_reset(&key);
> ++ strbuf_addf(&key, "branch.%s.remote", branch->name);
> ++ repo_config_set_gently(the_repository, key.buf, NULL);
> ++ }
> ++
> ++ strbuf_release(&key);
> ++ strset_clear(&spared);
> +}
> +
> +static int delete_merged_branches(int argc, const char **argv,
> @@ builtin/branch.c: static int parse_opt_forked(const struct option *opt, const ch
> + struct ref_array candidates = { 0 };
> + struct strset deletable = STRSET_INIT;
> + struct strvec to_delete = STRVEC_INIT;
> ++ struct hashmap_iter iter;
> ++ struct strmap_entry *entry;
> + int i, ret = 0;
> +
> + if (!argc)
> @@ builtin/branch.c: static int parse_opt_forked(const struct option *opt, const ch
> +
> + spare_stacked_bases(refs, &deletable);
> +
> -+ for (i = 0; i < candidates.nr; i++) {
> -+ const char *short_name;
> -+
> -+ if (skip_prefix(candidates.items[i]->refname, "refs/heads/",
> -+ &short_name) &&
> -+ strset_contains(&deletable, short_name))
> -+ strvec_push(&to_delete, short_name);
> -+ }
> ++ strset_for_each_entry(&deletable, &iter, entry)
> ++ strvec_push(&to_delete, entry->key);
> +
> + if (to_delete.nr)
> + ret = delete_branches(to_delete.nr, to_delete.v,
> @@ t/t3200-branch.sh: test_expect_success '--forked narrows a <pattern> argument' '
> + git checkout --detach
> + ) &&
> +
> ++ git -C repo branch --dry-run --delete-merged origin/next >out &&
> ++ test_grep ! "feature" out &&
> ++
> + git -C repo branch --delete-merged origin/next 2>err &&
> +
> + test_must_be_empty err &&
> + git -C repo rev-parse --verify refs/heads/feature &&
> -+ git -C repo rev-parse --verify refs/heads/topic
> ++ git -C repo rev-parse --verify refs/heads/topic &&
> ++ echo origin/next >expect &&
> ++ git -C repo rev-parse --abbrev-ref feature@{upstream} >actual &&
> ++ test_cmp expect actual &&
> ++ echo feature >expect &&
> ++ git -C repo rev-parse --abbrev-ref topic@{upstream} >actual &&
> ++ test_cmp expect actual
> +'
> +
> +test_expect_success '--delete-merged keeps a chain of upstreams of a kept branch' '
> @@ t/t3200-branch.sh: test_expect_success '--forked narrows a <pattern> argument' '
> + EOF
> + test_cmp expect actual
> +'
> ++
> ++test_expect_success '--delete-merged clears the upstream of a kept base whose own base is deleted' '
> ++ test_when_finished "rm -rf repo" &&
> ++ setup_repo_for_delete_merged &&
> ++ (
> ++ cd repo &&
> ++ git branch lower origin/next &&
> ++ git branch --set-upstream-to=origin/next lower &&
> ++ git branch mid origin/next &&
> ++ git branch --set-upstream-to=lower mid &&
> ++ git checkout -b tip mid &&
> ++ git commit --allow-empty -m "tip work" &&
> ++ git branch --set-upstream-to=mid tip &&
> ++ git checkout --detach
> ++ ) &&
> ++
> ++ git -C repo branch --delete-merged origin/next lower &&
> ++
> ++ test_must_fail git -C repo rev-parse --verify refs/heads/lower &&
> ++ git -C repo rev-parse --verify refs/heads/mid &&
> ++ test_must_fail git -C repo rev-parse mid@{upstream} &&
> ++ echo mid >expect &&
> ++ git -C repo rev-parse --abbrev-ref tip@{upstream} >actual &&
> ++ test_cmp expect actual
> ++'
> +
> test_done
> 6: 27903fbb1d ! 6: d52d717b70 branch: add branch.<name>.deleteMerged opt-out
> @@ builtin/branch.c: static int delete_merged_branches(int argc, const char **argv,
> struct strset deletable = STRSET_INIT;
> struct strvec to_delete = STRVEC_INIT;
> + struct strbuf key = STRBUF_INIT;
> + struct hashmap_iter iter;
> + struct strmap_entry *entry;
> + bool quiet = flags & DELETE_BRANCH_QUIET;
> int i, ret = 0;
>
> @@ builtin/branch.c: static int delete_merged_branches(int argc, const char **argv,
> ref_array_clear(&candidates);
>
> ## t/t3200-branch.sh ##
> -@@ t/t3200-branch.sh: test_expect_success '--delete-merged keeps a chain of upstreams of a kept branch
> +@@ t/t3200-branch.sh: test_expect_success '--delete-merged clears the upstream of a kept base whose ow
> test_cmp expect actual
> '
>
> 7: 49c1bcf1fb ! 7: 8d0323f4b3 branch: add --dry-run for --delete-merged
> @@ Documentation/git-branch.adoc: git branch (-m|-M) [<old-branch>] <new-branch>
>
> DESCRIPTION
> -----------
> -@@ Documentation/git-branch.adoc: A branch that another, surviving branch still tracks as its upstream
> - is kept, so a branch is never deleted out from under one stacked on
> - top of it.
> +@@ Documentation/git-branch.adoc: kept, so a branch is never deleted out from under one stacked on top
> + of it. If that kept branch in turn tracks a branch that is being
> + deleted, its now-stale upstream configuration is cleared.
>
> +`--dry-run`::
> + With `--delete-merged`, print which branches would be
>
^ permalink raw reply
* Re: [PATCH v18 5/7] branch: add --delete-merged <branch>
From: Phillip Wood @ 2026-07-10 15:24 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Kristoffer Haugsbakk, Johannes Sixt, Phillip Wood,
Harald Nordgren
In-Reply-To: <a84c555d99c98abcd4618833c14fa35b191fbda2.1782338106.git.gitgitgadget@gmail.com>
Hi Harald
On 24/06/2026 22:55, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> git branch --delete-merged <branch>...
This design means that unlike --forked there is no way to limit the
branches considered for deletion. I wonder if we'd be better to have
--delete-merged take an argument like --forked so that the user can
limit the branches that might be deleted without resorting to the config
setting added in the next patch.
> deletes the local branches that "--forked <branch>" would list,
> keeping only those whose tip is reachable from their configured
> upstream. The work has already landed on the upstream they track,
> so the local copy is no longer needed.
>
> A branch is not deleted when:
>
> * it is checked out in any worktree
> * its upstream remote-tracking branch no longer exists, since a
> missing upstream is not by itself a sign of integration
> * its push destination equals its upstream (<branch>@{push} is
> the same as <branch>@{upstream}), such as a local "main" that
> tracks and pushes to "origin/main". Right after a pull it just
> looks "fully merged", so it is kept. Only branches that push
> somewhere other than their upstream, typically topics in a fork
> workflow, are candidates.
>
> A branch whose work is not yet merged into its upstream is silently
> skipped, so one unmerged topic does not abort the whole sweep.
>
> A branch that another, surviving branch tracks as its upstream is
> also kept, so a branch is never deleted out from under one stacked
> on top of it. Such a kept branch is itself merged, so when its own
> upstream is being deleted, clear its now-stale upstream config.
The commit message explains the new feature really well. The
implementation looks good, I've left a few questions and comments on the
tests
> +static void spare_stacked_bases(struct ref_store *refs, struct strset *deletable)
> +{
> [...]
> + strbuf_addf(&key, "branch.%s.merge", branch->name);
> + repo_config_set_gently(the_repository, key.buf, NULL);
> + strbuf_reset(&key);
> + strbuf_addf(&key, "branch.%s.remote", branch->name);
> + repo_config_set_gently(the_repository, key.buf, NULL);
If there are any errors updating the config then the config code will
print a message and we continue. As clearing the config is really a
convenience feature I think it is fine to ignore errors here.
> +static int delete_merged_branches(int argc, const char **argv,
> + unsigned int flags)
> [...]
> + if (check_branch_commit(short_name, short_name,
> + &candidates.items[i]->objectname, NULL,
> + FILTER_REFS_BRANCHES, DELETE_BRANCH_SKIP_UNMERGED))
This check is performed again when we call delete_branches() but we need
to do it here to prune the branch in order to stop delete_branches()
printing an error message. The check involves finding a merge-base so it
is not necessarily cheap - if that becomes a problem in the future we
can add a flag to delete_branches() to skip the check there.
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 3104c555f6..047ba54778 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1839,4 +1839,189 @@ test_expect_success '--forked narrows a <pattern> argument' '
> test_cmp expect actual
> '
>
> +test_expect_success '--delete-merged: setup' '
> + git init -b main upstream &&
> + (
> + cd upstream &&
> + test_commit base &&
> + git checkout -b next &&
> + test_commit next-work &&
> + git checkout main
> + ) &&
> + git init -b main other &&
> + test_commit -C other other-base &&
> + git init -b main fork
> +'
> +
> +setup_repo_for_delete_merged () {
> + rm -rf repo &&
> + git clone upstream repo &&
> + (
> + cd repo &&
> + git remote add fork ../fork &&
> + git remote add other ../other &&
> + git config remote.pushDefault fork &&
> + git config push.default current &&
> + git fetch other
> + )
> +}
> +
> +merged_branch () {
A comment would have helped to explain what this helper does. Also
calling it create_merged_branch() would be clearer too I think.
> + (
> + cd repo &&
> + git checkout -b "$1" "$2" &&
If we add '--track' we can avoid having to run "git branch
--set-upstream-to" below. The same goes for many if not all of the
branches created by "git checkout -b" and "git branch" in these tests.
> + git commit --allow-empty -m "$1 work" &&
> + git push origin "$1:next" &&
We let the caller specify the upstream branch, but then always push to
origin/next - should be be using 'git push ${2%%/*} "$1:${2#*/}"', or if
we don't need that flexibility hard coding the upstream branch?
> + git fetch origin &&
We've just pushed, what are we fetching here?
> + git branch --set-upstream-to="$2" "$1"
> + )
> +}
> +
> +test_expect_success '--delete-merged deletes merged branches and spares the rest' '
> + test_when_finished "rm -rf repo" &&
The first thing setup_repo_for_delete does is delete repo so do we need
this as well?
> + setup_repo_for_delete_merged &&
> + merged_branch merged origin/next &&
> + (
> + cd repo &&
> + git checkout -b unmerged origin/next &&
> + git commit --allow-empty -m "unmerged work" &&
good - we have a branch with upstream origin/next that isn't merged and
one that is.
> + git branch --set-upstream-to=origin/next unmerged &&
> + git checkout -b tracks-other other/main &&
> + git branch --set-upstream-to=other/main tracks-other &&
> + git checkout --detach
I assume this is to ensure we don't spare a branch because it is checked
out?
> + ) &&
> + sha=$(git -C repo rev-parse --short merged) &&
> +
> + git -C repo branch --delete-merged origin/next >actual 2>&1 &&
> +
> + echo "Deleted branch merged (was $sha)." >expect &&
There doesn't seem to be any reason for these command or the ones below
to be outside the subshell - they're all running commands in "repo".
That seems to be a common pattern in these tests.
> + test_cmp expect actual &&
This is good we have two branches with an upstream of origin/next, but
only one of them is merged. We also check no other branches are deleted.
> + git -C repo for-each-ref --format="%(refname:short)" refs/heads/ >actual &&
> + cat >expect <<-\EOF &&
> + main
> + tracks-other
> + unmerged
> + EOF
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--delete-merged deletes merged branches and spares protected ones' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo_for_delete_merged &&
> + merged_branch on-next origin/next &&
> + merged_branch checked-out origin/next &&
> + merged_branch upstream-gone origin/next &&
Right, we create three branches that are all merged into origin/next
> + (
> + cd repo &&
> + git checkout -b mainline main &&
> + git checkout -b on-local mainline &&
> + git branch --set-upstream-to=mainline on-local &&
Why do we need on-local to track mainline rather than main? I'm a bit
confused what the point of mainline is.
> + git update-ref refs/remotes/origin/topic refs/remotes/origin/next &&
> + git branch --set-upstream-to=origin/topic upstream-gone &&
> + git update-ref -d refs/remotes/origin/topic &&
These three lines can be replaced by
git config branch.gone.merge does-not-exist
> + git branch --set-upstream-to=origin/main main &&
> + git config branch.main.pushRemote origin &&
What does this do? Isn't its pushRemote already origin?
> + git checkout -b tracks-other other/main &&
> + git branch --set-upstream-to=other/main tracks-other &&
> + git checkout checked-out
> + ) &&
> +
> + git -C repo branch --delete-merged origin/next mainline &&
Do we want to use "origin/*" here instead so that we check that main is
not deleted because its push destination matches its upstream?
> +
> + git -C repo for-each-ref --format="%(refname:short)" refs/heads/ >actual &&
> + cat >expect <<-\EOF &&
> + checked-out
> + main
> + mainline
> + tracks-other
> + upstream-gone
> + EOF
> + test_cmp expect actual
This checks we delete on-local - good. I wonder if we should add a
comment about the expected outcome so it is clear to the casual reader
what is happening.
> +'
> +
> +test_expect_success '--delete-merged requires at least one <branch>' '
> + test_must_fail git -C forked branch --delete-merged 2>err &&
> + test_grep "requires at least one <branch>" err
> +'
> +
> +test_expect_success '--delete-merged keeps a branch that is an upstream' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo_for_delete_merged &&
> + merged_branch feature origin/next &&
> + (
> + cd repo &&
> + git checkout -b topic feature &&
> + git commit --allow-empty -m "topic work" &&
> + git branch --set-upstream-to=feature topic &&
> + git checkout --detach
> + ) &&
> +
> + git -C repo branch --dry-run --delete-merged origin/next >out &&
This belongs in a later patch and shows that the patches in this series
have not been individually tested (c.f. my previous mail about running
"git rebase --keep-base --exec")
> + test_grep ! "feature" out &&
> +
> + git -C repo branch --delete-merged origin/next 2>err &&
> +
> + test_must_be_empty err &&
So we don't delete anything because feature is needed by topic
> + git -C repo rev-parse --verify refs/heads/feature &&
> + git -C repo rev-parse --verify refs/heads/topic &&
I preferred the way this as checked in the previous tests with
for-each-ref and test_cmp as that shows everything that was kept.
> + echo origin/next >expect &&
> + git -C repo rev-parse --abbrev-ref feature@{upstream} >actual &&
> + test_cmp expect actual &&
> + echo feature >expect &&
> + git -C repo rev-parse --abbrev-ref topic@{upstream} >actual &&
> + test_cmp expect actual
This is a bit of a faff. Perhaps
git config --local --get-regexp
"branch.(feature|topic).(merge|remote)" >actual
followed by test_cmp would be more concise and more clearly show that
we're interested in checking that the config settings still exist.
> +'
> +
> +test_expect_success '--delete-merged keeps a chain of upstreams of a kept branch' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo_for_delete_merged &&
> + (
> + cd repo &&
> + git branch b3 origin/next &&
> + git branch --set-upstream-to=origin/next b3 &&
> + git branch b2 origin/next &&
> + git branch --set-upstream-to=b3 b2 &&
> + git checkout -b b1 b2 &&
> + git commit --allow-empty -m "b1 work" &&
> + git branch --set-upstream-to=b2 b1 &&
> + git checkout --detach
> + ) &&
I'd find this easier to follow if the base branch which is created
firest was numbered 1, rather than the tip of the stack.
> + git -C repo branch --delete-merged origin/next &&
b3 is merged but cannot be deleted because it is the upstream for b2
which although it is merged into b3 isn't a candidate for deletion
because its upstream is b3.
I'm not quite sure what this test demonstrates that the next one does not.
> +
> + git -C repo for-each-ref --format="%(refname:short)" refs/heads/ >actual &&
> + cat >expect <<-\EOF &&
> + b1
> + b2
> + b3
> + main
> + EOF
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--delete-merged clears the upstream of a kept base whose own base is deleted' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo_for_delete_merged &&
> + (
> + cd repo &&
> + git branch lower origin/next &&
> + git branch --set-upstream-to=origin/next lower &&
> + git branch mid origin/next &&
> + git branch --set-upstream-to=lower mid &&
> + git checkout -b tip mid &&
> + git commit --allow-empty -m "tip work" &&
> + git branch --set-upstream-to=mid tip &&
> + git checkout --detach
> + ) &&
> +
> + git -C repo branch --delete-merged origin/next lower &&
We expect lower to be deleted, but not mid because although it is merged
it is the upstream of an unmerged branch. Again it would be nice to
check that with for-each-ref (maybe that is a common enough pattern to
justify a helper that takes the expected output on stdin
check_branches <<-\EOF
main
mid
tip
EOF
> + test_must_fail git -C repo rev-parse --verify refs/heads/lower &&
> + git -C repo rev-parse --verify refs/heads/mid &&
> + test_must_fail git -C repo rev-parse mid@{upstream} &&
> + echo mid >expect &&
> + git -C repo rev-parse --abbrev-ref tip@{upstream} >actual &&
> + test_cmp expect actual
I'd check the config settings here as suggested for the test above. The
test coverage looks good, there are just a few places where a comment
would help explain what's going on and some places where we can save a
few commands.
Thanks
Phillip
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH v18 3/7] branch: let delete_branches skip unmerged branches on bulk refusal
From: Phillip Wood @ 2026-07-10 15:18 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Kristoffer Haugsbakk, Johannes Sixt, Harald Nordgren
In-Reply-To: <a0fd5b4a6c1b9d7823b431be703ee1696ea41f6c.1782338106.git.gitgitgadget@gmail.com>
Hi Harald
On 24/06/2026 22:55, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> @@ -235,6 +240,7 @@ static int delete_branches(int argc, const char **argv, int kinds,
> int remote_branch = 0;
> bool force;
> bool quiet = flags & DELETE_BRANCH_QUIET;
> + bool skip_unmerged = flags & DELETE_BRANCH_SKIP_UNMERGED;
The same as the last patch and for the next patch - as we're modifying
flags lets keep it as the single source of truth.
Thanks
Phillip
> struct strbuf bname = STRBUF_INIT;
> enum interpret_branch_kind allowed_interpret;
> struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
> @@ -319,7 +325,8 @@ static int delete_branches(int argc, const char **argv, int kinds,
> if (!(ref_flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
> check_branch_commit(bname.buf, name, &oid, head_rev, kinds,
> flags)) {
> - ret = 1;
> + if (!skip_unmerged)
> + ret = 1;
> goto next;
> }
>
^ 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