* [PATCH v4 3/8] t6099, t6600: add side-exhaustion regression tests
From: Kristofer Karlsson via GitGitGadget @ 2026-06-28 12:25 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2149.v4.git.1782649547.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
Add t6099 to test the case where multiple merge-base candidates exist
and one is an ancestor of another. This exercises the side-exhaustion
optimization in paint_down_to_common together with the
remove_redundant safety net in get_merge_bases_many_0.
Add a mixed finite/INFINITY test to t6600 where one tip is outside
the commit-graph (INFINITY generation) and the other is inside.
This exercises the region transition: the walk starts in the
INFINITY region where side-exhaustion is disabled, then crosses
into the finite region where it can fire.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
t/meson.build | 1 +
t/t6099-merge-base-side-exhaustion.sh | 82 +++++++++++++++++++++++++++
t/t6600-test-reach.sh | 25 ++++++++
3 files changed, 108 insertions(+)
create mode 100755 t/t6099-merge-base-side-exhaustion.sh
diff --git a/t/meson.build b/t/meson.build
index 3219264fe7..ee6ebdffb9 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -786,6 +786,7 @@ integration_tests = [
't6041-bisect-submodule.sh',
't6050-replace.sh',
't6060-merge-index.sh',
+ 't6099-merge-base-side-exhaustion.sh',
't6100-rev-list-in-order.sh',
't6101-rev-parse-parents.sh',
't6102-rev-list-unexpected-objects.sh',
diff --git a/t/t6099-merge-base-side-exhaustion.sh b/t/t6099-merge-base-side-exhaustion.sh
new file mode 100755
index 0000000000..4f1e0d50ef
--- /dev/null
+++ b/t/t6099-merge-base-side-exhaustion.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='merge-base with ancestor among merge-base candidates
+
+Test that merge-base --all correctly handles cases where
+multiple merge-base candidates exist and one is an ancestor
+of another. The side-exhaustion optimization in
+paint_down_to_common may exit before STALE propagation
+removes the ancestor, but remove_redundant catches it.
+
+Graph shape (parents are below children):
+
+ A ----------- X
+ |\ /|
+ | B---------/ |
+ | | |
+ e2 \ f2
+ | | |
+ e1 d1 f1
+ \ | /
+ \ | /
+ \| /
+ C
+
+A and X are the two tips.
+B and C are both reachable from A and X.
+B reaches C through d1.
+Only B should appear in merge-base --all output.
+'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'setup ancestor merge-base candidate' '
+ test_commit C &&
+
+ git checkout -b d-chain HEAD &&
+ test_commit d1 &&
+ test_commit B &&
+
+ git checkout -b e-path C &&
+ test_commit e1 &&
+ test_commit e2 &&
+
+ git checkout -b f-path C &&
+ test_commit f1 &&
+ test_commit f2 &&
+
+ git checkout -b branch-A e-path &&
+ test_merge A B &&
+
+ git checkout -b branch-X f-path &&
+ test_merge X B &&
+
+ git commit-graph write --reachable
+'
+
+test_expect_success 'merge-base --all excludes ancestor candidate' '
+ git rev-parse B >expected &&
+ git merge-base --all A X >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'merge-base (single) finds shallowest' '
+ git rev-parse B >expected &&
+ git merge-base A X >actual &&
+ test_cmp expected actual
+'
+
+# Without commit-graph: generation numbers are INFINITY,
+# side-exhaustion optimization does not fire.
+test_expect_success 'merge-base --all without commit-graph' '
+ rm -f .git/objects/info/commit-graph &&
+ git rev-parse B >expected &&
+ git merge-base --all A X >actual &&
+ test_cmp expected actual
+'
+
+test_done
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index c2e091aad1..4b771b4c58 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -294,6 +294,31 @@ test_expect_success 'get_merge_bases_many:infinity-both-sides' '
test_all_modes get_merge_bases_many
'
+test_expect_success 'setup mixed finite/INFINITY topology' '
+ # Create a commit outside all saved commit-graph files so it always
+ # has INFINITY generation, while its parent (ps-X) is in the graph
+ # with a finite generation. Use the ps-* orphan topology so we do
+ # not pollute the grid-based rev-list tests.
+ git checkout ps-X &&
+ test_env GIT_TEST_COMMIT_GRAPH= test_commit pm-INF
+'
+
+test_expect_success 'get_merge_bases_many:mixed-finite-infinity' '
+ # One tip (pm-INF) is outside the commit-graph with INFINITY
+ # generation; the other (ps-B) is in the graph with finite
+ # generation. The walk starts in the INFINITY region and crosses
+ # into the finite region where side-exhaustion can fire.
+ cat >input <<-\EOF &&
+ A:pm-INF
+ X:ps-B
+ EOF
+ {
+ echo "get_merge_bases_many(A,X):" &&
+ git rev-parse ps-X
+ } >expect &&
+ test_all_modes get_merge_bases_many
+'
+
test_expect_success 'reduce_heads' '
cat >input <<-\EOF &&
X:commit-1-10
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 4/8] commit-reach: add trace2 instrumentation to paint_down_to_common()
From: Kristofer Karlsson via GitGitGadget @ 2026-06-28 12:25 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2149.v4.git.1782649547.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
Add a step counter and trace2_data_intmax() call so that the number
of commits visited during the paint walk is observable via
GIT_TRACE2_EVENT. This provides a way to measure the impact of
future optimizations without relying on wall-clock benchmarks alone.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
commit-reach.c | 5 ++++
t/t6600-test-reach.sh | 53 ++++++++++++++++++++++++++++++-------------
2 files changed, 42 insertions(+), 16 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index a9483759e0..f6a438550b 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -11,6 +11,7 @@
#include "tag.h"
#include "commit-reach.h"
#include "ewah/ewok.h"
+#include "trace2.h"
/* Remember to update object flag allocation in object.h */
#define PARENT1 (1u<<16)
@@ -112,6 +113,7 @@ static int paint_down_to_common(struct repository *r,
{ compare_commits_by_gen_then_commit_date }
};
int i;
+ int steps = 0;
timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
struct commit_list **tail = result;
@@ -135,6 +137,7 @@ static int paint_down_to_common(struct repository *r,
struct commit_list *parents;
int flags;
timestamp_t generation = commit_graph_generation(commit);
+ steps++;
if (min_generation && generation > last_gen)
BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
@@ -190,6 +193,8 @@ static int paint_down_to_common(struct repository *r,
}
clear_nonstale_queue(&queue);
+ trace2_data_intmax("paint_down_to_common", r,
+ "steps", steps);
commit_list_sort_by_date(result);
return 0;
}
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index 4b771b4c58..b3a31b80ac 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -118,24 +118,34 @@ test_expect_success 'setup' '
'
run_all_modes () {
- test_when_finished rm -rf .git/objects/info/commit-graph &&
- "$@" <input >actual &&
- test_cmp expect actual &&
- cp commit-graph-full .git/objects/info/commit-graph &&
- "$@" <input >actual &&
- test_cmp expect actual &&
- cp commit-graph-half .git/objects/info/commit-graph &&
- "$@" <input >actual &&
- test_cmp expect actual &&
- cp commit-graph-no-gdat .git/objects/info/commit-graph &&
- "$@" <input >actual &&
- test_cmp expect actual
+ graph=.git/objects/info/commit-graph &&
+ test_when_finished rm -rf "$graph" "${graph}s" &&
+ rm -f trace-mode-*.txt &&
+
+ for mode in none full half no-gdat
+ do
+ rm -rf "$graph" "${graph}s" &&
+ cp "commit-graph-${mode}" "$graph" 2>/dev/null ||
+ true &&
+ GIT_TRACE2_EVENT="$(pwd)/trace-mode-${mode}.txt" \
+ "$@" <input >actual &&
+ test_cmp expect actual || return 1
+ done
}
test_all_modes () {
run_all_modes test-tool reach "$@"
}
+test_paint_down_steps () {
+ for mode in none full half no-gdat
+ do
+ test_trace2_data paint_down_to_common steps "$1" \
+ <"trace-mode-${mode}.txt" || return 1
+ shift
+ done
+}
+
test_expect_success 'ref_newer:miss' '
cat >input <<-\EOF &&
A:commit-5-7
@@ -209,7 +219,8 @@ test_expect_success 'in_merge_bases_many:self' '
X:commit-6-8
EOF
echo "in_merge_bases_many(A,X):1" >expect &&
- test_all_modes in_merge_bases_many
+ test_all_modes in_merge_bases_many &&
+ test_paint_down_steps 45 2 25 3
'
test_expect_success 'is_descendant_of:hit' '
@@ -277,7 +288,8 @@ test_expect_success 'get_merge_bases_many:pending-stale' '
echo "get_merge_bases_many(A,X):" &&
git rev-parse ps-B
} >expect &&
- test_all_modes get_merge_bases_many
+ test_all_modes get_merge_bases_many &&
+ test_paint_down_steps 6 6 6 6
'
test_expect_success 'get_merge_bases_many:infinity-both-sides' '
@@ -291,7 +303,8 @@ test_expect_success 'get_merge_bases_many:infinity-both-sides' '
echo "get_merge_bases_many(A,X):" &&
git rev-parse pi-B
} >expect &&
- test_all_modes get_merge_bases_many
+ test_all_modes get_merge_bases_many &&
+ test_paint_down_steps 5 5 5 5
'
test_expect_success 'setup mixed finite/INFINITY topology' '
@@ -316,7 +329,15 @@ test_expect_success 'get_merge_bases_many:mixed-finite-infinity' '
echo "get_merge_bases_many(A,X):" &&
git rev-parse ps-X
} >expect &&
- test_all_modes get_merge_bases_many
+ test_all_modes get_merge_bases_many &&
+ test_paint_down_steps 3 3 3 3
+'
+
+test_expect_success 'merge-base --all commit-walk steps' '
+ >input &&
+ git rev-parse commit-9-1 >expect &&
+ run_all_modes git merge-base --all commit-9-9 commit-9-1 &&
+ test_paint_down_steps 81 80 81 81
'
test_expect_success 'reduce_heads' '
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 5/8] commit-reach: introduce struct paint_state with per-side counters
From: Kristofer Karlsson via GitGitGadget @ 2026-06-28 12:25 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2149.v4.git.1782649547.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
Add a paint_state struct for use by paint_down_to_common() that
wraps a prio_queue with per-side commit counters. Each non-stale
queued commit occupies exactly one counter bucket based on its
paint flags: PARENT1-only, PARENT2-only, or both sides (a pending
merge-base candidate).
The counters are maintained by paint_count_update() which adjusts
the appropriate bucket by a signed delta. An exhaustive switch on
the paint+stale bits documents all valid flag combinations in one
place.
Convert paint_down_to_common() to use paint_state. The loop now
drains the queue via paint_queue_get() which returns NULL when all
counters reach zero, replacing the old pointer-based termination
(max_nonstale). This is equivalent behavior -- both conditions
detect that no non-stale entries remain.
paint_queue_get() uses a "pop first" form: it dequeues a commit,
then checks the counters. This means the loop exits one iteration
earlier than the old code in some topologies (the popped stale
commit is never processed), so a few step counts drop by one.
The existing nonstale_queue is left in place for ahead_behind().
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
.../technical/paint-down-to-common.adoc | 9 +-
commit-reach.c | 94 ++++++++++++++++---
t/t6600-test-reach.sh | 4 +-
3 files changed, 85 insertions(+), 22 deletions(-)
diff --git a/Documentation/technical/paint-down-to-common.adoc b/Documentation/technical/paint-down-to-common.adoc
index a4dfcba038..ac3e2b39a5 100644
--- a/Documentation/technical/paint-down-to-common.adoc
+++ b/Documentation/technical/paint-down-to-common.adoc
@@ -88,15 +88,12 @@ re-enqueued is bounded by the number of flag transitions.
Termination
-----------
-The walk uses a `nonstale_queue` wrapper around `prio_queue` that
-tracks `max_nonstale`: the lowest-priority non-stale commit enqueued
-so far. Once that commit is dequeued, every remaining entry is known
-to be STALE and the loop terminates. Specifically, the main loop
+The walk tracks the number of commits of each type in the queue
+(PARENT1-only, PARENT2-only, pending merge-base). The main loop
ends when one of the following conditions holds:
1. The queue is empty.
- 2. `max_nonstale` has been dequeued, meaning the queue only contains
- STALE entries.
+ 2. The queue contains only stale entries.
3. Generation cutoff: the dequeued commit's generation is below
a caller-supplied `min_generation` threshold.
4. Single result: the caller only needs one merge base, one has
diff --git a/commit-reach.c b/commit-reach.c
index f6a438550b..9ae306f60c 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -97,6 +97,75 @@ static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue)
return commit;
}
+/*
+ * Priority queue with per-side commit counters for paint_down_to_common().
+ * Each non-stale queued commit occupies exactly one bucket: PARENT1-only,
+ * PARENT2-only, or both (a pending merge-base candidate).
+ */
+struct paint_state {
+ struct prio_queue queue;
+ size_t parent1_count;
+ size_t parent2_count;
+ size_t mb_candidate_count;
+};
+
+static void paint_count_update(struct paint_state *state,
+ unsigned flags, int delta)
+{
+ switch (flags & (PARENT1 | PARENT2 | STALE)) {
+ case PARENT1:
+ state->parent1_count += delta;
+ break;
+
+ case PARENT2:
+ state->parent2_count += delta;
+ break;
+
+ case PARENT1 | PARENT2:
+ state->mb_candidate_count += delta;
+ break;
+
+ case PARENT1 | PARENT2 | STALE:
+ break;
+
+ default:
+ BUG("unexpected paint state");
+ }
+}
+
+static void paint_queue_put(struct paint_state *state,
+ struct commit *c, unsigned add_flags)
+{
+ unsigned old_flags = c->object.flags;
+ c->object.flags |= add_flags;
+
+ if (old_flags & ENQUEUED) {
+ paint_count_update(state, old_flags, -1);
+ paint_count_update(state, c->object.flags, 1);
+ } else {
+ c->object.flags |= ENQUEUED;
+ prio_queue_put(&state->queue, c);
+ paint_count_update(state, c->object.flags, 1);
+ }
+}
+
+static struct commit *paint_queue_get(struct paint_state *state)
+{
+ struct commit *commit = prio_queue_get(&state->queue);
+
+ if (!commit)
+ return NULL;
+
+ commit->object.flags &= ~ENQUEUED;
+
+ if (!state->parent1_count && !state->parent2_count &&
+ !state->mb_candidate_count)
+ return NULL;
+
+ paint_count_update(state, commit->object.flags, -1);
+ return commit;
+}
+
/*
* See Documentation/technical/paint-down-to-common.adoc
*
@@ -109,31 +178,29 @@ static int paint_down_to_common(struct repository *r,
enum merge_base_flags mb_flags,
struct commit_list **result)
{
- struct nonstale_queue queue = {
- { compare_commits_by_gen_then_commit_date }
+ struct paint_state state = {
+ .queue = { compare_commits_by_gen_then_commit_date }
};
+ struct commit *commit;
int i;
int steps = 0;
timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
struct commit_list **tail = result;
if (!min_generation && !corrected_commit_dates_enabled(r))
- queue.pq.compare = compare_commits_by_commit_date;
+ state.queue.compare = compare_commits_by_commit_date;
one->object.flags |= PARENT1;
if (!n) {
commit_list_append(one, result);
return 0;
}
- nonstale_queue_put_dedup(&queue, one);
+ paint_queue_put(&state, one, 0);
- for (i = 0; i < n; i++) {
- twos[i]->object.flags |= PARENT2;
- nonstale_queue_put_dedup(&queue, twos[i]);
- }
+ for (i = 0; i < n; i++)
+ paint_queue_put(&state, twos[i], PARENT2);
- while (queue.max_nonstale) {
- struct commit *commit = nonstale_queue_get_dedup(&queue);
+ while ((commit = paint_queue_get(&state))) {
struct commit_list *parents;
int flags;
timestamp_t generation = commit_graph_generation(commit);
@@ -172,7 +239,7 @@ static int paint_down_to_common(struct repository *r,
if ((p->object.flags & flags) == flags)
continue;
if (repo_parse_commit(r, p)) {
- clear_nonstale_queue(&queue);
+ clear_prio_queue(&state.queue);
commit_list_free(*result);
*result = NULL;
/*
@@ -187,12 +254,11 @@ static int paint_down_to_common(struct repository *r,
return error(_("could not parse commit %s"),
oid_to_hex(&p->object.oid));
}
- p->object.flags |= flags;
- nonstale_queue_put_dedup(&queue, p);
+ paint_queue_put(&state, p, flags);
}
}
- clear_nonstale_queue(&queue);
+ clear_prio_queue(&state.queue);
trace2_data_intmax("paint_down_to_common", r,
"steps", steps);
commit_list_sort_by_date(result);
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index b3a31b80ac..51f3d70492 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -289,7 +289,7 @@ test_expect_success 'get_merge_bases_many:pending-stale' '
git rev-parse ps-B
} >expect &&
test_all_modes get_merge_bases_many &&
- test_paint_down_steps 6 6 6 6
+ test_paint_down_steps 5 5 5 5
'
test_expect_success 'get_merge_bases_many:infinity-both-sides' '
@@ -304,7 +304,7 @@ test_expect_success 'get_merge_bases_many:infinity-both-sides' '
git rev-parse pi-B
} >expect &&
test_all_modes get_merge_bases_many &&
- test_paint_down_steps 5 5 5 5
+ test_paint_down_steps 5 4 5 5
'
test_expect_success 'setup mixed finite/INFINITY topology' '
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 6/8] commit-reach: remove unused nonstale_queue dedup wrappers
From: Kristofer Karlsson via GitGitGadget @ 2026-06-28 12:25 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2149.v4.git.1782649547.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
nonstale_queue_put_dedup() and nonstale_queue_get_dedup() became
unused after the previous commit. The core nonstale_queue functions
remain in use by ahead_behind().
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
commit-reach.c | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 9ae306f60c..176ffd68d0 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -79,24 +79,6 @@ static void clear_nonstale_queue(struct nonstale_queue *queue)
queue->max_nonstale = NULL;
}
-static void nonstale_queue_put_dedup(struct nonstale_queue *queue,
- struct commit *c)
-{
- if (c->object.flags & ENQUEUED)
- return;
- c->object.flags |= ENQUEUED;
- nonstale_queue_put(queue, c);
-}
-
-static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue)
-{
- struct commit *commit = nonstale_queue_get(queue);
-
- if (commit)
- commit->object.flags &= ~ENQUEUED;
- return commit;
-}
-
/*
* Priority queue with per-side commit counters for paint_down_to_common().
* Each non-stale queued commit occupies exactly one bucket: PARENT1-only,
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 7/8] commit-reach: terminate merge-base walk when one paint side is exhausted
From: Kristofer Karlsson via GitGitGadget @ 2026-06-28 12:25 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2149.v4.git.1782649547.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
Add an early termination check to paint_down_to_common() using the
per-side counters introduced earlier. Once the walk enters the
finite-generation region, terminate early when one side's exclusive
count drops to zero -- no new merge-base can form without both paint
sides meeting.
The check also waits for pending_merge_bases to reach zero, ensuring
all merge-base candidates have been dequeued and recorded before
exiting.
The INFINITY gate ensures correctness: commits without a commit-graph
entry have GENERATION_NUMBER_INFINITY and are ordered by commit date,
which is not topologically reliable. The optimization only fires
once the walk enters the finite-generation region where ordering
guarantees hold.
Step counts measured with trace2 on git.git with commit-graph:
merge-base --all v2.0.0 v2.55.0-rc1:
before: 72264 steps after: 44589 steps
merge-base --all v2.55.0-rc1 v2.55.0-rc1~5:
before: 110 steps after: 7 steps
Helped-by: Derrick Stolee <stolee@gmail.com>
Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
.../technical/paint-down-to-common.adoc | 17 +++++++++++++++++
commit-reach.c | 17 ++++++++++++++---
t/t6600-test-reach.sh | 4 ++--
3 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/Documentation/technical/paint-down-to-common.adoc b/Documentation/technical/paint-down-to-common.adoc
index ac3e2b39a5..15adac7885 100644
--- a/Documentation/technical/paint-down-to-common.adoc
+++ b/Documentation/technical/paint-down-to-common.adoc
@@ -99,6 +99,9 @@ ends when one of the following conditions holds:
4. Single result: the caller only needs one merge base, one has
been found, and the walk has entered the finite-generation
region.
+ 5. Side exhaustion: no pure PARENT1 or pure PARENT2 commits
+ remain in the queue, no pending merge-base candidates exist,
+ and the walk has entered the finite-generation region.
Stale entry condition
~~~~~~~~~~~~~~~~~~~~~
@@ -109,6 +112,20 @@ existing candidates by proving one is an ancestor of another, but
`remove_redundant()` handles that as a post-processing step, so it
is safe to exit early.
+Side-exhaustion condition
+~~~~~~~~~~~~~~~~~~~~~~~~~
+A new merge-base requires commits from both sides to meet. When one
+side's exclusive counter reaches zero and there are no pending
+merge-base candidates, no future traversal step can produce a new
+candidate.
+
+This optimization only activates in the finite-generation region
+where topological ordering holds. In that region, children are
+always visited before parents, so paint flags are final at visit
+time and an exhausted side cannot reappear. In the INFINITY region,
+commit-date ordering can violate this guarantee, so the check is
+skipped.
+
Generation cutoff
~~~~~~~~~~~~~~~~~
Some callers (notably `remove_redundant()`) supply a `min_generation`
diff --git a/commit-reach.c b/commit-reach.c
index 176ffd68d0..e174b219c6 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -131,6 +131,10 @@ static void paint_queue_put(struct paint_state *state,
}
}
+/*
+ * Dequeue the next commit for the paint walk, or return NULL when
+ * no more merge bases can be discovered.
+ */
static struct commit *paint_queue_get(struct paint_state *state)
{
struct commit *commit = prio_queue_get(&state->queue);
@@ -140,9 +144,16 @@ static struct commit *paint_queue_get(struct paint_state *state)
commit->object.flags &= ~ENQUEUED;
- if (!state->parent1_count && !state->parent2_count &&
- !state->mb_candidate_count)
- return NULL;
+ if (!state->mb_candidate_count) {
+ /* only stale entries remain */
+ if (!state->parent1_count && !state->parent2_count)
+ return NULL;
+
+ /* one side is exhausted */
+ if ((!state->parent1_count || !state->parent2_count) &&
+ commit_graph_generation(commit) < GENERATION_NUMBER_INFINITY)
+ return NULL;
+ }
paint_count_update(state, commit->object.flags, -1);
return commit;
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index 51f3d70492..6365007560 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -220,7 +220,7 @@ test_expect_success 'in_merge_bases_many:self' '
EOF
echo "in_merge_bases_many(A,X):1" >expect &&
test_all_modes in_merge_bases_many &&
- test_paint_down_steps 45 2 25 3
+ test_paint_down_steps 45 1 25 1
'
test_expect_success 'is_descendant_of:hit' '
@@ -337,7 +337,7 @@ test_expect_success 'merge-base --all commit-walk steps' '
>input &&
git rev-parse commit-9-1 >expect &&
run_all_modes git merge-base --all commit-9-9 commit-9-1 &&
- test_paint_down_steps 81 80 81 81
+ test_paint_down_steps 81 9 57 10
'
test_expect_success 'reduce_heads' '
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 8/8] commit-reach: move min_generation check into paint_queue_get()
From: Kristofer Karlsson via GitGitGadget @ 2026-06-28 12:25 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2149.v4.git.1782649547.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
Consolidate the min_generation termination condition into
paint_queue_get(), alongside the existing stale-entry and
side-exhaustion checks.
Move last_gen into struct paint_state so that
commit_graph_generation() is called exactly once per dequeued commit
and the result is shared across all termination checks and the
monotonicity BUG assertion.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
commit-reach.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index e174b219c6..5c5c54d66e 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -89,6 +89,8 @@ struct paint_state {
size_t parent1_count;
size_t parent2_count;
size_t mb_candidate_count;
+ timestamp_t min_generation;
+ timestamp_t last_gen;
};
static void paint_count_update(struct paint_state *state,
@@ -138,11 +140,23 @@ static void paint_queue_put(struct paint_state *state,
static struct commit *paint_queue_get(struct paint_state *state)
{
struct commit *commit = prio_queue_get(&state->queue);
+ timestamp_t generation;
if (!commit)
return NULL;
commit->object.flags &= ~ENQUEUED;
+ generation = commit_graph_generation(commit);
+
+ if (state->min_generation && generation > state->last_gen)
+ BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
+ generation, state->last_gen,
+ oid_to_hex(&commit->object.oid));
+ state->last_gen = generation;
+
+ /* generation cutoff */
+ if (generation < state->min_generation)
+ return NULL;
if (!state->mb_candidate_count) {
/* only stale entries remain */
@@ -151,7 +165,7 @@ static struct commit *paint_queue_get(struct paint_state *state)
/* one side is exhausted */
if ((!state->parent1_count || !state->parent2_count) &&
- commit_graph_generation(commit) < GENERATION_NUMBER_INFINITY)
+ generation < GENERATION_NUMBER_INFINITY)
return NULL;
}
@@ -177,9 +191,10 @@ static int paint_down_to_common(struct repository *r,
struct commit *commit;
int i;
int steps = 0;
- timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
struct commit_list **tail = result;
+ state.min_generation = min_generation;
+ state.last_gen = GENERATION_NUMBER_INFINITY;
if (!min_generation && !corrected_commit_dates_enabled(r))
state.queue.compare = compare_commits_by_commit_date;
@@ -196,18 +211,8 @@ static int paint_down_to_common(struct repository *r,
while ((commit = paint_queue_get(&state))) {
struct commit_list *parents;
int flags;
- timestamp_t generation = commit_graph_generation(commit);
steps++;
- if (min_generation && generation > last_gen)
- BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
- generation, last_gen,
- oid_to_hex(&commit->object.oid));
- last_gen = generation;
-
- if (generation < min_generation)
- break;
-
flags = commit->object.flags & (PARENT1 | PARENT2 | STALE);
if (flags == (PARENT1 | PARENT2)) {
if (!(commit->object.flags & RESULT)) {
@@ -219,7 +224,7 @@ static int paint_down_to_common(struct repository *r,
* descendant of this one.
*/
if (!(mb_flags & MERGE_BASE_FIND_ALL) &&
- generation < GENERATION_NUMBER_INFINITY)
+ state.last_gen < GENERATION_NUMBER_INFINITY)
break;
}
/* Mark parents of a found merge stale */
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v4 1/1] environment: move excludes_file into repo_config_values
From: Tian Yuchen @ 2026-06-28 12:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <xmqqbjcv2h3j.fsf@gitster.g>
On 6/28/26 16:40, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
>
>>> Wouldn't we rather want to try to be more strict and say
>>>
>>> if (!repo || !repo->initialized)
>>> BUG("repo must be an initialied repository");
>>>
>>> here? Aren't all the callers of this function supposed to be
>>> dealing with an already initialized repository?
>>
>> That makes sense, but from my point of view...
>>
>> 'repo_config_values()' already has a check for 'repo->initialized'. If
>> we're absolutely certain that the 'repo' is initialized, wouldn't it be
>> better to simply remove all the checks inside the getter and leave the
>> judgment to 'repo_config_values()'?
>
> Yes, that was what I was getting at ;-).
A lot of CI tests are failing, but that just goes to show that the
"bugs" are being properly identified, doesn’t it?
It means there are a lot of "invalid" calls in the tests (if the way we
define a 'valid' call, i.e. repo must be initialized, is correct)... It
seems that code like 'if (repo != the_repository) return' or something
similar is inevitably going to end up somewhere, even though, as you
said, it’s "sweeping problems under the rug."
I’m not sure how to proceed from here either..
Regards, yuchen
^ permalink raw reply
* Re: [PATCH v4 8/8] commit-reach: move min_generation check into paint_queue_get()
From: Derrick Stolee @ 2026-06-28 15:15 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget, git
Cc: Elijah Newren, Kristofer Karlsson
In-Reply-To: <8dd15d44e6a60fc39bbf6d894628507e839f9248.1782649547.git.gitgitgadget@gmail.com>
On 6/28/26 8:25 AM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
...> @@ -138,11 +140,23 @@ static void paint_queue_put(struct paint_state *state,
> static struct commit *paint_queue_get(struct paint_state *state)
> {
> struct commit *commit = prio_queue_get(&state->queue);
> + timestamp_t generation;
>
> if (!commit)
> return NULL;
>
> commit->object.flags &= ~ENQUEUED;
> + generation = commit_graph_generation(commit);
> +
> + if (state->min_generation && generation > state->last_gen)
> + BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
> + generation, state->last_gen,
> + oid_to_hex(&commit->object.oid));
> + state->last_gen = generation;
> +
> + /* generation cutoff */
> + if (generation < state->min_generation)
> + return NULL;
...
> - if (min_generation && generation > last_gen)
> - BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
> - generation, last_gen,
> - oid_to_hex(&commit->object.oid));
> - last_gen = generation;
> -
> - if (generation < min_generation)
> - break;
I'm just stopping in to say that this looks like a clean code move
in this version, without mutating this chunk in the previous patch.
LGTM.
-Stolee
^ permalink raw reply
* Re: [PATCH v4 0/8] commit-reach: terminate merge-base walk when one side is exhausted
From: Derrick Stolee @ 2026-06-28 15:16 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget, git
Cc: Elijah Newren, Kristofer Karlsson
In-Reply-To: <pull.2149.v4.git.1782649547.gitgitgadget@gmail.com>
On 6/28/26 8:25 AM, Kristofer Karlsson via GitGitGadget wrote:
> commit-reach: terminate merge-base walk when one paint side is exhausted
>
> Optimize paint_down_to_common() for merge-base queries that hit large
> one-sided histories.
>
> When the walk from one side reaches a commit with a very low generation
> number that the other side never paints, the walk is forced to drain most of
> the graph. A common trigger is a repository import that grafts a separate
> history with its own root, but any merge that introduces a low-generation
> commit never painted by the other side has the same effect.
> Changes since v3:
>
> * Fixed BUG assertion that was accidentally made unconditional in v3:
> restored the min_generation guard so it only fires when generation-based
> ordering is active.
>
> * Moved generation cutoff and single-result termination conditions into the
> documentation in patch 1/8, since they describe existing behavior.
>
> * Renamed paint_state counter fields for clarity: p1_count ->
> parent1_count, p2_count -> parent2_count, pending_merge_bases ->
> mb_candidate_count. Changed counter types from int to size_t. (Suggested
> by Rene Scharfe.)
I reviewed the v3 discussion, the range-diff, and reread patch 8. I think
that this version is good to go.
Thanks for your hard work!
-Stolee
^ permalink raw reply
* Re: 2.54.0: fyi: endless loop at 100% CPU
From: Steffen Nurpmeso @ 2026-06-28 16:37 UTC (permalink / raw)
To: Michael Montalbo; +Cc: git, Steffen Nurpmeso
In-Reply-To: <CAC2Qwm+v2pRp30TYJpy8Wxzb7gbX+nzybZ_3A99cHb-xjjpCnQ@mail.gmail.com>
Michael Montalbo wrote in
<CAC2Qwm+v2pRp30TYJpy8Wxzb7gbX+nzybZ_3A99cHb-xjjpCnQ@mail.gmail.com>:
|On Sat, Jun 27, 2026 at 1:16 PM Steffen Nurpmeso <steffen@sdaoden.eu> \
|wrote:
|>
|> Thanks for these pointers, i did not know about such configuration
|> variables. I will set them like you show.
|
|No problem! Just to clarify, I'm not sure you should actually use those
|configuration values verbatim. I was more pointing in the direction of
|potentially relevant options for debugging / working around the issue.
We'll see. But if there was some kind of "with love from canada"
misconfiguration -- i have seen quite a bit of those, and
permanent sub-second page reload was one of those effects, in
a browser though .. and git has a little road 'till it gets to
that stage (i hope) -- then maybe these settings .. Or i have to
tweak.
Restartable "fetch" is likely not on that roadmap of git -- that
would (have) be(en) so cool. (But for years i now have
a WireGuard VPN and go through that, which has improved my TCP
connectivity / stability massively. But it is still a thriller to
go for some rate-limited fetch of large size ..)
Oh. Maybe i see.
$ git ls-remote https://gitlab.xiph.org/xiph/opus.git
fatal: unable to access 'https://gitlab.xiph.org/xiph/opus.git/': Operation too slow. Less than 1000 bytes/sec transferred the last 10 seconds
$ git ls-remote https://gitlab.xiph.org/xiph/opus.git
fatal: unable to access 'https://gitlab.xiph.org/xiph/opus.git/': Operation too slow. Less than 500 bytes/sec transferred the last 10 seconds
$ git ls-remote https://gitlab.xiph.org/xiph/opus.git
fatal: unable to access 'https://gitlab.xiph.org/xiph/opus.git/': Operation too slow. Less than 500 bytes/sec transferred the last 21 seconds
It succeeds with
lowSpeedLimit = 500
lowSpeedTime = 33
But at least it does not busy loop:
steffen 2960 2959 0 0.0 2738 2016 S+ 00:00:00 18:26 pts/4 /usr/lib/git-core/git remote-https https://gitlab.xiph.org/xiph/opus.git https://gitlab.xiph.org/xiph/opus.git
steffen 2961 2960 0 0.0 41213 11316 S+ 00:00:00 18:26 pts/4 /usr/lib/git-core/git-remote-https https://gitlab.xiph.org/xiph/opus.git https://gitlab.xiph.org/xiph/opus.git
Unfortunately no info where and why it busy looped. If it would
be non-blocking I/O and .. in this area, one could understand
a bit. Some ticks-per-sec limit (without X progress) is not
configurable? I am not really keen to create a rlimit wrapper
for git, or whatever. (I have limiting cgroups, but still.)
A nice Sunday,
Ciao and greetings from Germany,
--steffen
|
|Der Kragenbaer, The moon bear,
|der holt sich munter he cheerfully and one by one
|einen nach dem anderen runter wa.ks himself off
|(By Robert Gernhardt)
^ permalink raw reply
* Re: [PATCH 3/3] t5551: pack refs after creating many tags
From: Junio C Hamano @ 2026-06-28 21:25 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, Patrick Steinhardt, git
In-Reply-To: <20260628080710.GC107826@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So let's follow that recommendation and pack the refs ourselves.
> Unfortunately, this does not seem to produce an improvement to the
> run-time of the test script! That's because after producing this state,
> we perform only a few fetches of it. And packing the refs costs at least
> as much as serving a ref advertisement (both have to iterate the refs,
> but packing additionally must write .lock files as we pack).
Testing a pathological set-up with too many loose refs may have
extra value, as long as we are also testing the recommended set-up,
so ideally we should have both ;-) but if we have to pick only one
and drop the other, we probably should be testing the packed case.
> I'm iffy on whether this one is worth it.
I am ambivalent, too, about this change for the purpose of the
"yeek, apache times out while enumerating refs" issue. But see
above ;-)
^ permalink raw reply
* Re: [PATCH 2/3] t5551: put many-tags case into its own repo
From: Junio C Hamano @ 2026-06-28 21:44 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, Patrick Steinhardt, git
In-Reply-To: <20260628080345.GB107826@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index e236e526f0..cd851f24b8 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -397,15 +397,16 @@ create_tags () {
> }
>
> test_expect_success 'create 2,000 tags in the repo' '
> + git init "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
> (
> - cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> + cd "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
> create_tags 1 2000
> )
> '
While all the other repositories used in this tests are bare
repositories, this new one is a non-bare repository.
It shouldn't make any difference, but since I noticed it...
^ permalink raw reply
* Re: [PATCH] http: accept https:// proxies again
From: Junio C Hamano @ 2026-06-28 22:27 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Aliwoto, Johannes Schindelin
In-Reply-To: <xmqqjyrj2qsp.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> From this function nothing returns an error anymore, and looking at
>> the preimage of 663d7abe (http: reject unsupported proxy URL
>> schemes, 2026-05-05) that is the source of the bug, the original did
>> not do anything when the corresponding code did not find and set any
>> proxy settings, either.
>>
>> So perhaps it is a better fix to make it just a function that
>> returns void with early returns?
>
> Nah, I was being stupid. Disregard the above.
>
> The whole point of 663d7abe was that we wanted to reject what we did
> not recognise, and we cannot do so without returning "good/bad" from
> that function. The bug was that we did recognise https:// but still
> returned -1 because of the bug, which the patch in the thread fixed.
And as an important bugfix, this patch of course has been
fast-tracked. I'll make sure we have it in 'master' before Git 2.55
gets tagged.
Thanks.
^ permalink raw reply
* Re: [PATCH GSoC v14 09/13] serve: advertise object-info feature
From: Karthik Nayak @ 2026-06-28 22:55 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, chandrapratap3519, chriscool, eric.peijian, gitster,
jltobler, peff, toon, Calvin Wan, Jonathan Tan
In-Reply-To: <CAN5EUNTrdNArd5SX9df6x9bOhRfzE4c7dLOuNu7ONUdn4TLsUA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3367 bytes --]
Pablo Sabater <pabloosabaterr@gmail.com> writes:
> El sáb, 27 jun 2026 a las 0:23, Karthik Nayak
> (<karthik.188@gmail.com>) escribió:
>>
>> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>>
>> > From: Calvin Wan <calvinwan@google.com>
>> >
>> > In order for a client to know what object-info components a server can
>> > provide, advertise supported object-info features. This will allow a
>> > client to decide whether to query the server for object-info or fetch
>> > as a fallback.
>> >
>> > Helped-by: Jonathan Tan <jonathantanmy@google.com>
>> > Helped-by: Christian Couder <chriscool@tuxfamily.org>
>> > Signed-off-by: Calvin Wan <calvinwan@google.com>
>> > Signed-off-by: Eric Ju <eric.peijian@gmail.com>
>> > Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
>> > ---
>> > serve.c | 5 ++++-
>> > 1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/serve.c b/serve.c
>> > index 49a6e39b1d..2b07d922b3 100644
>> > --- a/serve.c
>> > +++ b/serve.c
>> > @@ -89,7 +89,7 @@ static void session_id_receive(struct repository *r UNUSED,
>> > trace2_data_string("transfer", NULL, "client-sid", client_sid);
>> > }
>> >
>> > -static int object_info_advertise(struct repository *r, struct strbuf *value UNUSED)
>> > +static int object_info_advertise(struct repository *r, struct strbuf *value)
>> > {
>> > if (advertise_object_info == -1 &&
>> > repo_config_get_bool(r, "transfer.advertiseobjectinfo",
>> > @@ -97,6 +97,9 @@ static int object_info_advertise(struct repository *r, struct strbuf *value UNUS
>> > /* disabled by default */
>> > advertise_object_info = 0;
>> > }
>> > + /* Currently only size is supported */
>> > + if (value && advertise_object_info)
>> > + strbuf_addstr(value, "size");
>>
>> So is the plan that further options will be added here to value? If so,
>> whats the format we will follow?
>
> Hi!
> The current documented format is at `gitprotocol-v2.adoc`, however I
> think it could be improved. I have a more complete version in the
> not-yet-sent %(objecttype) support series, but since the question
> comes up here, I will update the format documentation in this series
> for size only:
>
Ah nice, I didn't see that.
> oid <oid>
> Indicates to the server an object which the client wants to obtain
> - information for.
> + information for. They must be full object IDs.
>
> - info = PKT-LINE(attrs) LF)
> + info = PKT-LINE(attrs LF)
> *PKT-LINE(obj-info LF)
>
> attrs = attr | attrs SP attrs
>
> + obj-size = 1*DIGIT
> +
> attr = "size"
>
> - obj-info = obj-id SP obj-size
> + obj-info = obj-id SP [obj-size]
> +
> +If the server does not recognize the object id, the response will be
> +`obj-id SP` regardless of the number of attributes requested.
>
> About the names `size` and future ones `type` they are arbitrarily
> chosen, so for example: `delta:base` could be `delta`. They are
> appended to the buffer so in case of adding `type`, it would look
> like:
>
> strbuf_addstr(value, "size type");
>
> What do you think?
>
I didn't know this part was already documented, so its all good :)
> Thanks for the review,
> Pablo.
>
>>
>> > return advertise_object_info;
>> > }
>> >
>> >
>> > --
>> > 2.54.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH] meson: wire up USE_NSEC build knob
From: brian m. carlson @ 2026-06-29 0:23 UTC (permalink / raw)
To: Jeff King
Cc: Patrick Steinhardt, D. Ben Knoble, git, Junio C Hamano,
Ramsay Jones
In-Reply-To: <20260628084815.GA111587@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]
On 2026-06-28 at 08:48:15, Jeff King wrote:
> Oh, I also ran across this old thread:
>
> https://public-inbox.org/git/5605D88A.20104%40gmail.com/
>
> that implies similar:
>
> * In-core file times may not be properly rounded to on-disk
> precision, causing spurious file time changes when the cache is
> refreshed from disk. This was fixed for typical Unix file systems
> in kernel 2.6.11. The fix for CEPH, CIFS, NTFS, UFS and FUSE will
> be in kernel 4.3. There's no fix for FAT-based file systems yet.
>
> I also tested with CIFS on my system and it is fine. It looks like FAT
> systems were fixed since 2015. ;)
>
> But there is another interesting question raised there, which is how
> different implementations may interact (e.g., two versions of Git
> without and without USE_NSEC, or JGit which may have to use
> millisecond-resolution APIs, etc). It should all work correctly as long
> as each implementation consistently uses its own resolution (so JGit
> would have to compare in millisecond-space and treat ties as racy). And
> I think that is _probably_ what is happening now, since we already store
> nanoseconds unconditionally (and only use them with USE_NSEC).
>
> Though the opposite case is a performance problem but not a correctness
> one: if JGit writes out an index with milliseconds and USE_NSEC Git
> tries to read it, we will consider everything stat-dirty and re-read the
> contents.
>
> I don't know if these would be a problem in practice or not, but it's an
> interesting potential gotcha. And one that nobody may have noticed,
> because probably hardly anybody bothers to build with USE_NSEC now.
I would suggest that we provide a config knob and then build with
USE_NSEC by default. Most people are using Linux with typical Unix file
systems, NTFS, CIFS, or FUSE (e.g., sshfs). In the event someone
detects a problem, there's an easy solution—adjust the knob—and we can
then add a Linux-specific statfs call to determine if the file system is
a safe one in a future version.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] t5551: put many-tags case into its own repo
From: Jeff King @ 2026-06-29 0:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Montalbo, Patrick Steinhardt, git
In-Reply-To: <xmqqh5mm1gsf.fsf@gitster.g>
On Sun, Jun 28, 2026 at 02:44:32PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> > index e236e526f0..cd851f24b8 100755
> > --- a/t/t5551-http-fetch-smart.sh
> > +++ b/t/t5551-http-fetch-smart.sh
> > @@ -397,15 +397,16 @@ create_tags () {
> > }
> >
> > test_expect_success 'create 2,000 tags in the repo' '
> > + git init "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
> > (
> > - cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> > + cd "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
> > create_tags 1 2000
> > )
> > '
>
> While all the other repositories used in this tests are bare
> repositories, this new one is a non-bare repository.
>
> It shouldn't make any difference, but since I noticed it...
Ah, yeah. It should work either way, but it is slightly confusing for it
to be non-bare. I'll wait to re-send (though if nothing else comes up,
it may be simpler for you to just amend on your side).
-Peff
^ permalink raw reply
* Re: [PATCH v4 6/8] commit-reach: remove unused nonstale_queue dedup wrappers
From: SZEDER Gábor @ 2026-06-29 5:25 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget
Cc: git, Derrick Stolee, Elijah Newren, Kristofer Karlsson
In-Reply-To: <4db485b48aae810eeba28ea4feb47401ab352e88.1782649547.git.gitgitgadget@gmail.com>
On Sun, Jun 28, 2026 at 12:25:44PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> nonstale_queue_put_dedup() and nonstale_queue_get_dedup() became
> unused after the previous commit. The core nonstale_queue functions
> remain in use by ahead_behind().
Please squash this patch into the previous one. Since the last
callers of these static functions went away in that commit, it can't
be built with DEVELOPER=1:
commit-reach.c:91:23: warning: ‘nonstale_queue_get_dedup’ defined but not used [-Wunused-function]
91 | static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue)
| ^~~~~~~~~~~~~~~~~~~~~~~~
commit-reach.c:82:13: warning: ‘nonstale_queue_put_dedup’ defined but not used [-Wunused-function]
82 | static void nonstale_queue_put_dedup(struct nonstale_queue *queue,
| ^~~~~~~~~~~~~~~~~~~~~~~~
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> commit-reach.c | 18 ------------------
> 1 file changed, 18 deletions(-)
>
> diff --git a/commit-reach.c b/commit-reach.c
> index 9ae306f60c..176ffd68d0 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -79,24 +79,6 @@ static void clear_nonstale_queue(struct nonstale_queue *queue)
> queue->max_nonstale = NULL;
> }
>
> -static void nonstale_queue_put_dedup(struct nonstale_queue *queue,
> - struct commit *c)
> -{
> - if (c->object.flags & ENQUEUED)
> - return;
> - c->object.flags |= ENQUEUED;
> - nonstale_queue_put(queue, c);
> -}
> -
> -static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue)
> -{
> - struct commit *commit = nonstale_queue_get(queue);
> -
> - if (commit)
> - commit->object.flags &= ~ENQUEUED;
> - return commit;
> -}
> -
> /*
> * Priority queue with per-side commit counters for paint_down_to_common().
> * Each non-stale queued commit occupies exactly one bucket: PARENT1-only,
> --
> gitgitgadget
>
^ permalink raw reply
* Re: [PATCH v6 4/4] history: re-edit a squash with every message
From: Junio C Hamano @ 2026-06-29 5:50 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Harald Nordgren
In-Reply-To: <4edf012b77fd2f2fb2a51eb10863bbf852fffa40.1782635349.git.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> By default "git history squash" reuses the oldest commit's message.
> When --reedit-message is given it only reopened that one message, so the
> messages of the folded-in commits were lost.
>
> Gather the messages of every commit in the range, oldest first, and use
> them as the editor template when re-editing, mirroring how "git rebase
> -i" presents a squash.
I doubt it would make practical difference, but one thing I notice
is that unlike "git rebase -i", this one does not intersperse
markers like "# This is the 1st commit message" in between the
messages taken from the squashed commits, so it is not exactly
"mirroring".
^ permalink raw reply
* Re: [PATCH v6 3/4] history: add squash subcommand to fold a range
From: Junio C Hamano @ 2026-06-29 5:50 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Harald Nordgren
In-Reply-To: <811e393ab48e2f79e6f8c78d883a5b92311791b3.1782635349.git.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -11,6 +11,7 @@ SYNOPSIS
> git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)]
> git history reword <commit> [--dry-run] [--update-refs=(branches|head)]
> git history split <commit> [--dry-run] [--update-refs=(branches|head)] [--] [<pathspec>...]
> +git history squash <revision-range> [--dry-run] [--update-refs=(branches|head)] [--reedit-message]
Not your fault at all, as there are existing violators but the
gitcli tells us that the canonical order of arguments on the command
line is to have dashed options early before any real arguments.
Seeing <commit> followed by --dry-run, --update-refs, etc. with the
existing subcommands should also be corrected and this new command
should not spread the existing violation by mimicking. The revision
range (which is not just a single token A..B, but the usual range
notation that rev-list takes, like ^A B..C) should come after all
these dashed options.
> +The range is given in the usual `<base>..<tip>` form, where _<base>_ is
> +the commit just below the oldest commit to squash. For example, `git
> +history squash @~3..` folds the three most recent commits into one, and
It is OK to use in your personal development, but in the official
documentation, avoid cryptic @ and always spell HEAD (except for a
single place that explain that @ can stand in for HEAD, for obvious
reasons). It is extremely annoying and confusing to read as these
look so similar to reflog notation for the current branch, e.g., @~1
vs @{1}.
So "squash HEAD~3..HEAD" specifies a range of commits HEAD~2, HEAD~1
and HEAD (three commits), these are squashed into one commit on top
of HEAD~3, creating a sibling to HEAD~2 whose tree matches HEAD.
OK. Looking good.
^ permalink raw reply
* Re: [PATCH 3/3] t5551: pack refs after creating many tags
From: Patrick Steinhardt @ 2026-06-29 5:57 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, git, Junio C Hamano
In-Reply-To: <20260628080710.GC107826@coredump.intra.peff.net>
On Sun, Jun 28, 2026 at 04:07:10AM -0400, Jeff King wrote:
> We have two tests that create 2,000 and 100,000 tags respectively.
> After doing so, the resulting state can be a bit slow to work with when
> using the "files" ref backend, as each of those refs is in its own file.
>
> This isn't a very realistic scenario, as we'd expect most of those refs
> to be packed. If they accrue over time along with objects, they'd get
> packed by maintenance/gc runs. And if you have a process that creates a
> ton of refs at once (like a big fast-import), the usual recommendation
> is to run maintenance afterwards.
>
> So let's follow that recommendation and pack the refs ourselves.
> Unfortunately, this does not seem to produce an improvement to the
> run-time of the test script! That's because after producing this state,
> we perform only a few fetches of it. And packing the refs costs at least
> as much as serving a ref advertisement (both have to iterate the refs,
> but packing additionally must write .lock files as we pack).
> My wall-clock time was slightly improved (but within the noise) with
> this patch, but my user and system CPU time were slightly worse!
> However, on a loaded system with I/O bottlenecks, it may be a net win.
> That's somewhat of a guess, though.
>
> It would be nice if we had a way to generate all of these refs without
> writing so many individual files. But even if we taught the ref code to
> write large cases directly to the packed-refs file, we'd still need to
> take individual locks. The real solution is a backend like reftable,
> which shaves ~30% off of the test runtime.
We kind of already have this with the `REF_TRANSACTION_FLAG_INITIAL`
flag, but right now it is only used when performing a clone or when
migrating references. Also, it requires an empty repository that has no
references yet.
It raises the question whether we could also extend git-fast-import(1)
to use it, as it would typically be run on an almost-empty repository.
It's the "almost" that kills it though, as we already do have at least
the HEAD reference. So it could be feasible, but it's not as trivial as
just setting the flag and then we're magically faster.
And besides, in this particular test here we run git-fast-import(1)
multiple times in the same repository, so it wouldn't help us.
We could of course extend all of this so that Git is able to write into
the packed-refs directly, even with preexisting refs. But I agree with
your sentiment: it doesn't feel worth it as the reftable backend fixes
scenarios like this anyway.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I'm iffy on whether this one is worth it.
>
> If you apply just this patch without patch 2, then the run-time does
> improve quite a bit. The cost of packing is amortized by the improved
> performance for all of those subsequent tests (but after patch 2, they
> never even see the unpacked state).
>
> Likewise, I suspect this would make our timeout problems go away even
> without patch 1.
>
> So the whole series _could_ be reduced to just this one patch. But
> hopefully the reasoning given in the earlier patches makes sense, at
> which point this one is kind of superfluous.
Agreed. I'd just merge the first two patches and drop this one here.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v4 1/1] environment: move excludes_file into repo_config_values
From: Christian Couder @ 2026-06-29 6:03 UTC (permalink / raw)
To: Tian Yuchen
Cc: Junio C Hamano, git, cirnovskyv, szeder.dev, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <18ad7c1c-5ddc-4f62-ba7c-5cda53f5a48d@malon.dev>
On Sun, Jun 28, 2026 at 2:58 PM Tian Yuchen <cat@malon.dev> wrote:
>
> On 6/28/26 16:40, Junio C Hamano wrote:
> > Tian Yuchen <cat@malon.dev> writes:
> >
> >>> Wouldn't we rather want to try to be more strict and say
> >>>
> >>> if (!repo || !repo->initialized)
> >>> BUG("repo must be an initialied repository");
> >>>
> >>> here? Aren't all the callers of this function supposed to be
> >>> dealing with an already initialized repository?
> >>
> >> That makes sense, but from my point of view...
> >>
> >> 'repo_config_values()' already has a check for 'repo->initialized'. If
> >> we're absolutely certain that the 'repo' is initialized, wouldn't it be
> >> better to simply remove all the checks inside the getter and leave the
> >> judgment to 'repo_config_values()'?
> >
> > Yes, that was what I was getting at ;-).
>
> A lot of CI tests are failing, but that just goes to show that the
> "bugs" are being properly identified, doesn’t it?
>
> It means there are a lot of "invalid" calls in the tests (if the way we
> define a 'valid' call, i.e. repo must be initialized, is correct)... It
> seems that code like 'if (repo != the_repository) return' or something
> similar is inevitably going to end up somewhere, even though, as you
> said, it’s "sweeping problems under the rug."
>
> I’m not sure how to proceed from here either..
I agree that the best end state would be to have no `if (!repo ||
!repo->initialized)` check, but we shouldn't have to get there right
away. I think it's fine to proceed in several steps:
1) `if (!repo || !repo->initialized) return NULL;` allows us to remove
the global variable and use getters which will help us in the next
step.
2) `if (!repo || !repo->initialized) return BUG("repo must be an
initialized repository");` now we want to find and fix callers
(including tests) that haven't properly initialized things.
3) No `if (!repo || !repo->initialized)` check, as we are sure that
all the callers that didn't properly initialized things have been
found and fixed.
So I think 1) is fine for now as long as we properly explain in the
commit messages and in code comments (maybe using NEEDSWORK comments)
that we know there is more work to do on this in the future.
^ permalink raw reply
* Re: [PATCH] meson: wire up USE_NSEC build knob
From: Patrick Steinhardt @ 2026-06-29 6:08 UTC (permalink / raw)
To: Jeff King
Cc: D. Ben Knoble, git, brian m . carlson, Junio C Hamano,
Ramsay Jones
In-Reply-To: <20260628081806.GA3594700@coredump.intra.peff.net>
On Sun, Jun 28, 2026 at 04:18:06AM -0400, Jeff King wrote:
> On Mon, Jun 22, 2026 at 10:13:21AM +0200, Patrick Steinhardt wrote:
>
> > > So I guess if we wanted to go further it would take some digging as to
> > > how each platform behaves, and then flipping the config.make.uname knob
> > > for ones where it can be argued that the behavior is always reasonable.
> >
> > Yeah, it would be nice indeed to figure out whether these concerns still
> > apply. If they do, I would argue that it might even make sense to remove
> > the build option completely. It doesn't really make sense in my opinion
> > to have a build option that nobody uses and that is subtly broken when
> > enabled.
>
> I suspect it works just fine on some platforms and some filesystems
> (i.e., those that actually store nanoseconds on disk). So probably Linux
> with ext4 is OK. That's just guessing, though.
>
> If I understand the original problem correctly, then doing this:
>
> touch foo
> ls --full-time foo
> echo 3 | sudo tee /proc/sys/vm/drop_caches
> ls --full-time foo
>
> should be instructive. If it shows the same time for both "ls" calls,
> then USE_NSEC would be fine. If it doesn't, then the system is losing
> the nanosecond information when it drops the cache and has to reload
> from disk (and thus USE_NSEC would cause spurious stat mismatches).
>
> On my ext4 system, I get the same answers. So far so good.
>
> I get the same answers with a loopback-mounted ext2 system. Which
> surprised me a bit, but even unmounting and remounting the filesystem,
> the nanosecond times are still there. So...I guess ext2 supports
> nanoseconds.
>
> I tried with a vfat mount, and it also works: we don't have nanoseconds
> either before or after. That makes sense, and implies that modern Linux
> will always be OK (because it limits the cached VFS response to what the
> underlying filesystem can handle).
>
> So...maybe this is just a non-issue these days, at least on Linux?
>
> > > But that's all outside the scope of your patch here.
> >
> > Kind of, I guess. If we figure that this mechanism is still subtly broken
> > then I'd argue that it doesn't make sense to expose the option via
> > Meson.
>
> True, but AFAICT it probably is safe these days, at least one some
> platforms.
Hm. That makes me wonder whether it is the completely wrong approach to
make this a build option then. If it works on some systems and only on
some filesystems, then a build option is just too coarse-grained. A
distro wouldn't really be able to ever enable the option, unless it knew
that repositories will only ever exist on a filesystem that works. Which
I guess is an assumption that no distro can make.
So instead, I wonder whether we should treat this the same as for
example "core.ignoreCase", where we only use nanosecond resolution when
opted in by the user. Ideally, if we had a way to detect brokenness, we
could even make git-init(1) set it automatically.
If so, we could unconditionally enable nanoseconds on platforms that
support them, but still have a runtime toggle for filesystems that
don't.
Patrick
^ permalink raw reply
* Re: [PATCH 1/2] odb/source: generalize `reprepare()` callback
From: Patrick Steinhardt @ 2026-06-29 6:16 UTC (permalink / raw)
To: Toon Claes; +Cc: git
In-Reply-To: <87ldc14i4n.fsf@emacs.iotcl.com>
On Fri, Jun 26, 2026 at 02:10:32PM +0200, Toon Claes wrote:
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 8080d1bf5e..7361bf071e 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -1361,10 +1360,8 @@ int cmd_grep(int argc,
> > struct odb_source *source;
> >
> > odb_prepare_alternates(the_repository->objects);
> > - for (source = the_repository->objects->sources; source; source = source->next) {
> > - struct odb_source_files *files = odb_source_files_downcast(source);
>
> So you're downcasting inside the implementation by the backends itself.
> That makes sense, but would it be worth to say something about that in
> the commit message?
Hm. Would that provide much value? I'm probably quite a bit biased here,
but I think that it's implicit that the backends have to eventually cast
the generic structure to their own backend.
So I wouldn't really know how to clarify this. Did you have anything
specific in mind?
> > diff --git a/odb/source-packed.c b/odb/source-packed.c
> > index 42c28fba0e..fa5a072488 100644
> > --- a/odb/source-packed.c
> > +++ b/odb/source-packed.c
> > @@ -15,7 +15,7 @@ static int find_pack_entry(struct odb_source_packed *store,
> > {
> > struct packfile_list_entry *l;
> >
> > - odb_source_packed_prepare(store);
> > + odb_source_prepare(&store->base, 0);
>
> Why are you not using ODB_PREPARE_FLUSH_CACHES here? It used to do
> before?
Because this was calling `odb_source_packed_prepare()` before, not
`odb_source_reprepare()`. So this was calling the non-flushing variant.
> > if (store->midx && fill_midx_entry(store->midx, oid, e))
> > return 1;
> >
> > @@ -47,7 +47,7 @@ static int odb_source_packed_read_object_info(struct odb_source *source,
> > * been added since the last time we have prepared the packfile store.
> > */
> > if (flags & OBJECT_INFO_SECOND_READ)
> > - odb_source_reprepare(source);
> > + odb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);
>
> I think the new code is correct, but why wasn't `packed` used here in
> the past? The old odb_source_reprepare() expected a downcasted, didn't
> it?
No, `odb_source_reprepare()` is the generic variant. The naming schema
is typically:
- `odb_source_frobnicate()` for the generic variants, which receive a
`struct odb_source` as input.
- `odb_source_<type>_frobnitcate()` for their backend-specific
implementations, which cast down the generic `struct odb_source` to
their backend-specific struct.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH 2/2] odb: introduce `odb_prepare()`
From: Patrick Steinhardt @ 2026-06-29 6:16 UTC (permalink / raw)
To: Toon Claes; +Cc: git
In-Reply-To: <87o6gx4i5w.fsf@emacs.iotcl.com>
On Fri, Jun 26, 2026 at 02:09:47PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/odb.h b/odb.h
> > index c14c9030e4..b1c0f3767b 100644
> > --- a/odb.h
> > +++ b/odb.h
> > @@ -133,9 +133,13 @@ enum odb_prepare_flags {
> > };
> >
> > /*
> > - * Clear caches, reload alternates and then reload object sources so that new
> > - * objects may become accessible.
> > + * Prepare the object database for use. Calling this function is generally not
> > + * needed, but can be useful in case the caller wants to pre-open individual
> > + * sources.
> > */
> > +void odb_prepare(struct object_database *o, enum odb_prepare_flags flags);
> > +
> > +/* Equivalent to `odb_prepare(o, ODB_PREPARE_FLUSH_CACHES)`. */
> > void odb_reprepare(struct object_database *o);
>
> According to my grep results are there 17 callsites for odb_reprepare(),
> then I agree it makes sense to create this wrapper.
Yeah, I was a bit torn myself whether or not to keep the wrapper. I
eventually decided to just keep it because it reduces churn, and it's a
trivial wrapper anyway.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH] reftable: fix unlikely leak on API error
From: Patrick Steinhardt @ 2026-06-29 6:21 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20260628090314.GA661068@coredump.intra.peff.net>
On Sun, Jun 28, 2026 at 05:03:14AM -0400, Jeff King wrote:
> If the reftable writer sees a bogus block size, we return with
> REFTABLE_API_ERROR, leaking the reftable_writer struct we previously
> allocated. Originally this case was a BUG(), but it became a regular
> return in 445f9f4f35 (reftable: stop using `BUG()` in trivial cases,
> 2025-02-18).
>
> We could obviously fix it by calling "reftable_free(wp)". But we can
> observe that we never use the allocated "wp" until after we've validated
> the input options. So let's just bump the allocation down. That fixes
> the leak, and I think makes the flow of the function more logical
> (we validate our inputs before doing any work).
Another alternative would be to create a common exit path where we free
the structure when we're about to return an error. But that might not
even be worth it.
> diff --git a/reftable/writer.c b/reftable/writer.c
> index 0133b64975..1bd4aa388b 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -152,16 +152,16 @@ int reftable_writer_new(struct reftable_writer **out,
> struct reftable_write_options opts = {0};
> struct reftable_writer *wp;
>
> - wp = reftable_calloc(1, sizeof(*wp));
> - if (!wp)
> - return REFTABLE_OUT_OF_MEMORY_ERROR;
> -
> if (_opts)
> opts = *_opts;
> options_set_defaults(&opts);
> if (opts.block_size >= (1 << 24))
> return REFTABLE_API_ERROR;
>
> + wp = reftable_calloc(1, sizeof(*wp));
> + if (!wp)
> + return REFTABLE_OUT_OF_MEMORY_ERROR;
> +
> reftable_buf_init(&wp->block_writer_data.last_key);
> reftable_buf_init(&wp->last_key);
> reftable_buf_init(&wp->scratch);
Makes sense. There's another early return in this function, but there we
already know to free the writer.
Thanks!
Patrick
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox