* Re: [PATCH v3 2/4] approxidate: alias "today" to "now"
From: Junio C Hamano @ 2026-05-15 1:38 UTC (permalink / raw)
To: Tuomas Ahola; +Cc: git, Jeff King
In-Reply-To: <xmqqik8pea39.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Tuomas Ahola <taahol@utu.fi> writes:
>
>> Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> Tuomas Ahola <taahol@utu.fi> writes:
>>>
>>> > Sorry, I don't know if I understood. Does the patch change the behavior of
>>> > that command somehow? Is there some kind of edge case I missed?
>>>
>>> No, I did not think it was a good idea to carve the behaviour in
>>> stone that "git log --since=today" behaves as if it were given "git
>>> log --since=now". My reaction would have been very different if we
>>> were deliberatly and explicitly saying "today is synonym for now",
>>> but the thing is, it is not a designed behaviour but what
>>> approxidate does for anything it does not understand, e.g.
>>>
>>> git log --since=decay
>>> git log --since=bogus
>>>
>>> all behave as if it were given --since=now.
>>
>> Thanks for spelling that out. So, as there is no deliberative
>> decision behind the current behaviour of "today", the code has
>> to remain non-committed on that; we are not at liberty to codify
>> the status quo. Right?
>
> Not right. It is more like "Even though we try not to change
> existing behavoiur left and right without a good reason to avoid
> breaking existing users' expectations, we should be able to "fix"
> what is not intended behaviour but is something the code happened to
> be doing, especially if the current behaviour does not make sense.
And the other half of the discussion is that once we explicitly say
"today means right now" and make it official, it makes it much harder
to fix it later. So we need to be very careful in our first attempt.
^ permalink raw reply
* Re: [PATCH] fetch: add fetch.pruneLocalBranches config
From: Junio C Hamano @ 2026-05-15 1:28 UTC (permalink / raw)
To: Harald Nordgren; +Cc: git, gitgitgadget
In-Reply-To: <20260514221602.9918-1-haraldnordgren@gmail.com>
Harald Nordgren <haraldnordgren@gmail.com> writes:
>> I also wonder if, given that "=" in anywhere other than three-level
>> names, is invalid, we should just start accept
>>
>> git config foo.bar=baz
>> git config set foo.bar=baz
>>
>> and interpret them as
>>
>> git config set foo.bar baz
>
> That sounds good too! Probably even better.
>
>
> Harald
Why do I get the above, which apparently is a response to my review
for
[PATCH] config: suggest the correct form when key contains "="
under this thread? Am I dealing with some sort of mechanical slop?
^ permalink raw reply
* Re: [PATCH v3 2/4] approxidate: alias "today" to "now"
From: Junio C Hamano @ 2026-05-15 1:27 UTC (permalink / raw)
To: Tuomas Ahola; +Cc: git, Jeff King
In-Reply-To: <20260514210742.Yc6NZ%taahol@utu.fi>
Tuomas Ahola <taahol@utu.fi> writes:
> Junio C Hamano <gitster@pobox.com> wrote:
>
>> Tuomas Ahola <taahol@utu.fi> writes:
>>
>> > Sorry, I don't know if I understood. Does the patch change the behavior of
>> > that command somehow? Is there some kind of edge case I missed?
>>
>> No, I did not think it was a good idea to carve the behaviour in
>> stone that "git log --since=today" behaves as if it were given "git
>> log --since=now". My reaction would have been very different if we
>> were deliberatly and explicitly saying "today is synonym for now",
>> but the thing is, it is not a designed behaviour but what
>> approxidate does for anything it does not understand, e.g.
>>
>> git log --since=decay
>> git log --since=bogus
>>
>> all behave as if it were given --since=now.
>
> Thanks for spelling that out. So, as there is no deliberative
> decision behind the current behaviour of "today", the code has
> to remain non-committed on that; we are not at liberty to codify
> the status quo. Right?
Not right. It is more like "Even though we try not to change
existing behavoiur left and right without a good reason to avoid
breaking existing users' expectations, we should be able to "fix"
what is not intended behaviour but is something the code happened to
be doing, especially if the current behaviour does not make sense.
^ permalink raw reply
* [PATCH] fetch: add fetch.pruneLocalBranches config
From: Harald Nordgren @ 2026-05-14 22:16 UTC (permalink / raw)
To: gitster; +Cc: git, gitgitgadget, haraldnordgren
In-Reply-To: <xmqqqzndel8c.fsf@gitster.g>
> I also wonder if, given that "=" in anywhere other than three-level
> names, is invalid, we should just start accept
>
> git config foo.bar=baz
> git config set foo.bar=baz
>
> and interpret them as
>
> git config set foo.bar baz
That sounds good too! Probably even better.
Harald
^ permalink raw reply
* Re: [PATCH] trailer: change strbuf in-place in unfold_value()
From: Ramsay Jones @ 2026-05-14 21:30 UTC (permalink / raw)
To: René Scharfe, Git List; +Cc: Jeff King
In-Reply-To: <9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de>
On 14/05/2026 7:40 pm, René Scharfe wrote:
> Avoid an allocation by doing s/\n\s*/ /g (replacing NL and any following
> whitespace with a SP) right in the strbuf instead of copying the result
> to a temporary one and swapping them in the end. We can safely do that
> because the replacement is never longer than the original string.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Formatted with --function-context for easier review.
> Inspired by https://lore.kernel.org/git/20260513185408.GA147423@coredump.intra.peff.net/
>
> trailer.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index 470f86a4a2..b89fa12fe7 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -988,29 +988,25 @@ static int ends_with_blank_line(const char *buf, size_t len)
>
> static void unfold_value(struct strbuf *val)
> {
> - struct strbuf out = STRBUF_INIT;
> size_t i;
> + size_t pos = 0;
>
> - strbuf_grow(&out, val->len);
> i = 0;
> while (i < val->len) {
> char c = val->buf[i++];
> if (c == '\n') {
> /* Collapse continuation down to a single space. */
> while (i < val->len && isspace(val->buf[i]))
> i++;
> - strbuf_addch(&out, ' ');
> - } else {
> - strbuf_addch(&out, c);
> + val->buf[pos++] = ' ';
> + } else if (pos != i) {
Hmm, isn't 'pos' strictly (always) less than 'i' here? (note the post update
of 'i' when setting 'c' at the head of the loop).
> + val->buf[pos++] = c;
So, this (non-newline-or-'trailing'-space char) is always copied.
Not that it matters much (depending on how long the first line is, I doubt
the difference is measurable :) ).
[Unless I'm not reading it correctly, of course - in which case, oops!]
ATB,
Ramsay Jones
> }
> }
> + strbuf_setlen(val, pos);
>
> /* Empty lines may have left us with whitespace cruft at the edges */
> - strbuf_trim(&out);
> -
> - /* output goes back to val as if we modified it in-place */
> - strbuf_swap(&out, val);
> - strbuf_release(&out);
> + strbuf_trim(val);
> }
>
> static struct trailer_block *trailer_block_new(void)
^ permalink raw reply
* Re: [PATCH 1/3] daemon: fix IPv6 address corruption in lookup_hostname()
From: Junio C Hamano @ 2026-05-14 21:26 UTC (permalink / raw)
To: Sebastien Tardif via GitGitGadget; +Cc: git, Sebastien Tardif
In-Reply-To: <b2d81438117a716417a031c74b678a8f91701af4.1778773592.git.gitgitgadget@gmail.com>
"Sebastien Tardif via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Sebastien Tardif <sebtardif@ncf.ca>
>
> getaddrinfo() is called with AF_UNSPEC hints, so it may return IPv6
> results. However, the code unconditionally casts ai_addr to
> sockaddr_in and passes AF_INET to inet_ntop(). On IPv6-only hosts,
> this reads from the wrong struct offset, producing garbage IP
> addresses.
>
> Fix this by checking ai_family and extracting the address pointer
> into a local variable before calling inet_ntop() once with the
> correct family. Die on unexpected address families.
>
> Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
> ---
> daemon.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 0a7b1aae44..80fa0226d8 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -674,9 +674,20 @@ static void lookup_hostname(struct hostinfo *hi)
>
> gai = getaddrinfo(hi->hostname.buf, NULL, &hints, &ai);
> if (!gai) {
> - struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
> + void *addr;
> +
> + if (ai->ai_family == AF_INET) {
> + struct sockaddr_in *sa = (void *)ai->ai_addr;
> + addr = &sa->sin_addr;
> + } else if (ai->ai_family == AF_INET6) {
> + struct sockaddr_in6 *sa6 = (void *)ai->ai_addr;
> + addr = &sa6->sin6_addr;
> + } else {
> + die("unexpected address family: %d",
> + ai->ai_family);
> + }
The previous iteration used to more explicitly cast ai->ai_addr to
the target type, but the use of (void *) here is a cute way to make
the result shorter, which makes it a bit easier to read (it may take
readers a bit of practice to convince themselves that this type
conversion using (void *) as an intermediate type is perfectly fine,
though).
>
> - inet_ntop(AF_INET, &sin_addr->sin_addr,
> + inet_ntop(ai->ai_family, addr,
> addrbuf, sizeof(addrbuf));
> strbuf_addstr(&hi->ip_address, addrbuf);
^ permalink raw reply
* Re: [PATCH] config: suggest the correct form when key contains "="
From: Junio C Hamano @ 2026-05-14 21:26 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Harald Nordgren
In-Reply-To: <pull.2302.git.git.1778680725459.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> When a user types "git config foo.bar=baz", git_config_parse_key()
> rejects the key with "error: invalid key: foo.bar=baz" but gives no
> indication of what the user should have written. The mistake is a
> common one for users who reach for INI-file syntax or for the
> "--flag=value" convention used by other command-line tools.
>
> Since "=" is never a valid character in a config key, treat its
> presence as a strong signal of this specific mistake and follow the
> error with a one-line suggestion in the "(did you mean ...)" style
> used elsewhere in git, e.g.:
>
> $ git config pull.rebase=false
> error: invalid key: pull.rebase=false
> (did you mean "git config set pull.rebase false"?)
If the command line were
git config get foo.bar=baz
git config set foo.bar=baz nitfol
we shouldn't give an extra "did you mean?" at all.
The only cases you may want to do the "did you mean?" I think are
git config foo.bar=baz
git config set foo.bar=baz
And I think git_config_parse_key() is at a way too low level to tell
in what context we are seeing this faulty key to guess end-user's
intention to limit our "did you mean?"
I also wonder if, given that "=" in anywhere other than three-level
names, is invalid, we should just start accept
git config foo.bar=baz
git config set foo.bar=baz
and interpret them as
git config set foo.bar baz
We of course need to be careful about non-invalid keys, i.e.
git config foo.bar=baz.boo
is a request to read the value of that named variable, i.e.
[foo "bar=baz"]
boo = its value
so either you start offering unsolicited "did you mean?" or accepting
tokens with '=' in them as new style "set", you need to be extra
careful not to trigger a false positive.
^ permalink raw reply
* Re: [PATCH v3 2/4] approxidate: alias "today" to "now"
From: Tuomas Ahola @ 2026-05-14 21:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <xmqqwlx6f1fo.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> wrote:
> Tuomas Ahola <taahol@utu.fi> writes:
>
> > Sorry, I don't know if I understood. Does the patch change the behavior of
> > that command somehow? Is there some kind of edge case I missed?
>
> No, I did not think it was a good idea to carve the behaviour in
> stone that "git log --since=today" behaves as if it were given "git
> log --since=now". My reaction would have been very different if we
> were deliberatly and explicitly saying "today is synonym for now",
> but the thing is, it is not a designed behaviour but what
> approxidate does for anything it does not understand, e.g.
>
> git log --since=decay
> git log --since=bogus
>
> all behave as if it were given --since=now.
Thanks for spelling that out. So, as there is no deliberative
decision behind the current behaviour of "today", the code has
to remain non-committed on that; we are not at liberty to codify
the status quo. Right? But perhaps we can find a least common
denominator. "Today" means the current day (well, obviously),
and this seems to be enough to get "today at noon" and such to work:
```
static void date_today(struct tm *tm, struct tm *now, int *num)
{
*num = 0;
tm->tm_mday = now->tm_mday;
}
```
It gets the job done whitout setting in stone too much anything
that is up for debate. Do you see that could work?
--Tuomas
^ permalink raw reply
* Re: [PATCH 2/2] use __builtin_add_overflow() in st_add() with Clang
From: René Scharfe @ 2026-05-14 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jeff King
In-Reply-To: <xmqqjyt5g5zr.fsf@gitster.g>
On 5/14/26 9:12 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
>> Provide a variant of st_add() that wraps __builtin_add_overflow() to
>> help Clang optimize it. Use it on all platforms for simplicity.
>> ...
>> +/* Help Clang; GCC generates the same code for both variants. */
>> +#if defined(__clang__)
>> +static inline size_t st_add(size_t a, size_t b)
>> +{
>> + size_t sum;
>> + if (__builtin_add_overflow(a, b, &sum))
>> + die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
>> + (uintmax_t)a, (uintmax_t)b);
>> + return sum;
>> +}
>> +#else
>> static inline size_t st_add(size_t a, size_t b)
>> {
>> if (unsigned_add_overflows(a, b))
>> @@ -621,6 +632,7 @@ static inline size_t st_add(size_t a, size_t b)
>> (uintmax_t)a, (uintmax_t)b);
>> return a + b;
>> }
>> +#endif
>
> Makes me wonder if we tweaked unsigned_add_overflows() to take an
> extra *dst parameter to match __builtin_add_overflow(), which of
> course requires us to all of 18 callsites, it might make the whole
> thing a bit simpler. New uses of unsigned_add_overflows(), if we
> ever add them, would automatically benefit, right?
Hmm. It sounds like a lot of churn, but it would make sure that
we use the checked result and not check a + b and then go on and
use x + y because the code de-synced at some point.
How to do it, though? It needs to be generic and evaluate its
arguments only once. Perhaps like this?
diff --git a/git-compat-util.h b/git-compat-util.h
index ca89cfb0b3..27fbb622d7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -103,6 +103,21 @@ struct strbuf;
#define unsigned_add_overflows(a, b) \
((b) > maximum_unsigned_value_of_type(a) - (a))
+static bool uint_add_overflow(uintmax_t a, uintmax_t b,
+ uintmax_t *out, size_t out_size)
+{
+ if (b > UINTMAX_MAX - a)
+ return true;
+ a += b;
+ if (a > (UINTMAX_MAX >> (bitsizeof(uintmax_t) - CHAR_BIT * out_size)))
+ return true;
+ *out = a;
+ return false;
+}
+
+#define UINT_ADD_OVERFLOW(a, b, out) \
+ uint_add_overflow((a), (b), (out), sizeof(a))
+
/*
* Returns true if the multiplication of "a" and "b" will
* overflow. The types of "a" and "b" must match and must be unsigned.
@@ -616,10 +631,11 @@ int git_open_cloexec(const char *name, int flags);
static inline size_t st_add(size_t a, size_t b)
{
- if (unsigned_add_overflows(a, b))
+ size_t ret;
+ if (UINT_ADD_OVERFLOW(a, b, &ret))
die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
(uintmax_t)a, (uintmax_t)b);
- return a + b;
+ return ret;
}
#define st_add3(a,b,c) st_add(st_add((a),(b)),(c))
#define st_add4(a,b,c,d) st_add(st_add3((a),(b),(c)),(d))
^ permalink raw reply related
* Re: [PATCH 1/2] strbuf: use st_add3() in strbuf_grow()
From: René Scharfe @ 2026-05-14 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqo6ihg690.fsf@gitster.g>
On 5/14/26 9:07 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
>> Simplify the code by calling st_add3() to do overflow checks instead of
>> open-coding it. This changes the error message to include the offending
>> summands, which can be helpful when tracking down the cause.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> strbuf.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/strbuf.c b/strbuf.c
>> index 3e04addc22..bb04d3910e 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -106,12 +106,9 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
>> void strbuf_grow(struct strbuf *sb, size_t extra)
>> {
>> int new_buf = !sb->alloc;
>> - if (unsigned_add_overflows(extra, 1) ||
>> - unsigned_add_overflows(sb->len, extra + 1))
>> - die("you want to use way too much memory");
>> if (new_buf)
>> sb->buf = NULL;
>> - ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
>> + ALLOC_GROW(sb->buf, st_add3(sb->len, extra, 1), sb->alloc);
>
> ALLOC_GROW() being a macro that references its second argument three
> times, doesn't this rewrite rely on the compiler being clever enough
> to notice that checking for the same overflow three times is
> pointless and does it only once? I guess the original has the same
> issue already, so this may not be so bad but it makes me feel a bit
> queasy.
As long as it has no side-effect (as is the case for addition) and we
keep compiler optimization enabled we should be fine.
I guess the reason for having multiple references as opposed to
loading the value once into a private variable is to support arbitrary
types. In the end it needs to fit into a size_t, though. Something
like this could bring the reference count down to one:
--- 8< ---
diff --git a/git-compat-util.h b/git-compat-util.h
index ae1bdc90a4..ca89cfb0b3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -812,6 +812,15 @@ 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 *out)
+{
+ if (nr > alloc) {
+ *out = alloc_nr(alloc) < nr ? nr : alloc_nr(alloc);
+ return true;
+ }
+ return false;
+}
+
/**
* Dynamically growing an array using realloc() is error prone and boring.
*
@@ -857,12 +866,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)
--- >8 ---
Hmm, alloc_nr() doesn't do any overflow checking. It should, though,
shouldn't it?
René
^ permalink raw reply related
* Re: [PATCH] revision: use priority queue in limit_list()
From: Derrick Stolee @ 2026-05-14 19:57 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget, git; +Cc: Kristofer Karlsson
In-Reply-To: <pull.2114.git.1778777491939.gitgitgadget@gmail.com>
On 5/14/2026 12:51 PM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> limit_list() maintains a date-sorted work queue of commits using a
> linked list with commit_list_insert_by_date() for insertion. Each
> insertion walks the list to find the right position — O(n) per insert.
> In repositories with merge-heavy histories, the symmetric difference
> can contain thousands of commits, making this O(n) insertion the
> dominant cost.
Linear operations are bad, especially when multiplied by linear-ish
loops, causing quadratic behavior.
> Replace the sorted linked list with a prio_queue (binary heap). This
> gives O(log n) insertion and O(log n) extraction instead of O(n)
> insertion and O(1) extraction, which is a net win when the queue is
> large.
Yes, much better.
> The still_interesting() and everybody_uninteresting() helpers are
> updated to scan the prio_queue's contiguous array instead of walking a
> linked list. process_parents() already accepts both a commit_list and
> a prio_queue parameter, so the change in limit_list() simply switches
> which one is passed.
>
> Benchmark: git rev-list --left-right --count HEAD~N...HEAD
> Repository: 2.3M commits, merge-heavy DAG (monorepo)
> Best of 5 runs, times in seconds:
>
> commits in
> symmetric diff baseline patched speedup
> -------------- -------- ------- -------
> 10 0.01 0.01 1.0x
> 50 0.01 0.01 1.0x
> 3751 21.23 8.49 2.5x
> 4524 21.70 8.29 2.6x
> 10130 20.10 6.65 3.0x
>
> No change for small traversals; 2.5-3.0x faster when the queue grows
> to thousands of commits.
This is good. Is there any chance that you could demonstrate this with
any commits in the Git repo? It does have some interesting behavior,
especially around point releases that are independent from the 'master'
branch and thus could have lopsided symmetric differences using well-
established tag names.
> Switching to a prio_queue (binary heap) reduces insertion cost to O(log
> w), bringing total cost to O(N·log w). The practical result on the same
> repository:
>
> commits in
> symmetric diff before after speedup
> -------------- -------- ------- -------
> 3751 21.2s 8.5s 2.5x
> 4524 21.7s 8.3s 2.6x
> 10130 20.1s 6.6s 3.0x
Very nice! I notice that this data is in your cover letter, but
not the commit message. Is that intentional?
> This affects any command that triggers limit_list() — i.e., when
> revs->limited is set — including --left-right, --cherry-mark,
> --cherry-pick, --ancestry-path, bisect, and rebase's fork-point
> computation. The practical trigger is git status --ahead-behind on a
> branch that has diverged from upstream in a merge-heavy repository.
This also impacts 'git log --graph' when there is no serialized
commit-graph file. We are still using limit_list() in that case.
> The change is minimal (+21/−17 lines, single file) because
> process_parents() already accepts both a commit_list and a prio_queue
> parameter — limit_list() just switches which one it passes.
The key logic is turning the initial list into the starting
points for the priority queue and everything else is about
moving types around, it seems.
> @@ -1451,6 +1447,7 @@ static int limit_list(struct rev_info *revs)
> struct commit_list *newlist = NULL;
> struct commit_list **p = &newlist;
> struct commit *interesting_cache = NULL;
> + struct prio_queue queue = { .compare = compare_commits_by_commit_date };
Here, we are _not_ using generation numbers, which is correct
for this case because we are matching the date-based sorting
of the previous list.
> while (original_list) {
> struct commit *commit = pop_commit(&original_list);
> + prio_queue_put(&queue, commit);
> + }
> +
> + while (queue.nr) {
> + struct commit *commit = prio_queue_get(&queue);
> struct object *obj = &commit->object;
This is a fun reuse of lines to take the old "drain the
list as it is being mutated" loop and turn it into "fill
the priority queue" and "drain the priority queue as it
is being mutated"
This code change looks good. No new tests are needed, since
this is a performance-only change. Do any of the tests in
t/perf/ demonstrate this improvement?
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v4 0/3] Avoid hardcoded "good"/"bad" bisect terms
From: Junio C Hamano @ 2026-05-14 19:56 UTC (permalink / raw)
To: Jonas Rebmann; +Cc: git, Chris Down, Jeff King, Phillip Wood
In-Reply-To: <20260514-bisect-terms-v4-0-b3e3cf1b06ce@schlaraffenlan.de>
Jonas Rebmann <kernel@schlaraffenlan.de> writes:
> While checking whether all output messages of git bisect were covered by
> [PATCH 1/3] bisect: use selected alternate terms in status output I
> found hardcoded good/bad refs leading to incompatibility of git
> rev-parse --bisect with alternate bisect run terms. This is addressed by
> [PATCH 3/3] rev-parse: use selected alternate terms to look up refs
>
> Signed-off-by: Jonas Rebmann <kernel@schlaraffenlan.de>
> ---
> Changes in v4:
> - Always print bisect terms in single quote (Thanks, Junio)
> - Split out quoting of bisect terms into separate commit
>> - Link to v3: https://patch.msgid.link/20260417-bisect-terms-v3-0-d659fa547261@schlaraffenlan.de
Having this "Link" is better than nothing, but it alone is not
sufficient to help those who mechanise patch consumption with b4.
https://b4.docs.kernel.org/en/latest/index.html
Please make sure that your cover letter of the (i+1)th iteration
[v(i+1) 0/N] is a reply to the cover letter of the i-th iteration
[v(i) 0/M]. With that, anybody who has the i-th iteration can
1. Check out the i-th iteration, e.g.,
$ git checkout jr/bisect-custom-terms-in-output
2. Peek at any of the commits on the topic branch with notes/amlog
e.g.,
$ git notes --ref=amlog show HEAD
3. Check out the base, e.g.,
$ git checkout --detach master...
Note that the branch that holds the i-th iteration can now be
accessed via @{-1} (i.e., the branch previously checked out).
4. Tell B4 to fetch the latest round of the same series, by giving
one/any of the message-ID we learned earlier, and apply them.
$ b4 am -o- \
"<20260417-bisect-terms-v3-2-d659fa547261@schlaraffenlan.de>" |
git am -s
5. See what changed with range-diff
$ git range-diff @{-1}...
6. When satisified, replace the topic with the new iteration.
$ git checkout -B @{-1}
It is crucial that b4 can find the newer iteration when given a
message-ID from the older iteration in step 4 for this workflow to
work, and for that, [v4 0/3] must be a reply to [v3 0/2]. Otherwise
b4 will say that v3 is still the last iteration and we cannot make
progress.
> Changes in v3:
> - when referencing newly introduced terms, reference them in single
> quotes (Thanks, Phillip)
> - Prefer test_grep over grep in updated Tests (Thanks, Phillip)
> - Improve commit messages (Thanks, Phillip)
> - Don't leak memory after read_bisect_terms() (Thanks, Phillip)
> - Don't leak memory after xstrfmt() (Thanks, Junio)
> - Add test case to patch 2/2
> - Link to v2: https://patch.msgid.link/20260323-bisect-terms-v2-0-8d6bdb2c9c7e@schlaraffenlan.de
With these improvements, v3 was already in a quite good shape, but
the latest makes it look even better. Will replace. Thanks.
^ permalink raw reply
* Re: [PATCH] revision: use priority queue in limit_list()
From: Junio C Hamano @ 2026-05-14 19:40 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget; +Cc: git, Kristofer Karlsson
In-Reply-To: <pull.2114.git.1778777491939.gitgitgadget@gmail.com>
"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> Benchmark: git rev-list --left-right --count HEAD~N...HEAD
> Repository: 2.3M commits, merge-heavy DAG (monorepo)
> Best of 5 runs, times in seconds:
>
> commits in
> symmetric diff baseline patched speedup
> -------------- -------- ------- -------
> 10 0.01 0.01 1.0x
> 50 0.01 0.01 1.0x
> 3751 21.23 8.49 2.5x
> 4524 21.70 8.29 2.6x
> 10130 20.10 6.65 3.0x
>
> No change for small traversals; 2.5-3.0x faster when the queue grows
> to thousands of commits.
Impressive.
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> revision: use priority queue in limit_list()
> ...
> This affects any command that triggers limit_list() — i.e., when
> revs->limited is set — including --left-right, --cherry-mark,
> --cherry-pick, --ancestry-path, bisect, and rebase's fork-point
> computation. The practical trigger is git status --ahead-behind on a
> branch that has diverged from upstream in a merge-heavy repository.
I found this description a bit curious. Notably missing from the
above list of revs->limited users is a bog standard A..B and it is
unclear the omission is because that case is not improved and if so
why.
I think a major reason of the omission of A..B from the above is,
despite my recollection that such a range (i.e., any presense of
UNINTERESTING commit) computation _always_ worked on a limited list,
these days we conditionally do not when we have commit graph and we
are showing in --topo-order (which is implicitly enabled when many
options other than --topo-order is in effect) since 1b4d8827
(revision: use generation for A..B --topo-order queries, 2019-05-21).
It might be interesting to extend your benchmark over the same
history with the same command line, perhaps with and without an
explicit "--topo-order" added, in a repository _without_
commit-graph enabled.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/3] daemon: fix network address handling bugs
From: Junio C Hamano @ 2026-05-14 19:20 UTC (permalink / raw)
To: Sebastien Tardif via GitGitGadget; +Cc: git, Sebastien Tardif
In-Reply-To: <pull.2300.git.git.1778773592.gitgitgadget@gmail.com>
"Sebastien Tardif via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Fix three related issues in daemon.c's network address handling:
Thanks for separating patches so that each of them addresses one
specific issue.
It would have been better if you sent this series as [PATCH v2] as a
reply to <pull.2299.git.git.1778291290159.gitgitgadget@gmail.com>,
which is the previous round. That way, the mailing list archive
will keep the related discussions together on the same page. If we
visit the page for the cover letter I am responding to,
https://lore.kernel.org/git/pull.2300.git.git.1778773592.gitgitgadget@gmail.com/
nobody can see that there was a previous iteration so those who
looked at the earlier effort cannot refer back to it and compare.
> IPv6 address corruption in lookup_hostname(): getaddrinfo() is called with
> AF_UNSPEC hints, so it may return IPv6 results. However, the code
> unconditionally casts ai_addr to sockaddr_in and passes AF_INET to
> inet_ntop(). On IPv6-only hosts, this reads from the wrong struct offset,
> producing garbage IP addresses. Fixed by checking ai_family and handling
> both AF_INET and AF_INET6.
>
> IPv6 address truncation in ip2str(): The sockaddr struct size (ai_addrlen)
> is passed as the output buffer size to inet_ntop(). For IPv6,
> sizeof(sockaddr_in6) is 28 bytes but INET6_ADDRSTRLEN is 46, so long IPv6
> addresses are silently truncated. Fixed by passing sizeof(ip) instead, and
> dropping the now-unused len parameter.
>
> NULL pointer in execute() logging: REMOTE_PORT environment variable is used
> in a format string without a NULL check (only REMOTE_ADDR was checked). If
> REMOTE_PORT is unset, NULL is passed to printf's %s, which is undefined
> behavior. Fixed by using a fallback string.
>
> Sebastien Tardif (3):
> daemon: fix IPv6 address corruption in lookup_hostname()
> daemon: fix IPv6 address truncation in ip2str()
> daemon: guard NULL REMOTE_PORT in execute() logging
>
> daemon.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
>
> base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2300%2FSebTardif%2Ffix%2Fdaemon-ipv6-and-null-port-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2300/SebTardif/fix/daemon-ipv6-and-null-port-v1
> Pull-Request: https://github.com/git/git/pull/2300
^ permalink raw reply
* Re: [PATCH 2/2] use __builtin_add_overflow() in st_add() with Clang
From: Junio C Hamano @ 2026-05-14 19:12 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King
In-Reply-To: <0ded6062-f66a-4713-af24-d1b5aa654823@web.de>
René Scharfe <l.s.r@web.de> writes:
> Provide a variant of st_add() that wraps __builtin_add_overflow() to
> help Clang optimize it. Use it on all platforms for simplicity.
> ...
> +/* Help Clang; GCC generates the same code for both variants. */
> +#if defined(__clang__)
> +static inline size_t st_add(size_t a, size_t b)
> +{
> + size_t sum;
> + if (__builtin_add_overflow(a, b, &sum))
> + die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
> + (uintmax_t)a, (uintmax_t)b);
> + return sum;
> +}
> +#else
> static inline size_t st_add(size_t a, size_t b)
> {
> if (unsigned_add_overflows(a, b))
> @@ -621,6 +632,7 @@ static inline size_t st_add(size_t a, size_t b)
> (uintmax_t)a, (uintmax_t)b);
> return a + b;
> }
> +#endif
Makes me wonder if we tweaked unsigned_add_overflows() to take an
extra *dst parameter to match __builtin_add_overflow(), which of
course requires us to all of 18 callsites, it might make the whole
thing a bit simpler. New uses of unsigned_add_overflows(), if we
ever add them, would automatically benefit, right?
^ permalink raw reply
* Re: [PATCH 1/2] strbuf: use st_add3() in strbuf_grow()
From: Junio C Hamano @ 2026-05-14 19:07 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <c6e9b337-c4fc-4cbd-ac32-e8d3814749b0@web.de>
René Scharfe <l.s.r@web.de> writes:
> Simplify the code by calling st_add3() to do overflow checks instead of
> open-coding it. This changes the error message to include the offending
> summands, which can be helpful when tracking down the cause.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> strbuf.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index 3e04addc22..bb04d3910e 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -106,12 +106,9 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
> void strbuf_grow(struct strbuf *sb, size_t extra)
> {
> int new_buf = !sb->alloc;
> - if (unsigned_add_overflows(extra, 1) ||
> - unsigned_add_overflows(sb->len, extra + 1))
> - die("you want to use way too much memory");
> if (new_buf)
> sb->buf = NULL;
> - ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
> + ALLOC_GROW(sb->buf, st_add3(sb->len, extra, 1), sb->alloc);
ALLOC_GROW() being a macro that references its second argument three
times, doesn't this rewrite rely on the compiler being clever enough
to notice that checking for the same overflow three times is
pointless and does it only once? I guess the original has the same
issue already, so this may not be so bad but it makes me feel a bit
queasy.
> if (new_buf)
> sb->buf[0] = '\0';
> }
^ permalink raw reply
* [PATCH] trailer: change strbuf in-place in unfold_value()
From: René Scharfe @ 2026-05-14 18:40 UTC (permalink / raw)
To: Git List; +Cc: Jeff King
Avoid an allocation by doing s/\n\s*/ /g (replacing NL and any following
whitespace with a SP) right in the strbuf instead of copying the result
to a temporary one and swapping them in the end. We can safely do that
because the replacement is never longer than the original string.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Formatted with --function-context for easier review.
Inspired by https://lore.kernel.org/git/20260513185408.GA147423@coredump.intra.peff.net/
trailer.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/trailer.c b/trailer.c
index 470f86a4a2..b89fa12fe7 100644
--- a/trailer.c
+++ b/trailer.c
@@ -988,29 +988,25 @@ static int ends_with_blank_line(const char *buf, size_t len)
static void unfold_value(struct strbuf *val)
{
- struct strbuf out = STRBUF_INIT;
size_t i;
+ size_t pos = 0;
- strbuf_grow(&out, val->len);
i = 0;
while (i < val->len) {
char c = val->buf[i++];
if (c == '\n') {
/* Collapse continuation down to a single space. */
while (i < val->len && isspace(val->buf[i]))
i++;
- strbuf_addch(&out, ' ');
- } else {
- strbuf_addch(&out, c);
+ val->buf[pos++] = ' ';
+ } else if (pos != i) {
+ val->buf[pos++] = c;
}
}
+ strbuf_setlen(val, pos);
/* Empty lines may have left us with whitespace cruft at the edges */
- strbuf_trim(&out);
-
- /* output goes back to val as if we modified it in-place */
- strbuf_swap(&out, val);
- strbuf_release(&out);
+ strbuf_trim(val);
}
static struct trailer_block *trailer_block_new(void)
--
2.54.0
^ permalink raw reply related
* [PATCH v4 7/7] odb/transaction: make `write_object_stream()` pluggable
From: Justin Tobler @ 2026-05-14 18:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, peff, Justin Tobler
In-Reply-To: <20260514183740.1505171-1-jltobler@gmail.com>
How an ODB transaction handles writing objects is expected to vary
between implementations. Introduce a new `write_object_stream()`
callback in `struct odb_transaction` to make this function pluggable.
Rename `index_blob_packfile_transaction()` to
`odb_transaction_files_write_object_stream()` and wire it up for use
with `struct odb_transaction_files` accordingly.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
object-file.c | 16 +++++++++-------
odb/transaction.c | 7 +++++++
odb/transaction.h | 25 ++++++++++++++++++++++---
3 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/object-file.c b/object-file.c
index 0d492e6962..23f665df90 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1578,9 +1578,10 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction
* binary blobs, they generally do not want to get any conversion, and
* callers should avoid this code path when filters are requested.
*/
-static int index_blob_packfile_transaction(struct odb_transaction *base,
- struct odb_write_stream *stream,
- size_t size, struct object_id *result_oid)
+static int odb_transaction_files_write_object_stream(struct odb_transaction *base,
+ struct odb_write_stream *stream,
+ size_t size,
+ struct object_id *result_oid)
{
struct odb_transaction_files *transaction = container_of(base,
struct odb_transaction_files,
@@ -1664,10 +1665,10 @@ int index_fd(struct index_state *istate, struct object_id *oid,
struct object_database *odb = the_repository->objects;
struct odb_transaction *transaction = odb_transaction_begin(odb);
- ret = index_blob_packfile_transaction(odb->transaction,
- &stream,
- xsize_t(st->st_size),
- oid);
+ ret = odb_transaction_write_object_stream(odb->transaction,
+ &stream,
+ xsize_t(st->st_size),
+ oid);
odb_transaction_commit(transaction);
} else {
ret = hash_blob_stream(&stream,
@@ -2132,6 +2133,7 @@ struct odb_transaction *odb_transaction_files_begin(struct odb_source *source)
transaction = xcalloc(1, sizeof(*transaction));
transaction->base.source = source;
transaction->base.commit = odb_transaction_files_commit;
+ transaction->base.write_object_stream = odb_transaction_files_write_object_stream;
return &transaction->base;
}
diff --git a/odb/transaction.c b/odb/transaction.c
index 592ac84075..b16e07aebf 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -26,3 +26,10 @@ void odb_transaction_commit(struct odb_transaction *transaction)
transaction->source->odb->transaction = NULL;
free(transaction);
}
+
+int odb_transaction_write_object_stream(struct odb_transaction *transaction,
+ struct odb_write_stream *stream,
+ size_t len, struct object_id *oid)
+{
+ return transaction->write_object_stream(transaction, stream, len, oid);
+}
diff --git a/odb/transaction.h b/odb/transaction.h
index a56e392f21..854fda06f5 100644
--- a/odb/transaction.h
+++ b/odb/transaction.h
@@ -12,14 +12,24 @@
*
* Each ODB source is expected to implement its own transaction handling.
*/
-struct odb_transaction;
-typedef void (*odb_transaction_commit_fn)(struct odb_transaction *transaction);
struct odb_transaction {
/* The ODB source the transaction is opened against. */
struct odb_source *source;
/* The ODB source specific callback invoked to commit a transaction. */
- odb_transaction_commit_fn commit;
+ void (*commit)(struct odb_transaction *transaction);
+
+ /*
+ * This callback is expected to write the given object stream into
+ * the ODB transaction. Note that for now, only blobs support streaming.
+ *
+ * The resulting object ID shall be written into the out pointer. The
+ * callback is expected to return 0 on success, a negative error code
+ * otherwise.
+ */
+ int (*write_object_stream)(struct odb_transaction *transaction,
+ struct odb_write_stream *stream, size_t len,
+ struct object_id *oid);
};
/*
@@ -35,4 +45,13 @@ struct odb_transaction *odb_transaction_begin(struct object_database *odb);
*/
void odb_transaction_commit(struct odb_transaction *transaction);
+/*
+ * Writes the object in the provided stream into the transaction. The resulting
+ * object ID is written into the out pointer. Returns 0 on success, a negative
+ * error code otherwise.
+ */
+int odb_transaction_write_object_stream(struct odb_transaction *transaction,
+ struct odb_write_stream *stream,
+ size_t len, struct object_id *oid);
+
#endif
--
2.54.0.105.g59ff4886a5
^ permalink raw reply related
* [PATCH v4 6/7] object-file: generalize packfile writes to use odb_write_stream
From: Justin Tobler @ 2026-05-14 18:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, peff, Justin Tobler
In-Reply-To: <20260514183740.1505171-1-jltobler@gmail.com>
The `index_blob_packfile_transaction()` function streams blob data
directly from an fd. This makes it difficult to reuse as part of a
generic transactional object writing interface.
Refactor the packfile write path to operate on a `struct
odb_write_stream`, allowing callers to supply data from arbitrary
sources.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
object-file.c | 56 +++++++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 26 deletions(-)
diff --git a/object-file.c b/object-file.c
index 6d7afdb723..0d492e6962 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1447,18 +1447,19 @@ static int hash_blob_stream(struct odb_write_stream *stream,
}
/*
- * Read the contents from fd for size bytes, streaming it to the
+ * Read the contents from the stream provided, streaming it to the
* packfile in state while updating the hash in ctx.
*/
static void stream_blob_to_pack(struct transaction_packfile *state,
- struct git_hash_ctx *ctx, int fd, size_t size,
- const char *path)
+ struct git_hash_ctx *ctx, size_t size,
+ struct odb_write_stream *stream)
{
git_zstream s;
unsigned char ibuf[16384];
unsigned char obuf[16384];
unsigned hdrlen;
int status = Z_OK;
+ size_t bytes_read = 0;
git_deflate_init(&s, pack_compression_level);
@@ -1467,23 +1468,21 @@ static void stream_blob_to_pack(struct transaction_packfile *state,
s.avail_out = sizeof(obuf) - hdrlen;
while (status != Z_STREAM_END) {
- if (size && !s.avail_in) {
- size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
- ssize_t read_result = read_in_full(fd, ibuf, rsize);
- if (read_result < 0)
- die_errno("failed to read from '%s'", path);
- if ((size_t)read_result != rsize)
- die("failed to read %u bytes from '%s'",
- (unsigned)rsize, path);
+ if (!stream->is_finished && !s.avail_in) {
+ ssize_t rsize = odb_write_stream_read(stream, ibuf,
+ sizeof(ibuf));
+
+ if (rsize < 0)
+ die("failed to read blob data");
git_hash_update(ctx, ibuf, rsize);
s.next_in = ibuf;
s.avail_in = rsize;
- size -= rsize;
+ bytes_read += rsize;
}
- status = git_deflate(&s, size ? 0 : Z_FINISH);
+ status = git_deflate(&s, stream->is_finished ? Z_FINISH : 0);
if (!s.avail_out || status == Z_STREAM_END) {
size_t written = s.next_out - obuf;
@@ -1503,6 +1502,11 @@ static void stream_blob_to_pack(struct transaction_packfile *state,
die("unexpected deflate failure: %d", status);
}
}
+
+ if (bytes_read != size)
+ die("read %" PRIuMAX " bytes of blob data, but expected %" PRIuMAX " bytes",
+ (uintmax_t)bytes_read, (uintmax_t)size);
+
git_deflate_end(&s);
}
@@ -1574,10 +1578,13 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction
* binary blobs, they generally do not want to get any conversion, and
* callers should avoid this code path when filters are requested.
*/
-static int index_blob_packfile_transaction(struct odb_transaction_files *transaction,
- struct object_id *result_oid, int fd,
- size_t size, const char *path)
+static int index_blob_packfile_transaction(struct odb_transaction *base,
+ struct odb_write_stream *stream,
+ size_t size, struct object_id *result_oid)
{
+ struct odb_transaction_files *transaction = container_of(base,
+ struct odb_transaction_files,
+ base);
struct transaction_packfile *state = &transaction->packfile;
struct git_hash_ctx ctx;
unsigned char obuf[16384];
@@ -1611,7 +1618,7 @@ static int index_blob_packfile_transaction(struct odb_transaction_files *transac
hashfile_checkpoint(state->f, &checkpoint);
idx->offset = state->offset;
crc32_begin(state->f);
- stream_blob_to_pack(state, &ctx, fd, size, path);
+ stream_blob_to_pack(state, &ctx, size, stream);
git_hash_final_oid(result_oid, &ctx);
idx->crc32 = crc32_end(state->f);
@@ -1655,15 +1662,12 @@ int index_fd(struct index_state *istate, struct object_id *oid,
if (flags & INDEX_WRITE_OBJECT) {
struct object_database *odb = the_repository->objects;
- struct odb_transaction_files *files_transaction;
- struct odb_transaction *transaction;
-
- transaction = odb_transaction_begin(odb);
- files_transaction = container_of(odb->transaction,
- struct odb_transaction_files,
- base);
- ret = index_blob_packfile_transaction(files_transaction, oid, fd,
- xsize_t(st->st_size), path);
+ struct odb_transaction *transaction = odb_transaction_begin(odb);
+
+ ret = index_blob_packfile_transaction(odb->transaction,
+ &stream,
+ xsize_t(st->st_size),
+ oid);
odb_transaction_commit(transaction);
} else {
ret = hash_blob_stream(&stream,
--
2.54.0.105.g59ff4886a5
^ permalink raw reply related
* [PATCH v4 5/7] object-file: avoid fd seekback by checking object size upfront
From: Justin Tobler @ 2026-05-14 18:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, peff, Justin Tobler
In-Reply-To: <20260514183740.1505171-1-jltobler@gmail.com>
In certain scenarios, Git handles writing blobs that exceed
"core.bigFileThreshold" differently by streaming the object directly
into a packfile. When there is an active ODB transaction, these blobs
are streamed to the same packfile instead of using a separate packfile
for each. If "pack.packSizeLimit" is configured and streaming another
object causes the packfile to exceed the configured limit, the packfile
is truncated back to the previous object and the object write is
restarted in a new packfile.
This works fine, but requires the fd being read from to save a
checkpoint so it becomes possible to rewind the input source via seeking
back to a known offset at the beginning. In a subsequent commit, blob
streaming is converted to use `struct odb_write_stream` as a more
generic input source instead of an fd which doesn't provide a mechanism
for rewinding.
For this use case though, rewinding the fd is not strictly necessary
because the inflated size of the object is known and can be used to
approximate whether writing the object would cause the packfile to
exceed the configured limit prior to writing anything. These blobs
written to the packfile are never deltified thus the size difference
between what is written versus the inflated size is due to zlib
compression. While this does prevent packfiles from being filled to the
potential maximum is some cases, it should be good enough and still
prevents the packfile from exceeding any configured limit.
Use the inflated blob size to determine whether writing an object to a
packfile will exceed the configured "pack.packSizeLimit".
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
object-file.c | 86 +++++++++++++++------------------------------------
1 file changed, 25 insertions(+), 61 deletions(-)
diff --git a/object-file.c b/object-file.c
index a59030911f..6d7afdb723 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1448,29 +1448,17 @@ static int hash_blob_stream(struct odb_write_stream *stream,
/*
* Read the contents from fd for size bytes, streaming it to the
- * packfile in state while updating the hash in ctx. Signal a failure
- * by returning a negative value when the resulting pack would exceed
- * the pack size limit and this is not the first object in the pack,
- * so that the caller can discard what we wrote from the current pack
- * by truncating it and opening a new one. The caller will then call
- * us again after rewinding the input fd.
- *
- * The already_hashed_to pointer is kept untouched by the caller to
- * make sure we do not hash the same byte when we are called
- * again. This way, the caller does not have to checkpoint its hash
- * status before calling us just in case we ask it to call us again
- * with a new pack.
+ * packfile in state while updating the hash in ctx.
*/
-static int stream_blob_to_pack(struct transaction_packfile *state,
- struct git_hash_ctx *ctx, off_t *already_hashed_to,
- int fd, size_t size, const char *path)
+static void stream_blob_to_pack(struct transaction_packfile *state,
+ struct git_hash_ctx *ctx, int fd, size_t size,
+ const char *path)
{
git_zstream s;
unsigned char ibuf[16384];
unsigned char obuf[16384];
unsigned hdrlen;
int status = Z_OK;
- off_t offset = 0;
git_deflate_init(&s, pack_compression_level);
@@ -1487,15 +1475,9 @@ static int stream_blob_to_pack(struct transaction_packfile *state,
if ((size_t)read_result != rsize)
die("failed to read %u bytes from '%s'",
(unsigned)rsize, path);
- offset += rsize;
- if (*already_hashed_to < offset) {
- size_t hsize = offset - *already_hashed_to;
- if (rsize < hsize)
- hsize = rsize;
- if (hsize)
- git_hash_update(ctx, ibuf, hsize);
- *already_hashed_to = offset;
- }
+
+ git_hash_update(ctx, ibuf, rsize);
+
s.next_in = ibuf;
s.avail_in = rsize;
size -= rsize;
@@ -1506,14 +1488,6 @@ static int stream_blob_to_pack(struct transaction_packfile *state,
if (!s.avail_out || status == Z_STREAM_END) {
size_t written = s.next_out - obuf;
- /* would we bust the size limit? */
- if (state->nr_written &&
- pack_size_limit_cfg &&
- pack_size_limit_cfg < state->offset + written) {
- git_deflate_abort(&s);
- return -1;
- }
-
hashwrite(state->f, obuf, written);
state->offset += written;
s.next_out = obuf;
@@ -1530,7 +1504,6 @@ static int stream_blob_to_pack(struct transaction_packfile *state,
}
}
git_deflate_end(&s);
- return 0;
}
static void flush_packfile_transaction(struct odb_transaction_files *transaction)
@@ -1606,48 +1579,39 @@ static int index_blob_packfile_transaction(struct odb_transaction_files *transac
size_t size, const char *path)
{
struct transaction_packfile *state = &transaction->packfile;
- off_t seekback, already_hashed_to;
struct git_hash_ctx ctx;
unsigned char obuf[16384];
unsigned header_len;
struct hashfile_checkpoint checkpoint;
struct pack_idx_entry *idx;
- seekback = lseek(fd, 0, SEEK_CUR);
- if (seekback == (off_t)-1)
- return error("cannot find the current offset");
-
header_len = format_object_header((char *)obuf, sizeof(obuf),
OBJ_BLOB, size);
transaction->base.source->odb->repo->hash_algo->init_fn(&ctx);
git_hash_update(&ctx, obuf, header_len);
+ /*
+ * If writing another object to the packfile could result in it
+ * exceeding the configured size limit, flush the current packfile
+ * transaction.
+ *
+ * Note that this uses the inflated object size as an approximation.
+ * Blob objects written in this manner are not delta-compressed, so
+ * the difference between the inflated and on-disk size is limited
+ * to zlib compression and is sufficient for this check.
+ */
+ if (state->nr_written && pack_size_limit_cfg &&
+ pack_size_limit_cfg < state->offset + size)
+ flush_packfile_transaction(transaction);
+
CALLOC_ARRAY(idx, 1);
prepare_packfile_transaction(transaction);
hashfile_checkpoint_init(state->f, &checkpoint);
- already_hashed_to = 0;
-
- while (1) {
- prepare_packfile_transaction(transaction);
- hashfile_checkpoint(state->f, &checkpoint);
- idx->offset = state->offset;
- crc32_begin(state->f);
-
- if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
- fd, size, path))
- break;
- /*
- * Writing this object to the current pack will make
- * it too big; we need to truncate it, start a new
- * pack, and write into it.
- */
- hashfile_truncate(state->f, &checkpoint);
- state->offset = checkpoint.offset;
- flush_packfile_transaction(transaction);
- if (lseek(fd, seekback, SEEK_SET) == (off_t)-1)
- return error("cannot seek back");
- }
+ hashfile_checkpoint(state->f, &checkpoint);
+ idx->offset = state->offset;
+ crc32_begin(state->f);
+ stream_blob_to_pack(state, &ctx, fd, size, path);
git_hash_final_oid(result_oid, &ctx);
idx->crc32 = crc32_end(state->f);
--
2.54.0.105.g59ff4886a5
^ permalink raw reply related
* [PATCH v4 4/7] object-file: remove flags from transaction packfile writes
From: Justin Tobler @ 2026-05-14 18:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, peff, Justin Tobler
In-Reply-To: <20260514183740.1505171-1-jltobler@gmail.com>
The `index_blob_packfile_transaction()` function handles streaming a
blob from an fd to compute its object ID and conditionally writes the
object directly to a packfile if the INDEX_WRITE_OBJECT flag is set. A
subsequent commit will make these packfile object writes part of the
transaction interface. Consequently, having the object write be
conditional on this flag is a bit awkward.
In preparation for this change, introduce a dedicated
`hash_blob_stream()` helper that only computes the OID from a `struct
odb_write_stream`. This is invoked by `index_fd()` instead when the
INDEX_WRITE_OBJECT is not set. The object write performed via
`index_blob_packfile_transaction()` is made unconditional accordingly.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
object-file.c | 132 +++++++++++++++++++++++++++++-------------------
odb/streaming.c | 46 +++++++++++++++++
odb/streaming.h | 12 +++++
3 files changed, 138 insertions(+), 52 deletions(-)
diff --git a/object-file.c b/object-file.c
index a1afca23c5..a59030911f 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1397,11 +1397,10 @@ static int already_written(struct odb_transaction_files *transaction,
}
/* Lazily create backing packfile for the state */
-static void prepare_packfile_transaction(struct odb_transaction_files *transaction,
- unsigned flags)
+static void prepare_packfile_transaction(struct odb_transaction_files *transaction)
{
struct transaction_packfile *state = &transaction->packfile;
- if (!(flags & INDEX_WRITE_OBJECT) || state->f)
+ if (state->f)
return;
state->f = create_tmp_packfile(transaction->base.source->odb->repo,
@@ -1414,6 +1413,39 @@ static void prepare_packfile_transaction(struct odb_transaction_files *transacti
die_errno("unable to write pack header");
}
+static int hash_blob_stream(struct odb_write_stream *stream,
+ const struct git_hash_algo *hash_algo,
+ struct object_id *result_oid, size_t size)
+{
+ unsigned char buf[16384];
+ struct git_hash_ctx ctx;
+ unsigned header_len;
+ size_t bytes_hashed = 0;
+
+ header_len = format_object_header((char *)buf, sizeof(buf),
+ OBJ_BLOB, size);
+ hash_algo->init_fn(&ctx);
+ git_hash_update(&ctx, buf, header_len);
+
+ while (!stream->is_finished) {
+ ssize_t read_result = odb_write_stream_read(stream, buf,
+ sizeof(buf));
+
+ if (read_result < 0)
+ return -1;
+
+ git_hash_update(&ctx, buf, read_result);
+ bytes_hashed += read_result;
+ }
+
+ if (bytes_hashed != size)
+ return -1;
+
+ git_hash_final_oid(result_oid, &ctx);
+
+ return 0;
+}
+
/*
* Read the contents from fd for size bytes, streaming it to the
* packfile in state while updating the hash in ctx. Signal a failure
@@ -1431,15 +1463,13 @@ static void prepare_packfile_transaction(struct odb_transaction_files *transacti
*/
static int stream_blob_to_pack(struct transaction_packfile *state,
struct git_hash_ctx *ctx, off_t *already_hashed_to,
- int fd, size_t size, const char *path,
- unsigned flags)
+ int fd, size_t size, const char *path)
{
git_zstream s;
unsigned char ibuf[16384];
unsigned char obuf[16384];
unsigned hdrlen;
int status = Z_OK;
- int write_object = (flags & INDEX_WRITE_OBJECT);
off_t offset = 0;
git_deflate_init(&s, pack_compression_level);
@@ -1474,20 +1504,18 @@ static int stream_blob_to_pack(struct transaction_packfile *state,
status = git_deflate(&s, size ? 0 : Z_FINISH);
if (!s.avail_out || status == Z_STREAM_END) {
- if (write_object) {
- size_t written = s.next_out - obuf;
-
- /* would we bust the size limit? */
- if (state->nr_written &&
- pack_size_limit_cfg &&
- pack_size_limit_cfg < state->offset + written) {
- git_deflate_abort(&s);
- return -1;
- }
-
- hashwrite(state->f, obuf, written);
- state->offset += written;
+ size_t written = s.next_out - obuf;
+
+ /* would we bust the size limit? */
+ if (state->nr_written &&
+ pack_size_limit_cfg &&
+ pack_size_limit_cfg < state->offset + written) {
+ git_deflate_abort(&s);
+ return -1;
}
+
+ hashwrite(state->f, obuf, written);
+ state->offset += written;
s.next_out = obuf;
s.avail_out = sizeof(obuf);
}
@@ -1575,8 +1603,7 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction
*/
static int index_blob_packfile_transaction(struct odb_transaction_files *transaction,
struct object_id *result_oid, int fd,
- size_t size, const char *path,
- unsigned flags)
+ size_t size, const char *path)
{
struct transaction_packfile *state = &transaction->packfile;
off_t seekback, already_hashed_to;
@@ -1584,7 +1611,7 @@ static int index_blob_packfile_transaction(struct odb_transaction_files *transac
unsigned char obuf[16384];
unsigned header_len;
struct hashfile_checkpoint checkpoint;
- struct pack_idx_entry *idx = NULL;
+ struct pack_idx_entry *idx;
seekback = lseek(fd, 0, SEEK_CUR);
if (seekback == (off_t)-1)
@@ -1595,33 +1622,26 @@ static int index_blob_packfile_transaction(struct odb_transaction_files *transac
transaction->base.source->odb->repo->hash_algo->init_fn(&ctx);
git_hash_update(&ctx, obuf, header_len);
- /* Note: idx is non-NULL when we are writing */
- if ((flags & INDEX_WRITE_OBJECT) != 0) {
- CALLOC_ARRAY(idx, 1);
-
- prepare_packfile_transaction(transaction, flags);
- hashfile_checkpoint_init(state->f, &checkpoint);
- }
+ CALLOC_ARRAY(idx, 1);
+ prepare_packfile_transaction(transaction);
+ hashfile_checkpoint_init(state->f, &checkpoint);
already_hashed_to = 0;
while (1) {
- prepare_packfile_transaction(transaction, flags);
- if (idx) {
- hashfile_checkpoint(state->f, &checkpoint);
- idx->offset = state->offset;
- crc32_begin(state->f);
- }
+ prepare_packfile_transaction(transaction);
+ hashfile_checkpoint(state->f, &checkpoint);
+ idx->offset = state->offset;
+ crc32_begin(state->f);
+
if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
- fd, size, path, flags))
+ fd, size, path))
break;
/*
* Writing this object to the current pack will make
* it too big; we need to truncate it, start a new
* pack, and write into it.
*/
- if (!idx)
- BUG("should not happen");
hashfile_truncate(state->f, &checkpoint);
state->offset = checkpoint.offset;
flush_packfile_transaction(transaction);
@@ -1629,8 +1649,6 @@ static int index_blob_packfile_transaction(struct odb_transaction_files *transac
return error("cannot seek back");
}
git_hash_final_oid(result_oid, &ctx);
- if (!idx)
- return 0;
idx->crc32 = crc32_end(state->f);
if (already_written(transaction, result_oid)) {
@@ -1668,18 +1686,28 @@ int index_fd(struct index_state *istate, struct object_id *oid,
ret = index_core(istate, oid, fd, xsize_t(st->st_size),
type, path, flags);
} else {
- struct object_database *odb = the_repository->objects;
- struct odb_transaction_files *files_transaction;
- struct odb_transaction *transaction;
-
- transaction = odb_transaction_begin(odb);
- files_transaction = container_of(odb->transaction,
- struct odb_transaction_files,
- base);
- ret = index_blob_packfile_transaction(files_transaction, oid, fd,
- xsize_t(st->st_size),
- path, flags);
- odb_transaction_commit(transaction);
+ struct odb_write_stream stream;
+ odb_write_stream_from_fd(&stream, fd, xsize_t(st->st_size));
+
+ if (flags & INDEX_WRITE_OBJECT) {
+ struct object_database *odb = the_repository->objects;
+ struct odb_transaction_files *files_transaction;
+ struct odb_transaction *transaction;
+
+ transaction = odb_transaction_begin(odb);
+ files_transaction = container_of(odb->transaction,
+ struct odb_transaction_files,
+ base);
+ ret = index_blob_packfile_transaction(files_transaction, oid, fd,
+ xsize_t(st->st_size), path);
+ odb_transaction_commit(transaction);
+ } else {
+ ret = hash_blob_stream(&stream,
+ the_repository->hash_algo, oid,
+ xsize_t(st->st_size));
+ }
+
+ odb_write_stream_release(&stream);
}
close(fd);
diff --git a/odb/streaming.c b/odb/streaming.c
index a68dd2cbe3..20531e864c 100644
--- a/odb/streaming.c
+++ b/odb/streaming.c
@@ -237,6 +237,11 @@ ssize_t odb_write_stream_read(struct odb_write_stream *st, void *buf, size_t sz)
return st->read(st, buf, sz);
}
+void odb_write_stream_release(struct odb_write_stream *st)
+{
+ free(st->data);
+}
+
int odb_stream_blob_to_fd(struct object_database *odb,
int fd,
const struct object_id *oid,
@@ -292,3 +297,44 @@ int odb_stream_blob_to_fd(struct object_database *odb,
odb_read_stream_close(st);
return result;
}
+
+struct read_object_fd_data {
+ int fd;
+ size_t remaining;
+};
+
+static ssize_t read_object_fd(struct odb_write_stream *stream,
+ unsigned char *buf, size_t len)
+{
+ struct read_object_fd_data *data = stream->data;
+ ssize_t read_result;
+ size_t count;
+
+ if (stream->is_finished)
+ return 0;
+
+ count = data->remaining < len ? data->remaining : len;
+ read_result = read_in_full(data->fd, buf, count);
+ if (read_result < 0 || (size_t)read_result != count)
+ return -1;
+
+ data->remaining -= count;
+ if (!data->remaining)
+ stream->is_finished = 1;
+
+ return read_result;
+}
+
+void odb_write_stream_from_fd(struct odb_write_stream *stream, int fd,
+ size_t size)
+{
+ struct read_object_fd_data *data;
+
+ CALLOC_ARRAY(data, 1);
+ data->fd = fd;
+ data->remaining = size;
+
+ stream->data = data;
+ stream->read = read_object_fd;
+ stream->is_finished = 0;
+}
diff --git a/odb/streaming.h b/odb/streaming.h
index 65ced911fe..2a8cac19a4 100644
--- a/odb/streaming.h
+++ b/odb/streaming.h
@@ -5,6 +5,7 @@
#define STREAMING_H 1
#include "object.h"
+#include "odb.h"
struct object_database;
struct odb_read_stream;
@@ -65,6 +66,11 @@ struct odb_write_stream {
ssize_t odb_write_stream_read(struct odb_write_stream *stream, void *buf,
size_t len);
+/*
+ * Releases memory allocated for underlying stream data.
+ */
+void odb_write_stream_release(struct odb_write_stream *stream);
+
/*
* Look up the object by its ID and write the full contents to the file
* descriptor. The object must be a blob, or the function will fail. When
@@ -82,4 +88,10 @@ int odb_stream_blob_to_fd(struct object_database *odb,
struct stream_filter *filter,
int can_seek);
+/*
+ * Sets up an ODB write stream that reads from an fd.
+ */
+void odb_write_stream_from_fd(struct odb_write_stream *stream, int fd,
+ size_t size);
+
#endif /* STREAMING_H */
--
2.54.0.105.g59ff4886a5
^ permalink raw reply related
* [PATCH v4 3/7] odb: update `struct odb_write_stream` read() callback
From: Justin Tobler @ 2026-05-14 18:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, peff, Justin Tobler
In-Reply-To: <20260514183740.1505171-1-jltobler@gmail.com>
The `read()` callback used by `struct odb_write_stream` currently
returns a pointer to an internal buffer along with the number of bytes
read. This makes buffer ownership unclear and provides no way to report
errors.
Update the interface to instead require the caller to provide a buffer,
and have the callback return the number of bytes written to it or a
negative value on error. While at it, also move the `struct
odb_write_stream` definition to "odb/streaming.h". Call sites are
updated accordingly.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
builtin/unpack-objects.c | 20 ++++++++------------
object-file.c | 15 ++++++++++++---
odb.h | 6 +-----
odb/streaming.c | 5 +++++
odb/streaming.h | 18 ++++++++++++++++++
5 files changed, 44 insertions(+), 20 deletions(-)
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index bc9b1e047e..64e58e79fd 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -9,6 +9,7 @@
#include "hex.h"
#include "object-file.h"
#include "odb.h"
+#include "odb/streaming.h"
#include "odb/transaction.h"
#include "object.h"
#include "delta.h"
@@ -360,24 +361,21 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,
struct input_zstream_data {
git_zstream *zstream;
- unsigned char buf[8192];
int status;
};
-static const void *feed_input_zstream(struct odb_write_stream *in_stream,
- unsigned long *readlen)
+static ssize_t feed_input_zstream(struct odb_write_stream *in_stream,
+ unsigned char *buf, size_t buf_len)
{
struct input_zstream_data *data = in_stream->data;
git_zstream *zstream = data->zstream;
void *in = fill(1);
- if (in_stream->is_finished) {
- *readlen = 0;
- return NULL;
- }
+ if (in_stream->is_finished)
+ return 0;
- zstream->next_out = data->buf;
- zstream->avail_out = sizeof(data->buf);
+ zstream->next_out = buf;
+ zstream->avail_out = buf_len;
zstream->next_in = in;
zstream->avail_in = len;
@@ -385,9 +383,7 @@ static const void *feed_input_zstream(struct odb_write_stream *in_stream,
in_stream->is_finished = data->status != Z_OK;
use(len - zstream->avail_in);
- *readlen = sizeof(data->buf) - zstream->avail_out;
-
- return data->buf;
+ return buf_len - zstream->avail_out;
}
static void stream_blob(unsigned long size, unsigned nr)
diff --git a/object-file.c b/object-file.c
index bfbb632cf8..a1afca23c5 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1066,6 +1066,7 @@ int odb_source_loose_write_stream(struct odb_source *source,
struct git_hash_ctx c, compat_c;
struct strbuf tmp_file = STRBUF_INIT;
struct strbuf filename = STRBUF_INIT;
+ unsigned char buf[8192];
int dirlen;
char hdr[MAX_HEADER_LEN];
int hdrlen;
@@ -1098,9 +1099,17 @@ int odb_source_loose_write_stream(struct odb_source *source,
unsigned char *in0 = stream.next_in;
if (!stream.avail_in && !in_stream->is_finished) {
- const void *in = in_stream->read(in_stream, &stream.avail_in);
- stream.next_in = (void *)in;
- in0 = (unsigned char *)in;
+ ssize_t read_len = odb_write_stream_read(in_stream, buf,
+ sizeof(buf));
+ if (read_len < 0) {
+ close(fd);
+ err = -1;
+ goto cleanup;
+ }
+
+ stream.avail_in = read_len;
+ stream.next_in = buf;
+ in0 = buf;
/* All data has been read. */
if (in_stream->is_finished)
flush = 1;
diff --git a/odb.h b/odb.h
index ec5367b13e..6faeaa0589 100644
--- a/odb.h
+++ b/odb.h
@@ -529,11 +529,7 @@ static inline int odb_write_object(struct object_database *odb,
return odb_write_object_ext(odb, buf, len, type, oid, NULL, 0);
}
-struct odb_write_stream {
- const void *(*read)(struct odb_write_stream *, unsigned long *len);
- void *data;
- int is_finished;
-};
+struct odb_write_stream;
int odb_write_object_stream(struct object_database *odb,
struct odb_write_stream *stream, size_t len,
diff --git a/odb/streaming.c b/odb/streaming.c
index 5927a12954..a68dd2cbe3 100644
--- a/odb/streaming.c
+++ b/odb/streaming.c
@@ -232,6 +232,11 @@ struct odb_read_stream *odb_read_stream_open(struct object_database *odb,
return st;
}
+ssize_t odb_write_stream_read(struct odb_write_stream *st, void *buf, size_t sz)
+{
+ return st->read(st, buf, sz);
+}
+
int odb_stream_blob_to_fd(struct object_database *odb,
int fd,
const struct object_id *oid,
diff --git a/odb/streaming.h b/odb/streaming.h
index c7861f7e13..65ced911fe 100644
--- a/odb/streaming.h
+++ b/odb/streaming.h
@@ -47,6 +47,24 @@ int odb_read_stream_close(struct odb_read_stream *stream);
*/
ssize_t odb_read_stream_read(struct odb_read_stream *stream, void *buf, size_t len);
+/*
+ * A stream that provides an object to be written to the object database without
+ * loading all of it into memory.
+ */
+struct odb_write_stream {
+ ssize_t (*read)(struct odb_write_stream *, unsigned char *, size_t);
+ void *data;
+ int is_finished;
+};
+
+/*
+ * Read data from the stream into the buffer. Returns 0 when finished and the
+ * number of bytes read on success. Returns a negative error code in case
+ * reading from the stream fails.
+ */
+ssize_t odb_write_stream_read(struct odb_write_stream *stream, void *buf,
+ size_t len);
+
/*
* Look up the object by its ID and write the full contents to the file
* descriptor. The object must be a blob, or the function will fail. When
--
2.54.0.105.g59ff4886a5
^ permalink raw reply related
* [PATCH v4 2/7] odb/transaction: use pluggable `begin_transaction()`
From: Justin Tobler @ 2026-05-14 18:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, peff, Justin Tobler
In-Reply-To: <20260514183740.1505171-1-jltobler@gmail.com>
Each ODB source is expected to provide an ODB transaction implementation
that should be used when starting a transaction. With d6fc6fe6f8
(odb/source: make `begin_transaction()` function pluggable, 2026-03-05),
the `struct odb_source` now provides a pluggable callback for beginning
transactions. Use the callback provided by the ODB source accordingly.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
odb/transaction.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/odb/transaction.c b/odb/transaction.c
index 9bf3f347dc..592ac84075 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -1,5 +1,5 @@
#include "git-compat-util.h"
-#include "object-file.h"
+#include "odb/source.h"
#include "odb/transaction.h"
struct odb_transaction *odb_transaction_begin(struct object_database *odb)
@@ -7,7 +7,7 @@ struct odb_transaction *odb_transaction_begin(struct object_database *odb)
if (odb->transaction)
return NULL;
- odb->transaction = odb_transaction_files_begin(odb->sources);
+ odb_source_begin_transaction(odb->sources, &odb->transaction);
return odb->transaction;
}
--
2.54.0.105.g59ff4886a5
^ permalink raw reply related
* [PATCH v4 1/7] odb: split `struct odb_transaction` into separate header
From: Justin Tobler @ 2026-05-14 18:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, peff, Justin Tobler
In-Reply-To: <20260514183740.1505171-1-jltobler@gmail.com>
The current ODB transaction interface is colocated with other ODB
interfaces in "odb.{c,h}". Subsequent commits will expand `struct
odb_transaction` to support write operations on the transaction
directly. To keep things organized and prevent "odb.{c,h}" from becoming
more unwieldy, split out `struct odb_transaction` into a separate
header.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
Makefile | 1 +
builtin/add.c | 1 +
builtin/unpack-objects.c | 1 +
builtin/update-index.c | 1 +
cache-tree.c | 1 +
meson.build | 1 +
object-file.c | 1 +
odb.c | 25 -------------------------
odb.h | 31 -------------------------------
odb/transaction.c | 28 ++++++++++++++++++++++++++++
odb/transaction.h | 38 ++++++++++++++++++++++++++++++++++++++
read-cache.c | 1 +
12 files changed, 74 insertions(+), 56 deletions(-)
create mode 100644 odb/transaction.c
create mode 100644 odb/transaction.h
diff --git a/Makefile b/Makefile
index dbf0022054..6342db13e5 100644
--- a/Makefile
+++ b/Makefile
@@ -1219,6 +1219,7 @@ LIB_OBJS += odb.o
LIB_OBJS += odb/source.o
LIB_OBJS += odb/source-files.o
LIB_OBJS += odb/streaming.o
+LIB_OBJS += odb/transaction.o
LIB_OBJS += oid-array.o
LIB_OBJS += oidmap.o
LIB_OBJS += oidset.o
diff --git a/builtin/add.c b/builtin/add.c
index 7737ab878b..c859f66519 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -16,6 +16,7 @@
#include "run-command.h"
#include "object-file.h"
#include "odb.h"
+#include "odb/transaction.h"
#include "parse-options.h"
#include "path.h"
#include "preload-index.h"
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 6fc64e9e4b..bc9b1e047e 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -9,6 +9,7 @@
#include "hex.h"
#include "object-file.h"
#include "odb.h"
+#include "odb/transaction.h"
#include "object.h"
#include "delta.h"
#include "pack.h"
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 8a5907767b..bcc43852ef 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -19,6 +19,7 @@
#include "tree-walk.h"
#include "object-file.h"
#include "odb.h"
+#include "odb/transaction.h"
#include "refs.h"
#include "resolve-undo.h"
#include "parse-options.h"
diff --git a/cache-tree.c b/cache-tree.c
index 60bcc07c3b..f056869cfd 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -10,6 +10,7 @@
#include "cache-tree.h"
#include "object-file.h"
#include "odb.h"
+#include "odb/transaction.h"
#include "read-cache-ll.h"
#include "replace-object.h"
#include "repository.h"
diff --git a/meson.build b/meson.build
index 8309942d18..6dc23b3af2 100644
--- a/meson.build
+++ b/meson.build
@@ -405,6 +405,7 @@ libgit_sources = [
'odb/source.c',
'odb/source-files.c',
'odb/streaming.c',
+ 'odb/transaction.c',
'oid-array.c',
'oidmap.c',
'oidset.c',
diff --git a/object-file.c b/object-file.c
index f0b029ff0b..bfbb632cf8 100644
--- a/object-file.c
+++ b/object-file.c
@@ -21,6 +21,7 @@
#include "object-file.h"
#include "odb.h"
#include "odb/streaming.h"
+#include "odb/transaction.h"
#include "oidtree.h"
#include "pack.h"
#include "packfile.h"
diff --git a/odb.c b/odb.c
index 350e23f3c0..8c3cbc1b53 100644
--- a/odb.c
+++ b/odb.c
@@ -1069,28 +1069,3 @@ void odb_reprepare(struct object_database *o)
obj_read_unlock();
}
-
-struct odb_transaction *odb_transaction_begin(struct object_database *odb)
-{
- if (odb->transaction)
- return NULL;
-
- odb->transaction = odb_transaction_files_begin(odb->sources);
-
- return odb->transaction;
-}
-
-void odb_transaction_commit(struct odb_transaction *transaction)
-{
- if (!transaction)
- return;
-
- /*
- * Ensure the transaction ending matches the pending transaction.
- */
- ASSERT(transaction == transaction->source->odb->transaction);
-
- transaction->commit(transaction);
- transaction->source->odb->transaction = NULL;
- free(transaction);
-}
diff --git a/odb.h b/odb.h
index 9aee260105..ec5367b13e 100644
--- a/odb.h
+++ b/odb.h
@@ -35,24 +35,6 @@ struct packed_git;
struct packfile_store;
struct cached_object_entry;
-/*
- * A transaction may be started for an object database prior to writing new
- * objects via odb_transaction_begin(). These objects are not committed until
- * odb_transaction_commit() is invoked. Only a single transaction may be pending
- * at a time.
- *
- * Each ODB source is expected to implement its own transaction handling.
- */
-struct odb_transaction;
-typedef void (*odb_transaction_commit_fn)(struct odb_transaction *transaction);
-struct odb_transaction {
- /* The ODB source the transaction is opened against. */
- struct odb_source *source;
-
- /* The ODB source specific callback invoked to commit a transaction. */
- odb_transaction_commit_fn commit;
-};
-
/*
* The object database encapsulates access to objects in a repository. It
* manages one or more sources that store the actual objects which are
@@ -154,19 +136,6 @@ void odb_close(struct object_database *o);
*/
void odb_reprepare(struct object_database *o);
-/*
- * Starts an ODB transaction. Subsequent objects are written to the transaction
- * and not committed until odb_transaction_commit() is invoked on the
- * transaction. If the ODB already has a pending transaction, NULL is returned.
- */
-struct odb_transaction *odb_transaction_begin(struct object_database *odb);
-
-/*
- * Commits an ODB transaction making the written objects visible. If the
- * specified transaction is NULL, the function is a no-op.
- */
-void odb_transaction_commit(struct odb_transaction *transaction);
-
/*
* Find source by its object directory path. Returns a `NULL` pointer in case
* the source could not be found.
diff --git a/odb/transaction.c b/odb/transaction.c
new file mode 100644
index 0000000000..9bf3f347dc
--- /dev/null
+++ b/odb/transaction.c
@@ -0,0 +1,28 @@
+#include "git-compat-util.h"
+#include "object-file.h"
+#include "odb/transaction.h"
+
+struct odb_transaction *odb_transaction_begin(struct object_database *odb)
+{
+ if (odb->transaction)
+ return NULL;
+
+ odb->transaction = odb_transaction_files_begin(odb->sources);
+
+ return odb->transaction;
+}
+
+void odb_transaction_commit(struct odb_transaction *transaction)
+{
+ if (!transaction)
+ return;
+
+ /*
+ * Ensure the transaction ending matches the pending transaction.
+ */
+ ASSERT(transaction == transaction->source->odb->transaction);
+
+ transaction->commit(transaction);
+ transaction->source->odb->transaction = NULL;
+ free(transaction);
+}
diff --git a/odb/transaction.h b/odb/transaction.h
new file mode 100644
index 0000000000..a56e392f21
--- /dev/null
+++ b/odb/transaction.h
@@ -0,0 +1,38 @@
+#ifndef ODB_TRANSACTION_H
+#define ODB_TRANSACTION_H
+
+#include "odb.h"
+#include "odb/source.h"
+
+/*
+ * A transaction may be started for an object database prior to writing new
+ * objects via odb_transaction_begin(). These objects are not committed until
+ * odb_transaction_commit() is invoked. Only a single transaction may be pending
+ * at a time.
+ *
+ * Each ODB source is expected to implement its own transaction handling.
+ */
+struct odb_transaction;
+typedef void (*odb_transaction_commit_fn)(struct odb_transaction *transaction);
+struct odb_transaction {
+ /* The ODB source the transaction is opened against. */
+ struct odb_source *source;
+
+ /* The ODB source specific callback invoked to commit a transaction. */
+ odb_transaction_commit_fn commit;
+};
+
+/*
+ * Starts an ODB transaction. Subsequent objects are written to the transaction
+ * and not committed until odb_transaction_commit() is invoked on the
+ * transaction. If the ODB already has a pending transaction, NULL is returned.
+ */
+struct odb_transaction *odb_transaction_begin(struct object_database *odb);
+
+/*
+ * Commits an ODB transaction making the written objects visible. If the
+ * specified transaction is NULL, the function is a no-op.
+ */
+void odb_transaction_commit(struct odb_transaction *transaction);
+
+#endif
diff --git a/read-cache.c b/read-cache.c
index 5049f9baca..8147c7e94a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -20,6 +20,7 @@
#include "dir.h"
#include "object-file.h"
#include "odb.h"
+#include "odb/transaction.h"
#include "oid-array.h"
#include "tree.h"
#include "commit.h"
--
2.54.0.105.g59ff4886a5
^ permalink raw reply related
* [PATCH v4 0/7] odb: add write operation to ODB transaction interface
From: Justin Tobler @ 2026-05-14 18:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, peff, Justin Tobler
In-Reply-To: <20260402213220.2651523-1-jltobler@gmail.com>
Greetings,
This series lays the groundwork for introducing write operations to the
ODB transaction interface. The eventual goal is for all object writes
performed within a transaction to go through this interface explicitly,
rather than implicitly relying on the transaction to reconfigure ODB
sources so that writes are redirected to a temporary location.
For now, only `odb_transaction_write_object_stream()` is implemented and
wires up the existing logic for streaming "large" blobs directly into a
packfile as part of the transaction.
Most of the patches are structural refactorings to enable this, but
patch 4 introduces a behavioral change in how packfiles that would
exceed "pack.packSizeLimit" are handled.
Changes since V3:
- Fixed leak due to an fd not being closed when exiting prior to
close_loose_object() being invoked.
Changes since V2:
- Renamed some variables to improve clarity
- Make `odb_write_stream_from_fd()` fully initialize the underlying
`struct odb_write_stream`
- Move `struct odb_write_stream` to "odb/streaming.h"
- Make the `hash_blob_stream()` helper more generic by operating on a
`struct odb_write_stream` instead of reading from an fd directly.
- Introduce an `odb_write_stream_release()` helper to free the
underlying stream data.
Changes since V1:
- Fixed some typos
- Improved error handling
- Removed unnecessary guard statement
- Documented in comments why inflated object size is used to approximate
if object write will exceed "pack.packSizeLimit".
- Updated `struct odb_write_stream` read() callback to support returning
errors and using caller provided buffer
- Updated the `hash_blob_stream()` function signature to operate on a
`struct odb_write_stream` instead of an fd directly
- Renamed some variables/functions for better clarity
Thanks,
-Justin
Justin Tobler (7):
odb: split `struct odb_transaction` into separate header
odb/transaction: use pluggable `begin_transaction()`
odb: update `struct odb_write_stream` read() callback
object-file: remove flags from transaction packfile writes
object-file: avoid fd seekback by checking object size upfront
object-file: generalize packfile writes to use odb_write_stream
odb/transaction: make `write_object_stream()` pluggable
Makefile | 1 +
builtin/add.c | 1 +
builtin/unpack-objects.c | 21 ++--
builtin/update-index.c | 1 +
cache-tree.c | 1 +
meson.build | 1 +
object-file.c | 238 ++++++++++++++++++++-------------------
odb.c | 25 ----
odb.h | 37 +-----
odb/streaming.c | 51 +++++++++
odb/streaming.h | 30 +++++
odb/transaction.c | 35 ++++++
odb/transaction.h | 57 ++++++++++
read-cache.c | 1 +
14 files changed, 312 insertions(+), 188 deletions(-)
create mode 100644 odb/transaction.c
create mode 100644 odb/transaction.h
Range-diff against v3:
1: eee372b426 = 1: eee372b426 odb: split `struct odb_transaction` into separate header
2: 57ac075560 = 2: 57ac075560 odb/transaction: use pluggable `begin_transaction()`
3: 11321ad607 ! 3: d53ad95712 odb: update `struct odb_write_stream` read() callback
@@ object-file.c: int odb_source_loose_write_stream(struct odb_source *source,
+ ssize_t read_len = odb_write_stream_read(in_stream, buf,
+ sizeof(buf));
+ if (read_len < 0) {
++ close(fd);
+ err = -1;
+ goto cleanup;
+ }
4: 72d4656eee = 4: fa7a3ad5dc object-file: remove flags from transaction packfile writes
5: e4896101ff = 5: 1ca08e0590 object-file: avoid fd seekback by checking object size upfront
6: b3cb0a707c = 6: a548401057 object-file: generalize packfile writes to use odb_write_stream
7: e1d292a7ed = 7: 4765b1024a odb/transaction: make `write_object_stream()` pluggable
base-commit: 5361983c075154725be47b65cca9a2421789e410
--
2.54.0.105.g59ff4886a5
^ 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