* Re: [PATCH v7 0/3] includeIf: add "worktree" condition for matching working tree path
From: Patrick Steinhardt @ 2026-07-09 10:09 UTC (permalink / raw)
To: me; +Cc: git, Kristoffer Haugsbakk, Junio C Hamano, Phillip Wood
In-Reply-To: <20260709-includeif-worktree-v7-0-e87e705e8df6@black-desk.cn>
On Thu, Jul 09, 2026 at 10:41:40AM +0800, Chen Linxuan via B4 Relay wrote:
> 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
One note: it would be nice if you could send newer versions of your
patch series in reply to the old version. I see you're using the b4
relay, so this should be configurable via `b4.send-same-thread`.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v7 2/3] repository: keep a symlink-preserving copy of the worktree path
From: Patrick Steinhardt @ 2026-07-09 10:09 UTC (permalink / raw)
To: me; +Cc: git, Kristoffer Haugsbakk, Junio C Hamano, Phillip Wood
In-Reply-To: <20260709-includeif-worktree-v7-2-e87e705e8df6@black-desk.cn>
On Thu, Jul 09, 2026 at 10:41:42AM +0800, Chen Linxuan via B4 Relay wrote:
> diff --git a/repository.c b/repository.c
> index 73d80bcffdf5..a29d55a6fcd3 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -149,6 +149,11 @@ const char *repo_get_work_tree(struct repository *repo)
> return repo->worktree;
> }
>
> +const char *repo_get_work_tree_original(struct repository *repo)
> +{
> + return repo->worktree_original;
> +}
Feels a bit heavy-handed to have such an accessor, as we could've just
as well accessed the member directly via the structure.
> diff --git a/setup.c b/setup.c
> index 0de56a074f7c..fbbeb95f99db 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1213,12 +1213,94 @@ static const char *setup_explicit_git_dir(struct repository *repo,
> return NULL;
> }
>
> +/*
> + * Do "a" and "b" refer to the same filesystem entry? Both must report a
> + * nonzero (dev,ino): some filesystems return (0,0) for unrelated paths,
> + * which would otherwise look identical.
> + */
> +static int same_entry(const char *a, const char *b)
> +{
> + struct stat sa, sb;
> +
> + if (stat(a, &sa) || stat(b, &sb))
> + return 0;
> + return (sa.st_dev || sa.st_ino) &&
> + sa.st_dev == sb.st_dev && sa.st_ino == sb.st_ino;
> +}
> +
> +/*
> + * Recover the symlink-preserving spelling of the worktree root.
> + *
> + * strbuf_add_absolute_path() already consults $PWD to keep symlinks when
> + * resolving a relative path, so set_git_work_tree()'s other callers get a
> + * symlink-preserving worktree path for free. This function exists for the
> + * discovered-repository case: setup_git_directory_gently() chdir()s to the
> + * worktree root *before* set_git_work_tree(repo, ".") runs, so by the time
> + * "." is resolved $PWD still names the caller's original directory and no
> + * longer agrees with the physical cwd, and strbuf_add_absolute_path()
> + * falls back to the realpath. We close that gap by deriving the logical
> + * root here, from $PWD, while we still have the original physical cwd and
> + * the root offset in hand.
> + *
> + * "cwd" is the physical current directory (getcwd), and "root_len" is the
> + * length of the worktree root within it; cwd->buf[root_len..] is therefore
> + * the part of the path below the root (empty when git ran at the root).
> + *
> + * $PWD, maintained by the shell, may spell that same directory through
> + * symlinks. If we can confirm $PWD really names cwd's directory (same
> + * device/inode) and that the below-root suffix matches, we swap the
> + * physical root prefix for $PWD's prefix and keep the user's symlinks.
> + * Only symlinks in the root prefix itself are preserved: the below-root
> + * suffix is matched byte-for-byte, so a symlink below the root is not.
> + *
> + * Returns the allocated logical path, or NULL when $PWD is missing, already
> + * physical, or untrustworthy.
> + */
Oof.
> +static char *logical_path_from_cwd(struct strbuf *cwd, int root_len)
> +{
> + const char *pwd = getenv("PWD");
> + size_t suffix_len, pwd_len;
> + struct strbuf path = STRBUF_INIT;
> +
> + if (!pwd || !is_absolute_path(pwd) || !strcmp(pwd, cwd->buf))
> + return NULL;
> + /*
> + * $PWD is a plain environment variable: it can be set to anything,
> + * or left stale after a chdir. Only borrow its symlink-preserving
> + * spelling once we prove it still points at the same directory as
> + * the physical cwd; otherwise give up and return NULL.
> + */
> + if (!same_entry(cwd->buf, pwd))
> + return NULL;
> +
> + /*
> + * Drop the below-root suffix from $PWD. It must match the physical
> + * suffix exactly; the only spelling difference we accept is in the
> + * root prefix -- i.e. the symlinks we want to preserve.
> + */
> + suffix_len = cwd->len - root_len;
> + pwd_len = strlen(pwd);
> + if (suffix_len) {
> + const char *suffix = cwd->buf + root_len;
> +
> + if (suffix_len > pwd_len ||
> + fspathcmp(pwd + pwd_len - suffix_len, suffix))
> + return NULL;
> + pwd_len -= suffix_len;
> + }
> +
> + strbuf_add(&path, pwd, pwd_len);
> + return strbuf_detach(&path, NULL);
> +}
This feels quite awkward to me, and I assume that these changes will
lead to conflicts with ps/setup-split-discovery-and-setup.
I wonder whether we can maybe avoid this whole mess by removing the call
to chdir(3p) when discovering Git directories in the first place.
Instead, we'd only chdir(3p) after we have fully discovered the Git
repository's paths, and that may allow us to not have to worry about
reconstructing the logical path?
It's something that I wanted to explore after the mentioned patch series
has landed, but maybe it's something we should try to do as part of this
patch series here.
Alternatively, I'm less certain that this complexity is ultimately
really worth it now... so another alternative could be to document the
issue and fix it at a later point in time.
Patrick
^ permalink raw reply
* Re: [PATCH v3 4/5] builtin/refs: add "create" subcommand
From: Toon Claes @ 2026-07-09 10:05 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <87zf00mqv1.fsf@emacs.iotcl.com>
Toon Claes <toon@iotcl.com> writes:
> Patrick Steinhardt <ps@pks.im> writes:
>> This flag is somewhat weird. Having it is probably a sensible think to
>> do, but now that I think about it I wonder whether the default makes all
>> that much sense in the first place. That being said, _if_ we want to
>> change it then we should change it for all subcommands.
>
> Not sure how to make it better, so let's leave it like this.
Well, we could drop the `--no-deref` completely? If you want to modify a
ref, use git-ref(1). If you pass that command a symref, it always
dereferences down to the ref. If you want to modify a symref, use
git-symbolic-ref(1).
--
Cheers,
Toon
^ permalink raw reply
* Re: [PATCH v3 4/5] builtin/refs: add "create" subcommand
From: Toon Claes @ 2026-07-09 9:53 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <aktVdaB2xRk-iI_8@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Jul 03, 2026 at 04:19:58PM +0200, Toon Claes wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> > diff --git a/Documentation/git-refs.adoc b/Documentation/git-refs.adoc
>> > index 6475bdcc62..e6a3528349 100644
>> > --- a/Documentation/git-refs.adoc
>> > +++ b/Documentation/git-refs.adoc
>> > @@ -181,6 +184,53 @@ static int cmd_refs_optimize(int argc, const char **argv, const char *prefix,
>> > return pack_refs_core(argc, argv, prefix, repo, refs_optimize_usage);
>> > }
>> >
>> > +static int cmd_refs_create(int argc, const char **argv, const char *prefix,
>> > + struct repository *repo)
>> > +{
>> > + static char const * const refs_create_usage[] = {
>> > + REFS_CREATE_USAGE,
>> > + NULL
>> > + };
>> > + const char *message = NULL;
>> > + unsigned flags = 0;
>> > + struct option opts[] = {
>> > + OPT_STRING(0, "message", &message, N_("reason"),
>> > + N_("reason of the update")),
>> > + OPT_BIT(0 ,"no-deref", &flags,
>> > + N_("update <refname> not the one it points to"),
>> > + REF_NO_DEREF),
>>
>> Can `git refs create --no-deref` be used to create symrefs? Should we
>> add a test for that? Or can it not
>>
>> I understand the symmetry, but does it make sense to ask the user to
>> create symrefs with `--no-deref`? Feels a bit obscure. The docs say:
>>
>> `--no-deref`::
>> Operate on <ref> itself rather than the reference it points to via a
>> symbolic ref.
>>
>> That's far from obvious for a user to realize they need to pass that
>> option if they want to create a symref.
>
> It doesn't cause them to create a symref. What this flag controls is
> whether the command would fail when the refname exists already as a
> symbolic ref. That is:
>
> $ git symbolic-ref refs/heads/symref refs/heads/target
> $ git refs create refs/heads/symref $OID
> $ git refs exists refs/heads/target
That makes sense. Sort of.
So passing `--no-deref` to `git refs create` in the example above would
make sense if you want creation of refs/heads/target to fail. Okay,
doesn't seem very obvious, but feels correct.
> The git-refs(1) command would have created "refs/heads/target" in this
> case, and by passing "--no-deref" you'd instead make it fail.
>
> This flag is somewhat weird. Having it is probably a sensible think to
> do, but now that I think about it I wonder whether the default makes all
> that much sense in the first place. That being said, _if_ we want to
> change it then we should change it for all subcommands.
Not sure how to make it better, so let's leave it like this.
>> > diff --git a/t/t1466-refs-create.sh b/t/t1466-refs-create.sh
>> > new file mode 100755
>> > index 0000000000..cfb21bf863
>> > --- /dev/null
>> > +++ b/t/t1466-refs-create.sh
>> > @@ -0,0 +1,151 @@
> [snip]
>> > +test_expect_success 'create fails when the reference already exists' '
>> > + test_when_finished "rm -rf repo" &&
>> > + setup_repo repo &&
>> > + (
>> > + cd repo &&
>> > + A=$(git rev-parse A) &&
>> > + B=$(git rev-parse B) &&
>> > + git refs create refs/heads/foo $A &&
>> > + test_must_fail git refs create refs/heads/foo $B 2>err &&
>> > + test_grep "reference already exists" err &&
>> > + test_ref_matches refs/heads/foo "$A"
>> > + )
>> > +'
>>
>> I was curious about this test:
>>
>> test_expect_success 'create succeed when the reference exists with the same value' '
>> test_when_finished "rm -rf repo" &&
>> setup_repo repo &&
>> (
>> cd repo &&
>> A=$(git rev-parse A) &&
>> git refs create refs/heads/foo $A &&
>> git refs create refs/heads/foo $A &&
>> test_ref_matches refs/heads/foo "$A"
>> )
>> '
>>
>> That fails. It that intentional?
>
> Yes, this is intentional. We didn't end up creating the reference, which
> is what the user has asked us to do, and hence we fail.
Understood.
>> > +test_expect_success 'create with symref target and --no-deref refuses to create reference' '
>> > + test_when_finished "rm -rf repo" &&
>> > + setup_repo repo &&
>> > + (
>> > + cd repo &&
>> > + A=$(git rev-parse A) &&
>> > + git symbolic-ref refs/heads/symref refs/heads/target &&
>> > + test_must_fail git refs create --no-deref refs/heads/symref $A 2>err &&
>> > + test_grep "dangling symref already exists" err &&
>> > + test_must_fail git reflog exists refs/heads/target
>> > + )
>> > +'
>>
>> Would it make sense to add this test:
>>
>> test_expect_success 'create with symref target with --no-deref' '
>> test_when_finished "rm -rf repo" &&
>> setup_repo repo &&
>> (
>> cd repo &&
>> A=$(git rev-parse A) &&
>> git refs create refs/heads/target $A &&
>> git refs create --no-deref refs/heads/symref refs/heads/target &&
>> git reflog exists refs/heads/symref && false
>> )
>> '
>>
>> But that makes me think, this option `--no-deref` is pretty obscure for
>> use with `git refs create`. There are two situations:
>>
>> * The symref doesn't exists: so --no-deref basically is forcing the
>> command to create a symref. That's confusing
>
> No, it's not. It tells us that we only want to create the reference if
> it doesn't exist and is not a symref. Otherwise, we'd potentially create
> the reference that the symref is pointing to.
Okay, I better understand now with the example above. Thanks!
--
Cheers,
Toon
^ permalink raw reply
* Re: [PATCH v4 2/5] builtin/refs: add "delete" subcommand
From: Toon Claes @ 2026-07-09 9:44 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Junio C Hamano
In-Reply-To: <20260706-pks-refs-writing-subcommands-v4-2-d51f6ce7f830@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> +test_expect_success 'delete symref with --no-deref verifies target OID' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo repo &&
> + (
> + cd repo &&
> + A=$(git rev-parse A) &&
> + B=$(git rev-parse B) &&
> + git update-ref refs/heads/foo $A &&
> + git symbolic-ref refs/heads/symref refs/heads/foo &&
> +
> + test_must_fail git refs delete --no-deref refs/heads/symref $B &&
> + git refs exists refs/heads/symref &&
> +
> + git refs delete --no-deref refs/heads/symref $A &&
> + test_must_fail git refs exists refs/heads/symref &&
> + git refs exists refs/heads/foo
> + )
So with --no-deref it still checks the dererenced OID, not the target
ref? I expected it to work like this:
test_expect_success 'delete symref with --no-deref verifies target ref' '
test_when_finished "rm -rf repo" &&
setup_repo repo &&
(
cd repo &&
A=$(git rev-parse A) &&
B=$(git rev-parse B) &&
git update-ref refs/heads/foo $A &&
git symbolic-ref refs/heads/symref refs/heads/foo &&
test_must_fail git refs delete --no-deref refs/heads/symref refs/heads/bar &&
git refs exists refs/heads/symref &&
git refs delete --no-deref refs/heads/symref refs/heads/foo &&
test_must_fail git refs exists refs/heads/symref &&
git refs exists refs/heads/foo
)
--
Cheers,
Toon
^ permalink raw reply
* [PATCH 11/11] shallow: fix NULL dereference
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
After `write_one_shallow()` calls `lookup_commit()` to find the commit
object for a shallow graft entry, it then checks `if (!c || ...)`.
Inside that block, when the VERBOSE flag is set, it prints the OID being
removed, via `c->object.oid`. But `c` can be NULL (the first condition
in the `||` check).
This happens when a shallow graft entry references a commit object that
is not in the object store (e.g., after a partial fetch or in a
corrupted repository). In that case, `lookup_commit()` returns NULL
because the object cannot be found, the SEEN_ONLY check correctly
decides to remove this entry from .git/shallow, but the verbose message
crashes before the removal can complete.
Use `graft->oid` instead of `c->object.oid` for the message. The graft
entry's OID is the same value (it was used as the lookup key) and is
always available regardless of whether the commit object exists.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
shallow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/shallow.c b/shallow.c
index 07cae44ae5..3d2230351e 100644
--- a/shallow.c
+++ b/shallow.c
@@ -371,7 +371,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
if (!c || !(c->object.flags & SEEN)) {
if (data->flags & VERBOSE)
printf("Removing %s from .git/shallow\n",
- oid_to_hex(&c->object.oid));
+ oid_to_hex(&graft->oid));
return 0;
}
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 10/11] bisect: ensure non-NULL `head` before using it
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `refs_resolve_ref_unsafe()` is called to resolve HEAD, and returns
NULL (e.g., HEAD does not exist as a proper ref), the code falls back to
`repo_get_oid("HEAD")` to try to resolve the OID directly. If that
succeeds, execution continues with `head` still set to NULL.
Later, that variable is passed to `repo_get_oid()` and `starts_with()`,
both of which would dereference the NULL pointer.
The scenario "`refs_resolve_ref_unsafe()` returns NULL but
`repo_get_oid()` succeeds" can happen when HEAD is a detached bare OID
that the ref backend cannot resolve symbolically (a potential edge case
with the reftable backend) but the OID itself is valid. In this case,
the bisect-start file does not yet exist (this is a fresh "git bisect
start"), so the else branch is taken with the NULL `head`.
Simply assign "HEAD" to `head` as a fallback to address this.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/bisect.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/bisect.c b/builtin/bisect.c
index 6ff600c856..a69771c6d3 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -811,9 +811,11 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
*/
head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
"HEAD", 0, &head_oid, &flags);
- if (!head)
+ if (!head) {
if (repo_get_oid(the_repository, "HEAD", &head_oid))
return error(_("bad HEAD - I need a HEAD"));
+ head = "HEAD";
+ }
/*
* Check if we are bisecting
--
gitgitgadget
^ permalink raw reply related
* [PATCH 09/11] pack-bitmap: handle missing bitmap for base MIDX
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `prepare_midx_bitmap_git()` is called to load the bitmap for a
chained MIDX's base layer, if the base MIDX does not have an associated
bitmap file (e.g., it was not generated, or was deleted by gc), the
return value is NULL. It is then stored in `bitmap_git->base` and
immediately dereferenced on the next line.
This can happen in practice with incremental MIDX chains: the base MIDX
may have been written without `--write-bitmap-index`, or the bitmap may
have been pruned while the incremental layer's bitmap still references
it.
Check the return value and go to the cleanup label (which unmaps the
current bitmap and returns -1) so the caller falls back to non-bitmap
object enumeration, matching the handling of other bitmap loading
failures in the same function.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
pack-bitmap.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index e8a82945cc..ca7998c10b 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -523,6 +523,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
if (midx->base_midx) {
bitmap_git->base = prepare_midx_bitmap_git(midx->base_midx);
+ if (!bitmap_git->base) {
+ warning(_("could not open bitmap for base MIDX"));
+ goto cleanup;
+ }
bitmap_git->base_nr = bitmap_git->base->base_nr + 1;
} else {
bitmap_git->base_nr = 0;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 08/11] revision: avoid dereferencing NULL in `add_parents_only()`
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
This function resolves revision suffixes like commit^@ (all parents),
commit^! (commit minus parents), and commit^-N (exclude Nth parent). It
calls `get_reference()` in a loop to peel through tag objects until it
reaches a commit.
The existing NULL check after `get_reference()` only handles the
ignore_missing case, but get_reference() can return NULL through three
distinct paths:
1. revs->ignore_missing: the caller asked to silently skip missing
objects.
2. revs->exclude_promisor_objects: the object is a lazy promisor
object that should be excluded from the walk.
3. revs->do_not_die_on_missing_objects: the caller wants to record
missing OIDs for later reporting (used by `git rev-list
--missing=print`) rather than dying.
In the latter two instances, the code falls through to dereference the
NULL pointer.
Handle all three cases explicitly:
- ignore_missing: return 0, matching the existing behavior and
the pattern in `handle_revision_arg()`.
- do_not_die_on_missing_objects: return 0. The missing OID has already
been recorded in `revs->missing_commits` by `get_reference()`.
Returning 0 is consistent with `handle_revision_arg()` and
`process_parents()`, both of which continue without error when this flag
is set. The broader codebase pattern for this flag is "record and
continue": list-objects.c, builtin/rev-list.c, and process_parents
all skip the die/error and keep walking.
- everything else (only the `exclude_promisor_objects` case in
practice): return -1, consistent with `handle_revision_arg()` where
the condition only matches `ignore_missing` or
`do_not_die_on_missing_objects`, falling through to ret = -1 for the
promisor case.
Note: the callers of `add_parents_only()` in
`handle_revision_pseudo_opt()` treat any nonzero return as "handled"
(`if (add_parents_only(...)) { ret = 0; }`), so the -1 for the promisor
case is indistinguishable from success there. This means a
promisor-excluded tag target referenced via commit^@ would be silently
skipped rather than producing an error. This is a pre-existing
limitation of the caller's return value handling and not made worse by
this change; the alternative (a NULL dereference crash) _would be_
strictly worse.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
revision.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index e91d7e1f11..7f3999b551 100644
--- a/revision.c
+++ b/revision.c
@@ -1903,8 +1903,13 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
return 0;
while (1) {
it = get_reference(revs, arg, &oid, 0);
- if (!it && revs->ignore_missing)
- return 0;
+ if (!it) {
+ if (revs->ignore_missing)
+ return 0;
+ if (revs->do_not_die_on_missing_objects)
+ return 0;
+ return -1;
+ }
if (it->type != OBJ_TAG)
break;
if (!((struct tag*)it)->tagged)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 07/11] replay: die when --onto does not peel to a commit
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `peel_committish()` function calls `repo_peel_to_type()` to convert
the given object to a commit, but does not check the return value. When
the object exists but cannot be peeled to a commit (e.g., a tree or blob
OID is passed as --onto), the return value is NULL. Add an explicit NULL
check and die with a descriptive message in that case.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
replay.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/replay.c b/replay.c
index da531d5bc6..b38cd5efe4 100644
--- a/replay.c
+++ b/replay.c
@@ -36,12 +36,16 @@ static struct commit *peel_committish(struct repository *repo,
{
struct object *obj;
struct object_id oid;
+ struct commit *commit;
if (repo_get_oid(repo, name, &oid))
die(_("'%s' is not a valid commit-ish for %s"), name, mode);
obj = parse_object_or_die(repo, &oid, name);
- return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
- OBJ_COMMIT);
+ commit = (struct commit *)repo_peel_to_type(repo, name, 0, obj,
+ OBJ_COMMIT);
+ if (!commit)
+ die(_("'%s' does not point to a commit for %s"), name, mode);
+ return commit;
}
static char *get_author(const char *message)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 06/11] bisect: handle NULL commit in `bisect_successful()`
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `lookup_commit_reference_by_name()` is called to find the first bad
commit, the result is passed to `repo_format_commit_message()`
immediately, which dereferences commit without checking for NULL.
However, the commit could be NULL, even though in practice this is
unlikely because `bisect_successful()` is only called after a successful
bisect run has identified the bad commit, but the ref could still become
dangling due to a concurrent gc or repository corruption.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/bisect.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/builtin/bisect.c b/builtin/bisect.c
index e7c2d2f3bb..6ff600c856 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -663,6 +663,11 @@ static int bisect_successful(struct bisect_terms *terms)
refs_read_ref(get_main_ref_store(the_repository), bad_ref, &oid);
commit = lookup_commit_reference_by_name(bad_ref);
+ if (!commit) {
+ res = error(_("could not find commit for '%s'"), bad_ref);
+ free(bad_ref);
+ return res;
+ }
repo_format_commit_message(the_repository, commit, "%s", &commit_name,
&pp);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 05/11] mailsplit: move NULL check before first use of file handle
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `split_mbox()` function calls fileno(f) to check whether the input
is a terminal, but the NULL check for f (from `fopen()`) does not happen
until later. When the file cannot be opened, f is NULL, and
`fileno(NULL)` is undefined behavior, typically crashing with a
segmentation fault.
Move the NULL check above the `isatty()`/`fileno()` call so the error
path is taken before any use of the potentially-NULL handle.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/mailsplit.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 264df6259a..0993418e63 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -225,14 +225,14 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "r");
int file_done = 0;
- if (isatty(fileno(f)))
- warning(_("reading patches from stdin/tty..."));
-
if (!f) {
error_errno("cannot open mbox %s", file);
goto out;
}
+ if (isatty(fileno(f)))
+ warning(_("reading patches from stdin/tty..."));
+
do {
peek = fgetc(f);
if (peek == EOF) {
--
gitgitgadget
^ permalink raw reply related
* [PATCH 04/11] reftable/stack: guard against NULL list_file in stack_destroy
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When reftable_new_stack() fails partway through initialization
(e.g., reftable_buf_addstr returns an OOM error before
reftable_buf_detach assigns p->list_file), it jumps to the error
path which calls reftable_stack_destroy(p). At that point,
p->list_file is still NULL because the detach never happened.
reftable_stack_destroy() passes st->list_file unconditionally to
read_lines(), which calls open(filename, O_RDONLY). Passing NULL
to open() is undefined behavior and will typically crash.
Guard the read_lines() call with a NULL check on st->list_file.
When list_file is NULL, there are no table files to clean up
anyway, so skipping read_lines is the correct behavior.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
reftable/stack.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 1fba96ddb3..3fc3c0b2d1 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -171,7 +171,8 @@ void reftable_stack_destroy(struct reftable_stack *st)
st->merged = NULL;
}
- err = read_lines(st->list_file, &names);
+ if (st->list_file)
+ err = read_lines(st->list_file, &names);
if (err < 0) {
REFTABLE_FREE_AND_NULL(names);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 03/11] remote: guard `remote_tracking()` against NULL remote
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `remote_tracking()` function unconditionally dereferences
`remote->fetch` without checking whether remote is NULL.
In practice, this never happens because the only caller (`apply_cas()`)
guards the calls to this function by checking the `use_tracking` and
`use_tracking_for_rest` attributes.
However, it requires quite involved reasoning to reach that conclusion,
and is therefore fragile. Just return -1 ("no tracking ref") when there
is no remote to work with.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
remote.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/remote.c b/remote.c
index 00723b385e..34d0367f11 100644
--- a/remote.c
+++ b/remote.c
@@ -2681,6 +2681,8 @@ static int remote_tracking(struct remote *remote, const char *refname,
{
char *dst;
+ if (!remote)
+ return -1; /* no remote to look up tracking ref */
dst = apply_refspecs(&remote->fetch, refname);
if (!dst)
return -1; /* no tracking ref for refname at remote */
--
gitgitgadget
^ permalink raw reply related
* [PATCH 02/11] diff: handle NULL return from repo_get_commit_tree()
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `repo_get_commit_tree()` function can return NULL when a commit's
tree object is not available (e.g., the commit was parsed but its
maybe_tree field is unset and the commit is not in the commit-graph). In
cmd_diff(), the return value is immediately dereferenced via ->object
without a NULL check, which would crash if the tree cannot be loaded.
Add an explicit NULL check and die with a descriptive message.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/diff.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index 4b46e394ce..18b1083e98 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -579,9 +579,13 @@ int cmd_diff(int argc,
obj = deref_tag(the_repository, obj, NULL, 0);
if (!obj)
die(_("invalid object '%s' given."), name);
- if (obj->type == OBJ_COMMIT)
- obj = &repo_get_commit_tree(the_repository,
- ((struct commit *)obj))->object;
+ if (obj->type == OBJ_COMMIT) {
+ struct tree *tree = repo_get_commit_tree(
+ the_repository, (struct commit *)obj);
+ if (!tree)
+ die(_("unable to read tree object for commit '%s'"), name);
+ obj = &tree->object;
+ }
if (obj->type == OBJ_TREE) {
if (sdiff.skip && bitmap_get(sdiff.skip, i))
--
gitgitgadget
^ permalink raw reply related
* [PATCH 01/11] diffcore-break: guard against NULLed queue entries in merge loop
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The outer loop in `diffcore_merge_broken()` sets `q->queue[j]` to NULL
when it merges a broken pair back together, and has a NULL check to skip
such entries on subsequent iterations. The inner loop, however, lacks
this guard: when it scans forward looking for a matching peer, it can
encounter a slot that was NULLed by a previous outer-loop iteration and
dereference it unconditionally.
In practice this requires at least two broken pairs whose peers
both survive rename/copy detection and appear later in the queue,
which is rare but not impossible.
Add the same `if (!pp) continue` guard to the inner loop.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
diffcore-break.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/diffcore-break.c b/diffcore-break.c
index 17b5ad1fed..b5bcc956cc 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -289,6 +289,8 @@ void diffcore_merge_broken(void)
*/
for (j = i + 1; j < q->nr; j++) {
struct diff_filepair *pp = q->queue[j];
+ if (!pp)
+ continue;
if (pp->broken_pair &&
!strcmp(pp->one->path, pp->two->path) &&
!strcmp(p->one->path, pp->two->path)) {
--
gitgitgadget
^ permalink raw reply related
* [PATCH 00/11] coverity: avoid dereferencing NULL
From: Johannes Schindelin via GitGitGadget @ 2026-07-09 9:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
This is a continuation of the effort I started in the patch series that
became js/coverity-fixes. This next batch adds guards to avoid dereferencing
NULL pointers and accessing NULL file descriptors.
Johannes Schindelin (11):
diffcore-break: guard against NULLed queue entries in merge loop
diff: handle NULL return from repo_get_commit_tree()
remote: guard `remote_tracking()` against NULL remote
reftable/stack: guard against NULL list_file in stack_destroy
mailsplit: move NULL check before first use of file handle
bisect: handle NULL commit in `bisect_successful()`
replay: die when --onto does not peel to a commit
revision: avoid dereferencing NULL in `add_parents_only()`
pack-bitmap: handle missing bitmap for base MIDX
bisect: ensure non-NULL `head` before using it
shallow: fix NULL dereference
builtin/bisect.c | 9 ++++++++-
builtin/diff.c | 10 +++++++---
builtin/mailsplit.c | 6 +++---
diffcore-break.c | 2 ++
pack-bitmap.c | 4 ++++
reftable/stack.c | 3 ++-
remote.c | 2 ++
replay.c | 8 ++++++--
revision.c | 9 +++++++--
shallow.c | 2 +-
10 files changed, 42 insertions(+), 13 deletions(-)
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2174%2Fdscho%2Fcoverity-fixes-null-safety-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2174/dscho/coverity-fixes-null-safety-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2174
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v3 00/11] receive-pack: use ODB transactions to stage object writes
From: Patrick Steinhardt @ 2026-07-09 9:39 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, gitster
In-Reply-To: <20260708235925.3992097-1-jltobler@gmail.com>
On Wed, Jul 08, 2026 at 06:59:14PM -0500, Justin Tobler wrote:
> Changes since V2:
> - Clarified commit log reasoning for embedding
> `flush_loose_object_transaction()` logic in commit function.
> - Started printed some error messages on transaction errors.
> - Removed include statement.
> - Fixed transaction leak on `odb_transaction_commit()` error.
Thanks, the changes all look good to me and I don't have anything else
to add.
Patrick
^ permalink raw reply
* Re: [PATCH v2 06/11] odb/transaction: propagate begin errors
From: Patrick Steinhardt @ 2026-07-09 9:39 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, gitster
In-Reply-To: <ak57VEF56HkRKygQ@denethor>
On Wed, Jul 08, 2026 at 11:56:43AM -0500, Justin Tobler wrote:
> On 26/07/08 08:41AM, Patrick Steinhardt wrote:
> > On Tue, Jul 07, 2026 at 11:14:07PM -0500, Justin Tobler wrote:
> > > @@ -490,10 +491,12 @@ int cache_tree_update(struct index_state *istate, int flags)
> > >
> > > trace_performance_enter();
> > > trace2_region_enter("cache_tree", "update", istate->repo);
> > > - transaction = odb_transaction_begin(the_repository->objects);
> > > + if (!inflight)
> > > + odb_transaction_begin_or_die(the_repository->objects, &transaction);
> > > i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
> > > "", 0, &skip, flags);
> > > - odb_transaction_commit(transaction);
> > > + if (!inflight)
> > > + odb_transaction_commit(transaction);
> > > trace2_region_leave("cache_tree", "update", istate->repo);
> > > trace_performance_leave("cache_tree_update");
> > > if (i < 0)
> >
> > Callsites like this really make me wonder why we even care to create
> > a transaction in the first place if we basically just commit it
> > immediately anyway. And while it's a bit sad that we have so many sites
> > where we don't really know whether we even have a transaction, I think
> > it's a good change that we have now annotated them clearly. A subsequent
> > patch series may then eventually refactor those sites so that we stop
> > depending on `odb->transaction` and inject the transaction via a
> > parameter.
>
> Call sites like the one mentioned above are using ODB transactions as an
> optimization to batch the full fsyncs in bulk. In cases where the is not
> already a transaction, they start one to take advantage of it.
I know. But in the case where we don't want to batch we create the
transaction anyway as far as I can see, and then we commit it
immediately. So arguably, we could've just `odb_write_object()` and call
it a day.
> I fully agree though that an ODB transaction should ideally be started
> at a higher layer and wired down to these call sites. I have a couple of
> patches in my tree that start to tackle this which I plan to send in
> another series. :)
Yeah, let's not worry about that too much for now then. One step at a
time :)
> > > @@ -36,11 +38,21 @@ struct odb_transaction {
> > > };
> > >
> > > /*
> > > - * Starts an ODB transaction. Subsequent objects are written to the transaction
> > > - * and not committed until odb_transaction_commit() is invoked on the
> > > - * transaction. If the ODB already has a pending transaction, NULL is returned.
> > > + * Starts an ODB transaction and returns it via `out`. Subsequent objects are
> > > + * written to the transaction and not committed until odb_transaction_commit()
> > > + * is invoked on the transaction. Returns 0 on success and a negative value on
> > > + * error. Note that it is considered an error to start a new transaction if the
> > > + * ODB already has an inflight transaction pending.
> > > */
> > > -struct odb_transaction *odb_transaction_begin(struct object_database *odb);
> > > +int odb_transaction_begin(struct object_database *odb,
> > > + struct odb_transaction **out);
> > > +
> > > +static inline void odb_transaction_begin_or_die(struct object_database *odb,
> > > + struct odb_transaction **out)
> > > +{
> > > + if (odb_transaction_begin(odb, out))
> > > + die(_("failed to start ODB transaction"));
> > > +}
> >
> > We could make it a bit simpler to use this function by continuing to
> > return the transaction directly. But on the other hand this results in a
> > more consistent interface.
>
> Ya, I was a bit back and forth about this myself. I ultimately landed on
> keeping a more consistent interface though. Happy to change if others
> feel differently though.
As said, I can see both arguments. And ultimately, I don't care too
much, so it's fine if this is just kept as-is.
Patrick
^ permalink raw reply
* Re: [PATCH v3 03/11] object-file: embed transaction flush logic in commit function
From: Patrick Steinhardt @ 2026-07-09 9:38 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, gitster
In-Reply-To: <20260708235925.3992097-4-jltobler@gmail.com>
On Wed, Jul 08, 2026 at 06:59:17PM -0500, Justin Tobler wrote:
> When a "files" transaction is committed,
> `flush_loose_object_transaction()` is invoked to handle performing a
> hardware flush along with migrating the temporary object directory into
> the primary and configuring the repository ODB source accordingly. The
> function name here is a bit misleading because the helper is doing a bit
> more than just "flushing" the transaction contents. Also, in a
> subsequent commit, the transaction temporary directory is used to stage
> packfiles and not just loose objects anymore.
>
> Lift the helper function logic directly into
> `odb_transaction_files_commit()` to more accurately signal to readers
> the operation being performed.
This line break makes my eyes bleed.
Patrick
^ permalink raw reply
* Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite [and 1 more messages]
From: Ian Jackson @ 2026-07-09 9:36 UTC (permalink / raw)
To: Colin Stagner; +Cc: git, Johannes Schindelin
In-Reply-To: <9ef8cfcc-ab47-479b-9f23-71ba99e1e56b@howdoi.land>
Hi. Thanks for the review. I'll go through it point by point:
Colin Stagner writes ("Re: [PATCH 2/2] git-subtree: Bail out if we find output from Rust rewrite (test)"):
> It may be slightly faster to create only one repo and just make orphan
> branches, like `test_create_subtree_add()` does.
...
> `test_commit()` from test-lib-functions.sh may be superior to manually
> writing and committing this file.
Thanks for the suggestions. I'll take a look.
TBH I found this test framework quite awkward to work with. Maybe
folks here have some tips:
One thing I was missing was a primitive for "check this fails *and
produces an error message matching this regexp*". test_must_fail
makes it easy for a slips in the command (or some kinds of regression)
to go undetected: the test then passes because the command *does* fail
with a usage error or whatever. And AFAICT there isn't a way to
manually inspect the output when the tests pass? I resorted to
sabotaging the test by adding `&& false` to the end of the shell
snippet string, and eyeballing t/test-results/t7900-subtree.out.
Colin Stagner writes ("Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite"):
> > +reject_if_v2_config () {
> > + local config=.git-subtree/config
>
> This is a nit, but `local` is not specified by POSIX. I know it is used
> elsewhere within git-subtree, but it is specifically discouraged.
There are 7 existing uses of `local`. I think I prefer to use it here
too. In practice I think there are no shells we might want to use
that don't have local. The alternative is to change all the variable
names to be obviously globally unique, which is clumsy and also seems
to me to put us at greater risk of bugs.
> > + if git rev-parse --verify -q "$rev:$config"; then
>
> For subtree split, should we also test for this file in tree you are
> splitting: i.e., "$dir/$config"? The answer might be no.
You're right that we should consider this question. The answer is:
no, we should not. Briefly, whether to use the new or old algorithms
depends on whether the downstream has adopted the new git-subtree, not
on whether the upstream has added some optional config.
https://codeberg.org/diziet/git-subtree/src/branch/main/DATA-MODEL.md#control-of-unmarked-subtree-merges-guessing-config
> I think that subtree merge should only test the top-level project, as
> this patch does now.
By "top-level" I think you mean what I've taken to calling the
"downstream": the project where the subtree is in a subdir, and whose
top-level has other stuff. In which case I agree.
> On 7/6/26 06:58, Ian Jackson wrote:
> > Another, bigger, reason is that current git-subtree generates unmarked
> > subtree merges (ie, without any git-subtree trailers)
>
> Subtree merges can be performed without git-subtree, via the `-X
> subtree` merge strategy option. While the design of RIIR git-subtree is
> outside the scope of this patch series, this may be worth thinking about
> in your rewrite.
This is what I'm calling an "unmarked subtree merge". My rewrite is
not going to support this user behaviour. The problem is that it is
not possible to reliably determine whetheer something is an unmarked
subtree merge.
It is possible to guess based on tree similarity, but that's a
heuristic. It's also possible to guess based on root commits.
Both of these approaches can go wrong in some cases. I prefer to
write reliable software, which doesn't guess.
I'll advise against this practice in the documentation, but I'm
reasonably confident that if a user does this anyway the results won't
be terrible. The upstream input to an unmarked subtree merge in a
downstream that has already used my rewrite, will be treated as if it
were a downstream branch that predates the subtree addition. The
effect on split (in most cases) is a missing parent relationship,
which is undesirable but not catastrophic.I've made a note to add a
test case for this scenario.
Combining manual -X subtree merges with git-subtree --squash merges
could easily produce quite weird and wrong results in the tree (even
before anyone tries split, or something). I don't think I can even
reliably detect this situation after the user has done it, and of
course since that user is using plain git, I certainly can't prevent
it. This is another reason why manual use of -X subtree should be
discouraged.
Regards,
Ian.
--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk,
that is a private address which bypasses my fierce spamfilter.
^ permalink raw reply
* [PATCH 7/7] builtin/cat-file: filter objects via object database
From: Patrick Steinhardt @ 2026-07-09 8:35 UTC (permalink / raw)
To: git
In-Reply-To: <20260709-pks-odb-for-each-object-filter-v1-0-82fe014b12b3@pks.im>
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.175.ge4962bd3d5.dirty
^ permalink raw reply related
* [PATCH 6/7] odb: introduce object filters to `odb_for_each_object()`
From: Patrick Steinhardt @ 2026-07-09 8:35 UTC (permalink / raw)
To: git
In-Reply-To: <20260709-pks-odb-for-each-object-filter-v1-0-82fe014b12b3@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.
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 5d2af96e2f..ac9da9545f 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.175.ge4962bd3d5.dirty
^ permalink raw reply related
* [PATCH 5/7] pack-bitmap: introduce function to open bitmap for a single source
From: Patrick Steinhardt @ 2026-07-09 8:35 UTC (permalink / raw)
To: git
In-Reply-To: <20260709-pks-odb-for-each-object-filter-v1-0-82fe014b12b3@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 0e3e18a557..5d2af96e2f 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.175.ge4962bd3d5.dirty
^ permalink raw reply related
* [PATCH 4/7] pack-bitmap: iterate object sources when opening bitmaps
From: Patrick Steinhardt @ 2026-07-09 8:35 UTC (permalink / raw)
To: git
In-Reply-To: <20260709-pks-odb-for-each-object-filter-v1-0-82fe014b12b3@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 | 65 ++++++++++++++++++++++++++---------------------------------
1 file changed, 29 insertions(+), 36 deletions(-)
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;
+
+ 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())
+ break;
+
+ if (open_pack_bitmap_1(bitmap_git, e->pack))
+ continue;
+ ret = 0;
}
return ret;
}
-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;
+ int found = 0;
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);
+ found |= !open_bitmap_for_source(files->packed, bitmap_git);
- 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.175.ge4962bd3d5.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