* Re: [PATCH 2/2] git-subtree: Bail out if we find output from Rust rewrite (test)
From: Colin Stagner @ 2026-07-09 1:59 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20260706115816.20267-3-ijackson@chiark.greenend.org.uk>
On 7/6/26 06:58, Ian Jackson wrote:
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -439,6 +439,24 @@ test_expect_success 'split sub dir/ with --rejoin' '
> )
> '
>
> +test_expect_success 'split fail on RIIR git subtree data' '
> + subtree_test_create_repo "$test_count" &&
> + subtree_test_create_repo "$test_count/sub proj" &&
It may be slightly faster to create only one repo and just make orphan
branches, like `test_create_subtree_add()` does.
> + echo "# sabotage" >.git-subtree/config &&
> + git add .git-subtree/config &&
> + git commit -m sabotage &&
`test_commit()` from test-lib-functions.sh may be superior to manually
writing and committing this file.
Colin
^ permalink raw reply
* Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite
From: Colin Stagner @ 2026-07-09 1:49 UTC (permalink / raw)
To: Ian Jackson, git; +Cc: Johannes Schindelin
In-Reply-To: <20260706115816.20267-2-ijackson@chiark.greenend.org.uk>
On 7/6/26 06:58, Ian Jackson wrote:
> Another, bigger, reason is that current git-subtree generates unmarked
> subtree merges (ie, without any git-subtree trailers)
Subtree merges can be performed without git-subtree, via the `-X
subtree` merge strategy option. While the design of RIIR git-subtree is
outside the scope of this patch series, this may be worth thinking about
in your rewrite.
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -278,6 +278,20 @@ main () {
> +reject_if_v2_config () {
> + local config=.git-subtree/config
This is a nit, but `local` is not specified by POSIX. I know it is used
elsewhere within git-subtree, but it is specifically discouraged.
> + if git rev-parse --verify -q "$rev:$config"; then
For subtree split, should we also test for this file in tree you are
splitting: i.e., "$dir/$config"? The answer might be no.
I think that subtree merge should only test the top-level project, as
this patch does now.
Colin
^ permalink raw reply
* [PATCH v3 11/11] builtin/receive-pack: stage incoming objects via ODB transactions
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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 v3 08/11] odb/transaction: add transaction env interface
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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 | 14 ++++++++++++++
odb/transaction.c | 8 ++++++++
odb/transaction.h | 17 +++++++++++++++++
3 files changed, 39 insertions(+)
diff --git a/object-file.c b/object-file.c
index 358684beae..f0b066798a 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,18 @@ 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);
+
+ odb_transaction_files_prepare(&transaction->base);
+ strvec_pushv(env, tmp_objdir_env(transaction->objdir));
+
+ return 0;
+}
+
int odb_transaction_files_begin(struct odb_source *source,
struct odb_transaction **out)
{
@@ -1696,6 +1709,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 0a924e73f7..7f1b30945d 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -42,3 +42,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 v3 10/11] builtin/receive-pack: drop redundant tmpdir env
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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 v3 09/11] odb/transaction: introduce ODB transaction flags
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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 f0b066798a..d2508d148f 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),
@@ -1701,7 +1702,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;
@@ -1710,6 +1712,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 7f1b30945d..edf5488c81 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);
odb->transaction = *out;
return ret;
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 v3 07/11] odb/transaction: propagate commit errors
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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 a5fba7f908..0a924e73f7 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -17,19 +17,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 v3 06/11] odb/transaction: propagate begin errors
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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 | 13 +++++++++----
odb/transaction.h | 19 +++++++++++++++----
read-cache.c | 7 +++++--
8 files changed, 44 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..a5fba7f908 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -1,15 +1,20 @@
#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);
+ 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 v3 05/11] object-file: propagate files transaction errors
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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
* [PATCH v3 04/11] object-file: drop check for inflight transactions
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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 v3 03/11] object-file: embed transaction flush logic in commit function
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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 directly 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 v3 02/11] object-file: rename files transaction fsync function
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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
* [PATCH v3 01/11] object-file: rename files transaction prepare function
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708235925.3992097-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 v3 00/11] receive-pack: use ODB transactions to stage object writes
From: Justin Tobler @ 2026-07-08 23:59 UTC (permalink / raw)
To: git; +Cc: ps, gitster, Justin Tobler
In-Reply-To: <20260708041412.1157499-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 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 | 159 +++++++++++++++++++++++++--------------
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 | 32 ++++++--
odb/transaction.h | 59 ++++++++++++---
read-cache.c | 7 +-
14 files changed, 241 insertions(+), 130 deletions(-)
Range-diff against v2:
1: 9c14b219ad = 1: 9c14b219ad object-file: rename files transaction prepare function
2: 5703a9e93b = 2: 5703a9e93b object-file: rename files transaction fsync function
3: 4c37398ac8 ! 3: 76204847f2 object-file: embed transaction flush logic in commit function
@@ Commit message
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. In a subsequent commit, the temporary directory is also
- used to write packfiles.
+ 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.
- Instead of maintaining a separate helper function, embed the logic to
- flush and migrate the temporary directory directly into
- `odb_transaction_files_commit()`.
+ Lift the helper function logic directly into
+ `odb_transaction_files_commit()` to more accurately signal to readers
+ the operation being performed.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
4: 623c6b02ea = 4: c97eb7763f object-file: drop check for inflight transactions
5: ca59176657 ! 5: 1f3a1f7714 object-file: propagate files transaction errors
@@ object-file.c: static void odb_transaction_files_prepare(struct odb_transaction
- if (transaction->objdir)
- tmp_objdir_replace_primary_odb(transaction->objdir, 0);
+ if (!transaction->objdir)
-+ return -1;
++ return error(_("unable to create temporary object directory"));
+
+ tmp_objdir_replace_primary_odb(transaction->objdir, 0);
+
@@ object-file.c: static void odb_transaction_files_commit(struct odb_transaction *
*/
- tmp_objdir_migrate(transaction->objdir);
+ if (tmp_objdir_migrate(transaction->objdir))
-+ return -1;
++ return error(_("unable to migrate temporary objects"));
+
transaction->objdir = NULL;
}
6: 717a1ce9a7 ! 6: 09d13272d5 odb/transaction: propagate begin errors
@@ object-file.c: int index_fd(struct index_state *istate, struct object_id *oid,
## odb/transaction.c ##
@@
+ #include "git-compat-util.h"
++#include "gettext.h"
#include "odb/source.h"
#include "odb/transaction.h"
@@ odb/transaction.c
+
if (odb->transaction)
- return NULL;
-+ return -1;
++ return error(_("object database transaction already pending"));
- odb_source_begin_transaction(odb->sources, &odb->transaction);
+ ret = odb_source_begin_transaction(odb->sources, out);
@@ odb/transaction.h
#ifndef ODB_TRANSACTION_H
#define ODB_TRANSACTION_H
-+#include "git-compat-util.h"
+#include "gettext.h"
#include "odb.h"
#include "odb/source.h"
7: ff8e133965 ! 7: 12833d6773 odb/transaction: propagate commit errors
@@ odb/transaction.c: int odb_transaction_begin(struct object_database *odb,
- transaction->commit(transaction);
+ ret = transaction->commit(transaction);
-+ if (ret)
-+ return ret;
-+
transaction->source->odb->transaction = NULL;
free(transaction);
+
-+ return 0;
++ return ret;
}
int odb_transaction_write_object_stream(struct odb_transaction *transaction,
8: 264ba94b83 = 8: f2586f2f34 odb/transaction: add transaction env interface
9: 1e0a491ef2 ! 9: 9d082b5e47 odb/transaction: introduce ODB transaction flags
@@ object-file.c: static int odb_transaction_files_prepare(struct odb_transaction *
- 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 -1;
+ return error(_("unable to create temporary object directory"));
@@ object-file.c: int index_fd(struct index_state *istate, struct object_id *oid,
int inflight = !!transaction;
@@ odb/transaction.c
int ret;
if (odb->transaction)
- return -1;
+ return error(_("object database transaction already pending"));
- ret = odb_source_begin_transaction(odb->sources, out);
+ ret = odb_source_begin_transaction(odb->sources, out, flags);
@@ odb/transaction.c
## odb/transaction.h ##
@@
- #include "git-compat-util.h"
+
#include "gettext.h"
#include "odb.h"
-#include "odb/source.h"
10: 6c8d878349 = 10: e11d8a6676 builtin/receive-pack: drop redundant tmpdir env
11: 8db95fef56 = 11: fee57c2817 builtin/receive-pack: stage incoming objects via ODB transactions
base-commit: ab776a62a78576513ee121424adb19597fbb7613
--
2.55.0.122.gf85a7e6620
^ permalink raw reply
* Re: [PATCH v2] unpack-trees: avoid quadratic index scan in next_cache_entry()
From: Junio C Hamano @ 2026-07-08 21:58 UTC (permalink / raw)
To: Henrique Ferreiro via GitGitGadget; +Cc: git, Henrique Ferreiro
In-Reply-To: <pull.2353.v2.git.git.1783546933992.gitgitgadget@gmail.com>
"Henrique Ferreiro via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Henrique Ferreiro <hferreiro@igalia.com>
>
> Diffing the working tree against a commit with a pathspec can take
> time quadratic in the size of the index when the pathspec matches a
> subtree whose entries are the first entries of the index. Fix it by
> having next_cache_entry() record how far it scanned in cache_bottom,
> so repeated calls no longer rescan the growing prefix of
> already-unpacked entries. On a Chromium checkout (~500k index
> entries),
>
> git diff HEAD -- .agents/OWNERS
>
> took about 8 minutes before this change and 0.07 seconds after it.
> The same diff without the commit, without the pathspec, or with
> --cached was already instant.
>
> Add p0009-diff-pathspec.sh, which builds a 10,000-entry index whose
> first path lives in a subtree (100,000 entries under --long-tests),
> to guard against the regression. Comparing v2.55.0 with this change
> using GIT_TEST_LONG=t:
>
> Test v2.55.0 HEAD
> ------------------------------------------------------------------------
> 0009.2: diff pathspec subtree 7.16(7.12+0.01) 0.02(0.01+0.00) -99.7%
>
> Signed-off-by: Henrique Ferreiro <hferreiro@igalia.com>
> ---
> unpack-trees: avoid quadratic index scan in next_cache_entry()
>
> Changes since v1: adjust the synthetic index size based on the EXPENSIVE
> prerequisite.
Thanks.
^ permalink raw reply
* [PATCH v2] unpack-trees: avoid quadratic index scan in next_cache_entry()
From: Henrique Ferreiro via GitGitGadget @ 2026-07-08 21:42 UTC (permalink / raw)
To: git; +Cc: Henrique Ferreiro, Henrique Ferreiro, Henrique Ferreiro
In-Reply-To: <pull.2353.git.git.1783458106037.gitgitgadget@gmail.com>
From: Henrique Ferreiro <hferreiro@igalia.com>
Diffing the working tree against a commit with a pathspec can take
time quadratic in the size of the index when the pathspec matches a
subtree whose entries are the first entries of the index. Fix it by
having next_cache_entry() record how far it scanned in cache_bottom,
so repeated calls no longer rescan the growing prefix of
already-unpacked entries. On a Chromium checkout (~500k index
entries),
git diff HEAD -- .agents/OWNERS
took about 8 minutes before this change and 0.07 seconds after it.
The same diff without the commit, without the pathspec, or with
--cached was already instant.
Add p0009-diff-pathspec.sh, which builds a 10,000-entry index whose
first path lives in a subtree (100,000 entries under --long-tests),
to guard against the regression. Comparing v2.55.0 with this change
using GIT_TEST_LONG=t:
Test v2.55.0 HEAD
------------------------------------------------------------------------
0009.2: diff pathspec subtree 7.16(7.12+0.01) 0.02(0.01+0.00) -99.7%
Signed-off-by: Henrique Ferreiro <hferreiro@igalia.com>
---
unpack-trees: avoid quadratic index scan in next_cache_entry()
Changes since v1: adjust the synthetic index size based on the EXPENSIVE
prerequisite.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2353%2Fhferreiro%2Funpack-trees-quadratic-scan-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2353/hferreiro/unpack-trees-quadratic-scan-v2
Pull-Request: https://github.com/git/git/pull/2353
Range-diff vs v1:
1: cc1aaf01cf ! 1: c8f1ca389d unpack-trees: avoid quadratic index scan in next_cache_entry()
@@ Commit message
The same diff without the commit, without the pathspec, or with
--cached was already instant.
- Add p0009-diff-pathspec.sh, which builds a 100,000-entry index whose
- first path lives in a subtree, to guard against the regression.
- Comparing v2.55.0 with this change:
+ Add p0009-diff-pathspec.sh, which builds a 10,000-entry index whose
+ first path lives in a subtree (100,000 entries under --long-tests),
+ to guard against the regression. Comparing v2.55.0 with this change
+ using GIT_TEST_LONG=t:
Test v2.55.0 HEAD
------------------------------------------------------------------------
@@ t/perf/p0009-diff-pathspec.sh (new)
+
+test_perf_fresh_repo
+
++count=10000
++if test_have_prereq EXPENSIVE
++then
++ count=100000
++fi
++
+# The entries exist only in the index, which is enough to
+# exercise the index scan.
+test_expect_success 'setup' '
-+ count=100000 &&
+ blob=$(echo content | git hash-object -w --stdin) &&
+ {
+ printf "100644 $blob\taaa/file\n" &&
t/perf/p0009-diff-pathspec.sh | 32 ++++++++++++++++++++++++++++++++
unpack-trees.c | 4 +++-
2 files changed, 35 insertions(+), 1 deletion(-)
create mode 100755 t/perf/p0009-diff-pathspec.sh
diff --git a/t/perf/p0009-diff-pathspec.sh b/t/perf/p0009-diff-pathspec.sh
new file mode 100755
index 0000000000..6079db52c2
--- /dev/null
+++ b/t/perf/p0009-diff-pathspec.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='Tests performance of diffing the working tree with a pathspec'
+
+. ./perf-lib.sh
+
+test_perf_fresh_repo
+
+count=10000
+if test_have_prereq EXPENSIVE
+then
+ count=100000
+fi
+
+# The entries exist only in the index, which is enough to
+# exercise the index scan.
+test_expect_success 'setup' '
+ blob=$(echo content | git hash-object -w --stdin) &&
+ {
+ printf "100644 $blob\taaa/file\n" &&
+ printf "100644 $blob\tf%s\n" $(test_seq $count)
+ } | git update-index --index-info &&
+ git commit -q -m initial &&
+ mkdir -p aaa &&
+ echo content >aaa/file
+'
+
+test_perf 'diff pathspec subtree' '
+ git diff HEAD -- aaa/file
+'
+
+test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index b42020f16b..ed9fef453a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -671,8 +671,10 @@ static struct cache_entry *next_cache_entry(struct unpack_trees_options *o)
while (pos < index->cache_nr) {
struct cache_entry *ce = index->cache[pos];
- if (!(ce->ce_flags & CE_UNPACKED))
+ if (!(ce->ce_flags & CE_UNPACKED)) {
+ o->internal.cache_bottom = pos;
return ce;
+ }
pos++;
}
return NULL;
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] unpack-trees: avoid quadratic index scan in next_cache_entry()
From: Henrique Ferreiro @ 2026-07-08 21:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Henrique Ferreiro via GitGitGadget, git
In-Reply-To: <xmqqzf01thq0.fsf@gitster.g>
On 08/07/2026 21:16, Junio C Hamano wrote:
> Henrique Ferreiro <hferreiro@igalia.com> writes:
>
>> On 07/07/2026 23:30, Junio C Hamano wrote:
>>> "Henrique Ferreiro via GitGitGadget" <gitgitgadget@gmail.com>
>>> writes:
>>>
>>>> diff --git a/unpack-trees.c b/unpack-trees.c
>>>> index b42020f16b..ed9fef453a 100644
>>>> --- a/unpack-trees.c
>>>> +++ b/unpack-trees.c
>>>> @@ -671,8 +671,10 @@ static struct cache_entry *next_cache_entry(struct unpack_trees_options *o)
>>>>
>>>> while (pos < index->cache_nr) {
>>>> struct cache_entry *ce = index->cache[pos];
>>>> - if (!(ce->ce_flags & CE_UNPACKED))
>>>> + if (!(ce->ce_flags & CE_UNPACKED)) {
>>>> + o->internal.cache_bottom = pos;
>>>> return ce;
>>>> + }
>>>> pos++;
>>> Nice spotting.
>>>
>>> Does this trick work correctly even when a path's sorting order
>>> differs between the index and tree objects, which is precisely why
>>> .cache_bottom was introduced, to allow backward scanning while
>>> bounding the lookback distance?
>> IIUC, .cache_bottom points at the first entry that needs to be
>> processed. With this change, that still holds true even when entries are
>> processed out of index order. find_cache_pos() also advances
>> cache_bottom past unpacked entries since e53e6b4433 (unpack-trees: Make
>> index lookahead less pessimal, 2010-06-10).
> That sounds sensible.
>
>>>> diff --git a/t/perf/p0009-diff-pathspec.sh b/t/perf/p0009-diff-pathspec.sh
>>>> new file mode 100755
>>>> index 0000000000..0f1dccfbb4
>>>> --- /dev/null
>>>> +++ b/t/perf/p0009-diff-pathspec.sh
>>>> @@ -0,0 +1,27 @@
>>>> +#!/bin/sh
>>>> +
>>>> +test_description='Tests performance of diffing the working tree with a pathspec'
>>>> +
>>>> +. ./perf-lib.sh
>>>> +
>>>> +test_perf_fresh_repo
>>>> +
>>>> +# The entries exist only in the index, which is enough to
>>>> +# exercise the index scan.
>>>> +test_expect_success 'setup' '
>>>> + count=100000 &&
>>> You will probably want to mimic how t/perf/p4209-pickaxe.sh helps
>>> testers by adjusting the count based on how the EXPENSIVE
>>> prerequisite is configured.
> I think this comment still needs addressing, though.
>
> Thanks.
Sure. I've just sent v2 with those changes. Note that I used an initial
count of 1000, otherwise the improvement is not noticeable.
>>>> + blob=$(echo content | git hash-object -w --stdin) &&
>>>> + {
>>>> + printf "100644 $blob\taaa/file\n" &&
>>>> + printf "100644 $blob\tf%s\n" $(test_seq $count)
>>>> + } | git update-index --index-info &&
>>>> + git commit -q -m initial &&
>>>> + mkdir -p aaa &&
>>>> + echo content >aaa/file
>>>> +'
^ permalink raw reply
* Re: [PATCH v2 1/2] bundle-uri: drain remaining response on invalid bundle-uri lines
From: Junio C Hamano @ 2026-07-08 21:13 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Justin Tobler
In-Reply-To: <20260708-toon-bundle-uri-no-uri-v2-1-09a03d8db556@iotcl.com>
Toon Claes <toon@iotcl.com> writes:
> With this, clone now continues successfully if invalid bundle-URI data
> was sent by the server. This is intentional, because since the inception
> of `transport_get_remote_bundle_uri()` in 0cfde740f0 (clone: request the
> 'bundle-uri' command when available, 2022-12-22) the return value of
> that function is ignored in `cmd_clone()` so the clone can continue
> without bundles.
I am on the fence.
Alternatively, we could terminate the connection immediately, given
that we are clearly dealing with a broken server.
It is one thing to successfully parse the server's response (e.g.,
'fetch the bundle from this address') but fail to follow its
direction because, for example, the resource is unreachable. Since
bundles are optional, ignoring the failure and continuing makes
complete sense.
But it feels different when we can't even parse what the server is
saying.
While a malformed bundle-URI payload is benign enough to ignore
today, future protocol extensions might introduce mandatory
data. Eventually, we will need a robust way to tell ignorable and
fatal errors apart so we can react appropriately. That
classification can wait for a future topic, however.
The patch looks good and matches what you designed well.
Thanks.
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
> connect.c | 15 ++++++++++++---
> t/t5558-clone-bundle-uri.sh | 29 +++++++++++++++++++++++++++++
> 2 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index 47e39d2a73..1d74c1eda2 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -517,7 +517,7 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)
> int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
> struct bundle_list *bundles, int stateless_rpc)
> {
> - int line_nr = 1;
> + int line_nr = 1, err = 0;
>
> /* Assert bundle-uri support */
> ensure_server_supports_v2("bundle-uri");
> @@ -536,10 +536,19 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
> const char *line = reader->line;
> line_nr++;
>
> + /*
> + * Do not parse if an error was encountered, but
> + * continue draining the response so no stale data
> + * is left in the reader for subsequent protocol
> + * exchanges.
> + */
> + if (err)
> + continue;
> +
> if (!bundle_uri_parse_line(bundles, line))
> continue;
>
> - return error(_("error on bundle-uri response line %d: %s"),
> + err = error(_("error on bundle-uri response line %d: %s"),
> line_nr, line);
> }
>
> @@ -554,7 +563,7 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
> check_stateless_delimiter(stateless_rpc, reader,
> _("expected response end packet after ref listing"));
>
> - return 0;
> + return err;
> }
>
> struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> index 7a0943bd36..7cc8627e17 100755
> --- a/t/t5558-clone-bundle-uri.sh
> +++ b/t/t5558-clone-bundle-uri.sh
> @@ -1302,6 +1302,35 @@ test_expect_success 'bundles with newline in target path are rejected' '
> test_path_is_missing escape
> '
>
> +test_expect_success 'bundles advertised with missing URI' '
> + git clone --no-local --mirror clone-from \
> + "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config uploadpack.advertiseBundleURIs true &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.version 1 &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.mode all &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.bundle-1.creationToken 1 &&
> +
> + git -c transfer.bundleURI=true clone \
> + "$HTTPD_URL/smart/no-uri.git" target-no-uri 2>err &&
> + test_grep "bundle ${SQ}bundle-1${SQ} has no uri" err &&
> + test_grep ! "expected packfile" err
> +'
> +
> +test_expect_success 'bundles advertised with empty URI' '
> + git clone --no-local --mirror clone-from \
> + "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config uploadpack.advertiseBundleURIs true &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.version 1 &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.mode all &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.bundle-1.uri "" &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.bundle-1.creationToken 1 &&
> +
> + git -c transfer.bundleURI=true clone \
> + "$HTTPD_URL/smart/empty-uri.git" target-empty-uri 2>err &&
> + test_grep "bundle ${SQ}bundle-1${SQ} has no uri" err &&
> + test_grep ! "expected packfile" err
> +'
> +
> # Do not add tests here unless they use the HTTP server, as they will
> # not run unless the HTTP dependencies exist.
^ permalink raw reply
* Re: [PATCH v3] t1410-reflog.sh: avoid suppressing git's exit code in pipelines
From: Junio C Hamano @ 2026-07-08 20:41 UTC (permalink / raw)
To: Gatla Vishweshwar Reddy; +Cc: git
In-Reply-To: <20260708092448.35776-1-gatlavishweshwarreddy26@gmail.com>
Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:
> Piping git commands directly to wc -l suppresses the exit code of
> git, hiding potential failures from the test suite.
Correct.
> Capture the
> output to a temporary file first, then count the lines separately
> to preserve the exit code. Where the expected count is known ahead
> of time, use test_stdout_line_count instead.
Technically, the description is not telling any lies about the
solution, but the patch no longer does the caputuring or counting
itself at all. Rather, it exclusively uses test_stdout_line_count,
which internally does the saving to a temporary and counting the
lines ;-)
The changes in the patch are mostly good, except for the loss of a
blank line that separates two test pieces in the last hunk (below).
> @@ -319,13 +311,12 @@ test_expect_success 'git reflog expire unknown reference' '
> test_must_fail git reflog expire does-not-exist 2>stderr &&
> test_grep "error: reflog could not be found: ${SQ}does-not-exist${SQ}" stderr
> '
> -
> test_expect_success 'checkout should not delete log for packed ref' '
> - test $(git reflog main | wc -l) = 4 &&
> + test_stdout_line_count = 4 git reflog main &&
> git branch foo &&
> git pack-refs --all &&
> git checkout foo &&
> - test $(git reflog main | wc -l) = 4
> + test_stdout_line_count = 4 git reflog main
> '
>
> test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' '
Thanks.
^ permalink raw reply
* Re: [PATCH v3 0/4] parseopt: exit 0 on help
From: Junio C Hamano @ 2026-07-08 20:32 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, git
In-Reply-To: <20260708035930.GB41684@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jul 08, 2026 at 12:15:53AM +0000, brian m. carlson wrote:
>
>> Changes since v2:
>>
>> * Fix inverted condition in t1517.
>> * Stop checking for old versions of SVN Perl libraries since they are
>> so old nobody is using them.
>> * Adjust the various cases where we choose between the error and
>> non-error help output.
>
> Thanks, I have no complaints on this version.
>
> In the earlier thread I sketched out a hypothetical caller that might be
> affected by the change, but beyond raw speculation, I don't think we
> have any way of knowing how common such a thing is. So I'm inclined to
> proceed and see if anybody screams during this development cycle.
;-) 100% agreed on the issue of possible regressions.
^ permalink raw reply
* Re: [PATCH v8 0/3] Makefile: link osxkeychain helper against Rust
From: Junio C Hamano @ 2026-07-08 20:16 UTC (permalink / raw)
To: Shardul Natu via GitGitGadget
Cc: git, Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble
In-Reply-To: <pull.2288.v8.git.git.1783480879.gitgitgadget@gmail.com>
"Shardul Natu via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since v7:
>
> * Added inclusion of ../config.mak.uname to the top of contrib/Makefile in
> the canonical order. This guarantees that $(uname_S) is correctly defined
> on the shell, preventing the OS_CONTRIB additions from being silently
> ignored.
This round of patches looked good to me (even though I am not a
macOS user, so my review only goes on the surface without actual
testing).
^ permalink raw reply
* Re: [PATCH] submodule--helper: accept '-i' shorthand for update --init
From: Junio C Hamano @ 2026-07-08 20:12 UTC (permalink / raw)
To: Dominique Martinet
Cc: Ævar Arnfjörð Bjarmason, Glen Choo, Atharva Raykar,
Roy Eldar, git
In-Reply-To: <20260708-submodule-init-v1-1-719456077262@atmark-techno.com>
Dominique Martinet <dominique.martinet@atmark-techno.com> writes:
> commit 3ad0ba722744 ("git-submodule.sh: improve variables readability")
> made `git submodules update -i` pass `-i` as is to submodule--helper,
> but it fails with `error: unknown switch `i'` because the helper does
> not accept the short option.
>
> All other short options supported by git-submodule.sh are properly
> handle in the helper, so also add the alias for --init
>
> Fixes: 3ad0ba722744 ("git-submodule.sh: improve variables readability")
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> ---
> This apparently can't be used much because it's been broken since v2.49,
> and it's not in the help, but I guess we might as well fix this since it
> used to work?
3ad0ba7227 (git-submodule.sh: improve variables readability,
2024-12-11) is v2.49.0-rc0~107^2~1 and indeed we broke it and nobody
has noticed and reported since then. An obvious alternative would
be to give a single-liner:
diff --git c/git-submodule.sh w/git-submodule.sh
index 2999b31fad..9a989edb20 100755
--- c/git-submodule.sh
+++ w/git-submodule.sh
@@ -285,7 +285,7 @@ cmd_update()
progress=$1
;;
-i|--init)
- init=$1
+ init=--init
;;
--require-init)
require_init=$1
but since anybody should be directly running "git submodule--helper"
by hand, I think it is perfectly fine to teach it a short-option
like your patch does.
Will apply. Thanks.
^ permalink raw reply related
* Re: [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic
From: Junio C Hamano @ 2026-07-08 20:02 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget; +Cc: git, Michael Montalbo
In-Reply-To: <efd34c17157b3183cdc851c8b17e7967b6c85506.1783479584.git.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> -# Check if this is the first call (no state file exists)
> -if test -f "$state_file"
> +# Apache can run this CGI for concurrent requests, so the script decides
> +# whether this is the first call with a single atomic "mkdir": it succeeds for
> +# exactly one of any racing requests and fails for the rest. "permanent"
> +# always rate-limits and records no state.
> +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null
I think the last sentence in the above comment was meant to explain
why the new code checks the value of "$retry_after", but it is not
clear if it is needed for correctness (in other words, the original
was wrong to do "test -f && touch" but also was wrong to do so even
when "$retry_after" is set to "permanent), or if it is a mere
"optimization opportunity" you are taking advantage of. In either
case, it would be nice to see it explained in the proposed commit
log message.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] t/README: document writing concurrency-safe helpers
From: Junio C Hamano @ 2026-07-08 19:59 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget; +Cc: git, Michael Montalbo
In-Reply-To: <771d264d2999a780e0c93e64bb4451a05214ab75.1783479584.git.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Michael Montalbo <mmontalbo@gmail.com>
>
> The apply-one-time-script.sh and http-429.sh fixes addressed the same
> underlying problem: a test helper assuming it has exclusive access to a
> file when the web server can run it for several requests at once. The
> atomic idioms that avoid this are not specific to CGI or to HTTP, so
> document them generally, alongside the other guidance for writing tests,
> and leave a pointer from the lib-httpd helper list rather than a local
> comment. The note covers the anti-pattern (a "test -f" then a separate
> act) and the two safe operations (mkdir to elect a winner, rename to
> consume a one-shot marker), citing Git's own lockfile machinery and
> make_symlink() as precedent.
>
> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
> ---
> t/README | 32 ++++++++++++++++++++++++++++++++
> t/lib-httpd.sh | 3 +++
> 2 files changed, 35 insertions(+)
Thanks for a nice finishing touch.
> diff --git a/t/README b/t/README
> index 085921be4b..a9d425f392 100644
> --- a/t/README
> +++ b/t/README
> @@ -854,6 +854,38 @@ from the test harness library. At the end of the script, call
> 'test_done'.
>
>
> +Writing concurrency-safe helpers
> +--------------------------------
> +
> +Some test code runs concurrently: a test may background work with '&',
> +and the helper scripts installed for the web server (in t/lib-httpd) are
> +run once per request, so the same script can execute for several
> +requests at once. Such code cannot assume it has exclusive access to a
> +file.
> +
> +When exactly one of several concurrent processes needs to "win" a
> +decision, a single atomic filesystem operation can make it, rather than
> +a check followed by a separate action. A "test -f X" then "touch X"
> +(or "rm X") races: two processes can both pass the check before either
> +acts. Two atomic operations avoid this:
> +
> + - "mkdir dir", which fails if the directory already exists, so that
> + exactly one caller wins, electing a first or only request (see
> + t/lib-httpd/http-429.sh).
> +
> + - "mv src dst" (rename), which fails if the source is gone, so that
> + exactly one caller consumes it, claiming a planted one-shot marker
> + (see t/lib-httpd/apply-one-time-script.sh).
> +
> +A "$$" suffix on per-request scratch files keeps concurrent invocations
> +from clobbering each other's fixed-name files.
> +
> +This is a standard shell locking idiom, and the same reasoning behind
> +Git's own lockfile machinery, which creates its lock with O_CREAT|O_EXCL,
> +and make_symlink() in t/test-lib.sh, which uses an mkdir lock: an atomic
> +operation whose failure indicates that another process got there first.
> +
> +
> Test harness library
> --------------------
>
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index fc646447d5..d64f9c8c2d 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -159,6 +159,9 @@ prepare_httpd() {
> mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
> cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
> cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
> + # The web server can run any of these CGI scripts for two requests at
> + # once; a helper that keeps state between requests must do so with an
> + # atomic operation. See "Writing concurrency-safe helpers" in t/README.
> install_script incomplete-length-upload-pack-v2-http.sh
> install_script incomplete-body-upload-pack-v2-http.sh
> install_script error-no-report.sh
^ permalink raw reply
* Re: [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic
From: Junio C Hamano @ 2026-07-08 19:58 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget; +Cc: git, Michael Montalbo
In-Reply-To: <efd34c17157b3183cdc851c8b17e7967b6c85506.1783479584.git.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Michael Montalbo <mmontalbo@gmail.com>
>
> http-429.sh records "already returned 429 once" with a "test -f"
> followed by a "touch" of a shared state file. That check-then-act is not
> atomic: Apache can run this CGI for several requests at once, and two of
> them can both pass the "test -f" before either "touch"es, so both treat
> themselves as the first request. The retry flow that drives this
> endpoint is mostly sequential, so this has not been seen to fail, but
> the race is latent.
OK. And use of mkdir for atomicity is an obvious solution for such
a situtation.
> -if test -f "$state_file"
> +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null
> then
> # Already returned 429 once, forward to git-http-backend
> # Set PATH_INFO to just the repo path (without retry-after value)
> @@ -52,9 +55,6 @@ then
> exec "$GIT_EXEC_PATH/git-http-backend"
> fi
>
> -# Mark that we've returned 429
> -touch "$state_file"
> -
^ 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