* 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
* [PATCH 3/7] pack-bitmap: allow aborting iteration of bitmapped objects
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>
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.175.ge4962bd3d5.dirty
^ permalink raw reply related
* [PATCH 2/7] pack-bitmap: mark object filter as `const`
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()` 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.175.ge4962bd3d5.dirty
^ permalink raw reply related
* [PATCH 1/7] odb/source-packed: improve lookup when enumerating objects
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 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.
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.
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.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