* Re: [PATCH v2 3/3] replay: offer an option to linearize the commit topology
From: Elijah Newren @ 2026-06-14 6:56 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Johannes Schindelin
In-Reply-To: <20260610-toon-git-replay-drop-merges-v2-3-5714a71c6d83@iotcl.com>
Hi,
On Wed, Jun 10, 2026 at 7:51 AM Toon Claes <toon@iotcl.com> wrote:
>
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>
> One of the stated goals of git-replay(1) is to allow implementing the
> git-rebase(1) functionality on the server side.
>
> The default mode of git-rebase(1) is to act as if `--no-rebase-merges`
> was given. This mode drops merge commits instead of replaying them, and
> linearizes the commit history into a sequence of the
> regular (single-parent) commits.
>
> Add option `--linearize` to git-replay(1) to do the same.
I think this version is nicer overall than the one from my
replay-upstream branch; sorry for repeatedly getting distracted from
that, but this does look nice.
A few small comments:
> Co-authored-by: Toon Claes <toon@iotcl.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
> Documentation/git-replay.adoc | 5 +++++
> builtin/replay.c | 4 ++++
> replay.c | 30 +++++++++++++++++++++++-------
> replay.h | 5 +++++
> t/t3650-replay-basics.sh | 26 ++++++++++++++++++++++++++
> 5 files changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
> index a32f72aead..41c96c7061 100644
> --- a/Documentation/git-replay.adoc
> +++ b/Documentation/git-replay.adoc
> @@ -88,6 +88,11 @@ incompatible with `--contained` (which is a modifier for `--onto` only).
> +
> The default mode can be configured via the `replay.refAction` configuration variable.
>
> +--linearize::
> + In this mode, `git replay` imitates `git rebase --no-rebase-merges`,
> + i.e. it cherry-picks only non-merge commits, each one on top of the
> + previous one.
The SYNOPSIS block at the top of the file is missing this new flag.
The replay_usage[] variable in cmd_replay is also missing this new flag.
> <revision-range>::
> Range of commits to replay; see "Specifying Ranges" in
> linkgit:git-rev-parse[1]. In `--advance=<branch>` or
> diff --git a/builtin/replay.c b/builtin/replay.c
> index 39e3a86f6c..fedfe46dc6 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -111,6 +111,8 @@ int cmd_replay(int argc,
> N_("mode"),
> N_("control ref update behavior (update|print)"),
> PARSE_OPT_NONEG),
> + OPT_BOOL(0, "linearize", &opts.linearize,
> + N_("ignore merge commits instead of replaying them")),
"ignore" feels a bit ambiguous to me. Can we use "drop" instead,
matching your commit message?
> OPT_END()
> };
>
> @@ -132,6 +134,8 @@ int cmd_replay(int argc,
> opts.contained, "--contained");
> die_for_incompatible_opt2(!!opts.ref, "--ref",
> !!opts.contained, "--contained");
> + die_for_incompatible_opt2(!!opts.revert, "--revert",
> + opts.linearize, "--linearize");
Sensible; should the docs mention this incompatibility? (I'm not sure
myself; just throwing it out as food for thought.)
>
> /* Parse ref action mode from command line or config */
> ref_mode = get_ref_action_mode(repo, ref_action);
> diff --git a/replay.c b/replay.c
> index 7921d7dba3..81033fb889 100644
> --- a/replay.c
> +++ b/replay.c
> @@ -277,12 +277,16 @@ static struct commit *pick_regular_commit(struct repository *repo,
> struct commit *onto,
> struct merge_options *merge_opt,
> struct merge_result *result,
> + struct commit *replayed_base,
> bool reverse,
> enum replay_empty_commit_action empty)
> {
> - struct commit *base, *replayed_base;
> + struct commit *base;
> struct tree *pickme_tree, *base_tree, *replayed_base_tree;
>
> + if (replayed_base && reverse)
> + BUG("Linearizing commits is not supported when replaying in reverse");
> +
This is dead code given the die_for_incompatible_opt2 check above,
right? Just extra defense in depth?
> if (pickme->parents) {
> base = pickme->parents->item;
> base_tree = repo_get_commit_tree(repo, base);
> @@ -291,7 +295,8 @@ static struct commit *pick_regular_commit(struct repository *repo,
> base_tree = lookup_tree(repo, repo->hash_algo->empty_tree);
> }
>
> - replayed_base = get_mapped_commit(replayed_commits, base, onto);
> + if (!replayed_base)
> + replayed_base = get_mapped_commit(replayed_commits, base, onto);
> replayed_base_tree = repo_get_commit_tree(repo, replayed_base);
> pickme_tree = repo_get_commit_tree(repo, pickme);
>
> @@ -430,12 +435,23 @@ int replay_revisions(struct rev_info *revs,
> while ((commit = get_revision(revs))) {
> const struct name_decoration *decoration;
>
> - if (commit->parents && commit->parents->next)
> - die(_("replaying merge commits is not supported yet!"));
> + if (commit->parents && commit->parents->next) {
> + if (!opts->linearize)
> + die(_("replaying merge commits is not supported yet!"));
> + /*
> + * When linearizing, a merge commit itself is not picked,
> + * but refs that point to it might need updating.
> + */
Is it worth pointing out that last_commit is intentionally not updated
by this code path? That is implied by your comment, but it takes a
bit of reasoning to get there, and I think it might help future
readers to just explicitly state it.
> + } else {
> + struct commit *to_pick = reverse ? last_commit : onto;
> + last_commit =
> + pick_regular_commit(revs->repo, commit,
> + replayed_commits, to_pick,
> + &merge_opt, &result,
> + opts->linearize ? last_commit : NULL,
> + reverse, opts->empty);
> + }
>
> - last_commit = pick_regular_commit(revs->repo, commit, replayed_commits,
> - reverse ? last_commit : onto,
> - &merge_opt, &result, reverse, opts->empty);
> if (!last_commit)
> break;
>
> diff --git a/replay.h b/replay.h
> index 1851a07705..07e6fdcca3 100644
> --- a/replay.h
> +++ b/replay.h
> @@ -62,6 +62,11 @@ struct replay_revisions_options {
> * Defaults to REPLAY_EMPTY_COMMIT_DROP.
> */
> enum replay_empty_commit_action empty;
> +
> + /*
> + * Whether to linearize the commits (i.e. drop merge commits).
> + */
> + int linearize;
> };
>
> /* This struct is used as an out-parameter by `replay_revisions()`. */
> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> index 3353bc4a4d..64e0731188 100755
> --- a/t/t3650-replay-basics.sh
> +++ b/t/t3650-replay-basics.sh
> @@ -565,4 +565,30 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '
> test_grep "cannot be used with multiple revision ranges" err
> '
>
> +test_expect_success 'replay merge commit fails' '
> + echo "fatal: replaying merge commits is not supported yet!" >expect &&
> + test_must_fail git replay --ref-action=print --onto main I..P 2>actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'replay to rebase merge commit with --linearize' '
> + git replay --ref-action=print --linearize --onto main I..topic-with-merge >result &&
> +
> + test_line_count = 1 result &&
> +
> + git log --format=%s $(cut -f 3 -d " " result) >actual &&
> + test_write_lines O N J M L B A >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'replay to rebase merge commit with --linearize down to root commit' '
> + git replay --ref-action=print --linearize --onto main A..topic-with-merge >result &&
You'd need to drop "A.." to have it go down to the root commit, as
Junio mentioned elsewhere.
> +
> + test_line_count = 1 result &&
> +
> + git log --format=%s $(cut -f 3 -d " " result) >actual &&
> + test_write_lines O N J I M L B A >expect &&
> + test_cmp expect actual
> +'
> +
> test_done
Should there also be a testcase combining --linearize and --advance?
Should there be a test with the incompatibility of --revert &
--linearize? I think we have a few other tests for incompatible
options.
One additional testing idea, borrowed from an older variant of
this patch I had sitting in a local branch (dscho's original
linearize patch, adapted): in addition to checking specific commit
subjects, it's worth verifying that the linearized chain produces
the *same patches* as the original. Something along the lines of:
test_expect_success '--linearize preserves patches' '
test_when_finished "git update-ref -d refs/heads/merge_I_L" &&
test_tick &&
git checkout -b merge_I_L I &&
git merge --no-edit L &&
git replay --linearize --onto A B..merge_I_L &&
# range-diff ignores merges, so the original
# {I, L, merge} reduces to {I, L} on the LHS,
# and the replayed chain on the RHS should match.
git range-diff B..merge_I_L@{1} B..merge_I_L >out &&
! test_grep -v "=" out &&
git log --oneline A..merge_I_L >out &&
test_line_count = 2 out
'
The range-diff check is nice because it asserts patch equivalence
rather than tying the test to a particular replay ordering, which
makes the test less brittle if the rev-walk order ever changes.
Feel free to take, adapt, or ignore.
Anyway, thanks for working on this; looking good.
Elijah
^ permalink raw reply
* [PATCH v2 5/5] cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts
From: Elijah Newren via GitGitGadget @ 2026-06-14 6:37 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Christian Couder, Elijah Newren,
Elijah Newren, Elijah Newren
In-Reply-To: <pull.2096.v2.git.1781419047.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
verify_cache() checks that the index does not contain both "path" and
"path/file" before writing a tree. It does this by comparing only
adjacent entries, relying on the assumption that "path/file" would
immediately follow "path" in sorted order. Unfortunately, this
assumption does not always hold. For example:
docs <-- submodule entry
docs-internal/README.md <-- intervening entry
docs/requirements.txt <-- D/F conflict, NOT adjacent to "docs"
When this happens, verify_cache() silently misses the D/F conflict and
write-tree produces a corrupt tree object containing duplicate entries
(one for the submodule "docs" and one for the tree "docs").
I could not find any caller in current git that both allows the index to
get into this state and then tries to write it out without doing other
checks beyond the verify_cache() call in cache_tree_update(), but
verify_cache() is documented as a safety net for preventing corrupt
trees and should actually provide that guarantee. A downstream consumer
that relied solely on cache_tree_update()'s internal checking via
verify_cache() to prevent duplicate tree entries was bitten by the gap.
Add a test that constructs a corrupt index directly (bypassing the D/F
checks in add_index_entry) and verifies that write-tree now rejects it.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
cache-tree.c | 64 ++++++++++++++++++++++++++++------
t/meson.build | 1 +
t/t0093-direct-index-write.pl | 38 ++++++++++++++++++++
t/t0093-verify-cache-df-gap.sh | 59 +++++++++++++++++++++++++++++++
4 files changed, 151 insertions(+), 11 deletions(-)
create mode 100644 t/t0093-direct-index-write.pl
create mode 100755 t/t0093-verify-cache-df-gap.sh
diff --git a/cache-tree.c b/cache-tree.c
index 7881b42aa2..4d2669b312 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -161,6 +161,54 @@ void cache_tree_invalidate_path(struct index_state *istate, const char *path)
istate->cache_changed |= CACHE_TREE_CHANGED;
}
+/*
+ * Check whether this_ce and the next entry in the index form a D/F
+ * conflict ("path" vs "path/file"). Returns the conflicting "path/..."
+ * name when one is found, or NULL otherwise.
+ *
+ * The cache is sorted, so "path/file" sorts after "path" and the
+ * conflict is usually visible as adjacent entries. But other entries
+ * can sort between them -- e.g. "path-internal" sits between "path"
+ * and "path/file" because '-' (0x2D) precedes '/' (0x2F) -- so when
+ * the immediately following entry shares our prefix but starts with a
+ * character that sorts before '/', binary search for "path/" instead.
+ */
+static const char *find_df_conflict(struct index_state *istate,
+ const struct cache_entry *this_ce,
+ const struct cache_entry *next_ce)
+{
+ const char *this_name = this_ce->name;
+ const char *next_name = next_ce->name;
+ int this_len = ce_namelen(this_ce);
+ const struct cache_entry *other;
+ struct strbuf probe = STRBUF_INIT;
+ int pos;
+
+ if (this_len >= ce_namelen(next_ce) ||
+ next_name[this_len] > '/' ||
+ strncmp(this_name, next_name, this_len))
+ return NULL;
+
+ if (next_name[this_len] == '/')
+ return next_name;
+
+ strbuf_add(&probe, this_name, this_len);
+ strbuf_addch(&probe, '/');
+ pos = index_name_pos_sparse(istate, probe.buf, probe.len);
+ strbuf_release(&probe);
+
+ if (pos < 0)
+ pos = -pos - 1;
+ if (pos >= (int)istate->cache_nr)
+ return NULL;
+ other = istate->cache[pos];
+ if (ce_namelen(other) > this_len &&
+ other->name[this_len] == '/' &&
+ !strncmp(this_name, other->name, this_len))
+ return other->name;
+ return NULL;
+}
+
static int verify_cache(struct index_state *istate, int flags)
{
unsigned i, funny;
@@ -190,24 +238,18 @@ static int verify_cache(struct index_state *istate, int flags)
*/
funny = 0;
for (i = 0; i + 1 < istate->cache_nr; i++) {
- /* path/file always comes after path because of the way
- * the cache is sorted. Also path can appear only once,
- * which means conflicting one would immediately follow.
- */
const struct cache_entry *this_ce = istate->cache[i];
const struct cache_entry *next_ce = istate->cache[i + 1];
- const char *this_name = this_ce->name;
- const char *next_name = next_ce->name;
- int this_len = ce_namelen(this_ce);
- if (this_len < ce_namelen(next_ce) &&
- next_name[this_len] == '/' &&
- strncmp(this_name, next_name, this_len) == 0) {
+ const char *conflict_name;
+
+ conflict_name = find_df_conflict(istate, this_ce, next_ce);
+ if (conflict_name) {
if (10 < ++funny) {
fprintf(stderr, "...\n");
break;
}
fprintf(stderr, "You have both %s and %s\n",
- this_name, next_name);
+ this_ce->name, conflict_name);
}
}
if (funny)
diff --git a/t/meson.build b/t/meson.build
index 7528e5cda5..362177999b 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -124,6 +124,7 @@ integration_tests = [
't0090-cache-tree.sh',
't0091-bugreport.sh',
't0092-diagnose.sh',
+ 't0093-verify-cache-df-gap.sh',
't0095-bloom.sh',
't0100-previous.sh',
't0101-at-syntax.sh',
diff --git a/t/t0093-direct-index-write.pl b/t/t0093-direct-index-write.pl
new file mode 100644
index 0000000000..2881a3ebb2
--- /dev/null
+++ b/t/t0093-direct-index-write.pl
@@ -0,0 +1,38 @@
+#!/usr/bin/perl
+#
+# Build a v2 index file from entries listed on stdin.
+# Each line: "octalmode hex-oid name"
+# Output: binary index written to stdout.
+#
+# This bypasses all D/F safety checks in add_index_entry(), simulating
+# what happens when code uses ADD_CACHE_JUST_APPEND to bulk-load entries.
+use strict;
+use warnings;
+use Digest::SHA qw(sha1 sha256);
+
+my $hash_algo = $ENV{'GIT_DEFAULT_HASH'} || 'sha1';
+my $hash_func = $hash_algo eq 'sha256' ? \&sha256 : \&sha1;
+
+my @entries;
+while (my $line = <STDIN>) {
+ chomp $line;
+ my ($mode, $oid_hex, $name) = split(/ /, $line, 3);
+ push @entries, [$mode, $oid_hex, $name];
+}
+
+my $body = "DIRC" . pack("NN", 2, scalar @entries);
+
+for my $ent (@entries) {
+ my ($mode, $oid_hex, $name) = @{$ent};
+ # 10 x 32-bit stat fields (zeroed), with mode in position 7
+ my $stat = pack("N10", 0, 0, 0, 0, 0, 0, oct($mode), 0, 0, 0);
+ my $oid = pack("H*", $oid_hex);
+ my $flags = pack("n", length($name) & 0xFFF);
+ my $entry = $stat . $oid . $flags . $name . "\0";
+ # Pad to 8-byte boundary
+ while (length($entry) % 8) { $entry .= "\0"; }
+ $body .= $entry;
+}
+
+binmode STDOUT;
+print $body . $hash_func->($body);
diff --git a/t/t0093-verify-cache-df-gap.sh b/t/t0093-verify-cache-df-gap.sh
new file mode 100755
index 0000000000..0b6829d805
--- /dev/null
+++ b/t/t0093-verify-cache-df-gap.sh
@@ -0,0 +1,59 @@
+#!/bin/sh
+
+test_description='verify_cache() must catch non-adjacent D/F conflicts
+
+Ensure that verify_cache() can complain about bad entries like:
+
+ docs <-- submodule
+ docs-internal/... <-- sorts here because "-" < "/"
+ docs/... <-- D/F conflict with "docs" above, not adjacent
+
+In order to test verify_cache, we directly construct a corrupt index
+(bypassing the D/F safety checks in add_index_entry) and verify that
+write-tree rejects it.
+'
+
+. ./test-lib.sh
+
+if ! test_have_prereq PERL
+then
+ skip_all='skipping verify_cache D/F tests; Perl not available'
+ test_done
+fi
+
+# Build a v2 index from entries on stdin, bypassing D/F checks.
+# Each line: "octalmode hex-oid name" (entries must be pre-sorted).
+build_corrupt_index () {
+ perl "$TEST_DIRECTORY/t0093-direct-index-write.pl" >"$1"
+}
+
+test_expect_success 'setup objects' '
+ test_commit base &&
+ BLOB=$(git rev-parse HEAD:base.t) &&
+ SUB_COMMIT=$(git rev-parse HEAD)
+'
+
+test_expect_success 'adjacent D/F conflict is caught by verify_cache' '
+ cat >index-entries <<-EOF &&
+ 0160000 $SUB_COMMIT docs
+ 0100644 $BLOB docs/requirements.txt
+ EOF
+ build_corrupt_index .git/index <index-entries &&
+
+ test_must_fail git write-tree 2>err &&
+ test_grep "You have both docs and docs/requirements.txt" err
+'
+
+test_expect_success 'non-adjacent D/F conflict is caught by verify_cache' '
+ cat >index-entries <<-EOF &&
+ 0160000 $SUB_COMMIT docs
+ 0100644 $BLOB docs-internal/README.md
+ 0100644 $BLOB docs/requirements.txt
+ EOF
+ build_corrupt_index .git/index <index-entries &&
+
+ test_must_fail git write-tree 2>err &&
+ test_grep "You have both docs and docs/requirements.txt" err
+'
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 4/5] merge-ort: abort merge when trees have duplicate entries
From: Elijah Newren via GitGitGadget @ 2026-06-14 6:37 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Christian Couder, Elijah Newren,
Elijah Newren, Elijah Newren
In-Reply-To: <pull.2096.v2.git.1781419047.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
Trees with duplicate entries are malformed; fsck reports "contains
duplicate file entries" for them. merge-ort has from the beginning
assumed that we would never hit such trees. It was written with the
assumption that traverse_trees() calls collect_merge_info_callback() at
most once per path. The "sanity checks" in that callback (added in
d2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),
2020-12-13)) verify properties of each individual call but not that
invariant. The strmap_put() in setup_path_info() silently overwrites
the entry from any prior call for the same path, because it assumed
there would be no other path. Unfortunately, supplemental data
structures for various optimizations could still be tweaked before the
extra paths were overwritten, and those data structures not matching
expected state could trip various assertions.
Change the return type of setup_path_info() from void to int to allow us
to detect this case, and abort the merge with a clear error message when
it occurs.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-ort.c | 61 ++++++++++++++++------------
t/t6422-merge-rename-corner-cases.sh | 54 ++++++++++++++++++++++++
2 files changed, 88 insertions(+), 27 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 8f911cb639..be0829bbb7 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1026,18 +1026,18 @@ static int traverse_trees_wrapper(struct index_state *istate,
return ret < 0 ? ret : 0;
}
-static void setup_path_info(struct merge_options *opt,
- struct string_list_item *result,
- const char *current_dir_name,
- int current_dir_name_len,
- char *fullpath, /* we'll take over ownership */
- struct name_entry *names,
- struct name_entry *merged_version,
- unsigned is_null, /* boolean */
- unsigned df_conflict, /* boolean */
- unsigned filemask,
- unsigned dirmask,
- int resolved /* boolean */)
+static int setup_path_info(struct merge_options *opt,
+ struct string_list_item *result,
+ const char *current_dir_name,
+ int current_dir_name_len,
+ char *fullpath, /* we'll take over ownership */
+ struct name_entry *names,
+ struct name_entry *merged_version,
+ unsigned is_null, /* boolean */
+ unsigned df_conflict, /* boolean */
+ unsigned filemask,
+ unsigned dirmask,
+ int resolved /* boolean */)
{
/* result->util is void*, so mi is a convenience typed variable */
struct merged_info *mi;
@@ -1081,9 +1081,11 @@ static void setup_path_info(struct merge_options *opt,
*/
mi->is_null = 1;
}
- strmap_put(&opt->priv->paths, fullpath, mi);
+ if (strmap_put(&opt->priv->paths, fullpath, mi))
+ return error(_("tree has duplicate entries for '%s'"), fullpath);
result->string = fullpath;
result->util = mi;
+ return 0;
}
static void add_pair(struct merge_options *opt,
@@ -1350,9 +1352,10 @@ static int collect_merge_info_callback(int n,
*/
if (side1_matches_mbase && side2_matches_mbase) {
/* mbase, side1, & side2 all match; use mbase as resolution */
- setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
- names, names+0, mbase_null, 0 /* df_conflict */,
- filemask, dirmask, 1 /* resolved */);
+ if (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
+ names, names+0, mbase_null, 0 /* df_conflict */,
+ filemask, dirmask, 1 /* resolved */))
+ return -1; /* Quit traversing */
return mask;
}
@@ -1364,9 +1367,10 @@ static int collect_merge_info_callback(int n,
*/
if (sides_match && filemask == 0x07) {
/* use side1 (== side2) version as resolution */
- setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
- names, names+1, side1_null, 0,
- filemask, dirmask, 1);
+ if (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
+ names, names+1, side1_null, 0,
+ filemask, dirmask, 1))
+ return -1; /* Quit traversing */
return mask;
}
@@ -1378,18 +1382,20 @@ static int collect_merge_info_callback(int n,
*/
if (side1_matches_mbase && filemask == 0x07) {
/* use side2 version as resolution */
- setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
- names, names+2, side2_null, 0,
- filemask, dirmask, 1);
+ if (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
+ names, names+2, side2_null, 0,
+ filemask, dirmask, 1))
+ return -1; /* Quit traversing */
return mask;
}
/* Similar to above but swapping sides 1 and 2 */
if (side2_matches_mbase && filemask == 0x07) {
/* use side1 version as resolution */
- setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
- names, names+1, side1_null, 0,
- filemask, dirmask, 1);
+ if (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
+ names, names+1, side1_null, 0,
+ filemask, dirmask, 1))
+ return -1; /* Quit traversing */
return mask;
}
@@ -1413,8 +1419,9 @@ static int collect_merge_info_callback(int n,
* unconflict some more cases, but that comes later so all we can
* do now is record the different non-null file hashes.)
*/
- setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
- names, NULL, 0, df_conflict, filemask, dirmask, 0);
+ if (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
+ names, NULL, 0, df_conflict, filemask, dirmask, 0))
+ return -1; /* Quit traversing */
ci = pi.util;
VERIFY_CI(ci);
diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh
index e18d5a227d..81b645bb3b 100755
--- a/t/t6422-merge-rename-corner-cases.sh
+++ b/t/t6422-merge-rename-corner-cases.sh
@@ -1525,4 +1525,58 @@ test_expect_success 'submodule/directory preliminary conflict' '
)
'
+# Testcase: submodule/directory conflict with duplicate tree entries
+# One side has a path as a gitlink (submodule). The other side replaces
+# the gitlink with a directory. A third-party tool creates a tree on the
+# submodule side that has *both* a gitlink and a tree entry for the same
+# path (adding a file inside the submodule path ignoring that there's a
+# gitlink there). collect_merge_info_callback() should detect the
+# duplicate and abort rather than silently corrupting its bookkeeping.
+
+test_expect_success 'duplicate tree entries trigger an error' '
+ test_when_finished "rm -rf duplicate-entry" &&
+ git init duplicate-entry &&
+ (
+ cd duplicate-entry &&
+
+ # Base commit: "docs" is a gitlink (submodule)
+ empty_tree=$(git mktree </dev/null) &&
+ fake_commit=$(git commit-tree $empty_tree </dev/null) &&
+ git update-index --add --cacheinfo 160000,$fake_commit,docs &&
+ echo base >file.txt &&
+ git add file.txt &&
+ git commit -m base &&
+
+ # side1: remove the gitlink, replace with a directory
+ git checkout -b side1 &&
+ git rm --cached docs &&
+ mkdir -p docs &&
+ echo hello >docs/requirements.txt &&
+ git add docs/requirements.txt &&
+ git commit -m "side1: submodule to directory" &&
+
+ # side2: keep the gitlink but craft a tree that also
+ # contains a tree entry for "docs" (simulating a tool
+ # that adds files inside a submodule path without
+ # removing the gitlink first).
+ git checkout main &&
+ git checkout -b side2 &&
+ blob_oid=$(echo world | git hash-object -w --stdin) &&
+ docs_tree=$(printf "100644 blob %s\trequirements.txt\n" \
+ "$blob_oid" | git mktree) &&
+ cur_tree=$(git rev-parse HEAD^{tree}) &&
+ git cat-file -p $cur_tree >tree-listing &&
+ printf "040000 tree %s\tdocs\n" "$docs_tree" >>tree-listing &&
+ new_tree=$(git mktree <tree-listing) &&
+ side2_commit=$(git commit-tree $new_tree -p HEAD \
+ -m "side2: add file alongside submodule") &&
+ git update-ref refs/heads/side2 $side2_commit &&
+
+ # Merging must detect the duplicate and abort
+ git checkout side1 &&
+ test_must_fail git merge side2 2>err &&
+ test_grep "duplicate entries" err
+ )
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 3/5] merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()
From: Elijah Newren via GitGitGadget @ 2026-06-14 6:37 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Christian Couder, Elijah Newren,
Elijah Newren, Elijah Newren
In-Reply-To: <pull.2096.v2.git.1781419047.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
clear_or_reinit_internal_opts() is responsible for cleaning up the
various data structures in merge_options_internal. It already handles
many renames-related structures (dirs_removed, dir_renames,
relevant_sources, cached_pairs, deferred, etc.) but does not free
renames->pairs[].queue.
In the normal code path, resolve_and_process_renames() frees
pairs[s].queue and reinitializes it with diff_queue_init() before
clear_or_reinit_internal_opts() runs, so the omission is harmless.
However, if collect_merge_info() encounters an error and returns early
(before resolve_and_process_renames() is ever called), any diff pairs
already queued by collect_rename_info()/add_pair() will have their
backing array leaked.
Fix this by freeing renames->pairs[].queue in the cleanup function.
In the normal path the pointer is already NULL (from the earlier
diff_queue_init() in resolve_and_process_renames()), so free(NULL) is
a safe no-op.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-ort.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/merge-ort.c b/merge-ort.c
index 74e9636020..8f911cb639 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -728,6 +728,8 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
strintmap_clear_func(&renames->deferred[i].possible_trivial_merges);
strset_clear_func(&renames->deferred[i].target_dirs);
renames->deferred[i].trivial_merges_okay = 1; /* 1 == maybe */
+ free(renames->pairs[i].queue);
+ diff_queue_init(&renames->pairs[i]);
}
renames->cached_pairs_valid_side = 0;
renames->dir_rename_mask = 0;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/5] merge-ort: drop unnecessary show_all_errors from collect_merge_info()
From: Elijah Newren via GitGitGadget @ 2026-06-14 6:37 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Christian Couder, Elijah Newren,
Elijah Newren, Elijah Newren
In-Reply-To: <pull.2096.v2.git.1781419047.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
collect_merge_info() has set info.show_all_errors = 1 since
d2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),
2020-12-13). This setting was copied from unpack-trees.c where it
controls batching of error messages for porcelain display, but
merge-ort has no such error-batching logic and never needed it.
With show_all_errors set, traverse_trees() captures a negative callback
return but continues processing remaining entries rather than stopping
immediately. Removing the setting restores the default behavior where
a negative return from collect_merge_info_callback() breaks out of the
traversal loop right away, allowing a future commit to exit early when
a corrupt tree is detected.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-ort.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/merge-ort.c b/merge-ort.c
index 4b8e32209d..74e9636020 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1740,7 +1740,6 @@ static int collect_merge_info(struct merge_options *opt,
setup_traverse_info(&info, opt->priv->toplevel_dir);
info.fn = collect_merge_info_callback;
info.data = opt;
- info.show_all_errors = 1;
if (repo_parse_tree(opt->repo, merge_base) < 0 ||
repo_parse_tree(opt->repo, side1) < 0 ||
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 1/5] merge-ort: propagate callback errors from traverse_trees_wrapper()
From: Elijah Newren via GitGitGadget @ 2026-06-14 6:37 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Christian Couder, Elijah Newren,
Elijah Newren, Elijah Newren
In-Reply-To: <pull.2096.v2.git.1781419047.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
traverse_trees_wrapper() saves entries from a first pass through
traverse_trees() and then replays them through the real callback
(collect_merge_info_callback). However, the replay loop silently
discards the callback return value. This is not a deferred error;
it is an ignored error.
Today the only originator of a negative return in this entire call
graph is traverse_trees()'s "exceeded maximum allowed tree depth"
check; everything else (collect_merge_info_callback,
traverse_trees_wrapper, the inner traverse_trees recursion) only
relays that. So in current Git, the visible effect of dropping the
replay callback's return value is narrow but bad: a tree nested past
core.maxTreeDepth has its -1 swallowed, the subtree below the limit
is silently pruned, and the merge completes as if that were the
correct result.
A later patch in this series will teach collect_merge_info_callback()
to return -1 on an additional path -- detecting duplicate
entries in malformed trees -- which is similarly handled today by
just ignoring the problem (resulting in mostly a "last one wins" rule,
though the non-last entry can mutate various state flags).
Capture the return value, stop the loop on negative returns, and
propagate the error to the caller. The callback returns a positive mask
value on success, so normalize non-negative returns to
0 for the caller.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-ort.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 00923ce3cd..4b8e32209d 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1008,18 +1008,20 @@ static int traverse_trees_wrapper(struct index_state *istate,
info->traverse_path = renames->callback_data_traverse_path;
info->fn = old_fn;
for (i = old_offset; i < renames->callback_data_nr; ++i) {
- info->fn(n,
- renames->callback_data[i].mask,
- renames->callback_data[i].dirmask,
- renames->callback_data[i].names,
- info);
+ ret = info->fn(n,
+ renames->callback_data[i].mask,
+ renames->callback_data[i].dirmask,
+ renames->callback_data[i].names,
+ info);
+ if (ret < 0)
+ break;
}
renames->callback_data_nr = old_offset;
free(renames->callback_data_traverse_path);
renames->callback_data_traverse_path = old_callback_data_traverse_path;
info->traverse_path = NULL;
- return 0;
+ return ret < 0 ? ret : 0;
}
static void setup_path_info(struct merge_options *opt,
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/5] Duplicate entry hardening
From: Elijah Newren via GitGitGadget @ 2026-06-14 6:37 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Christian Couder, Elijah Newren,
Elijah Newren
In-Reply-To: <pull.2096.git.1776731171.gitgitgadget@gmail.com>
We had some corrupt trees with duplicate entries in real world repositories,
which triggered an assertion failure in merge-ort. Further, the corrupt tree
creation in the third party tool would have been avoided had verify_cache()
correctly checked for D/F conflicts. Provide fixes for both issues,
including 3 preparatory changes for the merge-ort fix.
Changes since v1:
* Add some more detail to the commit message of Patch 1
* Split some code out in Patch 5 into a helper function.
Elijah Newren (5):
merge-ort: propagate callback errors from traverse_trees_wrapper()
merge-ort: drop unnecessary show_all_errors from collect_merge_info()
merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()
merge-ort: abort merge when trees have duplicate entries
cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts
cache-tree.c | 64 +++++++++++++++++++----
merge-ort.c | 78 ++++++++++++++++------------
t/meson.build | 1 +
t/t0093-direct-index-write.pl | 38 ++++++++++++++
t/t0093-verify-cache-df-gap.sh | 59 +++++++++++++++++++++
t/t6422-merge-rename-corner-cases.sh | 54 +++++++++++++++++++
6 files changed, 249 insertions(+), 45 deletions(-)
create mode 100644 t/t0093-direct-index-write.pl
create mode 100755 t/t0093-verify-cache-df-gap.sh
base-commit: e8955061076952cc5eab0300424fc48b601fe12d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2096%2Fnewren%2Fduplicate-entry-hardening-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2096/newren/duplicate-entry-hardening-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2096
Range-diff vs v1:
1: 282f906d1b ! 1: dc0f596f31 merge-ort: propagate callback errors from traverse_trees_wrapper()
@@ Commit message
traverse_trees_wrapper() saves entries from a first pass through
traverse_trees() and then replays them through the real callback
(collect_merge_info_callback). However, the replay loop silently
- discards the callback return value. This means any error reported by
- the callback during replay -- including a future check for malformed
- trees -- would be ignored, allowing the merge to proceed with corrupt
- state.
+ discards the callback return value. This is not a deferred error;
+ it is an ignored error.
- Capture the return value, stop the loop on negative (error) returns,
- and propagate the error to the caller. Note that the callback returns
- a positive mask value on success, so we normalize non-negative returns
- to 0 for the caller.
+ Today the only originator of a negative return in this entire call
+ graph is traverse_trees()'s "exceeded maximum allowed tree depth"
+ check; everything else (collect_merge_info_callback,
+ traverse_trees_wrapper, the inner traverse_trees recursion) only
+ relays that. So in current Git, the visible effect of dropping the
+ replay callback's return value is narrow but bad: a tree nested past
+ core.maxTreeDepth has its -1 swallowed, the subtree below the limit
+ is silently pruned, and the merge completes as if that were the
+ correct result.
+
+ A later patch in this series will teach collect_merge_info_callback()
+ to return -1 on an additional path -- detecting duplicate
+ entries in malformed trees -- which is similarly handled today by
+ just ignoring the problem (resulting in mostly a "last one wins" rule,
+ though the non-last entry can mutate various state flags).
+
+ Capture the return value, stop the loop on negative returns, and
+ propagate the error to the caller. The callback returns a positive mask
+ value on success, so normalize non-negative returns to
+ 0 for the caller.
Signed-off-by: Elijah Newren <newren@gmail.com>
2: 949b5d8e3f = 2: b4ff725a77 merge-ort: drop unnecessary show_all_errors from collect_merge_info()
3: d422f73e12 = 3: 673fbea13f merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()
4: 0d81c027aa = 4: b5421244d4 merge-ort: abort merge when trees have duplicate entries
5: a87bbaa84f ! 5: cf50f1aabc cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts
@@ Commit message
Signed-off-by: Elijah Newren <newren@gmail.com>
## cache-tree.c ##
+@@ cache-tree.c: void cache_tree_invalidate_path(struct index_state *istate, const char *path)
+ istate->cache_changed |= CACHE_TREE_CHANGED;
+ }
+
++/*
++ * Check whether this_ce and the next entry in the index form a D/F
++ * conflict ("path" vs "path/file"). Returns the conflicting "path/..."
++ * name when one is found, or NULL otherwise.
++ *
++ * The cache is sorted, so "path/file" sorts after "path" and the
++ * conflict is usually visible as adjacent entries. But other entries
++ * can sort between them -- e.g. "path-internal" sits between "path"
++ * and "path/file" because '-' (0x2D) precedes '/' (0x2F) -- so when
++ * the immediately following entry shares our prefix but starts with a
++ * character that sorts before '/', binary search for "path/" instead.
++ */
++static const char *find_df_conflict(struct index_state *istate,
++ const struct cache_entry *this_ce,
++ const struct cache_entry *next_ce)
++{
++ const char *this_name = this_ce->name;
++ const char *next_name = next_ce->name;
++ int this_len = ce_namelen(this_ce);
++ const struct cache_entry *other;
++ struct strbuf probe = STRBUF_INIT;
++ int pos;
++
++ if (this_len >= ce_namelen(next_ce) ||
++ next_name[this_len] > '/' ||
++ strncmp(this_name, next_name, this_len))
++ return NULL;
++
++ if (next_name[this_len] == '/')
++ return next_name;
++
++ strbuf_add(&probe, this_name, this_len);
++ strbuf_addch(&probe, '/');
++ pos = index_name_pos_sparse(istate, probe.buf, probe.len);
++ strbuf_release(&probe);
++
++ if (pos < 0)
++ pos = -pos - 1;
++ if (pos >= (int)istate->cache_nr)
++ return NULL;
++ other = istate->cache[pos];
++ if (ce_namelen(other) > this_len &&
++ other->name[this_len] == '/' &&
++ !strncmp(this_name, other->name, this_len))
++ return other->name;
++ return NULL;
++}
++
+ static int verify_cache(struct index_state *istate, int flags)
+ {
+ unsigned i, funny;
@@ cache-tree.c: static int verify_cache(struct index_state *istate, int flags)
+ */
+ funny = 0;
for (i = 0; i + 1 < istate->cache_nr; i++) {
- /* path/file always comes after path because of the way
- * the cache is sorted. Also path can appear only once,
+- /* path/file always comes after path because of the way
+- * the cache is sorted. Also path can appear only once,
- * which means conflicting one would immediately follow.
-+ * so path/file is likely the immediately following path
-+ * but might be separated if there is e.g. a
-+ * path-internal/... file.
- */
+- */
const struct cache_entry *this_ce = istate->cache[i];
const struct cache_entry *next_ce = istate->cache[i + 1];
- const char *this_name = this_ce->name;
- const char *next_name = next_ce->name;
- int this_len = ce_namelen(this_ce);
-+ const char *conflict_name = NULL;
-+
- if (this_len < ce_namelen(next_ce) &&
+- const char *this_name = this_ce->name;
+- const char *next_name = next_ce->name;
+- int this_len = ce_namelen(this_ce);
+- if (this_len < ce_namelen(next_ce) &&
- next_name[this_len] == '/' &&
-+ next_name[this_len] <= '/' &&
- strncmp(this_name, next_name, this_len) == 0) {
-+ if (next_name[this_len] == '/') {
-+ conflict_name = next_name;
-+ } else if (next_name[this_len] < '/') {
-+ /*
-+ * The immediately next entry shares our
-+ * prefix but sorts before "path/" (e.g.,
-+ * "path-internal" between "path" and
-+ * "path/file", since '-' (0x2D) < '/'
-+ * (0x2F)). Binary search to find where
-+ * "path/" would be and check for a D/F
-+ * conflict there.
-+ */
-+ struct cache_entry *other;
-+ struct strbuf probe = STRBUF_INIT;
-+ int pos;
-+
-+ strbuf_add(&probe, this_name, this_len);
-+ strbuf_addch(&probe, '/');
-+ pos = index_name_pos_sparse(istate,
-+ probe.buf,
-+ probe.len);
-+ strbuf_release(&probe);
-+
-+ if (pos < 0)
-+ pos = -pos - 1;
-+ if (pos >= (int)istate->cache_nr)
-+ continue;
-+ other = istate->cache[pos];
-+ if (ce_namelen(other) > this_len &&
-+ other->name[this_len] == '/' &&
-+ !strncmp(this_name, other->name, this_len))
-+ conflict_name = other->name;
-+ }
-+ }
+- strncmp(this_name, next_name, this_len) == 0) {
++ const char *conflict_name;
+
++ conflict_name = find_df_conflict(istate, this_ce, next_ce);
+ if (conflict_name) {
if (10 < ++funny) {
fprintf(stderr, "...\n");
@@ cache-tree.c: static int verify_cache(struct index_state *istate, int flags)
}
fprintf(stderr, "You have both %s and %s\n",
- this_name, next_name);
-+ this_name, conflict_name);
++ this_ce->name, conflict_name);
}
}
if (funny)
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v5 2/2] graph: indent visual root in graph
From: Pablo Sabater @ 2026-06-14 5:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Taylor Blau, git, ayu.chandekar, chandrapratap3519,
christian.couder, jltobler, karthik.188, peff, phillip.wood,
siddharthasthana31
In-Reply-To: <xmqqo6hdepgy.fsf@gitster.g>
El dom, 14 jun 2026 a las 6:05, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
> [jc: Taylor CC'ed for his expertise and opinion on the quoted part
> that mucks with commit-graph files during the test]
>
> > diff --git a/t/t4218-log-graph-indentation.sh b/t/t4218-log-graph-indentation.sh
> > new file mode 100755
> > index 0000000000..ccf15c0a52
> > --- /dev/null
> > +++ b/t/t4218-log-graph-indentation.sh
> > @@ -0,0 +1,467 @@
> > +#!/bin/sh
> > ...
> > +# disable commit-graph topo order to have the graph to render in different
> > +# ways (used in --first-parent tests to have multiple visual roots while a
> > +# column is active at the same time).
> > +unset_commit_graph() {
> > + sane_unset GIT_TEST_COMMIT_GRAPH &&
> > + rm -f .git/objects/info/commit-graph &&
> > + rm -rf .git/objects/info/commit-graphs
> > +}
>
> I do not quite understand why having commit-graph makes the test
> result unpredictable here, but wouldn't we have a more stable way
> to disable use of commit-graph than going into filesystem and muck
> with the implementation detail like the above?
>
> Thanks.
Hi!
It does not make it unpredictable but it makes it not output what I
wanted to test, what I wanted to test is having an active column at
the same time that visual roots in different cases were being rendered
on another column. However having GIT_TEST_COMMIT_GRAPH in the last
text for example changes from:
* 41_octopus
| * 43_B
| \
| * 43_A
| * 42_B
| * 42_A
* 41_B
* 41_A
to:
* 41_octopus
* 41_B
\
* 41_A
* 43_B
\
* 43_A
* 42_B
* 42_A
While the output it's ok and the indentation works the excluded but
forced to be rendered parents are not being rendered at the same time
as an active column is.
On the unset_commit_graph function I had to remove those files because
even if it is GIT_TEST_COMMIT_GRAPH unset if there are files from
previous tests they still change the output.
Maybe there is a way to get this more cleanly but from what I tried it
didn't work as I wanted, sorry.
Thanks,
Pablo.
^ permalink raw reply
* Re: [RFC] commit-reach: terminate merge-base walk when one paint side is exhausted
From: Elijah Newren @ 2026-06-14 4:32 UTC (permalink / raw)
To: Kristofer Karlsson; +Cc: git, Derrick Stolee
In-Reply-To: <CAL71e4Mp7ewv0UGS8j=iTq6quyxLXzrr0uNDbWR8JKaOsTSVyA@mail.gmail.com>
On Fri, Jun 12, 2026 at 4:18 AM Kristofer Karlsson <krka@spotify.com> wrote:
>
> 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.
Wow...it appears this optimization was discovered by 3 separate people
in the last month. This optimization was implemented and is live at
GitHub...but it feels incomplete to me because my version doesn't
handle both sides having an infinite generation number (it just falls
back to the old algorithm when that happens). I had meant to fix that
and then upstream it, but other fires have been keeping me busy. And
I'm about to go on vacation on Monday.
I uploaded my version at
https://github.com/gitgitgadget/git/pull/2150. Unfortunately, it
conflicts with your recent good work in the area due to being based on
a version of main from about a month ago.
Do you want to take this over, rebase it, and extend to the infinite
generation number case? Or do you want me to rebase and see it
through after my vacation? Or some other mixture?
^ permalink raw reply
* Re: [PATCH v5 2/2] graph: indent visual root in graph
From: Junio C Hamano @ 2026-06-14 4:05 UTC (permalink / raw)
To: Pablo Sabater, Taylor Blau
Cc: git, ayu.chandekar, chandrapratap3519, christian.couder, jltobler,
karthik.188, peff, phillip.wood, siddharthasthana31
In-Reply-To: <20260613-ps-pre-commit-indent-v5-2-8d308efea63d@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
[jc: Taylor CC'ed for his expertise and opinion on the quoted part
that mucks with commit-graph files during the test]
> diff --git a/t/t4218-log-graph-indentation.sh b/t/t4218-log-graph-indentation.sh
> new file mode 100755
> index 0000000000..ccf15c0a52
> --- /dev/null
> +++ b/t/t4218-log-graph-indentation.sh
> @@ -0,0 +1,467 @@
> +#!/bin/sh
> ...
> +# disable commit-graph topo order to have the graph to render in different
> +# ways (used in --first-parent tests to have multiple visual roots while a
> +# column is active at the same time).
> +unset_commit_graph() {
> + sane_unset GIT_TEST_COMMIT_GRAPH &&
> + rm -f .git/objects/info/commit-graph &&
> + rm -rf .git/objects/info/commit-graphs
> +}
I do not quite understand why having commit-graph makes the test
result unpredictable here, but wouldn't we have a more stable way
to disable use of commit-graph than going into filesystem and muck
with the implementation detail like the above?
Thanks.
^ permalink raw reply
* Re: [PATCH 1/5] merge-ort: propagate callback errors from traverse_trees_wrapper()
From: Elijah Newren @ 2026-06-14 3:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <xmqq33z65ui1.fsf@gitster.g>
Sorry for the late reply...
On Mon, Jun 1, 2026 at 5:13 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > traverse_trees_wrapper() saves entries from a first pass through
> > traverse_trees() and then replays them through the real callback
> > (collect_merge_info_callback). However, the replay loop silently
> > discards the callback return value. This means any error reported by
> > the callback during replay -- including a future check for malformed
> > trees -- would be ignored, allowing the merge to proceed with corrupt
> > state.
> >
> > Capture the return value, stop the loop on negative (error) returns,
> > and propagate the error to the caller. Note that the callback returns
> > a positive mask value on success, so we normalize non-negative returns
> > to 0 for the caller.
>
> All makes perfect sense.
>
> How would the externally visible behaviour change at this step?
There's almost no change at this point. There is only one callpath
that can result in a negative return value, from near the top of
traverse_trees():
if (traverse_trees_cur_depth > r->settings.max_allowed_tree_depth)
return error("exceeded maximum allowed tree depth");
All other paths return non-negative values currently, so this patch is
mostly preparatory for later patches in this series.
> Upon an error from the callback, we used to keep going and processed
> other callback data in the renames structure. We now leave the rest
> unprocessed.
>
> The caller of this helper would never have seen a failure, but now
> they will. Both callers, collect_merge_info_callback() and
> handle_deferred_entries(), are reacting to a negative "error" return
> well (perhaps because they sometimes call traverse_trees() in the
> same control flow, which does return an error already), so
> presumably there is no downside caused by aborting the innermost
> process upon the first error return.
I'd state it a bit differently: not only is there no downwise to
aborting upon the first error, there IS a clear downside from ignoring
the errors and attempting to proceed anyway. This code wasn't a
deferred error kind of thing; it was an ignored error. For the
maximum allowed tree depth issue, we'd just prune the trees below that
depth and pretend that was the correct merge. And our lack of
detecting duplicate tree entries essentially means that we have a
"last one wins" (are we sure that's really the correct rule?) with the
added wrinkle that the first one can toggle various state flags that
can further tweak the merge and maybe even trip some assertions.
I'll add some of this info to the commit message.
^ permalink raw reply
* Re: [PATCH 5/5] cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts
From: Elijah Newren @ 2026-06-14 3:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <xmqqldcy4f07.fsf@gitster.g>
On Mon, Jun 1, 2026 at 5:33 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
[...]
> The narrow and tall comment block is a sign that this loop is
> getting too deeply nested. I wonder if it makes it easier to follow
> if we extract this new logic into a small helper function on its
> own?
Good point; I'll break it out into a small helper in v2.
> What the code checks and how it does so both make sense to me, though.
As always, thanks for the careful review.
^ permalink raw reply
* Re: [PATCH] clone: accept DEPTH env var as fallback for --depth
From: brian m. carlson @ 2026-06-14 1:08 UTC (permalink / raw)
To: Hadrien Loge; +Cc: gitster, git, gitgitgadget, hadean-eon-dev, m
In-Reply-To: <CADeHOfw6kNstNFucG7an6+Mbm2+=-PnOH8xtZkO9RK8=eWsx=w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2231 bytes --]
On 2026-06-13 at 17:43:23, Hadrien Loge wrote:
> Well mainly I'm asking this for packaging (Arch/Alpine/Etc)
> These all follow similar conventions (PKGBUILD/APKBUILD).
Debian builds packages without network access because they want to make
sure that every piece of the source is in the source package, that every
package is reproducibly built, and that users can rebuild source
packages if they like without worrying about needing network access or
having their location exposed to the author.
Wouldn't that be the right decision for reproducibility and privacy
reasons? If so, then the download of the source would happen before
building and could be tailored to meet your needs with the existing
command-line option.
> But in nested flows the ENV var seems like the proper solution.
>
> Mainly I gave this example on github:
>
> git clone --depth 1 url dest
> cd dest
> bash run.sh
> here run.sh has its own clone deps (perhaps even multiple)
> --depth 1 is now lost
>
> And only ENV vars that I can think of properly propagate for CI
> flows/clean chroot envirs.
> Thank you for considering the solution. It would be very useful
> for speeding up packaging.
> Even on 5k commits history it's 900kb vs 17mb.
>
> I have also reworked the commit to include tests/docs.
> and rename to GIT_CLONE_DEPTH
So say someone has this set in their environment and then they run a
script that clones a repository and runs `git describe`. That no longer
works and the script fails because it assumed that it had history.
It's also a problem if you then do a fetch into the shallow repository
because shallow fetches are _extremely_ expensive to serve (more
expensive than full clones), so having automation that now runs
thousands of these shallow fetches means that your requests will be
throttled by the server operator whereas they wouldn't with a regular
fetch.
There's no one right solution here and while I am sympathetic to your
situation, it will also result in hard-to-reproduce breakage for other
tooling. People rely on `git clone` and similar commands only honouring
command line arguments.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* Re: [RFC/PATCH] Suggestion: Safe Hook Verification for Unzipped/Local Repositories
From: brian m. carlson @ 2026-06-13 21:53 UTC (permalink / raw)
To: Jamison Phillips; +Cc: git
In-Reply-To: <CA+pATbgyg3Wqg7NnScPx3hUmo8nG23EFx2QUXuVAd3nJ6Z_CPw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3824 bytes --]
On 2026-06-13 at 21:20:43, Jamison Phillips wrote:
> Hello Git Community,
Hey,
> THE PROBLEM:
> When a user clones a repository, Git safely excludes the '.git/hooks'
> directory. However, if a developer downloads a project as a ZIP
> archive from an untrusted third party and extracts it, the archive can
> contain a fully formed '.git/hooks' directory populated with
> malicious, executable scripts.
>
> The moment the developer runs a standard command like 'git checkout'
> or 'git status' inside this unzipped folder, the hooks execute
> immediately without user consent or awareness. This is an active
> vector for supply-chain malware insertion on developer workstations.
Git's security model does not allow working with untrusted working
trees, period. The only things we guarantee are safe to do with an
untrusted repository are clone and fetch from it.
I'll also note that there are a variety of other methods that one can
use to execute arbitrary code with an untrusted working tree due to
unsafe configuration options. For example, one can set
`filter.<driver>.clean` and `filter.<driver>.smudge` and execute
arbitrary code with a crafted repository. This is why we don't let
people include config (or hooks) as part of the repository.
My concern is that we'd be misleading people that this was a safe way to
work by adopting your proposal when that is not true. We would then be
deluged by a whole host of invalid security reports.
Would you like to maybe share a little bit about why you (or others) are
distributing repositories as ZIP files instead of using the standard
protocol methods? Perhaps we can offer some thoughtful comments about
how to achieve the intended goals with a better security posture.
> PROPOSED FEATURE:
> I suggest implementing a "Safe Hook Verification" mechanism with the
> following logic:
>
> 1. First-Time Intercept: If Git detects executable scripts inside
> '.git/hooks' on a repository that does not have an explicit local
> clearance, it should halt execution and prompt the user: "Warning:
> This repository contains local hooks that have not been approved. Run
> them? (y/N)".
How would this work in a non-interactive situation? If I install Git
LFS, it installs hooks when its filter process is installed for the
first time. So if I then clone a repository using Git LFS in a CI job
or a container build process, the hooks won't work and my repository may
end up broken.
Note that the Unix philosophy does not have tools prompt users by
default. If I say `rm -fr ~`, then my home directory gets blown away
because that is what I asked for, even if that may have been improvident
and imprudent; no prompt is expected.
> 2. Out-of-Directory Verification State: If the user approves ('y'),
> Git should log this approval by saving a unique cryptographic hash of
> the approved hooks to a global state directory outside of the
> repository's working tree (e.g., inside
> ~/.config/git/approved_hooks/).
This is almost certainly going to grow indefinitely. I've been copying
my entire home directory from machine to machine as I get a new one for
19 years and I fully imagine that this would have grown quite a bit in
that time.
Another thing I think is worth noting is that just because I think a
hook is safe in one context does not mean I think it's safe in another.
A post-checkout hook that runs a particular command (say, `bin/setup`)
from the repository might be safe on my dotfiles (which I exclusively
control and maintain), but an identical hook on a repository from Bob's
Malware Emporium would probably not be a good idea. Reusing safe code
to do unsafe things has long been a favourite approach of attackers.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* [RFC/PATCH] Suggestion: Safe Hook Verification for Unzipped/Local Repositories
From: Jamison Phillips @ 2026-06-13 21:20 UTC (permalink / raw)
To: git
Hello Git Community,
I would like to propose a defensive security enhancement regarding how
Git handles hooks in repositories initialized outside of standard 'git
clone' pathways (such as repositories downloaded and extracted via
ZIP/tarball archives).
---
THE PROBLEM:
When a user clones a repository, Git safely excludes the '.git/hooks'
directory. However, if a developer downloads a project as a ZIP
archive from an untrusted third party and extracts it, the archive can
contain a fully formed '.git/hooks' directory populated with
malicious, executable scripts.
The moment the developer runs a standard command like 'git checkout'
or 'git status' inside this unzipped folder, the hooks execute
immediately without user consent or awareness. This is an active
vector for supply-chain malware insertion on developer workstations.
---
PROPOSED FEATURE:
I suggest implementing a "Safe Hook Verification" mechanism with the
following logic:
1. First-Time Intercept: If Git detects executable scripts inside
'.git/hooks' on a repository that does not have an explicit local
clearance, it should halt execution and prompt the user: "Warning:
This repository contains local hooks that have not been approved. Run
them? (y/N)".
2. Out-of-Directory Verification State: If the user approves ('y'),
Git should log this approval by saving a unique cryptographic hash of
the approved hooks to a global state directory outside of the
repository's working tree (e.g., inside
~/.config/git/approved_hooks/).
3. Subsequent Runs: On future commands, Git will check the current
hooks against the global hash map. If they match, they run silently.
If a hook file is modified or a new repository is unzipped, the prompt
appears again.
---
IMPACT:
This would close a massive blind spot for developers interacting with
shared zipped codebases, enforcing a model of explicit consent before
third-party code is executed locally by the VCS.
I look forward to hearing your thoughts on the feasibility or
alternative architectures for this defense-in-depth feature.
Regards,
Jamison Phillips
^ permalink raw reply
* [PATCH v5 2/2] graph: indent visual root in graph
From: Pablo Sabater @ 2026-06-13 19:09 UTC (permalink / raw)
To: git
Cc: ayu.chandekar, chandrapratap3519, christian.couder, gitster,
jltobler, karthik.188, peff, phillip.wood, siddharthasthana31,
Pablo Sabater
In-Reply-To: <20260613-ps-pre-commit-indent-v5-0-8d308efea63d@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 | 467 +++++++++++++++++++++++++++++++++++++++
3 files changed, 730 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..ccf15c0a52
--- /dev/null
+++ b/t/t4218-log-graph-indentation.sh
@@ -0,0 +1,467 @@
+#!/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; }
+}
+
+# disable commit-graph topo order to have the graph to render in different
+# ways (used in --first-parent tests to have multiple visual roots while a
+# column is active at the same time).
+unset_commit_graph() {
+ sane_unset GIT_TEST_COMMIT_GRAPH &&
+ rm -f .git/objects/info/commit-graph &&
+ rm -rf .git/objects/info/commit-graphs
+}
+
+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' '
+ (
+ unset_commit_graph &&
+ 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' '
+ (
+ unset_commit_graph &&
+ 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' '
+ (
+ unset_commit_graph &&
+ 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 v5 1/2] lib-log-graph: move check_graph function
From: Pablo Sabater @ 2026-06-13 19:09 UTC (permalink / raw)
To: git
Cc: ayu.chandekar, chandrapratap3519, christian.couder, gitster,
jltobler, karthik.188, peff, phillip.wood, siddharthasthana31,
Pablo Sabater
In-Reply-To: <20260613-ps-pre-commit-indent-v5-0-8d308efea63d@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 v5 0/2] graph: indent visual roots in graph
From: Pablo Sabater @ 2026-06-13 19:09 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 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/
V4 DIFF:
- Fixed test to be shown as expected by unsetting COMMIT_GRAPH
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 | 467 +++++++++++++++++++++++++++++
t/t6016-rev-list-graph-simplify-history.sh | 25 +-
6 files changed, 759 insertions(+), 34 deletions(-)
---
base-commit: 3e65291872de10c3f0bf05ea8c24187e7a71ebf0
change-id: 20260612-ps-pre-commit-indent-39ca72816382
Best regards,
--
Pablo Sabater <pabloosabaterr@gmail.com>
^ permalink raw reply
* [ANNOUNCE] tig-2.6.1
From: Thomas Koutcher @ 2026-06-13 18:02 UTC (permalink / raw)
To: git
In-Reply-To: <466060ab-ea6c-4c13-93f7-2de7a380429d@online.fr>
Hi,
I am pleased to announce Tig version 2.6.1 which brings a security fix as
well as some improvements and bugfixes. See the release notes below for a
detailed list of changes. Thanks to everyone who contributed.
What is Tig?
------------
Tig is an ncurses-based text-mode interface for git. It functions mainly
as a Git repository browser, but can also assist in staging changes for
commit at chunk level and act as a pager for output from various Git
commands.
- Homepage: https://github.com/jonas/tig
- Manual: https://www.mankier.com/7/tigmanual
- Tarballs: https://github.com/jonas/tig/releases
- Q&A: https://stackoverflow.com/questions/tagged/tig
Release notes
-------------
Security fixes:
- Fix editor command injection vulnerability (only affects
version 2.6.0). (#1432)
Bug fixes:
- Fix notes handling in the main view. (#1394)
- Honor init_size in string_map_put_to().
- Reset view->parent both when switching and closing view.
- Fix grep with revision argument. (#1398)
- Minor tweaks to the blame view.
- Fix repo info in bare repository with no HEAD.
- Document `%(status)`. (#1406)
- Update Cygwin build.
- Fix "DU" conflicts showing as 'Staged changes' in the main view. (#471)
- Ensure consistent blame view access from the blob view.
- Fix compiler warning with glibc-2.43.
- contrib: chocolate-theme: Fix cursor color in backgrounded view. (#1419)
- Fix date for 'Not Committed Yet' changes. (#1423)
- Fix slow tig status when given a subdirectory path. (#1424)
- Handle pure file rename in the diff view. (#1426)
Improvements:
- Handle the NO_COLOR environment variable. (#1402)
- Run tests against system's tig when SYSTEM_TIG=1. (#1400)
- Support option append. (#1413)
- Add an option to make the tree view recursive.
- Enable direct blob edits in clean HEAD state. (#1415, #1416)
Change summary
--------------
The diffstat and log summary for changes made in this release.
.gitignore | 1 +
INSTALL.adoc | 50 ++-----
Makefile | 36 +++--
NEWS.adoc | 38 ++++-
README.adoc | 5 +-
appveyor.yml | 2 +-
compat/ansidecl.h | 137 +++++++++++------
compat/compat.h | 2 +-
compat/hashtab.c | 141 +++++++++---------
compat/hashtab.h | 24 ++-
compat/wordexp.c | 2 +-
contrib/chocolate.theme.tigrc | 2 +
...ke-CYGWIN_NT-6.1 => config.make-CYGWIN_NT} | 17 +++
contrib/tig-completion.bash | 2 +-
contrib/tig.cygport.in | 30 ++++
doc/manual.adoc | 2 +-
doc/tig.1.adoc | 2 +-
doc/tigrc.5.adoc | 17 ++-
include/tig/apps.h | 2 +-
include/tig/argv.h | 2 +-
include/tig/blame.h | 2 +-
include/tig/blob.h | 2 +-
include/tig/diff.h | 2 +-
include/tig/display.h | 2 +-
include/tig/draw.h | 2 +-
include/tig/git.h | 6 +-
include/tig/graph.h | 2 +-
include/tig/grep.h | 2 +-
include/tig/help.h | 2 +-
include/tig/io.h | 2 +-
include/tig/keys.h | 2 +-
include/tig/line.h | 4 +-
include/tig/log.h | 2 +-
include/tig/main.h | 3 +-
include/tig/map.h | 2 +-
include/tig/options.h | 14 +-
include/tig/pager.h | 2 +-
include/tig/parse.h | 2 +-
include/tig/prompt.h | 2 +-
include/tig/refdb.h | 2 +-
include/tig/reflog.h | 2 +-
include/tig/refs.h | 2 +-
include/tig/repo.h | 2 +-
include/tig/request.h | 2 +-
include/tig/search.h | 2 +-
include/tig/stage.h | 2 +-
include/tig/stash.h | 2 +-
include/tig/status.h | 2 +-
include/tig/string.h | 2 +-
include/tig/tig.h | 2 +-
include/tig/tree.h | 2 +-
include/tig/types.h | 2 +-
include/tig/ui.h | 2 +-
include/tig/util.h | 2 +-
include/tig/view.h | 6 +-
include/tig/watch.h | 2 +-
src/apps.c | 2 +-
src/argv.c | 2 +-
src/blame.c | 18 ++-
src/blob.c | 38 +++--
src/diff.c | 47 +++---
src/display.c | 35 ++++-
src/draw.c | 2 +-
src/graph-v1.c | 2 +-
src/graph-v2.c | 2 +-
src/graph.c | 2 +-
src/grep.c | 31 +++-
src/help.c | 2 +-
src/io.c | 2 +-
src/keys.c | 2 +-
src/line.c | 5 +-
src/log.c | 4 +-
src/main.c | 14 +-
src/map.c | 4 +-
src/options.c | 45 ++++--
src/pager.c | 2 +-
src/parse.c | 2 +-
src/prompt.c | 2 +-
src/refdb.c | 2 +-
src/reflog.c | 2 +-
src/refs.c | 2 +-
src/repo.c | 10 +-
src/request.c | 2 +-
src/search.c | 2 +-
src/stage.c | 2 +-
src/stash.c | 2 +-
src/status.c | 23 +--
src/string.c | 2 +-
src/tig.c | 3 +-
src/tree.c | 10 +-
src/types.c | 2 +-
src/ui.c | 2 +-
src/util.c | 16 +-
src/view.c | 2 +-
src/watch.c | 2 +-
test/blame/blob-blame-test | 2 +-
test/blame/default-test | 8 +-
test/blame/initial-diff-test | 2 +-
test/blame/navigation-parent-test | 2 +-
test/blame/revargs-test | 4 +-
test/blame/start-on-line-test | 2 +-
test/blame/stash-test | 4 +-
test/grep/refspec-test | 59 ++++++++
test/main/filter-args-test | 2 +-
test/status/file-filter-test | 80 ++++++++++
test/tigrc/append-option-test | 33 ++++
test/tools/libgit.sh | 2 +-
test/tools/libtest.sh | 2 +-
test/tools/show-results.sh | 2 +-
test/tools/test-graph.c | 2 +-
test/tree/recurse-test | 64 ++++++++
tigrc | 4 +
tools/announcement.sh | 2 +-
tools/doc-gen.c | 2 +-
tools/header.h | 2 +-
tools/install.sh | 2 +-
tools/make-builtin-config.sh | 2 +-
tools/release.sh | 2 +-
tools/uninstall.sh | 2 +-
119 files changed, 873 insertions(+), 375 deletions(-)
Karl Liang (1):
support option append (#1413)
Lee Garrett (1):
Run tests against system's tig when SYSTEM_TIG=1
Marcel Holtmann (1):
Fix compiler warning with glibc-2.43
Siddh Raman Pant (1):
contrib: chocolate-theme: Fix cursor color in backgrounded view (#1419)
Thomas Koutcher (23):
Update manual link to point to mankier.com and remove link to Gitter
Fix notes handling in the main view
Update compat/hashtab.c with latest libiberty version
Honor init_size in string_map_put_to()
Reset view->parent both when switching and closing view
Fix grep with revision argument
Minor tweaks to the blame view
Handle the NO_COLOR environment variable
Bump copyright year to 2026
Fix repo info in bare repository with no HEAD
Document %(status)
Update Cygwin build
Add an option to make the tree view recursive
Fix "DU" conflicts showing as 'Staged changes' in the main view
Use compat/wordexp with Cygwin
Ensure consistent blame view access from the blob view
Skip generating manual.pdf when running make doc
Fix date for 'Not Committed Yet' changes
Add new AI related trailers to default tigrc
Handle pure file rename in the diff view
Fix editor command injection vulnerability
Update NEWS
tig-2.6.1
phpmac (1):
Enable direct blob edits in clean HEAD state (#1416)
apawn (1):
Fix slow tig status when given a subdirectory path (#1424)
--
Thomas Koutcher
^ permalink raw reply
* Re: [PATCH] clone: accept DEPTH env var as fallback for --depth
From: Hadrien Loge @ 2026-06-13 17:43 UTC (permalink / raw)
To: gitster; +Cc: git, gitgitgadget, hadean-eon-dev, m
Well mainly I'm asking this for packaging (Arch/Alpine/Etc)
These all follow similar conventions (PKGBUILD/APKBUILD).
But in nested flows the ENV var seems like the proper solution.
Mainly I gave this example on github:
git clone --depth 1 url dest
cd dest
bash run.sh
here run.sh has its own clone deps (perhaps even multiple)
--depth 1 is now lost
And only ENV vars that I can think of properly propagate for CI
flows/clean chroot envirs.
Thank you for considering the solution. It would be very useful
for speeding up packaging.
Even on 5k commits history it's 900kb vs 17mb.
I have also reworked the commit to include tests/docs.
and rename to GIT_CLONE_DEPTH
Kind regards,
Hade
^ permalink raw reply
* Re: [PATCH v11] checkout: extend --track with a "fetch" mode to refresh start-point
From: Harald Nordgren @ 2026-06-13 17:38 UTC (permalink / raw)
To: phillip.wood
Cc: Junio C Hamano, Harald Nordgren via GitGitGadget, git,
Ramsay Jones, D. Ben Knoble, Kristoffer Haugsbakk, Marc Branchaud
In-Reply-To: <01526f43-86aa-466f-a1e8-054284e1a2e1@gmail.com>
Hi Phillip and JCH!
Would it be possible to get another look here to know if it's worth
continuing with this topic. I think it's a useful feature, but the
feedback from this list has been a bit lukewarm.
Harald
On Thu, May 21, 2026 at 4:06 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 21/05/2026 13:58, Junio C Hamano wrote:
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> >>> One. Have you considered the case where the remote-tracking refs
> >>> are overlapping, e.g., where "origin" and "upstream" point at
> >>> different URLs but they both store in "refs/remotes/upstream/*"?
> >>> Perhaps their URLs may textually be different but are pointing
> >>> logically at the same place (e.g., one ssh:// the other https:// for
> >>> example).
> >>>
> >>> What should happen? What does happen after you apply this patch?
> >>
> >> It would be worth looking at what "git checkout --track" does in that
> >> case and seeing if we can share the code.
> >
> > It always is a good idea to think how we can share code for
> > different purposes to solve a new problem, but in this particular
> > one, I am not sure if "git checkout -t -b topic upstream/main"
> > codepath has much to offer to solve what the new "before the
> > checkout, update from the remote" feature wants to do. To the
> > former, it does not matter how refs/remotes/upstream/* are updated
> > and by fetching which remote at all.
>
> Don't we want to avoid creating a branch with an ambiguous upstream so
> that a subsequent "git pull" works though? Looking at
> branch.c:setup_tracking() it seems to reject upstream branches that
> match more than one remote.
>
> Thanks
>
> Phillip
>
> > The only thing it cares about
> > is to leave the record that this new "topic" branch works with
> > refs/remotes/upstrea/main. But the latter needs to be able to
> > compute which remote it should fetch from. It is a problem that
> > existing code had no need to solve.
> >
>
^ permalink raw reply
* Re: [RFC PATCH 2/2] doc: advise batching patch rerolls
From: Weijie Yuan @ 2026-06-13 17:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps
In-Reply-To: <xmqqwlw2e8dc.fsf@gitster.g>
On Sat, Jun 13, 2026 at 09:02:39AM -0700, Junio C Hamano wrote:
> Weijie Yuan <wy@wyuan.org> writes:
>
> > Contributors often need guidance on how quickly to send later iterations
> > of a patch series. Add a rough default of no more than one new version
> > of the same series per day so feedback can be batched and reviewers have
> > time to comment.
> >
> > Mention factors that can affect the timing, such as series size, review
> > depth, substantial rework, and how close the topic is to being accepted.
>
> Another good thing to discourage yourself from rerolling too quickly
> is that such a practice forces you to think twice and be very
> careful before sending patches out. As you have only one chance to
> get it right before, say, 24 hours, you'd want to make sure that you
> would not distract your reviewers with stupid typoes, off-by-one
> errors, and such, and concentrate their reviews more on what matters
> more, i.e., the higher level design, choice of algorithms, etc.
>
> > +This consideration applies not only when going from the initial patch to v2, but
> > +also to later iterations of the same series. There is no fixed rule for how long
> > +to wait before sending a new version. A useful default is to send at most one
> > +new version of the same patch series per day. This gives multiple reviewers time
> > +to comment, lets you batch feedback together, and gives you time to think
> > +through the comments you received.
>
> And the 24-hour gives equal chance to comment on your patches to
> anybody no matter where they live ;-)
Thanks for your comments above! Let me think about how to integrate
these contents with the patch.
> I see you CC'ed Patrick, and I am sure he'll give us more useful
> suggestions than I do here ;-)
This is his practical advice, and I just stole Patrick´s wording, to be
fair ;-) so of course I should CC him and let him know I am a wording
thief :-P, hope it wouldn't disturb him ;-)
Thank you very much.
^ permalink raw reply
* Re: [PATCH v4 0/3] compat/posix.h: enable UNUSED warning messages for Clang
From: Junio C Hamano @ 2026-06-13 16:39 UTC (permalink / raw)
To: Dominik Loidolt; +Cc: git, ps, asedeno, asedeno, avarab
In-Reply-To: <20260613122711.38662-1-dominik.loidolt@univie.ac.at>
Dominik Loidolt <dominik.loidolt@univie.ac.at> writes:
> This series enables the intended UNUSED warning message with Clang by
> adding a dedicated Clang version check. It also cleans up the nearby
> GIT_GNUC_PREREQ() and UNUSED macros.
>
> Changes since v3:
> - split style-only cleanups into their own patch
> - fix the UNUSED preprocessor indentation style
> - simplify the GIT_GNUC_PREREQ() comparison commit message
> - keep the Clang-specific note in the patch that adds GIT_CLANG_PREREQ()
>
> Thanks,
> Dominik
>
> Dominik Loidolt (3):
> compat/posix.h: enable UNUSED warning messages for Clang
> compat/posix.h: clean up GIT_GNUC_PREREQ() and UNUSED
> compat/posix.h: simplify GIT_GNUC_PREREQ() comparison
Looking good and all the points Patrick raised during the review of
the previous round seem to have been addressed nicely.
Will replace. Shall we mark it for 'next' now?
Thanks.
^ permalink raw reply
* Re: [PATCH 0/6] t: add lint-style.pl and convert grep to test_grep
From: Michael Montalbo @ 2026-06-13 16:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Montalbo via GitGitGadget, git, D. Ben Knoble,
Eric Sunshine
In-Reply-To: <xmqqldcovhnf.fsf@gitster.g>
On Mon, Jun 8, 2026 at 2:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> I do not think we want an automated tool that rewrites the source
> files. I was hoping that we would get a patch or two that _adds_ to
> existing test-lint framework (i.e., 'test-grep' that 'test-lint'
> target depends on in t/Makefile) that gives diagnosis in a similar
> fashion as test-lint-shell-syntax and test-chainlint do.
>
> Also some existing uses of "grep" are not end-user facing and should
> not be rewritten to "test_grep".
Apologies for not responding explicitly before sending a re-roll, I
will do that in
the future. v2 attempts to address these points as noted in the "changes"
section.
^ permalink raw reply
* Re: [PATCH 0/2] commit: preserve commit hash on a no-op amend
From: Harald Nordgren @ 2026-06-13 16:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Harald Nordgren via GitGitGadget, git
In-Reply-To: <xmqq33yqfnsa.fsf@gitster.g>
Interesting discussions! This sounds like showstopper, seems more
reasonable to leave this topic for now.
I just want to share that I've been running this for years to
re-trigger CI (because up until a few days ago I didn't realize that
the hash did indeed change even when nothing had changed), I had the
wrong mental model for commit hashes:
git commit --amend --no-edit --date="now"
Harald
^ 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