* [PATCH] copy: drop dependency on `the_repository`
From: Patrick Steinhardt @ 2026-07-16 9:56 UTC (permalink / raw)
To: git
When copying a file we need to potentially adapt permissions of the new
file based on whether or not "core.shared" is enabled. Parsing this
configuration makes us implicitly depend on `the_repository`.
Refactor the code to instead require the caller to pass in a repository
so that we can remove `USE_THE_REPOSITORY_VARIABLE`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Hi,
I guess the title says it all: this small patch removes the dependency
on `the_repository` in "copy.c". Thanks!
Patrick
---
builtin/clone.c | 2 +-
builtin/difftool.c | 4 ++--
builtin/worktree.c | 4 ++--
bundle-uri.c | 2 +-
copy.c | 12 ++++++------
copy.h | 8 ++++++--
refs/files-backend.c | 2 +-
rerere.c | 2 +-
sequencer.c | 6 +++---
setup.c | 2 +-
10 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index d60d1b60bc..18603dd4ce 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -335,7 +335,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
die_errno(_("failed to create link '%s'"), dest->buf);
option_no_hardlinks = 1;
}
- if (copy_file_with_time(dest->buf, src->buf, 0666))
+ if (copy_file_with_time(the_repository, dest->buf, src->buf, 0666))
die_errno(_("failed to copy file to '%s'"), dest->buf);
}
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 26778f8515..5e7777fbe4 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -552,7 +552,7 @@ static int run_dir_diff(struct repository *repo,
struct stat st;
if (stat(wtdir.buf, &st))
st.st_mode = 0644;
- if (copy_file(rdir.buf, wtdir.buf,
+ if (copy_file(repo, rdir.buf, wtdir.buf,
st.st_mode)) {
ret = error("could not copy '%s' to '%s'", wtdir.buf, rdir.buf);
goto finish;
@@ -658,7 +658,7 @@ static int run_dir_diff(struct repository *repo,
warning("%s", "");
err = 1;
} else if (unlink(wtdir.buf) ||
- copy_file(wtdir.buf, rdir.buf, st.st_mode))
+ copy_file(repo, wtdir.buf, rdir.buf, st.st_mode))
warning_errno(_("could not copy '%s' to '%s'"),
rdir.buf, wtdir.buf);
}
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d21c43fde3..84b01960fb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -349,7 +349,7 @@ static void copy_sparse_checkout(const char *worktree_git_dir)
if (file_exists(from_file)) {
if (safe_create_leading_directories(the_repository, to_file) ||
- copy_file(to_file, from_file, 0666))
+ copy_file(the_repository, to_file, from_file, 0666))
error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
from_file, to_file);
}
@@ -368,7 +368,7 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)
int bare;
if (safe_create_leading_directories(the_repository, to_file) ||
- copy_file(to_file, from_file, 0666)) {
+ copy_file(the_repository, to_file, from_file, 0666)) {
error(_("failed to copy worktree config from '%s' to '%s'"),
from_file, to_file);
goto worktree_copy_cleanup;
diff --git a/bundle-uri.c b/bundle-uri.c
index 3b2e347288..ef37aebf30 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -396,7 +396,7 @@ static int copy_uri_to_file(const char *filename, const char *uri)
uri = out;
/* Copy as a file */
- return copy_file(filename, uri, 0);
+ return copy_file(the_repository, filename, uri, 0);
}
static int unbundle_from_file(struct repository *r, const char *file)
diff --git a/copy.c b/copy.c
index b668209b6c..6074132050 100644
--- a/copy.c
+++ b/copy.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "git-compat-util.h"
#include "copy.h"
#include "path.h"
@@ -35,7 +33,8 @@ static int copy_times(const char *dst, const char *src)
return 0;
}
-int copy_file(const char *dst, const char *src, int mode)
+int copy_file(struct repository *repo,
+ const char *dst, const char *src, int mode)
{
int fdi, fdo, status;
@@ -59,15 +58,16 @@ int copy_file(const char *dst, const char *src, int mode)
if (close(fdo) != 0)
return error_errno("%s: close error", dst);
- if (!status && adjust_shared_perm(the_repository, dst))
+ if (!status && adjust_shared_perm(repo, dst))
return -1;
return status;
}
-int copy_file_with_time(const char *dst, const char *src, int mode)
+int copy_file_with_time(struct repository *repo,
+ const char *dst, const char *src, int mode)
{
- int status = copy_file(dst, src, mode);
+ int status = copy_file(repo, dst, src, mode);
if (!status)
return copy_times(dst, src);
return status;
diff --git a/copy.h b/copy.h
index 2af77cba86..1059b118d6 100644
--- a/copy.h
+++ b/copy.h
@@ -1,10 +1,14 @@
#ifndef COPY_H
#define COPY_H
+struct repository;
+
#define COPY_READ_ERROR (-2)
#define COPY_WRITE_ERROR (-3)
int copy_fd(int ifd, int ofd);
-int copy_file(const char *dst, const char *src, int mode);
-int copy_file_with_time(const char *dst, const char *src, int mode);
+int copy_file(struct repository *repo,
+ const char *dst, const char *src, int mode);
+int copy_file_with_time(struct repository *repo,
+ const char *dst, const char *src, int mode);
#endif /* COPY_H */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3df56c25c8..442c98414e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1736,7 +1736,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
goto out;
}
- if (copy && log && copy_file(tmp_renamed_log.buf, sb_oldref.buf, 0644)) {
+ if (copy && log && copy_file(refs->base.repo, tmp_renamed_log.buf, sb_oldref.buf, 0644)) {
ret = error("unable to copy logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
goto out;
diff --git a/rerere.c b/rerere.c
index 8232542585..bf5cfc6e51 100644
--- a/rerere.c
+++ b/rerere.c
@@ -756,7 +756,7 @@ static void do_rerere_one_path(struct index_state *istate,
/* Has the user resolved it already? */
if (variant >= 0) {
if (!handle_file(istate, path, NULL, NULL)) {
- copy_file(rerere_path(&buf, id, "postimage"), path, 0666);
+ copy_file(the_repository, rerere_path(&buf, id, "postimage"), path, 0666);
id->collection->status[variant] |= RR_HAS_POSTIMAGE;
fprintf_ln(stderr, _("Recorded resolution for '%s'."), path);
free_rerere_id(rr_item);
diff --git a/sequencer.c b/sequencer.c
index 1355a99a09..c9ede9c02d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2419,7 +2419,7 @@ static int do_pick_commit(struct repository *r,
} else {
const char *dest = git_path_squash_msg(r);
unlink(dest);
- if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
+ if (copy_file(the_repository, dest, rebase_path_squash_msg(), 0666)) {
res = error(_("could not copy '%s' to '%s'"),
rebase_path_squash_msg(), dest);
goto leave;
@@ -3864,11 +3864,11 @@ static int error_failed_squash(struct repository *r,
int subject_len,
const char *subject)
{
- if (copy_file(rebase_path_message(), rebase_path_squash_msg(), 0666))
+ if (copy_file(the_repository, rebase_path_message(), rebase_path_squash_msg(), 0666))
return error(_("could not copy '%s' to '%s'"),
rebase_path_squash_msg(), rebase_path_message());
unlink(git_path_merge_msg(r));
- if (copy_file(git_path_merge_msg(r), rebase_path_message(), 0666))
+ if (copy_file(the_repository, git_path_merge_msg(r), rebase_path_message(), 0666))
return error(_("could not copy '%s' to '%s'"),
rebase_path_message(),
git_path_merge_msg(r));
diff --git a/setup.c b/setup.c
index 0de56a074f..91d61a5939 100644
--- a/setup.c
+++ b/setup.c
@@ -2331,7 +2331,7 @@ static void copy_templates_1(struct repository *repo,
strbuf_release(&lnk);
}
else if (S_ISREG(st_template.st_mode)) {
- if (copy_file(path->buf, template_path->buf, st_template.st_mode))
+ if (copy_file(repo, path->buf, template_path->buf, st_template.st_mode))
die_errno(_("cannot copy '%s' to '%s'"),
template_path->buf, path->buf);
}
---
base-commit: d35c5399e3e54ac277bb391fc2f6be3e816d312b
change-id: 20260716-pks-copy-wo-the-repository-aa01ccdbed76
^ permalink raw reply related
* Re: [PATCH] stash: add 'rename' subcommand
From: Patrick Steinhardt @ 2026-07-16 10:08 UTC (permalink / raw)
To: Emin Özata via GitGitGadget
Cc: git, Junio C Hamano, Greg Hewgill, Micheil Smith,
Michael Haggerty, Ævar Arnfjörð Bjarmason,
Emin Özata
In-Reply-To: <pull.2180.git.1784190706028.gitgitgadget@gmail.com>
On Thu, Jul 16, 2026 at 08:31:45AM +0000, Emin Özata via GitGitGadget wrote:
> From: =?UTF-8?q?Emin=20=C3=96zata?= <eminozata@proton.me>
>
> There is no way to change the message of a stash entry after the
> fact. The only option is dropping the entry and re-storing it by
> hand, which moves it to the top of the stash list and gets fiddly
> for deeper entries.
>
> Add 'git stash rename <message> [<stash>]', defaulting to the
> latest entry like the other subcommands do. It reads the object id
> and reflog message of the target entry and of the entries above it,
> drops them all like 'git stash drop' would, and stores them back in
> the same order, with the new message going to the target. Position,
> contents and the reflog chain stay as they were.
>
> The command checks every entry it is about to rewrite and refuses
> to start if one of them does not look like a stash commit, which
> can only happen when refs/stash was written to by hand. Finding
> that out halfway through the sequence would lose entries. Should a
> write-back fail anyway, the entry's object id is reported so it can
> be recovered with 'git stash store', and the command only reports
> success when the reflog ended up in the requested state.
>
> This was proposed before: in 2010, as a "git reflog update" command
> that edited reflog entries in place [1]. When it came up again in
> 2013 [2], Junio rejected it on the grounds that reflogs are
> append-only recovery logs, and that whoever really cares about a
> stash message can pop and re-stash [3]. Michael Haggerty pointed
> out in that thread that refs/stash does not fit the description:
> its reflog is the primary data store for stash entries, and 'git
> stash drop' rewrites it all the time [4]. So this patch stays away
> from the reflog machinery entirely and does the suggested
> pop-and-re-stash workaround mechanically, without the detour
> through the working tree.
Hm. It's good to refer to to previous discussions. But I think it would
make sense to also document why explicitly _you_ want to have this
functionality. Like, what use case does it enable that you currently
cannot have right now? How is this different to what was proposed back
then that should make us reconsider whether or not to include it now?
> diff --git a/Documentation/git-stash.adoc b/Documentation/git-stash.adoc
> index 50bb89f483..03f2e03096 100644
> --- a/Documentation/git-stash.adoc
> +++ b/Documentation/git-stash.adoc
> @@ -163,6 +164,12 @@ with no conflicts.
> created by `export`, and add them to the list of stashes. To replace the
> existing stashes, use `clear` first.
>
> +`rename [-q | --quiet] <message> [<stash>]`::
> + Change the message of a single stash entry. The entry keeps its
> + position and its contents. _<stash>_ must name an entry by
> + index (e.g. `stash@{1}`); renaming refreshes the reflog
> + timestamps of the entry and of the entries above it.
I think "rename" is a bit of a misleading name, doubly so with the
recently introduced `git refs rename` feature that renames a reference.
I'd suggest "reword" instead.
> diff --git a/builtin/stash.c b/builtin/stash.c
> index c4809f299a..94e66d6074 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1190,6 +1204,166 @@ out:
[snip]
> +static int do_rename_stash(struct stash_info *info, size_t idx,
> + const char *msg, int quiet)
> +{
> + struct rename_data data = { .want = idx + 1 };
> + size_t i, missing = 0;
> + int ret = -1;
> +
> + refs_for_each_reflog_ent_reverse(get_main_ref_store(the_repository),
> + ref_stash, collect_rename_entries,
> + &data);
> + if (data.nr <= idx) {
> + error(_("%s does not exist"), info->revision.buf);
> + goto cleanup;
> + }
> +
> + if (!oideq(&info->w_commit, &data.entries[idx].oid)) {
> + error(_("%s changed concurrently; try again"),
> + info->revision.buf);
> + goto cleanup;
> + }
> +
> + /* refuse up front; do_store_stash() would die halfway through */
> + for (i = 0; i < data.nr; i++) {
> + struct commit *stash = lookup_commit_reference(the_repository,
> + &data.entries[i].oid);
> +
> + if (!stash || check_stash_topology(the_repository, stash)) {
> + error(_("%s does not look like a stash commit"),
> + oid_to_hex(&data.entries[i].oid));
> + goto cleanup;
> + }
> + }
This loop here has potentially-quadratic runtime. Not so much with the
"files" backend, where we'll simply append the data to the log. But with
the reftable backend we'll basically end up writing each reflog entry
into a new table, and we'll end up compacting the tables many times
over.
> +
> + while (missing <= idx) {
> + if (drop_reflog_entry("stash@{0}"))
> + goto restore;
> + missing++;
> + }
Same here, this will not perform well if you have a huge reflog.
We really should do all of this atomically, where we ideally delete the
old reflog and create the new reflog in a single transaction.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH 2/3] doc: document history signing options
From: Patrick Steinhardt @ 2026-07-16 10:18 UTC (permalink / raw)
To: Souma; +Cc: git, gitster
In-Reply-To: <20260703145037.69832-3-git@5ouma.me>
On Fri, Jul 03, 2026 at 11:50:36PM +0900, Souma wrote:
> The history manual and usage text should describe the signing controls now
> accepted by fixup, reword, and split.
>
> Document -S/--gpg-sign and --no-gpg-sign with the same key-id spelling and
> configuration override behavior used by commit-style signing options.
>
> Signed-off-by: Souma <git@5ouma.me>
> ---
> Documentation/git-history.adoc | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
I think this and the next commit can easily be merged into the first
one. They really belong together, and even worse t0450 probably breaks
with the first commit, only, as the change to the synopsis in our docs
and in the command itself is split up across two commits.
Patrick
^ permalink raw reply
* Re: [PATCH 1/3] builtin/history: sign rewritten commits
From: Patrick Steinhardt @ 2026-07-16 10:18 UTC (permalink / raw)
To: Souma; +Cc: git, gitster
In-Reply-To: <20260703145037.69832-2-git@5ouma.me>
On Fri, Jul 03, 2026 at 11:50:35PM +0900, Souma wrote:
> diff --git a/builtin/history.c b/builtin/history.c
> index 091465a59e..8d669cf539 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> @@ -98,6 +98,30 @@ enum commit_tree_flags {
> COMMIT_TREE_EDIT_MESSAGE = (1 << 0),
> };
>
> +static int history_config(const char *var, const char *value,
> + const struct config_context *ctx, void *data)
> +{
> + const char **sign_commit = data;
> +
> + if (!strcmp(var, "commit.gpgsign")) {
> + *sign_commit = git_config_bool(var, value) ? "" : NULL;
> + return 0;
> + }
> +
> + return git_default_config(var, value, ctx, data);
Shouldn't we rather pass `NULL` instead of `data`? It works, sure, but
only because `git_default_config()` doesn't use `data` at all.
> @@ -160,7 +185,8 @@ static int commit_tree_ext(struct repository *repo,
> static int commit_tree_with_edited_message(struct repository *repo,
> const char *action,
> struct commit *original,
> - struct commit **out)
> + struct commit **out,
> + const char *sign_commit)
Nit: the `out` parameter should continue to be the last one.
> @@ -515,12 +546,13 @@ static int cmd_history_fixup(int argc,
> bool skip_commit = false;
> int ret;
>
> + repo_config(repo, history_config, &sign_commit);
> +
> argc = parse_options(argc, argv, prefix, options, usage, 0);
> if (argc != 1) {
> ret = error(_("command expects a single revision"));
> goto out;
> }
> - repo_config(repo, git_default_config, NULL);
>
> if (action == REF_ACTION_DEFAULT)
> action = REF_ACTION_BRANCHES;
It might make sense to document in the commit message why we have to
change the order. I guess it's because of precedence, but not everyone
might realize that immediately.
> @@ -785,7 +822,8 @@ static int write_ondisk_index(struct repository *repo,
> static int split_commit(struct repository *repo,
> struct commit *original,
> struct pathspec *pathspec,
> - struct commit **out)
> + struct commit **out,
> + const char *sign_commit)
> {
> struct interactive_options interactive_opts = INTERACTIVE_OPTIONS_INIT;
> struct strbuf index_file = STRBUF_INIT;
Likewise, let's ensure that the `out` parameter remains last.
> diff --git a/replay.c b/replay.c
> index da531d5bc6..683c384ef8 100644
> --- a/replay.c
> +++ b/replay.c
It might make sense to split out the changes to "replay.c" into a
preparatory commit.
One interesting question is whether it really makes sense to sign _all_
commits. It's rather likely that the history will contain commits that
aren't even owned by you, so signing them with your signature might be a
bit of a weird choice. I guess that might be okay-ish, but it's
certainly something that's worth a discussion as part of the commit
message.
Thanks!
Patrick
^ permalink raw reply
* [PATCH] revision: fix --no-walk path filtering regression
From: Kristofer Karlsson via GitGitGadget @ 2026-07-16 10:47 UTC (permalink / raw)
To: git; +Cc: Peter Colberg, Kristofer Karlsson, Kristofer Karlsson
From: Kristofer Karlsson <krka@spotify.com>
Since dd4bc01c0a (revision: use priority queue for non-limited
streaming walks, 2026-05-27), "git rev-list --no-walk <commit>
-- <path>" ignores the path arguments and outputs all commits
regardless of whether they touch the given paths.
That commit introduced a REV_WALK_NO_WALK enum value to separate
--no-walk from the streaming walk in get_revision_1(). The new
case skips process_parents(), which is correct for not enqueuing
parents, but also skips try_to_simplify_commit() which
process_parents() calls to evaluate whether each commit touches
the given paths.
Add a call to try_to_simplify_commit() for the
REV_WALK_NO_WALK case, folding it into the existing
REV_WALK_REFLOG case which already does the same.
Add tests for --no-walk path filtering to t6017. The
"single commit, match" test is defensive and passes without
the fix, while the other two fail without it.
Reported-by: Peter Colberg <pcolberg@redhat.com>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
revision: fix --no-walk path filtering regression
Fix for a regression reported by Peter Colberg [1] where git rev-list
--no-walk <commit> -- <path> ignores path arguments since dd4bc01c0a.
Verified against linux.git with the exact example from the report:
git rev-list --topo-order v7.0..v7.1 -- drivers/gpu/drm/ |
git rev-list --stdin --no-walk=unsorted -- ':!drivers/gpu/drm/'
Without fix: 2026 commits (all pass through unfiltered) With fix: 146
commits (correctly filtered)
[1]
https://lore.kernel.org/git/CAL71e4NjDTHbKR8z7pSrPpzDrX19JOTR04sArm7P=m5ivqkskA@mail.gmail.com/T/#u
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2181%2Fspkrka%2Fkk%2Fno-walk-pathspec-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2181/spkrka/kk/no-walk-pathspec-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2181
revision.c | 2 +-
t/t6017-rev-list-stdin.sh | 18 ++++++++++++++++++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/revision.c b/revision.c
index ccbe2e03d1..e990e3f96b 100644
--- a/revision.c
+++ b/revision.c
@@ -4419,6 +4419,7 @@ static struct commit *get_revision_1(struct rev_info *revs)
switch (mode) {
case REV_WALK_REFLOG:
+ case REV_WALK_NO_WALK:
try_to_simplify_commit(revs, commit);
break;
case REV_WALK_TOPO:
@@ -4432,7 +4433,6 @@ static struct commit *get_revision_1(struct rev_info *revs)
oid_to_hex(&commit->object.oid));
}
break;
- case REV_WALK_NO_WALK:
case REV_WALK_LIMITED:
break;
}
diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh
index 4821b90e74..32284f1831 100755
--- a/t/t6017-rev-list-stdin.sh
+++ b/t/t6017-rev-list-stdin.sh
@@ -148,4 +148,22 @@ test_expect_success '--not via stdin does not influence revisions from command l
test_cmp expect actual
'
+test_expect_success '--no-walk filters by path (single commit, match)' '
+ git rev-parse side-1 >expect &&
+ git rev-list --no-walk side-1 -- file-1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--no-walk filters by path (single commit, no match)' '
+ git rev-list --no-walk side-2 -- file-1 >actual &&
+ test_must_be_empty actual
+'
+
+test_expect_success '--no-walk with pathspec exclusion' '
+ git rev-parse side-3 side-2 >expect &&
+ git rev-parse side-1 side-2 side-3 >input &&
+ git rev-list --stdin --no-walk -- ":!file-1" <input >actual &&
+ test_cmp expect actual
+'
+
test_done
base-commit: d35c5399e3e54ac277bb391fc2f6be3e816d312b
--
gitgitgadget
^ permalink raw reply related
* Re: git-last-modified(1) slower than git-log(1)?
From: Toon Claes @ 2026-07-16 11:42 UTC (permalink / raw)
To: Jeff King, Gusted; +Cc: git, Taylor Blau
In-Reply-To: <20260716042808.GA1151612@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Jul 14, 2026 at 08:33:59PM +0200, Gusted wrote:
>
>> The repository I'm currently using to evaluate the performance is
>> https://codeberg.org/ziglang/zig
>>
>> Reproduction steps:
>> 1. `git clone https://codeberg.org/ziglang/zig $(mktemp -d)`
>> 2. cd to tmp directory.
>> 3. `git commit-graph write --changed-paths`. As git-last-modified(1)
>> makes good use of the bloom filters.
>> 4. `hyperfine 'git last-modified -z -t --max-depth=0
>> 80d06578ac66bce3aa0a21e9610cdb782b9a0593 -- doc/langref/' 'git log
>> --name-status -c "--format=commit%x00%H %P%x00" --parents --no-renames
>> -t -z 80d06578ac66bce3aa0a21e9610cdb782b9a0593 -- ":(literal)doc/langref"'`
>
> Thanks for this concrete reproduction. I can see the same problem
> here.
As I mentioned, I was aware of that issue, but never felt the need (and
didn't have the deep Bloom filter knowledge) to fix it.
> Interestingly, if we turn off changed-paths, we get very different
> results.
>
> Without a commit graph at all, last-modified wins (this is using the zig
> repo and the commands above):
>
> - log: 150ms
> - last-modified: 79ms
>
> But with a graph and no changed-paths, they're about equal:
>
> - log: 61ms
> - last-modified: 61ms
>
> And then with changed-paths, the log command gets much faster but
> last-modified gets slower!
>
> - log: 20ms
> - last-modified: 64ms
>
> I think there's a tradeoff in the way that last-modified uses the bloom
> filters. It makes a key for every path we're interested in, and then for
> each commit, we check each key to say "is this in the commit's filter?".
>
> So if you have a subdirectory with a non-trivial number of entries (like
> doc/langref here which has 290), but most commits don't touch that path
> at all (only 120 out of ~39k in this case), we'll spend a lot of time
> checking each key against each filter. We save ourselves opening the
> trees, but at the cost of 290*39k filter comparisons).
>
> Whereas in the git-log case, we make a filter key out of the single
> pathspec we're given, and then check each commit against that. So we
> only do a single filter check for each commit to narrow it down to those
> 120 that matter (modulo a few filter false positives).
Oh, that's very useful of you to explain this. Thank you.
> But I don't see any reason that last-modified couldn't _also_ do that:
> pre-filter the commits with a commit matching the original pathspec, and
> discard most commits with a single filter check.
>
> The hacky patch below does this, and brings my last-modified runtime
> down to 16ms (a 4x improvement, and just a bit faster than git-log).
Funny, I was toying around with my AI agent, and they came with a
similar solution, but I'm working on a cleaner solution.
> It tries to reuse the logic from revision.c, so it's doing the exact
> same filtering that git-log would do. I think there are other ways to do
> it. E.g., we could make our own "root" bloom key that contains all of
> the paths and pre-filter with that. But it seemed to be a little slower
> when I tried it (~24ms). I'd guess that the problem is that because the
> bloom filter is probabilistic, if you shove too many items into a single
> key you'll end getting more and more false positives. So putting all 290
> entries into one key is too much, and we are better off just considering
> the shared prefix.
>
> Anyway, here's the patch. Toon, I'm not planning to take it further
> immediately, but you may be interested in poking at it. It probably
> needs at least:
>
> - some light refactoring of revision.c
Agreed.
> - tests? We don't seem to cover last-modified with changed-paths at
> all, and just rely on the test-vars CI job which sets
> GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS. It did pass for me with that
> flag, so surely I didn't introduce any bugs. :)
Fair of you calling that out. Thanks for checking.
> - more timing exploration; e.g., might it make things worse if
> doc/langref were touched in 99% of the commits? Probably not, but it
> might be nice to check timings against a few repo shapes and request
> depths.
Maybe, I tried a few things.
On gitlab-org/gitlab (our Rails monolith), there seems to be a noticable
improvement when running for `app/`:
Benchmark 1: old
Time (mean ± σ): 435.5 ms ± 9.8 ms [User: 369.5 ms, System: 64.4 ms]
Range (min … max): 425.3 ms … 450.9 ms 5 runs
Benchmark 2: new
Time (mean ± σ): 278.5 ms ± 32.3 ms [User: 208.0 ms, System: 69.3 ms]
Range (min … max): 246.2 ms … 314.8 ms 5 runs
Summary
new ran
1.56 ± 0.18 times faster than old
The app/ directory is touched by roughly 35% of the commits.
In gitlab-org/gitaly, I ran a benchmark on `internal/`, which is touched
in about 58% of the commits:
Benchmark 1: old
Time (mean ± σ): 13.2 ms ± 1.3 ms [User: 10.4 ms, System: 2.5 ms]
Range (min … max): 11.3 ms … 14.8 ms 10 runs
Benchmark 2: new
Time (mean ± σ): 9.9 ms ± 0.4 ms [User: 7.3 ms, System: 2.4 ms]
Range (min … max): 9.6 ms … 10.8 ms 10 runs
Summary
new ran
1.33 ± 0.14 times faster than old
(although Gitaly a lot less commits, ~23k commits vs ~530k in our Rails
monolith)
And also --recursive it's faster:
Benchmark 1: old
Time (mean ± σ): 204.6 ms ± 4.4 ms [User: 193.2 ms, System: 10.5 ms]
Range (min … max): 199.9 ms … 213.2 ms 10 runs
Benchmark 2: new
Time (mean ± σ): 181.5 ms ± 2.7 ms [User: 170.9 ms, System: 9.7 ms]
Range (min … max): 177.0 ms … 187.1 ms 10 runs
Summary
new ran
1.13 ± 0.03 times faster than old
Personally I'm not too worried any use-case would be at least equally
fast.
> - Not all pathspecs can support bloom filters (e.g., "*.c" would not).
> So in theory:
>
> git last-modified HEAD -- "*.c"
>
> could work, but wouldn't be optimized. I don't think it _does_ work
> now, because last-modified's max-depth logic complains. So it might
> be a non-issue.
>
> But I think it is solvable if we really wanted. Rather than
> traversing looking for "*.c", we actually expand the pathspec in the
> tip commit to a set of literal paths, and then as we traverse we
> look for those paths. So we could collect all of "*.c" and then
> add bloom keys for the shared prefixes. I think this does get tricky
> in the general case, though. If you have "a/b/c" and "a/b/d",
> looking for "a/b" is reasonable. But what if you also have "a/e"?
> Should you just have a key for "a/", or both "a/b" and "a/e"?
> There are some tradeoffs between how often uninteresting things in
> "a/" will give us a false positive, versus the cost of checking
> extra keys.
>
> So maybe an interesting area, but given that in practice most people
> will feed a single pathspec to last-modified, it's a lot easier to
> just use that.
Yeah, I rather not deal with that right now.
> - I know that last-modified was derived from GitHub's blame-tree
> implementation (which I originally wrote, but stopped paying
> attention to well before it learned about changed-path filters). I
> don't know if the problem was solved separately there, but it would
> be worth checking. +cc Taylor
>
> -Peff
>
> ---
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index 5478182f2e..c07169258f 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -254,6 +254,29 @@ static void pass_to_parent(struct bitmap *c,
> bitmap_set(p, pos);
> }
>
> +/*
> + * revision.c already has this functionality, but it is not public
> + * and it looks up the filter itself. But probably some refactoring
> + * could make it available at the right level?
I assume you're talking about check_maybe_different_in_bloom_filter()?
I was working on a fix to simply make it public and call it, but that's
a very valid point you're making. I'll change my plans.
> + */
> +static bool filter_contains_keyvec(const struct bloom_filter *filter,
> + struct rev_info *rev)
> +{
> ... [snip]
--
Cheers,
Toon
^ permalink raw reply
* [PATCH 0/3] refspec: remove dependency on `the_repository`
From: Patrick Steinhardt @ 2026-07-16 12:38 UTC (permalink / raw)
To: git
Hi,
this small patch series removes the dependency on `the_repository` in
"refspec.c". Thanks!
Patrick
---
Patrick Steinhardt (3):
refspec: group related structures and functions
refspec: let callers pass in hash algorithm when parsing items
refspec: stop depending on `the_repository`
builtin/fast-export.c | 4 +++-
builtin/fetch.c | 9 ++++++---
builtin/pull.c | 2 +-
builtin/push.c | 6 ++++--
builtin/send-pack.c | 5 ++++-
builtin/submodule--helper.c | 2 +-
http-push.c | 2 +-
refspec.c | 39 +++++++++++++++++++++------------------
refspec.h | 42 +++++++++++++++++++++++++++---------------
remote.c | 6 +++---
transport-helper.c | 2 +-
11 files changed, 72 insertions(+), 47 deletions(-)
---
base-commit: d35c5399e3e54ac277bb391fc2f6be3e816d312b
change-id: 20260716-pks-refspec-wo-the-repository-24a6fd303548
^ permalink raw reply
* [PATCH 1/3] refspec: group related structures and functions
From: Patrick Steinhardt @ 2026-07-16 12:38 UTC (permalink / raw)
To: git
In-Reply-To: <20260716-pks-refspec-wo-the-repository-v1-0-aa40844d067f@pks.im>
Reorganize the refspec header a bit so that structures and their related
functions are grouped closer together. While at it, fix a couple of
style violations.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refspec.h | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/refspec.h b/refspec.h
index 8b04f9995e..832d6f923c 100644
--- a/refspec.h
+++ b/refspec.h
@@ -1,6 +1,9 @@
#ifndef REFSPEC_H
#define REFSPEC_H
+struct string_list;
+struct strvec;
+
#define TAG_REFSPEC "refs/tags/*:refs/tags/*"
/**
@@ -30,10 +33,9 @@ struct refspec_item {
char *raw;
};
-struct string_list;
-
-#define REFSPEC_INIT_FETCH { .fetch = 1 }
-#define REFSPEC_INIT_PUSH { .fetch = 0 }
+int refspec_item_init_fetch(struct refspec_item *item, const char *refspec);
+int refspec_item_init_push(struct refspec_item *item, const char *refspec);
+void refspec_item_clear(struct refspec_item *item);
/**
* An array of strings can be parsed into a struct refspec using
@@ -47,20 +49,20 @@ struct refspec {
unsigned fetch : 1;
};
-int refspec_item_init_fetch(struct refspec_item *item, const char *refspec);
-int refspec_item_init_push(struct refspec_item *item, const char *refspec);
-void refspec_item_clear(struct refspec_item *item);
+#define REFSPEC_INIT_FETCH { .fetch = 1 }
+#define REFSPEC_INIT_PUSH { .fetch = 0 }
+
void refspec_init_fetch(struct refspec *rs);
void refspec_init_push(struct refspec *rs);
+void refspec_clear(struct refspec *rs);
+
void refspec_append(struct refspec *rs, const char *refspec);
__attribute__((format (printf,2,3)))
void refspec_appendf(struct refspec *rs, const char *fmt, ...);
void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
-void refspec_clear(struct refspec *rs);
int valid_fetch_refspec(const char *refspec);
-struct strvec;
/*
* Determine what <prefix> values to pass to the peer in ref-prefix lines
* (see linkgit:gitprotocol-v2[5]).
@@ -76,7 +78,7 @@ int refname_matches_negative_refspec_item(const char *refname, struct refspec *r
* Returns 1 if refname matches pattern, 0 otherwise.
*/
int match_refname_with_pattern(const char *pattern, const char *refname,
- const char *replacement, char **result);
+ const char *replacement, char **result);
/*
* Queries a refspec for a match and updates the query item.
@@ -89,8 +91,8 @@ int refspec_find_match(struct refspec *rs, struct refspec_item *query);
* list.
*/
void refspec_find_all_matches(struct refspec *rs,
- struct refspec_item *query,
- struct string_list *results);
+ struct refspec_item *query,
+ struct string_list *results);
/*
* Remove all entries in the input list which match any negative refspec in
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* [PATCH 2/3] refspec: let callers pass in hash algorithm when parsing items
From: Patrick Steinhardt @ 2026-07-16 12:38 UTC (permalink / raw)
To: git
In-Reply-To: <20260716-pks-refspec-wo-the-repository-v1-0-aa40844d067f@pks.im>
When parsing a refspec item we need to know about the hash algorithm
used by the repository so that we can decide whether or not a given
string is an exact object ID. We use `the_hash_algo` for this, which
makes the code implicitly depend on `the_repository`.
Refactor `refspec_item_init_fetch()`, `refspec_item_init_push()` and
`valid_fetch_refspec()` so that callers have to pass in the hash
algorithm explicitly and adapt callers accordingly. For now, all of
the callers simply pass `the_hash_algo`, so there is no change in
behaviour.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 3 ++-
builtin/pull.c | 2 +-
refspec.c | 30 +++++++++++++++++-------------
refspec.h | 9 ++++++---
remote.c | 2 +-
5 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8e676b79ba..1d4a129039 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -601,7 +601,8 @@ static struct ref *get_ref_map(struct remote *remote,
struct refspec_item tag_refspec;
/* also fetch all tags */
- refspec_item_init_push(&tag_refspec, TAG_REFSPEC);
+ refspec_item_init_push(&tag_refspec, TAG_REFSPEC,
+ the_hash_algo);
get_fetch_map(remote_refs, &tag_refspec, &tail, 0);
refspec_item_clear(&tag_refspec);
} else if (tags == TAGS_DEFAULT && *autotags) {
diff --git a/builtin/pull.c b/builtin/pull.c
index d49b09114a..db3ee0aab3 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -612,7 +612,7 @@ static const char *get_tracking_branch(const char *remote, const char *refspec)
const char *spec_src;
const char *merge_branch;
- if (!refspec_item_init_fetch(&spec, refspec))
+ if (!refspec_item_init_fetch(&spec, refspec, the_hash_algo))
die(_("invalid refspec '%s'"), refspec);
spec_src = spec.src;
if (!*spec_src || !strcmp(spec_src, "HEAD"))
diff --git a/refspec.c b/refspec.c
index fb89bce1db..33a6fb8e45 100644
--- a/refspec.c
+++ b/refspec.c
@@ -16,7 +16,8 @@
* Parses the provided refspec 'refspec' and populates the refspec_item 'item'.
* Returns 1 if successful and 0 if the refspec is invalid.
*/
-static int parse_refspec(struct refspec_item *item, const char *refspec, int fetch)
+static int parse_refspec(struct refspec_item *item, const char *refspec,
+ const struct git_hash_algo *algo, int fetch)
{
size_t llen;
int is_glob;
@@ -84,7 +85,7 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet
*/
if (!*item->src)
return 0; /* negative refspecs must not be empty */
- else if (llen == the_hash_algo->hexsz && !get_oid_hex(item->src, &unused))
+ else if (llen == algo->hexsz && !get_oid_hex_algop(item->src, &unused, algo))
return 0; /* negative refspecs cannot be exact sha1 */
else if (!check_refname_format(item->src, flags))
; /* valid looking ref is ok */
@@ -101,7 +102,7 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet
/* LHS */
if (!*item->src)
; /* empty is ok; it means "HEAD" */
- else if (llen == the_hash_algo->hexsz && !get_oid_hex(item->src, &unused))
+ else if (llen == algo->hexsz && !get_oid_hex_algop(item->src, &unused, algo))
item->exact_sha1 = 1; /* ok */
else if (!check_refname_format(item->src, flags))
; /* valid looking ref is ok */
@@ -154,21 +155,23 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet
}
static int refspec_item_init(struct refspec_item *item, const char *refspec,
- int fetch)
+ const struct git_hash_algo *algo, int fetch)
{
memset(item, 0, sizeof(*item));
item->raw = xstrdup(refspec);
- return parse_refspec(item, refspec, fetch);
+ return parse_refspec(item, refspec, algo, fetch);
}
-int refspec_item_init_fetch(struct refspec_item *item, const char *refspec)
+int refspec_item_init_fetch(struct refspec_item *item, const char *refspec,
+ const struct git_hash_algo *algo)
{
- return refspec_item_init(item, refspec, 1);
+ return refspec_item_init(item, refspec, algo, 1);
}
-int refspec_item_init_push(struct refspec_item *item, const char *refspec)
+int refspec_item_init_push(struct refspec_item *item, const char *refspec,
+ const struct git_hash_algo *algo)
{
- return refspec_item_init(item, refspec, 0);
+ return refspec_item_init(item, refspec, algo, 0);
}
void refspec_item_clear(struct refspec_item *item)
@@ -200,9 +203,9 @@ void refspec_append(struct refspec *rs, const char *refspec)
int ret;
if (rs->fetch)
- ret = refspec_item_init_fetch(&item, refspec);
+ ret = refspec_item_init_fetch(&item, refspec, the_hash_algo);
else
- ret = refspec_item_init_push(&item, refspec);
+ ret = refspec_item_init_push(&item, refspec, the_hash_algo);
if (!ret)
die(_("invalid refspec '%s'"), refspec);
@@ -246,10 +249,11 @@ void refspec_clear(struct refspec *rs)
rs->fetch = 0;
}
-int valid_fetch_refspec(const char *fetch_refspec_str)
+int valid_fetch_refspec(const char *fetch_refspec_str,
+ const struct git_hash_algo *algo)
{
struct refspec_item refspec;
- int ret = refspec_item_init_fetch(&refspec, fetch_refspec_str);
+ int ret = refspec_item_init_fetch(&refspec, fetch_refspec_str, algo);
refspec_item_clear(&refspec);
return ret;
}
diff --git a/refspec.h b/refspec.h
index 832d6f923c..e482b720a8 100644
--- a/refspec.h
+++ b/refspec.h
@@ -1,6 +1,7 @@
#ifndef REFSPEC_H
#define REFSPEC_H
+struct git_hash_algo;
struct string_list;
struct strvec;
@@ -33,8 +34,10 @@ struct refspec_item {
char *raw;
};
-int refspec_item_init_fetch(struct refspec_item *item, const char *refspec);
-int refspec_item_init_push(struct refspec_item *item, const char *refspec);
+int refspec_item_init_fetch(struct refspec_item *item, const char *refspec,
+ const struct git_hash_algo *algo);
+int refspec_item_init_push(struct refspec_item *item, const char *refspec,
+ const struct git_hash_algo *algo);
void refspec_item_clear(struct refspec_item *item);
/**
@@ -61,7 +64,7 @@ __attribute__((format (printf,2,3)))
void refspec_appendf(struct refspec *rs, const char *fmt, ...);
void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
-int valid_fetch_refspec(const char *refspec);
+int valid_fetch_refspec(const char *refspec, const struct git_hash_algo *algo);
/*
* Determine what <prefix> values to pass to the peer in ref-prefix lines
diff --git a/remote.c b/remote.c
index e6c52c850c..b4dff1e5f9 100644
--- a/remote.c
+++ b/remote.c
@@ -3039,7 +3039,7 @@ int valid_remote_name(const char *name)
int result;
struct strbuf refspec = STRBUF_INIT;
strbuf_addf(&refspec, "refs/heads/test:refs/remotes/%s/test", name);
- result = valid_fetch_refspec(refspec.buf);
+ result = valid_fetch_refspec(refspec.buf, the_hash_algo);
strbuf_release(&refspec);
return result;
}
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* [PATCH 3/3] refspec: stop depending on `the_repository`
From: Patrick Steinhardt @ 2026-07-16 12:38 UTC (permalink / raw)
To: git
In-Reply-To: <20260716-pks-refspec-wo-the-repository-v1-0-aa40844d067f@pks.im>
The only remaining user of `the_hash_algo` in "refspec.c" is
`refspec_append()`, which needs to know the hash algorithm so that it
can parse the appended refspec item. In contrast to the functions
adapted in the preceding commit, this function always operates on a
`struct refspec`. As that structure is expected to only ever contain
refspecs that all use the same hash function it doesn't make sense
though to adapt each caller.
Instead, adapt the structure itself so that it gets initialized with a
hash function and use that hash function to parse new refspec items.
Adapt callers accordingly.
This removes the final dependency on the global repository variable in
"refspec.c", so we can drop `USE_THE_REPOSITORY_VARIABLE`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fast-export.c | 4 +++-
builtin/fetch.c | 6 ++++--
builtin/push.c | 6 ++++--
builtin/send-pack.c | 5 ++++-
builtin/submodule--helper.c | 2 +-
http-push.c | 2 +-
refspec.c | 13 ++++++-------
refspec.h | 17 ++++++++++++-----
remote.c | 4 ++--
transport-helper.c | 2 +-
10 files changed, 38 insertions(+), 23 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 0be43104dc..8f4da4cfac 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -51,7 +51,7 @@ static int show_original_ids;
static int mark_tags;
static struct string_list extra_refs = STRING_LIST_INIT_DUP;
static struct string_list tag_refs = STRING_LIST_INIT_DUP;
-static struct refspec refspecs = REFSPEC_INIT_FETCH;
+static struct refspec refspecs;
static int anonymize;
static struct hashmap anonymized_seeds;
static struct revision_sources revision_sources;
@@ -1372,6 +1372,8 @@ int cmd_fast_export(int argc,
/* we handle encodings */
repo_config(the_repository, git_default_config, NULL);
+ refspec_init_fetch(&refspecs, the_hash_algo);
+
repo_init_revisions(the_repository, &revs, prefix);
init_revision_sources(&revision_sources);
revs.topo_order = 1;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1d4a129039..6e1a224553 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -96,7 +96,7 @@ static struct string_list deepen_not = STRING_LIST_INIT_NODUP;
static struct strbuf default_rla = STRBUF_INIT;
static struct transport *gtransport;
static struct transport *gsecondary;
-static struct refspec refmap = REFSPEC_INIT_FETCH;
+static struct refspec refmap;
static struct string_list server_options = STRING_LIST_INIT_DUP;
static struct string_list negotiation_restrict = STRING_LIST_INIT_NODUP;
static struct string_list negotiation_include = STRING_LIST_INIT_NODUP;
@@ -2429,7 +2429,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
const struct fetch_config *config,
struct list_objects_filter_options *filter_options)
{
- struct refspec rs = REFSPEC_INIT_FETCH;
+ struct refspec rs = REFSPEC_INIT_FETCH(the_hash_algo);
int i;
int exit_code;
int maybe_prune_tags;
@@ -2631,6 +2631,8 @@ int cmd_fetch(int argc,
filter_options.allow_auto_filter = 1;
+ refspec_init_fetch(&refmap, the_hash_algo);
+
packet_trace_identity("fetch");
/* Record the command line for the reflog */
diff --git a/builtin/push.c b/builtin/push.c
index 1b2ad3b8df..8ccdb07c40 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -66,7 +66,7 @@ static enum transport_family family;
static struct push_cas_option cas;
-static struct refspec rs = REFSPEC_INIT_PUSH;
+static struct refspec rs;
static struct string_list push_options_config = STRING_LIST_INIT_DUP;
@@ -749,6 +749,8 @@ int cmd_push(int argc,
: &push_options_config);
set_push_cert_flags(&flags, push_cert);
+ refspec_init_push(&rs, the_hash_algo);
+
die_for_incompatible_opt4(deleterefs, "--delete",
tags, "--tags",
flags & TRANSPORT_PUSH_ALL, "--all/--branches",
@@ -855,7 +857,7 @@ int cmd_push(int argc,
}
refspec_clear(&rs);
- rs = (struct refspec) REFSPEC_INIT_PUSH;
+ rs = (struct refspec) REFSPEC_INIT_PUSH(the_hash_algo);
if (tags)
refspec_append(&rs, "refs/tags/*");
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 1412b49bc8..d6cdbae472 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -153,7 +153,7 @@ int cmd_send_pack(int argc,
const char *prefix,
struct repository *repo)
{
- struct refspec rs = REFSPEC_INIT_PUSH;
+ struct refspec rs;
const char *remote_name = NULL;
struct remote *remote = NULL;
const char *dest = NULL;
@@ -214,6 +214,9 @@ int cmd_send_pack(int argc,
repo_config(repo, send_pack_config, NULL);
argc = parse_options(argc, argv, prefix, options, send_pack_usage, 0);
+
+ refspec_init_push(&rs, repo->hash_algo);
+
if (argc > 0) {
dest = argv[0];
refspec_appendn(&rs, argv + 1, argc - 1);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1cc82a134d..c396b826ba 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3150,7 +3150,7 @@ static int push_check(int argc, const char **argv, const char *prefix UNUSED,
if (argc > 2) {
int i;
struct ref *local_refs = get_local_heads();
- struct refspec refspec = REFSPEC_INIT_PUSH;
+ struct refspec refspec = REFSPEC_INIT_PUSH(the_hash_algo);
refspec_appendn(&refspec, argv + 2, argc - 2);
diff --git a/http-push.c b/http-push.c
index 3c23cbba27..969d984cb9 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1716,7 +1716,7 @@ int cmd_main(int argc, const char **argv)
{
struct transfer_request *request;
struct transfer_request *next_request;
- struct refspec rs = REFSPEC_INIT_PUSH;
+ struct refspec rs = REFSPEC_INIT_PUSH(the_hash_algo);
struct remote_lock *ref_lock = NULL;
struct remote_lock *info_ref_lock = NULL;
int delete_branch = 0;
diff --git a/refspec.c b/refspec.c
index 33a6fb8e45..7cb479983b 100644
--- a/refspec.c
+++ b/refspec.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
#define DISABLE_SIGN_COMPARE_WARNINGS
#include "git-compat-util.h"
@@ -185,15 +184,15 @@ void refspec_item_clear(struct refspec_item *item)
item->exact_sha1 = 0;
}
-void refspec_init_fetch(struct refspec *rs)
+void refspec_init_fetch(struct refspec *rs, const struct git_hash_algo *algo)
{
- struct refspec blank = REFSPEC_INIT_FETCH;
+ struct refspec blank = REFSPEC_INIT_FETCH(algo);
memcpy(rs, &blank, sizeof(*rs));
}
-void refspec_init_push(struct refspec *rs)
+void refspec_init_push(struct refspec *rs, const struct git_hash_algo *algo)
{
- struct refspec blank = REFSPEC_INIT_PUSH;
+ struct refspec blank = REFSPEC_INIT_PUSH(algo);
memcpy(rs, &blank, sizeof(*rs));
}
@@ -203,9 +202,9 @@ void refspec_append(struct refspec *rs, const char *refspec)
int ret;
if (rs->fetch)
- ret = refspec_item_init_fetch(&item, refspec, the_hash_algo);
+ ret = refspec_item_init_fetch(&item, refspec, rs->hash_algo);
else
- ret = refspec_item_init_push(&item, refspec, the_hash_algo);
+ ret = refspec_item_init_push(&item, refspec, rs->hash_algo);
if (!ret)
die(_("invalid refspec '%s'"), refspec);
diff --git a/refspec.h b/refspec.h
index e482b720a8..fadef67933 100644
--- a/refspec.h
+++ b/refspec.h
@@ -49,14 +49,21 @@ struct refspec {
int alloc;
int nr;
+ const struct git_hash_algo *hash_algo;
unsigned fetch : 1;
};
-#define REFSPEC_INIT_FETCH { .fetch = 1 }
-#define REFSPEC_INIT_PUSH { .fetch = 0 }
-
-void refspec_init_fetch(struct refspec *rs);
-void refspec_init_push(struct refspec *rs);
+#define REFSPEC_INIT_FETCH(algo) { \
+ .fetch = 1, \
+ .hash_algo = (algo), \
+}
+#define REFSPEC_INIT_PUSH(algo) { \
+ .fetch = 0, \
+ .hash_algo = (algo), \
+}
+
+void refspec_init_fetch(struct refspec *rs, const struct git_hash_algo *hash_algo);
+void refspec_init_push(struct refspec *rs, const struct git_hash_algo *hash_algo);
void refspec_clear(struct refspec *rs);
void refspec_append(struct refspec *rs, const char *refspec);
diff --git a/remote.c b/remote.c
index b4dff1e5f9..d151b1f9d9 100644
--- a/remote.c
+++ b/remote.c
@@ -150,8 +150,8 @@ static struct remote *make_remote(struct remote_state *remote_state,
ret->prune = -1; /* unspecified */
ret->prune_tags = -1; /* unspecified */
ret->name = xstrndup(name, len);
- refspec_init_push(&ret->push);
- refspec_init_fetch(&ret->fetch);
+ refspec_init_push(&ret->push, the_hash_algo);
+ refspec_init_fetch(&ret->fetch, the_hash_algo);
string_list_init_dup(&ret->server_options);
string_list_init_dup(&ret->negotiation_restrict);
string_list_init_dup(&ret->negotiation_include);
diff --git a/transport-helper.c b/transport-helper.c
index 80f90eb7ba..8a25707b03 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -162,7 +162,7 @@ static struct child_process *get_helper(struct transport *transport)
data->helper = helper;
data->no_disconnect_req = 0;
- refspec_init_fetch(&data->rs);
+ refspec_init_fetch(&data->rs, the_hash_algo);
/*
* Open the output as FILE* so strbuf_getline_*() family of
--
2.55.0.313.g8d093f411d.dirty
^ permalink raw reply related
* Re: [PATCH] submodule--helper: avoid use of %zu for now
From: Adrian Ratiu @ 2026-07-16 12:40 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqq4ii0ko9t.fsf@gitster.g>
On Wed, 15 Jul 2026, Junio C Hamano <gitster@pobox.com> wrote:
> Since d7d850e2b9 (CodingGuidelines: mention C99 features we can't
> use, 2022-10-10), our CodingGuidelines document has explicitly
> forbidden the use of '%z' and '%zu' printf() format specifiers,
> even though C99 does support them. However, a new instance crept
> in via 82c36fa0a9 (submodule: hash the submodule name for the
> gitdir path, 2026-01-12).
>
> We could claim that this is an unintentional weather balloon that
> nobody has complained about for the past six months since Git 2.54,
> proving that it is now safe to use these format specifiers. But
> (1) it is probably too early to make that claim, as distributions
> often stick to a stale version for several releases, and (2) it is
> unlikely that a failure in this code path would manifest as a
> major user-visible breakage that would trigger a failure report to
> percolate down to us.
>
> Instead, let's stick to the established workaround recommended by
> our CodingGuidelines, which is to cast the value to (uintmax_t) and
> format it with PRIuMAX, at least for now. Even if we eventually
> perform a bulk update using a Coccinelle script to transition to %z
> and %zu in the future, adding one more instance to the pile that
> will need such a conversion is hardly a tragedy.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> builtin/submodule--helper.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git i/builtin/submodule--helper.c w/builtin/submodule--helper.c
> index 1cc82a134d..92e38106c1 100644
> --- i/builtin/submodule--helper.c
> +++ w/builtin/submodule--helper.c
> @@ -549,7 +549,8 @@ static void create_default_gitdir_config(const char *submodule_name)
> }
>
> /* Case 2.4: If all the above failed, try a hash of the name as a last resort */
> - header_len = snprintf(header, sizeof(header), "blob %zu", strlen(submodule_name));
> + header_len = snprintf(header, sizeof(header),
> + "blob %"PRIuMAX, (uintmax_t)strlen(submodule_name));
> the_hash_algo->init_fn(&ctx);
> the_hash_algo->update_fn(&ctx, header, header_len);
> the_hash_algo->update_fn(&ctx, "\0", 1);
LGTM and sorry for not following the guideline. :)
^ permalink raw reply
* Re: [PATCH] submodule--helper: avoid use of %zu for now
From: Junio C Hamano @ 2026-07-16 13:18 UTC (permalink / raw)
To: Adrian Ratiu; +Cc: git
In-Reply-To: <87a4rrxg3r.fsf@gentoo.mail-host-address-is-not-set>
Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>
>
> LGTM and sorry for not following the guideline. :)
Thanks, and no worries. We all make mistakes.
^ permalink raw reply
* [RFC PATCH 0/7] repack: add --drop-filtered to reclaim space in partial clones
From: Siddharth Shrimali @ 2026-07-16 13:28 UTC (permalink / raw)
To: git
Cc: gitster, christian.couder, siddharthasthana31, me, ps,
johannes.schindelin, l.s.r, r.siddharth.shrimali
This is an RFC series seeking feedback on the design and approach.
Several pieces are still missing (noted below) and the commit
organization needs cleanup.
Partial clones let you work with large repositories without downloading
every blob up front and the missing blobs are lazily fetched from the promisor
remote on demand. Over time, though, these lazily-fetched blobs
accumulate locally and there is currently no safe, built-in way to
reclaim that disk space instead of re-cloning.
This series adds a "git repack --drop-filtered --filter=<spec>" command
that removes large, locally-held promisor blobs that are recoverable
from the promisor remote. The dropped blobs become absent locally but
remain lazily re-fetchable, making the partial-clone still reversible.
How it works:
* Enumerate promisor objects directly (ODB_FOR_EACH_OBJECT_PROMISOR_ONLY)
and select the blobs exceeding the filter threshold. Because every
enumerated object is a promisor object, it is guaranteed recoverable and
locally-created objects are never candidates.
* Rebuild the promisor pack without the selected blobs, reusing the
existing repack machinery, so the drop is crash-safe.
* Record each dropped object in a drop log
($GIT_DIR/objects/info/promisor-dropped) so a later change can
explain a failed lazy fetch (when it was dropped, which filter
matched, which remotes) instead of a bare "could not fetch" error.
* --dry-run lists the candidates and changes nothing.
Planned follow-ups:
* Safety guards: refuse to run while a merge/rebase/cherry-pick is in
progress, and refuse to drop blobs referenced by the current index.
* Authoritative remote verification: the drop log currently lists all
configured promisor remotes rather than the exact remote each object
is recoverable from, because there is no client-side way to query a
remote for object availability yet. A "remote-object-info" command
is being added to the "git cat-file --batch" protocol for this. Once
available, the exact remote can be recorded.
Known issues to address in v2:
* There is churn between "enumerate promisor blobs" and "actually drop
filtered promisor blobs". The former introduces
enumerate_promisor_blobs() with an interim signature that the latter
rewrites. These will be reorganized so the function is introduced
in its final form.
* The tests are in a standalone commit. They will instead be
distributed into the commits that introduce the behavior they test.
Siddharth Shrimali (7):
builtin/repack.c: add --drop-filtered and --dry-run options
list-objects-filter: add list_objects_filter__filter_oidset()
repack-promisor: allow excluding objects from the rebuilt promisor
pack
builtin/repack: enumerate promisor blobs for --drop-filtered
t7706: test --drop-filtered enumeration and validation
builtin/repack: actually drop filtered promisor blobs
repack-promisor: record dropped objects in a drop log
builtin/repack.c | 76 ++++++++++++++++-
list-objects-filter.c | 45 ++++++++++
list-objects-filter.h | 16 ++++
repack-filtered.c | 81 ++++++++++++++++++
repack-promisor.c | 106 ++++++++++++++++++++++-
repack.h | 12 ++-
t/meson.build | 1 +
t/t7706-repack-drop-filtered.sh | 145 ++++++++++++++++++++++++++++++++
8 files changed, 478 insertions(+), 4 deletions(-)
create mode 100755 t/t7706-repack-drop-filtered.sh
--
2.54.0
^ permalink raw reply
* [RFC PATCH 1/7] builtin/repack.c: add --drop-filtered and --dry-run options
From: Siddharth Shrimali @ 2026-07-16 13:28 UTC (permalink / raw)
To: git
Cc: gitster, christian.couder, siddharthasthana31, me, ps,
johannes.schindelin, l.s.r, r.siddharth.shrimali
In-Reply-To: <20260716132848.95982-1-r.siddharth.shrimali@gmail.com>
Add two new command-line options to 'git-repack':
--drop-filtered: intended to eventually delete objects that match
the filter specification. Requires --filter and -a,
and is incompatible with --filter-to.
--dry-run: show which objects would be dropped without making any
changes. Only meaningful with --drop-filtered.
--drop-filtered also requires a promisor remote to be configured,
since dropping objects without a remote to fetch them back from would
be permanent data loss.
--drop-filtered is incompatible with bitmap writing: filtering breaks
the "all objects in one pack" closure that bitmaps require. An explicit
-b is rejected with a clear error and a default-on bitmap configuration is
silently disabled for the duration of the command.
These options currently only perform validation. The actual enumeration
and deletion will be added in follow-up commits.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
builtin/repack.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/builtin/repack.c b/builtin/repack.c
index db504d673f..f4db0fc535 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -14,6 +14,7 @@
#include "promisor-remote.h"
#include "repack.h"
#include "shallow.h"
+#include "list-objects-filter-options.h"
#define ALL_INTO_ONE 1
#define LOOSEN_UNREACHABLE 2
@@ -28,6 +29,8 @@ static int use_delta_islands;
static int run_update_server_info = 1;
static char *packdir, *packtmp_name, *packtmp;
static int midx_must_contain_cruft = 1;
+static int drop_filtered;
+static int dry_run;
static const char *const git_repack_usage[] = {
N_("git repack [-a] [-A] [-d] [-f] [-F] [-l] [-n] [-q] [-b] [-m]\n"
@@ -231,6 +234,10 @@ int cmd_repack(int argc,
N_("pack prefix to store a pack containing pruned objects")),
OPT_STRING(0, "filter-to", &filter_to, N_("dir"),
N_("pack prefix to store a pack containing filtered out objects")),
+ OPT_BOOL(0, "drop-filtered", &drop_filtered,
+ N_("delete filtered out objects (requires --filter)")),
+ OPT_BOOL(0, "dry-run", &dry_run,
+ N_("only show which objects would be dropped")),
OPT_END()
};
@@ -252,6 +259,43 @@ int cmd_repack(int argc,
po_args.depth = xstrdup_or_null(opt_depth);
po_args.threads = xstrdup_or_null(opt_threads);
+ die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
+ !!filter_to, "--filter-to");
+
+ die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
+ write_bitmaps > 0, "--write-bitmap-index");
+
+ if (dry_run && !drop_filtered)
+ die(_("--dry-run only takes effect with --drop-filtered"));
+
+ if (drop_filtered) {
+ if (!dry_run)
+ die(_("--drop-filtered doesn't work without --dry-run yet"));
+
+ if (!po_args.filter_options.choice)
+ die(_("--drop-filtered requires --filter"));
+
+ if (!(pack_everything & ALL_INTO_ONE))
+ die(_("--drop-filtered requires -a"));
+
+ /*
+ * Only blob:limit=<n> is supported for now. Reject other
+ * filter choices early, before walking the object database.
+ */
+ if (po_args.filter_options.choice != LOFC_BLOB_LIMIT)
+ die(_("--drop-filtered only supports --filter=blob:limit=<n> for now"));
+
+ /*
+ * Without a promisor remote there is nowhere to re-fetch the
+ * dropped objects from, so dropping them would be permanent
+ * data loss.
+ */
+ if (!repo_has_promisor_remote(repo))
+ die(_("--drop-filtered requires a promisor remote"));
+
+ write_bitmaps = 0;
+ }
+
if (delete_redundant && repo->repository_format_precious_objects)
die(_("cannot delete packs in a precious-objects repo"));
--
2.54.0
^ permalink raw reply related
* [RFC PATCH 2/7] list-objects-filter: add list_objects_filter__filter_oidset()
From: Siddharth Shrimali @ 2026-07-16 13:28 UTC (permalink / raw)
To: git
Cc: gitster, christian.couder, siddharthasthana31, me, ps,
johannes.schindelin, l.s.r, r.siddharth.shrimali
In-Reply-To: <20260716132848.95982-1-r.siddharth.shrimali@gmail.com>
The existing filter entry point, list_objects_filter__filter_object(),
is built around the object-walk path: it expects traversal context and
provisional omit sets, and is meant to be called as objects are
visited during a walk. A caller that already has a set of OIDs in hand
and only wants to know which ones a filter would select has no usable
entry point into the filter API.
--drop-filtered is exactly such a caller: it collects promisor blobs
into an oidset and needs to know which of them exceed the filter
threshold, without performing an object walk.
Add a helper, list_objects_filter__filter_oidset(), that takes a set
of OIDs and populates an "omitted" set with those that would be
filtered out by the given filter options. Only blob:limit=N filters
are supported for now.
This helper does not actually reuse the existing filter machinery.
It reimplements the blob:limit size check directly. That machinery
is tied to the object-walk path and cannot easily be driven
from a plain oidset. A NEEDSWORK comment marks this so the helper can
later be refactored to reuse the real filter logic instead of
duplicating it.
OBJECT_INFO_SKIP_FETCH_OBJECT is passed when reading object info so
the helper never triggers a lazy fetch.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
list-objects-filter.c | 45 +++++++++++++++++++++++++++++++++++++++++++
list-objects-filter.h | 16 +++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/list-objects-filter.c b/list-objects-filter.c
index c912ff3079..6a2e9d5b24 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -828,3 +828,48 @@ void list_objects_filter__free(struct filter *filter)
filter->free_fn(filter->filter_data);
free(filter);
}
+
+/*
+ * NEEDSWORK: this reimplements the blob:limit size check rather than
+ * reusing the existing filter machinery in
+ * list_objects_filter__filter_object(). That machinery is currently
+ * tied to the object-walk path and cannot easily be driven from a
+ * plain oidset. It would be nice to refactor the filter code so this
+ * helper can reuse it instead of duplicating the size check.
+ */
+int list_objects_filter__filter_oidset(struct repository *r,
+ struct list_objects_filter_options *opts,
+ const struct oidset *in,
+ struct oidset *omitted)
+{
+ struct oidset_iter iter;
+ const struct object_id *oid;
+
+ if (opts->choice != LOFC_BLOB_LIMIT)
+ return error(_("filter_oidset: only blob:limit filters are supported"));
+
+ oidset_iter_init(in, &iter);
+ while ((oid = oidset_iter_next(&iter))) {
+ struct object_info info = OBJECT_INFO_INIT;
+ enum object_type type;
+ unsigned long size;
+
+ info.typep = &type;
+ info.sizep = &size;
+
+ /*
+ * Use OBJECT_INFO_SKIP_FETCH_OBJECT to avoid triggering
+ * a lazy fetch while inspecting candidates for removal.
+ */
+ if (odb_read_object_info_extended(r->objects, oid, &info,
+ OBJECT_INFO_SKIP_FETCH_OBJECT) < 0)
+ continue;
+
+ if (type != OBJ_BLOB)
+ continue;
+
+ if (size >= opts->blob_limit_value)
+ oidset_insert(omitted, oid);
+ }
+ return 0;
+}
diff --git a/list-objects-filter.h b/list-objects-filter.h
index 9e98814111..56a2d87aa0 100644
--- a/list-objects-filter.h
+++ b/list-objects-filter.h
@@ -94,4 +94,20 @@ enum list_objects_filter_result list_objects_filter__filter_object(
*/
void list_objects_filter__free(struct filter *filter);
+/*
+ * Given a set of OIDs in 'in', populate 'omitted' with those that
+ * would be filtered by 'opts'. Currently only blob:limit=N is
+ * supported. Objects that cannot be read are silently skipped.
+ *
+ * NEEDSWORK: this reimplements the blob:limit size check rather than
+ * reusing the existing filter machinery. See the matching comment in
+ * list-objects-filter.c.
+ *
+ * Return 0 on success, -1 if the filter is not supported.
+ */
+int list_objects_filter__filter_oidset(struct repository *r,
+ struct list_objects_filter_options *opts,
+ const struct oidset *in,
+ struct oidset *omitted);
+
#endif /* LIST_OBJECTS_FILTER_H */
--
2.54.0
^ permalink raw reply related
* [RFC PATCH 3/7] repack-promisor: allow excluding objects from the rebuilt promisor pack
From: Siddharth Shrimali @ 2026-07-16 13:28 UTC (permalink / raw)
To: git
Cc: gitster, christian.couder, siddharthasthana31, me, ps,
johannes.schindelin, l.s.r, r.siddharth.shrimali
In-Reply-To: <20260716132848.95982-1-r.siddharth.shrimali@gmail.com>
Add a to_drop oidset parameter to repack_promisor_objects(). When it is
non-NULL, write_oid() omits those objects from the rebuilt promisor
pack. This is the mechanism --drop-filtered will use to remove promisor
blobs, i.e. rebuild the promisor pack without them.
All existing callers pass NULL, so behavior is unchanged.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
builtin/repack.c | 2 +-
repack-promisor.c | 15 ++++++++++++++-
repack.h | 4 +++-
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index f4db0fc535..433b2c8205 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -406,7 +406,7 @@ int cmd_repack(int argc,
strvec_push(&cmd.args, "--delta-islands");
if (pack_everything & ALL_INTO_ONE) {
- repack_promisor_objects(repo, &po_args, &names, packtmp);
+ repack_promisor_objects(repo, &po_args, &names, packtmp, NULL);
if (existing_packs_has_non_kept(&existing) &&
delete_redundant &&
diff --git a/repack-promisor.c b/repack-promisor.c
index 90318ce150..fabfdc168a 100644
--- a/repack-promisor.c
+++ b/repack-promisor.c
@@ -6,10 +6,12 @@
#include "path.h"
#include "repository.h"
#include "run-command.h"
+#include "oidset.h"
struct write_oid_context {
struct child_process *cmd;
const struct git_hash_algo *algop;
+ const struct oidset *to_drop;
};
/*
@@ -23,6 +25,15 @@ static int write_oid(const struct object_id *oid,
struct write_oid_context *ctx = data;
struct child_process *cmd = ctx->cmd;
+ /*
+ * Objects in to_drop are being removed from the repository, so
+ * omit them from the rebuilt promisor pack. Each such object is a
+ * promisor object and therefore remains recoverable from the
+ * promisor remote.
+ */
+ if (ctx->to_drop && oidset_contains(ctx->to_drop, oid))
+ return 0;
+
if (cmd->in == -1) {
if (start_command(cmd))
die(_("could not start pack-objects to repack promisor objects"));
@@ -81,7 +92,8 @@ static void finish_repacking_promisor_objects(struct repository *repo,
void repack_promisor_objects(struct repository *repo,
const struct pack_objects_args *args,
- struct string_list *names, const char *packtmp)
+ struct string_list *names, const char *packtmp,
+ const struct oidset *to_drop)
{
struct write_oid_context ctx;
struct child_process cmd = CHILD_PROCESS_INIT;
@@ -98,6 +110,7 @@ void repack_promisor_objects(struct repository *repo,
*/
ctx.cmd = &cmd;
ctx.algop = repo->hash_algo;
+ ctx.to_drop = to_drop;
odb_for_each_object(repo->objects, NULL, write_oid, &ctx,
ODB_FOR_EACH_OBJECT_PROMISOR_ONLY);
diff --git a/repack.h b/repack.h
index f9fbc895f0..a5a3f7c6ba 100644
--- a/repack.h
+++ b/repack.h
@@ -3,6 +3,7 @@
#include "list-objects-filter-options.h"
#include "string-list.h"
+#include "oidset.h"
struct pack_objects_args {
char *window;
@@ -100,7 +101,8 @@ void generated_pack_install(struct generated_pack *pack, const char *name,
void repack_promisor_objects(struct repository *repo,
const struct pack_objects_args *args,
- struct string_list *names, const char *packtmp);
+ struct string_list *names, const char *packtmp,
+ const struct oidset *to_drop);
struct pack_geometry {
struct packed_git **pack;
--
2.54.0
^ permalink raw reply related
* [RFC PATCH 4/7] builtin/repack: enumerate promisor blobs for --drop-filtered
From: Siddharth Shrimali @ 2026-07-16 13:28 UTC (permalink / raw)
To: git
Cc: gitster, christian.couder, siddharthasthana31, me, ps,
johannes.schindelin, l.s.r, r.siddharth.shrimali
In-Reply-To: <20260716132848.95982-1-r.siddharth.shrimali@gmail.com>
Add enumeration logic for --drop-filtered. In --dry-run mode, print
the OIDs of locally-held promisor blobs that exceed the filter
threshold, as candidates for removal.
Reading from write_filtered_pack() cannot work for partial clones.
git repack routes promisor objects through a separate path:
repack_promisor_objects() repacks them first, and the main
pack-objects run uses --exclude-promisor-objects. By the time
write_filtered_pack() runs, the promisor blobs are already consumed by
the main pack. The filtered pack is always empty on a partial clone.
Instead, walk promisor objects directly via odb_for_each_object() with
ODB_FOR_EACH_OBJECT_PROMISOR_ONLY, collecting all promisor blobs into
an oidset. The blobs exceeding the filter threshold are then selected
using list_objects_filter__filter_oidset().
Every object enumerated this way is a promisor object by construction,
so it is guaranteed to be recoverable from the promisor remote and is
safe to drop. No separate is_promisor_object() check is needed.
OBJECT_INFO_SKIP_FETCH_OBJECT is passed to every object info query so
enumeration never triggers a lazy fetch.
Deletion of the enumerated objects, together with the required
promisor-remote verification, will be added separately.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
builtin/repack.c | 41 ++++++++++++++-------
repack-filtered.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++
repack.h | 4 +++
3 files changed, 124 insertions(+), 13 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 433b2c8205..c2b07477d2 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -590,19 +590,34 @@ int cmd_repack(int argc,
}
if (po_args.filter_options.choice) {
- struct write_pack_opts opts = {
- .po_args = &po_args,
- .destination = filter_to,
- .packdir = packdir,
- .packtmp = packtmp,
- };
-
- if (!opts.destination)
- opts.destination = packtmp;
-
- ret = write_filtered_pack(&opts, &existing, &names);
- if (ret)
- goto cleanup;
+ if (drop_filtered) {
+ /*
+ * Enumerate promisor objects directly rather than
+ * going through write_filtered_pack(). The filter
+ * machinery cannot see promisor objects because
+ * repack_promisor_objects() handles them separately
+ * before the filter runs.
+ */
+ ret = enumerate_promisor_blobs(repo,
+ &po_args.filter_options,
+ dry_run);
+ if (ret)
+ goto cleanup;
+ } else {
+ struct write_pack_opts opts = {
+ .po_args = &po_args,
+ .destination = filter_to,
+ .packdir = packdir,
+ .packtmp = packtmp,
+ };
+
+ if (!opts.destination)
+ opts.destination = packtmp;
+
+ ret = write_filtered_pack(&opts, &existing, &names);
+ if (ret)
+ goto cleanup;
+ }
}
string_list_sort(&names);
diff --git a/repack-filtered.c b/repack-filtered.c
index edcf7667c5..f5a1dae5b1 100644
--- a/repack-filtered.c
+++ b/repack-filtered.c
@@ -3,6 +3,12 @@
#include "repository.h"
#include "run-command.h"
#include "string-list.h"
+#include "hex.h"
+#include "packfile.h"
+#include "list-objects-filter-options.h"
+#include "list-objects-filter.h"
+#include "odb.h"
+#include "promisor-remote.h"
int write_filtered_pack(const struct write_pack_opts *opts,
struct existing_packs *existing,
@@ -49,3 +55,89 @@ int write_filtered_pack(const struct write_pack_opts *opts,
return finish_pack_objects_cmd(existing->repo->hash_algo, opts, &cmd,
names);
}
+
+struct collect_cb_data {
+ struct repository *repo;
+ struct oidset *set;
+};
+
+static int collect_promisor_blob(const struct object_id *oid,
+ struct object_info *oi UNUSED,
+ void *cb_data)
+{
+ struct collect_cb_data *data = cb_data;
+ struct object_info info = OBJECT_INFO_INIT;
+ enum object_type type;
+
+ info.typep = &type;
+
+ /*
+ * Use OBJECT_INFO_SKIP_FETCH_OBJECT to avoid triggering a
+ * lazy fetch while collecting promisor blobs.
+ */
+ if (odb_read_object_info_extended(data->repo->objects, oid, &info,
+ OBJECT_INFO_SKIP_FETCH_OBJECT) < 0)
+ return 0;
+
+ if (type == OBJ_BLOB)
+ oidset_insert(data->set, oid);
+
+ return 0;
+}
+
+int enumerate_promisor_blobs(struct repository *repo,
+ const struct list_objects_filter_options *filter,
+ int dry_run)
+{
+ struct oidset all_promisor_blobs = OIDSET_INIT;
+ struct oidset to_drop = OIDSET_INIT;
+ struct collect_cb_data cb = {
+ .repo = repo,
+ .set = &all_promisor_blobs
+ };
+ struct oidset_iter iter;
+ const struct object_id *oid;
+ int ret = 0;
+
+ /*
+ * The caller (cmd_repack) is responsible for validating that a
+ * blob:limit filter and a promisor remote are present before
+ * calling this function.
+ *
+ * Walk only promisor objects. Every object visited here is
+ * guaranteed to be recoverable from the promisor remote, so
+ * it is safe to drop.
+ *
+ * We do not use write_filtered_pack() here because git repack
+ * routes promisor objects through repack_promisor_objects()
+ * before the filter machinery runs, so the filtered pack never
+ * contains promisor blobs. Direct enumeration via
+ * ODB_FOR_EACH_OBJECT_PROMISOR_ONLY is the correct approach.
+ */
+ ret = odb_for_each_object(repo->objects, NULL,
+ collect_promisor_blob, &cb,
+ ODB_FOR_EACH_OBJECT_PROMISOR_ONLY);
+ if (ret)
+ goto cleanup;
+
+ /*
+ * Apply the filter to find which blobs exceed the threshold.
+ */
+ ret = list_objects_filter__filter_oidset(repo,
+ (struct list_objects_filter_options *)filter,
+ &all_promisor_blobs,
+ &to_drop);
+ if (ret)
+ goto cleanup;
+
+ if (dry_run) {
+ oidset_iter_init(&to_drop, &iter);
+ while ((oid = oidset_iter_next(&iter)))
+ printf("%s\n", oid_to_hex(oid));
+ }
+
+cleanup:
+ oidset_clear(&all_promisor_blobs);
+ oidset_clear(&to_drop);
+ return ret;
+}
diff --git a/repack.h b/repack.h
index a5a3f7c6ba..d08e25b852 100644
--- a/repack.h
+++ b/repack.h
@@ -167,6 +167,10 @@ int write_filtered_pack(const struct write_pack_opts *opts,
struct existing_packs *existing,
struct string_list *names);
+int enumerate_promisor_blobs(struct repository *repo,
+ const struct list_objects_filter_options *filter,
+ int dry_run);
+
int write_cruft_pack(const struct write_pack_opts *opts,
const char *cruft_expiration,
unsigned long combine_cruft_below_size,
--
2.54.0
^ permalink raw reply related
* [RFC PATCH 5/7] t7706: test --drop-filtered enumeration and validation
From: Siddharth Shrimali @ 2026-07-16 13:28 UTC (permalink / raw)
To: git
Cc: gitster, christian.couder, siddharthasthana31, me, ps,
johannes.schindelin, l.s.r, r.siddharth.shrimali
In-Reply-To: <20260716132848.95982-1-r.siddharth.shrimali@gmail.com>
Add tests for the --drop-filtered option.
* Validation: --drop-filtered requires --filter and -a, is
incompatible with --filter-to and --write-bitmap-index, --dry-run
only takes effect with --drop-filtered, and --drop-filtered
requires a promisor remote.
* Enumeration: in a repository with a promisor remote, --dry-run
lists promisor blobs above the filter threshold and excludes
smaller ones. Promisor blobs are created with a synthetic promisor
pack, following the helper pattern used in t0410.
* Safety: a locally created large blob, which is not a promisor
object and therefore not recoverable from the remote, is never
listed as a drop candidate.
* Non-destructiveness: --dry-run leaves the filtered objects intact
in the repository.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
t/meson.build | 1 +
t/t7706-repack-drop-filtered.sh | 139 ++++++++++++++++++++++++++++++++
2 files changed, 140 insertions(+)
create mode 100755 t/t7706-repack-drop-filtered.sh
diff --git a/t/meson.build b/t/meson.build
index 8ae6ab6c5f..37f272d7f4 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -962,6 +962,7 @@ integration_tests = [
't7703-repack-geometric.sh',
't7704-repack-cruft.sh',
't7705-repack-incremental-midx.sh',
+ 't7706-repack-drop-filtered.sh',
't7800-difftool.sh',
't7810-grep.sh',
't7811-grep-open.sh',
diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh
new file mode 100755
index 0000000000..b558807847
--- /dev/null
+++ b/t/t7706-repack-drop-filtered.sh
@@ -0,0 +1,139 @@
+#!/bin/sh
+
+test_description='git repack --drop-filtered enumerates
+filtered promisor blobs'
+
+. ./test-lib.sh
+
+delete_object () {
+ local repo="$1" &&
+ local obj="$2" &&
+ local path="$repo/.git/objects/$(test_oid_to_path "$obj")" &&
+ rm "$path"
+}
+
+# pack the objects into a promisor pack inside "repo",
+# it is a pack accompanied by an empty ".promisor" marker file. objects
+# in such a pack are treated as recoverable from the promisor remote.
+pack_as_from_promisor () {
+ HASH=$(git -C repo pack-objects .git/objects/pack/pack) &&
+ >repo/.git/objects/pack/pack-$HASH.promisor &&
+ echo $HASH
+}
+
+# write a blob of $1 bytes into "repo", record it as coming from the
+# promisor remote (promisor pack), and remove the loose copy so the
+# object is only present in the promisor pack.
+promisor_blob () {
+ test-tool genrandom "$1" "$2" >blob_content &&
+ OID=$(git -C repo hash-object -w --stdin <blob_content) &&
+ printf "%s\n" "$OID" | pack_as_from_promisor >/dev/null &&
+ delete_object repo "$OID" &&
+ echo "$OID"
+}
+
+# checks for options validations before any promisor walk
+test_expect_success 'setup plain repo for validation' '
+ git init plain &&
+ test_commit -C plain initial &&
+ git clone --bare plain plain.git &&
+ git -C plain.git repack -a -d
+'
+
+test_expect_success '--drop-filtered requires --filter' '
+ test_must_fail git -C plain.git repack --drop-filtered --dry-run -a 2>err &&
+ test_grep "drop-filtered requires --filter" err
+'
+
+test_expect_success '--drop-filtered cannot be used with --filter-to' '
+ test_must_fail git -C plain.git repack --drop-filtered \
+ --filter=blob:limit=1k --filter-to=./filter-out 2>err &&
+ test_grep "options .--drop-filtered. and .--filter-to. cannot be used together" err
+'
+
+test_expect_success '--dry-run only takes effect with --drop-filtered' '
+ test_must_fail git -C plain.git repack --dry-run 2>err &&
+ test_grep "dry-run only takes effect with --drop-filtered" err
+'
+
+test_expect_success '--drop-filtered without --dry-run is rejected' '
+ test_must_fail git -C plain.git repack --drop-filtered \
+ --filter=blob:limit=1k -a 2>err &&
+ test_grep "drop-filtered doesn.t work without --dry-run yet" err
+'
+
+test_expect_success '--drop-filtered requires -a' '
+ test_must_fail git -C plain.git repack --drop-filtered \
+ --filter=blob:limit=1k --dry-run 2>err &&
+ test_grep "drop-filtered requires -a" err
+'
+
+test_expect_success '--drop-filtered fails with --write-bitmap-index' '
+ test_must_fail git -C plain.git repack --drop-filtered \
+ --filter=blob:limit=1k --dry-run -a -b 2>err &&
+ test_grep "options .--drop-filtered. and .--write-bitmap-index. cannot be used together" err
+'
+
+test_expect_success '--drop-filtered fails without a promisor remote' '
+ test_must_fail git -C plain.git repack --drop-filtered \
+ --filter=blob:limit=1k --dry-run -a 2>err &&
+ test_grep "drop-filtered requires a promisor remote" err
+'
+
+# enumeration and safety tests using promisor packs
+test_expect_success 'setup repo with a promisor remote' '
+ rm -rf repo &&
+ test_create_repo repo &&
+ test_commit -C repo base &&
+
+ # mark the repo as a partial clone with a promisor remote so the
+ # promisor walk and the safety guard are satisfied.
+ git -C repo config core.repositoryformatversion 1 &&
+ git -C repo config extensions.partialclone origin &&
+ git -C repo config remote.origin.promisor true &&
+ git -C repo config remote.origin.url "." &&
+
+ BIG=$(promisor_blob big 3072) &&
+ SMALL=$(promisor_blob small 512) &&
+ echo "$BIG" >big_oid &&
+ echo "$SMALL" >small_oid
+'
+
+test_expect_success 'promisor blob over the threshold is listed' '
+ BIG=$(cat big_oid) &&
+ SMALL=$(cat small_oid) &&
+
+ git -C repo -c repack.writeBitmaps=false \
+ repack --drop-filtered --filter=blob:limit=1k --dry-run -a >out &&
+
+ test_grep "$BIG" out &&
+ test_grep ! "$SMALL" out
+'
+
+test_expect_success 'locally created blob is never listed' '
+ BIG=$(cat big_oid) &&
+
+ # large blob that exists only locally (no promisor pack) must
+ # never be a drop candidate: dropping it would be unrecoverable
+ # data loss.
+ test-tool genrandom local 4096 >local_content &&
+ LOCAL=$(git -C repo hash-object -w --stdin <local_content) &&
+
+ git -C repo -c repack.writeBitmaps=false \
+ repack --drop-filtered --filter=blob:limit=1k --dry-run -a >out &&
+
+ test_grep "$BIG" out &&
+ test_grep ! "$LOCAL" out
+'
+
+test_expect_success '--dry-run does not remove the filtered objects' '
+ BIG=$(cat big_oid) &&
+
+ git -C repo -c repack.writeBitmaps=false \
+ repack --drop-filtered --filter=blob:limit=1k --dry-run -a >out &&
+
+ # candidate blob must still be present after a dry run.
+ git -C repo cat-file -e "$BIG"
+'
+
+test_done
--
2.54.0
^ permalink raw reply related
* [RFC PATCH 6/7] builtin/repack: actually drop filtered promisor blobs
From: Siddharth Shrimali @ 2026-07-16 13:28 UTC (permalink / raw)
To: git
Cc: gitster, christian.couder, siddharthasthana31, me, ps,
johannes.schindelin, l.s.r, r.siddharth.shrimali
In-Reply-To: <20260716132848.95982-1-r.siddharth.shrimali@gmail.com>
Make --drop-filtered remove the enumerated promisor blobs instead of
only listing them.
The drop set is computed before repack_promisor_objects() runs, and on
a real run it is passed in so the rebuilt promisor pack omits those
blobs. --drop-filtered implies -d so the old promisor packs, which
still contain the dropped blobs, are removed. Without this the blobs
would survive in the redundant packs. The existing repack machinery
performs the write-before-delete and fsync, so the drop is crash-safe.
The dropped blobs become absent locally but remain recoverable from the
promisor remote, so a later access lazy-fetches them back
transparently. --dry-run keeps its previous behavior, i.e. it lists the
candidates and changes nothing.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
builtin/repack.c | 75 ++++++++++++++++++---------------
repack-filtered.c | 17 ++------
repack.h | 4 +-
t/t7706-repack-drop-filtered.sh | 18 +++++---
4 files changed, 59 insertions(+), 55 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index c2b07477d2..aa3257a98a 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -15,6 +15,8 @@
#include "repack.h"
#include "shallow.h"
#include "list-objects-filter-options.h"
+#include "oidset.h"
+#include "hex.h"
#define ALL_INTO_ONE 1
#define LOOSEN_UNREACHABLE 2
@@ -143,6 +145,7 @@ int cmd_repack(int argc,
struct string_list_item *item;
struct string_list names = STRING_LIST_INIT_DUP;
struct existing_packs existing = EXISTING_PACKS_INIT;
+ struct oidset drop_oids = OIDSET_INIT;
struct pack_geometry geometry = { 0 };
struct tempfile *refs_snapshot = NULL;
int i, ret;
@@ -269,9 +272,6 @@ int cmd_repack(int argc,
die(_("--dry-run only takes effect with --drop-filtered"));
if (drop_filtered) {
- if (!dry_run)
- die(_("--drop-filtered doesn't work without --dry-run yet"));
-
if (!po_args.filter_options.choice)
die(_("--drop-filtered requires --filter"));
@@ -294,6 +294,28 @@ int cmd_repack(int argc,
die(_("--drop-filtered requires a promisor remote"));
write_bitmaps = 0;
+
+ /*
+ * Dropping objects means rebuilding the promisor packs
+ * without them and then removing the old packs, so the
+ * redundant packs must be deleted. Imply -d on a real run.
+ */
+ if (!dry_run)
+ delete_redundant = 1;
+
+ ret = enumerate_promisor_blobs(repo, &po_args.filter_options, &drop_oids);
+
+ if (ret)
+ goto cleanup;
+
+ if (dry_run) {
+ struct oidset_iter iter;
+ const struct object_id *oid;
+
+ oidset_iter_init(&drop_oids, &iter);
+ while ((oid = oidset_iter_next(&iter)))
+ printf("%s\n", oid_to_hex(oid));
+ }
}
if (delete_redundant && repo->repository_format_precious_objects)
@@ -406,7 +428,8 @@ int cmd_repack(int argc,
strvec_push(&cmd.args, "--delta-islands");
if (pack_everything & ALL_INTO_ONE) {
- repack_promisor_objects(repo, &po_args, &names, packtmp, NULL);
+ repack_promisor_objects(repo, &po_args, &names, packtmp,
+ (drop_filtered && !dry_run) ? &drop_oids : NULL);
if (existing_packs_has_non_kept(&existing) &&
delete_redundant &&
@@ -589,35 +612,20 @@ int cmd_repack(int argc,
}
}
- if (po_args.filter_options.choice) {
- if (drop_filtered) {
- /*
- * Enumerate promisor objects directly rather than
- * going through write_filtered_pack(). The filter
- * machinery cannot see promisor objects because
- * repack_promisor_objects() handles them separately
- * before the filter runs.
- */
- ret = enumerate_promisor_blobs(repo,
- &po_args.filter_options,
- dry_run);
- if (ret)
- goto cleanup;
- } else {
- struct write_pack_opts opts = {
- .po_args = &po_args,
- .destination = filter_to,
- .packdir = packdir,
- .packtmp = packtmp,
- };
-
- if (!opts.destination)
- opts.destination = packtmp;
-
- ret = write_filtered_pack(&opts, &existing, &names);
- if (ret)
- goto cleanup;
- }
+ if (po_args.filter_options.choice && !drop_filtered) {
+ struct write_pack_opts opts = {
+ .po_args = &po_args,
+ .destination = filter_to,
+ .packdir = packdir,
+ .packtmp = packtmp,
+ };
+
+ if (!opts.destination)
+ opts.destination = packtmp;
+
+ ret = write_filtered_pack(&opts, &existing, &names);
+ if (ret)
+ goto cleanup;
}
string_list_sort(&names);
@@ -697,6 +705,7 @@ int cmd_repack(int argc,
cleanup:
string_list_clear(&keep_pack_list, 0);
string_list_clear(&names, 1);
+ oidset_clear(&drop_oids);
existing_packs_release(&existing);
pack_geometry_release(&geometry);
pack_objects_args_release(&po_args);
diff --git a/repack-filtered.c b/repack-filtered.c
index f5a1dae5b1..6f0cecca9b 100644
--- a/repack-filtered.c
+++ b/repack-filtered.c
@@ -87,16 +87,13 @@ static int collect_promisor_blob(const struct object_id *oid,
int enumerate_promisor_blobs(struct repository *repo,
const struct list_objects_filter_options *filter,
- int dry_run)
+ struct oidset *to_drop)
{
struct oidset all_promisor_blobs = OIDSET_INIT;
- struct oidset to_drop = OIDSET_INIT;
struct collect_cb_data cb = {
.repo = repo,
.set = &all_promisor_blobs
};
- struct oidset_iter iter;
- const struct object_id *oid;
int ret = 0;
/*
@@ -122,22 +119,14 @@ int enumerate_promisor_blobs(struct repository *repo,
/*
* Apply the filter to find which blobs exceed the threshold.
+ * The caller has to_drop and is responsible for clearing it.
*/
ret = list_objects_filter__filter_oidset(repo,
(struct list_objects_filter_options *)filter,
&all_promisor_blobs,
- &to_drop);
- if (ret)
- goto cleanup;
-
- if (dry_run) {
- oidset_iter_init(&to_drop, &iter);
- while ((oid = oidset_iter_next(&iter)))
- printf("%s\n", oid_to_hex(oid));
- }
+ to_drop);
cleanup:
oidset_clear(&all_promisor_blobs);
- oidset_clear(&to_drop);
return ret;
}
diff --git a/repack.h b/repack.h
index d08e25b852..61e554e4ed 100644
--- a/repack.h
+++ b/repack.h
@@ -168,8 +168,8 @@ int write_filtered_pack(const struct write_pack_opts *opts,
struct string_list *names);
int enumerate_promisor_blobs(struct repository *repo,
- const struct list_objects_filter_options *filter,
- int dry_run);
+ const struct list_objects_filter_options *filter,
+ struct oidset *to_drop);
int write_cruft_pack(const struct write_pack_opts *opts,
const char *cruft_expiration,
diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh
index b558807847..41e7941799 100755
--- a/t/t7706-repack-drop-filtered.sh
+++ b/t/t7706-repack-drop-filtered.sh
@@ -56,12 +56,6 @@ test_expect_success '--dry-run only takes effect with --drop-filtered' '
test_grep "dry-run only takes effect with --drop-filtered" err
'
-test_expect_success '--drop-filtered without --dry-run is rejected' '
- test_must_fail git -C plain.git repack --drop-filtered \
- --filter=blob:limit=1k -a 2>err &&
- test_grep "drop-filtered doesn.t work without --dry-run yet" err
-'
-
test_expect_success '--drop-filtered requires -a' '
test_must_fail git -C plain.git repack --drop-filtered \
--filter=blob:limit=1k --dry-run 2>err &&
@@ -136,4 +130,16 @@ test_expect_success '--dry-run does not remove the filtered objects' '
git -C repo cat-file -e "$BIG"
'
+test_expect_success '--drop-filtered removes the promisor blob locally' '
+ BIG=$(cat big_oid) &&
+ SMALL=$(cat small_oid) &&
+
+ git -C repo -c repack.writeBitmaps=false \
+ repack --drop-filtered --filter=blob:limit=1k -a &&
+
+ git -C repo cat-file --batch-all-objects --batch-check="%(objectname)" >present &&
+ ! grep -q "$BIG" present &&
+ grep -q "$SMALL" present
+'
+
test_done
--
2.54.0
^ permalink raw reply related
* [RFC PATCH 7/7] repack-promisor: record dropped objects in a drop log
From: Siddharth Shrimali @ 2026-07-16 13:28 UTC (permalink / raw)
To: git
Cc: gitster, christian.couder, siddharthasthana31, me, ps,
johannes.schindelin, l.s.r, r.siddharth.shrimali
In-Reply-To: <20260716132848.95982-1-r.siddharth.shrimali@gmail.com>
After --drop-filtered removes promisor blobs, append a record of each
dropped object to $GIT_DIR/objects/info/promisor-dropped. Each line
records the object ID, a reflog-style timestamp (Unix seconds and
timezone), the filter spec, and the promisor remote it was attested
recoverable from like the following:
<oid> <time> <tz> filter=<spec> remote=<name>
If a dropped object later becomes unrecoverable (for example, the
branch holding it is deleted on the promisor remote), a lazy fetch
fails with a generic error. This persistent record lets a later change
explain that the object was dropped deliberately, when, under which
filter, and from which remote it was expected to be recoverable.
The remote field lists all configured promisor remotes rather than the
specific one each dropped object is recoverable from. Determining the
exact remote would require asking the remote whether it has the object.
A "remote-object-info" command is being added to the "git cat-file
--batch" protocol for this kind of query, but it is not available yet.
A NEEDSWORK marks this for a follow-up.
The log is written only on a real run, i.e. --dry-run changes nothing.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
builtin/repack.c | 4 +++
repack-promisor.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++
repack.h | 4 +++
3 files changed, 99 insertions(+)
diff --git a/builtin/repack.c b/builtin/repack.c
index aa3257a98a..49dcbbc567 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -702,6 +702,10 @@ int cmd_repack(int argc,
write_midx_file(files->packed, NULL, NULL, flags);
}
+ if (drop_filtered && !dry_run)
+ append_drop_log(repo, &drop_oids,
+ expand_list_objects_filter_spec(&po_args.filter_options));
+
cleanup:
string_list_clear(&keep_pack_list, 0);
string_list_clear(&names, 1);
diff --git a/repack-promisor.c b/repack-promisor.c
index fabfdc168a..60913a5150 100644
--- a/repack-promisor.c
+++ b/repack-promisor.c
@@ -7,6 +7,97 @@
#include "repository.h"
#include "run-command.h"
#include "oidset.h"
+#include "date.h"
+#include "promisor-remote.h"
+#include "strbuf.h"
+
+/*
+ * Append the drop-log entries to the already-computed path.
+ * Returns -1 on any I/O failure so the caller can warn once.
+ * Keeping this in a separate helper avoids goto-based cleanup
+ * in append_drop_log();
+ */
+static int write_to_drop_log(struct repository *repo,
+ const char *path,
+ const struct oidset *dropped,
+ const char *stamp,
+ const char *filter_spec,
+ const char *remotes)
+{
+ struct oidset_iter iter;
+ const struct object_id *oid;
+ FILE *fp;
+
+ if (safe_create_leading_directories(repo, (char *)path)) {
+ warning(_("could not create leading directories for '%s'"), path);
+ return -1;
+ }
+
+ fp = fopen(path, "a");
+ if (!fp) {
+ warning_errno(_("could not open '%s'"), path);
+ return -1;
+ }
+
+ oidset_iter_init(dropped, &iter);
+ while ((oid = oidset_iter_next(&iter))) {
+ if (fprintf(fp, "%s %s filter=%s remote=%s\n",
+ oid_to_hex(oid), stamp,
+ filter_spec ? filter_spec : "",
+ remotes) < 0) {
+ warning(_("could not write to '%s'"), path);
+ fclose(fp);
+ return -1;
+ }
+ }
+
+ if (fclose(fp)) {
+ warning_errno(_("could not close '%s'"), path);
+ return -1;
+ }
+
+ return 0;
+}
+
+void append_drop_log(struct repository *repo,
+ const struct oidset *dropped,
+ const char *filter_spec)
+{
+ char *path;
+ struct strbuf stamp = STRBUF_INIT;
+ struct strbuf remotes = STRBUF_INIT;
+ struct promisor_remote *pr;
+
+ if (!oidset_size(dropped))
+ return;
+
+ datestamp(&stamp);
+
+ /*
+ * NEEDSWORK: we temporarily record all configured promisor remotes rather
+ * than the specific one a given object is recoverable from because there
+ * is currently no way to determine that locally. it would require
+ * asking the remote whether it has the object. A "remote-object-info"
+ * command is being added to the "git cat-file --batch" protocol for
+ * this kind of query. Once it is merged in the codebase, this should
+ * record the exact promisor remote that has each dropped object.
+ */
+ for (pr = repo_promisor_remote_find(repo, NULL); pr; pr = pr->next) {
+ if (remotes.len)
+ strbuf_addch(&remotes, ',');
+ strbuf_addstr(&remotes, pr->name);
+ }
+
+ path = repo_git_path(repo, "objects/info/promisor-dropped");
+
+ if (write_to_drop_log(repo, path, dropped, stamp.buf,
+ filter_spec, remotes.buf))
+ warning(_("could not record all dropped objects in the drop log"));
+
+ strbuf_release(&stamp);
+ strbuf_release(&remotes);
+ free(path);
+}
struct write_oid_context {
struct child_process *cmd;
diff --git a/repack.h b/repack.h
index 61e554e4ed..33309548ce 100644
--- a/repack.h
+++ b/repack.h
@@ -171,6 +171,10 @@ int enumerate_promisor_blobs(struct repository *repo,
const struct list_objects_filter_options *filter,
struct oidset *to_drop);
+void append_drop_log(struct repository *repo,
+ const struct oidset *dropped,
+ const char *filter_spec);
+
int write_cruft_pack(const struct write_pack_opts *opts,
const char *cruft_expiration,
unsigned long combine_cruft_below_size,
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] copy: drop dependency on `the_repository`
From: Phillip Wood @ 2026-07-16 13:41 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260716-pks-copy-wo-the-repository-v1-1-8f1e078bb82f@pks.im>
Hi Patrick
On 16/07/2026 10:56, Patrick Steinhardt wrote:
> When copying a file we need to potentially adapt permissions of the new
> file based on whether or not "core.shared" is enabled. Parsing this
> configuration makes us implicitly depend on `the_repository`.
>
> Refactor the code to instead require the caller to pass in a repository
> so that we can remove `USE_THE_REPOSITORY_VARIABLE`.
Sounds sensible
> diff --git a/sequencer.c b/sequencer.c
> index 1355a99a09..c9ede9c02d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2419,7 +2419,7 @@ static int do_pick_commit(struct repository *r,
> } else {
> const char *dest = git_path_squash_msg(r);
> unlink(dest);
> - if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
> + if (copy_file(the_repository, dest, rebase_path_squash_msg(), 0666)) {
The path for "dest" is obtained using a local repository instance "r",
but we're using "the_repository" to set the permissions on that path.
While that matches the current behavior it is clearly better to use the
same repository instance to obtain both the path and and permissions for
that path. In the hunk below we even have "the_repository" and "r" on
the same line which seems confusing. This patch uses a local repository
instance in refs/files-backend.c and setup.c, lets do the same here.
Thanks
Phillip
> res = error(_("could not copy '%s' to '%s'"),
> rebase_path_squash_msg(), dest);
> goto leave;
> @@ -3864,11 +3864,11 @@ static int error_failed_squash(struct repository *r,
> int subject_len,
> const char *subject)
> {
> - if (copy_file(rebase_path_message(), rebase_path_squash_msg(), 0666))
> + if (copy_file(the_repository, rebase_path_message(), rebase_path_squash_msg(), 0666))
> return error(_("could not copy '%s' to '%s'"),
> rebase_path_squash_msg(), rebase_path_message());
> unlink(git_path_merge_msg(r));
> - if (copy_file(git_path_merge_msg(r), rebase_path_message(), 0666))
> + if (copy_file(the_repository, git_path_merge_msg(r), rebase_path_message(), 0666))
> return error(_("could not copy '%s' to '%s'"),
> rebase_path_message(),
> git_path_merge_msg(r));
> diff --git a/setup.c b/setup.c
> index 0de56a074f..91d61a5939 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -2331,7 +2331,7 @@ static void copy_templates_1(struct repository *repo,
> strbuf_release(&lnk);
> }
> else if (S_ISREG(st_template.st_mode)) {
> - if (copy_file(path->buf, template_path->buf, st_template.st_mode))
> + if (copy_file(repo, path->buf, template_path->buf, st_template.st_mode))
> die_errno(_("cannot copy '%s' to '%s'"),
> template_path->buf, path->buf);
> }
>
> ---
> base-commit: d35c5399e3e54ac277bb391fc2f6be3e816d312b
> change-id: 20260716-pks-copy-wo-the-repository-aa01ccdbed76
>
>
^ permalink raw reply
* [PATCH v6 0/2] fetch: make submodule fetch errors configurable
From: Paulius Zaleckas @ 2026-07-16 14:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Ramsay Jones, Paulius Zaleckas
In-Reply-To: <20260715103518.526326-1-paulius.zaleckas@gmail.com>
When fetching with --recurse-submodules, git currently exits with a
non-zero status if any submodule references an OID that is not reachable
from the submodule's remote. This situation arises naturally when an
upstream branch is still in preparation (e.g. a topic branch in a merge
window): the local branch does not depend on the missing commit, so a
hard failure is unnecessarily disruptive.
Patch 1 fixes a pre-existing NEEDSWORK in submodule.c where a phase-1
fetch failure was recorded immediately, even when a phase-2 OID-based
retry was about to be scheduled. After this fix the existing fatal
behaviour is preserved but the logic is now structured so that errors
are only recorded when the phase-2 retry actually fails, or when there
is no phase-2 retry to fall back on.
Patch 2 introduces fetch.submoduleErrors (fail|warn) and
--submodule-errors=(fail|warn) to let users opt into non-fatal
behaviour. The default remains fail for full backwards compatibility.
Changes in v6:
- Clean up the fail/warn helpers to match the example Junio gave
earlier: singular array name, plain unsigned instead of size_t
casts
Changes in v5:
- Use test_grep instead of raw grep in the new tests (Ramsay, Junio)
- Parse and format the fail/warn values through a single name array
shared by config, option parsing and option forwarding; values are
now matched case-sensitively (Junio)
- Credit Jean-Noël for the v2 documentation fixes, which I forgot to
do back then
Changes in v4:
- Forward an explicit --submodule-errors=fail to child fetches as well,
so the command line overrides fetch.submoduleErrors=warn config in
the per-remote children of fetch --all/--multiple (noticed by Junio)
Changes in v3:
- Report a phase-1 failure also when the gitlink commits are already
present locally, instead of silently succeeding
- Route "Could not access submodule" through record_fetch_error() so it
shows up in the error summary and honors the warn mode
- Forward --submodule-errors to child fetches so it takes effect for
fetch --all/--multiple and nested submodule recursion
- Add tests for all of the above
- Documentation: don't imply git pull takes --submodule-errors, minor
wording and placement fixes
Changes in v2:
- Fix option synopsis to use (fail|warn) instead of <fail|warn>
(Jean-Noël)
- Add --submodule-errors documentation to Documentation/fetch-options.adoc
(Jean-Noël)
Paulius Zaleckas (2):
submodule: fix premature failure in recursive submodule fetch
fetch: add fetch.submoduleErrors to make submodule fetch errors
non-fatal
Documentation/config/fetch.adoc | 14 +++
Documentation/fetch-options.adoc | 8 ++
builtin/fetch.c | 70 +++++++++++++-
submodule.c | 58 ++++++++---
submodule.h | 7 +-
t/t5526-fetch-submodules.sh | 161 +++++++++++++++++++++++++++++++
6 files changed, 301 insertions(+), 17 deletions(-)
--
2.54.0
^ permalink raw reply
* [PATCH v6 1/2] submodule: fix premature failure in recursive submodule fetch
From: Paulius Zaleckas @ 2026-07-16 14:09 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ramsay Jones, Paulius Zaleckas, Elijah Newren,
Patrick Steinhardt, Jonathan Tan, Glen Choo
In-Reply-To: <20260716140956.1023740-1-paulius.zaleckas@gmail.com>
When git fetch --recurse-submodules encounters a failure fetching a
submodule's refs (phase 1), it immediately marks the overall operation
as failed, even though a subsequent OID-based fetch (phase 2) is about
to be attempted for any missing commits. If phase 2 succeeds, the
overall result should be success, but the prematurely set failure flag
makes it look like an error.
Restructure fetch_finish() so that a phase-1 failure does not record an
error immediately. Instead, the decision is deferred:
- If missing commits trigger a phase-2 (OID-based) retry and that
retry succeeds, no error is recorded.
- If the phase-2 retry also fails, the error is recorded then.
- If the submodule was fetched unconditionally (RECURSE_SUBMODULES_ON)
and is not in the changed list, a phase-1 failure is recorded right
away since there is no OID retry to fall back on.
- If phase 1 fails but all required commits are already present
locally, there is no retry to defer to; the failure is still
recorded, since the fetch itself went wrong (e.g. a transport
error) even though the wanted commits happen to be available.
This resolves the NEEDSWORK comment added by bd5e567dc7 (submodule:
explain first attempt failure clearly, 2019-03-13).
Extract the common error-recording logic into a helper
record_fetch_error() and use it in fetch_start_failure() and for the
"Could not access submodule" error in get_fetch_task_from_index() as
well; the latter now also lists the submodule in the final error
summary.
Add a test ensuring a failed submodule fetch is still reported when
the gitlinked commits happen to be present locally.
Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Paulius Zaleckas <paulius.zaleckas@gmail.com>
---
submodule.c | 52 +++++++++++++++++++--------
t/t5526-fetch-submodules.sh | 72 +++++++++++++++++++++++++++++++++++++
2 files changed, 110 insertions(+), 14 deletions(-)
diff --git a/submodule.c b/submodule.c
index fd91201a92..8bcef68a42 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1562,6 +1562,13 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
return NULL;
}
+static void record_fetch_error(struct submodule_parallel_fetch *spf,
+ const char *name)
+{
+ spf->result = 1;
+ strbuf_addf(&spf->submodules_with_errors, "\t%s\n", name);
+}
+
static struct fetch_task *
get_fetch_task_from_index(struct submodule_parallel_fetch *spf,
struct strbuf *err)
@@ -1599,7 +1606,7 @@ get_fetch_task_from_index(struct submodule_parallel_fetch *spf,
ce->name);
if (S_ISGITLINK(ce->ce_mode) &&
!is_empty_dir(empty_submodule_path.buf)) {
- spf->result = 1;
+ record_fetch_error(spf, ce->name);
strbuf_addf(err,
_("Could not access submodule '%s'\n"),
ce->name);
@@ -1753,7 +1760,7 @@ static int fetch_start_failure(struct strbuf *err UNUSED,
struct submodule_parallel_fetch *spf = cb;
struct fetch_task *task = task_cb;
- spf->result = 1;
+ record_fetch_error(spf, task->sub->name);
fetch_task_free(task);
return 0;
@@ -1779,18 +1786,12 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
if (!task || !task->sub)
BUG("callback cookie bogus");
- if (retvalue) {
+ if (retvalue && task->commits) {
/*
- * NEEDSWORK: This indicates that the overall fetch
- * failed, even though there may be a subsequent fetch
- * by commit hash that might work. It may be a good
- * idea to not indicate failure in this case, and only
- * indicate failure if the subsequent fetch fails.
+ * This is the second pass (OID-based fetch) and it failed.
+ * The commits are genuinely unavailable from the remote.
*/
- spf->result = 1;
-
- strbuf_addf(&spf->submodules_with_errors, "\t%s\n",
- task->sub->name);
+ record_fetch_error(spf, task->sub->name);
}
/* Is this the second time we process this submodule? */
@@ -1798,9 +1799,17 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
goto out;
it = string_list_lookup(&spf->changed_submodule_names, task->sub->name);
- if (!it)
- /* Could be an unchanged submodule, not contained in the list */
+ if (!it) {
+ /*
+ * This submodule is not in the changed list (e.g. it was
+ * fetched because RECURSE_SUBMODULES_ON fetches all populated
+ * submodules). A phase 1 failure here has no OID-based retry
+ * to fall back on, so it is a genuine error.
+ */
+ if (retvalue)
+ record_fetch_error(spf, task->sub->name);
goto out;
+ }
cs_data = it->util;
oid_array_filter(&cs_data->new_commits,
@@ -1809,6 +1818,11 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
/* Are there commits we want, but do not exist? */
if (cs_data->new_commits.nr) {
+ /*
+ * Schedule an OID-based phase 2 fetch to retrieve the missing
+ * commits directly. Defer any error from phase 1: if phase 2
+ * succeeds, the overall operation should still succeed.
+ */
task->commits = &cs_data->new_commits;
ALLOC_GROW(spf->oid_fetch_tasks,
spf->oid_fetch_tasks_nr + 1,
@@ -1818,6 +1832,16 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
return 0;
}
+ /*
+ * All required commits are already present locally (they were either
+ * fetched by phase 1 or existed beforehand), so there is no phase 2
+ * retry to defer to. If phase 1 failed, the fetch itself went wrong
+ * (e.g. a transport error) and must still be reported, even though
+ * the gitlinked commits are available.
+ */
+ if (retvalue)
+ record_fetch_error(spf, task->sub->name);
+
out:
fetch_task_free(task);
return 0;
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1242ee9185..7ad274ce04 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -1262,4 +1262,76 @@ test_expect_success "fetch --all with --no-recurse-submodules only fetches super
! grep "Fetching submodule" fetch-log
'
+# Create an isolated environment for submodule fetch error tests.
+#
+# Sets up sub_bare (the submodule upstream), super_bare (the superproject
+# upstream), super_work (a working clone of super_bare with an initialized
+# submodule), and clone (a clone of super_bare with an initialized submodule
+# at a reachable commit). The caller can then create an unreachable commit
+# and push the superproject to put the clone one commit behind a state it
+# cannot fully fetch.
+#
+# Usage: create_err_env <envdir>
+create_err_env () {
+ local envdir="$1" &&
+ mkdir "$envdir" &&
+
+ git init --bare "$envdir/sub_bare" &&
+ git clone "$envdir/sub_bare" "$envdir/sub_work" &&
+ test_commit -C "$envdir/sub_work" "${envdir}_base" &&
+ git -C "$envdir/sub_work" push &&
+
+ git init --bare "$envdir/super_bare" &&
+ git clone "$envdir/super_bare" "$envdir/super_work" &&
+ git -C "$envdir/super_work" submodule add \
+ "$pwd/$envdir/sub_bare" sub &&
+ git -C "$envdir/super_work" commit -m "add submodule" &&
+ git -C "$envdir/super_work" push &&
+
+ git clone "$envdir/super_bare" "$envdir/clone" &&
+ git -C "$envdir/clone" submodule update --init
+}
+
+# Push a commit to <envdir>/super_bare that records a submodule SHA that is
+# present locally in super_work/sub but NOT pushed to sub_bare, making the
+# submodule commit unreachable from clone's sub remote.
+push_unreachable_commit () {
+ local envdir="$1" &&
+ git -C "$envdir/super_work/sub" commit --allow-empty -m "unreachable" &&
+ git -C "$envdir/super_work" add sub &&
+ git -C "$envdir/super_work" commit -m "point sub to unreachable commit" &&
+ git -C "$envdir/super_work" push
+}
+
+test_expect_success 'setup for submodule fetch error tests' '
+ git config --global protocol.file.allow always
+'
+
+test_expect_success 'failed submodule fetch is fatal even when its commits are present locally' '
+ # Create the same commit (unreferenced, via commit-tree with fixed
+ # dates) in both super_work/sub and clone/sub, point the gitlink at
+ # it, and break clone/sub'\''s remote. The commit exists in clone/sub
+ # but is unreachable, so the submodule stays in the changed list; the
+ # fetch failure must still be reported even though there is nothing
+ # left to fetch by commit hash.
+ test_when_finished "rm -fr env_phase1" &&
+ create_err_env env_phase1 &&
+ commit=$(GIT_AUTHOR_DATE="1234567890 +0000" \
+ GIT_COMMITTER_DATE="1234567890 +0000" \
+ git -C env_phase1/super_work/sub commit-tree \
+ "HEAD^{tree}" -p HEAD -m present) &&
+ present=$(GIT_AUTHOR_DATE="1234567890 +0000" \
+ GIT_COMMITTER_DATE="1234567890 +0000" \
+ git -C env_phase1/clone/sub commit-tree \
+ "HEAD^{tree}" -p HEAD -m present) &&
+ test "$commit" = "$present" &&
+ git -C env_phase1/super_work/sub checkout "$commit" &&
+ git -C env_phase1/super_work add sub &&
+ git -C env_phase1/super_work commit -m "gitlink to locally-present commit" &&
+ git -C env_phase1/super_work push &&
+ git -C env_phase1/clone/sub remote set-url origin "$pwd/env_phase1/missing" &&
+ test_must_fail git -C env_phase1/clone fetch --recurse-submodules 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
test_done
--
2.54.0
^ permalink raw reply related
* [PATCH v6 2/2] fetch: add fetch.submoduleErrors to make submodule fetch errors non-fatal
From: Paulius Zaleckas @ 2026-07-16 14:09 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ramsay Jones, Paulius Zaleckas,
Jean-Noël Avila, Ævar Arnfjörð Bjarmason,
Glen Choo, Patrick Steinhardt
In-Reply-To: <20260716140956.1023740-1-paulius.zaleckas@gmail.com>
When fetching with --recurse-submodules, a submodule commit that is not
yet reachable from any of the submodule's remote refs causes the entire
fetch to fail. This is overly strict when the missing commit belongs to
an upstream branch that is still being prepared (e.g. an in-progress
merge topic): the local branch does not need that commit, so there is no
reason to treat its absence as fatal.
Add a new config key fetch.submoduleErrors (values: fail/warn) and a
corresponding --submodule-errors=(fail|warn) command-line option that
control this behaviour. The default remains fail (existing behaviour);
setting the value to warn causes submodule fetch failures to be reported
on stderr without affecting the overall exit status of git fetch / git
pull.
Forward the option to child fetches in add_options_to_argv() so that it
also takes effect for `git fetch --all` / `--multiple` (where per-remote
child processes handle the submodule recursion themselves) and for
nested submodule recursion. The resolved value is forwarded whenever it
was set explicitly, in either direction: the per-remote children re-read
the repository configuration, so a command-line --submodule-errors=fail
must be passed down to them to override fetch.submoduleErrors=warn from
the configuration. When neither the configuration nor the command line
sets a value, nothing is forwarded and the child processes fall back to
their own configuration.
Helped-by: Jean-Noël Avila <avila.jn@gmail.com>
Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Paulius Zaleckas <paulius.zaleckas@gmail.com>
---
Documentation/config/fetch.adoc | 14 +++++
Documentation/fetch-options.adoc | 8 +++
builtin/fetch.c | 70 ++++++++++++++++++++++++-
submodule.c | 8 ++-
submodule.h | 7 ++-
t/t5526-fetch-submodules.sh | 89 ++++++++++++++++++++++++++++++++
6 files changed, 192 insertions(+), 4 deletions(-)
diff --git a/Documentation/config/fetch.adoc b/Documentation/config/fetch.adoc
index 04ac90912d..5c9c942a70 100644
--- a/Documentation/config/fetch.adoc
+++ b/Documentation/config/fetch.adoc
@@ -10,6 +10,20 @@
reference.
Defaults to `on-demand`, or to the value of `submodule.recurse` if set.
+`fetch.submoduleErrors`::
+ Controls how errors from submodule fetches are handled when
+ `--recurse-submodules` is in effect. When set to `fail` (the default),
+ any submodule fetch error causes the overall `git fetch` or `git pull`
+ to exit with a non-zero status. When set to `warn`, submodule fetch
+ errors are reported to standard error but do not affect the exit
+ status of the command. This is useful when working in repositories
+ where some branches reference submodule commits that are not yet
+ available on the submodule remote, but those commits are not needed
+ for the currently checked-out branch.
++
+The value of this option can be overridden by the `--submodule-errors`
+option of linkgit:git-fetch[1].
+
`fetch.fsckObjects`::
If it is set to true, git-fetch-pack will check all fetched
objects. See `transfer.fsckObjects` for what's
diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
index 035f780e58..78525f6848 100644
--- a/Documentation/fetch-options.adoc
+++ b/Documentation/fetch-options.adoc
@@ -294,6 +294,14 @@ ifndef::git-pull[]
`--no-recurse-submodules`::
Disable recursive fetching of submodules (this has the same effect as
using the `--recurse-submodules=no` option).
+
+`--submodule-errors=(fail|warn)`::
+ Control how errors from submodule fetches are handled when
+ `--recurse-submodules` is in effect. When set to `fail` (the default),
+ any submodule fetch error causes the overall `git fetch` to exit with a
+ non-zero status. When set to `warn`, submodule fetch errors are reported
+ to standard error but do not affect the exit status of the command. Can
+ also be configured via `fetch.submoduleErrors`. See linkgit:git-config[1].
endif::git-pull[]
`--set-upstream`::
diff --git a/builtin/fetch.c b/builtin/fetch.c
index c1d7c672f4..2c583ed0cc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -110,8 +110,30 @@ struct fetch_config {
int recurse_submodules;
int parallel;
int submodule_fetch_jobs;
+ int submodule_errors;
};
+/* really private - use accessors below to parse and format */
+static const char *submodule_error_name[] = {
+ [SUBMODULE_ERRORS_FAIL] = "fail",
+ [SUBMODULE_ERRORS_WARN] = "warn",
+};
+
+static const char *submodule_error(unsigned num)
+{
+ if (ARRAY_SIZE(submodule_error_name) <= num)
+ BUG("invalid submodule errors mode %u", num);
+ return submodule_error_name[num];
+}
+
+static int parse_submodule_error(const char *name)
+{
+ for (unsigned num = 0; num < ARRAY_SIZE(submodule_error_name); num++)
+ if (!strcmp(submodule_error_name[num], name))
+ return num;
+ return -1;
+}
+
static int git_fetch_config(const char *k, const char *v,
const struct config_context *ctx, void *cb)
{
@@ -152,6 +174,19 @@ static int git_fetch_config(const char *k, const char *v,
return 0;
}
+ if (!strcmp(k, "fetch.submoduleerrors")) {
+ int mode;
+
+ if (!v)
+ return config_error_nonbool(k);
+ mode = parse_submodule_error(v);
+ if (mode < 0)
+ die(_("invalid value for '%s': '%s'"),
+ "fetch.submoduleErrors", v);
+ fetch_config->submodule_errors = mode;
+ return 0;
+ }
+
if (!strcmp(k, "fetch.parallel")) {
fetch_config->parallel = git_config_int(k, v, ctx->kvi);
if (fetch_config->parallel < 0)
@@ -2205,6 +2240,9 @@ static void add_options_to_argv(struct strvec *argv,
strvec_push(argv, "--no-recurse-submodules");
else if (config->recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
strvec_push(argv, "--recurse-submodules=on-demand");
+ if (config->submodule_errors != -1)
+ strvec_pushf(argv, "--submodule-errors=%s",
+ submodule_error(config->submodule_errors));
if (tags == TAGS_SET)
strvec_push(argv, "--tags");
else if (tags == TAGS_UNSET)
@@ -2464,6 +2502,23 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
return exit_code;
}
+static int option_parse_submodule_errors(const struct option *opt,
+ const char *arg, int unset)
+{
+ int *v = opt->value;
+ int mode;
+
+ if (unset) {
+ *v = SUBMODULE_ERRORS_FAIL;
+ return 0;
+ }
+ mode = parse_submodule_error(arg);
+ if (mode < 0)
+ die(_("invalid value for '%s': '%s'"), "--submodule-errors", arg);
+ *v = mode;
+ return 0;
+}
+
int cmd_fetch(int argc,
const char **argv,
const char *prefix,
@@ -2477,6 +2532,7 @@ int cmd_fetch(int argc,
.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,
.parallel = 1,
.submodule_fetch_jobs = -1,
+ .submodule_errors = -1, /* unset */
};
const char *submodule_prefix = "";
const char *bundle_uri;
@@ -2491,6 +2547,7 @@ int cmd_fetch(int argc,
int max_jobs = -1;
int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
+ int submodule_errors_cli = -1; /* -1: not set on command line */
int fetch_write_commit_graph = -1;
int stdin_refspecs = 0;
int negotiate_only = 0;
@@ -2527,6 +2584,10 @@ int cmd_fetch(int argc,
OPT_CALLBACK_F(0, "recurse-submodules", &recurse_submodules_cli, N_("on-demand"),
N_("control recursive fetching of submodules"),
PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
+ OPT_CALLBACK_F(0, "submodule-errors", &submodule_errors_cli,
+ N_("(fail|warn)"),
+ N_("control how submodule fetch errors are handled"),
+ 0, option_parse_submodule_errors),
OPT_BOOL(0, "dry-run", &dry_run,
N_("dry run")),
OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
@@ -2616,6 +2677,9 @@ int cmd_fetch(int argc,
if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
config.recurse_submodules = recurse_submodules_cli;
+ if (submodule_errors_cli != -1)
+ config.submodule_errors = submodule_errors_cli;
+
if (negotiate_only) {
switch (recurse_submodules_cli) {
case RECURSE_SUBMODULES_OFF:
@@ -2819,11 +2883,14 @@ int cmd_fetch(int argc,
if (!result && remote && (config.recurse_submodules != RECURSE_SUBMODULES_OFF)) {
struct strvec options = STRVEC_INIT;
int max_children = max_jobs;
+ int submodule_errors = config.submodule_errors;
if (max_children < 0)
max_children = config.submodule_fetch_jobs;
if (max_children < 0)
max_children = config.parallel;
+ if (submodule_errors < 0)
+ submodule_errors = SUBMODULE_ERRORS_FAIL;
add_options_to_argv(&options, &config);
trace2_region_enter_printf("fetch", "recurse-submodule", the_repository, "%s", submodule_prefix);
@@ -2833,7 +2900,8 @@ int cmd_fetch(int argc,
config.recurse_submodules,
recurse_submodules_default,
verbosity < 0,
- max_children);
+ max_children,
+ submodule_errors);
trace2_region_leave_printf("fetch", "recurse-submodule", the_repository, "%s", submodule_prefix);
strvec_clear(&options);
}
diff --git a/submodule.c b/submodule.c
index 8bcef68a42..da4ace751f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1409,6 +1409,7 @@ struct submodule_parallel_fetch {
int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
struct strbuf submodules_with_errors;
+ int submodule_errors;
};
#define SPF_INIT { \
.args = STRVEC_INIT, \
@@ -1565,7 +1566,8 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
static void record_fetch_error(struct submodule_parallel_fetch *spf,
const char *name)
{
- spf->result = 1;
+ if (spf->submodule_errors == SUBMODULE_ERRORS_FAIL)
+ spf->result = 1;
strbuf_addf(&spf->submodules_with_errors, "\t%s\n", name);
}
@@ -1851,7 +1853,8 @@ int fetch_submodules(struct repository *r,
const struct strvec *options,
const char *prefix, int command_line_option,
int default_option,
- int quiet, int max_parallel_jobs)
+ int quiet, int max_parallel_jobs,
+ int submodule_errors)
{
struct submodule_parallel_fetch spf = SPF_INIT;
const struct run_process_parallel_opts opts = {
@@ -1871,6 +1874,7 @@ int fetch_submodules(struct repository *r,
spf.default_option = default_option;
spf.quiet = quiet;
spf.prefix = prefix;
+ spf.submodule_errors = submodule_errors;
if (!r->worktree)
goto out;
diff --git a/submodule.h b/submodule.h
index b10e16e6c0..c80b687d2a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -90,12 +90,17 @@ int should_update_submodules(void);
*/
const struct submodule *submodule_from_ce(const struct cache_entry *ce);
void check_for_new_submodule_commits(struct object_id *oid);
+/* Values for the submodule_errors parameter of fetch_submodules(). */
+#define SUBMODULE_ERRORS_FAIL 0 /* submodule fetch errors are fatal (default) */
+#define SUBMODULE_ERRORS_WARN 1 /* submodule fetch errors are non-fatal warnings */
+
int fetch_submodules(struct repository *r,
const struct strvec *options,
const char *prefix,
int command_line_option,
int default_option,
- int quiet, int max_parallel_jobs);
+ int quiet, int max_parallel_jobs,
+ int submodule_errors);
unsigned is_submodule_modified(const char *path, int ignore_untracked);
int submodule_uses_gitfile(const char *path);
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 7ad274ce04..19d17440cf 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -1307,6 +1307,57 @@ test_expect_success 'setup for submodule fetch error tests' '
git config --global protocol.file.allow always
'
+test_expect_success 'fetch --recurse-submodules fails when submodule commit is unreachable (default)' '
+ test_when_finished "rm -fr env_default" &&
+ create_err_env env_default &&
+ push_unreachable_commit env_default &&
+ test_must_fail git -C env_default/clone fetch --recurse-submodules 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success 'fetch.submoduleErrors=warn: unreachable submodule commit is non-fatal' '
+ test_when_finished "rm -fr env_warn_cfg" &&
+ create_err_env env_warn_cfg &&
+ push_unreachable_commit env_warn_cfg &&
+ git -C env_warn_cfg/clone -c fetch.submoduleErrors=warn \
+ fetch --recurse-submodules 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success '--submodule-errors=warn: unreachable submodule commit is non-fatal' '
+ test_when_finished "rm -fr env_warn_cli" &&
+ create_err_env env_warn_cli &&
+ push_unreachable_commit env_warn_cli &&
+ git -C env_warn_cli/clone fetch --recurse-submodules \
+ --submodule-errors=warn 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success '--submodule-errors=fail: unreachable submodule commit is fatal' '
+ test_when_finished "rm -fr env_fail_cli" &&
+ create_err_env env_fail_cli &&
+ push_unreachable_commit env_fail_cli &&
+ test_must_fail git -C env_fail_cli/clone fetch --recurse-submodules \
+ --submodule-errors=fail 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success 'fetch.submoduleErrors=warn does not suppress successful fetch' '
+ # A new reachable submodule commit (pushed to sub_bare) should be
+ # fetched without any error summary.
+ test_when_finished "rm -fr env_ok" &&
+ create_err_env env_ok &&
+ test_commit -C env_ok/sub_work reachable_ok &&
+ git -C env_ok/sub_work push &&
+ git -C env_ok/super_work submodule update --remote &&
+ git -C env_ok/super_work add sub &&
+ git -C env_ok/super_work commit -m "point sub to reachable commit" &&
+ git -C env_ok/super_work push &&
+ git -C env_ok/clone -c fetch.submoduleErrors=warn \
+ fetch --recurse-submodules 2>err &&
+ test_grep ! "Errors during submodule fetch" err
+'
+
test_expect_success 'failed submodule fetch is fatal even when its commits are present locally' '
# Create the same commit (unreferenced, via commit-tree with fixed
# dates) in both super_work/sub and clone/sub, point the gitlink at
@@ -1334,4 +1385,42 @@ test_expect_success 'failed submodule fetch is fatal even when its commits are p
test_grep "Errors during submodule fetch" err
'
+test_expect_success '--submodule-errors=warn is honored by fetch --all' '
+ # A second remote forces fetch_multiple(), which hands the submodule
+ # recursion off to per-remote child processes; the option must be
+ # forwarded to them.
+ test_when_finished "rm -fr env_all" &&
+ create_err_env env_all &&
+ push_unreachable_commit env_all &&
+ git -C env_all/clone remote add second "$pwd/env_all/super_bare" &&
+ git -C env_all/clone fetch --all --recurse-submodules \
+ --submodule-errors=warn 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success '--submodule-errors=fail overrides warn config for fetch --all' '
+ # The per-remote child processes re-read the repository config, so
+ # the command-line override must be forwarded to them explicitly.
+ test_when_finished "rm -fr env_override" &&
+ create_err_env env_override &&
+ push_unreachable_commit env_override &&
+ git -C env_override/clone remote add second "$pwd/env_override/super_bare" &&
+ git -C env_override/clone config fetch.submoduleErrors warn &&
+ test_must_fail git -C env_override/clone fetch --all --recurse-submodules \
+ --submodule-errors=fail 2>err &&
+ test_grep "Errors during submodule fetch" err
+'
+
+test_expect_success 'fetch.submoduleErrors=warn: inaccessible submodule is non-fatal' '
+ test_when_finished "rm -fr env_access" &&
+ create_err_env env_access &&
+ rm env_access/clone/sub/.git &&
+ rm -r env_access/clone/.git/modules/sub &&
+ git -C env_access/clone -c fetch.submoduleErrors=warn \
+ fetch --recurse-submodules 2>err &&
+ test_grep "Could not access submodule" err &&
+ test_must_fail git -C env_access/clone fetch --recurse-submodules 2>err &&
+ test_grep "Could not access submodule" err
+'
+
test_done
--
2.54.0
^ permalink raw reply related
* [PATCH 0/2] Some wincred fixes
From: Johannes Schindelin via GitGitGadget @ 2026-07-16 14:27 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
These were rolled out as part of the security fix release Git for Windows
v2.55.0(3).
Johannes Schindelin (2):
wincred: avoid memory corruption when erasing a credential
wincred: prevent silent credential loss when storing OAuth tokens
contrib/credential/wincred/git-credential-wincred.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2182%2Fdscho%2Fwincred-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2182/dscho/wincred-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2182
--
gitgitgadget
^ 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;
as well as URLs for NNTP newsgroup(s).