* [PATCH v3 12/16] builtin/repack.c: convert `--write-midx` to an `OPT_CALLBACK`
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
Change the --write-midx (-m) flag from an OPT_BOOL to an OPT_CALLBACK
that accepts an optional mode argument. Introduce an enum with
REPACK_WRITE_MIDX_NONE and REPACK_WRITE_MIDX_DEFAULT to distinguish
between the two states, and update all existing boolean checks
accordingly.
For now, passing no argument (or just `-m`) selects the default mode,
preserving existing behavior. A subsequent commit will add a new mode
for writing incremental MIDXs.
Extract repack_write_midx() as a dispatcher that selects the
appropriate MIDX-writing implementation based on the mode.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 50 ++++++++++++++++++++++++++++++++++++------------
repack-midx.c | 14 +++++++++++++-
repack.h | 8 +++++++-
3 files changed, 58 insertions(+), 14 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 24be147d39a..5d366340c34 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -97,6 +97,24 @@ static int repack_config(const char *var, const char *value,
return git_default_config(var, value, ctx, cb);
}
+static int option_parse_write_midx(const struct option *opt, const char *arg,
+ int unset)
+{
+ enum repack_write_midx_mode *cfg = opt->value;
+
+ if (unset) {
+ *cfg = REPACK_WRITE_MIDX_NONE;
+ return 0;
+ }
+
+ if (!arg || !*arg)
+ *cfg = REPACK_WRITE_MIDX_DEFAULT;
+ else
+ return error(_("unknown value for %s: %s"), opt->long_name, arg);
+
+ return 0;
+}
+
int cmd_repack(int argc,
const char **argv,
const char *prefix,
@@ -119,7 +137,7 @@ int cmd_repack(int argc,
struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
struct pack_objects_args po_args = PACK_OBJECTS_ARGS_INIT;
struct pack_objects_args cruft_po_args = PACK_OBJECTS_ARGS_INIT;
- int write_midx = 0;
+ enum repack_write_midx_mode write_midx = REPACK_WRITE_MIDX_NONE;
const char *cruft_expiration = NULL;
const char *expire_to = NULL;
const char *filter_to = NULL;
@@ -185,8 +203,14 @@ int cmd_repack(int argc,
N_("do not repack this pack")),
OPT_INTEGER('g', "geometric", &geometry.split_factor,
N_("find a geometric progression with factor <N>")),
- OPT_BOOL('m', "write-midx", &write_midx,
- N_("write a multi-pack index of the resulting packs")),
+ OPT_CALLBACK_F(0, "write-midx", &write_midx,
+ N_("mode"),
+ N_("write a multi-pack index of the resulting packs"),
+ PARSE_OPT_OPTARG, option_parse_write_midx),
+ OPT_SET_INT_F('m', NULL, &write_midx,
+ N_("write a multi-pack index of the resulting packs"),
+ REPACK_WRITE_MIDX_DEFAULT,
+ PARSE_OPT_HIDDEN),
OPT_STRING(0, "expire-to", &expire_to, N_("dir"),
N_("pack prefix to store a pack containing pruned objects")),
OPT_STRING(0, "filter-to", &filter_to, N_("dir"),
@@ -221,14 +245,16 @@ int cmd_repack(int argc,
pack_everything |= ALL_INTO_ONE;
if (write_bitmaps < 0) {
- if (!write_midx &&
+ if (write_midx == REPACK_WRITE_MIDX_NONE &&
(!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
write_bitmaps = 0;
}
if (po_args.pack_kept_objects < 0)
- po_args.pack_kept_objects = write_bitmaps > 0 && !write_midx;
+ po_args.pack_kept_objects = write_bitmaps > 0 &&
+ write_midx == REPACK_WRITE_MIDX_NONE;
- if (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)
+ if (write_bitmaps && !(pack_everything & ALL_INTO_ONE) &&
+ write_midx == REPACK_WRITE_MIDX_NONE)
die(_(incremental_bitmap_conflict_error));
if (write_bitmaps && po_args.local &&
@@ -244,7 +270,7 @@ int cmd_repack(int argc,
write_bitmaps = 0;
}
- if (write_midx && write_bitmaps) {
+ if (write_midx != REPACK_WRITE_MIDX_NONE && write_bitmaps) {
struct strbuf path = STRBUF_INIT;
strbuf_addf(&path, "%s/%s_XXXXXX",
@@ -297,7 +323,7 @@ int cmd_repack(int argc,
}
if (repo_has_promisor_remote(repo))
strvec_push(&cmd.args, "--exclude-promisor-objects");
- if (!write_midx) {
+ if (write_midx == REPACK_WRITE_MIDX_NONE) {
if (write_bitmaps > 0)
strvec_push(&cmd.args, "--write-bitmap-index");
else if (write_bitmaps < 0)
@@ -519,7 +545,7 @@ int cmd_repack(int argc,
if (delete_redundant && pack_everything & ALL_INTO_ONE)
existing_packs_mark_for_deletion(&existing, &names);
- if (write_midx) {
+ if (write_midx != REPACK_WRITE_MIDX_NONE) {
struct repack_write_midx_opts opts = {
.existing = &existing,
.geometry = &geometry,
@@ -528,11 +554,11 @@ int cmd_repack(int argc,
.packdir = packdir,
.show_progress = show_progress,
.write_bitmaps = write_bitmaps > 0,
- .midx_must_contain_cruft = midx_must_contain_cruft
+ .midx_must_contain_cruft = midx_must_contain_cruft,
+ .mode = write_midx,
};
- ret = write_midx_included_packs(&opts);
-
+ ret = repack_write_midx(&opts);
if (ret)
goto cleanup;
}
diff --git a/repack-midx.c b/repack-midx.c
index 78f069c2151..4a568a2a9b8 100644
--- a/repack-midx.c
+++ b/repack-midx.c
@@ -315,7 +315,7 @@ static int repack_fill_midx_stdin_packs(struct child_process *cmd,
return finish_command(cmd);
}
-int write_midx_included_packs(struct repack_write_midx_opts *opts)
+static int write_midx_included_packs(struct repack_write_midx_opts *opts)
{
struct child_process cmd = CHILD_PROCESS_INIT;
struct string_list include = STRING_LIST_INIT_DUP;
@@ -378,3 +378,15 @@ int write_midx_included_packs(struct repack_write_midx_opts *opts)
return ret;
}
+
+int repack_write_midx(struct repack_write_midx_opts *opts)
+{
+ switch (opts->mode) {
+ case REPACK_WRITE_MIDX_NONE:
+ BUG("write_midx mode is NONE?");
+ case REPACK_WRITE_MIDX_DEFAULT:
+ return write_midx_included_packs(opts);
+ default:
+ BUG("unhandled write_midx mode: %d", opts->mode);
+ }
+}
diff --git a/repack.h b/repack.h
index 77d24ee45fb..81907fcce7f 100644
--- a/repack.h
+++ b/repack.h
@@ -134,6 +134,11 @@ void pack_geometry_release(struct pack_geometry *geometry);
struct tempfile;
+enum repack_write_midx_mode {
+ REPACK_WRITE_MIDX_NONE,
+ REPACK_WRITE_MIDX_DEFAULT,
+};
+
struct repack_write_midx_opts {
struct existing_packs *existing;
struct pack_geometry *geometry;
@@ -143,10 +148,11 @@ struct repack_write_midx_opts {
int show_progress;
int write_bitmaps;
int midx_must_contain_cruft;
+ enum repack_write_midx_mode mode;
};
void midx_snapshot_refs(struct repository *repo, struct tempfile *f);
-int write_midx_included_packs(struct repack_write_midx_opts *opts);
+int repack_write_midx(struct repack_write_midx_opts *opts);
int write_filtered_pack(const struct write_pack_opts *opts,
struct existing_packs *existing,
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 11/16] repack-geometry: prepare for incremental MIDX repacking
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
Teach `pack_geometry_init()` to optionally restrict the set of
repacking candidates to only packs in the tip MIDX layer when a
`midx_layer_threshold` is configured. If the tip layer has fewer packs
than the threshold, those packs are excluded entirely; otherwise only
packs in that layer participate in the geometric repack.
Also track whether any tip-layer packs were included in the rollup
(`midx_tip_rewritten`), which a subsequent commit will use to decide
how to update the MIDX chain after repacking.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
repack-geometry.c | 35 +++++++++++++++++++++++++++++++++++
repack.h | 4 ++++
2 files changed, 39 insertions(+)
diff --git a/repack-geometry.c b/repack-geometry.c
index 7cebd0cb45f..2408b8a3cc2 100644
--- a/repack-geometry.c
+++ b/repack-geometry.c
@@ -4,6 +4,7 @@
#include "repack.h"
#include "repository.h"
#include "hex.h"
+#include "midx.h"
#include "packfile.h"
static uint32_t pack_geometry_weight(struct packed_git *p)
@@ -31,8 +32,28 @@ void pack_geometry_init(struct pack_geometry *geometry,
{
struct packed_git *p;
struct strbuf buf = STRBUF_INIT;
+ struct multi_pack_index *m = get_multi_pack_index(existing->source);
repo_for_each_pack(existing->repo, p) {
+ if (geometry->midx_layer_threshold_set && m &&
+ p->multi_pack_index) {
+ /*
+ * When writing MIDX layers incrementally,
+ * ignore packs unless they are in the most
+ * recent MIDX layer *and* there are at least
+ * 'midx_layer_threshold' packs in that layer.
+ *
+ * Otherwise 'p' is either in an older layer, or
+ * the youngest layer does not have enough packs
+ * to consider its packs as candidates for
+ * repacking. In either of those cases we want
+ * to ignore the pack.
+ */
+ if (m->num_packs < geometry->midx_layer_threshold ||
+ !midx_layer_contains_pack(m, pack_basename(p)))
+ continue;
+ }
+
if (args->local && !p->pack_local)
/*
* When asked to only repack local packfiles we skip
@@ -173,6 +194,20 @@ void pack_geometry_split(struct pack_geometry *geometry)
geometry->promisor_split = compute_pack_geometry_split(geometry->promisor_pack,
geometry->promisor_pack_nr,
geometry->split_factor);
+ for (uint32_t i = 0; i < geometry->split; i++) {
+ struct packed_git *p = geometry->pack[i];
+ /*
+ * During incremental MIDX/bitmap repacking, any packs
+ * included in the rollup are either (a) not MIDX'd, or
+ * (b) contained in the tip layer iff it has at least
+ * the threshold number of packs.
+ *
+ * In the latter case, we can safely conclude that the
+ * tip of the MIDX chain will be rewritten.
+ */
+ if (p->multi_pack_index)
+ geometry->midx_tip_rewritten = true;
+ }
}
struct packed_git *pack_geometry_preferred_pack(struct pack_geometry *geometry)
diff --git a/repack.h b/repack.h
index c0e9f0ca647..77d24ee45fb 100644
--- a/repack.h
+++ b/repack.h
@@ -108,6 +108,10 @@ struct pack_geometry {
uint32_t promisor_pack_nr, promisor_pack_alloc;
uint32_t promisor_split;
+ uint32_t midx_layer_threshold;
+ bool midx_layer_threshold_set;
+ bool midx_tip_rewritten;
+
int split_factor;
};
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 10/16] repack-midx: extract `repack_fill_midx_stdin_packs()`
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
The function `write_midx_included_packs()` manages the lifecycle of
writing packs to stdin when running `git multi-pack-index write` as a
child process.
Extract a standalone `repack_fill_midx_stdin_packs()` helper, which
handles `--stdin-packs` argument setup, starting the command, writing
pack names to its standard input, and finishing the command.
This simplifies `write_midx_included_packs()` and prepares for a
subsequent commit where the same helper is called with `cmd->out = -1`
to capture the MIDX's checksum from the command's standard output,
which is needed when writing MIDX layers with `--no-write-chain-file`.
No functional changes are included in this patch.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
repack-midx.c | 38 ++++++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/repack-midx.c b/repack-midx.c
index 83151d4734a..78f069c2151 100644
--- a/repack-midx.c
+++ b/repack-midx.c
@@ -292,23 +292,42 @@ static void repack_prepare_midx_command(struct child_process *cmd,
strvec_push(&cmd->args, "--bitmap");
}
+static int repack_fill_midx_stdin_packs(struct child_process *cmd,
+ struct string_list *include)
+{
+ struct string_list_item *item;
+ FILE *in;
+ int ret;
+
+ cmd->in = -1;
+
+ strvec_push(&cmd->args, "--stdin-packs");
+
+ ret = start_command(cmd);
+ if (ret)
+ return ret;
+
+ in = xfdopen(cmd->in, "w");
+ for_each_string_list_item(item, include)
+ fprintf(in, "%s\n", item->string);
+ fclose(in);
+
+ return finish_command(cmd);
+}
+
int write_midx_included_packs(struct repack_write_midx_opts *opts)
{
struct child_process cmd = CHILD_PROCESS_INIT;
struct string_list include = STRING_LIST_INIT_DUP;
struct string_list_item *item;
struct packed_git *preferred = pack_geometry_preferred_pack(opts->geometry);
- FILE *in;
int ret = 0;
midx_included_packs(&include, opts);
if (!include.nr)
goto done;
- cmd.in = -1;
-
repack_prepare_midx_command(&cmd, opts, "write");
- strvec_push(&cmd.args, "--stdin-packs");
if (preferred)
strvec_pushf(&cmd.args, "--preferred-pack=%s",
@@ -350,16 +369,7 @@ int write_midx_included_packs(struct repack_write_midx_opts *opts)
strvec_pushf(&cmd.args, "--refs-snapshot=%s",
opts->refs_snapshot);
- ret = start_command(&cmd);
- if (ret)
- goto done;
-
- in = xfdopen(cmd.in, "w");
- for_each_string_list_item(item, &include)
- fprintf(in, "%s\n", item->string);
- fclose(in);
-
- ret = finish_command(&cmd);
+ ret = repack_fill_midx_stdin_packs(&cmd, &include);
done:
if (!ret && opts->write_bitmaps)
remove_redundant_bitmaps(&include, opts->packdir);
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 09/16] repack-midx: factor out `repack_prepare_midx_command()`
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
The `write_midx_included_packs()` function assembles and executes a
`git multi-pack-index write` command, constructing the argument list
inline.
Future commits will introduce additional callers that need to construct
similar `git multi-pack-index` commands (for both `write` and `compact`
subcommands), so extract the common portions of the command setup into a
reusable `repack_prepare_midx_command()` helper.
The extracted helper sets `git_cmd`, pushes the `multi-pack-index`
subcommand and verb, and handles `--progress`/`--no-progress` and
`--bitmap` flags. The remaining arguments that are specific to the
`write` subcommand (such as `--stdin-packs`) are left to the caller.
No functional changes are included in this patch.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
repack-midx.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/repack-midx.c b/repack-midx.c
index 0682b80c427..83151d4734a 100644
--- a/repack-midx.c
+++ b/repack-midx.c
@@ -275,6 +275,23 @@ static void remove_redundant_bitmaps(struct string_list *include,
strbuf_release(&path);
}
+static void repack_prepare_midx_command(struct child_process *cmd,
+ struct repack_write_midx_opts *opts,
+ const char *verb)
+{
+ cmd->git_cmd = 1;
+
+ strvec_pushl(&cmd->args, "multi-pack-index", verb, NULL);
+
+ if (opts->show_progress)
+ strvec_push(&cmd->args, "--progress");
+ else
+ strvec_push(&cmd->args, "--no-progress");
+
+ if (opts->write_bitmaps)
+ strvec_push(&cmd->args, "--bitmap");
+}
+
int write_midx_included_packs(struct repack_write_midx_opts *opts)
{
struct child_process cmd = CHILD_PROCESS_INIT;
@@ -289,18 +306,9 @@ int write_midx_included_packs(struct repack_write_midx_opts *opts)
goto done;
cmd.in = -1;
- cmd.git_cmd = 1;
- strvec_push(&cmd.args, "multi-pack-index");
- strvec_pushl(&cmd.args, "write", "--stdin-packs", NULL);
-
- if (opts->show_progress)
- strvec_push(&cmd.args, "--progress");
- else
- strvec_push(&cmd.args, "--no-progress");
-
- if (opts->write_bitmaps)
- strvec_push(&cmd.args, "--bitmap");
+ repack_prepare_midx_command(&cmd, opts, "write");
+ strvec_push(&cmd.args, "--stdin-packs");
if (preferred)
strvec_pushf(&cmd.args, "--preferred-pack=%s",
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 08/16] midx: expose `midx_layer_contains_pack()`
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
Rename the function `midx_contains_pack_1()` to instead be called
`midx_layer_contains_pack()` and make it accessible. Unlike
`midx_contains_pack()` (which recurses through the entire chain), this
function checks only a single MIDX layer.
This will be used by a subsequent commit to determine whether a given
pack belongs to the tip MIDX layer specifically, rather than to any
layer in the chain.
No functional changes are present in this commit.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx.c | 6 +++---
midx.h | 2 ++
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/midx.c b/midx.c
index bcb8c999015..dc86c8e7fee 100644
--- a/midx.c
+++ b/midx.c
@@ -667,8 +667,8 @@ static int midx_pack_names_cmp(const void *a, const void *b, void *m_)
m->pack_names[*(const size_t *)b]);
}
-static int midx_contains_pack_1(struct multi_pack_index *m,
- const char *idx_or_pack_name)
+int midx_layer_contains_pack(struct multi_pack_index *m,
+ const char *idx_or_pack_name)
{
uint32_t first = 0, last = m->num_packs;
@@ -709,7 +709,7 @@ static int midx_contains_pack_1(struct multi_pack_index *m,
int midx_contains_pack(struct multi_pack_index *m, const char *idx_or_pack_name)
{
for (; m; m = m->base_midx)
- if (midx_contains_pack_1(m, idx_or_pack_name))
+ if (midx_layer_contains_pack(m, idx_or_pack_name))
return 1;
return 0;
}
diff --git a/midx.h b/midx.h
index 77dd66de02b..3ee12dd08ec 100644
--- a/midx.h
+++ b/midx.h
@@ -119,6 +119,8 @@ struct object_id *nth_midxed_object_oid(struct object_id *oid,
int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid, struct pack_entry *e);
int midx_contains_pack(struct multi_pack_index *m,
const char *idx_or_pack_name);
+int midx_layer_contains_pack(struct multi_pack_index *m,
+ const char *idx_or_pack_name);
int midx_preferred_pack(struct multi_pack_index *m, uint32_t *pack_int_id);
int prepare_multi_pack_index_one(struct odb_source *source);
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 07/16] repack: track the ODB source via existing_packs
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
Store the ODB source in the `existing_packs` struct and use that in
place of the raw `repo->objects->sources` access within `cmd_repack()`.
The source used is still assigned from the first source in the list, so
there are no functional changes in this commit. The changes instead
serve two purposes (one immediate, one not):
- The incremental MIDX-based repacking machinery will need to know what
source is being used to read the existing MIDX/chain (should one
exist).
- In the future, if "git repack" is taught how to operate on other
object sources, this field will serve as the authoritative value for
that source.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 5 ++---
repack.c | 2 ++
repack.h | 1 +
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 4c5a82c2c8d..24be147d39a 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -417,7 +417,7 @@ int cmd_repack(int argc,
* midx_has_unknown_packs() will make the decision for
* us.
*/
- if (!get_multi_pack_index(repo->objects->sources))
+ if (!get_multi_pack_index(existing.source))
midx_must_contain_cruft = 1;
}
@@ -564,8 +564,7 @@ int cmd_repack(int argc,
unsigned flags = 0;
if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL, 0))
flags |= MIDX_WRITE_INCREMENTAL;
- write_midx_file(repo->objects->sources,
- NULL, NULL, flags);
+ write_midx_file(existing.source, NULL, NULL, flags);
}
cleanup:
diff --git a/repack.c b/repack.c
index 596841027af..2ee6b51420a 100644
--- a/repack.c
+++ b/repack.c
@@ -154,6 +154,8 @@ void existing_packs_collect(struct existing_packs *existing,
string_list_append(&existing->non_kept_packs, buf.buf);
}
+ existing->source = existing->repo->objects->sources;
+
string_list_sort(&existing->kept_packs);
string_list_sort(&existing->non_kept_packs);
string_list_sort(&existing->cruft_packs);
diff --git a/repack.h b/repack.h
index bc9f2e1a5de..c0e9f0ca647 100644
--- a/repack.h
+++ b/repack.h
@@ -56,6 +56,7 @@ struct packed_git;
struct existing_packs {
struct repository *repo;
+ struct odb_source *source;
struct string_list kept_packs;
struct string_list non_kept_packs;
struct string_list cruft_packs;
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 06/16] midx: support custom `--base` for incremental MIDX writes
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
Both `compact` and `write --incremental` fix the base of the resulting
MIDX layer: `compact` always places the compacted result on top of
"from's" immediate parent in the chain, and `write --incremental` always
appends a new layer to the existing tip. In both cases the base is not
configurable.
Future callers need additional flexibility. For instance, the incremental
MIDX-based repacking code may wish to write a layer based on some
intermediate ancestor rather than the current tip, or produce a root
layer when replacing the bottommost entries in the chain.
Introduce a new `--base` option for both subcommands to specify the
checksum of the MIDX layer to use as the base. The given checksum must
refer to a valid layer in the MIDX chain that is an ancestor of the
topmost layer being written or compacted.
The special value "none" is accepted to produce a root layer with no
parent. This will be needed when the incremental repacking machinery
determines that the bottommost layers of the chain should be replaced.
If no `--base` is given, behavior is unchanged: `compact` uses "from's"
immediate parent in the chain, and `write` appends to the existing tip.
For the `write` subcommand, `--base` requires `--no-write-chain-file`. A plain
`write --incremental` appends a new layer to the live chain tip with no
mechanism to atomically replace it; overriding the base would produce a
layer that does not extend the tip, breaking chain invariants. With
`--no-write-chain-file` the chain is left unmodified and the caller is
responsible for assembling a valid chain.
For `compact`, no such restriction applies. The compaction operation
atomically replaces the compacted range in the chain file, so writing
the result on top of any valid ancestor preserves chain invariants.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/git-multi-pack-index.adoc | 17 +++++-
builtin/multi-pack-index.c | 24 ++++++--
midx-write.c | 34 ++++++++++-
midx.h | 5 +-
t/t5334-incremental-multi-pack-index.sh | 30 ++++++++++
t/t5335-compact-multi-pack-index.sh | 77 +++++++++++++++++++++++++
6 files changed, 178 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-multi-pack-index.adoc b/Documentation/git-multi-pack-index.adoc
index c26196815e2..c6d23aeeb9a 100644
--- a/Documentation/git-multi-pack-index.adoc
+++ b/Documentation/git-multi-pack-index.adoc
@@ -12,8 +12,10 @@ SYNOPSIS
'git multi-pack-index' [<options>] write [--preferred-pack=<pack>]
[--[no-]bitmap] [--[no-]incremental] [--[no-]stdin-packs]
[--refs-snapshot=<path>] [--[no-]write-chain-file]
+ [--base=<checksum>]
'git multi-pack-index' [<options>] compact [--[no-]incremental]
- [--[no-]bitmap] [--[no-]write-chain-file] <from> <to>
+ [--[no-]bitmap] [--base=<checksum>] [--[no-]write-chain-file]
+ <from> <to>
'git multi-pack-index' [<options>] verify
'git multi-pack-index' [<options>] expire
'git multi-pack-index' [<options>] repack [--batch-size=<size>]
@@ -90,6 +92,13 @@ marker).
The checksum of the new layer is printed to standard
output, allowing the caller to assemble and write the
chain itself. Requires `--incremental`.
+
+ --base=<checksum>::
+ Specify the checksum of an existing MIDX layer to use
+ as the base when writing a new incremental layer.
+ The special value `none` indicates that the new layer
+ should have no base (i.e., it becomes a root layer).
+ Requires `--no-write-chain-file`.
--
compact::
@@ -110,6 +119,12 @@ compact::
MIDX layer but do not update the multi-pack-index-chain
file. The checksum of the new layer is printed to
standard output. Requires `--incremental`.
+
+ --base=<checksum>::
+ Specify the checksum of an existing MIDX layer to use
+ as the base for the compacted result, instead of using
+ the immediate parent of `<from>`. The special value
+ `none` indicates that the result should have no base.
--
+
Note that the compact command requires writing a version-2 midx that
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index f861b4b8394..00ffb36394d 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -16,11 +16,13 @@
#define BUILTIN_MIDX_WRITE_USAGE \
N_("git multi-pack-index [<options>] write [--preferred-pack=<pack>]\n" \
" [--[no-]bitmap] [--[no-]incremental] [--[no-]stdin-packs]\n" \
- " [--refs-snapshot=<path>] [--[no-]write-chain-file]")
+ " [--refs-snapshot=<path>] [--[no-]write-chain-file]\n" \
+ " [--base=<checksum>]")
#define BUILTIN_MIDX_COMPACT_USAGE \
N_("git multi-pack-index [<options>] compact [--[no-]incremental]\n" \
- " [--[no-]bitmap] [--[no-]write-chain-file] <from> <to>")
+ " [--[no-]bitmap] [--base=<checksum>] [--[no-]write-chain-file]\n" \
+ " <from> <to>")
#define BUILTIN_MIDX_VERIFY_USAGE \
N_("git multi-pack-index [<options>] verify")
@@ -63,6 +65,7 @@ static char const * const builtin_multi_pack_index_usage[] = {
static struct opts_multi_pack_index {
char *object_dir;
const char *preferred_pack;
+ const char *incremental_base;
char *refs_snapshot;
unsigned long batch_size;
unsigned flags;
@@ -151,6 +154,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,
N_("pack for reuse when computing a multi-pack bitmap")),
OPT_BIT(0, "bitmap", &opts.flags, N_("write multi-pack bitmap"),
MIDX_WRITE_BITMAP | MIDX_WRITE_REV_INDEX),
+ OPT_STRING(0, "base", &opts.incremental_base, N_("checksum"),
+ N_("base MIDX for incremental writes")),
OPT_BIT(0, "incremental", &opts.flags,
N_("write a new incremental MIDX"), MIDX_WRITE_INCREMENTAL),
OPT_NEGBIT(0, "write-chain-file", &opts.flags,
@@ -190,6 +195,13 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,
options);
}
+ if (opts.incremental_base &&
+ !(opts.flags & MIDX_WRITE_NO_CHAIN)) {
+ error(_("cannot use --base without --no-write-chain-file"));
+ usage_with_options(builtin_multi_pack_index_write_usage,
+ options);
+ }
+
source = handle_object_dir_option(repo);
FREE_AND_NULL(options);
@@ -201,7 +213,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,
ret = write_midx_file_only(source, &packs,
opts.preferred_pack,
- opts.refs_snapshot, opts.flags);
+ opts.refs_snapshot,
+ opts.incremental_base, opts.flags);
string_list_clear(&packs, 0);
free(opts.refs_snapshot);
@@ -229,6 +242,8 @@ static int cmd_multi_pack_index_compact(int argc, const char **argv,
struct option *options;
static struct option builtin_multi_pack_index_compact_options[] = {
+ OPT_STRING(0, "base", &opts.incremental_base, N_("checksum"),
+ N_("base MIDX for incremental writes")),
OPT_BIT(0, "bitmap", &opts.flags, N_("write multi-pack bitmap"),
MIDX_WRITE_BITMAP | MIDX_WRITE_REV_INDEX),
OPT_BIT(0, "incremental", &opts.flags,
@@ -290,7 +305,8 @@ static int cmd_multi_pack_index_compact(int argc, const char **argv,
die(_("MIDX %s must be an ancestor of %s"), argv[0], argv[1]);
}
- ret = write_midx_file_compact(source, from_midx, to_midx, opts.flags);
+ ret = write_midx_file_compact(source, from_midx, to_midx,
+ opts.incremental_base, opts.flags);
return ret;
}
diff --git a/midx-write.c b/midx-write.c
index 38c898e5ff5..561e9eedc0e 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1247,6 +1247,7 @@ struct write_midx_opts {
const char *preferred_pack_name;
const char *refs_snapshot;
+ const char *incremental_base;
unsigned flags;
};
@@ -1330,11 +1331,32 @@ static int write_midx_internal(struct write_midx_opts *opts)
/*
* If compacting MIDX layer(s) in the range [from, to], then the
- * compacted MIDX will share the same base MIDX as 'from'.
+ * compacted MIDX will share the same base MIDX as 'from',
+ * unless a custom --base is specified (see below).
*/
if (ctx.compact)
ctx.base_midx = ctx.compact_from->base_midx;
+ if (opts->incremental_base) {
+ if (!strcmp(opts->incremental_base, "none")) {
+ ctx.base_midx = NULL;
+ } else {
+ while (ctx.base_midx) {
+ const char *cmp = midx_get_checksum_hex(ctx.base_midx);
+ if (!strcmp(opts->incremental_base, cmp))
+ break;
+
+ ctx.base_midx = ctx.base_midx->base_midx;
+ }
+
+ if (!ctx.base_midx) {
+ error(_("could not find base MIDX '%s'"),
+ opts->incremental_base);
+ goto cleanup;
+ }
+ }
+ }
+
ctx.nr = 0;
ctx.alloc = ctx.m ? ctx.m->num_packs + ctx.m->num_packs_in_base : 16;
ctx.info = NULL;
@@ -1827,7 +1849,8 @@ static int write_midx_internal(struct write_midx_opts *opts)
int write_midx_file(struct odb_source *source,
const char *preferred_pack_name,
- const char *refs_snapshot, unsigned flags)
+ const char *refs_snapshot,
+ unsigned flags)
{
struct write_midx_opts opts = {
.source = source,
@@ -1842,13 +1865,16 @@ int write_midx_file(struct odb_source *source,
int write_midx_file_only(struct odb_source *source,
struct string_list *packs_to_include,
const char *preferred_pack_name,
- const char *refs_snapshot, unsigned flags)
+ const char *refs_snapshot,
+ const char *incremental_base,
+ unsigned flags)
{
struct write_midx_opts opts = {
.source = source,
.packs_to_include = packs_to_include,
.preferred_pack_name = preferred_pack_name,
.refs_snapshot = refs_snapshot,
+ .incremental_base = incremental_base,
.flags = flags,
};
@@ -1858,12 +1884,14 @@ int write_midx_file_only(struct odb_source *source,
int write_midx_file_compact(struct odb_source *source,
struct multi_pack_index *from,
struct multi_pack_index *to,
+ const char *incremental_base,
unsigned flags)
{
struct write_midx_opts opts = {
.source = source,
.compact_from = from,
.compact_to = to,
+ .incremental_base = incremental_base,
.flags = flags | MIDX_WRITE_COMPACT,
};
diff --git a/midx.h b/midx.h
index 5b193882dcf..77dd66de02b 100644
--- a/midx.h
+++ b/midx.h
@@ -132,10 +132,13 @@ int write_midx_file(struct odb_source *source,
int write_midx_file_only(struct odb_source *source,
struct string_list *packs_to_include,
const char *preferred_pack_name,
- const char *refs_snapshot, unsigned flags);
+ const char *refs_snapshot,
+ const char *incremental_base,
+ unsigned flags);
int write_midx_file_compact(struct odb_source *source,
struct multi_pack_index *from,
struct multi_pack_index *to,
+ const char *incremental_base,
unsigned flags);
void clear_midx_file(struct repository *r);
int verify_midx_file(struct odb_source *source, unsigned flags);
diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
index 66d6894761b..68a103d13d2 100755
--- a/t/t5334-incremental-multi-pack-index.sh
+++ b/t/t5334-incremental-multi-pack-index.sh
@@ -113,6 +113,36 @@ test_expect_success 'write non-incremental MIDX layer with --no-write-chain-file
test_grep "cannot use --no-write-chain-file without --incremental" err
'
+test_expect_success 'write MIDX layer with --base without --no-write-chain-file' '
+ test_must_fail git multi-pack-index write --bitmap --incremental \
+ --base=none 2>err &&
+ test_grep "cannot use --base without --no-write-chain-file" err
+'
+
+test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '
+ test_commit base-none &&
+ git repack -d &&
+
+ cp "$midx_chain" "$midx_chain.bak" &&
+ layer="$(git multi-pack-index write --bitmap --incremental \
+ --no-write-chain-file --base=none)" &&
+
+ test_cmp "$midx_chain.bak" "$midx_chain" &&
+ test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
+'
+
+test_expect_success 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
+ test_commit base-hash &&
+ git repack -d &&
+
+ cp "$midx_chain" "$midx_chain.bak" &&
+ layer="$(git multi-pack-index write --bitmap --incremental \
+ --no-write-chain-file --base="$(nth_line 1 "$midx_chain")")" &&
+
+ test_cmp "$midx_chain.bak" "$midx_chain" &&
+ test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
+'
+
for reuse in false single multi
do
test_expect_success "full clone (pack.allowPackReuse=$reuse)" '
diff --git a/t/t5335-compact-multi-pack-index.sh b/t/t5335-compact-multi-pack-index.sh
index 1a65d48b62b..ec1dafe89fc 100755
--- a/t/t5335-compact-multi-pack-index.sh
+++ b/t/t5335-compact-multi-pack-index.sh
@@ -304,6 +304,7 @@ test_expect_success 'MIDX compaction with --no-write-chain-file' '
layer="$(git multi-pack-index compact --incremental \
--no-write-chain-file \
+ --base="$(nth_line 1 "$midx_chain")" \
"$(nth_line 2 "$midx_chain")" \
"$(nth_line 3 "$midx_chain")")" &&
@@ -326,4 +327,80 @@ test_expect_success 'MIDX compaction with --no-write-chain-file' '
)
'
+test_expect_success 'MIDX compaction with --base' '
+ git init midx-compact-with--base &&
+ (
+ cd midx-compact-with--base &&
+
+ git config maintenance.auto false &&
+
+ write_packs A B C D &&
+
+ test_line_count = 4 "$midx_chain" &&
+
+ cp "$midx_chain" "$midx_chain.bak" &&
+
+ git multi-pack-index compact --incremental \
+ --base="$(nth_line 1 "$midx_chain")" \
+ "$(nth_line 3 "$midx_chain")" \
+ "$(nth_line 4 "$midx_chain")" &&
+ test_line_count = 2 $midx_chain &&
+
+ nth_line 1 "$midx_chain.bak" >expect &&
+ nth_line 1 "$midx_chain" >actual &&
+
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'MIDX compaction with --base=none' '
+ git init midx-compact-base-none &&
+ (
+ cd midx-compact-base-none &&
+
+ git config maintenance.auto false &&
+
+ write_packs A B C D &&
+
+ test_line_count = 4 $midx_chain &&
+
+ cp "$midx_chain" "$midx_chain".bak &&
+
+ # Compact the two bottommost layers (A and B) into a new
+ # root layer with no parent.
+ git multi-pack-index compact --incremental \
+ --base=none \
+ "$(nth_line 1 "$midx_chain")" \
+ "$(nth_line 2 "$midx_chain")" &&
+
+ test_line_count = 3 $midx_chain &&
+
+ # The upper layers (C and D) should be preserved
+ # unchanged.
+ nth_line 3 "$midx_chain.bak" >expect &&
+ nth_line 4 "$midx_chain.bak" >>expect &&
+ nth_line 2 "$midx_chain" >actual &&
+ nth_line 3 "$midx_chain" >>actual &&
+
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'MIDX compaction with bogus --base checksum' '
+ git init midx-compact-bogus-base &&
+ (
+ cd midx-compact-bogus-base &&
+
+ git config maintenance.auto false &&
+
+ write_packs A B C &&
+
+ test_must_fail git multi-pack-index compact --incremental \
+ --base=deadbeef \
+ "$(nth_line 2 "$midx_chain")" \
+ "$(nth_line 3 "$midx_chain")" 2>err &&
+ test_grep "could not find base MIDX" err
+ )
+'
+
test_done
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 05/16] midx: introduce `--no-write-chain-file` for incremental MIDX writes
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
When writing an incremental MIDX layer, the MIDX machinery writes the
new layer into the multi-pack-index.d directory and then updates the
multi-pack-index-chain file to include the freshly written layer.
Future callers however may not wish to immediately update the MIDX chain
itself, preferring instead to write out new layer(s) themselves before
atomically updating the chain. Concretely, the new incremental
MIDX-based repacking strategy will want to do exactly this (that is,
assemble the new MIDX chain itself before writing a new chain file and
atomically linking it into place).
Introduce a `--no-write-chain-file` flag that:
* writes the new MIDX layer into the multi-pack-index.d directory
* prints its checksum
* does not update the multi-pack-index-chain file.
The MIDX chain file (and thus, the lock protecting it) remain untouched,
allowing callers to assemble the chain themselves. This flag requires
`--incremental`, since the notion of a separate layer only makes sense
for incremental MIDXs.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/git-multi-pack-index.adoc | 17 ++++++++--
builtin/multi-pack-index.c | 28 +++++++++++++++--
midx-write.c | 42 ++++++++++++++++---------
midx.h | 1 +
t/t5334-incremental-multi-pack-index.sh | 17 ++++++++++
t/t5335-compact-multi-pack-index.sh | 36 +++++++++++++++++++++
6 files changed, 123 insertions(+), 18 deletions(-)
diff --git a/Documentation/git-multi-pack-index.adoc b/Documentation/git-multi-pack-index.adoc
index 3a5aa227784..c26196815e2 100644
--- a/Documentation/git-multi-pack-index.adoc
+++ b/Documentation/git-multi-pack-index.adoc
@@ -11,9 +11,9 @@ SYNOPSIS
[verse]
'git multi-pack-index' [<options>] write [--preferred-pack=<pack>]
[--[no-]bitmap] [--[no-]incremental] [--[no-]stdin-packs]
- [--refs-snapshot=<path>]
+ [--refs-snapshot=<path>] [--[no-]write-chain-file]
'git multi-pack-index' [<options>] compact [--[no-]incremental]
- [--[no-]bitmap] <from> <to>
+ [--[no-]bitmap] [--[no-]write-chain-file] <from> <to>
'git multi-pack-index' [<options>] verify
'git multi-pack-index' [<options>] expire
'git multi-pack-index' [<options>] repack [--batch-size=<size>]
@@ -83,6 +83,13 @@ marker).
and packs not present in an existing MIDX layer.
Migrates non-incremental MIDXs to incremental ones when
necessary.
+
+ --[no-]write-chain-file::
+ When used with `--incremental`, write a new MIDX layer
+ but do not update the multi-pack-index-chain file.
+ The checksum of the new layer is printed to standard
+ output, allowing the caller to assemble and write the
+ chain itself. Requires `--incremental`.
--
compact::
@@ -97,6 +104,12 @@ compact::
--[no-]bitmap::
Control whether or not a multi-pack bitmap is written.
+
+ --[no-]write-chain-file::
+ When used with `--incremental`, write a new compacted
+ MIDX layer but do not update the multi-pack-index-chain
+ file. The checksum of the new layer is printed to
+ standard output. Requires `--incremental`.
--
+
Note that the compact command requires writing a version-2 midx that
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 0f72d96c02d..f861b4b8394 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -16,11 +16,11 @@
#define BUILTIN_MIDX_WRITE_USAGE \
N_("git multi-pack-index [<options>] write [--preferred-pack=<pack>]\n" \
" [--[no-]bitmap] [--[no-]incremental] [--[no-]stdin-packs]\n" \
- " [--refs-snapshot=<path>]")
+ " [--refs-snapshot=<path>] [--[no-]write-chain-file]")
#define BUILTIN_MIDX_COMPACT_USAGE \
N_("git multi-pack-index [<options>] compact [--[no-]incremental]\n" \
- " [--[no-]bitmap] <from> <to>")
+ " [--[no-]bitmap] [--[no-]write-chain-file] <from> <to>")
#define BUILTIN_MIDX_VERIFY_USAGE \
N_("git multi-pack-index [<options>] verify")
@@ -153,6 +153,9 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,
MIDX_WRITE_BITMAP | MIDX_WRITE_REV_INDEX),
OPT_BIT(0, "incremental", &opts.flags,
N_("write a new incremental MIDX"), MIDX_WRITE_INCREMENTAL),
+ OPT_NEGBIT(0, "write-chain-file", &opts.flags,
+ N_("write the multi-pack-index chain file"),
+ MIDX_WRITE_NO_CHAIN),
OPT_BOOL(0, "stdin-packs", &opts.stdin_packs,
N_("write multi-pack index containing only given indexes")),
OPT_FILENAME(0, "refs-snapshot", &opts.refs_snapshot,
@@ -178,6 +181,15 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,
if (argc)
usage_with_options(builtin_multi_pack_index_write_usage,
options);
+
+ if (opts.flags & MIDX_WRITE_NO_CHAIN &&
+ !(opts.flags & MIDX_WRITE_INCREMENTAL)) {
+ error(_("cannot use %s without %s"),
+ "--no-write-chain-file", "--incremental");
+ usage_with_options(builtin_multi_pack_index_write_usage,
+ options);
+ }
+
source = handle_object_dir_option(repo);
FREE_AND_NULL(options);
@@ -221,6 +233,9 @@ static int cmd_multi_pack_index_compact(int argc, const char **argv,
MIDX_WRITE_BITMAP | MIDX_WRITE_REV_INDEX),
OPT_BIT(0, "incremental", &opts.flags,
N_("write a new incremental MIDX"), MIDX_WRITE_INCREMENTAL),
+ OPT_NEGBIT(0, "write-chain-file", &opts.flags,
+ N_("write the multi-pack-index chain file"),
+ MIDX_WRITE_NO_CHAIN),
OPT_END(),
};
@@ -239,6 +254,15 @@ static int cmd_multi_pack_index_compact(int argc, const char **argv,
if (argc != 2)
usage_with_options(builtin_multi_pack_index_compact_usage,
options);
+
+ if (opts.flags & MIDX_WRITE_NO_CHAIN &&
+ !(opts.flags & MIDX_WRITE_INCREMENTAL)) {
+ error(_("cannot use %s without %s"),
+ "--no-write-chain-file", "--incremental");
+ usage_with_options(builtin_multi_pack_index_compact_usage,
+ options);
+ }
+
source = handle_object_dir_option(the_repository);
FREE_AND_NULL(options);
diff --git a/midx-write.c b/midx-write.c
index 5d9409a9741..38c898e5ff5 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1257,7 +1257,7 @@ static int write_midx_internal(struct write_midx_opts *opts)
unsigned char midx_hash[GIT_MAX_RAWSZ];
uint32_t start_pack;
struct hashfile *f = NULL;
- struct lock_file lk;
+ struct lock_file lk = LOCK_INIT;
struct tempfile *incr;
struct write_midx_context ctx = {
.preferred_pack_idx = NO_PREFERRED_PACK,
@@ -1601,11 +1601,14 @@ static int write_midx_internal(struct write_midx_opts *opts)
}
if (ctx.incremental) {
- struct strbuf lock_name = STRBUF_INIT;
+ if (!(opts->flags & MIDX_WRITE_NO_CHAIN)) {
+ struct strbuf lock_name = STRBUF_INIT;
- get_midx_chain_filename(opts->source, &lock_name);
- hold_lock_file_for_update(&lk, lock_name.buf, LOCK_DIE_ON_ERROR);
- strbuf_release(&lock_name);
+ get_midx_chain_filename(opts->source, &lock_name);
+ hold_lock_file_for_update(&lk, lock_name.buf,
+ LOCK_DIE_ON_ERROR);
+ strbuf_release(&lock_name);
+ }
incr = mks_tempfile_m(midx_name.buf, 0444);
if (!incr) {
@@ -1707,16 +1710,23 @@ static int write_midx_internal(struct write_midx_opts *opts)
if (ctx.num_multi_pack_indexes_before == UINT32_MAX)
die(_("too many multi-pack-indexes"));
+ if (!is_lock_file_locked(&lk))
+ printf("%s\n", hash_to_hex_algop(midx_hash, r->hash_algo));
+ else if (opts->flags & MIDX_WRITE_NO_CHAIN)
+ BUG("lockfile held with MIDX_WRITE_NO_CHAIN set?");
+
if (ctx.incremental) {
- FILE *chainf = fdopen_lock_file(&lk, "w");
struct strbuf final_midx_name = STRBUF_INIT;
struct multi_pack_index *m = ctx.base_midx;
struct multi_pack_index **layers = NULL;
size_t layers_nr = 0, layers_alloc = 0;
- if (!chainf) {
- error_errno(_("unable to open multi-pack-index chain file"));
- goto cleanup;
+ if (is_lock_file_locked(&lk)){
+ FILE *chainf = fdopen_lock_file(&lk, "w");
+ if (!chainf) {
+ error_errno(_("unable to open multi-pack-index chain file"));
+ goto cleanup;
+ }
}
if (link_midx_to_chain(ctx.base_midx) < 0)
@@ -1773,8 +1783,10 @@ static int write_midx_internal(struct write_midx_opts *opts)
free(layers);
- for (size_t i = 0; i < keep_hashes.nr; i++)
- fprintf(get_lock_file_fp(&lk), "%s\n", keep_hashes.v[i]);
+ if (is_lock_file_locked(&lk))
+ for (size_t i = 0; i < keep_hashes.nr; i++)
+ fprintf(get_lock_file_fp(&lk), "%s\n",
+ keep_hashes.v[i]);
} else {
strvec_push(&keep_hashes,
hash_to_hex_algop(midx_hash, r->hash_algo));
@@ -1783,10 +1795,12 @@ static int write_midx_internal(struct write_midx_opts *opts)
if (ctx.m || ctx.base_midx)
odb_close(ctx.repo->objects);
- if (commit_lock_file(&lk) < 0)
- die_errno(_("could not write multi-pack-index"));
+ if (is_lock_file_locked(&lk)) {
+ if (commit_lock_file(&lk) < 0)
+ die_errno(_("could not write multi-pack-index"));
- clear_midx_files(opts->source, &keep_hashes, ctx.incremental);
+ clear_midx_files(opts->source, &keep_hashes, ctx.incremental);
+ }
result = 0;
cleanup:
diff --git a/midx.h b/midx.h
index 08f3728e520..5b193882dcf 100644
--- a/midx.h
+++ b/midx.h
@@ -83,6 +83,7 @@ struct multi_pack_index {
#define MIDX_WRITE_BITMAP_LOOKUP_TABLE (1 << 4)
#define MIDX_WRITE_INCREMENTAL (1 << 5)
#define MIDX_WRITE_COMPACT (1 << 6)
+#define MIDX_WRITE_NO_CHAIN (1 << 7)
#define MIDX_EXT_REV "rev"
#define MIDX_EXT_BITMAP "bitmap"
diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
index c9f5b4e87aa..66d6894761b 100755
--- a/t/t5334-incremental-multi-pack-index.sh
+++ b/t/t5334-incremental-multi-pack-index.sh
@@ -96,6 +96,23 @@ test_expect_success 'show object from second pack' '
git cat-file -p 2.2
'
+test_expect_success 'write MIDX layer with --no-write-chain-file' '
+ test_commit no-write-chain-file &&
+ git repack -d &&
+
+ cp "$midx_chain" "$midx_chain.bak" &&
+ layer="$(git multi-pack-index write --bitmap --incremental \
+ --no-write-chain-file)" &&
+
+ test_cmp "$midx_chain.bak" "$midx_chain" &&
+ test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
+'
+
+test_expect_success 'write non-incremental MIDX layer with --no-write-chain-file' '
+ test_must_fail git multi-pack-index write --bitmap --no-write-chain-file 2>err &&
+ test_grep "cannot use --no-write-chain-file without --incremental" err
+'
+
for reuse in false single multi
do
test_expect_success "full clone (pack.allowPackReuse=$reuse)" '
diff --git a/t/t5335-compact-multi-pack-index.sh b/t/t5335-compact-multi-pack-index.sh
index 40f3844282f..1a65d48b62b 100755
--- a/t/t5335-compact-multi-pack-index.sh
+++ b/t/t5335-compact-multi-pack-index.sh
@@ -290,4 +290,40 @@ test_expect_success 'MIDX compaction with bitmaps (non-trivial)' '
)
'
+test_expect_success 'MIDX compaction with --no-write-chain-file' '
+ git init midx-compact-with--no-write-chain-file &&
+ (
+ cd midx-compact-with--no-write-chain-file &&
+
+ git config maintenance.auto false &&
+
+ write_packs A B C D &&
+
+ test_line_count = 4 $midx_chain &&
+ cp "$midx_chain" "$midx_chain".bak &&
+
+ layer="$(git multi-pack-index compact --incremental \
+ --no-write-chain-file \
+ "$(nth_line 2 "$midx_chain")" \
+ "$(nth_line 3 "$midx_chain")")" &&
+
+ test_cmp "$midx_chain.bak" "$midx_chain" &&
+
+ # After writing the new layer, insert it into the chain
+ # manually. This is done in order to make $layer visible
+ # to the read-midx test helper below, and matches what
+ # the MIDX command would do without --no-write-chain-file.
+ {
+ nth_line 1 "$midx_chain.bak" &&
+ echo $layer &&
+ nth_line 4 "$midx_chain.bak"
+ } >$midx_chain &&
+
+ test-tool read-midx $objdir $layer >midx.data &&
+ grep "^pack-B-.*\.idx" midx.data &&
+ grep "^pack-C-.*\.idx" midx.data
+
+ )
+'
+
test_done
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 04/16] midx: use `strvec` for `keep_hashes`
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
The `keep_hashes` array in `write_midx_internal()` accumulates the
checksums of MIDX files that should be retained when pruning stale
entries from the MIDX chain. For similar reasons as in a previous
commit, rewrite this using a strvec, requiring us to pass one fewer
parameter.
Unlike the aforementioned previous commit, use a `strvec` instead of a
`string_list`, which provides a more ergonomic interface to adjust the
values at a particular index. The ordering is important here, as this
value is used to determine the contents of the resulting
`multi-pack-index-chain` file when writing with "--incremental".
Since the previous commit already builds the array in forward order, the
conversion is straightforward: replace indexed assignments with
`strvec_push()`, drop the pre-counting and `CALLOC_ARRAY()`, and
simplify cleanup via `strvec_clear()`.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx-write.c | 84 ++++++++++++++++++----------------------------------
midx.c | 20 ++++++-------
2 files changed, 38 insertions(+), 66 deletions(-)
diff --git a/midx-write.c b/midx-write.c
index 55c778a97cb..5d9409a9741 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -29,8 +29,7 @@ extern void clear_midx_files_ext(struct odb_source *source, const char *ext,
const char *keep_hash);
extern void clear_incremental_midx_files_ext(struct odb_source *source,
const char *ext,
- const char **keep_hashes,
- uint32_t hashes_nr);
+ const struct strvec *keep_hashes);
extern int cmp_idx_or_pack_name(const char *idx_or_pack_name,
const char *idx_name);
@@ -1109,8 +1108,7 @@ static int link_midx_to_chain(struct multi_pack_index *m)
}
static void clear_midx_files(struct odb_source *source,
- const char **hashes, uint32_t hashes_nr,
- unsigned incremental)
+ const struct strvec *hashes, unsigned incremental)
{
/*
* if incremental:
@@ -1124,13 +1122,15 @@ static void clear_midx_files(struct odb_source *source,
*/
struct strbuf buf = STRBUF_INIT;
const char *exts[] = { MIDX_EXT_BITMAP, MIDX_EXT_REV, MIDX_EXT_MIDX };
- uint32_t i, j;
+ uint32_t i;
for (i = 0; i < ARRAY_SIZE(exts); i++) {
- clear_incremental_midx_files_ext(source, exts[i],
- hashes, hashes_nr);
- for (j = 0; j < hashes_nr; j++)
- clear_midx_files_ext(source, exts[i], hashes[j]);
+ clear_incremental_midx_files_ext(source, exts[i], hashes);
+ if (hashes) {
+ for (size_t j = 0; j < hashes->nr; j++)
+ clear_midx_files_ext(source, exts[i],
+ hashes->v[j]);
+ }
}
if (incremental)
@@ -1267,8 +1267,7 @@ static int write_midx_internal(struct write_midx_opts *opts)
int pack_name_concat_len = 0;
int dropped_packs = 0;
int result = -1;
- const char **keep_hashes = NULL;
- size_t keep_hashes_nr = 0;
+ struct strvec keep_hashes = STRVEC_INIT;
struct chunkfile *cf;
trace2_region_enter("midx", "write_midx_internal", r);
@@ -1708,32 +1707,12 @@ static int write_midx_internal(struct write_midx_opts *opts)
if (ctx.num_multi_pack_indexes_before == UINT32_MAX)
die(_("too many multi-pack-indexes"));
- if (ctx.compact) {
- struct multi_pack_index *m;
-
- /*
- * Keep all MIDX layers excluding those in the range [from, to].
- */
- for (m = ctx.base_midx; m; m = m->base_midx)
- keep_hashes_nr++;
- for (m = ctx.m;
- m && midx_hashcmp(m, ctx.compact_to, r->hash_algo);
- m = m->base_midx)
- keep_hashes_nr++;
-
- keep_hashes_nr++; /* include the compacted layer */
- } else {
- keep_hashes_nr = ctx.num_multi_pack_indexes_before + 1;
- }
- CALLOC_ARRAY(keep_hashes, keep_hashes_nr);
-
if (ctx.incremental) {
FILE *chainf = fdopen_lock_file(&lk, "w");
struct strbuf final_midx_name = STRBUF_INIT;
struct multi_pack_index *m = ctx.base_midx;
struct multi_pack_index **layers = NULL;
size_t layers_nr = 0, layers_alloc = 0;
- size_t j = 0;
if (!chainf) {
error_errno(_("unable to open multi-pack-index chain file"));
@@ -1761,12 +1740,12 @@ static int write_midx_internal(struct write_midx_opts *opts)
layers[layers_nr++] = mp;
}
while (layers_nr)
- keep_hashes[j++] =
- xstrdup(midx_get_checksum_hex(layers[--layers_nr]));
+ strvec_push(&keep_hashes,
+ midx_get_checksum_hex(layers[--layers_nr]));
- keep_hashes[j++] =
- xstrdup(hash_to_hex_algop(midx_hash,
- r->hash_algo));
+ strvec_push(&keep_hashes,
+ hash_to_hex_algop(midx_hash,
+ r->hash_algo));
for (mp = ctx.m;
mp && midx_hashcmp(mp, ctx.compact_to,
@@ -1776,31 +1755,29 @@ static int write_midx_internal(struct write_midx_opts *opts)
layers[layers_nr++] = mp;
}
while (layers_nr)
- keep_hashes[j++] =
- xstrdup(midx_get_checksum_hex(layers[--layers_nr]));
+ strvec_push(&keep_hashes,
+ midx_get_checksum_hex(layers[--layers_nr]));
} else {
for (; m; m = m->base_midx) {
ALLOC_GROW(layers, layers_nr + 1, layers_alloc);
layers[layers_nr++] = m;
}
while (layers_nr)
- keep_hashes[j++] =
- xstrdup(midx_get_checksum_hex(layers[--layers_nr]));
+ strvec_push(&keep_hashes,
+ midx_get_checksum_hex(layers[--layers_nr]));
- keep_hashes[j++] =
- xstrdup(hash_to_hex_algop(midx_hash,
- r->hash_algo));
+ strvec_push(&keep_hashes,
+ hash_to_hex_algop(midx_hash,
+ r->hash_algo));
}
- ASSERT(j == keep_hashes_nr);
-
free(layers);
- for (uint32_t i = 0; i < j; i++)
- fprintf(get_lock_file_fp(&lk), "%s\n", keep_hashes[i]);
+ for (size_t i = 0; i < keep_hashes.nr; i++)
+ fprintf(get_lock_file_fp(&lk), "%s\n", keep_hashes.v[i]);
} else {
- keep_hashes[ctx.num_multi_pack_indexes_before] =
- xstrdup(hash_to_hex_algop(midx_hash, r->hash_algo));
+ strvec_push(&keep_hashes,
+ hash_to_hex_algop(midx_hash, r->hash_algo));
}
if (ctx.m || ctx.base_midx)
@@ -1809,8 +1786,7 @@ static int write_midx_internal(struct write_midx_opts *opts)
if (commit_lock_file(&lk) < 0)
die_errno(_("could not write multi-pack-index"));
- clear_midx_files(opts->source, keep_hashes, keep_hashes_nr,
- ctx.incremental);
+ clear_midx_files(opts->source, &keep_hashes, ctx.incremental);
result = 0;
cleanup:
@@ -1826,11 +1802,7 @@ static int write_midx_internal(struct write_midx_opts *opts)
free(ctx.entries);
free(ctx.pack_perm);
free(ctx.pack_order);
- if (keep_hashes) {
- for (uint32_t i = 0; i < keep_hashes_nr; i++)
- free((char *)keep_hashes[i]);
- free(keep_hashes);
- }
+ strvec_clear(&keep_hashes);
strbuf_release(&midx_name);
close_midx(midx_to_free);
diff --git a/midx.c b/midx.c
index f75e3c9fa6d..bcb8c999015 100644
--- a/midx.c
+++ b/midx.c
@@ -12,6 +12,7 @@
#include "chunk-format.h"
#include "pack-bitmap.h"
#include "pack-revindex.h"
+#include "strvec.h"
#define MIDX_PACK_ERROR ((void *)(intptr_t)-1)
@@ -19,8 +20,7 @@ int midx_checksum_valid(struct multi_pack_index *m);
void clear_midx_files_ext(struct odb_source *source, const char *ext,
const char *keep_hash);
void clear_incremental_midx_files_ext(struct odb_source *source, const char *ext,
- char **keep_hashes,
- uint32_t hashes_nr);
+ const struct strvec *keep_hashes);
int cmp_idx_or_pack_name(const char *idx_or_pack_name,
const char *idx_name);
@@ -799,22 +799,22 @@ void clear_midx_files_ext(struct odb_source *source, const char *ext,
}
void clear_incremental_midx_files_ext(struct odb_source *source, const char *ext,
- char **keep_hashes,
- uint32_t hashes_nr)
+ const struct strvec *keep_hashes)
{
struct clear_midx_data data = {
.keep = STRSET_INIT,
.ext = ext,
};
struct strbuf buf = STRBUF_INIT;
- uint32_t i;
- for (i = 0; i < hashes_nr; i++) {
- strbuf_reset(&buf);
- strbuf_addf(&buf, "multi-pack-index-%s.%s", keep_hashes[i],
- ext);
+ if (keep_hashes) {
+ for (size_t i = 0; i < keep_hashes->nr; i++) {
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "multi-pack-index-%s.%s",
+ keep_hashes->v[i], ext);
- strset_add(&data.keep, buf.buf);
+ strset_add(&data.keep, buf.buf);
+ }
}
for_each_file_in_pack_subdir(source->path, "multi-pack-index.d",
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 03/16] midx: build `keep_hashes` array in order
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
Instead of filling the keep_hashes array using reverse indexing (e.g.,
`keep_hashes[count - i - 1]`) while traversing linked lists forward,
collect linked list nodes into a temporary `layers` array and then
iterate it backwards to fill `keep_hashes` sequentially.
This makes the filling logic easier to follow, since each segment of the
array is filled with a simple forward-marching index. Moreover, this
change prepares us for a subsequent commit that will switch to using a
`strvec`.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx-write.c | 66 ++++++++++++++++++++++++++++------------------------
1 file changed, 36 insertions(+), 30 deletions(-)
diff --git a/midx-write.c b/midx-write.c
index 9328f65a201..55c778a97cb 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1731,6 +1731,9 @@ static int write_midx_internal(struct write_midx_opts *opts)
FILE *chainf = fdopen_lock_file(&lk, "w");
struct strbuf final_midx_name = STRBUF_INIT;
struct multi_pack_index *m = ctx.base_midx;
+ struct multi_pack_index **layers = NULL;
+ size_t layers_nr = 0, layers_alloc = 0;
+ size_t j = 0;
if (!chainf) {
error_errno(_("unable to open multi-pack-index chain file"));
@@ -1751,46 +1754,49 @@ static int write_midx_internal(struct write_midx_opts *opts)
strbuf_release(&final_midx_name);
if (ctx.compact) {
- struct multi_pack_index *m;
- uint32_t num_layers_before_from = 0;
- uint32_t i;
+ struct multi_pack_index *mp;
- for (m = ctx.base_midx; m; m = m->base_midx)
- num_layers_before_from++;
-
- m = ctx.base_midx;
- for (i = 0; i < num_layers_before_from; i++) {
- uint32_t j = num_layers_before_from - i - 1;
-
- keep_hashes[j] = xstrdup(midx_get_checksum_hex(m));
- m = m->base_midx;
+ for (mp = ctx.base_midx; mp; mp = mp->base_midx) {
+ ALLOC_GROW(layers, layers_nr + 1, layers_alloc);
+ layers[layers_nr++] = mp;
}
+ while (layers_nr)
+ keep_hashes[j++] =
+ xstrdup(midx_get_checksum_hex(layers[--layers_nr]));
- keep_hashes[i] = xstrdup(hash_to_hex_algop(midx_hash,
- r->hash_algo));
+ keep_hashes[j++] =
+ xstrdup(hash_to_hex_algop(midx_hash,
+ r->hash_algo));
- i = 0;
- for (m = ctx.m;
- m && midx_hashcmp(m, ctx.compact_to, r->hash_algo);
- m = m->base_midx) {
- keep_hashes[keep_hashes_nr - i - 1] =
- xstrdup(midx_get_checksum_hex(m));
- i++;
+ for (mp = ctx.m;
+ mp && midx_hashcmp(mp, ctx.compact_to,
+ r->hash_algo);
+ mp = mp->base_midx) {
+ ALLOC_GROW(layers, layers_nr + 1, layers_alloc);
+ layers[layers_nr++] = mp;
}
+ while (layers_nr)
+ keep_hashes[j++] =
+ xstrdup(midx_get_checksum_hex(layers[--layers_nr]));
} else {
- keep_hashes[ctx.num_multi_pack_indexes_before] =
+ for (; m; m = m->base_midx) {
+ ALLOC_GROW(layers, layers_nr + 1, layers_alloc);
+ layers[layers_nr++] = m;
+ }
+ while (layers_nr)
+ keep_hashes[j++] =
+ xstrdup(midx_get_checksum_hex(layers[--layers_nr]));
+
+ keep_hashes[j++] =
xstrdup(hash_to_hex_algop(midx_hash,
r->hash_algo));
-
- for (uint32_t i = 0; i < ctx.num_multi_pack_indexes_before; i++) {
- uint32_t j = ctx.num_multi_pack_indexes_before - i - 1;
-
- keep_hashes[j] = xstrdup(midx_get_checksum_hex(m));
- m = m->base_midx;
- }
}
- for (uint32_t i = 0; i < keep_hashes_nr; i++)
+ ASSERT(j == keep_hashes_nr);
+
+ free(layers);
+
+ for (uint32_t i = 0; i < j; i++)
fprintf(get_lock_file_fp(&lk), "%s\n", keep_hashes[i]);
} else {
keep_hashes[ctx.num_multi_pack_indexes_before] =
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 02/16] midx: use `strset` for retained MIDX files
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
Both `clear_midx_files_ext()` and `clear_incremental_midx_files_ext()`
build a list of filenames to keep while pruning stale MIDX files. Today
they hand-roll an array instead of using a `strset`, thus requiring us
to pass an additional length parameter, and makes lookups linear.
Replace the bare array with a `strset` which can be passed around as a
single parameter. Though it improves lookup performance, the difference
is likely immeasurable given how small the keep_hashes array typically
is.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx.c | 57 +++++++++++++++++++++++++++------------------------------
1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/midx.c b/midx.c
index 81d6ab11e6e..f75e3c9fa6d 100644
--- a/midx.c
+++ b/midx.c
@@ -758,8 +758,7 @@ int midx_checksum_valid(struct multi_pack_index *m)
}
struct clear_midx_data {
- char **keep;
- uint32_t keep_nr;
+ struct strset keep;
const char *ext;
};
@@ -767,15 +766,12 @@ static void clear_midx_file_ext(const char *full_path, size_t full_path_len UNUS
const char *file_name, void *_data)
{
struct clear_midx_data *data = _data;
- uint32_t i;
if (!(starts_with(file_name, "multi-pack-index-") &&
ends_with(file_name, data->ext)))
return;
- for (i = 0; i < data->keep_nr; i++) {
- if (!strcmp(data->keep[i], file_name))
- return;
- }
+ if (strset_contains(&data->keep, file_name))
+ return;
if (unlink(full_path))
die_errno(_("failed to remove %s"), full_path);
}
@@ -783,48 +779,49 @@ static void clear_midx_file_ext(const char *full_path, size_t full_path_len UNUS
void clear_midx_files_ext(struct odb_source *source, const char *ext,
const char *keep_hash)
{
- struct clear_midx_data data;
- memset(&data, 0, sizeof(struct clear_midx_data));
+ struct clear_midx_data data = {
+ .keep = STRSET_INIT,
+ .ext = ext,
+ };
if (keep_hash) {
- ALLOC_ARRAY(data.keep, 1);
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addf(&buf, "multi-pack-index-%s.%s", keep_hash, ext);
- data.keep[0] = xstrfmt("multi-pack-index-%s.%s", keep_hash, ext);
- data.keep_nr = 1;
+ strset_add(&data.keep, buf.buf);
+
+ strbuf_release(&buf);
}
- data.ext = ext;
- for_each_file_in_pack_dir(source->path,
- clear_midx_file_ext,
- &data);
+ for_each_file_in_pack_dir(source->path, clear_midx_file_ext, &data);
- if (keep_hash)
- free(data.keep[0]);
- free(data.keep);
+ strset_clear(&data.keep);
}
void clear_incremental_midx_files_ext(struct odb_source *source, const char *ext,
char **keep_hashes,
uint32_t hashes_nr)
{
- struct clear_midx_data data;
+ struct clear_midx_data data = {
+ .keep = STRSET_INIT,
+ .ext = ext,
+ };
+ struct strbuf buf = STRBUF_INIT;
uint32_t i;
- memset(&data, 0, sizeof(struct clear_midx_data));
+ for (i = 0; i < hashes_nr; i++) {
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "multi-pack-index-%s.%s", keep_hashes[i],
+ ext);
- ALLOC_ARRAY(data.keep, hashes_nr);
- for (i = 0; i < hashes_nr; i++)
- data.keep[i] = xstrfmt("multi-pack-index-%s.%s", keep_hashes[i],
- ext);
- data.keep_nr = hashes_nr;
- data.ext = ext;
+ strset_add(&data.keep, buf.buf);
+ }
for_each_file_in_pack_subdir(source->path, "multi-pack-index.d",
clear_midx_file_ext, &data);
- for (i = 0; i < hashes_nr; i++)
- free(data.keep[i]);
- free(data.keep);
+ strbuf_release(&buf);
+ strset_clear(&data.keep);
}
void clear_midx_file(struct repository *r)
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 01/16] midx-write: handle noop writes when converting incremental chains
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1777507303.git.me@ttaylorr.com>
When updating a MIDX, we optimize out writes that will result in an
identical MIDX as the one we already have on disk. See b3bab9d2729
(midx-write: extract function to test whether MIDX needs updating,
2025-12-10) for more details on exactly which writes are optimized out.
If `midx_needs_update()` can't rule out any of the obvious cases (e.g.,
the checksum is invalid, we're requesting a different version, or
performing compaction which always requires an update), then we compare
the packs we're writing to the packs we already know about. If there are
an equal number of packs being written as there are in any existing
MIDX layer(s), then we compare the packs by their name.
This comparison fails when we have an incremental MIDX chain with
at least two layers, since we do not recursively peel through earlier
layers, instead treating the `->pack_names` array of the tip MIDX layer
as containing all `m->num_packs + m->num_packs_in_base` packs.
Adjust this to instead look through the MIDX layers one by one when
comparing pack names. While we're at it, fix a typo above in the same
function.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx-write.c | 18 ++++++++++--------
t/t5334-incremental-multi-pack-index.sh | 16 ++++++++++++++++
2 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/midx-write.c b/midx-write.c
index a25cab75aba..9328f65a201 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1152,7 +1152,7 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c
/*
* Ensure that we have a valid checksum before consulting the
- * exisiting MIDX in order to determine if we can avoid an
+ * existing MIDX in order to determine if we can avoid an
* update.
*
* This is necessary because the given MIDX is loaded directly
@@ -1208,14 +1208,16 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c
BUG("same pack added twice?");
}
- for (uint32_t i = 0; i < ctx->nr; i++) {
- strbuf_reset(&buf);
- strbuf_addstr(&buf, midx->pack_names[i]);
- strbuf_strip_suffix(&buf, ".idx");
+ for (struct multi_pack_index *m = midx; m; m = m->base_midx) {
+ for (uint32_t i = 0; i < m->num_packs; i++) {
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, m->pack_names[i]);
+ strbuf_strip_suffix(&buf, ".idx");
- if (!strset_contains(&packs, buf.buf))
- goto out;
- strset_remove(&packs, buf.buf);
+ if (!strset_contains(&packs, buf.buf))
+ goto out;
+ strset_remove(&packs, buf.buf);
+ }
}
needed = false;
diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
index 99c7d44d8e9..c9f5b4e87aa 100755
--- a/t/t5334-incremental-multi-pack-index.sh
+++ b/t/t5334-incremental-multi-pack-index.sh
@@ -132,4 +132,20 @@ test_expect_success 'relink existing MIDX layer' '
'
+test_expect_success 'non-incremental write with existing incremental chain' '
+ git init non-incremental-write-with-existing &&
+ test_when_finished "rm -fr non-incremental-write-with-existing" &&
+
+ (
+ cd non-incremental-write-with-existing &&
+
+ git config set maintenance.auto false &&
+
+ write_midx_layer &&
+ write_midx_layer &&
+
+ git multi-pack-index write
+ )
+'
+
test_done
--
2.54.0.16.g1c05dfce579
^ permalink raw reply related
* [PATCH v3 00/16] repack: incremental MIDX/bitmap-based repacking
From: Taylor Blau @ 2026-04-30 0:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <cover.1774820449.git.me@ttaylorr.com>
This a small reroll of my series to implement the last remaining
component of the incremental MIDX/bitmap-based repacking strategy that I
have been working on.
I expect that this should be the final reroll absent any late-breaking
feedback. The only changes since last time are the following:
- Various stale references to "--checksum-only" have been eradicated
from commit message(s).
- `repack_prepare_midx_command()` now uses `pipe_command()` to
eliminate the possibility of a deadlock.
- `write_midx_included_packs()` now cleans up after itself properly
when receiving multiple lines of output.
- The errant test in t7705 was moved to the final commit (where it
belongs) instead of the penultimate one.
As usual, a range-diff is included below as well for convenience. Thanks
in advance for reviewing!
Taylor Blau (16):
midx-write: handle noop writes when converting incremental chains
midx: use `strset` for retained MIDX files
midx: build `keep_hashes` array in order
midx: use `strvec` for `keep_hashes`
midx: introduce `--no-write-chain-file` for incremental MIDX writes
midx: support custom `--base` for incremental MIDX writes
repack: track the ODB source via existing_packs
midx: expose `midx_layer_contains_pack()`
repack-midx: factor out `repack_prepare_midx_command()`
repack-midx: extract `repack_fill_midx_stdin_packs()`
repack-geometry: prepare for incremental MIDX repacking
builtin/repack.c: convert `--write-midx` to an `OPT_CALLBACK`
packfile: ensure `close_pack_revindex()` frees in-memory revindex
repack: implement incremental MIDX repacking
repack: introduce `--write-midx=incremental`
repack: allow `--write-midx=incremental` without `--geometric`
Documentation/config/repack.adoc | 18 +
Documentation/git-multi-pack-index.adoc | 32 +-
Documentation/git-repack.adoc | 44 +-
builtin/multi-pack-index.c | 48 +-
builtin/repack.c | 102 +++-
midx-write.c | 206 ++++---
midx.c | 104 ++--
midx.h | 11 +-
packfile.c | 2 +
repack-geometry.c | 48 +-
repack-midx.c | 710 +++++++++++++++++++++++-
repack.c | 58 +-
repack.h | 26 +-
t/meson.build | 1 +
t/t5334-incremental-multi-pack-index.sh | 63 +++
t/t5335-compact-multi-pack-index.sh | 113 ++++
t/t7705-repack-incremental-midx.sh | 525 ++++++++++++++++++
17 files changed, 1911 insertions(+), 200 deletions(-)
create mode 100755 t/t7705-repack-incremental-midx.sh
Range-diff against v2:
-: ----------- > 1: d6c27317c25 midx-write: handle noop writes when converting incremental chains
-: ----------- > 2: 629c8d23116 midx: use `strset` for retained MIDX files
-: ----------- > 3: e303bf6a4ac midx: build `keep_hashes` array in order
-: ----------- > 4: 42d76c70060 midx: use `strvec` for `keep_hashes`
-: ----------- > 5: 2c80aa34fac midx: introduce `--no-write-chain-file` for incremental MIDX writes
7: d9acef1334a ! 6: 2a05f4b86f3 repack: allow `--write-midx=incremental` without `--geometric`
@@ Metadata
Author: Taylor Blau <me@ttaylorr.com>
## Commit message ##
- repack: allow `--write-midx=incremental` without `--geometric`
+ midx: support custom `--base` for incremental MIDX writes
- Previously, `--write-midx=incremental` required `--geometric` and would
- die() without it. Relax this restriction so that incremental MIDX
- repacking can be used independently.
+ Both `compact` and `write --incremental` fix the base of the resulting
+ MIDX layer: `compact` always places the compacted result on top of
+ "from's" immediate parent in the chain, and `write --incremental` always
+ appends a new layer to the existing tip. In both cases the base is not
+ configurable.
- Without `--geometric`, the behavior is append-only: a single new MIDX
- layer is created containing whatever packs were written by the repack
- and appended to the existing chain (or a new chain is started). Existing
- layers are preserved as-is with no compaction or merging.
+ Future callers need additional flexibility. For instance, the incremental
+ MIDX-based repacking code may wish to write a layer based on some
+ intermediate ancestor rather than the current tip, or produce a root
+ layer when replacing the bottommost entries in the chain.
- Implement this via a new repack_make_midx_append_plan() that builds a
- plan consisting of a WRITE step for the freshly written packs followed
- by COPY steps for every existing MIDX layer. The existing compaction
- plan (repack_make_midx_compaction_plan) is used only when `--geometric`
- is active.
+ Introduce a new `--base` option for both subcommands to specify the
+ checksum of the MIDX layer to use as the base. The given checksum must
+ refer to a valid layer in the MIDX chain that is an ancestor of the
+ topmost layer being written or compacted.
- Update the documentation to describe the behavior with and without
- `--geometric`, and replace the test that enforced the old restriction
- with one exercising append-only incremental MIDX repacking.
+ The special value "none" is accepted to produce a root layer with no
+ parent. This will be needed when the incremental repacking machinery
+ determines that the bottommost layers of the chain should be replaced.
+
+ If no `--base` is given, behavior is unchanged: `compact` uses "from's"
+ immediate parent in the chain, and `write` appends to the existing tip.
+
+ For the `write` subcommand, `--base` requires `--no-write-chain-file`. A plain
+ `write --incremental` appends a new layer to the live chain tip with no
+ mechanism to atomically replace it; overriding the base would produce a
+ layer that does not extend the tip, breaking chain invariants. With
+ `--no-write-chain-file` the chain is left unmodified and the caller is
+ responsible for assembling a valid chain.
+
+ For `compact`, no such restriction applies. The compaction operation
+ atomically replaces the compacted range in the chain file, so writing
+ the result on top of any valid ancestor preserves chain invariants.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
- ## Documentation/git-repack.adoc ##
-@@ Documentation/git-repack.adoc: linkgit:git-multi-pack-index[1]).
+ ## Documentation/git-multi-pack-index.adoc ##
+@@ Documentation/git-multi-pack-index.adoc: SYNOPSIS
+ 'git multi-pack-index' [<options>] write [--preferred-pack=<pack>]
+ [--[no-]bitmap] [--[no-]incremental] [--[no-]stdin-packs]
+ [--refs-snapshot=<path>] [--[no-]write-chain-file]
++ [--base=<checksum>]
+ 'git multi-pack-index' [<options>] compact [--[no-]incremental]
+- [--[no-]bitmap] [--[no-]write-chain-file] <from> <to>
++ [--[no-]bitmap] [--base=<checksum>] [--[no-]write-chain-file]
++ <from> <to>
+ 'git multi-pack-index' [<options>] verify
+ 'git multi-pack-index' [<options>] expire
+ 'git multi-pack-index' [<options>] repack [--batch-size=<size>]
+@@ Documentation/git-multi-pack-index.adoc: marker).
+ The checksum of the new layer is printed to standard
+ output, allowing the caller to assemble and write the
+ chain itself. Requires `--incremental`.
++
++ --base=<checksum>::
++ Specify the checksum of an existing MIDX layer to use
++ as the base when writing a new incremental layer.
++ The special value `none` indicates that the new layer
++ should have no base (i.e., it becomes a root layer).
++ Requires `--no-write-chain-file`.
+ --
- `incremental`;;
- Write an incremental MIDX chain instead of a single
-- flat MIDX. This mode requires `--geometric`.
-+ flat MIDX.
- +
--The incremental mode maintains a chain of MIDX layers that is compacted
--over time using a geometric merging strategy. Each repack creates a new
--tip layer containing the newly written pack(s). Adjacent layers are then
--merged whenever the newer layer's object count exceeds
--`1/repack.midxSplitFactor` of the next deeper layer's count. Layers
--that do not meet this condition are retained as-is.
-+Without `--geometric`, a new MIDX layer is appended to the existing
-+chain (or a new chain is started) containing whatever packs were written
-+by the repack. Existing layers are preserved as-is.
-++
-+When combined with `--geometric`, the incremental mode maintains a chain
-+of MIDX layers that is compacted over time using a geometric merging
-+strategy. Each repack creates a new tip layer containing the newly
-+written pack(s). Adjacent layers are then merged whenever the newer
-+layer's object count exceeds `1/repack.midxSplitFactor` of the next
-+deeper layer's count. Layers that do not meet this condition are
-+retained as-is.
+ compact::
+@@ Documentation/git-multi-pack-index.adoc: compact::
+ MIDX layer but do not update the multi-pack-index-chain
+ file. The checksum of the new layer is printed to
+ standard output. Requires `--incremental`.
++
++ --base=<checksum>::
++ Specify the checksum of an existing MIDX layer to use
++ as the base for the compacted result, instead of using
++ the immediate parent of `<from>`. The special value
++ `none` indicates that the result should have no base.
+ --
+
- The result is that newer (tip) layers tend to contain many small packs
- with relatively few objects, while older (deeper) layers contain fewer,
+ Note that the compact command requires writing a version-2 midx that
- ## builtin/repack.c ##
-@@ builtin/repack.c: int cmd_repack(int argc,
- if (pack_everything & PACK_CRUFT)
- pack_everything |= ALL_INTO_ONE;
-
-- if (write_midx == REPACK_WRITE_MIDX_INCREMENTAL && !geometry.split_factor)
-- die(_("--write-midx=incremental requires --geometric"));
--
- if (write_bitmaps < 0) {
- if (write_midx == REPACK_WRITE_MIDX_NONE &&
- (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
-
- ## repack-midx.c ##
-@@ repack-midx.c: static void midx_compaction_step_release(struct midx_compaction_step *step)
- free(step->csum);
- }
+ ## builtin/multi-pack-index.c ##
+@@
+ #define BUILTIN_MIDX_WRITE_USAGE \
+ N_("git multi-pack-index [<options>] write [--preferred-pack=<pack>]\n" \
+ " [--[no-]bitmap] [--[no-]incremental] [--[no-]stdin-packs]\n" \
+- " [--refs-snapshot=<path>] [--[no-]write-chain-file]")
++ " [--refs-snapshot=<path>] [--[no-]write-chain-file]\n" \
++ " [--base=<checksum>]")
-+/*
-+ * Build an append-only MIDX plan: a single WRITE step for the freshly
-+ * written packs, plus COPY steps for every existing layer. No
-+ * compaction or merging is performed.
-+ */
-+static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,
-+ struct midx_compaction_step **steps_p,
-+ size_t *steps_nr_p)
-+{
-+ struct multi_pack_index *m;
-+ struct midx_compaction_step *steps = NULL;
-+ struct midx_compaction_step *step;
-+ size_t steps_nr = 0, steps_alloc = 0;
-+
-+ odb_reprepare(opts->existing->repo->objects);
-+ m = get_multi_pack_index(opts->existing->source);
+ #define BUILTIN_MIDX_COMPACT_USAGE \
+ N_("git multi-pack-index [<options>] compact [--[no-]incremental]\n" \
+- " [--[no-]bitmap] [--[no-]write-chain-file] <from> <to>")
++ " [--[no-]bitmap] [--base=<checksum>] [--[no-]write-chain-file]\n" \
++ " <from> <to>")
+
+ #define BUILTIN_MIDX_VERIFY_USAGE \
+ N_("git multi-pack-index [<options>] verify")
+@@ builtin/multi-pack-index.c: static char const * const builtin_multi_pack_index_usage[] = {
+ static struct opts_multi_pack_index {
+ char *object_dir;
+ const char *preferred_pack;
++ const char *incremental_base;
+ char *refs_snapshot;
+ unsigned long batch_size;
+ unsigned flags;
+@@ builtin/multi-pack-index.c: static int cmd_multi_pack_index_write(int argc, const char **argv,
+ N_("pack for reuse when computing a multi-pack bitmap")),
+ OPT_BIT(0, "bitmap", &opts.flags, N_("write multi-pack bitmap"),
+ MIDX_WRITE_BITMAP | MIDX_WRITE_REV_INDEX),
++ OPT_STRING(0, "base", &opts.incremental_base, N_("checksum"),
++ N_("base MIDX for incremental writes")),
+ OPT_BIT(0, "incremental", &opts.flags,
+ N_("write a new incremental MIDX"), MIDX_WRITE_INCREMENTAL),
+ OPT_NEGBIT(0, "write-chain-file", &opts.flags,
+@@ builtin/multi-pack-index.c: static int cmd_multi_pack_index_write(int argc, const char **argv,
+ options);
+ }
+
++ if (opts.incremental_base &&
++ !(opts.flags & MIDX_WRITE_NO_CHAIN)) {
++ error(_("cannot use --base without --no-write-chain-file"));
++ usage_with_options(builtin_multi_pack_index_write_usage,
++ options);
++ }
+
-+ if (opts->names->nr) {
-+ struct strbuf buf = STRBUF_INIT;
-+ uint32_t i;
+ source = handle_object_dir_option(repo);
+
+ FREE_AND_NULL(options);
+@@ builtin/multi-pack-index.c: static int cmd_multi_pack_index_write(int argc, const char **argv,
+
+ ret = write_midx_file_only(source, &packs,
+ opts.preferred_pack,
+- opts.refs_snapshot, opts.flags);
++ opts.refs_snapshot,
++ opts.incremental_base, opts.flags);
+
+ string_list_clear(&packs, 0);
+ free(opts.refs_snapshot);
+@@ builtin/multi-pack-index.c: static int cmd_multi_pack_index_compact(int argc, const char **argv,
+
+ struct option *options;
+ static struct option builtin_multi_pack_index_compact_options[] = {
++ OPT_STRING(0, "base", &opts.incremental_base, N_("checksum"),
++ N_("base MIDX for incremental writes")),
+ OPT_BIT(0, "bitmap", &opts.flags, N_("write multi-pack bitmap"),
+ MIDX_WRITE_BITMAP | MIDX_WRITE_REV_INDEX),
+ OPT_BIT(0, "incremental", &opts.flags,
+@@ builtin/multi-pack-index.c: static int cmd_multi_pack_index_compact(int argc, const char **argv,
+ die(_("MIDX %s must be an ancestor of %s"), argv[0], argv[1]);
+ }
+
+- ret = write_midx_file_compact(source, from_midx, to_midx, opts.flags);
++ ret = write_midx_file_compact(source, from_midx, to_midx,
++ opts.incremental_base, opts.flags);
+
+ return ret;
+ }
+
+ ## midx-write.c ##
+@@ midx-write.c: struct write_midx_opts {
+
+ const char *preferred_pack_name;
+ const char *refs_snapshot;
++ const char *incremental_base;
+ unsigned flags;
+ };
+
+@@ midx-write.c: static int write_midx_internal(struct write_midx_opts *opts)
+
+ /*
+ * If compacting MIDX layer(s) in the range [from, to], then the
+- * compacted MIDX will share the same base MIDX as 'from'.
++ * compacted MIDX will share the same base MIDX as 'from',
++ * unless a custom --base is specified (see below).
+ */
+ if (ctx.compact)
+ ctx.base_midx = ctx.compact_from->base_midx;
+
++ if (opts->incremental_base) {
++ if (!strcmp(opts->incremental_base, "none")) {
++ ctx.base_midx = NULL;
++ } else {
++ while (ctx.base_midx) {
++ const char *cmp = midx_get_checksum_hex(ctx.base_midx);
++ if (!strcmp(opts->incremental_base, cmp))
++ break;
+
-+ ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);
++ ctx.base_midx = ctx.base_midx->base_midx;
++ }
+
-+ step = &steps[steps_nr++];
-+ memset(step, 0, sizeof(*step));
++ if (!ctx.base_midx) {
++ error(_("could not find base MIDX '%s'"),
++ opts->incremental_base);
++ goto cleanup;
++ }
++ }
++ }
+
-+ step->type = MIDX_COMPACTION_STEP_WRITE;
-+ string_list_init_dup(&step->u.write);
+ ctx.nr = 0;
+ ctx.alloc = ctx.m ? ctx.m->num_packs + ctx.m->num_packs_in_base : 16;
+ ctx.info = NULL;
+@@ midx-write.c: static int write_midx_internal(struct write_midx_opts *opts)
+
+ int write_midx_file(struct odb_source *source,
+ const char *preferred_pack_name,
+- const char *refs_snapshot, unsigned flags)
++ const char *refs_snapshot,
++ unsigned flags)
+ {
+ struct write_midx_opts opts = {
+ .source = source,
+@@ midx-write.c: int write_midx_file(struct odb_source *source,
+ int write_midx_file_only(struct odb_source *source,
+ struct string_list *packs_to_include,
+ const char *preferred_pack_name,
+- const char *refs_snapshot, unsigned flags)
++ const char *refs_snapshot,
++ const char *incremental_base,
++ unsigned flags)
+ {
+ struct write_midx_opts opts = {
+ .source = source,
+ .packs_to_include = packs_to_include,
+ .preferred_pack_name = preferred_pack_name,
+ .refs_snapshot = refs_snapshot,
++ .incremental_base = incremental_base,
+ .flags = flags,
+ };
+
+@@ midx-write.c: int write_midx_file_only(struct odb_source *source,
+ int write_midx_file_compact(struct odb_source *source,
+ struct multi_pack_index *from,
+ struct multi_pack_index *to,
++ const char *incremental_base,
+ unsigned flags)
+ {
+ struct write_midx_opts opts = {
+ .source = source,
+ .compact_from = from,
+ .compact_to = to,
++ .incremental_base = incremental_base,
+ .flags = flags | MIDX_WRITE_COMPACT,
+ };
+
+
+ ## midx.h ##
+@@ midx.h: int write_midx_file(struct odb_source *source,
+ int write_midx_file_only(struct odb_source *source,
+ struct string_list *packs_to_include,
+ const char *preferred_pack_name,
+- const char *refs_snapshot, unsigned flags);
++ const char *refs_snapshot,
++ const char *incremental_base,
++ unsigned flags);
+ int write_midx_file_compact(struct odb_source *source,
+ struct multi_pack_index *from,
+ struct multi_pack_index *to,
++ const char *incremental_base,
+ unsigned flags);
+ void clear_midx_file(struct repository *r);
+ int verify_midx_file(struct odb_source *source, unsigned flags);
+
+ ## t/t5334-incremental-multi-pack-index.sh ##
+@@ t/t5334-incremental-multi-pack-index.sh: test_expect_success 'write non-incremental MIDX layer with --no-write-chain-file
+ test_grep "cannot use --no-write-chain-file without --incremental" err
+ '
+
++test_expect_success 'write MIDX layer with --base without --no-write-chain-file' '
++ test_must_fail git multi-pack-index write --bitmap --incremental \
++ --base=none 2>err &&
++ test_grep "cannot use --base without --no-write-chain-file" err
++'
+
-+ for (i = 0; i < opts->names->nr; i++) {
-+ strbuf_reset(&buf);
-+ strbuf_addf(&buf, "pack-%s.idx",
-+ opts->names->items[i].string);
-+ string_list_append(&step->u.write, buf.buf);
-+ }
++test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '
++ test_commit base-none &&
++ git repack -d &&
+
-+ strbuf_release(&buf);
-+ }
++ cp "$midx_chain" "$midx_chain.bak" &&
++ layer="$(git multi-pack-index write --bitmap --incremental \
++ --no-write-chain-file --base=none)" &&
+
-+ for (; m; m = m->base_midx) {
-+ ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);
++ test_cmp "$midx_chain.bak" "$midx_chain" &&
++ test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
++'
+
-+ step = &steps[steps_nr++];
-+ memset(step, 0, sizeof(*step));
++test_expect_success 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
++ test_commit base-hash &&
++ git repack -d &&
+
-+ step->type = MIDX_COMPACTION_STEP_COPY;
-+ step->u.copy = m;
-+ step->objects_nr = m->num_objects;
-+ }
++ cp "$midx_chain" "$midx_chain.bak" &&
++ layer="$(git multi-pack-index write --bitmap --incremental \
++ --no-write-chain-file --base="$(nth_line 1 "$midx_chain")")" &&
+
-+ *steps_p = steps;
-+ *steps_nr_p = steps_nr;
-+}
++ test_cmp "$midx_chain.bak" "$midx_chain" &&
++ test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
++'
+
- static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts,
- struct midx_compaction_step **steps_p,
- size_t *steps_nr_p)
-@@ repack-midx.c: static int write_midx_incremental(struct repack_write_midx_opts *opts)
- goto done;
- }
+ for reuse in false single multi
+ do
+ test_expect_success "full clone (pack.allowPackReuse=$reuse)" '
+
+ ## t/t5335-compact-multi-pack-index.sh ##
+@@ t/t5335-compact-multi-pack-index.sh: test_expect_success 'MIDX compaction with --no-write-chain-file' '
-- if (repack_make_midx_compaction_plan(opts, &steps, &steps_nr) < 0) {
-- ret = error(_("unable to generate compaction plan"));
-- goto done;
-+ if (opts->geometry->split_factor) {
-+ if (repack_make_midx_compaction_plan(opts, &steps, &steps_nr) < 0) {
-+ ret = error(_("unable to generate compaction plan"));
-+ goto done;
-+ }
-+ } else {
-+ repack_make_midx_append_plan(opts, &steps, &steps_nr);
- }
+ layer="$(git multi-pack-index compact --incremental \
+ --no-write-chain-file \
++ --base="$(nth_line 1 "$midx_chain")" \
+ "$(nth_line 2 "$midx_chain")" \
+ "$(nth_line 3 "$midx_chain")")" &&
- for (i = 0; i < steps_nr; i++) {
-
- ## t/t7705-repack-incremental-midx.sh ##
-@@ t/t7705-repack-incremental-midx.sh: create_layers () {
- done
- }
+@@ t/t5335-compact-multi-pack-index.sh: test_expect_success 'MIDX compaction with --no-write-chain-file' '
+ )
+ '
--test_expect_success '--write-midx=incremental requires --geometric' '
-- test_must_fail git repack --write-midx=incremental 2>err &&
-+test_expect_success '--write-midx=incremental without --geometric' '
-+ git init incremental-without-geometric &&
++test_expect_success 'MIDX compaction with --base' '
++ git init midx-compact-with--base &&
+ (
-+ cd incremental-without-geometric &&
-
-- test_grep -- "--write-midx=incremental requires --geometric" err
++ cd midx-compact-with--base &&
++
+ git config maintenance.auto false &&
+
-+ test_commit first &&
-+ git repack -d &&
++ write_packs A B C D &&
+
-+ test_commit second &&
-+ git repack --write-midx=incremental &&
++ test_line_count = 4 "$midx_chain" &&
+
-+ git multi-pack-index verify &&
-+ test_line_count = 1 $midx_chain &&
-+ cp $midx_chain $midx_chain.before &&
++ cp "$midx_chain" "$midx_chain.bak" &&
+
-+ # A second repack appends a new layer without
-+ # disturbing the existing one.
-+ test_commit third &&
-+ git repack --write-midx=incremental &&
-+
-+ git multi-pack-index verify &&
++ git multi-pack-index compact --incremental \
++ --base="$(nth_line 1 "$midx_chain")" \
++ "$(nth_line 3 "$midx_chain")" \
++ "$(nth_line 4 "$midx_chain")" &&
+ test_line_count = 2 $midx_chain &&
-+ head -n 1 $midx_chain.before >expect &&
-+ head -n 1 $midx_chain >actual &&
-+ test_cmp expect actual &&
+
-+ git fsck
++ nth_line 1 "$midx_chain.bak" >expect &&
++ nth_line 1 "$midx_chain" >actual &&
++
++ test_cmp expect actual
+ )
- '
-
- test_expect_success 'below layer threshold, tip packs excluded' '
-@@ t/t7705-repack-incremental-midx.sh: test_expect_success 'kept packs are excluded from repack' '
- # entirely, so no rollup occurs as there is only one
- # non-kept pack. A new MIDX layer is written containing
- # that pack.
-- git repack --geometric=2 -d --write-midx=incremental \
-- --write-bitmap-index &&
-+ git repack --geometric=2 -d --write-midx=incremental &&
-
- test-tool read-midx $objdir >actual &&
- grep "^pack-.*\.idx$" actual >actual.packs &&
++'
++
++test_expect_success 'MIDX compaction with --base=none' '
++ git init midx-compact-base-none &&
++ (
++ cd midx-compact-base-none &&
++
++ git config maintenance.auto false &&
++
++ write_packs A B C D &&
++
++ test_line_count = 4 $midx_chain &&
++
++ cp "$midx_chain" "$midx_chain".bak &&
++
++ # Compact the two bottommost layers (A and B) into a new
++ # root layer with no parent.
++ git multi-pack-index compact --incremental \
++ --base=none \
++ "$(nth_line 1 "$midx_chain")" \
++ "$(nth_line 2 "$midx_chain")" &&
++
++ test_line_count = 3 $midx_chain &&
++
++ # The upper layers (C and D) should be preserved
++ # unchanged.
++ nth_line 3 "$midx_chain.bak" >expect &&
++ nth_line 4 "$midx_chain.bak" >>expect &&
++ nth_line 2 "$midx_chain" >actual &&
++ nth_line 3 "$midx_chain" >>actual &&
++
++ test_cmp expect actual
++ )
++'
++
++test_expect_success 'MIDX compaction with bogus --base checksum' '
++ git init midx-compact-bogus-base &&
++ (
++ cd midx-compact-bogus-base &&
++
++ git config maintenance.auto false &&
++
++ write_packs A B C &&
++
++ test_must_fail git multi-pack-index compact --incremental \
++ --base=deadbeef \
++ "$(nth_line 2 "$midx_chain")" \
++ "$(nth_line 3 "$midx_chain")" 2>err &&
++ test_grep "could not find base MIDX" err
++ )
++'
++
+ test_done
-: ----------- > 7: 92aba3d366f repack: track the ODB source via existing_packs
-: ----------- > 8: d3ac65c1f11 midx: expose `midx_layer_contains_pack()`
-: ----------- > 9: 1bd2f194c6f repack-midx: factor out `repack_prepare_midx_command()`
1: 44f522ea04d ! 10: 2a87a1e4561 repack-midx: extract `repack_fill_midx_stdin_packs()`
@@ Commit message
This simplifies `write_midx_included_packs()` and prepares for a
subsequent commit where the same helper is called with `cmd->out = -1`
to capture the MIDX's checksum from the command's standard output,
- which is needed when writing MIDX layers with `--checksum-only`.
+ which is needed when writing MIDX layers with `--no-write-chain-file`.
No functional changes are included in this patch.
2: f5642a46bbd = 11: 3d32b9c88da repack-geometry: prepare for incremental MIDX repacking
3: 9fdcb253a96 = 12: 1f7a5479bb8 builtin/repack.c: convert `--write-midx` to an `OPT_CALLBACK`
4: 1e1b957bf12 = 13: b155f25d53c packfile: ensure `close_pack_revindex()` frees in-memory revindex
5: 93e152fb6aa ! 14: ef012314930 repack: implement incremental MIDX repacking
@@ Commit message
Unlike the default mode which writes a single flat MIDX, the incremental
mode constructs a compaction plan that determines which MIDX layers to
write, compact, or copy, and then executes each step using `git
- multi-pack-index` subcommands with the --checksum-only flag.
+ multi-pack-index` subcommands with the --no-write-chain-file flag.
The repacking strategy works as follows:
@@ Commit message
After writing the new layer, the strategy is evaluated among the
existing MIDX layers in order from oldest to newest. Each step that
- writes a new MIDX layer uses "--checksum-only" to avoid updating the
- multi-pack-index-chain file. After all steps are complete, the new chain
- file is written and then atomically moved into place.
+ writes a new MIDX layer uses "--no-write-chain-file" to avoid updating
+ the multi-pack-index-chain file. After all steps are complete, the new
+ chain file is written and then atomically moved into place.
At present, this functionality is exposed behind a new enum value,
`REPACK_WRITE_MIDX_INCREMENTAL`, but has no external callers. A
@@ repack-midx.c: static void repack_prepare_midx_command(struct child_process *cmd
+ struct string_list *include,
+ struct string_list *out)
{
++ struct strbuf in_buf = STRBUF_INIT;
++ struct strbuf out_buf = STRBUF_INIT;
struct string_list_item *item;
- FILE *in;
+- FILE *in;
int ret;
- cmd->in = -1;
-+ if (out)
-+ cmd->out = -1;
-
+- cmd->in = -1;
+-
strvec_push(&cmd->args, "--stdin-packs");
-@@ repack-midx.c: static int repack_fill_midx_stdin_packs(struct child_process *cmd,
- fprintf(in, "%s\n", item->string);
- fclose(in);
+- ret = start_command(cmd);
+- if (ret)
+- return ret;
+-
+- in = xfdopen(cmd->in, "w");
+ for_each_string_list_item(item, include)
+- fprintf(in, "%s\n", item->string);
+- fclose(in);
++ strbuf_addf(&in_buf, "%s\n", item->string);
-+ if (out) {
-+ struct strbuf buf = STRBUF_INIT;
-+ FILE *outf = xfdopen(cmd->out, "r");
+- return finish_command(cmd);
++ ret = pipe_command(cmd, in_buf.buf, in_buf.len,
++ out ? &out_buf : NULL, 0, NULL, 0);
+
-+ while (strbuf_getline(&buf, outf) != EOF)
-+ string_list_append(out, buf.buf);
-+ strbuf_release(&buf);
++ if (out)
++ string_list_split_f(out, out_buf.buf, "\n", -1,
++ STRING_LIST_SPLIT_NONEMPTY);
+
-+ fclose(outf);
-+ }
++ strbuf_release(&in_buf);
++ strbuf_release(&out_buf);
+
- return finish_command(cmd);
++ return ret;
}
+ static int write_midx_included_packs(struct repack_write_midx_opts *opts)
@@ repack-midx.c: static int write_midx_included_packs(struct repack_write_midx_opts *opts)
strvec_pushf(&cmd.args, "--refs-snapshot=%s",
opts->refs_snapshot);
@@ repack-midx.c: static int write_midx_included_packs(struct repack_write_midx_opt
+ while (strbuf_getline_lf(&buf, out) != EOF) {
+ if (step->csum) {
+ ret = error(_("unexpected MIDX output: '%s'"), buf.buf);
++ fclose(out);
++ out = NULL;
++ finish_command(&cmd);
+ goto out;
+ }
+ step->csum = strbuf_detach(&buf, NULL);
6: 6119f15d3e8 ! 15: 04cfecd5136 repack: introduce `--write-midx=incremental`
@@ t/t7705-repack-incremental-midx.sh (new)
+ )
+'
+
-+test_expect_success 'repack -ad --write-midx=incremental is safe' '
-+ git init ad-incremental-midx &&
-+ (
-+ cd ad-incremental-midx &&
-+
-+ git config maintenance.auto false &&
-+
-+ # Build a MIDX chain with multiple layers referencing
-+ # distinct packs.
-+ test_commit first &&
-+ git repack -d &&
-+
-+ test_commit second &&
-+ git repack -d --write-midx=incremental &&
-+
-+ git multi-pack-index verify &&
-+ test_line_count = 1 $midx_chain &&
-+
-+ # Now do a full -ad repack. The new pack contains all
-+ # objects, but any retained MIDX layers still reference
-+ # the now-deleted packs.
-+ test_commit third &&
-+ git repack -ad --write-midx=incremental &&
-+
-+ git multi-pack-index verify &&
-+ git fsck &&
-+ git rev-list --all --objects >/dev/null
-+ )
-+'
-+
+test_expect_success 'repack rejects invalid midxSplitFactor' '
+ test_when_finished "rm -fr bad-split-factor" &&
+ git init bad-split-factor &&
-: ----------- > 16: 1c05dfce579 repack: allow `--write-midx=incremental` without `--geometric`
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
--
2.54.0.16.g1c05dfce579
^ permalink raw reply
* Re: [PATCH v2 14/16] repack: implement incremental MIDX repacking
From: Taylor Blau @ 2026-04-29 23:39 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Elijah Newren, Patrick Steinhardt
In-Reply-To: <20260429081017.GB1269182@coredump.intra.peff.net>
On Wed, Apr 29, 2026 at 04:10:17AM -0400, Jeff King wrote:
> On Tue, Apr 21, 2026 at 04:37:54PM -0400, Taylor Blau wrote:
>
> > Unlike the default mode which writes a single flat MIDX, the incremental
> > mode constructs a compaction plan that determines which MIDX layers to
> > write, compact, or copy, and then executes each step using `git
> > multi-pack-index` subcommands with the --checksum-only flag.
>
> This should be --no-write-chain-file, I think.
>
> Ditto here:
Yup, thanks for spotting.
> > After writing the new layer, the strategy is evaluated among the
> > existing MIDX layers in order from oldest to newest. Each step that
> > writes a new MIDX layer uses "--checksum-only" to avoid updating the
> > multi-pack-index-chain file. After all steps are complete, the new chain
> > file is written and then atomically moved into place.
>
> In the code I think it is all good, though:
>
> > + strvec_pushl(&cmd.args, "--incremental", "--no-write-chain-file", NULL);
Heh. Clearly I 'git grep'd through the code, but didn't adjust the
commit messages. These should all be fixed, and now by the end of the
series we have:
$ git log -p @{u}.. | grep -c ..checksum.only
0
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 14/16] repack: implement incremental MIDX repacking
From: Taylor Blau @ 2026-04-29 23:36 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Elijah Newren, Patrick Steinhardt
In-Reply-To: <20260429075150.GA1267476@coredump.intra.peff.net>
On Wed, Apr 29, 2026 at 03:51:50AM -0400, Jeff King wrote:
> > @@ -312,6 +319,17 @@ static int repack_fill_midx_stdin_packs(struct child_process *cmd,
> > fprintf(in, "%s\n", item->string);
> > fclose(in);
> >
> > + if (out) {
> > + struct strbuf buf = STRBUF_INIT;
> > + FILE *outf = xfdopen(cmd->out, "r");
> > +
> > + while (strbuf_getline(&buf, outf) != EOF)
> > + string_list_append(out, buf.buf);
> > + strbuf_release(&buf);
> > +
> > + fclose(outf);
> > + }
>
> Is it possible to deadlock here where we block writing to the child, but
> the child is blocked trying to write back to us. It's probably quite
> unlikely as it implies both pipe buffers are filled up (and we are
> counting packs and midx hashes here, neither of which we'd expect to be
> too numerous).
>
> Using pipe_command() would solve this, but it also might be impossible
> to trigger if the child reads all input before generating any output. I
> _think_ that's the case looking at cmd_multi_pack_index_write(). So
> we're OK, but you might want to double check.
I think in theory this could deadlock, though in practice it is highly
unlikely. Regardless, using pipe_command() here is straightforward, and
guarantees that we'll avoid a nasty deadlock, so let's do that.
The resulting code is a lot easier to read, too, which is nice:
--- 8< ---
diff --git a/repack-midx.c b/repack-midx.c
index 8f3720772b8..9db59b18334 100644
--- a/repack-midx.c
+++ b/repack-midx.c
@@ -300,37 +300,27 @@ static int repack_fill_midx_stdin_packs(struct child_process *cmd,
struct string_list *include,
struct string_list *out)
{
+ struct strbuf in_buf = STRBUF_INIT;
+ struct strbuf out_buf = STRBUF_INIT;
struct string_list_item *item;
- FILE *in;
int ret;
- cmd->in = -1;
- if (out)
- cmd->out = -1;
-
strvec_push(&cmd->args, "--stdin-packs");
- ret = start_command(cmd);
- if (ret)
- return ret;
-
- in = xfdopen(cmd->in, "w");
for_each_string_list_item(item, include)
- fprintf(in, "%s\n", item->string);
- fclose(in);
+ strbuf_addf(&in_buf, "%s\n", item->string);
- if (out) {
- struct strbuf buf = STRBUF_INIT;
- FILE *outf = xfdopen(cmd->out, "r");
+ ret = pipe_command(cmd, in_buf.buf, in_buf.len,
+ out ? &out_buf : NULL, 0, NULL, 0);
- while (strbuf_getline(&buf, outf) != EOF)
- string_list_append(out, buf.buf);
- strbuf_release(&buf);
+ if (out)
+ string_list_split_f(out, out_buf.buf, "\n", -1,
+ STRING_LIST_SPLIT_NONEMPTY);
- fclose(outf);
- }
+ strbuf_release(&in_buf);
+ strbuf_release(&out_buf);
- return finish_command(cmd);
+ return ret;
}
static int write_midx_included_packs(struct repack_write_midx_opts *opts)
--- >8 ---
> > +static int midx_compaction_step_exec_compact(struct midx_compaction_step *step,
> > + struct repack_write_midx_opts *opts)
> > +{
> > [...]
> > + ret = start_command(&cmd);
> > + if (ret)
> > + goto out;
>
> OK, if we get an error here we'll jump to "out", but run-command.c will
> have cleaned up everything about "cmd" that we need.
>
> But later...
>
> > + out = xfdopen(cmd.out, "r");
> > + while (strbuf_getline_lf(&buf, out) != EOF) {
> > + if (step->csum) {
> > + ret = error(_("unexpected MIDX output: '%s'"), buf.buf);
> > + goto out;
> > + }
>
> ...here we jump to "out" but the command is left running. I guess it
> will eventually get SIGPIPE when we fclose(out), but of course we won't
> wait() for it and we're probably leaking some memory, too.
Good catch, I fixed this up in the way you suggested to call
`fclose(out)` and `finish_command()` within this loop before jumping
out.
Thanks,
Taylor
^ permalink raw reply related
* Git maintenance fails without meaningful error message if any remote is no longer available
From: Anselm Schüler @ 2026-04-29 23:13 UTC (permalink / raw)
To: git
I have a repo with multiple remotes, one of which no longer exists. When
git-maintenance runs on it, it fails during the prefetch stage because
that remote doesn’t exist anymore, and gives a mostly unhelpful error
message:
$ git maintenance run --schedule=daily
ERROR: Repository not found.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
error: failed to prefetch remotes
error: task 'prefetch' failed
I think that
1. git-maintenance should report which remote it’s encountering an error on
2. git-maintenance should continue fetching other remotes even if one fails
Now, on my system, the systemd timers for git-maintenance use
git-for-each-repo. Not sure if that’s upstream behaviour or something
Nix/home-manager does. But if it is upstream behaviour, it would also be
great to report the repo the error comes from, since I basically had to
guess right now which repo was erroring. Luckily I have only three repos
under maintenance so that was fine.
Let me know if you agree that this should be done. I would be open to
writing a patch (no promises though)
Anselm
^ permalink raw reply
* Re: [PATCH v2 10/16] repack-midx: extract `repack_fill_midx_stdin_packs()`
From: Taylor Blau @ 2026-04-29 22:40 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Elijah Newren, Patrick Steinhardt
In-Reply-To: <20260429080821.GA1269182@coredump.intra.peff.net>
On Wed, Apr 29, 2026 at 04:08:21AM -0400, Jeff King wrote:
> On Tue, Apr 21, 2026 at 04:37:42PM -0400, Taylor Blau wrote:
>
> > This simplifies `write_midx_included_packs()` and prepares for a
> > subsequent commit where the same helper is called with `cmd->out = -1`
> > to capture the MIDX's checksum from the command's standard output,
> > which is needed when writing MIDX layers with `--checksum-only`.
>
> This should be --no-write-chain-file now, right? It's not used in the
> code here, so it's just a commit message fixup.
Yup, good spotting. Fixed.
Thanks,
Taylor
^ permalink raw reply
* [PATCH v6 6/6] xdiff/xdl_cleanup_records: make execution of action easier to follow
From: Ezekiel Newren via GitGitGadget @ 2026-04-29 22:08 UTC (permalink / raw)
To: git
Cc: Yee Cheng Chin, Phillip Wood, René Scharfe, Jeff King,
D. Ben Knoble, SZEDER Gábor, Ezekiel Newren, Ezekiel Newren
In-Reply-To: <pull.2156.v6.git.git.1777500495.gitgitgadget@gmail.com>
From: Ezekiel Newren <ezekielnewren@gmail.com>
Helped-by: Phillip Wood
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
---
xdiff/xprepare.c | 40 ++++++++++++++++++++++++++++++----------
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index ddd0577676..beef711067 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -336,24 +336,44 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
*/
xdf1->nreff = 0;
for (i = xdf1->dstart; i <= xdf1->dend; i++) {
- if (action1[i] == KEEP ||
- (action1[i] == INVESTIGATE && !xdl_clean_mmatch(action1, i, xdf1->dstart, xdf1->dend))) {
+ uint8_t action = action1[i];
+
+ if (action == INVESTIGATE) {
+ if (!xdl_clean_mmatch(action1, i, xdf1->dstart, xdf1->dend))
+ action = KEEP;
+ else
+ action = DISCARD;
+ }
+
+ if (action == KEEP) {
xdf1->reference_index[xdf1->nreff++] = i;
- /* changed[i] remains false, i.e. keep */
- } else
+ /* changed[i] remains false */
+ } else if (action == DISCARD) {
xdf1->changed[i] = true;
- /* i.e. discard */
+ } else {
+ BUG("Illegal state for action");
+ }
}
xdf2->nreff = 0;
for (i = xdf2->dstart; i <= xdf2->dend; i++) {
- if (action2[i] == KEEP ||
- (action2[i] == INVESTIGATE && !xdl_clean_mmatch(action2, i, xdf2->dstart, xdf2->dend))) {
+ uint8_t action = action2[i];
+
+ if (action == INVESTIGATE) {
+ if (!xdl_clean_mmatch(action2, i, xdf2->dstart, xdf2->dend))
+ action = KEEP;
+ else
+ action = DISCARD;
+ }
+
+ if (action == KEEP) {
xdf2->reference_index[xdf2->nreff++] = i;
- /* changed[i] remains false, i.e. keep */
- } else
+ /* changed[i] remains false */
+ } else if (action == DISCARD) {
xdf2->changed[i] = true;
- /* i.e. discard */
+ } else {
+ BUG("Illegal state for action");
+ }
}
cleanup:
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 5/6] xdiff/xdl_cleanup_records: make setting action easier to follow
From: Ezekiel Newren via GitGitGadget @ 2026-04-29 22:08 UTC (permalink / raw)
To: git
Cc: Yee Cheng Chin, Phillip Wood, René Scharfe, Jeff King,
D. Ben Knoble, SZEDER Gábor, Ezekiel Newren, Ezekiel Newren
In-Reply-To: <pull.2156.v6.git.git.1777500495.gitgitgadget@gmail.com>
From: Ezekiel Newren <ezekielnewren@gmail.com>
Rewrite nested ternaries with a clear if/else ladder for
action1/action2 to improve readability while preserving
behavior.
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
---
xdiff/xprepare.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index 7141dbc058..ddd0577676 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -302,7 +302,12 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
size_t mph1 = xdf1->recs[i].minimal_perfect_hash;
rcrec = cf->rcrecs[mph1];
nm = rcrec ? rcrec->len2 : 0;
- action1[i] = (nm == 0) ? DISCARD: nm >= mlim1 ? INVESTIGATE: KEEP;
+ if (nm == 0)
+ action1[i] = DISCARD;
+ else if (nm < mlim1)
+ action1[i] = KEEP;
+ else /* nm >= mlim1 */
+ action1[i] = INVESTIGATE;
}
if (need_min) {
@@ -317,7 +322,12 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
size_t mph2 = xdf2->recs[i].minimal_perfect_hash;
rcrec = cf->rcrecs[mph2];
nm = rcrec ? rcrec->len1 : 0;
- action2[i] = (nm == 0) ? DISCARD: nm >= mlim2 ? INVESTIGATE: KEEP;
+ if (nm == 0)
+ action2[i] = DISCARD;
+ else if (nm < mlim2)
+ action2[i] = KEEP;
+ else /* nm >= mlim2 */
+ action2[i] = INVESTIGATE;
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 4/6] xdiff/xdl_cleanup_records: make limits more clear
From: Ezekiel Newren via GitGitGadget @ 2026-04-29 22:08 UTC (permalink / raw)
To: git
Cc: Yee Cheng Chin, Phillip Wood, René Scharfe, Jeff King,
D. Ben Knoble, SZEDER Gábor, Ezekiel Newren, Ezekiel Newren
In-Reply-To: <pull.2156.v6.git.git.1777500495.gitgitgadget@gmail.com>
From: Ezekiel Newren <ezekielnewren@gmail.com>
Make the handling of per-file limits and the minimal-case clearer.
* Use explicit per-file limit variables (mlim1, mlim2) and initialize
them.
* The additional condition `!need_min` is redudant now, remove it.
Best viewed with --color-words.
Helped-by: Phillip Wood
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
---
xdiff/xprepare.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index 386668a92d..7141dbc058 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -268,7 +268,7 @@ static bool xdl_clean_mmatch(uint8_t const *action, ptrdiff_t i, ptrdiff_t s, pt
* might be potentially discarded if they appear in a run of discardable.
*/
static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {
- ptrdiff_t i, nm, mlim;
+ ptrdiff_t i, nm, mlim1, mlim2;
xdlclass_t *rcrec;
uint8_t *action1 = NULL, *action2 = NULL;
bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
@@ -290,22 +290,34 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
/*
* Initialize temporary arrays with DISCARD, KEEP, or INVESTIGATE.
*/
- if ((mlim = (long)xdl_bogosqrt((uint64_t)xdf1->nrec)) > XDL_MAX_EQLIMIT)
- mlim = XDL_MAX_EQLIMIT;
+ if (need_min) {
+ /* i.e. infinity */
+ mlim1 = PTRDIFF_MAX;
+ } else {
+ mlim1 = xdl_bogosqrt((uint64_t)xdf1->nrec);
+ if (mlim1 > XDL_MAX_EQLIMIT)
+ mlim1 = XDL_MAX_EQLIMIT;
+ }
for (i = xdf1->dstart; i <= xdf1->dend; i++) {
size_t mph1 = xdf1->recs[i].minimal_perfect_hash;
rcrec = cf->rcrecs[mph1];
nm = rcrec ? rcrec->len2 : 0;
- action1[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
+ action1[i] = (nm == 0) ? DISCARD: nm >= mlim1 ? INVESTIGATE: KEEP;
}
- if ((mlim = (long)xdl_bogosqrt((uint64_t)xdf2->nrec)) > XDL_MAX_EQLIMIT)
- mlim = XDL_MAX_EQLIMIT;
+ if (need_min) {
+ /* i.e. infinity */
+ mlim2 = PTRDIFF_MAX;
+ } else {
+ mlim2 = xdl_bogosqrt((uint64_t)xdf2->nrec);
+ if (mlim2 > XDL_MAX_EQLIMIT)
+ mlim2 = XDL_MAX_EQLIMIT;
+ }
for (i = xdf2->dstart; i <= xdf2->dend; i++) {
size_t mph2 = xdf2->recs[i].minimal_perfect_hash;
rcrec = cf->rcrecs[mph2];
nm = rcrec ? rcrec->len1 : 0;
- action2[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
+ action2[i] = (nm == 0) ? DISCARD: nm >= mlim2 ? INVESTIGATE: KEEP;
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 3/6] xdiff/xdl_cleanup_records: use unambiguous types
From: Ezekiel Newren via GitGitGadget @ 2026-04-29 22:08 UTC (permalink / raw)
To: git
Cc: Yee Cheng Chin, Phillip Wood, René Scharfe, Jeff King,
D. Ben Knoble, SZEDER Gábor, Ezekiel Newren, Ezekiel Newren
In-Reply-To: <pull.2156.v6.git.git.1777500495.gitgitgadget@gmail.com>
From: Ezekiel Newren <ezekielnewren@gmail.com>
Change the parameters of xdl_clean_mmatch() and the local variables
i, nm, mlim in xdl_cleanup_records() to use unambiguous types. Best
viewed with --color-words.
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
---
xdiff/xprepare.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index 48fb5ce6fe..386668a92d 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -197,8 +197,8 @@ void xdl_free_env(xdfenv_t *xe) {
}
-static bool xdl_clean_mmatch(uint8_t const *action, long i, long s, long e) {
- long r, rdis0, rpdis0, rdis1, rpdis1;
+static bool xdl_clean_mmatch(uint8_t const *action, ptrdiff_t i, ptrdiff_t s, ptrdiff_t e) {
+ ptrdiff_t r, rdis0, rpdis0, rdis1, rpdis1;
/*
* Limits the window that is examined during the similar-lines
@@ -268,7 +268,7 @@ static bool xdl_clean_mmatch(uint8_t const *action, long i, long s, long e) {
* might be potentially discarded if they appear in a run of discardable.
*/
static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {
- long i, nm, mlim;
+ ptrdiff_t i, nm, mlim;
xdlclass_t *rcrec;
uint8_t *action1 = NULL, *action2 = NULL;
bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 2/6] xdiff: use unambiguous types in xdl_bogo_sqrt()
From: Ezekiel Newren via GitGitGadget @ 2026-04-29 22:08 UTC (permalink / raw)
To: git
Cc: Yee Cheng Chin, Phillip Wood, René Scharfe, Jeff King,
D. Ben Knoble, SZEDER Gábor, Ezekiel Newren, Ezekiel Newren
In-Reply-To: <pull.2156.v6.git.git.1777500495.gitgitgadget@gmail.com>
From: Ezekiel Newren <ezekielnewren@gmail.com>
There is no real square root for a negative number and size_t may not
be large enough for certain applications, replace long with uint64_t.
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
---
xdiff/xdiffi.c | 2 +-
xdiff/xprepare.c | 4 ++--
xdiff/xutils.c | 4 ++--
xdiff/xutils.h | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 4376f943db..88708c12a3 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -348,7 +348,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
kvdf += xe->xdf2.nreff + 1;
kvdb += xe->xdf2.nreff + 1;
- xenv.mxcost = xdl_bogosqrt(ndiags);
+ xenv.mxcost = (long)xdl_bogosqrt((uint64_t)ndiags);
if (xenv.mxcost < XDL_MAX_COST_MIN)
xenv.mxcost = XDL_MAX_COST_MIN;
xenv.snake_cnt = XDL_SNAKE_CNT;
diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index d6e1901d2d..48fb5ce6fe 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -290,7 +290,7 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
/*
* Initialize temporary arrays with DISCARD, KEEP, or INVESTIGATE.
*/
- if ((mlim = xdl_bogosqrt((long)xdf1->nrec)) > XDL_MAX_EQLIMIT)
+ if ((mlim = (long)xdl_bogosqrt((uint64_t)xdf1->nrec)) > XDL_MAX_EQLIMIT)
mlim = XDL_MAX_EQLIMIT;
for (i = xdf1->dstart; i <= xdf1->dend; i++) {
size_t mph1 = xdf1->recs[i].minimal_perfect_hash;
@@ -299,7 +299,7 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
action1[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
}
- if ((mlim = xdl_bogosqrt((long)xdf2->nrec)) > XDL_MAX_EQLIMIT)
+ if ((mlim = (long)xdl_bogosqrt((uint64_t)xdf2->nrec)) > XDL_MAX_EQLIMIT)
mlim = XDL_MAX_EQLIMIT;
for (i = xdf2->dstart; i <= xdf2->dend; i++) {
size_t mph2 = xdf2->recs[i].minimal_perfect_hash;
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 77ee1ad9c8..9a999acdc0 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -23,8 +23,8 @@
#include "xinclude.h"
-long xdl_bogosqrt(long n) {
- long i;
+uint64_t xdl_bogosqrt(uint64_t n) {
+ uint64_t i;
/*
* Classical integer square root approximation using shifts.
diff --git a/xdiff/xutils.h b/xdiff/xutils.h
index 615b4a9d35..58f9d74cda 100644
--- a/xdiff/xutils.h
+++ b/xdiff/xutils.h
@@ -25,7 +25,7 @@
-long xdl_bogosqrt(long n);
+uint64_t xdl_bogosqrt(uint64_t n);
int xdl_emit_diffrec(char const *rec, long size, char const *pre, long psize,
xdemitcb_t *ecb);
int xdl_cha_init(chastore_t *cha, long isize, long icount);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 1/6] xdiff/xdl_cleanup_records: delete local recs pointer
From: Ezekiel Newren via GitGitGadget @ 2026-04-29 22:08 UTC (permalink / raw)
To: git
Cc: Yee Cheng Chin, Phillip Wood, René Scharfe, Jeff King,
D. Ben Knoble, SZEDER Gábor, Ezekiel Newren, Ezekiel Newren
In-Reply-To: <pull.2156.v6.git.git.1777500495.gitgitgadget@gmail.com>
From: Ezekiel Newren <ezekielnewren@gmail.com>
Simplify the first 2 for loops by directly indexing the xdfile.recs.
recs is unused in the last 2 for loops, remove it. Best viewed with
--color-words.
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
---
xdiff/xprepare.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index cd4fc405eb..d6e1901d2d 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -269,7 +269,6 @@ static bool xdl_clean_mmatch(uint8_t const *action, long i, long s, long e) {
*/
static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {
long i, nm, mlim;
- xrecord_t *recs;
xdlclass_t *rcrec;
uint8_t *action1 = NULL, *action2 = NULL;
bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
@@ -293,16 +292,18 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
*/
if ((mlim = xdl_bogosqrt((long)xdf1->nrec)) > XDL_MAX_EQLIMIT)
mlim = XDL_MAX_EQLIMIT;
- for (i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart]; i <= xdf1->dend; i++, recs++) {
- rcrec = cf->rcrecs[recs->minimal_perfect_hash];
+ for (i = xdf1->dstart; i <= xdf1->dend; i++) {
+ size_t mph1 = xdf1->recs[i].minimal_perfect_hash;
+ rcrec = cf->rcrecs[mph1];
nm = rcrec ? rcrec->len2 : 0;
action1[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
}
if ((mlim = xdl_bogosqrt((long)xdf2->nrec)) > XDL_MAX_EQLIMIT)
mlim = XDL_MAX_EQLIMIT;
- for (i = xdf2->dstart, recs = &xdf2->recs[xdf2->dstart]; i <= xdf2->dend; i++, recs++) {
- rcrec = cf->rcrecs[recs->minimal_perfect_hash];
+ for (i = xdf2->dstart; i <= xdf2->dend; i++) {
+ size_t mph2 = xdf2->recs[i].minimal_perfect_hash;
+ rcrec = cf->rcrecs[mph2];
nm = rcrec ? rcrec->len1 : 0;
action2[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
}
@@ -312,8 +313,7 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
* false, or become true.
*/
xdf1->nreff = 0;
- for (i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart];
- i <= xdf1->dend; i++, recs++) {
+ for (i = xdf1->dstart; i <= xdf1->dend; i++) {
if (action1[i] == KEEP ||
(action1[i] == INVESTIGATE && !xdl_clean_mmatch(action1, i, xdf1->dstart, xdf1->dend))) {
xdf1->reference_index[xdf1->nreff++] = i;
@@ -324,8 +324,7 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
}
xdf2->nreff = 0;
- for (i = xdf2->dstart, recs = &xdf2->recs[xdf2->dstart];
- i <= xdf2->dend; i++, recs++) {
+ for (i = xdf2->dstart; i <= xdf2->dend; i++) {
if (action2[i] == KEEP ||
(action2[i] == INVESTIGATE && !xdl_clean_mmatch(action2, i, xdf2->dstart, xdf2->dend))) {
xdf2->reference_index[xdf2->nreff++] = i;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 0/6] Xdiff cleanup part 3
From: Ezekiel Newren via GitGitGadget @ 2026-04-29 22:08 UTC (permalink / raw)
To: git
Cc: Yee Cheng Chin, Phillip Wood, René Scharfe, Jeff King,
D. Ben Knoble, SZEDER Gábor, Ezekiel Newren
In-Reply-To: <pull.2156.v5.git.git.1775679988.gitgitgadget@gmail.com>
Changes in v6:
* implement suggestions by Phillip Wood [1,2]
Phillip's second "if" in [1] differs from his first one. In my changes I
made both of them structurally the same.
Something I'm confused by is the range-diff of patch 5. I'm confused why
range-diff states that this is different at all. I don't think this is a
problem, I just don't like not being able to explain a difference pointed
out by range-diff.
5: 88c68fa89a ! 5: 099b08c33f xdiff/xdl_cleanup_records: make setting action
easier to follow @@ xdiff/xprepare.c: static int
xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t * + action1[i] =
INVESTIGATE; }
- for (i = xdf2->dstart; i <= xdf2->dend; i++) {
+ if (need_min) {
+@@ xdiff/xprepare.c: static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
size_t mph2 = xdf2->recs[i].minimal_perfect_hash;
rcrec = cf->rcrecs[mph2];
nm = rcrec ? rcrec->len1 : 0;
[1] limits
https://lore.kernel.org/git/d88af7e1-e8dd-4423-9c6c-977e1f1dc074@gmail.com/
[2] action execution
https://lore.kernel.org/git/df244360-e9a9-44c0-946d-29288e6dd269@gmail.com/
Changes in v5:
* drop commit "xdiff/xdl_cleanup_records: simplify INVESTIGATE handling for
clarity".
* add braces around the else clause
I didn't see a better way to rewrite how action is used so I reverted to
what it used to be.
Changes in v4:
* Change SIZE_MAX to PTRDIFF_MAX.
Changes in v3:
* run make DEVELOPER=1 on each commit and fix all compiler issues
v2 is a radical departure from v1 Changes in v2:
* make the flow of xdl_cleanup_records() easier to follow
There is no performance or behavioral change introduced in this patch
series.
=== original cover letter bellow ===
Patch series summary:
* patch 1: Introduce the ivec type
* patch 2: Create the function xdl_do_classic_diff()
* patches 3-4: generic cleanup
* patches 5-8: convert from dstart/dend (in xdfile_t) to
delta_start/delta_end (in xdfenv_t)
* patches 9-10: move xdl_cleanup_records(), and related, from xprepare.c to
xdiffi.c
Things that will be addressed in future patch series:
* Make xdl_cleanup_records() easier to read
* convert recs/nrec into an ivec
* convert changed to an ivec
* remove reference_index/nreff from xdfile_t and turn it into an ivec
* splitting minimal_perfect_hash out as its own ivec
* improve the performance of the classifier and parsing/hashing lines
=== before this patch series typedef struct s_xdfile { xrecord_t *recs;
size_t nrec; ptrdiff_t dstart, dend; bool *changed; size_t *reference_index;
size_t nreff; } xdfile_t;
typedef struct s_xdfenv { xdfile_t xdf1, xdf2; } xdfenv_t;
=== after this patch series typedef struct s_xdfile { xrecord_t *recs;
size_t nrec; bool *changed; size_t *reference_index; size_t nreff; }
xdfile_t;
typedef struct s_xdfenv { xdfile_t xdf1, xdf2; size_t delta_start,
delta_end; size_t mph_size; } xdfenv_t;
Ezekiel Newren (6):
xdiff/xdl_cleanup_records: delete local recs pointer
xdiff: use unambiguous types in xdl_bogo_sqrt()
xdiff/xdl_cleanup_records: use unambiguous types
xdiff/xdl_cleanup_records: make limits more clear
xdiff/xdl_cleanup_records: make setting action easier to follow
xdiff/xdl_cleanup_records: make execution of action easier to follow
xdiff/xdiffi.c | 2 +-
xdiff/xprepare.c | 97 ++++++++++++++++++++++++++++++++++--------------
xdiff/xutils.c | 4 +-
xdiff/xutils.h | 2 +-
4 files changed, 73 insertions(+), 32 deletions(-)
base-commit: ca1db8a0f7dc0dbea892e99f5b37c5fe5861be71
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2156%2Fezekielnewren%2Fxdiff-cleanup-3-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2156/ezekielnewren/xdiff-cleanup-3-v6
Pull-Request: https://github.com/git/git/pull/2156
Range-diff vs v5:
1: b31924a949 = 1: b31924a949 xdiff/xdl_cleanup_records: delete local recs pointer
2: 1822166fef = 2: 1822166fef xdiff: use unambiguous types in xdl_bogo_sqrt()
3: 85aa0da90c = 3: 85aa0da90c xdiff/xdl_cleanup_records: use unambiguous types
4: fec2b0f38a ! 4: 51c62ed454 xdiff/xdl_cleanup_records: make limits more clear
@@ Commit message
* The additional condition `!need_min` is redudant now, remove it.
Best viewed with --color-words.
+ Helped-by: Phillip Wood
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
## xdiff/xprepare.c ##
@@ xdiff/xprepare.c: static bool xdl_clean_mmatch(uint8_t const *action, ptrdiff_t
uint8_t *action1 = NULL, *action2 = NULL;
bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
@@ xdiff/xprepare.c: static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
- goto cleanup;
- }
-
-+ if (need_min) {
-+ /* i.e. infinity */
-+ mlim1 = PTRDIFF_MAX;
-+ mlim2 = PTRDIFF_MAX;
-+ } else {
-+ mlim1 = XDL_MIN(xdl_bogosqrt(xdf1->nrec), XDL_MAX_EQLIMIT);
-+ mlim2 = XDL_MIN(xdl_bogosqrt(xdf2->nrec), XDL_MAX_EQLIMIT);
-+ }
-+
/*
* Initialize temporary arrays with DISCARD, KEEP, or INVESTIGATE.
*/
- if ((mlim = (long)xdl_bogosqrt((uint64_t)xdf1->nrec)) > XDL_MAX_EQLIMIT)
- mlim = XDL_MAX_EQLIMIT;
++ if (need_min) {
++ /* i.e. infinity */
++ mlim1 = PTRDIFF_MAX;
++ } else {
++ mlim1 = xdl_bogosqrt((uint64_t)xdf1->nrec);
++ if (mlim1 > XDL_MAX_EQLIMIT)
++ mlim1 = XDL_MAX_EQLIMIT;
++ }
for (i = xdf1->dstart; i <= xdf1->dend; i++) {
size_t mph1 = xdf1->recs[i].minimal_perfect_hash;
rcrec = cf->rcrecs[mph1];
@@ xdiff/xprepare.c: static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *
- if ((mlim = (long)xdl_bogosqrt((uint64_t)xdf2->nrec)) > XDL_MAX_EQLIMIT)
- mlim = XDL_MAX_EQLIMIT;
++ if (need_min) {
++ /* i.e. infinity */
++ mlim2 = PTRDIFF_MAX;
++ } else {
++ mlim2 = xdl_bogosqrt((uint64_t)xdf2->nrec);
++ if (mlim2 > XDL_MAX_EQLIMIT)
++ mlim2 = XDL_MAX_EQLIMIT;
++ }
for (i = xdf2->dstart; i <= xdf2->dend; i++) {
size_t mph2 = xdf2->recs[i].minimal_perfect_hash;
rcrec = cf->rcrecs[mph2];
5: 88c68fa89a ! 5: 45ad2ae62d xdiff/xdl_cleanup_records: make setting action easier to follow
@@ xdiff/xprepare.c: static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *
+ action1[i] = INVESTIGATE;
}
- for (i = xdf2->dstart; i <= xdf2->dend; i++) {
+ if (need_min) {
+@@ xdiff/xprepare.c: static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
size_t mph2 = xdf2->recs[i].minimal_perfect_hash;
rcrec = cf->rcrecs[mph2];
nm = rcrec ? rcrec->len1 : 0;
6: 699e198fa9 ! 6: a5174802f4 xdiff/xdl_cleanup_records: put braces around the else clause
@@ Metadata
Author: Ezekiel Newren <ezekielnewren@gmail.com>
## Commit message ##
- xdiff/xdl_cleanup_records: put braces around the else clause
+ xdiff/xdl_cleanup_records: make execution of action easier to follow
+ Helped-by: Phillip Wood
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
## xdiff/xprepare.c ##
@@ xdiff/xprepare.c: static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
- (action1[i] == INVESTIGATE && !xdl_clean_mmatch(action1, i, xdf1->dstart, xdf1->dend))) {
+ */
+ xdf1->nreff = 0;
+ for (i = xdf1->dstart; i <= xdf1->dend; i++) {
+- if (action1[i] == KEEP ||
+- (action1[i] == INVESTIGATE && !xdl_clean_mmatch(action1, i, xdf1->dstart, xdf1->dend))) {
++ uint8_t action = action1[i];
++
++ if (action == INVESTIGATE) {
++ if (!xdl_clean_mmatch(action1, i, xdf1->dstart, xdf1->dend))
++ action = KEEP;
++ else
++ action = DISCARD;
++ }
++
++ if (action == KEEP) {
xdf1->reference_index[xdf1->nreff++] = i;
- /* changed[i] remains false, i.e. keep */
+- /* changed[i] remains false, i.e. keep */
- } else
-+ } else {
++ /* changed[i] remains false */
++ } else if (action == DISCARD) {
xdf1->changed[i] = true;
- /* i.e. discard */
+- /* i.e. discard */
++ } else {
++ BUG("Illegal state for action");
+ }
}
xdf2->nreff = 0;
-@@ xdiff/xprepare.c: static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
- (action2[i] == INVESTIGATE && !xdl_clean_mmatch(action2, i, xdf2->dstart, xdf2->dend))) {
+ for (i = xdf2->dstart; i <= xdf2->dend; i++) {
+- if (action2[i] == KEEP ||
+- (action2[i] == INVESTIGATE && !xdl_clean_mmatch(action2, i, xdf2->dstart, xdf2->dend))) {
++ uint8_t action = action2[i];
++
++ if (action == INVESTIGATE) {
++ if (!xdl_clean_mmatch(action2, i, xdf2->dstart, xdf2->dend))
++ action = KEEP;
++ else
++ action = DISCARD;
++ }
++
++ if (action == KEEP) {
xdf2->reference_index[xdf2->nreff++] = i;
- /* changed[i] remains false, i.e. keep */
+- /* changed[i] remains false, i.e. keep */
- } else
-+ } else {
++ /* changed[i] remains false */
++ } else if (action == DISCARD) {
xdf2->changed[i] = true;
- /* i.e. discard */
+- /* i.e. discard */
++ } else {
++ BUG("Illegal state for action");
+ }
}
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v2 1/1] git-gui: protect rev-parse --show-toplevel call
From: Mark Levedahl @ 2026-04-29 20:14 UTC (permalink / raw)
To: Shroom Moo, git; +Cc: j6t
In-Reply-To: <tencent_AEE968E8E785907BA55A383977C8968ED406@qq.com>
On 4/29/26 1:32 PM, Shroom Moo wrote:
> When starting git-gui from a directory that is a bare repository or
> where the working tree is missing, git-gui previously executed
> 'rev-parse --show-toplevel' without error handling. This caused a
> fatal Tcl error ("this operation must be run in a work tree").
>
> Wrap the call in a catch to prevent the fatal error. The existing
> error paths after this call already handle bare repos and missing
> worktrees appropriately.
>
> Signed-off-by: Shroom Moo <egg_mushroomcow@foxmail.com>
> ---
> git-gui/git-gui.sh | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 23fe76e498..aee37685e1 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -1169,7 +1169,9 @@ if {![file isdirectory $_gitdir]} {
> load_config 0
> apply_config
>
> -set _gitworktree [git rev-parse --show-toplevel]
> +if {[catch {set _gitworktree [git rev-parse --show-toplevel]}]} {
> + set _gitworktree {}
> +}
>
> if {$_prefix ne {}} {
> if {$_gitworktree eq {}} {
Unfortunately, this allows starting git-gui inside the separate gitdir created by
git clone --separate-git-dir=/some/where/else ...
There is no hint where the workdir is, but git recognizes the repository is not bare:
git rev-parse --is-bare-repository ==> false
git rev-parse --is-inside-git-dir ==> true
git rev-parse --is-inside-work-tree ==> false
git rev-parse --show-toplevel ==> fatal: must be run in a work tree
git worktree list ==> absolute path to gitdir (not a worktree)
As git-gui has no protection against modifying what is effectively a bare repository,
allowing git-gui to run in this directory is dangerous, or possibly just very confusing.
git refuses to work in this gitdir: "git status" run in the above gitdir gives: "fatal:
this operation must be run in a work tree."
The simplest safe thing is to catch the error and abort with a more useful message than
currently provided. Or perhaps, check git rev-parse --is-inside-git-dir and abort, and do
so before trying --show-toplevel.
Mark
^ 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