* [PATCH v2 8/8] builtin/cat-file: filter objects via object database
From: Patrick Steinhardt @ 2026-07-10 8:49 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im>
When batching all objects, git-cat-file(1) reaches into the internals of
the object database and manually manages bitmaps to apply object
filters. This creates coupling between the command and the internals of
the respective backend.
Refactor git-cat-file(1) to use the new object filter option when
batching all objects. This significantly simplifies the logic and
ensures that we don't have to reach into internals of the "files" source
anymore.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/cat-file.c | 76 +++++-------------------------------------------------
1 file changed, 7 insertions(+), 69 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b4b99a73da..1458dd76d6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,7 +20,6 @@
#include "userdiff.h"
#include "oid-array.h"
#include "packfile.h"
-#include "pack-bitmap.h"
#include "object-file.h"
#include "object-name.h"
#include "odb.h"
@@ -844,28 +843,6 @@ static int batch_one_object_oi(const struct object_id *oid,
return payload->callback(oid, NULL, 0, payload->payload);
}
-static int batch_one_object_packed(const struct object_id *oid,
- struct packed_git *pack,
- uint32_t pos,
- void *_payload)
-{
- struct for_each_object_payload *payload = _payload;
- return payload->callback(oid, pack, nth_packed_object_offset(pack, pos),
- payload->payload);
-}
-
-static int batch_one_object_bitmapped(const struct object_id *oid,
- enum object_type type UNUSED,
- int flags UNUSED,
- uint32_t hash UNUSED,
- struct packed_git *pack,
- off_t offset,
- void *_payload)
-{
- struct for_each_object_payload *payload = _payload;
- return payload->callback(oid, pack, offset, payload->payload);
-}
-
static void batch_each_object(struct batch_options *opt,
for_each_object_fn callback,
unsigned flags,
@@ -875,56 +852,17 @@ static void batch_each_object(struct batch_options *opt,
.callback = callback,
.payload = _payload,
};
+ struct odb_source_info source_info;
+ struct object_info oi = {
+ .source_infop = &source_info,
+ };
struct odb_for_each_object_options opts = {
.flags = flags,
+ .filter = &opt->objects_filter,
};
- struct bitmap_index *bitmap = NULL;
- struct odb_source *source;
-
- /*
- * TODO: we still need to tap into implementation details of the object
- * database sources. Ideally, we should extend `odb_for_each_object()`
- * to handle object filters itself so that we can move the filtering
- * logic into the individual sources.
- */
- odb_prepare_alternates(the_repository->objects);
- for (source = the_repository->objects->sources; source; source = source->next) {
- struct odb_source_files *files = odb_source_files_downcast(source);
- int ret = odb_source_for_each_object(&files->loose->base, NULL, batch_one_object_oi,
- &payload, &opts);
- if (ret)
- break;
- }
-
- if (opt->objects_filter.choice != LOFC_DISABLED &&
- (bitmap = prepare_bitmap_git(the_repository)) &&
- !for_each_bitmapped_object(bitmap, &opt->objects_filter,
- batch_one_object_bitmapped, &payload)) {
- struct packed_git *pack;
-
- repo_for_each_pack(the_repository, pack) {
- if (bitmap_index_contains_pack(bitmap, pack) ||
- open_pack_index(pack))
- continue;
- for_each_object_in_pack(pack, batch_one_object_packed,
- &payload, flags);
- }
- } else {
- struct odb_source_info source_info;
- struct object_info oi = {
- .source_infop = &source_info,
- };
-
- for (source = the_repository->objects->sources; source; source = source->next) {
- struct odb_source_files *files = odb_source_files_downcast(source);
- int ret = odb_source_for_each_object(&files->packed->base, &oi,
- batch_one_object_oi, &payload, &opts);
- if (ret)
- break;
- }
- }
- free_bitmap_index(bitmap);
+ odb_for_each_object_ext(the_repository->objects, &oi,
+ batch_one_object_oi, &payload, &opts);
}
static int batch_objects(struct batch_options *opt)
--
2.55.0.229.g6434b31f56.dirty
^ permalink raw reply related
* [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH
From: Christian Couder @ 2026-07-10 8:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Karthik Nayak, Jeff King,
Elijah Newren, Christian Couder
Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
2024-04-16), lazy fetching has been controlled by the
`GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
nothing" boolean that is set to 'true' by default when calling `git
upload-pack` for security reasons.
Recently the "promisor-remote" capability was added to protocol v2,
allowing servers and clients to agree on the promisor remotes they
can safely use.
This series leverages that capability to implement a pragmatic middle
ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
fetching is allowed only when fetching from promisor remotes that are
both advertised by the server and accepted by the client.
Note that using an environment variable for this is probably not the
best from a usability perspective. An `upload-pack.allowLazyFetch`
configuration variable would likely be better.
Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
things currently work. It would be a much bigger and more invasive
change to implement `upload-pack.allowLazyFetch` in a way that is
compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
backward compatibility. Therefore, transitioning to a configuration
variable is left for future work.
High level overview of the patches
==================================
Patch 1/3: A refactor which separates the fetching logic from the
error handling and validation logic. This might also slightly increase
performance if there are several promisor remotes.
Patch 2/3: A preparatory commit that transitions `GIT_NO_LAZY_FETCH`
from a strict boolean check into an enum that can support multiple
states.
Patch 3/3: Introduces the 'fromAccepted' option, taking advantage of
the previous preparatory commits.
CI tests
========
They all pass, see:
https://github.com/chriscool/git/actions/runs/29078195030
Christian Couder (3):
promisor-remote: factor out lazy_fetch_objects()
promisor-remote: introduce enum allow_lazy_fetch
promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH
Documentation/git-upload-pack.adoc | 5 ++
Documentation/git.adoc | 6 +-
promisor-remote.c | 110 ++++++++++++++++++--------
promisor-remote.h | 14 ++++
setup.c | 5 +-
t/t5710-promisor-remote-capability.sh | 49 ++++++++++++
6 files changed, 154 insertions(+), 35 deletions(-)
--
2.55.0.125.g395cd2c8ec.dirty
^ permalink raw reply
* [PATCH 1/3] promisor-remote: factor out lazy_fetch_objects()
From: Christian Couder @ 2026-07-10 8:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Karthik Nayak, Jeff King,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260710085137.4171240-1-christian.couder@gmail.com>
In "promisor-remote.c:fetch_objects()", there is a check to disable
lazy fetching when the `GIT_NO_LAZY_FETCH` environment variable is
set. The fetch_objects() function is called once per promisor remote
though. So the check might be performed more times than necessary.
Also promisor_remote_get_direct() mixes up the logic deciding which
promisor remotes to try with the logic checking that the objects
that could not be fetched are promisor objects.
Let's refactor the lazy fetching logic out of these two functions
into a new lazy_fetch_objects() function. This will make it easier
to extend the lazy fetching logic in following commits.
This is a pure refactoring with no intended behavior change. Two
things shift in ways that are observably equivalent though:
- the `GIT_NO_LAZY_FETCH` check is now performed once up front,
instead of once per promisor remote, and
- promisor_remote_init() is no longer called when lazy fetching
is disabled, which is fine as nothing downstream of it, like
is_promisor_object(), needs it in that case.
While at it, let's also convert try_promisor_remotes() to return
'bool' instead of 'int', as it just returns whether all the objects
could be fetched, and document its return value.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
promisor-remote.c | 76 ++++++++++++++++++++++++++++-------------------
1 file changed, 45 insertions(+), 31 deletions(-)
diff --git a/promisor-remote.c b/promisor-remote.c
index 43505d1e1a..65496c69cf 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -31,15 +31,6 @@ static int fetch_objects(struct repository *repo,
FILE *child_in;
int quiet;
- if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
- static int warning_shown;
- if (!warning_shown) {
- warning_shown = 1;
- warning(_("lazy fetching disabled; some objects may not be available"));
- }
- return -1;
- }
-
child.git_cmd = 1;
child.in = -1;
if (repo != the_repository)
@@ -270,10 +261,15 @@ static int remove_fetched_oids(struct repository *repo,
return remaining_nr;
}
-static int try_promisor_remotes(struct repository *repo,
- struct object_id **remaining_oids,
- int *remaining_nr, int *to_free,
- bool accepted_only)
+/*
+ * Return 'true' if all the objects could be fetched from the
+ * (non-)accepted remotes, 'false' otherwise.
+ */
+static bool try_promisor_remotes(struct repository *repo,
+ struct object_id **remaining_oids,
+ int *remaining_nr,
+ int *to_free,
+ bool accepted_only)
{
struct promisor_remote *r = repo->promisor_remote_config->promisors;
@@ -290,9 +286,37 @@ static int try_promisor_remotes(struct repository *repo,
continue;
}
}
- return 1; /* all fetched */
+ return true; /* all fetched */
}
- return 0;
+ return false;
+}
+
+/*
+ * Return 'true' if all the objects could be fetched, 'false' otherwise.
+ */
+static bool lazy_fetch_objects(struct repository *repo,
+ struct object_id **remaining_oids,
+ int *remaining_nr,
+ int *to_free)
+{
+ if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+ static int warning_shown;
+ if (!warning_shown) {
+ warning_shown = 1;
+ warning(_("lazy fetching disabled; some objects may not be available"));
+ }
+ return false;
+ }
+
+ promisor_remote_init(repo);
+
+ /* Try accepted remotes first (those the server told us to use) */
+ if (try_promisor_remotes(repo, remaining_oids, remaining_nr,
+ to_free, true))
+ return true;
+
+ return try_promisor_remotes(repo, remaining_oids, remaining_nr,
+ to_free, false);
}
void promisor_remote_get_direct(struct repository *repo,
@@ -302,28 +326,18 @@ void promisor_remote_get_direct(struct repository *repo,
struct object_id *remaining_oids = (struct object_id *)oids;
int remaining_nr = oid_nr;
int to_free = 0;
- int i;
if (oid_nr == 0)
return;
- promisor_remote_init(repo);
-
- /* Try accepted remotes first (those the server told us to use) */
- if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
- &to_free, true))
- goto all_fetched;
- if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
- &to_free, false))
- goto all_fetched;
-
- for (i = 0; i < remaining_nr; i++) {
- if (is_promisor_object(repo, &remaining_oids[i]))
- die(_("could not fetch %s from promisor remote"),
- oid_to_hex(&remaining_oids[i]));
+ if (!lazy_fetch_objects(repo, &remaining_oids, &remaining_nr, &to_free)) {
+ for (int i = 0; i < remaining_nr; i++) {
+ if (is_promisor_object(repo, &remaining_oids[i]))
+ die(_("could not fetch %s from promisor remote"),
+ oid_to_hex(&remaining_oids[i]));
+ }
}
-all_fetched:
if (to_free)
free(remaining_oids);
}
--
2.55.0.125.g395cd2c8ec.dirty
^ permalink raw reply related
* [PATCH 2/3] promisor-remote: introduce enum allow_lazy_fetch
From: Christian Couder @ 2026-07-10 8:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Karthik Nayak, Jeff King,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260710085137.4171240-1-christian.couder@gmail.com>
The `GIT_NO_LAZY_FETCH` environment variable is currently parsed as
a Boolean, using git_env_bool(), in both "setup.c" and
"promisor-remote.c".
In a following commit, we are going to allow a third value for this
variable, on top of 'true' and 'false'.
To prepare for that, let's introduce an `enum allow_lazy_fetch` with
the possible results of parsing the variable, along with a
parse_allow_lazy_fetch_env() function to parse it, and let's use them
everywhere the variable is parsed.
Note that, as before, an invalid value makes us die(), only the error
message changes from "bad boolean environment value ..." to "bad
environment value ...".
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
promisor-remote.c | 24 +++++++++++++++++++++++-
promisor-remote.h | 13 +++++++++++++
setup.c | 5 ++++-
3 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/promisor-remote.c b/promisor-remote.c
index 65496c69cf..56f57c5267 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -21,6 +21,26 @@ struct promisor_remote_config {
struct promisor_remote **promisors_tail;
};
+enum allow_lazy_fetch parse_allow_lazy_fetch_env(void)
+{
+ const char *v = getenv(NO_LAZY_FETCH_ENVIRONMENT);
+ int val;
+
+ if (!v)
+ return LAZY_FETCH_ALL;
+
+ val = git_parse_maybe_bool(v);
+
+ if (!val)
+ return LAZY_FETCH_ALL;
+ if (val > 0)
+ return LAZY_FETCH_NONE;
+
+ die(_("bad environment value '%s' for '%s'; "
+ "only 'false/0' and 'true/1' are valid"),
+ v, NO_LAZY_FETCH_ENVIRONMENT);
+}
+
static int fetch_objects(struct repository *repo,
const char *remote_name,
const struct object_id *oids,
@@ -299,7 +319,9 @@ static bool lazy_fetch_objects(struct repository *repo,
int *remaining_nr,
int *to_free)
{
- if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+ enum allow_lazy_fetch lf = parse_allow_lazy_fetch_env();
+
+ if (lf == LAZY_FETCH_NONE) {
static int warning_shown;
if (!warning_shown) {
warning_shown = 1;
diff --git a/promisor-remote.h b/promisor-remote.h
index 301f5ac5cb..87fc24c9eb 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -25,6 +25,19 @@ void promisor_remote_clear(struct promisor_remote_config *config);
struct promisor_remote *repo_promisor_remote_find(struct repository *r, const char *remote_name);
int repo_has_promisor_remote(struct repository *r);
+/* Enum for lazy fetching parsing */
+enum allow_lazy_fetch {
+ LAZY_FETCH_NONE = 0, /* No lazy fetching */
+ LAZY_FETCH_ALL /* Lazy fetch from any promisor remotes */
+};
+
+/*
+ * Parse the NO_LAZY_FETCH_ENVIRONMENT env variable into an
+ * `enum allow_lazy_fetch`.
+ * If parsing fails, then die().
+ */
+enum allow_lazy_fetch parse_allow_lazy_fetch_env(void);
+
/*
* Fetches all requested objects from all promisor remotes, trying them one at
* a time until all objects are fetched.
diff --git a/setup.c b/setup.c
index 0de56a074f..0a81d9f045 100644
--- a/setup.c
+++ b/setup.c
@@ -24,6 +24,7 @@
#include "trace.h"
#include "trace2.h"
#include "worktree.h"
+#include "promisor-remote.h"
enum allowed_bare_repo {
ALLOWED_BARE_REPO_EXPLICIT = 0,
@@ -1051,6 +1052,7 @@ static void setup_git_env_internal(struct repository *repo,
const char *replace_ref_base;
struct set_gitdir_args args = { NULL };
struct strvec to_free = STRVEC_INIT;
+ enum allow_lazy_fetch lf;
args.commondir = getenv_safe(&to_free, GIT_COMMON_DIR_ENVIRONMENT);
args.graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT);
@@ -1072,7 +1074,8 @@ static void setup_git_env_internal(struct repository *repo,
if (shallow_file)
set_alternate_shallow_file(repo, shallow_file, 0);
- if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0))
+ lf = parse_allow_lazy_fetch_env();
+ if (lf == LAZY_FETCH_NONE)
fetch_if_missing = 0;
}
--
2.55.0.125.g395cd2c8ec.dirty
^ permalink raw reply related
* [PATCH 3/3] promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH
From: Christian Couder @ 2026-07-10 8:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Karthik Nayak, Jeff King,
Elijah Newren, Christian Couder, Christian Couder
In-Reply-To: <20260710085137.4171240-1-christian.couder@gmail.com>
The `GIT_NO_LAZY_FETCH` environment variable can be set to 'true' or
'false' to enable or disable lazy fetching. By default it is set to
'true' when calling `git upload-pack` to avoid security issues, see
7b70e9efb1 (upload-pack: disable lazy-fetching by default, 2024-04-16).
Recently though, the "promisor-remote" capability was introduced into
protocol v2, which allows a server to advertise some promisor remotes
and clients to accept them or not.
When promisor remotes are advertised by the server and accepted by the
client, it means that they are quite trusted. So the security risks
which come from lazy fetching from them could be considered much more
acceptable.
Let's introduce a 'fromAccepted' option on top of 'true' and 'false'
for `GIT_NO_LAZY_FETCH` to allow lazy fetching only from accepted
promisor remotes.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-upload-pack.adoc | 5 +++
Documentation/git.adoc | 6 ++--
promisor-remote.c | 14 +++++++-
promisor-remote.h | 1 +
t/t5710-promisor-remote-capability.sh | 49 +++++++++++++++++++++++++++
5 files changed, 71 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-upload-pack.adoc b/Documentation/git-upload-pack.adoc
index 9167a321d0..1c2ed9d7ba 100644
--- a/Documentation/git-upload-pack.adoc
+++ b/Documentation/git-upload-pack.adoc
@@ -71,6 +71,11 @@ This is implemented by having `upload-pack` internally set the
(because you are fetching from a partial clone, and you are sure
you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to
`0`.
++
+`GIT_NO_LAZY_FETCH` can also be set to 'fromAccepted' which allows
+lazy fetching only from remotes that are advertised and accepted using
+the "promisor-remote" protocol v2 capability. See
+linkgit:gitprotocol-v2[5]. This is safer than setting it to `0`.
SECURITY
--------
diff --git a/Documentation/git.adoc b/Documentation/git.adoc
index 8a5cdd3b3d..14a083bcdb 100644
--- a/Documentation/git.adoc
+++ b/Documentation/git.adoc
@@ -947,9 +947,9 @@ for full details.
pathspecs as case-insensitive.
`GIT_NO_LAZY_FETCH`::
- Setting this Boolean environment variable to true tells Git
- not to lazily fetch missing objects from the promisor remote
- on demand.
+ Setting this environment variable controls whether Git is
+ allowed to lazily fetch missing objects from a promisor remote
+ on demand. See linkgit:git-upload-pack[1].
`GIT_REFLOG_ACTION`::
When a ref is updated, reflog entries are created to keep
diff --git a/promisor-remote.c b/promisor-remote.c
index 56f57c5267..c80319f966 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -35,9 +35,11 @@ enum allow_lazy_fetch parse_allow_lazy_fetch_env(void)
return LAZY_FETCH_ALL;
if (val > 0)
return LAZY_FETCH_NONE;
+ if (!strcasecmp(v, "fromAccepted"))
+ return LAZY_FETCH_ACCEPTED;
die(_("bad environment value '%s' for '%s'; "
- "only 'false/0' and 'true/1' are valid"),
+ "only 'false/0', 'true/1' and 'fromAccepted' are valid"),
v, NO_LAZY_FETCH_ENVIRONMENT);
}
@@ -337,6 +339,16 @@ static bool lazy_fetch_objects(struct repository *repo,
to_free, true))
return true;
+ if (lf == LAZY_FETCH_ACCEPTED) {
+ static int warning_shown;
+ if (!warning_shown) {
+ warning_shown = 1;
+ warning(_("lazy fetching from accepted promisor remotes only; "
+ "some objects may not be available"));
+ }
+ return false;
+ }
+
return try_promisor_remotes(repo, remaining_oids, remaining_nr,
to_free, false);
}
diff --git a/promisor-remote.h b/promisor-remote.h
index 87fc24c9eb..0d05ff9d84 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -28,6 +28,7 @@ int repo_has_promisor_remote(struct repository *r);
/* Enum for lazy fetching parsing */
enum allow_lazy_fetch {
LAZY_FETCH_NONE = 0, /* No lazy fetching */
+ LAZY_FETCH_ACCEPTED, /* Lazy fetching only from accepted promisor remotes */
LAZY_FETCH_ALL /* Lazy fetch from any promisor remotes */
};
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 549acff23f..1c61b100b9 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -173,6 +173,55 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
initialize_server 1 "$oid"
'
+test_expect_success "clone with GIT_NO_LAZY_FETCH=fromAccepted and accepted promisor remote" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ # Clone from server to create a client
+ GIT_NO_LAZY_FETCH=fromAccepted git clone -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=All \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone with GIT_NO_LAZY_FETCH=fromAccepted and no accepted promisor remote" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ # Clone from server to create a client
+ # It should fail because the server cannot lazy fetch the missing blob
+ test_must_fail env GIT_NO_LAZY_FETCH=fromAccepted git clone -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=None \
+ --no-local --filter="blob:limit=5k" server client 2>err &&
+
+ test_grep "lazy fetching from accepted promisor remotes only" err &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone failure with GIT_NO_LAZY_FETCH=bogus" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ test_must_fail env GIT_NO_LAZY_FETCH=bogus git clone -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=All \
+ --no-local --filter="blob:limit=5k" server client 2>err &&
+
+ test_grep "bad environment value" err &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
test_expect_success "init + fetch with promisor.advertise set to 'true'" '
git -C server config promisor.advertise true &&
test_when_finished "rm -rf client" &&
--
2.55.0.125.g395cd2c8ec.dirty
^ permalink raw reply related
* [PATCH v8 0/5] history: add squash subcommand to fold a range
From: Harald Nordgren via GitGitGadget @ 2026-07-10 9:06 UTC (permalink / raw)
To: git
Cc: Phillip Wood, D. Ben Knoble, Patrick Steinhardt, Matt Hunter,
Harald Nordgren
In-Reply-To: <pull.2337.v7.git.git.1783327849.gitgitgadget@gmail.com>
Adds git history squash <revision-range> to fold a range of commits.
Changes in v8:
* --reedit-message now builds the same editor template as git rebase -i
--autosquash: fixup!, squash! and amend! commits are grouped under the
commit they target instead of shown in commit order, and an amend!
replaces its target's message.
* A fixup!, squash! or amend! is refused only when its target is outside
the range, so several fixups for an in-range commit fold together. A
range that is entirely markers for one below-range target is combined
into a single commit, keeping the last amend! message.
* Merges inside the range are folded when the range has a single base, with
no dedicated opt-in flag, --ancestry-path ensures only commits descended
from the base are folded, and a range reaching more than one base is
rejected.
* Rev-list options are accepted and sanitized the way git replay does,
forcing the walk order back with a warning, which also fixes git history
squash -- --reverse slipping past the previous option check.
* Kept this as an explicit squash subcommand rather than making
--reedit-message the default or renaming the command.
Changes in v7:
* --reedit-message now builds the same editor template git rebase -i shows
for a squash (a combination of N commits banner with each folded message
under its own header) and follows autosquash for markers: a fixup!
message falls out (commented under a will be skipped header), while a
squash! or amend! keeps its body with only the marker subject commented
so its remark can be reworded in. Only the message text is affected,
every commit's changes are always folded in.
* Reuse git rebase -i's squash-message code: a preparatory sequencer:
commit extracts the banner, header and marker-comment helpers so both
rebase and git history squash build the identical template from one
source.
* Refuse a range whose oldest commit is a fixup!, squash! or amend!, since
the marker's target cannot be inside the range.
* Reorder the squash usage so dashed options come before <revision-range>,
and spell out HEAD instead of @ in the documentation and examples.
* Expand the squash commit message and documentation with this overview,
and scope the merge limitation so it no longer contradicts squash folding
a single-base interior merge.
Changes in v6:
* git history squash now accepts multiple revision arguments, read like the
arguments to git-rev-list, so a compound range such as @~3.. ^topic
works.
* The base to reparent onto is now the oldest in-range commit's parent; a
boundary other than that base means the range has more than one base and
is rejected. This also fixes the earlier overly-restrictive handling of
merges and side branches.
* A single-commit range (e.g. @^!) is rejected with "nothing to squash"
(this also covers the @^!-style example that previously succeeded
silently).
* Commit messages reworded: the squash commit now gives an overview of
fixup!/squash!/amend! handling, rewording, merge-parent and ref behavior.
Changes in v5:
* The range walk now uses --ancestry-path, so only commits descended from
the base are folded; a single revision such as HEAD or HEAD~1 is now
rejected as "not a <base>..<tip> range" rather than treated as a squash
down to the root.
* This adopts the --ancestry-path suggestion; the multi-base rejection is
unchanged, so a side branch that forked before the base and merged in is
still refused.
* Added tests covering more merge topologies: two interior merges, a nested
merge, an octopus merge, an octopus arm forked before the base, a merge
among the descendants replayed above the range, and a ref pointing at an
interior merge commit.
Changes in v4:
* git history squash now detects when another ref points at a commit inside
the range being folded and refuses, with an advice.historyUpdateRefs hint
to use --update-refs=head.
* A merge inside the range is folded fine as long as the range has a single
base; a range with merge commit at the tip or base also folds correctly.
Only a range with more than one base is rejected.
Changes in v3:
* Moved the feature out of git rebase and into a new git history squash
<revision-range> subcommand, per the list discussion. git rebase --squash
is dropped.
* Takes an arbitrary range (git history squash @~3.., git history squash
@~5..@~2), folding it into the oldest commit and replaying any
descendants on top.
* Implemented as a single tree operation rather than picking each commit,
so there are no repeated conflict stops (addresses Phillip's efficiency
point).
* A merge inside the range is folded fine, only a range with more than one
base is rejected.
* --reedit-message seeds the editor with every folded-in message, not just
the oldest.
Harald Nordgren (5):
history: extract helper for a commit's parent tree
history: give commit_tree_ext a message template
history: add squash subcommand to fold a range
sequencer: share the squash message marker helpers and flags
history: re-edit a squash with every message
Documentation/config/advice.adoc | 4 +
Documentation/git-history.adoc | 54 ++-
advice.c | 1 +
advice.h | 1 +
builtin/history.c | 522 +++++++++++++++++++--
sequencer.c | 70 +--
sequencer.h | 30 ++
t/meson.build | 1 +
t/t3455-history-squash.sh | 776 +++++++++++++++++++++++++++++++
9 files changed, 1387 insertions(+), 72 deletions(-)
create mode 100755 t/t3455-history-squash.sh
base-commit: f85a7e662054a7b0d9070e432508831afa214b47
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2337%2FHaraldNordgren%2Frebase-fixup-fold-v8
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2337/HaraldNordgren/rebase-fixup-fold-v8
Pull-Request: https://github.com/git/git/pull/2337
Range-diff vs v7:
1: 56ed8fadbb = 1: ba77752282 history: extract helper for a commit's parent tree
2: 212e9c228f = 2: 50f3572887 history: give commit_tree_ext a message template
3: cf3346a1cd ! 3: 2d81a40a05 history: add squash subcommand to fold a range
@@ Commit message
every commit in the range into the oldest one, keeping that commit's
message and authorship and taking the tree of the newest commit, then
replays the commits above the range on top. The squashed message comes
- from the oldest commit, or from an editor with --reedit-message. As that
- message is reused, a range whose oldest commit is a fixup!, squash! or
- amend! is refused, since the marker's target cannot be in the range.
+ from the oldest commit, or from an editor with --reedit-message. A
+ fixup!, squash! or amend! commit is refused unless the commit it targets
+ is also in the range, so the fold does not silently absorb a marker meant
+ for a commit outside it. The check runs the range through
+ todo_list_rearrange_squash(), which leaves such a marker as a plain pick.
+ Markers whose target is in the range fold in as usual. As an exception, a
+ range made up entirely of markers for one target is combined anyway,
+ taking its message from the last amend! if there is one, so a batch of
+ fixups for the same commit can be collapsed.
The range is read like the arguments to "git rev-list", so several
- arguments such as "HEAD~3..HEAD ^topic" are allowed. A merge inside the
- range is folded when its other parent is reachable from the base,
- otherwise the range has more than one base and is rejected. By default
- the command also refuses when a ref points at a commit that the fold
- would discard. Use --update-refs=head to rewrite only the current branch
- instead.
+ revisions such as "HEAD~3..HEAD ^topic" may be given, and rev-list
+ options are accepted too. As "git replay" does, the walk options the fold
+ relies on are forced after setup_revisions() and a warning is printed if
+ an option changed them, so the first commit returned is the range's
+ oldest and its parent is the base regardless of what the user passed
+ (including after a "--"). A merge inside the range is folded when its
+ other parent is reachable from the base, otherwise the range has more
+ than one base and is rejected. By default the command also refuses when a
+ ref points at a commit that the fold would discard. Use --update-refs=head
+ to rewrite only the current branch instead.
Inspired-by: Sergey Chernov <serega.morph@gmail.com>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
@@ Documentation/git-history.adoc: linkgit:gitglossary[7].
+the commit just below the oldest commit to squash. For example, `git
+history squash HEAD~3..HEAD` folds the three most recent commits into
+one, and `git history squash HEAD~5..HEAD~2` squashes an interior range
-+while leaving the two newest commits in place. _<revision-range>_ is read
-+like the arguments to linkgit:git-rev-list[1], so several arguments may be
++while leaving the two newest commits in place. Several revisions may be
+given, for example `HEAD~3..HEAD ^topic` to additionally exclude what is
-+already on `topic`.
++already on `topic`. Rev-list options may also be given, but any that would
++change how the range is walked are overridden with a warning.
++
+The oldest commit's message and authorship are preserved by default,
+unless you specify `--reedit-message`. A merge commit inside the range is
@@ Documentation/git-history.adoc: linkgit:gitglossary[7].
+that reaches more than one entry point (for example a side branch that
+forked before the range and was later merged into it) is rejected.
++
-+Because the oldest commit's message is reused, the range may not begin
-+with a `fixup!`, `squash!`, or `amend!` commit, whose target is
-+necessarily outside the range.
++A `fixup!`, `squash!`, or `amend!` commit is refused unless the commit it
++targets is also in the range, so the fold does not silently absorb a
++marker meant for a commit outside it. As an exception, a range made up
++entirely of markers for one target is combined into a single commit,
++keeping the last `amend!` message if there is one.
++
+A branch or tag that points at a commit inside the range would be left
+dangling once those commits are folded away, so with the default
@@ builtin/history.c: out:
+ int ret;
+
+ repo_init_revisions(repo, &revs, NULL);
++ revs.reverse = 1;
++ revs.topo_order = 1;
++ revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
++ revs.simplify_history = 0;
++ revs.boundary = 1;
++
+ strvec_push(&args, "ignored");
-+ strvec_push(&args, "--reverse");
-+ strvec_push(&args, "--topo-order");
-+ strvec_push(&args, "--boundary");
+ strvec_push(&args, "--ancestry-path");
+ strvec_pushv(&args, argv);
+ setup_revisions_from_strvec(&args, &revs, NULL);
@@ builtin/history.c: out:
+ goto out;
+ }
+
++ if (revs.reverse != 1 || revs.topo_order != 1 ||
++ revs.sort_order != REV_SORT_IN_GRAPH_ORDER ||
++ revs.simplify_history != 0) {
++ warning(_("ignoring rev-list options that would change how the "
++ "range is walked"));
++ revs.reverse = 1;
++ revs.topo_order = 1;
++ revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
++ revs.simplify_history = 0;
++ }
++
+ /*
+ * A squash needs a base to reparent onto, so the range has to exclude
+ * something, as in "<base>..<tip>". A revision range with no such
@@ builtin/history.c: out:
+ goto out;
+ }
+
++ /*
++ * Set boundary commits aside for the base check below, and put every
++ * in-range commit but the tip into the interior set. A ref pointing
++ * at an interior commit would dangle once the range is folded away.
++ */
+ while ((commit = get_revision(&revs))) {
+ if (commit->object.flags & BOUNDARY) {
+ commit_list_insert(commit, &boundaries);
@@ builtin/history.c: out:
+ if (!oldest) {
+ ret = error(_("the revision range is empty"));
+ goto out;
-+ }
-+
-+ if (oldest == tip) {
++ } else if (oldest == tip) {
+ ret = error(_("the revision range holds a single commit; "
+ "nothing to squash"));
+ goto out;
-+ }
-+
-+ if (!oldest->parents)
++ } else if (!oldest->parents) {
+ BUG("an in-range commit must have a parent");
++ }
+ base = oldest->parents->item;
+
+ /*
@@ builtin/history.c: out:
+ return ret;
+}
+
-+static int reject_fixupish_oldest(struct repository *repo,
-+ struct commit *oldest)
++static const char *autosquash_target(const char *subject)
++{
++ const char *rest;
++
++ while (skip_prefix(subject, "fixup! ", &rest) ||
++ skip_prefix(subject, "squash! ", &rest) ||
++ skip_prefix(subject, "amend! ", &rest))
++ subject = rest;
++ return subject;
++}
++
++static int reject_dangling_fixups(struct repository *repo,
++ struct commit *base,
++ struct commit *tip,
++ struct commit *oldest,
++ struct commit **msg_source)
+{
-+ const char *message, *subject;
-+ int ret = 0;
-+
-+ message = repo_logmsg_reencode(repo, oldest, NULL, NULL);
-+ find_commit_subject(message, &subject);
-+ if (starts_with(subject, "fixup! ") ||
-+ starts_with(subject, "squash! ") ||
-+ starts_with(subject, "amend! "))
-+ ret = error(_("the range begins with a fixup!, squash! or amend! "
-+ "commit whose target is not in the range"));
-+ repo_unuse_commit_buffer(repo, oldest, message);
++ struct todo_list todo = TODO_LIST_INIT;
++ struct replay_opts opts = REPLAY_OPTS_INIT;
++ struct rev_info revs;
++ struct commit *commit, *last_amend = NULL;
++ struct strvec args = STRVEC_INIT;
++ char *dangling_subject = NULL, *dangling_target = NULL;
++ bool mixed_target = false, all_fixups_one_target;
++ int i, ret, nr_dangling = 0;
++
++ *msg_source = oldest;
++
++ repo_init_revisions(repo, &revs, NULL);
++ strvec_push(&args, "ignored");
++ strvec_push(&args, "--reverse");
++ strvec_push(&args, "--topo-order");
++ strvec_pushf(&args, "%s..%s", oid_to_hex(&base->object.oid),
++ oid_to_hex(&tip->object.oid));
++ setup_revisions_from_strvec(&args, &revs, NULL);
++
++ if (prepare_revision_walk(&revs) < 0) {
++ ret = error(_("error preparing revisions"));
++ goto out;
++ }
++ while ((commit = get_revision(&revs)))
++ strbuf_addf(&todo.buf, "pick %s\n",
++ oid_to_hex(&commit->object.oid));
++
++ if (todo_list_parse_insn_buffer(repo, &opts, todo.buf.buf, &todo) < 0 ||
++ todo_list_rearrange_squash(&todo) < 0) {
++ ret = error(_("could not check the range for fixups"));
++ goto out;
++ }
++
++ for (i = 0; i < todo.nr; i++) {
++ const char *message, *subject_start, *target;
++ char *subject;
++ size_t sublen;
++
++ if (todo.items[i].command != TODO_PICK)
++ continue;
++ message = repo_logmsg_reencode(repo, todo.items[i].commit,
++ NULL, NULL);
++ sublen = find_commit_subject(message, &subject_start);
++ subject = xmemdupz(subject_start, sublen);
++ target = autosquash_target(subject);
++ if (target != subject) {
++ nr_dangling++;
++ if (!dangling_target) {
++ dangling_target = xstrdup(target);
++ dangling_subject = xstrdup(subject);
++ } else if (strcmp(dangling_target, target)) {
++ mixed_target = true;
++ }
++ if (starts_with(subject, "amend! "))
++ last_amend = todo.items[i].commit;
++ }
++ free(subject);
++ repo_unuse_commit_buffer(repo, todo.items[i].commit, message);
++ }
++
++ all_fixups_one_target = nr_dangling == todo.nr && !mixed_target;
++ if (nr_dangling && !all_fixups_one_target) {
++ ret = error(_("cannot squash '%s': its target is not in the "
++ "range"), dangling_subject);
++ } else {
++ if (last_amend)
++ *msg_source = last_amend;
++ ret = 0;
++ }
++
++out:
++ free(dangling_subject);
++ free(dangling_target);
++ todo_list_release(&todo);
++ replay_opts_release(&opts);
++ reset_revision_walk();
++ release_revisions(&revs);
++ strvec_clear(&args);
+ return ret;
+}
+
@@ builtin/history.c: out:
+ };
+ struct strbuf reflog_msg = STRBUF_INIT;
+ struct oidset interior = OIDSET_INIT;
-+ struct commit *base, *oldest, *tip, *rewritten;
++ struct commit *base, *oldest, *tip, *rewritten, *msg_source;
+ const struct object_id *base_tree_oid, *tip_tree_oid;
+ struct commit_list *parents = NULL;
+ struct rev_info revs = { 0 };
+ int ret;
+
-+ argc = parse_options(argc, argv, prefix, options, usage, 0);
++ argc = parse_options(argc, argv, prefix, options, usage,
++ PARSE_OPT_KEEP_UNKNOWN_OPT);
+ if (!argc) {
+ ret = error(_("command expects a revision range"));
+ goto out;
@@ builtin/history.c: out:
+ if (ret < 0)
+ goto out;
+
-+ ret = reject_fixupish_oldest(repo, oldest);
++ ret = reject_dangling_fixups(repo, base, tip, oldest, &msg_source);
+ if (ret < 0)
+ goto out;
+
@@ builtin/history.c: out:
+ tip_tree_oid = &repo_get_commit_tree(repo, tip)->object.oid;
+ commit_list_append(base, &parents);
+
-+ ret = commit_tree_ext(repo, "squash", oldest, NULL, parents,
++ ret = commit_tree_ext(repo, "squash", msg_source, NULL, parents,
+ base_tree_oid, tip_tree_oid, &rewritten, flags);
+ if (ret < 0) {
+ ret = error(_("failed writing squashed commit"));
@@ t/t3455-history-squash.sh (new)
+ test_grep "squash: updating" reflog
+'
+
++test_expect_success 'sanitizes rev-list walk options, before and after --' '
++ git reset --hard three &&
++ tip_tree=$(git rev-parse HEAD^{tree}) &&
++
++ git history squash --date-order start.. 2>err &&
++ test_grep "ignoring rev-list options" err &&
++ test_cmp_rev start HEAD^ &&
++ test "$tip_tree" = "$(git rev-parse HEAD^{tree})" &&
++
++ git reset --hard three &&
++ git history squash -- --reverse start.. 2>err &&
++ test_grep "ignoring rev-list options" err &&
++ test_cmp_rev start HEAD^ &&
++ test "$tip_tree" = "$(git rev-parse HEAD^{tree})"
++'
++
+test_expect_success 'squashes an interior range and replays descendants verbatim' '
+ git reset --hard three &&
+ final_tree=$(git rev-parse HEAD^{tree}) &&
@@ t/t3455-history-squash.sh (new)
+'
+
+
-+test_expect_success 'reuses the message of a fixup! commit in the range' '
++test_expect_success 'folds fixups whose target is in the range' '
+ git reset --hard start &&
-+ test_commit --no-tag reg1 file b &&
-+ git commit --allow-empty -m "fixup! reg1" &&
-+ test_commit reg2 file c &&
++ test_commit --no-tag target file b &&
++ git commit --allow-empty -m "fixup! target" &&
++ git commit --allow-empty -m "fixup! target" &&
++ test_commit --no-tag later file c &&
+
+ git history squash start.. &&
+
++ git rev-list --count start..HEAD >count &&
++ echo 1 >expect &&
++ test_cmp expect count &&
+ git log --format="%s" -1 >actual &&
-+ echo reg1 >expect &&
++ echo target >expect &&
+ test_cmp expect actual
+'
+
-+test_expect_success 'refuses a range whose oldest commit is a fixup!' '
++test_expect_success 'refuses a below-range fixup! after an in-range commit' '
+ git reset --hard start &&
-+ test_commit --no-tag "fixup! something" file b &&
-+ test_commit --no-tag tail file c &&
++ test_commit --no-tag inside file b &&
++ test_commit --no-tag "fixup! outside" file c &&
+ head_before=$(git rev-parse HEAD) &&
+
+ test_must_fail git history squash start.. 2>err &&
@@ t/t3455-history-squash.sh (new)
+ test_cmp_rev "$head_before" HEAD
+'
+
-+test_expect_success 'does not interpret squash! or amend! markers' '
++test_expect_success 'combines a run of fixups for one commit below the range' '
++ git reset --hard start &&
++ echo b >file && git add file && git commit -m "fixup! base" &&
++ echo c >file && git add file && git commit -m "fixup! base" &&
++
++ git history squash start.. &&
++
++ git rev-list --count start..HEAD >count &&
++ echo 1 >expect &&
++ test_cmp expect count &&
++ git log --format="%s" -1 >actual &&
++ echo "fixup! base" >expect &&
++ test_cmp expect actual
++'
++
++test_expect_success 'combining below-range fixups keeps the last amend! message' '
++ git reset --hard start &&
++ echo b >file && git add file && git commit -m "fixup! base" &&
++ printf "amend! base\n\namended body\n" >msg &&
++ echo c >file && git add file && git commit -qF msg &&
++
++ git history squash start.. &&
++
++ git rev-list --count start..HEAD >count &&
++ echo 1 >expect &&
++ test_cmp expect count &&
++ git log --format="%s" -1 >actual &&
++ echo "amend! base" >expect &&
++ test_cmp expect actual &&
++ git log --format="%b" -1 >body &&
++ test_grep "amended body" body
++'
++
++test_expect_success 'refuses fixups for two different commits below the range' '
++ git reset --hard start &&
++ echo b >file && git add file && git commit -m "fixup! aaa" &&
++ echo c >file && git add file && git commit -m "fixup! bbb" &&
++ head_before=$(git rev-parse HEAD) &&
++
++ test_must_fail git history squash start.. 2>err &&
++ test_grep "target is not in the range" err &&
++ test_cmp_rev "$head_before" HEAD
++'
++
++test_expect_success 'keeps the oldest message for in-range squash! and amend!' '
+ git reset --hard start &&
+ test_commit --no-tag marker-oldest file b &&
+ git commit --allow-empty -m "squash! marker-oldest" &&
@@ t/t3455-history-squash.sh (new)
+test_expect_success 'advice.historyUpdateRefs silences the hint' '
+ git reset --hard three &&
+ git branch -f mid HEAD~1 &&
++ head_before=$(git rev-parse HEAD) &&
+
+ test_must_fail git -c advice.historyUpdateRefs=false \
+ history squash start.. 2>err &&
+ test_grep "points into the squashed range" err &&
+ test_grep ! "hint:" err &&
++ test_cmp_rev "$head_before" HEAD &&
+
+ git branch -D mid
+'
@@ t/t3455-history-squash.sh (new)
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})"
+'
+
-+test_expect_success 'refuses to squash a range with more than one base' '
-+ git reset --hard start &&
-+ main=$(git symbolic-ref --short HEAD) &&
-+ git checkout -b forked-before &&
-+ test_commit forked-side fside x &&
-+ git checkout "$main" &&
-+ test_commit forked-base file b &&
-+ base=$(git rev-parse HEAD) &&
-+ test_commit forked-main file c &&
-+ git merge --no-ff -m merge forked-before &&
-+ merged=$(git rev-parse HEAD) &&
-+
-+ test_must_fail git history squash "$base.." 2>err &&
-+ test_grep "more than one base" err &&
-+ test_cmp_rev "$merged" HEAD
-+'
-+
+test_expect_success 'folds a range with two interior merges' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
4: 001356db93 ! 4: 0a735117ad sequencer: extract helpers for the squash message markers
@@ Metadata
Author: Harald Nordgren <haraldnordgren@gmail.com>
## Commit message ##
- sequencer: extract helpers for the squash message markers
+ sequencer: share the squash message marker helpers and flags
When "git rebase -i" squashes commits it builds an editor template with a
"This is a combination of N commits." banner, a "This is the 1st/Nth
@@ Commit message
Pull the three pieces out into add_squash_combination_header(),
add_squash_message_header() (which takes a flag for the "will be skipped"
variant) and squash_subject_comment_len(), and use them from
- update_squash_messages() and append_squash_message(). A later change
- reuses them to give "git history squash --reedit-message" the same
- template.
+ update_squash_messages() and append_squash_message(). Also move the
+ todo_item_flags enum to the header, so a caller reading the output of
+ todo_list_rearrange_squash() can tell an amend! (TODO_REPLACE_FIXUP_MSG)
+ from a plain fixup!. A later change reuses all of this to give "git
+ history squash --reedit-message" the same template.
No change in behavior.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
## sequencer.c ##
-@@ sequencer.c: static const char skip_first_commit_msg_str[] = N_("The 1st commit message will
+@@ sequencer.c: static int is_pick_or_similar(enum todo_command command)
+ }
+ }
+
+-enum todo_item_flags {
+- TODO_EDIT_MERGE_MSG = (1 << 0),
+- TODO_REPLACE_FIXUP_MSG = (1 << 1),
+- TODO_EDIT_FIXUP_MSG = (1 << 2),
+-};
+-
+ static const char first_commit_msg_str[] = N_("This is the 1st commit message:");
+ static const char nth_commit_msg_fmt[] = N_("This is the commit message #%d:");
+ static const char skip_first_commit_msg_str[] = N_("The 1st commit message will be skipped:");
static const char skip_nth_commit_msg_fmt[] = N_("The commit message #%d will be skipped:");
static const char combined_commit_msg_fmt[] = N_("This is a combination of %d commits.");
@@ sequencer.c: static int update_squash_messages(struct repository *r,
comment_line_str);
## sequencer.h ##
+@@ sequencer.h: enum todo_command {
+ TODO_COMMENT
+ };
+
++/* Bits for the "flags" member of struct todo_item */
++enum todo_item_flags {
++ TODO_EDIT_MERGE_MSG = (1 << 0),
++ TODO_REPLACE_FIXUP_MSG = (1 << 1),
++ TODO_EDIT_FIXUP_MSG = (1 << 2),
++};
++
+ struct todo_item {
+ enum todo_command command;
+ struct commit *commit;
@@ sequencer.h: int todo_list_rearrange_squash(struct todo_list *todo_list);
*/
void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);
5: 615fe4dd3f ! 5: baf7e6f0a6 history: re-edit a squash with every message
@@ Commit message
By default "git history squash" reuses the oldest commit's message.
When --reedit-message is given it only reopened that one message, so the
- messages of the folded-in commits were lost.
+ messages of the other commits in the range were lost.
- Gather the messages of every commit in the range, oldest first, and build
- the same editor template that "git rebase -i" shows for a squash, using
+ Gather the message of every commit in the range and build the same editor
+ template that "git rebase -i --autosquash" shows for a squash, reusing
add_squash_combination_header(), add_squash_message_header() and
- squash_subject_comment_len(). Only the message text differs, the changes
- are always folded in. Following autosquash, a fixup!'s message is
- commented out in full under a "will be skipped" header, while a squash! or
- amend! keeps its body with only the marker subject commented.
+ squash_subject_comment_len(). Feed the range through
+ todo_list_rearrange_squash() so that each fixup!, squash! or amend! is
+ grouped under the commit it targets rather than shown in commit order,
+ exactly as autosquash would arrange them.
+
+ Only the message text differs, the changes are always folded in. A fixup!
+ message is commented out in full under a "will be skipped" header, a
+ squash! keeps its body with only the marker subject commented, and an
+ amend! replaces its target's message unless a squash! already folded into
+ that target, in which case it behaves like a squash!.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
## Documentation/git-history.adoc ##
-@@ Documentation/git-history.adoc: like the arguments to linkgit:git-rev-list[1], so several arguments may be
- given, for example `HEAD~3..HEAD ^topic` to additionally exclude what is
- already on `topic`.
+@@ Documentation/git-history.adoc: given, for example `HEAD~3..HEAD ^topic` to additionally exclude what is
+ already on `topic`. Rev-list options may also be given, but any that would
+ change how the range is walked are overridden with a warning.
+
-The oldest commit's message and authorship are preserved by default,
-unless you specify `--reedit-message`. A merge commit inside the range is
@@ Documentation/git-history.adoc: like the arguments to linkgit:git-rev-list[1], s
folded like any other, but the range must have a single base, so a range
that reaches more than one entry point (for example a side branch that
forked before the range and was later merged into it) is rejected.
- +
- Because the oldest commit's message is reused, the range may not begin
- with a `fixup!`, `squash!`, or `amend!` commit, whose target is
--necessarily outside the range.
-+necessarily outside the range. The changes from every commit in the range
-+are always folded in. Only the message text differs. With
-+`--reedit-message` the template mirrors `git rebase -i`: the message of a
-+`fixup!` elsewhere in the range is commented out in full, while a
-+`squash!` or `amend!` keeps its message body with only the marker subject
-+commented, so you can fold the remark into the result.
+@@ Documentation/git-history.adoc: A `fixup!`, `squash!`, or `amend!` commit is refused unless the commit it
+ targets is also in the range, so the fold does not silently absorb a
+ marker meant for a commit outside it. As an exception, a range made up
+ entirely of markers for one target is combined into a single commit,
+-keeping the last `amend!` message if there is one.
++keeping the last `amend!` message if there is one. The changes from every
++commit in the range are always folded in. Only the message text differs.
++With `--reedit-message` the template mirrors `git rebase -i --autosquash`:
++each `fixup!`, `squash!`, or `amend!` is grouped under the commit it
++targets rather than shown in commit order. A `fixup!` message is dropped
++(commented out in full), a `squash!` keeps its body with only the marker
++subject commented, and an `amend!` replaces its target's message, unless
++a `squash!` folded into that target first, in which case it keeps its
++body like a `squash!`.
+
A branch or tag that points at a commit inside the range would be left
dangling once those commits are folded away, so with the default
@@ builtin/history.c: static int find_interior_ref(const struct reference *ref, voi
return 0;
}
++static bool amend_replaces_target(struct todo_list *todo, int target)
++{
++ int i;
++
++ for (i = target + 1; i < todo->nr &&
++ todo->items[i].command != TODO_PICK; i++) {
++ if (todo->items[i].command == TODO_SQUASH)
++ return false;
++ if (todo->items[i].flags & TODO_REPLACE_FIXUP_MSG)
++ return true;
++ }
++ return false;
++}
++
+static int build_squash_message(struct repository *repo,
+ struct commit *base,
+ struct commit *tip,
+ struct strbuf *out)
+{
-+ struct commit_list *commits = NULL, **tail = &commits, *c;
+ struct rev_info revs;
+ struct commit *commit;
+ struct strvec args = STRVEC_INIT;
-+ int n = 0, total, ret;
++ struct todo_list todo = TODO_LIST_INIT;
++ struct replay_opts opts = REPLAY_OPTS_INIT;
++ int i, nr_commits, ret;
+
+ repo_init_revisions(repo, &revs, NULL);
+ strvec_push(&args, "ignored");
@@ builtin/history.c: static int find_interior_ref(const struct reference *ref, voi
+ }
+
+ while ((commit = get_revision(&revs)))
-+ tail = &commit_list_insert(commit, tail)->next;
-+ total = commit_list_count(commits);
++ strbuf_addf(&todo.buf, "pick %s\n",
++ oid_to_hex(&commit->object.oid));
+
-+ for (c = commits; c; c = c->next) {
++ if (todo_list_parse_insn_buffer(repo, &opts, todo.buf.buf, &todo) < 0 ||
++ todo_list_rearrange_squash(&todo) < 0) {
++ ret = error(_("could not prepare the squash message"));
++ goto out;
++ }
++
++ nr_commits = todo.nr;
++ for (i = 0; i < nr_commits; i++) {
++ struct todo_item *item = &todo.items[i];
+ const char *message, *body;
+ size_t commented_len;
-+ int skip;
++ bool skip, squashing;
+
-+ message = repo_logmsg_reencode(repo, c->item, NULL, NULL);
++ squashing = item->command == TODO_SQUASH ||
++ (item->flags & TODO_REPLACE_FIXUP_MSG);
++ if (item->command == TODO_PICK)
++ skip = amend_replaces_target(&todo, i);
++ else
++ skip = !squashing;
++
++ message = repo_logmsg_reencode(repo, item->commit, NULL, NULL);
+ find_commit_subject(message, &body);
+
-+ skip = starts_with(body, "fixup! ");
-+ commented_len = skip ? strlen(body) :
-+ squash_subject_comment_len(body, 1);
++ if (skip)
++ commented_len = strlen(body);
++ else if (squashing)
++ commented_len = squash_subject_comment_len(body, 1);
++ else
++ commented_len = 0;
+
-+ if (!n)
-+ add_squash_combination_header(out, total);
++ if (!i)
++ add_squash_combination_header(out, nr_commits);
+ strbuf_addch(out, '\n');
-+ add_squash_message_header(out, ++n, skip);
++ add_squash_message_header(out, i + 1, skip);
+ strbuf_addstr(out, "\n\n");
+ strbuf_add_commented_lines(out, body, commented_len, comment_line_str);
+ strbuf_addstr(out, body + commented_len);
+ strbuf_complete_line(out);
+
-+ repo_unuse_commit_buffer(repo, c->item, message);
++ repo_unuse_commit_buffer(repo, item->commit, message);
+ }
+
+ ret = 0;
+
+out:
-+ commit_list_free(commits);
++ todo_list_release(&todo);
++ replay_opts_release(&opts);
+ reset_revision_walk();
+ release_revisions(&revs);
+ strvec_clear(&args);
@@ builtin/history.c: static int cmd_history_squash(int argc,
struct strbuf reflog_msg = STRBUF_INIT;
+ struct strbuf message = STRBUF_INIT;
struct oidset interior = OIDSET_INIT;
- struct commit *base, *oldest, *tip, *rewritten;
+ struct commit *base, *oldest, *tip, *rewritten, *msg_source;
const struct object_id *base_tree_oid, *tip_tree_oid;
@@ builtin/history.c: static int cmd_history_squash(int argc,
}
@@ builtin/history.c: static int cmd_history_squash(int argc,
tip_tree_oid = &repo_get_commit_tree(repo, tip)->object.oid;
commit_list_append(base, &parents);
-- ret = commit_tree_ext(repo, "squash", oldest, NULL, parents,
-+ ret = commit_tree_ext(repo, "squash", oldest,
+- ret = commit_tree_ext(repo, "squash", msg_source, NULL, parents,
++ ret = commit_tree_ext(repo, "squash", msg_source,
+ message.len ? message.buf : NULL, parents,
base_tree_oid, tip_tree_oid, &rewritten, flags);
if (ret < 0) {
@@ t/t3455-history-squash.sh: test_expect_success 'preserves authorship of the olde
+ test_grep "amended message" final
+'
+
++test_expect_success '--reedit-message groups fixups under their targets' '
++ git reset --hard start &&
++ test_commit --no-tag alpha file a1 &&
++ test_commit --no-tag beta file b1 &&
++ printf "fixup! alpha\n" >msg &&
++ echo a2 >file &&
++ git add file &&
++ git commit -qF msg &&
++ printf "fixup! beta\n" >msg &&
++ echo b2 >file &&
++ git add file &&
++ git commit -qF msg &&
++
++ write_script editor <<-\EOF &&
++ cat "$1" >edited
++ EOF
++ test_set_editor "$(pwd)/editor" &&
++ git history squash --reedit-message start.. &&
++
++ cat >expect <<-EOF &&
++ # This is a combination of 4 commits.
++ # This is the 1st commit message:
++
++ alpha
++
++ # The commit message #2 will be skipped:
++
++ # fixup! alpha
++
++ # This is the commit message #3:
++
++ beta
++
++ # The commit message #4 will be skipped:
++
++ # fixup! beta
++
++ # Please enter the commit message for the squash changes. Lines starting
++ # with ${SQ}#${SQ} will be ignored, and an empty message aborts the commit.
++ # Changes to be committed:
++ # modified: file
++ #
++ EOF
++ test_cmp expect edited
++'
++
++test_expect_success '--reedit-message lets amend! replace its target message' '
++ git reset --hard start &&
++ test_commit --no-tag mark-base file b &&
++ printf "amend! mark-base\n\namended message\n" >msg &&
++ echo c >file &&
++ git add file &&
++ git commit -qF msg &&
++ printf "squash! mark-base\n\nsquash remark\n" >msg &&
++ echo d >file &&
++ git add file &&
++ git commit -qF msg &&
++
++ write_script editor <<-\EOF &&
++ cat "$1" >edited
++ EOF
++ test_set_editor "$(pwd)/editor" &&
++ git history squash --reedit-message start.. &&
++
++ cat >expect <<-EOF &&
++ # This is a combination of 3 commits.
++ # The 1st commit message will be skipped:
++
++ # mark-base
++
++ # This is the commit message #2:
++
++ # amend! mark-base
++
++ amended message
++
++ # This is the commit message #3:
++
++ # squash! mark-base
++
++ squash remark
++
++ # Please enter the commit message for the squash changes. Lines starting
++ # with ${SQ}#${SQ} will be ignored, and an empty message aborts the commit.
++ # Changes to be committed:
++ # modified: file
++ #
++ EOF
++ test_cmp expect edited &&
++ git log -1 --format="%B" >final &&
++ test_grep ! "mark-base" final &&
++ test_grep "amended message" final &&
++ test_grep "squash remark" final
++'
++
+test_expect_success '--reedit-message aborts on an empty message' '
+ git reset --hard three &&
+ head_before=$(git rev-parse HEAD) &&
--
gitgitgadget
^ permalink raw reply
* [PATCH v8 1/5] history: extract helper for a commit's parent tree
From: Harald Nordgren via GitGitGadget @ 2026-07-10 9:06 UTC (permalink / raw)
To: git
Cc: Phillip Wood, D. Ben Knoble, Patrick Steinhardt, Matt Hunter,
Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2337.v8.git.git.1783674396.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
Three places resolve the tree of a commit's first parent, falling back
to the empty tree for a root commit, each repeating the same parse and
oidcpy dance. Extract a first_parent_tree_oid() helper and route the
existing callers through it.
No change in behavior.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
builtin/history.c | 58 +++++++++++++++++++++--------------------------
1 file changed, 26 insertions(+), 32 deletions(-)
diff --git a/builtin/history.c b/builtin/history.c
index fd83de8265..9f516687fe 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -157,6 +157,25 @@ out:
return ret;
}
+static int first_parent_tree_oid(struct repository *repo,
+ struct commit *commit,
+ struct object_id *out)
+{
+ struct commit *parent = commit->parents ? commit->parents->item : NULL;
+
+ if (!parent) {
+ oidcpy(out, repo->hash_algo->empty_tree);
+ return 0;
+ }
+
+ if (repo_parse_commit(repo, parent))
+ return error(_("unable to parse parent commit %s"),
+ oid_to_hex(&parent->object.oid));
+
+ oidcpy(out, &repo_get_commit_tree(repo, parent)->object.oid);
+ return 0;
+}
+
static int commit_tree_with_edited_message(struct repository *repo,
const char *action,
struct commit *original,
@@ -164,21 +183,11 @@ static int commit_tree_with_edited_message(struct repository *repo,
{
struct object_id parent_tree_oid;
const struct object_id *tree_oid;
- struct commit *parent;
tree_oid = &repo_get_commit_tree(repo, original)->object.oid;
- parent = original->parents ? original->parents->item : NULL;
- if (parent) {
- if (repo_parse_commit(repo, parent)) {
- return error(_("unable to parse parent commit %s"),
- oid_to_hex(&parent->object.oid));
- }
-
- parent_tree_oid = repo_get_commit_tree(repo, parent)->object.oid;
- } else {
- oidcpy(&parent_tree_oid, repo->hash_algo->empty_tree);
- }
+ if (first_parent_tree_oid(repo, original, &parent_tree_oid) < 0)
+ return -1;
return commit_tree_ext(repo, action, original, original->parents,
&parent_tree_oid, tree_oid, out, COMMIT_TREE_EDIT_MESSAGE);
@@ -444,18 +453,10 @@ static int commit_became_empty(struct repository *repo,
struct commit *original,
struct tree *result)
{
- struct commit *parent = original->parents ? original->parents->item : NULL;
struct object_id parent_tree_oid;
- if (parent) {
- if (repo_parse_commit(repo, parent))
- return error(_("unable to parse parent of %s"),
- oid_to_hex(&original->object.oid));
-
- parent_tree_oid = repo_get_commit_tree(repo, parent)->object.oid;
- } else {
- oidcpy(&parent_tree_oid, repo->hash_algo->empty_tree);
- }
+ if (first_parent_tree_oid(repo, original, &parent_tree_oid) < 0)
+ return -1;
return oideq(&result->object.oid, &parent_tree_oid);
}
@@ -799,16 +800,9 @@ static int split_commit(struct repository *repo,
struct tree *split_tree;
int ret;
- if (original->parents) {
- if (repo_parse_commit(repo, original->parents->item)) {
- ret = error(_("unable to parse parent commit %s"),
- oid_to_hex(&original->parents->item->object.oid));
- goto out;
- }
-
- parent_tree_oid = *get_commit_tree_oid(original->parents->item);
- } else {
- oidcpy(&parent_tree_oid, repo->hash_algo->empty_tree);
+ if (first_parent_tree_oid(repo, original, &parent_tree_oid) < 0) {
+ ret = -1;
+ goto out;
}
original_commit_tree_oid = get_commit_tree_oid(original);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v8 2/5] history: give commit_tree_ext a message template
From: Harald Nordgren via GitGitGadget @ 2026-07-10 9:06 UTC (permalink / raw)
To: git
Cc: Phillip Wood, D. Ben Knoble, Patrick Steinhardt, Matt Hunter,
Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2337.v8.git.git.1783674396.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
commit_tree_ext() reuses the message of the commit it is handed. A
caller that folds several commits together wants to seed the message
from more than that single commit, so add an optional message_template
parameter. When NULL, the behavior is unchanged.
Pass NULL from the existing fixup and split callers.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
builtin/history.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/builtin/history.c b/builtin/history.c
index 9f516687fe..cbba25096f 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -101,6 +101,7 @@ enum commit_tree_flags {
static int commit_tree_ext(struct repository *repo,
const char *action,
struct commit *commit_with_message,
+ const char *message_template,
const struct commit_list *parents,
const struct object_id *old_tree,
const struct object_id *new_tree,
@@ -130,13 +131,16 @@ static int commit_tree_ext(struct repository *repo,
original_author = xmemdupz(ptr, len);
find_commit_subject(original_message, &original_body);
+ if (!message_template)
+ message_template = original_body;
+
if (flags & COMMIT_TREE_EDIT_MESSAGE) {
ret = fill_commit_message(repo, old_tree, new_tree,
- original_body, action, &commit_message);
+ message_template, action, &commit_message);
if (ret < 0)
goto out;
} else {
- strbuf_addstr(&commit_message, original_body);
+ strbuf_addstr(&commit_message, message_template);
}
original_extra_headers = read_commit_extra_headers(commit_with_message,
@@ -189,7 +193,7 @@ static int commit_tree_with_edited_message(struct repository *repo,
if (first_parent_tree_oid(repo, original, &parent_tree_oid) < 0)
return -1;
- return commit_tree_ext(repo, action, original, original->parents,
+ return commit_tree_ext(repo, action, original, NULL, original->parents,
&parent_tree_oid, tree_oid, out, COMMIT_TREE_EDIT_MESSAGE);
}
@@ -644,7 +648,7 @@ static int cmd_history_fixup(int argc,
goto out;
if (!skip_commit) {
- ret = commit_tree_ext(repo, "fixup", original, original->parents,
+ ret = commit_tree_ext(repo, "fixup", original, NULL, original->parents,
&original_tree->object.oid, &merge_result.tree->object.oid,
&rewritten, flags);
if (ret < 0) {
@@ -855,7 +859,7 @@ static int split_commit(struct repository *repo,
* The first commit is constructed from the split-out tree. The base
* that shall be diffed against is the parent of the original commit.
*/
- ret = commit_tree_ext(repo, "split-out", original, original->parents, &parent_tree_oid,
+ ret = commit_tree_ext(repo, "split-out", original, NULL, original->parents, &parent_tree_oid,
&split_tree->object.oid, &first_commit, COMMIT_TREE_EDIT_MESSAGE);
if (ret < 0) {
ret = error(_("failed writing first commit"));
@@ -872,7 +876,7 @@ static int split_commit(struct repository *repo,
old_tree_oid = &repo_get_commit_tree(repo, first_commit)->object.oid;
new_tree_oid = &repo_get_commit_tree(repo, original)->object.oid;
- ret = commit_tree_ext(repo, "split-out", original, parents, old_tree_oid,
+ ret = commit_tree_ext(repo, "split-out", original, NULL, parents, old_tree_oid,
new_tree_oid, &second_commit, COMMIT_TREE_EDIT_MESSAGE);
if (ret < 0) {
ret = error(_("failed writing second commit"));
--
gitgitgadget
^ permalink raw reply related
* [PATCH v8 3/5] history: add squash subcommand to fold a range
From: Harald Nordgren via GitGitGadget @ 2026-07-10 9:06 UTC (permalink / raw)
To: git
Cc: Phillip Wood, D. Ben Knoble, Patrick Steinhardt, Matt Hunter,
Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2337.v8.git.git.1783674396.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
Folding a series of commits into one required either an interactive
rebase where each commit after the first was hand-edited to "fixup", or
a "git reset --soft" to the merge base followed by "git commit --amend".
Add "git history squash <revision-range>" to do this directly. It folds
every commit in the range into the oldest one, keeping that commit's
message and authorship and taking the tree of the newest commit, then
replays the commits above the range on top. The squashed message comes
from the oldest commit, or from an editor with --reedit-message. A
fixup!, squash! or amend! commit is refused unless the commit it targets
is also in the range, so the fold does not silently absorb a marker meant
for a commit outside it. The check runs the range through
todo_list_rearrange_squash(), which leaves such a marker as a plain pick.
Markers whose target is in the range fold in as usual. As an exception, a
range made up entirely of markers for one target is combined anyway,
taking its message from the last amend! if there is one, so a batch of
fixups for the same commit can be collapsed.
The range is read like the arguments to "git rev-list", so several
revisions such as "HEAD~3..HEAD ^topic" may be given, and rev-list
options are accepted too. As "git replay" does, the walk options the fold
relies on are forced after setup_revisions() and a warning is printed if
an option changed them, so the first commit returned is the range's
oldest and its parent is the base regardless of what the user passed
(including after a "--"). A merge inside the range is folded when its
other parent is reachable from the base, otherwise the range has more
than one base and is rejected. By default the command also refuses when a
ref points at a commit that the fold would discard. Use --update-refs=head
to rewrite only the current branch instead.
Inspired-by: Sergey Chernov <serega.morph@gmail.com>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Documentation/config/advice.adoc | 4 +
Documentation/git-history.adoc | 45 ++-
advice.c | 1 +
advice.h | 1 +
builtin/history.c | 343 +++++++++++++++++++
t/meson.build | 1 +
t/t3455-history-squash.sh | 566 +++++++++++++++++++++++++++++++
7 files changed, 958 insertions(+), 3 deletions(-)
create mode 100755 t/t3455-history-squash.sh
diff --git a/Documentation/config/advice.adoc b/Documentation/config/advice.adoc
index c3c190ba6a..cd27f7e795 100644
--- a/Documentation/config/advice.adoc
+++ b/Documentation/config/advice.adoc
@@ -59,6 +59,10 @@ all advice messages.
forceDeleteBranch::
Shown when the user tries to delete a not fully merged
branch without the force option set.
+ historyUpdateRefs::
+ Shown when `git history squash` refuses because a ref points
+ into the range being folded, to tell the user about
+ `--update-refs=head`.
ignoredHook::
Shown when a hook is ignored because the hook is not
set as executable.
diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc
index 2ba8121795..ad921ac2be 100644
--- a/Documentation/git-history.adoc
+++ b/Documentation/git-history.adoc
@@ -11,6 +11,7 @@ SYNOPSIS
git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)]
git history reword <commit> [--dry-run] [--update-refs=(branches|head)]
git history split <commit> [--dry-run] [--update-refs=(branches|head)] [--] [<pathspec>...]
+git history squash [--dry-run] [--update-refs=(branches|head)] [--reedit-message] <revision-range>
DESCRIPTION
-----------
@@ -42,8 +43,11 @@ at once.
LIMITATIONS
-----------
-This command does not (yet) work with histories that contain merges. You
-should use linkgit:git-rebase[1] with the `--rebase-merges` flag instead.
+This command does not (yet) replay merge commits onto the rewritten
+history: if a commit that would be replayed is a merge, the operation is
+rejected, and you should use linkgit:git-rebase[1] with the
+`--rebase-merges` flag instead. The `squash` subcommand can still fold a
+merge that lies inside the range, as long as the range has a single base.
Furthermore, the command does not support operations that can result in merge
conflicts. This limitation is by design as history rewrites are not intended to
@@ -97,6 +101,40 @@ linkgit:gitglossary[7].
It is invalid to select either all or no hunks, as that would lead to
one of the commits becoming empty.
+`squash <revision-range>`::
+ Fold all commits in _<revision-range>_ into the oldest commit of that
+ range. The resulting commit keeps the oldest commit's message and
+ authorship and takes the tree of the range's newest commit, so the
+ whole range collapses into a single commit. Commits above the range
+ are replayed on top of the result.
++
+The range is given in the usual `<base>..<tip>` form, where _<base>_ is
+the commit just below the oldest commit to squash. For example, `git
+history squash HEAD~3..HEAD` folds the three most recent commits into
+one, and `git history squash HEAD~5..HEAD~2` squashes an interior range
+while leaving the two newest commits in place. Several revisions may be
+given, for example `HEAD~3..HEAD ^topic` to additionally exclude what is
+already on `topic`. Rev-list options may also be given, but any that would
+change how the range is walked are overridden with a warning.
++
+The oldest commit's message and authorship are preserved by default,
+unless you specify `--reedit-message`. A merge commit inside the range is
+folded like any other, but the range must have a single base, so a range
+that reaches more than one entry point (for example a side branch that
+forked before the range and was later merged into it) is rejected.
++
+A `fixup!`, `squash!`, or `amend!` commit is refused unless the commit it
+targets is also in the range, so the fold does not silently absorb a
+marker meant for a commit outside it. As an exception, a range made up
+entirely of markers for one target is combined into a single commit,
+keeping the last `amend!` message if there is one.
++
+A branch or tag that points at a commit inside the range would be left
+dangling once those commits are folded away, so with the default
+`--update-refs=branches` the command refuses. Rerun with
+`--update-refs=head` to rewrite only the current branch and leave such
+refs pointing at the old commits.
+
OPTIONS
-------
@@ -107,7 +145,8 @@ OPTIONS
ref updates is generally safe.
`--reedit-message`::
- Open an editor to modify the target commit's message.
+ Open an editor to modify the rewritten commit's message. For `squash`
+ the editor is pre-filled with the messages of all the folded commits.
`--empty=(drop|keep|abort)`::
Control what happens when a commit becomes empty as a result of the
diff --git a/advice.c b/advice.c
index 0018501b7b..5c6ff95e31 100644
--- a/advice.c
+++ b/advice.c
@@ -58,6 +58,7 @@ static struct {
[ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates" },
[ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch" },
[ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated" },
+ [ADVICE_HISTORY_UPDATE_REFS] = { "historyUpdateRefs" },
[ADVICE_IGNORED_HOOK] = { "ignoredHook" },
[ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity" },
[ADVICE_MERGE_CONFLICT] = { "mergeConflict" },
diff --git a/advice.h b/advice.h
index 8def280688..911b4e4643 100644
--- a/advice.h
+++ b/advice.h
@@ -25,6 +25,7 @@ enum advice_type {
ADVICE_FETCH_SHOW_FORCED_UPDATES,
ADVICE_FORCE_DELETE_BRANCH,
ADVICE_GRAFT_FILE_DEPRECATED,
+ ADVICE_HISTORY_UPDATE_REFS,
ADVICE_IGNORED_HOOK,
ADVICE_IMPLICIT_IDENTITY,
ADVICE_MERGE_CONFLICT,
diff --git a/builtin/history.c b/builtin/history.c
index cbba25096f..d3f535ded9 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -1,6 +1,7 @@
#define USE_THE_REPOSITORY_VARIABLE
#include "builtin.h"
+#include "advice.h"
#include "cache-tree.h"
#include "commit.h"
#include "commit-reach.h"
@@ -30,6 +31,8 @@
N_("git history reword <commit> [--dry-run] [--update-refs=(branches|head)]")
#define GIT_HISTORY_SPLIT_USAGE \
N_("git history split <commit> [--dry-run] [--update-refs=(branches|head)] [--] [<pathspec>...]")
+#define GIT_HISTORY_SQUASH_USAGE \
+ N_("git history squash [--dry-run] [--update-refs=(branches|head)] [--reedit-message] <revision-range>")
static void change_data_free(void *util, const char *str UNUSED)
{
@@ -973,6 +976,344 @@ out:
return ret;
}
+/*
+ * Resolve a "<base>..<tip>" revision range into the base commit just outside
+ * the range (which becomes the parent of the squashed commit), the oldest
+ * commit contained in the range (whose message the squash reuses), and the
+ * range tip (whose tree becomes the result). A merge inside the range is fine,
+ * but the range must have a single base and must not reach a root commit.
+ */
+static int resolve_squash_range(struct repository *repo,
+ const char **argv,
+ struct commit **base_out,
+ struct commit **oldest_out,
+ struct commit **tip_out,
+ struct oidset *interior_out)
+{
+ struct rev_info revs;
+ struct commit *commit, *base = NULL, *oldest = NULL, *tip = NULL;
+ struct commit_list *boundaries = NULL, *b;
+ struct strvec args = STRVEC_INIT;
+ size_t i;
+ int ret;
+
+ repo_init_revisions(repo, &revs, NULL);
+ revs.reverse = 1;
+ revs.topo_order = 1;
+ revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
+ revs.simplify_history = 0;
+ revs.boundary = 1;
+
+ strvec_push(&args, "ignored");
+ strvec_push(&args, "--ancestry-path");
+ strvec_pushv(&args, argv);
+ setup_revisions_from_strvec(&args, &revs, NULL);
+ if (args.nr != 1) {
+ ret = error(_("unrecognized argument: %s"), args.v[1]);
+ goto out;
+ }
+
+ if (revs.reverse != 1 || revs.topo_order != 1 ||
+ revs.sort_order != REV_SORT_IN_GRAPH_ORDER ||
+ revs.simplify_history != 0) {
+ warning(_("ignoring rev-list options that would change how the "
+ "range is walked"));
+ revs.reverse = 1;
+ revs.topo_order = 1;
+ revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
+ revs.simplify_history = 0;
+ }
+
+ /*
+ * A squash needs a base to reparent onto, so the range has to exclude
+ * something, as in "<base>..<tip>". A revision range with no such
+ * bottom commit cannot be squashed.
+ */
+ for (i = 0; i < revs.cmdline.nr; i++)
+ if (revs.cmdline.rev[i].flags & UNINTERESTING)
+ break;
+ if (i == revs.cmdline.nr) {
+ ret = error(_("not a '<base>..<tip>' revision range"));
+ goto out;
+ }
+
+ if (prepare_revision_walk(&revs) < 0) {
+ ret = error(_("error preparing revisions"));
+ goto out;
+ }
+
+ /*
+ * Set boundary commits aside for the base check below, and put every
+ * in-range commit but the tip into the interior set. A ref pointing
+ * at an interior commit would dangle once the range is folded away.
+ */
+ while ((commit = get_revision(&revs))) {
+ if (commit->object.flags & BOUNDARY) {
+ commit_list_insert(commit, &boundaries);
+ continue;
+ }
+ if (!oldest)
+ oldest = commit;
+ if (tip)
+ oidset_insert(interior_out, &tip->object.oid);
+ tip = commit;
+ }
+
+ if (!oldest) {
+ ret = error(_("the revision range is empty"));
+ goto out;
+ } else if (oldest == tip) {
+ ret = error(_("the revision range holds a single commit; "
+ "nothing to squash"));
+ goto out;
+ } else if (!oldest->parents) {
+ BUG("an in-range commit must have a parent");
+ }
+ base = oldest->parents->item;
+
+ /*
+ * A boundary other than the base is an in-range commit reaching a
+ * commit outside the range, so the range has more than one base.
+ */
+ for (b = boundaries; b; b = b->next) {
+ if (b->item != base) {
+ ret = error(_("the revision range has more than one base; "
+ "cannot squash"));
+ goto out;
+ }
+ }
+
+ *base_out = base;
+ *oldest_out = oldest;
+ *tip_out = tip;
+ ret = 0;
+
+out:
+ commit_list_free(boundaries);
+ reset_revision_walk();
+ release_revisions(&revs);
+ strvec_clear(&args);
+ return ret;
+}
+
+static const char *autosquash_target(const char *subject)
+{
+ const char *rest;
+
+ while (skip_prefix(subject, "fixup! ", &rest) ||
+ skip_prefix(subject, "squash! ", &rest) ||
+ skip_prefix(subject, "amend! ", &rest))
+ subject = rest;
+ return subject;
+}
+
+static int reject_dangling_fixups(struct repository *repo,
+ struct commit *base,
+ struct commit *tip,
+ struct commit *oldest,
+ struct commit **msg_source)
+{
+ struct todo_list todo = TODO_LIST_INIT;
+ struct replay_opts opts = REPLAY_OPTS_INIT;
+ struct rev_info revs;
+ struct commit *commit, *last_amend = NULL;
+ struct strvec args = STRVEC_INIT;
+ char *dangling_subject = NULL, *dangling_target = NULL;
+ bool mixed_target = false, all_fixups_one_target;
+ int i, ret, nr_dangling = 0;
+
+ *msg_source = oldest;
+
+ repo_init_revisions(repo, &revs, NULL);
+ strvec_push(&args, "ignored");
+ strvec_push(&args, "--reverse");
+ strvec_push(&args, "--topo-order");
+ strvec_pushf(&args, "%s..%s", oid_to_hex(&base->object.oid),
+ oid_to_hex(&tip->object.oid));
+ setup_revisions_from_strvec(&args, &revs, NULL);
+
+ if (prepare_revision_walk(&revs) < 0) {
+ ret = error(_("error preparing revisions"));
+ goto out;
+ }
+ while ((commit = get_revision(&revs)))
+ strbuf_addf(&todo.buf, "pick %s\n",
+ oid_to_hex(&commit->object.oid));
+
+ if (todo_list_parse_insn_buffer(repo, &opts, todo.buf.buf, &todo) < 0 ||
+ todo_list_rearrange_squash(&todo) < 0) {
+ ret = error(_("could not check the range for fixups"));
+ goto out;
+ }
+
+ for (i = 0; i < todo.nr; i++) {
+ const char *message, *subject_start, *target;
+ char *subject;
+ size_t sublen;
+
+ if (todo.items[i].command != TODO_PICK)
+ continue;
+ message = repo_logmsg_reencode(repo, todo.items[i].commit,
+ NULL, NULL);
+ sublen = find_commit_subject(message, &subject_start);
+ subject = xmemdupz(subject_start, sublen);
+ target = autosquash_target(subject);
+ if (target != subject) {
+ nr_dangling++;
+ if (!dangling_target) {
+ dangling_target = xstrdup(target);
+ dangling_subject = xstrdup(subject);
+ } else if (strcmp(dangling_target, target)) {
+ mixed_target = true;
+ }
+ if (starts_with(subject, "amend! "))
+ last_amend = todo.items[i].commit;
+ }
+ free(subject);
+ repo_unuse_commit_buffer(repo, todo.items[i].commit, message);
+ }
+
+ all_fixups_one_target = nr_dangling == todo.nr && !mixed_target;
+ if (nr_dangling && !all_fixups_one_target) {
+ ret = error(_("cannot squash '%s': its target is not in the "
+ "range"), dangling_subject);
+ } else {
+ if (last_amend)
+ *msg_source = last_amend;
+ ret = 0;
+ }
+
+out:
+ free(dangling_subject);
+ free(dangling_target);
+ todo_list_release(&todo);
+ replay_opts_release(&opts);
+ reset_revision_walk();
+ release_revisions(&revs);
+ strvec_clear(&args);
+ return ret;
+}
+
+struct interior_ref_cb {
+ const struct oidset *interior;
+ const char *name;
+};
+
+static int find_interior_ref(const struct reference *ref, void *cb_data)
+{
+ struct interior_ref_cb *data = cb_data;
+
+ if (oidset_contains(data->interior, ref->oid)) {
+ data->name = xstrdup(ref->name);
+ return 1;
+ }
+
+ return 0;
+}
+
+static int cmd_history_squash(int argc,
+ const char **argv,
+ const char *prefix,
+ struct repository *repo)
+{
+ const char * const usage[] = {
+ GIT_HISTORY_SQUASH_USAGE,
+ NULL,
+ };
+ enum ref_action action = REF_ACTION_DEFAULT;
+ enum commit_tree_flags flags = 0;
+ int dry_run = 0;
+ struct option options[] = {
+ OPT_CALLBACK_F(0, "update-refs", &action, "(branches|head)",
+ N_("control which refs should be updated"),
+ PARSE_OPT_NONEG, parse_ref_action),
+ OPT_BOOL('n', "dry-run", &dry_run,
+ N_("perform a dry-run without updating any refs")),
+ OPT_BIT(0, "reedit-message", &flags,
+ N_("open an editor to modify the commit message"),
+ COMMIT_TREE_EDIT_MESSAGE),
+ OPT_END(),
+ };
+ struct strbuf reflog_msg = STRBUF_INIT;
+ struct oidset interior = OIDSET_INIT;
+ struct commit *base, *oldest, *tip, *rewritten, *msg_source;
+ const struct object_id *base_tree_oid, *tip_tree_oid;
+ struct commit_list *parents = NULL;
+ struct rev_info revs = { 0 };
+ int ret;
+
+ argc = parse_options(argc, argv, prefix, options, usage,
+ PARSE_OPT_KEEP_UNKNOWN_OPT);
+ if (!argc) {
+ ret = error(_("command expects a revision range"));
+ goto out;
+ }
+ repo_config(repo, git_default_config, NULL);
+
+ if (action == REF_ACTION_DEFAULT)
+ action = REF_ACTION_BRANCHES;
+
+ ret = resolve_squash_range(repo, argv, &base, &oldest, &tip,
+ &interior);
+ if (ret < 0)
+ goto out;
+
+ ret = reject_dangling_fixups(repo, base, tip, oldest, &msg_source);
+ if (ret < 0)
+ goto out;
+
+ if (action == REF_ACTION_BRANCHES) {
+ struct interior_ref_cb cb = { .interior = &interior };
+
+ refs_for_each_ref(get_main_ref_store(repo),
+ find_interior_ref, &cb);
+ if (cb.name) {
+ ret = error(_("'%s' points into the squashed range"),
+ cb.name);
+ advise_if_enabled(ADVICE_HISTORY_UPDATE_REFS,
+ _("Use --update-refs=head to rewrite only "
+ "the current branch and leave such refs "
+ "untouched."));
+ free((char *)cb.name);
+ goto out;
+ }
+ }
+
+ ret = setup_revwalk(repo, action, tip, &revs);
+ if (ret < 0)
+ goto out;
+
+ base_tree_oid = &repo_get_commit_tree(repo, base)->object.oid;
+ tip_tree_oid = &repo_get_commit_tree(repo, tip)->object.oid;
+ commit_list_append(base, &parents);
+
+ ret = commit_tree_ext(repo, "squash", msg_source, NULL, parents,
+ base_tree_oid, tip_tree_oid, &rewritten, flags);
+ if (ret < 0) {
+ ret = error(_("failed writing squashed commit"));
+ goto out;
+ }
+
+ strbuf_addf(&reflog_msg, "squash: updating %s", argv[0]);
+
+ ret = handle_reference_updates(&revs, action, tip, rewritten,
+ reflog_msg.buf, dry_run,
+ REPLAY_EMPTY_COMMIT_ABORT);
+ if (ret < 0) {
+ ret = error(_("failed replaying descendants"));
+ goto out;
+ }
+
+ ret = 0;
+
+out:
+ strbuf_release(&reflog_msg);
+ oidset_clear(&interior);
+ commit_list_free(parents);
+ release_revisions(&revs);
+ return ret;
+}
+
int cmd_history(int argc,
const char **argv,
const char *prefix,
@@ -982,6 +1323,7 @@ int cmd_history(int argc,
GIT_HISTORY_FIXUP_USAGE,
GIT_HISTORY_REWORD_USAGE,
GIT_HISTORY_SPLIT_USAGE,
+ GIT_HISTORY_SQUASH_USAGE,
NULL,
};
parse_opt_subcommand_fn *fn = NULL;
@@ -989,6 +1331,7 @@ int cmd_history(int argc,
OPT_SUBCOMMAND("fixup", &fn, cmd_history_fixup),
OPT_SUBCOMMAND("reword", &fn, cmd_history_reword),
OPT_SUBCOMMAND("split", &fn, cmd_history_split),
+ OPT_SUBCOMMAND("squash", &fn, cmd_history_squash),
OPT_END(),
};
diff --git a/t/meson.build b/t/meson.build
index 7c3c070426..459e251623 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -400,6 +400,7 @@ integration_tests = [
't3451-history-reword.sh',
't3452-history-split.sh',
't3453-history-fixup.sh',
+ 't3455-history-squash.sh',
't3500-cherry.sh',
't3501-revert-cherry-pick.sh',
't3502-cherry-pick-merge.sh',
diff --git a/t/t3455-history-squash.sh b/t/t3455-history-squash.sh
new file mode 100755
index 0000000000..6bc0a89ebc
--- /dev/null
+++ b/t/t3455-history-squash.sh
@@ -0,0 +1,566 @@
+#!/bin/sh
+
+test_description='tests for git-history squash subcommand'
+
+. ./test-lib.sh
+
+test_expect_success 'setup linear history touching two files' '
+ test_commit base file a &&
+ git tag start &&
+ test_commit --no-tag one other x &&
+ test_commit --no-tag two file c &&
+ test_commit three file d
+'
+
+test_expect_success 'errors on missing range argument' '
+ test_must_fail git history squash 2>err &&
+ test_grep "expects a revision range" err
+'
+
+test_expect_success 'errors on an empty range' '
+ test_must_fail git history squash HEAD..HEAD 2>err &&
+ test_grep "the revision range is empty" err
+'
+
+test_expect_success 'errors on a single revision that is not a range' '
+ test_must_fail git history squash HEAD 2>err &&
+ test_grep "not a .*range" err &&
+ test_must_fail git history squash HEAD~1 2>err &&
+ test_grep "not a .*range" err
+'
+
+test_expect_success 'errors on a range holding a single commit' '
+ git reset --hard three &&
+ head_before=$(git rev-parse HEAD) &&
+
+ test_must_fail git history squash "HEAD^!" 2>err &&
+ test_grep "single commit; nothing to squash" err &&
+ test_cmp_rev "$head_before" HEAD
+'
+
+test_expect_success 'accepts multiple revision arguments with an exclusion' '
+ git reset --hard three &&
+ git branch -f keep HEAD~2 &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash start..HEAD ^keep &&
+
+ git log --format="%s" start..HEAD >actual &&
+ cat >expect <<-\EOF &&
+ two
+ one
+ EOF
+ test_cmp expect actual &&
+ test_cmp_rev keep HEAD~1 &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})" &&
+
+ git branch -D keep
+'
+
+test_expect_success 'squashes a branch the current branch is not on' '
+ git reset --hard three &&
+ main=$(git symbolic-ref --short HEAD) &&
+ head_before=$(git rev-parse HEAD) &&
+ git checkout -b off-history start &&
+ test_commit --no-tag off-one off a &&
+ test_commit --no-tag off-two off b &&
+ git checkout "$main" &&
+
+ git history squash start..off-history &&
+
+ git rev-list --count start..off-history >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test_cmp_rev "$head_before" HEAD &&
+
+ git branch -D off-history
+'
+
+test_expect_success 'squashes a range into a single commit without changing the tree' '
+ git reset --hard three &&
+ head_before=$(git rev-parse HEAD) &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash --dry-run start.. >out &&
+ predicted=$(awk "/^update refs\/heads\// {print \$3}" out) &&
+ test_cmp_rev "$head_before" HEAD &&
+
+ git history squash start.. &&
+
+ test "$predicted" = "$(git rev-parse HEAD)" &&
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test_cmp_rev start HEAD^ &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})" &&
+ git log --format="%s" -1 >subject &&
+ echo one >expect &&
+ test_cmp expect subject &&
+ git reflog >reflog &&
+ test_grep "squash: updating" reflog
+'
+
+test_expect_success 'sanitizes rev-list walk options, before and after --' '
+ git reset --hard three &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash --date-order start.. 2>err &&
+ test_grep "ignoring rev-list options" err &&
+ test_cmp_rev start HEAD^ &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})" &&
+
+ git reset --hard three &&
+ git history squash -- --reverse start.. 2>err &&
+ test_grep "ignoring rev-list options" err &&
+ test_cmp_rev start HEAD^ &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})"
+'
+
+test_expect_success 'squashes an interior range and replays descendants verbatim' '
+ git reset --hard three &&
+ final_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash start..@~1 &&
+
+ git log --format="%s" start..HEAD >actual &&
+ cat >expect <<-\EOF &&
+ three
+ one
+ EOF
+ test_cmp expect actual &&
+
+ test_cmp_rev start HEAD~2 &&
+ test "$final_tree" = "$(git rev-parse HEAD^{tree})"
+'
+
+test_expect_success 'squashes when the base is the root commit' '
+ git reset --hard three &&
+ root=$(git rev-list --max-parents=0 HEAD) &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash "$root.." &&
+
+ git rev-list --count "$root..HEAD" >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test_cmp_rev "$root" HEAD^ &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})"
+'
+
+
+test_expect_success 'folds fixups whose target is in the range' '
+ git reset --hard start &&
+ test_commit --no-tag target file b &&
+ git commit --allow-empty -m "fixup! target" &&
+ git commit --allow-empty -m "fixup! target" &&
+ test_commit --no-tag later file c &&
+
+ git history squash start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ git log --format="%s" -1 >actual &&
+ echo target >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'refuses a below-range fixup! after an in-range commit' '
+ git reset --hard start &&
+ test_commit --no-tag inside file b &&
+ test_commit --no-tag "fixup! outside" file c &&
+ head_before=$(git rev-parse HEAD) &&
+
+ test_must_fail git history squash start.. 2>err &&
+ test_grep "target is not in the range" err &&
+ test_cmp_rev "$head_before" HEAD
+'
+
+test_expect_success 'combines a run of fixups for one commit below the range' '
+ git reset --hard start &&
+ echo b >file && git add file && git commit -m "fixup! base" &&
+ echo c >file && git add file && git commit -m "fixup! base" &&
+
+ git history squash start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ git log --format="%s" -1 >actual &&
+ echo "fixup! base" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'combining below-range fixups keeps the last amend! message' '
+ git reset --hard start &&
+ echo b >file && git add file && git commit -m "fixup! base" &&
+ printf "amend! base\n\namended body\n" >msg &&
+ echo c >file && git add file && git commit -qF msg &&
+
+ git history squash start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ git log --format="%s" -1 >actual &&
+ echo "amend! base" >expect &&
+ test_cmp expect actual &&
+ git log --format="%b" -1 >body &&
+ test_grep "amended body" body
+'
+
+test_expect_success 'refuses fixups for two different commits below the range' '
+ git reset --hard start &&
+ echo b >file && git add file && git commit -m "fixup! aaa" &&
+ echo c >file && git add file && git commit -m "fixup! bbb" &&
+ head_before=$(git rev-parse HEAD) &&
+
+ test_must_fail git history squash start.. 2>err &&
+ test_grep "target is not in the range" err &&
+ test_cmp_rev "$head_before" HEAD
+'
+
+test_expect_success 'keeps the oldest message for in-range squash! and amend!' '
+ git reset --hard start &&
+ test_commit --no-tag marker-oldest file b &&
+ git commit --allow-empty -m "squash! marker-oldest" &&
+ git commit --allow-empty -m "amend! marker-oldest" &&
+ test_commit --no-tag marker-newest file c &&
+
+ git history squash start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ git log --format="%s" -1 >actual &&
+ echo marker-oldest >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'preserves authorship of the oldest commit' '
+ git reset --hard start &&
+ GIT_AUTHOR_NAME=Squasher GIT_AUTHOR_EMAIL=squash@example.com \
+ test_commit --no-tag oldest file b &&
+ test_commit newest file c &&
+
+ git history squash start.. &&
+
+ git log -1 --format="%an <%ae>" >actual &&
+ echo "Squasher <squash@example.com>" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success '--update-refs=head only moves HEAD' '
+ git reset --hard three &&
+ git branch -f other HEAD &&
+ other_before=$(git rev-parse other) &&
+
+ git history squash --update-refs=head start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test_cmp_rev "$other_before" other
+'
+
+test_expect_success 'refuses to fold a range a ref points into' '
+ git reset --hard three &&
+ git branch -f mid HEAD~1 &&
+ head_before=$(git rev-parse HEAD) &&
+
+ test_must_fail git history squash start.. 2>err &&
+ test_grep "error: .* points into the squashed range" err &&
+ test_grep "hint: .*--update-refs=head" err &&
+ test_cmp_rev "$head_before" HEAD &&
+
+ git branch -D mid
+'
+
+test_expect_success 'advice.historyUpdateRefs silences the hint' '
+ git reset --hard three &&
+ git branch -f mid HEAD~1 &&
+ head_before=$(git rev-parse HEAD) &&
+
+ test_must_fail git -c advice.historyUpdateRefs=false \
+ history squash start.. 2>err &&
+ test_grep "points into the squashed range" err &&
+ test_grep ! "hint:" err &&
+ test_cmp_rev "$head_before" HEAD &&
+
+ git branch -D mid
+'
+
+test_expect_success '--update-refs=head folds past a ref pointing into the range' '
+ git reset --hard three &&
+ git branch -f mid HEAD~1 &&
+ mid_before=$(git rev-parse mid) &&
+
+ git history squash --update-refs=head start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test_cmp_rev "$mid_before" mid &&
+
+ git branch -D mid
+'
+
+test_expect_success 'refuses to fold a range a tag points into' '
+ git reset --hard three &&
+ git tag -f mark HEAD~1 &&
+ head_before=$(git rev-parse HEAD) &&
+
+ test_must_fail git history squash start.. 2>err &&
+ test_grep "refs/tags/mark" err &&
+ test_grep "points into the squashed range" err &&
+ test_cmp_rev "$head_before" HEAD &&
+
+ git tag -d mark
+'
+
+test_expect_success 'squashes a range whose internal merge has a single base' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ test_commit --no-tag before-side file b &&
+ git checkout -b inner-side &&
+ test_commit --no-tag on-inner-side inner x &&
+ git checkout "$main" &&
+ test_commit --no-tag after-side file c &&
+ git merge --no-ff -m merge inner-side &&
+ git branch -D inner-side &&
+ test_commit --no-tag after-merge file d &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ git log --format="%s" -1 >subject &&
+ echo before-side >expect &&
+ test_cmp expect subject &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})" &&
+ test_path_is_file inner
+'
+
+test_expect_success 'folds a merge of a branch that forked at the base' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ git checkout -b base-fork-side &&
+ test_commit --no-tag base-fork-side side x &&
+ git checkout "$main" &&
+ test_commit --no-tag base-fork-main file b &&
+ git merge --no-ff -m "merge base-fork-side" base-fork-side &&
+ git branch -D base-fork-side &&
+ test_commit --no-tag base-fork-tail file c &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test_cmp_rev start HEAD^ &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})" &&
+ test_path_is_file side
+'
+
+test_expect_success 'refuses a merge whose other parent is outside the range' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ git checkout -b outside-parent &&
+ test_commit --no-tag outside-parent outside x &&
+ git checkout "$main" &&
+ test_commit --no-tag outside-main file b &&
+ base=$(git rev-parse HEAD) &&
+ test_commit --no-tag outside-mid file c &&
+ git merge --no-ff -m "merge outside-parent" outside-parent &&
+ git branch -D outside-parent &&
+ merged=$(git rev-parse HEAD) &&
+
+ test_must_fail git history squash "$base.." 2>err &&
+ test_grep "more than one base" err &&
+ test_cmp_rev "$merged" HEAD
+'
+
+test_expect_success 'folds a range whose tip is a merge commit' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ test_commit --no-tag tipmerge-base file b &&
+ git checkout -b tipmerge-side &&
+ test_commit --no-tag tipmerge-side side x &&
+ git checkout "$main" &&
+ test_commit --no-tag tipmerge-main file c &&
+ git merge --no-ff -m "merge tipmerge-side" tipmerge-side &&
+ git branch -D tipmerge-side &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})" &&
+ test_path_is_file side
+'
+
+test_expect_success 'folds a range whose base is a merge commit' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ git checkout -b basemerge-side &&
+ test_commit --no-tag basemerge-side side x &&
+ git checkout "$main" &&
+ test_commit --no-tag basemerge-main file b &&
+ git merge --no-ff -m "merge basemerge-side" basemerge-side &&
+ git branch -D basemerge-side &&
+ base=$(git rev-parse HEAD) &&
+ test_commit --no-tag basemerge-one file c &&
+ test_commit --no-tag basemerge-two file d &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash "$base.." &&
+
+ git rev-list --count "$base..HEAD" >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test_cmp_rev "$base" HEAD^ &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})"
+'
+
+test_expect_success 'folds a range with two interior merges' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ test_commit --no-tag two-merge-a file a1 &&
+ git checkout -b two-merge-s1 &&
+ test_commit --no-tag two-merge-s1 s1 x &&
+ git checkout "$main" &&
+ git merge --no-ff -m "merge s1" two-merge-s1 &&
+ test_commit --no-tag two-merge-b file b1 &&
+ git checkout -b two-merge-s2 &&
+ test_commit --no-tag two-merge-s2 s2 y &&
+ git checkout "$main" &&
+ git merge --no-ff -m "merge s2" two-merge-s2 &&
+ git branch -D two-merge-s1 two-merge-s2 &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})" &&
+ test_path_is_file s1 &&
+ test_path_is_file s2
+'
+
+test_expect_success 'folds a range with a nested merge' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ git checkout -b nested-outer &&
+ test_commit --no-tag nested-outer outer x &&
+ git checkout -b nested-inner &&
+ test_commit --no-tag nested-inner inner y &&
+ git checkout nested-outer &&
+ git merge --no-ff -m "merge inner" nested-inner &&
+ git checkout "$main" &&
+ test_commit --no-tag nested-main file b1 &&
+ git merge --no-ff -m "merge outer" nested-outer &&
+ git branch -D nested-outer nested-inner &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})" &&
+ test_path_is_file outer &&
+ test_path_is_file inner
+'
+
+test_expect_success 'folds a range with an octopus merge' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ test_commit --no-tag octo-base file a1 &&
+ git checkout -b octo-1 &&
+ test_commit --no-tag octo-1 o1 x &&
+ git checkout "$main" &&
+ git checkout -b octo-2 &&
+ test_commit --no-tag octo-2 o2 y &&
+ git checkout "$main" &&
+ git merge --no-ff -m octopus octo-1 octo-2 &&
+ git branch -D octo-1 octo-2 &&
+ tip_tree=$(git rev-parse HEAD^{tree}) &&
+
+ git history squash start.. &&
+
+ git rev-list --count start..HEAD >count &&
+ echo 1 >expect &&
+ test_cmp expect count &&
+ test "$tip_tree" = "$(git rev-parse HEAD^{tree})" &&
+ test_path_is_file o1 &&
+ test_path_is_file o2
+'
+
+test_expect_success 'refuses an octopus merge with an arm forked before the base' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ git checkout -b octo-pre &&
+ test_commit octo-pre-side pside x &&
+ git checkout "$main" &&
+ test_commit octo-pre-main file b1 &&
+ octo_base=$(git rev-parse HEAD) &&
+ git checkout -b octo-within &&
+ test_commit --no-tag octo-within wside y &&
+ git checkout "$main" &&
+ git merge --no-ff -m octopus octo-pre octo-within &&
+ merged=$(git rev-parse HEAD) &&
+ git branch -D octo-pre octo-within &&
+
+ test_must_fail git history squash "$octo_base.." 2>err &&
+ test_grep "more than one base" err &&
+ test_cmp_rev "$merged" HEAD
+'
+
+test_expect_success 'refuses when a descendant above the range is a merge' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ test_commit --no-tag desc-one file b &&
+ test_commit --no-tag desc-two file c &&
+ git tag desc-tip &&
+ git checkout -b desc-above &&
+ test_commit --no-tag desc-above above x &&
+ git checkout "$main" &&
+ test_commit --no-tag desc-main file d &&
+ git merge --no-ff -m "merge desc-above" desc-above &&
+ git branch -D desc-above &&
+ head_before=$(git rev-parse HEAD) &&
+
+ test_must_fail git history squash start..desc-tip 2>err &&
+ test_grep "merge commits is not supported" err &&
+ test_cmp_rev "$head_before" HEAD
+'
+
+test_expect_success 'refuses to fold a range a ref points into at a merge' '
+ git reset --hard start &&
+ main=$(git symbolic-ref --short HEAD) &&
+ test_commit --no-tag refmerge-base file b &&
+ git checkout -b refmerge-side &&
+ test_commit --no-tag refmerge-side side x &&
+ git checkout "$main" &&
+ test_commit --no-tag refmerge-main file c &&
+ git merge --no-ff -m "interior merge" refmerge-side &&
+ git branch -D refmerge-side &&
+ git branch at-merge HEAD &&
+ test_commit --no-tag refmerge-tail file d &&
+ head_before=$(git rev-parse HEAD) &&
+
+ test_must_fail git history squash start.. 2>err &&
+ test_grep "at-merge" err &&
+ test_grep "points into the squashed range" err &&
+ test_cmp_rev "$head_before" HEAD &&
+
+ git branch -D at-merge
+'
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v8 4/5] sequencer: share the squash message marker helpers and flags
From: Harald Nordgren via GitGitGadget @ 2026-07-10 9:06 UTC (permalink / raw)
To: git
Cc: Phillip Wood, D. Ben Knoble, Patrick Steinhardt, Matt Hunter,
Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2337.v8.git.git.1783674396.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
When "git rebase -i" squashes commits it builds an editor template with a
"This is a combination of N commits." banner, a "This is the 1st/Nth
commit message:" header above each kept message (or a "will be skipped"
header for a dropped one), and a commented-out subject for any fixup!,
squash! or amend! commit. The banner, the headers and the
subject-commenting all live in static helpers in sequencer.c wired to the
rebase state, so no other command can present a squash the same way.
Pull the three pieces out into add_squash_combination_header(),
add_squash_message_header() (which takes a flag for the "will be skipped"
variant) and squash_subject_comment_len(), and use them from
update_squash_messages() and append_squash_message(). Also move the
todo_item_flags enum to the header, so a caller reading the output of
todo_list_rearrange_squash() can tell an amend! (TODO_REPLACE_FIXUP_MSG)
from a plain fixup!. A later change reuses all of this to give "git
history squash --reedit-message" the same template.
No change in behavior.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
sequencer.c | 70 +++++++++++++++++++++++++++++------------------------
sequencer.h | 30 +++++++++++++++++++++++
2 files changed, 69 insertions(+), 31 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0fe8fed6c3..2387afd9b5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1880,18 +1880,38 @@ static int is_pick_or_similar(enum todo_command command)
}
}
-enum todo_item_flags {
- TODO_EDIT_MERGE_MSG = (1 << 0),
- TODO_REPLACE_FIXUP_MSG = (1 << 1),
- TODO_EDIT_FIXUP_MSG = (1 << 2),
-};
-
static const char first_commit_msg_str[] = N_("This is the 1st commit message:");
static const char nth_commit_msg_fmt[] = N_("This is the commit message #%d:");
static const char skip_first_commit_msg_str[] = N_("The 1st commit message will be skipped:");
static const char skip_nth_commit_msg_fmt[] = N_("The commit message #%d will be skipped:");
static const char combined_commit_msg_fmt[] = N_("This is a combination of %d commits.");
+void add_squash_combination_header(struct strbuf *buf, int n)
+{
+ strbuf_addf(buf, "%s ", comment_line_str);
+ strbuf_addf(buf, _(combined_commit_msg_fmt), n);
+}
+
+void add_squash_message_header(struct strbuf *buf, int n, int skip)
+{
+ strbuf_addf(buf, "%s ", comment_line_str);
+ if (n == 1)
+ strbuf_addstr(buf, skip ? _(skip_first_commit_msg_str) :
+ _(first_commit_msg_str));
+ else
+ strbuf_addf(buf, skip ? _(skip_nth_commit_msg_fmt) :
+ _(nth_commit_msg_fmt), n);
+}
+
+size_t squash_subject_comment_len(const char *body, int squashing)
+{
+ if (starts_with(body, "amend!") ||
+ (squashing && (starts_with(body, "squash!") ||
+ starts_with(body, "fixup!"))))
+ return commit_subject_length(body);
+ return 0;
+}
+
static int is_fixup_flag(enum todo_command command, unsigned flag)
{
return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) ||
@@ -2005,20 +2025,13 @@ static int append_squash_message(struct strbuf *buf, const char *body,
{
struct replay_ctx *ctx = opts->ctx;
const char *fixup_msg;
- size_t commented_len = 0, fixup_off;
- /*
- * amend is non-interactive and not normally used with fixup!
- * or squash! commits, so only comment out those subjects when
- * squashing commit messages.
- */
- if (starts_with(body, "amend!") ||
- ((command == TODO_SQUASH || seen_squash(ctx)) &&
- (starts_with(body, "squash!") || starts_with(body, "fixup!"))))
- commented_len = commit_subject_length(body);
+ size_t commented_len, fixup_off;
+
+ commented_len = squash_subject_comment_len(body,
+ command == TODO_SQUASH || seen_squash(ctx));
- strbuf_addf(buf, "\n%s ", comment_line_str);
- strbuf_addf(buf, _(nth_commit_msg_fmt),
- ++ctx->current_fixup_count + 1);
+ strbuf_addch(buf, '\n');
+ add_squash_message_header(buf, ++ctx->current_fixup_count + 1, 0);
strbuf_addstr(buf, "\n\n");
strbuf_add_commented_lines(buf, body, commented_len, comment_line_str);
/* buf->buf may be reallocated so store an offset into the buffer */
@@ -2083,9 +2096,8 @@ static int update_squash_messages(struct repository *r,
eol = !starts_with(buf.buf, comment_line_str) ?
buf.buf : strchrnul(buf.buf, '\n');
- strbuf_addf(&header, "%s ", comment_line_str);
- strbuf_addf(&header, _(combined_commit_msg_fmt),
- ctx->current_fixup_count + 2);
+ add_squash_combination_header(&header,
+ ctx->current_fixup_count + 2);
strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
strbuf_release(&header);
if (is_fixup_flag(command, flag) && !seen_squash(ctx))
@@ -2109,12 +2121,9 @@ static int update_squash_messages(struct repository *r,
repo_unuse_commit_buffer(r, head_commit, head_message);
return error(_("cannot write '%s'"), rebase_path_fixup_msg());
}
- strbuf_addf(&buf, "%s ", comment_line_str);
- strbuf_addf(&buf, _(combined_commit_msg_fmt), 2);
- strbuf_addf(&buf, "\n%s ", comment_line_str);
- strbuf_addstr(&buf, is_fixup_flag(command, flag) ?
- _(skip_first_commit_msg_str) :
- _(first_commit_msg_str));
+ add_squash_combination_header(&buf, 2);
+ strbuf_addch(&buf, '\n');
+ add_squash_message_header(&buf, 1, is_fixup_flag(command, flag));
strbuf_addstr(&buf, "\n\n");
if (is_fixup_flag(command, flag))
strbuf_add_commented_lines(&buf, body, strlen(body),
@@ -2133,9 +2142,8 @@ static int update_squash_messages(struct repository *r,
if (command == TODO_SQUASH || is_fixup_flag(command, flag)) {
res = append_squash_message(&buf, body, command, opts, flag);
} else if (command == TODO_FIXUP) {
- strbuf_addf(&buf, "\n%s ", comment_line_str);
- strbuf_addf(&buf, _(skip_nth_commit_msg_fmt),
- ++ctx->current_fixup_count + 1);
+ strbuf_addch(&buf, '\n');
+ add_squash_message_header(&buf, ++ctx->current_fixup_count + 1, 1);
strbuf_addstr(&buf, "\n\n");
strbuf_add_commented_lines(&buf, body, strlen(body),
comment_line_str);
diff --git a/sequencer.h b/sequencer.h
index 64a9c7fb1b..b01f897020 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -119,6 +119,13 @@ enum todo_command {
TODO_COMMENT
};
+/* Bits for the "flags" member of struct todo_item */
+enum todo_item_flags {
+ TODO_EDIT_MERGE_MSG = (1 << 0),
+ TODO_REPLACE_FIXUP_MSG = (1 << 1),
+ TODO_EDIT_FIXUP_MSG = (1 << 2),
+};
+
struct todo_item {
enum todo_command command;
struct commit *commit;
@@ -208,6 +215,29 @@ int todo_list_rearrange_squash(struct todo_list *todo_list);
*/
void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);
+/*
+ * Append the "This is a combination of N commits." banner that "git rebase
+ * -i" writes at the top of a squashed commit's message, commented out with
+ * the comment character.
+ */
+void add_squash_combination_header(struct strbuf *buf, int n);
+
+/*
+ * Append the header (1-based N) that "git rebase -i" writes above each message
+ * when squashing, commented out with the comment character. With SKIP it reads
+ * "The ... commit message will be skipped" for a message that is dropped (a
+ * fixup), otherwise "This is the ... commit message".
+ */
+void add_squash_message_header(struct strbuf *buf, int n, int skip);
+
+/*
+ * Return the length of the leading subject of BODY when it should be commented
+ * out in a squash message, or 0 otherwise. An "amend!" subject always
+ * qualifies; "squash!" and "fixup!" subjects only when SQUASHING, since a
+ * plain fixup chain keeps them.
+ */
+size_t squash_subject_comment_len(const char *body, int squashing);
+
void append_conflicts_hint(struct index_state *istate,
struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode);
enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
--
gitgitgadget
^ permalink raw reply related
* [PATCH v8 5/5] history: re-edit a squash with every message
From: Harald Nordgren via GitGitGadget @ 2026-07-10 9:06 UTC (permalink / raw)
To: git
Cc: Phillip Wood, D. Ben Knoble, Patrick Steinhardt, Matt Hunter,
Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2337.v8.git.git.1783674396.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
By default "git history squash" reuses the oldest commit's message.
When --reedit-message is given it only reopened that one message, so the
messages of the other commits in the range were lost.
Gather the message of every commit in the range and build the same editor
template that "git rebase -i --autosquash" shows for a squash, reusing
add_squash_combination_header(), add_squash_message_header() and
squash_subject_comment_len(). Feed the range through
todo_list_rearrange_squash() so that each fixup!, squash! or amend! is
grouped under the commit it targets rather than shown in commit order,
exactly as autosquash would arrange them.
Only the message text differs, the changes are always folded in. A fixup!
message is commented out in full under a "will be skipped" header, a
squash! keeps its body with only the marker subject commented, and an
amend! replaces its target's message unless a squash! already folded into
that target, in which case it behaves like a squash!.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Documentation/git-history.adoc | 15 ++-
builtin/history.c | 107 ++++++++++++++++-
t/t3455-history-squash.sh | 210 +++++++++++++++++++++++++++++++++
3 files changed, 328 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc
index ad921ac2be..697155f5d8 100644
--- a/Documentation/git-history.adoc
+++ b/Documentation/git-history.adoc
@@ -117,8 +117,9 @@ given, for example `HEAD~3..HEAD ^topic` to additionally exclude what is
already on `topic`. Rev-list options may also be given, but any that would
change how the range is walked are overridden with a warning.
+
-The oldest commit's message and authorship are preserved by default,
-unless you specify `--reedit-message`. A merge commit inside the range is
+The oldest commit's message and authorship are preserved by default. With
+`--reedit-message`, an editor opens pre-filled with the messages of all the
+folded commits so you can combine them. A merge commit inside the range is
folded like any other, but the range must have a single base, so a range
that reaches more than one entry point (for example a side branch that
forked before the range and was later merged into it) is rejected.
@@ -127,7 +128,15 @@ A `fixup!`, `squash!`, or `amend!` commit is refused unless the commit it
targets is also in the range, so the fold does not silently absorb a
marker meant for a commit outside it. As an exception, a range made up
entirely of markers for one target is combined into a single commit,
-keeping the last `amend!` message if there is one.
+keeping the last `amend!` message if there is one. The changes from every
+commit in the range are always folded in. Only the message text differs.
+With `--reedit-message` the template mirrors `git rebase -i --autosquash`:
+each `fixup!`, `squash!`, or `amend!` is grouped under the commit it
+targets rather than shown in commit order. A `fixup!` message is dropped
+(commented out in full), a `squash!` keeps its body with only the marker
+subject commented, and an `amend!` replaces its target's message, unless
+a `squash!` folded into that target first, in which case it keeps its
+body like a `squash!`.
+
A branch or tag that points at a commit inside the range would be left
dangling once those commits are folded away, so with the default
diff --git a/builtin/history.c b/builtin/history.c
index d3f535ded9..f82001508d 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -1211,6 +1211,102 @@ static int find_interior_ref(const struct reference *ref, void *cb_data)
return 0;
}
+static bool amend_replaces_target(struct todo_list *todo, int target)
+{
+ int i;
+
+ for (i = target + 1; i < todo->nr &&
+ todo->items[i].command != TODO_PICK; i++) {
+ if (todo->items[i].command == TODO_SQUASH)
+ return false;
+ if (todo->items[i].flags & TODO_REPLACE_FIXUP_MSG)
+ return true;
+ }
+ return false;
+}
+
+static int build_squash_message(struct repository *repo,
+ struct commit *base,
+ struct commit *tip,
+ struct strbuf *out)
+{
+ struct rev_info revs;
+ struct commit *commit;
+ struct strvec args = STRVEC_INIT;
+ struct todo_list todo = TODO_LIST_INIT;
+ struct replay_opts opts = REPLAY_OPTS_INIT;
+ int i, nr_commits, ret;
+
+ repo_init_revisions(repo, &revs, NULL);
+ strvec_push(&args, "ignored");
+ strvec_push(&args, "--reverse");
+ strvec_push(&args, "--topo-order");
+ strvec_pushf(&args, "%s..%s", oid_to_hex(&base->object.oid),
+ oid_to_hex(&tip->object.oid));
+ setup_revisions_from_strvec(&args, &revs, NULL);
+
+ if (prepare_revision_walk(&revs) < 0) {
+ ret = error(_("error preparing revisions"));
+ goto out;
+ }
+
+ while ((commit = get_revision(&revs)))
+ strbuf_addf(&todo.buf, "pick %s\n",
+ oid_to_hex(&commit->object.oid));
+
+ if (todo_list_parse_insn_buffer(repo, &opts, todo.buf.buf, &todo) < 0 ||
+ todo_list_rearrange_squash(&todo) < 0) {
+ ret = error(_("could not prepare the squash message"));
+ goto out;
+ }
+
+ nr_commits = todo.nr;
+ for (i = 0; i < nr_commits; i++) {
+ struct todo_item *item = &todo.items[i];
+ const char *message, *body;
+ size_t commented_len;
+ bool skip, squashing;
+
+ squashing = item->command == TODO_SQUASH ||
+ (item->flags & TODO_REPLACE_FIXUP_MSG);
+ if (item->command == TODO_PICK)
+ skip = amend_replaces_target(&todo, i);
+ else
+ skip = !squashing;
+
+ message = repo_logmsg_reencode(repo, item->commit, NULL, NULL);
+ find_commit_subject(message, &body);
+
+ if (skip)
+ commented_len = strlen(body);
+ else if (squashing)
+ commented_len = squash_subject_comment_len(body, 1);
+ else
+ commented_len = 0;
+
+ if (!i)
+ add_squash_combination_header(out, nr_commits);
+ strbuf_addch(out, '\n');
+ add_squash_message_header(out, i + 1, skip);
+ strbuf_addstr(out, "\n\n");
+ strbuf_add_commented_lines(out, body, commented_len, comment_line_str);
+ strbuf_addstr(out, body + commented_len);
+ strbuf_complete_line(out);
+
+ repo_unuse_commit_buffer(repo, item->commit, message);
+ }
+
+ ret = 0;
+
+out:
+ todo_list_release(&todo);
+ replay_opts_release(&opts);
+ reset_revision_walk();
+ release_revisions(&revs);
+ strvec_clear(&args);
+ return ret;
+}
+
static int cmd_history_squash(int argc,
const char **argv,
const char *prefix,
@@ -1235,6 +1331,7 @@ static int cmd_history_squash(int argc,
OPT_END(),
};
struct strbuf reflog_msg = STRBUF_INIT;
+ struct strbuf message = STRBUF_INIT;
struct oidset interior = OIDSET_INIT;
struct commit *base, *oldest, *tip, *rewritten, *msg_source;
const struct object_id *base_tree_oid, *tip_tree_oid;
@@ -1279,6 +1376,12 @@ static int cmd_history_squash(int argc,
}
}
+ if (flags & COMMIT_TREE_EDIT_MESSAGE) {
+ ret = build_squash_message(repo, base, tip, &message);
+ if (ret < 0)
+ goto out;
+ }
+
ret = setup_revwalk(repo, action, tip, &revs);
if (ret < 0)
goto out;
@@ -1287,7 +1390,8 @@ static int cmd_history_squash(int argc,
tip_tree_oid = &repo_get_commit_tree(repo, tip)->object.oid;
commit_list_append(base, &parents);
- ret = commit_tree_ext(repo, "squash", msg_source, NULL, parents,
+ ret = commit_tree_ext(repo, "squash", msg_source,
+ message.len ? message.buf : NULL, parents,
base_tree_oid, tip_tree_oid, &rewritten, flags);
if (ret < 0) {
ret = error(_("failed writing squashed commit"));
@@ -1308,6 +1412,7 @@ static int cmd_history_squash(int argc,
out:
strbuf_release(&reflog_msg);
+ strbuf_release(&message);
oidset_clear(&interior);
commit_list_free(parents);
release_revisions(&revs);
diff --git a/t/t3455-history-squash.sh b/t/t3455-history-squash.sh
index 6bc0a89ebc..9b24f546e0 100755
--- a/t/t3455-history-squash.sh
+++ b/t/t3455-history-squash.sh
@@ -250,6 +250,216 @@ test_expect_success 'preserves authorship of the oldest commit' '
test_cmp expect actual
'
+test_expect_success '--reedit-message offers every folded-in message' '
+ git reset --hard start &&
+ echo b >file &&
+ git add file &&
+ git commit -m "re-one subject" -m "re-one body line" &&
+ test_commit --no-tag re-two file c &&
+ test_commit re-three file d &&
+
+ write_script editor <<-\EOF &&
+ cat "$1" >edited &&
+ echo combined >"$1"
+ EOF
+ test_set_editor "$(pwd)/editor" &&
+ git history squash --reedit-message start.. &&
+
+ cat >expect <<-EOF &&
+ # This is a combination of 3 commits.
+ # This is the 1st commit message:
+
+ re-one subject
+
+ re-one body line
+
+ # This is the commit message #2:
+
+ re-two
+
+ # This is the commit message #3:
+
+ re-three
+
+ # Please enter the commit message for the squash changes. Lines starting
+ # with ${SQ}#${SQ} will be ignored, and an empty message aborts the commit.
+ # Changes to be committed:
+ # modified: file
+ #
+ EOF
+ test_cmp expect edited &&
+ echo combined >expect &&
+ git log --format="%s" -1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--reedit-message handles fixup!, squash! and amend! like rebase' '
+ git reset --hard start &&
+ test_commit --no-tag mark-base file b &&
+ printf "fixup! mark-base\n\nfixup body\n" >msg &&
+ echo c >file &&
+ git add file &&
+ git commit -qF msg &&
+ printf "squash! mark-base\n\nsquash remark\n" >msg &&
+ echo d >file &&
+ git add file &&
+ git commit -qF msg &&
+ printf "amend! mark-base\n\namended message\n" >msg &&
+ echo e >file &&
+ git add file &&
+ git commit -qF msg &&
+
+ write_script editor <<-\EOF &&
+ cat "$1" >edited
+ EOF
+ test_set_editor "$(pwd)/editor" &&
+ git history squash --reedit-message start.. &&
+
+ cat >expect <<-EOF &&
+ # This is a combination of 4 commits.
+ # This is the 1st commit message:
+
+ mark-base
+
+ # The commit message #2 will be skipped:
+
+ # fixup! mark-base
+ #
+ # fixup body
+
+ # This is the commit message #3:
+
+ # squash! mark-base
+
+ squash remark
+
+ # This is the commit message #4:
+
+ # amend! mark-base
+
+ amended message
+
+ # Please enter the commit message for the squash changes. Lines starting
+ # with ${SQ}#${SQ} will be ignored, and an empty message aborts the commit.
+ # Changes to be committed:
+ # modified: file
+ #
+ EOF
+ test_cmp expect edited &&
+ git log -1 --format="%B" >final &&
+ test_grep ! "fixup body" final &&
+ test_grep "squash remark" final &&
+ test_grep "amended message" final
+'
+
+test_expect_success '--reedit-message groups fixups under their targets' '
+ git reset --hard start &&
+ test_commit --no-tag alpha file a1 &&
+ test_commit --no-tag beta file b1 &&
+ printf "fixup! alpha\n" >msg &&
+ echo a2 >file &&
+ git add file &&
+ git commit -qF msg &&
+ printf "fixup! beta\n" >msg &&
+ echo b2 >file &&
+ git add file &&
+ git commit -qF msg &&
+
+ write_script editor <<-\EOF &&
+ cat "$1" >edited
+ EOF
+ test_set_editor "$(pwd)/editor" &&
+ git history squash --reedit-message start.. &&
+
+ cat >expect <<-EOF &&
+ # This is a combination of 4 commits.
+ # This is the 1st commit message:
+
+ alpha
+
+ # The commit message #2 will be skipped:
+
+ # fixup! alpha
+
+ # This is the commit message #3:
+
+ beta
+
+ # The commit message #4 will be skipped:
+
+ # fixup! beta
+
+ # Please enter the commit message for the squash changes. Lines starting
+ # with ${SQ}#${SQ} will be ignored, and an empty message aborts the commit.
+ # Changes to be committed:
+ # modified: file
+ #
+ EOF
+ test_cmp expect edited
+'
+
+test_expect_success '--reedit-message lets amend! replace its target message' '
+ git reset --hard start &&
+ test_commit --no-tag mark-base file b &&
+ printf "amend! mark-base\n\namended message\n" >msg &&
+ echo c >file &&
+ git add file &&
+ git commit -qF msg &&
+ printf "squash! mark-base\n\nsquash remark\n" >msg &&
+ echo d >file &&
+ git add file &&
+ git commit -qF msg &&
+
+ write_script editor <<-\EOF &&
+ cat "$1" >edited
+ EOF
+ test_set_editor "$(pwd)/editor" &&
+ git history squash --reedit-message start.. &&
+
+ cat >expect <<-EOF &&
+ # This is a combination of 3 commits.
+ # The 1st commit message will be skipped:
+
+ # mark-base
+
+ # This is the commit message #2:
+
+ # amend! mark-base
+
+ amended message
+
+ # This is the commit message #3:
+
+ # squash! mark-base
+
+ squash remark
+
+ # Please enter the commit message for the squash changes. Lines starting
+ # with ${SQ}#${SQ} will be ignored, and an empty message aborts the commit.
+ # Changes to be committed:
+ # modified: file
+ #
+ EOF
+ test_cmp expect edited &&
+ git log -1 --format="%B" >final &&
+ test_grep ! "mark-base" final &&
+ test_grep "amended message" final &&
+ test_grep "squash remark" final
+'
+
+test_expect_success '--reedit-message aborts on an empty message' '
+ git reset --hard three &&
+ head_before=$(git rev-parse HEAD) &&
+
+ write_script editor <<-\EOF &&
+ >"$1"
+ EOF
+ test_set_editor "$(pwd)/editor" &&
+ test_must_fail git history squash --reedit-message start.. &&
+
+ test_cmp_rev "$head_before" HEAD
+'
+
test_expect_success '--update-refs=head only moves HEAD' '
git reset --hard three &&
git branch -f other HEAD &&
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v1 0/3] worktree: add post-worktree-add and post-worktree-remove hooks
From: Phillip Wood @ 2026-07-10 9:34 UTC (permalink / raw)
To: domen, git
Cc: Eric Sunshine, Patrick Steinhardt,
Ævar Arnfjörð Bjarmason, Caleb White,
Junio C Hamano
In-Reply-To: <7c8b4673-37ac-45fa-ad8c-a1dc09afe5fe@mtasv.net>
Hi Domen
On 10/07/2026 00:36, Domen Kožar wrote:
>
> Today there is no reliable trigger to set that up when a worktree
> appears: post-checkout does not fire for --no-checkout or --orphan
> and cannot be told apart from a plain checkout. Nothing at all fires
> when a worktree goes away, so stale databases and services pile up
> after "git worktree remove" or a manual rm followed by "git worktree
> prune". Wrapping the worktree commands only helps when every tool,
> human or agent, goes through the wrapper.
I agree a hook that's run after the worktree is added is useful (I have
a patch for it that I've never got round to cleaning up and sending so
thank you for working on this). It is useful for copying across
untracked files to the new worktree like "config.mak".
> Patch 1 adds a post-worktree-add hook that fires after the working
> tree is fully set up. Patch 2 adds post-worktree-remove for "git
> worktree remove". Patch 3 extends it to "git worktree prune" so that
> manually deleted worktrees are also observed.
I don't have a strong opinion on a hook running when a worktree is
removed - an IDE that cares about that could set up a filesystem watch
on the directory but I guess adding a hook doesn't do any harm.
> Two design points I would especially appreciate feedback on:
>
> * post-worktree-add runs after post-checkout and is skipped when
> post-checkout fails. An argument could be made that it should run
> whenever the worktree was created, regardless of the earlier
> hook's exit status, since tooling registering worktrees would
> otherwise miss one that does exist.
Looking at the existing code, if the checkout fails then we remove the
worktree because "is_junk == 1" when remove_junk() is called via
atexit() so I think it is correct to skip the new hook in that case.
The new hook is run after the checkout, but before the post-checkout
hook - we should document their relative order. I see the hook is run in
the new worktree and passed the absolute directory and worktree id. I'm
wondering if either of those is useful if we're running the hook in the
new worktree.
> * for entries pruned because their gitdir file points to a location
> that no longer exists, the hook receives the recorded path; when
> the path cannot be determined at all (missing or corrupt gitdir
> file) it receives an empty string.
So the hook knows a worktree was removed but not which one?
Thanks
Phillip
> Thanks,
> Domen
>
> Domen Kožar (3):
> worktree: add post-worktree-add hook
> worktree: add post-worktree-remove hook
> worktree: run post-worktree-remove hook when pruning
>
> Documentation/githooks.adoc | 41 +++++++++++++
> builtin/worktree.c | 73 ++++++++++++++++++-----
> t/t2400-worktree-add.sh | 113 ++++++++++++++++++++++++++++++++++++
> t/t2401-worktree-prune.sh | 88 ++++++++++++++++++++++++++++
> t/t2403-worktree-move.sh | 44 ++++++++++++++
> worktree.c | 1 -
> worktree.h | 6 +-
> 7 files changed, 347 insertions(+), 19 deletions(-)
>
>
> base-commit: f85a7e662054a7b0d9070e432508831afa214b47
^ permalink raw reply
* Re: [PATCH v3 0/2] environment: move ignore_case into repo_config_values
From: Johannes Schindelin @ 2026-07-10 10:29 UTC (permalink / raw)
To: Tian Yuchen; +Cc: git, ps, phillip.wood123, stolee
In-Reply-To: <20260619155152.642760-1-cat@malon.dev>
Hi Yuchen,
On Fri, 10 Jul 2026, Tian Yuchen wrote:
> compat/win32/path-utils.c --- Is it appropriate to include the
> repository.h header file?
Since path-utils.c implements logic that is repository-dependent (as your
patch points out), including that header is appropriate.
Ciao,
Johannes
^ permalink raw reply
* [PATCH v3 0/2] reftable: fix quadratic behavior when re-creating deleted refs
From: Kristofer Karlsson via GitGitGadget @ 2026-07-10 10:36 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson
In-Reply-To: <pull.2166.v2.git.1783598912.gitgitgadget@gmail.com>
This series fixes quadratic behavior in the reftable backend when many
tombstones are present. Any operation that seeks into a range containing
tombstones is affected, including ref lookups and D/F conflict checks.
The root cause is the merged iterator's suppress_deletions flag, which
silently consumes tombstone records in a tight internal loop. This prevents
higher-level code from checking iteration bounds until after all tombstones
have been scanned, making both refs_verify_refnames_available() and
reftable_backend_read_ref() O(n) per call in the presence of tombstones.
The fix makes suppress_deletions configurable via reftable_stack_options
(defaulting to off) and handles deletion records at each call site in the
reftable backend, where prefix and refname bounds are available. This lets
existing bounds checks terminate iteration early when encountering
tombstones past the relevant bound.
Downstream users of the reftable library (e.g. libgit2) can enable
suppress_deletions through the stack options to retain the previous
behavior.
The first patch adds a perf test (p1401) exercising two tombstone scenarios
with 8000 refs. The second patch is the optimization. Both p1401 tests go
from ~13s to ~0.2s with the fix.
Note that auto-compaction typically merges tombstones before they accumulate
to this degree, so the quadratic behavior may not show up in every workflow.
But the fix ensures correct time complexity regardless of compaction state,
and the change is fairly contained.
Changes since v2:
* Add suppress_deletions to reftable_stack_options so downstream callers
can control it at stack creation time (suggested by Patrick)
Changes since v1:
* Keep suppress_deletions in the reftable library for downstream users;
only stop setting it in stack.c
* Broaden scope description to cover all readers, not just ref creation
* Use separate repositories in perf test to avoid cross-scenario state
* Drop correctness test (implicitly covered by t1400)
Previous discussion:
https://lore.kernel.org/git/20260701080014.GA3748390@coredump.intra.peff.net/
Kristofer Karlsson (2):
t/perf: add perf test for ref tombstone scenarios
reftable: fix quadratic behavior in the presence of tombstones
refs/reftable-backend.c | 54 ++++++++++++++++++++++------
reftable/reftable-stack.h | 2 ++
reftable/stack.c | 2 +-
t/perf/p1401-ref-store-tombstones.sh | 46 ++++++++++++++++++++++++
4 files changed, 92 insertions(+), 12 deletions(-)
create mode 100755 t/perf/p1401-ref-store-tombstones.sh
base-commit: f85a7e662054a7b0d9070e432508831afa214b47
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2166%2Fspkrka%2Freftable-tombstone-perf-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2166/spkrka/reftable-tombstone-perf-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2166
Range-diff vs v2:
1: 889d0d38bc = 1: 889d0d38bc t/perf: add perf test for ref tombstone scenarios
2: c13f15ddc2 ! 2: 4fdcec8440 reftable: fix quadratic behavior in the presence of tombstones
@@ Commit message
found", because the merged iterator skips the matching tombstone
and searches for the next live record.
- Fix this by no longer setting suppress_deletions on the stack's
- merged table and instead handling deletion records at each call site
- in the reftable backend, where prefix and refname bounds are
- available. Tombstones are now returned to callers, which skip them
- after their existing bounds checks. This allows iteration to
- terminate as soon as a tombstone past the relevant bound is
- encountered.
+ Fix this by making suppress_deletions configurable via
+ reftable_stack_options instead of unconditionally enabling it. Git
+ no longer sets the flag, so tombstones are now returned to callers in
+ the reftable backend, which skip them after their existing bounds
+ checks. This allows iteration to terminate as soon as a tombstone
+ past the relevant bound is encountered.
- The suppress_deletions flag and its logic in the merged iterator are
- retained for downstream users of the reftable library (e.g. libgit2).
+ Downstream users of the reftable library (e.g. libgit2) can still
+ enable suppress_deletions through the stack options to retain the
+ previous behavior.
This also requires adding deletion checks to the log iteration paths,
since suppress_deletions applied to both ref and log iterators.
- Both tests in p1401 go from ~14s to ~0.2s with this change.
+ Both tests in p1401 go from ~13s to ~0.2s with this change.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
@@ refs/reftable-backend.c: static int reftable_be_fsck(struct ref_store *ref_store
case REFTABLE_REF_VAL2: {
struct object_id oid;
+ ## reftable/reftable-stack.h ##
+@@ reftable/reftable-stack.h: struct reftable_stack_options {
+ */
+ void (*on_reload)(void *payload);
+ void *on_reload_payload;
++
++ int suppress_deletions;
+ };
+
+ /* open a new reftable stack. The tables along with the table list will be
+
## reftable/stack.c ##
@@ reftable/stack.c: static int reftable_stack_reload_once(struct reftable_stack *st,
/* Update the stack to point to the new tables. */
if (st->merged)
reftable_merged_table_free(st->merged);
- new_merged->suppress_deletions = 1;
++ new_merged->suppress_deletions = st->opts.suppress_deletions;
st->merged = new_merged;
if (st->tables)
--
gitgitgadget
^ permalink raw reply
* [PATCH v3 1/2] t/perf: add perf test for ref tombstone scenarios
From: Kristofer Karlsson via GitGitGadget @ 2026-07-10 10:36 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Kristofer Karlsson
In-Reply-To: <pull.2166.v3.git.1783679767.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
Add performance tests for update-ref when many tombstones are present
in a reftable.
The first test exercises the scenario where all refs are deleted
(creating tombstones) and then re-created with the same names, which
currently exhibits quadratic behavior.
The second test uses a separate repository with an asymmetric variant
where refs are deleted and then new, differently-named refs are
created. When the tombstones sort after the new refs, every create
scans all tombstones, making this case even worse than re-creating
the same refs.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
t/perf/p1401-ref-store-tombstones.sh | 46 ++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
create mode 100755 t/perf/p1401-ref-store-tombstones.sh
diff --git a/t/perf/p1401-ref-store-tombstones.sh b/t/perf/p1401-ref-store-tombstones.sh
new file mode 100755
index 0000000000..9e3d8031aa
--- /dev/null
+++ b/t/perf/p1401-ref-store-tombstones.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description="Tests performance of ref operations with many tombstones"
+
+. ./perf-lib.sh
+
+test_expect_success "setup" '
+ git init --ref-format=reftable repo &&
+ blob=$(echo foo | git -C repo hash-object -w --stdin) &&
+ for i in $(test_seq 8000)
+ do
+ printf "create refs/tags/tag-%d %s\n" "$i" "$blob" ||
+ return 1
+ done >repo/input &&
+ git -C repo update-ref --stdin <repo/input &&
+ git -C repo for-each-ref --format="delete %(refname)" |
+ git -C repo update-ref --stdin
+'
+
+test_perf "recreate refs after mass delete" '
+ git -C repo update-ref --stdin <repo/input &&
+ git -C repo for-each-ref --format="delete %(refname)" |
+ git -C repo update-ref --stdin
+'
+
+test_expect_success "setup asymmetric" '
+ git init --ref-format=reftable repo2 &&
+ blob=$(echo foo | git -C repo2 hash-object -w --stdin) &&
+ for i in $(test_seq 8000)
+ do
+ printf "create refs/tags/old-%d %s\n" "$i" "$blob" ||
+ return 1
+ done >repo2/input-old &&
+ sed "s/old-/new-/" <repo2/input-old >repo2/input-new &&
+ git -C repo2 update-ref --stdin <repo2/input-old &&
+ git -C repo2 for-each-ref --format="delete %(refname)" |
+ git -C repo2 update-ref --stdin
+'
+
+test_perf "create new refs after deleting differently-named refs" '
+ git -C repo2 update-ref --stdin <repo2/input-new &&
+ git -C repo2 for-each-ref --format="delete %(refname)" refs/tags/ |
+ git -C repo2 update-ref --stdin
+'
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 2/2] reftable: fix quadratic behavior in the presence of tombstones
From: Kristofer Karlsson via GitGitGadget @ 2026-07-10 10:36 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Kristofer Karlsson
In-Reply-To: <pull.2166.v3.git.1783679767.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
When many tombstones are present in a reftable, operations that need
to look up or iterate over refs exhibit quadratic behavior. With
8000 refs deleted and re-created, update-ref takes ~15s, quadrupling
for each doubling of input size.
The root cause is the merged iterator's suppress_deletions flag.
When set, merged_iter_next_void() silently consumes tombstone records
in a tight internal loop before returning to the caller. This
prevents higher-level code from checking iteration bounds (such as
prefix or refname comparisons) until after all tombstones have been
scanned.
This affects any code path that seeks into a range containing
tombstones, including:
- refs_verify_refnames_available() seeks to "refs/tags/foo-1/" to
check for D/F conflicts and must scan through all subsequent
tombstones before the caller can see that they are past the prefix
of interest.
- reftable_backend_read_ref() seeks to a specific refname and must
scan through all subsequent tombstones before returning "not
found", because the merged iterator skips the matching tombstone
and searches for the next live record.
Fix this by making suppress_deletions configurable via
reftable_stack_options instead of unconditionally enabling it. Git
no longer sets the flag, so tombstones are now returned to callers in
the reftable backend, which skip them after their existing bounds
checks. This allows iteration to terminate as soon as a tombstone
past the relevant bound is encountered.
Downstream users of the reftable library (e.g. libgit2) can still
enable suppress_deletions through the stack options to retain the
previous behavior.
This also requires adding deletion checks to the log iteration paths,
since suppress_deletions applied to both ref and log iterators.
Both tests in p1401 go from ~13s to ~0.2s with this change.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
refs/reftable-backend.c | 54 +++++++++++++++++++++++++++++++--------
reftable/reftable-stack.h | 2 ++
reftable/stack.c | 2 +-
3 files changed, 46 insertions(+), 12 deletions(-)
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 212408c769..028f0211af 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -84,7 +84,8 @@ static int reftable_backend_read_ref(struct reftable_backend *be,
if (ret)
goto done;
- if (strcmp(ref.refname, refname)) {
+ if (strcmp(ref.refname, refname) ||
+ reftable_ref_record_is_deletion(&ref)) {
ret = 1;
goto done;
}
@@ -110,7 +111,6 @@ static int reftable_backend_read_ref(struct reftable_backend *be,
oidread(oid, reftable_ref_record_val1(&ref),
&hash_algos[hash_id]);
} else {
- /* We got a tombstone, which should not happen. */
BUG("unhandled reference value type %d", ref.value_type);
}
@@ -652,6 +652,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
break;
}
+ if (iter->ref.value_type == REFTABLE_REF_DELETION)
+ continue;
+
if (iter->exclude_patterns && should_exclude_current_ref(iter))
continue;
@@ -1532,6 +1535,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
ret = 0;
break;
}
+ if (reftable_log_record_is_deletion(&log))
+ continue;
ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
tombstone = &logs[logs_nr++];
@@ -1929,6 +1934,8 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
ret = 0;
break;
}
+ if (reftable_log_record_is_deletion(&old_log))
+ continue;
free(old_log.refname);
@@ -2061,6 +2068,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
if (iter->err)
break;
+ if (reftable_log_record_is_deletion(&iter->log))
+ continue;
+
/*
* We want the refnames that we have reflogs for, so we skip if
* we've already produced this name. This could be faster by
@@ -2220,6 +2230,8 @@ static int reftable_be_for_each_reflog_ent_reverse(struct ref_store *ref_store,
ret = 0;
break;
}
+ if (reftable_log_record_is_deletion(&log))
+ continue;
ret = yield_log_record(refs, &log, fn, cb_data);
if (ret)
@@ -2272,6 +2284,10 @@ static int reftable_be_for_each_reflog_ent(struct ref_store *ref_store,
ret = 0;
break;
}
+ if (reftable_log_record_is_deletion(&log)) {
+ reftable_log_record_release(&log);
+ continue;
+ }
ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
logs[logs_nr++] = log;
@@ -2318,18 +2334,26 @@ static int reftable_be_reflog_exists(struct ref_store *ref_store,
goto done;
/*
- * Check whether we get at least one log record for the given ref name.
- * If so, the reflog exists, otherwise it doesn't.
+ * Check whether we get at least one non-deleted log record for the
+ * given ref name. If so, the reflog exists, otherwise it doesn't.
*/
- ret = reftable_iterator_next_log(&it, &log);
- if (ret < 0)
- goto done;
- if (ret > 0) {
- ret = 0;
- goto done;
+ while (1) {
+ ret = reftable_iterator_next_log(&it, &log);
+ if (ret < 0)
+ goto done;
+ if (ret > 0) {
+ ret = 0;
+ goto done;
+ }
+ if (strcmp(log.refname, refname)) {
+ ret = 0;
+ goto done;
+ }
+ if (!reftable_log_record_is_deletion(&log))
+ break;
}
- ret = strcmp(log.refname, refname) == 0;
+ ret = 1;
done:
reftable_iterator_destroy(&it);
@@ -2442,6 +2466,8 @@ static int write_reflog_delete_table(struct reftable_writer *writer, void *cb_da
ret = 0;
break;
}
+ if (reftable_log_record_is_deletion(&log))
+ continue;
tombstone.refname = (char *)arg->refname;
tombstone.value_type = REFTABLE_LOG_DELETION;
@@ -2625,6 +2651,10 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
reftable_log_record_release(&log);
break;
}
+ if (reftable_log_record_is_deletion(&log)) {
+ reftable_log_record_release(&log);
+ continue;
+ }
oidread(&old_oid, log.value.update.old_hash,
ref_store->repo->hash_algo);
@@ -2791,6 +2821,8 @@ static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,
report.path = refname.buf;
switch (ref.value_type) {
+ case REFTABLE_REF_DELETION:
+ continue;
case REFTABLE_REF_VAL1:
case REFTABLE_REF_VAL2: {
struct object_id oid;
diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
index 11f9963f4f..5d22d84e80 100644
--- a/reftable/reftable-stack.h
+++ b/reftable/reftable-stack.h
@@ -42,6 +42,8 @@ struct reftable_stack_options {
*/
void (*on_reload)(void *payload);
void *on_reload_payload;
+
+ int suppress_deletions;
};
/* open a new reftable stack. The tables along with the table list will be
diff --git a/reftable/stack.c b/reftable/stack.c
index ab12926708..caaedf24d6 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -337,7 +337,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
/* Update the stack to point to the new tables. */
if (st->merged)
reftable_merged_table_free(st->merged);
- new_merged->suppress_deletions = 1;
+ new_merged->suppress_deletions = st->opts.suppress_deletions;
st->merged = new_merged;
if (st->tables)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v8 0/4] graph: indent visual roots in graph
From: Pablo Sabater @ 2026-07-10 10:37 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, ayu.chandekar, chandrapratap3519,
christian.couder, gitster, jltobler, karthik.188, krka, peff,
phillip.wood, siddharthasthana31, mroik
In-Reply-To: <20260704-ps-pre-commit-indent-v7-0-a94706cc8376@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.
GitHub CI: https://github.com/pabloosabaterr/git/actions/runs/29082267633
[1]: https://lore.kernel.org/git/xmqqwnwajbuj.fsf@gitster.c.googlers.com/
V7 DIFF:
- New commit (2nd) "revision: add next_commit_to_show()" I wanted to
drop (!revs->max_count_stage && !revs->reverse_output_stage) because
--reverse is not compatible with --graph and I wanted the indentation
to work with --max-count-oldest. Dropping it broke tests at t4202
because the lookahead buffer was being populated from a different
source.
This new commit adds a helper to get the commits for the lookahead
from the same source.
- Typos and style.
- Added --max-count and --max-count-oldest tests.
- graph_get_lookahead_room() now uses ARRAY_SIZE() instead of the
hardcoded size 2.
- Added an assert on graph_pop_lookahead()
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
Pablo Sabater (4):
lib-log-graph: move check_graph function
revision: add next_commit_to_show()
graph: add a 2 commit buffer for lookahead
graph: indent visual root in graph
graph.c | 286 +++++++++++++++++
graph.h | 17 ++
revision.c | 48 ++-
t/lib-log-graph.sh | 5 +
t/meson.build | 1 +
t/t4215-log-skewed-merges.sh | 33 +-
t/t4218-log-graph-indentation.sh | 473 +++++++++++++++++++++++++++++
t/t6016-rev-list-graph-simplify-history.sh | 25 +-
8 files changed, 843 insertions(+), 45 deletions(-)
base-commit: f85a7e662054a7b0d9070e432508831afa214b47
^ permalink raw reply
* [PATCH v8 1/4] lib-log-graph: move check_graph function
From: Pablo Sabater @ 2026-07-10 10:37 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, ayu.chandekar, chandrapratap3519,
christian.couder, gitster, jltobler, karthik.188, krka, peff,
phillip.wood, siddharthasthana31, mroik
In-Reply-To: <20260710-ps-pre-commit-indent-v8-0-d3b636463bf4@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 v8 2/4] revision: add next_commit_to_show()
From: Pablo Sabater @ 2026-07-10 10:37 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, ayu.chandekar, chandrapratap3519,
christian.couder, gitster, jltobler, karthik.188, krka, peff,
phillip.wood, siddharthasthana31, mroik
In-Reply-To: <20260710-ps-pre-commit-indent-v8-0-d3b636463bf4@gmail.com>
get_revision() gets its commits from two sources depending on the mode:
1. Normally it gets the commits from get_revision_internal().
2. --max-count-oldest which was introduced at bb4ce23284 (revision.c:
implement --max-count-oldest, 2026-05-19) gets the commits by popping
from a saved list at revs->commits marking SHOWN and CHILD_SHOWN on
each popped commit.
Extract the choice logic into a helper, next_commit_to_show(), which
returns the next commit regardless of the source it comes from.
This has no change in behavior. The helper is needed in a subsequent
commit that pre-fetches two commits into a buffer for lookahead purposes
and needs to pre-fetch from the same source.
The --reverse branch keeps its own pop loop. Using the helper for
--reverse would additionally set SHOWN and CHILD_SHOWN which is not
desired and a behavior change.
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
revision.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/revision.c b/revision.c
index 0c95edef59..288935943f 100644
--- a/revision.c
+++ b/revision.c
@@ -4658,12 +4658,34 @@ static void retrieve_oldest_commits(struct rev_info *revs,
commit_list_insert(c, queue);
}
+/*
+ * Returns the next commit that will be shown, regardless of whether it comes
+ * directly from the revision walk or from the list saved by the staged output
+ * of --max-count-oldest.
+ */
+static struct commit *next_commit_to_show(struct rev_info *revs)
+{
+ struct commit *c;
+ struct commit_list *p;
+
+ if (!revs->max_count_stage)
+ return get_revision_internal(revs);
+
+ c = pop_commit(&revs->commits);
+ if (c) {
+ c->object.flags |= SHOWN;
+ if (!(c->object.flags & BOUNDARY))
+ for (p = c->parents; p; p = p->next)
+ p->item->object.flags |= CHILD_SHOWN;
+ }
+ return c;
+}
+
struct commit *get_revision(struct rev_info *revs)
{
struct commit *c;
struct commit_list *reversed;
struct commit_list *queue = NULL;
- struct commit_list *p;
if (revs->max_count_type == 1 && !revs->max_count_stage) {
retrieve_oldest_commits(revs, &queue);
@@ -4693,17 +4715,7 @@ struct commit *get_revision(struct rev_info *revs)
return c;
}
- if (revs->max_count_stage) {
- c = pop_commit(&revs->commits);
- if (c) {
- c->object.flags |= SHOWN;
- if (!(c->object.flags & BOUNDARY))
- for (p = c->parents; p; p = p->next)
- p->item->object.flags |= CHILD_SHOWN;
- }
- } else {
- c = get_revision_internal(revs);
- }
+ c = next_commit_to_show(revs);
if (c && revs->graph)
graph_update(revs->graph, c);
--
2.54.0
^ permalink raw reply related
* [PATCH v8 3/4] graph: add a 2 commit buffer for lookahead
From: Pablo Sabater @ 2026-07-10 10:37 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, ayu.chandekar, chandrapratap3519,
christian.couder, gitster, jltobler, karthik.188, krka, peff,
phillip.wood, siddharthasthana31, mroik
In-Reply-To: <20260710-ps-pre-commit-indent-v8-0-d3b636463bf4@gmail.com>
In a subsequent commit the graph renderer needs to know if the next
commit is a visual root or if it is the last commit to be shown. This
requires peeking 2 commits ahead.
Commits are pre-fetched in get_revision() through next_commit_to_show()
where they are also marked as SHOWN, regardless the source they come
from.
Update graph_is_interesting() so it considers commits inside the
lookahead buffer as interesting as well.
Helped-by: Kristofer Karlsson <krka@spotify.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
graph.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
graph.h | 17 +++++++++++++++++
revision.c | 18 ++++++++++++++++--
3 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/graph.c b/graph.c
index 842282685f..89ebcf7540 100644
--- a/graph.c
+++ b/graph.c
@@ -315,6 +315,14 @@ struct git_graph {
* diff_output_prefix_callback().
*/
struct strbuf prefix_buf;
+
+ /*
+ * Lookahead buffer: up to 2 pre-fetched commits that will be shown.
+ * Populated by get_revision() so graph_peek_next_visible() can use
+ * actual walk results instead of peeking at rev_info internals.
+ */
+ struct commit *lookahead[2];
+ int lookahead_nr;
};
static inline int graph_needs_truncation(struct git_graph *graph, int lane)
@@ -388,6 +396,9 @@ struct git_graph *graph_init(struct rev_info *opt)
graph->num_columns = 0;
graph->num_new_columns = 0;
graph->mapping_size = 0;
+ graph->lookahead[0] = NULL;
+ graph->lookahead[1] = NULL;
+ graph->lookahead_nr = 0;
/*
* Start the column color at the maximum value, since we'll
* always increment it for the first commit we output.
@@ -456,6 +467,15 @@ static void graph_ensure_capacity(struct git_graph *graph, int num_columns)
*/
static int graph_is_interesting(struct git_graph *graph, struct commit *commit)
{
+ /*
+ * Commits in the lookahead buffer have been pre-fetched by
+ * get_revision() and will be shown in the future. They already have
+ * the SHOWN flag set when they were pre-fetched but the graph still
+ * needs to treat them as interesting parents.
+ */
+ for (int i = 0; i < graph->lookahead_nr; i++)
+ if (graph->lookahead[i] == commit)
+ return 1;
/*
* If revs->boundary is set, commits whose children have
* been shown are always interesting, even if they have the
@@ -763,6 +783,37 @@ static int graph_needs_pre_commit_line(struct git_graph *graph)
graph->expansion_row < graph_num_expansion_rows(graph);
}
+struct commit *graph_pop_lookahead(struct git_graph *graph)
+{
+ struct commit *c;
+
+ if (!graph->lookahead_nr)
+ return NULL;
+
+ c = graph->lookahead[0];
+ if (!c)
+ BUG("lookahead buffer has %d entries but the first one is NULL",
+ graph->lookahead_nr);
+
+ graph->lookahead[0] = graph->lookahead[1];
+ graph->lookahead[1] = NULL;
+ graph->lookahead_nr--;
+ return c;
+}
+
+int graph_get_lookahead_room(struct git_graph *graph)
+{
+ return (int)ARRAY_SIZE(graph->lookahead) - graph->lookahead_nr;
+}
+
+void graph_push_lookahead(struct git_graph *graph, struct commit *c)
+{
+ if (!graph_get_lookahead_room(graph))
+ BUG("pushing into lookahead buffer when it is already full");
+
+ graph->lookahead[graph->lookahead_nr++] = c;
+}
+
void graph_update(struct git_graph *graph, struct commit *commit)
{
struct commit_list *parent;
diff --git a/graph.h b/graph.h
index 3fd1dcb2e9..1193711fb8 100644
--- a/graph.h
+++ b/graph.h
@@ -262,4 +262,21 @@ void graph_show_commit_msg(struct git_graph *graph,
FILE *file,
struct strbuf const *sb);
+/*
+ * Pop the first commit from the graph's lookahead buffer.
+ * Returns NULL if the buffer is empty.
+ */
+struct commit *graph_pop_lookahead(struct git_graph *graph);
+
+/*
+ * Returns how many more commits can be added to the lookahead buffer.
+ */
+int graph_get_lookahead_room(struct git_graph *graph);
+
+/*
+ * Push a commit into the lookahead buffer. Must only be called when
+ * graph_get_lookahead_room() returns > 0.
+ */
+void graph_push_lookahead(struct git_graph *graph, struct commit *c);
+
#endif /* GRAPH_H */
diff --git a/revision.c b/revision.c
index 288935943f..258c3cf782 100644
--- a/revision.c
+++ b/revision.c
@@ -4715,10 +4715,24 @@ struct commit *get_revision(struct rev_info *revs)
return c;
}
- c = next_commit_to_show(revs);
+ if (revs->graph) {
+ c = graph_pop_lookahead(revs->graph);
+ if (!c)
+ c = next_commit_to_show(revs);
+ } else {
+ c = next_commit_to_show(revs);
+ }
- if (c && revs->graph)
+ if (c && revs->graph) {
+ while (graph_get_lookahead_room(revs->graph)) {
+ struct commit *next = next_commit_to_show(revs);
+ if (!next)
+ break;
+ graph_push_lookahead(revs->graph, next);
+ }
graph_update(revs->graph, c);
+ }
+
if (!c) {
free_saved_parents(revs);
commit_list_free(revs->previous_parents);
--
2.54.0
^ permalink raw reply related
* [PATCH v8 4/4] graph: indent visual root in graph
From: Pablo Sabater @ 2026-07-10 10:37 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, ayu.chandekar, chandrapratap3519,
christian.couder, gitster, jltobler, karthik.188, krka, peff,
phillip.wood, siddharthasthana31, mroik
In-Reply-To: <20260710-ps-pre-commit-indent-v8-0-d3b636463bf4@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.
[1]: https://lore.kernel.org/git/xmqqwnwajbuj.fsf@gitster.c.googlers.com/
Helped-by: Kristofer Karlsson <krka@spotify.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 | 235 +++++++++++++++++++
t/meson.build | 1 +
t/t4218-log-graph-indentation.sh | 473 +++++++++++++++++++++++++++++++++++++++
3 files changed, 709 insertions(+)
diff --git a/graph.c b/graph.c
index 89ebcf7540..68ddde24b6 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 int is_merge_parent:1;
};
enum graph_state {
GRAPH_PADDING,
GRAPH_SKIP,
GRAPH_PRE_COMMIT,
+ GRAPH_PRE_ROOT,
GRAPH_COMMIT,
GRAPH_POST_MERGE,
GRAPH_COLLAPSING
@@ -323,6 +334,51 @@ struct git_graph {
*/
struct commit *lookahead[2];
int lookahead_nr;
+
+ /*
+ * If a commit is a visual root, we need to indent it to prevent
+ * unrelated commits from being vertically adjacent to it.
+ */
+ unsigned int 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 int visual_root_cascade:1;
+
+ /*
+ * Set when the current commit was already present in graph->columns
+ * before being processed.
+ */
+ unsigned int 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)
@@ -399,6 +455,8 @@ struct git_graph *graph_init(struct rev_info *opt)
graph->lookahead[0] = NULL;
graph->lookahead[1] = NULL;
graph->lookahead_nr = 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.
@@ -581,6 +639,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;
@@ -592,6 +655,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) {
@@ -630,6 +694,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)
@@ -721,10 +791,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
*/
@@ -814,9 +894,104 @@ void graph_push_lookahead(struct git_graph *graph, struct commit *c)
graph->lookahead[graph->lookahead_nr++] = c;
}
+/*
+ * 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);
+}
+
+/*
+ * Peeks the next commits via the lookahead buffer and sets the lookahead flags.
+ */
+static void graph_peek_next_visible(struct git_graph *graph,
+ struct graph_lookahead_flags *flags)
+{
+ flags->is_next_visible = 0;
+ flags->is_next_visual_root = 0;
+ flags->next_has_column = 0;
+
+ if (!graph->lookahead_nr)
+ return;
+
+ flags->is_next_visible = 1;
+ flags->next_has_column =
+ graph_find_new_column_by_commit(graph, graph->lookahead[0]) >= 0;
+
+ if (!graph_is_visual_root_candidate(graph->lookahead[0]))
+ return;
+
+ if (graph->lookahead_nr >= 2)
+ flags->is_next_visual_root = 1;
+}
+
+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
@@ -847,6 +1022,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;
/*
@@ -864,11 +1056,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
@@ -1116,6 +1313,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)) {
@@ -1487,6 +1695,30 @@ 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 must only be called on a
+ * visual root.
+ */
+ if (!graph->is_visual_root)
+ BUG("commit must be a visual root to call pre_root_line");
+
+ for (int 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;
@@ -1512,6 +1744,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 7c3c070426..cce5ba71f9 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -577,6 +577,7 @@ integration_tests = [
't4215-log-skewed-merges.sh',
't4216-log-bloom.sh',
't4217-log-limit.sh',
+ 't4218-log-graph-indentation.sh',
't4219-log-follow-merge.sh',
't4252-am-options.sh',
't4253-am-keep-cr-dos.sh',
diff --git a/t/t4218-log-graph-indentation.sh b/t/t4218-log-graph-indentation.sh
new file mode 100755
index 0000000000..a4768cd3f6
--- /dev/null
+++ b/t/t4218-log-graph-indentation.sh
@@ -0,0 +1,473 @@
+#!/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" &&
+ test_might_fail git rm -rf .
+}
+
+# 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
+'
+
+test_expect_success 'filtered parent cascading edge case' '
+ create_orphan _27 &&
+ echo test >foo.txt &&
+ git add foo.txt &&
+ test_tick &&
+ git commit -m "D (last)" &&
+
+ create_orphan _25 &&
+ echo test >other.txt &&
+ git add other.txt &&
+ test_tick &&
+ git commit -m "C-filtered" &&
+
+ echo test >foo.txt &&
+ git add foo.txt &&
+ test_tick &&
+ git commit -m "B (child of filtered)" &&
+
+ create_orphan _26 &&
+ echo test >foo.txt &&
+ git add foo.txt &&
+ test_tick &&
+ git commit -m "A (visual root)" &&
+
+ lib_test_check_graph _25 _26 _27 -- foo.txt <<-\EOF
+ * A (visual root)
+ * B (child of filtered)
+ * D (last)
+ EOF
+'
+
+test_expect_success 'multiple filtered parents in sequence' '
+ create_orphan _44 &&
+ 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 &&
+ 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 &&
+ echo e >foo.txt && git add foo.txt && git commit -m "46_A" &&
+
+ lib_test_check_graph _44 _45 _46 -- foo.txt <<-\EOF
+ * 44_C
+ * 45_C
+ * 46_A
+ EOF
+'
+
+# These 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 children' '
+ (
+ 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_expect_success 'two unrelated merges' '
+ create_orphan _50 && test_commit 50_A &&
+ git checkout -b _51 &&
+ test_commit 51_A && test_commit 51_B &&
+ git checkout _50 &&
+ git merge --no-ff _51 -m 50_B &&
+
+ create_orphan _52 && test_commit 52_A &&
+ git checkout -b _53 &&
+ test_commit 53_A && test_commit 53_B &&
+ git checkout _52 &&
+ git merge --no-ff _53 -m 52_B &&
+
+ lib_test_check_graph _52 _50 <<-\EOF
+ * 52_B
+ |\
+ | * 53_B
+ | * 53_A
+ |/
+ \
+ * 52_A
+ * 50_B
+ |\
+ | * 51_B
+ | * 51_A
+ |/
+ * 50_A
+ EOF
+'
+
+test_expect_success '--max-count treats the last visible commit as the last commit' '
+ lib_test_check_graph --max-count=2 _8 _9 _10 <<-\EOF
+ * 10_A
+ * 9_A
+ EOF
+'
+
+test_expect_success '--max-count=1 shows a single root without indentation' '
+ lib_test_check_graph --max-count=1 _8 _9 _10 <<-\EOF
+ * 10_A
+ EOF
+'
+
+test_expect_success '--max-count-oldest indents visual roots' '
+ lib_test_check_graph --max-count-oldest=3 _8 _9 _10 <<-\EOF
+ * 10_A
+ * 9_A
+ * 8_A
+ EOF
+'
+
+test_done
--
2.54.0
^ permalink raw reply related
* [PATCH v2 00/12] coverity: avoid dereferencing NULL
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
In-Reply-To: <pull.2174.git.1783590159.gitgitgadget@gmail.com>
This is a continuation of the effort I started in the patch series that
became js/coverity-fixes. This next batch adds guards to avoid dereferencing
NULL pointers and accessing NULL file descriptors.
Changes since v1:
* Calling remote_tracking() no longer returns -1 when remote is NULL, but
instead BUG()s out.
* bisect_successful() returns with BISECT_FAILED instead of the -1 that
only worked by happenstance.
* The commit "revision: avoid dereferencing NULL in add_parents_only()" now
comes with a regression test.
* The commit "bisect: ensure non-NULL head before using it" no longer
claims that the fixed bug can be triggered with the current code base.
* The missing shallow commit's OID is no longer computed twice.
* A follow-up commit was folded into this patch series that lets
write_one_shallow() avoid the rolling buffers of oid_to_hex(), as
suggested by Junio. It technically does not fit the goal of this patch
series (fixing issues pointed out by Coverity), but was asked for
explicitly.
Johannes Schindelin (12):
diffcore-break: guard against NULLed queue entries in merge loop
diff: handle NULL return from repo_get_commit_tree()
remote: guard `remote_tracking()` against NULL remote
reftable/stack: guard against NULL list_file in stack_destroy
mailsplit: move NULL check before first use of file handle
bisect: handle NULL commit in `bisect_successful()`
replay: die when --onto does not peel to a commit
revision: avoid dereferencing NULL in `add_parents_only()`
pack-bitmap: handle missing bitmap for base MIDX
bisect: ensure non-NULL `head` before using it
shallow: fix NULL dereference
shallow: give write_one_shallow() its own hex buffer
builtin/bisect.c | 9 ++++++++-
builtin/diff.c | 10 +++++++---
builtin/mailsplit.c | 6 +++---
diffcore-break.c | 2 ++
pack-bitmap.c | 4 ++++
reftable/stack.c | 3 ++-
remote.c | 2 ++
replay.c | 8 ++++++--
revision.c | 9 +++++++--
shallow.c | 7 ++++---
t/t0410-partial-clone.sh | 18 ++++++++++++++++++
11 files changed, 63 insertions(+), 15 deletions(-)
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2174%2Fdscho%2Fcoverity-fixes-null-safety-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2174/dscho/coverity-fixes-null-safety-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2174
Range-diff vs v1:
1: df00334f8b = 1: df00334f8b diffcore-break: guard against NULLed queue entries in merge loop
2: 4fdba0542b = 2: 4fdba0542b diff: handle NULL return from repo_get_commit_tree()
3: dcaefc5987 ! 3: 1398a2f120 remote: guard `remote_tracking()` against NULL remote
@@ remote.c: static int remote_tracking(struct remote *remote, const char *refname,
char *dst;
+ if (!remote)
-+ return -1; /* no remote to look up tracking ref */
++ BUG("remote_tracking() called with NULL remote");
dst = apply_refspecs(&remote->fetch, refname);
if (!dst)
return -1; /* no tracking ref for refname at remote */
4: d7bc7fce35 = 4: 285f019fb3 reftable/stack: guard against NULL list_file in stack_destroy
5: 41eef047ae = 5: 12c2c8450e mailsplit: move NULL check before first use of file handle
6: 7041375108 ! 6: ca818ee405 bisect: handle NULL commit in `bisect_successful()`
@@ builtin/bisect.c: static int bisect_successful(struct bisect_terms *terms)
refs_read_ref(get_main_ref_store(the_repository), bad_ref, &oid);
commit = lookup_commit_reference_by_name(bad_ref);
+ if (!commit) {
-+ res = error(_("could not find commit for '%s'"), bad_ref);
++ error(_("could not find commit for '%s'"), bad_ref);
+ free(bad_ref);
-+ return res;
++ return BISECT_FAILED;
+ }
repo_format_commit_message(the_repository, commit, "%s", &commit_name,
&pp);
7: a7245cdffa = 7: 8216769be9 replay: die when --onto does not peel to a commit
8: 0675767797 ! 8: 41285dd8e1 revision: avoid dereferencing NULL in `add_parents_only()`
@@ revision.c: static int add_parents_only(struct rev_info *revs, const char *arg_,
if (it->type != OBJ_TAG)
break;
if (!((struct tag*)it)->tagged)
+
+ ## t/t0410-partial-clone.sh ##
+@@ t/t0410-partial-clone.sh: test_expect_success 'rev-list dies for missing objects on cmd line' '
+ done
+ '
+
++test_expect_success '--exclude-promisor-objects with ^@ on missing object' '
++ rm -rf repo &&
++ test_create_repo repo &&
++ test_commit -C repo foo &&
++ test_commit -C repo bar &&
++
++ COMMIT=$(git -C repo rev-parse foo) &&
++ promise_and_delete "$COMMIT" &&
++
++ git -C repo config core.repositoryformatversion 1 &&
++ git -C repo config extensions.partialclone "arbitrary string" &&
++
++ # Ensure that "$COMMIT^@" is handled gracefully even though the
++ # actual commits are missing.
++ git -C repo rev-list --exclude-promisor-objects "$COMMIT^@" >out &&
++ test_must_be_empty out
++'
++
+ test_expect_success 'single promisor remote can be re-initialized gracefully' '
+ # ensure one promisor is in the promisors list
+ rm -rf repo &&
9: 0b27860478 = 9: cccd36137f pack-bitmap: handle missing bitmap for base MIDX
10: 428a3a006b ! 10: 376a6581cb bisect: ensure non-NULL `head` before using it
@@ Commit message
Later, that variable is passed to `repo_get_oid()` and `starts_with()`,
both of which would dereference the NULL pointer.
- The scenario "`refs_resolve_ref_unsafe()` returns NULL but
- `repo_get_oid()` succeeds" can happen when HEAD is a detached bare OID
- that the ref backend cannot resolve symbolically (a potential edge case
- with the reftable backend) but the OID itself is valid. In this case,
- the bisect-start file does not yet exist (this is a fresh "git bisect
- start"), so the else branch is taken with the NULL `head`.
-
- Simply assign "HEAD" to `head` as a fallback to address this.
-
- Pointed out by Coverity.
-
- Assisted-by: Claude Opus 4.6
+ A concrete trigger for `refs_resolve_ref_unsafe()` returning NULL while
+ `repo_get_oid()` succeeds could not be constructed against the ref
+ backends currently in the tree; the naive case (a symbolic HEAD pointing
+ at a nonexistent branch, in either the files or the reftable backend)
+ fails in both calls consistently and returns via the existing
+ `error(_("bad HEAD - I need a HEAD"))` path. Coverity, however, flags
+ the leftover use of `head` after the outer `if (!head)` on a formal
+ reading: `head` is still NULL at that point, and both `starts_with(head,
+ ...)` and the second `repo_get_oid(..., head, ...)` in the else-branch
+ would dereference it if that state were ever reached.
+
+ Removing the outer check would risk regressing to a crash if a future
+ ref backend ever manages to hit the "returns NULL for HEAD but has a
+ valid OID for HEAD" state. Assigning the literal string "HEAD" as a
+ safe fallback documents the intent and satisfies the analyzer without
+ changing behavior in any code path we can currently reach.
+
+ Assisted-by: Claude Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## builtin/bisect.c ##
11: 9f3a239484 ! 11: e581bc91ee shallow: fix NULL dereference
@@ Commit message
## shallow.c ##
@@ shallow.c: static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
+ struct commit *c = lookup_commit(the_repository, &graft->oid);
if (!c || !(c->object.flags & SEEN)) {
if (data->flags & VERBOSE)
- printf("Removing %s from .git/shallow\n",
+- printf("Removing %s from .git/shallow\n",
- oid_to_hex(&c->object.oid));
-+ oid_to_hex(&graft->oid));
++ printf("Removing %s from .git/shallow\n", hex);
return 0;
}
}
-: ---------- > 12: 2ef74b52fa shallow: give write_one_shallow() its own hex buffer
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 02/12] diff: handle NULL return from repo_get_commit_tree()
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `repo_get_commit_tree()` function can return NULL when a commit's
tree object is not available (e.g., the commit was parsed but its
maybe_tree field is unset and the commit is not in the commit-graph). In
cmd_diff(), the return value is immediately dereferenced via ->object
without a NULL check, which would crash if the tree cannot be loaded.
Add an explicit NULL check and die with a descriptive message.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/diff.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index 4b46e394ce..18b1083e98 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -579,9 +579,13 @@ int cmd_diff(int argc,
obj = deref_tag(the_repository, obj, NULL, 0);
if (!obj)
die(_("invalid object '%s' given."), name);
- if (obj->type == OBJ_COMMIT)
- obj = &repo_get_commit_tree(the_repository,
- ((struct commit *)obj))->object;
+ if (obj->type == OBJ_COMMIT) {
+ struct tree *tree = repo_get_commit_tree(
+ the_repository, (struct commit *)obj);
+ if (!tree)
+ die(_("unable to read tree object for commit '%s'"), name);
+ obj = &tree->object;
+ }
if (obj->type == OBJ_TREE) {
if (sdiff.skip && bitmap_get(sdiff.skip, i))
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 03/12] remote: guard `remote_tracking()` against NULL remote
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `remote_tracking()` function unconditionally dereferences
`remote->fetch` without checking whether remote is NULL.
In practice, this never happens because the only caller (`apply_cas()`)
guards the calls to this function by checking the `use_tracking` and
`use_tracking_for_rest` attributes.
However, it requires quite involved reasoning to reach that conclusion,
and is therefore fragile. Just return -1 ("no tracking ref") when there
is no remote to work with.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
remote.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/remote.c b/remote.c
index 00723b385e..887e388f9c 100644
--- a/remote.c
+++ b/remote.c
@@ -2681,6 +2681,8 @@ static int remote_tracking(struct remote *remote, const char *refname,
{
char *dst;
+ if (!remote)
+ BUG("remote_tracking() called with NULL remote");
dst = apply_refspecs(&remote->fetch, refname);
if (!dst)
return -1; /* no tracking ref for refname at remote */
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 01/12] diffcore-break: guard against NULLed queue entries in merge loop
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The outer loop in `diffcore_merge_broken()` sets `q->queue[j]` to NULL
when it merges a broken pair back together, and has a NULL check to skip
such entries on subsequent iterations. The inner loop, however, lacks
this guard: when it scans forward looking for a matching peer, it can
encounter a slot that was NULLed by a previous outer-loop iteration and
dereference it unconditionally.
In practice this requires at least two broken pairs whose peers
both survive rename/copy detection and appear later in the queue,
which is rare but not impossible.
Add the same `if (!pp) continue` guard to the inner loop.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
diffcore-break.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/diffcore-break.c b/diffcore-break.c
index 17b5ad1fed..b5bcc956cc 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -289,6 +289,8 @@ void diffcore_merge_broken(void)
*/
for (j = i + 1; j < q->nr; j++) {
struct diff_filepair *pp = q->queue[j];
+ if (!pp)
+ continue;
if (pp->broken_pair &&
!strcmp(pp->one->path, pp->two->path) &&
!strcmp(p->one->path, pp->two->path)) {
--
gitgitgadget
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox