* Re: [PATCH 7/9] http: discard hash in dumb-http http_object_request
From: Patrick Steinhardt @ 2026-07-03 11:27 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20260702080707.GG2029434@coredump.intra.peff.net>
On Thu, Jul 02, 2026 at 04:07:07AM -0400, Jeff King wrote:
> The flag handling could be removed if the hash-discard function were
> idempotent. This could be done easily-ish by having the underlying
> hash functions (like the ones in sha256/openssl.h) set the context
> pointer to NULL after free-ing. But it's something that every platform
> implementation would have to remember to do, and the benefit for the
> callers is not that huge (it would let us shave a few lines here and
> probably in a few other spots).
This answers an earlier question of mine. It would indeed be great if it
was idempotent -- I've been bitten by interfaces like this once too
much, where you have to be very careful to manage the lifetime of a
specific object. The prime example of this are (were? I don't quite
recall whether we fixed that interface) reference transactions, and that
caused a bunch of bugs in the past.
Patrick
^ permalink raw reply
* Re: [PATCH 4/9] csum-file: provide a function to release checkpoints
From: Patrick Steinhardt @ 2026-07-03 11:27 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20260702080319.GD2029434@coredump.intra.peff.net>
On Thu, Jul 02, 2026 at 04:03:19AM -0400, Jeff King wrote:
> A hashfile_checkpoint struct is basically just a copy of the hash_ctx
> state at a given point in the file. As such, it contains its own
> git_hash_ctx which may (depending on the underlying hash implementation)
> need to be discarded when we're done with it.
>
> Let's add a "release" function which cleans up the hash context it
> holds. I chose "release" here and not "discard" because you'd use this
> to clean up every checkpoint, whether you used it or not. As opposed to
> git_hash_discard(), which is needed only if you didn't call
> git_hash_final().
Okay, I was wondering about that a bit. With this explanation I'm also
somewhat fine with the `git_hash_discard()` name. It's still a function
that has release semantics, but you want to convey more intent than
that.
One thing I was wondering: is it safe to have a `git_hash_discard()`
that is being called on a potentially-already-discarded hash? If so, we
wouldn't have to discern whether the hash context was used successfully
or not.
Patrick
^ permalink raw reply
* Re: [PATCH 2/9] hash: add discard primitive
From: Patrick Steinhardt @ 2026-07-03 11:27 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20260702075953.GB2029434@coredump.intra.peff.net>
On Thu, Jul 02, 2026 at 03:59:53AM -0400, Jeff King wrote:
> diff --git a/hash.c b/hash.c
> index e925b9754e..63672a3d22 100644
> --- a/hash.c
> +++ b/hash.c
> @@ -283,6 +283,18 @@ void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
> ctx->algop->final_oid_fn(oid, ctx);
> }
>
> +void git_hash_discard(struct git_hash_ctx *ctx)
As the resident naming czar: shouldn't this rather be called
`git_hash_release()`?
Patrick
^ permalink raw reply
* Re: [PATCH 1/9] csum-file: drop discard_hashfile()
From: Patrick Steinhardt @ 2026-07-03 11:27 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20260702210601.GA2051171@coredump.intra.peff.net>
On Thu, Jul 02, 2026 at 05:06:01PM -0400, Jeff King wrote:
> On Thu, Jul 02, 2026 at 11:19:04AM -0700, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > So now we have two functions, discard_hashfile() and free_hashfile(),
> > > and we only need one. Which one do we want to keep?
> > >
> > > The only difference between them is that the discard variant also closes
> > > the descriptors held in the struct. Let's look at the three callers:
> > > ...
> > > Note that I said "descriptors" plural above. Those callers all care
> > > about the "fd" member of the struct. But discard_hashfile() also closes
> > > check_fd. That is only used if the struct is initialized with
> > > hashfd_check(), and neither of its two callers call either discard or
> > > free (they always "finalize" instead). So closing it is irrelevant for
> > > the current callers.
> > >
> > > I think we're better off sticking with the simpler free_hashfile()
> > > interface, and the handful of callers can decide how to handle the
> > > descriptors themselves.
> >
> > Sonds good.
> >
> > Our resident naming czar (already Cc'ed) may have preference about
> > the names and word order, though ;-)
>
> Heh, yes, it should be hashfile_free() but that would require changing
> the whole interface. We could do that on top, which might also be a good
> time to do s/free/discard/ without worrying about a subtle behavior
> change.
Heh :P
I think this being called a "free" function makes perfect sense, because
ultimately that's all we do here. So the semantics align with other free
functions.
We could of course fix the ordering while at it, but I don't want to
tack that onto this series. It already makes the codebase a better
place, so I'm happy enough.
Patrick
^ permalink raw reply
* Re: [PATCH v6 2/2] config: add "worktree" and "worktree/i" includeIf conditions
From: Patrick Steinhardt @ 2026-07-03 11:02 UTC (permalink / raw)
To: me; +Cc: git, Kristoffer Haugsbakk, Junio C Hamano, Phillip Wood
In-Reply-To: <20260703-includeif-worktree-v6-2-a13893ad9a7f@black-desk.cn>
On Fri, Jul 03, 2026 at 11:13:18AM +0800, Chen Linxuan via B4 Relay wrote:
> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index f3892578e4ff..4e840dfdb35b 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -396,4 +396,132 @@ test_expect_success 'onbranch without repository but explicit nonexistent Git di
[snip]
> +test_expect_success SYMLINKS 'conditional include, worktree resolves symlinks' '
> + mkdir real-wt &&
> + ln -s real-wt link-wt &&
> + git init link-wt/repo &&
> + (
> + cd link-wt/repo &&
> + # repo->worktree resolves symlinks, so use real path in pattern
> + echo "[includeIf \"worktree:**/real-wt/repo\"]path=bar-link" >>.git/config &&
> + echo "[test]wtlink=2" >.git/bar-link &&
> + echo 2 >expect &&
> + git config test.wtlink >actual &&
> + test_cmp expect actual
> + )
> +'
Okay, this covers one scenario. But with "gitdir:" we're actually able
to use both the symlinked and the real location:
test_expect_success SYMLINKS 'conditional include, worktree matching symlink' '
mkdir sym-real &&
ln -s sym-real sym-link &&
git init sym-link/repo &&
(
cd sym-link/repo &&
link_path="$(pwd)" &&
real_path="$(test-tool path-utils real_path "$link_path")" &&
cat >>.git/config <<-EOF &&
[includeIf "gitdir:$link_path/.git"]
path = gitdir-link
[includeIf "gitdir:$real_path/.git"]
path = gitdir-real
[includeIf "worktree:$link_path"]
path = worktree-link
[includeIf "worktree:$real_path"]
path = worktree-real
EOF
echo "[test]gitdirlink=1" >.git/gitdir-link &&
echo "[test]gitdirreal=1" >.git/gitdir-real &&
echo "[test]worktreelink=1" >.git/worktree-link &&
echo "[test]worktreereal=1" >.git/worktree-real &&
git config get test.gitdirlink &&
git config get test.gitdirreal &&
git config get test.worktreereal &&
test_must_fail git config test.worktreelink
)
'
The last call to git-config(1) fails, which is inconsistent with how
resolve the path for "gitdir".
Other than that I didn't have anything to add, thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v3 2/5] builtin/refs: add "delete" subcommand
From: Toon Claes @ 2026-07-03 10:54 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Junio C Hamano
In-Reply-To: <20260630-pks-refs-writing-subcommands-v3-2-deb04de1ecef@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Reference-related functionality in Git is currently spread across many
> different commands: git-update-ref(1), git-for-each-ref(1),
> git-show-ref(1), git-pack-refs(1) and git-symbolic-ref(1). This makes it
> hard for users to discover what functionality we have available to work
> with references.
>
> We have thus started to consolidate this functionality into git-refs(1),
> which is a toolbox of everything related to references. Until now, the
> command doesn't handle functionality of git-update-ref(1).
>
> Fix this gap by introducing a new "delete" subcommand, which is the
> equivalent of `git update-ref -d`.
>
> Note that we're intentionally not using a generic "write" subcommand
> with a "-d" flag. This is rather harder to discover, and subcommands
> that are implmented as flags tend to be hard to reason about in the code
> as we'd have to handle mutually-exclusive flags that stem from the other
> subcommand-like modes.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> Documentation/git-refs.adoc | 17 ++++++
> builtin/refs.c | 51 +++++++++++++++++
> t/meson.build | 1 +
> t/t1464-refs-delete.sh | 130 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 199 insertions(+)
>
> diff --git a/Documentation/git-refs.adoc b/Documentation/git-refs.adoc
> index fa33680cc7..2633934463 100644
> --- a/Documentation/git-refs.adoc
> +++ b/Documentation/git-refs.adoc
> @@ -20,6 +20,7 @@ git refs list [--count=<count>] [--shell|--perl|--python|--tcl]
> [ --stdin | (<pattern>...)]
> git refs exists <ref>
> git refs optimize [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]
> +git refs delete [--message=<reason>] [--no-deref] <ref> [<old-value>]
>
> DESCRIPTION
> -----------
> @@ -51,6 +52,12 @@ optimize::
> usage. This subcommand is an alias for linkgit:git-pack-refs[1] and
> offers identical functionality.
>
> +delete::
> + Delete the given reference. This subcommand mirrors `git update-ref -d`
> + (see linkgit:git-update-ref[1]). When `<old-value>` is given, the
> + reference is only deleted after verifying that it currently contains
> + `<old-value>`.
> +
> OPTIONS
> -------
>
> @@ -90,6 +97,16 @@ The following options are specific to 'git refs optimize':
>
> include::pack-refs-options.adoc[]
>
> +The following options are specific to commands which write references:
> +
> +`--message=<reason>`::
> + Use the given <reason> string for the reflog entry associated with the
> + update. An empty message is rejected.
> +
> +`--no-deref`::
> + Operate on <ref> itself rather than the reference it points to via a
> + symbolic ref.
> +
> KNOWN LIMITATIONS
> -----------------
>
> diff --git a/builtin/refs.c b/builtin/refs.c
> index f0faabf45a..edb7d61663 100644
> --- a/builtin/refs.c
> +++ b/builtin/refs.c
> @@ -21,6 +21,9 @@
> #define REFS_OPTIMIZE_USAGE \
> N_("git refs optimize " PACK_REFS_OPTS)
>
> +#define REFS_DELETE_USAGE \
> + N_("git refs delete [--message=<reason>] [--no-deref] <ref> [<old-value>]")
> +
> static int cmd_refs_migrate(int argc, const char **argv, const char *prefix,
> struct repository *repo)
> {
> @@ -175,6 +178,52 @@ 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_delete(int argc, const char **argv, const char *prefix,
> + struct repository *repo)
> +{
> + static char const * const refs_delete_usage[] = {
> + REFS_DELETE_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),
Would it make sense to allow both --deref and --no-deref? (and --deref
being the default)
> + OPT_END(),
> + };
> + struct object_id oldoid;
> + const char *refname;
> + int ret;
> +
> + argc = parse_options(argc, argv, prefix, opts, refs_delete_usage, 0);
> + if (argc < 1 || argc > 2)
> + usage(_("delete requires reference name and an optional old object ID"));
> +
> + if (message && !*message)
> + die(_("refusing to perform update with empty message"));
> +
> + repo_config(repo, git_default_config, NULL);
> +
> + refname = argv[0];
> + if (argc == 2) {
> + if (repo_get_oid_with_flags(repo, argv[1], &oldoid, GET_OID_SKIP_AMBIGUITY_CHECK))
> + die(_("invalid old object ID: '%s'"), argv[1]);
> + if (is_null_oid(&oldoid))
> + die(_("cannot delete reference with null old object ID"));
> + }
> +
> + ret = refs_delete_ref(get_main_ref_store(repo), message, refname,
> + argc == 2 ? &oldoid : NULL, flags);
> +
> + if (ret < 0)
> + ret = 1;
> + return ret;
> +}
> +
> int cmd_refs(int argc,
> const char **argv,
> const char *prefix,
> @@ -186,6 +235,7 @@ int cmd_refs(int argc,
> "git refs list " COMMON_USAGE_FOR_EACH_REF,
> REFS_EXISTS_USAGE,
> REFS_OPTIMIZE_USAGE,
> + REFS_DELETE_USAGE,
> NULL,
> };
> parse_opt_subcommand_fn *fn = NULL;
> @@ -195,6 +245,7 @@ int cmd_refs(int argc,
> OPT_SUBCOMMAND("list", &fn, cmd_refs_list),
> OPT_SUBCOMMAND("exists", &fn, cmd_refs_exists),
> OPT_SUBCOMMAND("optimize", &fn, cmd_refs_optimize),
> + OPT_SUBCOMMAND("delete", &fn, cmd_refs_delete),
> OPT_END(),
> };
>
> diff --git a/t/meson.build b/t/meson.build
> index c5832fee05..1ccf08a3b5 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -223,6 +223,7 @@ integration_tests = [
> 't1461-refs-list.sh',
> 't1462-refs-exists.sh',
> 't1463-refs-optimize.sh',
> + 't1464-refs-delete.sh',
> 't1500-rev-parse.sh',
> 't1501-work-tree.sh',
> 't1502-rev-parse-parseopt.sh',
> diff --git a/t/t1464-refs-delete.sh b/t/t1464-refs-delete.sh
> new file mode 100755
> index 0000000000..efff7d0574
> --- /dev/null
> +++ b/t/t1464-refs-delete.sh
> @@ -0,0 +1,130 @@
> +#!/bin/sh
> +
> +test_description='git refs delete'
> +
> +. ./test-lib.sh
> +
> +setup_repo () {
> + git init "$1" &&
> + test_commit -C "$1" A &&
> + test_commit -C "$1" B
> +}
> +
> +test_expect_success 'delete without oldvalue verification' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo repo &&
> + A=$(git -C repo rev-parse A) &&
> + git -C repo update-ref refs/heads/foo $A &&
> + git -C repo refs delete refs/heads/foo &&
> + test_must_fail git -C repo show-ref --verify -q refs/heads/foo
Why not use `git refs exists` here? And why use `git -C repo` in this
test, and `cd repo` in the other?
> +'
> +
> +test_expect_success 'delete with matching oldvalue' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo repo &&
> + (
> + cd repo &&
> + A=$(git rev-parse A) &&
> + git update-ref refs/heads/foo $A &&
> + git refs delete refs/heads/foo $A &&
> + test_must_fail git refs exists refs/heads/foo
> + )
> +'
> +
> +test_expect_success 'delete with stale oldvalue fails' '
> + 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 &&
> + test_must_fail git refs delete refs/heads/foo $B 2>err &&
> + test_grep " but expected " err &&
> + git refs exists refs/heads/foo
> + )
> +'
> +
> +test_expect_success 'delete with null oldvalue fails' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo repo &&
> + (
> + cd repo &&
> + A=$(git rev-parse A) &&
> + git update-ref refs/heads/foo $A &&
> + test_must_fail git refs delete refs/heads/foo $ZERO_OID 2>err &&
> + test_grep "null old object ID" err &&
> + git refs exists refs/heads/foo
> + )
> +'
> +
> +test_expect_success 'delete with invalid oldvalue fails' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo repo &&
> + (
> + cd repo &&
> + A=$(git rev-parse A) &&
> + git update-ref refs/heads/foo $A &&
> + test_must_fail git refs delete refs/heads/foo invalid-oid 2>err &&
> + test_grep "invalid old object ID" err &&
> + git refs exists refs/heads/foo
> + )
> +'
> +
> +test_expect_success 'delete symref with --no-deref leaves target intact' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo repo &&
> + (
> + cd repo &&
> + A=$(git rev-parse A) &&
> + git update-ref refs/heads/foo $A &&
> + git symbolic-ref refs/heads/symref refs/heads/foo &&
> + git refs delete --no-deref refs/heads/symref &&
> + test_must_fail git refs exists refs/heads/symref &&
> + git refs exists refs/heads/foo
> + )
What happens if you delete a symref and provide an <old-value>?
> +'
> +
> +test_expect_success 'delete with message records reason in reflog' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo repo &&
> + (
> + cd repo &&
> + A=$(git rev-parse A) &&
> + git update-ref refs/heads/foo $A &&
> + git symbolic-ref HEAD refs/heads/foo &&
> + git refs delete --message=delete-reason refs/heads/foo &&
> + test_must_fail git refs exists refs/heads/foo &&
> + test-tool ref-store main for-each-reflog-ent HEAD >actual &&
> + test_grep "delete-reason$" actual
> + )
> +'
> +
> +test_expect_success 'delete with empty message fails' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo repo &&
> + (
> + cd repo &&
> + A=$(git rev-parse A) &&
> + git update-ref refs/heads/foo $A &&
> + test_must_fail git refs delete --message= refs/heads/foo 2>err &&
> + test_grep "empty message" err &&
> + git refs exists refs/heads/foo
> + )
> +'
> +
> +test_expect_success 'delete without arguments fails' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo repo &&
> + test_must_fail git -C repo refs delete 2>err &&
> + test_grep "requires reference name" err
> +'
> +
> +test_expect_success 'delete with too many arguments fails' '
> + test_when_finished "rm -rf repo" &&
> + setup_repo repo &&
> + test_must_fail git refs delete one two three 2>err &&
> + test_grep "requires reference name" err
> +'
> +
> +test_done
>
> --
> 2.55.0.795.g602f6c329a.dirty
>
>
--
Cheers,
Toon
^ permalink raw reply
* Re: [PATCH v2 07/12] reftable/block: fix OOB read with bogus block size
From: Patrick Steinhardt @ 2026-07-03 10:32 UTC (permalink / raw)
To: Toon Claes; +Cc: git, oxsignal, Christian Couder
In-Reply-To: <87wlvc2zii.fsf@emacs.iotcl.com>
On Fri, Jul 03, 2026 at 11:28:21AM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> > index 088162483e..43b9d5fb59 100644
> > --- a/t/unit-tests/u-reftable-block.c
> > +++ b/t/unit-tests/u-reftable-block.c
> > @@ -497,3 +497,36 @@ void test_reftable_block__corrupt_log_block_size(void)
> > reftable_block_release(&block);
> > reftable_buf_release(&data);
> > }
> > +
> > +void test_reftable_block__corrupt_block_size(void)
> > +{
> > + struct reftable_block_source source = { 0 };
> > + struct reftable_record rec = {
> > + .type = REFTABLE_BLOCK_TYPE_REF,
> > + .u.ref = {
> > + .value_type = REFTABLE_REF_VAL1,
> > + .refname = (char *) "refs/heads/main",
> > + },
> > + };
> > + struct reftable_block block = { 0 };
> > + struct reftable_buf data = REFTABLE_BUF_INIT;
> > +
> > + cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
> > +
> > + /*
> > + * The block size is stored as a big-endian 24-bit integer right after
> > + * the one-byte block type at the start of the block. Corrupt it to
> > + * claim a size that is larger than the data we actually have. Reading
> > + * the restart count and restart table relative to such a bogus block
> > + * size must not access out-of-bounds memory.
> > + */
> > + reftable_put_be24((uint8_t *) data.buf + 1, 0xffffff);
>
> Same here, would it make sense to write a size that's `+1` too much?
>
> uint8_t *p = (uint8_t *)data.buf + 1;
> uint32_t block_size = reftable_get_be24(p);
> cl_assert_equal_i(block_size, 47);
> reftable_put_be24(p, block_size + 1);
In this case it'd work indeed though, and I agree that this is a
sensible change.
Patrick
^ permalink raw reply
* Re: [PATCH v2 06/12] reftable/block: fix OOB write with bogus inflated log size
From: Patrick Steinhardt @ 2026-07-03 10:32 UTC (permalink / raw)
To: Toon Claes; +Cc: git, oxsignal, Christian Couder
In-Reply-To: <871pdk4eau.fsf@emacs.iotcl.com>
On Fri, Jul 03, 2026 at 11:23:37AM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> > index f4e926ce3a..088162483e 100644
> > --- a/t/unit-tests/u-reftable-block.c
> > +++ b/t/unit-tests/u-reftable-block.c
> > @@ -465,3 +465,35 @@ void test_reftable_block__iterator(void)
> > reftable_block_release(&block);
> > reftable_buf_release(&data);
> > }
> > +
> > +void test_reftable_block__corrupt_log_block_size(void)
> > +{
> > + struct reftable_block_source source = { 0 };
> > + struct reftable_record rec = {
> > + .type = REFTABLE_BLOCK_TYPE_LOG,
> > + .u.log = {
> > + .refname = (char *) "refs/heads/main",
> > + .update_index = 1,
> > + .value_type = REFTABLE_LOG_UPDATE,
> > + },
> > + };
> > + struct reftable_block block = { 0 };
> > + struct reftable_buf data = REFTABLE_BUF_INIT;
> > +
> > + cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_LOG, &rec, 1);
> > +
> > + /*
> > + * Log blocks store their inflated size as a big-endian 24-bit integer
> > + * right after the one-byte block type. Rewrite it to claim a size that
> > + * is smaller than the block header.
> > + */
> > + reftable_put_be24((uint8_t *) data.buf + 1, 1);
>
> Can I suggest to make this test a bit more strict:
>
> /*
> * Log blocks store their inflated size as a big-endian 24-bit integer
> * right after the one-byte block type.
> * First sanity check if the expected value is actually there,
> * then rewrite it to claim a size that is smaller than the block header.
> */
> void *p = (uint8_t *) data.buf + 1;
> uint32_t block_size = reftable_get_be24(p);
> cl_assert_equal_i(block_size, 82);
> reftable_put_be24(p, 5);
>
> So first do a sanity check to see if the current block_size is what we
> expect, this helps us ensure we are actually reading the correct point
> in the buffers. Or does this depend on the architecture?
>
> Then write a size that's just below what the expected value is. By using
> a value that's just too small, maybe we can avoid off-by-one errors?
That would cause a failure, but it wouldn't catch the out-of-bounds
write that the current test catches. This is because the offset would
still point into the buffer, even though it would point at the wrong
location. The requirement here is that the block size must be smaller
than the header size itself, so it must be between 1 and 3.
So in this case I think it doesn't quite help us to assert the written
block size.
Patrick
^ permalink raw reply
* Re: Unexpected recursion in 'git rm'
From: Phillip Wood @ 2026-07-03 10:04 UTC (permalink / raw)
To: Patrick Steinhardt,
Евгений Плискин
Cc: git
In-Reply-To: <akdzSHrJ4DfdUWoS@pks.im>
On 03/07/2026 09:31, Patrick Steinhardt wrote:
> On Thu, Jul 02, 2026 at 10:49:10AM +0300, Евгений Плискин wrote:
>> Hello.
>>
>> The following git command does recurse directories as contrary to the
>> reference (https://git-scm.com/docs/git-rm):
>>
>> git rm -n *.json
>>
>> Without directory specification before '*.json' this command is not
>> expected to recurse directories, but it really does.
Are there any ".json" files in the directory where you're running this?
As the glob is not quoted, I think maybe what is happening is that there
are no matching files in the current directory so the shell is not
expanding the glob as you expect and is passing it to git which treats
it as Patrick explains below.
Thanks
Phillip
> This is expected behaviour, as the argument to git-rm(1) is a pathspec,
> and "*" matches directory separators by default, see also gitglossary(7)
> under "pathspec":
>
> • the pathspec up to the last slash represents a directory prefix. The
> scope of that pathspec is limited to that subtree.
>
> • the rest of the pathspec is a pattern for the remainder of the
> pathname. Paths relative to the directory prefix will be matched
> against that pattern using fnmatch(3); in particular, * and ? can
> match directory separators.
>
> For example, Documentation/*.jpg will match all .jpg files in the
> Documentation subtree, including Documentation/chapter_1/figure_1.jpg.
>
> Could you maybe clarify which part of git-rm(1) made you think that this
> wouldn't happen?
>
> Thanks!
>
> Patrick
>
^ permalink raw reply
* Re: [PATCH v2 07/12] reftable/block: fix OOB read with bogus block size
From: Toon Claes @ 2026-07-03 9:28 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: oxsignal, Christian Couder
In-Reply-To: <20260629-pks-reftable-hardening-v2-7-b0228e7d908d@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The block size is read from the block header, which is untrusted data.
> We use it without verification to access the restart count at the end of
> the block as well as to compute the restart table offset. With a bogus
> block size that exceeds the data we have actually read this can lead to
> an out-of-bounds read:
>
> ==1458284==ERROR: AddressSanitizer: SEGV on unknown address 0x7d8ff7de4b7d (pc 0x55555598c339 bp 0x7fffffff4ef0 sp 0x7fffffff4eb0 T0)
> ==1458284==The signal is caused by a READ memory access.
> #0 0x55555598c339 in reftable_get_be16 ./build/../reftable/basics.h:118:9
> #1 0x55555598bee2 in reftable_block_init ./build/../reftable/block.c:344:18
> #2 0x555555813e0e in test_reftable_block__corrupt_block_size ./build/../t/unit-tests/u-reftable-block.c:540:8
> #3 0x5555557f684e in clar_run_test ./build/../t/unit-tests/clar/clar.c:335:3
> #4 0x5555557f2e69 in clar_run_suite ./build/../t/unit-tests/clar/clar.c:431:3
> #5 0x5555557f2882 in clar_test_run ./build/../t/unit-tests/clar/clar.c:636:4
> #6 0x5555557f375f in clar_test ./build/../t/unit-tests/clar/clar.c:687:11
> #7 0x5555557fa49d in cmd_main ./build/../t/unit-tests/unit-test.c:62:8
> #8 0x55555584b55a in main ./build/../common-main.c:9:11
> #9 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
> #10 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
> #11 0x555555694c24 in _start (./build/t/unit-tests+0x140c24)
>
> ==1458284==Register values:
> rax = 0x00007d8ff7de4b7d rbx = 0x00007fffffff4f00 rcx = 0x0000000000000006 rdx = 0x0000000000000010
> rdi = 0x00007d8ff7de4b7d rsi = 0x00007bfff5cf0420 rbp = 0x00007fffffff4ef0 rsp = 0x00007fffffff4eb0
> r8 = 0x00000f807eb960b8 r9 = 0x0000000000000001 r10 = 0x00007bfff5cf05e7 r11 = 0x000000000000000f
> r12 = 0x00007fffffff58f8 r13 = 0x0000000000000001 r14 = 0x0000555555ee8160 r15 = 0x0000000000000000
> AddressSanitizer can not provide additional info.
>
> Verify that the claimed block size fits into the block data before using
> it.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> reftable/block.c | 9 +++++++++
> t/unit-tests/u-reftable-block.c | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index b86cb9ec5a..4d6b11c2e7 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -340,6 +340,15 @@ int reftable_block_init(struct reftable_block *block,
> full_block_size = block_size;
> }
>
> + /*
> + * Ensure that we have sufficient data available now to satisfy the
> + * claimed block size.
> + */
> + if (block_size > block->block_data.len) {
> + err = REFTABLE_FORMAT_ERROR;
> + goto done;
> + }
> +
> restart_count = reftable_get_be16(block->block_data.data + block_size - 2);
> restart_off = block_size - 2 - 3 * restart_count;
>
> diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> index 088162483e..43b9d5fb59 100644
> --- a/t/unit-tests/u-reftable-block.c
> +++ b/t/unit-tests/u-reftable-block.c
> @@ -497,3 +497,36 @@ void test_reftable_block__corrupt_log_block_size(void)
> reftable_block_release(&block);
> reftable_buf_release(&data);
> }
> +
> +void test_reftable_block__corrupt_block_size(void)
> +{
> + struct reftable_block_source source = { 0 };
> + struct reftable_record rec = {
> + .type = REFTABLE_BLOCK_TYPE_REF,
> + .u.ref = {
> + .value_type = REFTABLE_REF_VAL1,
> + .refname = (char *) "refs/heads/main",
> + },
> + };
> + struct reftable_block block = { 0 };
> + struct reftable_buf data = REFTABLE_BUF_INIT;
> +
> + cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
> +
> + /*
> + * The block size is stored as a big-endian 24-bit integer right after
> + * the one-byte block type at the start of the block. Corrupt it to
> + * claim a size that is larger than the data we actually have. Reading
> + * the restart count and restart table relative to such a bogus block
> + * size must not access out-of-bounds memory.
> + */
> + reftable_put_be24((uint8_t *) data.buf + 1, 0xffffff);
Same here, would it make sense to write a size that's `+1` too much?
uint8_t *p = (uint8_t *)data.buf + 1;
uint32_t block_size = reftable_get_be24(p);
cl_assert_equal_i(block_size, 47);
reftable_put_be24(p, block_size + 1);
> +
> + block_source_from_buf(&source, &data);
> + cl_assert_equal_i(reftable_block_init(&block, &source, 0, 0, data.len,
> + REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_REF),
> + REFTABLE_FORMAT_ERROR);
> +
> + reftable_block_release(&block);
> + reftable_buf_release(&data);
> +}
>
> --
> 2.55.0.rc2.803.g1fd1e6609c.dirty
>
>
--
Cheers,
Toon
^ permalink raw reply
* [PATCH v2 9/9] gitlab-ci: enable "GIT_TEST_LONG"
From: Patrick Steinhardt @ 2026-07-03 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im>
Starting with 7a094d68a2 (ci: run expensive tests on push builds to
integration branches, 2026-05-08) we run expensive tests in our CI for
certain events. So far, this has only been wired up for GitHub Workflows
though, which creates a test gap for GitLab CI.
Plug this gap by also making this work for the latter.
Note that these tests cannot be run on the Windows runners, as they only
have 7.5GB of RAM. This is insufficient for some of the EXPENSIVE tests,
so we explicitly disable "GIT_TEST_LONG" on these jobs.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 6 ++++++
ci/lib.sh | 12 ++++++++++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index a4aebe8b71..1c4d04da9d 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -147,6 +147,9 @@ test:mingw64:
needs:
- job: "build:mingw64"
artifacts: true
+ variables:
+ # Windows runners don't have enough RAM to run EXPENSIVE tests.
+ GIT_TEST_LONG: false
before_script:
- *windows_before_script
- git-sdk/usr/bin/bash.exe -l -c 'tar xf artifacts/artifacts.tar.gz'
@@ -195,6 +198,9 @@ test:msvc-meson:
script:
- |
& "C:/Program Files/Git/usr/bin/bash.exe" -l -c 'ci/run-test-slice-meson.sh build $CI_NODE_INDEX $CI_NODE_TOTAL'
+ variables:
+ # Windows runners don't have enough RAM to run EXPENSIVE tests.
+ GIT_TEST_LONG: false
after_script:
- |
if ($env:CI_JOB_STATUS -ne "success") {
diff --git a/ci/lib.sh b/ci/lib.sh
index 01a0bc6b75..6c52154eac 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -215,6 +215,7 @@ then
test macos != "$CI_OS_NAME" || CI_OS_NAME=osx
CI_REPO_SLUG="$GITHUB_REPOSITORY"
CI_JOB_ID="$GITHUB_RUN_ID"
+ CI_EVENT="$GITHUB_EVENT_NAME"
CC="${CC_PACKAGE:-${CC:-gcc}}"
DONT_SKIP_TAGS=t
handle_failed_tests () {
@@ -239,6 +240,13 @@ then
CI_BRANCH="$CI_COMMIT_REF_NAME"
CI_COMMIT="$CI_COMMIT_SHA"
+ case "$CI_PIPELINE_SOURCE" in
+ merge_request_event)
+ CI_EVENT=pull_request;;
+ *)
+ CI_EVENT="$CI_PIPELINE_SOURCE";;
+ esac
+
case "$OS,$CI_JOB_IMAGE" in
Windows_NT,*)
CI_OS_NAME=windows
@@ -319,9 +327,9 @@ export SKIP_DASHED_BUILT_INS=YesPlease
# enable "expensive" tests for PR events.
# In order to catch bugs introduced at integration time by mismerges,
# enable the long tests for pushes to the integration branches as well.
-case "$GITHUB_EVENT_NAME,$CI_BRANCH" in
+case "$CI_EVENT,$CI_BRANCH" in
pull_request,*|push,*next*|push,*master*|push,*main*|push,*maint*)
- export GIT_TEST_LONG=true
+ export GIT_TEST_LONG=${GIT_TEST_LONG:-true}
;;
esac
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v2 8/9] gitlab-ci: disable RAM disk on macOS jobs
From: Patrick Steinhardt @ 2026-07-03 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im>
When we added the macOS jobs to GitLab CI in 56090a35ab (ci: add macOS
jobs to GitLab CI, 2024-01-18) we had to work around some very slow
disks. This workaround essentially creates a RAM disk that we mount,
where all test data is being written into RAM instead of the real disk.
In the next commit though we're about to enable "GIT_TEST_LONG", which
will make tests run that are marked with the "EXPENSIVE" prerequisite.
This change will make a couple of tests run that write up to 8GB of data
into the test output directory. As our RAM disk is only 4GB in size,
this change will cause ENOSPC errors.
We could accommodate for this by increasing the size of the RAM disk.
In c9d708b7fc (gitlab-ci: upgrade macOS runners, 2026-05-21) we have
upgraded our runners to use the "large" runners, which have 16GB of RAM
available. So we could easily expand the RAM disk to a capacity of for
example 12GB. But some test runs have shown that this is still quite
flaky overall, as we get quite close to our limits.
Instead, drop the workaround completely. This does indeed slow down
execution of the test jobs:
- osx-clang goes from 18 minutes to 25 minutes
- osx-meson goes from 21 minutes to 33 minutes
- osx-reftable stays at 21 minutes
The last one seems like an outlier. The only explanation that I have is
that we end up writing significantly less files with the reftable
backend, which ultimately causes less I/O.
Overall though, it's preferable to have something that works with the
least amount of flakiness compared to having something else that is
faster but unstable. Despite that, the macOS jobs aren't even the
slowest jobs, so this doesn't extend the overall pipeline's length.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 1a8e90932c..a4aebe8b71 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -88,13 +88,8 @@ test:osx:
tags:
- saas-macos-large-m2pro
variables:
- TEST_OUTPUT_DIRECTORY: "/Volumes/RAMDisk"
+ TEST_OUTPUT_DIRECTORY: "/tmp/test-output"
before_script:
- # Create a 4GB RAM disk that we use to store test output on. This small hack
- # significantly speeds up tests by more than a factor of 2 because the
- # macOS runners use network-attached storage as disks, which is _really_
- # slow with the many small writes that our tests do.
- - sudo diskutil apfs create $(hdiutil attach -nomount ram://8192000) RAMDisk
- ./ci/install-dependencies.sh
script:
- ./ci/run-build-and-tests.sh
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v2 7/9] t: use `test_bool_env` to parse GIT_TEST_LONG
From: Patrick Steinhardt @ 2026-07-03 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im>
It's currently hard to explicitly disable GIT_TEST_LONG by setting it to
`false`. Fix this by using `test_bool_env` instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 2 +-
t/test-lib.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index b939110a6e..01a0bc6b75 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -321,7 +321,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease
# enable the long tests for pushes to the integration branches as well.
case "$GITHUB_EVENT_NAME,$CI_BRANCH" in
pull_request,*|push,*next*|push,*master*|push,*main*|push,*maint*)
- export GIT_TEST_LONG=YesPlease
+ export GIT_TEST_LONG=true
;;
esac
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ceefb99bff..623fcfb747 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -210,7 +210,7 @@ parse_option () {
-i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
immediate=t ;;
-l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests)
- GIT_TEST_LONG=t; export GIT_TEST_LONG ;;
+ GIT_TEST_LONG=true; export GIT_TEST_LONG ;;
-r)
mark_option_requires_arg "$opt" run_list
;;
@@ -1849,7 +1849,7 @@ test_lazy_prereq AUTOIDENT '
'
test_lazy_prereq EXPENSIVE '
- test -n "$GIT_TEST_LONG"
+ test_bool_env GIT_TEST_LONG false
'
test_lazy_prereq EXPENSIVE_ON_WINDOWS '
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v2 6/9] t7900: clean up large EXPENSIVE repository
From: Patrick Steinhardt @ 2026-07-03 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im>
One of the tests in t7900 is marked with EXPENSIVE because we create a
repository with 2GB of data that we end up repacking. We never clean up
that repository though, so we occupy the full 2GB of data until the end
of the test suite.
Besides clogging our disk, having an EXPENSIVE test that alters the
repository's state used by subsequent tests is also a bad idea, as it
can easily have an impact on the heuristics used by other maintenance
tasks.
Adapt the test so that we create the data in a standalone repository
that we clean up at the end of the test. While at it, also disable
auto-maintenance so that it does not race with our manual maintenance.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t7900-maintenance.sh | 56 ++++++++++++++++++++++++++++----------------------
1 file changed, 31 insertions(+), 25 deletions(-)
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index d7f82e1bec..8a7e1306d0 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -461,36 +461,42 @@ test_expect_success 'incremental-repack task' '
'
test_expect_success EXPENSIVE 'incremental-repack 2g limit' '
- test_config core.compression 0 &&
+ test_when_finished rm -rf expensive-repo &&
+ git init expensive-repo &&
+ (
+ cd expensive-repo &&
+ git config set core.compression 0 &&
+ git config set maintenance.auto false &&
- for i in $(test_seq 1 5)
- do
- test-tool genrandom foo$i $((512 * 1024 * 1024 + 1)) >>big ||
- return 1
- done &&
- git add big &&
- git commit -qm "Add big file (1)" &&
+ for i in $(test_seq 1 5)
+ do
+ test-tool genrandom foo$i $((512 * 1024 * 1024 + 1)) >>big ||
+ return 1
+ done &&
+ git add big &&
+ git commit -qm "Add big file (1)" &&
- # ensure any possible loose objects are in a pack-file
- git maintenance run --task=loose-objects &&
+ # ensure any possible loose objects are in a pack-file
+ git maintenance run --task=loose-objects &&
- rm big &&
- for i in $(test_seq 6 10)
- do
- test-tool genrandom foo$i $((512 * 1024 * 1024 + 1)) >>big ||
- return 1
- done &&
- git add big &&
- git commit -qm "Add big file (2)" &&
+ rm big &&
+ for i in $(test_seq 6 10)
+ do
+ test-tool genrandom foo$i $((512 * 1024 * 1024 + 1)) >>big ||
+ return 1
+ done &&
+ git add big &&
+ git commit -qm "Add big file (2)" &&
- # ensure any possible loose objects are in a pack-file
- git maintenance run --task=loose-objects &&
+ # ensure any possible loose objects are in a pack-file
+ git maintenance run --task=loose-objects &&
- # Now run the incremental-repack task and check the batch-size
- GIT_TRACE2_EVENT="$(pwd)/run-2g.txt" git maintenance run \
- --task=incremental-repack 2>/dev/null &&
- test_subcommand git multi-pack-index repack \
- --no-progress --batch-size=2147483647 <run-2g.txt
+ # Now run the incremental-repack task and check the batch-size
+ GIT_TRACE2_EVENT="$(pwd)/run-2g.txt" git maintenance run \
+ --task=incremental-repack 2>/dev/null &&
+ test_subcommand git multi-pack-index repack \
+ --no-progress --batch-size=2147483647 <run-2g.txt
+ )
'
run_incremental_repack_and_verify () {
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v2 5/9] t7508: skip EXPENSIVE test that is broken without SIZE_T_IS_32BIT
From: Patrick Steinhardt @ 2026-07-03 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im>
One of the tests in t7508 is marked as EXPENSIVE because it ends up
creating and adding files that are multiple gigabytes in size. This
takes a while to complete, hence the EXPENSIVE prerequisite.
Besides being expensive though the test can only work on systems where
`size_t` is at least 64 bit. This is because one of the created files
is larger than 4GB, and because Git tracks object size via `size_t` it
will eventually blow up.
This test has also been blowing up in the "linux32" CI job in GitHub
Workflows since 7a094d68a2 (ci: run expensive tests on push builds to
integration branches, 2026-05-08). But that job doesn't only fail, it
also hangs, and that has been concealing the failure.
Fix the issue by marking the test as requiring 64 bit `size_t`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t7508-status.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c2057bc94c..dfdd78b6fe 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1773,7 +1773,7 @@ test_expect_success 'slow status advice when core.untrackedCache true, and fsmon
)
'
-test_expect_success EXPENSIVE 'status does not re-read unchanged 4 or 8 GiB file' '
+test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'status does not re-read unchanged 4 or 8 GiB file' '
(
mkdir large-file &&
cd large-file &&
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v2 4/9] t5608: reduce maximum disk usage
From: Patrick Steinhardt @ 2026-07-03 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im>
The tests in t5608 perform a couple of clones of repositories that are
somewhat large. Ultimately, we end up creating:
- A setup repository that contains 2GB of uncompressed pack data.
- A bare clone that contains the same 2GB of data.
- A clone with worktree writes a 2GB packfile and a 2GB worktree.
- A second setup repository that contains a 4GB packfile.
- Two 4GB clone of that repository.
Some of these clones ultimately hardlink files, which ensures that we at
least don't end up with more than 20GB of data. But at the end of the
test we still have around 16GB of data, which is only a tiny bit better.
Refactor the test to prune repositories after they have no use anymore.
This reduced the peak disk usage of this test to 8GB.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t5608-clone-2gb.sh | 66 ++++++++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 31 deletions(-)
diff --git a/t/t5608-clone-2gb.sh b/t/t5608-clone-2gb.sh
index 4f8a95ddda..5d56debf1c 100755
--- a/t/t5608-clone-2gb.sh
+++ b/t/t5608-clone-2gb.sh
@@ -10,45 +10,47 @@ then
fi
test_expect_success 'setup' '
-
- git config pack.compression 0 &&
- git config pack.depth 0 &&
- blobsize=$((100*1024*1024)) &&
- blobcount=$((2*1024*1024*1024/$blobsize+1)) &&
- i=1 &&
- (while test $i -le $blobcount
- do
- printf "Generating blob $i/$blobcount\r" >&2 &&
- printf "blob\nmark :$i\ndata $blobsize\n" &&
- #test-tool genrandom $i $blobsize &&
- printf "%-${blobsize}s" $i &&
- echo "M 100644 :$i $i" >> commit &&
- i=$(($i+1)) ||
- echo $? > exit-status
- done &&
- echo "commit refs/heads/main" &&
- echo "author A U Thor <author@email.com> 123456789 +0000" &&
- echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
- echo "data 5" &&
- echo ">2gb" &&
- cat commit) |
- git fast-import --big-file-threshold=2 &&
- test ! -f exit-status
-
+ git init 2gb-repo &&
+ (
+ cd 2gb-repo &&
+ git config pack.compression 0 &&
+ git config pack.depth 0 &&
+ blobsize=$((100*1024*1024)) &&
+ blobcount=$((2*1024*1024*1024/$blobsize+1)) &&
+ i=1 &&
+ (while test $i -le $blobcount
+ do
+ printf "Generating blob $i/$blobcount\r" >&2 &&
+ printf "blob\nmark :$i\ndata $blobsize\n" &&
+ #test-tool genrandom $i $blobsize &&
+ printf "%-${blobsize}s" $i &&
+ echo "M 100644 :$i $i" >> commit &&
+ i=$(($i+1)) ||
+ echo $? > exit-status
+ done &&
+ echo "commit refs/heads/main" &&
+ echo "author A U Thor <author@email.com> 123456789 +0000" &&
+ echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
+ echo "data 5" &&
+ echo ">2gb" &&
+ cat commit) |
+ git fast-import --big-file-threshold=2 &&
+ test ! -f exit-status
+ )
'
test_expect_success 'clone - bare' '
-
- git clone --bare --no-hardlinks . clone-bare
-
+ test_when_finished rm -rf clone-bare &&
+ git clone --bare --no-hardlinks 2gb-repo clone-bare
'
test_expect_success 'clone - with worktree, file:// protocol' '
-
- git clone "file://$(pwd)" clone-wt
-
+ test_when_finished rm -rf clone-wt &&
+ git clone "file://$(pwd)/2gb-repo" clone-wt
'
+rm -rf 2gb-repo 2>/dev/null
+
test_expect_success SIZE_T_IS_64BIT,EXPENSIVE 'set up repo with >4GB object' '
large_blob_size=$((4*1024*1024*1024+1)) &&
git init --bare 4gb-repo &&
@@ -61,6 +63,7 @@ test_expect_success SIZE_T_IS_64BIT,EXPENSIVE 'set up repo with >4GB object' '
'
test_expect_success SIZE_T_IS_64BIT,EXPENSIVE 'clone >4GB object via unpack-objects' '
+ test_when_finished rm -rf 4gb-clone-unpack &&
# The synthesized pack has five objects, so a large unpack limit keeps
# fetch-pack on the unpack-objects path.
git -c fetch.unpackLimit=100 clone --bare \
@@ -77,6 +80,7 @@ test_expect_success SIZE_T_IS_64BIT,EXPENSIVE 'clone >4GB object via unpack-obje
'
test_expect_success SIZE_T_IS_64BIT,EXPENSIVE 'clone with >4GB object via index-pack' '
+ test_when_finished rm -rf 4gb-clone-index &&
# Force fetch-pack to hand the pack to index-pack instead.
git -c fetch.unpackLimit=1 clone --bare \
"file://$(pwd)/4gb-repo" 4gb-clone-index &&
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v2 3/9] t4141: fix inefficient use of dd(1)
From: Patrick Steinhardt @ 2026-07-03 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im>
In t4141 we generate a patch that is roughly 1GB in size to verify that
git-apply(1) indeed rejects that patch. We generate that patch by
prepending a patch header and then executing `test-tool genzeros`
without a limit. This causes us to print infinitely many zeros, and we
limit the overall amount of generated bytes via `test_copy_bytes`.
This test setup is extremely expensive, as `test_copy_bytes` is
implemented via `dd ibs=1 count="$1"`, which copies data one byte at a
time. So as we write 1GB of data, we end up doing 1 billion reads and
writes. This naturally takes a while: it takes 6 minutes on my system,
and around 40 minutes in some CI jobs!
We can do much better though, as genzeros already knows to handle an
optional limit of how much data it is supposed to write, which allows us
to remove the call to `test_copy_bytes`. Furthermore, it has already
been optimized to generate the data fast.
And indeed, doing this conversion drops the test execution to less than
a second on my machine. That means that in theory it becomes feasible to
drop the EXPENSIVE prerequisite now. But git-apply(1) still soaks up 1GB
of data into memory, which may count as being expensive. Consequently,
we keep the prerequisite intact.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t4141-apply-too-large.sh | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/t/t4141-apply-too-large.sh b/t/t4141-apply-too-large.sh
index eac6f7e151..9dbed940db 100755
--- a/t/t4141-apply-too-large.sh
+++ b/t/t4141-apply-too-large.sh
@@ -5,7 +5,6 @@ test_description='git apply with too-large patch'
. ./test-lib.sh
test_expect_success EXPENSIVE 'git apply rejects patches that are too large' '
- sz=$((1024 * 1024 * 1023)) &&
{
cat <<-\EOF &&
diff --git a/file b/file
@@ -14,8 +13,8 @@ test_expect_success EXPENSIVE 'git apply rejects patches that are too large' '
+++ b/file
@@ -0,0 +1 @@
EOF
- test-tool genzeros
- } | test_copy_bytes $sz | test_must_fail git apply 2>err &&
+ test-tool genzeros $((1024 * 1024 * 1023))
+ } | test_must_fail git apply 2>err &&
grep "patch too large" err
'
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v2 2/9] t0021: skip EXPENSIVE test that is broken without SIZE_T_IS_32BIT
From: Patrick Steinhardt @ 2026-07-03 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im>
One of the tests in t0021 writes a 2GB file and then roundtrips it
through the clean/sumdge filters. This test is broken on 32 bit
platforms because they typically don't handle files larger then
`SSIZE_MAX` well at all.
While our CI has a "linux32" job that should in theory hit this issue,
we never noticed it because we didn't use to run EXPENSIVE tests until
7a094d68a2 (ci: run expensive tests on push builds to integration
branches, 2026-05-08). And after that commit, the test does not fail but
instead hangs completely.
Ideally, we'd of course properly detect this situation and then test for
it. In practice, this turns out to be hard as the test failure are not
reliable as they often (but not always) run into ENOMEM errors.
Instead, skip the test altogether.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t0021-conversion.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 033b00a364..7b9a0ca877 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -296,7 +296,7 @@ test_expect_success 'filter that does not read is fine' '
test_cmp expect actual
'
-test_expect_success EXPENSIVE 'filter large file' '
+test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'filter large file' '
test_config filter.largefile.smudge cat &&
test_config filter.largefile.clean cat &&
test_seq -f "%1048576d" 1 2048 >2GB &&
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v2 1/9] README: add GitLab CI badge to make it more discoverable
From: Patrick Steinhardt @ 2026-07-03 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im>
The Git project uses CI systems from both GitHub and GitLab. While both
of these systems are extensively used in day-to-day work, we only have a
link to the GitHub Workflows in our README, which makes the GitLab CI
hard to discover.
Improve the situation by adding a second badge for GitLab CI to our
README.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
README.md | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/README.md b/README.md
index d87bca1b8c..46489b0971 100644
--- a/README.md
+++ b/README.md
@@ -1,4 +1,5 @@
-[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
+[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
+[](https://gitlab.com/git-scm/git/-/pipelines?ref=master)
Git - fast, scalable, distributed revision control system
=========================================================
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v2 0/9] t: fixes and improvements for GIT_TEST_LONG
From: Patrick Steinhardt @ 2026-07-03 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260702-b4-pks-t-fixes-for-GIT-TEST-LONG-v1-0-76b4d7bab3d0@pks.im>
Hi,
this series started out as a simple two-patch series that wired up the
GitLab CI badge in our README and GIT_TEST_LONG for GitLab CI. But as it
typically goes, tests broke on GitLab CI, which made me realize that
they are broken even on GitHub's master branch right now. Some tests are
failing in the linux32 job, and we only didn't notice because the whole
pipeline hangs.
So I had to go down the rabbit hole a bit, the result of which is this
patch series.
Changes in v2:
- Reinstate the EXPENSIVE prerequisite on t4141, as we still end up
reading 1GB of data into memory.
- Improve a rather hand-wavy commit message.
- Link to v1: https://patch.msgid.link/20260702-b4-pks-t-fixes-for-GIT-TEST-LONG-v1-0-76b4d7bab3d0@pks.im
Thanks!
Patrick
[1]: <akIfsaVMB_S6kfJQ@pks.im>
---
Patrick Steinhardt (9):
README: add GitLab CI badge to make it more discoverable
t0021: skip EXPENSIVE test that is broken without SIZE_T_IS_32BIT
t4141: fix inefficient use of dd(1)
t5608: reduce maximum disk usage
t7508: skip EXPENSIVE test that is broken without SIZE_T_IS_32BIT
t7900: clean up large EXPENSIVE repository
t: use `test_bool_env` to parse GIT_TEST_LONG
gitlab-ci: disable RAM disk on macOS jobs
gitlab-ci: enable "GIT_TEST_LONG"
.gitlab-ci.yml | 13 ++++-----
README.md | 3 ++-
ci/lib.sh | 12 +++++++--
t/t0021-conversion.sh | 2 +-
t/t4141-apply-too-large.sh | 5 ++--
t/t5608-clone-2gb.sh | 66 ++++++++++++++++++++++++----------------------
t/t7508-status.sh | 2 +-
t/t7900-maintenance.sh | 56 +++++++++++++++++++++------------------
t/test-lib.sh | 4 +--
9 files changed, 91 insertions(+), 72 deletions(-)
Range-diff versus v1:
1: 1f445b2106 = 1: a348e70b40 README: add GitLab CI badge to make it more discoverable
2: f2d21ef6f7 = 2: c7444bddf3 t0021: skip EXPENSIVE test that is broken without SIZE_T_IS_32BIT
3: 656d6b9ada ! 3: aea7f61bbb t4141: fix inefficient use of dd(1)
@@ Commit message
been optimized to generate the data fast.
And indeed, doing this conversion drops the test execution to less than
- a second on my machine, so that we can drop the EXPENSIVE prerequisite.
+ a second on my machine. That means that in theory it becomes feasible to
+ drop the EXPENSIVE prerequisite now. But git-apply(1) still soaks up 1GB
+ of data into memory, which may count as being expensive. Consequently,
+ we keep the prerequisite intact.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## t/t4141-apply-too-large.sh ##
@@ t/t4141-apply-too-large.sh: test_description='git apply with too-large patch'
-
. ./test-lib.sh
--test_expect_success EXPENSIVE 'git apply rejects patches that are too large' '
+ test_expect_success EXPENSIVE 'git apply rejects patches that are too large' '
- sz=$((1024 * 1024 * 1023)) &&
-+test_expect_success 'git apply rejects patches that are too large' '
{
cat <<-\EOF &&
diff --git a/file b/file
4: 91ea8610ad = 4: 30b618259e t5608: reduce maximum disk usage
5: 5d1f630617 = 5: 23898a60be t7508: skip EXPENSIVE test that is broken without SIZE_T_IS_32BIT
6: 4938e39e47 ! 6: 835fa0f8e1 t7900: clean up large EXPENSIVE repository
@@ Commit message
One of the tests in t7900 is marked with EXPENSIVE because we create a
repository with 2GB of data that we end up repacking. We never clean up
that repository though, so we occupy the full 2GB of data until the end
- of the test suite. Besides clogging our disk, it also means that all
- subsequent tests may have to repack this data multiple times.
+ of the test suite.
+
+ Besides clogging our disk, having an EXPENSIVE test that alters the
+ repository's state used by subsequent tests is also a bad idea, as it
+ can easily have an impact on the heuristics used by other maintenance
+ tasks.
Adapt the test so that we create the data in a standalone repository
that we clean up at the end of the test. While at it, also disable
7: 3a19541964 = 7: 13fa3db8cd t: use `test_bool_env` to parse GIT_TEST_LONG
8: 7d33694504 = 8: d8f94cb564 gitlab-ci: disable RAM disk on macOS jobs
9: 56c0239056 = 9: d4792b76a0 gitlab-ci: enable "GIT_TEST_LONG"
---
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
change-id: 20260701-b4-pks-t-fixes-for-GIT-TEST-LONG-78e538bf0e06
^ permalink raw reply
* Re: [PATCH v2 06/12] reftable/block: fix OOB write with bogus inflated log size
From: Toon Claes @ 2026-07-03 9:23 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: oxsignal, Christian Couder
In-Reply-To: <20260629-pks-reftable-hardening-v2-6-b0228e7d908d@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> index f4e926ce3a..088162483e 100644
> --- a/t/unit-tests/u-reftable-block.c
> +++ b/t/unit-tests/u-reftable-block.c
> @@ -465,3 +465,35 @@ void test_reftable_block__iterator(void)
> reftable_block_release(&block);
> reftable_buf_release(&data);
> }
> +
> +void test_reftable_block__corrupt_log_block_size(void)
> +{
> + struct reftable_block_source source = { 0 };
> + struct reftable_record rec = {
> + .type = REFTABLE_BLOCK_TYPE_LOG,
> + .u.log = {
> + .refname = (char *) "refs/heads/main",
> + .update_index = 1,
> + .value_type = REFTABLE_LOG_UPDATE,
> + },
> + };
> + struct reftable_block block = { 0 };
> + struct reftable_buf data = REFTABLE_BUF_INIT;
> +
> + cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_LOG, &rec, 1);
> +
> + /*
> + * Log blocks store their inflated size as a big-endian 24-bit integer
> + * right after the one-byte block type. Rewrite it to claim a size that
> + * is smaller than the block header.
> + */
> + reftable_put_be24((uint8_t *) data.buf + 1, 1);
Can I suggest to make this test a bit more strict:
/*
* Log blocks store their inflated size as a big-endian 24-bit integer
* right after the one-byte block type.
* First sanity check if the expected value is actually there,
* then rewrite it to claim a size that is smaller than the block header.
*/
void *p = (uint8_t *) data.buf + 1;
uint32_t block_size = reftable_get_be24(p);
cl_assert_equal_i(block_size, 82);
reftable_put_be24(p, 5);
So first do a sanity check to see if the current block_size is what we
expect, this helps us ensure we are actually reading the correct point
in the buffers. Or does this depend on the architecture?
Then write a size that's just below what the expected value is. By using
a value that's just too small, maybe we can avoid off-by-one errors?
> +
> + block_source_from_buf(&source, &data);
> + cl_assert_equal_i(reftable_block_init(&block, &source, 0, 0, data.len,
> + REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_LOG),
> + REFTABLE_FORMAT_ERROR);
> +
> + reftable_block_release(&block);
> + reftable_buf_release(&data);
> +}
--
Cheers,
Toon
^ permalink raw reply
* merci d'effacer nos donnees
From: christine.filbert6 @ 2026-07-03 9:14 UTC (permalink / raw)
To: git
jEnvoyé de mon iPhone
^ permalink raw reply
* Re: [PATCH v6 2/2] config: add "worktree" and "worktree/i" includeIf conditions
From: Junio C Hamano @ 2026-07-03 9:02 UTC (permalink / raw)
To: Chen Linxuan via B4 Relay
Cc: git, Kristoffer Haugsbakk, Patrick Steinhardt, Chen Linxuan,
Phillip Wood
In-Reply-To: <20260703-includeif-worktree-v6-2-a13893ad9a7f@black-desk.cn>
Chen Linxuan via B4 Relay <devnull+me.black-desk.cn@kernel.org>
writes:
> +`worktree`::
> + The data that follows the keyword `worktree` and a colon is used as a
> + glob pattern. If the working directory of the current worktree matches
> + the pattern, the include condition is met.
> ...
> +can be set once in a global or system-level configuration file (e.g.
> +`~/.config/git/config`) and applies to all repositories at once based on
> +their worktree location.
> +
> +`worktree/i`::
> + This is the same as `worktree` except that matching is done
> + case-insensitively (e.g. on case-insensitive file systems)
> +
OK. I briefly wondered if
`worktree`::
`worktree/i`::
What follows the keyword `worktree` (or `worktree/i`) and a
colon is used as a glob pattern. If the working directory of
the current worktree matches (with `/i` the match is made
case-insensitively) the pattern, ...
is easier for those who looks up 'worktree' to notice (without
having to scroll too far to look at the other entry) that there is a
case insensitive option available. As the construct used in this
patch mimicks how `gitdir` and `gitdir/i` are described, however, I
think such a change is better done as a separate topic, long after
this patch lands and graduates to the master, to clean up both
`gitdir` and `worktree` in the same commit. So let's leave it out
of this topic.
> else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
> return include_by_path(kvi, opts->git_dir, cond, cond_len, 1);
> + else if (skip_prefix_mem(cond, cond_len, "worktree:", &cond, &cond_len))
> + return include_by_path(kvi, inc->repo ? repo_get_work_tree(inc->repo) : NULL,
> + cond, cond_len, 0);
> + else if (skip_prefix_mem(cond, cond_len, "worktree/i:", &cond, &cond_len))
> + return include_by_path(kvi, inc->repo ? repo_get_work_tree(inc->repo) : NULL,
> + cond, cond_len, 1);
Fairly straight-forward.
> +# Use a loose pattern so the "present in non-worktree cases" check works
> +# for Unix-style absolute paths and Windows paths like D:/a/git/...
> +test_expect_success 'conditional include, worktree without repository' '
> + test_when_finished "rm -f .gitconfig config.inc" &&
> + git config set -f .gitconfig "includeIf.worktree:**.path" config.inc &&
> + git config set -f config.inc foo.bar baz &&
> + git config get foo.bar &&
> + test_must_fail nongit git config get foo.bar
> +'
This looks much easier to understand than the previous round.
Thanks.
^ permalink raw reply
* Re: [PATCH v8 06/11] reset: introduce ability to skip updating HEAD
From: Patrick Steinhardt @ 2026-07-03 8:42 UTC (permalink / raw)
To: Christian Couder
Cc: git, Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk,
Phillip Wood, Christian Couder
In-Reply-To: <CAP8UFD3RZv=PGC15xms3xZZkNQsGLpgq_ogt8P9ux-8p4HwW6A@mail.gmail.com>
On Fri, Jul 03, 2026 at 08:59:30AM +0200, Christian Couder wrote:
> On Wed, Jul 1, 2026 at 1:35 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> > @@ -113,6 +114,9 @@ int reset_working_tree(struct repository *r,
> > if (opts->branch_msg && !opts->branch)
> > BUG("branch reflog message given without a branch");
> >
> > + if (update_orig_head && !update_head)
> > + BUG("cannot update ORIG_HEAD without updating HEAD" );
>
> There is a spurious space character between `HEAD"` and `);`.
Ugh, indeed, well spotted. Junio, is this something you can fix up or
shall I send a new version for this?
Patrick
^ permalink raw reply
* Re: [PATCH] precompose_utf8: use a flex array for d_name
From: Patrick Steinhardt @ 2026-07-03 8:40 UTC (permalink / raw)
To: Ihar Hrachyshka; +Cc: git
In-Reply-To: <20260703023554.36577-1-ihar.hrachyshka@gmail.com>
On Thu, Jul 02, 2026 at 10:35:54PM -0400, Ihar Hrachyshka wrote:
> On macOS, git status may abort while reading a directory entry
> whose UTF-8 name grows past NAME_MAX bytes:
>
> __chk_fail_overflow
> __strlcpy_chk
> precompose_utf8_readdir
> read_directory_recursive
> wt_status_collect
> cmd_status
>
> The precompose wrapper already reallocates dirent_prec_psx for
> long names, but d_name is declared as char[NAME_MAX + 1]. A
> fortified libc can still see that declared object size and reject a
> larger strlcpy bound, even though the allocation was grown.
>
> Make d_name a FLEX_ARRAY and size allocations from offsetof(). That
> matches the actual object layout with the dynamic allocation, so the
> fortified copy sees a destination whose size can grow with max_name_len.
>
> Add a regression test that creates a 261-byte non-ASCII basename and
> runs status with core.precomposeunicode enabled.
Hm. Why does macOS even allow you to create a file that has a basename
longer than NAME_MAX? Does macOS count unicode characters specially?
> diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
> index 1711794..8077f62 100644
> --- a/compat/precompose_utf8.c
> +++ b/compat/precompose_utf8.c
> @@ -19,6 +19,11 @@ typedef char *iconv_ibp;
> static const char *repo_encoding = "UTF-8";
> static const char *path_encoding = "UTF-8-MAC";
>
> +static size_t dirent_prec_psx_size(size_t max_name_len)
> +{
> + return st_add(offsetof(dirent_prec_psx, d_name), max_name_len);
> +}
> +
> static size_t has_non_ascii(const char *s, size_t maxlen, size_t *strlen_c)
> {
> const uint8_t *ptr = (const uint8_t *)s;
> @@ -114,8 +119,8 @@ const char *precompose_argv_prefix(int argc, const char **argv, const char *pref
> PREC_DIR *precompose_utf8_opendir(const char *dirname)
> {
> PREC_DIR *prec_dir = xmalloc(sizeof(PREC_DIR));
> - prec_dir->dirent_nfc = xmalloc(sizeof(dirent_prec_psx));
> - prec_dir->dirent_nfc->max_name_len = sizeof(prec_dir->dirent_nfc->d_name);
> + prec_dir->dirent_nfc = xmalloc(dirent_prec_psx_size(NAME_MAX + 1));
> + prec_dir->dirent_nfc->max_name_len = NAME_MAX + 1;
We have the `FLEX_ALLOC_MEM()` macro that would probably be a better fit
compared to introducing `dirent_prec_psx_size()`.
Also, when converting this to a flex array, can't we do better here and
allocate the structures with the right size? Otherwise, I expect that we
overallocate most of the entrise.
> @@ -145,8 +150,7 @@ struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *prec_dir)
> int ret_errno = errno;
>
> if (new_maxlen > prec_dir->dirent_nfc->max_name_len) {
> - size_t new_len = sizeof(dirent_prec_psx) + new_maxlen -
> - sizeof(prec_dir->dirent_nfc->d_name);
> + size_t new_len = dirent_prec_psx_size(new_maxlen);
>
> prec_dir->dirent_nfc = xrealloc(prec_dir->dirent_nfc, new_len);
> prec_dir->dirent_nfc->max_name_len = new_maxlen;
Okay, here we indeed have to realloc though, and thus we can't quite
avoid `dirent_prec_psx_size()`. Too bad.
Thanks!
Patrick
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox