* [PATCH v3 8/9] odb: introduce object filters to `odb_for_each_object()`
From: Patrick Steinhardt @ 2026-07-13 14:41 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano, Jeff King, Taylor Blau
In-Reply-To: <20260713-pks-odb-for-each-object-filter-v3-0-b3c65c641073@pks.im>
The function `for_each_bitmapped_object()` can be used to iterate
through all objects covered by a bitmap. The benefit of this function is
that it allows the caller to efficiently handle some object filters. For
example, this can be used to filter out objects of a specific type with
some simple bitmap operations. But callers are currently required to
manually wire up the use of bitmaps though, and to do so they have to
reach into internals of a given object database source.
Introduce a new `struct odb_for_each_object_options::filter` field so
that the interface becomes generic. When set, then a backend may
optionally use the filter to skip some objects that it would have
otherwise yielded.
Note that the respective backends are free to ignore this field if they
cannot meaningfully optimize for a given filter, and consequently
callers need to verify whether they actually want the returned objects.
While annoying, we cannot easily lift this restriction anyway as the
object filter infrastructure supports some filters that cannot be
answered by the object database alone.
An alternative might be to limit the filters to only those that _can_ be
answered by backends. But ultimately, the filters that can be answered
efficiently by the "packed" backend are completely disjunct from those
that can be answered by the "loose" backend, and consequently the set of
filters supported by all backends would be empty. Furthermore, it would
require us to make assumptions about capabilities of future backends,
which may be able to efficiently handle more filters than current ones.
So in the end, this alternative would only limit us artificially.
Implement the logic for the "packed" source. Note that we use the new
function `prepare_source_bitmap_git()` to open the bitmap: as the
backend operates on a single object source, we must only use bitmaps
that belong to that specific source. Otherwise we might yield objects
that are not part of the source at all, and with multiple sources we
would enumerate the same bitmap once per source.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
odb.h | 12 +++++++++++
odb/source-packed.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
pack-bitmap.c | 3 +--
pack-bitmap.h | 3 +++
4 files changed, 78 insertions(+), 2 deletions(-)
diff --git a/odb.h b/odb.h
index a1e222f605..67d0b34942 100644
--- a/odb.h
+++ b/odb.h
@@ -8,6 +8,7 @@
#include "thread-utils.h"
struct cached_object_entry;
+struct list_objects_filter_options;
struct odb_source_inmemory;
struct packed_git;
struct repository;
@@ -490,6 +491,17 @@ struct odb_for_each_object_options {
*/
const struct object_id *prefix;
size_t prefix_hex_len;
+
+ /*
+ * Optional object filter that allows backends to skip yielding
+ * objects that are excluded by the filter as an optimization. The
+ * filter is a best-effort hint: backends may use it to skip
+ * excluded objects (e.g. by consulting a reachability bitmap), but
+ * are also free to ignore it entirely and yield every object. As a
+ * consequence, callers must re-apply the filter on yielded objects
+ * if they require strict filtering semantics.
+ */
+ const struct list_objects_filter_options *filter;
};
/*
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 9cfa02b7a2..4777395053 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -3,11 +3,13 @@
#include "chdir-notify.h"
#include "dir.h"
#include "git-zlib.h"
+#include "list-objects-filter-options.h"
#include "mergesort.h"
#include "midx.h"
#include "odb/source-packed.h"
#include "odb/streaming.h"
#include "packfile.h"
+#include "pack-bitmap.h"
static int find_pack_entry(struct odb_source_packed *store,
const struct object_id *oid,
@@ -315,6 +317,37 @@ static int odb_source_packed_for_each_prefixed_object(
return ret;
}
+struct bitmapped_for_each_object_data {
+ struct odb_source_packed *packed;
+ const struct object_info *request;
+ const struct odb_for_each_object_options *opts;
+ odb_for_each_object_cb cb;
+ void *cb_data;
+};
+
+static int bitmapped_for_each_object(const struct object_id *oid,
+ enum object_type type UNUSED,
+ int flags UNUSED,
+ uint32_t hash UNUSED,
+ struct packed_git *pack,
+ off_t offset,
+ void *cb_data)
+{
+ struct bitmapped_for_each_object_data *data = cb_data;
+
+ if (should_exclude_pack(pack, data->opts->flags))
+ return 0;
+
+ if (data->request) {
+ struct object_info oi = *data->request;
+ if (packed_object_info(data->packed, pack, offset, &oi) < 0)
+ return -1;
+ return data->cb(oid, &oi, data->cb_data);
+ }
+
+ return data->cb(oid, NULL, data->cb_data);
+}
+
static int odb_source_packed_for_each_object(struct odb_source *source,
const struct object_info *request,
odb_for_each_object_cb cb,
@@ -328,12 +361,33 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
.cb = cb,
.cb_data = cb_data,
};
+ struct bitmap_index *bitmap = NULL;
struct packfile_list_entry *e;
int pack_errors = 0, ret;
if (opts->prefix)
return odb_source_packed_for_each_prefixed_object(packed, opts, &data);
+ if (opts->filter &&
+ opts->filter->choice != LOFC_DISABLED &&
+ can_filter_bitmap(opts->filter))
+ bitmap = prepare_bitmap_git_for_source(packed);
+ if (bitmap) {
+ struct bitmapped_for_each_object_data bitmap_data = {
+ .packed = packed,
+ .request = request,
+ .opts = opts,
+ .cb = cb,
+ .cb_data = cb_data,
+ };
+
+ ret = for_each_bitmapped_object(bitmap, opts->filter,
+ bitmapped_for_each_object,
+ &bitmap_data);
+ if (ret)
+ goto out;
+ }
+
packed->skip_mru_updates = true;
for (e = packfile_store_get_packs(packed); e; e = e->next) {
@@ -342,6 +396,13 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
if (should_exclude_pack(p, opts->flags))
continue;
+ /*
+ * Objects covered by the bitmap have already been yielded
+ * above; skip them here to avoid duplicates.
+ */
+ if (bitmap && bitmap_index_contains_pack(bitmap, p))
+ continue;
+
if (open_pack_index(p)) {
pack_errors = 1;
continue;
@@ -357,6 +418,7 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
out:
packed->skip_mru_updates = false;
+ free_bitmap_index(bitmap);
if (!ret && pack_errors)
ret = -1;
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 09ba15d26b..f55a0859ea 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2039,12 +2039,11 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
return -1;
}
-static int can_filter_bitmap(const struct list_objects_filter_options *filter)
+bool can_filter_bitmap(const struct list_objects_filter_options *filter)
{
return !filter_bitmap(NULL, NULL, NULL, filter);
}
-
static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
struct bitmap *result)
{
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 9f20fb6e56..1385027c1f 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -92,6 +92,9 @@ int test_bitmap_pseudo_merge_objects(struct repository *r, uint32_t n);
struct list_objects_filter_options;
+/* Check whether the filter can be computed via the bitmap. */
+bool can_filter_bitmap(const struct list_objects_filter_options *filter);
+
/*
* Filter bitmapped objects and iterate through all resulting objects,
* executing `show_reach` for each of them. Returns `-1` in case the filter is
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* [PATCH v3 7/9] pack-bitmap: introduce function to open bitmap for a single source
From: Patrick Steinhardt @ 2026-07-13 14:41 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano, Jeff King, Taylor Blau
In-Reply-To: <20260713-pks-odb-for-each-object-filter-v3-0-b3c65c641073@pks.im>
The function `prepare_bitmap_git()` opens the first bitmap it can find
in any of the object sources connected to the repository. In a
subsequent commit, the "packed" object database backend will learn to
use bitmaps to answer object filters when enumerating objects. That
backend operates on a single object source though, so using a bitmap
that potentially belongs to a different source would be wrong:
- The source would yield objects that are not part of the source
itself.
- The object source info would be attributed to the wrong source.
- With multiple sources, each source would enumerate the same bitmap
another time.
Introduce a new function `prepare_source_bitmap_git()` that only opens
bitmaps belonging to the given object source.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
pack-bitmap.c | 12 ++++++++++++
pack-bitmap.h | 2 ++
2 files changed, 14 insertions(+)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 72c8ae3228..09ba15d26b 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -753,6 +753,18 @@ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
return NULL;
}
+struct bitmap_index *prepare_bitmap_git_for_source(struct odb_source_packed *source)
+{
+ struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
+
+ if (!open_bitmap_for_source(source, bitmap_git) &&
+ !load_bitmap(source->base.odb->repo, bitmap_git, 0))
+ return bitmap_git;
+
+ free_bitmap_index(bitmap_git);
+ return NULL;
+}
+
int bitmap_index_contains_pack(struct bitmap_index *bitmap, struct packed_git *pack)
{
for (; bitmap; bitmap = bitmap->base) {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index ae8dc491ac..9f20fb6e56 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -9,6 +9,7 @@
#include "string-list.h"
struct commit;
+struct odb_source_packed;
struct repository;
struct rev_info;
@@ -68,6 +69,7 @@ struct bitmapped_pack {
struct bitmap_index *prepare_bitmap_git(struct repository *r);
struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx);
+struct bitmap_index *prepare_bitmap_git_for_source(struct odb_source_packed *source);
/*
* Given a bitmap index, determine whether it contains the pack either directly
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* [PATCH v3 6/9] pack-bitmap: drop `_1` suffix from functions that open bitmaps
From: Patrick Steinhardt @ 2026-07-13 14:41 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano, Jeff King, Taylor Blau
In-Reply-To: <20260713-pks-odb-for-each-object-filter-v3-0-b3c65c641073@pks.im>
In the preceding commit we've refactored how we open bitmaps. As part of
the refactoring we have consolidated `open_pack_bitmap()` as well as
`open_midx_bitmap()` into `open_bitmap_for_source()`. Consequently, we
only have their `open_pack_bitmap_1()` and `open_midx_bitmap_1()`
variants left over, where the `_1` suffix doesn't really make much sense
anymore.
Drop the suffix.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
pack-bitmap.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index e32795a595..72c8ae3228 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -460,8 +460,8 @@ char *pack_bitmap_filename(struct packed_git *p)
return xstrfmt("%.*s.bitmap", (int)len, p->pack_name);
}
-static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
- struct multi_pack_index *midx)
+static int open_midx_bitmap(struct bitmap_index *bitmap_git,
+ struct multi_pack_index *midx)
{
struct stat st;
char *bitmap_name = midx_bitmap_filename(midx);
@@ -539,7 +539,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
return -1;
}
-static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
+static int open_pack_bitmap(struct bitmap_index *bitmap_git, struct packed_git *packfile)
{
int fd;
struct stat st;
@@ -603,7 +603,7 @@ static int load_reverse_index(struct repository *r, struct bitmap_index *bitmap_
/*
* The multi-pack-index's .rev file is already loaded via
- * open_pack_bitmap_1().
+ * open_pack_bitmap().
*
* But we still need to open the individual pack .rev files,
* since we will need to make use of them in pack-objects.
@@ -687,7 +687,7 @@ static int open_bitmap_for_source(struct odb_source_packed *source,
struct packfile_list_entry *e;
bool found = false;
- if (midx && !open_midx_bitmap_1(bitmap_git, midx))
+ if (midx && !open_midx_bitmap(bitmap_git, midx))
found = true;
for (e = packfile_store_get_packs(source); e; e = e->next) {
@@ -698,7 +698,7 @@ static int open_bitmap_for_source(struct odb_source_packed *source,
if (found && !trace2_is_enabled())
break;
- if (!open_pack_bitmap_1(bitmap_git, e->pack))
+ if (!open_pack_bitmap(bitmap_git, e->pack))
found = true;
}
@@ -746,7 +746,7 @@ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
{
struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
- if (!open_midx_bitmap_1(bitmap_git, midx))
+ if (!open_midx_bitmap(bitmap_git, midx))
return bitmap_git;
free_bitmap_index(bitmap_git);
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* [PATCH v3 5/9] pack-bitmap: iterate object sources when opening bitmaps
From: Patrick Steinhardt @ 2026-07-13 14:41 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano, Jeff King, Taylor Blau
In-Reply-To: <20260713-pks-odb-for-each-object-filter-v3-0-b3c65c641073@pks.im>
When opening a bitmap for a repository we perform two steps:
- We first look for a multi-pack index bitmap in any of the object
sources connected to the repository.
- We then look for a packfile bitmap in any of the packfiles of any of
the object sources.
Both of these steps thus iterate through object sources themselves, one
via `odb_prepare_alternates()` and one via `repo_for_each_pack()`. This
layout makes it hard to introduce a way to open the bitmap of one
specific object source, which is functionality that we'll require in a
subsequent commit.
Reverse the loop so that we instead loop through all sources in the
outer loop, and then for each source we try to load its bitmap via
either the multi-pack index or via a packfile.
Note that this changes the precedence of bitmaps in one specific edge
case: when an earlier object source only has a packfile bitmap, but a
later source has a multi-pack index bitmap, we now pick the packfile
bitmap of the earlier source. Previously, a multi-pack index bitmap from
any source would have taken precedence over all packfile bitmaps. Given
that object sources are ordered such that the local source comes first,
this arguably is an improvement, as we now prefer local bitmaps over
bitmaps in alternates. Furthermore, we already warn about repositories
that have multiple bitmaps, so this setup is broken and thus arguably
not worth worrying about too much.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
pack-bitmap.c | 69 +++++++++++++++++++++++++++--------------------------------
1 file changed, 31 insertions(+), 38 deletions(-)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index eda38a5433..e32795a595 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -680,60 +680,53 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
return 0;
}
-static int open_pack_bitmap(struct repository *r,
- struct bitmap_index *bitmap_git)
+static int open_bitmap_for_source(struct odb_source_packed *source,
+ struct bitmap_index *bitmap_git)
{
- struct packed_git *p;
- int ret = -1;
+ struct multi_pack_index *midx = get_multi_pack_index(source);
+ struct packfile_list_entry *e;
+ bool found = false;
- repo_for_each_pack(r, p) {
- if (open_pack_bitmap_1(bitmap_git, p) == 0) {
- ret = 0;
- /*
- * The only reason to keep looking is to report
- * duplicates.
- */
- if (!trace2_is_enabled())
- break;
- }
+ if (midx && !open_midx_bitmap_1(bitmap_git, midx))
+ found = true;
+
+ for (e = packfile_store_get_packs(source); e; e = e->next) {
+ /*
+ * When tracing is enabled we want to keep looking to report
+ * duplicates even if we have already found a bitmap.
+ */
+ if (found && !trace2_is_enabled())
+ break;
+
+ if (!open_pack_bitmap_1(bitmap_git, e->pack))
+ found = true;
}
- return ret;
+ return found ? 0 : -1;
}
-static int open_midx_bitmap(struct repository *r,
- struct bitmap_index *bitmap_git)
+static int open_bitmap(struct repository *r,
+ struct bitmap_index *bitmap_git)
{
struct odb_source *source;
- int ret = -1;
+ bool found = false;
assert(!bitmap_git->map);
odb_prepare_alternates(r->objects);
for (source = r->objects->sources; source; source = source->next) {
struct odb_source_files *files = odb_source_files_downcast(source);
- struct multi_pack_index *midx = get_multi_pack_index(files->packed);
- if (midx && !open_midx_bitmap_1(bitmap_git, midx))
- ret = 0;
- }
- return ret;
-}
-
-static int open_bitmap(struct repository *r,
- struct bitmap_index *bitmap_git)
-{
- int found;
- assert(!bitmap_git->map);
+ if (!open_bitmap_for_source(files->packed, bitmap_git))
+ found = true;
- found = !open_midx_bitmap(r, bitmap_git);
-
- /*
- * these will all be skipped if we opened a midx bitmap; but run it
- * anyway if tracing is enabled to report the duplicates
- */
- if (!found || trace2_is_enabled())
- found |= !open_pack_bitmap(r, bitmap_git);
+ /*
+ * The only reason to keep looking after having found a bitmap
+ * is to report duplicates.
+ */
+ if (found && !trace2_is_enabled())
+ break;
+ }
return found ? 0 : -1;
}
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* [PATCH v3 4/9] pack-bitmap: allow aborting iteration of bitmapped objects
From: Patrick Steinhardt @ 2026-07-13 14:41 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano, Jeff King, Taylor Blau
In-Reply-To: <20260713-pks-odb-for-each-object-filter-v3-0-b3c65c641073@pks.im>
In a subsequent commit we'll lift iteration of bitmapped objects into
the "packed" backend and make it accessible via `odb_for_each_object()`.
The calling convention for that function is that the callback may return
a non-zero exit code, and if so we'll abort iteration. This is currently
impossible to realize though, as `for_each_bitmapped_object()` will
ignore any return value and just churn through all objects completely.
This doesn't matter to the callers of `for_each_bitmapped_object()`, as
there's only one of them in git-cat-file(1), and the callbacks we pass
always return zero. But once we move the logic into the generic
infrastructure it becomes a latent bug waiting to happen.
Refactor the code so that the return value of the `show_reach` callback
is not ignored anymore. Instead, returning a non-zero value will cause
us to abort iteration in both `show_objects_for_type()` and in
`for_each_bitmapped_object()`.
Note though that there's a second user of `show_objects_for_type()` with
`traverse_bitmap_commit_list()`, and that function does indeed invoke
callbacks that may return non-zero. This non-zero return value never had
any effect at all though, and the callbacks that return non-zero values
are only ever invoked via `traverse_bitmap_commit_list()`. Consequently,
we adapt them to always return 0.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/pack-objects.c | 2 +-
builtin/rev-list.c | 2 +-
pack-bitmap.c | 31 +++++++++++++++++++++----------
pack-bitmap.h | 3 ++-
4 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 188c4f6d4b..3673b14b89 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1908,7 +1908,7 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,
return 0;
create_object_entry(oid, type, name_hash, 0, 0, pack, offset);
- return 1;
+ return 0;
}
struct pbase_tree_cache {
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 8f63003709..02818b81c6 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -486,7 +486,7 @@ static int show_object_fast(
void *payload UNUSED)
{
fprintf(stdout, "%s\n", oid_to_hex(oid));
- return 1;
+ return 0;
}
static void print_disk_usage(off_t size)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index a47c231632..eda38a5433 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1695,7 +1695,7 @@ static void init_type_iterator(struct ewah_or_iterator *it,
}
}
-static void show_objects_for_type(
+static int show_objects_for_type(
struct bitmap_index *bitmap_git,
struct bitmap *objects,
enum object_type object_type,
@@ -1704,6 +1704,7 @@ static void show_objects_for_type(
{
size_t i = 0;
uint32_t offset;
+ int ret;
struct ewah_or_iterator it;
eword_t filter;
@@ -1749,11 +1750,17 @@ static void show_objects_for_type(
hash = bitmap_name_hash(bitmap_git, index_pos);
- show_reach(&oid, object_type, 0, hash, pack, ofs, payload);
+ ret = show_reach(&oid, object_type, 0, hash, pack, ofs, payload);
+ if (ret)
+ goto out;
}
}
+ ret = 0;
+
+out:
ewah_or_iterator_release(&it);
+ return ret;
}
static int in_bitmapped_pack(struct bitmap_index *bitmap_git,
@@ -2062,6 +2069,12 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
show_reachable_fn show_reach,
void *payload)
{
+ const enum object_type types[] = {
+ OBJ_COMMIT,
+ OBJ_TREE,
+ OBJ_BLOB,
+ OBJ_TAG,
+ };
struct bitmap *filtered_bitmap = NULL;
uint32_t objects_nr;
size_t full_word_count;
@@ -2086,14 +2099,12 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
goto out;
}
- show_objects_for_type(bitmap_git, filtered_bitmap,
- OBJ_COMMIT, show_reach, payload);
- show_objects_for_type(bitmap_git, filtered_bitmap,
- OBJ_TREE, show_reach, payload);
- show_objects_for_type(bitmap_git, filtered_bitmap,
- OBJ_BLOB, show_reach, payload);
- show_objects_for_type(bitmap_git, filtered_bitmap,
- OBJ_TAG, show_reach, payload);
+ for (size_t i = 0; i < ARRAY_SIZE(types); i++) {
+ ret = show_objects_for_type(bitmap_git, filtered_bitmap,
+ types[i], show_reach, payload);
+ if (ret)
+ goto out;
+ }
ret = 0;
out:
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 47935eb24e..ae8dc491ac 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -93,7 +93,8 @@ struct list_objects_filter_options;
/*
* Filter bitmapped objects and iterate through all resulting objects,
* executing `show_reach` for each of them. Returns `-1` in case the filter is
- * not supported, `0` otherwise.
+ * not supported, `0` otherwise. Aborts iteration and bubbles up the return
+ * value in case `show_reach()` returns non-zero.
*/
int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
const struct list_objects_filter_options *filter,
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* [PATCH v3 3/9] pack-objects: drop unused return value from add_object_entry()
From: Patrick Steinhardt @ 2026-07-13 14:41 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano, Jeff King, Taylor Blau
In-Reply-To: <20260713-pks-odb-for-each-object-filter-v3-0-b3c65c641073@pks.im>
From: Jeff King <peff@peff.net>
This function returns 0/1 to its caller to tell them whether we actually
added a new entry (or if we considered it redundant). But nobody has
relied on that behavior since 5379a5c5ee (Thin pack generation:
optimization., 2006-04-05).
The extra return does not hurt much, but it is a bit confusing. We have
a sister function, add_object_entry_from_bitmap(), which has the same
return value semantics. That function is about to change to always return
0 (not void, because it must conform to a callback function interface).
So with that change, we'd have two related functions which both return
an "int" but with different semantics.
Let's drop the unused "int" return from add_object_entry() entirely,
which makes it more clear that the two functions have diverged.
Signed-off-by: Jeff King <peff@peff.net>
[ps: slightly massaged the commit message]
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/pack-objects.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ea5eab4cf8..188c4f6d4b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1867,8 +1867,8 @@ static const char no_closure_warning[] = N_(
"disabling bitmap writing, as some objects are not being packed"
);
-static int add_object_entry(const struct object_id *oid, enum object_type type,
- const char *name, int exclude)
+static void add_object_entry(const struct object_id *oid, enum object_type type,
+ const char *name, int exclude)
{
struct packed_git *found_pack = NULL;
off_t found_offset = 0;
@@ -1876,7 +1876,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
display_progress(progress_state, ++nr_seen);
if (have_duplicate_entry(oid, exclude))
- return 0;
+ return;
if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {
/* The pack is missing an object, so it will not have closure */
@@ -1885,13 +1885,12 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
warning(_(no_closure_warning));
write_bitmap_index = 0;
}
- return 0;
+ return;
}
create_object_entry(oid, type, pack_name_hash_fn(name),
exclude, name && no_try_delta(name),
found_pack, found_offset);
- return 1;
}
static int add_object_entry_from_bitmap(const struct object_id *oid,
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* [PATCH v3 2/9] pack-bitmap: mark object filter as `const`
From: Patrick Steinhardt @ 2026-07-13 14:41 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano, Jeff King, Taylor Blau
In-Reply-To: <20260713-pks-odb-for-each-object-filter-v3-0-b3c65c641073@pks.im>
The function `for_each_bitmapped_object()` accepts an optional object
filter. This filter is never modified by the function, but is not
declared as `const`. Fix this.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
pack-bitmap.c | 6 +++---
pack-bitmap.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 35774b6f0c..a47c231632 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1976,7 +1976,7 @@ static void filter_bitmap_object_type(struct bitmap_index *bitmap_git,
static int filter_bitmap(struct bitmap_index *bitmap_git,
struct object_list *tip_objects,
struct bitmap *to_filter,
- struct list_objects_filter_options *filter)
+ const struct list_objects_filter_options *filter)
{
if (!filter || filter->choice == LOFC_DISABLED)
return 0;
@@ -2027,7 +2027,7 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
return -1;
}
-static int can_filter_bitmap(struct list_objects_filter_options *filter)
+static int can_filter_bitmap(const struct list_objects_filter_options *filter)
{
return !filter_bitmap(NULL, NULL, NULL, filter);
}
@@ -2058,7 +2058,7 @@ static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
}
int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
- struct list_objects_filter_options *filter,
+ const struct list_objects_filter_options *filter,
show_reachable_fn show_reach,
void *payload)
{
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 19a8655457..47935eb24e 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -96,7 +96,7 @@ struct list_objects_filter_options;
* not supported, `0` otherwise.
*/
int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
- struct list_objects_filter_options *filter,
+ const struct list_objects_filter_options *filter,
show_reachable_fn show_reach,
void *payload);
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* [PATCH v3 1/9] odb/source-packed: improve lookup when enumerating objects
From: Patrick Steinhardt @ 2026-07-13 14:41 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano, Jeff King, Taylor Blau
In-Reply-To: <20260713-pks-odb-for-each-object-filter-v3-0-b3c65c641073@pks.im>
When iterating through objects of a packed source that have a specific
prefix we do so via two different methods:
- When a multi-pack index is available we use that one to efficiently
loop through all objects.
- We then loop through all packfiles that aren't covered by a
multi-pack index.
Regardless of which mechanism we use, we then iterate through all the
objects indexed by the respective data structure. Curiously though,
while we use the indices for enumerating the objects, we completely
ignore it for the actual object lookup. Instead, we call into the
generic `odb_source_read_object_info()` function, which will itself
consult the indices to figure out where the object in question even
lives.
This has two consequences:
- It's inefficient, as we basically have to figure out the position of
the object a second time.
- It's subtly wrong, as it may now happen that a specific object will
be looked up via a different pack in case it exists multiple times.
This is unlikely to have any real-world consequences, but it's still
the wrong thing to do.
Fix the issue by using `packed_object_info()` directly. While at it,
rename the `store` variable to `source`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
odb/source-packed.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0edea5356d..9cfa02b7a2 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -143,7 +143,7 @@ static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_f
}
static int for_each_prefixed_object_in_midx(
- struct odb_source_packed *store,
+ struct odb_source_packed *source,
struct multi_pack_index *m,
const struct odb_for_each_object_options *opts,
struct odb_source_packed_for_each_object_wrapper_data *data)
@@ -170,6 +170,7 @@ static int for_each_prefixed_object_in_midx(
*/
for (i = first; i < num; i++) {
const struct object_id *current = NULL;
+ struct packed_git *pack;
struct object_id oid;
current = nth_midxed_object_oid(&oid, m, i);
@@ -177,9 +178,8 @@ static int for_each_prefixed_object_in_midx(
if (!match_hash(len, opts->prefix->hash, current->hash))
break;
- if (opts->flags) {
+ if (opts->flags || data->request) {
uint32_t pack_id = nth_midxed_pack_int_id(m, i);
- struct packed_git *pack;
if (prepare_midx_pack(m, pack_id)) {
pack_errors = true;
@@ -193,9 +193,9 @@ static int for_each_prefixed_object_in_midx(
if (data->request) {
struct object_info oi = *data->request;
+ off_t offset = nth_midxed_offset(m, i);
- ret = odb_source_read_object_info(&store->base, current,
- &oi, 0);
+ ret = packed_object_info(source, pack, offset, &oi);
if (ret)
goto out;
@@ -219,7 +219,7 @@ static int for_each_prefixed_object_in_midx(
}
static int for_each_prefixed_object_in_pack(
- struct odb_source_packed *store,
+ struct odb_source_packed *source,
struct packed_git *p,
const struct odb_for_each_object_options *opts,
struct odb_source_packed_for_each_object_wrapper_data *data)
@@ -246,8 +246,9 @@ static int for_each_prefixed_object_in_pack(
if (data->request) {
struct object_info oi = *data->request;
+ off_t offset = nth_packed_object_offset(p, i);
- ret = odb_source_read_object_info(&store->base, &oid, &oi, 0);
+ ret = packed_object_info(source, p, offset, &oi);
if (ret)
goto out;
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* [PATCH v3 0/9] odb: introduce object filters to `odb_for_each_object()`
From: Patrick Steinhardt @ 2026-07-13 14:41 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Junio C Hamano, Jeff King, Taylor Blau
In-Reply-To: <20260709-pks-odb-for-each-object-filter-v1-0-82fe014b12b3@pks.im>
Hi,
this patch series introduces object filters to `odb_for_each_object()`.
The intent of this is to make `git cat-file --batch-all-objects` work
with pluggable object databases. Right now it doesn't because it reaches
into internals of the "packed" backend to efficiently handle bitmapped
objects.
The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
2026-07-06) with ps/odb-drop-whence at 8a7ad23e11 (odb: document object
info fields, 2026-07-02) merged into it.
Changes in v3:
- Weave Peff's patch into the patch series.
- Link to v2: https://patch.msgid.link/20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im
Changes in v2:
- Add another patch to drop the `_1()` prefixes that aren't required
anymore.
- Change the approach in `open_bitmap_for_source()` to also use a
`found` boolean instead of a confusing integer.
- Add some more explanations to commit messages.
- Link to v1: https://patch.msgid.link/20260709-pks-odb-for-each-object-filter-v1-0-82fe014b12b3@pks.im
Thanks!
Patrick
---
Jeff King (1):
pack-objects: drop unused return value from add_object_entry()
Patrick Steinhardt (8):
odb/source-packed: improve lookup when enumerating objects
pack-bitmap: mark object filter as `const`
pack-bitmap: allow aborting iteration of bitmapped objects
pack-bitmap: iterate object sources when opening bitmaps
pack-bitmap: drop `_1` suffix from functions that open bitmaps
pack-bitmap: introduce function to open bitmap for a single source
odb: introduce object filters to `odb_for_each_object()`
builtin/cat-file: filter objects via object database
builtin/cat-file.c | 76 +++--------------------------
builtin/pack-objects.c | 11 ++---
builtin/rev-list.c | 2 +-
odb.h | 12 +++++
odb/source-packed.c | 77 ++++++++++++++++++++++++++---
pack-bitmap.c | 129 +++++++++++++++++++++++++++----------------------
pack-bitmap.h | 10 +++-
7 files changed, 175 insertions(+), 142 deletions(-)
Range-diff versus v2:
1: baf2adb012 = 1: 7c0dc1be0d odb/source-packed: improve lookup when enumerating objects
2: 57eecf3031 = 2: 2e5908c9c3 pack-bitmap: mark object filter as `const`
-: ---------- > 3: f4d66ccfc6 pack-objects: drop unused return value from add_object_entry()
3: 92dd6a6f6e = 4: af475654b8 pack-bitmap: allow aborting iteration of bitmapped objects
4: 92fe41577d = 5: 6ca42587c9 pack-bitmap: iterate object sources when opening bitmaps
5: e5d59959e3 = 6: f62c3bbc81 pack-bitmap: drop `_1` suffix from functions that open bitmaps
6: ab3547ac2b = 7: b2d25b6e9b pack-bitmap: introduce function to open bitmap for a single source
7: 026f21f522 = 8: a5bf309bec odb: introduce object filters to `odb_for_each_object()`
8: 534b25c817 = 9: 600b15a907 builtin/cat-file: filter objects via object database
---
base-commit: 3c8e2790f2ce15e8b5d4b4e6ced711b12649f32a
change-id: 20260708-pks-odb-for-each-object-filter-13286fa3523d
^ permalink raw reply
* Re: [PATCH 1/6] SubmittingPatches: clarify expected structure of commit log message
From: Weijie Yuan @ 2026-07-13 14:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Montalbo, git
In-Reply-To: <xmqqcxwr3g7r.fsf@gitster.g>
On Sun, Jul 12, 2026 at 05:07:04PM -0700, Junio C Hamano wrote:
> Michael Montalbo <mmontalbo@gmail.com> writes:
>
> > I think collapsing the "Formatting and Style Guidelines" section with
> > the above would be clearer than having a separate section.
>
> Thanks for pointing it out; I tend to agree.
>
> Before rerolling the series in entirety, here is what I have in my
> editor buffer right now, after attempting to move the formatting and
> styles into the main description.
>
> I haven't checked if the formatting works as AsciiDoc yet, though.
>
> --- >8 ---
> [[meaningful-message]]
> ==== Structure of a Commit Message
>
> 1. Title:
> The first line of the commit log message is the title that lets
> readers of `git log --oneline` quickly understand what area the
> commit touches and what problem it addresses.
>
> - Keep it short (50 characters is the soft limit).
> - Skip the full stop at the end.
> - Prefix the subject with the modified area followed by a colon
> and a space (e.g., "area: subject"). The area is typically a
> filename or identifier (e.g., `doc:`, `transport:`, `t5601:`).
> Run `git log --no-merges` on target files to see conventions.
> - Do not capitalize the first word after the "area:" prefix
> unless there is a specific reason (e.g., `HEAD` is always in
> uppercase). For example, use "doc: clarify...", not "doc:
> Clarify...".
>
> 2. Body:
> A well-structured commit message body typically follows a
> three-part flow: Observation, Solution Design, and
> Implementation.
>
> - Leave a blank line between the title and the body.
> - Wrap lines in the body of the commit log message to around 70
> columns.
> - The body of the log message must be self-contained. Do not
> rely on external URLs (including mailing list archives) as the
> sole explanation. Summarize the relevant points of external
> material so that readers can understand the change with the log
> message alone.
>
> [[present-tense]]
> 3. Observation (The Status Quo):
> Explain the problem you are solving with your change by
> describing what is wrong with the current code *without* your
> change.
>
> - As this part is always about the current state by convention,
> words like "currently" are unnecessary.
> - Write this problem statement in the present tense (e.g., "The
> code does X when given input Y", not "The code did X").
>
> 4. Solution Design (The Approach):
> Explain the approach you took, justify how it solves the problem,
> and describe why you chose the particular design over other
> alternatives.
>
> - Focus on describing _why_, not _how_ (e.g., "The code does X
> when given input Y, but it should do Z _because_...").
> - If your change only addresses a subset of a larger problem
> (e.g., it handles directories but not files because ...),
> explain this limitation. This helps future developers
> understand the boundaries of your work and whether it can be
> safely extended.
> - If your change resolves design or viability concerns raised by
> the community during prior review rounds, ensure the message
> records the resolution, explaining why the chosen approach was
> accepted over alternatives.
>
> [[imperative-mood]]
> 5. Implementation (The Execution):
> Finally, describe how the changes are implemented.
>
> - Write this in the imperative mood (e.g., "Make xyzzy do frotz",
> not "This patch makes xyzzy do..." or "I changed xyzzy..."), as
> if you are instructing an agent to make changes to the
> codebase.
> - You do not have to repeat everything readers can discern from
> the patch text. Highlight the key points in your
> implementation.
I think this might confuse readers. Now you place these points in
parallel:
1. Title
2. Body
3. Observation (The Status Quo)
4. Solution Design (The Approach)
5. Implementation (The Execution)
But acatually you mean:
1. Title
2. Body
The body typically follows three parts:
a. Observation
b. Solution Design
c. Implementation
But I haven't written much about adoc, so I don't know its syntax and
how to write it.
^ permalink raw reply
* Re: [PATCH 1/6] SubmittingPatches: clarify expected structure of commit log message
From: Weijie Yuan @ 2026-07-13 14:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7bn042ez.fsf@gitster.g>
On Sun, Jul 12, 2026 at 09:07:32AM -0700, Junio C Hamano wrote:
> Weijie Yuan <wy@wyuan.org> writes:
>
> >> +2. **Solution (The Approach)**:
> >> +3. **Command (The Instruction)**:
> >> + [[imperative-mood]]
> >> + Command the codebase to change. Write this in the **imperative
> >> + mood** (e.g., "make xyzzy do frotz" instead of "This patch makes
> >> + xyzzy do..." or "I changed xyzzy..."), as if you are giving orders
> >> + to the codebase to change its behavior.
> >
> > Stopped and confused for a moment. I am not sure that "Command" belongs
> > alongside "Observation" and "Solution" as a third part of the message.
> > Sometimes the command still describes the solution. In other words,
> > Solution and Command seem not to be logically completely separable.
>
> I do not think "Command the codebase to change" is a good phrasing.
> It would have been better to highlight the distinction between the
> design of the solution (approach) and the implementation. Perhaps
>
> 2. Design (The Approach)
>
> 3. Implementation (The Changes)
> [[imperative-mood]]
> Describe how the change is implemented. Write this in the
> imperative mood. ...
>
> or something?
Yeah, that is much clearer. I'm reading your draft in your reply to
Michael, seems good.
> >> +* **The Body**:
> >> + * Explain the *why* rather than repeating the *what* of the diff.
> >> + * Try to make the explanation self-contained. Avoid relying on
> >> + external URLs (like mailing list archives) as the sole
> >> + explanation; summarize the relevant points of the discussion
> >> + instead.
> >> + * Wrap lines to 68-72 columns.
> >
> > MyFirstContribution:
> > This commit message is intentionally formatted to 72 columns per line
> >
> > Should we update both?
>
> Perhaps just to stick to "around 70".
>
> I do not think the defaults in various editors matter.
>
> The "wrap around 70 columns" rule exists so that in a text based
> email exchange, where you lose two columns to leading "> " when
> quoted, and an additional column with each subsequent reply, the
> lines will still fit on standard 80-column terminals.
Yes, got it. I just want to say that I often see 72 columns, but I
haven't seen 68 very often. (maybe I'm too young ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH v2 06/10] sequencer: simplify handing of fixup with conflicts
From: Oswald Buddenhagen @ 2026-07-13 14:09 UTC (permalink / raw)
To: Phillip Wood; +Cc: git, Uwe Kleine-König, Junio C Hamano, Farid Zakaria
In-Reply-To: <26dc48951cea663080bacf7d8d4760528125cbf5.1783948637.git.phillip.wood@dunelm.org.uk>
On Mon, Jul 13, 2026 at 02:17:23PM +0100, Phillip Wood wrote:
>Commit e032abd5a0 (rebase: fix rewritten list for failed pick,
>2023-09-06) introduced an early return when res == -1, so if we enter
>this conditional block then res is positive. After the last couple
>of commits the only possible positive value is 1 so we can simplify
>the code by removing the conditional call to intend_to_amend() and
>call it error_with_patch() instead.
>
that part makes no sense, subverting the argumentation.
(as-is, i actually can't follow the logic, but i suppose it would be
clear with (much) more diff context. i'm not sure whether the commit
message is supposed to substitute for that, or the reviewer is supposed
to deal with that on their end.)
^ permalink raw reply
* Re: [PATCH v10 7/7] graph: add --[no-]graph-indent and log.graphIndent
From: Mirko Faina @ 2026-07-13 14:06 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, ayu.chandekar, chandrapratap3519, christian.couder, gitster,
jltobler, karthik.188, krka, peff, phillip.wood,
siddharthasthana31, Mirko Faina
In-Reply-To: <20260713-ps-pre-commit-indent-v10-7-82ddab26bc96@gmail.com>
On Mon, Jul 13, 2026 at 12:44:42PM +0200, Pablo Sabater wrote:
> Some users may prefer to not have graph indentation.
>
> Add "log.graphIndent" config variable to graph_read_config() to read the
> default preference. By default is graph indentation is true.
>
> Add --graph-indent and --no-graph-indent options to overwrite the
> default preference.
>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
> Documentation/config/log.adoc | 4 +++
> Documentation/rev-list-options.adoc | 8 ++++++
> graph.c | 10 +++++--
> revision.c | 9 +++++++
> revision.h | 2 ++
> t/t4218-log-graph-indentation.sh | 52 +++++++++++++++++++++++++++++++++++++
> 6 files changed, 83 insertions(+), 2 deletions(-)
[snip]
> diff --git a/revision.h b/revision.h
> index 569b3fa1cb..49e1380b80 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -314,6 +314,8 @@ struct rev_info {
> /* Display history graph */
> struct git_graph *graph;
> int graph_max_lanes;
> + int no_graph_indent;
> + unsigned int graph_indent_set;
These are both boolean values and could be set to be 1 bit wide.
Other than that LGTM.
Thank you for the changes.
^ permalink raw reply
* [PATCH 2/2] t1100: move creation of expected output into setup test
From: Shlok Kulshreshtha @ 2026-07-13 14:01 UTC (permalink / raw)
To: git; +Cc: Shlok Kulshreshtha, Junio C Hamano
In-Reply-To: <20260713140142.27898-1-diy2903@gmail.com>
The "expected" file was created at the top level of the script, outside
of any test. Code that runs outside of a test is not protected by the
test harness: a failure there is not reported as a test failure and is
easy to miss.
Move the here-doc that creates "expected" into the existing setup test
("test preparation: write empty tree"), using a "<<-" here-doc so its
body can be indented along with the rest of the test.
Signed-off-by: Shlok Kulshreshtha <diy2903@gmail.com>
---
t/t1100-commit-tree-options.sh | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/t/t1100-commit-tree-options.sh b/t/t1100-commit-tree-options.sh
index fabe5a97cb..b434d1848e 100755
--- a/t/t1100-commit-tree-options.sh
+++ b/t/t1100-commit-tree-options.sh
@@ -14,15 +14,14 @@ Also make sure that command line parser understands the normal
. ./test-lib.sh
-cat >expected <<EOF
-tree $EMPTY_TREE
-author Author Name <author@email> 1117148400 +0000
-committer Committer Name <committer@email> 1117150200 +0000
-
-comment text
-EOF
-
test_expect_success 'test preparation: write empty tree' '
+ cat >expected <<-EOF &&
+ tree $EMPTY_TREE
+ author Author Name <author@email> 1117148400 +0000
+ committer Committer Name <committer@email> 1117150200 +0000
+
+ comment text
+ EOF
git write-tree >treeid
'
--
2.52.0
^ permalink raw reply related
* [PATCH 1/2] t1100: modernize test style
From: Shlok Kulshreshtha @ 2026-07-13 14:01 UTC (permalink / raw)
To: git; +Cc: Shlok Kulshreshtha, Junio C Hamano
In-Reply-To: <20260713140142.27898-1-diy2903@gmail.com>
The tests in this script use the old style in which the test title and
body are passed as separate backslash-continued arguments, with bodies
indented using spaces:
test_expect_success \
'title' \
'body'
Convert them to the modern style in which the body is a single-quoted
block on its own lines, indented with a tab:
test_expect_success 'title' '
body
'
This is a style-only change; no test logic is modified.
Signed-off-by: Shlok Kulshreshtha <diy2903@gmail.com>
---
t/t1100-commit-tree-options.sh | 44 +++++++++++++++++-----------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/t/t1100-commit-tree-options.sh b/t/t1100-commit-tree-options.sh
index ae66ba5bab..fabe5a97cb 100755
--- a/t/t1100-commit-tree-options.sh
+++ b/t/t1100-commit-tree-options.sh
@@ -22,28 +22,28 @@ committer Committer Name <committer@email> 1117150200 +0000
comment text
EOF
-test_expect_success \
- 'test preparation: write empty tree' \
- 'git write-tree >treeid'
-
-test_expect_success \
- 'construct commit' \
- 'echo comment text |
- GIT_AUTHOR_NAME="Author Name" \
- GIT_AUTHOR_EMAIL="author@email" \
- GIT_AUTHOR_DATE="2005-05-26 23:00" \
- GIT_COMMITTER_NAME="Committer Name" \
- GIT_COMMITTER_EMAIL="committer@email" \
- GIT_COMMITTER_DATE="2005-05-26 23:30" \
- TZ=GMT git commit-tree $(cat treeid) >commitid 2>/dev/null'
-
-test_expect_success \
- 'read commit' \
- 'git cat-file commit $(cat commitid) >commit'
-
-test_expect_success \
- 'compare commit' \
- 'test_cmp expected commit'
+test_expect_success 'test preparation: write empty tree' '
+ git write-tree >treeid
+'
+
+test_expect_success 'construct commit' '
+ echo comment text |
+ GIT_AUTHOR_NAME="Author Name" \
+ GIT_AUTHOR_EMAIL="author@email" \
+ GIT_AUTHOR_DATE="2005-05-26 23:00" \
+ GIT_COMMITTER_NAME="Committer Name" \
+ GIT_COMMITTER_EMAIL="committer@email" \
+ GIT_COMMITTER_DATE="2005-05-26 23:30" \
+ TZ=GMT git commit-tree $(cat treeid) >commitid 2>/dev/null
+'
+
+test_expect_success 'read commit' '
+ git cat-file commit $(cat commitid) >commit
+'
+
+test_expect_success 'compare commit' '
+ test_cmp expected commit
+'
test_expect_success 'flags and then non flags' '
--
2.52.0
^ permalink raw reply related
* [PATCH 0/2] t1100: modernize test script
From: Shlok Kulshreshtha @ 2026-07-13 14:01 UTC (permalink / raw)
To: git; +Cc: Shlok Kulshreshtha
Hi,
This is a GSoC/Outreachy microproject ("Modernize a test script"). It
cleans up t/t1100-commit-tree-options.sh following the guidance Eric
Sunshine gave for t7001 in:
https://lore.kernel.org/git/CAPig+cQpUu2UO-+jWn1nTaDykWnxwuEitzVB7PnW2SS_b7V8Hg@mail.gmail.com/
Each patch makes a single kind of change:
1/2 converts the tests from the old backslash-continued
test_expect_success style with space-indented bodies to the
modern quoted-body form indented with tabs.
2/2 moves the here-doc that creates the "expected" file out of the
script's top level and into the existing setup test, so it runs
under the protection of the test harness.
There is no change to what the tests actually verify; t1100 continues to
pass all 5 tests after each patch.
I confirmed t1100 does not appear to be currently claimed on the list;
please let me know if someone is already working on it.
Thanks,
Shlok
Shlok Kulshreshtha (2):
t1100: modernize test style
t1100: move creation of expected output into setup test
t/t1100-commit-tree-options.sh | 59 +++++++++++++++++-----------------
1 file changed, 29 insertions(+), 30 deletions(-)
--
2.52.0
^ permalink raw reply
* Re: [PATCH v2 03/10] sequencer: be more careful with external merge
From: Oswald Buddenhagen @ 2026-07-13 14:01 UTC (permalink / raw)
To: Phillip Wood; +Cc: git, Uwe Kleine-König, Junio C Hamano, Farid Zakaria
In-Reply-To: <3d79362332c1208eed1fb7f8b0d431ee92fe45c5.1783948637.git.phillip.wood@dunelm.org.uk>
On Mon, Jul 13, 2026 at 02:17:20PM +0100, Phillip Wood wrote:
>If an external merge strategy cannot merge (for example because it
>would overwrite an untracked file) it exits with a non-zero exit
>code other than 1. This should be treated differently to a merge
>
s/to/from/, i think?
>with conflicts
>which is signalled by an exit code of 1
>
parenthesize, and add comma?
>because as
>the merge failed
>
(maybe add comma? here it becomes muddy ...)
>we need to reschedule the last pick. The caller
>expects us to return -1 in this case. Also reschedule without trying
>to merge if the commit message cannot be written
>
add comma?
>as that prevents us
>from successfully picking the commit.
i know that most commas (and parens (or em-dashes)) are optional in
english, but they _really_ help parsing complex sentences, because they
reduce the amount of "read-ahead" required.
i'm stopping at this commit, but subsequent ones could also use the
treatment. i trust that you don't actually need detailed suggestions.
^ permalink raw reply
* Re: [PATCH v2 01/10] t3400: restore coverage for note copying with apply backend
From: Oswald Buddenhagen @ 2026-07-13 13:43 UTC (permalink / raw)
To: Phillip Wood; +Cc: git, Uwe Kleine-König, Junio C Hamano, Farid Zakaria
In-Reply-To: <65af2ac07a2bf85336245a7d9b9f0a8a0e8affdb.1783948637.git.phillip.wood@dunelm.org.uk>
On Mon, Jul 13, 2026 at 02:17:18PM +0100, Phillip Wood wrote:
>Now that the merge backend is the default
>
add comma here for ease of parsing?
> we have lost coverage for
>"git rebase --apply" copying notes. Fix this by replacing "-m" with
>"--apply"
>
and here?
>as the previous test which uses the default backend now
>checks the merge backend.
>
^ permalink raw reply
* Re: [PATCH v1 0/3] worktree: add post-worktree-add and post-worktree-remove hooks
From: Phillip Wood @ 2026-07-13 13:19 UTC (permalink / raw)
To: Domen Kožar, phillip.wood
Cc: git, Eric Sunshine, Patrick Steinhardt,
Ævar Arnfjörð Bjarmason, Caleb White,
Junio C Hamano
In-Reply-To: <CAMvcdZS=ZYbLmjKaGJvjQ_fWYhVbOzwMvYq+MMENWPYi_RiqvQ@mail.gmail.com>
Hi Domen
Unfortunately it doesn't look like your message appeared on the list,
sadly I'm not sure it accepts multipart/alternative messages even when
they contain a plain-text version of the message.
On 10/07/2026 18:20, Domen Kožar wrote:
> Hi Phillip,
>
> thanks for the quick and careful read.
>
> > It is useful for copying across untracked files to the new worktree
> > like "config.mak".
>
> That is a nice example, and it needs the hook to also fire for
> --no-checkout and --orphan, which post-checkout does not cover.
>
> > Looking at the existing code, if the checkout fails then we remove
> > the worktree because "is_junk == 1" when remove_junk() is called via
> > atexit() so I think it is correct to skip the new hook in that case.
>
> Right, when the checkout itself fails the worktree is removed as junk
> and neither hook runs; no disagreement there. The case I was asking
> about is the post-checkout hook itself failing: that runs after
> is_junk is cleared, so the worktree survives, but post-worktree-add
> is currently skipped and tooling that registers worktrees would miss
> one that exists. I kept the skip because a failing post-checkout
> already signals a broken setup, but I am happy to run
> post-worktree-add whenever the worktree was created, regardless of
> the earlier hook's exit status, if that is preferred.
Oh sorry I'd misunderstood the question. I think I'd lean towards
running the hook anyway because we've still populated a new worktree,
even if the post-checkout hooks fails.
> > The new hook is run after the checkout, but before the post-checkout
> > hook - we should document their relative order.
>
> Unless I am misreading my own series, it is the other way around:
> add_worktree() invokes post-checkout first and post-worktree-add
> after it, t2400 has a test pinning that order ('"add" runs
> post-worktree-add after post-checkout'), and githooks.adoc says "It
> runs after the post-checkout hook, and is skipped if that hook
> fails." If that did not come across I am happy to reword the
> documentation.
Oops, when I wrote that I was looking at the wrong branch - I had my
"add-worktree-hook" checked out and confused it with this patch. It's
great to see that there is a test and documentation for this.
> > I'm wondering if either of those is useful if we're running the
> > hook in the new worktree.
>
> Strictly they are derivable from inside, --show-toplevel for the path
> and the basename of --git-dir for the id.
Isn't the worktree path the current working directory of the hook script?
> I passed them anyway so
> that one script can serve both hooks: post-worktree-remove has to
> receive them as arguments because the worktree is gone by the time it
> runs, and keeping the two signatures identical makes shared hook code
> simpler. I can drop them from post-worktree-add if the symmetry is
> not considered worth it.
Oh right, as a counter argument I wonder if having a different argument
count for the two hooks makes it easier for a script that's shared
between the two hooks to determine which hook has invoked it. Is the
worktree id useful for anything apart from accessing on worktree's local
refs from another worktree?
> > So the hook knows a worktree was removed but not which one?
>
> It always gets the worktree id as $2; only the path in $1 can be
> empty, and only for entries whose gitdir file is missing or
> unreadable, where git itself no longer knows the path either. Tooling
> that recorded the id at post-worktree-add time can still match the
> removal.
That answers why you want the id.
One thought I had after I wrote my mail was that worktrees can be
renamed - do we want a hook for that so that external tools can move any
services they've started and update their id -> path mapping.
Thanks
Phillip
> Thanks,
> Domen
>
> On Fri, Jul 10, 2026 at 3:34 AM Phillip Wood <phillip.wood123@gmail.com
> <mailto:phillip.wood123@gmail.com>> wrote:
>
> Hi Domen
>
> On 10/07/2026 00:36, Domen Kožar wrote:
> >
> > Today there is no reliable trigger to set that up when a worktree
> > appears: post-checkout does not fire for --no-checkout or --orphan
> > and cannot be told apart from a plain checkout. Nothing at all fires
> > when a worktree goes away, so stale databases and services pile up
> > after "git worktree remove" or a manual rm followed by "git worktree
> > prune". Wrapping the worktree commands only helps when every tool,
> > human or agent, goes through the wrapper.
>
> I agree a hook that's run after the worktree is added is useful (I have
> a patch for it that I've never got round to cleaning up and sending so
> thank you for working on this). It is useful for copying across
> untracked files to the new worktree like "config.mak".
>
> > Patch 1 adds a post-worktree-add hook that fires after the working
> > tree is fully set up. Patch 2 adds post-worktree-remove for "git
> > worktree remove". Patch 3 extends it to "git worktree prune" so that
> > manually deleted worktrees are also observed.
>
> I don't have a strong opinion on a hook running when a worktree is
> removed - an IDE that cares about that could set up a filesystem watch
> on the directory but I guess adding a hook doesn't do any harm.
> > Two design points I would especially appreciate feedback on:
> >
> > * post-worktree-add runs after post-checkout and is skipped when
> > post-checkout fails. An argument could be made that it should run
> > whenever the worktree was created, regardless of the earlier
> > hook's exit status, since tooling registering worktrees would
> > otherwise miss one that does exist.
>
> Looking at the existing code, if the checkout fails then we remove the
> worktree because "is_junk == 1" when remove_junk() is called via
> atexit() so I think it is correct to skip the new hook in that case.
>
> The new hook is run after the checkout, but before the post-checkout
> hook - we should document their relative order. I see the hook is
> run in
> the new worktree and passed the absolute directory and worktree id. I'm
> wondering if either of those is useful if we're running the hook in the
> new worktree.
>
> > * for entries pruned because their gitdir file points to a location
> > that no longer exists, the hook receives the recorded path; when
> > the path cannot be determined at all (missing or corrupt gitdir
> > file) it receives an empty string.
>
> So the hook knows a worktree was removed but not which one?
>
> Thanks
>
> Phillip
>
> > Thanks,
> > Domen
> >
> > Domen Kožar (3):
> > worktree: add post-worktree-add hook
> > worktree: add post-worktree-remove hook
> > worktree: run post-worktree-remove hook when pruning
> >
> > Documentation/githooks.adoc | 41 +++++++++++++
> > builtin/worktree.c | 73 ++++++++++++++++++-----
> > t/t2400-worktree-add.sh | 113 +++++++++++++++++++++++++++++
> +++++++
> > t/t2401-worktree-prune.sh | 88 ++++++++++++++++++++++++++++
> > t/t2403-worktree-move.sh | 44 ++++++++++++++
> > worktree.c | 1 -
> > worktree.h | 6 +-
> > 7 files changed, 347 insertions(+), 19 deletions(-)
> >
> >
> > base-commit: f85a7e662054a7b0d9070e432508831afa214b47
>
>
^ permalink raw reply
* Re: [PATCH v3] sequencer: honor --empty when a fixup!/squash! empties its target
From: Phillip Wood @ 2026-07-13 13:18 UTC (permalink / raw)
To: Junio C Hamano, Farid Zakaria
Cc: git, Phillip Wood, Elijah Newren, Patrick Steinhardt
In-Reply-To: <xmqqh5m494yh.fsf@gitster.g>
On 12/07/2026 06:01, Junio C Hamano wrote:
> Farid Zakaria <farid.m.zakaria@gmail.com> writes:
>
>> When "git rebase --autosquash" melds a "fixup!" or "squash!" commit into
>> its target, the result can be a commit that no longer changes anything
>> relative to its parent, for example when the melded change reverts the
>> target. Rather than dropping or keeping this empty commit, the rebase
>> stops with
>>
>> You asked to amend the most recent commit, but doing so would
>> make it empty. ...
>>
>> and the "--empty" option has no effect on it. This makes backing a
>> change out of a series awkward: reverting a commit as a "fixup!" and
>> running "git rebase --autosquash --empty=drop" ought to remove both the
>> commit and its revert, but it halts instead.
>> ...
>> Changes in v3:
>> * Switch the new tests' assertions from grep to test_grep for better
>> diagnostics (per review).
>> * Link to v2: https://lore.kernel.org/r/20260710-fz-autosquash-empty-v2-1-fa1e277e05f8@gmail.com
>
> I see you are already working well with Phillip, which is great.
>
> This topic, when merged to 'seen', seems to have quite a lot of
> overlaps with his pw/rebase-drop-notes-with-commit topic.
Oh, I should have thought of that
> We are
> expecting the topic to be rerolled, and I was under the impression
> that the remaining issues in that topic were all minor (Phillip,
> correct me if I am wrong) and hopefully we will see it in 'next'
> not in so distant future.
I've just sent a new version and cc'd Farid, I'll try and take look at
this patch tomorrow
> So it might make sense for you to coordinate with Phillip, and wait
> for his topic to be merged to 'next'. After that happens, you would
> prepare a merge commit of the other branch into f85a7e6620 (Start
> Git 2.56 cycle, 2026-07-06) or some other stable point, and rebuild
> this patch on top of it. That way, it will be much less likely that
> I'd make stupid and unnecessary mismerges when attempting to
> integrate this topic into my tree.
That makes sense, assuming no-one has any more comments on
'pw/rebase-drop-notes-with-commit' it should in be 'next' fairly soon.
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH] sequencer: honor --empty when a fixup!/squash! empties its target
From: phillip.wood123 @ 2026-07-13 13:18 UTC (permalink / raw)
To: Yuxuan Chen
Cc: farid.m.zakaria@gmail.com, git@vger.kernel.org, gitster@pobox.com,
newren@gmail.com, phillip.wood@dunelm.org.uk, ps@pks.im
In-Reply-To: <20260710182937.716304-1-i@yuxuan.ch>
Hi Yuxuan
On 10/07/2026 19:30, Yuxuan Chen wrote:
>
>> Using an empty commit has a marker has the advantage that applying it cannot
>> create conflicts, so you only have to deal with the conflicts caused by the
>> commit being dropped, not the by fixup not applying cleanly.
>
> I am concerned, however, that representing a `drop!` commit as an empty marker
> would be semantically unsound. We expect `rebase --autosquash` to drop the
> target commit, but until that rebase happens, the repository is not in a state
> where we consider the target commit dropped: the target's changes are still
> present, and the empty marker changes nothing. Therefore, I think a `drop!`
> commit should contain the inverse of the patch we intend to drop. That way,
> the repository state reflects the intended removal even before autosquash
> rewrites the history.
That's a good point. Looking at the gitgitgadget issue tracker [1],
there is a suggestion to add a new option to revert that behaves like
git revert -n <commit> &&
git commit -m 'drop! '"$(git show -s --oneline <commit>)"
and then "git rebase --autosquash" would replace "pick" with "drop" for
the commit we want to drop and drop the "drop!" commit as well. That
avoids conflicts when dropping the commit and means anything built on
top of the "drop!" commit before the rebase does not see the changes in
the commit that we want to drop because it has been reverted. That seems
to be the best of both worlds.
> I recognize that applying the inverse patch may cause conflicts. However,
> this is not a new problem; `git revert` has the same issue when the inverse
> patch does not apply cleanly. Such conflicts reflect the actual difficulty of
> undoing the change at that point in the history.
I agree conflicts are a fact of life when rebasing, but I think it is
worth avoiding them where we can.
Thanks
Phillip
[1] https://github.com/gitgitgadget/git/issues/259
^ permalink raw reply
* [PATCH v2 10/10] sequencer: do not record dropped commits as rewritten
From: Phillip Wood @ 2026-07-13 13:17 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Junio C Hamano, Oswald Buddenhagen,
Farid Zakaria, Phillip Wood
In-Reply-To: <cover.1783948637.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
If a commit gets dropped because its changes are already upstream
then we should not record it as rewritten. As well as confusing any
post-rewrite hooks this means we end up copying the notes from the
dropped commit to the commit that was picked immediately before the
one that was dropped.
While we do not want to record the dropped commit is rewritten, if
it is the final commit in a chain of fixups then we need to flush
the list of rewritten commits. The behavior of an "edit" command
where the commit is dropped is changed so that "rebase --continue"
will not amend the previous pick. However, as the code comment notes
it will still be erroneously recorded as rewritten when the rebase
continues. That will need to be addressed separately along with not
recording skipped commits as rewritten.
The initialization of "drop_commit" is moved to ensure it is initialized
when rewording a fast-forwarded commit.
Reported-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Tested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 24 +++++++++++++++++++-----
t/t3400-rebase.sh | 12 ++++++++++++
t/t5407-post-rewrite-hook.sh | 23 +++++++++++++++++++++++
3 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 4b89349251b..7bc885085f9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2264,6 +2264,7 @@ enum pick_result {
PICK_RESULT_ERROR = -1,
PICK_RESULT_OK,
PICK_RESULT_CONFLICTS,
+ PICK_RESULT_DROPPED,
};
static enum pick_result do_pick_commit(struct repository *r,
@@ -2279,7 +2280,7 @@ static enum pick_result do_pick_commit(struct repository *r,
const char *base_label, *next_label, *reflog_action;
char *author = NULL;
struct commit_message msg = { NULL, NULL, NULL, NULL };
- int res, unborn = 0, reword = 0, allow, drop_commit;
+ int res, unborn = 0, reword = 0, allow, drop_commit = 0;
enum todo_command command = item->command;
struct commit *commit = item->commit;
@@ -2509,7 +2510,6 @@ static enum pick_result do_pick_commit(struct repository *r,
goto leave;
}
- drop_commit = 0;
allow = allow_empty(r, opts, commit);
if (allow < 0) {
res = allow;
@@ -2574,6 +2574,8 @@ static enum pick_result do_pick_commit(struct repository *r,
return PICK_RESULT_ERROR;
else if (res > 0)
return PICK_RESULT_CONFLICTS;
+ else if (drop_commit)
+ return PICK_RESULT_DROPPED;
else
return PICK_RESULT_OK;
}
@@ -4994,18 +4996,30 @@ static int pick_one_commit(struct repository *r,
} else if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
int res = pick_res == PICK_RESULT_CONFLICTS;
+ int to_amend = pick_res != PICK_RESULT_CONFLICTS &&
+ pick_res != PICK_RESULT_DROPPED;
- if (pick_res == PICK_RESULT_OK) {
+ /*
+ * NEEDSWORK: Do not record the commit as rewritten when
+ * continuing if it was dropped. Does it even make sense
+ * to stop if the commit was dropped?
+ */
+ if (pick_res == PICK_RESULT_OK ||
+ pick_res == PICK_RESULT_DROPPED) {
if (!opts->verbose)
term_clear_line();
fprintf(stderr, _("Stopped at %s... %.*s\n"),
short_commit_name(r, commit), item->arg_len, arg);
}
- return error_with_patch(r, commit,
- arg, item->arg_len, opts, res, !res);
+ return error_with_patch(r, commit, arg, item->arg_len, opts,
+ res, to_amend);
} else if (pick_res == PICK_RESULT_OK) {
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
+ return 0;
+ } else if (pick_res == PICK_RESULT_DROPPED) {
+ if (is_final_fixup(todo_list))
+ flush_rewritten_pending();
return 0;
} else if (pick_res == PICK_RESULT_CONFLICTS &&
is_fixup(item->command)) {
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index f0e7fcf649a..1d09886ea35 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -274,6 +274,18 @@ test_expect_success 'rebase --apply can copy notes' '
git reset --hard n3 &&
git rebase --apply --onto n1 n2 &&
test "a note" = "$(git notes show HEAD)"
+'
+
+test_expect_success 'rebase drops notes of dropped commits' '
+ git checkout n1 &&
+ echo n3 >n3.t &&
+ echo n4 >n4.t &&
+ git add n3.t n4.t &&
+ git commit -m n34 &&
+ git rebase HEAD n3 &&
+ test_commit_message HEAD -m n2 &&
+ test_must_fail git notes list HEAD >actual &&
+ test_must_be_empty actual
'
test_expect_success 'rebase commit with an ancient timestamp' '
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index ad7f8c6f002..51991956d1d 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -306,6 +306,29 @@ test_expect_success 'git rebase -i (exec)' '
cat >expected.data <<-EOF &&
$(git rev-parse C) $(git rev-parse HEAD^)
$(git rev-parse D) $(git rev-parse HEAD)
+ EOF
+ verify_hook_input
+'
+
+test_expect_success 'rebase with commits that become empty' '
+ cat >todo <<-\EOF &&
+ pick H
+ pick E
+ fixup I
+ fixup H
+ pick G
+ pick I
+ EOF
+ (
+ set_replace_editor todo &&
+ git rebase -i --empty=drop A A
+ ) &&
+ echo rebase >expected.args &&
+ cat >expected.data <<-EOF &&
+ $(git rev-parse H) $(git rev-parse HEAD~2)
+ $(git rev-parse E) $(git rev-parse HEAD~1)
+ $(git rev-parse I) $(git rev-parse HEAD~1)
+ $(git rev-parse G) $(git rev-parse HEAD)
EOF
verify_hook_input
'
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH v2 09/10] sequencer: use an enum to represent result of picking a commit
From: Phillip Wood @ 2026-07-13 13:17 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Junio C Hamano, Oswald Buddenhagen,
Farid Zakaria, Phillip Wood
In-Reply-To: <cover.1783948637.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Rather than using an integer where -1 is an error, 0 is success and
1 means there were conflicts use an enum. This is clearer and lets
us add a separate return value for commits that are dropped because
they become empty in the next commit.
Note we continue to use "return error(...)" to return errors and
take advantage of C's lax typing of enums
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 61 +++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 45 insertions(+), 16 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index ff4547d417e..4b89349251b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2260,10 +2260,16 @@ static const char *reflog_message(struct replay_opts *opts,
return buf.buf;
}
-static int do_pick_commit(struct repository *r,
- struct todo_item *item,
- struct replay_opts *opts,
- int final_fixup, int *check_todo)
+enum pick_result {
+ PICK_RESULT_ERROR = -1,
+ PICK_RESULT_OK,
+ PICK_RESULT_CONFLICTS,
+};
+
+static enum pick_result do_pick_commit(struct repository *r,
+ struct todo_item *item,
+ struct replay_opts *opts,
+ int final_fixup, int *check_todo)
{
struct replay_ctx *ctx = opts->ctx;
unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;
@@ -2564,7 +2570,12 @@ static int do_pick_commit(struct repository *r,
free(author);
update_abort_safety_file();
- return res;
+ if (res < 0)
+ return PICK_RESULT_ERROR;
+ else if (res > 0)
+ return PICK_RESULT_CONFLICTS;
+ else
+ return PICK_RESULT_OK;
}
static int prepare_revs(struct replay_opts *opts)
@@ -4960,37 +4971,47 @@ static int pick_one_commit(struct repository *r,
struct replay_opts *opts,
int *check_todo, int* reschedule)
{
- int res;
+ enum pick_result pick_res;
struct todo_item *item = todo_list->items + todo_list->current;
const char *arg = todo_item_get_arg(todo_list, item);
- res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
- check_todo);
+ pick_res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
+ check_todo);
if (!is_rebase_i(opts))
- return res;
+ switch (pick_res) {
+ case PICK_RESULT_ERROR:
+ return -1;
+ case PICK_RESULT_CONFLICTS:
+ return 1;
+ default:
+ return 0;
+ }
- if (res < 0) {
+ if (pick_res == PICK_RESULT_ERROR) {
/* Reschedule */
*reschedule = 1;
return -1;
} else if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
- if (!res) {
+ int res = pick_res == PICK_RESULT_CONFLICTS;
+
+ if (pick_res == PICK_RESULT_OK) {
if (!opts->verbose)
term_clear_line();
fprintf(stderr, _("Stopped at %s... %.*s\n"),
short_commit_name(r, commit), item->arg_len, arg);
}
return error_with_patch(r, commit,
arg, item->arg_len, opts, res, !res);
- } else if (!res) {
+ } else if (pick_res == PICK_RESULT_OK) {
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
return 0;
- } else if (res && is_fixup(item->command)) {
+ } else if (pick_res == PICK_RESULT_CONFLICTS &&
+ is_fixup(item->command)) {
return error_failed_squash(r, item->commit, opts,
item->arg_len, arg);
- } else if (res) {
+ } else if (pick_res == PICK_RESULT_CONFLICTS) {
int to_amend = 0;
struct object_id oid;
@@ -5008,7 +5029,7 @@ static int pick_one_commit(struct repository *r,
to_amend = 1;
return error_with_patch(r, item->commit, arg, item->arg_len,
- opts, res, to_amend);
+ opts, 1, to_amend);
}
BUG("Unhandled return value from do_pick_commit()");
@@ -5547,7 +5568,15 @@ static int single_pick(struct repository *r,
TODO_PICK : TODO_REVERT;
item.commit = cmit;
- return do_pick_commit(r, &item, opts, 0, &check_todo);
+ switch (do_pick_commit(r, &item, opts, 0, &check_todo)) {
+ case PICK_RESULT_ERROR:
+ return -1;
+ case PICK_RESULT_CONFLICTS:
+ return 1;
+ default:
+ return 0;
+ }
+
}
int sequencer_pick_revisions(struct repository *r,
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH v2 08/10] sequencer: simplify pick_one_commit()
From: Phillip Wood @ 2026-07-13 13:17 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Junio C Hamano, Oswald Buddenhagen,
Farid Zakaria, Phillip Wood
In-Reply-To: <cover.1783948637.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Unless we're rebasing all we do in pick_one_commit() is call
do_pick_commit() and return its result. Simplify the code by returning
early if we're not rebasing so that we don't have to continually call
is_rebase_i() in the rest of the function. Note that there are a couple
of conditions that do not call is_rebase_i() but they check for either
an "edit" or a "fixup" command, both of which imply we're rebasing.
The only block that does not return early is the one guarded by
"!res". Move the return into that block to make it clear that after
recording the commit as rewritten all we do is return from the function.
As the conditional blocks are all mutually exclusive (either the
conditions are mutually exclusive, or an earlier conditional block
that would match a later one contains a "return" statement) chain
them together with "else if" to make that clear.
While we could remove "res" from the conditions below "if (!res)"
they are left alone because, when we start using an enum in the next
commit, it makes it clear that these clauses are handling cases where
there are conflicts.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 5f5ff3783e6..ff4547d417e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4966,12 +4966,14 @@ static int pick_one_commit(struct repository *r,
res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
check_todo);
- if (is_rebase_i(opts) && res < 0) {
+ if (!is_rebase_i(opts))
+ return res;
+
+ if (res < 0) {
/* Reschedule */
*reschedule = 1;
return -1;
- }
- if (item->command == TODO_EDIT) {
+ } else if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
if (!res) {
if (!opts->verbose)
@@ -4981,14 +4983,14 @@ static int pick_one_commit(struct repository *r,
}
return error_with_patch(r, commit,
arg, item->arg_len, opts, res, !res);
- }
- if (is_rebase_i(opts) && !res)
+ } else if (!res) {
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
- if (res && is_fixup(item->command)) {
+ return 0;
+ } else if (res && is_fixup(item->command)) {
return error_failed_squash(r, item->commit, opts,
item->arg_len, arg);
- } else if (res && is_rebase_i(opts)) {
+ } else if (res) {
int to_amend = 0;
struct object_id oid;
@@ -5008,7 +5010,8 @@ static int pick_one_commit(struct repository *r,
return error_with_patch(r, item->commit, arg, item->arg_len,
opts, res, to_amend);
}
- return res;
+
+ BUG("Unhandled return value from do_pick_commit()");
}
static int pick_commits(struct repository *r,
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH v2 07/10] sequencer: remove unnecessary condition in pick_one_commit()
From: Phillip Wood @ 2026-07-13 13:17 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Junio C Hamano, Oswald Buddenhagen,
Farid Zakaria, Phillip Wood
In-Reply-To: <cover.1783948637.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
item->commit holds the commit to be picked and so it must be non-NULL
otherwise pick_one_commit() would not know which commit to pick.
It is also unconditionally dereferenced in do_pick_commit() which is
called at the top of this function. Therefore the check to see if it
is non-NULL is superfluous.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index a70889a107e..5f5ff3783e6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4988,7 +4988,7 @@ static int pick_one_commit(struct repository *r,
if (res && is_fixup(item->command)) {
return error_failed_squash(r, item->commit, opts,
item->arg_len, arg);
- } else if (res && is_rebase_i(opts) && item->commit) {
+ } else if (res && is_rebase_i(opts)) {
int to_amend = 0;
struct object_id oid;
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox