* [PATCH 6/7] packfile,delta: drop the `cast_size_t_to_ulong()` wrappers
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 10:51 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2137.git.1780570272.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When I started the transition from `unsigned long` to `size_t`, in the
interest of keeping the patches reviewable, I introduced these calls to
prevent data type narrowing from silently failing to handle large object
sizes. I also introduced `*_sz()` variants that would allow most of the
callers to keep using that `unsigned long` that the 90s kindly asked to
be returned.
After the preceding commits, the only places that called the narrow
wrappers either no longer exist or already use the `_sz` form
internally, so the wrappers just narrow values back through
`cast_size_t_to_ulong()` for no reason.
Drop them and rename the `_sz` variants back to the natural names.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
delta.h | 14 ++------------
packfile.c | 28 ++++++++--------------------
packfile.h | 2 +-
patch-delta.c | 4 ++--
4 files changed, 13 insertions(+), 35 deletions(-)
diff --git a/delta.h b/delta.h
index bb149dc82b..eb5c6d2fdb 100644
--- a/delta.h
+++ b/delta.h
@@ -86,11 +86,8 @@ void *patch_delta(const void *src_buf, size_t src_size,
* This must be called twice on the delta data buffer, first to get the
* expected source buffer size, and again to get the target buffer size.
*/
-/*
- * Size_t variant that doesn't truncate - use for >4GB objects on Windows.
- */
-static inline size_t get_delta_hdr_size_sz(const unsigned char **datap,
- const unsigned char *top)
+static inline size_t get_delta_hdr_size(const unsigned char **datap,
+ const unsigned char *top)
{
const unsigned char *data = *datap;
size_t cmd, size = 0;
@@ -104,11 +101,4 @@ static inline size_t get_delta_hdr_size_sz(const unsigned char **datap,
return size;
}
-static inline unsigned long get_delta_hdr_size(const unsigned char **datap,
- const unsigned char *top)
-{
- size_t size = get_delta_hdr_size_sz(datap, top);
- return cast_size_t_to_ulong(size);
-}
-
#endif
diff --git a/packfile.c b/packfile.c
index dab0a9b16d..c174982d10 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1164,11 +1164,12 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,
}
/*
- * Size_t variant for >4GB delta results on Windows.
+ * Read a delta object's header at curpos in p (already inflated as needed)
+ * and return the size of the result object (the post-application target).
*/
-static size_t get_size_from_delta_sz(struct packed_git *p,
- struct pack_window **w_curs,
- off_t curpos)
+size_t get_size_from_delta(struct packed_git *p,
+ struct pack_window **w_curs,
+ off_t curpos)
{
const unsigned char *data;
unsigned char delta_head[20], *in;
@@ -1215,18 +1216,10 @@ static size_t get_size_from_delta_sz(struct packed_git *p,
data = delta_head;
/* ignore base size */
- get_delta_hdr_size_sz(&data, delta_head+sizeof(delta_head));
+ get_delta_hdr_size(&data, delta_head+sizeof(delta_head));
/* Read the result size */
- return get_delta_hdr_size_sz(&data, delta_head+sizeof(delta_head));
-}
-
-unsigned long get_size_from_delta(struct packed_git *p,
- struct pack_window **w_curs,
- off_t curpos)
-{
- size_t size = get_size_from_delta_sz(p, w_curs, curpos);
- return cast_size_t_to_ulong(size);
+ return get_delta_hdr_size(&data, delta_head+sizeof(delta_head));
}
int unpack_object_header(struct packed_git *p,
@@ -1634,12 +1627,7 @@ static int packed_object_info_with_index_pos(struct packed_git *p, off_t obj_off
ret = -1;
goto out;
}
- /*
- * Use size_t variant to avoid die() on >4GB deltas.
- * oi->sizep is unsigned long, so truncation may occur,
- * but streaming code uses its own size_t tracking.
- */
- size = get_size_from_delta_sz(p, &w_curs, tmp_pos);
+ size = get_size_from_delta(p, &w_curs, tmp_pos);
if (size == 0) {
ret = -1;
goto out;
diff --git a/packfile.h b/packfile.h
index 0b5ae3f9fc..bd4494906d 100644
--- a/packfile.h
+++ b/packfile.h
@@ -458,7 +458,7 @@ int is_pack_valid(struct packed_git *);
void *unpack_entry(struct repository *r, struct packed_git *, off_t,
enum object_type *, size_t *);
unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, size_t *sizep);
-unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
+size_t get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, size_t *);
off_t get_delta_base(struct packed_git *p, struct pack_window **w_curs,
off_t *curpos, enum object_type type,
diff --git a/patch-delta.c b/patch-delta.c
index 44cda97994..42199fa956 100644
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -27,12 +27,12 @@ void *patch_delta(const void *src_buf, size_t src_size,
top = (const unsigned char *) delta_buf + delta_size;
/* make sure the orig file size matches what we expect */
- size = get_delta_hdr_size_sz(&data, top);
+ size = get_delta_hdr_size(&data, top);
if (size != src_size)
return NULL;
/* now the result size */
- size = get_delta_hdr_size_sz(&data, top);
+ size = get_delta_hdr_size(&data, top);
dst_buf = xmallocz(size);
out = dst_buf;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 5/7] pack-objects: use size_t for in-core object sizes
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 10:51 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2137.git.1780570272.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
`pack-objects` stores per-entry object sizes in either the 31-bit
`size_` member of the `struct object_entry` or, when the value does not
fit, the `pack->delta_size[]` spill array. The accessors (`oe_size`,
`oe_delta_size`, `oe_get_size_slow`, `oe_size_*_than`) and the setters
(`oe_set_size`, `oe_set_delta_size`) used `unsigned long` for the spill
type, which on Windows means the spill silently caps at 4 GiB per entry.
That is what made `upload-pack` die with "object too large to read on
this platform" when serving the >4 GiB blob in `t5608` tests 5 and 6
when run with `GIT_TEST_CLONE_2GB`.
Widen them all to `size_t` (including `pack->delta_size`) and drop the
three `cast_size_t_to_ulong()` calls in `check_object()` that guarded
`in_pack_size`. The two `SET_SIZE(entry, canonical_size)` calls in the
same function stay cast-free as before, since `canonical_size` is still
`unsigned long` until a later commit widens `object_info::sizep`.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pack-objects.c | 35 ++++++++++++++++++-----------------
pack-objects.h | 2 +-
2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 975f04d699..bb372d0b03 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -66,8 +66,8 @@ static inline struct object_entry *oe_delta(
return &pack->objects[e->delta_idx - 1];
}
-static inline unsigned long oe_delta_size(struct packing_data *pack,
- const struct object_entry *e)
+static inline size_t oe_delta_size(struct packing_data *pack,
+ const struct object_entry *e)
{
if (e->delta_size_valid)
return e->delta_size_;
@@ -83,11 +83,11 @@ static inline unsigned long oe_delta_size(struct packing_data *pack,
return pack->delta_size[e - pack->objects];
}
-unsigned long oe_get_size_slow(struct packing_data *pack,
- const struct object_entry *e);
+size_t oe_get_size_slow(struct packing_data *pack,
+ const struct object_entry *e);
-static inline unsigned long oe_size(struct packing_data *pack,
- const struct object_entry *e)
+static inline size_t oe_size(struct packing_data *pack,
+ const struct object_entry *e)
{
if (e->size_valid)
return e->size_;
@@ -145,7 +145,7 @@ static inline void oe_set_delta_sibling(struct packing_data *pack,
static inline void oe_set_size(struct packing_data *pack,
struct object_entry *e,
- unsigned long size)
+ size_t size)
{
if (size < pack->oe_size_limit) {
e->size_ = size;
@@ -159,7 +159,7 @@ static inline void oe_set_size(struct packing_data *pack,
static inline void oe_set_delta_size(struct packing_data *pack,
struct object_entry *e,
- unsigned long size)
+ size_t size)
{
if (size < pack->oe_delta_size_limit) {
e->delta_size_ = size;
@@ -496,7 +496,7 @@ static void copy_pack_data(struct hashfile *f,
static inline int oe_size_greater_than(struct packing_data *pack,
const struct object_entry *lhs,
- unsigned long rhs)
+ size_t rhs)
{
if (lhs->size_valid)
return lhs->size_ > rhs;
@@ -2277,7 +2277,7 @@ static void check_object(struct object_entry *entry, uint32_t object_index)
default:
/* Not a delta hence we've already got all we need. */
oe_set_type(entry, entry->in_pack_type);
- SET_SIZE(entry, cast_size_t_to_ulong(in_pack_size));
+ SET_SIZE(entry, in_pack_size);
entry->in_pack_header_size = used;
if (oe_type(entry) < OBJ_COMMIT || oe_type(entry) > OBJ_BLOB)
goto give_up;
@@ -2331,8 +2331,8 @@ static void check_object(struct object_entry *entry, uint32_t object_index)
if (have_base &&
can_reuse_delta(&base_ref, entry, &base_entry)) {
oe_set_type(entry, entry->in_pack_type);
- SET_SIZE(entry, cast_size_t_to_ulong(in_pack_size)); /* delta size */
- SET_DELTA_SIZE(entry, cast_size_t_to_ulong(in_pack_size));
+ SET_SIZE(entry, in_pack_size); /* delta size */
+ SET_DELTA_SIZE(entry, in_pack_size);
if (base_entry) {
SET_DELTA(entry, base_entry);
@@ -2355,7 +2355,8 @@ static void check_object(struct object_entry *entry, uint32_t object_index)
* object size from the delta header.
*/
delta_pos = entry->in_pack_offset + entry->in_pack_header_size;
- canonical_size = get_size_from_delta(p, &w_curs, delta_pos);
+ canonical_size = get_size_from_delta(p, &w_curs,
+ delta_pos);
if (canonical_size == 0)
goto give_up;
SET_SIZE(entry, canonical_size);
@@ -2711,7 +2712,7 @@ static pthread_mutex_t progress_mutex;
static inline int oe_size_less_than(struct packing_data *pack,
const struct object_entry *lhs,
- unsigned long rhs)
+ size_t rhs)
{
if (lhs->size_valid)
return lhs->size_ < rhs;
@@ -2734,8 +2735,8 @@ static inline void oe_set_tree_depth(struct packing_data *pack,
* reconstruction (so non-deltas are true object sizes, but deltas
* return the size of the delta data).
*/
-unsigned long oe_get_size_slow(struct packing_data *pack,
- const struct object_entry *e)
+size_t oe_get_size_slow(struct packing_data *pack,
+ const struct object_entry *e)
{
struct packed_git *p;
struct pack_window *w_curs;
@@ -2769,7 +2770,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
unuse_pack(&w_curs);
packing_data_unlock(&to_pack);
- return cast_size_t_to_ulong(size);
+ return size;
}
static int try_delta(struct unpacked *trg, struct unpacked *src,
diff --git a/pack-objects.h b/pack-objects.h
index 83299d4732..e97e84ddcb 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -141,7 +141,7 @@ struct packing_data {
uint32_t index_size;
unsigned int *in_pack_pos;
- unsigned long *delta_size;
+ size_t *delta_size;
/*
* Only one of these can be non-NULL and they have different
--
gitgitgadget
^ permalink raw reply related
* [PATCH 4/7] packfile: widen unpack_entry()'s size out-parameter to size_t
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 10:51 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2137.git.1780570272.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The topic `js/objects-larger-than-4gb-on-windows` widened the streaming,
index-pack and unpack-objects paths to `size_t` but deliberately stopped
at the in-memory `unpack_entry()` cascade, which still hands back the
unpacked size through `unsigned long *`. On Windows that boundary
truncates above 4 GiB because that data type is only 32 bits wide on
that platform.
Widen the code path. Except `packed_object_info_with_index_pos()`: It
cannot yet pass `oi->sizep` directly because the field is still
`unsigned long *`; bridge it with a `size_t` temporary that narrows
back, and let a later commit drop the bridge once the field is wide
too. `gfi_unpack_entry()` keeps its narrow signature because fast-import
tracks sizes through `unsigned long` everywhere it crosses subsystem
boundaries, keeping its signature allows the scope of this commit to be
somewhat reasonable, still.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/fast-import.c | 7 ++++++-
pack-check.c | 5 ++---
packfile.c | 28 +++++++++++++++++-----------
packfile.h | 3 ++-
4 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 82bc6dcc00..3dff898c43 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -1239,6 +1239,8 @@ static void *gfi_unpack_entry(
unsigned long *sizep)
{
enum object_type type;
+ size_t size_st = 0;
+ void *data;
struct packed_git *p = all_packs[oe->pack_id];
if (p == pack_data && p->pack_size < (pack_size + the_hash_algo->rawsz)) {
/* The object is stored in the packfile we are writing to
@@ -1260,7 +1262,10 @@ static void *gfi_unpack_entry(
*/
p->pack_size = pack_size + the_hash_algo->rawsz;
}
- return unpack_entry(the_repository, p, oe->idx.offset, &type, sizep);
+ data = unpack_entry(the_repository, p, oe->idx.offset, &type, &size_st);
+ if (sizep)
+ *sizep = cast_size_t_to_ulong(size_st);
+ return data;
}
static void load_tree(struct tree_entry *root)
diff --git a/pack-check.c b/pack-check.c
index 2792f34d25..5adfb3f272 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -143,9 +143,8 @@ static int verify_packfile(struct repository *r,
data = NULL;
data_valid = 0;
} else {
- unsigned long sz;
- data = unpack_entry(r, p, entries[i].offset, &type, &sz);
- size = sz;
+ data = unpack_entry(r, p, entries[i].offset, &type,
+ &size);
data_valid = 1;
}
diff --git a/packfile.c b/packfile.c
index e202f48837..dab0a9b16d 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1454,7 +1454,7 @@ struct delta_base_cache_entry {
struct delta_base_cache_key key;
struct list_head lru;
void *data;
- unsigned long size;
+ size_t size;
enum object_type type;
};
@@ -1525,7 +1525,7 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)
}
static void *cache_or_unpack_entry(struct repository *r, struct packed_git *p,
- off_t base_offset, unsigned long *base_size,
+ off_t base_offset, size_t *base_size,
enum object_type *type)
{
struct delta_base_cache_entry *ent;
@@ -1558,8 +1558,8 @@ void clear_delta_base_cache(void)
}
static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
- void *base, unsigned long base_size,
- unsigned long delta_base_cache_limit,
+ void *base, size_t base_size,
+ size_t delta_base_cache_limit,
enum object_type type)
{
struct delta_base_cache_entry *ent;
@@ -1614,10 +1614,13 @@ static int packed_object_info_with_index_pos(struct packed_git *p, off_t obj_off
* a "real" type later if the caller is interested.
*/
if (oi->contentp) {
- *oi->contentp = cache_or_unpack_entry(p->repo, p, obj_offset, oi->sizep,
- &type);
+ size_t size_st = 0;
+ *oi->contentp = cache_or_unpack_entry(p->repo, p, obj_offset,
+ &size_st, &type);
if (!*oi->contentp)
type = OBJ_BAD;
+ else if (oi->sizep)
+ *oi->sizep = cast_size_t_to_ulong(size_st);
} else if (oi->sizep || oi->typep || oi->delta_base_oid) {
type = unpack_object_header(p, &w_curs, &curpos, &size);
}
@@ -1735,7 +1738,7 @@ int packed_object_info(struct packed_git *p, off_t obj_offset,
static void *unpack_compressed_entry(struct packed_git *p,
struct pack_window **w_curs,
off_t curpos,
- unsigned long size)
+ size_t size)
{
int st;
git_zstream stream;
@@ -1790,11 +1793,11 @@ int do_check_packed_object_crc;
struct unpack_entry_stack_ent {
off_t obj_offset;
off_t curpos;
- unsigned long size;
+ size_t size;
};
void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
- enum object_type *final_type, unsigned long *final_size)
+ enum object_type *final_type, size_t *final_size)
{
struct pack_window *w_curs = NULL;
off_t curpos = obj_offset;
@@ -1911,7 +1914,7 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
void *delta_data;
void *base = data;
void *external_base = NULL;
- unsigned long delta_size, base_size = size;
+ size_t delta_size, base_size = size;
int i;
off_t base_obj_offset = obj_offset;
@@ -1928,6 +1931,7 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
struct object_id base_oid;
if (!(offset_to_pack_pos(p, obj_offset, &pos))) {
struct object_info oi = OBJECT_INFO_INIT;
+ unsigned long bsz_ul = 0;
nth_packed_object_id(&base_oid, p,
pack_pos_to_index(p, pos));
@@ -1938,11 +1942,13 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
mark_bad_packed_object(p, &base_oid);
oi.typep = &type;
- oi.sizep = &base_size;
+ oi.sizep = &bsz_ul;
oi.contentp = &base;
if (odb_read_object_info_extended(r->objects, &base_oid,
&oi, 0) < 0)
base = NULL;
+ else
+ base_size = bsz_ul;
external_base = base;
}
diff --git a/packfile.h b/packfile.h
index 49d6bdecf6..0b5ae3f9fc 100644
--- a/packfile.h
+++ b/packfile.h
@@ -455,7 +455,8 @@ off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);
off_t find_pack_entry_one(const struct object_id *oid, struct packed_git *);
int is_pack_valid(struct packed_git *);
-void *unpack_entry(struct repository *r, struct packed_git *, off_t, enum object_type *, unsigned long *);
+void *unpack_entry(struct repository *r, struct packed_git *, off_t,
+ enum object_type *, size_t *);
unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, size_t *sizep);
unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, size_t *);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/7] pack-objects(check_pack_inflate()): use size_t instead of unsigned long
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 10:51 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2137.git.1780570272.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
`write_reuse_object()` learned to track its packed-object size as
`size_t` in 606c192380 (odb, packfile: use size_t for streaming
object sizes, 2026-05-08), but the comparison sink it feeds,
`check_pack_inflate()`, still takes the expected decompressed size
as `unsigned long`. The call site bridges the mismatch with
`cast_size_t_to_ulong()`, which on Windows turns a >4 GiB object
into an immediate die().
That function only uses `expect` once: as the right-hand side of a
`stream.total_out == expect` equality test against zlib's counter.
zlib's own `total_out` counter is `uLong` and is therefore still
32-bit-bound on Windows. Widening `expect` to `size_t` cannot fix that,
but it is a strict improvement nonetheless: instead of dying outright,
an oversized object now simply makes the equality fail and lets
`write_reuse_object()` fall back to `write_no_reuse_object()`, which
decompresses and re-deflates the content (and which the larger
pack-objects widening series targets separately).
Drop the `cast_size_t_to_ulong()` shim at the call site now that
the receiving parameter speaks the same type as `entry_size`.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pack-objects.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index fe9fbecb30..975f04d699 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -453,7 +453,7 @@ static int check_pack_inflate(struct packed_git *p,
struct pack_window **w_curs,
off_t offset,
off_t len,
- unsigned long expect)
+ size_t expect)
{
git_zstream stream;
unsigned char fakebuf[4096], *in;
@@ -671,8 +671,7 @@ static off_t write_reuse_object(struct hashfile *f, struct object_entry *entry,
datalen -= entry->in_pack_header_size;
if (!pack_to_stdout && p->index_version == 1 &&
- check_pack_inflate(p, &w_curs, offset, datalen,
- cast_size_t_to_ulong(entry_size))) {
+ check_pack_inflate(p, &w_curs, offset, datalen, entry_size)) {
error(_("corrupt packed object for %s"),
oid_to_hex(&entry->idx.oid));
unuse_pack(&w_curs);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/7] patch-delta: use size_t for sizes
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 10:51 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2137.git.1780570272.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
`patch_delta()` takes the source and delta sizes by value and writes
back the reconstructed target size through an `unsigned long *`. That
datatype cannot represent a value that exceeds 4 GiB on systems where
`unsigned long` is 32-bit (notably 64-bit Windows builds), though, even
though the delta encoding itself, the on-disk layout, and the in-memory
buffers happily carry such sizes. A `size_t` companion to
`get_delta_hdr_size()`, `get_delta_hdr_size_sz()`, was introduced in
17fa077596 (delta, packfile: use size_t for delta header sizes,
2026-05-08) precisely so that `patch_delta()` could be widened without
changing the on-the-wire decoding helper's signature.
Widen `patch_delta()`'s three size parameters to `size_t` and switch
its internal use of `get_delta_hdr_size()` to the `_sz` variant.
Then propagate the wider type through the callers.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
apply.c | 2 +-
builtin/index-pack.c | 4 ++--
builtin/unpack-objects.c | 2 +-
delta.h | 6 +++---
packfile.c | 4 +---
patch-delta.c | 12 ++++++------
t/helper/test-delta.c | 10 ++++++----
7 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/apply.c b/apply.c
index 249248d4f2..3cf544e9a9 100644
--- a/apply.c
+++ b/apply.c
@@ -3232,7 +3232,7 @@ static int apply_binary_fragment(struct apply_state *state,
struct patch *patch)
{
struct fragment *fragment = patch->fragments;
- unsigned long len;
+ size_t len;
void *dst;
if (!fragment)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index cf0bd8280d..3c4474e681 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -71,7 +71,7 @@ struct base_data {
/* Not initialized by make_base(). */
struct list_head list;
void *data;
- unsigned long size;
+ size_t size;
};
/*
@@ -1048,7 +1048,7 @@ static struct base_data *resolve_delta(struct object_entry *delta_obj,
{
void *delta_data, *result_data;
struct base_data *result;
- unsigned long result_size;
+ size_t result_size;
if (show_stat) {
int i = delta_obj - objects;
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 59e9b8711e..e7a50c493c 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -314,7 +314,7 @@ static void resolve_delta(unsigned nr, enum object_type type,
void *delta, unsigned long delta_size)
{
void *result;
- unsigned long result_size;
+ size_t result_size;
result = patch_delta(base, base_size,
delta, delta_size,
diff --git a/delta.h b/delta.h
index fad68cfc45..bb149dc82b 100644
--- a/delta.h
+++ b/delta.h
@@ -75,9 +75,9 @@ diff_delta(const void *src_buf, unsigned long src_bufsize,
* *trg_bufsize is updated with its size. On failure a NULL pointer is
* returned. The returned buffer must be freed by the caller.
*/
-void *patch_delta(const void *src_buf, unsigned long src_size,
- const void *delta_buf, unsigned long delta_size,
- unsigned long *dst_size);
+void *patch_delta(const void *src_buf, size_t src_size,
+ const void *delta_buf, size_t delta_size,
+ size_t *dst_size);
/* the smallest possible delta size is 4 bytes */
#define DELTA_SIZE_MIN 4
diff --git a/packfile.c b/packfile.c
index 89366abfe3..e202f48837 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1964,10 +1964,8 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
(uintmax_t)curpos, p->pack_name);
data = NULL;
} else {
- unsigned long sz;
data = patch_delta(base, base_size, delta_data,
- delta_size, &sz);
- size = sz;
+ delta_size, &size);
/*
* We could not apply the delta; warn the user, but
diff --git a/patch-delta.c b/patch-delta.c
index b5c8594db6..44cda97994 100644
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -12,13 +12,13 @@
#include "git-compat-util.h"
#include "delta.h"
-void *patch_delta(const void *src_buf, unsigned long src_size,
- const void *delta_buf, unsigned long delta_size,
- unsigned long *dst_size)
+void *patch_delta(const void *src_buf, size_t src_size,
+ const void *delta_buf, size_t delta_size,
+ size_t *dst_size)
{
const unsigned char *data, *top;
unsigned char *dst_buf, *out, cmd;
- unsigned long size;
+ size_t size;
if (delta_size < DELTA_SIZE_MIN)
return NULL;
@@ -27,12 +27,12 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
top = (const unsigned char *) delta_buf + delta_size;
/* make sure the orig file size matches what we expect */
- size = get_delta_hdr_size(&data, top);
+ size = get_delta_hdr_size_sz(&data, top);
if (size != src_size)
return NULL;
/* now the result size */
- size = get_delta_hdr_size(&data, top);
+ size = get_delta_hdr_size_sz(&data, top);
dst_buf = xmallocz(size);
out = dst_buf;
diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 52ea00c937..8223a60229 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -21,7 +21,7 @@ int cmd__delta(int argc, const char **argv)
int fd;
struct strbuf from = STRBUF_INIT, data = STRBUF_INIT;
char *out_buf;
- unsigned long out_size;
+ size_t out_size;
if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p")))
usage(usage_str);
@@ -31,11 +31,13 @@ int cmd__delta(int argc, const char **argv)
if (strbuf_read_file(&data, argv[3], 0) < 0)
die_errno("unable to read '%s'", argv[3]);
- if (argv[1][1] == 'd')
+ if (argv[1][1] == 'd') {
+ unsigned long delta_size;
out_buf = diff_delta(from.buf, from.len,
data.buf, data.len,
- &out_size, 0);
- else
+ &delta_size, 0);
+ out_size = delta_size;
+ } else
out_buf = patch_delta(from.buf, from.len,
data.buf, data.len,
&out_size);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/7] compat/msvc: use _chsize_s for ftruncate
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 10:51 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2137.git.1780570272.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
On Windows, `unsigned long` and `long` are 32 bits even on 64-bit
builds. The MSVC compatibility header has shimmed `ftruncate()` with
#define ftruncate _chsize
ever since `compat/msvc-posix.h` was introduced. `_chsize()` takes a
32-bit `long` for the new length, which silently truncates files (and
the requested size) to 2 GiB. That is enough to make t7508 test 126
"git add fails gracefully with 4 GiB and 8 GiB files" fail under
MSVC: `test-tool truncate` creates a sparse 4 GiB or 8 GiB file via
the shimmed `ftruncate()`, and the test never gets off the ground.
`_chsize_s()` is the modern replacement, accepts a 64-bit `__int64`
length, and is the only sensible target on Windows. The catch is that
it does not follow the POSIX `-1` + `errno` convention: it returns
`0` on success and an errno value (a small positive integer) on
failure. A plain `#define ftruncate _chsize_s` would therefore
silently break callers that test the return value as `< 0` or against
`-1`, of which there are several: `http.c`, `parallel-checkout.c`,
and `t/helper/test-truncate.c` among them.
Introduce a `static inline` wrapper that calls `_chsize_s()`, copies
its errno return into `errno`, and translates the result to the
familiar `-1` / `0` convention, then point `ftruncate` at the
wrapper. Place the wrapper after `#include "mingw-posix.h"` so the
`off_t` parameter resolves to the already-widened `off64_t` rather
than the 32-bit `_off_t` from `compat/vcbuild/include/unistd.h`.
MinGW is unaffected: its `ftruncate()` already takes `off_t` and
routes through `ftruncate64()` when `_FILE_OFFSET_BITS=64`, which is
the default in our build.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/msvc-posix.h | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/compat/msvc-posix.h b/compat/msvc-posix.h
index c500b8b4aa..7ce39b8d3f 100644
--- a/compat/msvc-posix.h
+++ b/compat/msvc-posix.h
@@ -16,7 +16,6 @@
#define __attribute__(x)
#define strcasecmp _stricmp
#define strncasecmp _strnicmp
-#define ftruncate _chsize
#define strtoull _strtoui64
#define strtoll _strtoi64
@@ -30,4 +29,27 @@ typedef int sigset_t;
#include "mingw-posix.h"
+/*
+ * MSVC's `_chsize()` takes a 32-bit `long` and silently truncates files
+ * to 2 GiB. `_chsize_s()` accepts a 64-bit length but returns 0 on
+ * success or an errno value on failure, rather than the -1/errno
+ * convention POSIX `ftruncate()` callers expect. Wrap it so callers
+ * that test the return value as `< 0` or against `-1` keep working.
+ *
+ * Note: this declaration must follow `#include "mingw-posix.h"` so
+ * `off_t` resolves to `off64_t` and the parameter type matches the
+ * underlying `_chsize_s()` width.
+ */
+static inline int msvc_ftruncate(int fd, off_t length)
+{
+ int err = _chsize_s(fd, length);
+
+ if (err) {
+ errno = err;
+ return -1;
+ }
+ return 0;
+}
+#define ftruncate msvc_ftruncate
+
#endif /* COMPAT_MSVC_POSIX_H */
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/7] More work supporting objects larger than 4GB on Windows
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 10:51 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Johannes Schindelin
This patch series tries to address the problems pointed out by the expensive
tests that now run in CI: t5608 and t7508 verify various aspects about
objects larger than 4GB, which Git does not currently handle correctly when
run on a platform where size_t is 64-bit and unsigned long is 32-bit.
Unfortunately, this conflicts heavily with ps/odb-source-loose. I rebased
the branch onto seen and pushed the result to
https://github.com/dscho/git/tree/refs/heads/objects-larger-than-4gb-on-windows-pt2-seen,
to make it easier to resolve merge conflicts. Here is the relevant
range-diff:
1: f3aeae983a ! 1: 62adeb9818 odb: use size_t for object_info.sizep and the size APIs
@@ builtin/log.c: static int show_blob_object(const struct object_id *oid, struct r
## builtin/ls-files.c ##
@@ builtin/ls-files.c: static void expand_objectsize(struct repository *repo, struct strbuf *line,
- const enum object_type type, unsigned int padded)
- {
+ size_t len;
+
if (type == OBJ_BLOB) {
- unsigned long size;
+ size_t size;
@@ builtin/ls-files.c: static void expand_objectsize(struct repository *repo, struc
## builtin/ls-tree.c ##
@@ builtin/ls-tree.c: static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
- const enum object_type type, unsigned int padded)
- {
+ size_t len;
+
if (type == OBJ_BLOB) {
- unsigned long size;
+ size_t size;
@@ notes.c: static void format_note(struct notes_tree *t, const struct object_id *o
if (!t)
## object-file.c ##
-@@ object-file.c: static int parse_loose_header(const char *hdr, struct object_info *oi)
+@@ object-file.c: int parse_loose_header(const char *hdr, struct object_info *oi)
}
if (oi->sizep)
@@ object-file.c: static int parse_loose_header(const char *hdr, struct object_info
/*
* The length must be followed by a zero byte
-@@ object-file.c: static int read_object_info_from_path(struct odb_source *source,
- void *map = NULL;
- git_zstream stream, *stream_to_end = NULL;
- char hdr[MAX_HEADER_LEN];
-- unsigned long size_scratch;
-+ size_t size_scratch;
- enum object_type type_scratch;
- struct stat st;
-
@@ object-file.c: int force_object_loose(struct odb_source *source,
- {
+ struct odb_source_files *files = odb_source_files_downcast(source);
const struct git_hash_algo *compat = source->odb->repo->compat_hash_algo;
void *buf;
- unsigned long len;
@@ object-file.c: int read_loose_object(struct repository *repo,
fd = git_open(path);
if (fd >= 0)
-@@ object-file.c: int odb_source_loose_read_object_stream(struct odb_read_stream **out,
- struct object_info oi = OBJECT_INFO_INIT;
- struct odb_loose_read_stream *st;
- unsigned long mapsize;
-- unsigned long size_ul;
- void *mapped;
-
- mapped = odb_source_loose_map_object(source, oid, &mapsize);
-@@ object-file.c: int odb_source_loose_read_object_stream(struct odb_read_stream **out,
- goto error;
- }
-
-- /*
-- * object_info.sizep is unsigned long* (32-bit on Windows), but
-- * st->base.size is size_t (64-bit). Use temporary variable.
-- * Note: loose objects >4GB would still truncate here, but such
-- * large loose objects are uncommon (they'd normally be packed).
-- */
-- oi.sizep = &size_ul;
-+ oi.sizep = &st->base.size;
- oi.typep = &st->base.type;
-
- if (parse_loose_header(st->hdr, &oi) < 0 || st->base.type < 0)
- goto error;
-- st->base.size = size_ul;
-
- st->mapped = mapped;
- st->mapsize = mapsize;
## object.c ##
@@ object.c: struct object *parse_object_with_flags(struct repository *r,
@@ odb.h: int odb_read_object_info_extended(struct object_database *odb,
enum odb_has_object_flags {
/* Retry packed storage after checking packed and loose storage */
+ ## odb/source-loose.c ##
+@@ odb/source-loose.c: static int read_object_info_from_path(struct odb_source_loose *loose,
+ void *map = NULL;
+ git_zstream stream, *stream_to_end = NULL;
+ char hdr[MAX_HEADER_LEN];
+- unsigned long size_scratch;
++ size_t size_scratch;
+ enum object_type type_scratch;
+ struct stat st;
+
+@@ odb/source-loose.c: static int odb_source_loose_read_object_stream(struct odb_read_stream **out,
+ struct object_info oi = OBJECT_INFO_INIT;
+ struct odb_loose_read_stream *st;
+ unsigned long mapsize;
+- unsigned long size_ul;
+ void *mapped;
+
+ mapped = odb_source_loose_map_object(loose, oid, &mapsize);
+@@ odb/source-loose.c: static int odb_source_loose_read_object_stream(struct odb_read_stream **out,
+ goto error;
+ }
+
+- /*
+- * object_info.sizep is unsigned long* (32-bit on Windows), but
+- * st->base.size is size_t (64-bit). Use temporary variable.
+- * Note: loose objects >4GB would still truncate here, but such
+- * large loose objects are uncommon (they'd normally be packed).
+- */
+- oi.sizep = &size_ul;
++ oi.sizep = &st->base.size;
+ oi.typep = &st->base.type;
+
+ if (parse_loose_header(st->hdr, &oi) < 0 || st->base.type < 0)
+ goto error;
+- st->base.size = size_ul;
+
+ st->mapped = mapped;
+ st->mapsize = mapsize;
+
## odb/streaming.c ##
@@ odb/streaming.c: static int open_istream_incore(struct odb_read_stream **out,
.base.read = read_istream_incore,
Johannes Schindelin (7):
compat/msvc: use _chsize_s for ftruncate
patch-delta: use size_t for sizes
pack-objects(check_pack_inflate()): use size_t instead of unsigned
long
packfile: widen unpack_entry()'s size out-parameter to size_t
pack-objects: use size_t for in-core object sizes
packfile,delta: drop the `cast_size_t_to_ulong()` wrappers
odb: use size_t for object_info.sizep and the size APIs
apply.c | 8 ++--
archive.c | 4 +-
attr.c | 2 +-
bisect.c | 2 +-
blame.c | 15 +++++--
builtin/cat-file.c | 39 ++++++++++++-------
builtin/difftool.c | 2 +-
builtin/fast-export.c | 7 +++-
builtin/fast-import.c | 29 ++++++++++----
builtin/fsck.c | 2 +-
builtin/grep.c | 12 +++---
builtin/index-pack.c | 10 ++---
builtin/log.c | 2 +-
builtin/ls-files.c | 2 +-
builtin/ls-tree.c | 4 +-
builtin/merge-tree.c | 6 +--
builtin/mktag.c | 2 +-
builtin/notes.c | 6 +--
builtin/pack-objects.c | 73 +++++++++++++++++++++--------------
builtin/repo.c | 4 +-
builtin/tag.c | 4 +-
builtin/unpack-file.c | 2 +-
builtin/unpack-objects.c | 8 ++--
bundle.c | 2 +-
combine-diff.c | 4 +-
commit.c | 10 ++---
compat/msvc-posix.h | 24 +++++++++++-
config.c | 2 +-
delta.h | 20 +++-------
diff.c | 5 ++-
dir.c | 2 +-
entry.c | 4 +-
fmt-merge-msg.c | 4 +-
fsck.c | 2 +-
grep.c | 4 +-
http-push.c | 2 +-
list-objects-filter.c | 2 +-
mailmap.c | 2 +-
match-trees.c | 4 +-
merge-blobs.c | 6 +--
merge-blobs.h | 2 +-
merge-ort.c | 2 +-
notes-cache.c | 2 +-
notes-merge.c | 2 +-
notes.c | 8 ++--
object-file.c | 18 +++------
object.c | 2 +-
odb.c | 12 +++---
odb.h | 10 ++---
odb/streaming.c | 13 +------
pack-bitmap.c | 4 +-
pack-check.c | 5 +--
pack-objects.h | 2 +-
packfile.c | 54 ++++++++++----------------
packfile.h | 5 ++-
patch-delta.c | 8 ++--
path-walk.c | 2 +-
protocol-caps.c | 5 ++-
read-cache.c | 6 +--
ref-filter.c | 2 +-
reflog.c | 2 +-
rerere.c | 2 +-
submodule-config.c | 2 +-
t/helper/test-delta.c | 10 +++--
t/helper/test-pack-deltas.c | 3 +-
t/helper/test-partial-clone.c | 2 +-
t/unit-tests/u-odb-inmemory.c | 2 +-
tag.c | 4 +-
tree-walk.c | 10 +++--
tree.c | 2 +-
xdiff-interface.c | 2 +-
71 files changed, 296 insertions(+), 253 deletions(-)
base-commit: 9ac3f193c05c2237e2b14ebaa1149e9fc8a1abe0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2137%2Fdscho%2Fobjects-larger-than-4gb-on-windows-pt2-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2137/dscho/objects-larger-than-4gb-on-windows-pt2-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2137
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH] transport-helper: fix TSAN race in transfer_debug()
From: Pushkar Singh @ 2026-06-04 10:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff
In-Reply-To: <xmqqv7bzp0vc.fsf@gitster.g>
Hi Junio,
On Thu, Jun 4, 2026 at 6:39 AM Junio C Hamano <gitster@pobox.com> wrote:
> Would it be possible that transfer_debug_enabled is still -1 at this
> point? We would proceed in such a case, which is a bit different from
> what would have happened in the original.
>
> Perhaps
>
> if (transfer_debug_enabled <= 0)
> return;
>
> is what you want? I dunno.
You're right. The original code would never proceed while the value was still
negative, whereas my change would.
I'll update it to use <= 0 and send a v2.
Thanks,
Pushkar
^ permalink raw reply
* [PATCH v3 8/8] t: let prove fail when parsing invalid TAP output
From: Patrick Steinhardt @ 2026-06-04 10:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260604-pks-t7527-fix-tap-output-v3-0-7d766ed481e4@pks.im>
To make the result of our tests accessible we use the TAP protocol. This
protocol is parsed by either prove or by Meson. Unfortunately, these two
tools differ when it comes to their strictness when parsing the
protocol:
- Prove by default happily accepts lines not specified by the
protocol.
- Meson will also accept such lines, but prints a big and ugly warning
message.
We have fixed our test suite in the past to not print invalid TAP lines
anymore via b1dc2e796e (Merge branch 'ps/meson-tap-parse', 2025-06-17).
But as none of our tools perform a strict check it's still possible for
broken tests to sneak back in, like for example in 362f69547f (Merge
branch 'ps/t1006-tap-fix', 2025-07-16). This doesn't hurt at all when
using prove, but it's quite annoying when using Meson due to the
generated warnings.
Unfortunately, there doesn't seem to be a portable way to make all tools
complain about violations of the TAP format. The TAP 14 specification
has added pragmas to the protocol that would allow us to say `pragma
+strict`, and the effect of that would be to treat invalid TAP lines as
a test failure. But the release of TAP 14 is still rather recent, and
Test-Harness for example only gained support for it in version 3.48,
which was released in 2023.
In fact though, this pragma was already introduced as an inofficial
extension of the TAP protocol with Test-Harness 3.10, released in 2008.
So while not all tools understand the pragma, at least prove does for a
long time.
Unconditionally enable the pragma when using prove so that we'll detect
tests that emit broken TAP output right away. This would have detected
the issues fixed in preceding commits:
$ prove t7527-builtin-fsmonitor.sh
t7527-builtin-fsmonitor.sh .. All 69 subtests passed
(less 6 skipped subtests: 63 okay)
Test Summary Report
-------------------
t7527-builtin-fsmonitor.sh (Wstat: 0 Tests: 69 Failed: 0)
Parse errors: Unknown TAP token: "Initialized empty Git repository in /tmp/git/test_fsmonitor_smoke/.git/"
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/test-lib.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d1d24c4124..ceefb99bff 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1532,6 +1532,12 @@ then
BAIL_OUT 'You need to build test-tool; Run "make t/helper/test-tool" in the source (toplevel) directory'
fi
+if test -n "$HARNESS_ACTIVE"
+then
+ say "TAP version 13"
+ say "pragma +strict"
+fi
+
# Are we running this test at all?
remove_trash=
this_test=${0##*/}
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v3 7/8] t/lib-git-p4: silence output when killing p4d and its watchdog
From: Patrick Steinhardt @ 2026-06-04 10:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260604-pks-t7527-fix-tap-output-v3-0-7d766ed481e4@pks.im>
When stopping the p4d watchdog process via "kill -9", the shell may
print a job-control notification like:
./test-lib.sh: line 1269: 57960 Killed: 9 while true; do
if test $nr_tries_left -eq 0; then
kill -9 $p4d_pid; exit 1;
fi; sleep 1; nr_tries_left=$(($nr_tries_left - 1));
done 2> /dev/null 4>&2 (wd: ~)
This message is printed asynchronously by the shell when it reaps the
process. While harmless right now, this will cause breakage once we
enable strict parsing of the TAP protocol in a subsequent commit.
Fix this by using `wait` so that we can synchronously reap the watchdog
process and swallow the diagnostic.
While at it, deduplicate the logic we have in `stop_p4d_and_watchdog ()`
and `stop_and_cleanup_p4d ()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-git-p4.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index d22e9c684a..9108868187 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -65,6 +65,7 @@ pidfile="$TRASH_DIRECTORY/p4d.pid"
stop_p4d_and_watchdog () {
kill -9 $p4d_pid $watchdog_pid
+ wait $p4d_pid $watchdog_pid 2>/dev/null
}
# git p4 submit generates a temp file, which will
@@ -174,8 +175,7 @@ retry_until_success () {
}
stop_and_cleanup_p4d () {
- kill -9 $p4d_pid $watchdog_pid
- wait $p4d_pid
+ stop_p4d_and_watchdog
rm -rf "$db" "$cli" "$pidfile"
}
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v3 6/8] t/test-lib: silence EBUSY errors on Windows during test cleanup
From: Patrick Steinhardt @ 2026-06-04 10:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260604-pks-t7527-fix-tap-output-v3-0-7d766ed481e4@pks.im>
When tests have finished we clean up the trash directory via `rm -rf`.
On Windows this can fail with EBUSY in cases where a process still holds
some of the files open, for example when we have spawned a daemonized
process that wasn't properly terminated. We thus retry several times,
but every failure will result in error messages being printed, and that
in turn breaks the TAP output format.
One such case where this is causing issues is in t921x, which contains
tests related to Scalar. Some tests spawn the fsmonitor daemon, and we
never properly terminate it.
The obvious fix would be to ensure that we never leak any processes, but
that gets ugly fast. Instead, let's work around the issue by silencing
error messages printed by the `rm -rf` calls. We already know to print
an error when the retry loop fails, so we don't loose much.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/test-lib.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4a7357b547..d1d24c4124 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1299,10 +1299,10 @@ test_done () {
error "Tests passed but trash directory already removed before test cleanup; aborting"
cd "$TRASH_DIRECTORY/.." &&
- rm -fr "$TRASH_DIRECTORY" || {
+ rm -fr "$TRASH_DIRECTORY" 2>/dev/null || {
# try again in a bit
sleep 5;
- rm -fr "$TRASH_DIRECTORY"
+ rm -fr "$TRASH_DIRECTORY" 2>/dev/null
} ||
error "Tests passed but test cleanup failed; aborting"
fi
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v3 5/8] t7810: turn MB_REGEX check into a lazy prereq
From: Patrick Steinhardt @ 2026-06-04 10:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260604-pks-t7527-fix-tap-output-v3-0-7d766ed481e4@pks.im>
In t7810 we verify whether the system has proper multibyte locale
support by executing `test-tool regex` with a unicode character. When
this check fails though we'll output an error that breaks the TAP
format.
Fix this issue by turning the logic into a lazy prerequisite.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t7810-grep.sh | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1b195bee59..d61c4a4d73 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -18,8 +18,9 @@ test_invalid_grep_expression() {
'
}
-LC_ALL=en_US.UTF-8 test-tool regex '^.$' '¿' &&
- test_set_prereq MB_REGEX
+test_lazy_prereq MB_REGEX '
+ LC_ALL=en_US.UTF-8 test-tool regex "^.$" "¿"
+'
cat >hello.c <<EOF
#include <assert.h>
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v3 4/8] t7527: fix broken TAP output
From: Patrick Steinhardt @ 2026-06-04 10:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260604-pks-t7527-fix-tap-output-v3-0-7d766ed481e4@pks.im>
Before running the tests in t7527 we first verify whether the fsmonitor
even works, which seems to depend on the actual filesystem that is in
use. The verification executes outside of any prerequisite or test body,
so its stdout/stderr is not being redirected.
The consequence of this is that any command that prints to stdout/stderr
may break the TAP specification by printing invalid lines. And in fact
we already do that, as git-init(1) prints the path to the created Git
repository by default.
Fix this issue by moving the logic into a lazy prerequisite.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t7527-builtin-fsmonitor.sh | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index b63c162f9b..d881e27466 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -25,7 +25,8 @@ maybe_timeout () {
"$@"
fi
}
-verify_fsmonitor_works () {
+
+test_lazy_prereq FSMONITOR_WORKS '
git init test_fsmonitor_smoke || return 1
GIT_TRACE_FSMONITOR="$PWD/smoke.trace" &&
@@ -50,9 +51,9 @@ verify_fsmonitor_works () {
ret=$?
rm -rf test_fsmonitor_smoke smoke.trace
return $ret
-}
+'
-if ! verify_fsmonitor_works
+if ! test_have_prereq FSMONITOR_WORKS
then
skip_all="filesystem does not deliver fsmonitor events (container/overlayfs?)"
test_done
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v3 3/8] ci: unify Linux images across GitLab and GitHub
From: Patrick Steinhardt @ 2026-06-04 10:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260604-pks-t7527-fix-tap-output-v3-0-7d766ed481e4@pks.im>
The image for the "linux-breaking-changes" job has drifted apart across
GitHub and GitLab. Adapt it to use "ubuntu:rolling" on both systems.
With this change there's only one difference remaining: GitHub uses
"ubuntu:focal" for the "linux32" job while GitLab uses "ubuntu:20.04".
These are different names for the same image, so there is no actual
difference here. Adjust GitHub to use the "20.04" tag -- this matches
all the other jobs which use version numbers, and you don't have to
learn Ubuntu's release names by heart.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.github/workflows/main.yml | 2 +-
.gitlab-ci.yml | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 3da5326f0b..cf341d74db 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -407,7 +407,7 @@ jobs:
image: alpine:latest
# Supported until 2025-04-02.
- jobname: linux32
- image: i386/ubuntu:focal
+ image: i386/ubuntu:20.04
# A RHEL 8 compatible distro. Supported until 2029-05-31.
- jobname: almalinux-8
image: almalinux:8
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index a5bdec5159..49f3689b6a 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -47,7 +47,7 @@ test:linux:
CC: gcc
CC_PACKAGE: gcc-8
- jobname: linux-breaking-changes
- image: ubuntu:20.04
+ image: ubuntu:rolling
CC: gcc
- jobname: fedora-breaking-changes-meson
image: fedora:latest
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v3 2/8] gitlab-ci: add missing Linux jobs
From: Patrick Steinhardt @ 2026-06-04 10:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260604-pks-t7527-fix-tap-output-v3-0-7d766ed481e4@pks.im>
The GitLab CI definitions are missing jobs for AlmaLinux and Debian,
both of which exist in GitHub Workflows. Plug this gap.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 6 ++++++
ci/lib.sh | 2 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 8cb41baa14..a5bdec5159 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -68,6 +68,12 @@ test:linux:
# Supported until 2025-04-02.
- jobname: linux32
image: i386/ubuntu:20.04
+ # A RHEL 8 compatible distro. Supported until 2029-05-31.
+ - jobname: almalinux-8
+ image: almalinux:8
+ # Supported until 2026-08-31.
+ - jobname: debian-11
+ image: debian:11
artifacts:
paths:
- t/failed-test-artifacts
diff --git a/ci/lib.sh b/ci/lib.sh
index 6e3799cfc3..b939110a6e 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -254,7 +254,7 @@ then
CI_OS_NAME=osx
JOBS=$(nproc)
;;
- *,alpine:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
+ *,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
CI_OS_NAME=linux
JOBS=$(nproc)
;;
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v3 1/8] gitlab-ci: rearrange Linux jobs to match GitHub's order
From: Patrick Steinhardt @ 2026-06-04 10:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260604-pks-t7527-fix-tap-output-v3-0-7d766ed481e4@pks.im>
Rearrange the order of Linux jobs that we have defined in GitLab CI so
that it matches the order on GitHub's side. This makes it easier to
compare whether the list of jobs actually matches on both sides.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index e0b9a0d82b..8cb41baa14 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -42,15 +42,15 @@ test:linux:
- jobname: linux-reftable
image: ubuntu:rolling
CC: clang
+ - jobname: linux-TEST-vars
+ image: ubuntu:20.04
+ CC: gcc
+ CC_PACKAGE: gcc-8
- jobname: linux-breaking-changes
image: ubuntu:20.04
CC: gcc
- jobname: fedora-breaking-changes-meson
image: fedora:latest
- - jobname: linux-TEST-vars
- image: ubuntu:20.04
- CC: gcc
- CC_PACKAGE: gcc-8
- jobname: linux-leaks
image: ubuntu:rolling
CC: gcc
@@ -60,13 +60,14 @@ test:linux:
- jobname: linux-asan-ubsan
image: ubuntu:rolling
CC: clang
+ - jobname: linux-meson
+ image: ubuntu:rolling
+ CC: gcc
- jobname: linux-musl-meson
image: alpine:latest
+ # Supported until 2025-04-02.
- jobname: linux32
image: i386/ubuntu:20.04
- - jobname: linux-meson
- image: ubuntu:rolling
- CC: gcc
artifacts:
paths:
- t/failed-test-artifacts
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v3 0/8] t: fix broken TAP output
From: Patrick Steinhardt @ 2026-06-04 10:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260602-pks-t7527-fix-tap-output-v1-0-db3da2a1b137@pks.im>
Hi,
this small patch series fixes another instance of broken TAP output that
has landed via 4d11b9c218 (Merge branch 'pt/fsmonitor-linux', 2026-05-31).
As this has happened multiple times by now I decided to have a look at
whether we can fix this class of issues a bit more holistically. So this
series also contains a change that makes prove bail out when it sees
invalid TAP output, which uncovers a small set of preexisting issues in
our test suite.
Changes in v3:
- Fix a test gap for AlmaLinux and Debian in GitLab CI, which uncovers
an issue flagged by Peff.
- Fix TAP breakage in t7810.
- Link to v2: https://patch.msgid.link/20260603-pks-t7527-fix-tap-output-v2-0-cf3af5694e20@pks.im
Changes in v2:
- Fix waiting for p4d, and deduplicate the logic that does this.
- Link to v1: https://patch.msgid.link/20260602-pks-t7527-fix-tap-output-v1-0-db3da2a1b137@pks.im
Test runs can be found at [1] and [2]. Note that GitHub-side tests are
failing on Windows, but that is a preexisting failure on "master".
Thanks!
Patrick
[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/585
[2]: https://github.com/git/git/pull/2320
---
Patrick Steinhardt (8):
gitlab-ci: rearrange Linux jobs to match GitHub's order
gitlab-ci: add missing Linux jobs
ci: unify Linux images across GitLab and GitHub
t7527: fix broken TAP output
t7810: turn MB_REGEX check into a lazy prereq
t/test-lib: silence EBUSY errors on Windows during test cleanup
t/lib-git-p4: silence output when killing p4d and its watchdog
t: let prove fail when parsing invalid TAP output
.github/workflows/main.yml | 2 +-
.gitlab-ci.yml | 23 +++++++++++++++--------
ci/lib.sh | 2 +-
t/lib-git-p4.sh | 4 ++--
t/t7527-builtin-fsmonitor.sh | 7 ++++---
t/t7810-grep.sh | 5 +++--
t/test-lib.sh | 10 ++++++++--
7 files changed, 34 insertions(+), 19 deletions(-)
Range-diff versus v2:
-: ---------- > 1: 5e817b102f gitlab-ci: rearrange Linux jobs to match GitHub's order
-: ---------- > 2: 83646cc834 gitlab-ci: add missing Linux jobs
-: ---------- > 3: cca1567fbf ci: unify Linux images across GitLab and GitHub
1: 52abbd5280 = 4: 430bc51818 t7527: fix broken TAP output
-: ---------- > 5: 78ef22df8d t7810: turn MB_REGEX check into a lazy prereq
2: ea1f1eb466 = 6: 7bbaeff48c t/test-lib: silence EBUSY errors on Windows during test cleanup
3: e97a515470 = 7: abf2be09e6 t/lib-git-p4: silence output when killing p4d and its watchdog
4: 436d7d8cf3 = 8: 04367c34be t: let prove fail when parsing invalid TAP output
---
base-commit: 1666c1265231b0bc5f613fbbf3f0a9896cdef76e
change-id: 20260601-pks-t7527-fix-tap-output-105da1d73df0
^ permalink raw reply
* Re: git history feedback
From: Kristoffer Haugsbakk @ 2026-06-04 9:34 UTC (permalink / raw)
To: Rasmus Villemoes, git; +Cc: Patrick Steinhardt
In-Reply-To: <87ecimhg8s.fsf@rasmusvillemoes.dk>
On Thu, Jun 4, 2026, at 10:17, Rasmus Villemoes wrote:
>[snip]
>
> So today I had occasion to put it to real use, and then I found two
> things I'd like to be able to do with it:
>
> When a commit needs to be split into three or more commits, it is a
> little cumbersome to do iteratively, since the new commit to split
> obviously has a new sha, so one first has to figure out what that new id
> is and then do another "git history split". For higher values of "three"
> that becomes rather tedious. So it would be nice if there was an
> iterative mode, which after splitting off the first commit would
> automatically start again with the new child commit.
For commit subject `anchor` I would do something like this.
1. `git history split :/anchor`
2. Split out the first commit with a new message; keep the `anchor`
subject of the original
3. Repeat (1)
>[snip]
^ permalink raw reply
* Re: Mirror repositories for submodules
From: Simon Richter @ 2026-06-04 9:27 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Benson Muite, git
In-Reply-To: <20260604061605.GA3194609@coredump.intra.peff.net>
Hi,
On 6/4/26 3:16 PM, Jeff King wrote:
> Here's a thought experiment. What if you put the UUID into a URL, like:
> repoid://123456789.git
Yes, that's the idea, except I would want to use a relative URL, like
../123456789.git
This could solve the "naive cloning" problem, because it creates an
expectation that the submodules can be found on the same server, or in a
nearby path.
I'm aware that this is *also* bad for decentralization, because it makes
it easier to use one of the big forges where the repositories for
often-used submodules are are already likely to be present, but it plays
into our use case, where we want to share the repositories for
often-used subprojects.
> Now, all of that said, do we still need uuids at all? If the canonical
> submodule name is https://github.com/git/git.git, then anybody can just
> rewrite that locally in the same way using url.*.insteadOf config.
Yes, but we'd then need a mechanism for a server to indicate "for
cloning, you should use these 'insteadOf' settings, which is a massive
can of worms from a security standpoint.
I also don't think these canonical URLs can ever be stable if they refer
to infrastructure that is not under the control of the maintainer -- it
would tie the project identity to the hosting provider, and increase the
inertia to overcome for moves (such as the current exodus from github
and gitlab towards codeberg).
> Which makes me wonder if I am missing something about the original
> request that started this thread. But it sounds to me like it is just
> asking for the existing URL-rewriting feature.
The original mail has a similar problem as we do in Debian, and as my
employer has: CI jobs should exclusively talk to in-house
infrastructure, because continuously cloning repositories for each build
is bad for the environment.
The common goal is that a naive clone should get submodules from a local
server, ideally without us having to write some tool to make an initial
checkout, enumerate submodules, create insteadOf settings, clone first
layer of submodules, enumerate second layer, ...
Simon
^ permalink raw reply
* Re: [PATCH v2 2/4] doc: replay: improve config description
From: Patrick Steinhardt @ 2026-06-04 9:05 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Junio C Hamano, Siddharth Asthana, git
In-Reply-To: <fcc2cf52-cb10-4799-a4c9-eb5916187075@app.fastmail.com>
On Thu, Jun 04, 2026 at 08:31:57AM +0200, Kristoffer Haugsbakk wrote:
> On Thu, Jun 4, 2026, at 08:27, Patrick Steinhardt wrote:
> > On Wed, Jun 03, 2026 at 06:04:23PM +0200,
> > kristofferhaugsbakk@fastmail.com wrote:
> >> diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
> >> index f9ca2db2833..4de85088d6c 100644
> >> --- a/Documentation/git-replay.adoc
> >> +++ b/Documentation/git-replay.adoc
> >> @@ -211,6 +211,7 @@ to use bare commit IDs instead of branch names.
> >>
> >> CONFIGURATION
> >> -------------
> >> +:git-replay: 1
> >> include::config/replay.adoc[]
> >
> > Not quite sure, but was this change supposed to be part of the preceding
> > commit, where you also added the include?
>
> No, because the conditional is only being put to use now. That was the
> intention anyway. Maybe there is some reason to put it in the first
> commit?
Probably not. It just read funny, but I guess that it's my ignorance
about the adoc format that was also at play here.
Patrick
^ permalink raw reply
* Re: [PATCH v2 9/9] builtin/history: implement "drop" subcommand
From: Patrick Steinhardt @ 2026-06-04 9:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Pablo Sabater
In-Reply-To: <xmqqmrxbqir4.fsf@gitster.g>
On Thu, Jun 04, 2026 at 08:58:07AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > +static int update_worktree(struct repository *repo,
> > + const struct commit *old_head,
> > + const struct commit *new_head,
> > + bool dry_run)
> > +{
> > + struct reset_head_opts opts = {
> > + .oid_from = &old_head->object.oid,
> > + .oid = &new_head->object.oid,
> > + .flags = RESET_HEAD_SKIP_REF_UPDATES,
> > + };
> > + if (dry_run)
> > + opts.flags |= RESET_HEAD_DRY_RUN;
> > + return reset_head(repo, &opts);
> > +}
> > + ...
> > + /*
> > + * 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 (!dry_run && !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;
> > + }
> > + }
>
> This block is skipped under --dry-run, but update_worktree is
> equipped to (and indeed run unconditionally here) run in the dry-run
> mode. Does it mean that "git history drop --dry-run" that user runs
> to see which refs may be updated will not get warned about possible
> worktree conflicts that would prevent the real run from happening?
> Unless there is a compelling reason not to, I think --dry-run should
> be a close simulation of what would happen without it.
That's an oversight on my part indeed. We _should_ run this block with
dry-run, and it shows that we're definitely lacking test coverage here.
Will fix, thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v2 9/9] builtin/history: implement "drop" subcommand
From: Patrick Steinhardt @ 2026-06-04 9:02 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, Pablo Sabater, Junio C Hamano
In-Reply-To: <4b4672de-17cc-426f-8498-6384b1ad0d06@app.fastmail.com>
On Wed, Jun 03, 2026 at 09:04:33PM +0200, Kristoffer Haugsbakk wrote:
> On Wed, Jun 3, 2026, at 18:14, Patrick Steinhardt wrote:
> > diff --git a/Documentation/git-history.adoc
> > b/Documentation/git-history.adoc
> > index 2ba8121795..4eac732fd2 100644
> > --- a/Documentation/git-history.adoc
> > +++ b/Documentation/git-history.adoc
> > @@ -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.
>
> Should section names be “bare” or quoted like "LIMITATIONS"?
> I don’t know.
>
> Maybe add “above” since it’s a previous section.
Hm. I think I'd prefer to keep this as-is, as we already have
preexisting documentation in the same manpage that does it exactly like
the above.
> > ++
> > +If `HEAD` points at a commit that is to be rewritten, the index and working
> >[snip]
> > +Drop a commit
> > +~~~~~~~~~~~~~
> > +
> > +----------
> > +$ git log --oneline
> > +abc1234 (HEAD -> main) third
> > +def5678 second
> > +ghi9012 first
> > +
> > +$ git history drop def5678
>
> I know this is only the most simple example. And I might be dragging in
> something beyond the scope of this example. But I recall one
> demonstration on the first git-history(1) series which used a lot of
> revision expressions and someone saying that they couldn’t imagine a
> workflow where this would be more interactive than bringing up the
> git-rebase(1) todo editor.
>
> (I couldn’t find back to this right now.)
Yeah, I remember this discussion.
> Although it is slower in terms of machine cycles, the keyboard instinct
> for dropping a nearby commit might be to do `git rebase -i @~10`
> (sufficiently high number) and navigating quickly in the configured
> editor, deleting the line or using the keybind for `drop`. This example
> which by implication brings up the log in order to paste the abbreviated
> hash isn’t as ergonomic in comparison.
>
> But using a revision expression like searching the subject with
> `main^{/second}`, while not quicker probably, does distinguish itself
> from git-rebase(1) by being a pretty fast ad hoc invocation that can be
> done in one command without futzing with some weird sed(1) editor in
> order to navigate to the `second` line and deleting it, or
> something. And that’s a small win in isolation, but it segues much more
> naturally into letting you script, say, dropping the last commit that
> starts with the subject `TEMP`.
>
> Or maybe revision expressions is too much in this context?
No, I think that's actually a good idea to demonstrate how this can be
used without being too unergomonic.
Eventually I still have the idea in my mind to implement a TUI around
git-history(1) that allows to drive its several subcommands without
having to laboriously select the right commits. Ideally, you'd simply
select the commits you care about and then pick specific actions you
want to do on them, with a nice visual graph that updates after the
operation. But that's a bit into the future, I guess :)
> > diff --git a/t/t3454-history-drop.sh b/t/t3454-history-drop.sh
> > new file mode 100755
> > index 0000000000..37d8413e7e
> > --- /dev/null
> > +++ b/t/t3454-history-drop.sh
> > @@ -0,0 +1,513 @@
> > +#!/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
>
> Why not `test_cmp` since it’s a fixed error?
>
> Same for a few other tests like `errors on unknown revision`.
We tend to use test_grep for cases like this, even for static error
messages. I haven't really figured out myself when exactly we tend to
use one form over the other, to be honest.
> >[snip]
> > +test_expect_success 'errors with invalid --empty= value' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + test_commit -C repo initial &&
> > + test_commit -C repo second &&
> > + test_must_fail git -C repo history drop --empty=bogus HEAD 2>err &&
> > + test_grep "unrecognized.*--empty.*bogus" err
> > +'
>
> Style related I guess. Most tests here use a subshell but this one uses
> `git -C`? Why is that?
No good reason, will fix.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v2 5/9] reset: introduce ability to skip reference updates
From: Patrick Steinhardt @ 2026-06-04 9:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Pablo Sabater
In-Reply-To: <xmqqqzmnqj1o.fsf@gitster.g>
On Thu, Jun 04, 2026 at 08:51:47AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > @@ -112,6 +113,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
> > if (opts->branch_msg && !opts->branch)
> > BUG("branch reflog message given without a branch");
> >
> > + if (skip_ref_updates && (opts->branch || refs_only))
> > + BUG("asked to perform ref updates and skip them at the same time");
>
> ;-) That's certainly a careful safety valve.
>
> Would we also want to catch skip_ref_updates && update_orig_head
> being both set as a bogus request?
Yeah, I think that's a good idea.
Patrick
^ permalink raw reply
* Re: git history feedback
From: Patrick Steinhardt @ 2026-06-04 8:39 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: git
In-Reply-To: <87ecimhg8s.fsf@rasmusvillemoes.dk>
On Thu, Jun 04, 2026 at 10:17:07AM +0200, Rasmus Villemoes wrote:
> Hi
>
> As soon as I saw the announcement of 'git history', I knew that was
> something I was gonna use a lot. Especially the split functionality has
> always been somewhat of a hassle (at least for me) to do via an
> interactive rebase. I've played around with it a little, and it seems to
> work as it says on the tin.
Happy to hear!
> So today I had occasion to put it to real use, and then I found two
> things I'd like to be able to do with it:
>
> When a commit needs to be split into three or more commits, it is a
> little cumbersome to do iteratively, since the new commit to split
> obviously has a new sha, so one first has to figure out what that new id
> is and then do another "git history split". For higher values of "three"
> that becomes rather tedious. So it would be nice if there was an
> iterative mode, which after splitting off the first commit would
> automatically start again with the new child commit.
That's fair, and also something that was discussed when initially
introducing the "split" subcommand. We don't have that mode right now,
but I think it would make sense to maybe add a new option that makes us
split repeatedly until all chunks have been exploded into separate
commits.
> If "git add -p" had an answer meaning "yes to this hunk and all
> following in this file and all remaining files as well", this could
> probably even be the default behaviour of "git history split", as it
> would just require that one extra answer to be given after the first
> commit is split off in order to keep the current behaviour. Otherwise,
> I'd also be happy to have "git history iter-split" or "git history split
> --iter" or any other spelling.
Yeah. From my perspective, having this available as an option would be
the best path forward. But I'm also open to alternatives as you suggest.
> The other thing I'd like is a sort of ultimate version of the above:
> What I needed in the concrete case at hand was actually to split two
> commit into n individual hunks each, then do an interactive rebase to combine
> those 2n commits to n commits (I had done changes "row-wise", but needed
> to change them to "column-wise"). For that, I would like to have had a
> completely automatic "git history atomize" that would split a commit
> into individual hunks, prefixing the commit subject with
> e.g. "[<filename> -- hunk #nn]". A subsequent 'git rebase -i' could then easily
> rearrange those and squash the related hunks.
It could easily be another option: `git history split --explode` for
example, which seems to be a common term for such an operation.
Regarding the subsequent rebase: I have one more patch series cooking
locally that implements `git history move` to move around commits, and I
did have the plan to eventually also implement `git history squash` to
squash commit A into commit B. But when handling many commits it might
even be easier to use interactive rebases instead.
Well, unless we eventually maybe even get something like a graphical
interface. I was playing around with a TUI interface for git-history(1)
that lets you move commits around without the hassle of the command
line. But that's certainly something that's still going to take a while
to materialize, if it ever does.
> Aside: are experimental commands eligible for teaching the completion
> logic about them? I.e., can we add a __git_history() to
> git-completion.bash? Aside from the obvious "let it know about existing
> subcommands", I'd love for "git history split <TAB>" to show the most
> recent ~20 (or something) commits in one-line format, stopping if
> there's a merge commit.
I just haven't gotten around to it yet. I'd certainly welcome the
addition of command line completion.
Thanks for your feedback!
Patrick
^ permalink raw reply
* Re: [PATCH 2/2] builtin/add: use die_for_required_opt() helper
From: Christian Couder @ 2026-06-04 8:27 UTC (permalink / raw)
To: Siddharth Shrimali; +Cc: git, gitster, toon, jn.avila
In-Reply-To: <20260603111044.39116-3-r.siddharth.shrimali@gmail.com>
On Wed, Jun 3, 2026 at 1:11 PM Siddharth Shrimali
<r.siddharth.shrimali@gmail.com> wrote:
>
> Clean up manual option dependency checks by replacing explicit conditional
> blocks with the newly introduced die_for_required_opt() helper function.
>
> Specifically, simplify the prerequisite check logic for both
> '--ignore-missing' (which requires '--dry-run') and '--pathspec-file-nul'
> (which requires '--pathspec-from-file').
It's a good idea to use the new helper function for
'--pathspec-file-nul' requiring '--pathspec-from-file' because it
looks like this is tested a lot already:
$ git grep requires | grep 'the option'
t2026-checkout-pathspec-file.sh: test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err
t2072-restore-pathspec-file.sh: test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err &&
t3601-rm-pathspec-file.sh: test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err &&
t3704-add-pathspec-file.sh: test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err &&
t3909-stash-pathspec-file.sh: test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err
t7107-reset-pathspec-file.sh: test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err &&
t7526-commit-pathspec-file.sh: test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err &&
You could mention this in the commit message.
Also it might be worth squashing this patch into the previous one.
Thanks.
^ 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