* [PATCH v2 04/12] reftable/record: don't abort when decoding invalid ref value type
From: Patrick Steinhardt @ 2026-06-29 9:02 UTC (permalink / raw)
To: git; +Cc: oxsignal, Christian Couder
In-Reply-To: <20260629-pks-reftable-hardening-v2-0-b0228e7d908d@pks.im>
When decoding a ref record we read its value type from the block. In
case the type itself is invalid we call `abort()`. This is rather
heavy-handed though: the data we're reading is untrusted, so we should
treat the issue as a normal and not as a programming error.
Fix this by handling the error gracefully. Note that this also requires
us to set the value type later, as otherwise we might store an invalid
type in the record.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/record.c | 6 +++---
t/unit-tests/u-reftable-record.c | 24 ++++++++++++++++++++++++
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/reftable/record.c b/reftable/record.c
index fcd387ba5d..1fce441930 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -388,7 +388,6 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,
r->refname[key.len] = 0;
r->update_index = update_index;
- r->value_type = val_type;
switch (val_type) {
case REFTABLE_REF_VAL1:
if (in.len < hash_size) {
@@ -426,9 +425,10 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,
case REFTABLE_REF_DELETION:
break;
default:
- abort();
- break;
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
}
+ r->value_type = val_type;
return start.len - in.len;
diff --git a/t/unit-tests/u-reftable-record.c b/t/unit-tests/u-reftable-record.c
index 1bf2e170dc..9c95083ef4 100644
--- a/t/unit-tests/u-reftable-record.c
+++ b/t/unit-tests/u-reftable-record.c
@@ -11,6 +11,7 @@
#include "reftable/basics.h"
#include "reftable/constants.h"
#include "reftable/record.h"
+#include "reftable/reftable-error.h"
static void t_copy(struct reftable_record *rec)
{
@@ -202,6 +203,29 @@ void test_reftable_record__ref_record_roundtrip(void)
reftable_buf_release(&scratch);
}
+void test_reftable_record__ref_record_decode_invalid_value_type(void)
+{
+ struct reftable_buf scratch = REFTABLE_BUF_INIT;
+ struct reftable_record out = {
+ .type = REFTABLE_BLOCK_TYPE_REF,
+ };
+ struct reftable_buf key = REFTABLE_BUF_INIT;
+ uint8_t buffer[1024] = { 0 };
+ struct string_view dest = {
+ .buf = buffer,
+ .len = sizeof(buffer),
+ };
+
+ cl_must_pass(reftable_buf_addstr(&key, "refs/heads/master"));
+ cl_assert_equal_i(reftable_record_decode(&out, key, REFTABLE_NR_REF_VALUETYPES,
+ dest, REFTABLE_HASH_SIZE_SHA1, &scratch),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_record_release(&out);
+ reftable_buf_release(&key);
+ reftable_buf_release(&scratch);
+}
+
void test_reftable_record__log_record_comparison(void)
{
struct reftable_record in[3] = {
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v2 03/12] reftable/basics: fix OOB read on binary search of empty range
From: Patrick Steinhardt @ 2026-06-29 9:02 UTC (permalink / raw)
To: git; +Cc: oxsignal, Christian Couder
In-Reply-To: <20260629-pks-reftable-hardening-v2-0-b0228e7d908d@pks.im>
`binsearch()` performs a binary search over a range of `sz` elements by
repeatedly calling the comparison function with indices into that range.
When the range is empty though, there is no valid index to call the
comparison function with. We still end up executing the comparison
function though with an index of 0, which of course will cause an
out-of-bounds read.
Return early when the range is empty.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/basics.c | 3 +++
t/unit-tests/u-reftable-basics.c | 11 +++++++++++
2 files changed, 14 insertions(+)
diff --git a/reftable/basics.c b/reftable/basics.c
index e969927b61..f0442a46cf 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -152,6 +152,9 @@ size_t binsearch(size_t sz, int (*f)(size_t k, void *args), void *args)
size_t lo = 0;
size_t hi = sz;
+ if (!sz)
+ return 0;
+
/* Invariants:
*
* (hi == sz) || f(hi) == true
diff --git a/t/unit-tests/u-reftable-basics.c b/t/unit-tests/u-reftable-basics.c
index 73566ed0eb..c5d83b6714 100644
--- a/t/unit-tests/u-reftable-basics.c
+++ b/t/unit-tests/u-reftable-basics.c
@@ -60,6 +60,17 @@ void test_reftable_basics__binsearch(void)
}
}
+static int unreachable_lesseq(size_t i UNUSED, void *args UNUSED)
+{
+ cl_fail("comparison function called for empty range");
+ return 0;
+}
+
+void test_reftable_basics__binsearch_empty(void)
+{
+ cl_assert_equal_i(binsearch(0, &unreachable_lesseq, NULL), 0);
+}
+
void test_reftable_basics__names_length(void)
{
const char *a[] = { "a", "b", NULL };
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v2 02/12] oss-fuzz: add fuzzer for parsing reftables
From: Patrick Steinhardt @ 2026-06-29 9:02 UTC (permalink / raw)
To: git; +Cc: oxsignal, Christian Couder
In-Reply-To: <20260629-pks-reftable-hardening-v2-0-b0228e7d908d@pks.im>
Add a new fuzzer that exercises our parsing of reftables. Fallout from
this fuzzer will be fixed over subsequent commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Makefile | 1 +
ci/run-build-and-minimal-fuzzers.sh | 1 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-reftable.c | 74 +++++++++++++++++++++++++++++++++++++
oss-fuzz/meson.build | 1 +
5 files changed, 78 insertions(+)
diff --git a/Makefile b/Makefile
index 1cec251f43..89d3edd5ea 100644
--- a/Makefile
+++ b/Makefile
@@ -2599,6 +2599,7 @@ FUZZ_OBJS += oss-fuzz/fuzz-date.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
FUZZ_OBJS += oss-fuzz/fuzz-parse-attr-line.o
+FUZZ_OBJS += oss-fuzz/fuzz-reftable.o
FUZZ_OBJS += oss-fuzz/fuzz-url-decode-mem.o
.PHONY: fuzz-objs
fuzz-objs: $(FUZZ_OBJS)
diff --git a/ci/run-build-and-minimal-fuzzers.sh b/ci/run-build-and-minimal-fuzzers.sh
index e7b97952e7..37b24b092d 100755
--- a/ci/run-build-and-minimal-fuzzers.sh
+++ b/ci/run-build-and-minimal-fuzzers.sh
@@ -21,6 +21,7 @@ date
pack-headers
pack-idx
parse-attr-line
+reftable
url-decode-mem
"
diff --git a/oss-fuzz/.gitignore b/oss-fuzz/.gitignore
index f2d74de457..dc7a127a62 100644
--- a/oss-fuzz/.gitignore
+++ b/oss-fuzz/.gitignore
@@ -5,4 +5,5 @@ fuzz-date
fuzz-pack-headers
fuzz-pack-idx
fuzz-parse-attr-line
+fuzz-reftable
fuzz-url-decode-mem
diff --git a/oss-fuzz/fuzz-reftable.c b/oss-fuzz/fuzz-reftable.c
new file mode 100644
index 0000000000..c46eac2c6b
--- /dev/null
+++ b/oss-fuzz/fuzz-reftable.c
@@ -0,0 +1,74 @@
+#include "git-compat-util.h"
+#include "reftable/basics.h"
+#include "reftable/blocksource.h"
+#include "reftable/reftable-blocksource.h"
+#include "reftable/reftable-error.h"
+#include "reftable/reftable-iterator.h"
+#include "reftable/reftable-record.h"
+#include "reftable/reftable-table.h"
+#include "reftable/reftable-writer.h"
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
+ struct reftable_block_source source = { 0 };
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+ struct reftable_table *table = NULL;
+ int err;
+
+ if (reftable_buf_add(&buf, (const char *)data, size) < 0)
+ goto out;
+ block_source_from_buf(&source, &buf);
+
+ err = reftable_table_new(&table, &source, "fuzz-input");
+ if (err < 0)
+ goto out;
+
+ /*
+ * Exercise the ref, log and raw block iterators so that we cover as
+ * much of the parsing code as possible.
+ */
+ {
+ struct reftable_ref_record ref = { 0 };
+ struct reftable_iterator it = { 0 };
+
+ reftable_table_init_ref_iterator(table, &it);
+ if (!reftable_iterator_seek_ref(&it, ""))
+ while (!reftable_iterator_next_ref(&it, &ref))
+ ;
+
+ reftable_ref_record_release(&ref);
+ reftable_iterator_destroy(&it);
+ }
+
+ {
+ struct reftable_log_record log = { 0 };
+ struct reftable_iterator it = { 0 };
+
+ reftable_table_init_log_iterator(table, &it);
+ if (!reftable_iterator_seek_log(&it, ""))
+ while (!reftable_iterator_next_log(&it, &log))
+ ;
+
+ reftable_log_record_release(&log);
+ reftable_iterator_destroy(&it);
+ }
+
+ {
+ struct reftable_table_iterator it = { 0 };
+ const struct reftable_block *block;
+
+ if (!reftable_table_iterator_init(&it, table))
+ while (!reftable_table_iterator_next(&it, &block))
+ ;
+
+ reftable_table_iterator_release(&it);
+ }
+
+out:
+ if (table)
+ reftable_table_decref(table);
+ reftable_buf_release(&buf);
+ return 0;
+}
diff --git a/oss-fuzz/meson.build b/oss-fuzz/meson.build
index 10bcac2f6d..5a3854256b 100644
--- a/oss-fuzz/meson.build
+++ b/oss-fuzz/meson.build
@@ -6,6 +6,7 @@ fuzz_programs = [
'fuzz-pack-headers.c',
'fuzz-pack-idx.c',
'fuzz-parse-attr-line.c',
+ 'fuzz-reftable.c',
'fuzz-url-decode-mem.c',
]
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v2 01/12] meson: support building fuzzers with libFuzzer
From: Patrick Steinhardt @ 2026-06-29 9:02 UTC (permalink / raw)
To: git; +Cc: oxsignal, Christian Couder
In-Reply-To: <20260629-pks-reftable-hardening-v2-0-b0228e7d908d@pks.im>
To support fuzzing via libFuzzer one has to pass a couple of compiler
options:
- It is mandatory to enable the "fuzzer-no-link" sanitizer for
coverage feedback.
- It is recommended to enable at least one more sanitizer to catch
issues, like the "address" sanitizer.
- The fuzzing executables need to be linked with "-fsanitize=fuzzer"
to wire up libFuzzer itself.
The first two items can already be achieved via the "-Db_sanitize="
option. But the last item cannot easily be achieved, as we can only
configure global link arguments.
Introduce a new "-Dfuzzers_link_args=" build option to plug this gap.
Add documentation so that users know how to set up libFuzzer.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
meson.build | 15 +++++++++++++++
meson_options.txt | 2 ++
oss-fuzz/meson.build | 1 +
3 files changed, 18 insertions(+)
diff --git a/meson.build b/meson.build
index 3247697f74..9df6fbb0a5 100644
--- a/meson.build
+++ b/meson.build
@@ -161,6 +161,21 @@
# These machine files can be passed to `meson setup` via the `--native-file`
# option.
#
+# Fuzzing
+# =======
+#
+# Meson supports building the fuzzing targets by setting `-Dfuzzers=true`. By
+# default, the targets will be built without libFuzzer and thus won't be usable
+# for fuzzing. You have to configure a couple of options to properly wire up
+# libFuzzer:
+#
+# $ meson setup build-fuzzers \
+# -Db_sanitize=address,fuzzer-no-link \
+# -Dfuzzers=true \
+# -Dfuzzers_link_args=-fsanitize=fuzzer
+# $ meson compile -C build-fuzzers
+# $ ./build-fuzzers/oss-fuzz/fuzz-config <args>
+#
# Cross compilation
# =================
#
diff --git a/meson_options.txt b/meson_options.txt
index d936ada098..dc88f130d7 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -131,3 +131,5 @@ option('test_utf8_locale', type: 'string',
description: 'Name of a UTF-8 locale used for testing.')
option('fuzzers', type: 'boolean', value: false,
description: 'Enable building fuzzers.')
+option('fuzzers_link_args', type: 'array', value: [],
+ description: 'Linker arguments used to link fuzzers. Use -fsanitize=fuzzer for fuzzing.')
diff --git a/oss-fuzz/meson.build b/oss-fuzz/meson.build
index 878afd8426..10bcac2f6d 100644
--- a/oss-fuzz/meson.build
+++ b/oss-fuzz/meson.build
@@ -16,5 +16,6 @@ foreach fuzz_program : fuzz_programs
fuzz_program,
],
dependencies: [libgit_commonmain],
+ link_args: get_option('fuzzers_link_args'),
)
endforeach
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v2 00/12] reftable: harden against corrupted tables
From: Patrick Steinhardt @ 2026-06-29 9:02 UTC (permalink / raw)
To: git; +Cc: oxsignal, Christian Couder
In-Reply-To: <20260624-pks-reftable-hardening-v1-0-66e4ce87c6b9@pks.im>
Hi,
this patch series addresses a bunch of errors that may happen when
trying to read corrupted tables. These errors include out-of-bounds
writes, out-of-bounds reads and the ability to hit abort(3p) calls.
The out-of-bounds write was originally reported by awo on the security
mailing list. As we never transfer reftables over the protocol it would
require local disk access to create such corrupted reftables, so there
isn't really an easy way to exploit these.
In any case, I took that chance and wrote a fuzzer for parsing the
tables, which surfaced a bunch of issues. At the end of this series
though the fuzzer can now run for an extended amount of time (2hrs+)
without surfacing any new issues.
Changes in v2:
- Introduce a test helper that writes a reftable block.
- Link to v1: https://patch.msgid.link/20260624-pks-reftable-hardening-v1-0-66e4ce87c6b9@pks.im
Thanks!
Patrick
---
Patrick Steinhardt (12):
meson: support building fuzzers with libFuzzer
oss-fuzz: add fuzzer for parsing reftables
reftable/basics: fix OOB read on binary search of empty range
reftable/record: don't abort when decoding invalid ref value type
t/unit-tests: introduce test helper to write reftable blocks
reftable/block: fix OOB write with bogus inflated log size
reftable/block: fix OOB read with bogus block size
reftable/block: fix OOB read with bogus restart count
reftable/block: fix use of uninitialized memory when binsearch fails
reftable/block: fix OOB read with bogus restart offset
reftable/table: fix NULL pointer access when seeking to bogus offsets
reftable/table: fix OOB read on truncated table
Makefile | 1 +
ci/run-build-and-minimal-fuzzers.sh | 1 +
meson.build | 15 +++
meson_options.txt | 2 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-reftable.c | 74 +++++++++++++++
oss-fuzz/meson.build | 2 +
reftable/basics.c | 3 +
reftable/block.c | 39 +++++++-
reftable/record.c | 6 +-
reftable/table.c | 7 ++
t/unit-tests/u-reftable-basics.c | 11 +++
t/unit-tests/u-reftable-block.c | 184 ++++++++++++++++++++++++++++++++----
t/unit-tests/u-reftable-record.c | 24 +++++
t/unit-tests/u-reftable-table.c | 91 ++++++++++++++++++
15 files changed, 435 insertions(+), 26 deletions(-)
Range-diff versus v1:
1: 82275a5448 = 1: 5bb58da117 meson: support building fuzzers with libFuzzer
2: 8b234b5dc6 = 2: 8d11b15082 oss-fuzz: add fuzzer for parsing reftables
3: f265bcf6f4 = 3: 21186da3f1 reftable/basics: fix OOB read on binary search of empty range
4: a56c6cb50c = 4: 3c327bacc2 reftable/record: don't abort when decoding invalid ref value type
-: ---------- > 5: 4125c76a97 t/unit-tests: introduce test helper to write reftable blocks
5: 9074372e30 ! 6: e923c23518 reftable/block: fix OOB write with bogus inflated log size
@@ reftable/block.c: int reftable_block_init(struct reftable_block *block,
## t/unit-tests/u-reftable-block.c ##
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__iterator(void)
- block_writer_release(&writer);
+ reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_log_block_size(void)
+{
+ struct reftable_block_source source = { 0 };
-+ struct block_writer writer = {
-+ .last_key = REFTABLE_BUF_INIT,
-+ };
+ struct reftable_record rec = {
+ .type = REFTABLE_BLOCK_TYPE_LOG,
+ .u.log = {
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__iterator(void)
+ },
+ };
+ struct reftable_block block = { 0 };
-+ struct reftable_buf data;
-+
-+ data.len = 1024;
-+ REFTABLE_CALLOC_ARRAY(data.buf, data.len);
-+ cl_assert(data.buf != NULL);
++ struct reftable_buf data = REFTABLE_BUF_INIT;
+
-+ cl_must_pass(block_writer_init(&writer, REFTABLE_BLOCK_TYPE_LOG,
-+ (uint8_t *) data.buf, data.len,
-+ 0, hash_size(REFTABLE_HASH_SHA1)));
-+ cl_must_pass(block_writer_add(&writer, &rec));
-+ cl_assert(block_writer_finish(&writer) > 0);
++ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_LOG, &rec, 1);
+
+ /*
+ * Log blocks store their inflated size as a big-endian 24-bit integer
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__iterator(void)
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_block_release(&block);
-+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
6: 6877f58485 ! 7: 16c2904a96 reftable/block: fix OOB read with bogus block size
@@ reftable/block.c: int reftable_block_init(struct reftable_block *block,
## t/unit-tests/u-reftable-block.c ##
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_log_block_size(void)
- block_writer_release(&writer);
+ reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_block_size(void)
+{
+ struct reftable_block_source source = { 0 };
-+ struct block_writer writer = {
-+ .last_key = REFTABLE_BUF_INIT,
-+ };
+ struct reftable_record rec = {
+ .type = REFTABLE_BLOCK_TYPE_REF,
+ .u.ref = {
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_log_block_siz
+ },
+ };
+ struct reftable_block block = { 0 };
-+ struct reftable_buf data;
-+
-+ data.len = 1024;
-+ REFTABLE_CALLOC_ARRAY(data.buf, data.len);
-+ cl_assert(data.buf != NULL);
++ struct reftable_buf data = REFTABLE_BUF_INIT;
+
-+ cl_must_pass(block_writer_init(&writer, REFTABLE_BLOCK_TYPE_REF,
-+ (uint8_t *) data.buf, data.len,
-+ 0, hash_size(REFTABLE_HASH_SHA1)));
-+ cl_must_pass(block_writer_add(&writer, &rec));
-+ cl_assert(block_writer_finish(&writer) > 0);
++ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ /*
+ * The block size is stored as a big-endian 24-bit integer right after
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_log_block_siz
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_block_release(&block);
-+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
7: 3c022a4f97 ! 8: 872eca67bb reftable/block: fix OOB read with bogus restart count
@@ reftable/block.c: int reftable_block_init(struct reftable_block *block,
## t/unit-tests/u-reftable-block.c ##
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_block_size(void)
- block_writer_release(&writer);
+ reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_restart_count(void)
+{
+ struct reftable_block_source source = { 0 };
-+ struct block_writer writer = {
-+ .last_key = REFTABLE_BUF_INIT,
-+ };
+ struct reftable_record rec = {
+ .type = REFTABLE_BLOCK_TYPE_REF,
+ .u.ref = {
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_block_size(vo
+ },
+ };
+ struct reftable_block block = { 0 };
-+ struct reftable_buf data;
++ struct reftable_buf data = REFTABLE_BUF_INIT;
+ int block_size;
+
-+ data.len = 1024;
-+ REFTABLE_CALLOC_ARRAY(data.buf, data.len);
-+ cl_assert(data.buf != NULL);
-+
-+ cl_must_pass(block_writer_init(&writer, REFTABLE_BLOCK_TYPE_REF,
-+ (uint8_t *) data.buf, data.len,
-+ 0, hash_size(REFTABLE_HASH_SHA1)));
-+ cl_must_pass(block_writer_add(&writer, &rec));
-+ block_size = block_writer_finish(&writer);
-+ cl_assert(block_size > 0);
++ block_size = cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ /*
+ * Corrupt the restart count to claim a bogus number of restart points.
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_block_size(vo
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_block_release(&block);
-+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
8: af5697b85b = 9: c82d51c163 reftable/block: fix use of uninitialized memory when binsearch fails
9: e9d4eca613 ! 10: 16e1087a66 reftable/block: fix OOB read with bogus restart offset
@@ reftable/block.c: static int restart_needle_less(size_t idx, void *_args)
## t/unit-tests/u-reftable-block.c ##
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_restart_count(void)
- block_writer_release(&writer);
+ reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_restart_offset(void)
+{
+ struct reftable_block_source source = { 0 };
-+ struct block_writer writer = {
-+ .last_key = REFTABLE_BUF_INIT,
-+ };
+ struct reftable_record rec = {
+ .type = REFTABLE_BLOCK_TYPE_REF,
+ .u.ref = {
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_restart_count
+ struct reftable_block block = { 0 };
+ struct block_iter it = BLOCK_ITER_INIT;
+ struct reftable_buf want = REFTABLE_BUF_INIT;
-+ struct reftable_buf data;
-+
-+ data.len = 1024;
-+ REFTABLE_CALLOC_ARRAY(data.buf, data.len);
-+ cl_assert(data.buf != NULL);
++ struct reftable_buf data = REFTABLE_BUF_INIT;
+
-+ cl_must_pass(block_writer_init(&writer, REFTABLE_BLOCK_TYPE_REF,
-+ (uint8_t *) data.buf, data.len,
-+ 0, hash_size(REFTABLE_HASH_SHA1)));
-+ cl_must_pass(block_writer_add(&writer, &rec));
-+ cl_assert(block_writer_finish(&writer) > 0);
++ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ block_source_from_buf(&source, &data);
+ cl_must_pass(reftable_block_init(&block, &source, 0, 0, data.len,
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_restart_count
+ reftable_buf_release(&want);
+ block_iter_close(&it);
+ reftable_block_release(&block);
-+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
10: 4bb729aeb0 = 11: 63dd98f908 reftable/table: fix NULL pointer access when seeking to bogus offsets
11: e3bca6af6e = 12: 32696a01bc reftable/table: fix OOB read on truncated table
---
base-commit: ab776a62a78576513ee121424adb19597fbb7613
change-id: 20260623-pks-reftable-hardening-f54de69fea63
^ permalink raw reply
* Re: [PATCH v4 3/3] replay: offer an option to linearize the commit topology
From: Patrick Steinhardt @ 2026-06-29 8:04 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Elijah Newren, Johannes Schindelin
In-Reply-To: <87qzltyiao.fsf@emacs.iotcl.com>
On Fri, Jun 26, 2026 at 07:36:31AM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > git-rebase(1) essentially knows about three different modes:
> >
> > - "--no-rebase-merges", which is the default and maps to your
> > "--linearize".
> >
> > - "--rebase-merges", which by default doesn't rebase cousins by using
> > "--ancestry-path" internally.
> >
> > - "--rebase-merges=rebase-cousins", which doesn't pass the above
> > option.
> >
> > So it's not a simple boolean there, which makes me wonder whether we
> > should mirror the same interface so that all of git-rebase(1)'s modes
> > can be represented, as well.
>
> That's a valid question, although I don't know a good answer to that.
>
> Basically you're asking for what the command line options will look
> like? Allow me to think out loud.
>
> In this series I'm adding --linearize to git-replay(1). As mentioned, I
> don't think it makes sense to add it to git-history(1) as well. Without
> this option, the process aborts when it encounters a merge.
>
> Dscho sent a patch series to properly replay (2-way) merges. I think
> this should become the default for both git-replay(1) and
> git-history(1).
>
> But then, do we want to have an option that brings back the current
> behavior of aborting at merges? Maybe with --no-merges?
I think that would be a sensible option to have.
> Then there's the option of rebasing cousins left. That's something that
> isn't covered by Dscho's series yet. Maybe --replay-cousins?
>
> To reiterate what the final design could look like:
>
> * <nothing>: replay merges preserving topology.
> * "--linearize": flattens merges (only git-replay(1)).
> * "--no-merges": dies when the process tries to replay a merge.
> * "--replay-cousins": does what --rebase-merges=rebase-cousins does.
Right. And if we tried to be consistent with git-rebase(1), then this
could be done as:
- "--rebase-merges" to replay merges preserving topology, which is the
default once we support replaying them.
- "--no-rebase-merges" to flatten commits.
- "--rebase-merges=abort" to explicitly die when seeing merges.
- "--rebase-merges=rebase-cousins"
> Now, all these options are (I think) mutually exclusive, so we could
> consider an option "--replay-merges=<mode>", but personally I find
> "--<option>=<value>" arguments harder to use than specifying separate
> options.
>
> I think I'm avoiding your question, because the design of the command
> line parameters doesn't need tot 1-on-1 correlate to the internal
> datastructure. And I agree the mode isn't a boolean, but does that mean
> we want to use an enum internally? Well, I don't know. And I also don't
> think that matters right now. Code is easy to change, I think the
> command line options should be designed with the future in mind, which I
> believe we do with "--linearize".
>
> Sorry for this long-winded rambling, but bottom line I think it's fine
> to add --linearize and in the future add more options and see how the
> code should evolve to support those.
Hm, I dunno. You basically reasoned that we potentially want to have all
of the same options that git-rebase(1)'s "--rebase-merges=" already
supports. So that begs the question why we need to reinvent the wheel
then and not just use the same syntax.
Note that I'm not arguing that we should support all of these options
now. I'm merely arguing that we should try to be consistent, unless
there is a good argument not to do that. I'm fine with the interface if
there indeed is a good argument, but if so we should document why we
think that the current interface in git-rebase(1) is not a good fit for
this command.
Thanks!
Patrick
^ permalink raw reply
* [PATCH v7 11/11] builtin/history: implement "drop" subcommand
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
A common operation when editing the commit history is to drop a specific
commit from the history entirely, but this operation is not currently
covered by git-history(1).
A couple of noteworthy bits:
- This is the first git-history(1) command that will ultimately result
in changes to both the index and the working tree. We thus have to
add logic to merge resulting changes into those.
- It is still not possible to replay merge commits, so this limitation
is inherited for the new "drop" command.
- For now we refuse to drop root commits. While we _can_ indeed drop
root commits in the general case, there are edge cases where the
resulting history would become completely empty. This is thus left
to a subsequent patch series.
Other than that, most of the logic is rather straight-forward as we can
continue to build on the preexisting logic in git-history(1) for most of
the part.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-history.adoc | 38 ++-
builtin/history.c | 186 ++++++++++++++
t/meson.build | 1 +
t/t3454-history-drop.sh | 537 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 761 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc
index 2ba8121795..28b477cd37 100644
--- a/Documentation/git-history.adoc
+++ b/Documentation/git-history.adoc
@@ -8,6 +8,7 @@ git-history - EXPERIMENTAL: Rewrite history
SYNOPSIS
--------
[synopsis]
+git history drop <commit> [--dry-run] [--update-refs=(branches|head)] [--empty=(drop|keep|abort)]
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>...]
@@ -51,13 +52,28 @@ be stateful operations. The limitation can be lifted once (if) Git learns about
first-class conflicts.
When using `fixup` with `--empty=drop`, dropping the root commit is not yet
-supported.
+supported. Likewise, `drop` cannot remove the root commit or a merge commit.
COMMANDS
--------
The following commands are available to rewrite history in different ways:
+`drop <commit>`::
+ Remove the specified commit from the history. All descendants of the
+ commit are replayed directly onto its parent.
++
+The root commit cannot be dropped as that may lead to edge cases where refs
+end up with no commits anymore. Merge commits cannot be dropped either; see
+LIMITATIONS.
++
+If `HEAD` points at a commit that is to be rewritten, the index and working
+tree are updated to match the new `HEAD`. The command aborts before any
+references are updated in case local modifications would be overwritten.
++
+If replaying any descendant would result in a conflict, the command aborts
+with an error.
+
`fixup <commit>`::
Apply the currently staged changes to the specified commit. This is
similar in nature to `git commit --fixup=<commit>` followed by `git
@@ -170,6 +186,26 @@ The staged addition of `unrelated.txt` has been incorporated into the `first`
commit. All descendant commits have been replayed on top of the rewritten
history.
+Drop a commit
+~~~~~~~~~~~~~
+
+----------
+$ git log --oneline
+abc1234 (HEAD -> main) third
+def5678 second
+ghi9012 first
+
+$ git history drop 'main^{/second}'
+
+$ git log --oneline
+jkl3456 (HEAD -> main) third
+ghi9012 first
+----------
+
+The `second` commit has been removed from the history, and `third` has been
+replayed directly on top of `first`. All branches that pointed at the dropped
+commit have been moved to its parent.
+
Split a commit
~~~~~~~~~~~~~~
diff --git a/builtin/history.c b/builtin/history.c
index 22b9fcb4a4..7944207f38 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -17,13 +17,17 @@
#include "read-cache.h"
#include "refs.h"
#include "replay.h"
+#include "reset.h"
#include "revision.h"
#include "sequencer.h"
#include "strvec.h"
#include "tree.h"
+#include "tree-walk.h"
#include "unpack-trees.h"
#include "wt-status.h"
+#define GIT_HISTORY_DROP_USAGE \
+ N_("git history drop <commit> [--dry-run] [--update-refs=(branches|head)] [--empty=(drop|keep|abort)]")
#define GIT_HISTORY_FIXUP_USAGE \
N_("git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)]")
#define GIT_HISTORY_REWORD_USAGE \
@@ -999,12 +1003,193 @@ static int cmd_history_split(int argc,
return ret;
}
+static int update_worktree(struct repository *repo,
+ const struct commit *old_head,
+ const struct commit *new_head,
+ bool dry_run)
+{
+ struct reset_working_tree_options opts = {
+ .oid_from = &old_head->object.oid,
+ .oid = &new_head->object.oid,
+ };
+ if (dry_run)
+ opts.flags |= RESET_WORKING_TREE_DRY_RUN;
+ return reset_working_tree(repo, &opts);
+}
+
+static int find_head_tree_change(struct repository *repo,
+ const struct replay_result *result,
+ struct commit **old_head,
+ struct commit **new_head,
+ bool *changed)
+{
+ const struct replay_ref_update *head_update = NULL;
+ struct commit *old_head_commit, *new_head_commit;
+ struct tree *old_head_tree, *new_head_tree;
+ const char *head_target;
+ int head_flags;
+
+ *changed = false;
+
+ head_target = refs_resolve_ref_unsafe(get_main_ref_store(repo),
+ "HEAD", RESOLVE_REF_NO_RECURSE,
+ NULL, &head_flags);
+ if (!head_target)
+ return error(_("cannot look up HEAD"));
+ if (!(head_flags & REF_ISSYMREF))
+ head_target = "HEAD";
+
+ for (size_t i = 0; i < result->updates_nr; i++) {
+ if (!strcmp(result->updates[i].refname, head_target)) {
+ head_update = &result->updates[i];
+ break;
+ }
+ }
+
+ if (!head_update)
+ return 0;
+
+ old_head_commit = lookup_commit_reference(repo, &head_update->old_oid);
+ new_head_commit = lookup_commit_reference(repo, &head_update->new_oid);
+ if (!old_head_commit || !new_head_commit)
+ return error(_("cannot resolve HEAD commit"));
+
+ old_head_tree = repo_get_commit_tree(repo, old_head_commit);
+ new_head_tree = repo_get_commit_tree(repo, new_head_commit);
+ if (!old_head_tree || !new_head_tree)
+ return error(_("cannot resolve tree for HEAD"));
+
+ if (oideq(&old_head_tree->object.oid, &new_head_tree->object.oid))
+ return 0;
+
+ *old_head = old_head_commit;
+ *new_head = new_head_commit;
+ *changed = true;
+
+ return 0;
+}
+
+static int cmd_history_drop(int argc,
+ const char **argv,
+ const char *prefix,
+ struct repository *repo)
+{
+ const char * const usage[] = {
+ GIT_HISTORY_DROP_USAGE,
+ NULL,
+ };
+ enum replay_empty_commit_action empty = REPLAY_EMPTY_COMMIT_DROP;
+ enum ref_action action = REF_ACTION_DEFAULT;
+ 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_CALLBACK_F(0, "empty", &empty, "(drop|keep|abort)",
+ N_("how to handle descendants that become empty"),
+ PARSE_OPT_NONEG, parse_opt_empty),
+ OPT_END(),
+ };
+ struct strbuf reflog_msg = STRBUF_INIT;
+ struct commit *original, *rewritten;
+ struct rev_info revs = { 0 };
+ struct replay_result result = { 0 };
+ struct commit *old_head, *new_head;
+ bool head_moves = false;
+ int ret;
+
+ argc = parse_options(argc, argv, prefix, options, usage, 0);
+ if (argc != 1) {
+ ret = error(_("command expects a single revision"));
+ goto out;
+ }
+ repo_config(repo, git_default_config, NULL);
+
+ if (action == REF_ACTION_DEFAULT)
+ action = REF_ACTION_BRANCHES;
+
+ original = lookup_commit_reference_by_name(argv[0]);
+ if (!original) {
+ ret = error(_("commit cannot be found: %s"), argv[0]);
+ goto out;
+ }
+
+ if (!original->parents) {
+ ret = error(_("cannot drop root commit %s: "
+ "it has no parent to replay onto"),
+ argv[0]);
+ goto out;
+ } else if (original->parents->next) {
+ ret = error(_("cannot drop merge commit: %s"), argv[0]);
+ goto out;
+ }
+
+ ret = setup_revwalk(repo, action, original, &revs);
+ if (ret)
+ goto out;
+
+ rewritten = original->parents->item;
+
+ ret = compute_pending_ref_updates(&revs, action, original, rewritten,
+ empty, &result);
+ if (ret) {
+ ret = error(_("failed replaying descendants"));
+ goto out;
+ }
+
+ /*
+ * If HEAD will move as a result of the rewrite then we'll have to
+ * merge in the changes into the worktree and index. This merge can of
+ * course conflict, which will cause the whole operation to abort.
+ *
+ * If we had already updated the refs at that point then we'd have an
+ * inconsistent repository state. So we first perform a dry-run merge
+ * here before updating refs.
+ */
+ if (!is_bare_repository()) {
+ ret = find_head_tree_change(repo, &result, &old_head,
+ &new_head, &head_moves);
+ if (ret < 0)
+ goto out;
+
+ if (head_moves && update_worktree(repo, old_head, new_head, true) < 0) {
+ ret = error(_("dropping this commit would "
+ "overwrite local changes; aborting"));
+ goto out;
+ }
+ }
+
+ strbuf_addf(&reflog_msg, "drop: dropping %s", argv[0]);
+ ret = apply_pending_ref_updates(repo, &result, reflog_msg.buf, dry_run);
+ if (ret < 0) {
+ ret = error(_("failed to update references"));
+ goto out;
+ }
+
+ if (!dry_run && head_moves && update_worktree(repo, old_head, new_head, false) < 0) {
+ ret = error(_("could not update working tree to new commit %s"),
+ oid_to_hex(&new_head->object.oid));
+ goto out;
+ }
+
+ ret = 0;
+
+out:
+ replay_result_release(&result);
+ strbuf_release(&reflog_msg);
+ release_revisions(&revs);
+ return ret;
+}
+
int cmd_history(int argc,
const char **argv,
const char *prefix,
struct repository *repo)
{
const char * const usage[] = {
+ GIT_HISTORY_DROP_USAGE,
GIT_HISTORY_FIXUP_USAGE,
GIT_HISTORY_REWORD_USAGE,
GIT_HISTORY_SPLIT_USAGE,
@@ -1012,6 +1197,7 @@ int cmd_history(int argc,
};
parse_opt_subcommand_fn *fn = NULL;
struct option options[] = {
+ OPT_SUBCOMMAND("drop", &fn, cmd_history_drop),
OPT_SUBCOMMAND("fixup", &fn, cmd_history_fixup),
OPT_SUBCOMMAND("reword", &fn, cmd_history_reword),
OPT_SUBCOMMAND("split", &fn, cmd_history_split),
diff --git a/t/meson.build b/t/meson.build
index 2af8d01279..d5e71056b2 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -399,6 +399,7 @@ integration_tests = [
't3451-history-reword.sh',
't3452-history-split.sh',
't3453-history-fixup.sh',
+ 't3454-history-drop.sh',
't3500-cherry.sh',
't3501-revert-cherry-pick.sh',
't3502-cherry-pick-merge.sh',
diff --git a/t/t3454-history-drop.sh b/t/t3454-history-drop.sh
new file mode 100755
index 0000000000..0f33247212
--- /dev/null
+++ b/t/t3454-history-drop.sh
@@ -0,0 +1,537 @@
+#!/bin/sh
+
+test_description='tests for git-history drop subcommand'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-log-graph.sh"
+
+expect_graph () {
+ cat >expect &&
+ lib_test_cmp_graph --format=%s "$@"
+}
+
+expect_log () {
+ git log --format="%s" "$@" >actual &&
+ cat >expect &&
+ test_cmp expect actual
+}
+
+test_expect_success 'errors on missing commit argument' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_must_fail git history drop 2>err &&
+ test_grep "command expects a single revision" err
+ )
+'
+
+test_expect_success 'errors on too many arguments' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_must_fail git history drop HEAD HEAD 2>err &&
+ test_grep "command expects a single revision" err
+ )
+'
+
+test_expect_success 'errors on unknown revision' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_must_fail git history drop does-not-exist 2>err &&
+ test_grep "commit cannot be found: does-not-exist" err
+ )
+'
+
+test_expect_success 'errors with invalid --empty= value' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_commit second &&
+ test_must_fail git history drop --empty=bogus HEAD 2>err &&
+ test_grep "unrecognized.*--empty.*bogus" err
+ )
+'
+
+test_expect_success 'drops a commit in the middle and replays descendants' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+
+ git symbolic-ref HEAD >expect &&
+ git history drop HEAD~ &&
+ git symbolic-ref HEAD >actual &&
+ test_cmp expect actual &&
+
+ expect_log <<-\EOF &&
+ third
+ first
+ EOF
+
+ test_must_fail git show HEAD:second.t &&
+ test_path_is_missing second.t &&
+
+ git reflog >reflog &&
+ test_grep "drop: dropping HEAD~" reflog
+ )
+'
+
+test_expect_success 'drops the HEAD commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+
+ git history drop HEAD &&
+
+ expect_log <<-\EOF
+ first
+ EOF
+ )
+'
+
+test_expect_success 'drops a commit on detached HEAD' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+ git checkout --detach HEAD &&
+
+ git history drop HEAD~ &&
+
+ expect_log <<-\EOF
+ third
+ first
+ EOF
+ )
+'
+
+# Note: in this case it would actually be fine to drop the root commit, as we
+# do have a descendant commit, and no reference points to the root commit
+# directly. So this is something that we may relax eventually.
+test_expect_success 'refuses to drop the root commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+
+ test_must_fail git history drop HEAD~ 2>err &&
+ test_grep "cannot drop root commit" err
+ )
+'
+
+# In contrast to the above case, we actually don't want to drop the root commit
+# here as that would cause us to end up with an empty commit graph.
+test_expect_success 'refuses to drop the root commit when branch becomes empty' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+
+ test_must_fail git history drop HEAD 2>err &&
+ test_grep "cannot drop root commit" err
+ )
+'
+
+test_expect_success 'refuses to drop a merge commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ git branch branch &&
+ test_commit ours &&
+ git switch branch &&
+ test_commit theirs &&
+ git switch - &&
+ git merge theirs &&
+
+ test_must_fail git history drop HEAD 2>err &&
+ test_grep "cannot drop merge commit" err
+ )
+'
+
+test_expect_success 'refuses when descendants contain a merge commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit middle &&
+ git branch branch &&
+ test_commit ours &&
+ git switch branch &&
+ test_commit theirs &&
+ git switch - &&
+ git merge theirs &&
+
+ test_must_fail git history drop middle 2>err &&
+ test_grep "replaying merge commits is not supported yet" err
+ )
+'
+
+test_expect_success 'works in a bare repository' '
+ test_when_finished "rm -rf repo repo.git" &&
+
+ git init repo &&
+ test_commit -C repo first &&
+ test_commit -C repo second &&
+ test_commit -C repo third &&
+
+ git clone --bare repo repo.git &&
+ (
+ cd repo.git &&
+
+ git history drop HEAD~ &&
+ expect_log <<-\EOF
+ third
+ first
+ EOF
+ )
+'
+
+test_expect_success 'updates branches on other lines of descent' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit target &&
+ git branch theirs &&
+ test_commit ours &&
+ git switch theirs &&
+ test_commit theirs &&
+
+ expect_graph --branches <<-\EOF &&
+ * theirs
+ | * ours
+ |/
+ * target
+ * base
+ EOF
+
+ git history drop target &&
+
+ expect_graph --branches <<-\EOF
+ * ours
+ | * theirs
+ |/
+ * base
+ EOF
+ )
+'
+
+test_expect_success 'moves branch pointing at dropped commit to its parent' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ git branch points-at-second &&
+ test_commit third &&
+
+ git rev-parse first >expect &&
+ git history drop second &&
+ git rev-parse points-at-second >actual &&
+ test_cmp expect actual &&
+
+ expect_log --format="%s %D" --branches <<-\EOF
+ third HEAD -> main
+ first tag: first, points-at-second
+ EOF
+ )
+'
+
+test_expect_success '--dry-run prints ref updates without modifying repo' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit base &&
+ git branch branch &&
+ test_commit middle &&
+ test_commit ours &&
+ git switch branch &&
+ test_commit theirs &&
+
+ git refs list >refs-expect &&
+ git history drop --dry-run main~ >updates &&
+ git refs list >refs-actual &&
+ test_cmp refs-expect refs-actual &&
+ test_grep "update refs/heads/main" updates &&
+
+ git update-ref --stdin <updates &&
+ expect_log main <<-\EOF
+ ours
+ base
+ EOF
+ )
+'
+
+test_expect_success '--dry-run detects conflicts with modified working tree' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second modify-me &&
+ echo modified >modify-me &&
+
+ git refs list >refs-expect &&
+ git diff >diff-expect &&
+ test_must_fail git history drop --dry-run HEAD 2>err &&
+ test_grep "dropping this commit would overwrite local changes" err &&
+ git diff >diff-actual &&
+ git refs list >refs-actual &&
+
+ test_cmp diff-expect diff-actual &&
+ test_cmp refs-expect refs-actual
+ )
+'
+
+test_expect_success '--update-refs=head updates only HEAD' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit target &&
+ git branch theirs &&
+ test_commit ours &&
+ git switch theirs &&
+ test_commit theirs &&
+
+ # When told to update HEAD only, the command refuses to
+ # rewrite commits that are not an ancestor of HEAD.
+ test_must_fail git history drop --update-refs=head main 2>err &&
+ test_grep "rewritten commit must be an ancestor of HEAD" err &&
+
+ expect_graph --branches <<-\EOF &&
+ * theirs
+ | * ours
+ |/
+ * target
+ * base
+ EOF
+
+ git switch main &&
+ git history drop --update-refs=head target &&
+
+ expect_graph --branches <<-\EOF
+ * ours
+ | * theirs
+ | * target
+ |/
+ * base
+ EOF
+ )
+'
+
+test_expect_success 'conflict with replayed commit aborts cleanly' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit conflict-a file &&
+ test_commit conflict-b file &&
+
+ git refs list >refs-expect &&
+ test_must_fail git history drop HEAD~ 2>err &&
+ test_grep "failed replaying descendants" err &&
+ git refs list >refs-actual &&
+ test_cmp refs-expect refs-actual
+ )
+'
+
+# Build a history where a descendant of the drop target reverts the change
+# introduced by the drop target. After dropping, the descendant's diff applies
+# against a tree that already lacks the change, so it becomes empty.
+setup_empty_descendant_repo () {
+ git init "$1" &&
+ (
+ cd "$1" &&
+ echo C1 >file &&
+ git add file &&
+ git commit -m "base" &&
+ git tag base &&
+ echo C2 >file &&
+ git add file &&
+ git commit -m "drop-me" &&
+ git tag drop-me &&
+ test_commit middle &&
+ echo C1 >file &&
+ git add file &&
+ git commit -m "revert-drop-me" &&
+ git tag revert-drop-me
+ )
+}
+
+test_expect_success '--empty=drop drops descendants that become empty' '
+ test_when_finished "rm -rf repo" &&
+ setup_empty_descendant_repo repo &&
+ (
+ cd repo &&
+
+ git history drop --empty=drop drop-me &&
+
+ expect_log <<-\EOF
+ middle
+ base
+ EOF
+ )
+'
+
+test_expect_success '--empty=keep keeps descendants that become empty' '
+ test_when_finished "rm -rf repo" &&
+ setup_empty_descendant_repo repo &&
+ (
+ cd repo &&
+
+ git history drop --empty=keep drop-me &&
+
+ expect_log <<-\EOF &&
+ revert-drop-me
+ middle
+ base
+ EOF
+ git diff HEAD~ HEAD >diff &&
+ test_must_be_empty diff
+ )
+'
+
+test_expect_success '--empty=abort errors out when a descendant becomes empty' '
+ test_when_finished "rm -rf repo" &&
+ setup_empty_descendant_repo repo &&
+ (
+ cd repo &&
+
+ test_must_fail git history drop --empty=abort drop-me 2>err &&
+ test_grep "became empty after replay" err
+ )
+'
+
+test_expect_success 'updates index and worktree when HEAD moves' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+
+ git history drop second &&
+
+ # Worktree should no longer contain second.t.
+ test_path_is_missing second.t &&
+ test_path_is_file first.t &&
+ test_path_is_file third.t &&
+
+ # Index and worktree should both match the new HEAD.
+ git status --porcelain --untracked-files=no >status &&
+ test_must_be_empty status
+ )
+'
+
+test_expect_success 'updates worktree when dropping HEAD itself' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+
+ git history drop HEAD &&
+
+ test_path_is_missing second.t &&
+ test_path_is_file first.t &&
+
+ git status --porcelain --untracked-files=no >status &&
+ test_must_be_empty status
+ )
+'
+
+test_expect_success 'preserves unrelated unstaged modifications' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ echo first-content >unrelated.txt &&
+ git add unrelated.txt &&
+ git commit -m "add unrelated" &&
+ test_commit second &&
+ test_commit third &&
+
+ echo locally-modified >unrelated.txt &&
+
+ git diff >diff-expect &&
+ git history drop second &&
+ git diff >diff-actual &&
+ test_cmp diff-expect diff-actual &&
+ test_path_is_missing second.t
+ )
+'
+
+test_expect_success 'preserves unrelated staged changes' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ echo first-content >unrelated.txt &&
+ git add unrelated.txt &&
+ git commit -m "add unrelated" &&
+ test_commit second &&
+ test_commit third &&
+
+ echo staged-change >unrelated.txt &&
+ git add unrelated.txt &&
+
+ git diff --cached >diff-expect &&
+ git history drop second &&
+ git diff --cached >diff-actual &&
+ test_cmp diff-expect diff-actual &&
+ test_path_is_missing second.t
+ )
+'
+
+test_expect_success 'aborts when local modifications would be overwritten' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit conflict &&
+
+ echo local-edit >conflict.t &&
+ git diff >diff-expect &&
+ test_must_fail git history drop HEAD 2>err &&
+ test_grep "would overwrite local changes" err &&
+ git diff >diff-actual &&
+ test_cmp diff-expect diff-actual
+ )
+'
+
+test_done
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 10/11] builtin/history: split handling of ref updates into two phases
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
The function `handle_reference_updates()` is used by git-history(1) to
update all references that refer to commits that have been rewritten. As
such, it performs two steps:
- It gathers the references that need to be updated in the first
place.
- It prepares and commits the reference transaction.
In a subsequent commit we'll want to handle those two steps separately.
Prepare for this by splitting up the function into two.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/history.c | 100 +++++++++++++++++++++++++++++++++---------------------
1 file changed, 62 insertions(+), 38 deletions(-)
diff --git a/builtin/history.c b/builtin/history.c
index 0fc06fb204..22b9fcb4a4 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -333,21 +333,17 @@ static int handle_ref_update(struct ref_transaction *transaction,
NULL, NULL, 0, reflog_msg, err);
}
-static int handle_reference_updates(struct rev_info *revs,
- enum ref_action action,
- struct commit *original,
- struct commit *rewritten,
- const char *reflog_msg,
- int dry_run,
- enum replay_empty_commit_action empty)
+static int compute_pending_ref_updates(struct rev_info *revs,
+ enum ref_action action,
+ struct commit *original,
+ struct commit *rewritten,
+ enum replay_empty_commit_action empty,
+ struct replay_result *result)
{
const struct name_decoration *decoration;
struct replay_revisions_options opts = {
.empty = empty,
};
- struct replay_result result = { 0 };
- struct ref_transaction *transaction = NULL;
- struct strbuf err = STRBUF_INIT;
char hex[GIT_MAX_HEXSZ + 1];
bool detached_head;
int head_flags = 0;
@@ -359,34 +355,13 @@ static int handle_reference_updates(struct rev_info *revs,
opts.onto = oid_to_hex_r(hex, &rewritten->object.oid);
- ret = replay_revisions(revs, &opts, &result);
+ ret = replay_revisions(revs, &opts, result);
if (ret)
- goto out;
+ return ret;
if (action != REF_ACTION_BRANCHES && action != REF_ACTION_HEAD)
BUG("unsupported ref action %d", action);
- if (!dry_run) {
- transaction = ref_store_transaction_begin(get_main_ref_store(revs->repo), 0, &err);
- if (!transaction) {
- ret = error(_("failed to begin ref transaction: %s"), err.buf);
- goto out;
- }
- }
-
- for (size_t i = 0; i < result.updates_nr; i++) {
- ret = handle_ref_update(transaction,
- result.updates[i].refname,
- &result.updates[i].new_oid,
- &result.updates[i].old_oid,
- reflog_msg, &err);
- if (ret) {
- ret = error(_("failed to update ref '%s': %s"),
- result.updates[i].refname, err.buf);
- goto out;
- }
- }
-
/*
* `replay_revisions()` only updates references that are
* ancestors of `rewritten`, so we need to manually
@@ -414,14 +389,41 @@ static int handle_reference_updates(struct rev_info *revs,
!detached_head)
continue;
+ replay_result_queue_update(result, decoration->name,
+ &original->object.oid,
+ &rewritten->object.oid);
+ }
+
+ return 0;
+}
+
+static int apply_pending_ref_updates(struct repository *repo,
+ const struct replay_result *result,
+ const char *reflog_msg,
+ int dry_run)
+{
+ struct ref_transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;
+ int ret;
+
+ if (!dry_run) {
+ transaction = ref_store_transaction_begin(get_main_ref_store(repo),
+ 0, &err);
+ if (!transaction) {
+ ret = error(_("failed to begin ref transaction: %s"), err.buf);
+ goto out;
+ }
+ }
+
+ for (size_t i = 0; i < result->updates_nr; i++) {
ret = handle_ref_update(transaction,
- decoration->name,
- &rewritten->object.oid,
- &original->object.oid,
+ result->updates[i].refname,
+ &result->updates[i].new_oid,
+ &result->updates[i].old_oid,
reflog_msg, &err);
if (ret) {
ret = error(_("failed to update ref '%s': %s"),
- decoration->name, err.buf);
+ result->updates[i].refname, err.buf);
goto out;
}
}
@@ -435,11 +437,33 @@ static int handle_reference_updates(struct rev_info *revs,
out:
ref_transaction_free(transaction);
- replay_result_release(&result);
strbuf_release(&err);
return ret;
}
+static int handle_reference_updates(struct rev_info *revs,
+ enum ref_action action,
+ struct commit *original,
+ struct commit *rewritten,
+ const char *reflog_msg,
+ int dry_run,
+ enum replay_empty_commit_action empty)
+{
+ struct replay_result result = { 0 };
+ int ret;
+
+ ret = compute_pending_ref_updates(revs, action, original, rewritten,
+ empty, &result);
+ if (ret)
+ goto out;
+
+ ret = apply_pending_ref_updates(revs->repo, &result, reflog_msg, dry_run);
+
+out:
+ replay_result_release(&result);
+ return ret;
+}
+
static int commit_became_empty(struct repository *repo,
struct commit *original,
struct tree *result)
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 09/11] replay: expose `replay_result_queue_update()`
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
Expose `replay_result_queue_update()`, which is used to append another
reference update to the replay result. This function will be used in a
subsequent commit.
Suggested-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
replay.c | 8 ++++----
replay.h | 5 +++++
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/replay.c b/replay.c
index 4ef8abb607..7c8433107b 100644
--- a/replay.c
+++ b/replay.c
@@ -351,10 +351,10 @@ void replay_result_release(struct replay_result *result)
free(result->updates);
}
-static void replay_result_queue_update(struct replay_result *result,
- const char *refname,
- const struct object_id *old_oid,
- const struct object_id *new_oid)
+void replay_result_queue_update(struct replay_result *result,
+ const char *refname,
+ const struct object_id *old_oid,
+ const struct object_id *new_oid)
{
ALLOC_GROW(result->updates, result->updates_nr + 1, result->updates_alloc);
result->updates[result->updates_nr].refname = xstrdup(refname);
diff --git a/replay.h b/replay.h
index 1851a07705..da83b65345 100644
--- a/replay.h
+++ b/replay.h
@@ -80,6 +80,11 @@ struct replay_result {
void replay_result_release(struct replay_result *result);
+void replay_result_queue_update(struct replay_result *result,
+ const char *refname,
+ const struct object_id *old_oid,
+ const struct object_id *new_oid);
+
/*
* Replay a set of commits onto a new location. Leaves both the working tree,
* index and references untouched. Reference updates caused by the replay will
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 08/11] reset: stop assuming that the caller passes in a clean index
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
In 652bd0211d (rebase: use 'skip_cache_tree_update' option, 2022-11-10),
we updated `reset_working_tree()` to stop updating the index tree cache.
This was done as a performance optimization: the function is only called
by "sequencer.c" and "rebase.c", both of which assume a clean index
before they perform their operation, so we know that the end result will
be a clean index, too. Consequently, we can skip recomputing the cache
as we can instead use `prime_cache_tree()` directly.
In a subsequent commit we're about to add a new caller though where the
assumption doesn't hold anymore: the index may be dirty before calling
`reset_working_tree()`, and consequently we cannot prime the cache with
a given tree anymore as the index and tree will mismatch.
Adapt the logic so that we only skip the cache tree update in case we're
doing a hard reset. While we could introduce logic that only skips the
update in case the incoming index was dirty already, that doesn't really
feel worth it: after all, the mentioned commit says itself that the
performance improvement was negligible anyway.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reset.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/reset.c b/reset.c
index 06f375f296..ff87e3e357 100644
--- a/reset.c
+++ b/reset.c
@@ -167,10 +167,11 @@ int reset_working_tree(struct repository *r,
unpack_tree_opts.dry_run = dry_run;
unpack_tree_opts.merge = 1;
unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
- unpack_tree_opts.skip_cache_tree_update = 1;
init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
- if (reset_hard)
+ if (reset_hard) {
+ unpack_tree_opts.skip_cache_tree_update = 1;
unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
+ }
if (!reset_hard && !fill_tree_descriptor(r, &desc[nr++], &head_oid)) {
ret = error(_("failed to find tree of %s"),
@@ -197,7 +198,8 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
- prime_cache_tree(r, r->index, tree);
+ if (reset_hard)
+ prime_cache_tree(r, r->index, tree);
if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {
ret = error(_("could not write index"));
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 07/11] reset: allow the caller to specify the current HEAD object
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
When calling `reset_working_tree()` we automatically derive the commit
that the callers wants to move from by reading the HEAD commit. Some
callers may already have resolved it, or they may want to move from a
different commit that doesn't match HEAD.
Introduce a new `oid_from` option that lets the caller specify the
commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reset.c | 5 ++++-
reset.h | 5 +++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/reset.c b/reset.c
index 4bde5d8dc6..06f375f296 100644
--- a/reset.c
+++ b/reset.c
@@ -122,7 +122,10 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
- if (!repo_get_oid(r, "HEAD", &head_oid)) {
+ if (opts->oid_from) {
+ oidcpy(&head_oid, opts->oid_from);
+ head = &head_oid;
+ } else if (!repo_get_oid(r, "HEAD", &head_oid)) {
head = &head_oid;
} else if (!oid || !reset_hard) {
ret = error(_("could not determine HEAD revision"));
diff --git a/reset.h b/reset.h
index 38b2891b53..4c992ba671 100644
--- a/reset.h
+++ b/reset.h
@@ -37,6 +37,11 @@ struct reset_working_tree_options {
* The commit to checkout/reset to. Defaults to HEAD.
*/
const struct object_id *oid;
+ /*
+ * The commit to checkout/reset from when doing a two-way merge. This
+ * is used as one of the sides to merge.
+ */
+ const struct object_id *oid_from;
/*
* Optional value to set ORIG_HEAD. Defaults to HEAD.
*/
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 06/11] reset: introduce ability to skip updating HEAD
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
In a subsequent commit we'll introduce a new caller to
`reset_working_tree()` that really only wants to update the index and
working tree, without updating any references. Introduce a new flag that
makes the caller opt in to updating HEAD and adapt all callers to set
that flag.
Note that in a previous iteration we instead introduced a flag that made
callers opt out of updating any references. This was somewhat awkward
though because we already have the `UPDATE_ORIG_HEAD` flag, so the
result was somewhat inconsistent.
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/rebase.c | 14 ++++++++++----
reset.c | 9 +++++++--
reset.h | 9 ++++++---
sequencer.c | 4 +++-
4 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 06dcbaf5e8..10a306310c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -607,7 +607,8 @@ static int move_to_original_branch(struct rebase_options *opts)
strbuf_addf(&head_reflog, "%s (finish): returning to %s",
opts->reflog_action, opts->head_name);
ropts.branch = opts->head_name;
- ropts.flags = RESET_WORKING_TREE_REFS_ONLY;
+ ropts.flags = RESET_WORKING_TREE_REFS_ONLY |
+ RESET_WORKING_TREE_UPDATE_HEAD;
ropts.branch_msg = branch_reflog.buf;
ropts.head_msg = head_reflog.buf;
ret = reset_working_tree(the_repository, &ropts);
@@ -693,6 +694,7 @@ static int run_am(struct rebase_options *opts)
ropts.oid = &opts->orig_head->object.oid;
ropts.branch = opts->head_name;
ropts.default_reflog_action = opts->reflog_action;
+ ropts.flags = RESET_WORKING_TREE_UPDATE_HEAD;
reset_working_tree(the_repository, &ropts);
error(_("\ngit encountered an error while preparing the "
"patches to replay\n"
@@ -862,7 +864,8 @@ static int checkout_up_to_date(struct rebase_options *options)
options->reflog_action, options->switch_to);
ropts.oid = &options->orig_head->object.oid;
ropts.branch = options->head_name;
- ropts.flags = RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
+ ropts.flags = RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK |
+ RESET_WORKING_TREE_UPDATE_HEAD;
if (!ropts.branch)
ropts.flags |= RESET_WORKING_TREE_DETACH;
ropts.head_msg = buf.buf;
@@ -1384,7 +1387,8 @@ int cmd_rebase(int argc,
rerere_clear(the_repository, &merge_rr);
string_list_clear(&merge_rr, 1);
- ropts.flags = RESET_WORKING_TREE_HARD;
+ ropts.flags = RESET_WORKING_TREE_HARD |
+ RESET_WORKING_TREE_UPDATE_HEAD;
if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not discard worktree changes"));
remove_branch_state(the_repository, 0);
@@ -1409,7 +1413,8 @@ int cmd_rebase(int argc,
ropts.oid = &options.orig_head->object.oid;
ropts.head_msg = head_msg.buf;
ropts.branch = options.head_name;
- ropts.flags = RESET_WORKING_TREE_HARD;
+ ropts.flags = RESET_WORKING_TREE_HARD |
+ RESET_WORKING_TREE_UPDATE_HEAD;
if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not move back to %s"),
oid_to_hex(&options.orig_head->object.oid));
@@ -1877,6 +1882,7 @@ int cmd_rebase(int argc,
ropts.oid = &options.onto->object.oid;
ropts.orig_head = &options.orig_head->object.oid;
ropts.flags = RESET_WORKING_TREE_DETACH |
+ RESET_WORKING_TREE_UPDATE_HEAD |
RESET_WORKING_TREE_UPDATE_ORIG_HEAD |
RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
ropts.head_msg = msg.buf;
diff --git a/reset.c b/reset.c
index 99f2c1b012..4bde5d8dc6 100644
--- a/reset.c
+++ b/reset.c
@@ -92,6 +92,7 @@ int reset_working_tree(struct repository *r,
const char *switch_to_branch = opts->branch;
unsigned reset_hard = opts->flags & RESET_WORKING_TREE_HARD;
unsigned refs_only = opts->flags & RESET_WORKING_TREE_REFS_ONLY;
+ unsigned update_head = opts->flags & RESET_WORKING_TREE_UPDATE_HEAD;
unsigned update_orig_head = opts->flags & RESET_WORKING_TREE_UPDATE_ORIG_HEAD;
unsigned dry_run = opts->flags & RESET_WORKING_TREE_DRY_RUN;
struct object_id *head = NULL, head_oid;
@@ -113,6 +114,9 @@ int reset_working_tree(struct repository *r,
if (opts->branch_msg && !opts->branch)
BUG("branch reflog message given without a branch");
+ if (update_orig_head && !update_head)
+ BUG("cannot update ORIG_HEAD without updating HEAD" );
+
if (!refs_only && !dry_run && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
ret = -1;
goto leave_reset_head;
@@ -129,7 +133,7 @@ int reset_working_tree(struct repository *r,
oid = &head_oid;
if (refs_only) {
- if (!dry_run)
+ if (!dry_run && update_head)
return update_refs(r, opts, oid, head);
return 0;
}
@@ -197,7 +201,8 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
- if (oid != &head_oid || update_orig_head || switch_to_branch)
+ if (update_head &&
+ (oid != &head_oid || update_orig_head || switch_to_branch))
ret = update_refs(r, opts, oid, head);
leave_reset_head:
diff --git a/reset.h b/reset.h
index 898e4a1e95..38b2891b53 100644
--- a/reset.h
+++ b/reset.h
@@ -19,14 +19,17 @@ enum reset_working_tree_flags {
/* Only update refs, do not touch the worktree */
RESET_WORKING_TREE_REFS_ONLY = (1 << 3),
- /* Update ORIG_HEAD as well as HEAD */
- RESET_WORKING_TREE_UPDATE_ORIG_HEAD = (1 << 4),
+ /* Update HEAD */
+ RESET_WORKING_TREE_UPDATE_HEAD = (1 << 4),
+
+ /* Update ORIG_HEAD */
+ RESET_WORKING_TREE_UPDATE_ORIG_HEAD = (1 << 5),
/*
* Perform a dry-run by performing the operation without updating
* any user-visible state.
*/
- RESET_WORKING_TREE_DRY_RUN = (1 << 5),
+ RESET_WORKING_TREE_DRY_RUN = (1 << 6),
};
struct reset_working_tree_options {
diff --git a/sequencer.c b/sequencer.c
index 4efe831178..e905b1b2d9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4678,7 +4678,8 @@ static void create_autostash_internal(struct repository *r,
has_uncommitted_changes(r, 1)) {
struct child_process stash = CHILD_PROCESS_INIT;
struct reset_working_tree_options ropts = {
- .flags = RESET_WORKING_TREE_HARD,
+ .flags = RESET_WORKING_TREE_HARD |
+ RESET_WORKING_TREE_UPDATE_HEAD,
};
struct object_id oid;
@@ -4873,6 +4874,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
.oid = onto,
.orig_head = orig_head,
.flags = RESET_WORKING_TREE_DETACH |
+ RESET_WORKING_TREE_UPDATE_HEAD |
RESET_WORKING_TREE_UPDATE_ORIG_HEAD |
RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK,
.head_msg = reflog_message(opts, "start", "checkout %s",
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 05/11] reset: introduce dry-run mode
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
In a subsequent commit we'll add another caller to `reset_working_tree()`
that wants to perform a dry-run check of whether it would be possible to
update the index and working tree when moving to a new commit. Introduce
a new flag that lets the caller perform this operation.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reset.c | 44 +++++++++++++++++++++++++++++++++-----------
reset.h | 6 ++++++
2 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/reset.c b/reset.c
index 4ca7f23a25..99f2c1b012 100644
--- a/reset.c
+++ b/reset.c
@@ -93,11 +93,14 @@ int reset_working_tree(struct repository *r,
unsigned reset_hard = opts->flags & RESET_WORKING_TREE_HARD;
unsigned refs_only = opts->flags & RESET_WORKING_TREE_REFS_ONLY;
unsigned update_orig_head = opts->flags & RESET_WORKING_TREE_UPDATE_ORIG_HEAD;
+ unsigned dry_run = opts->flags & RESET_WORKING_TREE_DRY_RUN;
struct object_id *head = NULL, head_oid;
struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
struct unpack_trees_options unpack_tree_opts = { 0 };
struct tree *tree;
+ struct index_state scratch_index = INDEX_STATE_INIT(r);
+ struct index_state *istate;
const char *action;
int ret = 0, nr = 0;
@@ -110,7 +113,7 @@ int reset_working_tree(struct repository *r,
if (opts->branch_msg && !opts->branch)
BUG("branch reflog message given without a branch");
- if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
+ if (!refs_only && !dry_run && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
ret = -1;
goto leave_reset_head;
}
@@ -125,16 +128,36 @@ int reset_working_tree(struct repository *r,
if (!oid)
oid = &head_oid;
- if (refs_only)
- return update_refs(r, opts, oid, head);
+ if (refs_only) {
+ if (!dry_run)
+ return update_refs(r, opts, oid, head);
+ return 0;
+ }
+
+ if (dry_run) {
+ if (read_index_from(&scratch_index, r->index_file, r->gitdir) < 0 ||
+ index_state_unmerged_to_stage0(&scratch_index) < 0) {
+ ret = error(_("could not read index"));
+ goto leave_reset_head;
+ }
+
+ istate = &scratch_index;
+ } else {
+ if (repo_read_index_unmerged(r) < 0) {
+ ret = error(_("could not read index"));
+ goto leave_reset_head;
+ }
+ istate = r->index;
+ }
action = reset_hard ? "reset" : "checkout";
setup_unpack_trees_porcelain(&unpack_tree_opts, action);
unpack_tree_opts.head_idx = 1;
- unpack_tree_opts.src_index = r->index;
- unpack_tree_opts.dst_index = r->index;
+ unpack_tree_opts.src_index = istate;
+ unpack_tree_opts.dst_index = istate;
unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
- unpack_tree_opts.update = 1;
+ unpack_tree_opts.update = !dry_run;
+ unpack_tree_opts.dry_run = dry_run;
unpack_tree_opts.merge = 1;
unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
unpack_tree_opts.skip_cache_tree_update = 1;
@@ -142,11 +165,6 @@ int reset_working_tree(struct repository *r,
if (reset_hard)
unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
- if (repo_read_index_unmerged(r) < 0) {
- ret = error(_("could not read index"));
- goto leave_reset_head;
- }
-
if (!reset_hard && !fill_tree_descriptor(r, &desc[nr++], &head_oid)) {
ret = error(_("failed to find tree of %s"),
oid_to_hex(&head_oid));
@@ -163,6 +181,9 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
+ if (dry_run)
+ goto leave_reset_head;
+
tree = repo_parse_tree_indirect(r, oid);
if (!tree) {
ret = error(_("unable to read tree (%s)"), oid_to_hex(oid));
@@ -182,6 +203,7 @@ int reset_working_tree(struct repository *r,
leave_reset_head:
rollback_lock_file(&lock);
clear_unpack_trees_porcelain(&unpack_tree_opts);
+ release_index(&scratch_index);
while (nr)
free((void *)desc[--nr].buffer);
return ret;
diff --git a/reset.h b/reset.h
index 2e5826de99..898e4a1e95 100644
--- a/reset.h
+++ b/reset.h
@@ -21,6 +21,12 @@ enum reset_working_tree_flags {
/* Update ORIG_HEAD as well as HEAD */
RESET_WORKING_TREE_UPDATE_ORIG_HEAD = (1 << 4),
+
+ /*
+ * Perform a dry-run by performing the operation without updating
+ * any user-visible state.
+ */
+ RESET_WORKING_TREE_DRY_RUN = (1 << 5),
};
struct reset_working_tree_options {
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 04/11] reset: modernize flags passed to `reset_working_tree()`
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
The flags passed to `reset_working_tree()` are declared as defines. This
has fallen a bit out of practice nowadays, where we instead prefer to
use enums. Furthermore, the prefix of those flags does not match the
function name anymore after the rename in the preceding commit.
Adapt the code to follow modern best practices and adapt the flag names.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/rebase.c | 15 ++++++++-------
reset.c | 12 ++++++------
reset.h | 31 +++++++++++++++++++------------
sequencer.c | 9 ++++++---
4 files changed, 39 insertions(+), 28 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 22fbba3c62..06dcbaf5e8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -607,7 +607,7 @@ static int move_to_original_branch(struct rebase_options *opts)
strbuf_addf(&head_reflog, "%s (finish): returning to %s",
opts->reflog_action, opts->head_name);
ropts.branch = opts->head_name;
- ropts.flags = RESET_HEAD_REFS_ONLY;
+ ropts.flags = RESET_WORKING_TREE_REFS_ONLY;
ropts.branch_msg = branch_reflog.buf;
ropts.head_msg = head_reflog.buf;
ret = reset_working_tree(the_repository, &ropts);
@@ -862,9 +862,9 @@ static int checkout_up_to_date(struct rebase_options *options)
options->reflog_action, options->switch_to);
ropts.oid = &options->orig_head->object.oid;
ropts.branch = options->head_name;
- ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+ ropts.flags = RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
if (!ropts.branch)
- ropts.flags |= RESET_HEAD_DETACH;
+ ropts.flags |= RESET_WORKING_TREE_DETACH;
ropts.head_msg = buf.buf;
if (reset_working_tree(the_repository, &ropts) < 0)
ret = error(_("could not switch to %s"), options->switch_to);
@@ -1384,7 +1384,7 @@ int cmd_rebase(int argc,
rerere_clear(the_repository, &merge_rr);
string_list_clear(&merge_rr, 1);
- ropts.flags = RESET_HEAD_HARD;
+ ropts.flags = RESET_WORKING_TREE_HARD;
if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not discard worktree changes"));
remove_branch_state(the_repository, 0);
@@ -1409,7 +1409,7 @@ int cmd_rebase(int argc,
ropts.oid = &options.orig_head->object.oid;
ropts.head_msg = head_msg.buf;
ropts.branch = options.head_name;
- ropts.flags = RESET_HEAD_HARD;
+ ropts.flags = RESET_WORKING_TREE_HARD;
if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not move back to %s"),
oid_to_hex(&options.orig_head->object.oid));
@@ -1876,8 +1876,9 @@ int cmd_rebase(int argc,
options.reflog_action, options.onto_name);
ropts.oid = &options.onto->object.oid;
ropts.orig_head = &options.orig_head->object.oid;
- ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
- RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+ ropts.flags = RESET_WORKING_TREE_DETACH |
+ RESET_WORKING_TREE_UPDATE_ORIG_HEAD |
+ RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
ropts.head_msg = msg.buf;
ropts.default_reflog_action = options.reflog_action;
if (reset_working_tree(the_repository, &ropts)) {
diff --git a/reset.c b/reset.c
index 799596398b..4ca7f23a25 100644
--- a/reset.c
+++ b/reset.c
@@ -16,9 +16,9 @@ static int update_refs(struct repository *repo,
const struct object_id *oid,
const struct object_id *head)
{
- unsigned detach_head = opts->flags & RESET_HEAD_DETACH;
- unsigned run_hook = opts->flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
- unsigned update_orig_head = opts->flags & RESET_ORIG_HEAD;
+ unsigned detach_head = opts->flags & RESET_WORKING_TREE_DETACH;
+ unsigned run_hook = opts->flags & RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
+ unsigned update_orig_head = opts->flags & RESET_WORKING_TREE_UPDATE_ORIG_HEAD;
const struct object_id *orig_head = opts->orig_head;
const char *switch_to_branch = opts->branch;
const char *reflog_branch = opts->branch_msg;
@@ -90,9 +90,9 @@ int reset_working_tree(struct repository *r,
{
const struct object_id *oid = opts->oid;
const char *switch_to_branch = opts->branch;
- unsigned reset_hard = opts->flags & RESET_HEAD_HARD;
- unsigned refs_only = opts->flags & RESET_HEAD_REFS_ONLY;
- unsigned update_orig_head = opts->flags & RESET_ORIG_HEAD;
+ unsigned reset_hard = opts->flags & RESET_WORKING_TREE_HARD;
+ unsigned refs_only = opts->flags & RESET_WORKING_TREE_REFS_ONLY;
+ unsigned update_orig_head = opts->flags & RESET_WORKING_TREE_UPDATE_ORIG_HEAD;
struct object_id *head = NULL, head_oid;
struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
diff --git a/reset.h b/reset.h
index f130152014..2e5826de99 100644
--- a/reset.h
+++ b/reset.h
@@ -6,16 +6,22 @@
#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
-/* Request a detached checkout */
-#define RESET_HEAD_DETACH (1<<0)
-/* Request a reset rather than a checkout */
-#define RESET_HEAD_HARD (1<<1)
-/* Run the post-checkout hook */
-#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
-/* Only update refs, do not touch the worktree */
-#define RESET_HEAD_REFS_ONLY (1<<3)
-/* Update ORIG_HEAD as well as HEAD */
-#define RESET_ORIG_HEAD (1<<4)
+enum reset_working_tree_flags {
+ /* Request a detached checkout */
+ RESET_WORKING_TREE_DETACH = (1 << 0),
+
+ /* Request a reset rather than a checkout */
+ RESET_WORKING_TREE_HARD = (1 << 1),
+
+ /* Run the post-checkout hook */
+ RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK = (1 << 2),
+
+ /* Only update refs, do not touch the worktree */
+ RESET_WORKING_TREE_REFS_ONLY = (1 << 3),
+
+ /* Update ORIG_HEAD as well as HEAD */
+ RESET_WORKING_TREE_UPDATE_ORIG_HEAD = (1 << 4),
+};
struct reset_working_tree_options {
/*
@@ -33,7 +39,7 @@ struct reset_working_tree_options {
/*
* Flags defined above.
*/
- unsigned flags;
+ enum reset_working_tree_flags flags;
/*
* Optional reflog message for branch, defaults to head_msg.
*/
@@ -45,7 +51,8 @@ struct reset_working_tree_options {
const char *head_msg;
/*
* Optional reflog message for ORIG_HEAD, if this omitted and flags
- * contains RESET_ORIG_HEAD then default_reflog_action must be given.
+ * contains RESET_WORKING_TREE_UPDATE_ORIG_HEAD then
+ * default_reflog_action must be given.
*/
const char *orig_head_msg;
/*
diff --git a/sequencer.c b/sequencer.c
index d73ecf0384..4efe831178 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4677,7 +4677,9 @@ static void create_autostash_internal(struct repository *r,
if (has_unstaged_changes(r, 1) ||
has_uncommitted_changes(r, 1)) {
struct child_process stash = CHILD_PROCESS_INIT;
- struct reset_working_tree_options ropts = { .flags = RESET_HEAD_HARD };
+ struct reset_working_tree_options ropts = {
+ .flags = RESET_WORKING_TREE_HARD,
+ };
struct object_id oid;
strvec_pushl(&stash.args,
@@ -4870,8 +4872,9 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
struct reset_working_tree_options ropts = {
.oid = onto,
.orig_head = orig_head,
- .flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
- RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
+ .flags = RESET_WORKING_TREE_DETACH |
+ RESET_WORKING_TREE_UPDATE_ORIG_HEAD |
+ RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK,
.head_msg = reflog_message(opts, "start", "checkout %s",
onto_name),
.default_reflog_action = sequencer_reflog_action(opts)
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 03/11] reset: rename `reset_head()`
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
In a subsequent commit we're about to adapt `reset_head()` so that the
reference update to HEAD is optional, only. At this point the function
starts to feel misnamed, as it doesn't necessarily have anything to do
with the HEAD reference anymore. The gist of the function then is that
we reset the working tree to a specific new commit, updating both the
index and the checked-out files.
Rename it to `reset_working_tree()` to better reflect that.
Note that we don't adjust the flags yet. This will happen in a
subsequent commit.
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/rebase.c | 20 ++++++++++----------
reset.c | 5 +++--
reset.h | 4 ++--
sequencer.c | 8 ++++----
4 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index fa4f5d9306..22fbba3c62 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -592,7 +592,7 @@ static int finish_rebase(struct rebase_options *opts)
static int move_to_original_branch(struct rebase_options *opts)
{
struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
- struct reset_head_opts ropts = { 0 };
+ struct reset_working_tree_options ropts = { 0 };
int ret;
if (!opts->head_name)
@@ -610,7 +610,7 @@ static int move_to_original_branch(struct rebase_options *opts)
ropts.flags = RESET_HEAD_REFS_ONLY;
ropts.branch_msg = branch_reflog.buf;
ropts.head_msg = head_reflog.buf;
- ret = reset_head(the_repository, &ropts);
+ ret = reset_working_tree(the_repository, &ropts);
strbuf_release(&branch_reflog);
strbuf_release(&head_reflog);
@@ -685,7 +685,7 @@ static int run_am(struct rebase_options *opts)
status = run_command(&format_patch);
if (status) {
- struct reset_head_opts ropts = { 0 };
+ struct reset_working_tree_options ropts = { 0 };
unlink(rebased_patches);
free(rebased_patches);
child_process_clear(&am);
@@ -693,7 +693,7 @@ static int run_am(struct rebase_options *opts)
ropts.oid = &opts->orig_head->object.oid;
ropts.branch = opts->head_name;
ropts.default_reflog_action = opts->reflog_action;
- reset_head(the_repository, &ropts);
+ reset_working_tree(the_repository, &ropts);
error(_("\ngit encountered an error while preparing the "
"patches to replay\n"
"these revisions:\n"
@@ -855,7 +855,7 @@ static int rebase_config(const char *var, const char *value,
static int checkout_up_to_date(struct rebase_options *options)
{
struct strbuf buf = STRBUF_INIT;
- struct reset_head_opts ropts = { 0 };
+ struct reset_working_tree_options ropts = { 0 };
int ret = 0;
strbuf_addf(&buf, "%s: checkout %s",
@@ -866,7 +866,7 @@ static int checkout_up_to_date(struct rebase_options *options)
if (!ropts.branch)
ropts.flags |= RESET_HEAD_DETACH;
ropts.head_msg = buf.buf;
- if (reset_head(the_repository, &ropts) < 0)
+ if (reset_working_tree(the_repository, &ropts) < 0)
ret = error(_("could not switch to %s"), options->switch_to);
strbuf_release(&buf);
@@ -1116,7 +1116,7 @@ int cmd_rebase(int argc,
int reschedule_failed_exec = -1;
int allow_preemptive_ff = 1;
int preserve_merges_selected = 0;
- struct reset_head_opts ropts = { 0 };
+ struct reset_working_tree_options ropts = { 0 };
struct option builtin_rebase_options[] = {
OPT_STRING(0, "onto", &options.onto_name,
N_("revision"),
@@ -1385,7 +1385,7 @@ int cmd_rebase(int argc,
rerere_clear(the_repository, &merge_rr);
string_list_clear(&merge_rr, 1);
ropts.flags = RESET_HEAD_HARD;
- if (reset_head(the_repository, &ropts) < 0)
+ if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not discard worktree changes"));
remove_branch_state(the_repository, 0);
if (read_basic_state(&options))
@@ -1410,7 +1410,7 @@ int cmd_rebase(int argc,
ropts.head_msg = head_msg.buf;
ropts.branch = options.head_name;
ropts.flags = RESET_HEAD_HARD;
- if (reset_head(the_repository, &ropts) < 0)
+ if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not move back to %s"),
oid_to_hex(&options.orig_head->object.oid));
strbuf_release(&head_msg);
@@ -1880,7 +1880,7 @@ int cmd_rebase(int argc,
RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
ropts.head_msg = msg.buf;
ropts.default_reflog_action = options.reflog_action;
- if (reset_head(the_repository, &ropts)) {
+ if (reset_working_tree(the_repository, &ropts)) {
ret = error(_("Could not detach HEAD"));
goto cleanup_autostash;
}
diff --git a/reset.c b/reset.c
index 3b3cb74dab..799596398b 100644
--- a/reset.c
+++ b/reset.c
@@ -12,7 +12,7 @@
#include "hook.h"
static int update_refs(struct repository *repo,
- const struct reset_head_opts *opts,
+ const struct reset_working_tree_options *opts,
const struct object_id *oid,
const struct object_id *head)
{
@@ -85,7 +85,8 @@ static int update_refs(struct repository *repo,
return ret;
}
-int reset_head(struct repository *r, const struct reset_head_opts *opts)
+int reset_working_tree(struct repository *r,
+ const struct reset_working_tree_options *opts)
{
const struct object_id *oid = opts->oid;
const char *switch_to_branch = opts->branch;
diff --git a/reset.h b/reset.h
index a28f81829d..f130152014 100644
--- a/reset.h
+++ b/reset.h
@@ -17,7 +17,7 @@
/* Update ORIG_HEAD as well as HEAD */
#define RESET_ORIG_HEAD (1<<4)
-struct reset_head_opts {
+struct reset_working_tree_options {
/*
* The commit to checkout/reset to. Defaults to HEAD.
*/
@@ -55,6 +55,6 @@ struct reset_head_opts {
const char *default_reflog_action;
};
-int reset_head(struct repository *r, const struct reset_head_opts *opts);
+int reset_working_tree(struct repository *r, const struct reset_working_tree_options *opts);
#endif
diff --git a/sequencer.c b/sequencer.c
index 1ee4b2875b..d73ecf0384 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4677,7 +4677,7 @@ static void create_autostash_internal(struct repository *r,
if (has_unstaged_changes(r, 1) ||
has_uncommitted_changes(r, 1)) {
struct child_process stash = CHILD_PROCESS_INIT;
- struct reset_head_opts ropts = { .flags = RESET_HEAD_HARD };
+ struct reset_working_tree_options ropts = { .flags = RESET_HEAD_HARD };
struct object_id oid;
strvec_pushl(&stash.args,
@@ -4707,7 +4707,7 @@ static void create_autostash_internal(struct repository *r,
if (!silent)
printf(_("Created autostash: %s\n"), buf.buf);
- if (reset_head(r, &ropts) < 0)
+ if (reset_working_tree(r, &ropts) < 0)
die(_("could not reset --hard"));
discard_index(r->index);
if (repo_read_index(r) < 0)
@@ -4867,7 +4867,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
const char *onto_name, const struct object_id *onto,
const struct object_id *orig_head)
{
- struct reset_head_opts ropts = {
+ struct reset_working_tree_options ropts = {
.oid = onto,
.orig_head = orig_head,
.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
@@ -4876,7 +4876,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
onto_name),
.default_reflog_action = sequencer_reflog_action(opts)
};
- if (reset_head(r, &ropts)) {
+ if (reset_working_tree(r, &ropts)) {
apply_autostash(rebase_path_autostash());
sequencer_remove_state(opts);
return error(_("could not detach HEAD"));
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 02/11] reset: drop `USE_THE_REPOSITORY_VARIABLE`
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
In "reset.c" we still have references to `the_repository`, even though
the only entry point into the file already receives a repository as
parameter.
Update all uses of `the_repository` to instead use the passed-in repo
and drop `USE_THE_REPOSITORY_VARIABLE`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reset.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/reset.c b/reset.c
index 46e30e6394..3b3cb74dab 100644
--- a/reset.c
+++ b/reset.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "git-compat-util.h"
#include "cache-tree.h"
#include "gettext.h"
@@ -13,7 +11,8 @@
#include "unpack-trees.h"
#include "hook.h"
-static int update_refs(const struct reset_head_opts *opts,
+static int update_refs(struct repository *repo,
+ const struct reset_head_opts *opts,
const struct object_id *oid,
const struct object_id *head)
{
@@ -42,19 +41,19 @@ static int update_refs(const struct reset_head_opts *opts,
prefix_len = msg.len;
if (update_orig_head) {
- if (!repo_get_oid(the_repository, "ORIG_HEAD", &oid_old_orig))
+ if (!repo_get_oid(repo, "ORIG_HEAD", &oid_old_orig))
old_orig = &oid_old_orig;
if (head) {
if (!reflog_orig_head) {
strbuf_addstr(&msg, "updating ORIG_HEAD");
reflog_orig_head = msg.buf;
}
- refs_update_ref(get_main_ref_store(the_repository),
+ refs_update_ref(get_main_ref_store(repo),
reflog_orig_head, "ORIG_HEAD",
orig_head ? orig_head : head,
old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
} else if (old_orig)
- refs_delete_ref(get_main_ref_store(the_repository),
+ refs_delete_ref(get_main_ref_store(repo),
NULL, "ORIG_HEAD", old_orig, 0);
}
@@ -64,23 +63,23 @@ static int update_refs(const struct reset_head_opts *opts,
reflog_head = msg.buf;
}
if (!switch_to_branch)
- ret = refs_update_ref(get_main_ref_store(the_repository),
+ ret = refs_update_ref(get_main_ref_store(repo),
reflog_head, "HEAD", oid, head,
detach_head ? REF_NO_DEREF : 0,
UPDATE_REFS_MSG_ON_ERR);
else {
- ret = refs_update_ref(get_main_ref_store(the_repository),
+ ret = refs_update_ref(get_main_ref_store(repo),
reflog_branch ? reflog_branch : reflog_head,
switch_to_branch, oid, NULL, 0,
UPDATE_REFS_MSG_ON_ERR);
if (!ret)
- ret = refs_update_symref(get_main_ref_store(the_repository),
+ ret = refs_update_symref(get_main_ref_store(repo),
"HEAD", switch_to_branch,
reflog_head);
}
if (!ret && run_hook)
- run_hooks_l(the_repository, "post-checkout",
- oid_to_hex(head ? head : null_oid(the_hash_algo)),
+ run_hooks_l(repo, "post-checkout",
+ oid_to_hex(head ? head : null_oid(repo->hash_algo)),
oid_to_hex(oid), "1", NULL);
strbuf_release(&msg);
return ret;
@@ -126,7 +125,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
oid = &head_oid;
if (refs_only)
- return update_refs(opts, oid, head);
+ return update_refs(r, opts, oid, head);
action = reset_hard ? "reset" : "checkout";
setup_unpack_trees_porcelain(&unpack_tree_opts, action);
@@ -163,7 +162,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
goto leave_reset_head;
}
- tree = repo_parse_tree_indirect(the_repository, oid);
+ tree = repo_parse_tree_indirect(r, oid);
if (!tree) {
ret = error(_("unable to read tree (%s)"), oid_to_hex(oid));
goto leave_reset_head;
@@ -177,7 +176,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
}
if (oid != &head_oid || update_orig_head || switch_to_branch)
- ret = update_refs(opts, oid, head);
+ ret = update_refs(r, opts, oid, head);
leave_reset_head:
rollback_lock_file(&lock);
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 01/11] read-cache: split out function to drop unmerged entries to stage 0
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im>
In `repo_read_index_unmerged()` we read the index and then drop any
unmerged entries to stage 0. In a subsequent commit we'll want to
perform this operation on arbitrary indexes, not only the one of the
given repository.
Prepare for this by splitting out the functionality into a new function
that can act on an arbitrary index.
While at it, fix a signedness mismatch when iterating through the index
cache entries.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
read-cache-ll.h | 1 +
read-cache.c | 12 +++++++-----
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/read-cache-ll.h b/read-cache-ll.h
index 2c8b4b21b1..71b87615eb 100644
--- a/read-cache-ll.h
+++ b/read-cache-ll.h
@@ -309,6 +309,7 @@ int write_locked_index(struct index_state *, struct lock_file *lock, unsigned fl
void discard_index(struct index_state *);
void move_index_extensions(struct index_state *dst, struct index_state *src);
int unmerged_index(const struct index_state *);
+int index_state_unmerged_to_stage0(struct index_state *istate);
/**
* Returns 1 if istate differs from tree, 0 otherwise. If tree is NULL,
diff --git a/read-cache.c b/read-cache.c
index 21829102ae..799a5bc719 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3403,13 +3403,15 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
*/
int repo_read_index_unmerged(struct repository *repo)
{
- struct index_state *istate;
- int i;
+ repo_read_index(repo);
+ return index_state_unmerged_to_stage0(repo->index);
+}
+
+int index_state_unmerged_to_stage0(struct index_state *istate)
+{
int unmerged = 0;
- repo_read_index(repo);
- istate = repo->index;
- for (i = 0; i < istate->cache_nr; i++) {
+ for (unsigned int i = 0; i < istate->cache_nr; i++) {
struct cache_entry *ce = istate->cache[i];
struct cache_entry *new_ce;
int len;
--
2.55.0.rc2.803.g1fd1e6609c.dirty
^ permalink raw reply related
* [PATCH v7 00/11] builtin/history: introduce "drop" subcommand
From: Patrick Steinhardt @ 2026-06-29 7:34 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260601-b4-pks-history-drop-v1-0-643e32340d55@pks.im>
Hi,
this small patch series introduces the new "drop" subcommand for
git-history(1). As a reader might guess, the command does exactly that:
given a commit, it will drop that commit from the commit history and
replay descendant branches on top of it.
Changes in v7:
- Expose `replay_result_queue_update()` so that we don't have to
duplicate its functionality.
- Add missing SOB.
- Link to v6: https://patch.msgid.link/20260615-b4-pks-history-drop-v6-0-2e329e536d78@pks.im
Changes in v6:
- Fix bad interactions of DRY_RUN with UPDATE_HEAD
- Link to v5: https://patch.msgid.link/20260611-b4-pks-history-drop-v5-0-34d35725559c@pks.im
Changes in v5:
- Reject UPDATE_ORIG_HEAD without UPDATE_HEAD.
- Link to v4: https://patch.msgid.link/20260610-b4-pks-history-drop-v4-0-70d5f0ae8c25@pks.im
Changes in v4:
- Remove the `SKIP_REF_UPDATES` flag in favor of a new `UPDATE_HEAD`
flag, as suggested by Phillip.
- Rename `reset_head()` to `reset_working_tree()`. This better matches
the new scope of the function, and it helps us to catch any
in-flight patches that would now have to set the `UPDATE_HEAD` flag.
- Link to v3: https://patch.msgid.link/20260608-b4-pks-history-drop-v3-0-84ca8e43e937@pks.im
Changes in v3:
- Fix commit message typos.
- Make `update_orig_head` and `skip_ref_updates` mutually exclusive.
- Use fancy revisions to specify the commit to drop in the example
section.
- Detect conflicting changes in the index/working tree in dry-run
mode.
- Consistently use a subshell.
- Rename `RESET_HEAD_ORIG_HEAD` to `RESET_HEAD_UPDATE_ORIG_HEAD`.
-
- Link to v2: https://patch.msgid.link/20260603-b4-pks-history-drop-v2-0-742cb5b5176d@pks.im
Changes in v2:
- Reworked `update_worktree()` to use `reset_head()`, which required a
bunch of changes to `reset_head()`.
- Consistently mention the commit that cannot be dropped as part of
error messages.
- Adapt error message to not use backticks anymore.
- Drop redundant "--graph" flag in a test helper.
- Link to v1: https://patch.msgid.link/20260601-b4-pks-history-drop-v1-0-643e32340d55@pks.im
Thanks!
Patrick
---
Patrick Steinhardt (11):
read-cache: split out function to drop unmerged entries to stage 0
reset: drop `USE_THE_REPOSITORY_VARIABLE`
reset: rename `reset_head()`
reset: modernize flags passed to `reset_working_tree()`
reset: introduce dry-run mode
reset: introduce ability to skip updating HEAD
reset: allow the caller to specify the current HEAD object
reset: stop assuming that the caller passes in a clean index
replay: expose `replay_result_queue_update()`
builtin/history: split handling of ref updates into two phases
builtin/history: implement "drop" subcommand
Documentation/git-history.adoc | 38 ++-
builtin/history.c | 286 +++++++++++++++++++---
builtin/rebase.c | 41 ++--
read-cache-ll.h | 1 +
read-cache.c | 12 +-
replay.c | 8 +-
replay.h | 5 +
reset.c | 102 +++++---
reset.h | 51 ++--
sequencer.c | 17 +-
t/meson.build | 1 +
t/t3454-history-drop.sh | 537 +++++++++++++++++++++++++++++++++++++++++
12 files changed, 978 insertions(+), 121 deletions(-)
Range-diff versus v6:
1: 61c6eb1bdc = 1: 640b51b963 read-cache: split out function to drop unmerged entries to stage 0
2: 50a61da426 = 2: 485dee2858 reset: drop `USE_THE_REPOSITORY_VARIABLE`
3: 96ffa9d2a6 ! 3: fd82a0b592 reset: rename `reset_head()`
@@ Commit message
subsequent commit.
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
+ Signed-off-by: Patrick Steinhardt <ps@pks.im>
## builtin/rebase.c ##
@@ builtin/rebase.c: static int finish_rebase(struct rebase_options *opts)
4: 874c3ecd59 = 4: 7fe6db0459 reset: modernize flags passed to `reset_working_tree()`
5: bba2845f2d = 5: b0fba42b75 reset: introduce dry-run mode
6: ada93af1da = 6: db635dbea4 reset: introduce ability to skip updating HEAD
7: 3cf1dcf549 = 7: 5422a99683 reset: allow the caller to specify the current HEAD object
8: ff28ad814c = 8: a264b72376 reset: stop assuming that the caller passes in a clean index
-: ---------- > 9: 8060e462d1 replay: expose `replay_result_queue_update()`
9: 7b048d5a16 ! 10: 7233b48732 builtin/history: split handling of ref updates into two phases
@@ builtin/history.c: static int handle_reference_updates(struct rev_info *revs,
!detached_head)
continue;
-+ ALLOC_GROW(result->updates, result->updates_nr + 1, result->updates_alloc);
-+ result->updates[result->updates_nr].refname = xstrdup(decoration->name);
-+ result->updates[result->updates_nr].old_oid = original->object.oid;
-+ result->updates[result->updates_nr].new_oid = rewritten->object.oid;
-+ result->updates_nr++;
++ replay_result_queue_update(result, decoration->name,
++ &original->object.oid,
++ &rewritten->object.oid);
+ }
+
+ return 0;
10: 7389e0432a = 11: 61668ea59a builtin/history: implement "drop" subcommand
---
base-commit: 1666c1265231b0bc5f613fbbf3f0a9896cdef76e
change-id: 20260601-b4-pks-history-drop-28f6c6399e7b
^ permalink raw reply
* Re: [PATCH v6 09/10] builtin/history: split handling of ref updates into two phases
From: Patrick Steinhardt @ 2026-06-29 7:33 UTC (permalink / raw)
To: Christian Couder
Cc: git, Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk,
Phillip Wood
In-Reply-To: <CAP8UFD1evTZqj1ymW9g5g2RmMkYMaE0rPa0Hzt+irH94M6LD6A@mail.gmail.com>
On Thu, Jun 25, 2026 at 03:37:42PM +0200, Christian Couder wrote:
> On Mon, Jun 15, 2026 at 3:56 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> > @@ -414,14 +389,43 @@ static int handle_reference_updates(struct rev_info *revs,
> > !detached_head)
> > continue;
> >
> > + ALLOC_GROW(result->updates, result->updates_nr + 1, result->updates_alloc);
> > + result->updates[result->updates_nr].refname = xstrdup(decoration->name);
> > + result->updates[result->updates_nr].old_oid = original->object.oid;
> > + result->updates[result->updates_nr].new_oid = rewritten->object.oid;
> > + result->updates_nr++;
>
> It looks like this duplicates what replay_result_queue_update() from
> replay.c does.
It indeed is. That function is internal to "replay.c" though. We could
expose it, but I wonder whether that's worth it. Goes looking... you
know, let me just do it.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH 0/3] fixing expensive http test timeouts
From: Patrick Steinhardt @ 2026-06-29 7:33 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, git, Junio C Hamano
In-Reply-To: <20260628075716.GA3525066@coredump.intra.peff.net>
On Sun, Jun 28, 2026 at 03:57:16AM -0400, Jeff King wrote:
> On Fri, Jun 26, 2026 at 04:26:28PM -0700, Michael Montalbo wrote:
>
> > I think Peff and Patrick's suggestion to just increase the Apache timeout
> > makes sense. I ran some experiments using a really long timeout with an
> > artificially slowed down CI runner and all the jobs made progress
> > (if slowly) without stalling, and eventually completed successfully:
> >
> > https://github.com/mmontalbo/git/actions/runs/28267019651
> >
> > I haven't spent a lot of time trying to figure out what the right timeout
> > value should be. An hour definitely seems like overkill, with something
> > on the order of 5-10 minutes seeming more reasonable, but I don't
> > have a principled number.
>
> Here are some patches to keep things moving along. I arbitrarily picked
> 10 minutes, because multiplying the 1-minute default by 10 felt right. ;)
>
> The first one just bumps the timeout and should make our problems go
> away. The other two are optimizations, but I'm on the fence on whether
> the final patch is worth it.
>
> Thanks again for all of the digging.
>
> [1/3]: t/lib-httpd: bump apache timeout
> [2/3]: t5551: put many-tags case into its own repo
> [3/3]: t5551: pack refs after creating many tags
By the way, the only reason why we at GitLab haven't been feeling the
pain is that we only enable GIT_TEST_LONG for GitHub. So I was wondering
whether we want to have something like the below patch on top.
Patrick
diff --git a/ci/lib.sh b/ci/lib.sh
index b939110a6e..57801586aa 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -215,6 +215,14 @@ then
test macos != "$CI_OS_NAME" || CI_OS_NAME=osx
CI_REPO_SLUG="$GITHUB_REPOSITORY"
CI_JOB_ID="$GITHUB_RUN_ID"
+
+ case "$GITHUB_EVENT_NAME" in
+ pull_request)
+ CI_EVENT=pull_request;;
+ push)
+ CI_EVENT=push;;
+ esac
+
CC="${CC_PACKAGE:-${CC:-gcc}}"
DONT_SKIP_TAGS=t
handle_failed_tests () {
@@ -239,6 +247,13 @@ then
CI_BRANCH="$CI_COMMIT_REF_NAME"
CI_COMMIT="$CI_COMMIT_SHA"
+ case "$CI_PIPELINE_SOURCE" in
+ merge_request_event)
+ CI_EVENT=pull_request;;
+ push)
+ CI_EVENT=push;;
+ esac
+
case "$OS,$CI_JOB_IMAGE" in
Windows_NT,*)
CI_OS_NAME=windows
@@ -319,7 +334,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease
# enable "expensive" tests for PR events.
# In order to catch bugs introduced at integration time by mismerges,
# enable the long tests for pushes to the integration branches as well.
-case "$GITHUB_EVENT_NAME,$CI_BRANCH" in
+case "$CI_EVENT,$CI_BRANCH" in
pull_request,*|push,*next*|push,*master*|push,*main*|push,*maint*)
export GIT_TEST_LONG=YesPlease
;;
^ permalink raw reply related
* receive-pack hangs on zero-object push into promisor-shaped repository
From: Wei Hu @ 2026-06-29 7:10 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 860 bytes --]
Hello,
I found a reproducible hang in `git receive-pack` when pushing a ref update
that sends zero objects into a repository that has promisor remote
configuration and `.promisor` pack sidecar files.
The same zero-object ref update returns normally when the receiving
repository
is a normal non-bare repository or a bare repository. It also returns
normally
if I remove either the promisor remote config or the `.promisor` sidecar
files
from the receiving repository.
Check the attached script to reproduce the bug.
Environment:
git version 2.54.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
rust: disabled
gettext: enabled
libcurl: 8.5.0
zlib: 1.3
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1
OS: Ubuntu 24.04.4 LTS (Noble Numbat)
[-- Attachment #1.2: Type: text/html, Size: 1024 bytes --]
[-- Attachment #2: git-promisor-zero-object-push-repro.sh --]
[-- Type: text/x-sh, Size: 1723 bytes --]
#!/bin/sh
set -eu
GIT=${GIT:-git}
ROOT=$(mktemp -d "${TMPDIR:-/tmp}/git-promisor-push-hang.XXXXXX")
SRC=$ROOT/src
DST=$ROOT/dst
UPSTREAM=$ROOT/upstream.git
TRACE=$ROOT/trace.log
echo "root: $ROOT"
echo "git: $($GIT --version)"
$GIT init -q "$SRC"
$GIT -C "$SRC" config user.name Repro
$GIT -C "$SRC" config user.email repro@example.invalid
printf A >"$SRC/file"
$GIT -C "$SRC" add file
$GIT -C "$SRC" commit -q -m A
$GIT -C "$SRC" branch topic
OLD=$($GIT -C "$SRC" rev-parse topic)
printf B >"$SRC/file"
$GIT -C "$SRC" commit -q -am B
$GIT -C "$SRC" branch -M main
NEW=$($GIT -C "$SRC" rev-parse main)
$GIT clone -q --bare "$SRC" "$UPSTREAM"
$GIT init -q "$DST"
$GIT -C "$DST" config receive.denycurrentbranch updateInstead
$GIT -C "$SRC" push -q "$DST" main:main topic:topic
$GIT -C "$DST" checkout -q main
$GIT -C "$DST" config remote.origin.url "file://$UPSTREAM"
$GIT -C "$DST" config remote.origin.promisor true
$GIT -C "$DST" config remote.origin.partialclonefilter blob:none
$GIT -C "$DST" gc -q
for pack in "$DST"/.git/objects/pack/*.pack
do
: >"${pack%.pack}.promisor"
done
$GIT -C "$DST" update-ref refs/heads/topic "$OLD"
status=0
timeout --kill-after=2s 8 \
env GIT_TRACE=1 GIT_TRACE_PACKET=1 \
$GIT -C "$SRC" push --porcelain "$DST" HEAD:topic \
>"$TRACE" 2>&1 || status=$?
AFTER=$($GIT -C "$DST" rev-parse refs/heads/topic)
ZERO_PACK=no
grep -q -- '--pack_header=2,0' "$TRACE" && ZERO_PACK=yes
echo "old: $OLD"
echo "new: $NEW"
echo "after: $AFTER"
echo "push status: $status"
echo "zero-object pack observed: $ZERO_PACK"
echo "trace: $TRACE"
if test "$status" = 124 && test "$AFTER" = "$OLD" && test "$ZERO_PACK" = yes
then
echo "BUG REPRODUCED"
exit 0
fi
echo "BUG NOT REPRODUCED"
exit 1
^ permalink raw reply
* Re: [PATCH 05/11] reftable/block: fix OOB write with bogus inflated log size
From: Patrick Steinhardt @ 2026-06-29 7:08 UTC (permalink / raw)
To: Christian Couder; +Cc: git, oxsignal
In-Reply-To: <CAP8UFD0y0GVjdnWYDkOsk6R9-ReGfzr6ZEm8PbyHOHrdAETXzg@mail.gmail.com>
On Fri, Jun 26, 2026 at 09:48:36AM +0200, Christian Couder wrote:
> On Wed, Jun 24, 2026 at 10:24 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> > diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> > index f4bded7d26..40274af5c0 100644
> > --- a/t/unit-tests/u-reftable-block.c
> > +++ b/t/unit-tests/u-reftable-block.c
> > @@ -456,3 +456,47 @@ void test_reftable_block__iterator(void)
> > block_writer_release(&writer);
> > reftable_buf_release(&data);
> > }
> > +
> > +void test_reftable_block__corrupt_log_block_size(void)
> > +{
> > + struct reftable_block_source source = { 0 };
> > + struct block_writer writer = {
> > + .last_key = REFTABLE_BUF_INIT,
> > + };
> > + struct reftable_record rec = {
> > + .type = REFTABLE_BLOCK_TYPE_LOG,
> > + .u.log = {
> > + .refname = (char *) "refs/heads/main",
> > + .update_index = 1,
> > + .value_type = REFTABLE_LOG_UPDATE,
> > + },
> > + };
> > + struct reftable_block block = { 0 };
> > + struct reftable_buf data;
> > +
> > + data.len = 1024;
> > + REFTABLE_CALLOC_ARRAY(data.buf, data.len);
> > + cl_assert(data.buf != NULL);
> > +
> > + cl_must_pass(block_writer_init(&writer, REFTABLE_BLOCK_TYPE_LOG,
> > + (uint8_t *) data.buf, data.len,
> > + 0, hash_size(REFTABLE_HASH_SHA1)));
> > + cl_must_pass(block_writer_add(&writer, &rec));
> > + cl_assert(block_writer_finish(&writer) > 0);
>
> It looks like some of the block writing code above could be simplified
> using an helper function like:
>
> int cl_reftable_write_block(struct reftable_buf *buf, uint8_t block_type,
> size_t block_size, uint32_t header_off,
> struct reftable_record *recs, size_t nrecs)
> {
> struct block_writer writer = {
> .last_key = REFTABLE_BUF_INIT,
> };
> int block_end;
>
> REFTABLE_CALLOC_ARRAY(buf->buf, block_size);
> cl_assert(buf->buf != NULL);
> buf->len = block_size;
>
> cl_must_pass(block_writer_init(&writer, block_type, (uint8_t *) buf->buf,
> block_size, header_off,
> hash_size(REFTABLE_HASH_SHA1)));
> for (size_t i = 0; i < nrecs; i++)
> cl_must_pass(block_writer_add(&writer, &recs[i]));
>
> block_end = block_writer_finish(&writer);
> cl_assert(block_end > 0);
>
> block_writer_release(&writer);
>
> return block_end;
> }
>
> This function could be introduced by a preparatory commit in
> t/unit-tests/lib-reftable.{c,h}. It would be kind of similar to the
> existing cl_reftable_write_to_buf() helper in those files.
>
> It looks like it could already simplify existing tests like:
>
> - test_reftable_block__log_read_write
> - test_reftable_block__obj_read_write
> - test_reftable_block__ref_read_write
> - test_reftable_block__iterator
>
> and it could simplify the new tests introduced by other patches in this series:
>
> - 06/11 reftable/block: fix OOB read with bogus block size
> - 07/11 reftable/block: fix OOB read with bogus restart count
> - 09/11 reftable/block: fix OOB read with bogus restart offset
Good point, will do. Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH] history: streamline message preparation and plug file stream leak
From: Patrick Steinhardt @ 2026-06-29 6:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin
In-Reply-To: <xmqqecht8df1.fsf@gitster.g>
On Fri, Jun 26, 2026 at 09:38:42AM -0700, Junio C Hamano wrote:
> diff --git a/builtin/history.c b/builtin/history.c
> index 8dcb9a6046..f17ec049c0 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> @@ -41,11 +41,6 @@ static int fill_commit_message(struct repository *repo,
> " empty message aborts the commit.\n");
> struct wt_status s;
>
> - strbuf_addstr(out, default_message);
> - strbuf_addch(out, '\n');
> - strbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);
> - write_file_buf(path, out->buf, out->len);
> -
> wt_status_prepare(repo, &s);
> FREE_AND_NULL(s.branch);
> s.ahead_behind_flags = AHEAD_BEHIND_QUICK;
> @@ -57,14 +52,20 @@ static int fill_commit_message(struct repository *repo,
> s.whence = FROM_COMMIT;
> s.committable = 1;
>
> - s.fp = fopen(git_path_commit_editmsg(), "a");
Here we reuse the local `path` variable, which already carries the
result of `git_path_commit_editmsg()`.
> + s.fp = fopen(path, "w");
> if (!s.fp)
> - return error_errno(_("could not open '%s'"), git_path_commit_editmsg());
> + return error_errno(_("could not open '%s'"), path);
Likewise.
> + strbuf_addstr(out, default_message);
> + strbuf_addch(out, '\n');
> + strbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);
> + fwrite(out->buf, 1, out->len, s.fp);
>
> wt_status_collect_changes_trees(&s, old_tree, new_tree);
> wt_status_print(&s);
> wt_status_collect_free_buffers(&s);
> string_list_clear_func(&s.change, change_data_free);
> + fclose(s.fp);
This is fixing the leaked file descriptor.
One thing I wonder though is that we don't perform any error checking on
the file in the new version. Previously, we would have died in case
`write_file_buf()` failed. But now we just `fwrite()` without error
checking. I don't think that "wt-status.c" does error checking either,
so we might end up with a partially-written file without us noticing.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v5 0/4] history: add squash subcommand to fold a range
From: Patrick Steinhardt @ 2026-06-29 6:26 UTC (permalink / raw)
To: phillip.wood; +Cc: Harald Nordgren via GitGitGadget, git, Harald Nordgren
In-Reply-To: <d37e8f4f-d1f9-45aa-8c95-ebe676d54671@gmail.com>
On Fri, Jun 26, 2026 at 09:52:57AM +0100, Phillip Wood wrote:
> Hi Harald
>
> On 24/06/2026 22:54, Harald Nordgren via GitGitGadget wrote:
> > Adds git history squash <revision-range> to fold a range of commits.
>
> It would be helpful to give a bit more detail here about the command so that
> the reader has an overview of what is actually being implemented.
>
> - what does it do with fixup!, squash! and amend! commits? Can it use
> the message from amend! commits to reword the commit?
> - can the user reword the commit message?
Good things to document/discuss.
> - what happens if a merge commit inside the range has a parent outside
> the range?
Yeah, I agree that we should punt on merge commits for now. They are a
can of worms, and I'm not sure that we should just squash them. I would
at least like the user to ask a flag that tells us that it's fine
squashing those.
> - what happens to branches that point to commits inside the range?
Yeah, this should be documented indeed.
> I had a quick play and found that it accepts ranges that containing a single
> commit (e.g. @^!) where there is nothing to squash. It also accepts ranges
> that are not ancestors of HEAD (e.g. checkout master and run "git history
> squash --dry-run origin/seen^2^!") without printing an error message. Only
> accepting a single argument is quite limiting as one cannot say
>
> git history squash ^:/base :/tip
Note that it is intentional that you can rewrite branches that are not
currently checked out, and the other subcommands work the same. So I'd
argue this should also be the case for "squash".
Patrick
^ permalink raw reply
* Re: [PATCH] reftable: fix unlikely leak on API error
From: Patrick Steinhardt @ 2026-06-29 6:21 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20260628090314.GA661068@coredump.intra.peff.net>
On Sun, Jun 28, 2026 at 05:03:14AM -0400, Jeff King wrote:
> If the reftable writer sees a bogus block size, we return with
> REFTABLE_API_ERROR, leaking the reftable_writer struct we previously
> allocated. Originally this case was a BUG(), but it became a regular
> return in 445f9f4f35 (reftable: stop using `BUG()` in trivial cases,
> 2025-02-18).
>
> We could obviously fix it by calling "reftable_free(wp)". But we can
> observe that we never use the allocated "wp" until after we've validated
> the input options. So let's just bump the allocation down. That fixes
> the leak, and I think makes the flow of the function more logical
> (we validate our inputs before doing any work).
Another alternative would be to create a common exit path where we free
the structure when we're about to return an error. But that might not
even be worth it.
> diff --git a/reftable/writer.c b/reftable/writer.c
> index 0133b64975..1bd4aa388b 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -152,16 +152,16 @@ int reftable_writer_new(struct reftable_writer **out,
> struct reftable_write_options opts = {0};
> struct reftable_writer *wp;
>
> - wp = reftable_calloc(1, sizeof(*wp));
> - if (!wp)
> - return REFTABLE_OUT_OF_MEMORY_ERROR;
> -
> if (_opts)
> opts = *_opts;
> options_set_defaults(&opts);
> if (opts.block_size >= (1 << 24))
> return REFTABLE_API_ERROR;
>
> + wp = reftable_calloc(1, sizeof(*wp));
> + if (!wp)
> + return REFTABLE_OUT_OF_MEMORY_ERROR;
> +
> reftable_buf_init(&wp->block_writer_data.last_key);
> reftable_buf_init(&wp->last_key);
> reftable_buf_init(&wp->scratch);
Makes sense. There's another early return in this function, but there we
already know to free the writer.
Thanks!
Patrick
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox