* Re: [PATCH 04/16] odb/source-packed: start converting to a proper `struct odb_source`
From: Karthik Nayak @ 2026-06-08 15:29 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260604-pks-odb-source-packed-v1-4-2e7ab31b4b5c@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2816 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> Start converting `struct odb_source_packed` into a proper pluggable
> `struct odb_source` by embedding the base struct and assigning it the
> new `ODB_SOURCE_PACKED` type. Furthermore, wire up lifecycle management
> of this source by implementing the `free` callback and taking ownership
> of the chdir notifications.
>
> Note that the packed source is not yet functional as a standalone `struct
> odb_source`, as it's missing all of the callback implementations. These
> will be wired up in subsequent commits.
Okay, so individual commits going on will implement the callbacks.
[snip]
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 12e785be48..f81a990cbd 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -1,11 +1,50 @@
> #include "git-compat-util.h"
> +#include "abspath.h"
> +#include "chdir-notify.h"
> #include "odb/source-packed.h"
> +#include "packfile.h"
> +
> +static void odb_source_packed_reparent(const char *name UNUSED,
> + const char *old_cwd,
> + const char *new_cwd,
> + void *cb_data)
> +{
> + struct odb_source_packed *packed = cb_data;
> + char *path = reparent_relative_path(old_cwd, new_cwd,
> + packed->base.path);
> + free(packed->base.path);
> + packed->base.path = path;
> +}
> +
> +static void odb_source_packed_free(struct odb_source *source)
> +{
> + struct odb_source_packed *packed = odb_source_packed_downcast(source);
> +
> + chdir_notify_unregister(NULL, odb_source_packed_reparent, packed);
> +
> + for (struct packfile_list_entry *e = packed->packs.head; e; e = e->next)
> + free(e->pack);
> + packfile_list_clear(&packed->packs);
> +
> + strmap_clear(&packed->packs_by_path, 0);
> + odb_source_release(&packed->base);
> + free(packed);
> +}
>
> struct odb_source_packed *odb_source_packed_new(struct odb_source_files *parent)
> {
> - struct odb_source_packed *store;
> - CALLOC_ARRAY(store, 1);
> - store->files = parent;
> - strmap_init(&store->packs_by_path);
> - return store;
> + struct odb_source_packed *packed;
> +
Nit: we could've had a better diff if we used `struct odb_source_packed
*packed` from the start. But its tiny and doesn't bother me.
> + CALLOC_ARRAY(packed, 1);
> + odb_source_init(&packed->base, parent->base.odb, ODB_SOURCE_PACKED,
> + parent->base.path, parent->base.local);
> + packed->files = parent;
> + strmap_init(&packed->packs_by_path);
> +
> + packed->base.free = odb_source_packed_free;
> +
> + if (!is_absolute_path(parent->base.path))
> + chdir_notify_register(NULL, odb_source_packed_reparent, packed);
> +
Tangent: seems like no one sets the 'name' field within
`chdir_notify_register()`. It is meant for tracing purposes, but if no
one is using it, we might as well remove it...? Perhaps #leftoverbits
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 05/16] odb/source-packed: wire up `close()` callback
From: Karthik Nayak @ 2026-06-08 15:31 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260604-pks-odb-source-packed-v1-5-2e7ab31b4b5c@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> Wire up a new `close()` callback for the packed source and call it from
> the "files" source via the generic `odb_source_close()` interface.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> odb/source-files.c | 2 +-
> odb/source-packed.c | 16 ++++++++++++++++
> packfile.c | 12 ------------
> packfile.h | 6 ------
> 4 files changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 3608808e7c..9b0fa9ccdc 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -38,7 +38,7 @@ static void odb_source_files_close(struct odb_source *source)
> {
> struct odb_source_files *files = odb_source_files_downcast(source);
> odb_source_close(&files->loose->base);
> - packfile_store_close(files->packed);
> + odb_source_close(&files->packed->base);
> }
>
> static void odb_source_files_reprepare(struct odb_source *source)
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index f81a990cbd..74805be1dd 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -1,6 +1,7 @@
> #include "git-compat-util.h"
> #include "abspath.h"
> #include "chdir-notify.h"
> +#include "midx.h"
> #include "odb/source-packed.h"
> #include "packfile.h"
>
> @@ -16,6 +17,20 @@ static void odb_source_packed_reparent(const char *name UNUSED,
> packed->base.path = path;
> }
>
> +static void odb_source_packed_close(struct odb_source *source)
> +{
> + struct odb_source_packed *packed = odb_source_packed_downcast(source);
> +
> + for (struct packfile_list_entry *e = packed->packs.head; e; e = e->next) {
> + if (e->pack->do_not_close)
> + BUG("want to close pack marked 'do-not-close'");
> + close_pack(e->pack);
> + }
> + if (packed->midx)
> + close_midx(packed->midx);
> + packed->midx = NULL;
> +}
> +
>
Most of my ODB understandings is coming from reviewing your patches. But
I really like how we can map the current workings to the ODB interface.
> static void odb_source_packed_free(struct odb_source *source)
> {
> struct odb_source_packed *packed = odb_source_packed_downcast(source);
> @@ -42,6 +57,7 @@ struct odb_source_packed *odb_source_packed_new(struct odb_source_files *parent)
> strmap_init(&packed->packs_by_path);
>
> packed->base.free = odb_source_packed_free;
> + packed->base.close = odb_source_packed_close;
>
This is what I mean :)
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH] describe: limit default ref iteration to tags
From: Tamir Duberstein @ 2026-06-08 15:46 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Shawn O. Pearce, Junio C Hamano, Jeff King
In-Reply-To: <aiZoYE8koq1UKlWq@pks.im>
On Sun, Jun 7, 2026 at 11:59 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Sun, Jun 07, 2026 at 04:51:53PM -0400, Tamir Duberstein wrote:
> > Unless --all is given, get_name() rejects every ref outside refs/tags/.
> > The rejection happens only after the ref backend has enumerated the ref,
> > so repositories with many other refs spend most of a simple describe
> > invocation visiting refs which cannot affect its result.
>
> Right. The relevant block is this one:
>
> if (skip_prefix(ref->name, "refs/tags/", &path_to_match)) {
> is_tag = 1;
> } else if (all) {
> if ((exclude_patterns.nr || patterns.nr) &&
> !skip_prefix(ref->name, "refs/heads/", &path_to_match) &&
> !skip_prefix(ref->name, "refs/remotes/", &path_to_match)) {
> /* Only accept reference of known type if there are match/exclude patterns */
> return 0;
> }
> } else {
> /* Reject anything outside refs/tags/ unless --all */
> return 0;
> }
>
> So we really only use tags unless "--all" is given.
>
> > Commit 8a5a1884e9 (Avoid accessing non-tag refs in git-describe unless
> > --all is requested, 2008-02-24) moved this rejection before object
> > lookup, but left iteration unscoped. Pass the existing refs/tags/
> > restriction to the iterator unless --all is given so the backend can
> > avoid unrelated refs.
> >
> > On a checkout with 124,357 refs, of which 330 were tags, I ran the
> > following command with the parent and patched binaries:
> >
> > hyperfine --warmup 3 --runs 15 \
> > 'git describe --always --long --abbrev=40 HEAD'
> >
> > The results were:
> >
> > parent this commit
> > elapsed 196.2 ms 63.3 ms
> > user 69.5 ms 48.0 ms
> > system 123.0 ms 12.0 ms
>
> It's a bit curious that you don't post the hyperfine(1) results as-is
> here.
Agreed, will include that in v2. For reference:
Benchmark 1: parent
Time (mean ± σ): 171.7 ms ± 18.5 ms [User: 23.9 ms,
System: 133.6 ms]
Range (min … max): 142.3 ms … 198.3 ms 15 runs
Benchmark 2: this commit
Time (mean ± σ): 9.9 ms ± 1.1 ms [User: 3.3 ms,
System: 4.7 ms]
Range (min … max): 8.8 ms … 13.1 ms 15 runs
>
> > The wall-time standard deviations were 13.2 ms and 2.6 ms, respectively,
> > for a 3.10x speedup.
>
> Makes sense that this would result in a sizeable speedup, depending of
> course on the shape of the existing refs in the repository.
>
> > diff --git a/builtin/describe.c b/builtin/describe.c
> > index 1c47d7c0b7..3532c8ff22 100644
> > --- a/builtin/describe.c
> > +++ b/builtin/describe.c
> > @@ -740,6 +740,9 @@ int cmd_describe(int argc,
> > return ret;
> > }
> >
> > + if (!all)
> > + for_each_ref_opts.prefix = "refs/tags/";
> > +
> > hashmap_init(&names, commit_name_neq, NULL, 0);
> > refs_for_each_ref_ext(get_main_ref_store(the_repository),
> > get_name, NULL, &for_each_ref_opts);
>
> Another performance optimization that we could do here is to wire up the
> exclude patterns via `for_each_ref_opts.exclude_patterns`. But that's
> outside the scope of this patch series, and also much less likely to
> help many use cases out there.
I tried this and have a separate patch prepared.
The patterns cannot be passed through verbatim: `git describe
--exclude=foo` excludes the exact name `foo`, while the refs API would
treat `foo` as a directory prefix and also skip `foo/*`. The patch
therefore passes only patterns consisting of a literal prefix followed
by trailing asterisks, adds back the applicable ref namespace, and
retains the existing callback filtering.
With 30,000 packed remote-tracking refs under an excluded prefix, the
perf test invokes `git describe` ten times per run:
```
master patched
describe excluding many refs 0.16(0.07+0.05) 0.12(0.04+0.05)
```
That is a 25% wall-time reduction, with user CPU falling from 0.07 to
0.04 seconds.
I also tested a larger checkout with 62,170 refs under
`refs/remotes/origin/`:
```
git describe --all --exact-match --exclude='origin/*' HEAD
```
This improved from 176.7 ms to 161.3 ms, or about 10%. Startup work
unrelated to ref iteration dominates more of that repository's runtime.
>
> > diff --git a/t/perf/p6100-describe.sh b/t/perf/p6100-describe.sh
> > index 069f91ce49..dfcaf59e90 100755
> > --- a/t/perf/p6100-describe.sh
> > +++ b/t/perf/p6100-describe.sh
> > @@ -5,6 +5,12 @@ test_description='performance of git-describe'
> >
> > test_perf_default_repo
> >
> > +test_lazy_prereq PERF_REFFILES '
> > + test "$(git rev-parse --show-ref-format)" = files
> > +'
> > +
> > +ref_count=10000
>
> Let's not declare this variable outside of tests.
Done in v2.
>
> > @@ -27,4 +33,18 @@ test_perf 'describe HEAD with one tag' '
> > git describe --match=new HEAD
> > '
> >
> > +test_expect_success PERF_REFFILES 'set up many unrelated refs' '
> > + git tag -m tip tip HEAD &&
> > + for i in $(test_seq $ref_count)
> > + do
> > + printf "create refs/heads/describe-perf/%05d HEAD\n" $i ||
> > + return 1
> > + done >instructions &&
> > + git update-ref --stdin <instructions
> > +'
>
> Why is this limited to the "files" backend, only? The logic should work
> for both backends as-is.
You're right, fixed in v2.
>
> Thanks!
Thanks for the quick review!
^ permalink raw reply
* Re: [PATCH] describe: limit default ref iteration to tags
From: Tamir Duberstein @ 2026-06-08 15:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Patrick Steinhardt
In-Reply-To: <xmqqecihyzse.fsf@gitster.g>
On Mon, Jun 8, 2026 at 5:36 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tamir Duberstein <tamird@gmail.com> writes:
>
> [jc: Removing Shawn from CC who passed away quite a while ago, RIP].
>
> > Unless --all is given, get_name() rejects every ref outside refs/tags/.
> > The rejection happens only after the ref backend has enumerated the ref,
> > so repositories with many other refs spend most of a simple describe
> > invocation visiting refs which cannot affect its result.
> > ...
> > Both revisions were built with -O3, -mcpu=native, and ThinLTO using
> > Apple clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro
> > (Mac16,6) with a 16-core Apple M4 Max (12 performance and four
> > efficiency cores) and 128 GB RAM.
> >
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> > builtin/describe.c | 3 +++
> > t/perf/p6100-describe.sh | 20 ++++++++++++++++++++
> > 2 files changed, 23 insertions(+)
>
> Interesting. How would this relate to and work well with
> <20260601233727.43558-1-jacob.e.keller@intel.com>?
They are orthogonal. That patch changes the argument construction
inside the `contains` block, which invokes `cmd_name_rev()` and
returns. This patch changes the ref iterator used after that block, so
it only affects the ordinary, non-`--contains` path.
>
> > +test_lazy_prereq PERF_REFFILES '
> > + test "$(git rev-parse --show-ref-format)" = files
> > +'
> > +
> > +ref_count=10000
> > +
> > # clear out old tags and give us a known state
> > test_expect_success 'set up tags' '
> > git for-each-ref --format="delete %(refname)" refs/tags >to-delete &&
> > @@ -27,4 +33,18 @@ test_perf 'describe HEAD with one tag' '
> > git describe --match=new HEAD
> > '
> >
> > +test_expect_success PERF_REFFILES 'set up many unrelated refs' '
> > + git tag -m tip tip HEAD &&
> > + for i in $(test_seq $ref_count)
> > + do
> > + printf "create refs/heads/describe-perf/%05d HEAD\n" $i ||
> > + return 1
> > + done >instructions &&
> > + git update-ref --stdin <instructions
> > +'
> > +
> > +test_perf 'describe exact tag with many loose refs' --prereq PERF_REFFILES '
> > + git describe --exact-match HEAD
> > +'
> > +
>
> Is there a strong reason to guard this new test behind
> `PERF_REFFILES`?
>
> Even though the penalty of enumerating 10,000 unrelated loose
> references may be most pronounced in the `files` backend, skipping
> unnecessary reference enumeration is an architectural win for other
> backends (like `reftable` or a fully packed repository) as well.
>
> If we drop `PERF_REFFILES` and retitle the test to "describe exact
> tag with many unrelated refs", we could run it unconditionally to
> benchmark the improvement across all storage formats.
Yeah, there's no good reason - and Patrick made the same observation.
In v2 I will remove the prerequisite and rename the case to refer to
unrelated rather than loose refs.
^ permalink raw reply
* Re: [PATCH 07/16] packfile: use higher-level interface to implement `has_object_pack()`
From: Karthik Nayak @ 2026-06-08 16:07 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260604-pks-odb-source-packed-v1-7-2e7ab31b4b5c@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1705 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> In `has_object_pack()` we're checking whether a specific object exists
> as part of a packfile. This is done by calling the low-level function
> `find_pack_entry()`, but this function will eventually be moved into
> "odb/source-packed.c" and made file-local.
>
> Refactor the code to use `packfile_store_read_object_info()` instead.
> This refactoring is functionally equivalent as that function will call
> `find_pack_entry()` itself and then return immediately when it ain't got
> no object info pointer as parameter.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> packfile.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/packfile.c b/packfile.c
> index 902b7f70f2..3ee71d7f71 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -2132,14 +2132,12 @@ struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed
> int has_object_pack(struct repository *r, const struct object_id *oid)
> {
> struct odb_source *source;
> - struct pack_entry e;
>
> odb_prepare_alternates(r->objects);
> for (source = r->objects->sources; source; source = source->next) {
> struct odb_source_files *files = odb_source_files_downcast(source);
> - int ret = find_pack_entry(files->packed, oid, &e);
> - if (ret)
> - return ret;
> + if (!packfile_store_read_object_info(files->packed, oid, NULL, 0))
> + return 1;
> }
>
I was wondering if there would be an added cost of actually obtaining
the object-info. Seems like there isn't, because we pass `NULL` for the
`struct object_info *oi`, which means it will return before reading the
object info.
> return 0;
>
> --
> 2.54.0.1064.gd145956f57.dirty
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 11/16] odb/source-packed: wire up `count_objects()` callback
From: Karthik Nayak @ 2026-06-08 16:12 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260604-pks-odb-source-packed-v1-11-2e7ab31b4b5c@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
[snip]
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index a61c809c8c..013d8a50f8 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -338,6 +338,39 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
> return ret;
> }
>
> +static int odb_source_packed_count_objects(struct odb_source *source,
> + enum odb_count_objects_flags flags UNUSED,
> + unsigned long *out)
> +{
> + struct odb_source_packed *packed = odb_source_packed_downcast(source);
> + struct packfile_list_entry *e;
> + struct multi_pack_index *m;
> + unsigned long count = 0;
> + int ret;
> +
> + m = get_multi_pack_index(&packed->files->base);
> + if (m)
> + count += m->num_objects + m->num_objects_in_base;
> +
> + for (e = packfile_store_get_packs(packed); e; e = e->next) {
> + if (e->pack->multi_pack_index)
> + continue;
> + if (open_pack_index(e->pack)) {
> + ret = -1;
> + goto out;
> + }
> +
> + count += e->pack->num_objects;
> + }
> +
> + *out = count;
> + ret = 0;
> +
> +out:
> + return ret;
> +}
> +
> +
Nit: extra newline.
> void (*report_garbage)(unsigned seen_bits, const char *path);
>
> static void report_helper(const struct string_list *list,
> @@ -549,6 +582,7 @@ struct odb_source_packed *odb_source_packed_new(struct odb_source_files *parent)
> packed->base.read_object_info = odb_source_packed_read_object_info;
> packed->base.read_object_stream = odb_source_packed_read_object_stream;
> packed->base.for_each_object = odb_source_packed_for_each_object;
> + packed->base.count_objects = odb_source_packed_count_objects;
>
> if (!is_absolute_path(parent->base.path))
> chdir_notify_register(NULL, odb_source_packed_reparent, packed);
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH] worktree: record creation time and free-form note
From: Kiesel, Norbert @ 2026-06-08 16:12 UTC (permalink / raw)
To: git, Junio C Hamano; +Cc: phillip.wood, Chris Torek, kristofferhaugsbakk
In-Reply-To: <CAPx1Gvegc0KvE8zb90n7vLJLKx6EkmBvCWW=NPf+nwiZc+oWdQ@mail.gmail.com>
Hi team,
I updated my proposed extension in a couple of ways you suggested, and
also added some more test code.
Best,
Norbert
diff --git Documentation/git-worktree.adoc Documentation/git-worktree.adoc
index fbf8426cd9..1cdbdc8dbe 100644
--- Documentation/git-worktree.adoc
+++ Documentation/git-worktree.adoc
@@ -10,8 +10,11 @@ SYNOPSIS
--------
[synopsis]
git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]
+ [--description <string>]
[--orphan] [(-b | -B) <new-branch>] <path> [<commit-ish>]
-git worktree list [-v | --porcelain [-z]]
+git worktree describe <worktree> [<description>]
+git worktree list [-v | --porcelain [-z]] [--show-created]
+ [--show-updated] [--show-description] [--sort=<key>]
git worktree lock [--reason <string>] <worktree>
git worktree move <worktree> <new-path>
git worktree prune [-n] [-v] [--expire <expire>]
@@ -106,6 +109,16 @@ passed to the command. In the event the
repository has a remote and
command fails with a warning reminding the user to fetch from their remote
first (or override by using `-f`/`--force`).
+`describe <worktree> [<description>]`::
+
+Set, replace, or clear a free-form description on a linked worktree.
+Useful for recording what a worktree was created for so it can be identified
+later. With _<description>_, the worktree's description is set or replaced;
+without a description argument, the existing description is cleared. The
+description for a worktree may also be set at creation time with
+`git worktree add --description <description>`. The main worktree cannot be
+described.
+
`list`::
List details of each worktree. The main worktree is listed first,
@@ -114,6 +127,28 @@ whether the worktree is bare, the revision
currently checked out, the
branch currently checked out (or "detached HEAD" if none), "locked" if
the worktree is locked, "prunable" if the worktree can be pruned by the
`prune` command.
++
+Each worktree's creation timestamp is recorded when it is created with
+`git worktree add`. Worktrees created before this feature existed have no
+recorded creation timestamp; for them, `list` reports `created: unknown`
+in human output and omits the `created` line in `--porcelain` output. Pass
+`--show-created` to include creation timestamps in human output. Worktrees
+without a recorded timestamp sort last (or first when reversed) with
+`--sort=created`.
++
+Pass `--show-updated` to include each worktree's last-updated timestamp,
+which is the modification time of the worktree's `HEAD` file and so
+reflects checkouts, commits, resets, rebases, and similar Git operations.
++
+Pass `--show-description` to include any user-provided description in human
+output. In `--porcelain` output, the `created`, `updated`, and
+`description` lines are emitted whenever the underlying data is available.
++
+Use `--sort=<key>` (where _<key>_ is `path`, `created`, or `updated`,
+optionally prefixed with `-` to reverse) to order the linked worktrees;
+the main worktree always remains first. Sorting by `created` or `updated`
+implies the matching `--show-created` / `--show-updated` flag so the order
+is visible alongside the data.
`lock`::
@@ -286,6 +321,46 @@ _<time>_.
With `lock` or with `add --lock`, an explanation why the worktree
is locked.
+`--description <string>`::
+ With `add`, attach a free-form description to the new worktree.
+ The description is stored alongside the worktree's administrative
+ files and can be displayed with `git worktree list --show-description`
+ or in `--porcelain` output. It can be changed later with
+ `git worktree describe`.
+
+`--show-created`::
+ With `list`, include each worktree's creation timestamp in the
+ human-readable output. Worktrees with no recorded creation time are
+ shown as `created: unknown`. In `--porcelain` output, the creation
+ timestamp is always included (when available) on a `created` line.
+
+`--show-updated`::
+ With `list`, include each linked worktree's last-updated timestamp in
+ the human-readable output, derived from the modification time of the
+ worktree's `HEAD` file. Linked worktrees whose `HEAD` cannot be read
+ are shown as `updated: unknown`. The main worktree is not annotated
+ with an updated timestamp. In `--porcelain` output, the timestamp is
+ included on an `updated` line whenever it is available (and the
+ worktree is not the main worktree).
+
+`--show-description`::
+ With `list`, include each worktree's description (if set) in the
+ human-readable output. In `--porcelain` output, the description is
+ always included (when set) on a `description` line.
+
+`--sort=<key>`::
+ With `list`, sort linked worktrees by _<key>_, which is one of
+ `path`, `created`, or `updated`. Prefix with `-` to reverse the order,
+ e.g. `--sort=-created` lists newest first. The main worktree is always
+ listed first regardless of sort order. For `created`, worktrees with no
+ recorded creation timestamp sort after those that have one (or before,
+ when reversed). For `updated`, ordering is by the modification time of
+ each worktree's `HEAD` file (a proxy for when the worktree was last
+ touched by checkout, commit, reset or rebase); worktrees whose `HEAD`
+ cannot be read sort after those that can. Sorting by `created` or
+ `updated` implies the matching `--show-created` / `--show-updated`
+ option so the values driving the order appear in human output.
+
_<worktree>_::
Worktrees can be identified by path, either relative or absolute.
+
@@ -462,7 +537,10 @@ are terminated with NUL rather than a newline.
Attributes are listed with a
label and value separated by a single space. Boolean attributes (like `bare`
and `detached`) are listed as a label only, and are present only
if the value is true. Some attributes (like `locked`) can be listed as a label
-only or with a value depending upon whether a reason is available. The first
+only or with a value depending upon whether a reason is available. Optional
+valued attributes (like `created`, `updated`, and `description`) appear
+only when the corresponding metadata has been recorded for that worktree.
+The first
attribute of a worktree is always `worktree`, an empty line indicates the
end of the record. For example:
@@ -474,10 +552,15 @@ bare
worktree /path/to/linked-worktree
HEAD abcd1234abcd1234abcd1234abcd1234abcd1234
branch refs/heads/master
+created 2026-06-01T12:34:56Z
+updated 2026-06-04T17:20:11Z
+description investigating login bug
worktree /path/to/other-linked-worktree
HEAD 1234abc1234abc1234abc1234abc1234abc1234a
detached
+created 2026-05-28T08:15:00Z
+updated 2026-05-30T09:42:08Z
worktree /path/to/linked-worktree-locked-no-reason
HEAD 5678abc5678abc5678abc5678abc5678abc5678c
diff --git builtin/worktree.c builtin/worktree.c
index d21c43fde3..132de668e3 100644
--- builtin/worktree.c
+++ builtin/worktree.c
@@ -27,13 +27,17 @@
#include "utf8.h"
#include "worktree.h"
#include "quote.h"
+#include "date.h"
#define BUILTIN_WORKTREE_ADD_USAGE \
N_("git worktree add [-f] [--detach] [--checkout] [--lock [--reason
<string>]]\n" \
+ " [--description <string>]\n" \
" [--orphan] [(-b | -B) <new-branch>] <path>
[<commit-ish>]")
#define BUILTIN_WORKTREE_LIST_USAGE \
- N_("git worktree list [-v | --porcelain [-z]]")
+ N_("git worktree list [-v | --porcelain [-z]] [--show-created]\n" \
+ " [--show-updated] [--show-description]\n" \
+ " [--sort=<key>]")
#define BUILTIN_WORKTREE_LOCK_USAGE \
N_("git worktree lock [--reason <string>] <worktree>")
#define BUILTIN_WORKTREE_MOVE_USAGE \
@@ -46,6 +50,8 @@
N_("git worktree repair [<path>...]")
#define BUILTIN_WORKTREE_UNLOCK_USAGE \
N_("git worktree unlock <worktree>")
+#define BUILTIN_WORKTREE_DESCRIBE_USAGE \
+ N_("git worktree describe <worktree> [<description>]")
#define WORKTREE_ADD_DWIM_ORPHAN_INFER_TEXT \
_("No possible source branch, inferring '--orphan'")
@@ -66,6 +72,7 @@
static const char * const git_worktree_usage[] = {
BUILTIN_WORKTREE_ADD_USAGE,
+ BUILTIN_WORKTREE_DESCRIBE_USAGE,
BUILTIN_WORKTREE_LIST_USAGE,
BUILTIN_WORKTREE_LOCK_USAGE,
BUILTIN_WORKTREE_MOVE_USAGE,
@@ -116,6 +123,11 @@ static const char * const git_worktree_unlock_usage[] = {
NULL
};
+static const char * const git_worktree_describe_usage[] = {
+ BUILTIN_WORKTREE_DESCRIBE_USAGE,
+ NULL
+};
+
struct add_opts {
int force;
int detach;
@@ -124,6 +136,7 @@ struct add_opts {
int orphan;
int relative_paths;
const char *keep_locked;
+ const char *description;
};
static int show_only;
@@ -131,6 +144,9 @@ static int verbose;
static int guess_remote;
static int use_relative_paths;
static timestamp_t expire;
+static int show_created = -1;
+static int show_updated = -1;
+static int show_description;
static int git_worktree_config(const char *var, const char *value,
const struct config_context *ctx, void *cb)
@@ -544,6 +560,16 @@ static int add_worktree(const char *path, const
char *refname,
strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%s/created", sb_repo.buf);
+ write_file(sb.buf, "%"PRItime, (timestamp_t) time(NULL));
+
+ if (opts->description && *opts->description) {
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%s/description", sb_repo.buf);
+ write_file(sb.buf, "%s", opts->description);
+ }
+
/*
* Set up the ref store of the worktree and create the HEAD reference.
*/
@@ -815,6 +841,8 @@ static int add(int ac, const char **av, const char *prefix,
OPT_BOOL(0, "lock", &keep_locked, N_("keep the new working tree locked")),
OPT_STRING(0, "reason", &lock_reason, N_("string"),
N_("reason for locking")),
+ OPT_STRING(0, "description", &opts.description, N_("string"),
+ N_("attach a free-form description to the worktree")),
OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
OPT_PASSTHRU(0, "track", &opt_track, NULL,
N_("set up tracking mode (see git-branch(1))"),
@@ -963,6 +991,8 @@ static int add(int ac, const char **av, const char *prefix,
static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
{
const char *reason;
+ const char *description;
+ timestamp_t created;
printf("worktree %s%c", wt->path, line_terminator);
if (wt->is_bare)
@@ -975,6 +1005,26 @@ static void show_worktree_porcelain(struct
worktree *wt, int line_terminator)
printf("branch %s%c", wt->head_ref, line_terminator);
}
+ created = worktree_created_at(wt);
+ if (created)
+ printf("created %s%c",
+ show_date(created, 0, DATE_MODE(ISO8601_STRICT)),
+ line_terminator);
+
+ {
+ timestamp_t updated = worktree_updated_at(wt);
+ if (updated)
+ printf("updated %s%c",
+ show_date(updated, 0, DATE_MODE(ISO8601_STRICT)),
+ line_terminator);
+ }
+
+ description = worktree_description(wt);
+ if (description && *description) {
+ fputs("description ", stdout);
+ write_name_quoted(description, stdout, line_terminator);
+ }
+
reason = worktree_lock_reason(wt);
if (reason) {
fputs("locked", stdout);
@@ -1034,6 +1084,32 @@ static void show_worktree(struct worktree *wt,
struct worktree_display *display,
else if (reason)
strbuf_addstr(&sb, " prunable");
+ if (show_created > 0 || verbose) {
+ timestamp_t created = worktree_created_at(wt);
+ struct date_mode mode = { .type = DATE_ISO8601, .local = 1 };
+ if (created)
+ strbuf_addf(&sb, "\n\tcreated: %s",
+ show_date(created, 0, mode));
+ else if (show_created > 0 && !is_main_worktree(wt))
+ strbuf_addstr(&sb, "\n\tcreated: unknown");
+ }
+
+ if (show_updated > 0 || verbose) {
+ timestamp_t updated = worktree_updated_at(wt);
+ struct date_mode mode = { .type = DATE_ISO8601, .local = 1 };
+ if (updated)
+ strbuf_addf(&sb, "\n\tupdated: %s",
+ show_date(updated, 0, mode));
+ else if (show_updated > 0 && !is_main_worktree(wt))
+ strbuf_addstr(&sb, "\n\tupdated: unknown");
+ }
+
+ if (show_description || verbose) {
+ const char *description = worktree_description(wt);
+ if (description && *description)
+ strbuf_addf(&sb, "\n\tdescription: %s", description);
+ }
+
printf("%s\n", sb.buf);
strbuf_release(&sb);
}
@@ -1068,6 +1144,48 @@ static int pathcmp(const void *a_, const void *b_)
return fspathcmp((*a)->path, (*b)->path);
}
+static int createdcmp(const void *a_, const void *b_)
+{
+ struct worktree *const *a = a_;
+ struct worktree *const *b = b_;
+ timestamp_t ta = worktree_created_at(*a);
+ timestamp_t tb = worktree_created_at(*b);
+
+ /* Worktrees without a recorded timestamp (legacy) sort after those
with one. */
+ if (!ta && !tb)
+ return fspathcmp((*a)->path, (*b)->path);
+ if (!ta)
+ return 1;
+ if (!tb)
+ return -1;
+ if (ta < tb)
+ return -1;
+ if (ta > tb)
+ return 1;
+ return 0;
+}
+
+static int updatedcmp(const void *a_, const void *b_)
+{
+ struct worktree *const *a = a_;
+ struct worktree *const *b = b_;
+ timestamp_t ta = worktree_updated_at(*a);
+ timestamp_t tb = worktree_updated_at(*b);
+
+ /* Worktrees whose HEAD mtime can't be read sort after those that can. */
+ if (!ta && !tb)
+ return fspathcmp((*a)->path, (*b)->path);
+ if (!ta)
+ return 1;
+ if (!tb)
+ return -1;
+ if (ta < tb)
+ return -1;
+ if (ta > tb)
+ return 1;
+ return 0;
+}
+
static void pathsort(struct worktree **wt)
{
int n = 0;
@@ -1078,11 +1196,45 @@ static void pathsort(struct worktree **wt)
QSORT(wt, n, pathcmp);
}
+static int sort_worktrees(struct worktree **wt, const char *key)
+{
+ int n = 0, reverse = 0;
+ struct worktree **p = wt;
+ int (*cmp)(const void *, const void *);
+
+ if (*key == '-') {
+ reverse = 1;
+ key++;
+ }
+ if (!strcmp(key, "path"))
+ cmp = pathcmp;
+ else if (!strcmp(key, "created"))
+ cmp = createdcmp;
+ else if (!strcmp(key, "updated"))
+ cmp = updatedcmp;
+ else
+ return -1;
+
+ while (*p++)
+ n++;
+ QSORT(wt, n, cmp);
+ if (reverse) {
+ int i;
+ for (i = 0; i < n / 2; i++) {
+ struct worktree *tmp = wt[i];
+ wt[i] = wt[n - 1 - i];
+ wt[n - 1 - i] = tmp;
+ }
+ }
+ return 0;
+}
+
static int list(int ac, const char **av, const char *prefix,
struct repository *repo UNUSED)
{
int porcelain = 0;
int line_terminator = '\n';
+ const char *sort_key = NULL;
struct option options[] = {
OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
@@ -1091,6 +1243,14 @@ static int list(int ac, const char **av, const
char *prefix,
N_("add 'prunable' annotation to missing worktrees older than <time>")),
OPT_SET_INT('z', NULL, &line_terminator,
N_("terminate records with a NUL character"), '\0'),
+ OPT_BOOL(0, "show-created", &show_created,
+ N_("show worktree creation timestamps")),
+ OPT_BOOL(0, "show-updated", &show_updated,
+ N_("show worktree last-updated timestamps")),
+ OPT_BOOL(0, "show-description", &show_description,
+ N_("show worktree descriptions")),
+ OPT_STRING(0, "sort", &sort_key, N_("key"),
+ N_("sort worktrees by key (path, created, updated); prefix with -
to reverse")),
OPT_END()
};
@@ -1107,8 +1267,27 @@ static int list(int ac, const char **av, const
char *prefix,
int path_maxwidth = 0, abbrev = DEFAULT_ABBREV, i;
struct worktree_display *display = NULL;
- /* sort worktrees by path but keep main worktree at top */
- pathsort(worktrees + 1);
+ /* sort worktrees but keep main worktree at top */
+ if (sort_key) {
+ const char *bare_key = sort_key;
+ if (*bare_key == '-')
+ bare_key++;
+ /*
+ * Sorting by a timestamp without showing it would
+ * leave the user guessing why the order is what it
+ * is, so opt in the matching display by default.
+ * An explicit --show-* / --no-show-* still wins.
+ */
+ if (!strcmp(bare_key, "created") && show_created < 0)
+ show_created = 1;
+ else if (!strcmp(bare_key, "updated") && show_updated < 0)
+ show_updated = 1;
+
+ if (sort_worktrees(worktrees + 1, sort_key))
+ die(_("unknown sort key '%s'"), sort_key);
+ } else {
+ pathsort(worktrees + 1);
+ }
if (!porcelain)
measure_widths(worktrees, &abbrev,
@@ -1200,6 +1379,32 @@ static int unlock_worktree(int ac, const char
**av, const char *prefix,
return ret;
}
+static int describe_worktree(int ac, const char **av, const char *prefix,
+ struct repository *repo UNUSED)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+ struct worktree **worktrees, *wt;
+ int ret;
+
+ ac = parse_options(ac, av, prefix, options, git_worktree_describe_usage, 0);
+ if (ac < 1 || ac > 2)
+ usage_with_options(git_worktree_describe_usage, options);
+
+ worktrees = get_worktrees();
+ wt = find_worktree(worktrees, prefix, av[0]);
+ if (!wt)
+ die(_("'%s' is not a working tree"), av[0]);
+ if (is_main_worktree(wt))
+ die(_("The main working tree cannot be described"));
+
+ ret = set_worktree_description(wt, ac == 2 ? av[1] : NULL);
+
+ free_worktrees(worktrees);
+ return ret;
+}
+
static void validate_no_submodules(const struct worktree *wt)
{
struct index_state istate = INDEX_STATE_INIT(the_repository);
@@ -1469,6 +1674,7 @@ int cmd_worktree(int ac,
parse_opt_subcommand_fn *fn = NULL;
struct option options[] = {
OPT_SUBCOMMAND("add", &fn, add),
+ OPT_SUBCOMMAND("describe", &fn, describe_worktree),
OPT_SUBCOMMAND("prune", &fn, prune),
OPT_SUBCOMMAND("list", &fn, list),
OPT_SUBCOMMAND("lock", &fn, lock_worktree),
diff --git t/meson.build t/meson.build
index 2af8d01279..7b6e8435d7 100644
--- t/meson.build
+++ t/meson.build
@@ -308,6 +308,7 @@ integration_tests = [
't2405-worktree-submodule.sh',
't2406-worktree-repair.sh',
't2407-worktree-heads.sh',
+ 't2410-worktree-metadata.sh',
't2500-untracked-overwriting.sh',
't2501-cwd-empty.sh',
't3000-ls-files-others.sh',
diff --git t/t2402-worktree-list.sh t/t2402-worktree-list.sh
index e0c6abd2f5..fb1f4b1d3c 100755
--- t/t2402-worktree-list.sh
+++ t/t2402-worktree-list.sh
@@ -71,7 +71,8 @@ test_expect_success '"list" all worktrees --porcelain' '
echo "HEAD $(git rev-parse HEAD)" >>expect &&
echo "detached" >>expect &&
echo >>expect &&
- git worktree list --porcelain >actual &&
+ git worktree list --porcelain >actual.raw &&
+ grep -Ev "^(created|updated) " actual.raw >actual &&
test_cmp expect actual
'
@@ -86,7 +87,7 @@ test_expect_success '"list" all worktrees --porcelain -z' '
"$(git -C here rev-parse --show-toplevel)" \
"$(git rev-parse HEAD)" >>expect &&
git worktree list --porcelain -z >_actual &&
- nul_to_q <_actual >actual &&
+ nul_to_q <_actual | tr Q "\n" | grep -Ev "^(created|updated) " | tr
"\n" Q >actual &&
test_cmp expect actual
'
@@ -220,7 +221,7 @@ test_expect_success '"list" all worktrees from bare main' '
'
test_expect_success '"list" all worktrees --porcelain from bare main' '
- test_when_finished "rm -rf there actual expect && git -C bare1
worktree prune" &&
+ test_when_finished "rm -rf there actual actual.raw expect && git -C
bare1 worktree prune" &&
git -C bare1 worktree add --detach ../there main &&
echo "worktree $(pwd)/bare1" >expect &&
echo "bare" >>expect &&
@@ -229,7 +230,8 @@ test_expect_success '"list" all worktrees
--porcelain from bare main' '
echo "HEAD $(git -C there rev-parse HEAD)" >>expect &&
echo "detached" >>expect &&
echo >>expect &&
- git -C bare1 worktree list --porcelain >actual &&
+ git -C bare1 worktree list --porcelain >actual.raw &&
+ grep -Ev "^(created|updated) " actual.raw >actual &&
test_cmp expect actual
'
diff --git t/t2410-worktree-metadata.sh t/t2410-worktree-metadata.sh
new file mode 100755
index 0000000000..e1ecb1c1bf
--- /dev/null
+++ t/t2410-worktree-metadata.sh
@@ -0,0 +1,245 @@
+#!/bin/sh
+
+test_description='git worktree creation timestamp and description metadata'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit init
+'
+
+test_expect_success 'add writes created file' '
+ test_when_finished "git worktree remove -f wt1 && git worktree prune" &&
+ git worktree add wt1 &&
+ test_path_is_file .git/worktrees/wt1/created &&
+ # contents should be a positive integer (unix timestamp)
+ created=$(cat .git/worktrees/wt1/created) &&
+ test "$created" -gt 0
+'
+
+test_expect_success 'add --description writes description file' '
+ test_when_finished "git worktree remove -f wt2 && git worktree prune" &&
+ git worktree add --description "investigating bug" wt2 &&
+ test_path_is_file .git/worktrees/wt2/description &&
+ echo "investigating bug" >expect &&
+ test_cmp expect .git/worktrees/wt2/description
+'
+
+test_expect_success 'add without --description does not create
description file' '
+ test_when_finished "git worktree remove -f wt3 && git worktree prune" &&
+ git worktree add wt3 &&
+ test_path_is_missing .git/worktrees/wt3/description
+'
+
+test_expect_success 'describe sets a description on an existing worktree' '
+ test_when_finished "git worktree remove -f wt4 && git worktree prune" &&
+ git worktree add wt4 &&
+ git worktree describe wt4 "later description" &&
+ echo "later description" >expect &&
+ test_cmp expect .git/worktrees/wt4/description
+'
+
+test_expect_success 'describe replaces an existing description' '
+ test_when_finished "git worktree remove -f wt5 && git worktree prune" &&
+ git worktree add --description "old" wt5 &&
+ git worktree describe wt5 "new" &&
+ echo "new" >expect &&
+ test_cmp expect .git/worktrees/wt5/description
+'
+
+test_expect_success 'describe with no text clears the description' '
+ test_when_finished "git worktree remove -f wt6 && git worktree prune" &&
+ git worktree add --description "to delete" wt6 &&
+ test_path_is_file .git/worktrees/wt6/description &&
+ git worktree describe wt6 &&
+ test_path_is_missing .git/worktrees/wt6/description
+'
+
+test_expect_success 'describe refuses to operate on the main worktree' '
+ test_must_fail git worktree describe . "should fail" 2>err &&
+ grep -i "main working tree" err
+'
+
+test_expect_success 'list --show-description displays description in
human output' '
+ test_when_finished "git worktree remove -f wt7 && git worktree prune" &&
+ git worktree add --description "release branch" wt7 &&
+ git worktree list --show-description >actual &&
+ grep "description: release branch" actual
+'
+
+test_expect_success 'list --show-created displays created timestamp' '
+ test_when_finished "git worktree remove -f wt8 && git worktree prune" &&
+ git worktree add wt8 &&
+ git worktree list --show-created >actual &&
+ grep "created: " actual
+'
+
+test_expect_success 'list --show-created shows unknown for legacy worktrees' '
+ test_when_finished "git worktree remove -f wt9 && git worktree prune" &&
+ git worktree add wt9 &&
+ rm .git/worktrees/wt9/created &&
+ git worktree list --show-created >actual &&
+ grep "created: unknown" actual
+'
+
+test_expect_success 'list --show-updated displays updated timestamp' '
+ test_when_finished "git worktree remove -f wt8u && git worktree prune" &&
+ git worktree add wt8u &&
+ git worktree list --show-updated >actual &&
+ grep "updated: " actual
+'
+
+test_expect_success 'list --porcelain always includes created,
updated, and description' '
+ test_when_finished "git worktree remove -f wtp && git worktree prune" &&
+ git worktree add --description "porcelain test" wtp &&
+ git worktree list --porcelain >actual &&
+ grep "^created " actual &&
+ grep "^updated " actual &&
+ grep "^description porcelain test" actual
+'
+
+test_expect_success 'list --sort=created orders by creation time' '
+ test_when_finished "git worktree remove -f a && git worktree remove
-f b && git worktree remove -f c && git worktree prune" &&
+ git worktree add a &&
+ git worktree add b &&
+ git worktree add c &&
+ echo 1000 >.git/worktrees/a/created &&
+ echo 2000 >.git/worktrees/b/created &&
+ echo 3000 >.git/worktrees/c/created &&
+ git worktree list --sort=created --porcelain >actual &&
+ grep "^worktree " actual | sed -n "2,4p" >linked &&
+ awk "NR==1" linked | grep -q "/a$" &&
+ awk "NR==2" linked | grep -q "/b$" &&
+ awk "NR==3" linked | grep -q "/c$"
+'
+
+test_expect_success 'list --sort=-created reverses order' '
+ test_when_finished "git worktree remove -f a && git worktree remove
-f b && git worktree remove -f c && git worktree prune" &&
+ git worktree add a &&
+ git worktree add b &&
+ git worktree add c &&
+ echo 1000 >.git/worktrees/a/created &&
+ echo 2000 >.git/worktrees/b/created &&
+ echo 3000 >.git/worktrees/c/created &&
+ git worktree list --sort=-created --porcelain >actual &&
+ grep "^worktree " actual | sed -n "2,4p" >linked &&
+ awk "NR==1" linked | grep -q "/c$" &&
+ awk "NR==2" linked | grep -q "/b$" &&
+ awk "NR==3" linked | grep -q "/a$"
+'
+
+test_expect_success 'list --sort=created places legacy worktrees last' '
+ test_when_finished "git worktree remove -f early && git worktree
remove -f legacy && git worktree prune" &&
+ git worktree add early &&
+ echo 1000 >.git/worktrees/early/created &&
+ git worktree add legacy &&
+ rm .git/worktrees/legacy/created &&
+ git worktree list --sort=created --porcelain >actual &&
+ grep "^worktree " actual | sed -n "2,3p" >linked &&
+ awk "NR==1" linked | grep -q "/early$" &&
+ awk "NR==2" linked | grep -q "/legacy$"
+'
+
+test_expect_success 'list --sort=updated orders by HEAD mtime' '
+ test_when_finished "git worktree remove -f u1 && git worktree remove
-f u2 && git worktree remove -f u3 && git worktree prune" &&
+ git worktree add u1 &&
+ git worktree add u2 &&
+ git worktree add u3 &&
+ # Force a known ordering: u2 oldest, u1 middle, u3 newest.
+ test-tool chmtime =1000 .git/worktrees/u2/HEAD &&
+ test-tool chmtime =2000 .git/worktrees/u1/HEAD &&
+ test-tool chmtime =3000 .git/worktrees/u3/HEAD &&
+ git worktree list --sort=updated --porcelain >actual &&
+ grep "^worktree " actual | sed -n "2,4p" >linked &&
+ awk "NR==1" linked | grep -q "/u2$" &&
+ awk "NR==2" linked | grep -q "/u1$" &&
+ awk "NR==3" linked | grep -q "/u3$"
+'
+
+test_expect_success 'list --sort=-updated reverses order' '
+ test_when_finished "git worktree remove -f u1 && git worktree remove
-f u2 && git worktree remove -f u3 && git worktree prune" &&
+ git worktree add u1 &&
+ git worktree add u2 &&
+ git worktree add u3 &&
+ test-tool chmtime =1000 .git/worktrees/u2/HEAD &&
+ test-tool chmtime =2000 .git/worktrees/u1/HEAD &&
+ test-tool chmtime =3000 .git/worktrees/u3/HEAD &&
+ git worktree list --sort=-updated --porcelain >actual &&
+ grep "^worktree " actual | sed -n "2,4p" >linked &&
+ awk "NR==1" linked | grep -q "/u3$" &&
+ awk "NR==2" linked | grep -q "/u1$" &&
+ awk "NR==3" linked | grep -q "/u2$"
+'
+
+test_expect_success 'list --sort=created auto-shows created timestamp' '
+ test_when_finished "git worktree remove -f autoc && git worktree prune" &&
+ git worktree add autoc &&
+ git worktree list --sort=created >actual &&
+ grep "created: " actual
+'
+
+test_expect_success 'list --sort=-created auto-shows created timestamp' '
+ test_when_finished "git worktree remove -f autocr && git worktree prune" &&
+ git worktree add autocr &&
+ git worktree list --sort=-created >actual &&
+ grep "created: " actual
+'
+
+test_expect_success 'list --sort=updated auto-shows updated timestamp' '
+ test_when_finished "git worktree remove -f autou && git worktree prune" &&
+ git worktree add autou &&
+ git worktree list --sort=updated >actual &&
+ grep "updated: " actual
+'
+
+test_expect_success 'list --sort=-updated auto-shows updated timestamp' '
+ test_when_finished "git worktree remove -f autour && git worktree prune" &&
+ git worktree add autour &&
+ git worktree list --sort=-updated >actual &&
+ grep "updated: " actual
+'
+
+test_expect_success 'list --sort=path does not auto-show timestamps' '
+ test_when_finished "git worktree remove -f autop && git worktree prune" &&
+ git worktree add autop &&
+ git worktree list --sort=path >actual &&
+ ! grep "created: " actual &&
+ ! grep "updated: " actual
+'
+
+test_expect_success 'list --sort with unknown key fails' '
+ test_must_fail git worktree list --sort=bogus 2>err &&
+ grep -i "unknown sort key" err
+'
+
+test_expect_success 'list --sort=updated --no-show-updated suppresses
auto-show' '
+ test_when_finished "git worktree remove -f noshowu && git worktree prune" &&
+ git worktree add noshowu &&
+ git worktree list --sort=updated --no-show-updated >actual &&
+ ! grep "updated: " actual
+'
+
+test_expect_success 'list --sort=created --no-show-created suppresses
auto-show' '
+ test_when_finished "git worktree remove -f noshowc && git worktree prune" &&
+ git worktree add noshowc &&
+ git worktree list --sort=created --no-show-created >actual &&
+ ! grep "created: " actual
+'
+
+test_expect_success 'list --show-updated formats human output in
local timezone' '
+ test_when_finished "git worktree remove -f tz && git worktree prune" &&
+ git worktree add tz &&
+ # Pin HEAD mtime to a fixed unix time outside any DST transition
+ # so the rendered offset is deterministic in PST8PDT (-0700 in July).
+ test-tool chmtime =1500000000 .git/worktrees/tz/HEAD &&
+ TZ=PST8PDT git worktree list --show-updated >human &&
+ grep "updated: 2017-07-13 19:40:00 -0700" human &&
+ # Porcelain stays in UTC ISO-8601 strict form regardless of TZ.
+ TZ=PST8PDT git worktree list --porcelain >porcelain &&
+ grep "^updated 2017-07-14T02:40:00Z$" porcelain
+'
+
+test_done
diff --git worktree.c worktree.c
index 97eddc3916..4b019a532b 100644
--- worktree.c
+++ worktree.c
@@ -14,6 +14,8 @@
#include "dir.h"
#include "wt-status.h"
#include "config.h"
+#include "date.h"
+#include "wrapper.h"
void free_worktree(struct worktree *worktree)
{
@@ -24,6 +26,7 @@ void free_worktree(struct worktree *worktree)
free(worktree->head_ref);
free(worktree->lock_reason);
free(worktree->prune_reason);
+ free(worktree->description);
free(worktree);
}
@@ -324,6 +327,100 @@ const char *worktree_lock_reason(struct worktree *wt)
return wt->lock_reason;
}
+timestamp_t worktree_created_at(struct worktree *wt)
+{
+ if (is_main_worktree(wt))
+ return 0;
+
+ if (!wt->created_at_valid) {
+ struct strbuf path = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addstr(&path, worktree_git_path(wt, "created"));
+ if (file_exists(path.buf) &&
+ strbuf_read_file(&buf, path.buf, 0) >= 0) {
+ char *end;
+ timestamp_t t;
+ strbuf_trim(&buf);
+ t = parse_timestamp(buf.buf, &end, 10);
+ if (end != buf.buf && *end == '\0')
+ wt->created_at = t;
+ }
+ wt->created_at_valid = 1;
+ strbuf_release(&path);
+ strbuf_release(&buf);
+ }
+
+ return wt->created_at;
+}
+
+timestamp_t worktree_updated_at(struct worktree *wt)
+{
+ struct stat st;
+ char *git_dir;
+ char *head_path;
+ timestamp_t result = 0;
+
+ if (is_main_worktree(wt))
+ return 0;
+
+ git_dir = get_worktree_git_dir(wt);
+ head_path = xstrfmt("%s/HEAD", git_dir);
+ if (!stat(head_path, &st))
+ result = (timestamp_t) st.st_mtime;
+ free(head_path);
+ free(git_dir);
+ return result;
+}
+
+const char *worktree_description(struct worktree *wt)
+{
+ if (is_main_worktree(wt))
+ return NULL;
+
+ if (!wt->description_valid) {
+ struct strbuf path = STRBUF_INIT;
+
+ strbuf_addstr(&path, worktree_git_path(wt, "description"));
+ if (file_exists(path.buf)) {
+ struct strbuf description = STRBUF_INIT;
+ if (strbuf_read_file(&description, path.buf, 0) < 0)
+ die_errno(_("failed to read '%s'"), path.buf);
+ strbuf_trim_trailing_newline(&description);
+ wt->description = strbuf_detach(&description, NULL);
+ } else
+ wt->description = NULL;
+ wt->description_valid = 1;
+ strbuf_release(&path);
+ }
+
+ return wt->description;
+}
+
+int set_worktree_description(struct worktree *wt, const char *text)
+{
+ char *path;
+ int ret = 0;
+
+ if (is_main_worktree(wt))
+ return error(_("cannot set description on the main worktree"));
+
+ path = repo_common_path(wt->repo, "worktrees/%s/description", wt->id);
+ if (!text || !*text) {
+ if (file_exists(path) && unlink(path))
+ ret = error_errno(_("failed to remove '%s'"), path);
+ } else {
+ write_file(path, "%s", text);
+ }
+
+ /* invalidate cache so a follow-up worktree_description() re-reads */
+ FREE_AND_NULL(wt->description);
+ wt->description_valid = 0;
+
+ free(path);
+ return ret;
+}
+
const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
{
struct strbuf reason = STRBUF_INIT;
diff --git worktree.h worktree.h
index 1075409f9a..2568830237 100644
--- worktree.h
+++ worktree.h
@@ -13,12 +13,16 @@ struct worktree {
char *head_ref; /* NULL if HEAD is broken or detached */
char *lock_reason; /* private - use worktree_lock_reason */
char *prune_reason; /* private - use worktree_prune_reason */
+ char *description; /* private - use worktree_description */
struct object_id head_oid;
+ timestamp_t created_at; /* private - use worktree_created_at; 0 if unknown */
int is_detached;
int is_bare;
int is_current; /* does `path` match `repo->worktree` */
int lock_reason_valid; /* private */
int prune_reason_valid; /* private */
+ int description_valid; /* private */
+ int created_at_valid; /* private */
};
/*
@@ -96,6 +100,34 @@ int is_main_worktree(const struct worktree *wt);
*/
const char *worktree_lock_reason(struct worktree *wt);
+/*
+ * Return the worktree's recorded creation timestamp, or 0 if no timestamp
+ * was recorded (e.g. a worktree created before this metadata existed, or
+ * the main worktree which never carries the file).
+ */
+timestamp_t worktree_created_at(struct worktree *wt);
+
+/*
+ * Return the modification time of the worktree's HEAD file as an
+ * approximation of "when was this worktree last touched by Git" (checkout,
+ * commit, reset, rebase, etc.). Returns 0 for the main worktree, and 0 if
+ * HEAD cannot be stat'd.
+ */
+timestamp_t worktree_updated_at(struct worktree *wt);
+
+/*
+ * Return the user-supplied description for the given worktree, or NULL
+ * if none was set.
+ */
+const char *worktree_description(struct worktree *wt);
+
+/*
+ * Write or replace the worktree's description. Pass NULL or "" to delete
+ * the description. Returns 0 on success, -1 on failure. Not valid for the
+ * main worktree.
+ */
+int set_worktree_description(struct worktree *wt, const char *text);
+
/*
* Return the reason string if the given worktree should be pruned, otherwise
* NULL if it should not be pruned. `expire` defines a grace period to prune
On Fri, Jun 5, 2026 at 9:57 AM Chris Torek <chris.torek@gmail.com> wrote:
>
> On Fri, Jun 5, 2026 at 8:31 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > Isn't "what is the worktree for" a property of the branch that's checked
> > out, not the worktree itself?
>
> I don't think it is.
>
> A lot of things within Git have, shall way say, "less than optimal"
> names, with "branch" (with at least three different meanings),
> "HEAD", and "index" being examples of this. (This is just an
> observation, not a complaint: we know from studies that
> oddities in names don't matter that much after a bit of usage
> of some system. They're just minor stumbling blocks when
> getting started.)
>
> Work-tree or working tree is not one of them, though. It's
> concise and pointed: a working tree is where you do work.
>
> As such, the *purpose* of a working tree is exactly as general
> as the purpose of doing work! That's a wide-open set.
>
> Git's internal constraint, of requiring each working tree that
> is using a branch name to have a unique-to-that-tree branch
> name, is a property specific to branch names, not to branching
> in general (an example of the ambiguity of "branch" here).
> And of course, as you note, any working tree can be on
> a detached HEAD.
>
> Exactly what properties any given working tree should
> have, and the weird entanglement Git has between the
> "primary" working tree (the one created by any non-bare
> clone) and all "secondary" working trees, is a mere (ahem)
> matter of implementation. Descriptions, creation times,
> modification times, etc., are all potentially useful.
>
> I think, had Git initially made all repositories effectively
> bare, with separate working trees added later, this might
> all be a little clearer, but of course that ship sailed,
> crossed *all* the oceans, sank, was refloated and refitted,
> and sailed for another decade already. :-)
>
> Chris
--
Norbert Kiesel | Staff Software Engineer | Credit Karma
norbert.kiesel@creditkarma.com | www.creditkarma.com
This email may contain confidential and privileged information. Any
review, use, distribution, or disclosure by anyone other than the
intended recipient(s) is prohibited. If you are not the intended
recipient, please contact the sender by reply email and delete all
copies of this message.
^ permalink raw reply related
* Re: [PATCH 00/16] odb: make packed object source a proper `struct odb_source`
From: Karthik Nayak @ 2026-06-08 16:15 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260604-pks-odb-source-packed-v1-0-2e7ab31b4b5c@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> Hi,
>
> this patch series converts the "packed" source into a proper `struct
> odb_source`. It's thus the equivalent to [1], which did the same thing
> for the "loose" source.
>
> This series here is unfortunately a bit bigger, mostly because I'm also
> renaming `struct packfile_store` to `struct odb_source_packed`. Back
> when I introduced the packfile store I didn't yet have the full vision
> of how the final layout will look like, so I didn't have the foresight
> yet to call it `struct odb_source_packed`. But now that the layout has
> materialized I think it's sensible to adjust its naming to match all the
> other sources that we have.
>
> Also: I don't have anything else in the pipeline anymore that moves
> around large pieces of our code in the vicinity of the object database.
> So after this series got merged, subsequent changes should be of a more
> incremental nature.
>
> This series is built on top of 9ac3f193c0 (The 11th batch, 2026-06-02)
> with ps/odb-source-loose at ef4778bcba (odb/source-loose: drop pointer
> to the "files" source, 2026-06-01) merged into it.
This was a good read. The commits towards the end are mostly simple code
movements. Overall the series looks to be in good shape.
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH RFC 1/2] builtin/history: abort reword on unchanged message
From: Ben Knoble @ 2026-06-08 16:37 UTC (permalink / raw)
To: Pablo Sabater; +Cc: git, Patrick Steinhardt, Kaartic Sivaraam, Pablo Sabater
In-Reply-To: <20260607-ps-history-reword-v1-1-ba43a3cbb81b@gmail.com>
I don’t have any strong opinions on the rest…
> Le 7 juin 2026 à 16:08, Pablo Sabater <pabloosabaterr@gmail.com> a écrit :
>
> When using `git history reword` if the new message is the same as the
> original it continues anyway creating a new commit with the same
> message and updates its descendants, modifying the history after this
> 'reworded' commit even though there was no actual change.
>
> `git commit --amend` and `git rebase -i` + reword share this behavior,
> however `git history reword` is different:
> 1. Works in-memory without touching the index or the worktree [1], so
> there are no side effects like staged files that could justify
> rewriting the history when the commit message is the same.
> 2. `git history` by default updates all the branches [2] that contain the
> original commit making it more costly than `git rebase -i` that only
> updates the current branch.
>
> Add a check if the original commit message is the same as the new one
> and abort if so.
>
> [1]: https://lore.kernel.org/git/20260113-b4-pks-history-builtin-v11-8-e74ebfa2652d@pks.im/
> [2]: https://git-scm.com/docs/git-history#_description
>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
> builtin/history.c | 10 ++++++++++
> t/t3451-history-reword.sh | 20 ++++++++++++++++++++
> 2 files changed, 30 insertions(+)
>
> diff --git a/builtin/history.c b/builtin/history.c
> index 0fc06fb204..51a22a9a1c 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> @@ -135,6 +135,13 @@ static int commit_tree_ext(struct repository *repo,
> original_body, action, &commit_message);
> if (ret < 0)
> goto out;
> +
> + if (!strcmp(original_body, commit_message.buf)) {
> + fprintf(stderr, _("Message unchanged,"
> + " aborting reword.\n"));
> + ret = 1;
> + goto out;
> + }
> } else {
> strbuf_addstr(&commit_message, original_body);
> }
> @@ -718,6 +725,9 @@ static int cmd_history_reword(int argc,
> if (ret < 0) {
> ret = error(_("failed writing reworded commit"));
> goto out;
> + } else if (ret == 1) {
> + ret = 0;
> + goto out;
> }
>
> strbuf_addf(&reflog_msg, "reword: updating %s", argv[0]);
> diff --git a/t/t3451-history-reword.sh b/t/t3451-history-reword.sh
> index de7b357685..54ea8a7207 100755
> --- a/t/t3451-history-reword.sh
> +++ b/t/t3451-history-reword.sh
> @@ -396,4 +396,24 @@ test_expect_success 'retains changes in the worktree and index' '
> )
> '
>
> +test_expect_success 'aborts if the commit message is the same' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> + test_commit first &&
> + test_commit second &&
> +
> + git rev-parse HEAD >oid-before &&
> + write_script fake-editor.sh <<-\EOF &&
> + true
> + EOF
> + test_set_editor "$(pwd)"/fake-editor.sh &&
> + git history reword HEAD 2>err &&
> + git rev-parse HEAD >oid-after &&
> + test_cmp oid-before oid-after &&
> + test_grep "Message unchanged" err
> + )
…but I think this test case could do something like "GIT_EDITOR=true git history reword HEAD" and avoid the script?
> +'
> +
> test_done
>
> --
> 2.54.0
Best,
Ben
^ permalink raw reply
* Re: [PATCH RFC 1/2] builtin/history: abort reword on unchanged message
From: Ben Knoble @ 2026-06-08 16:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pablo Sabater, git, Patrick Steinhardt, Kaartic Sivaraam
In-Reply-To: <xmqqmrx5z0po.fsf@gitster.g>
> Le 8 juin 2026 à 08:23, Junio C Hamano <gitster@pobox.com> a écrit :
>
[snip]
> Having said that, I personally think that the current behaviour of
> `commit --amend` and `history reword` are both _wrong_ [*2*].
>
> You may start `git commit --amend`, and after staring at the
> existing commit log message for some time in your editor, it is
> quite natural for you to decide that leaving the commit as-is is the
> right thing [*3*] in your situation. It may have been a better
> design for the system to notice this situation and leave the commit
> as-is, with an override option `--force` to allow users to forcibly
> update the committer ident and timestamp in the commit header. I am
> not a `history reword` user (yet), but from the motivation you
> described for this patch, I sense that the story is the same there.
FWIW, in this situation I abort my editor (:cquit in Vim) so that the amend gets an error-valued exit code from the subprocess and aborts itself.
Perhaps there could/should be a better side-channel for communicating that, though? I do not know how easy it is to tell other editors to « quit with errors ».
> [Footnote]
>
> *1* Besides, doesn't "--update-refs" in "rebase -i" allow you to
> adjust the branches?
>
> *2* But it is an established behaviour people _rely_ on, so even
> though it may have been better if these commands behaved
> differently, it probably is a bit too late to change it now.
>
> *3* This includes the case where the original author is especially
> difficult to work with and would complain any change to their
> commits, even if the only change you made for them is a
> typofix. Fixing a small typo/grammo may not be worth your time
> and unpleasant exchanges with them after touching their commit.
^ permalink raw reply
* Re: [PATCH RFC 2/2] builtin/history: print feedback after successful reword
From: Ben Knoble @ 2026-06-08 16:47 UTC (permalink / raw)
To: Pablo Sabater; +Cc: Junio C Hamano, git, Patrick Steinhardt, Kaartic Sivaraam
In-Reply-To: <CAN5EUNQNj86Q+hi6PouOZNWo1T4QTQ6sE5Hs9USZXWpkTedTcw@mail.gmail.com>
> Le 8 juin 2026 à 09:29, Pablo Sabater <pabloosabaterr@gmail.com> a écrit :
>
> El lun, 8 jun 2026 a las 14:16, Junio C Hamano (<gitster@pobox.com>) escribió:
>>
>> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>>
>>> Unlike `git commit --amend` and `git rebase -i`, `git history reword`
>>> doesn't print anything, this makes it feel empty for a porcelain command
>>> and hard to tell if the command did anything without using other
>>> commands like `git log <commit>` to check if the reword was done.
>>>
>>> Print a message on successful rewords so the user has feedback about it.
>>>
>>> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
>>> ---
>>> builtin/history.c | 4 ++++
>>> t/t3451-history-reword.sh | 14 ++++++++++++++
>>> 2 files changed, 18 insertions(+)
>>>
>>> diff --git a/builtin/history.c b/builtin/history.c
>>> index 51a22a9a1c..0f1ba3b531 100644
>>> --- a/builtin/history.c
>>> +++ b/builtin/history.c
>>> @@ -739,6 +739,10 @@ static int cmd_history_reword(int argc,
>>> goto out;
>>> }
>>>
>>> + fprintf(stderr, _("Successfully reworded commit %s to %s\n"),
>>> + repo_find_unique_abbrev(repo, &original->object.oid, DEFAULT_ABBREV),
>>> + repo_find_unique_abbrev(repo, &rewritten->object.oid, DEFAULT_ABBREV));
>>> +
>>> ret = 0;
>>>
>>> out:
>>
>> Do other commands in "git history" (split is in 'master', drop and
>> fixup are cooking) behave with similar verbosity? Consistency within
>> the same "history" umbrella matters more than being similar with
>> other commands that can be used for similar purposes.
>
> They do not, they are thought with the rule of silence in mind.
> However I think that this output is valuable information I might have
> explained myself better at [1] but my thought is:
>
> git history reword aabb
>
> Now that I have my commit aabb rewritten I want to check it again just
> to make sure I did what I wanted correctly,
Some thoughts:
- If the rewritten commit is an ancestor of HEAD, look at the log of HEAD@{1} or the log between HEAD and the aforementioned reflog entry. (git-range-diff may also be helpful there.)
- Similarly, if the rewritten commit is reachable from some ref R, check R@{1} etc.
> but git log aabb is still
> the old commit, the rewritten one has a different hash which I do not
> know unless I search for it, if it's far from HEAD I'd have to git log
> --oneline, get the hash and then git log new_hash. I think that git
> history reword that does have the information about the new hash
> should print it to avoid this search.
> What I want is something like:
>
> git history reword aabb
> Successfully reworded aabb to ccdd
>
> So I can just git log ccdd without having to search.
>
> I want to say I haven't looked as much as I'd like to split, drop and
> fixup, but I think it would be a good addition for them also. On [1]
> Patrick wrote about a --verbose for git history, I think that the
> basic information i.e. at reword which is the new hash should be
> always printed but if it's preferred it could go there.
>
> For split it can print the hashes of the new commits like:
> "...split into ccdd and eeff."
> For fixup the commit hash also changes, so the same as reword.
> The one that will have more friction would be drop is the one that
> doesn't end up with new commits.
>
> [1]: https://lore.kernel.org/git/CAN5EUNSAOMRvmLGVfzQiwWoOn9VGNVU5rVMZizOryn_q2fbCNA@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH] worktree: record creation time and free-form note
From: Junio C Hamano @ 2026-06-08 16:59 UTC (permalink / raw)
To: Kiesel, Norbert; +Cc: git
In-Reply-To: <CAPGaHktHLPUeSuhETwyBo+jE2fMu40jHW284PN+2oY1YJ2j0Yw@mail.gmail.com>
"Kiesel, Norbert" <norbert.kiesel@creditkarma.com> writes:
> I looked at the usage of `.git/description` and I could not find any
> usage.
GitWeb shows it.
^ permalink raw reply
* Re: [PATCH 2/2] builtin/add: use die_for_required_opt() helper
From: Junio C Hamano @ 2026-06-08 17:00 UTC (permalink / raw)
To: Siddharth Shrimali; +Cc: git, christian.couder, toon, jn.avila
In-Reply-To: <20260603111044.39116-3-r.siddharth.shrimali@gmail.com>
Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:
> - if (!show_only && ignore_missing)
> - die(_("the option '%s' requires '%s'"), "--ignore-missing", "--dry-run");
> + die_for_required_opt(ignore_missing, "--ignore-missing", show_only, "--dry-run");
As builtin_add_options[] knows that ignore_missing (variable) comes
from the use of "--ignore-missing" (option), and similarly the value
of show_only (variable) is tightly linked to "--dry-run" (option),
it feels quite wasteful having to pass both.
I wonder if we can do this more declaratively, perhaps by
introducing extra types of elements in struct option[] that tells
"--ignore-missing" requires "--dry-run", so that the client code
does not have to do anything more than calling parse_options() to
implement this?
A possible counter-argument may be that the value of, say,
ignore_missing may be different at this point in the code from what
was set by parse_options() when the command line was processed, but
then it means that the message (with or without your patch) is
misleading, so I am not sure if that counter-argument is valid.
> if (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||
> chmod_arg[1] != 'x' || chmod_arg[2]))
> @@ -462,6 +461,8 @@ int cmd_add(int argc,
> PATHSPEC_SYMLINK_LEADING_PATH,
> prefix, argv);
>
> + die_for_required_opt(pathspec_file_nul, "--pathspec-file-nul",
> + !!pathspec_from_file, "--pathspec-from-file");
> if (pathspec_from_file) {
> if (pathspec.nr)
> die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
> @@ -470,8 +471,6 @@ int cmd_add(int argc,
> PATHSPEC_PREFER_FULL |
> PATHSPEC_SYMLINK_LEADING_PATH,
> prefix, pathspec_from_file, pathspec_file_nul);
> - } else if (pathspec_file_nul) {
> - die(_("the option '%s' requires '%s'"), "--pathspec-file-nul", "--pathspec-from-file");
> }
>
> if (require_pathspec && pathspec.nr == 0) {
^ permalink raw reply
* Re: [PATCH v3 4/6] diff: add long-running diff process via diff.<driver>.process
From: Junio C Hamano @ 2026-06-08 17:19 UTC (permalink / raw)
To: Michael Montalbo
Cc: Johannes Schindelin, Michael Montalbo via GitGitGadget, git
In-Reply-To: <CAC2QwmKNA6wv-jG07fgJj7Xj2J+dzzWEiqV5Q+8HJpjA_GtkFw@mail.gmail.com>
Michael Montalbo <mmontalbo@gmail.com> writes:
>> So the conscious project direction has been: fold pkt-line test backends
>> into `test-tool` and drop the scripting-language prereq. Reintroducing the
>> same shape in Python would walk this back.
>>
>> Patrick's careful effort in 27bd8ee311719 (Merge branch 'ps/fewer-perl',
>> 2025-04-29) has been another clear sign that the Git project is actively
>> _removing_ scripting-language dependencies from the build and test
>> infrastructure, not adding new ones.
>
> Now I wonder if the extension / addition of more Perl test infra with my other
> series:
>
> https://lore.kernel.org/git/pull.2135.git.1780559158.gitgitgadget@gmail.com/T/
>
> also goes against the project direction w.r.t. removing scripting languages.
> Maybe I should also re-evaluate the usage of Perl there. I am leveraging
> existing shell parsing logic written in Perl, but maybe the preference for
> Perl-based lint rules is a mistake and should be avoided.
That sounds prudent (even though it is outside the scope of _this_
topic, of course).
Thanks.
^ permalink raw reply
* Re: [GSoC PATCH v2 1/4] path: introduce format_path() for centralized path formatting
From: Justin Tobler @ 2026-06-08 17:28 UTC (permalink / raw)
To: K Jayatheerth
Cc: git, a3205153416, gitster, kumarayushjha123, lucasseikioshiro,
phillip.wood, sandals
In-Reply-To: <20260605163012.181089-2-jayatheerthkulkarni2005@gmail.com>
On 26/06/05 10:00PM, K Jayatheerth wrote:
> The path-formatting logic inside `builtin/rev-parse.c` handles absolute,
> canonical, and relative formatting rules based on user-supplied options.
> However, this logic is tightly coupled to `rev-parse` and writes directly
> to stdout.
>
> To allow other builtins (such as the upcoming `git repo` path keys) to
> re-use this logic, extract the core path-formatting algorithm into a centralized
> helper function, `format_path()`, in `path.c`.
Makes sense.
> Expose a single, streamlined `path_format` enum in `path.h` to let callers
> explicitly declare their formatting strategy (UNMODIFIED, RELATIVE,
> RELATIVE_IF_SHARED, or CANONICAL). This decouples the core algorithm from
> the localized fallback mechanics specific to `rev-parse`.
Ok, so rev-parse has its own logic to select the formatting strategy
used when printing paths that either relies on what the user provides or
a designated fallback format that is specific to the type of path. Since
that is specific to rev-parse, it makes to factor it out of the generic
helper function here.
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> Mentored-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
> path.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> path.h | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 88 insertions(+)
>
> diff --git a/path.c b/path.c
> index d7e17bf174..2fcd24c5eb 100644
> --- a/path.c
> +++ b/path.c
> @@ -1579,6 +1579,64 @@ char *xdg_cache_home(const char *filename)
> return NULL;
> }
>
> +void format_path(struct strbuf *buf, const char *path,
> + const char *prefix, enum path_format format)
> +{
> + if (format == PATH_FORMAT_UNMODIFIED) {
> + strbuf_addstr(buf, path);
> + return;
> + }
> +
> + if (format == PATH_FORMAT_RELATIVE) {
nit: we could just continue the "else if" chain here instead of
restarting it.
> + struct strbuf relative_buf = STRBUF_INIT;
> + struct strbuf real_path = STRBUF_INIT;
> + struct strbuf real_prefix = STRBUF_INIT;
> + char *cwd = NULL;
> +
> + /*
> + * We don't ever produce a relative path if prefix is NULL,
> + * so set the prefix to the current directory so that we can
> + * produce a relative path whenever possible.
> + */
> + if (!prefix)
> + prefix = cwd = xgetcwd();
> +
> + if (!is_absolute_path(path)) {
> + strbuf_realpath_forgiving(&real_path, path, 1);
> + path = real_path.buf;
> + }
> + if (!is_absolute_path(prefix)) {
> + strbuf_realpath_forgiving(&real_prefix, prefix, 1);
> + prefix = real_prefix.buf;
> + }
> +
> + strbuf_addstr(buf, relative_path(path, prefix, &relative_buf));
> +
> + strbuf_release(&relative_buf);
> + strbuf_release(&real_path);
> + strbuf_release(&real_prefix);
> + free(cwd);
> + } else if (format == PATH_FORMAT_RELATIVE_IF_SHARED) {
> + struct strbuf relative_buf = STRBUF_INIT;
> +
> + /*
> + * If we're using RELATIVE_IF_SHARED mode, then we want an
> + * absolute path unless the two share a common prefix, so don't
> + * default the prefix to the current working directory. Doing so
> + * would cause a relative path to always be produced if possible.
> + */
> + strbuf_addstr(buf, relative_path(path, prefix, &relative_buf));
> + strbuf_release(&relative_buf);
> + } else if (format == PATH_FORMAT_CANONICAL) {
> + struct strbuf canonical_buf = STRBUF_INIT;
> +
> + strbuf_realpath_forgiving(&canonical_buf, path, 1);
> + strbuf_addbuf(buf, &canonical_buf);
Do we need `canonical_buf` here? Can we just add the path to `buf`
directly?
> +
> + strbuf_release(&canonical_buf);
> + }
> +}
> +
> REPO_GIT_PATH_FUNC(squash_msg, "SQUASH_MSG")
> REPO_GIT_PATH_FUNC(merge_msg, "MERGE_MSG")
> REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR")
> diff --git a/path.h b/path.h
> index 0434ba5e07..a78e0fc141 100644
> --- a/path.h
> +++ b/path.h
> @@ -262,6 +262,36 @@ enum scld_error safe_create_leading_directories_no_share(char *path);
> int safe_create_file_with_leading_directories(struct repository *repo,
> const char *path);
>
> +/**
> + * The formatting strategy to apply when writing a path into a buffer.
> + */
> +enum path_format {
> + /* Output the path exactly as-is without any modifications. */
> + PATH_FORMAT_UNMODIFIED,
> +
> + /* Output a path relative to the provided directory prefix. */
> + PATH_FORMAT_RELATIVE,
> +
> + /* Output a relative path only if the path shares a root with the prefix. */
> + PATH_FORMAT_RELATIVE_IF_SHARED,
> +
> + /* Output a fully resolved, absolute canonical path. */
> + PATH_FORMAT_CANONICAL
> +};
> +
> +/**
> + * Format a path according to the specified formatting strategy and append
> + * the result to the given strbuf.
> + *
> + * `buf` : The string buffer to append the formatted path to.
> + * `path` : The path string that needs to be formatted.
> + * `prefix` : The directory prefix to calculate relative offsets against.
> + * Pass NULL to default to the current working directory where applicable.
> + * `format` : The formatting behavior rule to execute.
> + */
> +void format_path(struct strbuf *buf, const char *path,
> + const char *prefix, enum path_format format);
> +
Ok so in this patch we are just adding the new path formatting
interface and will integrate it in the next one. Overall the direction
of this patch looks good to me.
-Justin
^ permalink raw reply
* Re: [GSoC PATCH v2 2/4] rev-parse: use format_path for path formatting
From: Justin Tobler @ 2026-06-08 17:54 UTC (permalink / raw)
To: K Jayatheerth
Cc: git, a3205153416, gitster, kumarayushjha123, lucasseikioshiro,
phillip.wood, sandals
In-Reply-To: <20260605163012.181089-3-jayatheerthkulkarni2005@gmail.com>
On 26/06/05 10:00PM, K Jayatheerth wrote:
> Now that the core path-formatting logic has been abstracted into
> format_path() inside path.c, remove the localized duplicate formatting
> mechanics from builtin/rev-parse.c.
>
> Drop the usage of the old local format_type and default_type enums,
> and update print_path() to act as a light wrapper around the new shared
> engine. Resolve user-provided formatting flags directly within rev-parse
> to pass the final determined path_format to format_path().
So if the format isn't explicitly set by the user via the
`--path-format` option, the default formatting strategy used depends on
the path being printed. IOW, there is no consistent default path format
here.
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> Mentored-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
> builtin/rev-parse.c | 103 ++++++++++----------------------------------
> 1 file changed, 23 insertions(+), 80 deletions(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 218b5f34d6..c78bdc04c1 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -632,73 +632,16 @@ static void handle_ref_opt(const char *pattern, const char *prefix)
> clear_ref_exclusions(&ref_excludes);
> }
>
> -enum format_type {
> - /* We would like a relative path. */
> - FORMAT_RELATIVE,
> - /* We would like a canonical absolute path. */
> - FORMAT_CANONICAL,
> - /* We would like the default behavior. */
> - FORMAT_DEFAULT,
> -};
> -
> -enum default_type {
> - /* Our default is a relative path. */
> - DEFAULT_RELATIVE,
> - /* Our default is a relative path if there's a shared root. */
> - DEFAULT_RELATIVE_IF_SHARED,
> - /* Our default is a canonical absolute path. */
> - DEFAULT_CANONICAL,
> - /* Our default is not to modify the item. */
> - DEFAULT_UNMODIFIED,
> -};
> -
> -static void print_path(const char *path, const char *prefix, enum format_type format, enum default_type def)
> +static void print_path(const char *path, const char *prefix,
> + int arg_path_format, enum path_format def_format)
> {
[snip]
> + struct strbuf sb = STRBUF_INIT;
> + enum path_format fmt = (arg_path_format != -1) ? arg_path_format : def_format;
hmmm, so `arg_path_format` specifies what the user-provided format and
acts as a sentinel to signal there is no value provided and the fallback
format needs to be used. This feels a tad bit awkward to me.
I wonder if we should introduce a PATH_FORMAT_DEFAULT to the
`path_format` enum that maps to one of the existing enum values in
`path.c:format_path()`. Here in `print_path()`, we could then intercept
a PATH_FORMAT_DEFAULT value and override it to the specified
`def_format`. I'm not sure if this is ultimately that much better
though.
-Justin
^ permalink raw reply
* Re: inconsistent order of --diff-algorithm variants in man pages
From: Junio C Hamano @ 2026-06-08 17:56 UTC (permalink / raw)
To: Vincent Lefevre; +Cc: git
In-Reply-To: <20260608112656.GI1082778@cventin.lip.ens-lyon.fr>
Vincent Lefevre <vincent@vinc17.net> writes:
> In Documentation/diff-algorithm-option.adoc, which is used by the
> git-blame(1) and git-diff(1) man pages:
>
> `--diff-algorithm=(patience|minimal|histogram|myers)`::
> Choose a diff algorithm. The variants are as follows:
> +
> --
> `default`;;
> `myers`;;
> The basic greedy diff algorithm. Currently, this is the default.
> `minimal`;;
> Spend extra time to make sure the smallest possible diff is
> produced.
> `patience`;;
> Use "patience diff" algorithm when generating patches.
> `histogram`;;
> This algorithm extends the patience algorithm to "support
> low-occurrence common elements".
> --
>
> I think that using the same order in the --diff-algorithm line and
> in the description that follows would be better, i.e.
>
> --diff-algorithm=(myers|minimal|patience|histogram)
>
> FYI, the text was added in 07924d4d50e5304fb53eb60aaba8aef31d4c4e5e
> in 2013, but without any explanation on this difference.
I think this is meant to list them as equals without any precedence
or preference order, so it is understandable that nobody paid much
attention. Until now, that is.
I agree that being consistent between these two places makes tons of
sense. I just do not know what the right ordering should be. When
listing a set of equals without any precedence or preference order,
the most easy to see to new readers is alphabetical, except that the
built-in default (myers) is a head above among other equals, so it
does make sense to present it first.
^ permalink raw reply
* [PATCH 0/3] Teach git-replay(1) to linearize merge commits
From: Toon Claes @ 2026-06-08 18:37 UTC (permalink / raw)
To: git; +Cc: Toon Claes, Johannes Schindelin
As an alternative to dscho's patch series to replay merges[1], add
option to git-replay(1) to linearize merges. This mimics wath
git-rebase(1) does too with --no-rebase-merges (the default).
The first two patches do some refactoring. The third patch implements
the actual change. I was kindly helped by dscho to implement this
change.
The --linearize option is only added to git-replay(1) and not to
git-history(1) because in my opinion doesn't make much sense to do so,
but I'm happy to hear if anyone disagrees.
This series might conflict with Kristoffer's series to make
documentation changes[2], but should be trivial to resolve. And I don't
think there's a conflict with Patrick's series on adding "drop" to
git-history(1)[3].
dscho's series to replay merges[1] need a bit of rework to fit on top of
this, but I'm happy to help figuring that out.
[1]: <pull.2106.git.1778107405.gitgitgadget@gmail.com>
[2]: <V2_CV_doc_replay_config.767@msgid.xyz>
[3]: <20260603-b4-pks-history-drop-v2-0-742cb5b5176d@pks.im>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
Johannes Schindelin (1):
replay: offer an option to linearize the commit topology
Toon Claes (2):
replay: refactor enum replay_mode into a bool
replay: add helper to put entry into mapped_commits
Documentation/git-replay.adoc | 5 ++
builtin/replay.c | 4 ++
replay.c | 109 +++++++++++++++++++++++-------------------
replay.h | 5 ++
t/t3650-replay-basics.sh | 22 +++++++++
5 files changed, 97 insertions(+), 48 deletions(-)
---
base-commit: 9ac3f193c05c2237e2b14ebaa1149e9fc8a1abe0
change-id: 20260604-toon-git-replay-drop-merges-807fa008d395
^ permalink raw reply
* [PATCH 1/3] replay: refactor enum replay_mode into a bool
From: Toon Claes @ 2026-06-08 18:37 UTC (permalink / raw)
To: git; +Cc: Toon Claes
In-Reply-To: <20260608-toon-git-replay-drop-merges-v1-0-e3ee71fce7b4@iotcl.com>
In 2760ee4983 (replay: add --revert mode to reverse commit changes,
2026-03-26) the enum `replay_mode` was introduced. This has two possible
values:
- The value `REPLAY_MODE_REVERT` is used when option `--revert` is
passed to git-replay(1). When using this value the commits are
possible in reverse order and the inverse of the changes are applied.
- The value `REPLAY_MODE_PICK` is used when either option `--onto` or
`--advance` is used. In both cases the commits are pocessed in normal
order, and the changes are applied as-is.
Since there are only two possible values of this enum, simplify the code
by converting the enum into a bool. This avoid adding code paths that
check for invalid vaues of the enum, and shortens code where the value
is checked with a ternary operator.
Signed-off-by: Toon Claes <toon@iotcl.com>
---
replay.c | 59 +++++++++++++++++++++++++----------------------------------
1 file changed, 25 insertions(+), 34 deletions(-)
diff --git a/replay.c b/replay.c
index 4ef8abb607..1f8e5b083b 100644
--- a/replay.c
+++ b/replay.c
@@ -18,11 +18,6 @@
*/
#define the_repository DO_NOT_USE_THE_REPOSITORY
-enum replay_mode {
- REPLAY_MODE_PICK,
- REPLAY_MODE_REVERT,
-};
-
static const char *short_commit_name(struct repository *repo,
struct commit *commit)
{
@@ -81,7 +76,7 @@ static struct commit *create_commit(struct repository *repo,
struct tree *tree,
struct commit *based_on,
struct commit *parent,
- enum replay_mode mode)
+ bool reverse)
{
struct object_id ret;
struct object *obj = NULL;
@@ -98,15 +93,13 @@ static struct commit *create_commit(struct repository *repo,
commit_list_insert(parent, &parents);
extra = read_commit_extra_headers(based_on, exclude_gpgsig);
- if (mode == REPLAY_MODE_REVERT) {
+ if (reverse) {
generate_revert_message(&msg, based_on, repo);
/* For revert, use current user as author (NULL = use default) */
- } else if (mode == REPLAY_MODE_PICK) {
+ } else {
find_commit_subject(message, &orig_message);
strbuf_addstr(&msg, orig_message);
author = get_author(message);
- } else {
- BUG("unexpected replay mode %d", mode);
}
reset_ident_date();
if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
@@ -269,7 +262,7 @@ static struct commit *pick_regular_commit(struct repository *repo,
struct commit *onto,
struct merge_options *merge_opt,
struct merge_result *result,
- enum replay_mode mode,
+ bool reverse,
enum replay_empty_commit_action empty)
{
struct commit *base, *replayed_base;
@@ -287,7 +280,21 @@ static struct commit *pick_regular_commit(struct repository *repo,
replayed_base_tree = repo_get_commit_tree(repo, replayed_base);
pickme_tree = repo_get_commit_tree(repo, pickme);
- if (mode == REPLAY_MODE_PICK) {
+ if (reverse) {
+ /* Revert: swap base and pickme to reverse the diff */
+ const char *pickme_name = short_commit_name(repo, pickme);
+ merge_opt->branch1 = short_commit_name(repo, replayed_base);
+ merge_opt->branch2 = xstrfmt("parent of %s", pickme_name);
+ merge_opt->ancestor = pickme_name;
+
+ merge_incore_nonrecursive(merge_opt,
+ pickme_tree,
+ replayed_base_tree,
+ base_tree,
+ result);
+
+ free((char *)merge_opt->branch2);
+ } else {
/* Cherry-pick: normal order */
merge_opt->branch1 = short_commit_name(repo, replayed_base);
merge_opt->branch2 = short_commit_name(repo, pickme);
@@ -303,22 +310,6 @@ static struct commit *pick_regular_commit(struct repository *repo,
result);
free((char *)merge_opt->ancestor);
- } else if (mode == REPLAY_MODE_REVERT) {
- /* Revert: swap base and pickme to reverse the diff */
- const char *pickme_name = short_commit_name(repo, pickme);
- merge_opt->branch1 = short_commit_name(repo, replayed_base);
- merge_opt->branch2 = xstrfmt("parent of %s", pickme_name);
- merge_opt->ancestor = pickme_name;
-
- merge_incore_nonrecursive(merge_opt,
- pickme_tree,
- replayed_base_tree,
- base_tree,
- result);
-
- free((char *)merge_opt->branch2);
- } else {
- BUG("unexpected replay mode %d", mode);
}
merge_opt->ancestor = NULL;
merge_opt->branch2 = NULL;
@@ -341,7 +332,7 @@ static struct commit *pick_regular_commit(struct repository *repo,
}
}
- return create_commit(repo, result->tree, pickme, replayed_base, mode);
+ return create_commit(repo, result->tree, pickme, replayed_base, reverse);
}
void replay_result_release(struct replay_result *result)
@@ -381,13 +372,13 @@ int replay_revisions(struct rev_info *revs,
char *revert;
const char *ref;
struct object_id old_oid;
- enum replay_mode mode = REPLAY_MODE_PICK;
+ bool reverse;
int ret;
advance = xstrdup_or_null(opts->advance);
revert = xstrdup_or_null(opts->revert);
- if (revert)
- mode = REPLAY_MODE_REVERT;
+ reverse = !!revert;
+
set_up_replay_mode(revs->repo, &revs->cmdline, opts->onto,
&detached_head, &advance, &revert, &onto, &update_refs);
@@ -430,8 +421,8 @@ int replay_revisions(struct rev_info *revs,
die(_("replaying merge commits is not supported yet!"));
last_commit = pick_regular_commit(revs->repo, commit, replayed_commits,
- mode == REPLAY_MODE_REVERT ? last_commit : onto,
- &merge_opt, &result, mode, opts->empty);
+ reverse ? last_commit : onto,
+ &merge_opt, &result, reverse, opts->empty);
if (!last_commit)
break;
--
2.53.0.1323.g189a785ab5
^ permalink raw reply related
* [PATCH 2/3] replay: add helper to put entry into mapped_commits
From: Toon Claes @ 2026-06-08 18:37 UTC (permalink / raw)
To: git; +Cc: Toon Claes
In-Reply-To: <20260608-toon-git-replay-drop-merges-v1-0-e3ee71fce7b4@iotcl.com>
The function replay_revisions() in replay.c is rather lengthy. Extract
the logic to put commit entry into mapped_commits into a helper
function.
Signed-off-by: Toon Claes <toon@iotcl.com>
---
replay.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/replay.c b/replay.c
index 1f8e5b083b..7921d7dba3 100644
--- a/replay.c
+++ b/replay.c
@@ -243,9 +243,9 @@ static void set_up_replay_mode(struct repository *repo,
strset_clear(&rinfo.positive_refs);
}
-static struct commit *mapped_commit(kh_oid_map_t *replayed_commits,
- struct commit *commit,
- struct commit *fallback)
+static struct commit *get_mapped_commit(kh_oid_map_t *replayed_commits,
+ struct commit *commit,
+ struct commit *fallback)
{
khint_t pos;
if (!commit)
@@ -256,6 +256,21 @@ static struct commit *mapped_commit(kh_oid_map_t *replayed_commits,
return kh_value(replayed_commits, pos);
}
+static void put_mapped_commit(kh_oid_map_t *replayed_commits,
+ struct commit *commit,
+ struct commit *new_commit)
+{
+ khint_t pos;
+ int ret;
+
+ pos = kh_put_oid_map(replayed_commits, commit->object.oid, &ret);
+ if (ret == 0)
+ BUG("Duplicate rewritten commit: %s\n",
+ oid_to_hex(&commit->object.oid));
+
+ kh_value(replayed_commits, pos) = new_commit;
+}
+
static struct commit *pick_regular_commit(struct repository *repo,
struct commit *pickme,
kh_oid_map_t *replayed_commits,
@@ -276,7 +291,7 @@ static struct commit *pick_regular_commit(struct repository *repo,
base_tree = lookup_tree(repo, repo->hash_algo->empty_tree);
}
- replayed_base = mapped_commit(replayed_commits, base, onto);
+ replayed_base = get_mapped_commit(replayed_commits, base, onto);
replayed_base_tree = repo_get_commit_tree(repo, replayed_base);
pickme_tree = repo_get_commit_tree(repo, pickme);
@@ -414,8 +429,6 @@ int replay_revisions(struct rev_info *revs,
replayed_commits = kh_init_oid_map();
while ((commit = get_revision(revs))) {
const struct name_decoration *decoration;
- khint_t pos;
- int hr;
if (commit->parents && commit->parents->next)
die(_("replaying merge commits is not supported yet!"));
@@ -427,11 +440,7 @@ int replay_revisions(struct rev_info *revs,
break;
/* Record commit -> last_commit mapping */
- pos = kh_put_oid_map(replayed_commits, commit->object.oid, &hr);
- if (hr == 0)
- BUG("Duplicate rewritten commit: %s\n",
- oid_to_hex(&commit->object.oid));
- kh_value(replayed_commits, pos) = last_commit;
+ put_mapped_commit(replayed_commits, commit, last_commit);
/* Update any necessary branches */
if (ref)
--
2.53.0.1323.g189a785ab5
^ permalink raw reply related
* [PATCH 3/3] replay: offer an option to linearize the commit topology
From: Toon Claes @ 2026-06-08 18:37 UTC (permalink / raw)
To: git; +Cc: Toon Claes, Johannes Schindelin
In-Reply-To: <20260608-toon-git-replay-drop-merges-v1-0-e3ee71fce7b4@iotcl.com>
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
One of the stated goals of git-replay(1) is to allow implementing the
git-rebase(1) functionality on the server side.
The default mode of git-rebase(1) is to act as if `--no-rebase-merges`
was given. This mode drops merge commits instead of replaying them, and
linearized the commit history into a sequence of the
regular (single-parent) commits.
Add option `--linearize` to git-replay(1) do the same.
Co-authored-by: Toon Claes <toon@iotcl.com>
---
Documentation/git-replay.adoc | 5 +++++
builtin/replay.c | 4 ++++
replay.c | 25 +++++++++++++++++++------
replay.h | 5 +++++
t/t3650-replay-basics.sh | 22 ++++++++++++++++++++++
5 files changed, 55 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
index a32f72aead..41c96c7061 100644
--- a/Documentation/git-replay.adoc
+++ b/Documentation/git-replay.adoc
@@ -88,6 +88,11 @@ incompatible with `--contained` (which is a modifier for `--onto` only).
+
The default mode can be configured via the `replay.refAction` configuration variable.
+--linearize::
+ In this mode, `git replay` imitates `git rebase --no-rebase-merges`,
+ i.e. it cherry-picks only non-merge commits, each one on top of the
+ previous one.
+
<revision-range>::
Range of commits to replay; see "Specifying Ranges" in
linkgit:git-rev-parse[1]. In `--advance=<branch>` or
diff --git a/builtin/replay.c b/builtin/replay.c
index 39e3a86f6c..fedfe46dc6 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -111,6 +111,8 @@ int cmd_replay(int argc,
N_("mode"),
N_("control ref update behavior (update|print)"),
PARSE_OPT_NONEG),
+ OPT_BOOL(0, "linearize", &opts.linearize,
+ N_("ignore merge commits instead of replaying them")),
OPT_END()
};
@@ -132,6 +134,8 @@ int cmd_replay(int argc,
opts.contained, "--contained");
die_for_incompatible_opt2(!!opts.ref, "--ref",
!!opts.contained, "--contained");
+ die_for_incompatible_opt2(!!opts.revert, "--revert",
+ opts.linearize, "--linearize");
/* Parse ref action mode from command line or config */
ref_mode = get_ref_action_mode(repo, ref_action);
diff --git a/replay.c b/replay.c
index 7921d7dba3..3e36908131 100644
--- a/replay.c
+++ b/replay.c
@@ -277,12 +277,16 @@ static struct commit *pick_regular_commit(struct repository *repo,
struct commit *onto,
struct merge_options *merge_opt,
struct merge_result *result,
+ struct commit *replayed_base,
bool reverse,
enum replay_empty_commit_action empty)
{
- struct commit *base, *replayed_base;
+ struct commit *base;
struct tree *pickme_tree, *base_tree, *replayed_base_tree;
+ if (replayed_base && reverse)
+ BUG("Linearizing commits is not supported when replaying in reverse");
+
if (pickme->parents) {
base = pickme->parents->item;
base_tree = repo_get_commit_tree(repo, base);
@@ -291,7 +295,8 @@ static struct commit *pick_regular_commit(struct repository *repo,
base_tree = lookup_tree(repo, repo->hash_algo->empty_tree);
}
- replayed_base = get_mapped_commit(replayed_commits, base, onto);
+ if (!replayed_base)
+ replayed_base = get_mapped_commit(replayed_commits, base, onto);
replayed_base_tree = repo_get_commit_tree(repo, replayed_base);
pickme_tree = repo_get_commit_tree(repo, pickme);
@@ -430,12 +435,20 @@ int replay_revisions(struct rev_info *revs,
while ((commit = get_revision(revs))) {
const struct name_decoration *decoration;
- if (commit->parents && commit->parents->next)
+ if (opts->linearize && (!commit->parents || commit->parents->next))
+ ; /* map current commit to the same as the previous commit */
+ else if (commit->parents && commit->parents->next)
die(_("replaying merge commits is not supported yet!"));
+ else {
+ struct commit *to_pick = reverse ? last_commit : onto;
+ last_commit =
+ pick_regular_commit(revs->repo, commit,
+ replayed_commits, to_pick,
+ &merge_opt, &result,
+ opts->linearize ? last_commit : NULL,
+ reverse, opts->empty);
+ }
- last_commit = pick_regular_commit(revs->repo, commit, replayed_commits,
- reverse ? last_commit : onto,
- &merge_opt, &result, reverse, opts->empty);
if (!last_commit)
break;
diff --git a/replay.h b/replay.h
index 1851a07705..07e6fdcca3 100644
--- a/replay.h
+++ b/replay.h
@@ -62,6 +62,11 @@ struct replay_revisions_options {
* Defaults to REPLAY_EMPTY_COMMIT_DROP.
*/
enum replay_empty_commit_action empty;
+
+ /*
+ * Whether to linearize the commits (i.e. drop merge commits).
+ */
+ int linearize;
};
/* This struct is used as an out-parameter by `replay_revisions()`. */
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 3353bc4a4d..c781a3bb1b 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -565,4 +565,26 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '
test_grep "cannot be used with multiple revision ranges" err
'
+test_expect_success 'linearize the commit topology' '
+ test_tick &&
+ N=$(git commit-tree -m N -p L -p I L:) &&
+ N=$(git commit-tree -m N-child -p $N L:) &&
+ git update-ref refs/heads/N $N &&
+
+ git replay --ref-action=print --linearize \
+ --onto A B..refs/heads/N >out &&
+
+ test_line_count = 1 out &&
+ read N1 N2 N3 N4 <out &&
+
+ cat >expect <<-EOF &&
+ * N-child
+ * I
+ * L
+ o A
+ EOF
+ git log --format=%s --graph --boundary A...$N3 >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.53.0.1323.g189a785ab5
^ permalink raw reply related
* Re: [GSoC PATCH v2 3/4] repo: add path.gitdir with absolute and relative suffix formatting
From: Justin Tobler @ 2026-06-08 18:50 UTC (permalink / raw)
To: K Jayatheerth
Cc: git, a3205153416, gitster, kumarayushjha123, lucasseikioshiro,
phillip.wood, sandals
In-Reply-To: <20260605163012.181089-4-jayatheerthkulkarni2005@gmail.com>
On 26/06/05 10:00PM, K Jayatheerth wrote:
> Scripts often need to locate the `.git` directory. While `git rev-parse`
> provides this, it relies on command-line flags to dictate path formatting.
>
> Introduce `path.gitdir.absolute` and `path.gitdir.relative` keys to
> `git repo info`. Exposing separate format-specific keys instead of a base
> `path.gitdir` key avoids default fallbacks and requires callers to state
> their format requirements explicitly. Both keys use `format_path()` to
> resolve paths.
Makes sense.
> To test these keys, introduce the `test_repo_info_path` helper in
> `t/t1900-repo-info.sh`. The helper evaluates paths dynamically and accepts
> environment variable prefixes. This prepares the test suite for future path
> keys that depend on environment overrides, such as `commondir`.
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> Mentored-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
> Documentation/git-repo.adoc | 6 ++++++
> builtin/repo.c | 26 ++++++++++++++++++++++++++
> t/t1900-repo-info.sh | 33 +++++++++++++++++++++++++++++++++
> 3 files changed, 65 insertions(+)
>
> diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> index 42262c1983..a0dca7ce88 100644
> --- a/Documentation/git-repo.adoc
> +++ b/Documentation/git-repo.adoc
> @@ -104,6 +104,12 @@ values that they return:
> `object.format`::
> The object format (hash algorithm) used in the repository.
>
> +`path.gitdir.absolute`::
> + The canonical absolute path to the Git repository directory (the `.git` directory).
> +
> +`path.gitdir.relative`::
> + The path to the Git repository directory relative to the current working directory.
> +
> `references.format`::
> The reference storage format. The valid values are:
> +
> diff --git a/builtin/repo.c b/builtin/repo.c
> index 71a5c1c29c..6e97f6a0e4 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -7,12 +7,14 @@
> #include "hex.h"
> #include "odb.h"
> #include "parse-options.h"
> +#include "path.h"
> #include "path-walk.h"
> #include "progress.h"
> #include "quote.h"
> #include "ref-filter.h"
> #include "refs.h"
> #include "revision.h"
> +#include "setup.h"
> #include "strbuf.h"
> #include "string-list.h"
> #include "shallow.h"
> @@ -75,6 +77,28 @@ static int get_object_format(struct repository *repo, struct strbuf *buf)
> return 0;
> }
>
> +static int get_path_gitdir_absolute(struct repository *repo, struct strbuf *buf)
> +{
> + const char *git_dir = repo_get_git_dir(repo);
> +
> + if (!git_dir)
> + return error(_("unable to get git directory"));
> +
> + format_path(buf, git_dir, startup_info->prefix, PATH_FORMAT_CANONICAL);
For absolute paths, I don't think we actually need the prefix, but
providing it doesn't probably matter too much either way.
> + return 0;
> +}
> +
> +static int get_path_gitdir_relative(struct repository *repo, struct strbuf *buf)
> +{
> + const char *git_dir = repo_get_git_dir(repo);
> +
> + if (!git_dir)
> + return error(_("unable to get git directory"));
> +
> + format_path(buf, git_dir, startup_info->prefix, PATH_FORMAT_RELATIVE);
> + return 0;
> +}
Looks good.
> +
> static int get_references_format(struct repository *repo, struct strbuf *buf)
> {
> strbuf_addstr(buf,
> @@ -87,6 +111,8 @@ static const struct repo_info_field repo_info_field[] = {
> { "layout.bare", get_layout_bare },
> { "layout.shallow", get_layout_shallow },
> { "object.format", get_object_format },
> + { "path.gitdir.absolute", get_path_gitdir_absolute },
> + { "path.gitdir.relative", get_path_gitdir_relative },
> { "references.format", get_references_format },
> };
>
> diff --git a/t/t1900-repo-info.sh b/t/t1900-repo-info.sh
> index 39bb77dda0..0660b00bbc 100755
> --- a/t/t1900-repo-info.sh
> +++ b/t/t1900-repo-info.sh
> @@ -155,4 +155,37 @@ test_expect_success 'git repo info -h shows only repo info usage' '
> test_grep ! "git repo structure" actual
> '
>
> +test_repo_info_path () {
> + field_name=$1
> + expect_absolute_eval=$2
> + expect_relative=$3
> + env_prefix=$4
nit: I was a bit uncertain regarding the purpose of env_prefix here.
Since the env_prefix is not used by any tests yet, I wonder if it we
should delay adding it until the next patch. If we want to reduce churn
though, I think we could also swap the order of patch 3 and 4.
> +
> + test_expect_success "query individual key: path.$field_name.absolute${env_prefix:+ ($env_prefix)}" '
> + (
> + cd test-repo/sub &&
> + expect_absolute=$(eval "$expect_absolute_eval") &&
Can we just compute `expect_absolute` prior to passing it instead of
using eval here?
> + echo "path.$field_name.absolute=$expect_absolute" >expect &&
> + eval "${env_prefix:+$env_prefix }git repo info \"path.$field_name.absolute\"" >actual &&
> + test_cmp expect actual
> + )
> + '
> +
> + test_expect_success "query individual key: path.$field_name.relative${env_prefix:+ ($env_prefix)}" '
> + (
> + cd test-repo/sub &&
> + echo "path.$field_name.relative=$expect_relative" >expect &&
> + eval "${env_prefix:+$env_prefix }git repo info \"path.$field_name.relative\"" >actual &&
> + test_cmp expect actual
> + )
> + '
> +}
> +
> +test_expect_success 'setup test repository layout for path fields' '
> + git init test-repo &&
> + mkdir -p test-repo/sub
> +'
> +
> +test_repo_info_path 'gitdir' 'echo "$(cd .. && pwd)/.git"' '../.git'
hmmm, do we expect the path suffix to be the same between relative and
absolute paths for all test cases? If so, we could just have a single
`expect_path_suffix` argument and let the helper compute the appropriate
absolute and relative paths internally.
-Justin
^ permalink raw reply
* [PATCH v4 0/2] prio-queue: fold lazy_queue into prio_queue for automatic get+put fusion
From: Kristofer Karlsson via GitGitGadget @ 2026-06-08 19:10 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Kristofer Karlsson, Kristofer Karlsson
In-Reply-To: <pull.2140.v3.git.1780832592.gitgitgadget@gmail.com>
Rene's lazy_queue wrapper in describe.c was a clever optimization -- by
deferring the get, a following put becomes a simple replace, avoiding a full
remove-rebalance-insert cycle.
It turns out this pattern is so common in git's traversal code that it makes
sense to fold it into prio_queue itself. Gets and puts are interleaved in
virtually every commit walk, so the fusion is essentially always a win.
This is mostly a code simplification -- three callers had independently
reimplemented the same optimization, and they all collapse to plain get+put
now. The 1.7-2.7% speedup on traversal-heavy workloads is a nice bonus.
More details and benchmark numbers in the commit message.
Related to but independent of the cascade sift-down work in
kk/prio-queue-cascade-sift -- the two can land in either order.
Changes in v4:
* Thanks Junio for review, applied all suggestions.
* Renamed .nr_internal to .nr_
* Restored flush_get() as a static inline helper instead of inlining the
flush logic into get() and peek().
* Guard empty-queue check with nr_ <= get_pending.
* Flipped commit order: the rename/accessor commit is now first, and the
behavioral fusion change is second. This was partly messy -- the first
rename commit introduces some ugly intermediate code (e.g. describe.c's
prio_queue_for_each with a skip variable) that gets cleaned up in commit
2 when the lazy get makes it unnecessary.
Changes in v3:
* Adopted Rene's suggestion to move the flush logic below the LIFO
early-return (LIFO mode never sets get_pending, so flushing there is a
no-op).
* Went a step further and inlined the flush logic directly into get() and
peek(), eliminating the flush_get() helper and its forward declaration of
sift_down_root().
* Updated benchmark numbers with more rigorous methodology: 30 interleaved
runs with paired t-test on an idle server. Split results into code paths
that already had manual fusion (neutral) vs code paths that benefit from
the new automatic fusion (1.7-2.7% improvement).
Changes in v2:
* Added a second commit that renames .nr to .nr_internal so that direct
access from outside prio-queue.c is a compile error. Verified that after
the rename, only prio-queue.c references nr_internal.
* Added prio_queue_for_each() macro for callers that need to walk all
elements (describe.c, show-branch.c, commit-reach.c, revision.c,
negotiator/skipping.c).
* Converted remaining .nr loop conditions to use
prio_queue_get()/prio_queue_peek() as the loop condition, or
prio_queue_size() where get/peek isn't suitable.
* Fixed several callers missed in v1 (object-name.c, fetch-pack.c,
path-walk.c, pack-bitmap-write.c, negotiator/default.c,
negotiator/skipping.c, revision.c, builtin/last-modified.c).
Kristofer Karlsson (2):
prio-queue: rename .nr to .nr_ and add accessor helpers
prio-queue: fold lazy_queue into prio_queue for automatic get+put
fusion
builtin/describe.c | 70 ++++++-----------------
builtin/last-modified.c | 7 +--
builtin/show-branch.c | 24 +++-----
commit-reach.c | 14 ++---
commit.c | 11 +---
fetch-pack.c | 4 +-
negotiator/default.c | 4 +-
negotiator/skipping.c | 12 ++--
object-name.c | 2 +-
pack-bitmap-write.c | 10 ++--
path-walk.c | 8 +--
prio-queue.c | 110 +++++++++++++++++++-----------------
prio-queue.h | 19 ++++---
revision.c | 16 +++---
t/unit-tests/u-prio-queue.c | 6 +-
walker.c | 4 +-
16 files changed, 141 insertions(+), 180 deletions(-)
base-commit: 9ac3f193c05c2237e2b14ebaa1149e9fc8a1abe0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2140%2Fspkrka%2Flazy-prio-queue-pr-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2140/spkrka/lazy-prio-queue-pr-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/2140
Range-diff vs v3:
2: 033215e304 ! 1: d0f2294661 prio-queue: rename .nr to .nr_internal to prevent direct access
@@ Metadata
Author: Kristofer Karlsson <krka@spotify.com>
## Commit message ##
- prio-queue: rename .nr to .nr_internal to prevent direct access
+ prio-queue: rename .nr to .nr_ and add accessor helpers
- Rename the .nr member to .nr_internal so that callers outside
- prio-queue.c that directly reference .nr get a compilation error.
- This catches both existing misuse and future in-flight topics.
+ Rename the .nr member to .nr_ so that callers outside prio-queue.c
+ that directly reference .nr get a compilation error. This catches
+ both existing misuse and future in-flight topics.
- Add prio_queue_for_each() macro for callers that need to walk all
- elements in the queue, accounting for the get_pending offset.
+ Add prio_queue_size() for callers that need to know the element count
+ and prio_queue_for_each() for callers that need to walk all elements.
Convert all external .nr users:
- Loop conditions: use prio_queue_size(), prio_queue_get(), or
@@ Commit message
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
## builtin/describe.c ##
-@@ builtin/describe.c: static unsigned long finish_depth_computation(struct prio_queue *queue,
- struct oidset unflagged = OIDSET_INIT;
- struct commit *c;
+@@ builtin/describe.c: static void lazy_queue_put(struct lazy_queue *queue, void *thing)
-- for (size_t i = queue->get_pending; i < queue->nr; i++) {
-- struct commit *commit = queue->array[i].data;
-- if (!(commit->object.flags & best->flag_within))
-- oidset_insert(&unflagged, &commit->object.oid);
-+ prio_queue_for_each(queue, c) {
-+ if (!(c->object.flags & best->flag_within))
-+ oidset_insert(&unflagged, &c->object.oid);
- }
+ static bool lazy_queue_empty(const struct lazy_queue *queue)
+ {
+- return queue->queue.nr == (queue->get_pending ? 1 : 0);
++ return prio_queue_size(&queue->queue) == (queue->get_pending ? 1 : 0);
+ }
- while ((c = prio_queue_get(queue))) {
+ static void lazy_queue_clear(struct lazy_queue *queue)
+@@ builtin/describe.c: static unsigned long finish_depth_computation(struct lazy_queue *queue,
+ {
+ unsigned long seen_commits = 0;
+ struct oidset unflagged = OIDSET_INIT;
++ struct commit *commit;
++ int skip = queue->get_pending ? 1 : 0;
+
+- for (size_t i = queue->get_pending ? 1 : 0; i < queue->queue.nr; i++) {
+- struct commit *commit = queue->queue.array[i].data;
++ prio_queue_for_each(&queue->queue, commit) {
++ if (skip) {
++ skip = 0;
++ continue;
++ }
+ if (!(commit->object.flags & best->flag_within))
+ oidset_insert(&unflagged, &commit->object.oid);
+ }
## builtin/last-modified.c ##
@@ builtin/last-modified.c: static void process_parent(struct last_modified *lm,
static int last_modified_run(struct last_modified *lm)
{
int max_count, queue_popped = 0;
-- struct commit *c;
+ struct commit *c, *n;
struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
struct prio_queue not_queue = { compare_commits_by_gen_then_commit_date };
struct commit_list *list;
+@@ builtin/last-modified.c: static int last_modified_run(struct last_modified *lm)
+ }
+ }
+
+- while (queue.nr) {
++ while ((c = prio_queue_get(&queue))) {
+ int parent_i;
+ struct commit_list *p;
+- struct commit *c = prio_queue_get(&queue);
+ struct bitmap *active_c = active_paths_for(lm, c);
+
+ if ((0 <= max_count && max_count < ++queue_popped) ||
@@ builtin/last-modified.c: static int last_modified_run(struct last_modified *lm)
*/
repo_parse_commit(lm->rev.repo, c);
@@ builtin/show-branch.c: static const char *get_color_reset_code(void)
static struct commit *interesting(struct prio_queue *queue)
{
-- for (size_t i = queue->get_pending; i < queue->nr; i++) {
+- for (size_t i = 0; i < queue->nr; i++) {
- struct commit *commit = queue->array[i].data;
- if (commit->object.flags & UNINTERESTING)
- continue;
@@ builtin/show-branch.c: static const char *get_color_reset_code(void)
}
return NULL;
}
+@@ builtin/show-branch.c: static void join_revs(struct prio_queue *queue,
+ {
+ int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
+ int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
++ struct commit *commit;
+
+- while (queue->nr) {
++ while ((commit = prio_queue_peek(queue))) {
+ struct commit_list *parents;
+ int still_interesting = !!interesting(queue);
+- struct commit *commit = prio_queue_peek(queue);
+ bool get_pending = true;
+ int flags = commit->object.flags & all_mask;
+
## commit-reach.c ##
@@ commit-reach.c: static int compare_commits_by_gen(const void *_a, const void *_b)
@@ commit-reach.c: static int compare_commits_by_gen(const void *_a, const void *_b
return 1;
}
@@ commit-reach.c: void ahead_behind(struct repository *r,
- struct commit **commits, size_t commits_nr,
struct ahead_behind_count *counts, size_t counts_nr)
{
-+ struct commit *c;
struct prio_queue queue = { .compare = compare_commits_by_gen_then_commit_date };
++ void *entry;
size_t width = DIV_ROUND_UP(commits_nr, BITS_IN_EWORD);
-@@ commit-reach.c: void ahead_behind(struct repository *r,
- init_bit_arrays(&bit_arrays);
-
- for (size_t i = 0; i < commits_nr; i++) {
-- struct commit *c = commits[i];
-- struct bitmap *bitmap = get_bit_array(c, width);
-+ struct bitmap *bitmap;
-+ c = commits[i];
-+ bitmap = get_bit_array(c, width);
-
- bitmap_set(bitmap, i);
- insert_no_dup(&queue, c);
- }
-
- while (queue_has_nonstale(&queue)) {
-- struct commit *c = prio_queue_get(&queue);
- struct commit_list *p;
-- struct bitmap *bitmap_c = get_bit_array(c, width);
-+ struct bitmap *bitmap_c;
-+ c = prio_queue_get(&queue);
-+ bitmap_c = get_bit_array(c, width);
-
- for (size_t i = 0; i < counts_nr; i++) {
- int reach_from_tip = !!bitmap_get(bitmap_c, counts[i].tip_index);
+ if (!commits_nr || !counts_nr)
@@ commit-reach.c: void ahead_behind(struct repository *r,
/* STALE is used here, PARENT2 is used by insert_no_dup(). */
repo_clear_commit_marks(r, PARENT2 | STALE);
- for (size_t i = 0; i < queue.nr; i++)
- free_bit_array(queue.array[i].data);
-+ prio_queue_for_each(&queue, c)
-+ free_bit_array(c);
++ prio_queue_for_each(&queue, entry)
++ free_bit_array(entry);
clear_bit_arrays(&bit_arrays);
clear_prio_queue(&queue);
}
+@@ commit-reach.c: int get_branch_base_for_tip(struct repository *r,
+ size_t bases_nr)
+ {
+ int best_index = -1;
+- struct commit *branch_point = NULL;
++ struct commit *c, *branch_point = NULL;
+ struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
+ int found_missing_gen = 0;
+
+@@ commit-reach.c: int get_branch_base_for_tip(struct repository *r,
+ prio_queue_put(&queue, c);
+ }
+
+- while (queue.nr) {
+- struct commit *c = prio_queue_get(&queue);
++ while ((c = prio_queue_get(&queue))) {
+ int best_for_c = get_best(c);
+ int best_for_p, positive;
+ struct commit *parent;
## fetch-pack.c ##
@@ fetch-pack.c: static int mark_complete_oid(const struct reference *ref, void *cb_data UNUSED)
@@ object-name.c: static int get_oid_oneline(struct repository *r,
## pack-bitmap-write.c ##
@@ pack-bitmap-write.c: static int fill_bitmap_commit(struct bitmap_writer *writer,
+ struct bitmap_index *old_bitmap,
const uint32_t *mapping)
{
- struct commit *c;
++ struct commit *c;
+ struct tree *tree;
int found;
uint32_t pos;
if (!ent->bitmap)
@@ pack-bitmap-write.c: static int fill_bitmap_commit(struct bitmap_writer *writer,
+
+ prio_queue_put(queue, commit);
+
+- while (queue->nr) {
++ while ((c = prio_queue_get(queue))) {
+ struct commit_list *p;
+- struct commit *c = prio_queue_get(queue);
+
+ if (old_bitmap && mapping) {
+ struct ewah_bitmap *old;
+@@ pack-bitmap-write.c: static int fill_bitmap_commit(struct bitmap_writer *writer,
}
}
@@ prio-queue.c: void prio_queue_reverse(struct prio_queue *queue)
if (queue->compare)
BUG("prio_queue_reverse() on non-LIFO queue");
- if (!queue->nr)
-+ if (!queue->nr_internal)
++ if (!queue->nr_)
return;
- for (i = 0; i < (j = (queue->nr - 1) - i); i++)
-+ for (i = 0; i < (j = (queue->nr_internal - 1) - i); i++)
++ for (i = 0; i < (j = (queue->nr_ - 1) - i); i++)
swap(queue, i, j);
}
@@ prio-queue.c: void prio_queue_reverse(struct prio_queue *queue)
{
FREE_AND_NULL(queue->array);
- queue->nr = 0;
-+ queue->nr_internal = 0;
++ queue->nr_ = 0;
queue->alloc = 0;
queue->insertion_ctr = 0;
- queue->get_pending = 0;
-@@ prio-queue.c: static void sift_down_root(struct prio_queue *queue)
- {
- size_t ix, child;
-
-- for (ix = 0; ix * 2 + 1 < queue->nr; ix = child) {
-- child = ix * 2 + 1;
-- if (child + 1 < queue->nr &&
-+ /* Push down the one at the root */
-+ for (ix = 0; ix * 2 + 1 < queue->nr_internal; ix = child) {
-+ child = ix * 2 + 1; /* left */
-+ if (child + 1 < queue->nr_internal &&
- compare(queue, child, child + 1) >= 0)
-- child++;
-+ child++; /* use right child */
-+
- if (compare(queue, ix, child) <= 0)
- break;
-+
- swap(queue, child, ix);
- }
}
@@ prio-queue.c: void prio_queue_put(struct prio_queue *queue, void *thing)
- return;
- }
+ size_t ix, parent;
+ /* Append at the end */
- ALLOC_GROW(queue->array, queue->nr + 1, queue->alloc);
- queue->array[queue->nr].ctr = queue->insertion_ctr++;
- queue->array[queue->nr].data = thing;
- queue->nr++;
-+ /* Append at the end */
-+ ALLOC_GROW(queue->array, queue->nr_internal + 1, queue->alloc);
-+ queue->array[queue->nr_internal].ctr = queue->insertion_ctr++;
-+ queue->array[queue->nr_internal].data = thing;
-+ queue->nr_internal++;
++ ALLOC_GROW(queue->array, queue->nr_ + 1, queue->alloc);
++ queue->array[queue->nr_].ctr = queue->insertion_ctr++;
++ queue->array[queue->nr_].data = thing;
++ queue->nr_++;
if (!queue->compare)
-- return;
-+ return; /* LIFO */
+ return; /* LIFO */
+ /* Bubble up the new one */
- for (ix = queue->nr - 1; ix; ix = parent) {
-+ /* Bubble up the new one */
-+ for (ix = queue->nr_internal - 1; ix; ix = parent) {
++ for (ix = queue->nr_ - 1; ix; ix = parent) {
parent = (ix - 1) / 2;
if (compare(queue, parent, ix) <= 0)
break;
-+
- swap(queue, parent, ix);
- }
- }
+@@ prio-queue.c: static void sift_down_root(struct prio_queue *queue)
+ size_t ix, child;
+
+ /* Push down the one at the root */
+- for (ix = 0; ix * 2 + 1 < queue->nr; ix = child) {
++ for (ix = 0; ix * 2 + 1 < queue->nr_; ix = child) {
+ child = ix * 2 + 1; /* left */
+- if (child + 1 < queue->nr &&
++ if (child + 1 < queue->nr_ &&
+ compare(queue, child, child + 1) >= 0)
+ child++; /* use right child */
- void *prio_queue_get(struct prio_queue *queue)
+@@ prio-queue.c: void *prio_queue_get(struct prio_queue *queue)
{
+ void *result;
+
- if (!queue->nr)
-+ if (!queue->nr_internal)
++ if (!queue->nr_)
return NULL;
if (!queue->compare)
-- return queue->array[--queue->nr].data;
-+ return queue->array[--queue->nr_internal].data; /* LIFO */
-
- if (queue->get_pending) {
-- if (!--queue->nr) {
-+ if (!--queue->nr_internal) {
- queue->get_pending = 0;
- return NULL;
- }
-- queue->array[0] = queue->array[queue->nr];
-+ queue->array[0] = queue->array[queue->nr_internal];
- sift_down_root(queue);
- }
-
-@@ prio-queue.c: void *prio_queue_get(struct prio_queue *queue)
+- return queue->array[--queue->nr].data; /* LIFO */
++ return queue->array[--queue->nr_].data; /* LIFO */
+
+ result = queue->array[0].data;
+- if (!--queue->nr)
++ if (!--queue->nr_)
+ return result;
+
+- queue->array[0] = queue->array[queue->nr];
++ queue->array[0] = queue->array[queue->nr_];
+ sift_down_root(queue);
+ return result;
+ }
void *prio_queue_peek(struct prio_queue *queue)
{
- if (!queue->nr)
-+ if (!queue->nr_internal)
++ if (!queue->nr_)
return NULL;
if (!queue->compare)
- return queue->array[queue->nr - 1].data;
-+ return queue->array[queue->nr_internal - 1].data;
-
- if (queue->get_pending) {
- queue->get_pending = 0;
-- if (!--queue->nr)
-+ if (!--queue->nr_internal)
- return NULL;
-- queue->array[0] = queue->array[queue->nr];
-+ queue->array[0] = queue->array[queue->nr_internal];
- sift_down_root(queue);
- }
++ return queue->array[queue->nr_ - 1].data;
+ return queue->array[0].data;
+ }
+ void prio_queue_replace(struct prio_queue *queue, void *thing)
+ {
+- if (!queue->nr) {
++ if (!queue->nr_) {
+ prio_queue_put(queue, thing);
+ } else if (!queue->compare) {
+- queue->array[queue->nr - 1].ctr = queue->insertion_ctr++;
+- queue->array[queue->nr - 1].data = thing;
++ queue->array[queue->nr_ - 1].ctr = queue->insertion_ctr++;
++ queue->array[queue->nr_ - 1].data = thing;
+ } else {
+ queue->array[0].ctr = queue->insertion_ctr++;
+ queue->array[0].data = thing;
## prio-queue.h ##
@@ prio-queue.h: struct prio_queue {
@@ prio-queue.h: struct prio_queue {
size_t insertion_ctr;
void *cb_data;
- size_t alloc, nr;
-+ size_t alloc, nr_internal; /* use prio_queue_size() for logical count */
++ size_t alloc, nr_;
struct prio_queue_entry *array;
- unsigned get_pending;
};
-@@ prio-queue.h: void *prio_queue_peek(struct prio_queue *);
- static inline size_t prio_queue_size(struct prio_queue *queue)
- {
-- return queue->nr - queue->get_pending;
-+ return queue->nr_internal - queue->get_pending;
- }
+@@ prio-queue.h: void *prio_queue_get(struct prio_queue *);
+ */
+ void *prio_queue_peek(struct prio_queue *);
++static inline size_t prio_queue_size(const struct prio_queue *queue)
++{
++ return queue->nr_;
++}
++
+#define prio_queue_for_each(queue, it) \
-+ for (size_t pq_ix_ = (queue)->get_pending; \
-+ pq_ix_ < (queue)->nr_internal && ((it) = (queue)->array[pq_ix_].data, 1); \
++ for (size_t pq_ix_ = 0; \
++ pq_ix_ < (queue)->nr_ && ((it) = (queue)->array[pq_ix_].data, 1); \
+ pq_ix_++)
+
- void clear_prio_queue(struct prio_queue *);
-
- /* Reverse the LIFO elements */
+ /*
+ * Replace the "thing" that compares the smallest with a new "thing",
+ * like prio_queue_get()+prio_queue_put() would do, but in a more
## revision.c ##
@@ revision.c: static struct commit *handle_commit(struct rev_info *revs,
@@ revision.c: static struct commit *handle_commit(struct rev_info *revs,
if (commit->object.flags & UNINTERESTING)
continue;
+@@ revision.c: static int limit_list(struct rev_info *revs)
+ struct commit_list *original_list = revs->commits;
+ struct commit_list *newlist = NULL;
+ struct commit_list **p = &newlist;
+- struct commit *interesting_cache = NULL;
++ struct commit *commit, *interesting_cache = NULL;
+ struct prio_queue queue = { .compare = compare_commits_by_commit_date };
+
+ if (revs->ancestry_path_implicit_bottoms) {
+@@ revision.c: static int limit_list(struct rev_info *revs)
+ prio_queue_put(&queue, commit);
+ }
+
+- while (queue.nr) {
+- struct commit *commit = prio_queue_get(&queue);
++ while ((commit = prio_queue_get(&queue))) {
+ struct object *obj = &commit->object;
+
+ if (commit == interesting_cache)
@@ revision.c: static enum rewrite_result rewrite_one_1(struct rev_info *revs,
static void merge_queue_into_list(struct prio_queue *q, struct commit_list **list)
@@ revision.c: static enum rewrite_result rewrite_one_1(struct rev_info *revs,
struct commit_list *p = *list;
if (p && p->item->date >= item->date)
+
+ ## walker.c ##
+@@ walker.c: static struct prio_queue complete = { compare_commits_by_commit_date };
+ static int process_commit(struct walker *walker, struct commit *commit)
+ {
+ struct commit_list *parents;
++ struct commit *item;
+
+ if (repo_parse_commit(the_repository, commit))
+ return -1;
+
+- while (complete.nr) {
+- struct commit *item = prio_queue_peek(&complete);
++ while ((item = prio_queue_peek(&complete))) {
+ if (item->date < commit->date)
+ break;
+ pop_most_recent_commit(&complete, COMPLETE);
1: e882206d29 ! 2: a3f4cb57f2 prio-queue: fold lazy_queue into prio_queue for automatic get+put fusion
@@ Commit message
Defer the actual removal in prio_queue_get() until the next
operation. If that next operation is a prio_queue_put(), the
- removal and insertion are fused into a single replace, writing
- the new element at the root and sifting it down which avoids
+ removal and insertion are fused into a single replace — writing
+ the new element at the root and sifting it down — which avoids
a full remove-rebalance-insert cycle.
This matches the dominant usage pattern in git's commit traversal:
- get a commit, then put its parents. The first parent insertion
+ get a commit, then put its parents. The first parent insertion
after each get is now a replace operation automatically.
This generalizes the lazy_queue pattern from builtin/describe.c
- (introduced in 08bb69d70f) into prio_queue itself. Three callers
+ (introduced in 08bb69d70f) into prio_queue itself. Three callers
independently implemented the same get+put fusion:
- builtin/describe.c had a full lazy_queue wrapper
@@ Commit message
- builtin/show-branch.c:join_revs() used peek+replace
All three now collapse to plain _get() and _put(), with the data
- structure handling the fusion internally. This simplifies callers
+ structure handling the fusion internally. This simplifies callers
and means every prio_queue user gets the optimization for free
without needing to implement it manually.
Remove prio_queue_replace() since no external callers remain.
- Add prio_queue_size() for callers that need the logical element
- count, since the physical nr may temporarily include a
- pending-removal element.
- Benchmarked on a large monorepo (30 interleaved runs,
+ Benchmarked on a 1.8M-commit monorepo (30 interleaved runs,
paired t-test, Xeon @ 2.20GHz):
Code paths that previously did eager get+put (new optimization):
- Command base patched change p
+ Command base patched change p
merge-base --all A A~1000 3828ms 3725ms -2.69% 0.0001
rev-list --count A~1000..A 3055ms 2986ms -2.27% 0.0601
log --oneline A~1000..A 3408ms 3350ms -1.71% 0.0482
Code paths that already had manual get+put fusion (expect
- neutral - the optimization moves into prio_queue but the number
+ neutral — the optimization moves into prio_queue but the number
of heap operations stays the same):
- Command base patched change p
- show-branch A A~1000 9156ms 9127ms -0.32% 0.3470
- describe (git.git) 1983ms 1963ms -1.02% <0.001
+ Command base patched change p
+ show-branch A A~1000 9156ms 9127ms -0.32% 0.3470
+ describe (4751 revs, 81K repo) 1983ms 1963ms -1.02% <0.001
No regressions in any scenario.
@@ builtin/describe.c: static int compare_pt(const void *a_, const void *b_)
-
-static bool lazy_queue_empty(const struct lazy_queue *queue)
-{
-- return queue->queue.nr == (queue->get_pending ? 1 : 0);
+- return prio_queue_size(&queue->queue) == (queue->get_pending ? 1 : 0);
-}
-
-static void lazy_queue_clear(struct lazy_queue *queue)
@@ builtin/describe.c: static int compare_pt(const void *a_, const void *b_)
{
unsigned long seen_commits = 0;
struct oidset unflagged = OIDSET_INIT;
+- struct commit *commit;
+- int skip = queue->get_pending ? 1 : 0;
+ struct commit *c;
-- for (size_t i = queue->get_pending ? 1 : 0; i < queue->queue.nr; i++) {
-- struct commit *commit = queue->queue.array[i].data;
-+ for (size_t i = queue->get_pending; i < queue->nr; i++) {
-+ struct commit *commit = queue->array[i].data;
- if (!(commit->object.flags & best->flag_within))
- oidset_insert(&unflagged, &commit->object.oid);
+- prio_queue_for_each(&queue->queue, commit) {
+- if (skip) {
+- skip = 0;
+- continue;
+- }
+- if (!(commit->object.flags & best->flag_within))
+- oidset_insert(&unflagged, &commit->object.oid);
++ prio_queue_for_each(queue, c) {
++ if (!(c->object.flags & best->flag_within))
++ oidset_insert(&unflagged, &c->object.oid);
}
- while (!lazy_queue_empty(queue)) {
@@ builtin/describe.c: static void describe_commit(struct commit *cmit, struct strb
if (debug) {
static int label_width = -1;
- ## builtin/last-modified.c ##
-@@ builtin/last-modified.c: static void process_parent(struct last_modified *lm,
- static int last_modified_run(struct last_modified *lm)
- {
- int max_count, queue_popped = 0;
-+ struct commit *c;
- struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
- struct prio_queue not_queue = { compare_commits_by_gen_then_commit_date };
- struct commit_list *list;
-@@ builtin/last-modified.c: static int last_modified_run(struct last_modified *lm)
- }
- }
-
-- while (queue.nr) {
-+ while ((c = prio_queue_get(&queue))) {
- int parent_i;
- struct commit_list *p;
-- struct commit *c = prio_queue_get(&queue);
- struct bitmap *active_c = active_paths_for(lm, c);
-
- if ((0 <= max_count && max_count < ++queue_popped) ||
-
## builtin/show-branch.c ##
-@@ builtin/show-branch.c: static const char *get_color_reset_code(void)
-
- static struct commit *interesting(struct prio_queue *queue)
- {
-- for (size_t i = 0; i < queue->nr; i++) {
-+ for (size_t i = queue->get_pending; i < queue->nr; i++) {
- struct commit *commit = queue->array[i].data;
- if (commit->object.flags & UNINTERESTING)
- continue;
@@ builtin/show-branch.c: static void join_revs(struct prio_queue *queue,
- {
- int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
- int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
-+ struct commit *commit;
-
-- while (queue->nr) {
-+ while ((commit = prio_queue_peek(queue))) {
+ while ((commit = prio_queue_peek(queue))) {
struct commit_list *parents;
int still_interesting = !!interesting(queue);
-- struct commit *commit = prio_queue_peek(queue);
- bool get_pending = true;
int flags = commit->object.flags & all_mask;
@@ builtin/show-branch.c: static void join_revs(struct prio_queue *queue,
/*
- ## commit-reach.c ##
-@@ commit-reach.c: int get_branch_base_for_tip(struct repository *r,
- size_t bases_nr)
- {
- int best_index = -1;
-- struct commit *branch_point = NULL;
-+ struct commit *c, *branch_point = NULL;
- struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
- int found_missing_gen = 0;
-
-@@ commit-reach.c: int get_branch_base_for_tip(struct repository *r,
- prio_queue_put(&queue, c);
- }
-
-- while (queue.nr) {
-- struct commit *c = prio_queue_get(&queue);
-+ while ((c = prio_queue_get(&queue))) {
- int best_for_c = get_best(c);
- int best_for_p, positive;
- struct commit *parent;
-
## commit.c ##
@@ commit.c: void commit_list_sort_by_date(struct commit_list **list)
struct commit *pop_most_recent_commit(struct prio_queue *queue,
@@ commit.c: void commit_list_sort_by_date(struct commit_list **list)
}
- ## pack-bitmap-write.c ##
-@@ pack-bitmap-write.c: static int fill_bitmap_commit(struct bitmap_writer *writer,
- struct bitmap_index *old_bitmap,
- const uint32_t *mapping)
- {
-+ struct commit *c;
- int found;
- uint32_t pos;
- if (!ent->bitmap)
-@@ pack-bitmap-write.c: static int fill_bitmap_commit(struct bitmap_writer *writer,
-
- prio_queue_put(queue, commit);
-
-- while (queue->nr) {
-+ while ((c = prio_queue_get(queue))) {
- struct commit_list *p;
-- struct commit *c = prio_queue_get(queue);
-
- if (old_bitmap && mapping) {
- struct ewah_bitmap *old;
-
## prio-queue.c ##
@@ prio-queue.c: void clear_prio_queue(struct prio_queue *queue)
- queue->nr = 0;
+ queue->nr_ = 0;
queue->alloc = 0;
queue->insertion_ctr = 0;
+ queue->get_pending = 0;
@@ prio-queue.c: void clear_prio_queue(struct prio_queue *queue)
+{
+ size_t ix, child;
+
-+ for (ix = 0; ix * 2 + 1 < queue->nr; ix = child) {
-+ child = ix * 2 + 1;
-+ if (child + 1 < queue->nr &&
++ /* Push down the one at the root */
++ for (ix = 0; ix * 2 + 1 < queue->nr_; ix = child) {
++ child = ix * 2 + 1; /* left */
++ if (child + 1 < queue->nr_ &&
+ compare(queue, child, child + 1) >= 0)
-+ child++;
++ child++; /* use right child */
++
+ if (compare(queue, ix, child) <= 0)
+ break;
++
+ swap(queue, child, ix);
+ }
++}
++
++static inline void flush_get(struct prio_queue *queue)
++{
++ if (!queue->get_pending)
++ return;
++ queue->get_pending = 0;
++ queue->array[0] = queue->array[--queue->nr_];
++ sift_down_root(queue);
}
void prio_queue_put(struct prio_queue *queue, void *thing)
{
size_t ix, parent;
-- /* Append at the end */
+ if (queue->get_pending) {
+ queue->get_pending = 0;
+ queue->array[0].ctr = queue->insertion_ctr++;
@@ prio-queue.c: void clear_prio_queue(struct prio_queue *queue)
+ return;
+ }
+
- ALLOC_GROW(queue->array, queue->nr + 1, queue->alloc);
- queue->array[queue->nr].ctr = queue->insertion_ctr++;
- queue->array[queue->nr].data = thing;
- queue->nr++;
- if (!queue->compare)
-- return; /* LIFO */
-+ return;
-
-- /* Bubble up the new one */
- for (ix = queue->nr - 1; ix; ix = parent) {
- parent = (ix - 1) / 2;
- if (compare(queue, parent, ix) <= 0)
- break;
--
- swap(queue, parent, ix);
+ /* Append at the end */
+ ALLOC_GROW(queue->array, queue->nr_ + 1, queue->alloc);
+ queue->array[queue->nr_].ctr = queue->insertion_ctr++;
+@@ prio-queue.c: void prio_queue_put(struct prio_queue *queue, void *thing)
}
}
@@ prio-queue.c: void clear_prio_queue(struct prio_queue *queue)
- size_t ix, child;
-
- /* Push down the one at the root */
-- for (ix = 0; ix * 2 + 1 < queue->nr; ix = child) {
+- for (ix = 0; ix * 2 + 1 < queue->nr_; ix = child) {
- child = ix * 2 + 1; /* left */
-- if (child + 1 < queue->nr &&
+- if (child + 1 < queue->nr_ &&
- compare(queue, child, child + 1) >= 0)
- child++; /* use right child */
-
@@ prio-queue.c: void clear_prio_queue(struct prio_queue *queue)
{
- void *result;
-
- if (!queue->nr)
+- if (!queue->nr_)
++ if (queue->nr_ <= queue->get_pending) {
++ queue->nr_ = 0;
++ queue->get_pending = 0;
return NULL;
++ }
if (!queue->compare)
-- return queue->array[--queue->nr].data; /* LIFO */
--
+ return queue->array[--queue->nr_].data; /* LIFO */
+
- result = queue->array[0].data;
-- if (!--queue->nr)
+- if (!--queue->nr_)
- return result;
-+ return queue->array[--queue->nr].data;
-+
-+ if (queue->get_pending) {
-+ if (!--queue->nr) {
-+ queue->get_pending = 0;
-+ return NULL;
-+ }
-+ queue->array[0] = queue->array[queue->nr];
-+ sift_down_root(queue);
-+ }
++ flush_get(queue);
-- queue->array[0] = queue->array[queue->nr];
+- queue->array[0] = queue->array[queue->nr_];
- sift_down_root(queue);
- return result;
+ queue->get_pending = 1;
@@ prio-queue.c: void clear_prio_queue(struct prio_queue *queue)
}
void *prio_queue_peek(struct prio_queue *queue)
-@@ prio-queue.c: void *prio_queue_peek(struct prio_queue *queue)
+ {
+- if (!queue->nr_)
++ if (queue->nr_ <= queue->get_pending) {
++ queue->nr_ = 0;
++ queue->get_pending = 0;
return NULL;
++ }
if (!queue->compare)
- return queue->array[queue->nr - 1].data;
+ return queue->array[queue->nr_ - 1].data;
- return queue->array[0].data;
-}
-void prio_queue_replace(struct prio_queue *queue, void *thing)
-{
-- if (!queue->nr) {
+- if (!queue->nr_) {
- prio_queue_put(queue, thing);
- } else if (!queue->compare) {
-- queue->array[queue->nr - 1].ctr = queue->insertion_ctr++;
-- queue->array[queue->nr - 1].data = thing;
+- queue->array[queue->nr_ - 1].ctr = queue->insertion_ctr++;
+- queue->array[queue->nr_ - 1].data = thing;
- } else {
- queue->array[0].ctr = queue->insertion_ctr++;
- queue->array[0].data = thing;
-+ if (queue->get_pending) {
-+ queue->get_pending = 0;
-+ if (!--queue->nr)
-+ return NULL;
-+ queue->array[0] = queue->array[queue->nr];
- sift_down_root(queue);
- }
+- sift_down_root(queue);
+- }
++ flush_get(queue);
+
+ return queue->array[0].data;
}
## prio-queue.h ##
@@ prio-queue.h: struct prio_queue {
+ prio_queue_compare_fn compare;
+ size_t insertion_ctr;
void *cb_data;
- size_t alloc, nr;
+- size_t alloc, nr_;
++ size_t alloc, nr_; /* use prio_queue_size() for logical count */
struct prio_queue_entry *array;
+ unsigned get_pending;
};
/*
-@@ prio-queue.h: void *prio_queue_get(struct prio_queue *);
- */
- void *prio_queue_peek(struct prio_queue *);
+@@ prio-queue.h: void *prio_queue_peek(struct prio_queue *);
+
+ static inline size_t prio_queue_size(const struct prio_queue *queue)
+ {
+- return queue->nr_;
++ return queue->nr_ - queue->get_pending;
+ }
+
+ #define prio_queue_for_each(queue, it) \
+- for (size_t pq_ix_ = 0; \
++ for (size_t pq_ix_ = (queue)->get_pending; \
+ pq_ix_ < (queue)->nr_ && ((it) = (queue)->array[pq_ix_].data, 1); \
+ pq_ix_++)
-/*
- * Replace the "thing" that compares the smallest with a new "thing",
@@ prio-queue.h: void *prio_queue_get(struct prio_queue *);
- * empty.
- */
-void prio_queue_replace(struct prio_queue *queue, void *thing);
-+static inline size_t prio_queue_size(struct prio_queue *queue)
-+{
-+ return queue->nr - queue->get_pending;
-+}
-
+-
void clear_prio_queue(struct prio_queue *);
-
- ## revision.c ##
-@@ revision.c: static int limit_list(struct rev_info *revs)
- struct commit_list *original_list = revs->commits;
- struct commit_list *newlist = NULL;
- struct commit_list **p = &newlist;
-- struct commit *interesting_cache = NULL;
-+ struct commit *commit, *interesting_cache = NULL;
- struct prio_queue queue = { .compare = compare_commits_by_commit_date };
-
- if (revs->ancestry_path_implicit_bottoms) {
-@@ revision.c: static int limit_list(struct rev_info *revs)
- prio_queue_put(&queue, commit);
- }
-
-- while (queue.nr) {
-- struct commit *commit = prio_queue_get(&queue);
-+ while ((commit = prio_queue_get(&queue))) {
- struct object *obj = &commit->object;
-
- if (commit == interesting_cache)
+ /* Reverse the LIFO elements */
## t/unit-tests/u-prio-queue.c ##
@@ t/unit-tests/u-prio-queue.c: static void test_prio_queue(int *input, size_t input_size,
@@ t/unit-tests/u-prio-queue.c: static void test_prio_queue(int *input, size_t inpu
break;
default:
prio_queue_put(&pq, &input[i]);
-
- ## walker.c ##
-@@ walker.c: static struct prio_queue complete = { compare_commits_by_commit_date };
- static int process_commit(struct walker *walker, struct commit *commit)
- {
- struct commit_list *parents;
-+ struct commit *item;
-
- if (repo_parse_commit(the_repository, commit))
- return -1;
-
-- while (complete.nr) {
-- struct commit *item = prio_queue_peek(&complete);
-+ while ((item = prio_queue_peek(&complete))) {
- if (item->date < commit->date)
- break;
- pop_most_recent_commit(&complete, COMPLETE);
--
gitgitgadget
^ permalink raw reply
* [PATCH v4 1/2] prio-queue: rename .nr to .nr_ and add accessor helpers
From: Kristofer Karlsson via GitGitGadget @ 2026-06-08 19:10 UTC (permalink / raw)
To: git
Cc: René Scharfe, Kristofer Karlsson, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2140.v4.git.1780945851.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
Rename the .nr member to .nr_ so that callers outside prio-queue.c
that directly reference .nr get a compilation error. This catches
both existing misuse and future in-flight topics.
Add prio_queue_size() for callers that need to know the element count
and prio_queue_for_each() for callers that need to walk all elements.
Convert all external .nr users:
- Loop conditions: use prio_queue_size(), prio_queue_get(), or
prio_queue_peek() as the loop condition
- Array iterations: use prio_queue_for_each()
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
builtin/describe.c | 11 ++++++++---
builtin/last-modified.c | 7 +++----
builtin/show-branch.c | 13 ++++++-------
commit-reach.c | 14 +++++++-------
fetch-pack.c | 4 ++--
negotiator/default.c | 4 +++-
negotiator/skipping.c | 12 +++++++-----
object-name.c | 2 +-
pack-bitmap-write.c | 10 +++++-----
path-walk.c | 8 ++++----
prio-queue.c | 38 +++++++++++++++++++-------------------
prio-queue.h | 12 +++++++++++-
revision.c | 16 +++++++---------
walker.c | 4 ++--
14 files changed, 85 insertions(+), 70 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 1c47d7c0b7..8e88bdeea6 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -278,7 +278,7 @@ static void lazy_queue_put(struct lazy_queue *queue, void *thing)
static bool lazy_queue_empty(const struct lazy_queue *queue)
{
- return queue->queue.nr == (queue->get_pending ? 1 : 0);
+ return prio_queue_size(&queue->queue) == (queue->get_pending ? 1 : 0);
}
static void lazy_queue_clear(struct lazy_queue *queue)
@@ -292,9 +292,14 @@ static unsigned long finish_depth_computation(struct lazy_queue *queue,
{
unsigned long seen_commits = 0;
struct oidset unflagged = OIDSET_INIT;
+ struct commit *commit;
+ int skip = queue->get_pending ? 1 : 0;
- for (size_t i = queue->get_pending ? 1 : 0; i < queue->queue.nr; i++) {
- struct commit *commit = queue->queue.array[i].data;
+ prio_queue_for_each(&queue->queue, commit) {
+ if (skip) {
+ skip = 0;
+ continue;
+ }
if (!(commit->object.flags & best->flag_within))
oidset_insert(&unflagged, &commit->object.oid);
}
diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 8900ceece1..5478182f2e 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -344,6 +344,7 @@ static void process_parent(struct last_modified *lm,
static int last_modified_run(struct last_modified *lm)
{
int max_count, queue_popped = 0;
+ struct commit *c, *n;
struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
struct prio_queue not_queue = { compare_commits_by_gen_then_commit_date };
struct commit_list *list;
@@ -389,10 +390,9 @@ static int last_modified_run(struct last_modified *lm)
}
}
- while (queue.nr) {
+ while ((c = prio_queue_get(&queue))) {
int parent_i;
struct commit_list *p;
- struct commit *c = prio_queue_get(&queue);
struct bitmap *active_c = active_paths_for(lm, c);
if ((0 <= max_count && max_count < ++queue_popped) ||
@@ -416,9 +416,8 @@ static int last_modified_run(struct last_modified *lm)
*/
repo_parse_commit(lm->rev.repo, c);
- while (not_queue.nr) {
+ while ((n = prio_queue_get(¬_queue))) {
struct commit_list *np;
- struct commit *n = prio_queue_get(¬_queue);
repo_parse_commit(lm->rev.repo, n);
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f02831b085..8846f2376f 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -62,11 +62,10 @@ static const char *get_color_reset_code(void)
static struct commit *interesting(struct prio_queue *queue)
{
- for (size_t i = 0; i < queue->nr; i++) {
- struct commit *commit = queue->array[i].data;
- if (commit->object.flags & UNINTERESTING)
- continue;
- return commit;
+ struct commit *commit;
+ prio_queue_for_each(queue, commit) {
+ if (!(commit->object.flags & UNINTERESTING))
+ return commit;
}
return NULL;
}
@@ -228,11 +227,11 @@ static void join_revs(struct prio_queue *queue,
{
int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
+ struct commit *commit;
- while (queue->nr) {
+ while ((commit = prio_queue_peek(queue))) {
struct commit_list *parents;
int still_interesting = !!interesting(queue);
- struct commit *commit = prio_queue_peek(queue);
bool get_pending = true;
int flags = commit->object.flags & all_mask;
diff --git a/commit-reach.c b/commit-reach.c
index 9b3ea46d6f..a849de653e 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -41,8 +41,8 @@ static int compare_commits_by_gen(const void *_a, const void *_b)
static int queue_has_nonstale(struct prio_queue *queue)
{
- for (size_t i = 0; i < queue->nr; i++) {
- struct commit *commit = queue->array[i].data;
+ struct commit *commit;
+ prio_queue_for_each(queue, commit) {
if (!(commit->object.flags & STALE))
return 1;
}
@@ -1070,6 +1070,7 @@ void ahead_behind(struct repository *r,
struct ahead_behind_count *counts, size_t counts_nr)
{
struct prio_queue queue = { .compare = compare_commits_by_gen_then_commit_date };
+ void *entry;
size_t width = DIV_ROUND_UP(commits_nr, BITS_IN_EWORD);
if (!commits_nr || !counts_nr)
@@ -1135,8 +1136,8 @@ void ahead_behind(struct repository *r,
/* STALE is used here, PARENT2 is used by insert_no_dup(). */
repo_clear_commit_marks(r, PARENT2 | STALE);
- for (size_t i = 0; i < queue.nr; i++)
- free_bit_array(queue.array[i].data);
+ prio_queue_for_each(&queue, entry)
+ free_bit_array(entry);
clear_bit_arrays(&bit_arrays);
clear_prio_queue(&queue);
}
@@ -1269,7 +1270,7 @@ int get_branch_base_for_tip(struct repository *r,
size_t bases_nr)
{
int best_index = -1;
- struct commit *branch_point = NULL;
+ struct commit *c, *branch_point = NULL;
struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
int found_missing_gen = 0;
@@ -1322,8 +1323,7 @@ int get_branch_base_for_tip(struct repository *r,
prio_queue_put(&queue, c);
}
- while (queue.nr) {
- struct commit *c = prio_queue_get(&queue);
+ while ((c = prio_queue_get(&queue))) {
int best_for_c = get_best(c);
int best_for_p, positive;
struct commit *parent;
diff --git a/fetch-pack.c b/fetch-pack.c
index 120e01f3cf..29c41132ee 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -662,8 +662,8 @@ static int mark_complete_oid(const struct reference *ref, void *cb_data UNUSED)
static void mark_recent_complete_commits(struct fetch_pack_args *args,
timestamp_t cutoff)
{
- while (complete.nr) {
- struct commit *item = prio_queue_peek(&complete);
+ struct commit *item;
+ while ((item = prio_queue_peek(&complete))) {
if (item->date < cutoff)
break;
print_verbose(args, _("Marking %s as complete"),
diff --git a/negotiator/default.c b/negotiator/default.c
index 78d58d57ce..19cdf3808c 100644
--- a/negotiator/default.c
+++ b/negotiator/default.c
@@ -113,10 +113,12 @@ static const struct object_id *get_rev(struct negotiation_state *ns)
unsigned int mark;
struct commit_list *parents;
- if (ns->rev_list.nr == 0 || ns->non_common_revs == 0)
+ if (ns->non_common_revs == 0)
return NULL;
commit = prio_queue_get(&ns->rev_list);
+ if (!commit)
+ return NULL;
repo_parse_commit(the_repository, commit);
parents = commit->parents;
diff --git a/negotiator/skipping.c b/negotiator/skipping.c
index 68c9b3b997..db90fa77b5 100644
--- a/negotiator/skipping.c
+++ b/negotiator/skipping.c
@@ -143,8 +143,7 @@ static int push_parent(struct data *data, struct entry *entry,
/*
* Find the existing entry and use it.
*/
- for (size_t i = 0; i < data->rev_list.nr; i++) {
- parent_entry = data->rev_list.array[i].data;
+ prio_queue_for_each(&data->rev_list, parent_entry) {
if (parent_entry->commit == to_push)
goto parent_found;
}
@@ -181,10 +180,12 @@ static const struct object_id *get_rev(struct data *data)
struct commit_list *p;
int parent_pushed = 0;
- if (data->rev_list.nr == 0 || data->non_common_revs == 0)
+ if (data->non_common_revs == 0)
return NULL;
entry = prio_queue_get(&data->rev_list);
+ if (!entry)
+ return NULL;
commit = entry->commit;
commit->object.flags |= POPPED;
if (!(commit->object.flags & COMMON))
@@ -253,8 +254,9 @@ static void have_sent(struct fetch_negotiator *n, struct commit *c)
static void release(struct fetch_negotiator *n)
{
struct data *data = n->data;
- for (size_t i = 0; i < data->rev_list.nr; i++)
- free(data->rev_list.array[i].data);
+ void *entry;
+ prio_queue_for_each(&data->rev_list, entry)
+ free(entry);
clear_prio_queue(&data->rev_list);
FREE_AND_NULL(data);
}
diff --git a/object-name.c b/object-name.c
index 9ac86f19c7..2fedfe1761 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1208,7 +1208,7 @@ static int get_oid_oneline(struct repository *r,
l->item->object.flags |= ONELINE_SEEN;
prio_queue_put(©, l->item);
}
- while (copy.nr) {
+ while (prio_queue_size(©)) {
const char *p, *buf;
struct commit *commit;
int matches;
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 1c8070f99c..ed9714b135 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -513,6 +513,8 @@ static int fill_bitmap_commit(struct bitmap_writer *writer,
struct bitmap_index *old_bitmap,
const uint32_t *mapping)
{
+ struct commit *c;
+ struct tree *tree;
int found;
uint32_t pos;
if (!ent->bitmap)
@@ -520,9 +522,8 @@ static int fill_bitmap_commit(struct bitmap_writer *writer,
prio_queue_put(queue, commit);
- while (queue->nr) {
+ while ((c = prio_queue_get(queue))) {
struct commit_list *p;
- struct commit *c = prio_queue_get(queue);
if (old_bitmap && mapping) {
struct ewah_bitmap *old;
@@ -574,9 +575,8 @@ static int fill_bitmap_commit(struct bitmap_writer *writer,
}
}
- while (tree_queue->nr) {
- if (fill_bitmap_tree(writer, ent->bitmap,
- prio_queue_get(tree_queue)) < 0)
+ while ((tree = prio_queue_get(tree_queue))) {
+ if (fill_bitmap_tree(writer, ent->bitmap, tree) < 0)
return -1;
}
return 0;
diff --git a/path-walk.c b/path-walk.c
index 94ff90bd15..cf3b2d0765 100644
--- a/path-walk.c
+++ b/path-walk.c
@@ -699,6 +699,7 @@ int walk_objects_by_path(struct path_walk_info *info)
int ret;
size_t commits_nr = 0, paths_nr = 0;
struct commit *c;
+ char *path;
struct type_and_oid_list *root_tree_list;
struct type_and_oid_list *commit_list;
struct path_walk_context ctx = {
@@ -808,8 +809,7 @@ int walk_objects_by_path(struct path_walk_info *info)
free(commit_list);
trace2_region_enter("path-walk", "path-walk", info->revs->repo);
- while (!ret && ctx.path_stack.nr) {
- char *path = prio_queue_get(&ctx.path_stack);
+ while (!ret && (path = prio_queue_get(&ctx.path_stack))) {
paths_nr++;
ret = walk_path(&ctx, path);
@@ -821,12 +821,12 @@ int walk_objects_by_path(struct path_walk_info *info)
if (!strmap_empty(&ctx.paths_to_lists)) {
struct hashmap_iter iter;
struct strmap_entry *entry;
+ char *path;
strmap_for_each_entry(&ctx.paths_to_lists, &iter, entry)
push_to_stack(&ctx, entry->key);
- while (!ret && ctx.path_stack.nr) {
- char *path = prio_queue_get(&ctx.path_stack);
+ while (!ret && (path = prio_queue_get(&ctx.path_stack))) {
paths_nr++;
ret = walk_path(&ctx, path);
diff --git a/prio-queue.c b/prio-queue.c
index 9748528ce6..ead4faf4bb 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -22,16 +22,16 @@ void prio_queue_reverse(struct prio_queue *queue)
if (queue->compare)
BUG("prio_queue_reverse() on non-LIFO queue");
- if (!queue->nr)
+ if (!queue->nr_)
return;
- for (i = 0; i < (j = (queue->nr - 1) - i); i++)
+ for (i = 0; i < (j = (queue->nr_ - 1) - i); i++)
swap(queue, i, j);
}
void clear_prio_queue(struct prio_queue *queue)
{
FREE_AND_NULL(queue->array);
- queue->nr = 0;
+ queue->nr_ = 0;
queue->alloc = 0;
queue->insertion_ctr = 0;
}
@@ -41,15 +41,15 @@ void prio_queue_put(struct prio_queue *queue, void *thing)
size_t ix, parent;
/* Append at the end */
- ALLOC_GROW(queue->array, queue->nr + 1, queue->alloc);
- queue->array[queue->nr].ctr = queue->insertion_ctr++;
- queue->array[queue->nr].data = thing;
- queue->nr++;
+ ALLOC_GROW(queue->array, queue->nr_ + 1, queue->alloc);
+ queue->array[queue->nr_].ctr = queue->insertion_ctr++;
+ queue->array[queue->nr_].data = thing;
+ queue->nr_++;
if (!queue->compare)
return; /* LIFO */
/* Bubble up the new one */
- for (ix = queue->nr - 1; ix; ix = parent) {
+ for (ix = queue->nr_ - 1; ix; ix = parent) {
parent = (ix - 1) / 2;
if (compare(queue, parent, ix) <= 0)
break;
@@ -63,9 +63,9 @@ static void sift_down_root(struct prio_queue *queue)
size_t ix, child;
/* Push down the one at the root */
- for (ix = 0; ix * 2 + 1 < queue->nr; ix = child) {
+ for (ix = 0; ix * 2 + 1 < queue->nr_; ix = child) {
child = ix * 2 + 1; /* left */
- if (child + 1 < queue->nr &&
+ if (child + 1 < queue->nr_ &&
compare(queue, child, child + 1) >= 0)
child++; /* use right child */
@@ -80,36 +80,36 @@ void *prio_queue_get(struct prio_queue *queue)
{
void *result;
- if (!queue->nr)
+ if (!queue->nr_)
return NULL;
if (!queue->compare)
- return queue->array[--queue->nr].data; /* LIFO */
+ return queue->array[--queue->nr_].data; /* LIFO */
result = queue->array[0].data;
- if (!--queue->nr)
+ if (!--queue->nr_)
return result;
- queue->array[0] = queue->array[queue->nr];
+ queue->array[0] = queue->array[queue->nr_];
sift_down_root(queue);
return result;
}
void *prio_queue_peek(struct prio_queue *queue)
{
- if (!queue->nr)
+ if (!queue->nr_)
return NULL;
if (!queue->compare)
- return queue->array[queue->nr - 1].data;
+ return queue->array[queue->nr_ - 1].data;
return queue->array[0].data;
}
void prio_queue_replace(struct prio_queue *queue, void *thing)
{
- if (!queue->nr) {
+ if (!queue->nr_) {
prio_queue_put(queue, thing);
} else if (!queue->compare) {
- queue->array[queue->nr - 1].ctr = queue->insertion_ctr++;
- queue->array[queue->nr - 1].data = thing;
+ queue->array[queue->nr_ - 1].ctr = queue->insertion_ctr++;
+ queue->array[queue->nr_ - 1].data = thing;
} else {
queue->array[0].ctr = queue->insertion_ctr++;
queue->array[0].data = thing;
diff --git a/prio-queue.h b/prio-queue.h
index da7fad2f1f..7f2aa986b1 100644
--- a/prio-queue.h
+++ b/prio-queue.h
@@ -30,7 +30,7 @@ struct prio_queue {
prio_queue_compare_fn compare;
size_t insertion_ctr;
void *cb_data;
- size_t alloc, nr;
+ size_t alloc, nr_;
struct prio_queue_entry *array;
};
@@ -52,6 +52,16 @@ void *prio_queue_get(struct prio_queue *);
*/
void *prio_queue_peek(struct prio_queue *);
+static inline size_t prio_queue_size(const struct prio_queue *queue)
+{
+ return queue->nr_;
+}
+
+#define prio_queue_for_each(queue, it) \
+ for (size_t pq_ix_ = 0; \
+ pq_ix_ < (queue)->nr_ && ((it) = (queue)->array[pq_ix_].data, 1); \
+ pq_ix_++)
+
/*
* Replace the "thing" that compares the smallest with a new "thing",
* like prio_queue_get()+prio_queue_put() would do, but in a more
diff --git a/revision.c b/revision.c
index 5693618be4..34e2d146f4 100644
--- a/revision.c
+++ b/revision.c
@@ -476,16 +476,15 @@ static struct commit *handle_commit(struct rev_info *revs,
static int everybody_uninteresting(struct prio_queue *orig,
struct commit **interesting_cache)
{
- size_t i;
+ struct commit *commit;
if (*interesting_cache) {
- struct commit *commit = *interesting_cache;
+ commit = *interesting_cache;
if (!(commit->object.flags & UNINTERESTING))
return 0;
}
- for (i = 0; i < orig->nr; i++) {
- struct commit *commit = orig->array[i].data;
+ prio_queue_for_each(orig, commit) {
if (commit->object.flags & UNINTERESTING)
continue;
@@ -1446,7 +1445,7 @@ static int limit_list(struct rev_info *revs)
struct commit_list *original_list = revs->commits;
struct commit_list *newlist = NULL;
struct commit_list **p = &newlist;
- struct commit *interesting_cache = NULL;
+ struct commit *commit, *interesting_cache = NULL;
struct prio_queue queue = { .compare = compare_commits_by_commit_date };
if (revs->ancestry_path_implicit_bottoms) {
@@ -1461,8 +1460,7 @@ static int limit_list(struct rev_info *revs)
prio_queue_put(&queue, commit);
}
- while (queue.nr) {
- struct commit *commit = prio_queue_get(&queue);
+ while ((commit = prio_queue_get(&queue))) {
struct object *obj = &commit->object;
if (commit == interesting_cache)
@@ -4028,8 +4026,8 @@ static enum rewrite_result rewrite_one_1(struct rev_info *revs,
static void merge_queue_into_list(struct prio_queue *q, struct commit_list **list)
{
- while (q->nr) {
- struct commit *item = prio_queue_peek(q);
+ struct commit *item;
+ while ((item = prio_queue_peek(q))) {
struct commit_list *p = *list;
if (p && p->item->date >= item->date)
diff --git a/walker.c b/walker.c
index e98eb6da53..e3de77f092 100644
--- a/walker.c
+++ b/walker.c
@@ -84,12 +84,12 @@ static struct prio_queue complete = { compare_commits_by_commit_date };
static int process_commit(struct walker *walker, struct commit *commit)
{
struct commit_list *parents;
+ struct commit *item;
if (repo_parse_commit(the_repository, commit))
return -1;
- while (complete.nr) {
- struct commit *item = prio_queue_peek(&complete);
+ while ((item = prio_queue_peek(&complete))) {
if (item->date < commit->date)
break;
pop_most_recent_commit(&complete, COMPLETE);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 2/2] prio-queue: fold lazy_queue into prio_queue for automatic get+put fusion
From: Kristofer Karlsson via GitGitGadget @ 2026-06-08 19:10 UTC (permalink / raw)
To: git
Cc: René Scharfe, Kristofer Karlsson, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2140.v4.git.1780945851.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
Defer the actual removal in prio_queue_get() until the next
operation. If that next operation is a prio_queue_put(), the
removal and insertion are fused into a single replace — writing
the new element at the root and sifting it down — which avoids
a full remove-rebalance-insert cycle.
This matches the dominant usage pattern in git's commit traversal:
get a commit, then put its parents. The first parent insertion
after each get is now a replace operation automatically.
This generalizes the lazy_queue pattern from builtin/describe.c
(introduced in 08bb69d70f) into prio_queue itself. Three callers
independently implemented the same get+put fusion:
- builtin/describe.c had a full lazy_queue wrapper
- commit.c:pop_most_recent_commit() used peek+replace
- builtin/show-branch.c:join_revs() used peek+replace
All three now collapse to plain _get() and _put(), with the data
structure handling the fusion internally. This simplifies callers
and means every prio_queue user gets the optimization for free
without needing to implement it manually.
Remove prio_queue_replace() since no external callers remain.
Benchmarked on a 1.8M-commit monorepo (30 interleaved runs,
paired t-test, Xeon @ 2.20GHz):
Code paths that previously did eager get+put (new optimization):
Command base patched change p
merge-base --all A A~1000 3828ms 3725ms -2.69% 0.0001
rev-list --count A~1000..A 3055ms 2986ms -2.27% 0.0601
log --oneline A~1000..A 3408ms 3350ms -1.71% 0.0482
Code paths that already had manual get+put fusion (expect
neutral — the optimization moves into prio_queue but the number
of heap operations stays the same):
Command base patched change p
show-branch A A~1000 9156ms 9127ms -0.32% 0.3470
describe (4751 revs, 81K repo) 1983ms 1963ms -1.02% <0.001
No regressions in any scenario.
Suggested-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
builtin/describe.c | 75 +++++++-----------------------
builtin/show-branch.c | 11 ++---
commit.c | 11 +----
prio-queue.c | 92 ++++++++++++++++++++-----------------
prio-queue.h | 15 ++----
t/unit-tests/u-prio-queue.c | 6 +--
6 files changed, 78 insertions(+), 132 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 8e88bdeea6..64424543ef 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -251,61 +251,19 @@ static int compare_pt(const void *a_, const void *b_)
return 0;
}
-struct lazy_queue {
- struct prio_queue queue;
- bool get_pending;
-};
-
-#define LAZY_QUEUE_INIT { { compare_commits_by_commit_date }, false }
-
-static void *lazy_queue_get(struct lazy_queue *queue)
-{
- if (queue->get_pending)
- prio_queue_get(&queue->queue);
- else
- queue->get_pending = true;
- return prio_queue_peek(&queue->queue);
-}
-
-static void lazy_queue_put(struct lazy_queue *queue, void *thing)
-{
- if (queue->get_pending)
- prio_queue_replace(&queue->queue, thing);
- else
- prio_queue_put(&queue->queue, thing);
- queue->get_pending = false;
-}
-
-static bool lazy_queue_empty(const struct lazy_queue *queue)
-{
- return prio_queue_size(&queue->queue) == (queue->get_pending ? 1 : 0);
-}
-
-static void lazy_queue_clear(struct lazy_queue *queue)
-{
- clear_prio_queue(&queue->queue);
- queue->get_pending = false;
-}
-
-static unsigned long finish_depth_computation(struct lazy_queue *queue,
+static unsigned long finish_depth_computation(struct prio_queue *queue,
struct possible_tag *best)
{
unsigned long seen_commits = 0;
struct oidset unflagged = OIDSET_INIT;
- struct commit *commit;
- int skip = queue->get_pending ? 1 : 0;
+ struct commit *c;
- prio_queue_for_each(&queue->queue, commit) {
- if (skip) {
- skip = 0;
- continue;
- }
- if (!(commit->object.flags & best->flag_within))
- oidset_insert(&unflagged, &commit->object.oid);
+ prio_queue_for_each(queue, c) {
+ if (!(c->object.flags & best->flag_within))
+ oidset_insert(&unflagged, &c->object.oid);
}
- while (!lazy_queue_empty(queue)) {
- struct commit *c = lazy_queue_get(queue);
+ while ((c = prio_queue_get(queue))) {
struct commit_list *parents = c->parents;
seen_commits++;
if (c->object.flags & best->flag_within) {
@@ -321,7 +279,7 @@ static unsigned long finish_depth_computation(struct lazy_queue *queue,
repo_parse_commit(the_repository, p);
seen = p->object.flags & SEEN;
if (!seen)
- lazy_queue_put(queue, p);
+ prio_queue_put(queue, p);
flag_before = p->object.flags & best->flag_within;
p->object.flags |= c->object.flags;
flag_after = p->object.flags & best->flag_within;
@@ -369,8 +327,8 @@ static void append_suffix(int depth, const struct object_id *oid, struct strbuf
static void describe_commit(struct commit *cmit, struct strbuf *dst)
{
- struct commit *gave_up_on = NULL;
- struct lazy_queue queue = LAZY_QUEUE_INIT;
+ struct commit *c, *gave_up_on = NULL;
+ struct prio_queue queue = { compare_commits_by_commit_date };
struct commit_name *n;
struct possible_tag all_matches[MAX_TAGS];
unsigned int match_cnt = 0, annotated_cnt = 0, cur_match;
@@ -412,9 +370,8 @@ static void describe_commit(struct commit *cmit, struct strbuf *dst)
}
cmit->object.flags = SEEN;
- lazy_queue_put(&queue, cmit);
- while (!lazy_queue_empty(&queue)) {
- struct commit *c = lazy_queue_get(&queue);
+ prio_queue_put(&queue, cmit);
+ while ((c = prio_queue_get(&queue))) {
struct commit_list *parents = c->parents;
struct commit_name **slot;
@@ -448,7 +405,7 @@ static void describe_commit(struct commit *cmit, struct strbuf *dst)
t->depth++;
}
/* Stop if last remaining path already covered by best candidate(s) */
- if (annotated_cnt && lazy_queue_empty(&queue)) {
+ if (annotated_cnt && !prio_queue_size(&queue)) {
int best_depth = INT_MAX;
unsigned best_within = 0;
for (cur_match = 0; cur_match < match_cnt; cur_match++) {
@@ -471,7 +428,7 @@ static void describe_commit(struct commit *cmit, struct strbuf *dst)
struct commit *p = parents->item;
repo_parse_commit(the_repository, p);
if (!(p->object.flags & SEEN))
- lazy_queue_put(&queue, p);
+ prio_queue_put(&queue, p);
p->object.flags |= c->object.flags;
parents = parents->next;
@@ -486,7 +443,7 @@ static void describe_commit(struct commit *cmit, struct strbuf *dst)
strbuf_add_unique_abbrev(dst, cmit_oid, abbrev);
if (suffix)
strbuf_addstr(dst, suffix);
- lazy_queue_clear(&queue);
+ clear_prio_queue(&queue);
return;
}
if (unannotated_cnt)
@@ -502,11 +459,11 @@ static void describe_commit(struct commit *cmit, struct strbuf *dst)
QSORT(all_matches, match_cnt, compare_pt);
if (gave_up_on) {
- lazy_queue_put(&queue, gave_up_on);
+ prio_queue_put(&queue, gave_up_on);
seen_commits--;
}
seen_commits += finish_depth_computation(&queue, &all_matches[0]);
- lazy_queue_clear(&queue);
+ clear_prio_queue(&queue);
if (debug) {
static int label_width = -1;
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 8846f2376f..2435e8aeda 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -232,12 +232,13 @@ static void join_revs(struct prio_queue *queue,
while ((commit = prio_queue_peek(queue))) {
struct commit_list *parents;
int still_interesting = !!interesting(queue);
- bool get_pending = true;
int flags = commit->object.flags & all_mask;
if (!still_interesting && extra <= 0)
break;
+ prio_queue_get(queue);
+
mark_seen(commit, seen_p);
if ((flags & all_revs) == all_revs)
flags |= UNINTERESTING;
@@ -253,14 +254,8 @@ static void join_revs(struct prio_queue *queue,
if (mark_seen(p, seen_p) && !still_interesting)
extra--;
p->object.flags |= flags;
- if (get_pending)
- prio_queue_replace(queue, p);
- else
- prio_queue_put(queue, p);
- get_pending = false;
+ prio_queue_put(queue, p);
}
- if (get_pending)
- prio_queue_get(queue);
}
/*
diff --git a/commit.c b/commit.c
index fd8723502e..976bfc4618 100644
--- a/commit.c
+++ b/commit.c
@@ -795,24 +795,17 @@ void commit_list_sort_by_date(struct commit_list **list)
struct commit *pop_most_recent_commit(struct prio_queue *queue,
unsigned int mark)
{
- struct commit *ret = prio_queue_peek(queue);
- int get_pending = 1;
+ struct commit *ret = prio_queue_get(queue);
struct commit_list *parents = ret->parents;
while (parents) {
struct commit *commit = parents->item;
if (!repo_parse_commit(the_repository, commit) && !(commit->object.flags & mark)) {
commit->object.flags |= mark;
- if (get_pending)
- prio_queue_replace(queue, commit);
- else
- prio_queue_put(queue, commit);
- get_pending = 0;
+ prio_queue_put(queue, commit);
}
parents = parents->next;
}
- if (get_pending)
- prio_queue_get(queue);
return ret;
}
diff --git a/prio-queue.c b/prio-queue.c
index ead4faf4bb..199775d5af 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -34,12 +34,48 @@ void clear_prio_queue(struct prio_queue *queue)
queue->nr_ = 0;
queue->alloc = 0;
queue->insertion_ctr = 0;
+ queue->get_pending = 0;
+}
+
+static void sift_down_root(struct prio_queue *queue)
+{
+ size_t ix, child;
+
+ /* Push down the one at the root */
+ for (ix = 0; ix * 2 + 1 < queue->nr_; ix = child) {
+ child = ix * 2 + 1; /* left */
+ if (child + 1 < queue->nr_ &&
+ compare(queue, child, child + 1) >= 0)
+ child++; /* use right child */
+
+ if (compare(queue, ix, child) <= 0)
+ break;
+
+ swap(queue, child, ix);
+ }
+}
+
+static inline void flush_get(struct prio_queue *queue)
+{
+ if (!queue->get_pending)
+ return;
+ queue->get_pending = 0;
+ queue->array[0] = queue->array[--queue->nr_];
+ sift_down_root(queue);
}
void prio_queue_put(struct prio_queue *queue, void *thing)
{
size_t ix, parent;
+ if (queue->get_pending) {
+ queue->get_pending = 0;
+ queue->array[0].ctr = queue->insertion_ctr++;
+ queue->array[0].data = thing;
+ sift_down_root(queue);
+ return;
+ }
+
/* Append at the end */
ALLOC_GROW(queue->array, queue->nr_ + 1, queue->alloc);
queue->array[queue->nr_].ctr = queue->insertion_ctr++;
@@ -58,61 +94,33 @@ void prio_queue_put(struct prio_queue *queue, void *thing)
}
}
-static void sift_down_root(struct prio_queue *queue)
-{
- size_t ix, child;
-
- /* Push down the one at the root */
- for (ix = 0; ix * 2 + 1 < queue->nr_; ix = child) {
- child = ix * 2 + 1; /* left */
- if (child + 1 < queue->nr_ &&
- compare(queue, child, child + 1) >= 0)
- child++; /* use right child */
-
- if (compare(queue, ix, child) <= 0)
- break;
-
- swap(queue, child, ix);
- }
-}
-
void *prio_queue_get(struct prio_queue *queue)
{
- void *result;
-
- if (!queue->nr_)
+ if (queue->nr_ <= queue->get_pending) {
+ queue->nr_ = 0;
+ queue->get_pending = 0;
return NULL;
+ }
if (!queue->compare)
return queue->array[--queue->nr_].data; /* LIFO */
- result = queue->array[0].data;
- if (!--queue->nr_)
- return result;
+ flush_get(queue);
- queue->array[0] = queue->array[queue->nr_];
- sift_down_root(queue);
- return result;
+ queue->get_pending = 1;
+ return queue->array[0].data;
}
void *prio_queue_peek(struct prio_queue *queue)
{
- if (!queue->nr_)
+ if (queue->nr_ <= queue->get_pending) {
+ queue->nr_ = 0;
+ queue->get_pending = 0;
return NULL;
+ }
if (!queue->compare)
return queue->array[queue->nr_ - 1].data;
- return queue->array[0].data;
-}
-void prio_queue_replace(struct prio_queue *queue, void *thing)
-{
- if (!queue->nr_) {
- prio_queue_put(queue, thing);
- } else if (!queue->compare) {
- queue->array[queue->nr_ - 1].ctr = queue->insertion_ctr++;
- queue->array[queue->nr_ - 1].data = thing;
- } else {
- queue->array[0].ctr = queue->insertion_ctr++;
- queue->array[0].data = thing;
- sift_down_root(queue);
- }
+ flush_get(queue);
+
+ return queue->array[0].data;
}
diff --git a/prio-queue.h b/prio-queue.h
index 7f2aa986b1..570b48e648 100644
--- a/prio-queue.h
+++ b/prio-queue.h
@@ -30,8 +30,9 @@ struct prio_queue {
prio_queue_compare_fn compare;
size_t insertion_ctr;
void *cb_data;
- size_t alloc, nr_;
+ size_t alloc, nr_; /* use prio_queue_size() for logical count */
struct prio_queue_entry *array;
+ unsigned get_pending;
};
/*
@@ -54,22 +55,14 @@ void *prio_queue_peek(struct prio_queue *);
static inline size_t prio_queue_size(const struct prio_queue *queue)
{
- return queue->nr_;
+ return queue->nr_ - queue->get_pending;
}
#define prio_queue_for_each(queue, it) \
- for (size_t pq_ix_ = 0; \
+ for (size_t pq_ix_ = (queue)->get_pending; \
pq_ix_ < (queue)->nr_ && ((it) = (queue)->array[pq_ix_].data, 1); \
pq_ix_++)
-/*
- * Replace the "thing" that compares the smallest with a new "thing",
- * like prio_queue_get()+prio_queue_put() would do, but in a more
- * efficient way. Does the same as prio_queue_put() if the queue is
- * empty.
- */
-void prio_queue_replace(struct prio_queue *queue, void *thing);
-
void clear_prio_queue(struct prio_queue *);
/* Reverse the LIFO elements */
diff --git a/t/unit-tests/u-prio-queue.c b/t/unit-tests/u-prio-queue.c
index 63e58114ae..af3e0b8598 100644
--- a/t/unit-tests/u-prio-queue.c
+++ b/t/unit-tests/u-prio-queue.c
@@ -53,13 +53,13 @@ static void test_prio_queue(int *input, size_t input_size,
prio_queue_reverse(&pq);
break;
case REPLACE:
- peek = prio_queue_peek(&pq);
+ get = prio_queue_get(&pq);
cl_assert(i + 1 < input_size);
cl_assert(input[i + 1] >= 0);
cl_assert(j < result_size);
- cl_assert_equal_i(result[j], show(peek));
+ cl_assert_equal_i(result[j], show(get));
j++;
- prio_queue_replace(&pq, &input[++i]);
+ prio_queue_put(&pq, &input[++i]);
break;
default:
prio_queue_put(&pq, &input[i]);
--
gitgitgadget
^ 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