* Re: [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic
From: Junio C Hamano @ 2026-07-08 19:58 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget; +Cc: git, Michael Montalbo
In-Reply-To: <efd34c17157b3183cdc851c8b17e7967b6c85506.1783479584.git.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Michael Montalbo <mmontalbo@gmail.com>
>
> http-429.sh records "already returned 429 once" with a "test -f"
> followed by a "touch" of a shared state file. That check-then-act is not
> atomic: Apache can run this CGI for several requests at once, and two of
> them can both pass the "test -f" before either "touch"es, so both treat
> themselves as the first request. The retry flow that drives this
> endpoint is mostly sequential, so this has not been seen to fail, but
> the race is latent.
OK. And use of mkdir for atomicity is an obvious solution for such
a situtation.
> -if test -f "$state_file"
> +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null
> then
> # Already returned 429 once, forward to git-http-backend
> # Set PATH_INFO to just the repo path (without retry-after value)
> @@ -52,9 +55,6 @@ then
> exec "$GIT_EXEC_PATH/git-http-backend"
> fi
>
> -# Mark that we've returned 429
> -touch "$state_file"
> -
^ permalink raw reply
* Re: [PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests
From: Junio C Hamano @ 2026-07-08 19:54 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget; +Cc: git, Michael Montalbo
In-Reply-To: <9f48aa6d6ddea681b700f689f0509c4b30a7007d.1783479584.git.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Michael Montalbo <mmontalbo@gmail.com>
>
> apply-one-time-script.sh checks for the "one-time-script" marker, runs
> it, captures the git-http-backend response in the fixed-name files "out"
> and "out_modified", and removes the marker only after it has finished
> serving the modified response. Because the client receives the response
> body before that removal, it can start its next request while the marker
> still exists. Apache can then run this CGI for two requests at once: a
> partial fetch that receives a REF_DELTA against a missing promisor
> object lazily fetches that base while the first response is still in
> flight. The second request passes the marker check, the first request
> then removes the marker, and the second fails to exec the now-missing
> marker, emits no output, and the server answers HTTP 500:
>
> fatal: ... The requested URL returned error: 500
> fatal: could not fetch <oid> from promisor remote
>
> This has been seen as a flaky failure of t5616.47 on the macOS CI
> runners.
Thanks for this detailed write-up. The analysis looks good.
> Claim the marker atomically with a rename, and only once the one-time
> script has succeeded and actually changed the response; give the scratch
> files per-request names. A request that loses the rename, or whose
> script fails or leaves the response unchanged, serves the unmodified
> body and keeps the marker for a later request. No path emits an empty
> body, so the HTTP 500 no longer occurs.
Hmph.
> +#
> +# Apache can run this CGI for concurrent requests (for example a partial fetch
> +# that lazily fetches a missing object while the first response is still in
> +# flight), so the helper claims the marker atomically with a rename, and only
> +# once it has decided to modify the response. A request that loses the race
> +# finds the marker already gone and serves its response unchanged; no request
> +# is left emitting an empty body, which the server would report as HTTP 500.
> +# Scratch files are per-request ($$) so concurrent requests do not clobber each
> +# other.
> +
> +test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
>
> - "$GIT_EXEC_PATH/git-http-backend" >out
> - ./one-time-script out >out_modified
> +LC_ALL=C
> +export LC_ALL
The original was somehow inconsistent in that it forced C locale
only when one-time-script munged the output, and otherwise the
backend was run in the original locale. I am not sure if that
matters very much.
> +out=out.$$
> +modified=out-modified.$$
> +"$GIT_EXEC_PATH/git-http-backend" >"$out"
> +
> +if ./one-time-script "$out" 2>/dev/null >"$modified" &&
> + ! cmp -s "$out" "$modified" &&
> + mv one-time-script one-time-script.$$ 2>/dev/null
> +then
> + cat "$modified"
> else
> + cat "$out"
> fi
We may run the one-time script, find that it modified the payload,
and then another instance of us may start running before we can move
the one-time script away, so the second request can see "ah,
one-time-script is there, nobody has claimed it by renaming" and run
it again, no? So this solution may shrink the race window but may
not completely eliminate it, unless we have some coordination among
ourselves, perhaps?
Ah, we assume running one-time-script itself multiple times is safe
and does not cause issues. Our objective is to avoid returning
modified output twice. So while the first instance of us
successfully renames one-time-script to one-time-script.$$ and emits
the modified result, even if the second instance raced and managed
to run the script again, it will fail to rename with "mv", and
discard the modified output, and instead show the unmodified output
generated by the backend.
OK. It is a bit tricky. It may help future readers if we said
something about this in the proposed log message (i.e., we consider
that it is perfectly fine to run one-time-script more than once; we
only want to avoid letting the second invocation's output used).
Thanks.
^ permalink raw reply
* Re: [PATCH] unpack-trees: avoid quadratic index scan in next_cache_entry()
From: Junio C Hamano @ 2026-07-08 19:16 UTC (permalink / raw)
To: Henrique Ferreiro; +Cc: Henrique Ferreiro via GitGitGadget, git
In-Reply-To: <4c0a31e9-9b20-46c8-8f1f-0fda34515270@igalia.com>
Henrique Ferreiro <hferreiro@igalia.com> writes:
> On 07/07/2026 23:30, Junio C Hamano wrote:
>> "Henrique Ferreiro via GitGitGadget" <gitgitgadget@gmail.com>
>> writes:
>>
>>> diff --git a/unpack-trees.c b/unpack-trees.c
>>> index b42020f16b..ed9fef453a 100644
>>> --- a/unpack-trees.c
>>> +++ b/unpack-trees.c
>>> @@ -671,8 +671,10 @@ static struct cache_entry *next_cache_entry(struct unpack_trees_options *o)
>>>
>>> while (pos < index->cache_nr) {
>>> struct cache_entry *ce = index->cache[pos];
>>> - if (!(ce->ce_flags & CE_UNPACKED))
>>> + if (!(ce->ce_flags & CE_UNPACKED)) {
>>> + o->internal.cache_bottom = pos;
>>> return ce;
>>> + }
>>> pos++;
>> Nice spotting.
>>
>> Does this trick work correctly even when a path's sorting order
>> differs between the index and tree objects, which is precisely why
>> .cache_bottom was introduced, to allow backward scanning while
>> bounding the lookback distance?
> IIUC, .cache_bottom points at the first entry that needs to be
> processed. With this change, that still holds true even when entries are
> processed out of index order. find_cache_pos() also advances
> cache_bottom past unpacked entries since e53e6b4433 (unpack-trees: Make
> index lookahead less pessimal, 2010-06-10).
That sounds sensible.
>>> diff --git a/t/perf/p0009-diff-pathspec.sh b/t/perf/p0009-diff-pathspec.sh
>>> new file mode 100755
>>> index 0000000000..0f1dccfbb4
>>> --- /dev/null
>>> +++ b/t/perf/p0009-diff-pathspec.sh
>>> @@ -0,0 +1,27 @@
>>> +#!/bin/sh
>>> +
>>> +test_description='Tests performance of diffing the working tree with a pathspec'
>>> +
>>> +. ./perf-lib.sh
>>> +
>>> +test_perf_fresh_repo
>>> +
>>> +# The entries exist only in the index, which is enough to
>>> +# exercise the index scan.
>>> +test_expect_success 'setup' '
>>> + count=100000 &&
>>
>> You will probably want to mimic how t/perf/p4209-pickaxe.sh helps
>> testers by adjusting the count based on how the EXPENSIVE
>> prerequisite is configured.
I think this comment still needs addressing, though.
Thanks.
>>> + blob=$(echo content | git hash-object -w --stdin) &&
>>> + {
>>> + printf "100644 $blob\taaa/file\n" &&
>>> + printf "100644 $blob\tf%s\n" $(test_seq $count)
>>> + } | git update-index --index-info &&
>>> + git commit -q -m initial &&
>>> + mkdir -p aaa &&
>>> + echo content >aaa/file
>>> +'
^ permalink raw reply
* Re: [PATCH] unpack-trees: avoid quadratic index scan in next_cache_entry()
From: Henrique Ferreiro @ 2026-07-08 18:31 UTC (permalink / raw)
To: Junio C Hamano, Henrique Ferreiro via GitGitGadget; +Cc: git
In-Reply-To: <xmqqv7aqzdvq.fsf@gitster.g>
On 07/07/2026 23:30, Junio C Hamano wrote:
> "Henrique Ferreiro via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index b42020f16b..ed9fef453a 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -671,8 +671,10 @@ static struct cache_entry *next_cache_entry(struct unpack_trees_options *o)
>>
>> while (pos < index->cache_nr) {
>> struct cache_entry *ce = index->cache[pos];
>> - if (!(ce->ce_flags & CE_UNPACKED))
>> + if (!(ce->ce_flags & CE_UNPACKED)) {
>> + o->internal.cache_bottom = pos;
>> return ce;
>> + }
>> pos++;
> Nice spotting.
>
> Does this trick work correctly even when a path's sorting order
> differs between the index and tree objects, which is precisely why
> .cache_bottom was introduced, to allow backward scanning while
> bounding the lookback distance?
IIUC, .cache_bottom points at the first entry that needs to be
processed. With this change, that still holds true even when entries are
processed out of index order. find_cache_pos() also advances
cache_bottom past unpacked entries since e53e6b4433 (unpack-trees: Make
index lookahead less pessimal, 2010-06-10).
>
>> }
>> return NULL;
>
>> diff --git a/t/perf/p0009-diff-pathspec.sh b/t/perf/p0009-diff-pathspec.sh
>> new file mode 100755
>> index 0000000000..0f1dccfbb4
>> --- /dev/null
>> +++ b/t/perf/p0009-diff-pathspec.sh
>> @@ -0,0 +1,27 @@
>> +#!/bin/sh
>> +
>> +test_description='Tests performance of diffing the working tree with a pathspec'
>> +
>> +. ./perf-lib.sh
>> +
>> +test_perf_fresh_repo
>> +
>> +# The entries exist only in the index, which is enough to
>> +# exercise the index scan.
>> +test_expect_success 'setup' '
>> + count=100000 &&
> You will probably want to mimic how t/perf/p4209-pickaxe.sh helps
> testers by adjusting the count based on how the EXPENSIVE
> prerequisite is configured.
>
>> + blob=$(echo content | git hash-object -w --stdin) &&
>> + {
>> + printf "100644 $blob\taaa/file\n" &&
>> + printf "100644 $blob\tf%s\n" $(test_seq $count)
>> + } | git update-index --index-info &&
>> + git commit -q -m initial &&
>> + mkdir -p aaa &&
>> + echo content >aaa/file
>> +'
>> +
>> +test_perf 'diff pathspec subtree' '
>> + git diff HEAD -- aaa/file
>> +'
>> +
>> +test_done
> Thanks.
^ permalink raw reply
* Re: [PATCH v2] SubmittingPatches: document how to retract a topic
From: Michael Montalbo @ 2026-07-08 18:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqpl0xv25e.fsf@gitster.g>
On Wed, Jul 08, 2026 at 10:09:49AM -0700, Junio C Hamano wrote:
> While this document outlines an idealized lifecycle where an author
> develops a patch, refines it with reviewer feedback, and
> successfully merges it into Git, reality is rarely so seamless.
>
> Sometimes, a topic must be abandoned. Doing so explicitly is far
> better than leaving it in limbo, especially since topics can always
> be resurrected later.
>
> Clearly state that we encourage contributors to retract any topic
> that does not pan out.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * Added the third bullet that talks about when and how a topic
> becomes stalled and discarded.
>
> Documentation/SubmittingPatches | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index d2d82eb543..c269e474e3 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -123,6 +123,20 @@ of review.
> top, it gets merged to the 'master' branch and waits to become part
> of the next major release.
>
> +But sometimes things do not work as planned:
> +
> +. If a mailing list discussion convinces you that your changes aren't
> + ideal, please explicitly retract the topic to save the maintainer
> + time and effort.
> +
> +. If you must drop a topic due to shifting priorities, lack of time,
> + or other commitments, notify the list as a courtesy so others can
> + take over. Anyone can resurrect the topic later when they have the
> + capacity to do so.
> +
> +. Topics with unaddressed review comments that remain inactive for
> + four weeks may be discarded by the maintainer.
> +
Nice, I think these points read smoothly and the additional "four weeks
until may be discarded" guidance is helpful.
LGTM.
^ permalink raw reply
* Re: [GSoC Blog] Week 6 : Improve Disk Space Recovery for Partial Clones
From: Siddharth Shrimali @ 2026-07-08 18:15 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Siddharth Asthana
In-Reply-To: <CAGWgyh-Udy9p9yF_8EDnVgK4bxyG9BsX1gio8oKr0G9b-sdQFA@mail.gmail.com>
Hello everyone,
My latest blog post, covering week 6, is now live:
https://siddharth.shrimali.info/#post/8
Please feel free to review my work and share your feedback.
Always open to discussions! :)
Regards,
Siddharth Shrimali
^ permalink raw reply
* Re: What's cooking in git.git (Jul 2026, #03)
From: Kristofer Karlsson @ 2026-07-08 18:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq33xtwj9m.fsf@gitster.g>
On Wed, 8 Jul 2026 at 18:14, Junio C Hamano <gitster@pobox.com> wrote:
>
> When topic A depends on topic B and topic C, rather than rebasing A
> on 'next' (if B and C are in 'next'), I would often do
>
> $ git checkout -b A master
> $ git merge --no-ff B
> $ git merge --no-ff C
>
> these days to prepare the "base" to apply patches that belong to A.
>
> Many contributors mimic this and state in their cover letter that
> the patches in the series are to be applied on top of master at
> commit X with branches Y and Z merged, or something to that effect.
Yes, that worked locally for me but I think my mistake was
that I started off using gitgitgadget which only supports
workflows that reference an existing remote branch as base.
In the future I will try to learn the b4 tool so I can save myself
some headache.
Thanks for the guidance,
Kristofer
^ permalink raw reply
* Re: Programmatically edit the git rebase sequence?
From: Johannes Schindelin @ 2026-07-08 17:58 UTC (permalink / raw)
To: Matthias Beyer; +Cc: git, neikos
In-Reply-To: <akei64goQf3nFhX4@hikari>
Hi Matthias,
On Fri, 3 Jul 2026, Matthias Beyer wrote:
> in a recent conversation at work, the question of how to
> programmatically edit the git-rebase sequence came up.
The easiest way is probably to have the interactive rebase stop at the top
of the rebase script:
# insert a `b` (for `break`) before the first line
GIT_SEQUENCE_EDITOR="sed -i 1ib" git rebase -i ...
(Note that this is GNU sed syntax, with BSD/macOS sed you'll have to get a
bit creative, or write a real script and refer to it via the path.)
Then you can use a script to transform the rebase script before `git
rebase --continue`.
I frequently use this technique with LLMs because they seem to be unable
to control an interactive editor correctly.
> I have a branch that touches a number of files, adds some files and
> removes some files.
> When rebasing, I want to split all commits that touched a certain subset
> of files, for the clearity of the history.
>
> I look at the output of
>
> git log master..mybranch --oneline --diff-filter=M -- "./subdir/*.rs"
Noting that the OIDs in $(git rev-parse --git-path
rebase-merge/git-rebase-todo) are expanded, you could match them via the
OIDs obtained using `rev-list` instead of `log`, to enumerate the OIDs
instead of full commit history, then turning that into some kind of regex.
Something along the lines
regex=$(git rev-list master..mybranch --diff-filter=M -- "subdir/*.rs" |
# read all lines, then turn newlines into `\|`
sed ':1;$b2;N;b1;:2;s/\n/\\|/g')
GIT_REBASE_TODO="$(git rev-parse --git-path rebase-merge/git-rebase-todo)"
test -z "$regex" ||
sed -i "s/^pick \($regex\)/edit \1/" $GIT_REBASE_TODO
This _should_ work, but you'll most likely want to adapt it to your
particular use cases.
Ciao,
Johannes
>
> to find all commits in that subdir that only touched the files. All of
> these commits are to be "edit"ed.
>
> Now I fire up `git rebase -i master` and manually(!) match the list from
> above `git-log` call and find the respective commits to edit them.
>
> Is there a way I am not aware of to do that manual step programatically?
> Something like
>
> git rebase -i master --edit-commits="$(git log master..mybranch --diff-filter=M --format="%H" -- "./subdir/*.rs")"
>
> would be convenient here, although I would understand if that is too
> much clutter for the already very heavy git CLI interface :-)
>
> Maybe I am just not aware of the obvious solution - I would be happy to
> learn that there is already one!
>
> Best,
> Matthias
>
^ permalink raw reply
* [PATCH v3 2/2] prio-queue: use cascade for unfused gets
From: Kristofer Karlsson via GitGitGadget @ 2026-07-08 17:49 UTC (permalink / raw)
To: git
Cc: René Scharfe, Kristofer Karlsson, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2132.v3.git.1783532989.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
When flush_get() removes the root without an immediate replacement,
use a cascade-then-sift-up strategy instead of sift-down.
Standard sift-down places the last element at the root and sifts it
down. This needs two comparisons per level (pick the smaller child,
then compare against the element), even though the displaced element
almost always ends up near the bottom where it came from.
cascade_down() instead moves the vacancy down by promoting the
smaller child at each level (one comparison per level), leaving the
vacancy at a leaf. The last element is then placed at the vacancy
and sift_up() floats it to its correct position, which is typically
very little work since it already belongs near the bottom.
This is the well-known "bottom-up" variant of sift-down [1].
[1] https://en.wikipedia.org/wiki/Heapsort#Bottom-up_heapsort
Helped-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
prio-queue.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/prio-queue.c b/prio-queue.c
index 926fc04e85..230d6f5e33 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -66,13 +66,31 @@ static void sift_down_root(struct prio_queue *queue)
}
}
+/* Cascade vacancy toward a leaf, promoting the smaller child at each level */
+static size_t cascade_down(struct prio_queue *queue)
+{
+ size_t ix, child;
+
+ for (ix = 0; (child = ix * 2 + 1) < queue->nr_; ix = child) {
+ if (child + 1 < queue->nr_ &&
+ compare(queue, child, child + 1) >= 0)
+ child++;
+ queue->array[ix] = queue->array[child];
+ }
+ return ix;
+}
+
static inline void flush_get(struct prio_queue *queue)
{
+ size_t ix;
+
if (!queue->get_pending)
return;
queue->get_pending = 0;
- queue->array[0] = queue->array[--queue->nr_];
- sift_down_root(queue);
+ --queue->nr_;
+ ix = cascade_down(queue);
+ queue->array[ix] = queue->array[queue->nr_];
+ sift_up(queue, ix);
}
void prio_queue_put(struct prio_queue *queue, void *thing)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 1/2] prio-queue: extract sift_up() from prio_queue_put()
From: Kristofer Karlsson via GitGitGadget @ 2026-07-08 17:49 UTC (permalink / raw)
To: git
Cc: René Scharfe, Kristofer Karlsson, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2132.v3.git.1783532989.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
Factor out the bubble-up loop from prio_queue_put() into a
standalone sift_up() function. This is a pure refactor with
no behavior change, preparing for reuse in a subsequent commit.
Suggested-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
prio-queue.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/prio-queue.c b/prio-queue.c
index 199775d5af..926fc04e85 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -37,6 +37,17 @@ void clear_prio_queue(struct prio_queue *queue)
queue->get_pending = 0;
}
+static void sift_up(struct prio_queue *queue, size_t ix)
+{
+ while (ix) {
+ size_t parent = (ix - 1) / 2;
+ if (compare(queue, parent, ix) <= 0)
+ break;
+ swap(queue, parent, ix);
+ ix = parent;
+ }
+}
+
static void sift_down_root(struct prio_queue *queue)
{
size_t ix, child;
@@ -66,8 +77,6 @@ static inline void flush_get(struct prio_queue *queue)
void prio_queue_put(struct prio_queue *queue, void *thing)
{
- size_t ix, parent;
-
if (queue->get_pending) {
queue->get_pending = 0;
queue->array[0].ctr = queue->insertion_ctr++;
@@ -85,13 +94,7 @@ void prio_queue_put(struct prio_queue *queue, void *thing)
return; /* LIFO */
/* Bubble up the new one */
- for (ix = queue->nr_ - 1; ix; ix = parent) {
- parent = (ix - 1) / 2;
- if (compare(queue, parent, ix) <= 0)
- break;
-
- swap(queue, parent, ix);
- }
+ sift_up(queue, queue->nr_ - 1);
}
void *prio_queue_get(struct prio_queue *queue)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 0/2] prio-queue: use bottom-up sift for extract-min
From: Kristofer Karlsson via GitGitGadget @ 2026-07-08 17:49 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Kristofer Karlsson, Kristofer Karlsson
In-Reply-To: <pull.2132.v2.git.1780301856444.gitgitgadget@gmail.com>
This tweaks the prio_queue implementation to use a bottom-up approach
sifting for get [1].
In practice, the performance boost is small, but measurable for reasonably
large prio_queue:s (thousands of elements, not millions) but it should never
increase the work.
Minor note on v3: After the most recent discussion I am not 100% sure how to
reason about the value of this change - both the value gain and code cost
seem small, but since there was some interest and research done by René I
wanted to complete this v3 anyway so it can be properly discussed (though
still maybe ultimately closed).
Here's how it works:
Instead of placing the last element at the root and sifting it down with two
comparisons per level, cascade the vacancy down by promoting the smaller
child (one comparison per level), then place the last element at the vacancy
and sift it up. Since the displaced element is likely to belong near the
bottom of the heap, sift_up() typically does very little work.
sift_down_root() is kept as-is for the fused replace path in
prio_queue_put(), where the new element is arbitrary and may belong near the
root -- Rene's testing showed that cascade regresses on git-describe for
this reason.
Benchmarks (rev-list --all --count) on public repos confirm no regression on
git.git and linux.git. On a large example repo with thousands of active
branches the cascade yields a measurable (~2%) end-to-end improvement; the
gain is modest because the lazy-fold optimization (now in next) already
fuses most get+put pairs, leaving only the remaining unfused gets to benefit
from cascade.
René's exhaustive analysis [2] of all permutations up to n=12 confirms that
cascade never requires more comparisons than standard sift-down for a full
drain.
Note: sift_up() currently uses swap, matching the existing code style. It
could be further optimized to use copy (hold the element in a temp, shift
parents down, write once), but that would require changing compare() to
accept element values instead of array indices. Left for a potential
follow-up.
Changes since v2:
* Rebased on kk/prio-queue-get-put-fusion (now in next).
* Split into two commits - refactoring and then introducing cascade_down.
Changes since v1:
* Kept sift_down_root() and prio_queue_replace() completely unchanged,
preserving René's optimization that avoids the get+put overhead for
replace. The cascade approach now only applies to prio_queue_get().
* Extracted the new logic into a separate sift_up_rebalance() function
rather than inlining it in prio_queue_get().
* Updated benchmark numbers for ascending, descending and random insertion
ordering. No regressions in any scenario.
[1] https://en.wikipedia.org/wiki/Heapsort#Bottom-up_heapsort [2]
https://lore.kernel.org/git/pull.2132.git.1780250236304.gitgitgadget@gmail.com/T/#m114df6e1c2845acbbc64d875ed7dc1d7d9193ed5
Kristofer Karlsson (2):
prio-queue: extract sift_up() from prio_queue_put()
prio-queue: use cascade for unfused gets
prio-queue.c | 43 ++++++++++++++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 11 deletions(-)
base-commit: 00534a21ce949ef80a5b8b9d7fc20b7d381038e9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2132%2Fspkrka%2Fcascade-sift-down-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2132/spkrka/cascade-sift-down-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2132
Range-diff vs v2:
-: ---------- > 1: ec6a448563 prio-queue: extract sift_up() from prio_queue_put()
1: 6051d44e59 ! 2: 89a22c6a75 prio-queue: use cascade-down for faster extract-min
@@ Metadata
Author: Kristofer Karlsson <krka@spotify.com>
## Commit message ##
- prio-queue: use cascade-down for faster extract-min
+ prio-queue: use cascade for unfused gets
- Add sift_up_rebalance(), an alternative to sift_down_root() that
- halves the number of comparisons per extract-min.
+ When flush_get() removes the root without an immediate replacement,
+ use a cascade-then-sift-up strategy instead of sift-down.
- The standard extract places the last array element at the root and
- sifts it down. At each level this requires two comparisons (left
- vs right child, then element vs winner) and a swap.
+ Standard sift-down places the last element at the root and sifts it
+ down. This needs two comparisons per level (pick the smaller child,
+ then compare against the element), even though the displaced element
+ almost always ends up near the bottom where it came from.
- sift_up_rebalance() instead promotes the smaller child into the
- root slot at each level — one comparison and one copy — until the
- vacancy reaches a leaf. The last array element is placed at the
- vacancy and sifted up to restore heap order. In practice the
- sift-up rarely moves more than a level or two because the last
- array element tends to be large.
+ cascade_down() instead moves the vacancy down by promoting the
+ smaller child at each level (one comparison per level), leaving the
+ vacancy at a leaf. The last element is then placed at the vacancy
+ and sift_up() floats it to its correct position, which is typically
+ very little work since it already belongs near the bottom.
- Work per extract drops from 2d comparisons + d swaps to
- d comparisons + d copies + a short sift-up.
+ This is the well-known "bottom-up" variant of sift-down [1].
- prio_queue_get() now calls sift_up_rebalance() instead of placing
- the last element at root and calling sift_down_root().
-
- sift_down_root() and prio_queue_replace() are left unchanged.
-
- Synthetic benchmark (10 rounds of 10M put+get cycles, CPU-pinned,
- same compiler and Makefile flags):
-
- Ascending keys (git's typical pattern — parents have lower
- priority than children):
-
- queue width baseline patched speedup
- 10 4.39s 3.91s 1.12x
- 100 9.10s 6.61s 1.38x
- 1,000 11.84s 9.25s 1.28x
- 10,000 17.50s 13.92s 1.26x
- 100,000 23.97s 20.19s 1.19x
-
- Descending keys (worst case — last element always sinks to leaf):
-
- queue width baseline patched speedup
- 10 4.94s 4.95s 1.00x
- 100 9.75s 9.42s 1.03x
- 1,000 15.01s 15.29s 0.98x
- 10,000 24.79s 23.88s 1.04x
- 100,000 29.69s 28.24s 1.05x
-
- Random keys:
-
- queue width baseline patched speedup
- 10 5.05s 4.99s 1.01x
- 100 9.90s 9.50s 1.04x
- 1,000 15.35s 14.77s 1.04x
- 10,000 25.35s 24.21s 1.05x
- 100,000 65.71s 63.38s 1.04x
-
- No regressions in any scenario.
-
- End-to-end benchmark on the linux kernel repo (1.4M commits,
- range v5.0..v6.0, 311K commits, 20 interleaved runs, 1 warmup):
-
- Command baseline patched speedup
- rev-list --count v5.0..v6.0 484ms 474ms 1.02x
-
- The improvement scales with DAG width: wider DAGs produce larger
- priority queues, amplifying the per-level savings. In small or
- narrow repositories the queues stay shallow and the sift-down
- cost is already negligible.
+ [1] https://en.wikipedia.org/wiki/Heapsort#Bottom-up_heapsort
+ Helped-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
## prio-queue.c ##
@@ prio-queue.c: static void sift_down_root(struct prio_queue *queue)
}
}
-+static void sift_up_rebalance(struct prio_queue *queue)
++/* Cascade vacancy toward a leaf, promoting the smaller child at each level */
++static size_t cascade_down(struct prio_queue *queue)
+{
+ size_t ix, child;
+
-+ /* Cascade: promote smaller child at each level. */
-+ for (ix = 0; (child = ix * 2 + 1) < queue->nr; ix = child) {
-+ if (child + 1 < queue->nr &&
++ for (ix = 0; (child = ix * 2 + 1) < queue->nr_; ix = child) {
++ if (child + 1 < queue->nr_ &&
+ compare(queue, child, child + 1) >= 0)
+ child++;
+ queue->array[ix] = queue->array[child];
+ }
-+
-+ /* Place the last element at the vacancy and sift up. */
-+ queue->array[ix] = queue->array[queue->nr];
-+ while (ix) {
-+ size_t parent = (ix - 1) / 2;
-+ if (compare(queue, parent, ix) <= 0)
-+ break;
-+ swap(queue, parent, ix);
-+ ix = parent;
-+ }
++ return ix;
+}
+
- void *prio_queue_get(struct prio_queue *queue)
+ static inline void flush_get(struct prio_queue *queue)
{
- void *result;
-@@ prio-queue.c: void *prio_queue_get(struct prio_queue *queue)
- if (!--queue->nr)
- return result;
-
-- queue->array[0] = queue->array[queue->nr];
++ size_t ix;
++
+ if (!queue->get_pending)
+ return;
+ queue->get_pending = 0;
+- queue->array[0] = queue->array[--queue->nr_];
- sift_down_root(queue);
-+ sift_up_rebalance(queue);
- return result;
++ --queue->nr_;
++ ix = cascade_down(queue);
++ queue->array[ix] = queue->array[queue->nr_];
++ sift_up(queue, ix);
}
+ void prio_queue_put(struct prio_queue *queue, void *thing)
--
gitgitgadget
^ permalink raw reply
* Re: [RFC] clone: allow sparse-checkout paths to be specified during clone
From: Pushkar Singh @ 2026-07-08 17:48 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, ps, Derrick Stolee
In-Reply-To: <20260630053235.GB2495216@coredump.intra.peff.net>
Hi Jeff,
Thanks for taking the time to look at this, and sorry for the delayed reply.
> IIRC the sparseness is contained in a patterns file, so I'd have
> expected the first level of fix to be "you can provide that file at
> clone time, rather than afterwards".
That's a good point.
My thinking was mainly from the perspective of someone using the existing
"git sparse-checkout" workflow instead of editing the patterns file
directly. Personally, I've always used "git sparse-checkout set" and
never really edited the patterns file myself. So I was thinking of this
as a convenience layer over the existing workflow rather than exposing
the patterns file itself.
> You might try cc-ing folks who worked on sparse checkouts, especially
> Stolee.
Thanks for the suggestion! I've cc'd Derrick here in case he has any
thoughts on this :-)
> One final thought from a non-sparse-checkout user: you're coming at it
> from the point of view of ergonomics (it is annoying to clone and then
> set up sparsity separately) but there is also a performance question.
I was mostly thinking about the workflow rather than the performance
side. My main goal was just to make the workflow a bit simpler and more
intuitive. Internally, I was still thinking of it as using the existing
sparse-checkout machinery after clone.
If there is a way to make use of the selected paths earlier in the clone
process as well, that would definitely be a nice bonus, although I
haven't looked into whether that's practical.
> Like I said, I don't have any experience. :)
Even then, I really appreciate you taking the time to read through the
RFC and share your thoughts :-)
Thanks again!
- Pushkar
^ permalink raw reply
* Re: [PATCH] merge --abort: don't delete autostash before reset succeeds
From: Junio C Hamano @ 2026-07-08 17:46 UTC (permalink / raw)
To: Phillip Wood; +Cc: Kris Point, git@vger.kernel.org
In-Reply-To: <0b7e6d74-0287-4be5-a19f-ed8c5fbc9217@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> I'm afraid I don't think this is the right solution. We only want to
> save the stash if there are conflicts when we apply it - that is why
> MERGE_AUTOSTASH is deleted before we do the reset - we want to prevent
> remove_branch_state() from saving it. If the stash applies cleanly then
> we should not save it. If the reset fails then we should keep
> MERGE_AUTOSTASH along with the other merge state files rather than
> saving the stash (which is actually what happens after this patch
> because cmd_reset() dies before it calls remove_branch_state()).
Thanks for pointing it out that reset calls remove_branch_state(),
which in turn calls remove_merge_branch_state(), which in turn calls
save_autostash_ref(). We end up (when cmd_reset() is successful)
applying the autostash (which is good) but also saving a new stash.
> I think the solution is probably to stop calling
> builtin/reset.c:cmd_reset() and instead ...
Great. In general, it is a bad pattern we should find and fix for
cmd_A() to call cmd_B() in its implementation as a subroutine. To
clean any such instance is a great thing to do.
> ...
> So we only delete MERGE_AUTOSTASH after a successful reset and we only
> save the stash if it applies with conflicts. That's all a bit more
> involved than the patch here - please do give me a shout if you want
> some more information.
>
> Thanks
>
> Phillip
Thanks.
^ permalink raw reply
* Re: [PATCH v2 00/11] receive-pack: use ODB transactions to stage object writes
From: Justin Tobler @ 2026-07-08 17:36 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster
In-Reply-To: <ak3xYym22Z7PFZ5y@pks.im>
On 26/07/08 08:42AM, Patrick Steinhardt wrote:
> On Tue, Jul 07, 2026 at 11:14:01PM -0500, Justin Tobler wrote:
> > Changes since V1:
> >
> > - Adapted other "file" ODB transaction helpers to be more consistent
> > with current naming scheme.
> > - Removed redundant NULL transaction handling from
> > `odb_transaction_files_begin()`.
> > - `odb_transaction_begin()` now returns an error if there is already
> > an inflight transaction pending instead of setting the `out` pointer
> > to NULL.
> > - Updated `odb_transaction_env()` to return an error code and append
> > environment variables to a strvec provided as an argument.
> > - Removed redundant setting of tmpdir environment variables for child
> > processes after tmpdir has been migrated.
> > - Split changes adding ODB transaction flags into a separate commit.
> > - Consistently wire the ODB transaction throughout git-receive-pack
> > code instead of reading it from `the_repository`.
> > - Updated user facing error message.
> > - Updated some comments to better document functions/flags.
> > - Clarified some commit messages.
> > - Fixed typos.
>
> I've got a couple smaller nits, but overall I'm quite happy with the
> shape of this series now. Thanks!
Thanks for the review! I'll send another version later today. :)
-Justin
^ permalink raw reply
* Re: [PATCH v2 09/11] odb/transaction: introduce ODB transaction flags
From: Justin Tobler @ 2026-07-08 17:34 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster
In-Reply-To: <ak3xMNbhQnGCMe2c@pks.im>
On 26/07/08 08:41AM, Patrick Steinhardt wrote:
> On Tue, Jul 07, 2026 at 11:14:10PM -0500, Justin Tobler wrote:
> > +/* Flags used to configure an ODB transaction. */
> > +enum odb_transaction_flags {
> > + /* Configures the transaction for use with git-receive-pack(1). */
> > + ODB_TRANSACTION_RECEIVE = (1 << 0),
> > +};
> > +
> > /*
> > * Starts an ODB transaction and returns it via `out`. Subsequent objects are
> > * written to the transaction and not committed until odb_transaction_commit()
>
> And this is the reason you have to add the include, so that the flags
> are visible in both "odb/source.h" and in "odb/transaction.h".
>
> This makes me wonder whether there's really much value in having this
> header here be split out of "odb/source.h".
Ya, I've started wondering the same thing. A transaction implementation
is always going to be tightly coupled to the ODB source it pertains too.
It probably makes sense to merge "odb/transaction.{c,h}" with
"odb/source.{c,h}". I'll leave it as-is for now and likely explore this
is a future series though.
-Justin
^ permalink raw reply
* Re: [PATCH 2/2] commit-reach: guard !FIND_ALL early exit with generation ordering check
From: Junio C Hamano @ 2026-07-08 17:24 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget
Cc: git, Derrick Stolee, Kristofer Karlsson
In-Reply-To: <ba3f2bb6e8463eb2f101ebf5c7d8d83d353731a2.1782739162.git.gitgitgadget@gmail.com>
"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Kristofer Karlsson <krka@spotify.com>
>
> When paint_down_to_common() falls back to commit-date ordering (for
> v1 commit graphs without corrected commit dates), the !FIND_ALL early
> exit incorrectly fires. The exit assumes the queue is generation-
> ordered, so the first RESULT commit found must be the shallowest.
> With date ordering this is not guaranteed: a closer merge base with
> a lower committer date (clock skew) may still be in the queue behind
> deeper commits.
Excellent description of a good observation.
> Add a gen_ordered flag that is cleared when the date fallback fires,
> and require it for the early exit.
The solution is simple and straight-forward.
The flag is initialized to true but we drop it when generation order
is not in effect, and the early exit requires the flag to be still
true.
> Update the test from the previous commit to test_expect_success.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
Let's mark it for 'next'. Thanks.
> commit-reach.c | 10 +++++++---
> t/t6600-test-reach.sh | 2 +-
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/commit-reach.c b/commit-reach.c
> index 5df471a313..708798a39b 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -108,11 +108,14 @@ static int paint_down_to_common(struct repository *r,
> { compare_commits_by_gen_then_commit_date }
> };
> int i;
> + int gen_ordered = 1;
> timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
> struct commit_list **tail = result;
>
> - if (!min_generation && !corrected_commit_dates_enabled(r))
> + if (!min_generation && !corrected_commit_dates_enabled(r)) {
> queue.pq.compare = compare_commits_by_commit_date;
> + gen_ordered = 0;
> + }
>
> one->object.flags |= PARENT1;
> if (!n) {
> @@ -147,11 +150,12 @@ static int paint_down_to_common(struct repository *r,
> commit->object.flags |= RESULT;
> tail = commit_list_append(commit, tail);
> /*
> - * The queue is generation-ordered; no
> - * remaining common ancestor can be a
> + * When the queue is generation-ordered,
> + * no remaining common ancestor can be a
> * descendant of this one.
> */
> if (!(mb_flags & MERGE_BASE_FIND_ALL) &&
> + gen_ordered &&
> generation < GENERATION_NUMBER_INFINITY)
> break;
> }
> diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
> index 1090104220..0ff41381ff 100755
> --- a/t/t6600-test-reach.sh
> +++ b/t/t6600-test-reach.sh
> @@ -1003,7 +1003,7 @@ test_expect_success 'merge-base without --all is one of --all results' '
> grep -F -f single all
> '
>
> -test_expect_failure 'merge-base without --all, clock skew, v1 commit-graph' '
> +test_expect_success 'merge-base without --all, clock skew, v1 commit-graph' '
> git rev-parse skew-M2 >expect &&
> merge_base_all_modes skew-P1 skew-P2
> '
^ permalink raw reply
* Re: [PATCH v2 07/11] odb/transaction: propagate commit errors
From: Justin Tobler @ 2026-07-08 17:24 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster
In-Reply-To: <ak3xKlQTPP5OXYRn@pks.im>
On 26/07/08 08:41AM, Patrick Steinhardt wrote:
> On Tue, Jul 07, 2026 at 11:14:08PM -0500, Justin Tobler wrote:
> > diff --git a/odb/transaction.c b/odb/transaction.c
> > index df4275151b..51af2c9a61 100644
> > --- a/odb/transaction.c
> > +++ b/odb/transaction.c
> > @@ -16,19 +16,26 @@ int odb_transaction_begin(struct object_database *odb,
> > return ret;
> > }
> >
> > -void odb_transaction_commit(struct odb_transaction *transaction)
> > +int odb_transaction_commit(struct odb_transaction *transaction)
> > {
> > + int ret;
> > +
> > if (!transaction)
> > - return;
> > + return 0;
> >
> > /*
> > * Ensure the transaction ending matches the pending transaction.
> > */
> > ASSERT(transaction == transaction->source->odb->transaction);
> >
> > - transaction->commit(transaction);
> > + ret = transaction->commit(transaction);
> > + if (ret)
> > + return ret;
> > +
> > transaction->source->odb->transaction = NULL;
> > free(transaction);
> > +
> > + return 0;
> > }
>
> Doesn't this cause a leak now?
Good call. Ya, if odb_transaction_commit() fails, we don't free the
transaction. In the next version I'll go ahead and clear the transaction
if we fail.
> I think this interface here is doing the same mistake that our reference
> transactions did, where we automatically released the transaction on
> commit. That caused multiple lifetime issues with references all over
> the place.
Ya, it probaby makes sense to introduce a separate
`odb_transaction_release()` function to make this explicit and update
callers accordingly. I have another series I working on that introduces
`odb_transaction_abort()`. This might be a good place to add it in too.
-Justin
^ permalink raw reply
* [PATCH v2] SubmittingPatches: document how to retract a topic
From: Junio C Hamano @ 2026-07-08 17:09 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqzf02xonc.fsf@gitster.g>
While this document outlines an idealized lifecycle where an author
develops a patch, refines it with reviewer feedback, and
successfully merges it into Git, reality is rarely so seamless.
Sometimes, a topic must be abandoned. Doing so explicitly is far
better than leaving it in limbo, especially since topics can always
be resurrected later.
Clearly state that we encourage contributors to retract any topic
that does not pan out.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Added the third bullet that talks about when and how a topic
becomes stalled and discarded.
Documentation/SubmittingPatches | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index d2d82eb543..c269e474e3 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -123,6 +123,20 @@ of review.
top, it gets merged to the 'master' branch and waits to become part
of the next major release.
+But sometimes things do not work as planned:
+
+. If a mailing list discussion convinces you that your changes aren't
+ ideal, please explicitly retract the topic to save the maintainer
+ time and effort.
+
+. If you must drop a topic due to shifting priorities, lack of time,
+ or other commitments, notify the list as a courtesy so others can
+ take over. Anyone can resurrect the topic later when they have the
+ capacity to do so.
+
+. Topics with unaddressed review comments that remain inactive for
+ four weeks may be discarded by the maintainer.
+
In the following sections, many techniques and conventions are listed
to help your patches get reviewed effectively in such a life cycle.
--
2.55.0-302-gdbd766b094
^ permalink raw reply related
* Re: [PATCH v2 06/11] odb/transaction: propagate begin errors
From: Justin Tobler @ 2026-07-08 16:56 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster
In-Reply-To: <ak3xJFDqTSN1Naoy@pks.im>
On 26/07/08 08:41AM, Patrick Steinhardt wrote:
> On Tue, Jul 07, 2026 at 11:14:07PM -0500, Justin Tobler wrote:
> > @@ -490,10 +491,12 @@ int cache_tree_update(struct index_state *istate, int flags)
> >
> > trace_performance_enter();
> > trace2_region_enter("cache_tree", "update", istate->repo);
> > - transaction = odb_transaction_begin(the_repository->objects);
> > + if (!inflight)
> > + odb_transaction_begin_or_die(the_repository->objects, &transaction);
> > i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
> > "", 0, &skip, flags);
> > - odb_transaction_commit(transaction);
> > + if (!inflight)
> > + odb_transaction_commit(transaction);
> > trace2_region_leave("cache_tree", "update", istate->repo);
> > trace_performance_leave("cache_tree_update");
> > if (i < 0)
>
> Callsites like this really make me wonder why we even care to create
> a transaction in the first place if we basically just commit it
> immediately anyway. And while it's a bit sad that we have so many sites
> where we don't really know whether we even have a transaction, I think
> it's a good change that we have now annotated them clearly. A subsequent
> patch series may then eventually refactor those sites so that we stop
> depending on `odb->transaction` and inject the transaction via a
> parameter.
Call sites like the one mentioned above are using ODB transactions as an
optimization to batch the full fsyncs in bulk. In cases where the is not
already a transaction, they start one to take advantage of it.
I fully agree though that an ODB transaction should ideally be started
at a higher layer and wired down to these call sites. I have a couple of
patches in my tree that start to tackle this which I plan to send in
another series. :)
> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index d52f0533ce..36032a5365 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -1,6 +1,8 @@
> > #ifndef ODB_TRANSACTION_H
> > #define ODB_TRANSACTION_H
> >
> > +#include "git-compat-util.h"
>
> We typically don't include "git-compat-util.h" in header files.
Will remove. Thanks
> > @@ -36,11 +38,21 @@ struct odb_transaction {
> > };
> >
> > /*
> > - * Starts an ODB transaction. Subsequent objects are written to the transaction
> > - * and not committed until odb_transaction_commit() is invoked on the
> > - * transaction. If the ODB already has a pending transaction, NULL is returned.
> > + * Starts an ODB transaction and returns it via `out`. Subsequent objects are
> > + * written to the transaction and not committed until odb_transaction_commit()
> > + * is invoked on the transaction. Returns 0 on success and a negative value on
> > + * error. Note that it is considered an error to start a new transaction if the
> > + * ODB already has an inflight transaction pending.
> > */
> > -struct odb_transaction *odb_transaction_begin(struct object_database *odb);
> > +int odb_transaction_begin(struct object_database *odb,
> > + struct odb_transaction **out);
> > +
> > +static inline void odb_transaction_begin_or_die(struct object_database *odb,
> > + struct odb_transaction **out)
> > +{
> > + if (odb_transaction_begin(odb, out))
> > + die(_("failed to start ODB transaction"));
> > +}
>
> We could make it a bit simpler to use this function by continuing to
> return the transaction directly. But on the other hand this results in a
> more consistent interface.
Ya, I was a bit back and forth about this myself. I ultimately landed on
keeping a more consistent interface though. Happy to change if others
feel differently though.
-Justin
^ permalink raw reply
* Re: [PATCH] SubmittingPatches: abandoning a series
From: Junio C Hamano @ 2026-07-08 16:33 UTC (permalink / raw)
To: Weijie Yuan; +Cc: Michael Montalbo, git
In-Reply-To: <ak3jl3vu_P8eBXa-@wyuan.org>
Weijie Yuan <wy@wyuan.org> writes:
> On Tue, Jul 07, 2026 at 09:52:14PM -0700, Junio C Hamano wrote:
>> Michael Montalbo <mmontalbo@gmail.com> writes:
>> [...]
>> > Finally, maybe a rule of thumb as to how old a topic a topic should get
>> > before a "staleness" update is provided would be helpful, or maybe that
>> > is too contextually dependent / would potentially introduce some
>> > unwanted scheduling contract?
>> >
>> > Overall, I think the explicit guidance is helpful.
>>
>> I've been working on streamlining my workflow to manage the "What's
>> cooking" report, and writing down guidelines with concrete numbers.
>>
>> * When the discussion seems to reach rough consensus that the
>> latest round looks good for 'next', the topic is immediately
>> marked as "Will merge to 'next'?" in my draft copy (note: I do
>> not want to spam the list with "What's cooking" too often, but it
>> is the document I work from, and it is updated multiple times a
>> day).
>>
>> * After no negative opinions are seen on a topic in "Will merge to
>> 'next'?" state for about 36 hours, the topic is marked as "Will
>> merge to 'next'". I will merge such topics during the next
>> integration cycle (note: I can only have up to two integration
>> cycles per day due to time constraints).
>>
>> * Imagine that a topic was last updated more than 4 weeks ago. If
>> there are review comments on the topic that are left unanswered
>> by the author for more than a week, and if nothing happens in the
>> discussion thread other than inquiry on the current status, the
>> topic is marked as "stalled". I will try to notice and ping a
>> stalled topic once or twice, but after that I may discard such a
>> topic (which by the way I really hate having to decide to do so).
>>
>> * After a topic is merged to 'next', if nothing negative that needs
>> fixing is discovered for 7 calendar days, the topic is merged to
>> 'master'. I may shorten this depending on how complex the topic
>> is.
>>
>> There may be more, but these are what I can think of offhand.
>
> Integrating the above parts into the document seems like a good idea.
It could be, but not to the document we are discussing. The current
document is about contributor's guide, telling them what to do and
how, and "'next' usually holds a topic for 7 days" is not as
interesting.
> btw, do we need to synchronize MyFirstContribution simultaneously?
>
> Quoting Patrick's words [1]:
>
> Overall it's a bit on the annoying side that we have to always make sure
> to update both SubmittingPatches and MyFirstContribution in tandem.
> Makes me wonder whether they are mostly redundant and whether it would
> make sense to eventually merge them.
Surely, but that is outside the scope of this patch.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 05/11] object-file: propagate files transaction errors
From: Justin Tobler @ 2026-07-08 16:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster
In-Reply-To: <ak3xHiOQfNxqFR58@pks.im>
On 26/07/08 08:41AM, Patrick Steinhardt wrote:
> On Tue, Jul 07, 2026 at 11:14:06PM -0500, Justin Tobler wrote:
> > diff --git a/object-file.c b/object-file.c
> > index e51389833a..64cb874fe7 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -511,11 +511,15 @@ static void odb_transaction_files_prepare(struct odb_transaction *base)
> > * added at the time they call odb_transaction_files_begin.
> > */
> > if (!transaction || transaction->objdir)
> > - return;
> > + return 0;
> >
> > transaction->objdir = tmp_objdir_create(base->source->odb->repo, "bulk-fsync");
> > - if (transaction->objdir)
> > - tmp_objdir_replace_primary_odb(transaction->objdir, 0);
> > + if (!transaction->objdir)
> > + return -1;
>
> As far as I can see we don't report any errors as part of
> `tmp_objdir_create()`, so we should probably print an error here.
>
> > @@ -1668,14 +1672,19 @@ static void odb_transaction_files_commit(struct odb_transaction *base)
> > * Make the object files visible in the primary ODB after their data is
> > * fully durable.
> > */
> > - tmp_objdir_migrate(transaction->objdir);
> > + if (tmp_objdir_migrate(transaction->objdir))
> > + return -1;
> > +
>
> Likewise.
Ya, printing some error messages seems like a good idea here. Will do so
in the next version.
-Justin
^ permalink raw reply
* Re: What's cooking in git.git (Jul 2026, #03)
From: Junio C Hamano @ 2026-07-08 16:14 UTC (permalink / raw)
To: Kristofer Karlsson; +Cc: git
In-Reply-To: <CAL71e4MrQ25WJHp-08bTS2=y-gd36zs9CVYmYGeRv-6iCpdfdQ@mail.gmail.com>
Kristofer Karlsson <krka@spotify.com> writes:
> I tried building v6 of kk/merge-base-exhaustion on top of
> kk/commit-reach-find-all-fix but since that one is based
> on kk/paint-down-to-common-optim it does not include
> the changes from kk/commit-reach-optim which I also depend
> on.
>
> I thus think the status of kk/merge-base-exhaustion should
> instead be:
> "On hold, waiting for kk/commit-reach-find-all-fix to land first."
>
> Alternatively you could rebase kk/commit-reach-find-all-fix
> on master (triggers a small conflict though) and that would
> also unblock a reroll (but I don't want to generate more work for you).
When topic A depends on topic B and topic C, rather than rebasing A
on 'next' (if B and C are in 'next'), I would often do
$ git checkout -b A master
$ git merge --no-ff B
$ git merge --no-ff C
these days to prepare the "base" to apply patches that belong to A.
Many contributors mimic this and state in their cover letter that
the patches in the series are to be applied on top of master at
commit X with branches Y and Z merged, or something to that effect.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 03/11] object-file: embed transaction flush logic in commit function
From: Justin Tobler @ 2026-07-08 16:08 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster
In-Reply-To: <ak3xFCzvgj1-Ev_3@pks.im>
On 26/07/08 08:41AM, Patrick Steinhardt wrote:
> On Tue, Jul 07, 2026 at 11:14:04PM -0500, Justin Tobler wrote:
> > When a "files" transaction is committed,
> > `flush_loose_object_transaction()` is invoked to handle performing a
> > hardware flush along with migrating the temporary object directory into
> > the primary. In a subsequent commit, the temporary directory is also
> > used to write packfiles.
> >
> > Instead of maintaining a separate helper function, embed the logic to
> > flush and migrate the temporary directory directly into
> > `odb_transaction_files_commit()`.
>
> The change itself looks simple enough, but this makes me wonder why we
> want to change this. Like, what subsequent step does this enable that
> would otherwise be harder to do?
Originally, I was planning to rename both
`{fsync,flush}_loose_object_transaction()` to
`odb_transaction_files_{fsync,flush}()` respectively. For the flush
helper though, it's doing more than just "flushing" the temporary
directory files; it's also migrating the objects to the primary ODB and
reconfiguring the repository ODB sources accordingly.
This is really what you think of happening during the commit phase.
Consequently, it made more sense to me organizationally to just embed
the logic `odb_transaction_files_commit()`.
> Maybe this will be answered by a subsequent commit.
Will update the commit message to properly explain this.
-Justin
^ permalink raw reply
* [PATCH v8 9/9] environment: move object_creation_mode into repo_config_values
From: Tian Yuchen @ 2026-07-08 16:03 UTC (permalink / raw)
To: git
Cc: cirnovskyv, szeder.dev, Tian Yuchen, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260708160300.8852-1-cat@malon.dev>
The global variable 'object_creation_mode' controls how Git creates
object files, specifically determining whether to use hardlinks or
renames when moving temporary files into the object database. Move
it into 'struct repo_config_values' to continue the libification
effort.
Move the 'enum object_creation_mode' definition higher up in
'environment.h' to ensure it is visible to the structure. Initialize
the per-repository value to its default macro value
OBJECT_CREATION_MODE inside 'repo_config_values_init()'.
Update configuration parsing in 'git_default_core_config()' to write
directly to the repository-specific configuration structure.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
environment.c | 6 +++---
environment.h | 12 ++++++------
object-file.c | 2 +-
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/environment.c b/environment.c
index 7701aa3bc0..e50beda918 100644
--- a/environment.c
+++ b/environment.c
@@ -61,7 +61,6 @@ char *check_roundtrip_encoding;
#ifndef OBJECT_CREATION_MODE
#define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
#endif
-enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
unsigned long pack_size_limit_cfg;
@@ -511,9 +510,9 @@ int git_default_core_config(const char *var, const char *value,
if (!value)
return config_error_nonbool(var);
if (!strcmp(value, "rename"))
- object_creation_mode = OBJECT_CREATION_USES_RENAMES;
+ cfg->object_creation_mode = OBJECT_CREATION_USES_RENAMES;
else if (!strcmp(value, "link"))
- object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
+ cfg->object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
else
die(_("invalid mode for object creation: %s"), value);
return 0;
@@ -727,6 +726,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->apply_default_ignorewhitespace = NULL;
cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
cfg->autorebase = AUTOREBASE_NEVER;
+ cfg->object_creation_mode = OBJECT_CREATION_MODE;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
diff --git a/environment.h b/environment.h
index 464ff73136..eaa0aba7bc 100644
--- a/environment.h
+++ b/environment.h
@@ -109,6 +109,11 @@ enum rebase_setup_type {
AUTOREBASE_ALWAYS
};
+enum object_creation_mode {
+ OBJECT_CREATION_USES_HARDLINKS = 0,
+ OBJECT_CREATION_USES_RENAMES = 1
+};
+
struct repo_config_values {
/* section "core" config values */
char *attributes_file;
@@ -120,6 +125,7 @@ struct repo_config_values {
char *apply_default_ignorewhitespace;
enum push_default_type push_default;
enum rebase_setup_type autorebase;
+ enum object_creation_mode object_creation_mode;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -213,12 +219,6 @@ extern unsigned long pack_size_limit_cfg;
extern int protect_hfs;
extern int protect_ntfs;
-enum object_creation_mode {
- OBJECT_CREATION_USES_HARDLINKS = 0,
- OBJECT_CREATION_USES_RENAMES = 1
-};
-extern enum object_creation_mode object_creation_mode;
-
extern int grafts_keep_true_parents;
const char *get_log_output_encoding(void);
diff --git a/object-file.c b/object-file.c
index 9afa842da2..cbbfc8f1dc 100644
--- a/object-file.c
+++ b/object-file.c
@@ -415,7 +415,7 @@ int finalize_object_file_flags(struct repository *repo,
retry:
ret = 0;
- if (object_creation_mode == OBJECT_CREATION_USES_RENAMES)
+ if (repo_config_values(repo)->object_creation_mode == OBJECT_CREATION_USES_RENAMES)
goto try_rename;
else if (link(tmpfile, filename))
ret = errno;
--
2.43.0
^ permalink raw reply related
* [PATCH v8 8/9] environment: move autorebase into repo_config_values
From: Tian Yuchen @ 2026-07-08 16:02 UTC (permalink / raw)
To: git
Cc: cirnovskyv, szeder.dev, Tian Yuchen, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260708160300.8852-1-cat@malon.dev>
The global variable 'autorebase' dictates whether a newly created
branch should be configured to automatically rebase by default.
Move it into 'struct repo_config_values' to continue the
libification effort.
The 'enum rebase_setup_type' definition is moved higher up in
'environment.h' so that it is visible to the repository-specific
structure. The default state AUTOREBASE_NEVER is now correctly
initialized in 'repo_config_values_init()'.
Configuration parsing in 'git_default_branch_config()' is updated to
write directly to the repository's configuration instance.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
branch.c | 2 +-
environment.c | 10 +++++-----
environment.h | 16 ++++++++--------
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 243db7d0fc..e1c1f8c89d 100644
--- a/branch.c
+++ b/branch.c
@@ -61,7 +61,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)
static int should_setup_rebase(const char *origin)
{
- switch (autorebase) {
+ switch (repo_config_values(the_repository)->autorebase) {
case AUTOREBASE_NEVER:
return 0;
case AUTOREBASE_LOCAL:
diff --git a/environment.c b/environment.c
index 09de2fee87..7701aa3bc0 100644
--- a/environment.c
+++ b/environment.c
@@ -58,7 +58,6 @@ enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
enum eol core_eol = EOL_UNSET;
int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
char *check_roundtrip_encoding;
-enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
#ifndef OBJECT_CREATION_MODE
#define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
#endif
@@ -600,13 +599,13 @@ static int git_default_branch_config(const char *var, const char *value)
if (!value)
return config_error_nonbool(var);
else if (!strcmp(value, "never"))
- autorebase = AUTOREBASE_NEVER;
+ cfg->autorebase = AUTOREBASE_NEVER;
else if (!strcmp(value, "local"))
- autorebase = AUTOREBASE_LOCAL;
+ cfg->autorebase = AUTOREBASE_LOCAL;
else if (!strcmp(value, "remote"))
- autorebase = AUTOREBASE_REMOTE;
+ cfg->autorebase = AUTOREBASE_REMOTE;
else if (!strcmp(value, "always"))
- autorebase = AUTOREBASE_ALWAYS;
+ cfg->autorebase = AUTOREBASE_ALWAYS;
else
return error(_("malformed value for %s"), var);
return 0;
@@ -727,6 +726,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->apply_default_whitespace = NULL;
cfg->apply_default_ignorewhitespace = NULL;
cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
+ cfg->autorebase = AUTOREBASE_NEVER;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
diff --git a/environment.h b/environment.h
index 72859b5d76..464ff73136 100644
--- a/environment.h
+++ b/environment.h
@@ -102,6 +102,13 @@ enum push_default_type {
PUSH_DEFAULT_UNSPECIFIED
};
+enum rebase_setup_type {
+ AUTOREBASE_NEVER = 0,
+ AUTOREBASE_LOCAL,
+ AUTOREBASE_REMOTE,
+ AUTOREBASE_ALWAYS
+};
+
struct repo_config_values {
/* section "core" config values */
char *attributes_file;
@@ -112,6 +119,7 @@ struct repo_config_values {
char *apply_default_whitespace;
char *apply_default_ignorewhitespace;
enum push_default_type push_default;
+ enum rebase_setup_type autorebase;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -205,14 +213,6 @@ extern unsigned long pack_size_limit_cfg;
extern int protect_hfs;
extern int protect_ntfs;
-enum rebase_setup_type {
- AUTOREBASE_NEVER = 0,
- AUTOREBASE_LOCAL,
- AUTOREBASE_REMOTE,
- AUTOREBASE_ALWAYS
-};
-extern enum rebase_setup_type autorebase;
-
enum object_creation_mode {
OBJECT_CREATION_USES_HARDLINKS = 0,
OBJECT_CREATION_USES_RENAMES = 1
--
2.43.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox