* Re: [PATCH v1 09/11] git-gui: support using repository parent dir as a worktree
From: Johannes Sixt @ 2026-05-16 8:14 UTC (permalink / raw)
To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260514143322.865587-10-mlevedahl@gmail.com>
Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> git-gui, since 87cd09f43e ("git-gui: work from the .git dir",
> 2010-01-23), has had the intent to allow starting from inside a
> repository, then switching to the parent directory if that is a valid
> worktree.
>
> This certainly hasn't worked since 2d92ab32fd ("rev-parse: make
> --show-toplevel without a worktree an error", 2019-11-19) in git, but
> breaking this git-gui feature was unintentional.
>
> Add a proc to test if the parent of the git repository is a valid
> worktree, and set that directory as the worktree if so. Use invocations
> of git rev-parse to assure all validity and safety checks included in
> git-core are executed.
BTW, missing sign-off.
> ---
> git-gui.sh | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index a03eaa7..e326401 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1100,6 +1100,23 @@ unset argv0dir
> ##
> ## repository setup
>
> +proc is_parent_worktree {} {
> + # Directory 'parent' of a repository named 'parent/.git' might be the worktree
> + set ok 0
> + if {[file tail $::_gitdir] eq {.git}} {
> + set gitdir_parent [file join $::_gitdir {..}]
> + set expected_worktree [file normalize $gitdir_parent]
We have [file dirname ...]. Is there a reason to avoid it?
> + catch {set git_worktree [git -C $gitdir_parent rev-parse --show-toplevel]}
> + if {[string compare $expected_worktree $git_worktree] == 0} {
The purpose of this check should be explained in a comment. I think it is:
For a repository with the database in a directory named .git we assume
that the working tree is the directory containing .git. But
configuration may point to a different worktree. Then we do not want to
hold on to our assumption.
However, whether [git -C elsewhere ...] uses the same gitdir that we
have discovered so far cannot be told from this piece of code alone.
Therefore, I think it is wrong to extract this check into a function.
Also, I don't think we can use string comparison here. On Windows, the
command returns the Windows style path, but Tcl my operate with a POSIX
style path.
> + set ::_prefix {}
> + set ::_gitworktree $git_worktree
> + cd $git_worktree
So many side-effects in a function whose name suggests that it only does
some checks. Please, don't do that.
> + set ok 1
> + }
> + }
> + return $ok
> +}
> +
> proc is_gitvars_error {err} {
> set havevars 0
> set GIT_DIR {}
In general, I am not a fan of commits that add new functions, but no
call sites. Please squash this into 10/11. Ditto for is_gitvars_error in
06/11.
-- Hannes
^ permalink raw reply
* Re: [PATCH v1 08/11] git-gui: simplify [is_bare] to report if a worktree is known
From: Johannes Sixt @ 2026-05-16 8:12 UTC (permalink / raw)
To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260514143322.865587-9-mlevedahl@gmail.com>
Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> git-gui includes proc is_bare, used in several places to make decisions
> on whether a worktree exists, but also in discovery to tell if a
> worktree can be supported.
>
> But, is_bare is out of date with regard to multiple worktrees, safe
> repository guards, and possibly other relevant features known to git
> rev-parse. Also, is_bare caches its result on the first call, so is not
> useful if a later step in the discovery process finds a worktree.
>
> So, simplify is_bare to report whether git-gui has a worktree or is
> working only from a repository.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
> git-gui.sh | 25 +------------------------
> 1 file changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 81789dd..a03eaa7 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -372,7 +372,6 @@ if {[tk windowingsystem] eq "aqua"} {
> set _appname {Git Gui}
> set _gitdir {}
> set _gitworktree {}
> -set _isbare {}
> set _githtmldir {}
> set _prefix {}
> set _reponame {}
> @@ -524,29 +523,7 @@ proc get_config {name} {
> }
>
> proc is_bare {} {
> - global _isbare
> - global _gitdir
> - global _gitworktree
> -
> - if {$_isbare eq {}} {
> - if {[catch {
> - set _bare [git rev-parse --is-bare-repository]
> - switch -- $_bare {
> - true { set _isbare 1 }
> - false { set _isbare 0}
> - default { throw }
> - }
> - }]} {
> - if {[is_config_true core.bare]
> - || ($_gitworktree eq {}
> - && [lindex [file split $_gitdir] end] ne {.git})} {
> - set _isbare 1
> - } else {
> - set _isbare 0
> - }
> - }
> - }
> - return $_isbare
> + return [expr {$::_gitworktree eq {}}]
> }
>
> ######################################################################
Very nice!
IMO, regardless of which way we end up rewriting repository discovery,
the end result should be that we can use $_gitworktree like this to tell
whether we are in a bare repository or not.
-- Hannes
^ permalink raw reply
* Re: [PATCH] evaluate the second argument of ALLOC_GROW only once
From: Johannes Sixt @ 2026-05-16 6:55 UTC (permalink / raw)
To: Jeff King, René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <20260515195049.GA149960@coredump.intra.peff.net>
Am 15.05.26 um 21:50 schrieb Jeff King:
> On Fri, May 15, 2026 at 03:08:18PM -0400, Jeff King wrote:
>
>> On Fri, May 15, 2026 at 08:16:50PM +0200, René Scharfe wrote:
>>
>>> + size_t alloc_grow_new_alloc_; \
>>> + if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
>>> + alloc = alloc_grow_new_alloc_; \
>>> + REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
>>> } \
>>
>> What happens if a caller passes in an argument that isn't a size_t?
>> We'll check for overflow in the size_t space, and then truncate it when
>> we assign to alloc, I think.
>
> Hmm, playing with it and looking a little closer, I think we don't end
> up overflowing the buffer because you use the size_t for
> REALLOC_ARRAY(). So the result is big, but then "alloc" is truncated.
Protect against double-evaluation of "alloc", too, using
size_t *palloc = &(alloc);
and use *palloc in the two places, then all callers are forced to work
with a size_t as third argument. Don't know what the damage would be,
though.
-- Hannes
^ permalink raw reply
* Re: UBSan failing on expensive test(s)
From: Derrick Stolee @ 2026-05-16 2:55 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <20260516021343.GA174647@coredump.intra.peff.net>
On 5/15/26 10:13 PM, Jeff King wrote:
> On Sat, May 16, 2026 at 08:43:07AM +0900, Junio C Hamano wrote:
>
>> This started happening on 'next' that runs EXPENSIVE tests thanks to
>> Dscho's recent updates to enable them in CI.
>>
>> https://github.com/git/git/actions/runs/25896439353/job/76110441841#step:10:2172
>>
>> It claims that """
>>
>> commit.c:1574:6: runtime error: signed integer overflow:
>> -2147483648 - 1 cannot be represented in type 'int'
>>
>> """.
>>
>> Another is related in the sense that it used to be hidden behind
>> EXPENSIVE prerequisite, but is probably unrelated.
>>
>> https://github.com/git/git/actions/runs/25896439353/job/76110441842#step:10:156
>
> These patches should fix both.
>
> [1/2]: apply: plug leak on "patch too large" error
> [2/2]: commit: handle large commit messages in utf8 verification
Thanks for the fast fixes. I agree with you that 2GB commit
messages are silly, but the methods you change could be used
for other things so having better types is good. We can
consider blocking large commits as a feature another time.
-Stolee
^ permalink raw reply
* Re: [PATCH] evaluate the second argument of ALLOC_GROW only once
From: Jeff King @ 2026-05-16 2:51 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <1d79d7bc-5441-4e72-9cb0-e8900f57172c@web.de>
On Sat, May 16, 2026 at 01:01:05AM +0200, René Scharfe wrote:
> > I think as long as the behavior remains "slow, but we do not overflow
> > any buffers" when you reach these limits, that's OK. Nobody is going to
> > do it in practice, and we just want to make sure that malicious inputs
> > cannot get out-of-bounds writes. It might be worth adding a comment,
> > though, to make sure nobody ever swaps "alloc_grow_new_alloc_" for
> > "alloc" in that macro.
> There is no overflow check in either version (yet), so neither is safe
> to operate close to the boundary. Close meaning the intermediate term
> (alloc + 16) * 3 being bigger than the maximum value.
Yes, but for some definition of safe. Both before and after your patch,
as we get close to the boundary the allocation will grow slower than it
should, but we'll never write out of bounds. The behavior for the "git
foo" I showed earlier is slightly different:
- before your patch, ~2GB we stop doubling and instead start growing
the array by one at each ALLOC_GROW() call. This is because
alloc_nr() overflows to a small value, but the:
if (alloc_nr(alloc) < (nr))
alloc = (nr);
check kicks in.
- after your patch we grow to ~4GB, and then things get super slow.
This is because we correctly compute the new allocation as a size_t,
but then truncate it while assigning to alloc. So on the next
ALLOC_GROW() call, we'll think the buffer is way too small and try
to realloc again. I don't know why this is so much slower than the
grow-by-one above, but it is.
Neither is really correct, but both are in the realm of OK: stupidly
large input doesn't perform well, but there's no buffer overflow
vulnerability.
What I was worried about is what happens if you tweak your patch like
this:
diff --git a/git-compat-util.h b/git-compat-util.h
index 2bc1f43f48..0730dd24ad 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -870,7 +870,7 @@ static inline bool st_alloc_nr(size_t nr, size_t alloc, size_t *outp)
size_t alloc_grow_new_alloc_; \
if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
alloc = alloc_grow_new_alloc_; \
- REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
+ REALLOC_ARRAY(x, alloc); \
} \
} while (0)
In that case we really do end up with too-small allocations and
out-of-bounds writes.
Maybe you saw that coming and that's why you wrote it as you did. But it
is definitely subtle enough that I think it would merit a big warning
comment that "alloc" and "alloc_grow_new_alloc_" are not necessarily the
same type, and hence not necessarily the same value.
> Here's a demo program exercising the arithmetic part of the macros:
I think the difference isn't in the arithmetic values that come out, but
in what is fed to realloc() itself. And in your harness, realloc is just
"x = true". If you actually store the value that would be passed to
realloc() like this:
diff --git a/foo.c.orig b/foo.c
index 2fbce8c..7498f36 100644
--- a/foo.c.orig
+++ b/foo.c
@@ -11,7 +11,7 @@
alloc = (nr); \
else \
alloc = alloc_nr(alloc); \
- x = true; \
+ x = alloc; \
} \
} while (0)
@@ -31,7 +31,7 @@ static inline bool st_alloc_nr(size_t nr, size_t alloc, size_t *outp)
size_t alloc_grow_new_alloc_; \
if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
alloc = alloc_grow_new_alloc_; \
- x = true; \
+ x = alloc_grow_new_alloc_; \
} \
} while (0)
@@ -44,7 +44,7 @@ int main(int argc, char **argv)
for (T i = 0;; i++) {
for (T j = MIN;; j++) {
T alloc1 = j, alloc2 = j;
- bool allocated1 = false, allocated2 = false;
+ size_t allocated1 = 0, allocated2 = 0;
ALLOC_GROW1(allocated1, i, alloc1);
ALLOC_GROW2(allocated2, i, alloc2);
if (alloc1 != alloc2 || allocated1 != allocated2)
then you see the differences. For negative values, yeah, you end up with
big size_t values. But for an unsigned type you get different small
allocations.
-Peff
^ permalink raw reply related
* [PATCH 2/2] commit: handle large commit messages in utf8 verification
From: Jeff King @ 2026-05-16 2:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20260516021343.GA174647@coredump.intra.peff.net>
Running t4205 under UBSan with the EXPENSIVE prereq enabled triggers an
error when we try to create a commit message that is over 2GB:
commit.c:1574:6: runtime error: signed integer overflow:
-2147483648 - 1 cannot be represented in type 'int'
The problem is that find_invalid_utf8() is not prepared to handle
large buffers, as it uses an "int" to represent buffer sizes and
offsets.
We can fix this with a few changes:
1. We'll take in "len" as a size_t (which is what the caller has
anyway, since it's working with a strbuf).
2. We need to return a size_t to give the offset to the invalid utf8,
but we also need a sentinel value for "no invalid value"
(previously "-1"). Let's split these to return a bool for "found
invalid utf8" and then pass back the offset as an out-parameter.
We'll switch the function name to match the new semantics.
3. The caller in verify_utf8() uses a "long" to store buffer
positions, which is a bit funny. This goes back to 08a94a145c
(commit/commit-tree: correct latin1 to utf-8, 2012-06-28) and is
perhaps trying to match our use of "unsigned long" for object sizes
(though we don't care about it ever becoming negative here). This
should be a size_t, too, as some platforms (like Windows) still use
a 32-bit long on machines with 64-bit pointers.
4. The "bytes" field within find_invalid_utf() does not have range
problems. It is the number of bytes the utf8 sequence claims to
have, so is limited by how many bits can be set in a single 8-bit
byte. However, if we leave it as an "int" then the compiler will
complain about the sign mismatch when comparing it to "len". So
let's make it unsigned, too.
All of this is a little silly, of course, because 2GB text commit
messages are clearly nonsense. So we might consider rejecting them
outright, but it is easy enough to make these helper functions more
robust in the meantime.
Signed-off-by: Jeff King <peff@peff.net>
---
I tried to look carefully for any reasons why these variables could ever
be negative, but beyond the sentinel value in the return type, didn't
see one. But reviewers should double check. Note that "bad_offset =
offset-1" in the context looks suspicious, but we are guaranteed that
offset has been advanced at this point.
commit.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/commit.c b/commit.c
index 4385ae4329..8cf09be39d 100644
--- a/commit.c
+++ b/commit.c
@@ -1558,16 +1558,16 @@ int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree,
return result;
}
-static int find_invalid_utf8(const char *buf, int len)
+static bool has_invalid_utf8(const char *buf, size_t len, size_t *bad_offset)
{
- int offset = 0;
+ size_t offset = 0;
static const unsigned int max_codepoint[] = {
0x7f, 0x7ff, 0xffff, 0x10ffff
};
while (len) {
unsigned char c = *buf++;
- int bytes, bad_offset;
+ unsigned bytes;
unsigned int codepoint;
unsigned int min_val, max_val;
@@ -1578,7 +1578,7 @@ static int find_invalid_utf8(const char *buf, int len)
if (c < 0x80)
continue;
- bad_offset = offset-1;
+ *bad_offset = offset-1;
/*
* Count how many more high bits set: that's how
@@ -1595,11 +1595,11 @@ static int find_invalid_utf8(const char *buf, int len)
* codepoints beyond U+10FFFF, which are guaranteed never to exist.
*/
if (bytes < 1 || 3 < bytes)
- return bad_offset;
+ return true;
/* Do we *have* that many bytes? */
if (len < bytes)
- return bad_offset;
+ return true;
/*
* Place the encoded bits at the bottom of the value and compute the
@@ -1617,23 +1617,23 @@ static int find_invalid_utf8(const char *buf, int len)
codepoint <<= 6;
codepoint |= *buf & 0x3f;
if ((*buf++ & 0xc0) != 0x80)
- return bad_offset;
+ return true;
} while (--bytes);
/* Reject codepoints that are out of range for the sequence length. */
if (codepoint < min_val || codepoint > max_val)
- return bad_offset;
+ return true;
/* Surrogates are only for UTF-16 and cannot be encoded in UTF-8. */
if ((codepoint & 0x1ff800) == 0xd800)
- return bad_offset;
+ return true;
/* U+xxFFFE and U+xxFFFF are guaranteed non-characters. */
if ((codepoint & 0xfffe) == 0xfffe)
- return bad_offset;
+ return true;
/* So are anything in the range U+FDD0..U+FDEF. */
if (codepoint >= 0xfdd0 && codepoint <= 0xfdef)
- return bad_offset;
+ return true;
}
- return -1;
+ return false;
}
/*
@@ -1645,15 +1645,14 @@ static int find_invalid_utf8(const char *buf, int len)
static int verify_utf8(struct strbuf *buf)
{
int ok = 1;
- long pos = 0;
+ size_t pos = 0;
for (;;) {
- int bad;
+ size_t bad;
unsigned char c;
unsigned char replace[2];
- bad = find_invalid_utf8(buf->buf + pos, buf->len - pos);
- if (bad < 0)
+ if (!has_invalid_utf8(buf->buf + pos, buf->len - pos, &bad))
return ok;
pos += bad;
ok = 0;
--
2.54.0.490.gaeb18d0c26
^ permalink raw reply related
* [PATCH 1/2] apply: plug leak on "patch too large" error
From: Jeff King @ 2026-05-16 2:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20260516021343.GA174647@coredump.intra.peff.net>
In apply_patch(), we return immediately if read_patch_file() returns an
error. Traditionally this was OK, since an error from strbuf_read()
would restore the strbuf to its unallocated state.
But since f1c0e3946e (apply: reject patches larger than ~1 GiB,
2022-10-25), we may also return an error if we successfully read the
patch but it is too large. In this case we leak the strbuf contents when
apply_patch() returns.
You can see it in action by running t4141 under LSan with the EXPENSIVE
prereq enabled.
We can fix this in one of two places:
1. In read_patch_file(), we could release the buffer before returning
the error, behaving more like a raw strbuf_read() call.
2. In apply_patch(), we can release the strbuf ourselves before
returning.
I picked the latter, since it future proofs us against read_patch_file()
getting new error modes. We also have a cleanup label in that function
already, so now our error handling at this spot matches the rest of the
function (and all of the variables are initialized such that the rest of
the cleanup is correctly a noop at this point).
Signed-off-by: Jeff King <peff@peff.net>
---
apply.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/apply.c b/apply.c
index 4aa1694cfa..249248d4f2 100644
--- a/apply.c
+++ b/apply.c
@@ -4881,8 +4881,10 @@ static int apply_patch(struct apply_state *state,
state->patch_input_file = filename;
state->linenr = 1;
- if (read_patch_file(&buf, fd) < 0)
- return -128;
+ if (read_patch_file(&buf, fd) < 0) {
+ res = -128;
+ goto end;
+ }
offset = 0;
while (offset < buf.len) {
struct patch *patch;
--
2.54.0.490.gaeb18d0c26
^ permalink raw reply related
* Re: UBSan failing on expensive test(s)
From: Jeff King @ 2026-05-16 2:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <871pfcdyt0.fsf@gitster.g>
On Sat, May 16, 2026 at 08:43:07AM +0900, Junio C Hamano wrote:
> This started happening on 'next' that runs EXPENSIVE tests thanks to
> Dscho's recent updates to enable them in CI.
>
> https://github.com/git/git/actions/runs/25896439353/job/76110441841#step:10:2172
>
> It claims that """
>
> commit.c:1574:6: runtime error: signed integer overflow:
> -2147483648 - 1 cannot be represented in type 'int'
>
> """.
>
> Another is related in the sense that it used to be hidden behind
> EXPENSIVE prerequisite, but is probably unrelated.
>
> https://github.com/git/git/actions/runs/25896439353/job/76110441842#step:10:156
These patches should fix both.
[1/2]: apply: plug leak on "patch too large" error
[2/2]: commit: handle large commit messages in utf8 verification
apply.c | 6 ++++--
commit.c | 31 +++++++++++++++----------------
2 files changed, 19 insertions(+), 18 deletions(-)
-Peff
^ permalink raw reply
* [PATCH] pack-objects: fix typo in code comment
From: Clinton Phillips @ 2026-05-16 1:01 UTC (permalink / raw)
To: git; +Cc: Clinton Phillips
"accomodate" -> "accommodate". Pure typo fix, no behavior change.
Signed-off-by: Clinton Phillips <clintdotphillips@gmail.com>
---
builtin/pack-objects.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index dd2480a7..80606890 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1341,7 +1341,7 @@ static void write_pack_file(void)
* length of them as buffer length.
*
* Note that we need to subtract one though to
- * accomodate for the sideband byte.
+ * accommodate for the sideband byte.
*/
struct hashfd_options opts = {
.progress = progress_state,
--
2.49.0
^ permalink raw reply related
* [PATCH] Documentation: fix typos in user docs and release notes
From: Clinton Phillips @ 2026-05-16 1:00 UTC (permalink / raw)
To: git; +Cc: Clinton Phillips
Pure typo fixes, no semantic change.
- RelNotes/2.49.1.adoc: "updates to to Fedora base image" -> "updates to Fedora base image"
- RelNotes/2.49.0.adoc: "which turns out that that the object" -> "which turns out that the object"
- howto/revert-branch-rebase.adoc: "I happen to know that that merge" -> "I happen to know that the merge"
- config/bitmap-pseudo-merge.adoc: "psuedo-merge" -> "pseudo-merge"
- technical/bitmap-format.adoc: "included in the this psuedo-merge" -> "included in this pseudo-merge"
Signed-off-by: Clinton Phillips <clintdotphillips@gmail.com>
---
Documentation/RelNotes/2.49.0.adoc | 2 +-
Documentation/RelNotes/2.49.1.adoc | 2 +-
Documentation/config/bitmap-pseudo-merge.adoc | 2 +-
Documentation/howto/revert-branch-rebase.adoc | 2 +-
Documentation/technical/bitmap-format.adoc | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/RelNotes/2.49.0.adoc b/Documentation/RelNotes/2.49.0.adoc
index 494c8309..35684515 100644
--- a/Documentation/RelNotes/2.49.0.adoc
+++ b/Documentation/RelNotes/2.49.0.adoc
@@ -118,7 +118,7 @@ Fixes since v2.48
placeholder signal (e.g. "--option=<value>").
(merge 5b34dd08d0 as/long-option-help-i18n later to maint).
- * CI jobs gave sporadic failures, which turns out that that the
+ * CI jobs gave sporadic failures, which turns out that the
object finalization code was giving an error when it did not have
to.
(merge d7fcbe2c56 ps/object-collision-check later to maint).
diff --git a/Documentation/RelNotes/2.49.1.adoc b/Documentation/RelNotes/2.49.1.adoc
index c619e8b4..697f0f22 100644
--- a/Documentation/RelNotes/2.49.1.adoc
+++ b/Documentation/RelNotes/2.49.1.adoc
@@ -9,4 +9,4 @@ notes for v2.43.7 for details.
It also contains some updates to various CI bits to work around
and/or to adjust to the deprecation of use of Ubuntu 20.04 GitHub
-Actions CI, updates to to Fedora base image.
+Actions CI, updates to Fedora base image.
diff --git a/Documentation/config/bitmap-pseudo-merge.adoc b/Documentation/config/bitmap-pseudo-merge.adoc
index 1f264eca..a0604a41 100644
--- a/Documentation/config/bitmap-pseudo-merge.adoc
+++ b/Documentation/config/bitmap-pseudo-merge.adoc
@@ -88,4 +88,4 @@ more useful).
bitmapPseudoMerge.<name>.stableSize::
Determines the size (in number of commits) of a stable
- psuedo-merge bitmap. The default is `512`.
+ pseudo-merge bitmap. The default is `512`.
diff --git a/Documentation/howto/revert-branch-rebase.adoc b/Documentation/howto/revert-branch-rebase.adoc
index a3e5595a..d56dde64 100644
--- a/Documentation/howto/revert-branch-rebase.adoc
+++ b/Documentation/howto/revert-branch-rebase.adoc
@@ -27,7 +27,7 @@ $ git checkout -b revert-c99 master
Now I am on the 'revert-c99' branch. Let's figure out which commit to
revert. I happen to know that the top of the 'master' branch is a
merge, and its second parent (i.e. foreign commit I merged from) has
-the change I would want to undo. Further I happen to know that that
+the change I would want to undo. Further I happen to know that the
merge introduced 5 commits or so:
------------------------------------------------
diff --git a/Documentation/technical/bitmap-format.adoc b/Documentation/technical/bitmap-format.adoc
index bfb0ec7b..59859c80 100644
--- a/Documentation/technical/bitmap-format.adoc
+++ b/Documentation/technical/bitmap-format.adoc
@@ -338,7 +338,7 @@ the end of a `.bitmap` file. The format is as follows:
* One or more pseudo-merge bitmaps, each containing:
** `commits_bitmap`, an EWAH-compressed bitmap describing the set of
- commits included in the this psuedo-merge.
+ commits included in this pseudo-merge.
** `merge_bitmap`, an EWAH-compressed bitmap describing the union of
the set of objects reachable from all commits listed in the
--
2.49.0
^ permalink raw reply related
* Re: [RFC PATCH] approxidate: make "today" wrap to midnight
From: Junio C Hamano @ 2026-05-16 0:03 UTC (permalink / raw)
To: Tuomas Ahola; +Cc: git, Jeff King
In-Reply-To: <20260515205803.26211-1-taahol@utu.fi>
Tuomas Ahola <taahol@utu.fi> writes:
> Although some commands do reject invalid approxidate expressions,
> in other cases those are simply evaluated as the current time.
> Oftentimes that is a perfectly good compromise to handle silly
> requests, but it isn't without rough edges.
>
> Let's consider what "git log --since=today" should yield.
> As it happens that "today" isn't actually a valid approxidate
> format, the command currently tries to list commits with
> *future* timestamps. Perhaps it would make more sense if
> it returned the commits made since midnight---that is,
> during the current day.
I actually am of two minds about this.
What should "git log --until=today" do when you run it in the late
afternoon? Wouldn't you want to see what you did in the morning and
early afternoon?
Because we cannot define "today" as "--since=today means the latest
midnight and later, while --until=today means until the end of today
[*]" without introducing an extra hint to calls to approxidate() to
tell it who is calling for what, it is impossible to give these two
sensible behavior at the same time.
Side note: but because the existing history is all about the past,
"until the end of today" is by definition a synonym for "up to
now", so defining "today" the same as "now" would make "until"
behave just as sensibly as if we define it as "the end of today".
In practice, using "--until" to *not* truncate at all (which is what
--until=now or --until=end.of.today would essentially mean) has no
practical value, while "--since=beginning.of.today" does have more
utility, allowing you to specify "the last 8 hours and 50 minutes"
without knowing that it is now at 08:50 in the morning. So I am
still in favor of interpreting "today" as "the latest midnight, the
beginning of today" because that would give us a more useful
behavior than other possible definitions. We may want to strengthen
the justification behind the chosen definition of why we chose what
we chose over any other time in today with something like what I
said above, mentioning "--until".
Thanks.
^ permalink raw reply
* UBSan failing on expensive test(s)
From: Junio C Hamano @ 2026-05-15 23:43 UTC (permalink / raw)
To: git
This started happening on 'next' that runs EXPENSIVE tests thanks to
Dscho's recent updates to enable them in CI.
https://github.com/git/git/actions/runs/25896439353/job/76110441841#step:10:2172
It claims that """
commit.c:1574:6: runtime error: signed integer overflow:
-2147483648 - 1 cannot be represented in type 'int'
""".
Another is related in the sense that it used to be hidden behind
EXPENSIVE prerequisite, but is probably unrelated.
https://github.com/git/git/actions/runs/25896439353/job/76110441842#step:10:156
^ permalink raw reply
* [PATCH v7] revision.c: implement --max-count-oldest
From: Mirko Faina @ 2026-05-15 23:29 UTC (permalink / raw)
To: git
Cc: Mirko Faina, Junio C Hamano, Jeff King, Jean-Noël Avila,
Patrick Steinhardt, Tian Yuchen, Ben Knoble, Johannes Sixt,
Chris Torek
In-Reply-To: <ce8d1ff49ef418ae3720265a124ef53a959d289e.1778017966.git.mroik@delayed.space>
--max-count is a commit limiting option sets a maximum amount of commits
to be shown. If a user wants to see only the first N commits of the
history (the oldest commits) they'd have to do something like
git log $(git rev-list HEAD | tail -n N | head -n 1)
This is not very user-friendly.
Teach get_revision() the --max-count-oldest option.
Signed-off-by: Mirko Faina <mroik@delayed.space>
---
Since v6 I've simplified the docs, replaced die messages with
die_for_incompatible_opt2 and fixed graph output when used with
--boundary.
Documentation/rev-list-options.adoc | 5 +-
revision.c | 103 +++++++++++++++++++++++++++-
revision.h | 2 +
t/t4202-log.sh | 33 +++++++++
4 files changed, 139 insertions(+), 4 deletions(-)
diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index 2d195a1474..e8c88d0f1c 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -16,7 +16,10 @@ ordering and formatting options, such as `--reverse`.
`-<number>`::
`-n <number>`::
`--max-count=<number>`::
- Limit the output to _<number>_ commits.
+ Limit the output to the first _<number>_ commits that would be shown.
+
+`--max-count-oldest=<number>`::
+ Limit the output to the last _<number>_ commits that would be shown.
`--skip=<number>`::
Skip _<number>_ commits before starting to show the commit output.
diff --git a/revision.c b/revision.c
index 599b3a66c3..7fc79049b2 100644
--- a/revision.c
+++ b/revision.c
@@ -2339,10 +2339,28 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
}
if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
+ if (revs->max_count_type == 1)
+ die_for_incompatible_opt2(1, "--max-count", 1,
+ "--max-count-oldest");
revs->max_count = parse_count(optarg);
revs->no_walk = 0;
+ revs->max_count_type = 0;
return argcount;
+ } else if ((argcount = parse_long_opt("max-count-oldest", argv, &optarg))) {
+ if (revs->max_count_type == 0 && revs->max_count != -1)
+ die_for_incompatible_opt2(1, "--max-count", 1,
+ "--max-count-oldest");
+ if (revs->skip_count > 0)
+ die_for_incompatible_opt2(1, "--skip", 1,
+ "--max-count-oldest");
+ revs->max_count = parse_count(optarg);
+ revs->no_walk = 0;
+ revs->max_count_type = 1;
+ revs->max_count_stage = 0;
} else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
+ if (revs->max_count_type == 1)
+ die_for_incompatible_opt2(1, "--skip", 1,
+ "--max-count-oldest");
revs->skip_count = parse_count(optarg);
return argcount;
} else if ((*arg == '-') && isdigit(arg[1])) {
@@ -4521,15 +4539,83 @@ static struct commit *get_revision_internal(struct rev_info *revs)
return c;
}
+static void retrieve_oldest_commits(struct rev_info *revs,
+ struct commit_list **queue)
+{
+ struct commit *c;
+ int max_count = revs->max_count;
+ int queuei_count = 0;
+ int queueo_count = 0;
+ struct commit_list *queueo = NULL;
+ struct commit_list *queuei = NULL;
+ struct commit_list *reversed_queue = NULL;
+
+ revs->max_count = -1;
+ while ((c = get_revision_internal(revs))) {
+ /*
+ * We need to reset SHOWN status otherwise --graph breaks.
+ * It is fine to do, get_revision_internal() doesn't consider
+ * children commits as they have been already processed and the
+ * traversal happens only child to parent.
+ *
+ * We do this because the --graph machinery relies on the status
+ * of the parents to decide how the printing will happen.
+ *
+ * We can't simply replace this instruction with a
+ * graph_update() as it doesn't do the actualy printing, we'd
+ * have to remove any commit that goes over the
+ * --max-count-oldest limit from revs->graph.
+ */
+ c->object.flags &= ~(SHOWN | CHILD_SHOWN);
+ commit_list_insert(c, &queuei);
+ queuei_count++;
+ while (queuei_count + queueo_count > max_count) {
+ if (!queueo_count) {
+ while (queuei_count > 0) {
+ c = pop_commit(&queuei);
+ queuei_count--;
+ commit_list_insert(c, &queueo);
+ queueo_count++;
+ }
+ }
+ pop_commit(&queueo);
+ queueo_count--;
+ }
+ }
+
+ while ((c = pop_commit(&queueo)))
+ commit_list_insert(c, &reversed_queue);
+ while ((c = pop_commit(&queuei)))
+ commit_list_insert(c, &queueo);
+ while ((c = pop_commit(&queueo)))
+ commit_list_insert(c, &reversed_queue);
+
+ while ((c = pop_commit(&reversed_queue)))
+ commit_list_insert(c, queue);
+}
+
struct commit *get_revision(struct rev_info *revs)
{
struct commit *c;
struct commit_list *reversed;
+ struct commit_list *queue = NULL;
+ struct commit_list *p;
+
+ if (revs->max_count_type == 1 && !revs->max_count_stage) {
+ retrieve_oldest_commits(revs, &queue);
+ commit_list_free(revs->commits);
+ revs->commits = queue;
+ revs->max_count_stage = 1;
+ }
if (revs->reverse) {
reversed = NULL;
- while ((c = get_revision_internal(revs)))
- commit_list_insert(c, &reversed);
+ if (revs->max_count_type == 1)
+ while ((c = pop_commit(&revs->commits)))
+ commit_list_insert(c, &reversed);
+ else
+ while ((c = get_revision_internal(revs)))
+ commit_list_insert(c, &reversed);
commit_list_free(revs->commits);
revs->commits = reversed;
revs->reverse = 0;
@@ -4543,7 +4629,18 @@ struct commit *get_revision(struct rev_info *revs)
return c;
}
- c = get_revision_internal(revs);
+ if (revs->max_count_stage) {
+ c = pop_commit(&revs->commits);
+ if (c) {
+ c->object.flags |= SHOWN;
+ if (!(c->object.flags & BOUNDARY))
+ for (p = c->parents; p; p = p->next)
+ p->item->object.flags |= CHILD_SHOWN;
+ }
+ } else {
+ c = get_revision_internal(revs);
+ }
+
if (c && revs->graph)
graph_update(revs->graph, c);
if (!c) {
diff --git a/revision.h b/revision.h
index 584f1338b5..e157463cb1 100644
--- a/revision.h
+++ b/revision.h
@@ -309,6 +309,8 @@ struct rev_info {
/* special limits */
int skip_count;
int max_count;
+ unsigned int max_count_type:1;
+ unsigned int max_count_stage:1;
timestamp_t max_age;
timestamp_t max_age_as_filter;
timestamp_t min_age;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 05cee9e41b..8f2471e7e4 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1882,6 +1882,39 @@ test_expect_success 'log --graph with --name-status' '
test_cmp_graph --name-status tangle..reach
'
+test_expect_success 'log --max-count-oldest=3 --oneline' '
+ test_when_finished rm expect &&
+ git log --oneline | tail -n3 >expect &&
+ git log --oneline --max-count-oldest=3 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
+ test_when_finished rm expect &&
+ git log --oneline --reverse | head -n3 >expect &&
+ git log --oneline --max-count-oldest=3 --reverse >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'log --max-count-oldest with --max-count' '
+ test_when_finished rm stderr &&
+ test_must_fail git log --max-count-oldest=3 --max-count=3 2>stderr &&
+ test_grep "cannot be used together" stderr
+'
+
+test_expect_success 'log --max-count-oldest with --skip' '
+ test_when_finished rm stderr &&
+ test_must_fail git log --max-count-oldest=3 --skip=1 2>stderr &&
+ test_grep "cannot be used together" stderr
+'
+
+test_expect_success 'log --max-count-oldest=1000 --graph --boundary' '
+ test_when_finished rm expect actual &&
+ git log --graph --boundary >expect &&
+ git log --max-count-oldest=1000 --graph --boundary >actual &&
+ test_cmp expect actual
+'
+
cat >expect <<-\EOF
* reach
|
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] evaluate the second argument of ALLOC_GROW only once
From: René Scharfe @ 2026-05-15 23:01 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
In-Reply-To: <20260515195049.GA149960@coredump.intra.peff.net>
On 5/15/26 9:50 PM, Jeff King wrote:
> On Fri, May 15, 2026 at 03:08:18PM -0400, Jeff King wrote:
>
>> On Fri, May 15, 2026 at 08:16:50PM +0200, René Scharfe wrote:
>>
>>> + size_t alloc_grow_new_alloc_; \
>>> + if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
>>> + alloc = alloc_grow_new_alloc_; \
>>> + REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
>>> } \
>>
>> What happens if a caller passes in an argument that isn't a size_t?
>> We'll check for overflow in the size_t space, and then truncate it when
>> we assign to alloc, I think.
>>
>> I think we generally try to hold allocations in size_t these days, but
>> I'd be surprised if there weren't a few "int" holdouts. Grepping around,
>> alloc_node() seems to be an example.
>>
>> BTW, non-size_t arguments nullifies my earlier hand-waving around "nr +
>> 1 overflowing implies we've filled up the address space". But we are
>> still protected in the existing code by the:
>>
>> if (alloc_nr(alloc) < (nr))
>> alloc = (nr);
>>
>> logic. But with your patch, that all happens in the size_t space, so I
>> think it would actually introduce possible array overflows when the
>> caller is using a smaller type.
>
> Hmm, playing with it and looking a little closer, I think we don't end
> up overflowing the buffer because you use the size_t for
> REALLOC_ARRAY(). So the result is big, but then "alloc" is truncated.
>
> And then on the next call, we think "oh no, the allocation is way too
> small" because we are using the truncated value. So we try to size up
> for every single allocation, even though it's actually big enough, and
> the program slows to a crawl. ;)
>
> For reference, IU was using this hack to play around and demonstrate:
>
> diff --git a/git.c b/git.c
> index 5a40eab8a2..638bbc69e4 100644
> --- a/git.c
> +++ b/git.c
> @@ -969,6 +969,19 @@ int cmd_main(int argc, const char **argv)
>
> cmd = argv[0];
>
> + if (!strcmp(cmd, "foo")) {
> + unsigned char *buf = NULL;
> + unsigned nr = 0, alloc = 0;
> + for (unsigned i = 0; i < UINT_MAX; i++) {
> + ALLOC_GROW(buf, nr + 1, alloc);
> + if (i % 313370 == 0)
> + warning("at i=%u, alloc=%u, nr=%u", i, alloc, nr);
> + buf[nr++] = i % 256;
> + }
> + printf("done, final nr=%u, alloc=%u\n", nr, alloc);
> + return 0;
> + }
> +
> /*
> * We use PATH to find git commands, but we prepend some higher
> * precedence paths: the "--exec-path" option, the GIT_EXEC_PATH
>
>
> The same problem exists in several places in actual code, but I'm not
> sure how practical it is to trigger. The alloc_node() is counting not
> just objects, but blocks of objects. So you'd need 2*31 * 1024 objects
> of one type, which is probably going to run afoul of other limitations.
> Other cases are similar; for example "yes | git fetch-pack --stdin foo"
> will grow an array indefinitely, but at one strbuf per line it starts
> swapping on my 64GB machine at only 350M entries.
>
> I think as long as the behavior remains "slow, but we do not overflow
> any buffers" when you reach these limits, that's OK. Nobody is going to
> do it in practice, and we just want to make sure that malicious inputs
> cannot get out-of-bounds writes. It might be worth adding a comment,
> though, to make sure nobody ever swaps "alloc_grow_new_alloc_" for
> "alloc" in that macro.
There is no overflow check in either version (yet), so neither is safe
to operate close to the boundary. Close meaning the intermediate term
(alloc + 16) * 3 being bigger than the maximum value.
Does the size_t arithmetic make matters worse? The only change I can
see is that it interprets negative values as big unsigned ones and
then doesn't reallocate. The outcome for positive values is the same,
overflow and all, no?
Here's a demo program exercising the arithmetic part of the macros:
#include <limits.h>
#include <stdbool.h>
#include <stdio.h>
#define alloc_nr(x) (((x)+16)*3/2)
#define ALLOC_GROW1(x, nr, alloc) \
do { \
if ((nr) > alloc) { \
if (alloc_nr(alloc) < (nr)) \
alloc = (nr); \
else \
alloc = alloc_nr(alloc); \
x = true; \
} \
} while (0)
static inline bool st_alloc_nr(size_t nr, size_t alloc, size_t *outp)
{
if (nr > alloc) {
size_t out = alloc_nr(alloc);
*outp = out < nr ? nr : out;
return true;
}
return false;
}
#define ALLOC_GROW2(x, nr, alloc) \
do { \
size_t alloc_grow_new_alloc_; \
if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
alloc = alloc_grow_new_alloc_; \
x = true; \
} \
} while (0)
#define T signed char
#define MIN 0
#define MAX SCHAR_MAX
int main(int argc, char **argv)
{
for (T i = 0;; i++) {
for (T j = MIN;; j++) {
T alloc1 = j, alloc2 = j;
bool allocated1 = false, allocated2 = false;
ALLOC_GROW1(allocated1, i, alloc1);
ALLOC_GROW2(allocated2, i, alloc2);
if (alloc1 != alloc2 || allocated1 != allocated2)
printf("%zu %zu %d %zu %d %zu\n",
(size_t)i, (size_t)j,
allocated1, (size_t)alloc1,
allocated2, (size_t)alloc2);
if (j == MAX)
break;
}
if (i == MAX)
break;
}
return 0;
}
^ permalink raw reply
* Re: [PATCH] commit-reach: use the decoration hash for tips_reachable_from_bases()
From: Jeff King @ 2026-05-15 21:14 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget; +Cc: git, Kristofer Karlsson
In-Reply-To: <pull.2116.git.1778868463992.gitgitgadget@gmail.com>
On Fri, May 15, 2026 at 06:07:43PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> tips_reachable_from_bases() walks the commit graph from a set of base
> commits to find which tip commits are reachable. The inner loop does
> a linear scan over the tips array to check whether each visited commit
> is a tip, making the overall cost O(C * T) where C is commits walked
> and T is the number of tips.
>
> Replace the linear scan with the decoration hash for lookups, reducing
> the per-commit tip check from O(T) to O(1) and the overall cost from
> O(C * T) to O(C + T).
>
> This function is called by `git for-each-ref --merged` and
> `git branch/tag --contains/--no-contains` via reach_filter() in
> ref-filter.c.
>
> Benchmark on a merge-heavy monorepo (2.3M commits, 10,000 refs):
>
> Command Before After Speedup
> for-each-ref --merged HEAD 6.64s 1.66s 4.0x
> for-each-ref --no-merged HEAD 6.75s 1.74s 3.9x
> branch --merged HEAD 0.68s 0.61s 10%
> branch --no-merged HEAD 0.65s 0.61s 8%
> tag --merged HEAD 0.12s 0.12s -
>
> The large speedup for for-each-ref is because it checks all 10,000
> refs as tips, making the O(T) inner loop expensive. The branch
> subcommand only checks local branches (fewer tips), so the improvement
> is smaller.
Hmm, I couldn't reproduce the speedup on something like linux.git (~1.4M
commits) with a lot of synthetic branches. I'd think that old branches
would be the most expensive, so I did:
old=$(git rev-list --reverse HEAD | head -n1)
seq --format="update refs/heads/branch%g $old" 10000 |
git update-ref --stdin
Running "git for-each-ref --no-merged HEAD" takes ~650ms with stock Git.
But with your patch, it goes to ~830ms!
So what am I missing about your repo that it is so slow in the first
place?
> * Hacking the array index into the decoration value as (void *)(i + 1)
> instead of storing a proper pointer
The decoration API is not the most generic option here. There's an
oidmap type, but you have to embed the hashmap bits into your struct,
which is a lot of boilerplate if you're just storing an int. You can
define a khash with a custom value type, and I think the existing
oid_pos uses an int, which might be enough. All of those will store an
extra copy of the oid, though for the sizes we're talking about that's
not the end of the world.
Since we're always mapping commits, you could define a commit-slab (each
commit struct gets a unique id which we then index into a big array).
See commit-slab.h for an example.
I'm not very familiar with this code, but I wonder if we actually need
to map at all. It looks like we are mostly interested in set inclusion,
so perhaps an oidset() would work. Or even a bit in the object-flags.
-Peff
^ permalink raw reply
* [RFC PATCH] approxidate: make "today" wrap to midnight
From: Tuomas Ahola @ 2026-05-15 20:58 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Tuomas Ahola
Although some commands do reject invalid approxidate expressions,
in other cases those are simply evaluated as the current time.
Oftentimes that is a perfectly good compromise to handle silly
requests, but it isn't without rough edges.
Let's consider what "git log --since=today" should yield.
As it happens that "today" isn't actually a valid approxidate
format, the command currently tries to list commits with
*future* timestamps. Perhaps it would make more sense if
it returned the commits made since midnight---that is,
during the current day.
Moreover, a revision parameter "@{today}" is currently outright
rejected. Making "today" a valid approxidate time format could
make a natural way to specify the state of the ref at the start
of the current day.
Bind "today" to new function `date_today()` as an approxidate
special. Make it return the last midnight if no specific time
is given; i.e. retain the old behavior of "noon today" and such.
Signed-off-by: Tuomas Ahola <taahol@utu.fi>
---
Notes:
The "Jan 5 today" test is adapted from
c27cc94fad (approxidate: handle pending number for "specials", 2018-11-02):
> (saying "Jan 5 yesterday" should not respect the number at all).
date.c | 10 ++++++++++
t/t0006-date.sh | 2 ++
2 files changed, 12 insertions(+)
diff --git a/date.c b/date.c
index 17a95077cf..343d6aab6f 100644
--- a/date.c
+++ b/date.c
@@ -1192,6 +1192,15 @@ static void date_never(struct tm *tm, struct tm *now UNUSED, int *num)
*num = 0;
}
+static void date_today(struct tm *tm, struct tm *now, int *num UNUSED)
+{
+ if (tm->tm_hour == now->tm_hour &&
+ tm->tm_min == now->tm_min &&
+ tm->tm_sec == now->tm_sec)
+ date_time(tm, now, 0);
+ update_tm(tm, now, 0);
+}
+
static const struct special {
const char *name;
void (*fn)(struct tm *, struct tm *, int *);
@@ -1204,6 +1213,7 @@ static const struct special {
{ "AM", date_am },
{ "never", date_never },
{ "now", date_now },
+ { "today", date_today },
{ NULL }
};
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 53ced36df4..07bf6115ab 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -164,6 +164,7 @@ check_approxidate() {
}
check_approxidate now '2009-08-30 19:20:00'
+check_approxidate today '2009-08-30 00:00:00'
check_approxidate '5 seconds ago' '2009-08-30 19:19:55'
check_approxidate 5.seconds.ago '2009-08-30 19:19:55'
check_approxidate 10.minutes.ago '2009-08-30 19:10:00'
@@ -187,6 +188,7 @@ check_approxidate 'last tuesday' '2009-08-25 19:20:00'
check_approxidate 'July 5th' '2009-07-05 19:20:00'
check_approxidate '06/05/2009' '2009-06-05 19:20:00'
check_approxidate '06.05.2009' '2009-05-06 19:20:00'
+check_approxidate 'Jan 5 today' '2009-01-30 00:00:00'
check_approxidate 'Jun 6, 5AM' '2009-06-06 05:00:00'
check_approxidate '5AM Jun 6' '2009-06-06 05:00:00'
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
--
2.30.2
^ permalink raw reply related
* Re: [PATCH] evaluate the second argument of ALLOC_GROW only once
From: Jeff King @ 2026-05-15 19:50 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <20260515190818.GA98370@coredump.intra.peff.net>
On Fri, May 15, 2026 at 03:08:18PM -0400, Jeff King wrote:
> On Fri, May 15, 2026 at 08:16:50PM +0200, René Scharfe wrote:
>
> > + size_t alloc_grow_new_alloc_; \
> > + if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
> > + alloc = alloc_grow_new_alloc_; \
> > + REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
> > } \
>
> What happens if a caller passes in an argument that isn't a size_t?
> We'll check for overflow in the size_t space, and then truncate it when
> we assign to alloc, I think.
>
> I think we generally try to hold allocations in size_t these days, but
> I'd be surprised if there weren't a few "int" holdouts. Grepping around,
> alloc_node() seems to be an example.
>
> BTW, non-size_t arguments nullifies my earlier hand-waving around "nr +
> 1 overflowing implies we've filled up the address space". But we are
> still protected in the existing code by the:
>
> if (alloc_nr(alloc) < (nr))
> alloc = (nr);
>
> logic. But with your patch, that all happens in the size_t space, so I
> think it would actually introduce possible array overflows when the
> caller is using a smaller type.
Hmm, playing with it and looking a little closer, I think we don't end
up overflowing the buffer because you use the size_t for
REALLOC_ARRAY(). So the result is big, but then "alloc" is truncated.
And then on the next call, we think "oh no, the allocation is way too
small" because we are using the truncated value. So we try to size up
for every single allocation, even though it's actually big enough, and
the program slows to a crawl. ;)
For reference, IU was using this hack to play around and demonstrate:
diff --git a/git.c b/git.c
index 5a40eab8a2..638bbc69e4 100644
--- a/git.c
+++ b/git.c
@@ -969,6 +969,19 @@ int cmd_main(int argc, const char **argv)
cmd = argv[0];
+ if (!strcmp(cmd, "foo")) {
+ unsigned char *buf = NULL;
+ unsigned nr = 0, alloc = 0;
+ for (unsigned i = 0; i < UINT_MAX; i++) {
+ ALLOC_GROW(buf, nr + 1, alloc);
+ if (i % 313370 == 0)
+ warning("at i=%u, alloc=%u, nr=%u", i, alloc, nr);
+ buf[nr++] = i % 256;
+ }
+ printf("done, final nr=%u, alloc=%u\n", nr, alloc);
+ return 0;
+ }
+
/*
* We use PATH to find git commands, but we prepend some higher
* precedence paths: the "--exec-path" option, the GIT_EXEC_PATH
The same problem exists in several places in actual code, but I'm not
sure how practical it is to trigger. The alloc_node() is counting not
just objects, but blocks of objects. So you'd need 2*31 * 1024 objects
of one type, which is probably going to run afoul of other limitations.
Other cases are similar; for example "yes | git fetch-pack --stdin foo"
will grow an array indefinitely, but at one strbuf per line it starts
swapping on my 64GB machine at only 350M entries.
I think as long as the behavior remains "slow, but we do not overflow
any buffers" when you reach these limits, that's OK. Nobody is going to
do it in practice, and we just want to make sure that malicious inputs
cannot get out-of-bounds writes. It might be worth adding a comment,
though, to make sure nobody ever swaps "alloc_grow_new_alloc_" for
"alloc" in that macro.
-Peff
^ permalink raw reply related
* Re: [PATCH] evaluate the second argument of ALLOC_GROW only once
From: Jeff King @ 2026-05-15 19:08 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <323f5677-301b-4d7a-b552-6606597c2b1f@web.de>
On Fri, May 15, 2026 at 08:16:50PM +0200, René Scharfe wrote:
> + size_t alloc_grow_new_alloc_; \
> + if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
> + alloc = alloc_grow_new_alloc_; \
> + REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
> } \
What happens if a caller passes in an argument that isn't a size_t?
We'll check for overflow in the size_t space, and then truncate it when
we assign to alloc, I think.
I think we generally try to hold allocations in size_t these days, but
I'd be surprised if there weren't a few "int" holdouts. Grepping around,
alloc_node() seems to be an example.
BTW, non-size_t arguments nullifies my earlier hand-waving around "nr +
1 overflowing implies we've filled up the address space". But we are
still protected in the existing code by the:
if (alloc_nr(alloc) < (nr))
alloc = (nr);
logic. But with your patch, that all happens in the size_t space, so I
think it would actually introduce possible array overflows when the
caller is using a smaller type.
-Peff
^ permalink raw reply
* [PATCH] evaluate the second argument of ALLOC_GROW only once
From: René Scharfe @ 2026-05-15 18:16 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Allow the new element count passed to ALLOC_GROW to be a complex
expression with side-effects by evaluating it only once, as a parameter
to a new helper function.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
git-compat-util.h | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index ae1bdc90a4..2bc1f43f48 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -812,6 +812,16 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)
#define alloc_nr(x) (((x)+16)*3/2)
+static inline bool st_alloc_nr(size_t nr, size_t alloc, size_t *outp)
+{
+ if (nr > alloc) {
+ size_t out = alloc_nr(alloc);
+ *outp = out < nr ? nr : out;
+ return true;
+ }
+ return false;
+}
+
/**
* Dynamically growing an array using realloc() is error prone and boring.
*
@@ -857,12 +867,10 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)
*/
#define ALLOC_GROW(x, nr, alloc) \
do { \
- if ((nr) > alloc) { \
- if (alloc_nr(alloc) < (nr)) \
- alloc = (nr); \
- else \
- alloc = alloc_nr(alloc); \
- REALLOC_ARRAY(x, alloc); \
+ size_t alloc_grow_new_alloc_; \
+ if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
+ alloc = alloc_grow_new_alloc_; \
+ REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
} \
} while (0)
--
2.54.0
^ permalink raw reply related
* [PATCH] commit-reach: use the decoration hash for tips_reachable_from_bases()
From: Kristofer Karlsson via GitGitGadget @ 2026-05-15 18:07 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Kristofer Karlsson
From: Kristofer Karlsson <krka@spotify.com>
tips_reachable_from_bases() walks the commit graph from a set of base
commits to find which tip commits are reachable. The inner loop does
a linear scan over the tips array to check whether each visited commit
is a tip, making the overall cost O(C * T) where C is commits walked
and T is the number of tips.
Replace the linear scan with the decoration hash for lookups, reducing
the per-commit tip check from O(T) to O(1) and the overall cost from
O(C * T) to O(C + T).
This function is called by `git for-each-ref --merged` and
`git branch/tag --contains/--no-contains` via reach_filter() in
ref-filter.c.
Benchmark on a merge-heavy monorepo (2.3M commits, 10,000 refs):
Command Before After Speedup
for-each-ref --merged HEAD 6.64s 1.66s 4.0x
for-each-ref --no-merged HEAD 6.75s 1.74s 3.9x
branch --merged HEAD 0.68s 0.61s 10%
branch --no-merged HEAD 0.65s 0.61s 8%
tag --merged HEAD 0.12s 0.12s -
The large speedup for for-each-ref is because it checks all 10,000
refs as tips, making the O(T) inner loop expensive. The branch
subcommand only checks local branches (fewer tips), so the improvement
is smaller.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
commit-reach: use the decoration hash for tips_reachable_from_bases()
This is a single small commit that replaces an O(C*T) linear scan in
tips_reachable_from_bases() with an O(1) lookup using the decoration
hash.
The function is called by git for-each-ref --merged and git branch/tag
--contains/--no-contains via reach_filter() in ref-filter.c. On a
merge-heavy monorepo with 2.3M commits and 10,000 refs, git for-each-ref
--merged HEAD goes from 6.6s to 1.7s (4x).
The diff is intentionally minimal (+9/-6) to make the idea easy to
discuss before polishing. Things I'm not fully happy about:
* Extra block scope { } just to preserve indentation of the inner body
* Hacking the array index into the decoration value as (void *)(i + 1)
instead of storing a proper pointer
* Relying on unsigned wraparound (- 1 on a size_t 0) to check for
not-found via j < tips_nr
Happy to clean all of these up in a follow-up commit if the approach
makes sense.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2116%2Fspkrka%2Ftips-reachable-minimal-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2116/spkrka/tips-reachable-minimal-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2116
commit-reach.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index d3a9b3ed6f..70b056eae0 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -1150,6 +1150,7 @@ void tips_reachable_from_bases(struct repository *r,
size_t min_generation_index = 0;
timestamp_t min_generation;
struct commit_list *stack = NULL;
+ struct decoration tip_index = { "tip_index" };
if (!bases || !tips || !tips_nr)
return;
@@ -1173,6 +1174,10 @@ void tips_reachable_from_bases(struct repository *r,
QSORT(commits, tips_nr, compare_commit_and_index_by_generation);
min_generation = commits[0].generation;
+ for (size_t i = 0; i < tips_nr; i++)
+ add_decoration(&tip_index, &commits[i].commit->object,
+ (void *)(i + 1));
+
while (bases) {
repo_parse_commit(r, bases->item);
commit_list_insert(bases->item, &stack);
@@ -1183,14 +1188,11 @@ void tips_reachable_from_bases(struct repository *r,
int explored_all_parents = 1;
struct commit_list *p;
struct commit *c = stack->item;
- timestamp_t c_gen = commit_graph_generation(c);
/* Does it match any of our tips? */
- for (size_t j = min_generation_index; j < tips_nr; j++) {
- if (c_gen < commits[j].generation)
- break;
-
- if (commits[j].commit == c) {
+ {
+ size_t j = (size_t)lookup_decoration(&tip_index, &c->object) - 1;
+ if (j < tips_nr) {
tips[commits[j].index]->object.flags |= mark;
if (j == min_generation_index) {
@@ -1232,6 +1234,7 @@ void tips_reachable_from_bases(struct repository *r,
}
done:
+ clear_decoration(&tip_index, NULL);
free(commits);
repo_clear_commit_marks(r, SEEN);
commit_list_free(stack);
base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v3 5/8] environment: move "precomposed_unicode" into `struct repo_config_values`
From: Tian Yuchen @ 2026-05-15 17:15 UTC (permalink / raw)
To: Olamide Caleb Bello, git
Cc: phillip.wood123, gitster, christian.couder, usmanakinyemi202,
kaartic.sivaraam, me
In-Reply-To: <20260423165432.143598-6-belkid98@gmail.com>
On 4/24/26 00:54, Olamide Caleb Bello wrote:
> struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *prec_dir)
> {
> + struct repo_config_values *cfg = repo_config_values(the_repository);
> struct dirent *res;
> +
> res = readdir(prec_dir->dirp);
> if (res) {
> size_t namelenz = strlen(res->d_name) + 1; /* \0 */
This 'precompose_utf8_readdir()' appears to be a wrapper for
'readdir()', so that Git thinks it is calling a standard POSIX function.
Looking at it this way, I feel the architectural design here is a bit
ambiguous. I think the role of 'readdir()' (or its wrapper) should be:
"You give me a directory handle, and I give you a file entry".
Conceptually, it shouldn’t even be related to Git: you can take a look
at the rest of code of this wrapper.
Since there is a wrapper for 'readdir()', there must also be one for
'opendir()'. To me, initializing a flag like 'perform_precompose' when
the PREC_DIR handle is created in 'opendir()' , and pass it down through
the handle looks a bit more optimal. In other words, If one of them has
to handle the repo pointer, I think it would be better to leave that to
'opendir()' ;)
Good night, yuchen
^ permalink raw reply
* Re: What's cooking in git.git (May 2026, #03)
From: Junio C Hamano @ 2026-05-15 17:06 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <CAOLa=ZS8a2R79+8hD-r1JpJmHUatHo4VEC=ybjf02jLEHWsRoQ@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Hello Junio,
>
> I've not been active on the list past few weeks, did we reach a
> consensus about
> 20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com ? Or was
> it missed, I thought it was in a ready state, but happy to reiterate as
> needed.
>
> Lore: https://lore.kernel.org/git/20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com/#t
I've not been active on the list past few weeks, either, so please
don't expect me to know anything that happened during my 3-week
absense ;-)
My understanding of the status of that thread is that after
https://lore.kernel.org/git/CAOLa=ZT1zE+MLeaYE_5jWmNzSvtTTBw3ZAopai+2Ei27kmYm2g@mail.gmail.com/
that said you "Will add ... locally", we are all waiting for you to
say either "after waiting for sufficient amount of time, there
wasn't any other major change necessary, so I won't add it locally
after all" or "we have waited for sufficient amount of time, so here
is the hopefully final edition that includes what I added locally
following Patrick's review".
^ permalink raw reply
* Re: [PATCH] http: handle absolute-path alternates from server root
From: Jeff King @ 2026-05-15 17:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Junio C Hamano, git, slonkazoid
In-Reply-To: <agbOEsZ8NmE8SyfV@pks.im>
On Fri, May 15, 2026 at 09:41:06AM +0200, Patrick Steinhardt wrote:
> > We talked about dropping it a few years ago, but Eric countered that
> > dumb clones are easier on the server in some cases (like gigantic
> > public-inbox repos that are packed to keep most of the old history in
> > one big pack that is never updated). The verbatim pack-reuse feature
> > tries to get smart clones closer to that, but it's hard to beat serving
> > a static file from the server's perspective. I haven't measured anything
> > in that area in a while, though.
>
> In theory we can get much closer with packfile URIs, too, can't we? If
> the packfiles are directly accessible anyway the server could just
> announce these directly and have the client fetch them. That should
> significantly reduce the load on the server even further.
Packfile URIs help with the actual pack generation (even if we're
blitting out bits from the disk with verbatim packfile reuse, we still
have to handle gaps and compute the checksum over the output pack).
But it doesn't help with the server computing the set of objects the
client needs in the first place. IIRC, packfile URIs work by the server
saying "oh, I was going to send you object XYZ, but you can get it from
this stable pack instead". So the server still has to compute the set of
objects (and send any that are not mentioned in URI packs). Bitmaps
help, but there's still non-trivial computation and storage on the
server.
Contrast that with a client that instead pulls a packfile over dumb
storage on its own, and then comes to the server for a top-off fetch.
The server still has to do some computation, but it's usually quite
small, because both sides agree quickly that there's no need to dig down
further than the tips in that dumb packfile.
> Of course, the big downside is that "fetch.uriProtocols" is empty by
> default, so Git will not use them. Makes me wonder whether this is
> something we want to eventually change, but I guess the current default
> behaviour is somewhat insecure as it would allow the server to redirect
> clients to arbitrary locations. It would be great if we had a mechanism
> that only allowed packfile URIs that use the same host, which would make
> this a lot more reasonable to enable by default.
It's been a while since I've looked at it, but I seem to recall that the
server-side tools for specifying which packfile URIs to use were not
that mature. Maybe that has changed, though (I'm probably 5 years out of
date since the last time I really thought about these things).
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] use __builtin_add_overflow() in st_add() with Clang
From: Jeff King @ 2026-05-15 16:53 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <26b71f9c-0cc5-4bfd-9175-f45b584e202e@web.de>
On Fri, May 15, 2026 at 04:36:15PM +0200, René Scharfe wrote:
> > I guess the ideal primitive is probably a wrapper with the same
> > interface as __builtin_add_overflow(), which could then be used
> > everywhere that unsigned_add_overflows() with some minor conversion.
>
> Junio said the same. :)
> > But it gets awkward to do as a macro, and using an inline function runs
> > into type questions.
> Indeed. If it was easy then this wouldn't exist as a builtin. We
> can approximate it somewhat, but will it be robust enough?
I always thought it was a builtin because the most efficient way
involves checking the carry flag, which can't be accessed from C.
But yeah, the type issues are real, too. ;)
I think we should take what you posted for now, and we can iterate on a
more general add-and-check interface later (or never if it's too
tricky).
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] strbuf: use st_add3() in strbuf_grow()
From: Jeff King @ 2026-05-15 16:50 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Git List
In-Reply-To: <459f5f2b-2565-4dae-9f9f-8848a5cb9d94@web.de>
On Fri, May 15, 2026 at 04:30:34PM +0200, René Scharfe wrote:
> > That's all assuming that no overflow happens before ALLOC_GROW() gets
> > the values. We also tend to do unchecked computions for the "nr" field
> > there, but it's usually just "nr_foo + 1", so the same logic applies:
> > you'd have to have an existing array consuming the entire address space
> > minus one byte to trigger an overflow.
>
> The use in read-cache.c::do_read_index() looks odd. Has been present
> since commit one. Is the point that it over-allocates to have room for
> additions right from the start? For read-only commands this only wastes
> memory, no?
Hmm, yeah, that is weird, and unusual to use alloc_nr() directly. We are
presumably picking up istate->cache_nr from the on-disk file, so it
could be anything, and that alloc_nr() could overflow.
We'd store the too-small value in alloc, so we _know_ it's too small. So
later when we use ALLOC_GROW(), the problem would be resolved as we grow
the array. I'm not convinced the initial load might not overflow the
array, though.
-Peff
^ 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