* Re: [PATCH v3 00/12] Improve git gui operation without a worktree
From: Johannes Sixt @ 2026-06-02 21:05 UTC (permalink / raw)
To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <6f4276f9-cf0e-4840-88ce-9e0009c669a9@gmail.com>
Am 02.06.26 um 20:54 schrieb Mark Levedahl:
> You have duplicate signoffs on 02/12 in your repo.
That's intentional. The patch went from my hands to yours, then back to
mine.
-- Hannes
^ permalink raw reply
* [PATCH] worktree: record creation time and free-form note
From: Kiesel, Norbert @ 2026-06-02 21:40 UTC (permalink / raw)
To: git
From 130cd5e4a25e6672b2a97268e1100b6ef03fa552 Mon Sep 17 00:00:00 2001
From: Norbert Kiesel <norbert.kiesel@creditkarma.com>
Date: Mon, 1 Jun 2026 17:03:39 -0700
Subject: [PATCH] worktree: record creation time and free-form note
Add per-worktree metadata so users can answer "what is this worktree
for, and when did I make it?" without resorting to external notes.
When `git worktree add` creates a linked worktree, it now writes a
`created` file containing the unix timestamp. A new `--note <string>`
option to `add`, and a new `git worktree annotate <worktree> [<note>]`
subcommand, store an optional free-form description in a `note` file
next to the other administrative files. Passing `annotate` without a
note clears it. The main worktree carries no metadata and cannot be
annotated.
`git worktree list` learns `--show-created` and `--show-note` for
human-readable output, and `--sort=<key>` (path or created, optionally
prefixed with `-` to reverse) for ordering linked worktrees; the main
worktree always stays first. Worktrees without a recorded timestamp
(those created before this change) display as `created: unknown` and
sort after timestamped ones. Porcelain output unconditionally emits
`created` and `note` lines when the corresponding metadata is present.
Tests cover add/annotate/list behaviour and the legacy-worktree case.
The two existing porcelain assertions in t2402 are taught to strip the
new `created` line so they continue to pass.
Signed-off-by: Norbert Kiesel <norbert.kiesel@creditkarma.com>
---
Documentation/git-worktree.adoc | 61 ++++++++++++-
builtin/worktree.c | 152 +++++++++++++++++++++++++++++++-
t/meson.build | 1 +
t/t2402-worktree-list.sh | 10 ++-
t/t2410-worktree-metadata.sh | 143 ++++++++++++++++++++++++++++++
worktree.c | 78 ++++++++++++++++
worktree.h | 23 +++++
7 files changed, 459 insertions(+), 9 deletions(-)
create mode 100755 t/t2410-worktree-metadata.sh
diff --git a/Documentation/git-worktree.adoc b/Documentation/git-worktree.adoc
index fbf8426cd9..200f3d7772 100644
--- a/Documentation/git-worktree.adoc
+++ b/Documentation/git-worktree.adoc
@@ -10,8 +10,11 @@ SYNOPSIS
--------
[synopsis]
git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]
+ [--note <string>]
[--orphan] [(-b | -B) <new-branch>] <path> [<commit-ish>]
-git worktree list [-v | --porcelain [-z]]
+git worktree annotate <worktree> [<note>]
+git worktree list [-v | --porcelain [-z]] [--show-created] [--show-note]
+ [--sort=<key>]
git worktree lock [--reason <string>] <worktree>
git worktree move <worktree> <new-path>
git worktree prune [-n] [-v] [--expire <expire>]
@@ -106,6 +109,15 @@ 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`).
+`annotate <worktree> [<note>]`::
+
+Set, replace, or clear a free-form note (description) on a linked worktree.
+Useful for recording what a worktree was created for so it can be identified
+later. With _<note>_, the worktree's note is set or replaced; without a note
+argument, the existing note is cleared. The note for a worktree may also be
+set at creation time with `git worktree add --note <note>`. The main
+worktree cannot be annotated.
+
`list`::
List details of each worktree. The main worktree is listed first,
@@ -114,6 +126,20 @@ 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-note` to include any user-provided note in human output. In
+`--porcelain` output, both `created` and `note` lines are emitted whenever
+present. Use `--sort=<key>` (where _<key>_ is `path` or `created`,
+optionally prefixed with `-` to reverse) to order the linked worktrees;
+the main worktree always remains first.
`lock`::
@@ -286,6 +312,32 @@ _<time>_.
With `lock` or with `add --lock`, an explanation why the worktree
is locked.
+`--note <string>`::
+ With `add`, attach a free-form note (description) to the new worktree.
+ The note is stored alongside the worktree's administrative files and
+ can be displayed with `git worktree list --show-note` or in
+ `--porcelain` output. It can be changed later with
+ `git worktree annotate`.
+
+`--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-note`::
+ With `list`, include each worktree's note (if set) in the
+ human-readable output. In `--porcelain` output, the note is always
+ included (when set) on a `note` line.
+
+`--sort=<key>`::
+ With `list`, sort linked worktrees by _<key>_, which is one of
+ `path` or `created`. Prefix with `-` to reverse the order, e.g.
+ `--sort=-created` lists newest first. The main worktree is always
+ listed first regardless of sort order. Worktrees with no recorded
+ creation timestamp sort after those that have one (or before, when
+ reversed).
+
_<worktree>_::
Worktrees can be identified by path, either relative or absolute.
+
@@ -462,7 +514,9 @@ 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` and `note`) 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 +528,13 @@ bare
worktree /path/to/linked-worktree
HEAD abcd1234abcd1234abcd1234abcd1234abcd1234
branch refs/heads/master
+created 2026-06-01T12:34:56Z
+note investigating login bug
worktree /path/to/other-linked-worktree
HEAD 1234abc1234abc1234abc1234abc1234abc1234a
detached
+created 2026-05-28T08:15:00Z
worktree /path/to/linked-worktree-locked-no-reason
HEAD 5678abc5678abc5678abc5678abc5678abc5678c
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d21c43fde3..ac22277d6c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -27,13 +27,16 @@
#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" \
+ " [--note <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]
[--show-note]\n" \
+ " [--sort=<key>]")
#define BUILTIN_WORKTREE_LOCK_USAGE \
N_("git worktree lock [--reason <string>] <worktree>")
#define BUILTIN_WORKTREE_MOVE_USAGE \
@@ -46,6 +49,8 @@
N_("git worktree repair [<path>...]")
#define BUILTIN_WORKTREE_UNLOCK_USAGE \
N_("git worktree unlock <worktree>")
+#define BUILTIN_WORKTREE_ANNOTATE_USAGE \
+ N_("git worktree annotate <worktree> [<note>]")
#define WORKTREE_ADD_DWIM_ORPHAN_INFER_TEXT \
_("No possible source branch, inferring '--orphan'")
@@ -66,6 +71,7 @@
static const char * const git_worktree_usage[] = {
BUILTIN_WORKTREE_ADD_USAGE,
+ BUILTIN_WORKTREE_ANNOTATE_USAGE,
BUILTIN_WORKTREE_LIST_USAGE,
BUILTIN_WORKTREE_LOCK_USAGE,
BUILTIN_WORKTREE_MOVE_USAGE,
@@ -116,6 +122,11 @@ static const char * const git_worktree_unlock_usage[] = {
NULL
};
+static const char * const git_worktree_annotate_usage[] = {
+ BUILTIN_WORKTREE_ANNOTATE_USAGE,
+ NULL
+};
+
struct add_opts {
int force;
int detach;
@@ -124,6 +135,7 @@ struct add_opts {
int orphan;
int relative_paths;
const char *keep_locked;
+ const char *note;
};
static int show_only;
@@ -131,6 +143,8 @@ static int verbose;
static int guess_remote;
static int use_relative_paths;
static timestamp_t expire;
+static int show_created;
+static int show_note;
static int git_worktree_config(const char *var, const char *value,
const struct config_context *ctx, void *cb)
@@ -544,6 +558,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->note && *opts->note) {
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%s/note", sb_repo.buf);
+ write_file(sb.buf, "%s", opts->note);
+ }
+
/*
* Set up the ref store of the worktree and create the HEAD reference.
*/
@@ -815,6 +839,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, "note", &opts.note, N_("string"),
+ N_("attach a free-form note/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 +989,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 *note;
+ timestamp_t created;
printf("worktree %s%c", wt->path, line_terminator);
if (wt->is_bare)
@@ -975,6 +1003,18 @@ 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);
+
+ note = worktree_note(wt);
+ if (note && *note) {
+ fputs("note ", stdout);
+ write_name_quoted(note, stdout, line_terminator);
+ }
+
reason = worktree_lock_reason(wt);
if (reason) {
fputs("locked", stdout);
@@ -1034,6 +1074,21 @@ static void show_worktree(struct worktree *wt,
struct worktree_display *display,
else if (reason)
strbuf_addstr(&sb, " prunable");
+ if (show_created || verbose) {
+ timestamp_t created = worktree_created_at(wt);
+ if (created)
+ strbuf_addf(&sb, "\n\tcreated: %s",
+ show_date(created, 0, DATE_MODE(ISO8601)));
+ else if (show_created && !is_main_worktree(wt))
+ strbuf_addstr(&sb, "\n\tcreated: unknown");
+ }
+
+ if (show_note || verbose) {
+ const char *note = worktree_note(wt);
+ if (note && *note)
+ strbuf_addf(&sb, "\n\tnote: %s", note);
+ }
+
printf("%s\n", sb.buf);
strbuf_release(&sb);
}
@@ -1068,6 +1123,27 @@ 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 void pathsort(struct worktree **wt)
{
int n = 0;
@@ -1078,11 +1154,43 @@ 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
+ 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 +1199,12 @@ 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-note", &show_note,
+ N_("show worktree notes")),
+ OPT_STRING(0, "sort", &sort_key, N_("key"),
+ N_("sort worktrees by key (path, created); prefix with - to reverse")),
OPT_END()
};
@@ -1107,8 +1221,13 @@ 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) {
+ 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 +1319,32 @@ static int unlock_worktree(int ac, const char
**av, const char *prefix,
return ret;
}
+static int annotate_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_annotate_usage, 0);
+ if (ac < 1 || ac > 2)
+ usage_with_options(git_worktree_annotate_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 annotated"));
+
+ ret = set_worktree_note(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 +1614,7 @@ int cmd_worktree(int ac,
parse_opt_subcommand_fn *fn = NULL;
struct option options[] = {
OPT_SUBCOMMAND("add", &fn, add),
+ OPT_SUBCOMMAND("annotate", &fn, annotate_worktree),
OPT_SUBCOMMAND("prune", &fn, prune),
OPT_SUBCOMMAND("list", &fn, list),
OPT_SUBCOMMAND("lock", &fn, lock_worktree),
diff --git a/t/meson.build b/t/meson.build
index 2af8d01279..7b6e8435d7 100644
--- a/t/meson.build
+++ b/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 a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index e0c6abd2f5..8422340443 100755
--- a/t/t2402-worktree-list.sh
+++ b/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 -v "^created " 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 -v "^created " | 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 -v "^created " actual.raw >actual &&
test_cmp expect actual
'
diff --git a/t/t2410-worktree-metadata.sh b/t/t2410-worktree-metadata.sh
new file mode 100755
index 0000000000..3f8b508593
--- /dev/null
+++ b/t/t2410-worktree-metadata.sh
@@ -0,0 +1,143 @@
+#!/bin/sh
+
+test_description='git worktree creation timestamp and note 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 --note writes note file' '
+ test_when_finished "git worktree remove -f wt2 && git worktree prune" &&
+ git worktree add --note "investigating bug" wt2 &&
+ test_path_is_file .git/worktrees/wt2/note &&
+ echo "investigating bug" >expect &&
+ test_cmp expect .git/worktrees/wt2/note
+'
+
+test_expect_success 'add without --note does not create note file' '
+ test_when_finished "git worktree remove -f wt3 && git worktree prune" &&
+ git worktree add wt3 &&
+ test_path_is_missing .git/worktrees/wt3/note
+'
+
+test_expect_success 'annotate sets a note on an existing worktree' '
+ test_when_finished "git worktree remove -f wt4 && git worktree prune" &&
+ git worktree add wt4 &&
+ git worktree annotate wt4 "later note" &&
+ echo "later note" >expect &&
+ test_cmp expect .git/worktrees/wt4/note
+'
+
+test_expect_success 'annotate replaces an existing note' '
+ test_when_finished "git worktree remove -f wt5 && git worktree prune" &&
+ git worktree add --note "old" wt5 &&
+ git worktree annotate wt5 "new" &&
+ echo "new" >expect &&
+ test_cmp expect .git/worktrees/wt5/note
+'
+
+test_expect_success 'annotate with no text clears the note' '
+ test_when_finished "git worktree remove -f wt6 && git worktree prune" &&
+ git worktree add --note "to delete" wt6 &&
+ test_path_is_file .git/worktrees/wt6/note &&
+ git worktree annotate wt6 &&
+ test_path_is_missing .git/worktrees/wt6/note
+'
+
+test_expect_success 'annotate refuses to operate on the main worktree' '
+ test_must_fail git worktree annotate . "should fail" 2>err &&
+ grep -i "main working tree" err
+'
+
+test_expect_success 'list --show-note displays note in human output' '
+ test_when_finished "git worktree remove -f wt7 && git worktree prune" &&
+ git worktree add --note "release branch" wt7 &&
+ git worktree list --show-note >actual &&
+ grep "note: 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 --porcelain always includes created and note' '
+ test_when_finished "git worktree remove -f wtp && git worktree prune" &&
+ git worktree add --note "porcelain test" wtp &&
+ git worktree list --porcelain >actual &&
+ grep "^created " actual &&
+ grep "^note 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 with unknown key fails' '
+ test_must_fail git worktree list --sort=bogus 2>err &&
+ grep -i "unknown sort key" err
+'
+
+test_done
diff --git a/worktree.c b/worktree.c
index 97eddc3916..7989e694b7 100644
--- a/worktree.c
+++ b/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->note);
free(worktree);
}
@@ -324,6 +327,81 @@ 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;
+}
+
+const char *worktree_note(struct worktree *wt)
+{
+ if (is_main_worktree(wt))
+ return NULL;
+
+ if (!wt->note_valid) {
+ struct strbuf path = STRBUF_INIT;
+
+ strbuf_addstr(&path, worktree_git_path(wt, "note"));
+ if (file_exists(path.buf)) {
+ struct strbuf note = STRBUF_INIT;
+ if (strbuf_read_file(¬e, path.buf, 0) < 0)
+ die_errno(_("failed to read '%s'"), path.buf);
+ strbuf_trim_trailing_newline(¬e);
+ wt->note = strbuf_detach(¬e, NULL);
+ } else
+ wt->note = NULL;
+ wt->note_valid = 1;
+ strbuf_release(&path);
+ }
+
+ return wt->note;
+}
+
+int set_worktree_note(struct worktree *wt, const char *text)
+{
+ char *path;
+ int ret = 0;
+
+ if (is_main_worktree(wt))
+ return error(_("cannot set note on the main worktree"));
+
+ path = repo_common_path(wt->repo, "worktrees/%s/note", 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_note() re-reads */
+ FREE_AND_NULL(wt->note);
+ wt->note_valid = 0;
+
+ free(path);
+ return ret;
+}
+
const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
{
struct strbuf reason = STRBUF_INIT;
diff --git a/worktree.h b/worktree.h
index 1075409f9a..0fcdb8bd1b 100644
--- a/worktree.h
+++ b/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 *note; /* private - use worktree_note */
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 note_valid; /* private */
+ int created_at_valid; /* private */
};
/*
@@ -96,6 +100,25 @@ 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 user-supplied note/description for the given worktree, or NULL
+ * if none was set.
+ */
+const char *worktree_note(struct worktree *wt);
+
+/*
+ * Write or replace the worktree's note. Pass NULL or "" to delete the note.
+ * Returns 0 on success, -1 on failure. Not valid for the main worktree.
+ */
+int set_worktree_note(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
--
^ permalink raw reply related
* [PATCH v2 0/4] pack-objects: support bitmaps and delta-islands with `--path-walk`
From: Taylor Blau @ 2026-06-02 22:21 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Junio C Hamano, Jeff King, Elijah Newren
In-Reply-To: <cover.1779923907.git.me@ttaylorr.com>
Note to the maintainer:
* This series is based on 'ds/path-walk-filters' with Patrick's
'ps/clang-w-glibc-2.43-and-_Generic' merged in. The former has since
graduated. These are the three remaining patches from my earlier RFC
after Stolee's series incorporated the filter-related pieces.
Here is a very small reroll of my series to make `--path-walk` work with
reachability bitmaps and delta-islands.
Since the previous round, the only changes are:
* A new commit making some adjustments to p5311 to facilitate
performance testing bitmaps in repositories repacked with
'--path-walk'.
* Updates to (what is now) the second commit's message, including
performance results based on the aforementioned changes.
Outside of the above, the series is otherwise unchanged.
Thanks in advance for your review!
Taylor Blau (4):
t/perf: drop p5311's lookup-table permutation
pack-objects: support reachability bitmaps with `--path-walk`
pack-objects: extract `record_tree_depth()` helper
pack-objects: support `--delta-islands` with `--path-walk`
Documentation/git-pack-objects.adoc | 12 ++---
builtin/pack-objects.c | 68 +++++++++++++++++++++--------
t/perf/p5311-pack-bitmaps-fetch.sh | 20 +++++----
t/t5310-pack-bitmaps.sh | 36 +++++++++++++++
t/t5320-delta-islands.sh | 29 ++++++++++++
5 files changed, 134 insertions(+), 31 deletions(-)
Range-diff against v1:
-: ----------- > 1: 52d63e8910e t/perf: drop p5311's lookup-table permutation
1: 3fa8bfbfd59 ! 2: ffad584a43e pack-objects: support reachability bitmaps with `--path-walk`
@@ Commit message
bitmap can answer the request, use it; otherwise fall back to
path-walk's own enumeration.
+ As a result, we can see significantly reduced pack sizes from p5311
+ before this commit:
+
+ Test HEAD^ HEAD
+ ----------------------------------------------------------------------------------
+ 5311.38: server (1 days, --path-walk) 2.56(2.52+0.03) 0.01(0.01+0.00) -99.6%
+ 5311.39: size (1 days, --path-walk) 123.9K 123.9K +0.0%
+ 5311.40: client (1 days, --path-walk) 0.00(0.01+0.00) 0.00(0.00+0.00) =
+ 5311.42: server (2 days, --path-walk) 2.57(2.52+0.05) 0.01(0.01+0.00) -99.6%
+ 5311.43: size (2 days, --path-walk) 123.9K 123.9K +0.0%
+ 5311.44: client (2 days, --path-walk) 0.00(0.00+0.00) 0.00(0.00+0.00) =
+ 5311.46: server (4 days, --path-walk) 2.58(2.51+0.07) 0.01(0.01+0.00) -99.6%
+ 5311.47: size (4 days, --path-walk) 123.9K 123.9K +0.0%
+ 5311.48: client (4 days, --path-walk) 0.00(0.00+0.00) 0.00(0.00+0.00) =
+ 5311.50: server (8 days, --path-walk) 2.58(2.53+0.04) 0.02(0.02+0.00) -99.2%
+ 5311.51: size (8 days, --path-walk) 152.4K 152.4K +0.0%
+ 5311.52: client (8 days, --path-walk) 0.00(0.01+0.00) 0.00(0.01+0.00) =
+ 5311.54: server (16 days, --path-walk) 2.58(2.52+0.05) 0.03(0.02+0.00) -98.8%
+ 5311.55: size (16 days, --path-walk) 205.3K 205.3K +0.0%
+ 5311.56: client (16 days, --path-walk) 0.01(0.01+0.00) 0.01(0.01+0.00) +0.0%
+ 5311.58: server (32 days, --path-walk) 2.59(2.53+0.06) 0.03(0.03+0.00) -98.8%
+ 5311.59: size (32 days, --path-walk) 209.3K 209.3K +0.0%
+ 5311.60: client (32 days, --path-walk) 0.01(0.02+0.00) 0.01(0.02+0.00) +0.0%
+ 5311.62: server (64 days, --path-walk) 2.70(2.76+0.06) 0.16(0.24+0.04) -94.1%
+ 5311.63: size (64 days, --path-walk) 4.1M 4.1M +0.0%
+ 5311.64: client (64 days, --path-walk) 0.44(0.50+0.02) 0.44(0.51+0.02) +0.0%
+ 5311.66: server (128 days, --path-walk) 2.88(3.20+0.05) 0.34(0.65+0.05) -88.2%
+ 5311.67: size (128 days, --path-walk) 9.0M 9.0M -0.0%
+ 5311.68: client (128 days, --path-walk) 0.93(1.22+0.07) 0.93(1.20+0.08) +0.0%
+
+ We get the same size of output pack, but this commit allows us to do so
+ in a significantly shorter amount of time. Intuitively, we're generating
+ the same pack (hence the unchanged 'test_size' output from run to run),
+ but varying how we get there. Before this commit, pack-objects prefers
+ '--path-walk' to '--use-bitmap-index', so we generate the output pack by
+ performing a normal '--path-walk' traversal. With this commit, we are
+ operating over a *repacked* state (that itself was done with a
+ '--path-walk' traversal), but are able to perform pack-reuse on that
+ repacked state via bitmaps.
+
There is one wrinkle when it comes to '--boundary', which we must not
pass into the bitmap walk in the presence of both '--path-walk' and
'--use-bitmap-index'. Path-walk needs boundary commits when it performs
its own traversal, in order to discover bases for thin packs, but the
- bitmap traversal expects the usual non-boundary state. Work around this
- by setting `revs->boundary` as late as possible within
- `get_object_list_path_walk()`, after any bitmap attempt has either
- succeeded or declined to answer the request.
+ bitmap traversal does not expect this. Work around this by setting
+ `revs->boundary` as late as possible within the '--path-walk' traversal,
+ after any bitmap attempt has either succeeded or declined to answer the
+ request.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
@@ builtin/pack-objects.c: int cmd_pack_objects(int argc,
use_internal_rev_list = 1;
strvec_push(&rp, shallow
+ ## t/perf/p5311-pack-bitmaps-fetch.sh ##
+@@ t/perf/p5311-pack-bitmaps-fetch.sh: test_description='performance of fetches from bitmapped packs'
+ . ./perf-lib.sh
+
+ test_fetch_bitmaps () {
++ argv=$1
++ export argv
++
+ test_expect_success 'setup test directory' '
+ rm -fr * .git
+ '
+
+ test_perf_default_repo
+
+- test_expect_success 'create bitmapped server repo' '
++ test_expect_success "create bitmapped server repo ${argv:+($argv)}" '
+ git config pack.writebitmaps true &&
+- git repack -ad
++ git repack -ad $argv
+ '
+
+ # simulate a fetch from a repository that last fetched N days ago, for
+@@ t/perf/p5311-pack-bitmaps-fetch.sh: test_fetch_bitmaps () {
+ # and assume the first entry in the chain that is N days older than the current
+ # HEAD is where the HEAD would have been then.
+ for days in 1 2 4 8 16 32 64 128; do
+- title=$(printf '%10s' "($days days)")
++ title=$(printf '%10s' "($days days${argv:+, $argv})")
+ test_expect_success "setup revs from $days days ago" '
+ now=$(git log -1 --format=%ct HEAD) &&
+ then=$(($now - ($days * 86400))) &&
+@@ t/perf/p5311-pack-bitmaps-fetch.sh: test_fetch_bitmaps () {
+ done
+ }
+
+-test_fetch_bitmaps
++for argv in '' --path-walk
++do
++ test_fetch_bitmaps $argv || return 1
++done
+
+ test_done
+
## t/t5310-pack-bitmaps.sh ##
@@ t/t5310-pack-bitmaps.sh: test_bitmap_cases
2: bdae873eaab = 3: 069c50d3370 pack-objects: extract `record_tree_depth()` helper
3: a642305e3c9 = 4: ae57607b57f pack-objects: support `--delta-islands` with `--path-walk`
base-commit: 45a9ecee26839cc880fdd5e704339dd3cf4ffc26
--
2.54.0.23.gae57607b57f
^ permalink raw reply
* [PATCH v2 1/4] t/perf: drop p5311's lookup-table permutation
From: Taylor Blau @ 2026-06-02 22:21 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Junio C Hamano, Jeff King, Elijah Newren
In-Reply-To: <cover.1780438896.git.me@ttaylorr.com>
p5311 measures the cost of serving a fetch from a bitmapped pack and
indexing the resulting pack on the client. Since 761416ef91d
(bitmap-lookup-table: add performance tests for lookup table,
2022-08-14), p5311 effectively runs itself twice: once with the bitmap's
lookup table extension enabled, and again with it disabled.
This comparison has served its useful purpose, as the lookup table is
almost four years old, and the de-facto default in server-side Git
deployments.
A following commit will want to test a different combination (repacking
with and without '--path-walk' instead of the lookup table). Instead of
multiplying the current test count by two again to produce four
variations of `test_fetch_bitmaps()`, drop the lookup table option to
reduce the number of perf tests we run. Retain `test_fetch_bitmaps()`
itself, since we will use this in the future for the new
parameterization.
(As an aside, a future commit outside of this series will adjust the
default value of 'pack.writeBitmapLookupTable' to "true", matching the
de-facto norm for deployments where the existence of bitmap lookup
tables is meaningful. Punt on that to a later series and instead make
the minimal change for now.)
Suggested-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
t/perf/p5311-pack-bitmaps-fetch.sh | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/t/perf/p5311-pack-bitmaps-fetch.sh b/t/perf/p5311-pack-bitmaps-fetch.sh
index 047efb995d6..5bea5c64e7b 100755
--- a/t/perf/p5311-pack-bitmaps-fetch.sh
+++ b/t/perf/p5311-pack-bitmaps-fetch.sh
@@ -12,7 +12,6 @@ test_fetch_bitmaps () {
test_expect_success 'create bitmapped server repo' '
git config pack.writebitmaps true &&
- git config pack.writeBitmapLookupTable '"$1"' &&
git repack -ad
'
@@ -32,7 +31,7 @@ test_fetch_bitmaps () {
} >revs
'
- test_perf "server $title (lookup=$1)" '
+ test_perf "server $title" '
git pack-objects --stdout --revs \
--thin --delta-base-offset \
<revs >tmp.pack
@@ -42,13 +41,12 @@ test_fetch_bitmaps () {
test_file_size tmp.pack
'
- test_perf "client $title (lookup=$1)" '
+ test_perf "client $title" '
git index-pack --stdin --fix-thin <tmp.pack
'
done
}
-test_fetch_bitmaps true
-test_fetch_bitmaps false
+test_fetch_bitmaps
test_done
--
2.54.0.23.gae57607b57f
^ permalink raw reply related
* [PATCH v2 2/4] pack-objects: support reachability bitmaps with `--path-walk`
From: Taylor Blau @ 2026-06-02 22:21 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Junio C Hamano, Jeff King, Elijah Newren
In-Reply-To: <cover.1780438896.git.me@ttaylorr.com>
When 'pack-objects' is invoked with '--path-walk', it prevents us from
using reachability bitmaps.
This behavior dates back to 70664d2865c (pack-objects: add --path-walk
option, 2025-05-16), which included a comment in the relevant portion of
the command-line arguments handling that read as follows:
/*
* We must disable the bitmaps because we are removing
* the --objects / --objects-edge[-aggressive] options.
*/
In fb2c309b7d3 (pack-objects: pass --objects with --path-walk,
2026-05-02), path-walk learned to pass '--objects' again, but still
kept bitmap traversal disabled. That leaves two useful cases
unsupported:
* A path-walk repack that writes bitmaps does not give the bitmap
selector any commits, because path-walk reveals commits through
`add_objects_by_path()` rather than through `show_commit()`, where
`index_commit_for_bitmap()` is normally called.
* An invocation like "git pack-objects --use-bitmap-index --path-walk"
never tries an existing bitmap, even when one is available and could
answer the request.
Fortunately for us, neither restriction is required.
* On the writing side: teach the path-walk object callback to call
`index_commit_for_bitmap()` for commits that it adds to the pack.
That gives the bitmap selector the commit candidates it would have
seen from the regular traversal.
* For bitmap reading, keep passing '--objects' to the internal rev_list
machinery, but stop clearing `use_bitmap_index`. If an existing
bitmap can answer the request, use it; otherwise fall back to
path-walk's own enumeration.
As a result, we can see significantly reduced pack sizes from p5311
before this commit:
Test HEAD^ HEAD
----------------------------------------------------------------------------------
5311.38: server (1 days, --path-walk) 2.56(2.52+0.03) 0.01(0.01+0.00) -99.6%
5311.39: size (1 days, --path-walk) 123.9K 123.9K +0.0%
5311.40: client (1 days, --path-walk) 0.00(0.01+0.00) 0.00(0.00+0.00) =
5311.42: server (2 days, --path-walk) 2.57(2.52+0.05) 0.01(0.01+0.00) -99.6%
5311.43: size (2 days, --path-walk) 123.9K 123.9K +0.0%
5311.44: client (2 days, --path-walk) 0.00(0.00+0.00) 0.00(0.00+0.00) =
5311.46: server (4 days, --path-walk) 2.58(2.51+0.07) 0.01(0.01+0.00) -99.6%
5311.47: size (4 days, --path-walk) 123.9K 123.9K +0.0%
5311.48: client (4 days, --path-walk) 0.00(0.00+0.00) 0.00(0.00+0.00) =
5311.50: server (8 days, --path-walk) 2.58(2.53+0.04) 0.02(0.02+0.00) -99.2%
5311.51: size (8 days, --path-walk) 152.4K 152.4K +0.0%
5311.52: client (8 days, --path-walk) 0.00(0.01+0.00) 0.00(0.01+0.00) =
5311.54: server (16 days, --path-walk) 2.58(2.52+0.05) 0.03(0.02+0.00) -98.8%
5311.55: size (16 days, --path-walk) 205.3K 205.3K +0.0%
5311.56: client (16 days, --path-walk) 0.01(0.01+0.00) 0.01(0.01+0.00) +0.0%
5311.58: server (32 days, --path-walk) 2.59(2.53+0.06) 0.03(0.03+0.00) -98.8%
5311.59: size (32 days, --path-walk) 209.3K 209.3K +0.0%
5311.60: client (32 days, --path-walk) 0.01(0.02+0.00) 0.01(0.02+0.00) +0.0%
5311.62: server (64 days, --path-walk) 2.70(2.76+0.06) 0.16(0.24+0.04) -94.1%
5311.63: size (64 days, --path-walk) 4.1M 4.1M +0.0%
5311.64: client (64 days, --path-walk) 0.44(0.50+0.02) 0.44(0.51+0.02) +0.0%
5311.66: server (128 days, --path-walk) 2.88(3.20+0.05) 0.34(0.65+0.05) -88.2%
5311.67: size (128 days, --path-walk) 9.0M 9.0M -0.0%
5311.68: client (128 days, --path-walk) 0.93(1.22+0.07) 0.93(1.20+0.08) +0.0%
We get the same size of output pack, but this commit allows us to do so
in a significantly shorter amount of time. Intuitively, we're generating
the same pack (hence the unchanged 'test_size' output from run to run),
but varying how we get there. Before this commit, pack-objects prefers
'--path-walk' to '--use-bitmap-index', so we generate the output pack by
performing a normal '--path-walk' traversal. With this commit, we are
operating over a *repacked* state (that itself was done with a
'--path-walk' traversal), but are able to perform pack-reuse on that
repacked state via bitmaps.
There is one wrinkle when it comes to '--boundary', which we must not
pass into the bitmap walk in the presence of both '--path-walk' and
'--use-bitmap-index'. Path-walk needs boundary commits when it performs
its own traversal, in order to discover bases for thin packs, but the
bitmap traversal does not expect this. Work around this by setting
`revs->boundary` as late as possible within the '--path-walk' traversal,
after any bitmap attempt has either succeeded or declined to answer the
request.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/git-pack-objects.adoc | 6 +++--
builtin/pack-objects.c | 18 +++++++++++++--
t/perf/p5311-pack-bitmaps-fetch.sh | 14 +++++++----
t/t5310-pack-bitmaps.sh | 36 +++++++++++++++++++++++++++++
4 files changed, 66 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc
index 8a27aa19fd3..0adce8961a3 100644
--- a/Documentation/git-pack-objects.adoc
+++ b/Documentation/git-pack-objects.adoc
@@ -402,8 +402,10 @@ will be automatically changed to version `1`.
of filenames that cause collisions in Git's default name-hash
algorithm.
+
-Incompatible with `--delta-islands`. The `--use-bitmap-index` option is
-ignored in the presence of `--path-walk`. The `--path-walk` option
+Incompatible with `--delta-islands`. When `--use-bitmap-index` is
+specified with `--path-walk`, a successful bitmap traversal is used for
+object enumeration, with path-walk remaining as the fallback traversal
+when the bitmap cannot satisfy the request. The `--path-walk` option
supports the `--filter=<spec>` forms `blob:none`, `blob:limit=<n>`,
`tree:0`, `object:type=<type>`, and `sparse:<oid>`. These supported filter
types can be combined with the `combine:<spec>+<spec>` form.
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b783dc62bc9..e4dcb563b7d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4732,6 +4732,15 @@ static int add_objects_by_path(const char *path,
continue;
add_object_entry(oid, type, path, exclude);
+
+ if (type == OBJ_COMMIT && write_bitmap_index) {
+ struct commit *commit;
+
+ commit = lookup_commit(the_repository, oid);
+ if (!commit)
+ die(_("could not find commit %s"), oid_to_hex(oid));
+ index_commit_for_bitmap(commit);
+ }
}
oe_end = to_pack.nr_objects;
@@ -4764,6 +4773,13 @@ static int get_object_list_path_walk(struct rev_info *revs)
info.path_fn = add_objects_by_path;
info.path_fn_data = &processed;
+ /*
+ * Path-walk needs boundary commits to discover thin-pack bases, but
+ * bitmap traversal does not understand the boundary state. Set it
+ * here so any prior bitmap attempt sees the usual non-boundary walk.
+ */
+ revs->boundary = 1;
+
/*
* Allow the --[no-]sparse option to be interesting here, if only
* for testing purposes. Paths with no interesting objects will not
@@ -5195,9 +5211,7 @@ int cmd_pack_objects(int argc,
}
}
if (path_walk) {
- strvec_push(&rp, "--boundary");
strvec_push(&rp, "--objects");
- use_bitmap_index = 0;
} else if (thin) {
use_internal_rev_list = 1;
strvec_push(&rp, shallow
diff --git a/t/perf/p5311-pack-bitmaps-fetch.sh b/t/perf/p5311-pack-bitmaps-fetch.sh
index 5bea5c64e7b..1b115d921a1 100755
--- a/t/perf/p5311-pack-bitmaps-fetch.sh
+++ b/t/perf/p5311-pack-bitmaps-fetch.sh
@@ -4,15 +4,18 @@ test_description='performance of fetches from bitmapped packs'
. ./perf-lib.sh
test_fetch_bitmaps () {
+ argv=$1
+ export argv
+
test_expect_success 'setup test directory' '
rm -fr * .git
'
test_perf_default_repo
- test_expect_success 'create bitmapped server repo' '
+ test_expect_success "create bitmapped server repo ${argv:+($argv)}" '
git config pack.writebitmaps true &&
- git repack -ad
+ git repack -ad $argv
'
# simulate a fetch from a repository that last fetched N days ago, for
@@ -20,7 +23,7 @@ test_fetch_bitmaps () {
# and assume the first entry in the chain that is N days older than the current
# HEAD is where the HEAD would have been then.
for days in 1 2 4 8 16 32 64 128; do
- title=$(printf '%10s' "($days days)")
+ title=$(printf '%10s' "($days days${argv:+, $argv})")
test_expect_success "setup revs from $days days ago" '
now=$(git log -1 --format=%ct HEAD) &&
then=$(($now - ($days * 86400))) &&
@@ -47,6 +50,9 @@ test_fetch_bitmaps () {
done
}
-test_fetch_bitmaps
+for argv in '' --path-walk
+do
+ test_fetch_bitmaps $argv || return 1
+done
test_done
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index f693cb56691..69c5da1580a 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -577,6 +577,42 @@ test_bitmap_cases
sane_unset GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL
+test_expect_success 'path-walk repack can write and use bitmap indexes' '
+ test_when_finished "rm -rf path-walk-bitmap" &&
+ git init path-walk-bitmap &&
+ (
+ cd path-walk-bitmap &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+
+ git repack -a -d -b --path-walk &&
+ git rev-list --test-bitmap --use-bitmap-index HEAD &&
+
+ git rev-parse HEAD >in &&
+
+ git rev-list --objects --no-object-names HEAD >expect.raw &&
+ sort expect.raw >expect &&
+
+ for reuse in true false
+ do
+ : >trace.txt &&
+
+ GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
+ git -c pack.allowPackReuse=$reuse pack-objects \
+ --stdout --revs --path-walk --use-bitmap-index \
+ <in >out.pack &&
+ grep "\"category\":\"bitmap\",\"key\":\"bitmap/hits\"" trace.txt &&
+
+ git index-pack out.pack &&
+
+ list_packed_objects out.idx >actual.raw &&
+ sort actual.raw >actual &&
+ test_cmp expect actual || return 1
+ done
+ )
+'
+
test_expect_success 'incremental repack fails when bitmaps are requested' '
test_commit more-1 &&
test_must_fail git repack -d 2>err &&
--
2.54.0.23.gae57607b57f
^ permalink raw reply related
* [PATCH v2 3/4] pack-objects: extract `record_tree_depth()` helper
From: Taylor Blau @ 2026-06-02 22:21 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Junio C Hamano, Jeff King, Elijah Newren
In-Reply-To: <cover.1780438896.git.me@ttaylorr.com>
Prepare for a subsequent change that needs to record tree depths from a
second call site by factoring the delta-islands tree-depth bookkeeping
out of `show_object()` and into a helper, `record_tree_depth()`.
The helper looks up the object in `to_pack`, returns early when the
object was not added there, computes the depth from the slash count in
the supplied name, and preserves the existing max-depth-wins behavior
when a tree is reached by more than one path.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/pack-objects.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index e4dcb563b7d..ec02e2b21d2 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2722,6 +2722,22 @@ static inline void oe_set_tree_depth(struct packing_data *pack,
pack->tree_depth[e - pack->objects] = tree_depth;
}
+static void record_tree_depth(const struct object_id *oid, const char *name)
+{
+ const char *p;
+ unsigned depth;
+ struct object_entry *ent;
+
+ /* the empty string is a root tree, which is depth 0 */
+ depth = *name ? 1 : 0;
+ for (p = strchr(name, '/'); p; p = strchr(p + 1, '/'))
+ depth++;
+
+ ent = packlist_find(&to_pack, oid);
+ if (ent && depth > oe_tree_depth(&to_pack, ent))
+ oe_set_tree_depth(&to_pack, ent, depth);
+}
+
/*
* Return the size of the object without doing any delta
* reconstruction (so non-deltas are true object sizes, but deltas
@@ -4375,20 +4391,8 @@ static void show_object(struct object *obj, const char *name,
add_preferred_base_object(name);
add_object_entry(&obj->oid, obj->type, name, 0);
- if (use_delta_islands) {
- const char *p;
- unsigned depth;
- struct object_entry *ent;
-
- /* the empty string is a root tree, which is depth 0 */
- depth = *name ? 1 : 0;
- for (p = strchr(name, '/'); p; p = strchr(p + 1, '/'))
- depth++;
-
- ent = packlist_find(&to_pack, &obj->oid);
- if (ent && depth > oe_tree_depth(&to_pack, ent))
- oe_set_tree_depth(&to_pack, ent, depth);
- }
+ if (use_delta_islands)
+ record_tree_depth(&obj->oid, name);
}
static void show_object__ma_allow_any(struct object *obj, const char *name, void *data)
--
2.54.0.23.gae57607b57f
^ permalink raw reply related
* [PATCH v2 4/4] pack-objects: support `--delta-islands` with `--path-walk`
From: Taylor Blau @ 2026-06-02 22:21 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Junio C Hamano, Jeff King, Elijah Newren
In-Reply-To: <cover.1780438896.git.me@ttaylorr.com>
Since the inception of `--path-walk`, this option has had a documented
incompatibility with `--delta-islands`.
When discussing those original patches on the list, a message from
Stolee in [1] noted the following:
this could be remedied by [...] doing a separate walk to identify
islands using the normal method
In a related portion of the thread, Peff explains[2]:
The delta islands code already does its own tree walk to propagate
the bits down (it does rely on the base walk's show_commit() to
propagate through the commits).
Once each object has its island bitmaps, I think however you
choose to come up with delta candidates [...] you should be able
to use it. It's fundamentally just answering the question of "am
I allowed to delta between these two objects".
That is similar to what this patch does, and it turns out the cheaper
option is sufficient: perform the same island side effects from the
path-walk callback rather than doing a second walk.
Recall how delta-islands are computed during a normal repack:
- `show_commit()` calls `propagate_island_marks()` for each commit,
which merges the commit's island bitset onto its root tree object and
onto each of its parent commits.
- `show_object()` for a tree records the tree's depth derived from the
slash-separated pathname. Subsequent `resolve_tree_islands()` uses
that depth to walk trees in increasing-depth order, propagating each
tree's marks to its children.
- At delta-search time, `in_same_island()` enforces that a delta
target's island bitmap is a subset of its base's: every island that
reaches the target must also reach the base.
Path-walk's enumeration callback is `add_objects_by_path()`. It already
adds objects to `to_pack`, but until now did not perform the
island-related side effects. Two things are needed:
- For each commit batch, call `propagate_island_marks()` on commits,
exactly as `show_commit()` does.
We have to be careful about the order in which we call this function,
and we must see a commit before its parents in order to have
island marks to propagate.
The path-walk batch preserves that order. Path-walk appends commits
to its `OBJ_COMMIT` batch as they come back from the same
`get_revision()` loop the regular traversal uses, and
`add_objects_by_path()` iterates the batch in array order. So every
commit reaches `propagate_island_marks()` in the same sequence that
`show_commit()` would have seen it, and the descendant-first chain
that the algorithm relies on is intact.
Skip island propagation for excluded commits to match the regular
traversal, whose `show_commit()` callback is only invoked for
interesting commits. Boundary commits may still be present in
path-walk's callback so they can serve as thin-pack bases, but they
should not contribute island marks.
- For each tree batch, record the tree's depth from the path. Use the
`record_tree_depth()` helper from the previous commit so both
callbacks behave identically, including the max-depth-wins behavior
when a tree is reached via more than one path. The helper accepts
both the `show_object()` path shape ("foo", "foo/bar") and the
path-walk shape with a trailing slash ("foo/", "foo/bar/"), so depths
recorded from either traversal mode are directly comparable.
This is implicit in the implementation sketch from Peff above.
`resolve_tree_islands()` sorts trees by `oe->tree_depth` in
increasing-depth order before propagating marks down, so that a
parent tree's marks are finalized before its children inherit them.
Without recording the depth at path-walk time, every
path-walk-discovered tree would land at depth 0 in `to_pack`, the
sort would lose its ordering, and children could inherit marks from
parents whose own contributions had not yet been merged in.
With those two pieces in place, `resolve_tree_islands()` receives the
same island inputs from path-walk as it would from the regular
traversal, so the existing island checks can be reused unchanged.
Drop the documented incompatibility between `--path-walk` and
`--delta-islands`, and add t5320 coverage for path-walk island repacks
with and without bitmap writing, as well as the same-island case where a
delta remains allowed.
[1]: https://lore.kernel.org/git/9aa2471b-0850-4707-9733-d3b33609f5f2@gmail.com/
[2]: https://lore.kernel.org/git/20240911063203.GA1538586@coredump.intra.peff.net/
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/git-pack-objects.adoc | 14 +++++++-------
builtin/pack-objects.c | 22 ++++++++++++++++++----
t/t5320-delta-islands.sh | 29 +++++++++++++++++++++++++++++
3 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc
index 0adce8961a3..65cd00c152f 100644
--- a/Documentation/git-pack-objects.adoc
+++ b/Documentation/git-pack-objects.adoc
@@ -402,13 +402,13 @@ will be automatically changed to version `1`.
of filenames that cause collisions in Git's default name-hash
algorithm.
+
-Incompatible with `--delta-islands`. When `--use-bitmap-index` is
-specified with `--path-walk`, a successful bitmap traversal is used for
-object enumeration, with path-walk remaining as the fallback traversal
-when the bitmap cannot satisfy the request. The `--path-walk` option
-supports the `--filter=<spec>` forms `blob:none`, `blob:limit=<n>`,
-`tree:0`, `object:type=<type>`, and `sparse:<oid>`. These supported filter
-types can be combined with the `combine:<spec>+<spec>` form.
+When `--use-bitmap-index` is specified with `--path-walk`, a successful
+bitmap traversal is used for object enumeration, with path-walk
+remaining as the fallback traversal when the bitmap cannot satisfy the
+request. The `--path-walk` option supports the `--filter=<spec>` forms
+`blob:none`, `blob:limit=<n>`, `tree:0`, `object:type=<type>`, and
+`sparse:<oid>`. These supported filter types can be combined with the
+`combine:<spec>+<spec>` form.
DELTA ISLANDS
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ec02e2b21d2..f48ea7a888b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4737,13 +4737,29 @@ static int add_objects_by_path(const char *path,
add_object_entry(oid, type, path, exclude);
- if (type == OBJ_COMMIT && write_bitmap_index) {
+ if (type == OBJ_COMMIT) {
struct commit *commit;
+ if (!write_bitmap_index && !use_delta_islands)
+ continue;
+
commit = lookup_commit(the_repository, oid);
if (!commit)
die(_("could not find commit %s"), oid_to_hex(oid));
- index_commit_for_bitmap(commit);
+ if (write_bitmap_index)
+ index_commit_for_bitmap(commit);
+ /*
+ * Skip island propagation for boundary commits.
+ * The regular traversal's show_commit() is only
+ * called for interesting commits; matching that
+ * here keeps path-walk from doing extra work that
+ * would only be a no-op anyway (boundary commits
+ * are not in island_marks).
+ */
+ if (use_delta_islands && !exclude)
+ propagate_island_marks(the_repository, commit);
+ } else if (type == OBJ_TREE && use_delta_islands) {
+ record_tree_depth(oid, path);
}
}
@@ -5205,8 +5221,6 @@ int cmd_pack_objects(int argc,
const char *option = NULL;
if (!path_walk_filter_compatible(&filter_options))
option = "--filter";
- else if (use_delta_islands)
- option = "--delta-islands";
if (option) {
warning(_("cannot use %s with %s"),
diff --git a/t/t5320-delta-islands.sh b/t/t5320-delta-islands.sh
index 2c961c70963..9b28344a0a3 100755
--- a/t/t5320-delta-islands.sh
+++ b/t/t5320-delta-islands.sh
@@ -53,6 +53,35 @@ test_expect_success 'separate islands disallows delta' '
! is_delta_base $two $one
'
+test_expect_success 'path-walk island repack respects islands' '
+ GIT_TRACE2_EVENT="$(pwd)/trace.path-walk-islands" \
+ git -c "pack.island=refs/heads/(.*)" repack -adfi \
+ --path-walk 2>err &&
+ test_region pack-objects path-walk trace.path-walk-islands &&
+ test_grep ! "cannot use --delta-islands with --path-walk" err &&
+ ! is_delta_base $one $two &&
+ ! is_delta_base $two $one
+'
+
+test_expect_success 'path-walk island bitmap repack respects islands' '
+ GIT_TRACE2_EVENT="$(pwd)/trace.path-walk-island-bitmap" \
+ git -c "pack.island=refs/heads/(.*)" repack -a -d -f -i -b \
+ --path-walk 2>err &&
+ test_region pack-objects path-walk trace.path-walk-island-bitmap &&
+ test_path_is_file .git/objects/pack/*.bitmap &&
+ git rev-list --test-bitmap --use-bitmap-index one &&
+ test_grep ! "cannot use --delta-islands with --path-walk" err &&
+ ! is_delta_base $one $two &&
+ ! is_delta_base $two $one
+'
+
+test_expect_success 'path-walk same island allows delta' '
+ GIT_TRACE2_EVENT="$(pwd)/trace.path-walk-same-island" \
+ git -c "pack.island=refs/heads" repack -adfi --path-walk &&
+ test_region pack-objects path-walk trace.path-walk-same-island &&
+ is_delta_base $one $two
+'
+
test_expect_success 'same island allows delta' '
git -c "pack.island=refs/heads" repack -adfi &&
is_delta_base $one $two
--
2.54.0.23.gae57607b57f
^ permalink raw reply related
* Re: [PATCH v2] prio-queue: use cascade-down for faster extract-min
From: Kristofer Karlsson @ 2026-06-02 22:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <90270818-c52b-4611-8da2-6cee20628fc2@web.de>
On Tue, 2 Jun 2026 at 18:37, René Scharfe <l.s.r@web.de> wrote:
>
> Would you be interested in benchmarking the following patch for making
> prio_queue_replace() unnecessary by doing its optimization
> automatically? I get a 1% performance hit for the describe command
> that I can't explain. And it leaves the heap unbalanced after a
> prio_queue_get(), which complicates things, so I found it lacking.
> But I wonder how it stacks up against your cascade approach for your
> use case and if there's anything to salvage.
>
> René
Thank you for the detailed feedback and the patch! It was very
helpful to have a concrete alternative to compare against.
I spent some time benchmarking the different approaches on a
large monorepo with a wide DAG.
All measurements include the nonstale O(1) tracking from my other
series as a common base, since that dominates the merge-base path.
The approaches I compared:
1. cascade-only: the sift_up_rebalance from this patch (v2)
2. rene-lazy: your deferred sift_down_root patch
3. cascade+lazy: cascade for unfused gets, lazy fusion for
get+put pairs
Results (10 runs, 1 warmup, CPU pinned to performance):
merge-base --all master master~1000 (~4s workload):
cascade-only 4.18s (median)
rene-lazy 4.25s
cascade+lazy 4.24s
rev-list --count master~1000..master (~3.8s workload):
cascade-only 3.86s
rene-lazy 3.75s
cascade+lazy 3.74s
The lazy approaches show a small win on rev-list (~3%) where get+put
pairs are common in limit_list. On merge-base --all, everything is
within noise, the prio_queue is a small fraction of total runtime
there. Combining cascade with lazy fusion didn't produce additional
gains beyond what each gives individually.
Looking at your patch, I think the deferred sift-down logic is
essentially the same optimization as the lazy_queue wrapper you
wrote for describe.c - both defer the work from get and fuse it
with a following put. So I'd be hesitant to add a second form of
that deferral directly into prio_queue when lazy_queue already
"owns" that responsibility as a wrapper.
That said, I think it would make sense to fold lazy_queue entirely
into prio_queue. It's an optimization that never hurts as far as I can
tell, and it would simplify several callers. pop_most_recent_commit
and show-branch both independently re-implement the same
peek+replace pattern that lazy_queue formalizes. Making it automatic
in prio_queue would clean up all of them.
I have a local branch exploring that direction. Maybe it makes more
sense to do the lazy_queue fold first, and then see if the cascade
change is still worth adding on top?
Either way, I think the two directions are complementary - cascade
reduces comparisons per sift, while lazy fusion can eliminate full
rebalance cycles.
I'm on a company offsite now so I may be slow to answer, but I will
definitely resume this when I get back home.
- Kristofer
^ permalink raw reply
* Re: [PATCH] worktree: record creation time and free-form note
From: Junio C Hamano @ 2026-06-02 23:52 UTC (permalink / raw)
To: Kiesel, Norbert; +Cc: git
In-Reply-To: <CAPGaHku+RAV+FA3C0md0xHiavfdB_anoqcMM06MAiU1VyMAdLA@mail.gmail.com>
"Kiesel, Norbert" <norbert.kiesel@creditkarma.com> writes:
> From 130cd5e4a25e6672b2a97268e1100b6ef03fa552 Mon Sep 17 00:00:00 2001
> From: Norbert Kiesel <norbert.kiesel@creditkarma.com>
> Date: Mon, 1 Jun 2026 17:03:39 -0700
> Subject: [PATCH] worktree: record creation time and free-form note
>
> Add per-worktree metadata so users can answer "what is this worktree
> for, and when did I make it?" without resorting to external notes.
Although I am not personally interested in this topic all that much,
let me point out that we have $GIT_DIR/description file that may be
useful for something like this. It has been the canonical place for
the main repository to identify itself long before secondary worktrees
were invented and $GIT_COMMON_DIR/worktrees/$worktree/description would
be a natural extension of the concept, I'd presume.
^ permalink raw reply
* Re: [PATCH] worktree: record creation time and free-form note
From: Junio C Hamano @ 2026-06-02 23:57 UTC (permalink / raw)
To: Kiesel, Norbert; +Cc: git
In-Reply-To: <xmqq1peots9i.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Kiesel, Norbert" <norbert.kiesel@creditkarma.com> writes:
>
>> From 130cd5e4a25e6672b2a97268e1100b6ef03fa552 Mon Sep 17 00:00:00 2001
>> From: Norbert Kiesel <norbert.kiesel@creditkarma.com>
>> Date: Mon, 1 Jun 2026 17:03:39 -0700
>> Subject: [PATCH] worktree: record creation time and free-form note
Ah, I forgot to mention another thing. Please do not add these four
lines to your message body. The information belongs to the e-mail
header, and as long as your e-mail software is configured correctly
there shouldn't be a need to use From: or override the time when the
patch was made public with Date: in-body header.
Thanks.
^ permalink raw reply
* Re: [PATCH] worktree: record creation time and free-form note
From: Kiesel, Norbert @ 2026-06-03 0:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1peots9i.fsf@gitster.g>
Yes, I could change my PR to use $GIT_COMMON_DIR/worktrees/$worktree/description
instead of the currently used $GIT_COMMON_DIR/worktrees/$worktree/note.
Give me a day, and I can create the updated diff.
Best,
Norbert
On Tue, Jun 2, 2026 at 4:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kiesel, Norbert" <norbert.kiesel@creditkarma.com> writes:
>
> > From 130cd5e4a25e6672b2a97268e1100b6ef03fa552 Mon Sep 17 00:00:00 2001
> > From: Norbert Kiesel <norbert.kiesel@creditkarma.com>
> > Date: Mon, 1 Jun 2026 17:03:39 -0700
> > Subject: [PATCH] worktree: record creation time and free-form note
> >
> > Add per-worktree metadata so users can answer "what is this worktree
> > for, and when did I make it?" without resorting to external notes.
>
> Although I am not personally interested in this topic all that much,
> let me point out that we have $GIT_DIR/description file that may be
> useful for something like this. It has been the canonical place for
> the main repository to identify itself long before secondary worktrees
> were invented and $GIT_COMMON_DIR/worktrees/$worktree/description would
> be a natural extension of the concept, I'd presume.
--
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
* [PATCH v3] index-pack: retain child bases in delta cache
From: Arijit Banerjee via GitGitGadget @ 2026-06-03 0:05 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
Derrick Stolee, Jeff King, Arijit Banerjee, Arijit Banerjee
In-Reply-To: <pull.2131.v2.git.1780330402264.gitgitgadget@gmail.com>
From: Arijit Banerjee <arijit@effectiveailabs.com>
When resolving a delta whose result has children of its own,
index-pack adds the result to work_head, accounts its data in
base_cache_used, and calls prune_base_data(). It then immediately frees
that same data.
This bypasses the existing delta base cache policy and can force later
descendants to reconstruct the queued base again. Let the existing
delta_base_cache_limit pruning policy decide whether to keep or evict
the data instead.
This does not add a new cache or increase the cache limit. The object
data is already accounted in base_cache_used before prune_base_data()
runs. Once all direct children of a base have been dispatched, and no
thread is actively retaining that base for patch_delta(), release the
cached bytes. The base_data struct itself remains alive until the
existing children_remaining bookkeeping says the whole subtree is done.
On a quiet Ubuntu 24.04 VM with 16 vCPUs, 32 GiB RAM, and local SSD,
standard p5302-pack-index.sh runs improved as follows:
libgit2: 3.17(11.49+0.60) -> 2.69(10.52+0.28), 15.1% faster
redis: 5.84(15.56+0.63) -> 4.95(14.05+0.32), 15.2% faster
git.git: 11.17(38.04+1.29) -> 9.67(35.29+0.60), 13.4% faster
cpython: 32.69(117.85+4.37) -> 28.60(109.25+1.91), 12.5% faster
linux: 279.22(797.69+40.86) -> 236.34(723.13+19.02), 15.4% faster
The linux p5302 number is from a single repeat; the others are from the
default three repeats.
End-to-end local full-clone spot checks also improved:
libgit2: 5.00s -> 4.54s, 9.2% faster
redis: 8.75s -> 7.92s, 9.5% faster
git.git: 25.04s -> 23.71s, 5.3% faster
cpython: 56.72s -> 55.94s, 1.4% faster
linux: 556.17s -> 523.83s, 5.8% faster
t/t5302-pack-index.sh passed, and GitGitGadget's linux-leaks CI also
exercised that test under SANITIZE=leak.
Signed-off-by: Arijit Banerjee <arijit@effectiveailabs.com>
---
index-pack: retain child bases in delta cache
Speed up the local index-pack phase used by clone/fetch for large
delta-compressed packs.
When index-pack reconstructs a child base and queues it for resolving
descendant deltas, it currently frees that data immediately. This can
force the same base to be reconstructed again. This patch keeps the data
in the existing delta base cache instead of immediately freeing it.
This does not add a new cache or increase the cache limit. The object
data is already accounted in base_cache_used, and prune_base_data() is
already called at this point.
To keep the retained data lifetime precise, the patch also releases
cached bytes once all direct children of a base have been dispatched and
no thread is actively retaining that base for patch_delta(). The
base_data struct itself still stays alive until the existing
children_remaining bookkeeping says the whole subtree is done.
Changes since v2:
* Addressed Jeff King's review question by releasing cached base data
after all direct children have been dispatched, while keeping the
existing subtree bookkeeping intact.
* Re-ran t/t5302-pack-index.sh, p5302-pack-index.sh, and end-to-end
full clone spot checks with the precise-release version.
Changes since v1:
* Added benchmark results and leak-safety context to the commit
message.
* Included standard p5302-pack-index.sh perf-suite results.
Correctness:
* t/t5302-pack-index.sh passed.
* GitGitGadget's linux-leaks CI exercises t5302-pack-index.sh under
SANITIZE=leak.
Benchmarks on a quiet Ubuntu 24.04 VM, 16 vCPU, 32 GiB RAM, local SSD:
Standard p5302-pack-index.sh perf-suite results using
GIT_PERF_LARGE_REPO=<repo> ./run HEAD~1 HEAD -- p5302-pack-index.sh:
repo HEAD~1 HEAD wall-time change libgit2 3.17(11.49+0.60)
2.69(10.52+0.28) 15.1% faster redis 5.84(15.56+0.63) 4.95(14.05+0.32)
15.2% faster git.git 11.17(38.04+1.29) 9.67(35.29+0.60) 13.4% faster
cpython 32.69(117.85+4.37) 28.60(109.25+1.91) 12.5% faster linux
279.22(797.69+40.86) 236.34(723.13+19.02) 15.4% faster
The linux p5302 row is from a single repeat; the others use the default
three repeats. These timings isolate the index-pack phase affected by
this patch.
End-to-end local full-clone spot checks also showed wins, though these
timings include both server-side pack-objects and client-side index-pack
running concurrently over a local file:// transport, so they are not
isolated index-pack timings.
These runs used git clone --bare --no-local, dropped page cache before
each clone, and used the matching build's bin-wrappers/git as the client
plus the matching bin-wrappers/git-upload-pack via --upload-pack.
repo baseline patched wall-time change libgit2 5.00s 4.54s 9.2% faster
redis 8.75s 7.92s 9.5% faster git.git 25.04s 23.71s 5.3% faster cpython
56.72s 55.94s 1.4% faster linux 556.17s 523.83s 5.8% faster
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2131%2Farijit91%2Findex-pack-retain-child-base-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2131/arijit91/index-pack-retain-child-base-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2131
Range-diff vs v2:
1: 42eca38f51 ! 1: 51967f9edf index-pack: retain child bases in delta cache
@@ Commit message
This does not add a new cache or increase the cache limit. The object
data is already accounted in base_cache_used before prune_base_data()
- runs, and the existing pruning and base cleanup paths still release it.
+ runs. Once all direct children of a base have been dispatched, and no
+ thread is actively retaining that base for patch_delta(), release the
+ cached bytes. The base_data struct itself remains alive until the
+ existing children_remaining bookkeeping says the whole subtree is done.
On a quiet Ubuntu 24.04 VM with 16 vCPUs, 32 GiB RAM, and local SSD,
- direct index-pack timings on single-pack Linux fixtures improved as
- follows:
+ standard p5302-pack-index.sh runs improved as follows:
- linux blobless: 69.17s -> 57.98s (16.2% faster), RSS flat
- linux full: 280.72s -> 236.32s (15.8% faster), RSS +1.9%
+ libgit2: 3.17(11.49+0.60) -> 2.69(10.52+0.28), 15.1% faster
+ redis: 5.84(15.56+0.63) -> 4.95(14.05+0.32), 15.2% faster
+ git.git: 11.17(38.04+1.29) -> 9.67(35.29+0.60), 13.4% faster
+ cpython: 32.69(117.85+4.37) -> 28.60(109.25+1.91), 12.5% faster
+ linux: 279.22(797.69+40.86) -> 236.34(723.13+19.02), 15.4% faster
- Five-repeat medians on public repositories also improved:
+ The linux p5302 number is from a single repeat; the others are from the
+ default three repeats.
- git.git: 12.31s -> 10.70s (13.1% faster)
- libgit2: 3.35s -> 2.88s (14.0% faster)
- redis: 6.52s -> 5.64s (13.5% faster)
- cpython: 33.02s -> 31.44s (4.8% faster)
+ End-to-end local full-clone spot checks also improved:
- The standard p5302 perf test on a smaller git.git fixture was neutral:
-
- 5302.9 index-pack default threads:
- 11.21(38.07+1.33) -> 11.16(37.90+1.31), -0.4%
+ libgit2: 5.00s -> 4.54s, 9.2% faster
+ redis: 8.75s -> 7.92s, 9.5% faster
+ git.git: 25.04s -> 23.71s, 5.3% faster
+ cpython: 56.72s -> 55.94s, 1.4% faster
+ linux: 556.17s -> 523.83s, 5.8% faster
t/t5302-pack-index.sh passed, and GitGitGadget's linux-leaks CI also
exercised that test under SANITIZE=leak.
@@ Commit message
Signed-off-by: Arijit Banerjee <arijit@effectiveailabs.com>
## builtin/index-pack.c ##
+@@ builtin/index-pack.c: static void free_base_data(struct base_data *c)
+ }
+ }
+
++static int base_data_has_remaining_direct_children(struct base_data *c)
++{
++ return c->ref_first <= c->ref_last ||
++ c->ofs_first <= c->ofs_last;
++}
++
+ static void prune_base_data(struct base_data *retain)
+ {
+ struct list_head *pos;
+@@ builtin/index-pack.c: static void *threaded_second_pass(void *data)
+ }
+
+ work_lock();
+- if (parent)
++ if (parent) {
+ parent->retain_data--;
++ if (!parent->retain_data &&
++ !base_data_has_remaining_direct_children(parent))
++ free_base_data(parent);
++ }
+
+ if (child && child->data) {
+ /*
@@ builtin/index-pack.c: static void *threaded_second_pass(void *data)
list_add(&child->list, &work_head);
base_cache_used += child->size;
builtin/index-pack.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index cf0bd8280d..00b4dff419 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -433,6 +433,12 @@ static void free_base_data(struct base_data *c)
}
}
+static int base_data_has_remaining_direct_children(struct base_data *c)
+{
+ return c->ref_first <= c->ref_last ||
+ c->ofs_first <= c->ofs_last;
+}
+
static void prune_base_data(struct base_data *retain)
{
struct list_head *pos;
@@ -1201,8 +1207,12 @@ static void *threaded_second_pass(void *data)
}
work_lock();
- if (parent)
+ if (parent) {
parent->retain_data--;
+ if (!parent->retain_data &&
+ !base_data_has_remaining_direct_children(parent))
+ free_base_data(parent);
+ }
if (child && child->data) {
/*
@@ -1212,7 +1222,6 @@ static void *threaded_second_pass(void *data)
list_add(&child->list, &work_head);
base_cache_used += child->size;
prune_base_data(NULL);
- free_base_data(child);
} else if (child) {
/*
* This child does not have its own children. It may be
base-commit: c69baaf57ba26cf117c2b6793802877f19738b0d
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] http: preserve wwwauth_headers across redirects
From: Junio C Hamano @ 2026-06-03 0:15 UTC (permalink / raw)
To: Aaron Plattner; +Cc: git, Rahul Rameshbabu
In-Reply-To: <20260602161150.1527493-1-aplattner@nvidia.com>
Aaron Plattner <aplattner@nvidia.com> writes:
> diff --git a/http.c b/http.c
> index ea9b16861b..cac8c9bfc9 100644
> --- a/http.c
> +++ b/http.c
> @@ -2425,7 +2425,21 @@ static int http_request_recoverable(const char *url,
> if (options->effective_url && options->base_url) {
> if (update_url_from_redirect(options->base_url,
> url, options->effective_url)) {
> + struct strvec wwwauth_headers = STRVEC_INIT;
> +
> + /*
> + * Preserve wwwauth_headers across the call to
> + * credential_from_url(): if the effective URL doesn't
> + * specify its own credentials, a credential helper
> + * might need the wwwauth[] array from the server's
> + * redirect response in order to authenticate.
> + */
> + strvec_pushv(&wwwauth_headers,
> + http_auth.wwwauth_headers.v);
> credential_from_url(&http_auth, options->base_url->buf);
> + strvec_pushv(&http_auth.wwwauth_headers,
> + wwwauth_headers.v);
> + strvec_clear(&wwwauth_headers);
> url = options->effective_url->buf;
> }
> }
As strvec_pushv() makes copies of the strings contained in .v[]
array, the above will
- make a deep copy of http_auth.wwwauth_headers.v[] and store it away
in wwwauth_headers.v[];
- let credential_from_url() get rid of
http_auth.wwwauth_headers.v[] (the original is freed here, but we
have a deep copy stashed away safely), and perhaps add some of
its own there; then
- add what we stashed away back to http_auth.wwwauth_headers.v[].
So it does not leak and it does not have use-after-free, either,
which is good, even though it may be a bit inefficient having to
copy these strings so many times.
I briefly wondered if it is unconditionally adding back the original
wwwauth_headers always the right thing to do, but I think this is
good. In the context of http_request_recovorable(), the redirect
has already happened, and the request to the redirect target has
failed with a 401. The wwwauth_headers currently in http_auth were
populated from this 401 response from the redirect target. Since we
are updating http_auth's URL to match this redirect target (in order
to query the helper for the correct host), the headers we currently
have are the active challenges for this new URL. Thus, they must be
preserved and passed to the helper.
A few design questions that came to my mind are:
- Is wwwauth_headers the _only_ thing that needs to be preserved in
the existing credential in http_auth? Will it stay to be the
only thing, or will we need to rethink what this patch did in the
future when we add such a new member to "struct credential"?
- If we need to preserve some other members in "struct credential",
or if we add such members to the struct in the future, what would
be the recommended way to extend what this patch does to cover?
If we add new members in the future to store other transient
response-based authentication state (e.g. Authentication-Info
headers, or proxy authentication states), they will be wiped by
credential_from_url() and will need to be preserved the same way,
no? This observation and thought experiment may hint that the
manual save-and-restore approach is not robust against future
extensions of struct credential.
The current approach of manually saving and restoring
wwwauth_headers in http.c creates a tight coupling between the HTTP
layer and the internals of struct credential. If new transient
fields are added in the future, developers must remember to update
http.c to preserve them, which may be error-prone.
I wonder if it would make the design more robust and future-proof to
encapsulate this logic in credential.c instead. For example, we
could introduce a helper function:
void credential_update_url(struct credential *c, const char *url)
that does what the new code added around credential_from_url() by
this patch does, perhaps?
^ permalink raw reply
* Re: [PATCH] http: preserve wwwauth_headers across redirects
From: Aaron Plattner @ 2026-06-03 0:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Rahul Rameshbabu
In-Reply-To: <xmqqpl28scll.fsf@gitster.g>
On 6/2/26 5:15 PM, Junio C Hamano wrote:
> Aaron Plattner <aplattner@nvidia.com> writes:
>
>> diff --git a/http.c b/http.c
>> index ea9b16861b..cac8c9bfc9 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -2425,7 +2425,21 @@ static int http_request_recoverable(const char *url,
>> if (options->effective_url && options->base_url) {
>> if (update_url_from_redirect(options->base_url,
>> url, options->effective_url)) {
>> + struct strvec wwwauth_headers = STRVEC_INIT;
>> +
>> + /*
>> + * Preserve wwwauth_headers across the call to
>> + * credential_from_url(): if the effective URL doesn't
>> + * specify its own credentials, a credential helper
>> + * might need the wwwauth[] array from the server's
>> + * redirect response in order to authenticate.
>> + */
>> + strvec_pushv(&wwwauth_headers,
>> + http_auth.wwwauth_headers.v);
>> credential_from_url(&http_auth, options->base_url->buf);
>> + strvec_pushv(&http_auth.wwwauth_headers,
>> + wwwauth_headers.v);
>> + strvec_clear(&wwwauth_headers);
>> url = options->effective_url->buf;
>> }
>> }
>
> As strvec_pushv() makes copies of the strings contained in .v[]
> array, the above will
>
> - make a deep copy of http_auth.wwwauth_headers.v[] and store it away
> in wwwauth_headers.v[];
>
> - let credential_from_url() get rid of
> http_auth.wwwauth_headers.v[] (the original is freed here, but we
> have a deep copy stashed away safely), and perhaps add some of
> its own there; then
>
> - add what we stashed away back to http_auth.wwwauth_headers.v[].
>
> So it does not leak and it does not have use-after-free, either,
> which is good, even though it may be a bit inefficient having to
> copy these strings so many times.
I agree, although in the grand scheme wwwauth_headers is a drop in the
bucket compared to, say, nearly any git object.
I tried to find an easy way to just not clear it in the first place, but
that doesn't really match what credential_clear() is intended to do.
> I briefly wondered if it is unconditionally adding back the original
> wwwauth_headers always the right thing to do, but I think this is
> good. In the context of http_request_recovorable(), the redirect
> has already happened, and the request to the redirect target has
> failed with a 401. The wwwauth_headers currently in http_auth were
> populated from this 401 response from the redirect target. Since we
> are updating http_auth's URL to match this redirect target (in order
> to query the helper for the correct host), the headers we currently
> have are the active challenges for this new URL. Thus, they must be
> preserved and passed to the helper.
This matches my understanding and I think it points to a more
fundamental design issue: wwwauth_headers aren't really credentials at
all, and maybe they shouldn't be in struct credential in the first
place. I wonder if it would make sense to encapsulate it in some other
http-related structure that lives alongside the credentials, presumably
along with the protocol, host, and path.
> A few design questions that came to my mind are:
>
> - Is wwwauth_headers the _only_ thing that needs to be preserved in
> the existing credential in http_auth? Will it stay to be the
> only thing, or will we need to rethink what this patch did in the
> future when we add such a new member to "struct credential"?
>
> - If we need to preserve some other members in "struct credential",
> or if we add such members to the struct in the future, what would
> be the recommended way to extend what this patch does to cover?
>
> If we add new members in the future to store other transient
> response-based authentication state (e.g. Authentication-Info
> headers, or proxy authentication states), they will be wiped by
> credential_from_url() and will need to be preserved the same way,
> no? This observation and thought experiment may hint that the
> manual save-and-restore approach is not robust against future
> extensions of struct credential.
>
> The current approach of manually saving and restoring
> wwwauth_headers in http.c creates a tight coupling between the HTTP
> layer and the internals of struct credential. If new transient
> fields are added in the future, developers must remember to update
> http.c to preserve them, which may be error-prone.
>
> I wonder if it would make the design more robust and future-proof to
> encapsulate this logic in credential.c instead. For example, we
> could introduce a helper function:
>
> void credential_update_url(struct credential *c, const char *url)
>
> that does what the new code added around credential_from_url() by
> this patch does, perhaps?
Yeah, maybe. I'll think about this design some more.
-- Aaron
^ permalink raw reply
* Re: [PATCH v2 0/3] contrib/subtree: reduce recursion during split
From: Colin Stagner @ 2026-06-03 1:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ian Jackson, git, Christian Heusel, george, Christian Hesse,
Phillip Wood
In-Reply-To: <xmqqv7c13o5l.fsf@gitster.g>
On 6/1/26 17:13, Junio C Hamano wrote:
> I am tempted to mark the topic as stalled, to be discarded for
> inaction
No objection. I'd still like to see this reviewed, but we can revisit
this later if interest develops.
> While I do agree that avoiding bash-isms in the main part of Git and
> sticking to vanilla POSIX has merit, this particular one seems more
> like an artificial limit imposed by dash than sticking to the POSIX
> as the common denoninator, at least to me.
Correct, this topic is a workaround for an artificial limit. The limit
is Debian-specific and was introduced as a downstream patch in 2018 [1],
[2].
This git-subtree issue has been reported before in
<CAN7rbOve-EFOGPjr1wrD77r-3RQ+3+qso82_oV5Qud-skobL7w@mail.gmail.com>,
<26263.63341.878041.155047@chiark.greenend.org.uk>,
and probably other places. These are old reports, and I haven't found
anyone there still interested in a fix.
Colin
[1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=579815
[2]:
https://sources.debian.org/patches/dash/0.5.12-12/0009-dash-Fix-stack-overflow-from-infinite-recursion-in-s.patch/
^ permalink raw reply
* Re: [PATCH 1/2] b4: introduce configuration for the Git project
From: Weijie Yuan @ 2026-06-03 2:12 UTC (permalink / raw)
To: Tuomas Ahola; +Cc: Patrick Steinhardt, git, Junio C Hamano
In-Reply-To: <20260602170955.Z4b7y%taahol@utu.fi>
On Tue, Jun 02, 2026 at 08:09:55PM +0300, Tuomas Ahola wrote:
> Huh? Doesn't MyFirstContribution speak *against* shallow threading?
>
> [...] make sure to replace it with the correct Message-ID for your
> **previous cover letter** - that is, if you're sending v2, use the Message-ID
> from v1; if you're sending v3, use the Message-ID from v2.
I don't get it. Doesn't shallow threading means every following patches
are replying to the cover letter? Replying to the previous one is
--chain-reply-to, if I'm not mistaken.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] b4: introduce configuration for the Git project
From: Junio C Hamano @ 2026-06-03 2:59 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Patrick Steinhardt, git
In-Reply-To: <8dbdb553-9633-46bb-8a51-040d06d0d10e@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> On 02/06/2026 2:32 pm, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>>> We're about to extend our documentation to recommend b4 for sending
>>> patch series ot the mailing list. Prepare for this by introducing a b4
>>> configuration so that the tool knows to honor our preferences. For now,
>>> this configuration does two things:
>>> ...
>> (hence making the tree dirty).
>
> Hmm, for those of us not in the know, perhaps mention the b4 documentation
> at 'b4.docs.kernel.org' (which includes how to install b4 ... ;) ).
Thanks for raising an excellent point.
^ permalink raw reply
* [PATCH v2 0/4] t: fix broken TAP output
From: Patrick Steinhardt @ 2026-06-03 5:39 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20260602-pks-t7527-fix-tap-output-v1-0-db3da2a1b137@pks.im>
Hi,
this small patch series fixes another instance of broken TAP output that
has landed via 4d11b9c218 (Merge branch 'pt/fsmonitor-linux', 2026-05-31).
As this has happened multiple times by now I decided to have a look at
whether we can fix this class of issues a bit more holistically. So this
series also contains a change that makes prove bail out when it sees
invalid TAP output, which uncovers a small set of preexisting issues in
our test suite.
Changes in v2:
- Fix waiting for p4d, and deduplicate the logic that does this.
- Link to v1: https://patch.msgid.link/20260602-pks-t7527-fix-tap-output-v1-0-db3da2a1b137@pks.im
Thanks!
Patrick
---
Patrick Steinhardt (4):
t7527: fix broken TAP output
t/test-lib: silence EBUSY errors on Windows during test cleanup
t/lib-git-p4: silence output when killing p4d and its watchdog
t: let prove fail when parsing invalid TAP output
t/lib-git-p4.sh | 4 ++--
t/t7527-builtin-fsmonitor.sh | 7 ++++---
t/test-lib.sh | 10 ++++++++--
3 files changed, 14 insertions(+), 7 deletions(-)
Range-diff versus v1:
1: 09977059d1 = 1: 18b4fc7b81 t7527: fix broken TAP output
2: 162d3d42d8 = 2: 8dd921534b t/test-lib: silence EBUSY errors on Windows during test cleanup
3: 4ecb8cb1ce < -: ---------- t/lib-git-p4: silence output when killing p4d and its watchdog
-: ---------- > 3: 8b343176fe t/lib-git-p4: silence output when killing p4d and its watchdog
4: 95fb0d07ae = 4: e69aa0ab79 t: let prove fail when parsing invalid TAP output
---
base-commit: 1666c1265231b0bc5f613fbbf3f0a9896cdef76e
change-id: 20260601-pks-t7527-fix-tap-output-105da1d73df0
^ permalink raw reply
* [PATCH v2 1/4] t7527: fix broken TAP output
From: Patrick Steinhardt @ 2026-06-03 5:39 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20260603-pks-t7527-fix-tap-output-v2-0-cf3af5694e20@pks.im>
Before running the tests in t7527 we first verify whether the fsmonitor
even works, which seems to depend on the actual filesystem that is in
use. The verification executes outside of any prerequisite or test body,
so its stdout/stderr is not being redirected.
The consequence of this is that any command that prints to stdout/stderr
may break the TAP specification by printing invalid lines. And in fact
we already do that, as git-init(1) prints the path to the created Git
repository by default.
Fix this issue by moving the logic into a lazy prerequisite.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t7527-builtin-fsmonitor.sh | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index b63c162f9b..d881e27466 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -25,7 +25,8 @@ maybe_timeout () {
"$@"
fi
}
-verify_fsmonitor_works () {
+
+test_lazy_prereq FSMONITOR_WORKS '
git init test_fsmonitor_smoke || return 1
GIT_TRACE_FSMONITOR="$PWD/smoke.trace" &&
@@ -50,9 +51,9 @@ verify_fsmonitor_works () {
ret=$?
rm -rf test_fsmonitor_smoke smoke.trace
return $ret
-}
+'
-if ! verify_fsmonitor_works
+if ! test_have_prereq FSMONITOR_WORKS
then
skip_all="filesystem does not deliver fsmonitor events (container/overlayfs?)"
test_done
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v2 2/4] t/test-lib: silence EBUSY errors on Windows during test cleanup
From: Patrick Steinhardt @ 2026-06-03 5:39 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20260603-pks-t7527-fix-tap-output-v2-0-cf3af5694e20@pks.im>
When tests have finished we clean up the trash directory via `rm -rf`.
On Windows this can fail with EBUSY in cases where a process still holds
some of the files open, for example when we have spawned a daemonized
process that wasn't properly terminated. We thus retry several times,
but every failure will result in error messages being printed, and that
in turn breaks the TAP output format.
One such case where this is causing issues is in t921x, which contains
tests related to Scalar. Some tests spawn the fsmonitor daemon, and we
never properly terminate it.
The obvious fix would be to ensure that we never leak any processes, but
that gets ugly fast. Instead, let's work around the issue by silencing
error messages printed by the `rm -rf` calls. We already know to print
an error when the retry loop fails, so we don't loose much.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/test-lib.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4a7357b547..d1d24c4124 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1299,10 +1299,10 @@ test_done () {
error "Tests passed but trash directory already removed before test cleanup; aborting"
cd "$TRASH_DIRECTORY/.." &&
- rm -fr "$TRASH_DIRECTORY" || {
+ rm -fr "$TRASH_DIRECTORY" 2>/dev/null || {
# try again in a bit
sleep 5;
- rm -fr "$TRASH_DIRECTORY"
+ rm -fr "$TRASH_DIRECTORY" 2>/dev/null
} ||
error "Tests passed but test cleanup failed; aborting"
fi
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v2 3/4] t/lib-git-p4: silence output when killing p4d and its watchdog
From: Patrick Steinhardt @ 2026-06-03 5:39 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20260603-pks-t7527-fix-tap-output-v2-0-cf3af5694e20@pks.im>
When stopping the p4d watchdog process via "kill -9", the shell may
print a job-control notification like:
./test-lib.sh: line 1269: 57960 Killed: 9 while true; do
if test $nr_tries_left -eq 0; then
kill -9 $p4d_pid; exit 1;
fi; sleep 1; nr_tries_left=$(($nr_tries_left - 1));
done 2> /dev/null 4>&2 (wd: ~)
This message is printed asynchronously by the shell when it reaps the
process. While harmless right now, this will cause breakage once we
enable strict parsing of the TAP protocol in a subsequent commit.
Fix this by using `wait` so that we can synchronously reap the watchdog
process and swallow the diagnostic.
While at it, deduplicate the logic we have in `stop_p4d_and_watchdog ()`
and `stop_and_cleanup_p4d ()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-git-p4.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index d22e9c684a..9108868187 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -65,6 +65,7 @@ pidfile="$TRASH_DIRECTORY/p4d.pid"
stop_p4d_and_watchdog () {
kill -9 $p4d_pid $watchdog_pid
+ wait $p4d_pid $watchdog_pid 2>/dev/null
}
# git p4 submit generates a temp file, which will
@@ -174,8 +175,7 @@ retry_until_success () {
}
stop_and_cleanup_p4d () {
- kill -9 $p4d_pid $watchdog_pid
- wait $p4d_pid
+ stop_p4d_and_watchdog
rm -rf "$db" "$cli" "$pidfile"
}
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* [PATCH v2 4/4] t: let prove fail when parsing invalid TAP output
From: Patrick Steinhardt @ 2026-06-03 5:39 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20260603-pks-t7527-fix-tap-output-v2-0-cf3af5694e20@pks.im>
To make the result of our tests accessible we use the TAP protocol. This
protocol is parsed by either prove or by Meson. Unfortunately, these two
tools differ when it comes to their strictness when parsing the
protocol:
- Prove by default happily accepts lines not specified by the
protocol.
- Meson will also accept such lines, but prints a big and ugly warning
message.
We have fixed our test suite in the past to not print invalid TAP lines
anymore via b1dc2e796e (Merge branch 'ps/meson-tap-parse', 2025-06-17).
But as none of our tools perform a strict check it's still possible for
broken tests to sneak back in, like for example in 362f69547f (Merge
branch 'ps/t1006-tap-fix', 2025-07-16). This doesn't hurt at all when
using prove, but it's quite annoying when using Meson due to the
generated warnings.
Unfortunately, there doesn't seem to be a portable way to make all tools
complain about violations of the TAP format. The TAP 14 specification
has added pragmas to the protocol that would allow us to say `pragma
+strict`, and the effect of that would be to treat invalid TAP lines as
a test failure. But the release of TAP 14 is still rather recent, and
Test-Harness for example only gained support for it in version 3.48,
which was released in 2023.
In fact though, this pragma was already introduced as an inofficial
extension of the TAP protocol with Test-Harness 3.10, released in 2008.
So while not all tools understand the pragma, at least prove does for a
long time.
Unconditionally enable the pragma when using prove so that we'll detect
tests that emit broken TAP output right away. This would have detected
the issues fixed in preceding commits:
$ prove t7527-builtin-fsmonitor.sh
t7527-builtin-fsmonitor.sh .. All 69 subtests passed
(less 6 skipped subtests: 63 okay)
Test Summary Report
-------------------
t7527-builtin-fsmonitor.sh (Wstat: 0 Tests: 69 Failed: 0)
Parse errors: Unknown TAP token: "Initialized empty Git repository in /tmp/git/test_fsmonitor_smoke/.git/"
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/test-lib.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d1d24c4124..ceefb99bff 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1532,6 +1532,12 @@ then
BAIL_OUT 'You need to build test-tool; Run "make t/helper/test-tool" in the source (toplevel) directory'
fi
+if test -n "$HARNESS_ACTIVE"
+then
+ say "TAP version 13"
+ say "pragma +strict"
+fi
+
# Are we running this test at all?
remove_trash=
this_test=${0##*/}
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
* Re: [PATCH 1/2] b4: introduce configuration for the Git project
From: Patrick Steinhardt @ 2026-06-03 6:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, git
In-Reply-To: <871peopbvf.fsf@gitster.g>
On Wed, Jun 03, 2026 at 11:59:48AM +0900, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
> > On 02/06/2026 2:32 pm, Junio C Hamano wrote:
> >> Patrick Steinhardt <ps@pks.im> writes:
> >>
> >>> We're about to extend our documentation to recommend b4 for sending
> >>> patch series ot the mailing list. Prepare for this by introducing a b4
> >>> configuration so that the tool knows to honor our preferences. For now,
> >>> this configuration does two things:
> >>> ...
> >> (hence making the tree dirty).
> >
> > Hmm, for those of us not in the know, perhaps mention the b4 documentation
> > at 'b4.docs.kernel.org' (which includes how to install b4 ... ;) ).
>
> Thanks for raising an excellent point.
I already refer to the docs in the second commit. Let me maybe reorder
them so that we first show how it's used before tweaking it.
Patrick
^ permalink raw reply
* Re: [PATCH 2/2] Documentation/MyFirstContribution: recommend the use of b4
From: Patrick Steinhardt @ 2026-06-03 6:54 UTC (permalink / raw)
To: Weijie Yuan; +Cc: git, Junio C Hamano
In-Reply-To: <ah8ALHMDVA2Gzz10@wyuan.org>
On Wed, Jun 03, 2026 at 12:09:16AM +0800, Weijie Yuan wrote:
> > +Contributors are encouraged to use `b4`, which automates much of the
> > +bookkeeping that is otherwise done by hand.
>
> So for statement like this and with my personal experience, I would say
> b4 is a more suitable option for senior contributors, as they already
> know, for example, what Message-ID and range-diffs are. But apparently,
> whose who use forges may not know.
I think it's perfectly suitable for newcomers, too. It automates so many
of the concepts that a contributor has to learn way less about mailing
list specific concepts, which reduces the learning curve.
> Back to the patch, I think regarding b4 as a more advanced contribution
> way for those who had contributed via mailing lists for more than one
> time is a better expression or formulation. Here I mean "b4 prep", other
> usage like "b4 mbox" and "b4 am" are of course more basic, and be
> mentioned as tips when interacting with Git mailing list.
>
> A bit too wordy, in conclusion: Suggest that new contributors master
> classic git operations first. When they are familiar with those process,
> b4 might be a good option.
Ah, that's what you're hinting at. So you mean to say that folks should
first understand the basics before basically automating all of the parts
for them?
I guess I can see where you're coming from, but I'm not sure I agree
with this a 100%. My main goal is to make it easier for new community
members to contribute to Git, and that means that we should automate all
the hard parts as far as possible. This saves those new contributors
from frustration, and it means that reviewers on the mailing list won't
have to teach every single new contributor about how they should thread
the mails, generate range-diffs and the like.
So in the end, it saves both their and our time, but the learning
opportunity is of course a bit diminished. I'd gladly accept that
tradeoff though.
Thanks for your input!
Patrick
^ permalink raw reply
* Re: [PATCH 1/2] b4: introduce configuration for the Git project
From: Patrick Steinhardt @ 2026-06-03 6:55 UTC (permalink / raw)
To: Weijie Yuan; +Cc: Tuomas Ahola, git, Junio C Hamano
In-Reply-To: <ah-Nhr2PboWUq6eU@wyuan.org>
On Wed, Jun 03, 2026 at 10:12:22AM +0800, Weijie Yuan wrote:
> On Tue, Jun 02, 2026 at 08:09:55PM +0300, Tuomas Ahola wrote:
> > Huh? Doesn't MyFirstContribution speak *against* shallow threading?
> >
> > [...] make sure to replace it with the correct Message-ID for your
> > **previous cover letter** - that is, if you're sending v2, use the Message-ID
> > from v1; if you're sending v3, use the Message-ID from v2.
>
> I don't get it. Doesn't shallow threading means every following patches
> are replying to the cover letter? Replying to the previous one is
> --chain-reply-to, if I'm not mistaken.
Shallow threading basically means that all patches are sent as a
response to the current cover letter, and the current cover letter is
always attached to the cover letter of the _first_ version.
So this quote is definitely at odds with the configuration I have
proposed. It's actually quite surprising to me that we recommend deep
threading -- I personally find it extremely hard to navigate as the
nesting eventually gets way too deep.
You know -- I'll include a patch that changes the wording there to also
use shallow nesting, mostly to kick off a discussion and arrive at a
decision there.
Thanks!
Patrick
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox