* [PATCH] sequencer: honor --empty when a fixup!/squash! empties its target
From: Farid Zakaria @ 2026-07-10 4:13 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Elijah Newren, Patrick Steinhardt, Junio C Hamano,
Farid Zakaria
When "git rebase --autosquash" melds a "fixup!" or "squash!" commit into
its target, the result can be a commit that no longer changes anything
relative to its parent, for example when the melded change reverts the
target. Rather than dropping or keeping this empty commit, the rebase
stops with
You asked to amend the most recent commit, but doing so would
make it empty. ...
and the "--empty" option has no effect on it. This makes backing a
change out of a series awkward: reverting a commit as a "fixup!" and
running "git rebase --autosquash --empty=drop" ought to remove both the
commit and its revert, but it halts instead.
The reason is that allow_empty() decides emptiness with
is_index_unchanged(), which compares the index to HEAD. A "fixup!" is
applied by amending HEAD, so the commit it produces has HEAD's parent as
its parent; it is empty when the index matches the tree of that parent,
not of HEAD. A meld that cancels out its target is therefore never
recognized as having become empty, and falls through to "git commit
--amend", which refuses to create an empty commit.
Teach is_index_unchanged() to compare against the tree of HEAD's parent
when amending, and teach allow_empty() to classify the result as "became
empty" (and thus subject to --empty) unless the commit being melded into
was itself already empty, in which case it "started empty" and is
governed by allow_empty as before.
When --empty=drop applies, the emptied commit has already been created
by the preceding "pick", so drop it by moving HEAD back to its parent.
Do so before the rewritten-commit list is flushed, so that --update-refs
and the other rewrite consumers map the dropped commit to its parent.
Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
---
At Meta we maintain a fork of LLVM that we regularly rebase onto
upstream. A set of internal patches rides on top, and we keep each one
as a single commit by folding follow-up changes into it with autosquash
"fixup!" commits. That works well for evolving a patch, but not for
retiring one: to back an internal patch out today we delete it from the
history by hand with an interactive rebase and then force-push, which is
easy to get wrong on a shared branch.
It would be nicer to retire a patch the same way we amend one: commit a
revert of it as a "fixup!" and let autosquash fold the two together.
The net change is empty, so the commit should just drop out of the
series. Today it does not -- the rebase stops instead.
For example, starting from a commit we want to retire:
$ git log --oneline
4d5e6f7 add feature patch
9a1b2c3 base
# revert the feature and mark the revert as a fixup of it
$ git revert --no-edit HEAD
$ git commit --amend -m "fixup! add feature patch"
$ git rebase -i --autosquash --empty=drop 9a1b2c3
Rebasing (2/2)
You asked to amend the most recent commit, but doing so would
make it empty. You can repeat your command with --allow-empty [...]
Could not apply 8e9f0a1... # fixup! add feature patch
The "--empty=drop" is ignored. "--empty" only governs commits that are
picked empty, whereas a "fixup!" is applied by amending, and the
emptiness of an amended commit is measured against the wrong parent. So
the rebase falls through to "git commit --amend", which refuses to
create an empty commit, and halts.
With this patch the emptied commit is recognized and handled according
to "--empty", the same as any other commit that becomes empty during a
rebase:
$ git rebase -i --autosquash --empty=drop 9a1b2c3
Rebasing (2/2)
dropping 8e9f0a1... fixup! add feature patch -- resulting commit is empty
Successfully rebased and updated refs/heads/main.
$ git log --oneline
9a1b2c3 base
"--empty=keep" retains it as an empty commit, and "--empty=stop" (the
default under "-i") halts so the user can decide -- matching how these
options already behave for commits that become empty when picked.
One open question, for a possible follow-up. A natural next step would
be a "revert!" autosquash directive (and a "git commit --revert" to
create it), mirroring "fixup!"/"squash!", so
that retiring a patch would not require generating the reverse diff by
hand. I have deliberately left it out of this series, because its
semantics are not obvious: in particular, whether a "revert!" commit
should carry the reverse patch as its own content (and thus be an
ordinary fixup that this patch already drops), or be an empty marker
that instructs the rebase to revert the target commit during the meld.
Opinions on whether such a directive is wanted, and which of those two
shapes is preferred, would be welcome before I attempt it.
---
base-commit: f60db8d575adb79761d363e026fb49bddf330c73
---
Documentation/git-rebase.adoc | 12 ++++++
sequencer.c | 96 +++++++++++++++++++++++++++++++++++++++----
t/t3415-rebase-autosquash.sh | 64 +++++++++++++++++++++++++++++
3 files changed, 163 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
index f6c22d1598..7eb8bbe95f 100644
--- a/Documentation/git-rebase.adoc
+++ b/Documentation/git-rebase.adoc
@@ -282,6 +282,11 @@ by `git log --cherry-mark ...`) are detected and dropped as a
preliminary step (unless `--reapply-cherry-picks` or `--keep-base` is
passed).
+
+A commit can also become empty as a result of `--autosquash`, when a
+`fixup!` or `squash!` commit cancels out all of the changes of the
+commit it is melded into. Such a commit is treated the same way and is
+dropped, kept, or stopped at according to this option.
++
See also INCOMPATIBLE OPTIONS below.
--no-keep-empty::
@@ -591,6 +596,13 @@ changed from `pick` to `squash`, `fixup` or `fixup -C`, respectively, and they
are moved right after the commit they modify. The `--interactive` option can
be used to review and edit the todo list before proceeding.
+
+If melding a `fixup!` or `squash!` commit cancels out all of the changes of
+the commit it is applied to, the result is an empty commit. The handling of
+these empty commits can be configured with the `--empty` option: the emptied
+commit is dropped, kept, or stopped at. This makes it possible to back a
+change out of a series by committing a revert of it as a `fixup!` and letting
+`--autosquash --empty=drop` remove both.
++
The recommended way to create commits with squash markers is by using the
`--squash`, `--fixup`, `--fixup=amend:` or `--fixup=reword:` options of
linkgit:git-commit[1], which take the target commit as an argument and
diff --git a/sequencer.c b/sequencer.c
index 0fe8fed6c3..435b100e3d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -823,7 +823,7 @@ static struct object_id *get_cache_tree_oid(struct index_state *istate)
return &istate->cache_tree->oid;
}
-static int is_index_unchanged(struct repository *r)
+static int is_index_unchanged(struct repository *r, int amend)
{
struct object_id head_oid, *cache_tree_oid;
const struct object_id *head_tree_oid;
@@ -856,7 +856,26 @@ static int is_index_unchanged(struct repository *r)
if (repo_parse_commit(r, head_commit))
return -1;
- head_tree_oid = get_commit_tree_oid(head_commit);
+ if (amend) {
+ /*
+ * When amending (e.g. melding a "fixup!" or "squash!"),
+ * the commit we are about to create replaces HEAD, so
+ * its parent is HEAD's parent. It is therefore empty
+ * when the index matches the tree of HEAD's parent
+ * rather than the tree of HEAD itself.
+ */
+ if (head_commit->parents) {
+ struct commit *parent =
+ head_commit->parents->item;
+ if (repo_parse_commit(r, parent))
+ return -1;
+ head_tree_oid = get_commit_tree_oid(parent);
+ } else {
+ head_tree_oid = the_hash_algo->empty_tree;
+ }
+ } else {
+ head_tree_oid = get_commit_tree_oid(head_commit);
+ }
}
if (!(cache_tree_oid = get_cache_tree_oid(istate)))
@@ -1786,7 +1805,7 @@ static int is_original_commit_empty(struct commit *commit)
*/
static int allow_empty(struct repository *r,
struct replay_opts *opts,
- struct commit *commit)
+ struct commit *commit, int amend)
{
int index_unchanged, originally_empty;
@@ -1798,13 +1817,33 @@ static int allow_empty(struct repository *r,
* drop_redundant_commits determine whether the commit should be kept or
* dropped. If neither is specified, halt.
*/
- index_unchanged = is_index_unchanged(r);
+ index_unchanged = is_index_unchanged(r, amend);
if (index_unchanged < 0)
return index_unchanged;
if (!index_unchanged)
return 0; /* we do not have to say --allow-empty */
- originally_empty = is_original_commit_empty(commit);
+ /*
+ * When amending (melding a "fixup!"/"squash!"), the resulting commit
+ * replaces HEAD, so whether it "started" empty or "became" empty is
+ * decided by whether the commit being melded into was itself empty: if
+ * HEAD had content that the fixup cancelled out, the commit became empty
+ * and is subject to keep/drop_redundant; if HEAD was already empty, the
+ * commit started empty and is subject to allow_empty as usual.
+ */
+ if (amend) {
+ struct object_id head_oid;
+ struct commit *head_commit;
+
+ if (repo_get_oid(r, "HEAD", &head_oid))
+ return error(_("could not resolve HEAD commit"));
+ head_commit = lookup_commit_reference(r, &head_oid);
+ if (!head_commit)
+ return -1;
+ originally_empty = is_original_commit_empty(head_commit);
+ } else {
+ originally_empty = is_original_commit_empty(commit);
+ }
if (originally_empty < 0)
return originally_empty;
if (originally_empty)
@@ -2260,6 +2299,30 @@ static const char *reflog_message(struct replay_opts *opts,
return buf.buf;
}
+/*
+ * A "fixup!"/"squash!" that melds into HEAD may empty it out. In that case,
+ * with --empty=drop, we want to drop the commit entirely. Since the commit
+ * being amended has already been created (by the preceding "pick"), and the
+ * index and worktree already match the tree of its parent, dropping it is a
+ * matter of moving HEAD back to that parent.
+ */
+static int reset_head_to_parent(struct repository *r, struct replay_opts *opts,
+ struct object_id *head)
+{
+ struct commit *head_commit = lookup_commit_reference(r, head);
+
+ if (!head_commit || repo_parse_commit(r, head_commit))
+ return error(_("could not parse HEAD commit"));
+ if (!head_commit->parents)
+ return error(_("cannot drop the root commit"));
+
+ return refs_update_ref(get_main_ref_store(r),
+ reflog_message(opts, "fixup",
+ "dropping emptied commit"),
+ "HEAD", &head_commit->parents->item->object.oid,
+ head, 0, UPDATE_REFS_MSG_ON_ERR);
+}
+
static int do_pick_commit(struct repository *r,
struct todo_item *item,
struct replay_opts *opts,
@@ -2493,7 +2556,7 @@ static int do_pick_commit(struct repository *r,
}
drop_commit = 0;
- allow = allow_empty(r, opts, commit);
+ allow = allow_empty(r, opts, commit, flags & AMEND_MSG);
if (allow < 0) {
res = allow;
goto leave;
@@ -2506,9 +2569,24 @@ static int do_pick_commit(struct repository *r,
unlink(git_path_merge_msg(r));
refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
NULL, REF_NO_DEREF);
- fprintf(stderr,
- _("dropping %s %s -- patch contents already upstream\n"),
- oid_to_hex(&commit->object.oid), msg.subject);
+ if (flags & AMEND_MSG) {
+ /*
+ * The "fixup!"/"squash!" emptied out the commit it was
+ * melded into; that commit was already created by the
+ * preceding "pick", so drop it by moving HEAD back to
+ * its parent.
+ */
+ res = reset_head_to_parent(r, opts, &head);
+ if (res)
+ goto leave;
+ fprintf(stderr,
+ _("dropping %s %s -- resulting commit is empty\n"),
+ oid_to_hex(&commit->object.oid), msg.subject);
+ } else {
+ fprintf(stderr,
+ _("dropping %s %s -- patch contents already upstream\n"),
+ oid_to_hex(&commit->object.oid), msg.subject);
+ }
} /* else allow == 0 and there's nothing special to do */
if (!opts->no_commit && !drop_commit) {
if (author || command == TODO_REVERT || (flags & AMEND_MSG))
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 5033411a43..508dcc7527 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -510,4 +510,68 @@ test_expect_success 'pick and fixup respect commit.cleanup' '
test_commit_message HEAD -m "something"
'
+test_expect_success 'fixup! that empties its target is dropped with --empty=drop' '
+ git reset --hard base &&
+ test_commit --no-tag addX fileX 1 &&
+ test_commit --no-tag changeX fileX 2 &&
+ test_commit --no-tag later fileW hello &&
+ echo 1 >fileX &&
+ git commit -m "fixup! changeX" fileX &&
+
+ git rebase -i --autosquash --empty=drop HEAD~4 &&
+
+ git log --format=%s >actual &&
+ ! grep changeX actual &&
+ grep addX actual &&
+ grep later actual &&
+ echo 1 >expect &&
+ test_cmp expect fileX &&
+ echo hello >expect &&
+ test_cmp expect fileW
+'
+
+test_expect_success 'fixup! that empties its target is kept with --empty=keep' '
+ git reset --hard base &&
+ test_commit --no-tag addY fileY 1 &&
+ test_commit --no-tag changeY fileY 2 &&
+ echo 1 >fileY &&
+ git commit -m "fixup! changeY" fileY &&
+
+ git rebase -i --autosquash --empty=keep HEAD~3 &&
+
+ git log --format=%s >actual &&
+ grep changeY actual &&
+ : "the retained commit is empty" &&
+ git diff --exit-code HEAD~1 HEAD &&
+ echo 1 >expect &&
+ test_cmp expect fileY
+'
+
+test_expect_success 'fixup! that empties its target stops with --empty=stop' '
+ git reset --hard base &&
+ test_commit --no-tag addZ fileZ 1 &&
+ test_commit --no-tag changeZ fileZ 2 &&
+ echo 1 >fileZ &&
+ git commit -m "fixup! changeZ" fileZ &&
+
+ test_when_finished "git rebase --abort" &&
+ test_must_fail git rebase -i --autosquash --empty=stop HEAD~3
+'
+
+test_expect_success 'squash! that empties its target is dropped with --empty=drop' '
+ git reset --hard base &&
+ test_commit --no-tag addS fileS 1 &&
+ test_commit --no-tag changeS fileS 2 &&
+ echo 1 >fileS &&
+ git commit -m "squash! changeS" fileS &&
+
+ git rebase -i --autosquash --empty=drop HEAD~3 &&
+
+ git log --format=%s >actual &&
+ ! grep changeS actual &&
+ grep addS actual &&
+ echo 1 >expect &&
+ test_cmp expect fileS
+'
+
test_done
^ permalink raw reply related
* Re: [PATCH 0/7] refs: remove use of `the_repository`
From: Patrick Steinhardt @ 2026-07-10 5:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq5x2nlwyg.fsf@gitster.g>
On Thu, Jul 09, 2026 at 01:39:03PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
> > 2026-07-06) with ps/refs-writing-subcommands at 002fe677ca
> > (builtin/refs: add "rename" subcommand, 2026-07-06) merged into it.
> > Despite that, there's a small set of conflicts with "seen" that can be
> > merged like this:
>
> Thanks for a heads-up.
>
> This seems to break so many tests when merged to either 'jch' or
> 'seen', even though all of them pass standalone. I did not have
> time to figure out what interactions with which other topic are
> causing the breakages.
Oh, interesting. I'll investigate what other topic this has interactions
with. Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH 1/7] refs/packed: de-globalize handling of "core.packedRefsTimeout"
From: Patrick Steinhardt @ 2026-07-10 5:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7bn4ov1g.fsf@gitster.g>
On Thu, Jul 09, 2026 at 11:52:11AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > index 499cb55dfa..5c49c06493 100644
> > --- a/refs/packed-backend.c
> > +++ b/refs/packed-backend.c
> > @@ -162,6 +162,13 @@ struct packed_ref_store {
> > * `packed_ref_store`) must not be freed.
> > */
> > struct tempfile *tempfile;
> > +
> > + /*
> > + * Timeout when taking the "packed-refs.lock" file. configurable via
> > + * "core.packedRefsTimeout".
> > + */
> > + bool timeout_configured;
> > + int timeout_value;
> > };
> >
> > /*
> > @@ -1233,12 +1240,10 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
> > struct packed_ref_store *refs =
> > packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
> > "packed_refs_lock");
> > - static int timeout_configured = 0;
> > - static int timeout_value = 1000;
> >
> > - if (!timeout_configured) {
> > - repo_config_get_int(the_repository, "core.packedrefstimeout", &timeout_value);
> > - timeout_configured = 1;
>
> In the original code, when core.packedrefstimeout is not configured,
> our call to repo_config_get_int() does not touch timeout_value. As
> a result, we get the static 1000 and flip the "configured" flag to
> prevent this _value from further getting updated.
>
> > + if (!refs->timeout_configured) {
> > + repo_config_get_int(ref_store->repo, "core.packedrefstimeout", &refs->timeout_value);
> > + refs->timeout_configured = true;
>
> But what happens in the new code when core.packedrefstimeout is not
> configured? It is up to whoever initialised refs->timeout_value.
>
> If I am not mistaken, packed_ref_store_init() does xcalloc(), lets
> base_ref_store_init() initialise some members, initialises a few
> members itself (such as .store_flags and .path), and leaves other
> members, including .timeout_configured and .timeout_value,
> NUL-filled. .timeout_configured starting as false is perfectly
> fine, but shouldn't we initialise .timeout_value to 1000 as before?
Ugh, we should. That's what happens when you tack on a last-minute patch
to a series you had sitting around for weeks. Thanks for noticing!
Patrick
^ permalink raw reply
* Re: [PATCH 0/7] refs: remove use of `the_repository`
From: Patrick Steinhardt @ 2026-07-10 6:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <alCJgLcjXKEgNwFF@pks.im>
On Fri, Jul 10, 2026 at 07:56:19AM +0200, Patrick Steinhardt wrote:
> On Thu, Jul 09, 2026 at 01:39:03PM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
> > > 2026-07-06) with ps/refs-writing-subcommands at 002fe677ca
> > > (builtin/refs: add "rename" subcommand, 2026-07-06) merged into it.
> > > Despite that, there's a small set of conflicts with "seen" that can be
> > > merged like this:
> >
> > Thanks for a heads-up.
> >
> > This seems to break so many tests when merged to either 'jch' or
> > 'seen', even though all of them pass standalone. I did not have
> > time to figure out what interactions with which other topic are
> > causing the breakages.
>
> Oh, interesting. I'll investigate what other topic this has interactions
> with. Thanks!
Hm, curious, I cannot reproduce any of these failures at all, everything
is passing locally when merging "seen" into my branch. Did you maybe
mismerge the changes in "setup.c" by accident? That seems like the most
likely reason as you mention that it breaks lots of tests, and "setup.c"
is of course involved with all of them.
For reference, this is what the final result of the conflicting part
looks like on my side:
if (real_git_dir) {
struct stat st;
if (!exist_ok && !stat(git_dir, &st))
die(_("%s already exists"), git_dir);
if (!exist_ok && !stat(real_git_dir, &st))
die(_("%s already exists"), real_git_dir);
apply_and_export_relative_gitdir(repo, real_git_dir, 1);
git_dir = repo_get_git_dir(repo);
separate_git_dir(repo, git_dir, original_git_dir);
} else {
apply_and_export_relative_gitdir(repo, git_dir, 1);
git_dir = repo_get_git_dir(repo);
}
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH] builtin/add.c: replace run_command() with direct apply_all_patches() call
From: Junio C Hamano @ 2026-07-10 6:41 UTC (permalink / raw)
To: Gatla Vishweshwar Reddy; +Cc: git
In-Reply-To: <20260709192619.46791-1-gatlavishweshwarreddy26@gmail.com>
Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:
> When the user runs "git add -e", the diff of the working tree changes
> is written to a temporary file, opened in an editor, and then applied
> back to the index. The application step was done by spawning a child
"was" -> "is"; in the first part of the log message that gives an
observation, we describe the status quo in the present tense.
> process running "git apply --recount --cached <file>", which is an
> unnecessary subprocess since the apply machinery is available as a
> native C API.
> @@ -187,7 +186,6 @@ static int edit_patch(struct repository *repo,
> const char *prefix)
> {
> char *file = repo_git_path(repo, "ADD_EDIT.patch");
> - struct child_process child = CHILD_PROCESS_INIT;
> struct rev_info rev;
> int out;
> struct stat st;
> @@ -217,11 +215,15 @@ static int edit_patch(struct repository *repo,
> if (!st.st_size)
> die(_("empty patch. aborted"));
>
> - child.git_cmd = 1;
> - strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
> - NULL);
> - if (run_command(&child))
> + struct apply_state state;
> + const char *apply_argv[] = { file, NULL };
> +
> + if (init_apply_state(&state, repo, prefix))
> + die(_("could not initialize apply state"));
> + state.cached = 1;
> + if (apply_all_patches(&state, 1, apply_argv, APPLY_OPT_RECOUNT))
> die(_("could not apply '%s'"), file);
> + clear_apply_state(&state);
Compared to existing callers of the apply_all_patches() API
function, this implementation curiously lacks a prior call to
check_apply_state().
Has this been tested, and do we have sufficient test coverage for it?
Calling check_apply_state() should flip state->check_index on, given
that state.cached is set to 1 above. If I remember correctly, having
this bit enabled is required for apply_patch() to toggle the
.update_index member, which in turn allows apply_all_patches() to
update the index with the patch results. Please double-check this
logic, since it has been a while since I looked at these specific
code paths.
If my assumption holds, this patch might inadvertently stop writing
the result to the index, even though the original intent of
replacing 'apply --cached' was clearly to update it.
Thanks.
>
> unlink(file);
> free(file);
^ permalink raw reply
* [PATCH v8 1/2] config: refactor include_by_gitdir() into include_by_path()
From: Chen Linxuan via B4 Relay @ 2026-07-10 6:43 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Junio C Hamano, Patrick Steinhardt,
Chen Linxuan, Phillip Wood
In-Reply-To: <20260710-includeif-worktree-v8-0-04686d8a616c@black-desk.cn>
From: Chen Linxuan <me@black-desk.cn>
The include_by_gitdir() function matches the realpath of a given
path against a glob pattern, but its interface is tightly coupled to
the gitdir condition: it takes a struct config_options *opts and
extracts opts->git_dir internally.
Refactor it into a more generic include_by_path() helper that takes
a const char *path parameter directly, and update the gitdir and
gitdir/i callers to pass opts->git_dir explicitly. No behavior
change, just preparing for the addition of a new worktree condition
that will reuse the same path-matching logic with a different path.
Signed-off-by: Chen Linxuan <me@black-desk.cn>
---
config.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/config.c b/config.c
index 6a0de86e3ae9..00eeeea370c9 100644
--- a/config.c
+++ b/config.c
@@ -235,23 +235,20 @@ static int prepare_include_condition_pattern(const struct key_value_info *kvi,
return 0;
}
-static int include_by_gitdir(const struct key_value_info *kvi,
- const struct config_options *opts,
- const char *cond, size_t cond_len, int icase)
+static int include_by_path(const struct key_value_info *kvi,
+ const char *path,
+ const char *cond, size_t cond_len, int icase)
{
struct strbuf text = STRBUF_INIT;
struct strbuf pattern = STRBUF_INIT;
size_t prefix;
int ret = 0;
- const char *git_dir;
int already_tried_absolute = 0;
- if (opts->git_dir)
- git_dir = opts->git_dir;
- else
+ if (!path)
goto done;
- strbuf_realpath(&text, git_dir, 1);
+ strbuf_realpath(&text, path, 1);
strbuf_add(&pattern, cond, cond_len);
ret = prepare_include_condition_pattern(kvi, &pattern, &prefix);
if (ret < 0)
@@ -284,7 +281,7 @@ static int include_by_gitdir(const struct key_value_info *kvi,
* which'll do the right thing
*/
strbuf_reset(&text);
- strbuf_add_absolute_path(&text, git_dir);
+ strbuf_add_absolute_path(&text, path);
already_tried_absolute = 1;
goto again;
}
@@ -400,9 +397,9 @@ static int include_condition_is_true(const struct key_value_info *kvi,
const struct config_options *opts = inc->opts;
if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
- return include_by_gitdir(kvi, opts, cond, cond_len, 0);
+ return include_by_path(kvi, opts->git_dir, cond, cond_len, 0);
else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
- return include_by_gitdir(kvi, opts, cond, cond_len, 1);
+ return include_by_path(kvi, opts->git_dir, cond, cond_len, 1);
else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len))
return include_by_branch(inc, cond, cond_len);
else if (skip_prefix_mem(cond, cond_len, "hasconfig:remote.*.url:", &cond,
--
2.53.0
^ permalink raw reply related
* [PATCH v8 2/2] config: add "worktree" and "worktree/i" includeIf conditions
From: Chen Linxuan via B4 Relay @ 2026-07-10 6:43 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Junio C Hamano, Patrick Steinhardt,
Chen Linxuan, Phillip Wood
In-Reply-To: <20260710-includeif-worktree-v8-0-04686d8a616c@black-desk.cn>
From: Chen Linxuan <me@black-desk.cn>
The includeIf mechanism already supports matching on the .git
directory path (gitdir) and the currently checked out branch
(onbranch). But in multi-worktree setups the .git directory of a
linked worktree points into the main repository's .git/worktrees/
area, which makes gitdir patterns cumbersome when one wants to
include config based on the working tree's checkout path instead.
Introduce two new condition keywords:
- worktree:<pattern> matches the realpath of the current worktree's
working directory (i.e. repo_get_work_tree()) against a glob
pattern. This is the path returned by git rev-parse
--show-toplevel.
- worktree/i:<pattern> is the case-insensitive variant.
The implementation reuses the include_by_path() helper introduced in
the previous commit, passing the worktree path in place of the
gitdir. The condition never matches in bare repositories (where
there is no worktree) or during early config reading (where no
repository is available).
Add documentation describing the new conditions, including a comparison
with extensions.worktreeConfig and a note that worktree matching currently
uses the realpath-resolved worktree location. Add tests covering bare
repositories, multiple worktrees, realpath-resolved symlinked worktree
paths, case-sensitive and case-insensitive matching, early config reading,
and non-repository scenarios.
Signed-off-by: Chen Linxuan <me@black-desk.cn>
---
Documentation/config.adoc | 53 +++++++++++++++++++
config.c | 6 +++
t/t1305-config-include.sh | 128 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 187 insertions(+)
diff --git a/Documentation/config.adoc b/Documentation/config.adoc
index 15b1a4d59347..1ef72de62f2b 100644
--- a/Documentation/config.adoc
+++ b/Documentation/config.adoc
@@ -146,6 +146,51 @@ refer to linkgit:gitignore[5] for details. For convenience:
This is the same as `gitdir` except that matching is done
case-insensitively (e.g. on case-insensitive file systems)
+`worktree`::
+ The data that follows the keyword `worktree` and a colon is used as a
+ glob pattern. If the working directory of the current worktree matches
+ the pattern, the include condition is met.
++
+The worktree location is the path where files are checked out (as returned
+by `git rev-parse --show-toplevel`). This is different from `gitdir`, which
+matches the `.git` directory path. In a linked worktree, the worktree path
+is the directory where that worktree's files are located, not the main
+repository's `.git` directory.
++
+The pattern uses the same glob syntax as `gitdir` (including `~/`, `./`,
+`**/`, and trailing-`/` prefix matching). This condition will never match
+in a bare repository (which has no worktree).
++
+Unlike `gitdir`, the `worktree` condition currently matches only the
+realpath-resolved worktree location. If the working tree was entered via a
+symbolic link, a pattern that uses the symbolic-link spelling may not match;
+use the real path instead.
++
+This is useful when you want to apply configuration based on where the
+working tree is located on the filesystem. For example, a contributor who
+works on the same project both personally and as an employee can use
+different `user.name` and `user.email` values depending on which directory
+the worktree is checked out under:
++
+----
+[includeIf "worktree:/home/user/work/"]
+ path = ~/.config/git/work.inc
+[includeIf "worktree:/home/user/personal/"]
+ path = ~/.config/git/personal.inc
+----
++
+While `extensions.worktreeConfig` (see linkgit:git-worktree[1]) also supports
+per-worktree configuration, it stores the config inside each repository's
+`.git/config.worktree` file and requires running `git config --worktree`
+inside each worktree individually. In contrast, `includeIf "worktree:..."`
+can be set once in a global or system-level configuration file (e.g.
+`~/.config/git/config`) and applies to all repositories at once based on
+their worktree location.
+
+`worktree/i`::
+ This is the same as `worktree` except that matching is done
+ case-insensitively (e.g. on case-insensitive file systems)
+
`onbranch`::
The data that follows the keyword `onbranch` and a colon is taken to be a
pattern with standard globbing wildcards and two additional
@@ -244,6 +289,14 @@ Example
[includeIf "gitdir:~/to/group/"]
path = /path/to/foo.inc
+; include if the worktree is at /path/to/project-build
+[includeIf "worktree:/path/to/project-build"]
+ path = build-config.inc
+
+; include for all worktrees inside /path/to/group
+[includeIf "worktree:/path/to/group/"]
+ path = group-config.inc
+
; relative paths are always relative to the including
; file (if the condition is true); their location is not
; affected by the condition
diff --git a/config.c b/config.c
index 00eeeea370c9..9d6d7872d76c 100644
--- a/config.c
+++ b/config.c
@@ -400,6 +400,12 @@ static int include_condition_is_true(const struct key_value_info *kvi,
return include_by_path(kvi, opts->git_dir, cond, cond_len, 0);
else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
return include_by_path(kvi, opts->git_dir, cond, cond_len, 1);
+ else if (skip_prefix_mem(cond, cond_len, "worktree:", &cond, &cond_len))
+ return include_by_path(kvi, inc->repo ? repo_get_work_tree(inc->repo) : NULL,
+ cond, cond_len, 0);
+ else if (skip_prefix_mem(cond, cond_len, "worktree/i:", &cond, &cond_len))
+ return include_by_path(kvi, inc->repo ? repo_get_work_tree(inc->repo) : NULL,
+ cond, cond_len, 1);
else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len))
return include_by_branch(inc, cond, cond_len);
else if (skip_prefix_mem(cond, cond_len, "hasconfig:remote.*.url:", &cond,
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index f3892578e4ff..4e840dfdb35b 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -396,4 +396,132 @@ test_expect_success 'onbranch without repository but explicit nonexistent Git di
test_must_fail nongit git --git-dir=nonexistent config get foo.bar
'
+# worktree: conditional include tests
+
+test_expect_success 'conditional include, worktree bare repo' '
+ git init --bare wt-bare &&
+ (
+ cd wt-bare &&
+ echo "[includeIf \"worktree:/\"]path=bar-bare" >>config &&
+ echo "[test]wtbare=1" >bar-bare &&
+ test_must_fail git config test.wtbare
+ )
+'
+
+test_expect_success 'conditional include, worktree multiple worktrees' '
+ git init wt-multi &&
+ (
+ cd wt-multi &&
+ test_commit initial &&
+ git worktree add -b linked-branch ../wt-linked HEAD &&
+ git worktree add -b prefix-branch ../wt-prefix/linked HEAD
+ ) &&
+ wt_main="$(cd wt-multi && pwd)" &&
+ wt_linked="$(cd wt-linked && pwd)" &&
+ wt_prefix_parent="$(cd wt-prefix && pwd)" &&
+ cat >>wt-multi/.git/config <<-EOF &&
+ [includeIf "worktree:$wt_main"]
+ path = main-config
+ [includeIf "worktree:$wt_linked"]
+ path = linked-config
+ [includeIf "worktree:$wt_prefix_parent/"]
+ path = prefix-config
+ EOF
+ echo "[test]mainvar=main" >wt-multi/.git/main-config &&
+ echo "[test]linkedvar=linked" >wt-multi/.git/linked-config &&
+ echo "[test]prefixvar=prefix" >wt-multi/.git/prefix-config &&
+ echo main >expect &&
+ git -C wt-multi config test.mainvar >actual &&
+ test_cmp expect actual &&
+ test_must_fail git -C wt-multi config test.linkedvar &&
+ test_must_fail git -C wt-multi config test.prefixvar &&
+ echo linked >expect &&
+ git -C wt-linked config test.linkedvar >actual &&
+ test_cmp expect actual &&
+ test_must_fail git -C wt-linked config test.mainvar &&
+ test_must_fail git -C wt-linked config test.prefixvar &&
+ echo prefix >expect &&
+ git -C wt-prefix/linked config test.prefixvar >actual &&
+ test_cmp expect actual &&
+ test_must_fail git -C wt-prefix/linked config test.mainvar &&
+ test_must_fail git -C wt-prefix/linked config test.linkedvar
+'
+
+test_expect_success SYMLINKS 'conditional include, worktree resolves symlinks' '
+ mkdir real-wt &&
+ ln -s real-wt link-wt &&
+ git init link-wt/repo &&
+ (
+ cd link-wt/repo &&
+ # repo->worktree resolves symlinks, so use real path in pattern
+ echo "[includeIf \"worktree:**/real-wt/repo\"]path=bar-link" >>.git/config &&
+ echo "[test]wtlink=2" >.git/bar-link &&
+ echo 2 >expect &&
+ git config test.wtlink >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success !CASE_INSENSITIVE_FS 'conditional include, worktree, case sensitive' '
+ git init wt-case &&
+ (
+ cd wt-case &&
+ test_commit initial &&
+ wt_path="$(pwd)" &&
+ wt_upper=$(echo "$wt_path" | tr a-z A-Z) &&
+ echo "[includeIf \"worktree:$wt_upper\"]path=case-inc" >>.git/config &&
+ echo "[test]wtcase=1" >.git/case-inc &&
+ test_must_fail git config test.wtcase
+ )
+'
+
+test_expect_success 'conditional include, worktree, icase' '
+ git init wt-icase &&
+ (
+ cd wt-icase &&
+ test_commit initial &&
+ wt_path="$(pwd)" &&
+ wt_upper=$(echo "$wt_path" | tr a-z A-Z) &&
+ echo "[includeIf \"worktree/i:$wt_upper\"]path=icase-inc" >>.git/config &&
+ echo "[test]wticase=1" >.git/icase-inc &&
+ echo 1 >expect &&
+ git config test.wticase >actual &&
+ test_cmp expect actual
+ )
+'
+
+# The "worktree" condition cannot match during early config reading
+# because the repository object is not yet fully initialized and
+# repo_get_work_tree() returns NULL.
+test_expect_success 'conditional include, worktree does not match in early config' '
+ git init wt-early &&
+ (
+ cd wt-early &&
+ test_commit initial &&
+ wt_path="$(pwd)" &&
+ echo "[includeIf \"worktree:$wt_path\"]path=early-inc" >>.git/config &&
+ echo "[test]wtearly=1" >.git/early-inc &&
+ test-tool config read_early_config test.wtearly >actual &&
+ test_must_be_empty actual
+ )
+'
+
+# Use a loose pattern so the "present in non-worktree cases" check works
+# for Unix-style absolute paths and Windows paths like D:/a/git/...
+test_expect_success 'conditional include, worktree without repository' '
+ test_when_finished "rm -f .gitconfig config.inc" &&
+ git config set -f .gitconfig "includeIf.worktree:**.path" config.inc &&
+ git config set -f config.inc foo.bar baz &&
+ git config get foo.bar &&
+ test_must_fail nongit git config get foo.bar
+'
+
+test_expect_success 'conditional include, worktree without repository but explicit nonexistent Git directory' '
+ test_when_finished "rm -f .gitconfig config.inc" &&
+ git config set -f .gitconfig "includeIf.worktree:**.path" config.inc &&
+ git config set -f config.inc foo.bar baz &&
+ git config get foo.bar &&
+ test_must_fail nongit git --git-dir=nonexistent config get foo.bar
+'
+
test_done
--
2.53.0
^ permalink raw reply related
* [PATCH v8 0/2] includeIf: add "worktree" condition for matching working tree path
From: Chen Linxuan via B4 Relay @ 2026-07-10 6:43 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Junio C Hamano, Patrick Steinhardt,
Chen Linxuan, Phillip Wood
In-Reply-To: <20260709-includeif-worktree-v7-0-e87e705e8df6@black-desk.cn>
The `includeIf` mechanism already supports matching on the `.git`
directory path (`gitdir`) and the currently checked out branch
(`onbranch`). But in multi-worktree setups the `.git` directory of a
linked worktree points into the main repository's `.git/worktrees/`
area, which makes `gitdir` patterns cumbersome when one wants to
include config based on the working tree's checkout path instead.
Introduce two new condition keywords:
- `worktree:<pattern>` matches the working directory of the current
worktree against a glob pattern.
- `worktree/i:<pattern>` is the case-insensitive variant.
Supported pattern features: glob wildcards, `**/` and `/**`, `~`
expansion, `./` relative paths, and trailing-`/` prefix matching.
The condition never matches in a bare repository.
Signed-off-by: Chen Linxuan <me@black-desk.cn>
---
Changes in v8:
- Drop the v7 symlink-preserving worktree path implementation. Patrick
pointed out that the setup-side plumbing was too invasive and likely to
conflict with the ongoing setup discovery work.
- Document the current limitation instead: includeIf "worktree:" matches
the realpath-resolved worktree location, so symlink spellings may not
match.
- Return the series to two patches, based on v6 plus the documentation
update.
- Link to v7: https://lore.kernel.org/r/20260709-includeif-worktree-v7-0-e87e705e8df6@black-desk.cn
Changes in v7:
- Preserve the symlinked spelling of the worktree path and match
includeIf "worktree:" against it, so the condition now matches both
the symlinked and the real path, consistent with "gitdir:"
(Patrick Steinhardt, v6 review).
- Split the work into a preparatory commit that stores a non-realpath
worktree path and a follow-up that wires it into includeIf.
- Extend symlink test coverage to subdirectories and linked worktrees.
- Link to v6: https://lore.kernel.org/r/20260703-includeif-worktree-v6-0-a13893ad9a7f@black-desk.cn
Changes in v6:
- Rebase onto current `master` at Git 2.55.
- Add an in-code comment explaining why the non-repository worktree
tests use the loose `**.path` pattern (suggested by Junio C Hamano).
- Link to v5: https://lore.kernel.org/r/20260525-includeif-worktree-v5-0-1efe525d025a@black-desk.cn
Changes in v5:
- Fix Windows CI failure: use `**` glob pattern instead of `/` in the
"worktree without repository" tests, since `/` as a path pattern is
Unix-specific and does not match Windows paths.
Github CI pass: https://github.com/black-desk/git/actions/runs/26380466288
- Add a test verifying case-sensitive matching by default, with the
`!CASE_INSENSITIVE_FS` prerequisite (suggested by Patrick Steinhardt).
- Link to v4: https://lore.kernel.org/r/20260513-includeif-worktree-v4-0-f8e6212d1fba@black-desk.cn
Changes in v4:
- Deduplicate the worktree pattern documentation by referencing the
gitdir syntax instead of repeating the full pattern description
(suggested by Patrick Steinhardt).
- Add documentation comparing includeIf "worktree:" with
extensions.worktreeConfig, including a concrete use case example
(suggested by Phillip Wood, Junio C Hamano).
- Add a test verifying that the worktree condition does not match
during early config reading (suggested by Patrick Steinhardt).
- Add tests for the non-repository (nongit) scenario (suggested by
Patrick Steinhardt).
- Add a test for the case-insensitive "worktree/i" variant
- Link to v3: https://lore.kernel.org/r/20260403-includeif-worktree-v3-0-109ce5782b03@black-desk.cn
Changes in v3:
- Apply Junio's suggestion.
- Link to v2: https://lore.kernel.org/r/20260402-includeif-worktree-v2-0-36e339b898d7@black-desk.cn
Changes in v2:
- Add missing signed-off-by lines.
- Link to v1: https://lore.kernel.org/r/20260401-includeif-worktree-v1-0-906db69f2c79@black-desk.cn
---
Chen Linxuan (2):
config: refactor include_by_gitdir() into include_by_path()
config: add "worktree" and "worktree/i" includeIf conditions
Documentation/config.adoc | 53 +++++++++++++++++++
config.c | 25 +++++----
t/t1305-config-include.sh | 128 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 195 insertions(+), 11 deletions(-)
Range-diff versus v7:
1: 510f28d207f8 = 1: 731d928b1dfa config: refactor include_by_gitdir() into include_by_path()
2: 0f83ffee0338 < -: ------------ repository: keep a symlink-preserving copy of the worktree path
3: 18d1abc325fc ! 2: 56c792090625 config: add "worktree" and "worktree/i" includeIf conditions
@@ Commit message
Introduce two new condition keywords:
- - worktree:<pattern> matches the working directory of the current
- worktree (the path returned by git rev-parse --show-toplevel)
- against a glob pattern.
+ - worktree:<pattern> matches the realpath of the current worktree's
+ working directory (i.e. repo_get_work_tree()) against a glob
+ pattern. This is the path returned by git rev-parse
+ --show-toplevel.
- worktree/i:<pattern> is the case-insensitive variant.
- The implementation reuses the include_by_path() helper, passing
- repo_get_work_tree_original() (added in the previous commit; it keeps
- the symlink-preserving spelling of the worktree path) in place of the
- gitdir. As with gitdir, include_by_path() then matches both the
- realpath and the original spelling, so a pattern may use either. The
- condition never matches in bare repositories (where there is no
- worktree) or during early config reading (where no repository is
- available).
+ The implementation reuses the include_by_path() helper introduced in
+ the previous commit, passing the worktree path in place of the
+ gitdir. The condition never matches in bare repositories (where
+ there is no worktree) or during early config reading (where no
+ repository is available).
Add documentation describing the new conditions, including a comparison
- with extensions.worktreeConfig. Add tests covering bare repositories,
- multiple worktrees, symlinked and subdir-of-symlinked worktree paths,
- case-sensitive and case-insensitive matching, early config reading,
+ with extensions.worktreeConfig and a note that worktree matching currently
+ uses the realpath-resolved worktree location. Add tests covering bare
+ repositories, multiple worktrees, realpath-resolved symlinked worktree
+ paths, case-sensitive and case-insensitive matching, early config reading,
and non-repository scenarios.
Signed-off-by: Chen Linxuan <me@black-desk.cn>
@@ Documentation/config.adoc: refer to linkgit:gitignore[5] for details. For conven
+`**/`, and trailing-`/` prefix matching). This condition will never match
+in a bare repository (which has no worktree).
++
++Unlike `gitdir`, the `worktree` condition currently matches only the
++realpath-resolved worktree location. If the working tree was entered via a
++symbolic link, a pattern that uses the symbolic-link spelling may not match;
++use the real path instead.
+++
+This is useful when you want to apply configuration based on where the
+working tree is located on the filesystem. For example, a contributor who
+works on the same project both personally and as an employee can use
@@ config.c: static int include_condition_is_true(const struct key_value_info *kvi,
else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
return include_by_path(kvi, opts->git_dir, cond, cond_len, 1);
+ else if (skip_prefix_mem(cond, cond_len, "worktree:", &cond, &cond_len))
-+ return include_by_path(kvi, inc->repo ? repo_get_work_tree_original(inc->repo) : NULL,
++ return include_by_path(kvi, inc->repo ? repo_get_work_tree(inc->repo) : NULL,
+ cond, cond_len, 0);
+ else if (skip_prefix_mem(cond, cond_len, "worktree/i:", &cond, &cond_len))
-+ return include_by_path(kvi, inc->repo ? repo_get_work_tree_original(inc->repo) : NULL,
++ return include_by_path(kvi, inc->repo ? repo_get_work_tree(inc->repo) : NULL,
+ cond, cond_len, 1);
else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len))
return include_by_branch(inc, cond, cond_len);
@@ t/t1305-config-include.sh: test_expect_success 'onbranch without repository but
+ test_must_fail git -C wt-prefix/linked config test.linkedvar
+'
+
-+test_expect_success SYMLINKS 'conditional include, worktree matching symlink' '
-+ mkdir sym-real &&
-+ ln -s sym-real sym-link &&
-+ git init sym-link/repo &&
-+ (
-+ cd sym-link/repo &&
-+ link_path="$(pwd)" &&
-+ real_path="$(test-tool path-utils real_path "$link_path")" &&
-+ cat >>.git/config <<-EOF &&
-+ [includeIf "gitdir:$link_path/.git"]
-+ path = gitdir-link
-+ [includeIf "gitdir:$real_path/.git"]
-+ path = gitdir-real
-+ [includeIf "worktree:$link_path"]
-+ path = worktree-link
-+ [includeIf "worktree:$real_path"]
-+ path = worktree-real
-+ EOF
-+ echo "[test]gitdirlink=1" >.git/gitdir-link &&
-+ echo "[test]gitdirreal=1" >.git/gitdir-real &&
-+ echo "[test]worktreelink=1" >.git/worktree-link &&
-+ echo "[test]worktreereal=1" >.git/worktree-real &&
-+ git config get test.gitdirlink &&
-+ git config get test.gitdirreal &&
-+ git config get test.worktreelink &&
-+ git config get test.worktreereal &&
-+ # from a subdirectory, the logical worktree path is recovered by
-+ # stripping the below-root suffix, so both spellings still match
-+ mkdir d &&
-+ cd d &&
-+ git config get test.worktreelink &&
-+ git config get test.worktreereal
-+ )
-+'
-+
-+test_expect_success SYMLINKS 'conditional include, worktree matching symlink of a linked worktree' '
-+ git init wt-main &&
-+ ( cd wt-main && test_commit initial ) &&
-+ git -C wt-main worktree add --detach ../wt-real &&
-+ ln -s wt-real wt-link &&
-+ wt_main="$(cd wt-main && pwd)" &&
++test_expect_success SYMLINKS 'conditional include, worktree resolves symlinks' '
++ mkdir real-wt &&
++ ln -s real-wt link-wt &&
++ git init link-wt/repo &&
+ (
-+ cd wt-link &&
-+ link_path="$(pwd)" &&
-+ real_path="$(test-tool path-utils real_path "$link_path")" &&
-+ cat >>"$wt_main/.git/config" <<-EOF &&
-+ [includeIf "worktree:$link_path"]
-+ path = wt-link
-+ [includeIf "worktree:$real_path"]
-+ path = wt-real
-+ EOF
-+ echo "[test]wtlink=1" >"$wt_main/.git/wt-link" &&
-+ echo "[test]wtreal=1" >"$wt_main/.git/wt-real" &&
-+ test "$(git config get test.wtlink)" = "1" &&
-+ test "$(git config get test.wtreal)" = "1"
++ cd link-wt/repo &&
++ # repo->worktree resolves symlinks, so use real path in pattern
++ echo "[includeIf \"worktree:**/real-wt/repo\"]path=bar-link" >>.git/config &&
++ echo "[test]wtlink=2" >.git/bar-link &&
++ echo 2 >expect &&
++ git config test.wtlink >actual &&
++ test_cmp expect actual
+ )
+'
+
---
base-commit: f85a7e662054a7b0d9070e432508831afa214b47
^ permalink raw reply
* Re: [PATCH 1/7] odb/source-packed: improve lookup when enumerating objects
From: Patrick Steinhardt @ 2026-07-10 7:08 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <ak_uXc0UxB_9Vk9z@denethor>
On Thu, Jul 09, 2026 at 02:54:17PM -0500, Justin Tobler wrote:
> On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> > When iterating through packed objects via `odb_for_each_object()` we
> > do so via two different mechanisms:
> >
> > - When a multi-pack index is available we use that one to efficiently
> > loop through all objects.
> >
> > - We then loop through all packfiles that aren't covered by a
> > multi-pack index.
>
> To be specific, we are talking only about the for_each_object callback
> for the packed source `odb_source_packed_for_each_object()` correct?
> Also, this appears to only matter when we are enumerating OIDs with a
> specific prefix.
Yeah, true. I'll clarify this a bit.
> > Regardless of which mechanism we use, we then iterate through all the
> > objects indexed by the respective data structure. Curiously though,
> > while we use the indices for enumerating the objects, we completely
> > ignore it for the actual object lookup. Instead, we call into the
> > generic `odb_source_read_object_info()` function, which will itself
> > consult the indices to figure out where the object in question even
> > lives.
> >
> > This has two consequences:
> >
> > - It's inefficient, as we basically have to figure out the position of
> > the object a second time.
>
> Since we already have the position from the index, there is no need to
> start over. Makes sense.
>
> > - It's subtly wrong, as it may now happen that a specific object will
> > be looked up via a different pack in case it exists multiple times.
>
> Naive question: Is there any real harm in reading the same object, but
> from a different packfile here?
The answer is probably "no". At least I cannot think of any case where
it'd really matter, but semantically it's the wrong thing to do anyway.
> > diff --git a/odb/source-packed.c b/odb/source-packed.c
> > index 0edea5356d..9cfa02b7a2 100644
> > --- a/odb/source-packed.c
> > +++ b/odb/source-packed.c
> > @@ -177,9 +178,8 @@ static int for_each_prefixed_object_in_midx(
> > if (!match_hash(len, opts->prefix->hash, current->hash))
> > break;
> >
> > - if (opts->flags) {
> > + if (opts->flags || data->request) {
>
> I'm not sure I follow why the above condition needed to change.
This needs to change because we now require access to the pack so that
we can call `packed_object_info()`. Otherwise the pack would be not be
populated if we're invoked without any flags.
Patrick
^ permalink raw reply
* Re: [PATCH 3/7] pack-bitmap: allow aborting iteration of bitmapped objects
From: Patrick Steinhardt @ 2026-07-10 7:08 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <alAAN6_ZqLj9tlgV@denethor>
On Thu, Jul 09, 2026 at 03:19:52PM -0500, Justin Tobler wrote:
> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index ea5eab4cf8..8ff92c5272 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -1909,7 +1909,7 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,
> > return 0;
> >
> > create_object_entry(oid, type, name_hash, 0, 0, pack, offset);
> > - return 1;
> > + return 0;
>
> I wonder why this was even returning 1 to begin with? As you mentioned,
> the return value appears to be ignored anyways. I'm assuming it was
> signal that an object entry was created?
The function is only called from a single location, and the return value
was completely ignored until this commit. It has always been this way
since the function was originally introduced in 6b8fda2db1
(pack-objects: use bitmaps when packing objects, 2013-12-21), so it
never seemed to have any purpose. The commit message doesn't mention
anything either.
> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > index 8f63003709..02818b81c6 100644
> > --- a/builtin/rev-list.c
> > +++ b/builtin/rev-list.c
> > @@ -486,7 +486,7 @@ static int show_object_fast(
> > void *payload UNUSED)
> > {
> > fprintf(stdout, "%s\n", oid_to_hex(oid));
> > - return 1;
> > + return 0;
>
> Also curious about this one too. It probably doesn't matter though.
Likewise, this was introduced in aa32939fea (rev-list: add bitmap mode
to speed up object lists, 2013-12-21), and the return value wasn't ever
used for anything.
Patrick
^ permalink raw reply
* Re: [PATCH 4/7] pack-bitmap: iterate object sources when opening bitmaps
From: Patrick Steinhardt @ 2026-07-10 7:08 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <alADU8qRZcPB0Zcv@denethor>
On Thu, Jul 09, 2026 at 04:08:31PM -0500, Justin Tobler wrote:
> On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> > When opening a bitmap for a repository we perform two steps:
> >
> > - We first look for a multi-pack index bitmap in any of the object
> > sources connected to the repository.
> >
> > - We then look for a packfile bitmap in any of the packfiles of any of
> > the object sources.
>
> So IIUC, we generally stop searching for a bitmap once we find one.
Except that we continue searching so that we can print a warning, but
later results are simply being ignored.
> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index eda38a5433..0e3e18a557 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -680,60 +680,53 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
> > return 0;
> > }
> >
> > -static int open_pack_bitmap(struct repository *r,
> > - struct bitmap_index *bitmap_git)
> > +static int open_bitmap_for_source(struct odb_source_packed *source,
> > + struct bitmap_index *bitmap_git)
> > {
> > - struct packed_git *p;
> > + struct multi_pack_index *midx = get_multi_pack_index(source);
> > + struct packfile_list_entry *e;
> > int ret = -1;
> >
> > - repo_for_each_pack(r, p) {
> > - if (open_pack_bitmap_1(bitmap_git, p) == 0) {
> > - ret = 0;
> > - /*
> > - * The only reason to keep looking is to report
> > - * duplicates.
> > - */
> > - if (!trace2_is_enabled())
> > - break;
> > - }
> > + if (midx && !open_midx_bitmap_1(bitmap_git, midx))
> > + ret = 0;
>
> Ok, open_midx_bitmap_1() returns 0 if it find a MIDX and -1 otherwise.
> Probably just a matter of preference, but I think writing out like below
> is a little bit easier on the eyes:
>
> if (midx)
> ret = open_midx_bitmap_1(bitmap_git, midx);
>
> it might just be that I find the return values a bit confusing though.
> Maybe we could instead use `found` like a bit later in this patch.
That's fair, and the result would be both easier to reason about and
more consistent indeed. I'll adapt this accordingly.
Patrick
^ permalink raw reply
* Re: [PATCH 6/7] odb: introduce object filters to `odb_for_each_object()`
From: Patrick Steinhardt @ 2026-07-10 7:09 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <alATd_YS2d_l3CHq@denethor>
On Thu, Jul 09, 2026 at 04:43:58PM -0500, Justin Tobler wrote:
> On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> > The function `for_each_bitmapped_object()` can be used to iterate
> > through all objects covered by a bitmap. The benefit of this function is
> > that it allows the caller to efficiently handle some object filters. For
> > example, this can be used to filter out objects of a specific type with
> > some simple bitmap operations. But callers are currently required to
> > manually wire up the use of bitmaps though, and to do so they have to
> > reach into internals of a given object database source.
> >
> > Introduce a new `struct odb_for_each_object_options::filter` field so
> > that the interface becomes generic. When set, then a backend may
> > optionally use the filter to skip some objects that it would have
> > otherwise yielded.
> >
> > Note that the respective backends are free to ignore this field if they
> > cannot meaningfully optimize for a given filter, and consequently
> > callers need to verify whether they actually want the returned objects.
> > While annoying, we cannot easily lift this restriction anyway as the
> > object filter infrastructure supports some filters that cannot be
> > answered by the object database alone.
>
> Huh, this feels rather awkward. So callers will always still have to
> ensure correctness by filtering the result a second time? IIUC, the idea
> is that the backend may be able to more efficiently process object
> filtering so we would want it to attempt the first pass.
>
> Is there a subset of object filters that we should expect any backend to
> be able to answer? If so, maybe we should define a separate list of
> object filter options specific to this interface? Any filtering not
> supported would have to be deligated to the caller then.
It's a bit awkward, but it's also similar to how we handle this for
example in the reference backends with the exclude patterns. I don't
really think it makes sense to enforce that backends may only handle a
subset of object filters that we know the current backends support, as
that would artificially limit us.
For example, the "loose" backend already cannot efficiently handle many
of the filters that the "packed" backend can handle, like for example
filtering by type. So ultimately, the subset of filters that can be
handled efficiently by both backends is empty. And as the "files"
backend always combines both of these backends we wouldn't be able to
ever use the object filter at all there.
The same could be true for any future backend: we cannot assume how they
store their objects, so they might be able to efficiently handle filters
that the current backends cannot.
An alternative going forward could be to perform filtering of yielded
objects inside `odb_for_each_object()` itself so that it will filter out
any objects that the backends themselves couldn't filter efficiently.
But I'm not sure I want to go there as part of this series -- we only
have a single caller anyway that iterates with a filter, and that caller
already knows to manually filter references.
I'll add a bit of an explanation to the commit message.
Patrick
^ permalink raw reply
* Re: [PATCH 7/7] builtin/cat-file: filter objects via object database
From: Patrick Steinhardt @ 2026-07-10 7:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq33xsoupa.fsf@gitster.g>
On Thu, Jul 09, 2026 at 11:59:29AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Refactor git-cat-file(1) to use the new object filter option when
> > batching all objects. This significantly simplifies the logic and
> > ensures that we don't have to reach into internals of the "files" source
> > anymore.
>
> This would become more convincing if you spent a few lines before
> presenting the solution to give an observation of what the current
> code does, e.g.,
>
> When batching all objects, git-cat-file(1) reaches into the
> internals of the object database and manually manages bitmaps to
> apply object filters. This creates coupling between the command
> and ODB backend internals.
>
> to highlight the perceived problem in it. That would flow naturally
> to the description of your solution.
Good point, will add.
Patrick
^ permalink raw reply
* [PATCH] b4: include change-id in cover template
From: Chen Linxuan via B4 Relay @ 2026-07-10 7:22 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Junio C Hamano, Chen Linxuan
From: Chen Linxuan <me@black-desk.cn>
With b4 0.15.2, I hit a local failure after sending a series with the
in-tree cover template. The generated sent/<change-id>-vN tag contained
base-commit, but did not contain change-id, and later b4 commands failed
when trying to read it:
CRITICAL: Tag sent/... does not contain change-id info
Looking at b4's source, the sent tag message is derived from the rendered
cover letter. The same code later parses that tag and expects both
base-commit and change-id to be present. The default b4 cover template
has both trailers, but our in-tree template only has base-commit.
Add the missing change-id trailer next to base-commit so sent tags
produced from the project template remain readable by b4's reroll and
comparison logic.
Signed-off-by: Chen Linxuan <me@black-desk.cn>
---
.b4-cover-template | 1 +
1 file changed, 1 insertion(+)
diff --git a/.b4-cover-template b/.b4-cover-template
index ab864933b5c8..8168d8a10b3a 100644
--- a/.b4-cover-template
+++ b/.b4-cover-template
@@ -8,4 +8,5 @@ ${diffstat}
${range_diff}
---
base-commit: ${base_commit}
+change-id: ${change_id}
${prerequisites}
---
base-commit: f60db8d575adb79761d363e026fb49bddf330c73
change-id: 20260710-add-change-id-to-b4-template-f9fd20937027
^ permalink raw reply related
* [PATCH v2] builtin/add.c: replace run_command() with direct apply_all_patches() call
From: Gatla Vishweshwar Reddy @ 2026-07-10 7:32 UTC (permalink / raw)
To: git; +Cc: Gatla Vishweshwar Reddy
In-Reply-To: <xmqqmrvzfitd.fsf@gitster.g>
When the user runs "git add -e", the diff of the working tree changes
is written to a temporary file, opened in an editor, and then applied
back to the index. The application step is done by spawning a child
process running "git apply --recount --cached <file>", which is an
unnecessary subprocess since the apply machinery is available as a
native C API.
Replace the run_command() call with a direct call to apply_all_patches()
using an initialized apply_state with the cached and recount options set
appropriately. This avoids the overhead of forking a subprocess, keeps
the operation within the same process, and makes the intent of the code
clearer to the reader.
Remove the now-unused includes of "run-command.h" and "strvec.h" since
no other code in this file requires them after this change.
Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
---
Changes in v2:
- Fixed commit message: "was done" -> "is done" (present tense)
- Added check_apply_state() call after setting state.cached = 1,
which sets state.check_index = 1 required for index updates
In response to review:
- check_apply_state() with cached=1 correctly
sets check_index=1, ensuring apply_all_patches() updates the index
as intended. Verified by reading apply.c lines 172-175.
- Tested with t3700-add.sh: all 58 tests pass
builtin/add.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c859f66519..a7266020cd 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -13,7 +13,6 @@
#include "dir.h"
#include "gettext.h"
#include "pathspec.h"
-#include "run-command.h"
#include "object-file.h"
#include "odb.h"
#include "odb/transaction.h"
@@ -23,9 +22,9 @@
#include "diff.h"
#include "read-cache.h"
#include "revision.h"
-#include "strvec.h"
#include "submodule.h"
#include "add-interactive.h"
+#include "apply.h"
static const char * const builtin_add_usage[] = {
N_("git add [<options>] [--] <pathspec>..."),
@@ -187,7 +186,6 @@ static int edit_patch(struct repository *repo,
const char *prefix)
{
char *file = repo_git_path(repo, "ADD_EDIT.patch");
- struct child_process child = CHILD_PROCESS_INIT;
struct rev_info rev;
int out;
struct stat st;
@@ -217,11 +215,17 @@ static int edit_patch(struct repository *repo,
if (!st.st_size)
die(_("empty patch. aborted"));
- child.git_cmd = 1;
- strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
- NULL);
- if (run_command(&child))
+ struct apply_state state;
+ const char *apply_argv[] = { file, NULL };
+
+ if (init_apply_state(&state, repo, prefix))
+ die(_("could not initialize apply state"));
+ state.cached = 1;
+ if (check_apply_state(&state, 0))
+ die(_("could not check apply state"));
+ if (apply_all_patches(&state, 1, apply_argv, APPLY_OPT_RECOUNT))
die(_("could not apply '%s'"), file);
+ clear_apply_state(&state);
unlink(file);
free(file);
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] b4: include change-id in cover template
From: Patrick Steinhardt @ 2026-07-10 8:46 UTC (permalink / raw)
To: me; +Cc: git, Junio C Hamano
In-Reply-To: <20260710-add-change-id-to-b4-template-v1-1-1bd37a25064e@black-desk.cn>
On Fri, Jul 10, 2026 at 03:22:13PM +0800, Chen Linxuan via B4 Relay wrote:
> From: Chen Linxuan <me@black-desk.cn>
>
> With b4 0.15.2, I hit a local failure after sending a series with the
> in-tree cover template. The generated sent/<change-id>-vN tag contained
> base-commit, but did not contain change-id, and later b4 commands failed
> when trying to read it:
>
> CRITICAL: Tag sent/... does not contain change-id info
>
> Looking at b4's source, the sent tag message is derived from the rendered
> cover letter. The same code later parses that tag and expects both
> base-commit and change-id to be present. The default b4 cover template
> has both trailers, but our in-tree template only has base-commit.
>
> Add the missing change-id trailer next to base-commit so sent tags
> produced from the project template remain readable by b4's reroll and
> comparison logic.
Ah, that's indeed an oversight on my side. So this change looks good to
me, thanks!
Patrick
^ permalink raw reply
* [PATCH v2 0/8] odb: introduce object filters to `odb_for_each_object()`
From: Patrick Steinhardt @ 2026-07-10 8:48 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano
In-Reply-To: <20260709-pks-odb-for-each-object-filter-v1-0-82fe014b12b3@pks.im>
Hi,
this patch series introduces object filters to `odb_for_each_object()`.
The intent of this is to make `git cat-file --batch-all-objects` work
with pluggable object databases. Right now it doesn't because it reaches
into internals of the "packed" backend to efficiently handle bitmapped
objects.
The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
2026-07-06) with ps/odb-drop-whence at 8a7ad23e11 (odb: document object
info fields, 2026-07-02) merged into it.
Changes in v2:
- Add another patch to drop the `_1()` prefixes that aren't required
anymore.
- Change the approach in `open_bitmap_for_source()` to also use a
`found` boolean instead of a confusing integer.
- Add some more explanations to commit messages.
- Link to v1: https://patch.msgid.link/20260709-pks-odb-for-each-object-filter-v1-0-82fe014b12b3@pks.im
Thanks!
Patrick
---
Patrick Steinhardt (8):
odb/source-packed: improve lookup when enumerating objects
pack-bitmap: mark object filter as `const`
pack-bitmap: allow aborting iteration of bitmapped objects
pack-bitmap: iterate object sources when opening bitmaps
pack-bitmap: drop `_1` suffix from functions that open bitmaps
pack-bitmap: introduce function to open bitmap for a single source
odb: introduce object filters to `odb_for_each_object()`
builtin/cat-file: filter objects via object database
builtin/cat-file.c | 76 +++--------------------------
builtin/pack-objects.c | 2 +-
builtin/rev-list.c | 2 +-
odb.h | 12 +++++
odb/source-packed.c | 77 ++++++++++++++++++++++++++---
pack-bitmap.c | 129 +++++++++++++++++++++++++++----------------------
pack-bitmap.h | 10 +++-
7 files changed, 171 insertions(+), 137 deletions(-)
Range-diff versus v1:
1: 7a1a92acbe ! 1: b675967b78 odb/source-packed: improve lookup when enumerating objects
@@ Metadata
## Commit message ##
odb/source-packed: improve lookup when enumerating objects
- When iterating through packed objects via `odb_for_each_object()` we
- do so via two different mechanisms:
+ When iterating through objects of a packed source that have a specific
+ prefix we do so via two different methods:
- When a multi-pack index is available we use that one to efficiently
loop through all objects.
@@ Commit message
- It's subtly wrong, as it may now happen that a specific object will
be looked up via a different pack in case it exists multiple times.
+ This is unlikely to have any real-world consequences, but it's still
+ the wrong thing to do.
Fix the issue by using `packed_object_info()` directly. While at it,
rename the `store` variable to `source`.
2: a6c8bd7a61 = 2: d3f9b2f781 pack-bitmap: mark object filter as `const`
3: c38b06636b = 3: 825920205a pack-bitmap: allow aborting iteration of bitmapped objects
4: 450cdd13b7 ! 4: a33ca8fa3b pack-bitmap: iterate object sources when opening bitmaps
@@ pack-bitmap.c: static int load_bitmap(struct repository *r, struct bitmap_index
+ struct bitmap_index *bitmap_git)
{
- struct packed_git *p;
+- int ret = -1;
+ struct multi_pack_index *midx = get_multi_pack_index(source);
+ struct packfile_list_entry *e;
- int ret = -1;
++ bool found = false;
- repo_for_each_pack(r, p) {
- if (open_pack_bitmap_1(bitmap_git, p) == 0) {
@@ pack-bitmap.c: static int load_bitmap(struct repository *r, struct bitmap_index
- break;
- }
+ if (midx && !open_midx_bitmap_1(bitmap_git, midx))
-+ ret = 0;
++ found = true;
+
+ for (e = packfile_store_get_packs(source); e; e = e->next) {
+ /*
+ * When tracing is enabled we want to keep looking to report
+ * duplicates even if we have already found a bitmap.
+ */
-+ if (!ret && !trace2_is_enabled())
++ if (found && !trace2_is_enabled())
+ break;
+
-+ if (open_pack_bitmap_1(bitmap_git, e->pack))
-+ continue;
-+ ret = 0;
++ if (!open_pack_bitmap_1(bitmap_git, e->pack))
++ found = true;
}
- return ret;
+- return ret;
++ return found ? 0 : -1;
}
-static int open_midx_bitmap(struct repository *r,
@@ pack-bitmap.c: static int load_bitmap(struct repository *r, struct bitmap_index
{
struct odb_source *source;
- int ret = -1;
-+ int found = 0;
++ bool found = false;
assert(!bitmap_git->map);
@@ pack-bitmap.c: static int load_bitmap(struct repository *r, struct bitmap_index
- int found;
- assert(!bitmap_git->map);
-+ found |= !open_bitmap_for_source(files->packed, bitmap_git);
++ if (!open_bitmap_for_source(files->packed, bitmap_git))
++ found = true;
- found = !open_midx_bitmap(r, bitmap_git);
-
-: ---------- > 5: b890ed7163 pack-bitmap: drop `_1` suffix from functions that open bitmaps
5: 26b1957f8b = 6: f7e466217b pack-bitmap: introduce function to open bitmap for a single source
6: 722727c76d ! 7: 27ecc0802f odb: introduce object filters to `odb_for_each_object()`
@@ Commit message
object filter infrastructure supports some filters that cannot be
answered by the object database alone.
+ An alternative might be to limit the filters to only those that _can_ be
+ answered by backends. But ultimately, the filters that can be answered
+ efficiently by the "packed" backend are completely disjunct from those
+ that can be answered by the "loose" backend, and consequently the set of
+ filters supported by all backends would be empty. Furthermore, it would
+ require us to make assumptions about capabilities of future backends,
+ which may be able to efficiently handle more filters than current ones.
+ So in the end, this alternative would only limit us artificially.
+
Implement the logic for the "packed" source. Note that we use the new
function `prepare_source_bitmap_git()` to open the bitmap: as the
backend operates on a single object source, we must only use bitmaps
7: 90be28e904 ! 8: e4c6aeab0a builtin/cat-file: filter objects via object database
@@ Metadata
## Commit message ##
builtin/cat-file: filter objects via object database
+ When batching all objects, git-cat-file(1) reaches into the internals of
+ the object database and manually manages bitmaps to apply object
+ filters. This creates coupling between the command and the internals of
+ the respective backend.
+
Refactor git-cat-file(1) to use the new object filter option when
batching all objects. This significantly simplifies the logic and
ensures that we don't have to reach into internals of the "files" source
---
base-commit: 3c8e2790f2ce15e8b5d4b4e6ced711b12649f32a
change-id: 20260708-pks-odb-for-each-object-filter-13286fa3523d
^ permalink raw reply
* [PATCH v2 1/8] odb/source-packed: improve lookup when enumerating objects
From: Patrick Steinhardt @ 2026-07-10 8:48 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im>
When iterating through objects of a packed source that have a specific
prefix we do so via two different methods:
- When a multi-pack index is available we use that one to efficiently
loop through all objects.
- We then loop through all packfiles that aren't covered by a
multi-pack index.
Regardless of which mechanism we use, we then iterate through all the
objects indexed by the respective data structure. Curiously though,
while we use the indices for enumerating the objects, we completely
ignore it for the actual object lookup. Instead, we call into the
generic `odb_source_read_object_info()` function, which will itself
consult the indices to figure out where the object in question even
lives.
This has two consequences:
- It's inefficient, as we basically have to figure out the position of
the object a second time.
- It's subtly wrong, as it may now happen that a specific object will
be looked up via a different pack in case it exists multiple times.
This is unlikely to have any real-world consequences, but it's still
the wrong thing to do.
Fix the issue by using `packed_object_info()` directly. While at it,
rename the `store` variable to `source`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
odb/source-packed.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0edea5356d..9cfa02b7a2 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -143,7 +143,7 @@ static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_f
}
static int for_each_prefixed_object_in_midx(
- struct odb_source_packed *store,
+ struct odb_source_packed *source,
struct multi_pack_index *m,
const struct odb_for_each_object_options *opts,
struct odb_source_packed_for_each_object_wrapper_data *data)
@@ -170,6 +170,7 @@ static int for_each_prefixed_object_in_midx(
*/
for (i = first; i < num; i++) {
const struct object_id *current = NULL;
+ struct packed_git *pack;
struct object_id oid;
current = nth_midxed_object_oid(&oid, m, i);
@@ -177,9 +178,8 @@ static int for_each_prefixed_object_in_midx(
if (!match_hash(len, opts->prefix->hash, current->hash))
break;
- if (opts->flags) {
+ if (opts->flags || data->request) {
uint32_t pack_id = nth_midxed_pack_int_id(m, i);
- struct packed_git *pack;
if (prepare_midx_pack(m, pack_id)) {
pack_errors = true;
@@ -193,9 +193,9 @@ static int for_each_prefixed_object_in_midx(
if (data->request) {
struct object_info oi = *data->request;
+ off_t offset = nth_midxed_offset(m, i);
- ret = odb_source_read_object_info(&store->base, current,
- &oi, 0);
+ ret = packed_object_info(source, pack, offset, &oi);
if (ret)
goto out;
@@ -219,7 +219,7 @@ static int for_each_prefixed_object_in_midx(
}
static int for_each_prefixed_object_in_pack(
- struct odb_source_packed *store,
+ struct odb_source_packed *source,
struct packed_git *p,
const struct odb_for_each_object_options *opts,
struct odb_source_packed_for_each_object_wrapper_data *data)
@@ -246,8 +246,9 @@ static int for_each_prefixed_object_in_pack(
if (data->request) {
struct object_info oi = *data->request;
+ off_t offset = nth_packed_object_offset(p, i);
- ret = odb_source_read_object_info(&store->base, &oid, &oi, 0);
+ ret = packed_object_info(source, p, offset, &oi);
if (ret)
goto out;
--
2.55.0.229.g6434b31f56.dirty
^ permalink raw reply related
* [PATCH v2 2/8] pack-bitmap: mark object filter as `const`
From: Patrick Steinhardt @ 2026-07-10 8:48 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im>
The function `for_each_bitmapped_object()` accepts an optional object
filter. This filter is never modified by the function, but is not
declared as `const`. Fix this.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
pack-bitmap.c | 6 +++---
pack-bitmap.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 35774b6f0c..a47c231632 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1976,7 +1976,7 @@ static void filter_bitmap_object_type(struct bitmap_index *bitmap_git,
static int filter_bitmap(struct bitmap_index *bitmap_git,
struct object_list *tip_objects,
struct bitmap *to_filter,
- struct list_objects_filter_options *filter)
+ const struct list_objects_filter_options *filter)
{
if (!filter || filter->choice == LOFC_DISABLED)
return 0;
@@ -2027,7 +2027,7 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
return -1;
}
-static int can_filter_bitmap(struct list_objects_filter_options *filter)
+static int can_filter_bitmap(const struct list_objects_filter_options *filter)
{
return !filter_bitmap(NULL, NULL, NULL, filter);
}
@@ -2058,7 +2058,7 @@ static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
}
int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
- struct list_objects_filter_options *filter,
+ const struct list_objects_filter_options *filter,
show_reachable_fn show_reach,
void *payload)
{
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 19a8655457..47935eb24e 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -96,7 +96,7 @@ struct list_objects_filter_options;
* not supported, `0` otherwise.
*/
int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
- struct list_objects_filter_options *filter,
+ const struct list_objects_filter_options *filter,
show_reachable_fn show_reach,
void *payload);
--
2.55.0.229.g6434b31f56.dirty
^ permalink raw reply related
* [PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects
From: Patrick Steinhardt @ 2026-07-10 8:48 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im>
In a subsequent commit we'll lift iteration of bitmapped objects into
the "packed" backend and make it accessible via `odb_for_each_object()`.
The calling convention for that function is that the callback may return
a non-zero exit code, and if so we'll abort iteration. This is currently
impossible to realize though, as `for_each_bitmapped_object()` will
ignore any return value and just churn through all objects completely.
This doesn't matter to the callers of `for_each_bitmapped_object()`, as
there's only one of them in git-cat-file(1), and the callbacks we pass
always return zero. But once we move the logic into the generic
infrastructure it becomes a latent bug waiting to happen.
Refactor the code so that the return value of the `show_reach` callback
is not ignored anymore. Instead, returning a non-zero value will cause
us to abort iteration in both `show_objects_for_type()` and in
`for_each_bitmapped_object()`.
Note though that there's a second user of `show_objects_for_type()` with
`traverse_bitmap_commit_list()`, and that function does indeed invoke
callbacks that may return non-zero. This non-zero return value never had
any effect at all though, and the callbacks that return non-zero values
are only ever invoked via `traverse_bitmap_commit_list()`. Consequently,
we adapt them to always return 0.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/pack-objects.c | 2 +-
builtin/rev-list.c | 2 +-
pack-bitmap.c | 31 +++++++++++++++++++++----------
pack-bitmap.h | 3 ++-
4 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ea5eab4cf8..8ff92c5272 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1909,7 +1909,7 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,
return 0;
create_object_entry(oid, type, name_hash, 0, 0, pack, offset);
- return 1;
+ return 0;
}
struct pbase_tree_cache {
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 8f63003709..02818b81c6 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -486,7 +486,7 @@ static int show_object_fast(
void *payload UNUSED)
{
fprintf(stdout, "%s\n", oid_to_hex(oid));
- return 1;
+ return 0;
}
static void print_disk_usage(off_t size)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index a47c231632..eda38a5433 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1695,7 +1695,7 @@ static void init_type_iterator(struct ewah_or_iterator *it,
}
}
-static void show_objects_for_type(
+static int show_objects_for_type(
struct bitmap_index *bitmap_git,
struct bitmap *objects,
enum object_type object_type,
@@ -1704,6 +1704,7 @@ static void show_objects_for_type(
{
size_t i = 0;
uint32_t offset;
+ int ret;
struct ewah_or_iterator it;
eword_t filter;
@@ -1749,11 +1750,17 @@ static void show_objects_for_type(
hash = bitmap_name_hash(bitmap_git, index_pos);
- show_reach(&oid, object_type, 0, hash, pack, ofs, payload);
+ ret = show_reach(&oid, object_type, 0, hash, pack, ofs, payload);
+ if (ret)
+ goto out;
}
}
+ ret = 0;
+
+out:
ewah_or_iterator_release(&it);
+ return ret;
}
static int in_bitmapped_pack(struct bitmap_index *bitmap_git,
@@ -2062,6 +2069,12 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
show_reachable_fn show_reach,
void *payload)
{
+ const enum object_type types[] = {
+ OBJ_COMMIT,
+ OBJ_TREE,
+ OBJ_BLOB,
+ OBJ_TAG,
+ };
struct bitmap *filtered_bitmap = NULL;
uint32_t objects_nr;
size_t full_word_count;
@@ -2086,14 +2099,12 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
goto out;
}
- show_objects_for_type(bitmap_git, filtered_bitmap,
- OBJ_COMMIT, show_reach, payload);
- show_objects_for_type(bitmap_git, filtered_bitmap,
- OBJ_TREE, show_reach, payload);
- show_objects_for_type(bitmap_git, filtered_bitmap,
- OBJ_BLOB, show_reach, payload);
- show_objects_for_type(bitmap_git, filtered_bitmap,
- OBJ_TAG, show_reach, payload);
+ for (size_t i = 0; i < ARRAY_SIZE(types); i++) {
+ ret = show_objects_for_type(bitmap_git, filtered_bitmap,
+ types[i], show_reach, payload);
+ if (ret)
+ goto out;
+ }
ret = 0;
out:
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 47935eb24e..ae8dc491ac 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -93,7 +93,8 @@ struct list_objects_filter_options;
/*
* Filter bitmapped objects and iterate through all resulting objects,
* executing `show_reach` for each of them. Returns `-1` in case the filter is
- * not supported, `0` otherwise.
+ * not supported, `0` otherwise. Aborts iteration and bubbles up the return
+ * value in case `show_reach()` returns non-zero.
*/
int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
const struct list_objects_filter_options *filter,
--
2.55.0.229.g6434b31f56.dirty
^ permalink raw reply related
* [PATCH v2 4/8] pack-bitmap: iterate object sources when opening bitmaps
From: Patrick Steinhardt @ 2026-07-10 8:48 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im>
When opening a bitmap for a repository we perform two steps:
- We first look for a multi-pack index bitmap in any of the object
sources connected to the repository.
- We then look for a packfile bitmap in any of the packfiles of any of
the object sources.
Both of these steps thus iterate through object sources themselves, one
via `odb_prepare_alternates()` and one via `repo_for_each_pack()`. This
layout makes it hard to introduce a way to open the bitmap of one
specific object source, which is functionality that we'll require in a
subsequent commit.
Reverse the loop so that we instead loop through all sources in the
outer loop, and then for each source we try to load its bitmap via
either the multi-pack index or via a packfile.
Note that this changes the precedence of bitmaps in one specific edge
case: when an earlier object source only has a packfile bitmap, but a
later source has a multi-pack index bitmap, we now pick the packfile
bitmap of the earlier source. Previously, a multi-pack index bitmap from
any source would have taken precedence over all packfile bitmaps. Given
that object sources are ordered such that the local source comes first,
this arguably is an improvement, as we now prefer local bitmaps over
bitmaps in alternates. Furthermore, we already warn about repositories
that have multiple bitmaps, so this setup is broken and thus arguably
not worth worrying about too much.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
pack-bitmap.c | 69 +++++++++++++++++++++++++++--------------------------------
1 file changed, 31 insertions(+), 38 deletions(-)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index eda38a5433..e32795a595 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -680,60 +680,53 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
return 0;
}
-static int open_pack_bitmap(struct repository *r,
- struct bitmap_index *bitmap_git)
+static int open_bitmap_for_source(struct odb_source_packed *source,
+ struct bitmap_index *bitmap_git)
{
- struct packed_git *p;
- int ret = -1;
+ struct multi_pack_index *midx = get_multi_pack_index(source);
+ struct packfile_list_entry *e;
+ bool found = false;
- repo_for_each_pack(r, p) {
- if (open_pack_bitmap_1(bitmap_git, p) == 0) {
- ret = 0;
- /*
- * The only reason to keep looking is to report
- * duplicates.
- */
- if (!trace2_is_enabled())
- break;
- }
+ if (midx && !open_midx_bitmap_1(bitmap_git, midx))
+ found = true;
+
+ for (e = packfile_store_get_packs(source); e; e = e->next) {
+ /*
+ * When tracing is enabled we want to keep looking to report
+ * duplicates even if we have already found a bitmap.
+ */
+ if (found && !trace2_is_enabled())
+ break;
+
+ if (!open_pack_bitmap_1(bitmap_git, e->pack))
+ found = true;
}
- return ret;
+ return found ? 0 : -1;
}
-static int open_midx_bitmap(struct repository *r,
- struct bitmap_index *bitmap_git)
+static int open_bitmap(struct repository *r,
+ struct bitmap_index *bitmap_git)
{
struct odb_source *source;
- int ret = -1;
+ bool found = false;
assert(!bitmap_git->map);
odb_prepare_alternates(r->objects);
for (source = r->objects->sources; source; source = source->next) {
struct odb_source_files *files = odb_source_files_downcast(source);
- struct multi_pack_index *midx = get_multi_pack_index(files->packed);
- if (midx && !open_midx_bitmap_1(bitmap_git, midx))
- ret = 0;
- }
- return ret;
-}
-
-static int open_bitmap(struct repository *r,
- struct bitmap_index *bitmap_git)
-{
- int found;
- assert(!bitmap_git->map);
+ if (!open_bitmap_for_source(files->packed, bitmap_git))
+ found = true;
- found = !open_midx_bitmap(r, bitmap_git);
-
- /*
- * these will all be skipped if we opened a midx bitmap; but run it
- * anyway if tracing is enabled to report the duplicates
- */
- if (!found || trace2_is_enabled())
- found |= !open_pack_bitmap(r, bitmap_git);
+ /*
+ * The only reason to keep looking after having found a bitmap
+ * is to report duplicates.
+ */
+ if (found && !trace2_is_enabled())
+ break;
+ }
return found ? 0 : -1;
}
--
2.55.0.229.g6434b31f56.dirty
^ permalink raw reply related
* [PATCH v2 5/8] pack-bitmap: drop `_1` suffix from functions that open bitmaps
From: Patrick Steinhardt @ 2026-07-10 8:48 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im>
In the preceding commit we've refactored how we open bitmaps. As part of
the refactoring we have consolidated `open_pack_bitmap()` as well as
`open_midx_bitmap()` into `open_bitmap_for_source()`. Consequently, we
only have their `open_pack_bitmap_1()` and `open_midx_bitmap_1()`
variants left over, where the `_1` suffix doesn't really make much sense
anymore.
Drop the suffix.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
pack-bitmap.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index e32795a595..72c8ae3228 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -460,8 +460,8 @@ char *pack_bitmap_filename(struct packed_git *p)
return xstrfmt("%.*s.bitmap", (int)len, p->pack_name);
}
-static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
- struct multi_pack_index *midx)
+static int open_midx_bitmap(struct bitmap_index *bitmap_git,
+ struct multi_pack_index *midx)
{
struct stat st;
char *bitmap_name = midx_bitmap_filename(midx);
@@ -539,7 +539,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
return -1;
}
-static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
+static int open_pack_bitmap(struct bitmap_index *bitmap_git, struct packed_git *packfile)
{
int fd;
struct stat st;
@@ -603,7 +603,7 @@ static int load_reverse_index(struct repository *r, struct bitmap_index *bitmap_
/*
* The multi-pack-index's .rev file is already loaded via
- * open_pack_bitmap_1().
+ * open_pack_bitmap().
*
* But we still need to open the individual pack .rev files,
* since we will need to make use of them in pack-objects.
@@ -687,7 +687,7 @@ static int open_bitmap_for_source(struct odb_source_packed *source,
struct packfile_list_entry *e;
bool found = false;
- if (midx && !open_midx_bitmap_1(bitmap_git, midx))
+ if (midx && !open_midx_bitmap(bitmap_git, midx))
found = true;
for (e = packfile_store_get_packs(source); e; e = e->next) {
@@ -698,7 +698,7 @@ static int open_bitmap_for_source(struct odb_source_packed *source,
if (found && !trace2_is_enabled())
break;
- if (!open_pack_bitmap_1(bitmap_git, e->pack))
+ if (!open_pack_bitmap(bitmap_git, e->pack))
found = true;
}
@@ -746,7 +746,7 @@ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
{
struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
- if (!open_midx_bitmap_1(bitmap_git, midx))
+ if (!open_midx_bitmap(bitmap_git, midx))
return bitmap_git;
free_bitmap_index(bitmap_git);
--
2.55.0.229.g6434b31f56.dirty
^ permalink raw reply related
* [PATCH v2 6/8] pack-bitmap: introduce function to open bitmap for a single source
From: Patrick Steinhardt @ 2026-07-10 8:48 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im>
The function `prepare_bitmap_git()` opens the first bitmap it can find
in any of the object sources connected to the repository. In a
subsequent commit, the "packed" object database backend will learn to
use bitmaps to answer object filters when enumerating objects. That
backend operates on a single object source though, so using a bitmap
that potentially belongs to a different source would be wrong:
- The source would yield objects that are not part of the source
itself.
- The object source info would be attributed to the wrong source.
- With multiple sources, each source would enumerate the same bitmap
another time.
Introduce a new function `prepare_source_bitmap_git()` that only opens
bitmaps belonging to the given object source.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
pack-bitmap.c | 12 ++++++++++++
pack-bitmap.h | 2 ++
2 files changed, 14 insertions(+)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 72c8ae3228..09ba15d26b 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -753,6 +753,18 @@ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
return NULL;
}
+struct bitmap_index *prepare_bitmap_git_for_source(struct odb_source_packed *source)
+{
+ struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
+
+ if (!open_bitmap_for_source(source, bitmap_git) &&
+ !load_bitmap(source->base.odb->repo, bitmap_git, 0))
+ return bitmap_git;
+
+ free_bitmap_index(bitmap_git);
+ return NULL;
+}
+
int bitmap_index_contains_pack(struct bitmap_index *bitmap, struct packed_git *pack)
{
for (; bitmap; bitmap = bitmap->base) {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index ae8dc491ac..9f20fb6e56 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -9,6 +9,7 @@
#include "string-list.h"
struct commit;
+struct odb_source_packed;
struct repository;
struct rev_info;
@@ -68,6 +69,7 @@ struct bitmapped_pack {
struct bitmap_index *prepare_bitmap_git(struct repository *r);
struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx);
+struct bitmap_index *prepare_bitmap_git_for_source(struct odb_source_packed *source);
/*
* Given a bitmap index, determine whether it contains the pack either directly
--
2.55.0.229.g6434b31f56.dirty
^ permalink raw reply related
* [PATCH v2 7/8] odb: introduce object filters to `odb_for_each_object()`
From: Patrick Steinhardt @ 2026-07-10 8:48 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im>
The function `for_each_bitmapped_object()` can be used to iterate
through all objects covered by a bitmap. The benefit of this function is
that it allows the caller to efficiently handle some object filters. For
example, this can be used to filter out objects of a specific type with
some simple bitmap operations. But callers are currently required to
manually wire up the use of bitmaps though, and to do so they have to
reach into internals of a given object database source.
Introduce a new `struct odb_for_each_object_options::filter` field so
that the interface becomes generic. When set, then a backend may
optionally use the filter to skip some objects that it would have
otherwise yielded.
Note that the respective backends are free to ignore this field if they
cannot meaningfully optimize for a given filter, and consequently
callers need to verify whether they actually want the returned objects.
While annoying, we cannot easily lift this restriction anyway as the
object filter infrastructure supports some filters that cannot be
answered by the object database alone.
An alternative might be to limit the filters to only those that _can_ be
answered by backends. But ultimately, the filters that can be answered
efficiently by the "packed" backend are completely disjunct from those
that can be answered by the "loose" backend, and consequently the set of
filters supported by all backends would be empty. Furthermore, it would
require us to make assumptions about capabilities of future backends,
which may be able to efficiently handle more filters than current ones.
So in the end, this alternative would only limit us artificially.
Implement the logic for the "packed" source. Note that we use the new
function `prepare_source_bitmap_git()` to open the bitmap: as the
backend operates on a single object source, we must only use bitmaps
that belong to that specific source. Otherwise we might yield objects
that are not part of the source at all, and with multiple sources we
would enumerate the same bitmap once per source.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
odb.h | 12 +++++++++++
odb/source-packed.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
pack-bitmap.c | 3 +--
pack-bitmap.h | 3 +++
4 files changed, 78 insertions(+), 2 deletions(-)
diff --git a/odb.h b/odb.h
index a1e222f605..67d0b34942 100644
--- a/odb.h
+++ b/odb.h
@@ -8,6 +8,7 @@
#include "thread-utils.h"
struct cached_object_entry;
+struct list_objects_filter_options;
struct odb_source_inmemory;
struct packed_git;
struct repository;
@@ -490,6 +491,17 @@ struct odb_for_each_object_options {
*/
const struct object_id *prefix;
size_t prefix_hex_len;
+
+ /*
+ * Optional object filter that allows backends to skip yielding
+ * objects that are excluded by the filter as an optimization. The
+ * filter is a best-effort hint: backends may use it to skip
+ * excluded objects (e.g. by consulting a reachability bitmap), but
+ * are also free to ignore it entirely and yield every object. As a
+ * consequence, callers must re-apply the filter on yielded objects
+ * if they require strict filtering semantics.
+ */
+ const struct list_objects_filter_options *filter;
};
/*
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 9cfa02b7a2..4777395053 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -3,11 +3,13 @@
#include "chdir-notify.h"
#include "dir.h"
#include "git-zlib.h"
+#include "list-objects-filter-options.h"
#include "mergesort.h"
#include "midx.h"
#include "odb/source-packed.h"
#include "odb/streaming.h"
#include "packfile.h"
+#include "pack-bitmap.h"
static int find_pack_entry(struct odb_source_packed *store,
const struct object_id *oid,
@@ -315,6 +317,37 @@ static int odb_source_packed_for_each_prefixed_object(
return ret;
}
+struct bitmapped_for_each_object_data {
+ struct odb_source_packed *packed;
+ const struct object_info *request;
+ const struct odb_for_each_object_options *opts;
+ odb_for_each_object_cb cb;
+ void *cb_data;
+};
+
+static int bitmapped_for_each_object(const struct object_id *oid,
+ enum object_type type UNUSED,
+ int flags UNUSED,
+ uint32_t hash UNUSED,
+ struct packed_git *pack,
+ off_t offset,
+ void *cb_data)
+{
+ struct bitmapped_for_each_object_data *data = cb_data;
+
+ if (should_exclude_pack(pack, data->opts->flags))
+ return 0;
+
+ if (data->request) {
+ struct object_info oi = *data->request;
+ if (packed_object_info(data->packed, pack, offset, &oi) < 0)
+ return -1;
+ return data->cb(oid, &oi, data->cb_data);
+ }
+
+ return data->cb(oid, NULL, data->cb_data);
+}
+
static int odb_source_packed_for_each_object(struct odb_source *source,
const struct object_info *request,
odb_for_each_object_cb cb,
@@ -328,12 +361,33 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
.cb = cb,
.cb_data = cb_data,
};
+ struct bitmap_index *bitmap = NULL;
struct packfile_list_entry *e;
int pack_errors = 0, ret;
if (opts->prefix)
return odb_source_packed_for_each_prefixed_object(packed, opts, &data);
+ if (opts->filter &&
+ opts->filter->choice != LOFC_DISABLED &&
+ can_filter_bitmap(opts->filter))
+ bitmap = prepare_bitmap_git_for_source(packed);
+ if (bitmap) {
+ struct bitmapped_for_each_object_data bitmap_data = {
+ .packed = packed,
+ .request = request,
+ .opts = opts,
+ .cb = cb,
+ .cb_data = cb_data,
+ };
+
+ ret = for_each_bitmapped_object(bitmap, opts->filter,
+ bitmapped_for_each_object,
+ &bitmap_data);
+ if (ret)
+ goto out;
+ }
+
packed->skip_mru_updates = true;
for (e = packfile_store_get_packs(packed); e; e = e->next) {
@@ -342,6 +396,13 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
if (should_exclude_pack(p, opts->flags))
continue;
+ /*
+ * Objects covered by the bitmap have already been yielded
+ * above; skip them here to avoid duplicates.
+ */
+ if (bitmap && bitmap_index_contains_pack(bitmap, p))
+ continue;
+
if (open_pack_index(p)) {
pack_errors = 1;
continue;
@@ -357,6 +418,7 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
out:
packed->skip_mru_updates = false;
+ free_bitmap_index(bitmap);
if (!ret && pack_errors)
ret = -1;
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 09ba15d26b..f55a0859ea 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2039,12 +2039,11 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
return -1;
}
-static int can_filter_bitmap(const struct list_objects_filter_options *filter)
+bool can_filter_bitmap(const struct list_objects_filter_options *filter)
{
return !filter_bitmap(NULL, NULL, NULL, filter);
}
-
static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
struct bitmap *result)
{
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 9f20fb6e56..1385027c1f 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -92,6 +92,9 @@ int test_bitmap_pseudo_merge_objects(struct repository *r, uint32_t n);
struct list_objects_filter_options;
+/* Check whether the filter can be computed via the bitmap. */
+bool can_filter_bitmap(const struct list_objects_filter_options *filter);
+
/*
* Filter bitmapped objects and iterate through all resulting objects,
* executing `show_reach` for each of them. Returns `-1` in case the filter is
--
2.55.0.229.g6434b31f56.dirty
^ permalink raw reply related
* [PATCH v2 8/8] builtin/cat-file: filter objects via object database
From: Patrick Steinhardt @ 2026-07-10 8:49 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im>
When batching all objects, git-cat-file(1) reaches into the internals of
the object database and manually manages bitmaps to apply object
filters. This creates coupling between the command and the internals of
the respective backend.
Refactor git-cat-file(1) to use the new object filter option when
batching all objects. This significantly simplifies the logic and
ensures that we don't have to reach into internals of the "files" source
anymore.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/cat-file.c | 76 +++++-------------------------------------------------
1 file changed, 7 insertions(+), 69 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b4b99a73da..1458dd76d6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,7 +20,6 @@
#include "userdiff.h"
#include "oid-array.h"
#include "packfile.h"
-#include "pack-bitmap.h"
#include "object-file.h"
#include "object-name.h"
#include "odb.h"
@@ -844,28 +843,6 @@ static int batch_one_object_oi(const struct object_id *oid,
return payload->callback(oid, NULL, 0, payload->payload);
}
-static int batch_one_object_packed(const struct object_id *oid,
- struct packed_git *pack,
- uint32_t pos,
- void *_payload)
-{
- struct for_each_object_payload *payload = _payload;
- return payload->callback(oid, pack, nth_packed_object_offset(pack, pos),
- payload->payload);
-}
-
-static int batch_one_object_bitmapped(const struct object_id *oid,
- enum object_type type UNUSED,
- int flags UNUSED,
- uint32_t hash UNUSED,
- struct packed_git *pack,
- off_t offset,
- void *_payload)
-{
- struct for_each_object_payload *payload = _payload;
- return payload->callback(oid, pack, offset, payload->payload);
-}
-
static void batch_each_object(struct batch_options *opt,
for_each_object_fn callback,
unsigned flags,
@@ -875,56 +852,17 @@ static void batch_each_object(struct batch_options *opt,
.callback = callback,
.payload = _payload,
};
+ struct odb_source_info source_info;
+ struct object_info oi = {
+ .source_infop = &source_info,
+ };
struct odb_for_each_object_options opts = {
.flags = flags,
+ .filter = &opt->objects_filter,
};
- struct bitmap_index *bitmap = NULL;
- struct odb_source *source;
-
- /*
- * TODO: we still need to tap into implementation details of the object
- * database sources. Ideally, we should extend `odb_for_each_object()`
- * to handle object filters itself so that we can move the filtering
- * logic into the individual sources.
- */
- odb_prepare_alternates(the_repository->objects);
- for (source = the_repository->objects->sources; source; source = source->next) {
- struct odb_source_files *files = odb_source_files_downcast(source);
- int ret = odb_source_for_each_object(&files->loose->base, NULL, batch_one_object_oi,
- &payload, &opts);
- if (ret)
- break;
- }
-
- if (opt->objects_filter.choice != LOFC_DISABLED &&
- (bitmap = prepare_bitmap_git(the_repository)) &&
- !for_each_bitmapped_object(bitmap, &opt->objects_filter,
- batch_one_object_bitmapped, &payload)) {
- struct packed_git *pack;
-
- repo_for_each_pack(the_repository, pack) {
- if (bitmap_index_contains_pack(bitmap, pack) ||
- open_pack_index(pack))
- continue;
- for_each_object_in_pack(pack, batch_one_object_packed,
- &payload, flags);
- }
- } else {
- struct odb_source_info source_info;
- struct object_info oi = {
- .source_infop = &source_info,
- };
-
- for (source = the_repository->objects->sources; source; source = source->next) {
- struct odb_source_files *files = odb_source_files_downcast(source);
- int ret = odb_source_for_each_object(&files->packed->base, &oi,
- batch_one_object_oi, &payload, &opts);
- if (ret)
- break;
- }
- }
- free_bitmap_index(bitmap);
+ odb_for_each_object_ext(the_repository->objects, &oi,
+ batch_one_object_oi, &payload, &opts);
}
static int batch_objects(struct batch_options *opt)
--
2.55.0.229.g6434b31f56.dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox