* [PATCH v4 2/2] graph: indent visual root in graph
From: Pablo Sabater @ 2026-06-12 13:48 UTC (permalink / raw)
To: git
Cc: ayu.chandekar, chandrapratap3519, christian.couder, gitster,
jltobler, karthik.188, peff, phillip.wood, siddharthasthana31,
Pablo Sabater
In-Reply-To: <20260612-ps-pre-commit-indent-v4-0-e8492037ebae@gmail.com>
When rendering a graph, if the history contains multiple "visual roots",
actual roots or commits that look like roots (i.e. have their parents
filtered out) can end up being vertically adjacent to unrelated commits,
falsely appearing to be related.
A fix for this issue was already attempted [1] a while ago.
This happens because the commits fill the space from left to right and
when a visual root ends, its column becomes free for the following
commit even if they are not related. Once this happens the unrelated
commit is rendered below the visual root. Because there is no special
character or way to identify when a visual root is rendered making the
graph confusing.
By indenting the visual roots when there are still commits to show the
vertical adjacency can be avoided.
Add is_visual_root flag to git_graph making it visible in all graph states,
give graph_update() a new function, graph_is_visual_root() to know if the
current commit is a visual root and set is_visual_root.
The different handled cases are:
- If a visual root has children: similar to GRAPH_PRE_COMMIT state when
octopus merges need space, an edge row needs to be printed to connect
the child with the indented visual root. A new state GRAPH_PRE_ROOT is
needed to connect the child with the visual root:
* child of the visual root
\ GRAPH_PRE_ROOT
* visual root indented
- If a visual root is child-less we can skip GRAPH_PRE_ROOT state and
render the indented commit directly.
* visual root indented
* unrelated commit
- If two or more visual roots are adjacent: by having a lookahead to the
next commit that will be rendered, if the next commit is also a visual
root and we are on a visual root, meaning two visual root adjacent in
the history, the top one can omit the indent, making the one below to
indent only once, if there are more adjacent visual commits, the
indentation will increase for each adjacent one, cascading.
* visual root
* visual root
* visual root
* last commit
Even if the last commit is a root, because there is nothing that will be
rendered below we can omit the indentation on purpose.
The lookahead is not completely reliable, on graphs with filtered parents,
the walker when processing the current commit it will simplify its
parents by removing the ones that won't be shown, (They have the
TREESAME flag when filtering by path for example), but it doesn't act
for the grandparents or the next commit if it is unrelated until we move
to the next.
For example given
A visual root
B child
C parent of B, visual root FILTERED
D last commit
We would expect
A
B
D
When processing A, for the walker and the information at the renderer, B
is still a child of C, as B parent, hasn't been removed yet. This makes
cascade to not trigger as the lookahead fails to detect if the next
commit will be a visual root.
Once at B, its parent has been removed and has become a visual root, and
it just adds its indent to the one left by A. We end up with an extra
indent:
A
B
D
The output isn't broken as unrelated commits are successfully separated
by indentation, but an indent level should have been avoided.
Create a new test file for graph indentations test called
't4218-log-graph-indentation.sh'.
The filtered parents edge case is documented as a NEEDSWORK on the
lookahead function and it has its own 'test_expect_failure' at 't4218'.
[1]: https://lore.kernel.org/git/xmqqwnwajbuj.fsf@gitster.c.googlers.com/
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
graph.c | 262 ++++++++++++++++++++++
t/meson.build | 1 +
t/t4218-log-graph-indentation.sh | 455 +++++++++++++++++++++++++++++++++++++++
3 files changed, 718 insertions(+)
diff --git a/graph.c b/graph.c
index 842282685f..e0d1e2a510 100644
--- a/graph.c
+++ b/graph.c
@@ -60,12 +60,23 @@ struct column {
* index into column_colors.
*/
unsigned short color;
+ /*
+ * Marks if a commit is a non-first parent of a merge. These columns are
+ * already visually connected to the merge commit and do not need
+ * indentation.
+ *
+ * The first parent is the one that inherits the column and it can need
+ * indentation if turns out to be a visual root and there's still
+ * commits to render.
+ */
+ unsigned is_merge_parent:1;
};
enum graph_state {
GRAPH_PADDING,
GRAPH_SKIP,
GRAPH_PRE_COMMIT,
+ GRAPH_PRE_ROOT,
GRAPH_COMMIT,
GRAPH_POST_MERGE,
GRAPH_COLLAPSING
@@ -315,6 +326,48 @@ struct git_graph {
* diff_output_prefix_callback().
*/
struct strbuf prefix_buf;
+
+ /*
+ * If a commit is a visual root, we need to indent it to prevent
+ * unrelated commits from being vertically adjacent to it.
+ */
+ unsigned is_visual_root:1;
+
+ /*
+ * Indentation increases for each visual root adjacent to another visual
+ * root, making visual root commits indentation cascade.
+ */
+ unsigned int visual_root_depth;
+
+ /*
+ * When a visual root is adjacent to other visual roots, the first one
+ * can avoid indentation and the rest cascades, increasing the indentation
+ * for each one.
+ */
+ unsigned visual_root_cascade:1;
+
+ /*
+ * Set when the current commit was already present in graph->columns
+ * before being processed.
+ */
+ unsigned commit_in_columns:1;
+};
+
+struct graph_lookahead_flags {
+ /*
+ * Set when there will be a commit after the current one that will be
+ * rendered.
+ */
+ unsigned int is_next_visible:1;
+ /*
+ * Set when the next visible commit is candidate to be a visual root.
+ */
+ unsigned int is_next_visual_root:1;
+ /*
+ * Set when the next visible commit will be rendered under the current
+ * commit.
+ */
+ unsigned int next_has_column:1;
};
static inline int graph_needs_truncation(struct git_graph *graph, int lane)
@@ -388,6 +441,8 @@ struct git_graph *graph_init(struct rev_info *opt)
graph->num_columns = 0;
graph->num_new_columns = 0;
graph->mapping_size = 0;
+ graph->visual_root_depth = 0;
+ graph->visual_root_cascade = 0;
/*
* Start the column color at the maximum value, since we'll
* always increment it for the first commit we output.
@@ -561,6 +616,11 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
struct commit *commit,
int idx)
{
+ /*
+ * Get the initial merge_layout before it's modified to know if this
+ * is a merge.
+ */
+ int initial_merge_layout = graph->merge_layout;
int i = graph_find_new_column_by_commit(graph, commit);
int mapping_idx;
@@ -572,6 +632,7 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
i = graph->num_new_columns++;
graph->new_columns[i].commit = commit;
graph->new_columns[i].color = graph_find_commit_color(graph, commit);
+ graph->new_columns[i].is_merge_parent = 0;
}
if (graph->num_parents > 1 && idx > -1 && graph->merge_layout == -1) {
@@ -610,6 +671,12 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
}
graph->mapping[mapping_idx] = i;
+
+ /*
+ * Mark non-first parents of a merge.
+ */
+ if (graph->num_parents > 1 && initial_merge_layout >= 0 && idx > -1)
+ graph->new_columns[i].is_merge_parent = 1;
}
static void graph_update_columns(struct git_graph *graph)
@@ -701,10 +768,20 @@ static void graph_update_columns(struct git_graph *graph)
if (graph->num_parents == 0)
graph->width += 2;
} else {
+ int j;
graph_insert_into_new_columns(graph, col_commit, -1);
+ /*
+ * This column is not the current commit, but we need to
+ * propagate the flag until the commit is processed.
+ */
+ j = graph_find_new_column_by_commit(graph, col_commit);
+ if (j >= 0 && graph->columns[i].is_merge_parent)
+ graph->new_columns[j].is_merge_parent = 1;
}
}
+ graph->commit_in_columns = is_commit_in_columns;
+
/*
* If graph_max_lanes is set, cap the width
*/
@@ -763,9 +840,135 @@ static int graph_needs_pre_commit_line(struct git_graph *graph)
graph->expansion_row < graph_num_expansion_rows(graph);
}
+/*
+ * A commit can be a visual root when:
+ * - It has no parents.
+ *
+ * - It has parents but they are all filtered out and
+ * commit->parents arrives NULL.
+ *
+ * - It is not a boundary commit. Boundary commits also have no visible
+ * parents, but they are not selected as visual roots because they cannot
+ *. cause the ambiguity of being vertically adjacent because:
+ *
+ * 1. A boundary only appears because an included commit is its child.
+ * Children are always above, and the renderer draws an edge down to
+ * the boundary from that child. Rather than starting a column like a
+ * visual root would do, it inherits its child column.
+ *
+ * 2. Included commits cannot appear below a boundary. Boundaries are
+ * ancestors of the exclusion point; if an included commit were an
+ * ancestor of the boundary it would be excluded and not rendered.
+ * Boundaries therefore always sink to the bottom.
+ */
+static int graph_is_visual_root_candidate(struct commit *c)
+{
+ return c->parents == NULL && !(c->object.flags & BOUNDARY);
+}
+
+static int graph_is_visual_root(struct git_graph *graph,
+ struct graph_lookahead_flags *flags)
+{
+ /*
+ * This must be only called for the current commit as graph contains
+ * the state for the current commit only.
+ *
+ * To check if a commit is a visual root, call graph_is_visual_root_candidate()
+ * but we won't know if it is really a visual root until we get to the
+ * next commit state.
+ *
+ * The current commit is an actual visual root if it is a candidate and
+ * the commit is not a non-first parent of a merge.
+ *
+ * *
+ * |\
+ * | * <- it is a visual root candidate but it shouldn't be indented
+ * * because it is already connected by an edge.
+ * ^ if commit_in_columns && is_merge_parent means the commit
+ * | was put by a merge and is connected.
+ * |
+ * `-------- if !is_next_visible means we're on the last commit, avoid
+ * indentation unless the one before is a visual root, then
+ * we need to differentiate from the one above.
+ *
+ * If next_has_columns means that the next commit has
+ * already a column, so it will not be rendered below, the
+ * current commit has to act as the last commit and omit
+ * indentation.
+ */
+ return graph_is_visual_root_candidate(graph->commit) &&
+ !(graph->commit_in_columns &&
+ graph->columns[graph->commit_index].is_merge_parent) &&
+ flags->is_next_visible &&
+ (!flags->next_has_column || graph->visual_root_depth > 0);
+}
+
+/*
+ * Iterates the commits queue searching for the next visible commit, once found
+ * sets visibleness and visual-root flags.
+ * Knowing if the next commit is also a visual root avoids redundant indentations
+ *
+ * NEEDSWORK: The queue is actively being modified by the walker, for each commit
+ * its parents and itself get simplified and their flags set, but for the next
+ * unrelated commit or the grandparents they are not simplified yet, which means
+ * that a commit whose parents are all filtered will not be marked as a visual
+ * root candidate at the lookahead.
+ * This causes the lookahead to fail, failing to set the cascade flag to avoid
+ * redundant indentations.
+ * See 'test_expect_failure' at t4218-log-graph-indentation.sh.
+ */
+static void graph_peek_next_visible(struct git_graph *graph,
+ struct graph_lookahead_flags *flags)
+{
+ struct commit_list *cl;
+
+ flags->is_next_visible = 0;
+ flags->is_next_visual_root = 0;
+ flags->next_has_column = 0;
+
+ for (cl = graph->revs->commits; cl; cl = cl->next) {
+ if (get_commit_action(graph->revs, cl->item) != commit_show)
+ continue;
+ flags->is_next_visible = 1;
+ flags->next_has_column = graph_find_new_column_by_commit(graph, cl->item) >= 0;
+ /*
+ * We do not need graph->commit_in_columns or is_merge_parent,
+ * because we only need to know whether the next one might be a
+ * visual root, affecting the current commit where the cascade
+ * would have to be set and the first visual root not indented.
+ *
+ * It will set next_is_visual_root to true for merge parents that
+ * graph_is_visual_root() would return false, but if the next is
+ * a merge parent, the current commit is the child and cannot
+ * be a visual root and therefore having no effect.
+ */
+ if (!graph_is_visual_root_candidate(cl->item))
+ return;
+
+ /*
+ * The next visible commit is a visual root candidate, but
+ * only set cascade if it's not the last commit to be rendered.
+ */
+ for (cl = cl->next; cl; cl = cl->next) {
+ if (get_commit_action(graph->revs, cl->item) != commit_show)
+ continue;
+ flags->is_next_visual_root = 1;
+ return;
+ }
+ return;
+ }
+}
+
+static int graph_needs_pre_root_line(struct git_graph *graph)
+{
+ return graph->commit_in_columns && graph->is_visual_root &&
+ graph->num_columns > 0 && !graph->visual_root_cascade;
+}
+
void graph_update(struct git_graph *graph, struct commit *commit)
{
struct commit_list *parent;
+ struct graph_lookahead_flags flags;
/*
* Set the new commit
@@ -796,6 +999,23 @@ void graph_update(struct git_graph *graph, struct commit *commit)
*/
graph_update_columns(graph);
+ graph_peek_next_visible(graph, &flags);
+
+ graph->is_visual_root = graph_is_visual_root(graph, &flags);
+
+ if (graph->is_visual_root) {
+ /*
+ * If next is a visual root we can omit the indent for the first
+ * visual root and start cascading.
+ */
+ if (!graph->visual_root_depth && flags.is_next_visual_root)
+ graph->visual_root_cascade = 1;
+ graph->visual_root_depth++;
+ } else {
+ graph->visual_root_depth = 0;
+ graph->visual_root_cascade = 0;
+ }
+
graph->expansion_row = 0;
/*
@@ -813,11 +1033,16 @@ void graph_update(struct git_graph *graph, struct commit *commit)
* room for it. We need to do this only if there is a branch row
* (or more) to the right of this commit.
*
+ * If it is a visual root, we need to print an extra row to
+ * connect the indentation.
+ *
* If there are less than 3 parents, we can immediately print the
* commit line.
*/
if (graph->state != GRAPH_PADDING)
graph->state = GRAPH_SKIP;
+ else if (graph_needs_pre_root_line(graph))
+ graph->state = GRAPH_PRE_ROOT;
else if (graph_needs_pre_commit_line(graph))
graph->state = GRAPH_PRE_COMMIT;
else
@@ -1065,6 +1290,17 @@ static void graph_output_commit_line(struct git_graph *graph, struct graph_line
if (col_commit == graph->commit) {
seen_this = 1;
+ if (graph->is_visual_root) {
+ int depth = graph->visual_root_depth;
+ /*
+ * Each visual column is 2 characters wide.
+ * Omit the indentation for the first visual
+ * root in cascade mode.
+ */
+ int padding = (depth - graph->visual_root_cascade) * 2;
+ graph_line_addchars(line, ' ', padding);
+ graph->width += padding;
+ }
graph_output_commit_char(graph, line);
if (graph_needs_truncation(graph, i)) {
@@ -1436,6 +1672,29 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l
graph_update_state(graph, GRAPH_PADDING);
}
+static void graph_output_pre_root_line(struct git_graph *graph, struct graph_line *line)
+{
+ /*
+ * This function adds a row before a visual root, to connect the
+ * branch to the indented commit. It should only be called on a
+ * visual root.
+ */
+ assert(graph->is_visual_root);
+
+ for (size_t i = 0; i < graph->num_columns; i++) {
+ struct column *col = &graph->columns[i];
+ if (col->commit == graph->commit) {
+ graph_line_addch(line, ' ');
+ graph_line_write_column(line, col, '\\');
+ } else {
+ graph_line_write_column(line, col, '|');
+ }
+ graph_line_addch(line, ' ');
+ }
+
+ graph_update_state(graph, GRAPH_COMMIT);
+}
+
int graph_next_line(struct git_graph *graph, struct strbuf *sb)
{
int shown_commit_line = 0;
@@ -1461,6 +1720,9 @@ int graph_next_line(struct git_graph *graph, struct strbuf *sb)
case GRAPH_PRE_COMMIT:
graph_output_pre_commit_line(graph, &line);
break;
+ case GRAPH_PRE_ROOT:
+ graph_output_pre_root_line(graph, &line);
+ break;
case GRAPH_COMMIT:
graph_output_commit_line(graph, &line);
shown_commit_line = 1;
diff --git a/t/meson.build b/t/meson.build
index c5832fee05..17037a8465 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -576,6 +576,7 @@ integration_tests = [
't4215-log-skewed-merges.sh',
't4216-log-bloom.sh',
't4217-log-limit.sh',
+ 't4218-log-graph-indentation.sh',
't4252-am-options.sh',
't4253-am-keep-cr-dos.sh',
't4254-am-corrupt.sh',
diff --git a/t/t4218-log-graph-indentation.sh b/t/t4218-log-graph-indentation.sh
new file mode 100755
index 0000000000..f1c9584ba5
--- /dev/null
+++ b/t/t4218-log-graph-indentation.sh
@@ -0,0 +1,455 @@
+#!/bin/sh
+
+test_description='git log --graph visual root indentations'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-log-graph.sh
+
+check_graph_with_description () {
+ cat >expect &&
+ lib_test_cmp_graph --format="%s%ndescription%nsecond-line" "$@"
+}
+
+create_orphan () {
+ git checkout --orphan "$1" &&
+ { git rm -rf . || true; }
+}
+
+test_expect_success 'single root commit is not indented' '
+ create_orphan _1 && test_commit 1_A &&
+ lib_test_check_graph _1 <<-\EOF
+ * 1_A
+ EOF
+'
+
+test_expect_success 'visual root indented before unrelated branch' '
+ create_orphan _2 && test_commit 2_A && test_commit 2_B &&
+ create_orphan _3 && test_commit 3_A &&
+ lib_test_check_graph _2 _3 <<-\EOF
+ * 3_A
+ * 2_B
+ * 2_A
+ EOF
+'
+
+test_expect_success 'visual root indentation with --left-right' '
+ lib_test_check_graph --left-right _2..._3 <<-\EOF
+ > 3_A
+ < 2_B
+ < 2_A
+ EOF
+'
+
+# A better case of why indentation is still needed with '--left-right' flag is
+# that unrelated branches can be on the same side, so it's needed to
+# differentiate visual roots on the same side.
+test_expect_success 'visual root indentation with --left-right having unrelated commits on the same side' '
+ lib_test_check_graph --left-right _2..._3 _1 <<-\EOF
+ > 3_A
+ < 2_B
+ \
+ < 2_A
+ > 1_A
+ EOF
+'
+
+test_expect_success 'visual root indents the description also' '
+ check_graph_with_description _2 _3 <<-\EOF
+ * 3_A
+ description
+ second-line
+ * 2_B
+ | description
+ | second-line
+ * 2_A
+ description
+ second-line
+ EOF
+'
+
+test_expect_success 'indented visual root parent gets connected to its child' '
+ create_orphan _4 && test_commit 4_A && test_commit 4_B &&
+ create_orphan _5 && test_commit 5_A && test_commit 5_B &&
+ lib_test_check_graph _4 _5<<-\EOF
+ * 5_B
+ \
+ * 5_A
+ * 4_B
+ * 4_A
+ EOF
+'
+
+test_expect_success 'indented visual root parent gets connected to its child with description' '
+ check_graph_with_description _4 _5 <<-\EOF
+ * 5_B
+ | description
+ | second-line
+ \
+ * 5_A
+ description
+ second-line
+ * 4_B
+ | description
+ | second-line
+ * 4_A
+ description
+ second-line
+ EOF
+'
+
+test_expect_success 'visual roots cascade and last root does not' '
+ create_orphan _7 && test_commit 7_A && test_commit 7_B &&
+ create_orphan _8 && test_commit 8_A &&
+ create_orphan _9 && test_commit 9_A &&
+ create_orphan _10 && test_commit 10_A &&
+ lib_test_check_graph _7 _8 _9 _10 <<-\EOF
+ * 10_A
+ * 9_A
+ * 8_A
+ * 7_B
+ * 7_A
+ EOF
+'
+
+test_expect_success 'last root does not cascade' '
+ lib_test_check_graph _8 _9 _10 <<-\EOF
+ * 10_A
+ * 9_A
+ * 8_A
+ EOF
+'
+
+test_expect_success 'merge parents are roots between them but they do not indent' '
+ create_orphan _11 && test_commit 11_A &&
+ create_orphan _12 && test_commit 12_A &&
+ create_orphan _13 && test_commit 13_A &&
+ git checkout _11 &&
+ TREE=$(git write-tree) &&
+ MERGE=$(git commit-tree $TREE -p _11 -p _12 -p _13 -m 11_octopus) &&
+ git reset --hard $MERGE &&
+ lib_test_check_graph _11 <<-\EOF
+ *-. 11_octopus
+ |\ \
+ | | * 13_A
+ | * 12_A
+ * 11_A
+ EOF
+'
+
+# The last parent of a merge can be indented if nothing related to it needs to
+# be rendered after, if it's another visual root, merge parent must not get
+# indented but rather activate cascading.
+test_expect_success 'merge then unrelated visual root and unrelated branch' '
+ create_orphan _16 && test_commit 16_A && test_commit 16_B &&
+ create_orphan _17 && test_commit 17_A &&
+ create_orphan _18 && test_commit 18_A &&
+ create_orphan _19 && test_commit 19_A &&
+ create_orphan _20 && test_commit 20_A &&
+ git checkout _18 &&
+ TREE=$(git write-tree) &&
+ MERGE=$(git commit-tree $TREE -p _18 -p _19 -p _20 -m 18_octopus) &&
+ git reset --hard $MERGE &&
+ lib_test_check_graph _18 _17 _16 <<-\EOF
+ *-. 18_octopus
+ |\ \
+ | | * 20_A
+ | * 19_A
+ * 18_A
+ * 17_A
+ * 16_B
+ * 16_A
+ EOF
+'
+
+# The last commit root does not get indented, if the next thing after the root
+# merge parent is the last commit, indent the merge parent.
+test_expect_success 'merge then unrelated root indents merge parent' '
+ lib_test_check_graph _18 _17 <<-\EOF
+ *-. 18_octopus
+ |\ \
+ | | * 20_A
+ | * 19_A
+ \
+ * 18_A
+ * 17_A
+ EOF
+'
+
+test_expect_success 'merge then unrelated branch indents merge parent' '
+ lib_test_check_graph _18 _16 <<-\EOF
+ *-. 18_octopus
+ |\ \
+ | | * 20_A
+ | * 19_A
+ \
+ * 18_A
+ * 16_B
+ * 16_A
+ EOF
+'
+
+test_expect_success 'two-parent merge of orphans' '
+ create_orphan _21 && test_commit 21_A &&
+ create_orphan _22 && test_commit 22_A &&
+ git checkout _21 &&
+ TREE=$(git write-tree) &&
+ MERGE=$(git commit-tree $TREE -p _21 -p _22 -m 21_merge) &&
+ git reset --hard $MERGE &&
+ lib_test_check_graph _21 <<-\EOF
+ * 21_merge
+ |\
+ | * 22_A
+ * 21_A
+ EOF
+'
+
+test_expect_success 'commit with filtered parent becomes a visual root' '
+ create_orphan _23 &&
+ echo test >other.txt &&
+ git add other.txt &&
+ git commit -m "23_A" &&
+ echo test >foo.txt &&
+ git add foo.txt &&
+ git commit -m "23_B" &&
+ create_orphan _24 &&
+ echo test >foo.txt &&
+ git add foo.txt &&
+ git commit -m "24_A" &&
+ lib_test_check_graph _23 _24 -- foo.txt <<-\EOF
+ * 23_B
+ * 24_A
+ EOF
+'
+
+# The walker simplifies the commit for the current one and its parents, removing
+# the filtered parents, but it doesn't go one step ahead, this causes some edge
+# cases with the lookahead.
+# Given A (orphan), the walker only processes A, and when we lookahead for B
+# (child of C) even tho C will be filtered, it hasn't been simplified yet, so we
+# don't see B as a visual root, therefore cascade indentation isn't applied to A.
+# (cascade indentation starts the indentation at the second visual root, to avoid
+# redundant indentation). So A gets an extra indent, and once B is processed,
+# when rendering it, C has been removed, B is a visual root and as the last commit
+# isn't considered a visual root as it cannot have unrelated commits below it,
+# cascading isn't also applied, giving B another indent.
+#
+# The final result is an extra indent for A and B:
+#
+# A
+# B
+# D
+#
+# This will happen for any case where we find ourselves with the next commit
+# being a unrelated child of a parent the will be filtered.
+#
+# instead of the expected:
+test_expect_failure 'filtered parent cascading edge case' '
+ create_orphan _25 &&
+ git rm -rf . &&
+ echo test >other.txt &&
+ git add other.txt &&
+ git commit -m "C-filtered" &&
+
+ echo test >foo.txt &&
+ git add foo.txt &&
+ git commit -m "B (child of filtered)" &&
+
+ create_orphan _26 &&
+ git rm -rf . &&
+ echo test >foo.txt &&
+ git add foo.txt &&
+ git commit -m "A (visual root)" &&
+
+ create_orphan _27 &&
+ git rm -rf . &&
+ echo test >foo.txt &&
+ git add foo.txt &&
+ git commit -m "D (last)" &&
+
+ lib_test_check_graph _25 _26 _27 -- foo.txt <<-\EOF
+ * A (visual root)
+ * B (child of filtered)
+ * D (last)
+ EOF
+'
+
+test_expect_failure 'multiple filtered parents in sequence' '
+ create_orphan _44 &&
+ git rm -rf . &&
+ echo a >other.txt && git add other.txt && git commit -m "44_F" &&
+ echo b >foo.txt && git add foo.txt && git commit -m "44_C" &&
+
+ create_orphan _45 &&
+ git rm -rf . &&
+ echo c >other.txt && git add other.txt && git commit -m "45_F" &&
+ echo d >foo.txt && git add foo.txt && git commit -m "45_C" &&
+
+ create_orphan _46 &&
+ git rm -rf . &&
+ echo e >foo.txt && git add foo.txt && git commit -m "46_A" &&
+
+ lib_test_check_graph _44 _45 _46 -- foo.txt <<-\EOF
+ * 46_A
+ * 45_C
+ * 44_C
+ EOF
+'
+
+test_expect_failure 'real orphan root followed by child of filtered parent' '
+ create_orphan _47 &&
+ git rm -rf . &&
+ echo a >foo.txt && git add foo.txt && git commit -m "47_A" &&
+
+ create_orphan _48 &&
+ git rm -rf . &&
+ echo b >other.txt && git add other.txt && git commit -m "48_filtered" &&
+ echo c >foo.txt && git add foo.txt && git commit -m "48_B" &&
+
+ create_orphan _49 &&
+ git rm -rf . &&
+ echo d >foo.txt && git add foo.txt && git commit -m "49_last" &&
+
+ lib_test_check_graph _47 _48 _49 -- foo.txt <<-\EOF
+ * 47_A
+ * 48_B
+ * 49_last
+ EOF
+'
+
+# This tests prove why there is no need to have indentation for boundary
+# commits.
+#
+# Boundary commits rather than starting a column they 'inherit' the one of
+# its child so there will always be an edge that connects it removing the
+# ambiguity.
+test_expect_success 'unrelated boundaries are not ambiguous' '
+ create_orphan _28 && test_commit 28_A && test_commit 28_B &&
+ test_commit 28_C &&
+ create_orphan _29 && test_commit 29_A && test_commit 29_B &&
+ lib_test_check_graph --boundary 28_A.._28 29_A.._29 <<-\EOF
+ * 29_B
+ | * 28_C
+ | * 28_B
+ | o 28_A
+ o 29_A
+ EOF
+'
+
+# Same structure as t6016
+test_expect_success 'boundary commits big test' '
+ # 3 commits on branch _30
+ create_orphan _30 &&
+ test_commit 30_A &&
+ test_commit 30_B &&
+ test_commit 30_C &&
+
+ # 2 commits on branch _31, started from 30_A
+ git checkout -b _31 30_A &&
+ test_commit 31_A &&
+ test_commit 31_B &&
+
+ # 2 commits on branch _32, started from 30_B
+ git checkout -b _32 30_B &&
+ test_commit 32_A &&
+ test_commit 32_B &&
+
+ # Octopus merge _31 and _32 into -30
+ git checkout _30 &&
+ git merge _31 _32 -m 30_D &&
+ git tag 30_D &&
+ test_commit 30_E &&
+
+ # More commits on _32, then merge _32 into _30
+ git checkout _32 &&
+ test_commit 32_C &&
+ test_commit 32_D &&
+ git checkout _30 &&
+ git merge -s ours _32 -m 30_F &&
+ git tag 30_F &&
+ test_commit 30_G &&
+ lib_test_check_graph --boundary _30 _31 _32 ^32_C <<-\EOF
+ * 30_G
+ * 30_F
+ |\
+ | * 32_D
+ * | 30_E
+ | |
+ | \
+ *-. \ 30_D
+ |\ \ \
+ | * | | 31_B
+ | * | | 31_A
+ * | | | 30_C
+ o | | | 30_B
+ |/ / /
+ o / / 30_A
+ / /
+ | o 32_C
+ |/
+ o 32_B
+ EOF
+'
+
+# Filter by --first-parent and then forcing the filtered parents to be shown.
+test_expect_success '--first-parent flag with the filtered parents' '
+ (
+ create_orphan _35 && test_commit 35_A && test_commit 35_B &&
+ create_orphan _36 && test_commit 36_A &&
+ create_orphan _37 && test_commit 37_A &&
+ git checkout _35 &&
+ TREE=$(git write-tree) &&
+ MERGE=$(git commit-tree $TREE -p _35 -p _36 -p _37 -m 35_octopus) &&
+ git reset --hard $MERGE &&
+ lib_test_check_graph --first-parent _35 _36 _37 <<-\EOF
+ * 35_octopus
+ | * 37_A
+ | * 36_A
+ * 35_B
+ * 35_A
+ EOF
+ )
+'
+
+test_expect_success '--first-parent with filtered parents but one has a child' '
+ (
+ create_orphan _38 && test_commit 38_A && test_commit 38_B &&
+ create_orphan _39 && test_commit 39_A &&
+ create_orphan _40 && test_commit 40_A && test_commit 40_B &&
+ git checkout _38 &&
+ TREE=$(git write-tree) &&
+ MERGE=$(git commit-tree $TREE -p _38 -p _39 -p _40 -m 38_octopus) &&
+ git reset --hard $MERGE &&
+ lib_test_check_graph --first-parent _38 _39 _40 <<-\EOF
+ * 38_octopus
+ | * 40_B
+ | * 40_A
+ | * 39_A
+ * 38_B
+ * 38_A
+ EOF
+ )
+'
+
+test_expect_success '--first-parent with filtered parents but both have childs' '
+ (
+ create_orphan _41 && test_commit 41_A && test_commit 41_B &&
+ create_orphan _42 && test_commit 42_A && test_commit 42_B &&
+ create_orphan _43 && test_commit 43_A && test_commit 43_B &&
+ git checkout _41 &&
+ TREE=$(git write-tree) &&
+ MERGE=$(git commit-tree $TREE -p _41 -p _42 -p _43 -m 41_octopus) &&
+ git reset --hard $MERGE &&
+ lib_test_check_graph --first-parent _41 _42 _43 <<-\EOF
+ * 41_octopus
+ | * 43_B
+ | \
+ | * 43_A
+ | * 42_B
+ | * 42_A
+ * 41_B
+ * 41_A
+ EOF
+ )
+'
+
+test_done
--
2.54.0
^ permalink raw reply related
* [PATCH v4 1/2] lib-log-graph: move check_graph function
From: Pablo Sabater @ 2026-06-12 13:48 UTC (permalink / raw)
To: git
Cc: ayu.chandekar, chandrapratap3519, christian.couder, gitster,
jltobler, karthik.188, peff, phillip.wood, siddharthasthana31,
Pablo Sabater
In-Reply-To: <20260612-ps-pre-commit-indent-v4-0-e8492037ebae@gmail.com>
check_graph is a function shared in the test files t4215 and t6016 used
to format the output graph, but instead of being in a file called by
both test, the function code is repeated in each file.
Move check_graph to lib-log-graph.sh file which both tests already
import graph functions from, renaming it to lib_test_check_graph.
This function is needed for the following commit which includes graph
tests in a new file and requires check_graph.
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
t/lib-log-graph.sh | 5 +++++
t/t4215-log-skewed-merges.sh | 33 +++++++++++++-----------------
t/t6016-rev-list-graph-simplify-history.sh | 25 +++++++++-------------
3 files changed, 29 insertions(+), 34 deletions(-)
diff --git a/t/lib-log-graph.sh b/t/lib-log-graph.sh
index bf952ef920..1eae8f60c2 100644
--- a/t/lib-log-graph.sh
+++ b/t/lib-log-graph.sh
@@ -26,3 +26,8 @@ lib_test_cmp_colored_graph () {
test_decode_color <output.colors.raw | sed "s/ *\$//" >output.colors &&
test_cmp expect.colors output.colors
}
+
+lib_test_check_graph () {
+ cat >expect &&
+ lib_test_cmp_graph --format=%s "$@"
+}
diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index 1612f05f1b..eebab71039 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -5,11 +5,6 @@ test_description='git log --graph of skewed merges'
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-log-graph.sh
-check_graph () {
- cat >expect &&
- lib_test_cmp_graph --format=%s "$@"
-}
-
test_expect_success 'log --graph with merge fusing with its left and right neighbors' '
git checkout --orphan _p &&
test_commit A &&
@@ -21,7 +16,7 @@ test_expect_success 'log --graph with merge fusing with its left and right neigh
git checkout _p && git merge --no-ff _r -m G &&
git checkout @^^ && git merge --no-ff _p -m H &&
- check_graph <<-\EOF
+ lib_test_check_graph <<-\EOF
* H
|\
| * G
@@ -49,7 +44,7 @@ test_expect_success 'log --graph with left-skewed merge' '
git checkout 0_p && git merge --no-ff 0_s -m 0_G &&
git checkout @^ && git merge --no-ff 0_q 0_r 0_t 0_p -m 0_H &&
- check_graph <<-\EOF
+ lib_test_check_graph <<-\EOF
*-----. 0_H
|\ \ \ \
| | | | * 0_G
@@ -83,7 +78,7 @@ test_expect_success 'log --graph with nested left-skewed merge' '
git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
git checkout @^^ && git merge --no-ff 1_p -m 1_H &&
- check_graph <<-\EOF
+ lib_test_check_graph <<-\EOF
* 1_H
|\
| * 1_G
@@ -115,7 +110,7 @@ test_expect_success 'log --graph with nested left-skewed merge following normal
git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
git checkout 2_p && git merge --no-ff 2_s -m 2_K &&
- check_graph <<-\EOF
+ lib_test_check_graph <<-\EOF
* 2_K
|\
| * 2_J
@@ -151,7 +146,7 @@ test_expect_success 'log --graph with nested right-skewed merge following left-s
git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
git checkout @^^ && git merge --no-ff 3_p -m 3_J &&
- check_graph <<-\EOF
+ lib_test_check_graph <<-\EOF
* 3_J
|\
| * 3_H
@@ -182,7 +177,7 @@ test_expect_success 'log --graph with right-skewed merge following a left-skewed
git merge --no-ff 4_p -m 4_G &&
git checkout @^^ && git merge --no-ff 4_s -m 4_H &&
- check_graph --date-order <<-\EOF
+ lib_test_check_graph --date-order <<-\EOF
* 4_H
|\
| * 4_G
@@ -218,7 +213,7 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
git checkout 5_r &&
git merge --no-ff 5_s -m 5_H &&
- check_graph <<-\EOF
+ lib_test_check_graph <<-\EOF
* 5_H
|\
| *-. 5_G
@@ -257,7 +252,7 @@ test_expect_success 'log --graph with multiple tips' '
git checkout 6_1 &&
git merge --no-ff 6_2 -m 6_I &&
- check_graph 6_1 6_3 6_5 <<-\EOF
+ lib_test_check_graph 6_1 6_3 6_5 <<-\EOF
* 6_I
|\
| | * 6_H
@@ -334,7 +329,7 @@ test_expect_success 'log --graph with multiple tips' '
git checkout -b M_7 7_1 &&
git merge --no-ff 7_2 7_3 -m 7_M4 &&
- check_graph M_1 M_3 M_5 M_7 <<-\EOF
+ lib_test_check_graph M_1 M_3 M_5 M_7 <<-\EOF
* 7_M1
|\
| | * 7_M2
@@ -371,7 +366,7 @@ test_expect_success 'log --graph with multiple tips' '
'
test_expect_success 'log --graph --graph-lane-limit=2 limited to two lanes' '
- check_graph --graph-lane-limit=2 M_7 <<-\EOF
+ lib_test_check_graph --graph-lane-limit=2 M_7 <<-\EOF
*-. 7_M4
|\ \
| | * 7_G
@@ -388,7 +383,7 @@ test_expect_success 'log --graph --graph-lane-limit=2 limited to two lanes' '
'
test_expect_success 'log --graph --graph-lane-limit=1 truncate mid octopus merge' '
- check_graph --graph-lane-limit=1 M_7 <<-\EOF
+ lib_test_check_graph --graph-lane-limit=1 M_7 <<-\EOF
*-~ 7_M4
|\~
| ~ 7_G
@@ -405,7 +400,7 @@ test_expect_success 'log --graph --graph-lane-limit=1 truncate mid octopus merge
'
test_expect_success 'log --graph --graph-lane-limit=3 limited to three lanes' '
- check_graph --graph-lane-limit=3 M_1 M_3 M_5 M_7 <<-\EOF
+ lib_test_check_graph --graph-lane-limit=3 M_1 M_3 M_5 M_7 <<-\EOF
* 7_M1
|\
| | * 7_M2
@@ -441,7 +436,7 @@ test_expect_success 'log --graph --graph-lane-limit=3 limited to three lanes' '
'
test_expect_success 'log --graph --graph-lane-limit=6 check if it only shows first of 3 parent merge' '
- check_graph --graph-lane-limit=6 M_1 M_3 M_5 M_7 <<-\EOF
+ lib_test_check_graph --graph-lane-limit=6 M_1 M_3 M_5 M_7 <<-\EOF
* 7_M1
|\
| | * 7_M2
@@ -478,7 +473,7 @@ test_expect_success 'log --graph --graph-lane-limit=6 check if it only shows fir
'
test_expect_success 'log --graph --graph-lane-limit=7 check if it shows all 3 parent merge' '
- check_graph --graph-lane-limit=7 M_1 M_3 M_5 M_7 <<-\EOF
+ lib_test_check_graph --graph-lane-limit=7 M_1 M_3 M_5 M_7 <<-\EOF
* 7_M1
|\
| | * 7_M2
diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh
index 54b0a6f5f8..e0d9c3c1ac 100755
--- a/t/t6016-rev-list-graph-simplify-history.sh
+++ b/t/t6016-rev-list-graph-simplify-history.sh
@@ -13,11 +13,6 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-log-graph.sh
-check_graph () {
- cat >expect &&
- lib_test_cmp_graph --format=%s "$@"
-}
-
test_expect_success 'set up rev-list --graph test' '
# 3 commits on branch A
test_commit A1 foo.txt &&
@@ -54,7 +49,7 @@ test_expect_success 'set up rev-list --graph test' '
'
test_expect_success '--graph --all' '
- check_graph --all <<-\EOF
+ lib_test_check_graph --all <<-\EOF
* A7
* A6
|\
@@ -82,7 +77,7 @@ test_expect_success '--graph --all' '
# that undecorated merges are interesting, even with --simplify-by-decoration
test_expect_success '--graph --simplify-by-decoration' '
git tag -d A4 &&
- check_graph --all --simplify-by-decoration <<-\EOF
+ lib_test_check_graph --all --simplify-by-decoration <<-\EOF
* A7
* A6
|\
@@ -114,7 +109,7 @@ test_expect_success 'setup: get rid of decorations on B' '
# Graph with branch B simplified away
test_expect_success '--graph --simplify-by-decoration prune branch B' '
- check_graph --simplify-by-decoration --all <<-\EOF
+ lib_test_check_graph --simplify-by-decoration --all <<-\EOF
* A7
* A6
|\
@@ -133,7 +128,7 @@ test_expect_success '--graph --simplify-by-decoration prune branch B' '
'
test_expect_success '--graph --full-history -- bar.txt' '
- check_graph --full-history --all -- bar.txt <<-\EOF
+ lib_test_check_graph --full-history --all -- bar.txt <<-\EOF
* A7
* A6
|\
@@ -148,7 +143,7 @@ test_expect_success '--graph --full-history -- bar.txt' '
'
test_expect_success '--graph --full-history --simplify-merges -- bar.txt' '
- check_graph --full-history --simplify-merges --all -- bar.txt <<-\EOF
+ lib_test_check_graph --full-history --simplify-merges --all -- bar.txt <<-\EOF
* A7
* A6
|\
@@ -161,7 +156,7 @@ test_expect_success '--graph --full-history --simplify-merges -- bar.txt' '
'
test_expect_success '--graph -- bar.txt' '
- check_graph --all -- bar.txt <<-\EOF
+ lib_test_check_graph --all -- bar.txt <<-\EOF
* A7
* A5
* A3
@@ -172,7 +167,7 @@ test_expect_success '--graph -- bar.txt' '
'
test_expect_success '--graph --sparse -- bar.txt' '
- check_graph --sparse --all -- bar.txt <<-\EOF
+ lib_test_check_graph --sparse --all -- bar.txt <<-\EOF
* A7
* A6
* A5
@@ -189,7 +184,7 @@ test_expect_success '--graph --sparse -- bar.txt' '
'
test_expect_success '--graph ^C4' '
- check_graph --all ^C4 <<-\EOF
+ lib_test_check_graph --all ^C4 <<-\EOF
* A7
* A6
* A5
@@ -202,7 +197,7 @@ test_expect_success '--graph ^C4' '
'
test_expect_success '--graph ^C3' '
- check_graph --all ^C3 <<-\EOF
+ lib_test_check_graph --all ^C3 <<-\EOF
* A7
* A6
|\
@@ -220,7 +215,7 @@ test_expect_success '--graph ^C3' '
# that important, but this test depends on it. If the ordering ever changes
# in the code, we'll need to update this test.
test_expect_success '--graph --boundary ^C3' '
- check_graph --boundary --all ^C3 <<-\EOF
+ lib_test_check_graph --boundary --all ^C3 <<-\EOF
* A7
* A6
|\
--
2.54.0
^ permalink raw reply related
* [PATCH v4 0/2] graph: indent visual roots in graph
From: Pablo Sabater @ 2026-06-12 13:48 UTC (permalink / raw)
To: git
Cc: ayu.chandekar, chandrapratap3519, christian.couder, gitster,
jltobler, karthik.188, peff, phillip.wood, siddharthasthana31,
Pablo Sabater
In-Reply-To: <20260427102838.44867-1-pabloosabaterr@gmail.com>
When rendering a graph, if the history contains multiple "visual roots",
actual roots or commits that look like roots (i.e. have their parents
filtered out) can end up being vertically adjacent to unrelated commits,
falsely appearing to be related.
A fix for this issue was already attempted [1] a while ago.
This series adds indentation to the visual root commits, so they cannot be
vertically adjacent anymore making it easier to identify them.
before indentation:
* A
* B1
* B2
* C1
* C2
after indentation:
* A
* B1
\
* B2
* C1
* C2
Indents the visual root commits that have still commits to show after them, and
if they have children it connects them with an edge at a new row.
If there are multiple visual roots adjacent in history, the indentation starts
with the second one, avoiding redundant indentation of the first one and cascades
after the second.
* A
* B
* C
* D1
* D2
This series first commit is a cleanup that brings a common function from t4215
and t6016 to a graph functions file which they both use, so the new test file
for indentation, t4218, can use it as well.
The lookahead used to set the cascading and avoid extra indentation is not
completely reliable, as the walker goes through the commits it simplifies the
history of the current commit and its parents, but it doesn't simplify it
for the next unrelated or the grandparents. When the walker simplifies the
history, it removes filtered commits from the history and sets its flags.
When the next commit is an unrelated commit and its parents will be filtered
out, for the lookahead the commit is still a child of, it cannot know that the
next commit once simplified (advancing the walker) it will become a visual root.
This makes the lookahead fail, failing to set the cascading and starting it
with the first visual root, carrying an extra indent for the cascade.
given:
* A unrelated (visual root)
* B child of C
* C visual root WILL BE FILTERED OUT
* D unrelated (visual root)
the actual output is:
* A
* B
* D
A test has been added to t4218 and a NEEDSWORK to the lookahead function to
document this edge case but I'm not that familiar with revision.c. Maybe there's
a better way to make the lookahead more reliable.
[1]: https://lore.kernel.org/git/xmqqwnwajbuj.fsf@gitster.c.googlers.com/
V3 DIFF:
- Completly changes the approach to indent the visual roots instead of the
commits after the visual roots.
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
Pablo Sabater (2):
lib-log-graph: move check_graph function
graph: indent visual root in graph
graph.c | 262 +++++++++++++++++
t/lib-log-graph.sh | 5 +
t/meson.build | 1 +
t/t4215-log-skewed-merges.sh | 33 +--
t/t4218-log-graph-indentation.sh | 455 +++++++++++++++++++++++++++++
t/t6016-rev-list-graph-simplify-history.sh | 25 +-
6 files changed, 747 insertions(+), 34 deletions(-)
---
base-commit: 3e65291872de10c3f0bf05ea8c24187e7a71ebf0
change-id: 20260612-ps-pre-commit-indent-39ca72816382
Best regards,
--
Pablo Sabater <pabloosabaterr@gmail.com>
^ permalink raw reply
* Automated reviews by AI (was Re: [PATCH 0/5] Duplicate entry hardening)
From: Christian Couder @ 2026-06-12 13:29 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Junio C Hamano, Elijah Newren via GitGitGadget, git,
Elijah Newren, Konstantin Ryabitsev, Taylor Blau
In-Reply-To: <ah2PLBluBFy44AQI@pks.im>
On Tue, Jun 2, 2026 at 8:16 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Jun 01, 2026 at 09:33:10PM +0900, Junio C Hamano wrote:
> > This is a fix to an important corner of our system, but somehow left
> > in "Needs review" state for much longer than I would have liked, so
> > even though I am officially on vacation ;-), I took some time to
> > read these through (by the way it was a pleasant read, thank you).
>
> Honestly, I always shy away from the merge-related subsystems. It has a
> lot of subtleties that I don't have any experience with, so I never
> really consider my input to be helpful here.
>
> > I wonder if we create a rule like
> >
> > Those of you who have more than 30 commits in our project are
> > expected to review one topic (or more) from other contributors
> > for every three patches you send and ask for reviews by others.
>
> Heh, that would make me condense patch series into fewer patches ;)
>
> > it would help balance the patch vs review ratio, perhaps?
>
> It's a good question. I typically try to aim for reviewing series on the
> mailing list at least every second day, and I always encourage other
> folks in my team to do the same. But recently I (well, rather we)
> haven't really been able to due to the current situation at GitLab,
> which forces us to put almost all of our focus towards a different
> project for a while.
>
> Overall I agree that everyone who is a core contributor should also make
> reviews part of their regular worflow. At least for corporate
> contributors that might also make it easier to communicate this to their
> respective employers. Regardless of that, my expectation is that there
> will be times where it works well, and other times where it works less
> well.
Sashiko (https://github.com/sashiko-dev/sashiko) is used these days by
Linux kernel developers and seems to work well for them.
At GitLab and probably in other companies, some of us also use AI to
review our work before sending it to the mailing list. And yeah, it
helps find issues before our patches reach the mailing list.
In the same way as we require that patches must pass CI, do we want to
require that patches "pass" an AI review before they get accepted?
The benefit would be that it would hopefully catch a lot of trivial
things like indentation, typos/grammos, etc, and a lot of things a bit
more difficult to spot like memory issues. Perhaps with some amount of
prompting/configuration (for example pointing it at our
CodingGuidelines and SubmittingPatches) it could also catch issues
like style issues, commits that do too many things, refactoring
opportunities, etc.
We would likely still require at least one human review (by someone
who is not the maintainer) to validate architectural decisions, to
make sure it goes in the same direction as other efforts, and perhaps
also to make sure that AI suggestions were properly handled by the
patch author.
If we decide to require it, then there are a lot of questions that we
will have to answer.
Do we want to have our own system somehow managed by us or would we be
happy to use existing systems already in place in some companies as
long as we can still tweak them in some ways, like the current CI
systems we use?
If we use existing systems likely at GitLab and GitHub, it might be
more difficult to get coherent results as they might use different
LLMs, but maybe it could help tighten our docs to make sure everyone
is aligned, and we could get better reviews by using multiple systems
because an LLM might find an issue that the other LLM missed.
Do we want an AI review right after a patch is posted or only if there
is no human review in the next X days?
Also what if the AI makes a long concrete suggestion to improve on the
patches? Could that be incompatible with our AI policy to apply it?
Should we try to prevent the AI from making such a suggestion in the
first place?
I haven't looked at how Sashiko is used for the kernel, but maybe
there will need to be some kinds of restrictions/authentications to
avoid potential abuse.
^ permalink raw reply
* Re: [PATCH v3 2/2] compat/posix.h: simplify GIT_GNUC_PREREQ() comparison
From: Patrick Steinhardt @ 2026-06-12 13:27 UTC (permalink / raw)
To: Dominik Loidolt; +Cc: git, gitster, asedeno, asedeno, avarab
In-Reply-To: <20260608124419.38905-2-dominik.loidolt@univie.ac.at>
On Mon, Jun 08, 2026 at 02:44:19PM +0200, Dominik Loidolt wrote:
> Replace the glibc-style bit-shift version comparison with an explicit
> major/minor comparison. This is easier to read and is consistent with
> the format already used by GIT_CLANG_PREREQ() and many BSD
It's a bit funny to use `GIT_CLANG_PREREQ()` as an argument here as
we've just added it in the preceding commit.
> <sys/cdefs.h> headers.
>
> This has no runtime impact, as the macro is evaluated at compile time.
> It is also more future-proof, as it no longer assumes that GCC version
> components stay below 65536.
I feel like all the message needs to say is "let's do it for
consistency, and it's easier to read". That would've been sufficient,
whereas this argument here feels a bit thin.
Doesn't matter much though, and I think ultimately the message is fine
as-is, even though the reasoning is a bit funny.
> diff --git a/compat/posix.h b/compat/posix.h
> index ffdfd91c7b..deefc43f28 100644
> --- a/compat/posix.h
> +++ b/compat/posix.h
> @@ -4,22 +4,24 @@
> #define _FILE_OFFSET_BITS 64
>
> /*
> - * Derived from Linux "Features Test Macro" header
> - * Convenience macros to test the versions of gcc (or
> - * a compatible compiler).
> + * Convenience macros to test the versions of GCC (or a compatible compiler).
> * Use them like this:
> * #if GIT_GNUC_PREREQ (2,8)
> - * ... code requiring gcc 2.8 or later ...
> + * ... code requiring GCC 2.8 or later ...
> * #endif
> *
> + * Note that Clang and other compilers define __GNUC__ for compatibility; use
> + * GIT_CLANG_PREREQ() to check for specific Clang versions.
> + *
> * This macro of course is not part of POSIX, but we need it for the UNUSED
> * macro which is used by some of our POSIX compatibility wrappers.
> -*/
> + */
It would've been nice to either move these changes into a preparatory
commit or at least mention them
> #if defined(__GNUC__) && defined(__GNUC_MINOR__)
> # define GIT_GNUC_PREREQ(maj, min) \
> - ((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min))
> + ((__GNUC__ > (maj)) || \
> + (__GNUC__ == (maj) && __GNUC_MINOR__ >= (min)))
> #else
> - #define GIT_GNUC_PREREQ(maj, min) 0
> +# define GIT_GNUC_PREREQ(maj, min) 0
> #endif
The change itself makes sense to me.
I'm not sure myself whether this could use another reroll. It's all just
nits, and the intent is clear enough.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v2 2/4] pack-objects: support reachability bitmaps with `--path-walk`
From: Derrick Stolee @ 2026-06-12 13:24 UTC (permalink / raw)
To: Taylor Blau, git; +Cc: Junio C Hamano, Jeff King, Elijah Newren
In-Reply-To: <ffad584a43ebf3cb2138e8dce7daef84ab72712f.1780438896.git.me@ttaylorr.com>
On 6/2/2026 6:21 PM, Taylor Blau wrote:
> When 'pack-objects' is invoked with '--path-walk', it prevents us from
> using reachability bitmaps.
My earlier response focused on the _use_ of bitmaps when creating a
packfile, but your patch also enables _writing_ bitmaps with the
--path-walk option, which is significant and potentially more
interesting from my perspective: we have evidence that --path-walk
can produce significantly smaller packfiles than the standard
algorithm, and once those packfiles are created we can benefit from
that size in later packfile creation steps by reusing those deltas.
In this sense, I think the _writing_ is more important during a
repack scenario. The fetch/clone scenarios can benefit directly even
without integrating --path-walk with --use-bitmap-indexes.
> * A path-walk repack that writes bitmaps does not give the bitmap
> selector any commits, because path-walk reveals commits through
> `add_objects_by_path()` rather than through `show_commit()`, where
> `index_commit_for_bitmap()` is normally called.
...
> * On the writing side: teach the path-walk object callback to call
> `index_commit_for_bitmap()` for commits that it adds to the pack.
> That gives the bitmap selector the commit candidates it would have
> seen from the regular traversal.
My earlier reply to this patch was focused on the performance results
when using the "reading bitmaps" case, and I expressed suspicion about
the "exact" sizes of the packfiles.
Even more important here is that we have demonstrated examples of repos
that change their packfile size when using the --path-walk method. We
should demonstrate that the size continues to shrink with --path-walk
even when producing a matching .bitmap file with --write-bitmap-index.
The other thing that I notice here is that the bitmaps will need to
compute their reachable object set independently from the path-walk
algorithm. But I suppose that already happens separately from the
revision-walk approach that normally produces the packfile contents.
Note: A lot of my thoughts around asking for more evidence here is
that this patch seems suspiciously simple for integrating two
complicated features. The test suite (especially with
GIT_TEST_PACK_PATH_WALK=1) helps to guarantee that the result is
_correct_, but with performance features like this it's not enough to
"just" be correct. I want to see that we're having the intended
results.
From my perspective, the point of integrating these two things are:
1. Reachability bitmaps make it much faster to discover the reachable
set and reuse bits of existing packfiles. (Your performance table
demonstrates this is true.)
2. The --path-walk option can shrink packfile sizes by grouping
trees and blobs by path before those paths collide in the name-hash
sort. (I haven't seen evidence that this is happening.)
With evidence of (1) and not (2), it's not clear from the data that
these features are integrating completely. Without looking at the
code, those numbers would be the same if we had instead swapped the
preference of "the --path-walk option disables bitmaps" to "bitmaps
disable --path-walk".
Finally, I'll just note that I don't expect the _bitmaps_ to change
size dramatically. The --path-walk option does change the order of
the objects for its first pass of delta compression, but then uses
the (name-hash, size) sort to finalize the object ordering, so the
final object ordering _should_ be the same (unless I'm mistaken, in
which case the bitmaps could change size due to bitmap compression
concerns).
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v4 06/16] midx: support custom `--base` for incremental MIDX writes
From: Junio C Hamano @ 2026-06-12 13:21 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Taylor Blau, git, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <aiuaf3fKJ6kIITrf@szeder.dev>
SZEDER Gábor <szeder.dev@gmail.com> writes:
>> + layer="$(git multi-pack-index write --bitmap --incremental \
>> + --no-write-chain-file --base="$(nth_line 1 "$midx_chain")")" &&
>
> There is no 'nth_line' helper function in this test script.
Good eyes. It has been there in the file next door t5335 since
February, but not available here in t5334.
^ permalink raw reply
* Re: [PATCH v2 2/4] pack-objects: support reachability bitmaps with `--path-walk`
From: Derrick Stolee @ 2026-06-12 13:03 UTC (permalink / raw)
To: Taylor Blau, git; +Cc: Junio C Hamano, Jeff King, Elijah Newren
In-Reply-To: <ffad584a43ebf3cb2138e8dce7daef84ab72712f.1780438896.git.me@ttaylorr.com>
On 6/2/2026 6:21 PM, Taylor Blau wrote:
> As a result, we can see significantly reduced pack sizes from p5311
> before this commit:
I mentioned this before, but the pack _sizes_ aren't changing in this
example. We are computing them more quickly, though.
> Test HEAD^ HEAD
> ----------------------------------------------------------------------------------
> 5311.38: server (1 days, --path-walk) 2.56(2.52+0.03) 0.01(0.01+0.00) -99.6%
> 5311.39: size (1 days, --path-walk) 123.9K 123.9K +0.0%
> 5311.40: client (1 days, --path-walk) 0.00(0.01+0.00) 0.00(0.00+0.00) =
> 5311.42: server (2 days, --path-walk) 2.57(2.52+0.05) 0.01(0.01+0.00) -99.6%
> 5311.43: size (2 days, --path-walk) 123.9K 123.9K +0.0%
> 5311.44: client (2 days, --path-walk) 0.00(0.00+0.00) 0.00(0.00+0.00) =
> 5311.46: server (4 days, --path-walk) 2.58(2.51+0.07) 0.01(0.01+0.00) -99.6%
> 5311.47: size (4 days, --path-walk) 123.9K 123.9K +0.0%
> 5311.48: client (4 days, --path-walk) 0.00(0.00+0.00) 0.00(0.00+0.00) =
> 5311.50: server (8 days, --path-walk) 2.58(2.53+0.04) 0.02(0.02+0.00) -99.2%
> 5311.51: size (8 days, --path-walk) 152.4K 152.4K +0.0%
> 5311.52: client (8 days, --path-walk) 0.00(0.01+0.00) 0.00(0.01+0.00) =
> 5311.54: server (16 days, --path-walk) 2.58(2.52+0.05) 0.03(0.02+0.00) -98.8%
> 5311.55: size (16 days, --path-walk) 205.3K 205.3K +0.0%
> 5311.56: client (16 days, --path-walk) 0.01(0.01+0.00) 0.01(0.01+0.00) +0.0%
> 5311.58: server (32 days, --path-walk) 2.59(2.53+0.06) 0.03(0.03+0.00) -98.8%
> 5311.59: size (32 days, --path-walk) 209.3K 209.3K +0.0%
> 5311.60: client (32 days, --path-walk) 0.01(0.02+0.00) 0.01(0.02+0.00) +0.0%
> 5311.62: server (64 days, --path-walk) 2.70(2.76+0.06) 0.16(0.24+0.04) -94.1%
> 5311.63: size (64 days, --path-walk) 4.1M 4.1M +0.0%
> 5311.64: client (64 days, --path-walk) 0.44(0.50+0.02) 0.44(0.51+0.02) +0.0%
> 5311.66: server (128 days, --path-walk) 2.88(3.20+0.05) 0.34(0.65+0.05) -88.2%
> 5311.67: size (128 days, --path-walk) 9.0M 9.0M -0.0%
> 5311.68: client (128 days, --path-walk) 0.93(1.22+0.07) 0.93(1.20+0.08) +0.0%
Since we are testing --path-walk on both sides, the change across this
commit is that we are using the bitmaps for the "counting objects" phase
and then potentially using the --path-walk algorithm to construct the
packfile.
The fact that the packfile sizes are _identical_ is suspicious to me. I'd
expect some amount of difference here due to the change in algorithm. It's
possible that this could be explained by the repository shape not getting
any benefit from --path-walk because there are no name-hash collisions to
worry about.
The one thing that might be hinting towards _some_ difference is that the
relative sizes are showing as both "+0.0%" and "-0.0%", so perhaps the
exact sizes do have differences that are hidden behind the human-readable
sizes: 4.1M -> 4.1M is +0.0% but 9.0M -> 9.0M is -0.0%.
> We get the same size of output pack, but this commit allows us to do so
> in a significantly shorter amount of time.
Ok, you have the correct interpretation here, just a lingering typo in
the earlier sentence before the table.
> Intuitively, we're generating
> the same pack (hence the unchanged 'test_size' output from run to run),
> but varying how we get there. Before this commit, pack-objects prefers
> '--path-walk' to '--use-bitmap-index', so we generate the output pack by
> performing a normal '--path-walk' traversal. With this commit, we are
> operating over a *repacked* state (that itself was done with a
> '--path-walk' traversal), but are able to perform pack-reuse on that
> repacked state via bitmaps.
And I wonder if the test setup creates a situation where we are always
reusing deltas from the underlying packfile, so the --path-walk algorithm
isn't doing anything to help with delta compression at this point and the
difference in this patch is that we are replacing the object reachability
calculation entirely with bitmaps.
I suppose what I'm really worried about is that I'm hoping to see some
evidence from a large-scale test that demonstrates that the two algorithms
are working in tandem in a non-trivial way. I haven't seen it yet, but I
also don't have evidence that they _aren't_ working together.
Thanks,
-Stolee
^ permalink raw reply
* Re: [RFC] commit-reach: terminate merge-base walk when one paint side is exhausted
From: Derrick Stolee @ 2026-06-12 12:52 UTC (permalink / raw)
To: Kristofer Karlsson, git
In-Reply-To: <CAL71e4Mp7ewv0UGS8j=iTq6quyxLXzrr0uNDbWR8JKaOsTSVyA@mail.gmail.com>
On 6/12/2026 7:15 AM, Kristofer Karlsson wrote:
> I have an idea to make it faster for fetching all merge-bases for
> common flows in large repos, as long as the commit graph is
> reasonably up to date.
>
> The key part is the exit condition in paint_down_to_common.
> Instead of waiting for the queue to only contain stale entries,
> it is enough to wait for one of the sides to be exhausted,
> i.e. side 1 is exhausted if no more commits exist in the
> traversal queue flagged with only PARENT1. For example, if
> the two sides are origin/HEAD and a small PR branch, the PR
> branch will quickly become exhausted at the merge-base, while
> the main side will continue.
Generally, you'd replace the queue_has_nonstale() condition
with a more generic queue_can_halt() condition.
> Now you may ask: why is that a safe condition?
>
> The traversal in paint_down_to_common has two logical phases
> due to the priority queue ordering:
>
> 1. Process all commits with infinite generation numbers.
> This includes all commits when there is no commit-graph.
> 2. Process all commits with finite generation numbers.
>
> These happen in strict order -- all INFINITY commits are popped
> before any finite-generation commit.
>
> The optimization only applies after the walk enters the second phase.
> In the first phase, the traversal behaves exactly as today
> and uses the existing termination condition.
This would mean that queue_can_halt() would need to know the
following:
1. If we peek at the top of the queue, is that a commit with
infinite generation number? If so, then we can only use
queue_has_nonstale().
2. Otherwise, we know that all commits in the queue are ordered
topologically and can use a different, faster check. To start,
we need to keep going as long as at least one commit has only
one side
> In the second phase, traversal follows strict topological
> order -- descendants are processed before ancestors. Paint flags
> propagate from each processed commit to its parents, which have
> strictly lower generation and are therefore not yet examined.
>
> A new merge-base candidate can only form when a PARENT1-only path
> meets a PARENT2-only path. Once a commit acquires both paint flags
> in this phase, any descendant carrying both paint flags would
> already have been processed.
>
> Once one side is exhausted from the queue, no new meeting between
> pure sides can occur. Any commit that subsequently acquires both
> paint flags must inherit them from a commit that already had both
> flags -- it is deeper in the graph and cannot affect the final
> merge-base set. We can stop.
The important thing to realize at this point is that commits in the
queue have received flags from their children (children were walked
topologically and "push" flags to their parents).
The STALE bit is pushed from commits that have bits for both sides
of the merge. This isn't something that we can learn from just
walking each side: we need some amount of walking within the
intersection.
This doesn't matter if we are looking for a single merge base, but
when we want the full set of independent merge bases, then the STALE
bit becomes very important.
> On a large monorepo with previously expensive merge-base and
> merge-tree queries, I observed speedups ranging from roughly 300x
> to 1000x. The nice thing is that this works for merge-base --all
> and every internal caller of paint_down_to_common -- we no longer
> have to restrict the optimization to finding just the first merge-base.
>
> Does the correctness argument above hold?
I think that it doesn't work when trying to get all merge bases. It
requires
A X
/| __/|
| |/ |
| B |
| | |
..........
| | __/
\|/
C
In this example, B can reach C through some long list of commits.
This makes B (and X) have much higher generation number than C.
After exhausting both sides of A...X, we have B and C in the queue
with both side bits and neither are stale. But we need to walk
from B to C to discover that C should be stale.
> Happy to come back with a patch later if the logic holds and the
> overall approach is wanted.
You are identifying a point where optimizations are possible, based
on your measurements of the time spent in this walk waiting for
queue_has_nonstale() to end the loop. Specifically, the cost of using
a BFS approach is costing time.
One place that I would recommend here is to take the work you are
doing to investigate the behavior of tips_reachable_from_bases()
or get_reachable_subset() to see if we can use a DFS-based approach
_in this case_ where we have exhausted both side and are only caring
about the STALE bit checking these cases.
Remember that the DFS idea only helps in the case where we find a
path between commits (B to C in this case) without walking all of the
commits above the minimum generation (generation of C). In an alternate
case where B and C are truly independent, this would not save any time.
But these "they are mutually unreachable" cases always require walking
the full set based on the generation number. The good news is that the
vast majority of cases do not actually have multiple independent merge
bases, so there is potential here.
Thanks,
-Stolee
^ permalink raw reply
* Re: [GIT PULL] git-gui: repo discovery with rev-parse; pick and gui subcommands; silent make -s
From: Junio C Hamano @ 2026-06-12 12:42 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
In-Reply-To: <ca428e6e-c840-4ee6-9fcf-39889fc07400@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> The following changes since commit bb52cdac6254c006e06bf0bb820268dcf024fc22:
>
> git-gui: grey out comment lines in commit message (2026-03-04 08:04:37 +0100)
>
> are available in the Git repository at:
>
> https://github.com/j6t/git-gui.git master
>
> for you to fetch changes up to 1b2c2a2edbaa1638becef4c3755b3e0633b9c304:
>
> Merge branch 'ml/repo-discovery' (2026-06-12 11:05:28 +0200)
Thanks. Will pull together with the gitk updates.
^ permalink raw reply
* Re: [PATCH v3] ref-filter: restore prefix-scoped iteration
From: Patrick Steinhardt @ 2026-06-12 11:48 UTC (permalink / raw)
To: Tamir Duberstein
Cc: git, Karthik Nayak, Junio C Hamano, Victoria Dye, ZheNing Hu
In-Reply-To: <20260610-fix-git-branch-regression-v3-1-6fd48fad7a53@gmail.com>
On Wed, Jun 10, 2026 at 05:29:49AM -0700, Tamir Duberstein wrote:
> dabecb9db2 (for-each-ref: introduce a '--start-after' option,
> 2025-07-15) changed branch, remote-tracking branch, and tag enumeration
> from constructing an iterator with the namespace prefix to constructing
> an unscoped iterator and seeking to the prefix.
>
> The files backend constructs its loose-ref iterator with cache priming
> enabled. cache_ref_iterator_begin() immediately applies the construction
> prefix through cache_ref_iterator_set_prefix(), reading loose refs
> beneath it before packed refs are opened. An empty prefix therefore
> reads every loose ref, and a later seek cannot undo that I/O.
>
> For these single-kind filters, construct the iterator with the namespace
> prefix when start_after is not set. Keep the existing unscoped
> construction for start_after, whose seek position may differ from the
> namespace prefix.
>
> With 10,000 unrelated loose refs, the p6300 tests improve as follows:
>
> before after
> branch 2.74 s 0.11 s
> branch --remotes 2.81 s 0.12 s
> tag 3.01 s 0.11 s
>
> Link: https://lore.kernel.org/git/aGZidwwlToWThkn8@pks.im/
> Link: https://lore.kernel.org/git/xmqqikjq7s16.fsf@gitster.g/
> Link: https://lore.kernel.org/r/CAOLa=ZRHKNNymXGk31YgECjUmF9nZ8GsPUdQb7aKBH5DKMz7=w@mail.gmail.com
I honestly have no idea what you want to say with these links, as they
seem to just link to random reviews mails when the above mentioned
commit was reviewed. In general, we typically try to embed references
like this into the explanation, like:
In [1], we discussed... and this is relevant because of ...
[1]: https://lore.kernel.org/git/aGZidwwlToWThkn8@pks.im/
Just dropping the links as-is without much of an explanation isn't
helpful.
> diff --git a/ref-filter.c b/ref-filter.c
> index 1da4c0e60d..9b04e3af85 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -3316,15 +3316,14 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, refs_for
>
> if (prefix) {
> struct ref_iterator *iter;
> + struct ref_store *store = get_main_ref_store(the_repository);
>
> - iter = refs_ref_iterator_begin(get_main_ref_store(the_repository),
> - "", NULL, 0, 0);
> -
> - if (filter->start_after)
> + if (filter->start_after) {
> + iter = refs_ref_iterator_begin(store, "", NULL, 0, 0);
> ret = start_ref_iterator_after(iter, filter->start_after);
> - else
> - ret = ref_iterator_seek(iter, prefix,
> - REF_ITERATOR_SEEK_SET_PREFIX);
> + } else {
> + iter = refs_ref_iterator_begin(store, prefix, NULL, 0, 0);
> + }
>
> if (!ret)
> ret = do_for_each_ref_iterator(iter, fn, cb_data);
The patch itself seems sensible to me.
> diff --git a/t/perf/p6300-for-each-ref.sh b/t/perf/p6300-for-each-ref.sh
> index fa7289c752..ed9c1c6a19 100755
> --- a/t/perf/p6300-for-each-ref.sh
> +++ b/t/perf/p6300-for-each-ref.sh
> @@ -1,6 +1,6 @@
> #!/bin/sh
>
> -test_description='performance of for-each-ref'
> +test_description='performance of ref-filter users'
> . ./perf-lib.sh
>
> test_perf_fresh_repo
> @@ -84,4 +84,41 @@ test_expect_success 'pack refs' '
> '
> run_tests "packed"
>
> +test_expect_success REFFILES 'setup many unrelated loose refs' '
> + git init scoped &&
> + test_commit -C scoped --no-tag base &&
> + test_seq $ref_count_per_type |
> + sed "s,.*,update refs/custom/unrelated_& HEAD," |
> + git -C scoped update-ref --stdin &&
> + git -C scoped update-ref refs/remotes/origin/main HEAD &&
> + git -C scoped update-ref refs/tags/only HEAD
> +'
I've already called this out before on other patches, but the REFFILES
prerequisite just doesn't make any sense here as this test logic is
generic.
Patrick
^ permalink raw reply
* Re: [PATCH 2/6] SubmittingPatches: discuss non-ident trailers
From: Patrick Steinhardt @ 2026-06-12 11:35 UTC (permalink / raw)
To: kristofferhaugsbakk; +Cc: git, Kristoffer Haugsbakk
In-Reply-To: <non-ident_trailers.8f5@msgid.xyz>
On Thu, Jun 11, 2026 at 12:22:45AM +0200, kristofferhaugsbakk@fastmail.com wrote:
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 0b12badf86d..51c308a89a8 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -474,7 +474,10 @@ These are the common trailers in use:
>
> While you can also create your own trailer if the situation warrants it, we
> encourage you to instead use one of the common trailers in this project
> -highlighted above.
> +highlighted above. A trailer that credits someone might be more likely
> +to be accepted since these are the most common ones. But another kind of
> +trailer might be relevant, for example to link to an issue tracker
> +belonging to a downstream project that is affected by a bug in Git.
Hm, I wonder whether this is a bit too vague to really be helpful for a
newcomer. Instead of alluding to such trailers, wouldn't it be
preferable if we added those as actual examples to the list of known
trailers and then tell folks that they can invent their own ones if
there is a good reason to do so?
Patrick
^ permalink raw reply
* Re: [PATCH v2 0/7] setup: drop global state
From: Patrick Steinhardt @ 2026-06-12 11:25 UTC (permalink / raw)
To: Toon Claes; +Cc: Justin Tobler, git
In-Reply-To: <87ldckyygk.fsf@emacs.iotcl.com>
On Fri, Jun 12, 2026 at 10:06:35AM +0200, Toon Claes wrote:
> Justin Tobler <jltobler@gmail.com> writes:
>
> > I find the additional explaination here quite helpful. Thanks.
>
> Yeah, hard to follow series looking at the code only, but commit
> messages make more bearable.
>
> > The changes in this version of the series looks good to me.
>
> I've only reviewed v2 and I agree this version looks good.
There's only a single change to a commit message I've queued, so I'll
for now not send another iteration.
Thanks for your review!
Patrick
^ permalink raw reply
* [RFC] commit-reach: terminate merge-base walk when one paint side is exhausted
From: Kristofer Karlsson @ 2026-06-12 11:15 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee
Hi! I previously sent a patch[1] to optimize paint_down_to_common
for the single merge-base case. I believe I have found a stronger
optimization, but before sending a patch I wanted to discuss the
correctness argument.
The main problem to solve is that computing merge-bases is slow today
in some scenarios, especially large monorepos with complex graphs.
This affects multiple operations, including merge-base and merge-tree.
The previous patch improved it for the special case of the
merge-base being part of the commit-graph and the caller only
needing to know about one merge-base.
I have an idea to make it faster for fetching all merge-bases for
common flows in large repos, as long as the commit graph is
reasonably up to date.
The key part is the exit condition in paint_down_to_common.
Instead of waiting for the queue to only contain stale entries,
it is enough to wait for one of the sides to be exhausted,
i.e. side 1 is exhausted if no more commits exist in the
traversal queue flagged with only PARENT1. For example, if
the two sides are origin/HEAD and a small PR branch, the PR
branch will quickly become exhausted at the merge-base, while
the main side will continue.
Now you may ask: why is that a safe condition?
The traversal in paint_down_to_common has two logical phases
due to the priority queue ordering:
1. Process all commits with infinite generation numbers.
This includes all commits when there is no commit-graph.
2. Process all commits with finite generation numbers.
These happen in strict order -- all INFINITY commits are popped
before any finite-generation commit.
The optimization only applies after the walk enters the second phase.
In the first phase, the traversal behaves exactly as today
and uses the existing termination condition.
In the second phase, traversal follows strict topological
order -- descendants are processed before ancestors. Paint flags
propagate from each processed commit to its parents, which have
strictly lower generation and are therefore not yet examined.
A new merge-base candidate can only form when a PARENT1-only path
meets a PARENT2-only path. Once a commit acquires both paint flags
in this phase, any descendant carrying both paint flags would
already have been processed.
Once one side is exhausted from the queue, no new meeting between
pure sides can occur. Any commit that subsequently acquires both
paint flags must inherit them from a commit that already had both
flags -- it is deeper in the graph and cannot affect the final
merge-base set. We can stop.
On a large monorepo with previously expensive merge-base and
merge-tree queries, I observed speedups ranging from roughly 300x
to 1000x. The nice thing is that this works for merge-base --all
and every internal caller of paint_down_to_common -- we no longer
have to restrict the optimization to finding just the first merge-base.
Does the correctness argument above hold?
Happy to come back with a patch later if the logic holds and the
overall approach is wanted.
Thanks,
Kristofer
[1] https://lore.kernel.org/git/pull.2109.v4.git.1778504352.gitgitgadget@gmail.com/
^ permalink raw reply
* [PATCH 2/2] push: suggest <remote> <branch> for a slash slip
From: Harald Nordgren via GitGitGadget @ 2026-06-12 11:10 UTC (permalink / raw)
To: git; +Cc: Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2331.git.git.1781262619.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
"git push origin/main" is treated as a repository and dies with
"'origin/main' does not appear to be a git repository", with no hint
that a space was meant instead of a slash.
When the argument is not an existing path or configured remote but its
part before the first slash names one, suggest the intended
"git push <remote> <branch>" form. The suggestion is shown as advice so
it can be silenced with advice.pushRepoLooksLikeRef.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Documentation/config/advice.adoc | 5 +++++
advice.c | 1 +
advice.h | 1 +
builtin/push.c | 26 +++++++++++++++++++++++++-
t/t5529-push-errors.sh | 31 +++++++++++++++++++++++++++++++
5 files changed, 63 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/advice.adoc b/Documentation/config/advice.adoc
index 257db58918..fa77a5110e 100644
--- a/Documentation/config/advice.adoc
+++ b/Documentation/config/advice.adoc
@@ -90,6 +90,11 @@ all advice messages.
Shown when linkgit:git-push[1] rejects a forced update of
a branch when its remote-tracking ref has updates that we
do not have locally.
+ pushRepoLooksLikeRef::
+ Shown when the repository given to linkgit:git-push[1] is not
+ a configured remote but looks like a `<remote>/<branch>` ref,
+ suggesting that the remote and branch be given as separate
+ arguments.
pushUnqualifiedRefname::
Shown when linkgit:git-push[1] gives up trying to
guess based on the source and destination refs what
diff --git a/advice.c b/advice.c
index 0018501b7b..63bf8b0c5f 100644
--- a/advice.c
+++ b/advice.c
@@ -69,6 +69,7 @@ static struct {
[ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent" },
[ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching" },
[ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate" },
+ [ADVICE_PUSH_REPO_LOOKS_LIKE_REF] = { "pushRepoLooksLikeRef" },
[ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" },
[ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" },
[ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */
diff --git a/advice.h b/advice.h
index 8def280688..66f6cd6a77 100644
--- a/advice.h
+++ b/advice.h
@@ -36,6 +36,7 @@ enum advice_type {
ADVICE_PUSH_NON_FF_CURRENT,
ADVICE_PUSH_NON_FF_MATCHING,
ADVICE_PUSH_REF_NEEDS_UPDATE,
+ ADVICE_PUSH_REPO_LOOKS_LIKE_REF,
ADVICE_PUSH_UNQUALIFIED_REF_NAME,
ADVICE_PUSH_UPDATE_REJECTED,
ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
diff --git a/builtin/push.c b/builtin/push.c
index 6021b71d66..c21febadbe 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -8,6 +8,7 @@
#include "advice.h"
#include "branch.h"
#include "config.h"
+#include "dir.h"
#include "environment.h"
#include "gettext.h"
#include "hex.h"
@@ -744,6 +745,29 @@ int cmd_push(int argc,
if (repo) {
if (!add_remote_or_group(repo, &remote_group)) {
+ const char *slash = strchr(repo, '/');
+ struct remote *r;
+
+ /*
+ * A "<remote>/<branch>" argument that does not name
+ * a path is likely a slip for the separate
+ * "<remote> <branch>" form, so suggest that instead.
+ */
+ if (slash && slash[1] && !file_exists(repo)) {
+ struct strbuf name = STRBUF_INIT;
+
+ strbuf_add(&name, repo, slash - repo);
+ if (remote_is_configured(remote_get(name.buf), 0)) {
+ int code = die_message(_("'%s' is not a valid push target"), repo);
+ advise_if_enabled(ADVICE_PUSH_REPO_LOOKS_LIKE_REF,
+ _("Did you mean to use: git push %s %s?"),
+ name.buf, slash + 1);
+ strbuf_release(&name);
+ exit(code);
+ }
+ strbuf_release(&name);
+ }
+
/*
* Not a configured remote name or group name.
* Try treating it as a direct URL or path, e.g.
@@ -753,7 +777,7 @@ int cmd_push(int argc,
* from the URL so the loop below can handle it
* identically to a named remote.
*/
- struct remote *r = pushremote_get(repo);
+ r = pushremote_get(repo);
if (!r)
die(_("bad repository '%s'"), repo);
string_list_append(&remote_group, r->name);
diff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh
index 80b06a0cd2..cfb294305d 100755
--- a/t/t5529-push-errors.sh
+++ b/t/t5529-push-errors.sh
@@ -54,6 +54,37 @@ test_expect_success 'detect empty remote with targeted refspec' '
grep "fatal: bad repository ${SQ}${SQ}" stderr
'
+test_expect_success 'suggest <remote> <branch> for a <remote>/<branch> slip' '
+ test_must_fail git push origin/main 2>stderr &&
+ grep "${SQ}origin/main${SQ} is not a valid push target" stderr &&
+ grep "hint: Did you mean to use: git push origin main?" stderr &&
+ test_must_fail git -c advice.pushRepoLooksLikeRef=false push origin/main 2>stderr &&
+ ! grep "Did you mean" stderr
+'
+
+test_expect_success 'suggest <remote> <branch> when the branch has slashes' '
+ test_must_fail git push origin/feature/x 2>stderr &&
+ grep "hint: Did you mean to use: git push origin feature/x?" stderr
+'
+
+test_expect_success 'no suggestion when prefix is not a configured remote' '
+ test_must_fail git push not-a-remote/main 2>stderr &&
+ ! grep "Did you mean" stderr
+'
+
+test_expect_success 'no suggestion for a trailing slash with no branch' '
+ test_must_fail git push origin/ 2>stderr &&
+ ! grep "Did you mean" stderr
+'
+
+test_expect_success 'no suggestion when the argument is an existing path' '
+ test_when_finished "rm -rf origin" &&
+ git init --bare origin/main &&
+ git push origin/main HEAD:refs/heads/pushed 2>stderr &&
+ ! grep "Did you mean" stderr &&
+ git -C origin/main rev-parse --verify refs/heads/pushed
+'
+
test_expect_success 'detect ambiguous refs early' '
git branch foo &&
git tag foo &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/2] branch: suggest <remote>/<branch> on upstream slip
From: Harald Nordgren via GitGitGadget @ 2026-06-12 11:10 UTC (permalink / raw)
To: git; +Cc: Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2331.git.git.1781262619.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
"git branch --set-upstream-to origin main" reads the trailing word as
the local branch to operate on and dies with "branch 'main' does not
exist", pointing at the wrong problem.
When that branch is missing and "<remote>/<branch>" names a real
remote-tracking ref, suggest the intended
"git branch --set-upstream-to=<remote>/<branch>" form.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
builtin/branch.c | 17 +++++++++++++++++
t/t3200-branch.sh | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+)
diff --git a/builtin/branch.c b/builtin/branch.c
index 1572a4f9ef..7ad3efb908 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -957,6 +957,23 @@ int cmd_branch(int argc,
if (!refs_ref_exists(get_main_ref_store(the_repository), branch->refname)) {
if (!argc || branch_checked_out(branch->refname))
die(_("no commit on branch '%s' yet"), branch->name);
+ if (argc == 1 && !strchr(new_upstream, '/') &&
+ remote_is_configured(remote_get(new_upstream), 0)) {
+ struct strbuf remote_ref = STRBUF_INIT;
+
+ strbuf_addf(&remote_ref, "refs/remotes/%s/%s",
+ new_upstream, argv[0]);
+ if (refs_ref_exists(get_main_ref_store(the_repository),
+ remote_ref.buf)) {
+ int code = die_message(_("--set-upstream-to takes a single <remote>/<branch> argument"));
+ advise_if_enabled(ADVICE_SET_UPSTREAM_FAILURE,
+ _("Did you mean to use: git branch --set-upstream-to=%s/%s?"),
+ new_upstream, argv[0]);
+ strbuf_release(&remote_ref);
+ exit(code);
+ }
+ strbuf_release(&remote_ref);
+ }
die(_("branch '%s' does not exist"), branch->name);
}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e7829c2c4b..e2682a83a0 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1022,6 +1022,44 @@ test_expect_success '--set-upstream-to fails on a missing dst branch' '
test_cmp expect err
'
+test_expect_success '--set-upstream-to suggests <remote>/<branch> on slip' '
+ test_when_finished "git remote remove slip-remote" &&
+ git remote add slip-remote . &&
+ git update-ref refs/remotes/slip-remote/slip-feature HEAD &&
+ test_must_fail git branch --set-upstream-to slip-remote slip-feature 2>err &&
+ test_grep "takes a single <remote>/<branch> argument" err &&
+ test_grep "hint: Did you mean to use: git branch --set-upstream-to=slip-remote/slip-feature?" err &&
+ test_must_fail git -c advice.setUpstreamFailure=false \
+ branch --set-upstream-to slip-remote slip-feature 2>err &&
+ test_grep ! "Did you mean" err
+'
+
+test_expect_success '--set-upstream-to does not suggest when no matching remote ref' '
+ test_when_finished "git remote remove slip-remote" &&
+ git remote add slip-remote . &&
+ test_must_fail git branch --set-upstream-to slip-remote no-such-branch 2>err &&
+ test_grep "branch ${SQ}no-such-branch${SQ} does not exist" err &&
+ test_grep ! "Did you mean" err
+'
+
+test_expect_success '--set-upstream-to to a local branch is not mistaken for a slip' '
+ git branch slip-local-upstream &&
+ git branch slip-local-target &&
+ git branch --set-upstream-to=slip-local-upstream slip-local-target 2>err &&
+ test_grep ! "Did you mean" err &&
+ echo refs/heads/slip-local-upstream >expect &&
+ git config branch.slip-local-target.merge >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--set-upstream-to slip suggestion keeps a slashed branch name' '
+ test_when_finished "git remote remove slip-remote" &&
+ git remote add slip-remote . &&
+ git update-ref refs/remotes/slip-remote/slip/feature HEAD &&
+ test_must_fail git branch --set-upstream-to slip-remote slip/feature 2>err &&
+ test_grep "hint: Did you mean to use: git branch --set-upstream-to=slip-remote/slip/feature?" err
+'
+
test_expect_success '--set-upstream-to fails on a missing src branch' '
test_must_fail git branch --set-upstream-to does-not-exist main 2>err &&
test_grep "the requested upstream branch '"'"'does-not-exist'"'"' does not exist" err
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/2] branch/push: suggest intended form when remote/branch slip given
From: Harald Nordgren via GitGitGadget @ 2026-06-12 11:10 UTC (permalink / raw)
To: git; +Cc: Harald Nordgren
When the repository or upstream argument is a slip like "origin/main" or
"origin main", suggest the intended "git push origin main" or "git branch
--set-upstream-to=origin/main" form instead of failing with an unrelated
error.
Harald Nordgren (2):
branch: suggest <remote>/<branch> on upstream slip
push: suggest <remote> <branch> for a slash slip
Documentation/config/advice.adoc | 5 +++++
advice.c | 1 +
advice.h | 1 +
builtin/branch.c | 17 ++++++++++++++
builtin/push.c | 26 +++++++++++++++++++++-
t/t3200-branch.sh | 38 ++++++++++++++++++++++++++++++++
t/t5529-push-errors.sh | 31 ++++++++++++++++++++++++++
7 files changed, 118 insertions(+), 1 deletion(-)
base-commit: 3e65291872de10c3f0bf05ea8c24187e7a71ebf0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2331%2FHaraldNordgren%2Fsuggest-remote-branch-slips-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2331/HaraldNordgren/suggest-remote-branch-slips-v1
Pull-Request: https://github.com/git/git/pull/2331
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 9/9] refs: always use absolute paths for reference stores
From: Karthik Nayak @ 2026-06-12 9:58 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-9-56c864b01c43@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7719 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> Both the "files" and "reftable" backends use
> `refs_compute_filesystem_location()` to figure out the location of both
> the git and common directories. Depending on how the function is called
> we may or may not return an absolute path.
>
> There isn't really a good reason to use relative paths though. Quite on
> the contrary, because we sometimes use relative paths we are forced to
> register for chdir(3p) notifications via `chdir_notify_reparent()`.
>
With the previous changes added, we register via
`chdir_notify_register()`
> Adapt the function to always return absolute paths. This results in a
> user-visible change in behaviour where we now unconditionally print
> absolute paths in error messages. But arguably, that change in behaviour
> is acceptable and may even be good in cases where a Git command may end
> up accessing references across multiple different repositories.
>
> Furthermore, drop the calls to `chdir_notify_reparent()`, which aren't
> required anymore now that the paths are always absolute.
>
Same here, should be `chdir_notify_register()`
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> refs.c | 11 ++++++++---
> refs/files-backend.c | 22 ----------------------
> refs/packed-backend.c | 18 +-----------------
> refs/reftable-backend.c | 17 -----------------
> t/pack-refs-tests.sh | 6 +++---
> t/t0600-reffiles-backend.sh | 4 ++--
> t/t1423-ref-backend.sh | 9 ++++++---
> t/t5510-fetch.sh | 2 +-
> 8 files changed, 21 insertions(+), 68 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 4912510590..8679677bf7 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3579,15 +3579,16 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload,
> * worktree path, as the 'gitdir' here is already the worktree
> * path and is different from 'commondir' denoted by 'ref_common_dir'.
> */
> + strbuf_reset(refdir);
> strbuf_addstr(refdir, gitdir);
> - return;
> + goto out;
> }
>
> if (!is_absolute_path(payload)) {
> strbuf_addf(ref_common_dir, "/%s", payload);
> - strbuf_realpath(ref_common_dir, ref_common_dir->buf, 1);
> } else {
> - strbuf_realpath(ref_common_dir, payload, 1);
> + strbuf_reset(ref_common_dir);
> + strbuf_addstr(ref_common_dir, payload);
> }
>
> strbuf_addbuf(refdir, ref_common_dir);
> @@ -3598,4 +3599,8 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload,
> BUG("worktree path does not contain slash");
> strbuf_addf(refdir, "/worktrees/%s", wt_id + 1);
> }
> +
> +out:
> + strbuf_realpath(ref_common_dir, ref_common_dir->buf, 1);
> + strbuf_realpath(refdir, refdir->buf, 1);
> }
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 296981584b..762f392e67 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -21,7 +21,6 @@
> #include "../lockfile.h"
> #include "../path.h"
> #include "../dir.h"
> -#include "../chdir-notify.h"
> #include "../setup.h"
> #include "../worktree.h"
> #include "../wrapper.h"
> @@ -100,23 +99,6 @@ static void clear_loose_ref_cache(struct files_ref_store *refs)
> }
> }
>
> -static void files_ref_store_reparent(const char *name UNUSED,
> - const char *old_cwd,
> - const char *new_cwd,
> - void *payload)
> -{
> - struct files_ref_store *refs = payload;
> - char *tmp;
> -
> - tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
> - free(refs->base.gitdir);
> - refs->base.gitdir = tmp;
> -
> - tmp = reparent_relative_path(old_cwd, new_cwd, refs->gitcommondir);
> - free(refs->gitcommondir);
> - refs->gitcommondir = tmp;
> -}
> -
> /*
> * Create a new submodule ref cache and add it to the internal
> * set of caches.
> @@ -145,10 +127,7 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
>
> repo_config_get_bool(repo, "core.prefersymlinkrefs", &refs->prefer_symlink_refs);
>
> - chdir_notify_register(NULL, files_ref_store_reparent, refs);
> -
> strbuf_release(&refdir);
> -
> return ref_store;
> }
>
> @@ -197,7 +176,6 @@ static void files_ref_store_release(struct ref_store *ref_store)
> free(refs->gitcommondir);
> ref_store_release(refs->packed_ref_store);
> free(refs->packed_ref_store);
> - chdir_notify_unregister(NULL, files_ref_store_reparent, refs);
> }
>
> static void files_reflog_path(struct files_ref_store *refs,
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 499cb55dfa..89e41a35a3 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -13,7 +13,6 @@
> #include "packed-backend.h"
> #include "../iterator.h"
> #include "../lockfile.h"
> -#include "../chdir-notify.h"
> #include "../statinfo.h"
> #include "../worktree.h"
> #include "../wrapper.h"
> @@ -211,19 +210,6 @@ static size_t snapshot_hexsz(const struct snapshot *snapshot)
> return snapshot->refs->base.repo->hash_algo->hexsz;
> }
>
> -static void packed_ref_store_reparent(const char *name UNUSED,
> - const char *old_cwd,
> - const char *new_cwd,
> - void *payload)
> -{
> - struct packed_ref_store *refs = payload;
> - char *tmp;
> -
> - tmp = reparent_relative_path(old_cwd, new_cwd, refs->path);
> - free(refs->path);
> - refs->path = tmp;
> -}
> -
> /*
> * Since packed-refs is only stored in the common dir, don't parse the
> * payload and rely on the files-backend to set 'gitdir' correctly.
> @@ -239,10 +225,9 @@ struct ref_store *packed_ref_store_init(struct repository *repo,
>
> base_ref_store_init(ref_store, repo, gitdir, &refs_be_packed);
> refs->store_flags = opts->access_flags;
> -
> strbuf_addf(&sb, "%s/packed-refs", gitdir);
> refs->path = strbuf_detach(&sb, NULL);
> - chdir_notify_register(NULL, packed_ref_store_reparent, refs);
> +
> return ref_store;
> }
>
> @@ -287,7 +272,6 @@ static void packed_ref_store_release(struct ref_store *ref_store)
> clear_snapshot(refs);
> rollback_lock_file(&refs->lock);
> delete_tempfile(&refs->tempfile);
> - chdir_notify_unregister(NULL, packed_ref_store_reparent, refs);
> free(refs->path);
> }
>
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 8c93070677..8cc1dbbbdd 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -2,7 +2,6 @@
>
> #include "../git-compat-util.h"
> #include "../abspath.h"
> -#include "../chdir-notify.h"
> #include "../config.h"
> #include "../dir.h"
> #include "../environment.h"
> @@ -365,19 +364,6 @@ static int reftable_be_config(const char *var, const char *value,
> return 0;
> }
>
> -static void reftable_be_reparent(const char *name UNUSED,
> - const char *old_cwd,
> - const char *new_cwd,
> - void *payload)
> -{
> - struct reftable_ref_store *refs = payload;
> - char *tmp;
> -
> - tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
> - free(refs->base.gitdir);
> - refs->base.gitdir = tmp;
> -}
> -
> static struct ref_store *reftable_be_init(struct repository *repo,
> const char *payload,
> const char *gitdir,
> @@ -460,8 +446,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,
> goto done;
> }
>
> - chdir_notify_register(NULL, reftable_be_reparent, refs);
> -
> done:
> assert(refs->err != REFTABLE_API_ERROR);
> strbuf_release(&ref_common_dir);
> @@ -487,7 +471,6 @@ static void reftable_be_release(struct ref_store *ref_store)
> free(be);
> }
> strmap_clear(&refs->worktree_backends, 0);
> - chdir_notify_unregister(NULL, reftable_be_reparent, refs);
> }
>
> static int reftable_be_create_on_disk(struct ref_store *ref_store,
>
The changes look good to me.
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* [GIT PULL] git-gui: repo discovery with rev-parse; pick and gui subcommands; silent make -s
From: Johannes Sixt @ 2026-06-12 9:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
The following changes since commit bb52cdac6254c006e06bf0bb820268dcf024fc22:
git-gui: grey out comment lines in commit message (2026-03-04 08:04:37 +0100)
are available in the Git repository at:
https://github.com/j6t/git-gui.git master
for you to fetch changes up to 1b2c2a2edbaa1638becef4c3755b3e0633b9c304:
Merge branch 'ml/repo-discovery' (2026-06-12 11:05:28 +0200)
----------------------------------------------------------------
Harald Nordgren (1):
git-gui: silence install recipes under "make -s"
Johannes Sixt (2):
git-gui: remove unnecessary 'cd $_gitworktree' from do_gitk
Merge branch 'ml/repo-discovery'
Mark Levedahl (11):
git-gui: use HEAD as current branch when detached
git-gui: guard set/unset of GIT_DIR and GIT_WORK_TREE
git-gui: do not change global vars in choose_repository::pick
git-gui: use --absolute-git-dir
git-gui: use rev-parse exclusively to find a repository
git-gui: use git rev-parse for worktree discovery
git-gui: simplify [is_bare] to report if a worktree is known
git-gui: try harder to find worktree from gitdir
git-gui: allow specifying path '.' to the browser
git-gui: check browser/blame arguments carefully
git-gui: add gui and pick as explicit subcommands
Makefile | 6 +-
git-gui.sh | 376 ++++++++++++++++++++++++++--------------------
lib/choose_repository.tcl | 21 +--
3 files changed, 224 insertions(+), 179 deletions(-)
^ permalink raw reply
* [GIT PULL] gitk: horizontal scrollbar for commit list
From: Johannes Sixt @ 2026-06-12 9:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
The following changes since commit c8c5df79df34b40119c4bf8e3079520762f258d1:
Merge branch 'jx/i18n-fix' of github.com:jiangxin/gitk (2026-03-20 09:23:32 +0100)
are available in the Git repository at:
https://github.com/j6t/gitk.git master
for you to fetch changes up to bad83ada0ebf9e293d570e6e7ca4f1cd7877f482:
Merge branch 'horizontal-scroll' of github.com:ramcdona/gitk (2026-06-12 11:30:22 +0200)
----------------------------------------------------------------
Johannes Sixt (1):
Merge branch 'horizontal-scroll' of github.com:ramcdona/gitk
Rob McDonald (1):
gitk: add horizontal scrollbar to the commit list pane
gitk | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
^ permalink raw reply
* Re: [PATCH v2 5/7] environment: split up concerns of `is_bare_repository_cfg`
From: Patrick Steinhardt @ 2026-06-12 9:28 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Justin Tobler
In-Reply-To: <87wlw4yys1.fsf@emacs.iotcl.com>
On Fri, Jun 12, 2026 at 09:59:42AM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/setup.c b/setup.c
> > index 71fc6b33da..32f14a8688 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -795,10 +795,22 @@ static int check_repository_format_gently(const char *gitdir,
> > has_common = 0;
> > }
> >
> > - if (!has_common) {
> > - if (candidate->is_bare != -1)
> > - is_bare_repository_cfg = candidate->is_bare;
> > - } else {
> > + if (startup_info->force_bare_repository) {
> > + candidate->is_bare = 1;
> > + FREE_AND_NULL(candidate->work_tree);
> > + } else if (has_common) {
> > + /*
> > + * When sharing a common dir with another repository (e.g. a
> > + * linked worktree), do not let this repository's config
> > + * dictate bareness; it is inherited from the main worktree.
> > + */
> > + candidate->is_bare = -1;
> > +
> > + /*
> > + * Furthermore, "core.worktree" is supposed to be ignored when
> > + * we have a commondir configured, unless it comes from the
> > + * per-worktree configuration.
> > + */
> > FREE_AND_NULL(candidate->work_tree);
> > }
>
> Looking at the diff, this is really hard to understand. But your
> refactor makes sense and the after state is easier to comprehend.
Yeah, I'm in the same boat. Honestly, I really hope that our test suite
has enough coverage so that this refactoring won't cause any significant
regressions. Which isn't exactly a statement of confidence, but rather a
statement of "oh boy, this is awfully complex and has lots of weird edge
cases".
> > @@ -2571,7 +2584,7 @@ static int create_default_files(struct repository *repo,
> > repo_settings_set_shared_repository(repo,
> > init_shared_repository);
> >
> > - is_bare_repository_cfg = !work_tree;
> > + repo->bare_cfg = !work_tree;
>
> I'm curious, do we still need this? If I'm not mistaken, this function
> is called after check_and_apply_repository_format(), which calls
> apply_repository_format() and sets repo->bare_cfg too (see the diff
> above). Or is it explained by what Justin said[1]?
We can't drop this because after we've applied the format we call
`repo_config()` with `git_default_core_config()`, which will potentially
set the `bare_cfg` variable if "core.bare" is set.
Patrick
^ permalink raw reply
* Re: [PATCH v2 1/7] builtin/init: stop modifying global `git_work_tree_cfg` variable
From: Patrick Steinhardt @ 2026-06-12 9:27 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Justin Tobler
In-Reply-To: <87pl1wyyjw.fsf@emacs.iotcl.com>
On Fri, Jun 12, 2026 at 10:04:35AM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > When executing git-init(1) we need to figure out the final location of
> > the worktree. This location can be configured in a couple of ways: via
> > an environment variable, via the preexisting "core.worktree" config in
> > case we're reinitializing, or implicitly when reinitializing a non-bare
> > repository.
>
> Do you mean:
>
> > case we're reinitializing, or implicitly when initializing a non-bare
> > repository.
>
> So the second 'init' without the 're'?
>
> Obviously not worth a reroll on it's own.
It can actually happen in both cases. I've queued the following change
locally. Thanks!
Patrick
1: 0808dbb336 ! 1: cc6999257c builtin/init: stop modifying global `git_work_tree_cfg` variable
@@ Commit message
When executing git-init(1) we need to figure out the final location of
the worktree. This location can be configured in a couple of ways: via
an environment variable, via the preexisting "core.worktree" config in
- case we're reinitializing, or implicitly when reinitializing a non-bare
- repository.
+ case we're reinitializing, or implicitly when (re)initializing a
+ non-bare repository.
When checking for the worktree location in "builtin/init-db.c" we
populate any potentially-discovered value both by setting the global
^ permalink raw reply
* Re: [PATCH v2] commit-reach: remove get_reachable_subset()
From: Weijie Yuan @ 2026-06-12 9:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee, Kristofer Karlsson via GitGitGadget, git,
Kristofer Karlsson, Patrick Steinhardt
In-Reply-To: <xmqq7bo5nf31.fsf@gitster.g>
On Thu, Jun 11, 2026 at 10:48:18AM -0700, Junio C Hamano wrote:
> I wonder if we should talk about it in the SubmittingPatches and/or
> MyFirstContribution document?
Hi, I think it might be a good idea to cover these details in
MyFirstContribution, then cross-reference them from the part of
SubmittingPatches that discusses sending a new version.
More specifically, I think these details could fit around steps 3 and 4
of "A typical life cycle of a patch series" in SubmittingPatches,
starting around line 54. That section may need some reworking of the
existing wording, rather than just an additive change.
Also, for the part about sending a new version, I wonder whether it
would be better to summarize and fold in Patrick's explanation here,
thank you Patrick:
---
From: Patrick Steinhardt <ps@pks.im>
Message-ID: <aietF4BX1Ewt3cpG@pks.im>
> By the way, how long should I wait before sending new versions of my
> patches? I have 4 outstanding at the moment.
I typically aim to send at most one version per day per patch series.
This avoids that you're "flooding" the mailing list with too many
versions of the same series, allows you to address feedback from
multiple folks in batches, and it gives you enough time to think about
the feedback without having to rush anything.
Whether I actually do end up sending a series depends on a couple of
factors:
- How big is the series? The bigger it is the more time I give folks
to perform reviews.
- How substantial were the reviews you received? Is it just a couple
of small typos? Then it probably makes sense to wait one or two more
days to get some more involved reviews. Is it something that
requires signifciant rework? Then I'd send out soon so that others
don't review a patch series that will change significantly anyway.
- How close to being merged is the series? The closer it is the less
substantial the reviews will (hopefully) get, so it makes sense to
reroll a bit faster even if you only received minor feedback.
So there isn't really a golden rule to follow here, but a lot of this
depends on gut feeling. You probably won't have that feeling yet when
starting out in a new project, but that's fine. In case we see that
behaviour doesn't quite match the norm we'll typically give a hint that
the contributor should slow down or maybe send a new iteration.
Patrick
---
Patrick's point may be beyond the scope of this thread, so I only
mention it as an aside. Maybe these could be part of the same series.
Thanks.
^ permalink raw reply
* Re: [PATCH 6/9] repository: free main reference database
From: Karthik Nayak @ 2026-06-12 9:20 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-6-56c864b01c43@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> While we release worktree and submodule reference databases when
> clearing a repository, we don't ever release the main reference
> database. This memory leak went unnoticed because its pointer is
> kept alive by the "chdir_notify" subsystem.
>
> Fix the memory leak.
>
Funny, cause long ago I looked into this and thought I was clearly
missing something and eventually forgot about it. Good to know that I
was correct :)
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> repository.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/repository.c b/repository.c
> index 187dd471c4..e2b5c6712b 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -421,6 +421,11 @@ void repo_clear(struct repository *repo)
> FREE_AND_NULL(repo->remote_state);
> }
>
> + if (repo->refs_private) {
> + ref_store_release(repo->refs_private);
> + FREE_AND_NULL(repo->refs_private);
> + }
> +
> strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e)
> ref_store_release(e->value);
> strmap_clear(&repo->submodule_ref_stores, 1);
>
> --
> 2.54.0.1189.g8c84645362.dirty
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 4/9] refs: unregister reference stores from "chdir_notify"
From: Karthik Nayak @ 2026-06-12 9:18 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-4-56c864b01c43@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6726 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> When creating reference stores we register them with the "chdir_notify"
> subsystem. This is required because some of the paths we track may be
> relative paths, so we have to reparent them in case the current working
> directory changes.
>
> But while we register the reference stores, we never unregister them.
> This can have multiple outcomes:
>
> - For a repository's main reference database we essentially keep the
> pointer alive. We never free that database, either, and our leak
> checker doesn't notice because it's still registered.
>
> - For submodule and worktree reference databases we do eventually free
> them in `repo_clear()`, so we may keep pointers to free'd memory
> registered. We never notice though as we don't tend to chdir around
> in the middle of the process.
>
So `ref_store_release()` is what is called to release a ref_store. So
in the former's case, we never release the ref_store even if the
repository is closed, wow.
> We never noticed either of these symptoms, but they are obviously bad.
>
> Partially fix those issues by unregistering the reference stores when
> releasing them. The leak of the main reference database will be fixed in
> a subsequent commit.
>
> Note that this requires us to use `chdir_notify_register()` instead of
> `chdir_notify_parent()`, as there is no infrastructure to unregister the
Shouldn't this be s/chdir_notify_parent/chdir_notify_reparent ?
> latter. It ultimately doesn't matter much though: in a subsequent commit
> we'll drop this infrastructure completely. We merely require this step
> here so that we can fix the memory leaks ahead of time.
Right, we can't unregister when using `chdir_notify_reparent()` because
it internally calls `chdir_notify_register()` with a private cb
function, and we need to supply the callback function during
un-registering. Makes sense.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> refs/files-backend.c | 22 +++++++++++++++++++---
> refs/packed-backend.c | 16 +++++++++++++++-
> refs/reftable-backend.c | 16 +++++++++++++++-
> 3 files changed, 49 insertions(+), 5 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index a4c7858787..296981584b 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -100,6 +100,23 @@ static void clear_loose_ref_cache(struct files_ref_store *refs)
> }
> }
>
> +static void files_ref_store_reparent(const char *name UNUSED,
> + const char *old_cwd,
> + const char *new_cwd,
> + void *payload)
> +{
> + struct files_ref_store *refs = payload;
> + char *tmp;
> +
> + tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
> + free(refs->base.gitdir);
> + refs->base.gitdir = tmp;
> +
> + tmp = reparent_relative_path(old_cwd, new_cwd, refs->gitcommondir);
> + free(refs->gitcommondir);
> + refs->gitcommondir = tmp;
> +}
> +
Looks similar to `void reparent_cb()` but for both the directories.
> /*
> * Create a new submodule ref cache and add it to the internal
> * set of caches.
> @@ -128,9 +145,7 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
>
> repo_config_get_bool(repo, "core.prefersymlinkrefs", &refs->prefer_symlink_refs);
>
> - chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir);
> - chdir_notify_reparent("files-backend $GIT_COMMONDIR",
> - &refs->gitcommondir);
> + chdir_notify_register(NULL, files_ref_store_reparent, refs);
>
> strbuf_release(&refdir);
>
> @@ -182,6 +197,7 @@ static void files_ref_store_release(struct ref_store *ref_store)
> free(refs->gitcommondir);
> ref_store_release(refs->packed_ref_store);
> free(refs->packed_ref_store);
> + chdir_notify_unregister(NULL, files_ref_store_reparent, refs);
> }
>
> static void files_reflog_path(struct files_ref_store *refs,
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 0acde48c45..499cb55dfa 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -211,6 +211,19 @@ static size_t snapshot_hexsz(const struct snapshot *snapshot)
> return snapshot->refs->base.repo->hash_algo->hexsz;
> }
>
> +static void packed_ref_store_reparent(const char *name UNUSED,
> + const char *old_cwd,
> + const char *new_cwd,
> + void *payload)
> +{
> + struct packed_ref_store *refs = payload;
> + char *tmp;
> +
> + tmp = reparent_relative_path(old_cwd, new_cwd, refs->path);
> + free(refs->path);
> + refs->path = tmp;
> +}
> +
> /*
> * Since packed-refs is only stored in the common dir, don't parse the
> * payload and rely on the files-backend to set 'gitdir' correctly.
> @@ -229,7 +242,7 @@ struct ref_store *packed_ref_store_init(struct repository *repo,
>
> strbuf_addf(&sb, "%s/packed-refs", gitdir);
> refs->path = strbuf_detach(&sb, NULL);
> - chdir_notify_reparent("packed-refs", &refs->path);
> + chdir_notify_register(NULL, packed_ref_store_reparent, refs);
> return ref_store;
> }
>
> @@ -274,6 +287,7 @@ static void packed_ref_store_release(struct ref_store *ref_store)
> clear_snapshot(refs);
> rollback_lock_file(&refs->lock);
> delete_tempfile(&refs->tempfile);
> + chdir_notify_unregister(NULL, packed_ref_store_reparent, refs);
> free(refs->path);
> }
>
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 4ae22922de..8c93070677 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -365,6 +365,19 @@ static int reftable_be_config(const char *var, const char *value,
> return 0;
> }
>
> +static void reftable_be_reparent(const char *name UNUSED,
> + const char *old_cwd,
> + const char *new_cwd,
> + void *payload)
> +{
> + struct reftable_ref_store *refs = payload;
> + char *tmp;
> +
> + tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
> + free(refs->base.gitdir);
> + refs->base.gitdir = tmp;
> +}
> +
> static struct ref_store *reftable_be_init(struct repository *repo,
> const char *payload,
> const char *gitdir,
> @@ -447,7 +460,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,
> goto done;
> }
>
> - chdir_notify_reparent("reftables-backend $GIT_DIR", &refs->base.gitdir);
> + chdir_notify_register(NULL, reftable_be_reparent, refs);
>
> done:
> assert(refs->err != REFTABLE_API_ERROR);
> @@ -474,6 +487,7 @@ static void reftable_be_release(struct ref_store *ref_store)
> free(be);
> }
> strmap_clear(&refs->worktree_backends, 0);
> + chdir_notify_unregister(NULL, reftable_be_reparent, refs);
> }
>
> static int reftable_be_create_on_disk(struct ref_store *ref_store,
>
> --
> 2.54.0.1189.g8c84645362.dirty
The changes here look good.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ 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