* [PATCH 3/7] hash: document function pointers and wrappers
From: Jeff King @ 2026-07-07 5:05 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
In-Reply-To: <20260707045556.GA1288172@coredump.intra.peff.net>
We want people to use the git_hash_*() wrappers rather than the bare
function pointers in the git_hash_algo struct. Let's document them
rather than the bare pointers, and warn people away from the pointers.
Coccinelle will eventually force the use of the wrappers, but it's
helpful to lead readers in the right direction from the start.
While we're here we can document a few other bits of wisdom I've turned
up while working in this area:
- You have to initialize the destination of a git_hash_clone(). This
is something we may eventually change for efficiency, but we should
definitely document the requirement for now.
- You must eventually finalize or discard a hash, since some backends
may allocate resources during initialization.
Signed-off-by: Jeff King <peff@peff.net>
---
hash.h | 43 ++++++++++++++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 11 deletions(-)
diff --git a/hash.h b/hash.h
index 0a23ef4dfd..5686914b71 100644
--- a/hash.h
+++ b/hash.h
@@ -309,22 +309,15 @@ struct git_hash_algo {
/* The block size of the hash. */
size_t blksz;
- /* The hash initialization function. */
+ /*
+ * Low-level implementation hooks. Callers should use the git_hash_*
+ * wrappers below rather than invoking these directly.
+ */
git_hash_init_fn init_fn;
-
- /* The hash context cloning function. */
git_hash_clone_fn clone_fn;
-
- /* The hash update function. */
git_hash_update_fn update_fn;
-
- /* The hash finalization function. */
git_hash_final_fn final_fn;
-
- /* The hash finalization function for object IDs. */
git_hash_final_oid_fn final_oid_fn;
-
- /* Discard an initialized hash without finalizing. */
git_hash_discard_fn discard_fn;
/* The OID of the empty tree. */
@@ -341,12 +334,40 @@ struct git_hash_algo {
};
extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
+/*
+ * Prepare an uninitialized hash context for use. You must eventually release
+ * the context with with git_hash_final() (or final_oid()) or by calling
+ * git_hash_discard().
+ */
void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop);
+
+/*
+ * Clone the state of a hash. Both src and dst must have been initialized with
+ * git_hash_init().
+ */
void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src);
+
+/*
+ * Add more data to an initialized hash context.
+ */
void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len);
+
+/*
+ * Retrieve the final hash value from a context, releasing any resources.
+ */
void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx);
+
+/*
+ * Like git_hash_final(), but write the result into an object_id.
+ */
void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx);
+
+/*
+ * Discard a hash context without computing the final value, but still
+ * releasing any resources.
+ */
void git_hash_discard(struct git_hash_ctx *ctx);
+
const struct git_hash_algo *hash_algo_ptr_by_number(uint32_t algo);
struct git_hash_ctx *git_hash_alloc(void);
void git_hash_free(struct git_hash_ctx *ctx);
--
2.55.0.459.g1b256877c9
^ permalink raw reply related
* [PATCH 2/7] hash: convert remaining direct function calls
From: Jeff King @ 2026-07-07 5:04 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
In-Reply-To: <20260707045556.GA1288172@coredump.intra.peff.net>
The previous patch added a coccinelle rule to make sure callers always
use git_hash_init() rather than direct function pointers from the algo
struct.
Let's do the same for the rest of the git_hash_*() wrappers. I split
these out because they're a bit different: they implicitly use the algop
pointer in the git_hash_ctx. So when we convert:
-algo->update_fn(&ctx, buf, len);
+git_hash_update(&ctx, buf, len);
we drop the reference to algo entirely! But this is always going to be
the right thing. If "algo" does not match what is in ctx.algop, then
we'd already be invoking undefined behavior.
So in addition to making it possible to add more logic to the
git_hash_*() functions, we're avoiding the need to pass around the extra
algo pointer and make sure that it matches what's in "ctx".
The rest of the patch is the mechanical application of that coccinelle
patch, plus a minor cleanup in test-synthesize.c to drop a now-unused
function parameter (since we don't have to pass around the algo
separately anymore).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/submodule--helper.c | 8 +++---
t/helper/test-synthesize.c | 29 ++++++++++----------
tools/coccinelle/hash.cocci | 53 +++++++++++++++++++++++++++++++++++++
3 files changed, 71 insertions(+), 19 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index bf114a7856..510f193a15 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -551,10 +551,10 @@ static void create_default_gitdir_config(const char *submodule_name)
/* Case 2.4: If all the above failed, try a hash of the name as a last resort */
header_len = snprintf(header, sizeof(header), "blob %zu", strlen(submodule_name));
git_hash_init(&ctx, the_hash_algo);
- the_hash_algo->update_fn(&ctx, header, header_len);
- the_hash_algo->update_fn(&ctx, "\0", 1);
- the_hash_algo->update_fn(&ctx, submodule_name, strlen(submodule_name));
- the_hash_algo->final_fn(raw_name_hash, &ctx);
+ git_hash_update(&ctx, header, header_len);
+ git_hash_update(&ctx, "\0", 1);
+ git_hash_update(&ctx, submodule_name, strlen(submodule_name));
+ git_hash_final(raw_name_hash, &ctx);
hash_to_hex_algop_r(hex_name_hash, raw_name_hash, the_hash_algo);
strbuf_reset(&gitdir_path);
repo_git_path_append(the_repository, &gitdir_path, "modules/%s", hex_name_hash);
diff --git a/t/helper/test-synthesize.c b/t/helper/test-synthesize.c
index 7719fb3a76..fd116c87ba 100644
--- a/t/helper/test-synthesize.c
+++ b/t/helper/test-synthesize.c
@@ -25,8 +25,7 @@ static const unsigned char zeros[BLOCK_SIZE];
* Updates the pack checksum context.
*/
static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
- const void *data, size_t len,
- const struct git_hash_algo *algo)
+ const void *data, size_t len)
{
unsigned char zlib_header[2] = { 0x78, 0x01 }; /* CMF, FLG */
unsigned char block_header[5];
@@ -37,7 +36,7 @@ static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
/* Write zlib header */
fwrite_or_die(f, zlib_header, sizeof(zlib_header));
- algo->update_fn(pack_ctx, zlib_header, 2);
+ git_hash_update(pack_ctx, zlib_header, 2);
/* Write uncompressed blocks (max 64KB each) */
do {
@@ -52,11 +51,11 @@ static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
block_header[4] = block_header[2] ^ 0xff;
fwrite_or_die(f, block_header, sizeof(block_header));
- algo->update_fn(pack_ctx, block_header, 5);
+ git_hash_update(pack_ctx, block_header, 5);
if (block_len) {
fwrite_or_die(f, block_data, block_len);
- algo->update_fn(pack_ctx, block_data, block_len);
+ git_hash_update(pack_ctx, block_data, block_len);
adler = adler32(adler, block_data, block_len);
}
@@ -68,7 +67,7 @@ static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
/* Write adler32 checksum */
put_be32(adler_buf, adler);
fwrite_or_die(f, adler_buf, sizeof(adler_buf));
- algo->update_fn(pack_ctx, adler_buf, 4);
+ git_hash_update(pack_ctx, adler_buf, 4);
}
/*
@@ -92,24 +91,24 @@ static void write_pack_object(FILE *f, struct git_hash_ctx *pack_ctx,
sizeof(pack_header),
type, len);
fwrite_or_die(f, pack_header, pack_header_len);
- algo->update_fn(pack_ctx, pack_header, pack_header_len);
+ git_hash_update(pack_ctx, pack_header, pack_header_len);
/* Write the data as uncompressed zlib */
- write_uncompressed_zlib(f, pack_ctx, data, len, algo);
+ write_uncompressed_zlib(f, pack_ctx, data, len);
git_hash_init(&ctx, algo);
object_header_len = format_object_header(object_header,
sizeof(object_header),
type, len);
- algo->update_fn(&ctx, object_header, object_header_len);
+ git_hash_update(&ctx, object_header, object_header_len);
if (data)
- algo->update_fn(&ctx, data, len);
+ git_hash_update(&ctx, data, len);
else {
for (size_t i = len / BLOCK_SIZE; i; i--)
- algo->update_fn(&ctx, zeros, BLOCK_SIZE);
- algo->update_fn(&ctx, zeros, len % BLOCK_SIZE);
+ git_hash_update(&ctx, zeros, BLOCK_SIZE);
+ git_hash_update(&ctx, zeros, len % BLOCK_SIZE);
}
- algo->final_oid_fn(oid, &ctx);
+ git_hash_final_oid(oid, &ctx);
}
/*
@@ -434,7 +433,7 @@ static int generate_pack_with_large_object(const char *path, size_t blob_size,
/* Write pack header */
fwrite_or_die(f, &pack_header, sizeof(pack_header));
- algo->update_fn(&pack_ctx, &pack_header, sizeof(pack_header));
+ git_hash_update(&pack_ctx, &pack_header, sizeof(pack_header));
/* 1. Write the large blob */
write_pack_object(f, &pack_ctx, OBJ_BLOB, NULL, blob_size, &blob_oid, algo);
@@ -472,7 +471,7 @@ static int generate_pack_with_large_object(const char *path, size_t blob_size,
write_pack_object(f, &pack_ctx, OBJ_COMMIT, buf.buf, buf.len, &final_commit_oid, algo);
/* Write pack trailer (checksum) */
- algo->final_fn(pack_hash, &pack_ctx);
+ git_hash_final(pack_hash, &pack_ctx);
fwrite_or_die(f, pack_hash, algo->rawsz);
if (fclose(f))
die_errno(_("could not close '%s'"), path);
diff --git a/tools/coccinelle/hash.cocci b/tools/coccinelle/hash.cocci
index 5a7af6c544..d0e2e5f4b1 100644
--- a/tools/coccinelle/hash.cocci
+++ b/tools/coccinelle/hash.cocci
@@ -8,3 +8,56 @@ struct git_hash_ctx *CTX;
+ git_hash_init(CTX, ALGO);
...>}
+@@
+identifier f != git_hash_clone;
+expression ALGO;
+struct git_hash_ctx *SRC;
+struct git_hash_ctx *DST;
+@@
+ f(...) {<...
+- ALGO->clone_fn(DST, SRC);
++ git_hash_clone(DST, SRC);
+ ...>}
+
+@@
+identifier f != git_hash_update;
+expression ALGO;
+struct git_hash_ctx *CTX;
+expression list ARGS;
+@@
+ f(...) {<...
+- ALGO->update_fn(CTX, ARGS);
++ git_hash_update(CTX, ARGS);
+ ...>}
+
+@@
+identifier f != git_hash_final;
+expression ALGO;
+struct git_hash_ctx *CTX;
+expression list ARGS;
+@@
+ f(...) {<...
+- ALGO->final_fn(ARGS, CTX);
++ git_hash_final(ARGS, CTX);
+ ...>}
+
+@@
+identifier f != git_hash_final_oid;
+expression ALGO;
+struct git_hash_ctx *CTX;
+expression list ARGS;
+@@
+ f(...) {<...
+- ALGO->final_oid_fn(ARGS, CTX);
++ git_hash_final_oid(ARGS, CTX);
+ ...>}
+
+@@
+identifier f != git_hash_discard;
+expression ALGO;
+struct git_hash_ctx *CTX;
+@@
+ f(...) {<...
+- ALGO->discard_fn(CTX);
++ git_hash_discard(CTX);
+ ...>}
--
2.55.0.459.g1b256877c9
^ permalink raw reply related
* [PATCH 1/7] hash: use git_hash_init() consistently
From: Jeff King @ 2026-07-07 5:01 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
In-Reply-To: <20260707045556.GA1288172@coredump.intra.peff.net>
We'd like to add more logic to git_hash_init(), but many callers skip it
and call algop->init_fn() directly. Let's make sure we're consistently
using the wrapper by adding a coccinelle rule.
Besides the coccinelle file itself, this is a purely mechanical
conversion based on the patch it generates. There should be no bare
init_fn() calls left (except for the one in the wrapper).
Signed-off-by: Jeff King <peff@peff.net>
---
It feels like the "expression ALGO" in the rule should be a
"git_hash_algo", but I had trouble getting coccinelle to recognize all
cases when I did that. Probably not worth digging too far into, as
the presence of the git_hash_ctx type means we should never hit any
false positives.
builtin/fast-import.c | 4 ++--
builtin/index-pack.c | 6 +++---
builtin/patch-id.c | 2 +-
builtin/receive-pack.c | 6 +++---
builtin/submodule--helper.c | 2 +-
builtin/unpack-objects.c | 4 ++--
csum-file.c | 6 +++---
diff.c | 4 ++--
http-push.c | 2 +-
http.c | 4 ++--
object-file.c | 17 +++++++++--------
pack-check.c | 2 +-
pack-write.c | 6 +++---
read-cache.c | 6 +++---
rerere.c | 5 +++--
t/helper/test-hash-speed.c | 2 +-
t/helper/test-hash.c | 2 +-
t/helper/test-synthesize.c | 4 ++--
t/unit-tests/u-hash.c | 2 +-
tools/coccinelle/hash.cocci | 10 ++++++++++
trace2/tr2_sid.c | 2 +-
21 files changed, 55 insertions(+), 43 deletions(-)
create mode 100644 tools/coccinelle/hash.cocci
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index f6473dcc8e..6692f7cd81 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -969,7 +969,7 @@ static int store_object(
hdrlen = format_object_header((char *)hdr, sizeof(hdr), type,
dat->len);
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, hdr, hdrlen);
git_hash_update(&c, dat->buf, dat->len);
git_hash_final_oid(&oid, &c);
@@ -1131,7 +1131,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
hdrlen = format_object_header((char *)out_buf, out_sz, OBJ_BLOB, len);
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, out_buf, hdrlen);
crc32_begin(pack_file);
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f396658468..53a8cb9dd7 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -374,7 +374,7 @@ static const char *open_pack_file(const char *pack_name)
output_fd = -1;
nothread_data.pack_fd = input_fd;
}
- the_hash_algo->init_fn(&input_ctx);
+ git_hash_init(&input_ctx, the_hash_algo);
return pack_name;
}
@@ -481,7 +481,7 @@ static void *unpack_entry_data(off_t offset, size_t size,
if (!is_delta_type(type)) {
hdrlen = format_object_header(hdr, sizeof(hdr), type, size);
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, hdr, hdrlen);
} else
oid = NULL;
@@ -1291,7 +1291,7 @@ static void parse_pack_objects(unsigned char *hash)
/* Check pack integrity */
flush();
- the_hash_algo->init_fn(&tmp_ctx);
+ git_hash_init(&tmp_ctx, the_hash_algo);
git_hash_clone(&tmp_ctx, &input_ctx);
git_hash_final(hash, &tmp_ctx);
if (!hasheq(fill(the_hash_algo->rawsz), hash, the_repository->hash_algo))
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 57d9bd4a65..22f36ecf80 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -73,7 +73,7 @@ static size_t get_one_patchid(struct object_id *next_oid, struct object_id *resu
char pre_oid_str[GIT_MAX_HEXSZ + 1], post_oid_str[GIT_MAX_HEXSZ + 1];
struct git_hash_ctx ctx;
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
oidclr(result, the_repository->hash_algo);
while (strbuf_getwholeline(line_buf, stdin, '\n') != EOF) {
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 19eb6a1b61..faf0f120ac 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -615,7 +615,7 @@ static void hmac_hash(unsigned char *out,
/* RFC 2104 2. (1) */
memset(key, '\0', GIT_MAX_BLKSZ);
if (the_hash_algo->blksz < key_len) {
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
git_hash_update(&ctx, key_in, key_len);
git_hash_final(key, &ctx);
} else {
@@ -629,13 +629,13 @@ static void hmac_hash(unsigned char *out,
}
/* RFC 2104 2. (3) & (4) */
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
git_hash_update(&ctx, k_ipad, sizeof(k_ipad));
git_hash_update(&ctx, text, text_len);
git_hash_final(out, &ctx);
/* RFC 2104 2. (6) & (7) */
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
git_hash_update(&ctx, k_opad, sizeof(k_opad));
git_hash_update(&ctx, out, the_hash_algo->rawsz);
git_hash_final(out, &ctx);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1cc82a134d..bf114a7856 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -550,7 +550,7 @@ static void create_default_gitdir_config(const char *submodule_name)
/* Case 2.4: If all the above failed, try a hash of the name as a last resort */
header_len = snprintf(header, sizeof(header), "blob %zu", strlen(submodule_name));
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
the_hash_algo->update_fn(&ctx, header, header_len);
the_hash_algo->update_fn(&ctx, "\0", 1);
the_hash_algo->update_fn(&ctx, submodule_name, strlen(submodule_name));
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index f3849bb654..93a9caa582 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -670,10 +670,10 @@ int cmd_unpack_objects(int argc,
/* We don't take any non-flag arguments now.. Maybe some day */
usage(unpack_usage);
}
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
unpack_all();
git_hash_update(&ctx, buffer, offset);
- the_hash_algo->init_fn(&tmp_ctx);
+ git_hash_init(&tmp_ctx, the_hash_algo);
git_hash_clone(&tmp_ctx, &ctx);
git_hash_final_oid(&oid, &tmp_ctx);
if (strict) {
diff --git a/csum-file.c b/csum-file.c
index b166f89624..7e81391524 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -175,7 +175,7 @@ struct hashfile *hashfd_ext(const struct git_hash_algo *algop,
f->skip_hash = 0;
f->algop = unsafe_hash_algo(algop);
- f->algop->init_fn(&f->ctx);
+ git_hash_init(&f->ctx, f->algop);
f->buffer_len = opts->buffer_len ? opts->buffer_len : DEFAULT_IO_BUFFER_SIZE;
f->buffer = xmalloc(f->buffer_len);
@@ -200,7 +200,7 @@ void hashfile_checkpoint_init(struct hashfile *f,
struct hashfile_checkpoint *checkpoint)
{
memset(checkpoint, 0, sizeof(*checkpoint));
- f->algop->init_fn(&checkpoint->ctx);
+ git_hash_init(&checkpoint->ctx, f->algop);
}
void hashfile_checkpoint(struct hashfile *f, struct hashfile_checkpoint *checkpoint)
@@ -252,7 +252,7 @@ int hashfile_checksum_valid(const struct git_hash_algo *algop,
if (total_len < algop->rawsz)
return 0; /* say "too short"? */
- algop->init_fn(&ctx);
+ git_hash_init(&ctx, algop);
git_hash_update(&ctx, data, data_len);
git_hash_final(got, &ctx);
diff --git a/diff.c b/diff.c
index 1568f0ed9c..589c1969e4 100644
--- a/diff.c
+++ b/diff.c
@@ -6855,7 +6855,7 @@ void flush_one_hunk(struct object_id *result, struct git_hash_ctx *ctx)
int i;
git_hash_final(hash, ctx);
- the_hash_algo->init_fn(ctx);
+ git_hash_init(ctx, the_hash_algo);
/* 20-byte sum, with carry */
for (i = 0; i < the_hash_algo->rawsz; ++i) {
carry += result->hash[i] + hash[i];
@@ -6899,7 +6899,7 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid
struct git_hash_ctx ctx;
struct patch_id_t data;
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
memset(&data, 0, sizeof(struct patch_id_t));
data.ctx = &ctx;
oidclr(oid, the_repository->hash_algo);
diff --git a/http-push.c b/http-push.c
index 3c23cbba27..60f6f8f054 100644
--- a/http-push.c
+++ b/http-push.c
@@ -776,7 +776,7 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)
} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {
lock->token = xstrdup(ctx->cdata);
- the_hash_algo->init_fn(&hash_ctx);
+ git_hash_init(&hash_ctx, the_hash_algo);
git_hash_update(&hash_ctx, lock->token, strlen(lock->token));
git_hash_final(lock_token_hash, &hash_ctx);
diff --git a/http.c b/http.c
index 63abbaae8a..0341de5031 100644
--- a/http.c
+++ b/http.c
@@ -2879,7 +2879,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
git_inflate_init(&freq->stream);
- the_hash_algo->init_fn(&freq->c);
+ git_hash_init(&freq->c, the_hash_algo);
freq->hash_ctx_valid = 1;
freq->url = get_remote_object_url(base_url, hex, 0);
@@ -2916,7 +2916,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
git_inflate_end(&freq->stream);
memset(&freq->stream, 0, sizeof(freq->stream));
git_inflate_init(&freq->stream);
- the_hash_algo->init_fn(&freq->c);
+ git_hash_init(&freq->c, the_hash_algo);
if (prev_posn>0) {
prev_posn = 0;
lseek(freq->localfile, 0, SEEK_SET);
diff --git a/object-file.c b/object-file.c
index e3c68cfb66..f292683c2d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -124,7 +124,7 @@ int stream_object_signature(struct repository *r,
hdrlen = format_object_header(hdr, sizeof(hdr), st->type, st->size);
/* Sha1.. */
- r->hash_algo->init_fn(&c);
+ git_hash_init(&c, r->hash_algo);
git_hash_update(&c, hdr, hdrlen);
for (;;) {
char buf[1024 * 16];
@@ -320,7 +320,7 @@ static void hash_object_body(const struct git_hash_algo *algo, struct git_hash_c
struct object_id *oid,
char *hdr, size_t *hdrlen)
{
- algo->init_fn(c);
+ git_hash_init(c, algo);
git_hash_update(c, hdr, *hdrlen);
git_hash_update(c, buf, len);
git_hash_final_oid(oid, c);
@@ -681,9 +681,10 @@ static int start_loose_object_common(struct odb_source_loose *loose,
git_deflate_init(stream, cfg->zlib_compression_level);
stream->next_out = buf;
stream->avail_out = buflen;
- algo->init_fn(c);
- if (compat && compat_c)
- compat->init_fn(compat_c);
+ git_hash_init(c, algo);
+ if (compat && compat_c) {
+ git_hash_init(compat_c, compat);
+ }
/* Start to feed header to zlib stream */
stream->next_in = (unsigned char *)hdr;
@@ -1141,7 +1142,7 @@ static int hash_blob_stream(struct odb_write_stream *stream,
header_len = format_object_header((char *)buf, sizeof(buf),
OBJ_BLOB, size);
- hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, hash_algo);
git_hash_update(&ctx, buf, header_len);
while (!stream->is_finished) {
@@ -1313,7 +1314,7 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
header_len = format_object_header((char *)obuf, sizeof(obuf),
OBJ_BLOB, size);
- transaction->base.source->odb->repo->hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, transaction->base.source->odb->repo->hash_algo);
git_hash_update(&ctx, obuf, header_len);
/*
@@ -1560,7 +1561,7 @@ static int check_stream_oid(git_zstream *stream,
unsigned long total_read;
int status = Z_OK;
- algop->init_fn(&c);
+ git_hash_init(&c, algop);
git_hash_update(&c, hdr, stream->total_out);
/*
diff --git a/pack-check.c b/pack-check.c
index 5adfb3f272..c3b8db7c5c 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -69,7 +69,7 @@ static int verify_packfile(struct repository *r,
if (!is_pack_valid(p))
return error("packfile %s cannot be accessed", p->pack_name);
- r->hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, r->hash_algo);
do {
unsigned long remaining;
unsigned char *in = use_pack(p, w_curs, offset, &remaining);
diff --git a/pack-write.c b/pack-write.c
index 83eaf88541..24033a9101 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -402,8 +402,8 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
char *buf;
ssize_t read_result;
- hash_algo->init_fn(&old_hash_ctx);
- hash_algo->init_fn(&new_hash_ctx);
+ git_hash_init(&old_hash_ctx, hash_algo);
+ git_hash_init(&new_hash_ctx, hash_algo);
if (lseek(pack_fd, 0, SEEK_SET) != 0)
die_errno("Failed seeking to start of '%s'", pack_name);
@@ -455,7 +455,7 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
* pack, which also means making partial_pack_offset
* big enough not to matter anymore.
*/
- hash_algo->init_fn(&old_hash_ctx);
+ git_hash_init(&old_hash_ctx, hash_algo);
partial_pack_offset = ~partial_pack_offset;
partial_pack_offset -= MSB(partial_pack_offset, 1);
}
diff --git a/read-cache.c b/read-cache.c
index 7c1cdcf696..5fa747e6fc 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1722,7 +1722,7 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)
if (oideq(&oid, null_oid(the_hash_algo)))
return 0;
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, hdr, size - the_hash_algo->rawsz);
git_hash_final(hash, &c);
if (!hasheq(hash, start, the_repository->hash_algo))
@@ -2957,7 +2957,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
*/
if (offset && record_eoie()) {
CALLOC_ARRAY(eoie_c, 1);
- the_hash_algo->init_fn(eoie_c);
+ git_hash_init(eoie_c, the_hash_algo);
}
/*
@@ -3598,7 +3598,7 @@ static size_t read_eoie_extension(const char *mmap, size_t mmap_size)
* "REUC" + <binary representation of M>)
*/
src_offset = offset;
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
while (src_offset < mmap_size - the_hash_algo->rawsz - EOIE_SIZE_WITH_HEADER) {
/* After an array of active_nr index entries,
* there can be arbitrary number of extended
diff --git a/rerere.c b/rerere.c
index 8232542585..2e932439a4 100644
--- a/rerere.c
+++ b/rerere.c
@@ -438,8 +438,9 @@ static int handle_path(unsigned char *hash, struct rerere_io *io, int marker_siz
struct git_hash_ctx ctx;
struct strbuf buf = STRBUF_INIT, out = STRBUF_INIT;
int has_conflicts = 0;
- if (hash)
- the_hash_algo->init_fn(&ctx);
+ if (hash) {
+ git_hash_init(&ctx, the_hash_algo);
+ }
while (!io->getline(&buf, io)) {
if (is_cmarker(buf.buf, '<', marker_size)) {
diff --git a/t/helper/test-hash-speed.c b/t/helper/test-hash-speed.c
index fbf67fe6bd..89b0268011 100644
--- a/t/helper/test-hash-speed.c
+++ b/t/helper/test-hash-speed.c
@@ -5,7 +5,7 @@
static inline void compute_hash(const struct git_hash_algo *algo, struct git_hash_ctx *ctx, uint8_t *final, const void *p, size_t len)
{
- algo->init_fn(ctx);
+ git_hash_init(ctx, algo);
git_hash_update(ctx, p, len);
git_hash_final(final, ctx);
}
diff --git a/t/helper/test-hash.c b/t/helper/test-hash.c
index f0ee61c8b4..1f7163695f 100644
--- a/t/helper/test-hash.c
+++ b/t/helper/test-hash.c
@@ -29,7 +29,7 @@ int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)
die("OOPS");
}
- algop->init_fn(&ctx);
+ git_hash_init(&ctx, algop);
while (1) {
ssize_t sz, this_sz;
diff --git a/t/helper/test-synthesize.c b/t/helper/test-synthesize.c
index 3fa534fbdf..7719fb3a76 100644
--- a/t/helper/test-synthesize.c
+++ b/t/helper/test-synthesize.c
@@ -97,7 +97,7 @@ static void write_pack_object(FILE *f, struct git_hash_ctx *pack_ctx,
/* Write the data as uncompressed zlib */
write_uncompressed_zlib(f, pack_ctx, data, len, algo);
- algo->init_fn(&ctx);
+ git_hash_init(&ctx, algo);
object_header_len = format_object_header(object_header,
sizeof(object_header),
type, len);
@@ -430,7 +430,7 @@ static int generate_pack_with_large_object(const char *path, size_t blob_size,
f = xfopen(path, "wb");
- algo->init_fn(&pack_ctx);
+ git_hash_init(&pack_ctx, algo);
/* Write pack header */
fwrite_or_die(f, &pack_header, sizeof(pack_header));
diff --git a/t/unit-tests/u-hash.c b/t/unit-tests/u-hash.c
index bd4ac6a6e1..19f4efd410 100644
--- a/t/unit-tests/u-hash.c
+++ b/t/unit-tests/u-hash.c
@@ -12,7 +12,7 @@ static void check_hash_data(const void *data, size_t data_length,
unsigned char hash[GIT_MAX_HEXSZ];
const struct git_hash_algo *algop = &hash_algos[i];
- algop->init_fn(&ctx);
+ git_hash_init(&ctx, algop);
git_hash_update(&ctx, data, data_length);
git_hash_final(hash, &ctx);
diff --git a/tools/coccinelle/hash.cocci b/tools/coccinelle/hash.cocci
new file mode 100644
index 0000000000..5a7af6c544
--- /dev/null
+++ b/tools/coccinelle/hash.cocci
@@ -0,0 +1,10 @@
+@@
+identifier f != git_hash_init;
+expression ALGO;
+struct git_hash_ctx *CTX;
+@@
+ f(...) {<...
+- ALGO->init_fn(CTX);
++ git_hash_init(CTX, ALGO);
+ ...>}
+
diff --git a/trace2/tr2_sid.c b/trace2/tr2_sid.c
index 1c1d27b0ee..131b4f5a62 100644
--- a/trace2/tr2_sid.c
+++ b/trace2/tr2_sid.c
@@ -45,7 +45,7 @@ static void tr2_sid_append_my_sid_component(void)
if (xgethostname(hostname, sizeof(hostname)))
strbuf_add(&tr2sid_buf, "Localhost", 9);
else {
- algo->init_fn(&ctx);
+ git_hash_init(&ctx, algo);
git_hash_update(&ctx, hostname, strlen(hostname));
git_hash_final(hash, &ctx);
hash_to_hex_algop_r(hex, hash, algo);
--
2.55.0.459.g1b256877c9
^ permalink raw reply related
* [PATCH 0/7] git_hash_*() quality-of-life improvements
From: Jeff King @ 2026-07-07 4:55 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
This implements the "idempotent git_hash_discard()" discussed in this
subthread:
https://lore.kernel.org/git/20260702080707.GG2029434@coredump.intra.peff.net/
with associated cleanups.
It should be applied on top of jk/hash-algo-leak-fixes.
[1/7]: hash: use git_hash_init() consistently
[2/7]: hash: convert remaining direct function calls
[3/7]: hash: document function pointers and wrappers
[4/7]: hash: make git_hash_discard() idempotent
[5/7]: csum-file: use idempotent git_hash_discard()
[6/7]: http: use idempotent git_hash_discard()
[7/7]: hash: check ctx->active flag in all wrapper functions
builtin/fast-import.c | 4 +--
builtin/index-pack.c | 6 ++--
builtin/patch-id.c | 2 +-
builtin/receive-pack.c | 6 ++--
builtin/submodule--helper.c | 10 +++---
builtin/unpack-objects.c | 4 +--
csum-file.c | 23 +++++---------
diff.c | 4 +--
hash.c | 16 ++++++++++
hash.h | 44 +++++++++++++++++++-------
http-push.c | 2 +-
http.c | 9 ++----
http.h | 1 -
object-file.c | 17 +++++-----
pack-check.c | 2 +-
pack-write.c | 6 ++--
read-cache.c | 6 ++--
rerere.c | 5 +--
t/helper/test-hash-speed.c | 2 +-
t/helper/test-hash.c | 2 +-
t/helper/test-synthesize.c | 33 ++++++++++---------
t/unit-tests/u-hash.c | 2 +-
tools/coccinelle/hash.cocci | 63 +++++++++++++++++++++++++++++++++++++
trace2/tr2_sid.c | 2 +-
24 files changed, 181 insertions(+), 90 deletions(-)
create mode 100644 tools/coccinelle/hash.cocci
-Peff
^ permalink raw reply
* Re: [PATCH] meson: wire up USE_NSEC build knob
From: Jeff King @ 2026-07-07 4:38 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: D. Ben Knoble, git, brian m . carlson, Junio C Hamano,
Ramsay Jones
In-Reply-To: <aktOn-3K41Uhl9cr@pks.im>
On Mon, Jul 06, 2026 at 08:43:43AM +0200, Patrick Steinhardt wrote:
> > To summarize: If we're all leaning in the direction of a run-time flag
> > instead, I can noodle in that direction. That certainly involves a bit
> > more surgery than just giving Meson access to the option, but the
> > dynamism may be nice. I'm not too sure how we'd write a test case for
> > it, though.
>
> I don't think we'd necessarily need a way to detect this. Our current
> build default is to have this disabled, so I'd keep it this way, but
> automatically compile nsec-support into Git if available. And then we
> provide a way for users to opt-in to the new behaviour via the config.
Yeah, agreed. Even if we eventually auto-detect, the first step is
adding the config at all. And then we can decide whether to stop there
or not.
I'm agnostic on whether we add USE_NSEC to meson in the meantime, if it
might eventually be ripped out of the Makefile. We _could_ retain
USE_NSEC to change the unconfigured default for a given build, but I'd
be inclined to just remove it entirely once the runtime config is
available.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/9] t: fixes and improvements for GIT_TEST_LONG
From: Jeff King @ 2026-07-07 4:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, git, Johannes Schindelin, SZEDER Gábor
In-Reply-To: <xmqq8q7namkm.fsf@gitster.g>
On Mon, Jul 06, 2026 at 01:29:45PM -0700, Junio C Hamano wrote:
> > Changes in v3:
> > - Fix commit subjects to mention correct prerequisite.
> > - Link to v2: https://patch.msgid.link/20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im
>
> The interdiff looks trivially correct ;-).
>
> Hopefully we are now ready to declare victory and plan to merge this
> to 'next'?
Yeah. I wouldn't say I did a super-deep review, but I did look over the
original and have no complaints.
-Peff
^ permalink raw reply
* Re: [PATCH] rebase -i: introduce `pick -x` to add "cherry picked from commit ..."
From: Jeff King @ 2026-07-07 4:27 UTC (permalink / raw)
To: Phillip Wood
Cc: Trevor Gross, git, Junio C Hamano, Stefan Haller, Derrick Stolee,
Phillip Wood
In-Reply-To: <5d238e0d-18ba-429a-a9a4-a3988b00e1e1@gmail.com>
On Mon, Jul 06, 2026 at 11:08:18AM +0100, Phillip Wood wrote:
> > To be clear, I don't know the answer. It's been ages since I've looked
> > at sequencer code, so there might be more gotchas. That's just my gut
> > feeling from a high level after reading your message.
>
> I don't think it would be much work. The code that edits the todo list is
> rebase specific because it deals with rebase.missingCommitsCheck but it
> shouldn't be too difficult to generalize it. I do wonder though if it makes
> sense to support all of the usual commands when cherry-picking especially
> with `-x`. In particular I'm not sure about adding support for `edit -x`, or
> for `pick -x` followed by `fixup` - what does the trailer mean when the
> commit has been edited or fixed up? (though if you're back-porting bug fixes
> I guess some degree of editing is inevitable)
I'd probably err on the side of assuming the user knows what they're
doing, and will mention any edits in the commit message as appropriate.
Maybe that's being too optimistic. :)
> On a slight tangent I've sometimes wanted to be able to do
>
> git cherry-pick --exec 'make test' some commits
Yeah, though in that case I'd usually cherry-pick and then just do an
in-place "rebase -x 'make test'". You could really do _almost_ any
cherry-pick sequencer operation like that, which is perhaps why we
haven't see a huge number of requests for it.
This "-x" thing is special because it's inherently about looking at the
original commit id, as opposed to fiddling with our rebased version. But
I guess you could "cherry-pick -x" and then rebase (doing whatever
rearranging and markup you wanted) the result.
-Peff
^ permalink raw reply
* Re: [PATCH] t0213: skip ancestry tests under user-mode emulation
From: Junio C Hamano @ 2026-07-07 2:30 UTC (permalink / raw)
To: Jamie Magee via GitGitGadget; +Cc: git, Jamie Magee
In-Reply-To: <pull.2168.git.1783359242130.gitgitgadget@gmail.com>
"Jamie Magee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> -# Determine if cmd_ancestry is supported on this platform.
> +# Enable these tests only when cmd_ancestry reports real process names.
> +# The procinfo stub emits no event; under user-mode emulation (e.g.
> +# qemu-user) /proc reports the emulator, not the guest. Spawn test-tool
> +# from test-tool and require "test-tool" in the child's ancestry.
T.r.i.c.k.y. ;-)
> test_expect_success 'detect cmd_ancestry support' '
> test_when_finished "rm -f trace.detect" &&
> GIT_TRACE2_BRIEF=1 GIT_TRACE2="$(pwd)/trace.detect" \
> - test-tool trace2 001return 0 &&
> - if grep -q "^cmd_ancestry" trace.detect
> + test-tool trace2 004child test-tool trace2 001return 0 &&
> + if grep -q "^cmd_ancestry.*test-tool" trace.detect
This will be happy even if "test-tool-trash" that happens to have
"test-tool" as its prefix appears on a cmd_ancestry line (for that
matter, things like "cmd_ancestry-not-quite" that has "cmd_ancestry"
as its prefix would be accepted). I guess that is OK because we are
testing this in a fairly tightly controlled environment (trace keys
are taken from known vocabulary, not arbitrary strings, for example).
Will queue. Thanks.
> then
> test_set_prereq TRACE2_ANCESTRY
> fi
>
> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
^ permalink raw reply
* Re: [PATCH] t1410-reflog.sh: avoid suppressing git's exit code in pipelines
From: Junio C Hamano @ 2026-07-07 2:16 UTC (permalink / raw)
To: Gatla Vishweshwar Reddy; +Cc: git
In-Reply-To: <20260706205036.3453-1-gatlavishweshwarreddy26@gmail.com>
Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:
> Piping git commands directly to wc -l suppresses the exit code of
> git, hiding potential failures from the test suite. Capture the
> output to a temporary file first, then count the lines separately
> to preserve the exit code.
>
> Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
> ---
> t/t1410-reflog.sh | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index ce71f9a30a..397f94b039 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -244,8 +244,10 @@ test_expect_success 'delete' '
> test_tick &&
> git commit -m tiger C &&
>
> - HEAD_entry_count=$(git reflog | wc -l) &&
> - main_entry_count=$(git reflog show main | wc -l) &&
> + git reflog >reflog_output &&
> + HEAD_entry_count=$(wc -l <reflog_output) &&
> + git reflog show main >reflog_main_output &&
> + main_entry_count=$(wc -l <reflog_main_output) &&
>
> test $HEAD_entry_count = 5 &&
> test $main_entry_count = 5 &&
If you _know_ output from certain command must be 5 lines, would it
make more sense to use test_stdout_line_count, perhaps like
test_stdout_line_count = 5 git reflog
or something?
> @@ -254,16 +256,23 @@ test_expect_success 'delete' '
> git reflog delete main@{1} &&
> git reflog show main > output &&
> test_line_count = $(($main_entry_count - 1)) output &&
> - test $HEAD_entry_count = $(git reflog | wc -l) &&
> + git reflog >reflog_output &&
> + test $HEAD_entry_count = $(wc -l <reflog_output) &&
> ! grep ox < output &&
>
> main_entry_count=$(wc -l < output) &&
>
> git reflog delete HEAD@{1} &&
> - test $(($HEAD_entry_count -1)) = $(git reflog | wc -l) &&
> - test $main_entry_count = $(git reflog show main | wc -l) &&
> + git reflog >reflog_output &&
> + test $(($HEAD_entry_count -1)) = $(wc -l <reflog_output) &&
> + git reflog show main >reflog_main_output &&
> + test $main_entry_count = $(wc -l <reflog_main_output) &&
> +
> +
> + git reflog >reflog_output &&
> + HEAD_entry_count=$(wc -l <reflog_output) &&
> +
>
> - HEAD_entry_count=$(git reflog | wc -l) &&
>
> git reflog delete main@{07.04.2005.15:15:00.-0700} &&
Can you explain the addition of these consecutive blank lines? The
same question applies to the blank lines at the end of the test in
the next hunk. I ask because formatting issues like this often
resemble unedited AI-generated code that hasn't been properly
cleaned up before submission.
> git reflog show main > output &&
> @@ -321,11 +330,15 @@ test_expect_success 'git reflog expire unknown reference' '
> '
>
> test_expect_success 'checkout should not delete log for packed ref' '
> - test $(git reflog main | wc -l) = 4 &&
> + git reflog main >reflog_output &&
> + test $(wc -l <reflog_output) = 4 &&
> git branch foo &&
> git pack-refs --all &&
> git checkout foo &&
> - test $(git reflog main | wc -l) = 4
> + git reflog main >reflog_output &&
> + test $(wc -l <reflog_output) = 4
> +
> +
> '
>
> test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' '
^ permalink raw reply
* [PATCH v6 3/3] contrib: wire up osxkeychain in contrib/Makefile on macOS
From: Shardul Natu via GitGitGadget @ 2026-07-06 22:52 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble, Shardul Natu
In-Reply-To: <pull.2288.v6.git.git.1783378333.gitgitgadget@gmail.com>
From: Shardul Natu <snatu@google.com>
When running "make test" with TEST_CONTRIB_TOO=yes (which is default in
macOS CI workflows), $(MAKE) -C contrib/ test is invoked. However,
contrib/Makefile only invoked tests for diff-highlight and subtree,
meaning git-credential-osxkeychain was never built or verified during
standard CI test runs.
Add a "test" target to contrib/credential/osxkeychain/Makefile that
depends on building git-credential-osxkeychain. Additionally, wire up
credential/osxkeychain in contrib/Makefile under "all", "test", and
"clean" whenever running on macOS (Darwin).
This ensures that running "make test" or "make all" in contrib on macOS
automatically builds and links git-credential-osxkeychain, preventing
future build or symbol linking regressions from slipping through CI.
Signed-off-by: Shardul Natu <snatu@google.com>
---
contrib/Makefile | 10 ++++++++++
contrib/credential/osxkeychain/Makefile | 4 +++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/contrib/Makefile b/contrib/Makefile
index 787cd07f52..7962a9ff12 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -1,10 +1,20 @@
+-include ../config.mak.autogen
+-include ../config.mak
+
+ifeq ($(uname_S),Darwin)
+OS_CONTRIB += credential/osxkeychain
+endif
+
all::
+ $(foreach dir,$(OS_CONTRIB),$(MAKE) -C $(dir) $@;)
test::
$(MAKE) -C diff-highlight $@
$(MAKE) -C subtree $@
+ $(foreach dir,$(OS_CONTRIB),$(MAKE) -C $(dir) $@;)
clean::
$(MAKE) -C contacts $@
$(MAKE) -C diff-highlight $@
$(MAKE) -C subtree $@
+ $(foreach dir,$(OS_CONTRIB),$(MAKE) -C $(dir) $@;)
diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile
index 219b0d7f49..d9fba07e8d 100644
--- a/contrib/credential/osxkeychain/Makefile
+++ b/contrib/credential/osxkeychain/Makefile
@@ -10,4 +10,6 @@ install:
clean:
$(MAKE) -C ../../.. clean-git-credential-osxkeychain
-.PHONY: all git-credential-osxkeychain install clean
+test: git-credential-osxkeychain
+
+.PHONY: all git-credential-osxkeychain install clean test
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 2/3] Makefile: support universal macOS builds via RUST_TARGETS
From: Shardul Natu via GitGitGadget @ 2026-07-06 22:52 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble, Shardul Natu
In-Reply-To: <pull.2288.v6.git.git.1783378333.gitgitgadget@gmail.com>
From: Shardul Natu <snatu@google.com>
On macOS, Universal Binaries contain native executable code for
multiple architectures (such as Intel x86_64 and Apple Silicon arm64)
bundled into a single file. This is standard practice for macOS
distribution and CI packaging (such as internal distribution packages
or tooling like Burrito/Homebrew), allowing a single build artifact
to run natively across all Macs without Rosetta emulation or
maintaining separate packages.
When building Git C code for multiple architectures on macOS, the
Apple toolchain (clang) natively supports universal builds via
CFLAGS/LDFLAGS. When "-arch x86_64 -arch arm64" is passed, clang
automatically compiles and links universal binaries for all C object
files and executables out of the box.
Cargo and rustc, however, do not support multiple "-arch" flags or
emitting universal binaries in a single invocation. Instead, Cargo
requires invoking each target triple independently (e.g., passing
"--target x86_64-apple-darwin" and "--target aarch64-apple-darwin").
To bridge this gap when Rust is enabled:
1. Allow specifying space-separated target triples in RUST_TARGETS.
2. Introduce declarative pattern rules (target/%/...) to compile
each target-specific library slice via Cargo.
3. On macOS, if multiple targets are specified, use "lipo" (part of
the mandatory Xcode Command Line Tools) to combine the resulting
static libraries into target/release/libgitcore.a.
Once $(RUST_LIB) is compiled into a universal static archive, the
standard C linker seamlessly links it with the C object files to
produce universal Git executables.
Signed-off-by: Shardul Natu <snatu@google.com>
---
Makefile | 39 +++++++++++++++++++++++++++++++++++----
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/Makefile b/Makefile
index 7db38ecce9..9921af992b 100644
--- a/Makefile
+++ b/Makefile
@@ -500,6 +500,14 @@ include shared.mak
#
# Building Rust code requires Cargo.
#
+# Define RUST_TARGETS if you want to cross-compile. If left unspecified, it uses
+# the default Rust target on the system.
+#
+# On macOS, this supports specifying multiple targets, separated by a space.
+# This will produce a Universal static library using `lipo`.
+#
+# Example: RUST_TARGETS="aarch64-apple-darwin x86_64-apple-darwin"
+#
# == SHA-1 and SHA-256 defines ==
#
# === SHA-1 backend ===
@@ -941,16 +949,17 @@ LIB_FILE = libgit.a
ifndef NO_RUST
ifdef DEBUG
-RUST_TARGET_DIR = target/debug
+RUST_BUILD_CONFIG = debug
else
-RUST_TARGET_DIR = target/release
+RUST_BUILD_CONFIG = release
endif
ifeq ($(uname_S),Windows)
-RUST_LIB = $(RUST_TARGET_DIR)/gitcore.lib
+RUST_LIB_NAME = gitcore.lib
else
-RUST_LIB = $(RUST_TARGET_DIR)/libgitcore.a
+RUST_LIB_NAME = libgitcore.a
endif
+RUST_LIB = target/$(RUST_BUILD_CONFIG)/$(RUST_LIB_NAME)
endif
GITLIBS = common-main.o $(LIB_FILE)
@@ -3022,8 +3031,30 @@ $(LIB_FILE): $(LIB_OBJS)
$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
ifndef NO_RUST
+ifeq ($(RUST_TARGETS),)
$(RUST_LIB): Cargo.toml $(RUST_SOURCES) $(LIB_FILE)
$(QUIET_CARGO)cargo build $(CARGO_ARGS)
+else
+ifneq ($(words $(RUST_TARGETS)),1)
+ifneq ($(uname_S),Darwin)
+$(error Building universal Rust libraries requires macOS (lipo is not available on $(uname_S)))
+endif
+endif
+
+RUST_MEMBER_LIBS = $(foreach target,$(RUST_TARGETS),target/$(target)/$(RUST_BUILD_CONFIG)/$(RUST_LIB_NAME))
+$(RUST_MEMBER_LIBS): target/%/$(RUST_BUILD_CONFIG)/$(RUST_LIB_NAME): Cargo.toml $(RUST_SOURCES) $(LIB_FILE)
+ $(QUIET_CARGO)cargo build $(CARGO_ARGS) --target $*
+
+$(RUST_LIB): $(RUST_MEMBER_LIBS)
+ $(call mkdir_p_parent_template)
+ $(QUIET_GEN)\
+ if test $(words $(RUST_TARGETS)) -gt 1; \
+ then \
+ lipo -create $^ -output $@; \
+ else \
+ cp $< $@; \
+ fi
+endif
.PHONY: rust
rust: $(RUST_LIB)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 1/3] Makefile: add $(RUST_LIB) prerequisite to osxkeychain
From: Shardul Natu via GitGitGadget @ 2026-07-06 22:52 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble, Shardul Natu
In-Reply-To: <pull.2288.v6.git.git.1783378333.gitgitgadget@gmail.com>
From: Shardul Natu <snatu@google.com>
When Rust is enabled, the git-credential-osxkeychain helper depends on
Rust symbols compiled into $(RUST_LIB). While commit 522ea8ef7d
("osxkeychain: fix build with Rust") updated the linker command line to
use $(LIBS), it omitted $(RUST_LIB) from the target prerequisite list.
Without this prerequisite, running a parallel build ("make -j") from a
clean working tree can fail because Make does not know to invoke Cargo
to build libgitcore.a before linking git-credential-osxkeychain.
Note that we depend explicitly on $(LIB_FILE) and $(RUST_LIB) rather
than $(GITLIBS). Unlike standard Git builtins and programs like scalar
(which define cmd_main() and rely on common-main.o to supply main()),
git-credential-osxkeychain.c defines its own standalone int main().
If $(GITLIBS) were used, $(filter %.o,$^) in the link recipe would
match both git-credential-osxkeychain.o and common-main.o, causing a
duplicate symbol linking error for _main on macOS.
Additionally, wrap the definitions of $(RUST_LIB) and the "rust" build
target in "ifndef NO_RUST". This ensures that when NO_RUST=1 is
specified, $(RUST_LIB) evaluates to empty, making the Rust dependency a
clean no-op without needing intermediate variables.
Signed-off-by: Shardul Natu <snatu@google.com>
---
Makefile | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 1f3f099f5c..7db38ecce9 100644
--- a/Makefile
+++ b/Makefile
@@ -939,6 +939,7 @@ TEST_SHELL_PATH = $(SHELL_PATH)
LIB_FILE = libgit.a
+ifndef NO_RUST
ifdef DEBUG
RUST_TARGET_DIR = target/debug
else
@@ -950,6 +951,7 @@ RUST_LIB = $(RUST_TARGET_DIR)/gitcore.lib
else
RUST_LIB = $(RUST_TARGET_DIR)/libgitcore.a
endif
+endif
GITLIBS = common-main.o $(LIB_FILE)
EXTLIBS =
@@ -3019,11 +3021,13 @@ scalar$X: scalar.o GIT-LDFLAGS $(GITLIBS)
$(LIB_FILE): $(LIB_OBJS)
$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
+ifndef NO_RUST
$(RUST_LIB): Cargo.toml $(RUST_SOURCES) $(LIB_FILE)
$(QUIET_CARGO)cargo build $(CARGO_ARGS)
.PHONY: rust
rust: $(RUST_LIB)
+endif
export DEFAULT_EDITOR DEFAULT_PAGER
@@ -4074,7 +4078,8 @@ $(LIBGIT_HIDDEN_EXPORT): $(LIBGIT_PARTIAL_EXPORT)
contrib/libgit-sys/libgitpub.a: $(LIBGIT_HIDDEN_EXPORT)
$(AR) $(ARFLAGS) $@ $^
-contrib/credential/osxkeychain/git-credential-osxkeychain: contrib/credential/osxkeychain/git-credential-osxkeychain.o $(LIB_FILE) GIT-LDFLAGS
+# When Rust is enabled, git-credential-osxkeychain depends on Rust symbols in $(RUST_LIB)
+contrib/credential/osxkeychain/git-credential-osxkeychain: contrib/credential/osxkeychain/git-credential-osxkeychain.o $(LIB_FILE) $(RUST_LIB) GIT-LDFLAGS
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS) -framework Security -framework CoreFoundation
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 0/3] Makefile: link osxkeychain helper against Rust
From: Shardul Natu via GitGitGadget @ 2026-07-06 22:52 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble
In-Reply-To: <pull.2288.v5.git.git.1783358097.gitgitgadget@gmail.com>
This series improves macOS build reliability, automated CI verification, and
distribution support when Rust is enabled in the Git build system. It
addresses three distinct challenges: a parallel build race condition in
git-credential-osxkeychain, support for macOS Universal Binaries
(multi-architecture distribution), and missing automated CI test wiring for
macOS contrib utilities.
Why This Series is Needed
=========================
1. Parallel Build Race Condition (make -j): While commit 522ea8ef7d
("osxkeychain: fix build with Rust") updated the link command for
git-credential-osxkeychain to pass $(LIBS), it omitted $(RUST_LIB) from
the target prerequisite list. When running a parallel build (make -j)
from a clean working tree, Make can attempt to link
git-credential-osxkeychain before Cargo has finished compiling
libgitcore.a, causing linker failures.
2. macOS Universal Binary (lipo) Support: On macOS, Universal Binaries
bundle native executable code for multiple architectures (Intel x86_64
and Apple Silicon arm64) into a single file. This is standard practice
for macOS distribution and CI packaging (such as Burrito, Homebrew, and
Git's macOS CI runners), allowing a single artifact to run natively
across all Macs without Rosetta translation.
While Apple's C compiler (clang) natively supports universal builds by
passing -arch x86_64 -arch arm64 in CFLAGS and LDFLAGS, Cargo and rustc do
not support multiple -arch flags in a single invocation. Instead, Cargo must
be invoked separately for each target triple (--target x86_64-apple-darwin
and --target aarch64-apple-darwin). This series bridges that gap.
3. Automated CI Verification for Contrib on macOS: When running make test
with TEST_CONTRIB_TOO=yes (default in macOS CI workflows), $(MAKE) -C
contrib/ test is invoked. However, contrib/Makefile only invoked tests
for diff-highlight and subtree, meaning git-credential-osxkeychain was
never compiled or verified during standard CI test runs.
Overview of Patches
===================
* Patch 1: Makefile: add $(RUST_LIB) prerequisite to osxkeychain Adds
$(RUST_LIB) as a prerequisite dependency to the osxkeychain target,
eliminating the parallel build race condition. Additionally, wraps the
definitions of $(RUST_LIB) and the rust build target in ifndef NO_RUST so
that disabling Rust cleanly makes the dependency a no-op.
* Patch 2: Makefile: support universal macOS builds via RUST_TARGETS Allows
users to specify space-separated target triples in RUST_TARGETS.
Introduces declarative pattern rules (target/%/...) to compile each
target slice via Cargo, and uses lipo (part of the mandatory Xcode
Command Line Tools) to combine the resulting static archives into a
universal library at target/release/libgitcore.a. Uses
mkdir_p_parent_template to guarantee directory creation before lipo.
* Patch 3: contrib: wire up osxkeychain in contrib/Makefile on macOS Adds
a test target to contrib/credential/osxkeychain/Makefile that depends
on building git-credential-osxkeychain. Introduces a generic OS_CONTRIB
variable in contrib/Makefile to conditionally wire
credential/osxkeychain into all, test, and clean whenever running on
macOS (Darwin). This guarantees that standard CI test runs on macOS
automatically compile and link the helper, preventing build
regressions.
Changes since v5:
* Reverted Patch 1 to depend explicitly on $(LIB_FILE) $(RUST_LIB) rather
than $(GITLIBS). Unlike Git builtins or scalar (which define cmd_main()),
git-credential-osxkeychain.c defines its own standalone main(), meaning
$(GITLIBS) caused a duplicate symbol error for _main during linking.
* Added Patch 3 ("contrib: wire up osxkeychain in contrib/Makefile on
macOS") using a scalable OS_CONTRIB variable so that running make test
with TEST_CONTRIB_TOO=yes in macOS CI workflows automatically verifies
compilation and linking integrity.
Changes since v4:
* Changed the osxkeychain prerequisite dependency from $(LIB_FILE)
$(RUST_LIB) to $(GITLIBS) to match the canonical prerequisite pattern
used by all other core Git targets linking $(LIBS).
Changes since v3:
* Removed leading @ from $(call mkdir_p_parent_template) so it relies on
the built-in $(QUIET_MKDIR_P_PARENT) behavior, matching existing Makefile
conventions.
* Replaced if [ with if test in Bourne shell recipe snippets to strictly
adhere to the project's CodingGuidelines.
Changes since v2:
* Split the original combined commit into a two-patch series to separate
prerequisite bug fixes from Universal Binary features.
* Added $(call mkdir_p_parent_template) prior to invoking lipo to guarantee
that parent target directories exist.
Shardul Natu (3):
Makefile: add $(RUST_LIB) prerequisite to osxkeychain
Makefile: support universal macOS builds via RUST_TARGETS
contrib: wire up osxkeychain in contrib/Makefile on macOS
Makefile | 46 ++++++++++++++++++++++---
contrib/Makefile | 10 ++++++
contrib/credential/osxkeychain/Makefile | 4 ++-
3 files changed, 54 insertions(+), 6 deletions(-)
base-commit: 602f6c329a7d99df269d382df353b4e1bbbbd8aa
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2288%2Fkiranani%2Fnext-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2288/kiranani/next-v6
Pull-Request: https://github.com/git/git/pull/2288
Range-diff vs v5:
1: e0bb18ff01 ! 1: 0d21513940 Makefile: add $(GITLIBS) prerequisite to osxkeychain
@@ Metadata
Author: Shardul Natu <snatu@google.com>
## Commit message ##
- Makefile: add $(GITLIBS) prerequisite to osxkeychain
+ Makefile: add $(RUST_LIB) prerequisite to osxkeychain
When Rust is enabled, the git-credential-osxkeychain helper depends on
Rust symbols compiled into $(RUST_LIB). While commit 522ea8ef7d
@@ Commit message
clean working tree can fail because Make does not know to invoke Cargo
to build libgitcore.a before linking git-credential-osxkeychain.
- All other core Git targets that link $(LIBS) already depend on
- $(GITLIBS), which bundles common-main.o, $(LIB_FILE), and $(RUST_LIB)
- when Rust is enabled. Add $(GITLIBS) as a prerequisite dependency to the
- git-credential-osxkeychain target to make it consistent with the rest of
- the codebase.
+ Note that we depend explicitly on $(LIB_FILE) and $(RUST_LIB) rather
+ than $(GITLIBS). Unlike standard Git builtins and programs like scalar
+ (which define cmd_main() and rely on common-main.o to supply main()),
+ git-credential-osxkeychain.c defines its own standalone int main().
+ If $(GITLIBS) were used, $(filter %.o,$^) in the link recipe would
+ match both git-credential-osxkeychain.o and common-main.o, causing a
+ duplicate symbol linking error for _main on macOS.
Additionally, wrap the definitions of $(RUST_LIB) and the "rust" build
target in "ifndef NO_RUST". This ensures that when NO_RUST=1 is
@@ Makefile: $(LIBGIT_HIDDEN_EXPORT): $(LIBGIT_PARTIAL_EXPORT)
-contrib/credential/osxkeychain/git-credential-osxkeychain: contrib/credential/osxkeychain/git-credential-osxkeychain.o $(LIB_FILE) GIT-LDFLAGS
+# When Rust is enabled, git-credential-osxkeychain depends on Rust symbols in $(RUST_LIB)
-+contrib/credential/osxkeychain/git-credential-osxkeychain: contrib/credential/osxkeychain/git-credential-osxkeychain.o $(GITLIBS) GIT-LDFLAGS
++contrib/credential/osxkeychain/git-credential-osxkeychain: contrib/credential/osxkeychain/git-credential-osxkeychain.o $(LIB_FILE) $(RUST_LIB) GIT-LDFLAGS
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS) -framework Security -framework CoreFoundation
2: 66f71fb0d7 = 2: 21dedb91f0 Makefile: support universal macOS builds via RUST_TARGETS
-: ---------- > 3: 8455e449f3 contrib: wire up osxkeychain in contrib/Makefile on macOS
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 07/13] setup: move prefix into repository
From: Justin Tobler @ 2026-07-06 22:33 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-7-13864eb5a032@pks.im>
On 26/06/30 01:47PM, Patrick Steinhardt wrote:
> The repository prefix is currently stored in the startup info. This
> feels somewhat awkward though, as it is inherently a property of a given
> repository.
Agreed.
> Move the prefix into the repository accordingly.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> @@ -832,7 +832,8 @@ int cmd_rev_parse(int argc,
> prefix = argv[++i];
> if (!prefix)
> die(_("--prefix requires an argument"));
> - startup_info->prefix = prefix;
> + FREE_AND_NULL(the_repository->prefix);
> + the_repository->prefix = xstrdup(prefix);
git-rev-parse(1) has an option to explicitly set the prefix and we honor
that here.
[snip]
> @@ -2105,10 +2105,10 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
> */
> if (prefix) {
> prefix = precompose_string_if_needed(prefix);
> - startup_info->prefix = prefix;
> + repo->prefix = xstrdup(prefix);
> setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
> } else {
> - startup_info->prefix = NULL;
> + FREE_AND_NULL(repo->prefix);
> setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
> }
We set the startup_info prefix here is `setup_git_directory_gently()`
aleady, so we might as well just set it in the repository. I think this
is a good change.
-Justin
^ permalink raw reply
* Re: [PATCH 05/13] setup: introduce explicit repository discovery
From: Justin Tobler @ 2026-07-06 22:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-5-13864eb5a032@pks.im>
On 26/06/30 01:47PM, Patrick Steinhardt wrote:
> When setting up the global repository we intermix repository discovery
> and repository configuration: we repeatedly call `set_git_work_tree()`
> and `apply_and_export_relative_gitdir()` until we're happy with the
> result. The result of this is then a partially-configured repository
> that we use for further setup.
>
> This process is quite hard to follow, as it's never quite clear which
> parts of the repository have been configured already and which haven't.
> Furthermore, it means that the repository configuration is distributed
> across many different places instead of having it neatly contained in a
> single location. Ultimately, this is the reason that we cannot use a
> central function like `repo_init()`.
>
> Refactor the logic so that we stop partially-configuring a repository
> and instead populate a new `struct repo_discovery`. This allow us to
> essentially split repository setup into two phases:
>
> - The first phase only figures out parameters required to configure
> the repository.
>
> - The second phase then takes these parameters and applies them to the
> repository.
Ok so `struct repo_discovery` is just an intermediate structure to store
all the repository configuration so we can apply it all at once. Makes
sense.
> Like this, we'll never end up with a partially-configured repository and
> can eventually extend `repo_init()` to handle the full initialization
> for us.
So IIUC the expectation here would be for all configuration of the
repository to happen prior to it being applied? Would it be a bug to
attempt to apply configuration to a repository more than once?
Overall, I like the direction of this patch so far :)
-Justin
^ permalink raw reply
* Re: [PATCH 03/13] setup: unify setup of shallow file
From: Justin Tobler @ 2026-07-06 22:02 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-3-13864eb5a032@pks.im>
On 26/06/30 01:47PM, Patrick Steinhardt wrote:
> It is possible to configure an arbitrary "shallow" file via two
> mechanisms, and the respective logic to handle these is split across two
> locations:
>
> - Via the "GIT_SHALLOW_FILE" environment variable, which is handled in
> `setup_git_env_internal()`.
>
> - Via the global "--shallow-file=" command line option, which is
> handled in `handle_options()`.
Ok.
> We can rather easily unify this logic by not configuring the shallow
> file in `handle_options()`, but instead overwriting the environment
> variable. The environment variable itself is then handled inside of
> `apply_repository_format()`, which is responsible for configuring a
> discovered Git directory.
What is supposed to be the correct order for processing shallow file
configuration here? Does this mean that the `--shallow-file` option now
overwrites the environment variable? Was this how it already was?
> This new logic is similar in nature to how we handle the other global
> options already, all of which end up setting an environment variable.
> So for one this gives us more consistency. But more importantly, this
> change means that `the_repository` will not contain any relevant state
> anymore before we hit `apply_repository_format()` once we're at the end
> of this patch series. Consequently, it will become possible for us to
> completely discard `the_repository` and populate it anew.
I can't say that I'm a fan of using environment variables to store
global state in this manner, but I guess if there is precdent and this
is making us more consistent, it is probably fine. I guess the other
option would be to store the read configuration is some intermediate
structure to be applied later, but that may not be worth it here.
-Justin
^ permalink raw reply
* Re: [PATCH v2] prio-queue: use cascade-down for faster extract-min
From: Kristofer Karlsson @ 2026-07-06 21:52 UTC (permalink / raw)
To: René Scharfe, Junio C Hamano
Cc: Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <1aa5b755-0f74-46d5-bd6e-a9cb7f3fbb12@web.de>
On Sun, 7 Jun 2026 at 09:30, René Scharfe <l.s.r@web.de> wrote:
>
> So I guess we keep the full sift-down for prio_queue_replace(), knowing
> that sometimes we have a lot of items that end up at or close to the
> root of the heap.
The lazy-fold series (kk/prio-queue-get-put-fusion) is in next now.
I rebased this cascade patch on top of it to check if it's still
useful.
With lazy-fold in place the regression scenario you identified
is resolved. The only remaining change is in flush_get(),
where unfused gets now cascade instead of sifting down:
- queue->array[0] = queue->array[--queue->nr_];
- sift_down_root(queue);
+ --queue->nr_;
+ sift_up_rebalance(queue);
plus the ~20-line sift_up_rebalance() implementation.
I benchmarked this on the linux kernel repo and on a large
merge-heavy repo.
The results are consistent: a real but small 1-2% end-to-end
improvement across commands. A prio-queue microbenchmark
would likely show a larger difference, but the queue
is only a fraction of the total work in any real git operation.
The lazy-fold optimization cannibalized some of the value here,
so cascade only helps the remaining unfused gets. As you observed,
cascade is better there, but there are fewer of them now that there
is more fusing happening.
I am on the fence about whether 1-2% end-to-end justifies adding
another sift function. If you (René and Junio) think the benefit
is too small for the code cost, I am happy to drop this patch.
Otherwise I can submit a small reroll on top of
kk/prio-queue-get-put-fusion (or rather next, in practice).
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH 02/13] setup: mark bogus worktree in `apply_repository_format()`
From: Justin Tobler @ 2026-07-06 21:49 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-2-13864eb5a032@pks.im>
On 26/06/30 01:47PM, Patrick Steinhardt wrote:
> When a repository is configured to have both "core.worktree" and
> "core.bare" we emit a warning and mark the worktree configuration as
> bogus so that the next call to `setup_work_tree()` will cause us to die.
> This allows us to still use the misconfigured repository, at least as
> long as we don't try to use its worktree.
Ok.
> This condition is handled in `setup_explicit_git_dir()`. In a subsequent
> commit we'll refactor this function so that it doesn't receive a repo as
> input anymore though, and consequently we cannot set the "bogus" bit
> anymore.
Ok IIUC, `setup_explicit_git_dir()` is currently responsible for
checking if both "core.worktree" and "core.bare" are set.
> Move the logic into `apply_repository_format()` instead to prepare for
> this. While at it, fix up formatting a bit.
So `apply_repository_format()` is expected to still have the repository
info which has access to the "bogus" field.
> Note that this change requires us to also explicitly unset the value of
> "core.worktree" in case we have the "GIT_WORK_TREE" environment variable
> set. This is because the environment variable overrides the repository's
> configuration, and we don't want to warn or die in case the work tree
> has been configured explicitly regardless of whether or not "core.bare"
> is set.
Hmmm, does this mean we now just silently ignore the misconfiguration if
done via environment variable?
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> setup.c | 37 +++++++++++++++++++++----------------
> 1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 118416e350..f54eac5e5a 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1147,24 +1147,24 @@ static const char *setup_explicit_git_dir(struct repository *repo,
> }
>
> /* #3, #7, #11, #15, #19, #23, #27, #31 (see t1510) */
> - if (work_tree_env)
> + if (work_tree_env) {
> + /*
> + * The environment variable overrides "core.worktree". This
> + * also has the consequence that we don't want to flag cases as
> + * bogus where we have both "core.worktree" and "core.bare", so
> + * we have to exlicitly unset the configuration.
> + */
> + FREE_AND_NULL(repo_fmt->work_tree);
Ok, this confused me a bit a first, but IIUC we have to unset the
environment variable because we now defer setting the bogus flag to a
later point when `apply_repository_format()` is executed.
> set_git_work_tree(repo, work_tree_env);
> - else if (repo_fmt->is_bare > 0) {
> - if (repo_fmt->work_tree) {
> - /* #22.2, #30 */
> - warning("core.bare and core.worktree do not make sense");
> - repo->worktree_config_is_bogus = true;
> - }
> -
> + } else if (repo_fmt->is_bare > 0) {
> /* #18, #26 */
> set_git_dir(repo, gitdirenv, 0);
> free(gitfile);
> return NULL;
> - }
> - else if (repo_fmt->work_tree) { /* #6, #14 */
> - if (is_absolute_path(repo_fmt->work_tree))
> + } else if (repo_fmt->work_tree) { /* #6, #14 */
> + if (is_absolute_path(repo_fmt->work_tree)) {
> set_git_work_tree(repo, repo_fmt->work_tree);
> - else {
> + } else {
> char *core_worktree;
> if (chdir(gitdirenv))
> die_errno(_("cannot chdir to '%s'"), gitdirenv);
> @@ -1176,15 +1176,14 @@ static const char *setup_explicit_git_dir(struct repository *repo,
> set_git_work_tree(repo, core_worktree);
> free(core_worktree);
> }
> - }
> - else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
> + } else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
> /* #16d */
> set_git_dir(repo, gitdirenv, 0);
> free(gitfile);
> return NULL;
> - }
> - else /* #2, #10 */
> + } else { /* #2, #10 */
> set_git_work_tree(repo, ".");
> + }
Some random curly brace cleanup above.
>
> /* set_git_work_tree() must have been called by now */
> worktree = repo_get_work_tree(repo);
> @@ -1768,6 +1767,12 @@ int apply_repository_format(struct repository *repo,
> if (verify_repository_format(format, err) < 0)
> return -1;
>
> + if (format->is_bare > 0 && format->work_tree) {
> + /* #22.2, #30 */
> + warning("core.bare and core.worktree do not make sense");
> + repo->worktree_config_is_bogus = true;
> + }
We now perform this validation in `apply_repository_format()`. Does
deferring this check have any meaningful impact? Or is
`apply_repository_format()` always called after
`setup_explicit_git_dir()`?
-Justin
^ permalink raw reply
* Re: [PATCH 01/13] setup: rename `check_repository_format_gently()`
From: Justin Tobler @ 2026-07-06 21:27 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-1-13864eb5a032@pks.im>
On 26/06/30 01:47PM, Patrick Steinhardt wrote:
> The function `check_repository_format_gently()` receives a format as
> input. An unknowing reader may thus suspect that this function actually
> checks the passed-in format for consistency. While the function indeed
> checks the repository format, it actually serves two purposes:
>
> - It reads the repository's format and populates the passed-in format
> with that information.
>
> - It then indeed checks whether the format is consistent.
Ok
> Rename the function to `read_and_verify_repository_format()` to clarify
> its functionality. While at it, reorder the parameters so that the
> format comes first to better match other functions that pass around the
> format.
I agree that the current name is a bit misleading so this change sounds
reasonable to me.
The patch itself is just a trivial rename and reorder of parameters.
Looks good.
-Justin
^ permalink raw reply
* [PATCH] t1410-reflog.sh: avoid suppressing git's exit code in pipelines
From: Gatla Vishweshwar Reddy @ 2026-07-06 20:50 UTC (permalink / raw)
To: git; +Cc: Gatla Vishweshwar Reddy
Piping git commands directly to wc -l suppresses the exit code of
git, hiding potential failures from the test suite. Capture the
output to a temporary file first, then count the lines separately
to preserve the exit code.
Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
---
t/t1410-reflog.sh | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index ce71f9a30a..397f94b039 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -244,8 +244,10 @@ test_expect_success 'delete' '
test_tick &&
git commit -m tiger C &&
- HEAD_entry_count=$(git reflog | wc -l) &&
- main_entry_count=$(git reflog show main | wc -l) &&
+ git reflog >reflog_output &&
+ HEAD_entry_count=$(wc -l <reflog_output) &&
+ git reflog show main >reflog_main_output &&
+ main_entry_count=$(wc -l <reflog_main_output) &&
test $HEAD_entry_count = 5 &&
test $main_entry_count = 5 &&
@@ -254,16 +256,23 @@ test_expect_success 'delete' '
git reflog delete main@{1} &&
git reflog show main > output &&
test_line_count = $(($main_entry_count - 1)) output &&
- test $HEAD_entry_count = $(git reflog | wc -l) &&
+ git reflog >reflog_output &&
+ test $HEAD_entry_count = $(wc -l <reflog_output) &&
! grep ox < output &&
main_entry_count=$(wc -l < output) &&
git reflog delete HEAD@{1} &&
- test $(($HEAD_entry_count -1)) = $(git reflog | wc -l) &&
- test $main_entry_count = $(git reflog show main | wc -l) &&
+ git reflog >reflog_output &&
+ test $(($HEAD_entry_count -1)) = $(wc -l <reflog_output) &&
+ git reflog show main >reflog_main_output &&
+ test $main_entry_count = $(wc -l <reflog_main_output) &&
+
+
+ git reflog >reflog_output &&
+ HEAD_entry_count=$(wc -l <reflog_output) &&
+
- HEAD_entry_count=$(git reflog | wc -l) &&
git reflog delete main@{07.04.2005.15:15:00.-0700} &&
git reflog show main > output &&
@@ -321,11 +330,15 @@ test_expect_success 'git reflog expire unknown reference' '
'
test_expect_success 'checkout should not delete log for packed ref' '
- test $(git reflog main | wc -l) = 4 &&
+ git reflog main >reflog_output &&
+ test $(wc -l <reflog_output) = 4 &&
git branch foo &&
git pack-refs --all &&
git checkout foo &&
- test $(git reflog main | wc -l) = 4
+ git reflog main >reflog_output &&
+ test $(wc -l <reflog_output) = 4
+
+
'
test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' '
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v7 0/5] history: add squash subcommand to fold a range
From: Junio C Hamano @ 2026-07-06 20:42 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Harald Nordgren
In-Reply-To: <pull.2337.v7.git.git.1783327849.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Adds git history squash <revision-range> to fold a range of commits.
What I saw in the range-diff looked all reasonable.
> 3: 811e393ab4 ! 3: cf3346a1cd history: add squash subcommand to fold a range
> @@ Commit message
> Add "git history squash <revision-range>" to do this directly. It folds
> every commit in the range into the oldest one, keeping that commit's
> message and authorship and taking the tree of the newest commit, then
> - replays the commits above the range on top. fixup!, squash! and amend!
> - commits are folded like any other and are not interpreted, so the
> - squashed message comes from the oldest commit, or from an editor with
> - --reedit-message.
> + replays the commits above the range on top. The squashed message comes
> + from the oldest commit, or from an editor with --reedit-message. As that
> + message is reused, a range whose oldest commit is a fixup!, squash! or
> + amend! is refused, since the marker's target cannot be in the range.
> ...
> -+git history squash <revision-range> [--dry-run] [--update-refs=(branches|head)] [--reedit-message]
> ++git history squash [--dry-run] [--update-refs=(branches|head)] [--reedit-message] <revision-range>
> ++static int reject_fixupish_oldest(struct repository *repo,
> ++ struct commit *oldest)
> ++{
> ++ const char *message, *subject;
> ++ int ret = 0;
> ++
> ++ message = repo_logmsg_reencode(repo, oldest, NULL, NULL);
> ++ find_commit_subject(message, &subject);
> ++ if (starts_with(subject, "fixup! ") ||
> ++ starts_with(subject, "squash! ") ||
> ++ starts_with(subject, "amend! "))
> ++ ret = error(_("the range begins with a fixup!, squash! or amend! "
> ++ "commit whose target is not in the range"));
> ++ repo_unuse_commit_buffer(repo, oldest, message);
> ++ return ret;
> ++}
Nice. I often see myself getting rescued by the corresponding sanity
checks in the sequencer.
Will replace. Thanks.
^ permalink raw reply
* Re: [PATCH] blame: reserve mark column only if necessary
From: Junio C Hamano @ 2026-07-06 20:33 UTC (permalink / raw)
To: René Scharfe; +Cc: Laszlo Ersek, git
In-Reply-To: <92991b5e-0667-4315-89d5-1514a5499297@web.de>
René Scharfe <l.s.r@web.de> writes:
> `--abbrev=<n>`::
> + Instead of using the default _7_ hexadecimal digits as the
> + abbreviated object name, use at least _<n>_ digits, but ensure
> + the commit object names are unique.
> + If commits marked with caret (boundary), question mark (ignored)
> + or asterisk (unblamable) are shown, extend unmarked object names
> + to align them.
OK.
> +static inline int maybe_putc(int c, FILE *out)
> +{
> + return out ? putc(c, out) : 0;
> +}
> +
> +static size_t print_marks(FILE *out, const struct blame_entry *ent, int opt)
> +{
> + size_t len = 0;
> +
> + if ((ent->suspect->commit->object.flags & UNINTERESTING) &&
> + !blank_boundary && !(opt & OUTPUT_ANNOTATE_COMPAT)) {
> + maybe_putc('^', out);
> + len++;
> + }
> + if (mark_unblamable_lines && ent->unblamable) {
> + maybe_putc('*', out);
> + len++;
> + }
> + if (mark_ignored_lines && ent->ignored) {
> + maybe_putc('?', out);
> + len++;
> + }
> + return len;
> +}
Quite straight-forward.
> +static size_t count_marks(const struct blame_entry *ent, int opt)
> +{
> + return print_marks(NULL, ent, opt);
> +}
OK.
> @@ -499,23 +529,10 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent,
> if (color)
> fputs(color, stdout);
>
> - if (suspect->commit->object.flags & UNINTERESTING) {
> - if (blank_boundary) {
> - memset(hex, ' ', strlen(hex));
> - } else if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {
> - length--;
> - putchar('^');
> - }
> - }
> -
> - if (mark_unblamable_lines && ent->unblamable) {
> - length--;
> - putchar('*');
> - }
> - if (mark_ignored_lines && ent->ignored) {
> - length--;
> - putchar('?');
> - }
> + if ((suspect->commit->object.flags & UNINTERESTING) &&
> + blank_boundary)
> + memset(hex, ' ', strlen(hex));
> + length -= print_marks(stdout, ent, opt);
>
> printf("%.*s", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex);
> if (opt & OUTPUT_ANNOTATE_COMPAT) {
> @@ -647,11 +664,15 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)
> struct blame_entry *e;
> int compute_auto_abbrev = (abbrev < 0);
> int auto_abbrev = DEFAULT_ABBREV;
> + size_t max_marks_count = 0;
>
> for (e = sb->ent; e; e = e->next) {
> struct blame_origin *suspect = e->suspect;
> int num;
> + size_t marks_count = count_marks(e, *option);
>
> + if (max_marks_count < marks_count)
> + max_marks_count = marks_count;
> if (compute_auto_abbrev)
> auto_abbrev = update_auto_abbrev(auto_abbrev, suspect);
> if (strcmp(suspect->path, sb->path))
> @@ -685,8 +706,12 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)
> max_score_digits = decimal_width(largest_score);
>
> if (compute_auto_abbrev)
> - /* one more abbrev length is needed for the boundary commit */
> - abbrev = auto_abbrev + 1;
> + abbrev = auto_abbrev;
> + if (abbrev < (int)the_hash_algo->hexsz) {
> + abbrev += max_marks_count;
> + if (abbrev > (int)the_hash_algo->hexsz)
> + abbrev = the_hash_algo->hexsz;
> + }
> }
>
> static void sanity_check_on_fail(struct blame_scoreboard *sb, int baa)
> @@ -1047,10 +1072,7 @@ int cmd_blame(int argc,
> } else if (show_progress < 0)
> show_progress = isatty(2);
>
> - if (0 < abbrev && abbrev < (int)the_hash_algo->hexsz)
> - /* one more abbrev length is needed for the boundary commit */
> - abbrev++;
> - else if (!abbrev)
> + if (!abbrev)
> abbrev = the_hash_algo->hexsz;
OK.
> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
> index 7822947f028..bf04b8273ef 100755
> --- a/t/t8002-blame.sh
> +++ b/t/t8002-blame.sh
> @@ -113,8 +113,7 @@ test_expect_success 'set up abbrev tests' '
> '
>
> test_expect_success 'blame --abbrev=<n> works' '
> - # non-boundary commits get +1 for alignment
> - check_abbrev 31 --abbrev=30 HEAD &&
> + check_abbrev 30 --abbrev=30 HEAD &&
> check_abbrev 30 --abbrev=30 ^HEAD
> '
>
> @@ -141,10 +140,8 @@ test_expect_success 'blame --abbrev gets truncated with boundary commit' '
> '
>
> test_expect_success 'blame --abbrev -b truncates the blank boundary' '
> - # Note that `--abbrev=` always gets incremented by 1, which is why we
> - # expect 11 leading spaces and not 10.
> cat >expect <<-EOF &&
> - $(printf "%11s" "") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev
> + $(printf "%10s" "") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev
> EOF
OK.
> git blame -b --abbrev=10 ^HEAD -- abbrev.t >actual &&
> test_cmp expect actual
^ permalink raw reply
* Re: [PATCH v3 0/9] t: fixes and improvements for GIT_TEST_LONG
From: Junio C Hamano @ 2026-07-06 20:29 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Johannes Schindelin, SZEDER Gábor, Jeff King
In-Reply-To: <20260706-b4-pks-t-fixes-for-GIT-TEST-LONG-v3-0-4f6c5a37fd1f@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Hi,
>
> this series started out as a simple two-patch series that wired up the
> GitLab CI badge in our README and GIT_TEST_LONG for GitLab CI. But as it
> typically goes, tests broke on GitLab CI, which made me realize that
> they are broken even on GitHub's master branch right now. Some tests are
> failing in the linux32 job, and we only didn't notice because the whole
> pipeline hangs.
>
> So I had to go down the rabbit hole a bit, the result of which is this
> patch series.
>
> Changes in v3:
> - Fix commit subjects to mention correct prerequisite.
> - Link to v2: https://patch.msgid.link/20260703-b4-pks-t-fixes-for-GIT-TEST-LONG-v2-0-79076a7e0c62@pks.im
The interdiff looks trivially correct ;-).
Hopefully we are now ready to declare victory and plan to merge this
to 'next'?
Thanks.
> 1: e4add14ea7 = 1: afc7563e22 README: add GitLab CI badge to make it more discoverable
> 2: d762b4d46e ! 2: 753e950eaf t0021: skip EXPENSIVE test that is broken without SIZE_T_IS_32BIT
> @@ Metadata
> Author: Patrick Steinhardt <ps@pks.im>
>
> ## Commit message ##
> - t0021: skip EXPENSIVE test that is broken without SIZE_T_IS_32BIT
> + t0021: skip EXPENSIVE test that is broken without SIZE_T_IS_64BIT
>
> One of the tests in t0021 writes a 2GB file and then roundtrips it
> through the clean/sumdge filters. This test is broken on 32 bit
> 3: 8d43eb2819 = 3: f776e0fb5f t4141: fix inefficient use of dd(1)
> 4: fcd048f6f7 = 4: 9754b96a43 t5608: reduce maximum disk usage
> 5: 11df7f2cb9 ! 5: 0f2e28dc11 t7508: skip EXPENSIVE test that is broken without SIZE_T_IS_32BIT
> @@ Metadata
> Author: Patrick Steinhardt <ps@pks.im>
>
> ## Commit message ##
> - t7508: skip EXPENSIVE test that is broken without SIZE_T_IS_32BIT
> + t7508: skip EXPENSIVE test that is broken without SIZE_T_IS_64BIT
>
> One of the tests in t7508 is marked as EXPENSIVE because it ends up
> creating and adding files that are multiple gigabytes in size. This
> 6: a16bc1754b = 6: d329a2cd40 t7900: clean up large EXPENSIVE repository
> 7: b2e6b0d517 = 7: a336d4ce9e t: use `test_bool_env` to parse GIT_TEST_LONG
> 8: 9632b19164 = 8: cfff94c79e gitlab-ci: disable RAM disk on macOS jobs
> 9: a42c613012 = 9: ed5e8807fe gitlab-ci: enable "GIT_TEST_LONG"
>
> ---
> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
> change-id: 20260701-b4-pks-t-fixes-for-GIT-TEST-LONG-78e538bf0e06
^ permalink raw reply
* Re: [PATCH] rebase -i: introduce `pick -x` to add "cherry picked from commit ..."
From: Junio C Hamano @ 2026-07-06 20:24 UTC (permalink / raw)
To: Phillip Wood
Cc: Jeff King, Trevor Gross, git, Stefan Haller, Derrick Stolee,
Phillip Wood
In-Reply-To: <5d238e0d-18ba-429a-a9a4-a3988b00e1e1@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
>> Usually a rebase is about rewriting the commits on a new base so that
>> you can throw away the old ones. And that's why git-rebase generally
>> rewrites the branch you're on, and replaces those old commits. So adding
>> a "cherry-picked from..." annotation doesn't make sense there; nobody
>> would have those old commits!
>
> Exactly
;-)
Whew. Briefly I wondered if I were the only one who felt 'rebase'
and 'cherry-pick' serve two different purposes and need to behave
differently, e.g., with respect to how notes on old commits are
dealt with.
> On a slight tangent I've sometimes wanted to be able to do
>
> git cherry-pick --exec 'make test' some commits
Yes, I agree that is something quite handy.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite
From: Junio C Hamano @ 2026-07-06 20:16 UTC (permalink / raw)
To: Ian Jackson; +Cc: git, Colin Stagner, Johannes Schindelin
In-Reply-To: <27211.50096.133710.528147@chiark.greenend.org.uk>
Ian Jackson <ijackson@chiark.greenend.org.uk> writes:
>> if git rev-parse --verify -q "$1:$config"
>> then
>> die "fatal: tree contains $config: has been processed with new standalone (Rust) git-subtree; use that tool instead of this one. See https://codeberg.org/diziet/git-subtree https://crates.io/crates/git-subtree"
>> fi
>>
>> Overly long output does not look very easy to read, but I kept it
>> the same as the original.
>
> I'm not a great fan of the long error message myself, but it seemed to
> be what the rest of the script was doing. I didn't find any
> multi-line calls to die, so that's why I did it this way.
>
> I'm happy to reformat this to your taste.
Nah, it seems your plan is to deprecate this script over time and
move everybody to a newer implementation, so as long as "die" does
its job to stop and prevent breakages from spreading, that would be
fine.
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