* [PATCH v2 17/18] odb/source-loose: stub out remaining callbacks
From: Patrick Steinhardt @ 2026-06-01 8:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20260601-b4-pks-odb-source-loose-v2-0-90ff159430af@pks.im>
Stub out remaining callback functions for the "loose" backend.
Note that we also stub out transactions for loose objects. In fact, we
already have the infrastructure in place for those, and we could in
theory implement those, as well. But there are separate efforts ongoing
to polish up transactional interfaces, and doing so now would likely
result in some messiness. This omission will thus be worked on in a
subsequent patch series, once the dust has settled.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
odb/source-loose.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/odb/source-loose.c b/odb/source-loose.c
index e52fc289a2..e174941318 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -645,6 +645,25 @@ static int odb_source_loose_write_object_stream(struct odb_source *source,
return odb_source_loose_write_stream(loose, in_stream, len, oid);
}
+static int odb_source_loose_begin_transaction(struct odb_source *source UNUSED,
+ struct odb_transaction **out UNUSED)
+{
+ /* TODO: this is a known omission that we'll want to address eventually. */
+ return error("loose source does not support transactions");
+}
+
+static int odb_source_loose_read_alternates(struct odb_source *source UNUSED,
+ struct strvec *out UNUSED)
+{
+ return 0;
+}
+
+static int odb_source_loose_write_alternate(struct odb_source *source UNUSED,
+ const char *alternate UNUSED)
+{
+ return error("loose source does not support alternates");
+}
+
static void odb_source_loose_clear_cache(struct odb_source_loose *loose)
{
oidtree_clear(loose->cache);
@@ -706,6 +725,9 @@ struct odb_source_loose *odb_source_loose_new(struct odb_source_files *files)
loose->base.freshen_object = odb_source_loose_freshen_object;
loose->base.write_object = odb_source_loose_write_object;
loose->base.write_object_stream = odb_source_loose_write_object_stream;
+ loose->base.begin_transaction = odb_source_loose_begin_transaction;
+ loose->base.read_alternates = odb_source_loose_read_alternates;
+ loose->base.write_alternate = odb_source_loose_write_alternate;
if (!is_absolute_path(loose->base.path))
chdir_notify_register(NULL, odb_source_loose_reparent, loose);
--
2.54.0.926.g75ba10bac6.dirty
^ permalink raw reply related
* [PATCH v2 18/18] odb/source-loose: drop pointer to the "files" source
From: Patrick Steinhardt @ 2026-06-01 8:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20260601-b4-pks-odb-source-loose-v2-0-90ff159430af@pks.im>
Now that all callbacks of the loose source operate on `struct
odb_source_loose` directly we no longer have to reach into the "files"
source at all.
Drop this field and update `odb_source_loose_new()` to instead accept
all parameters required to initialize itself. This ensures that the
"loose" backend is a fully standalone source.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
odb/source-files.c | 2 +-
odb/source-loose.c | 8 ++++----
odb/source-loose.h | 7 ++++---
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/odb/source-files.c b/odb/source-files.c
index 83f8066c67..5bdd042922 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -268,7 +268,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
CALLOC_ARRAY(files, 1);
odb_source_init(&files->base, odb, ODB_SOURCE_FILES, path, local);
- files->loose = odb_source_loose_new(files);
+ files->loose = odb_source_loose_new(odb, path, local);
files->packed = packfile_store_new(&files->base);
files->base.free = odb_source_files_free;
diff --git a/odb/source-loose.c b/odb/source-loose.c
index e174941318..7d7ea2fb84 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -705,14 +705,14 @@ static void odb_source_loose_free(struct odb_source *source)
free(loose);
}
-struct odb_source_loose *odb_source_loose_new(struct odb_source_files *files)
+struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
+ const char *path,
+ bool local)
{
struct odb_source_loose *loose;
CALLOC_ARRAY(loose, 1);
- odb_source_init(&loose->base, files->base.odb, ODB_SOURCE_LOOSE,
- files->base.path, files->base.local);
- loose->files = files;
+ odb_source_init(&loose->base, odb, ODB_SOURCE_LOOSE, path, local);
loose->base.free = odb_source_loose_free;
loose->base.close = odb_source_loose_close;
diff --git a/odb/source-loose.h b/odb/source-loose.h
index 4dd4fd6ce3..6070aaf3ce 100644
--- a/odb/source-loose.h
+++ b/odb/source-loose.h
@@ -9,11 +9,10 @@ struct oidtree;
/*
* An object database source that stores its objects in loose format, one
- * file per object. This source is part of the files source.
+ * file per object.
*/
struct odb_source_loose {
struct odb_source base;
- struct odb_source_files *files;
/*
* Used to store the results of readdir(3) calls when we are OK
@@ -31,7 +30,9 @@ struct odb_source_loose {
struct loose_object_map *map;
};
-struct odb_source_loose *odb_source_loose_new(struct odb_source_files *files);
+struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
+ const char *path,
+ bool local);
/*
* Cast the given object database source to the loose backend. This will cause
--
2.54.0.926.g75ba10bac6.dirty
^ permalink raw reply related
* Re: [PATCH 2/2] builtin/init-db: deprecate alias for git-init(1)
From: Kristoffer Haugsbakk @ 2026-06-01 9:31 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260601-pks-deprecate-git-init-db-v1-2-ea3e6eebe674@pks.im>
On Mon, Jun 1, 2026, at 09:56, Patrick Steinhardt wrote:
> The git-init-db(1) command was initially only initializing the object
> database of a Git repository. This has changed over time so that the
> command also initializes all the other data structures, which is why we
> have eventually introduced git-init(1) as a more aptly named replacement
> for it.
>
> This has all happened in 2007 already, and with 5c94f87e6b (use 'init'
> instead of 'init-db' for shipped docs and tools, 2007-01-12) we have
> also adapted all user-facing documentation to mention the replacement.
> It is thus safe to assume that (almost) nobody uses git-init-db(1)
> nowadays anymore.
>
> Deprecate the command in favor of git-init(1) and wire up the removal
> when compiling Git with breaking changes enabled.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>[snip]
> diff --git a/git.c b/git.c
> index a72394b599..6bf6a60360 100644
> --- a/git.c
> +++ b/git.c
> @@ -591,7 +591,9 @@ static struct cmd_struct commands[] = {
> { "hook", cmd_hook, RUN_SETUP_GENTLY },
> { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT },
> { "init", cmd_init },
> +#ifndef WITH_BREAKING_CHANGES
> { "init-db", cmd_init },
This can be marked as deprecated.
{ "init-db", cmd_init, DEPRECATED },
> +#endif
> { "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
> { "last-modified", cmd_last_modified, RUN_SETUP },
> { "log", cmd_log, RUN_SETUP },
>[snip]
^ permalink raw reply
* Re: [PATCH v1 3/4] environment: move 'trust_executable_bit' into repo_config_values
From: Tian Yuchen @ 2026-06-01 10:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, christian.couder, ps, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <xmqq7bokebct.fsf@gitster.g>
Hi Junio,
Thanks for the feedback!
On 5/31/26 07:17, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
>
>> diff --git a/apply.c b/apply.c
>> index 249248d4f2..73ca9907f8 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -3890,10 +3890,12 @@ static int check_preimage(struct apply_state *state,
>> }
>>
>> if (!state->cached && !previous) {
>> + struct repo_config_values *cfg = repo_config_values(the_repository);
>> +
>> if (*ce && !(*ce)->ce_mode)
>> BUG("ce_mode == 0 for path '%s'", old_name);
>>
>> - if (trust_executable_bit || !S_ISREG(st->st_mode))
>> + if (cfg->trust_executable_bit || !S_ISREG(st->st_mode))
>> st_mode = ce_mode_from_stat(*ce, st->st_mode);
>> else if (*ce)
>> st_mode = (*ce)->ce_mode;
>> diff --git a/read-cache.c b/read-cache.c
>> index 54150fe756..18af533649 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -204,10 +204,12 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st
>>
>> unsigned int ce_mode_from_stat(const struct cache_entry *ce, unsigned int mode)
>> {
>> + struct repo_config_values *cfg = repo_config_values(the_repository);
>> +
>> if (!has_symlinks && S_ISREG(mode) &&
>> ce && S_ISLNK(ce->ce_mode))
>> return ce->ce_mode;
>> - if (!trust_executable_bit && S_ISREG(mode)) {
>> + if (!cfg->trust_executable_bit && S_ISREG(mode)) {
>> if (ce && S_ISREG(ce->ce_mode))
>> return ce->ce_mode;
>> return create_ce_mode(0666);
>
> How hot are the code paths that call into this helper function? In
> the original under some condition, it was possible to return without
> even consulting the trust_executable_bit variable, but in the
> updated code, the helper unconditionally makes a call to the
> repo_config_values() helper function even before it knows it needs
> to know the value of trust_executable_bit.
That sounds reasonable to me. I’ll adjust the conditional logic in some
of the statements so that they short-circuit appropriately to avoid
performance overhead.
>
>> @@ -217,11 +219,13 @@ unsigned int ce_mode_from_stat(const struct cache_entry *ce, unsigned int mode)
>>
>> static unsigned int st_mode_from_ce(const struct cache_entry *ce)
>> {
>> + struct repo_config_values *cfg = repo_config_values(the_repository);
>> +
>> switch (ce->ce_mode & S_IFMT) {
>> case S_IFLNK:
>> return has_symlinks ? S_IFLNK : (S_IFREG | 0644);
>> case S_IFREG:
>> - return (ce->ce_mode & (trust_executable_bit ? 0755 : 0644)) | S_IFREG;
>> + return (ce->ce_mode & (cfg->trust_executable_bit ? 0755 : 0644)) | S_IFREG;
>> case S_IFGITLINK:
>> return S_IFDIR | 0755;
>> case S_IFDIR:
>
> Ditto.
>
>> @@ -321,6 +325,7 @@ static int ce_modified_check_fs(struct index_state *istate,
>> static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)
>> {
>> unsigned int changed = 0;
>> + struct repo_config_values *cfg = repo_config_values(the_repository);
>>
>> if (ce->ce_flags & CE_REMOVE)
>> return MODE_CHANGED | DATA_CHANGED | TYPE_CHANGED;
>> @@ -331,7 +336,7 @@ static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)
>> /* We consider only the owner x bit to be relevant for
>> * "mode changes"
>> */
>> - if (trust_executable_bit &&
>> + if (cfg->trust_executable_bit &&
>> (0100 & (ce->ce_mode ^ st->st_mode)))
>> changed |= MODE_CHANGED;
>> break;
>
> Ditto.
>
>> @@ -732,6 +737,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>> (intent_only ? ADD_CACHE_NEW_ONLY : 0));
>> unsigned hash_flags = pretend ? 0 : INDEX_WRITE_OBJECT;
>>
>> + struct repo_config_values *cfg = repo_config_values(the_repository);
>> +
>
> Lose the excess blank line before the new declaration.
>
>> if (flags & ADD_CACHE_RENORMALIZE)
>> hash_flags |= INDEX_RENORMALIZE;
>>
>> @@ -752,7 +759,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>> ce->ce_flags |= CE_INTENT_TO_ADD;
>>
>>
>> - if (trust_executable_bit && has_symlinks) {
>> + if (cfg->trust_executable_bit && has_symlinks) {
>> ce->ce_mode = create_ce_mode(st_mode);
>> } else {
>> /* If there is an existing entry, pick the mode bits and type
>
> Almost all of these places that care about trust_executable_bit also
> cares about has_symlinks. I wonder if they should be converted to
> repo-local settings in the same series.
That’s true: I had actually planned to start migrating has_symlinks as
soon as this series was approved. Since you think it would be better to
merge them into a single series, I’ll go ahead and do that ;)
Thanks, yuchen
^ permalink raw reply
* Re: [PATCH 2/2] builtin/init-db: deprecate alias for git-init(1)
From: Patrick Steinhardt @ 2026-06-01 12:10 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <276a92ac-b2cb-4a89-96d0-9071ab6200be@app.fastmail.com>
On Mon, Jun 01, 2026 at 11:31:46AM +0200, Kristoffer Haugsbakk wrote:
> On Mon, Jun 1, 2026, at 09:56, Patrick Steinhardt wrote:
> > diff --git a/git.c b/git.c
> > index a72394b599..6bf6a60360 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -591,7 +591,9 @@ static struct cmd_struct commands[] = {
> > { "hook", cmd_hook, RUN_SETUP_GENTLY },
> > { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT },
> > { "init", cmd_init },
> > +#ifndef WITH_BREAKING_CHANGES
> > { "init-db", cmd_init },
>
> This can be marked as deprecated.
>
> { "init-db", cmd_init, DEPRECATED },
Ah, indeed! Added locally now, thanks.
Patrick
^ permalink raw reply
* Re: [PATCH 1/5] merge-ort: propagate callback errors from traverse_trees_wrapper()
From: Junio C Hamano @ 2026-06-01 12:13 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <282f906d1b4767d95e2a66072c280c2294a93a9f.1776731171.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> traverse_trees_wrapper() saves entries from a first pass through
> traverse_trees() and then replays them through the real callback
> (collect_merge_info_callback). However, the replay loop silently
> discards the callback return value. This means any error reported by
> the callback during replay -- including a future check for malformed
> trees -- would be ignored, allowing the merge to proceed with corrupt
> state.
>
> Capture the return value, stop the loop on negative (error) returns,
> and propagate the error to the caller. Note that the callback returns
> a positive mask value on success, so we normalize non-negative returns
> to 0 for the caller.
All makes perfect sense.
How would the externally visible behaviour change at this step?
Upon an error from the callback, we used to keep going and processed
other callback data in the renames structure. We now leave the rest
unprocessed.
The caller of this helper would never have seen a failure, but now
they will. Both callers, collect_merge_info_callback() and
handle_deferred_entries(), are reacting to a negative "error" return
well (perhaps because they sometimes call traverse_trees() in the
same control flow, which does return an error already), so
presumably there is no downside caused by aborting the innermost
process upon the first error return.
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> merge-ort.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 00923ce3cd..4b8e32209d 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -1008,18 +1008,20 @@ static int traverse_trees_wrapper(struct index_state *istate,
> info->traverse_path = renames->callback_data_traverse_path;
> info->fn = old_fn;
> for (i = old_offset; i < renames->callback_data_nr; ++i) {
> - info->fn(n,
> - renames->callback_data[i].mask,
> - renames->callback_data[i].dirmask,
> - renames->callback_data[i].names,
> - info);
> + ret = info->fn(n,
> + renames->callback_data[i].mask,
> + renames->callback_data[i].dirmask,
> + renames->callback_data[i].names,
> + info);
> + if (ret < 0)
> + break;
> }
>
> renames->callback_data_nr = old_offset;
> free(renames->callback_data_traverse_path);
> renames->callback_data_traverse_path = old_callback_data_traverse_path;
> info->traverse_path = NULL;
> - return 0;
> + return ret < 0 ? ret : 0;
> }
>
> static void setup_path_info(struct merge_options *opt,
^ permalink raw reply
* Re: [PATCH 2/5] merge-ort: drop unnecessary show_all_errors from collect_merge_info()
From: Junio C Hamano @ 2026-06-01 12:23 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <949b5d8e3f3aefd9497a7b85d860259b9d5db418.1776731171.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> collect_merge_info() has set info.show_all_errors = 1 since
> d2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),
> 2020-12-13). This setting was copied from unpack-trees.c where it
> controls batching of error messages for porcelain display, but
> merge-ort has no such error-batching logic and never needed it.
>
> With show_all_errors set, traverse_trees() captures a negative callback
> return but continues processing remaining entries rather than stopping
> immediately. Removing the setting restores the default behavior where
> a negative return from collect_merge_info_callback() breaks out of the
> traversal loop right away, allowing a future commit to exit early when
> a corrupt tree is detected.
Nice spotting. As the error handling eventually is to die without
making any further damange, returning early without seeing "more
errors" is a good change.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> merge-ort.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 4b8e32209d..74e9636020 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -1740,7 +1740,6 @@ static int collect_merge_info(struct merge_options *opt,
> setup_traverse_info(&info, opt->priv->toplevel_dir);
> info.fn = collect_merge_info_callback;
> info.data = opt;
> - info.show_all_errors = 1;
>
> if (repo_parse_tree(opt->repo, merge_base) < 0 ||
> repo_parse_tree(opt->repo, side1) < 0 ||
^ permalink raw reply
* Re: [PATCH 4/5] merge-ort: abort merge when trees have duplicate entries
From: Junio C Hamano @ 2026-06-01 12:23 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <0d81c027aafcb386398836ffc73b058b7ea4c702.1776731171.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> Trees with duplicate entries are malformed; fsck reports "contains
> duplicate file entries" for them. merge-ort has from the beginning
> assumed that we would never hit such trees. It was written with the
> assumption that traverse_trees() calls collect_merge_info_callback() at
> most once per path. The "sanity checks" in that callback (added in
> d2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),
> 2020-12-13)) verify properties of each individual call but not that
> invariant. The strmap_put() in setup_path_info() silently overwrites
> the entry from any prior call for the same path, because it assumed
> there would be no other path. Unfortunately, supplemental data
> structures for various optimizations could still be tweaked before the
> extra paths were overwritten, and those data structures not matching
> expected state could trip various assertions.
>
> Change the return type of setup_path_info() from void to int to allow us
> to detect this case, and abort the merge with a clear error message when
> it occurs.
OK.
> @@ -1081,9 +1081,11 @@ static void setup_path_info(struct merge_options *opt,
> */
> mi->is_null = 1;
> }
> - strmap_put(&opt->priv->paths, fullpath, mi);
> + if (strmap_put(&opt->priv->paths, fullpath, mi))
> + return error(_("tree has duplicate entries for '%s'"), fullpath);
OK. I was wondering what _other_ kind of malformed trees would the
updated code by this change is prepared to handle (most notably,
tree entries must be sorted, and one way to detect duplicate is to
remember one single path that we saw earlier, which would work as
long as the entries are sorted). This "ah, we saw that path already"
approach is much more robust in that it does not have to depend on a
sorted tree.
Makes sense.
^ permalink raw reply
* Re: [PATCH 0/5] Duplicate entry hardening
From: Junio C Hamano @ 2026-06-01 12:33 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <pull.2096.git.1776731171.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> We had some corrupt trees with duplicate entries in real world repositories,
> which triggered an assertion failure in merge-ort. Further, the corrupt tree
> creation in the third party tool would have been avoided had verify_cache()
> correctly checked for D/F conflicts. Provide fixes for both issues,
> including 3 preparatory changes for the merge-ort fix.
>
> Elijah Newren (5):
> merge-ort: propagate callback errors from traverse_trees_wrapper()
> merge-ort: drop unnecessary show_all_errors from collect_merge_info()
> merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()
> merge-ort: abort merge when trees have duplicate entries
> cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts
This is a fix to an important corner of our system, but somehow left
in "Needs review" state for much longer than I would have liked, so
even though I am officially on vacation ;-), I took some time to
read these through (by the way it was a pleasant read, thank you).
I wonder if we create a rule like
Those of you who have more than 30 commits in our project are
expected to review one topic (or more) from other contributors
for every three patches you send and ask for reviews by others.
it would help balance the patch vs review ratio, perhaps?
^ permalink raw reply
* Re: [PATCH 5/5] cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts
From: Junio C Hamano @ 2026-06-01 12:33 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <a87bbaa84fd5dcb2a585f82c4a5dfa1572b54588.1776731171.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> I could not find any caller in current git that both allows the index to
> get into this state and then tries to write it out without doing other
> checks beyond the verify_cache() call in cache_tree_update(), but
> verify_cache() is documented as a safety net for preventing corrupt
> trees and should actually provide that guarantee.
Oh, absolutely. This kind of tightening is very much appreciated.
> diff --git a/cache-tree.c b/cache-tree.c
> index 7881b42aa2..f11844fe72 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -192,22 +192,62 @@ static int verify_cache(struct index_state *istate, int flags)
> for (i = 0; i + 1 < istate->cache_nr; i++) {
> /* path/file always comes after path because of the way
> * the cache is sorted. Also path can appear only once,
> - * which means conflicting one would immediately follow.
> + * so path/file is likely the immediately following path
> + * but might be separated if there is e.g. a
> + * path-internal/... file.
> */
> const struct cache_entry *this_ce = istate->cache[i];
> const struct cache_entry *next_ce = istate->cache[i + 1];
> const char *this_name = this_ce->name;
> const char *next_name = next_ce->name;
> int this_len = ce_namelen(this_ce);
> + const char *conflict_name = NULL;
> +
> if (this_len < ce_namelen(next_ce) &&
> - next_name[this_len] == '/' &&
> + next_name[this_len] <= '/' &&
> strncmp(this_name, next_name, this_len) == 0) {
> + if (next_name[this_len] == '/') {
> + conflict_name = next_name;
> + } else if (next_name[this_len] < '/') {
> + /*
> + * The immediately next entry shares our
> + * prefix but sorts before "path/" (e.g.,
> + * "path-internal" between "path" and
> + * "path/file", since '-' (0x2D) < '/'
> + * (0x2F)). Binary search to find where
> + * "path/" would be and check for a D/F
> + * conflict there.
> + */
> + struct cache_entry *other;
> + struct strbuf probe = STRBUF_INIT;
> + int pos;
> +
> + strbuf_add(&probe, this_name, this_len);
> + strbuf_addch(&probe, '/');
> + pos = index_name_pos_sparse(istate,
> + probe.buf,
> + probe.len);
> + strbuf_release(&probe);
> +
> + if (pos < 0)
> + pos = -pos - 1;
> + if (pos >= (int)istate->cache_nr)
> + continue;
> + other = istate->cache[pos];
> + if (ce_namelen(other) > this_len &&
> + other->name[this_len] == '/' &&
> + !strncmp(this_name, other->name, this_len))
> + conflict_name = other->name;
> + }
> + }
The narrow and tall comment block is a sign that this loop is
getting too deeply nested. I wonder if it makes it easier to follow
if we extract this new logic into a small helper function on its
own?
What the code checks and how it does so both make sense to me, though.
Thanks.
^ permalink raw reply
* Re: [PATCH] index-pack: retain child bases in delta cache
From: Derrick Stolee @ 2026-06-01 12:50 UTC (permalink / raw)
To: Arijit Banerjee via GitGitGadget, git
Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
Arijit Banerjee, Arijit Banerjee
In-Reply-To: <pull.2131.git.1780070763044.gitgitgadget@gmail.com>
On 5/29/2026 12:06 PM, Arijit Banerjee via GitGitGadget wrote:
> From: Arijit Banerjee <arijit@effectiveailabs.com>
>
> When resolving a delta whose result has children of its own,
> index-pack adds the result to work_head, accounts its data in
> base_cache_used, and calls prune_base_data(). It then immediately
> frees that same data.
>
> This bypasses the existing delta base cache policy and can force later
> descendants to reconstruct the queued base again. Let the existing
> delta_base_cache_limit pruning policy decide whether to keep or evict
> the data instead.
>
> Signed-off-by: Arijit Banerjee <arijit@effectiveailabs.com>
> ---
> index-pack: retain child bases in delta cache
>
> Speed up the local pack indexing phase of clone/fetch for large
> delta-compressed packs by keeping reconstructed delta bases available
> for reuse when they are queued for later delta resolution.
>
> When index-pack reconstructs a child base and queues it for resolving
> descendant deltas, it currently frees that data immediately. This can
> force the same base to be reconstructed again. Instead, keep it in the
> existing delta base cache and let the existing delta_base_cache_limit
> policy decide whether to retain or evict it.
>
> This does not add a new cache or increase the cache limit. The object
> data is already accounted in base_cache_used, and prune_base_data() is
> already called at this point.
>
> Correctness:
>
> * t/t5302-pack-index.sh passed all 36 tests.
Is there any chance that you ran this also with SANITIZE=leak to make
sure that we aren't introducing a memory leak? (It's hard to tell just
from the patch context, though your description is convincing.)
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2131%2Farijit91%2Findex-pack-retain-child-base-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2131/arijit91/index-pack-retain-child-base-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2131
Indeed, this PR has a passing linux-leaks build that exercises this
test script. [1]
[1] https://github.com/gitgitgadget/git/actions/runs/26605615549/job/78399938323?pr=2131#step:9:1405
> Benchmarks on a quiet Ubuntu 24.04 VM, 16 vCPU, 32 GiB RAM, local SSD:
>
> pack baseline patched wall-time change RSS change linux blobless 69.17s
> 57.98s 16.2% faster -0.0% linux full 280.72s 236.32s 15.8% faster +1.9%
>
> Five-repeat public-repo medians also improved: git.git 13.1%, libgit2
> 14.0%, redis 13.5%, cpython 4.8%.
>
> Perf on the linux blobless pack showed the same direction under
> profiling: 76.64s baseline vs 61.09s patched, with similar RSS.
A lot of this information that is in your cover letter would be helpful
to include in your commit message, for posterity.
Also, I prefer to see performance numbers for these repos reflected in
results from our performance test suite. We have a test for this purpose,
so you could try running this from t/perf/ for your local copies of these
repos:
GIT_PERF_LARGE_REPO=<path> ./run HEAD~1 HEAD -- p5302-pack-index.sh
And this should result in a standard comparison table that will help
present your results in a way that is familiar to Git contributors.
> @@ -1212,7 +1212,6 @@ static void *threaded_second_pass(void *data)
> list_add(&child->list, &work_head);
> base_cache_used += child->size;
> prune_base_data(NULL);
> - free_base_data(child);
> } else if (child) {
A nice and simple change. Good find!
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
From: Lorenzo Pegorari @ 2026-06-01 13:27 UTC (permalink / raw)
To: Jeff King; +Cc: git, Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox
In-Reply-To: <20260529053659.GC1099450@coredump.intra.peff.net>
On Fri, May 29, 2026 at 01:36:59AM -0400, Jeff King wrote:
> On Fri, May 29, 2026 at 01:49:44AM +0200, LorenzoPegorari wrote:
>
> > Inside the function `fetch_and_setup_pack_index()`, when the pack
> > obtained using `parse_pack_index()` fails to be verified by
> > `verify_pack_index()`, the function returns without closing and freeing
> > said pack.
> >
> > Fix this by calling `close_pack_index()` to munmap the index file for
> > the leaking pack (which might have been mmapped by `fetch_pack_index()`
> > or `verify_pack_index()`), and then free it, when the verification
> > fails.
> >
> > Also, do some more cleanup by removing the useless call to the function
> > `unlink()`. This is not necessary anymore since 63aca3f7f1 (dumb-http:
> > store downloaded pack idx as tempfile, 2024-10-25), when
> > `fetch_pack_index()` started registering its return value (in this case
> > `tmp_idx`) as a tempfile to be deleted at process exit.
>
> I think the patch as-is is OK. But when I see this kind of "also, do
> this..." in a commit message it is a good time to consider whether that
> should happen in a separate patch.
>
> Here it does not make sense to remove the unlink() afterwards; you'd
> wonder why it was not present in the cleanup added by your patch.
>
> But it _could_ be done as a preparatory patch. And the rationale for
> doing that on its own I think is roughly:
>
> 1. It is mostly doing nothing, because 63aca3f7f1 registered it as a
> tempfile, so it will be cleaned up at process end anyway (whether
> we succeed in fetching it or not).
>
> 2. It is maybe a little harmful, because we are going to unlink() it
> now, and then later the tempfile code will try to unlink() it again
> (so a simultaneous fetch could have created the same file).
>
> For something this small, though, I am OK just lumping it together.
> There are diminishing returns from polishing it further.
Yeah, this makes sense. I will separate it in 2 different patches.
> -Peff
Thanks,
Lorenzo
^ permalink raw reply
* Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
From: Lorenzo Pegorari @ 2026-06-01 13:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox
In-Reply-To: <20260529054024.GA1104383@coredump.intra.peff.net>
On Fri, May 29, 2026 at 01:40:24AM -0400, Jeff King wrote:
> On Fri, May 29, 2026 at 01:36:59AM -0400, Jeff King wrote:
>
> > But it _could_ be done as a preparatory patch. And the rationale for
> > doing that on its own I think is roughly:
> >
> > 1. It is mostly doing nothing, because 63aca3f7f1 registered it as a
> > tempfile, so it will be cleaned up at process end anyway (whether
> > we succeed in fetching it or not).
> >
> > 2. It is maybe a little harmful, because we are going to unlink() it
> > now, and then later the tempfile code will try to unlink() it again
> > (so a simultaneous fetch could have created the same file).
>
> BTW, for (2) I wondered about going in the opposite direction. If we
> actually passed the tempfile back up, like in the patch below, then we
> could use delete_tempfile() to do the unlink (and remove it from the
> tempfile list).
>
> And then your patch would want to similarly delete_tempfile() in its
> error path.
>
> But I don't think it really buys us much. _If_ we were going to keep
> passing the tempfile struct up the call stack on success, then we could
> store it and call delete_tempfile() as soon as we had ran index-pack on
> it. But that's even more surgery, for again little gain (we delete our
> tempfiles a little earlier, rather than at process end).
>
> So I'm inclined to go in the direction that shortens the code. ;)
>
> -Peff
>
> ---
> diff --git a/http.c b/http.c
> index ea9b16861b..e83a3857b3 100644
> --- a/http.c
> +++ b/http.c
> @@ -2546,9 +2546,10 @@ int http_fetch_ref(const char *base, struct ref *ref)
> }
>
> /* Helpers for fetching packs */
> -static char *fetch_pack_index(unsigned char *hash, const char *base_url)
> +static struct tempfile *fetch_pack_index(unsigned char *hash, const char *base_url)
> {
> char *url, *tmp;
> + struct tempfile *ret;
> struct strbuf buf = STRBUF_INIT;
>
> if (http_is_verbose)
> @@ -2575,23 +2576,24 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)
> tmp = xstrfmt("%s/tmp_pack_%s.idx",
> repo_get_object_directory(the_repository),
> hash_to_hex(hash));
> - register_tempfile(tmp);
> + ret = register_tempfile(tmp);
> + free(tmp);
>
> - if (http_get_file(url, tmp, NULL) != HTTP_OK) {
> + if (http_get_file(url, ret->filename.buf, NULL) != HTTP_OK) {
> error("Unable to get pack index %s", url);
> - FREE_AND_NULL(tmp);
> + delete_tempfile(&ret);
> }
>
> free(url);
> - return tmp;
> + return ret;
> }
>
> static int fetch_and_setup_pack_index(struct packfile_list *packs,
> unsigned char *sha1,
> const char *base_url)
> {
> struct packed_git *new_pack, *p;
> - char *tmp_idx = NULL;
> + struct tempfile *tmp_idx;
> int ret;
>
> /*
> @@ -2607,11 +2609,9 @@ static int fetch_and_setup_pack_index(struct packfile_list *packs,
> if (!tmp_idx)
> return -1;
>
> - new_pack = parse_pack_index(the_repository, sha1, tmp_idx);
> + new_pack = parse_pack_index(the_repository, sha1, tmp_idx->filename.buf);
> if (!new_pack) {
> - unlink(tmp_idx);
> - free(tmp_idx);
> -
> + delete_tempfile(&tmp_idx);
> return -1; /* parse_pack_index() already issued error message */
> }
Yeah, I also explored the possibility (as you suggested in your first
reply to v1) of manually deleting the tempfile. In my opinion, this
isn't worth the effort, and it's complicating the code for no reason, so
in the end I opted for keeping it as simple and minimal as possible.
Thanks,
Lorenzo
^ permalink raw reply
* Re: [PATCH 2/2] builtin/init-db: deprecate alias for git-init(1)
From: Phillip Wood @ 2026-06-01 13:48 UTC (permalink / raw)
To: Patrick Steinhardt, Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <ah12uk7IFxS92OR1@pks.im>
On 01/06/2026 13:10, Patrick Steinhardt wrote:
> On Mon, Jun 01, 2026 at 11:31:46AM +0200, Kristoffer Haugsbakk wrote:
>> On Mon, Jun 1, 2026, at 09:56, Patrick Steinhardt wrote:
>>> diff --git a/git.c b/git.c
>>> index a72394b599..6bf6a60360 100644
>>> --- a/git.c
>>> +++ b/git.c
>>> @@ -591,7 +591,9 @@ static struct cmd_struct commands[] = {
>>> { "hook", cmd_hook, RUN_SETUP_GENTLY },
>>> { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT },
>>> { "init", cmd_init },
>>> +#ifndef WITH_BREAKING_CHANGES
>>> { "init-db", cmd_init },
>>
>> This can be marked as deprecated.
>>
>> { "init-db", cmd_init, DEPRECATED },
>
> Ah, indeed! Added locally now, thanks.
Deprecating this command seems very sensible to me. As well as marking
it deprecated, do we want to print a warning when it is run? I imagine
anyone who has this command in their muscle memory is unlikely to be
reading the man page on a regular basis so wont see the warning there.
Thanks
Phillip
> Patrick
>
^ permalink raw reply
* [PATCH v3 0/2] http: fix memory leak in fetch_and_setup_pack_index()
From: LorenzoPegorari @ 2026-06-01 13:51 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox, Jeff King
In-Reply-To: <ahjUmMCKxREamQE-@lorenzo-VM>
Patch series that does some cleanup and fixes a memory leak present
inside the function `fetch_and_setup_pack_index()`.
LorenzoPegorari (2):
http: cleanup function fetch_and_setup_pack_index()
http: fix memory leak in fetch_and_setup_pack_index()
http.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
--
2.54.0.129.g2dffd77b94.dirty
^ permalink raw reply
* [PATCH v3 1/2] http: cleanup function fetch_and_setup_pack_index()
From: LorenzoPegorari @ 2026-06-01 13:52 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox, Jeff King
In-Reply-To: <cover.1780321770.git.lorenzo.pegorari2002@gmail.com>
Cleanup the function `fetch_and_setup_pack_index()` by removing the
useless call to the function `unlink()`.
This is not necessary anymore since 63aca3f7f1 (dumb-http: store
downloaded pack idx as tempfile, 2024-10-25), when `fetch_pack_index()`
started registering its return value (in this case `tmp_idx`) as a
tempfile to be deleted at process exit.
Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
---
http.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/http.c b/http.c
index 67c9c6fc60..b8443b1ef4 100644
--- a/http.c
+++ b/http.c
@@ -2538,9 +2538,7 @@ static int fetch_and_setup_pack_index(struct packfile_list *packs,
new_pack = parse_pack_index(the_repository, sha1, tmp_idx);
if (!new_pack) {
- unlink(tmp_idx);
free(tmp_idx);
-
return -1; /* parse_pack_index() already issued error message */
}
--
2.54.0.129.g2dffd77b94.dirty
^ permalink raw reply related
* [PATCH v3 2/2] http: fix memory leak in fetch_and_setup_pack_index()
From: LorenzoPegorari @ 2026-06-01 13:52 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox, Jeff King
In-Reply-To: <cover.1780321770.git.lorenzo.pegorari2002@gmail.com>
Inside the function `fetch_and_setup_pack_index()`, when the pack
obtained using `parse_pack_index()` fails to be verified by
`verify_pack_index()`, the function returns without closing and freeing
said pack.
Fix this by calling `close_pack_index()` to munmap the index file for
the leaking pack (which might have been mmapped by `fetch_pack_index()`
or `verify_pack_index()`), and then free it, when the verification
fails.
Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
---
http.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/http.c b/http.c
index b8443b1ef4..99da4d7529 100644
--- a/http.c
+++ b/http.c
@@ -2543,11 +2543,13 @@ static int fetch_and_setup_pack_index(struct packfile_list *packs,
}
ret = verify_pack_index(new_pack);
- if (!ret)
- close_pack_index(new_pack);
+
+ close_pack_index(new_pack);
free(tmp_idx);
- if (ret)
+ if (ret) {
+ free(new_pack);
return -1;
+ }
packfile_list_prepend(packs, new_pack);
return 0;
--
2.54.0.129.g2dffd77b94.dirty
^ permalink raw reply related
* Re: [PATCH 1/2] t3404: add failing branch symref test
From: Phillip Wood @ 2026-06-01 13:52 UTC (permalink / raw)
To: Son Luong Ngoc via GitGitGadget, git; +Cc: Son Luong Ngoc
In-Reply-To: <a550923440a233daea0b9819e05d6c380de00d09.1779946921.git.gitgitgadget@gmail.com>
On 28/05/2026 06:42, Son Luong Ngoc via GitGitGadget wrote:
> From: Son Luong Ngoc <sluongng@gmail.com>
>
> rebase --update-refs queues local branch decorations by their literal
> refnames. When a branch such as refs/heads/main is a symbolic ref to
> the current branch, the normal rebase path first updates the current
> branch and the queued symref update later tries to update the same
> referent with the old value it recorded before the rebase.
>
> Add a known-breakage test that exercises this case so that the fix can
> flip it to test_expect_success. The expected behavior is that the branch
> symref keeps pointing at the rebased current branch.
Thanks for adding a test, I'd find it easier to review this series if
the test was added in the same patch as the fix which is our usual practice.
> +test_expect_failure '--update-refs skips branch symrefs to current branch' '
> + test_when_finished "
> + test_might_fail git rebase --abort &&
> + git checkout primary &&
> + test_might_fail git symbolic-ref -d refs/heads/update-refs-symref-alias &&
> + test_might_fail git branch -D update-refs-symref update-refs-symref-base
> + " &&
> + git checkout -B update-refs-symref-base primary &&
> + test_commit --no-tag update-refs-symref-base symref-base.t &&
> + git checkout -B update-refs-symref &&
> + test_commit --no-tag update-refs-symref-topic symref-topic.t &&
> + git checkout update-refs-symref-base &&
> + test_commit --no-tag update-refs-symref-newbase symref-newbase.t &&
> + git checkout update-refs-symref &&
> + git symbolic-ref refs/heads/update-refs-symref-alias refs/heads/update-refs-symref &&
I think we want to test a symref that does not match HEAD as well.
Rather than adding a new test, can we instead add a couple of symref
branches to the test "--update-refs updates refs correctly"?
Thanks
Phillip
> +
> + git rebase --update-refs update-refs-symref-base 2>err &&
> +
> + test_cmp_rev update-refs-symref-base update-refs-symref^ &&
> + test_cmp_rev refs/heads/update-refs-symref refs/heads/update-refs-symref-alias &&
> + test_write_lines refs/heads/update-refs-symref >expect &&
> + git symbolic-ref refs/heads/update-refs-symref-alias >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success '--update-refs updates refs correctly' '
> git checkout -B update-refs no-conflict-branch &&
> git branch -f base HEAD~4 &&
^ permalink raw reply
* Re: [PATCH 0/5] Duplicate entry hardening
From: Patrick Steinhardt @ 2026-06-01 13:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren via GitGitGadget, git, Elijah Newren
In-Reply-To: <xmqqpl2a4f09.fsf@gitster.g>
On Mon, Jun 01, 2026 at 09:33:10PM +0900, Junio C Hamano wrote:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > We had some corrupt trees with duplicate entries in real world repositories,
> > which triggered an assertion failure in merge-ort. Further, the corrupt tree
> > creation in the third party tool would have been avoided had verify_cache()
> > correctly checked for D/F conflicts. Provide fixes for both issues,
> > including 3 preparatory changes for the merge-ort fix.
> >
> > Elijah Newren (5):
> > merge-ort: propagate callback errors from traverse_trees_wrapper()
> > merge-ort: drop unnecessary show_all_errors from collect_merge_info()
> > merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()
> > merge-ort: abort merge when trees have duplicate entries
> > cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts
>
> This is a fix to an important corner of our system, but somehow left
> in "Needs review" state for much longer than I would have liked, so
> even though I am officially on vacation ;-), I took some time to
> read these through (by the way it was a pleasant read, thank you).
Honestly, I always shy away from the merge-related subsystems. It has a
lot of subtleties that I don't have any experience with, so I never
really consider my input to be helpful here.
> I wonder if we create a rule like
>
> Those of you who have more than 30 commits in our project are
> expected to review one topic (or more) from other contributors
> for every three patches you send and ask for reviews by others.
Heh, that would make me condense patch series into fewer patches ;)
> it would help balance the patch vs review ratio, perhaps?
It's a good question. I typically try to aim for reviewing series on the
mailing list at least every second day, and I always encourage other
folks in my team to do the same. But recently I (well, rather we)
haven't really been able to due to the current situation at GitLab,
which forces us to put almost all of our focus towards a different
project for a while.
Overall I agree that everyone who is a core contributor should also make
reviews part of their regular worflow. At least for corporate
contributors that might also make it easier to communicate this to their
respective employers. Regardless of that, my expectation is that there
will be times where it works well, and other times where it works less
well.
Patrick
^ permalink raw reply
* Re: [PATCH v3 1/8] environment: move "trust_ctime" into `struct repo_config_values`
From: Bello Olamide @ 2026-06-01 14:01 UTC (permalink / raw)
To: Tian Yuchen
Cc: git, Phillip Wood, Junio C Hamano, Christain Couder,
Usman Akinyemi, Kaartic Sivaraam, Taylor Blau
In-Reply-To: <08efcc49-0db8-49f6-8971-633aa55eb66c@malon.dev>
On Thu, May 21, 2026, 5:37 PM Tian Yuchen <cat@malon.dev> wrote:
>
> Hi Bello!
>
> On 4/24/26 00:54, Olamide Caleb Bello wrote:
>
> The code itself looks great to me, but I have some reservations about
> the description here (in terms of why trust_ctime is eagerly parsed):
>
> > `core.trustctime` is parsed eagerly
> > because it is used in low‑level stat‑matching functions
> > (`match_stat_data()`), where a lazy parse could cause unexpected
> > fatal errors and complicate libification efforts.
>
> It's true that if we use repo_config_get_bool() to parse trust_ctime,
> following the call stack downwards, there is a die() call. The terminate
> condition is that the configuration does not exist or contains invalid
> characters.
>
> But I think there is another factor: match_stat_data() is called on a
> hot path. The following code is implemented in read-cache.c,
> refresh_index() function:
>
> for (i = 0; i < istate->cache_nr; i++) {
> ...
> new_entry = refresh_cache_ent(istate, ce, options,
> &cache_errno, &changed,
> &t2_did_lstat, &t2_did_scan);
> t2_sum_lstat += t2_did_lstat;
> t2_sum_scan += t2_did_scan;
> if (new_entry == ce)
> ...
>
> The call chain: refresh_index() -> refresh_cache_ent() ->
> ie_match_stat() -> ce_match_stat_basic() -> *match_stat_data()*
>
> Therefore, if the variable is lazily parsed, this means there will be a
> performance regression whenever the index status needs to be checked,
> e.g. 'git status'.
>
> So, I guess it would be better to extend a bit:
>
> '...where a lazy parse could cause unexpected fatal, and result in a
> performance regression...'
noted...
>
> Thanks, yuchen
Thank you, Yuchen.
^ permalink raw reply
* Re: [PATCH 2/2] rebase: skip branch symref aliases
From: Phillip Wood @ 2026-06-01 14:10 UTC (permalink / raw)
To: Son Luong Ngoc via GitGitGadget, git; +Cc: Son Luong Ngoc
In-Reply-To: <0ab0a717441e9fc7c494da194065a948a35a7f01.1779946921.git.gitgitgadget@gmail.com>
On 28/05/2026 06:42, Son Luong Ngoc via GitGitGadget wrote:
> From: Son Luong Ngoc <sluongng@gmail.com>
>
> rebase --update-refs records local branch decorations before replaying
> commits. If a decoration is a symbolic branch such as refs/heads/main
> pointing at refs/heads/master, updating it later dereferences back to
> master and can fail because the normal rebase path already moved that
> branch.
Good explanation, thanks for working on this.
> Resolve local branch symref decorations to their referents before
s/referents/targets/ ?
> queuing update-ref commands, and skip duplicates. This keeps branch
> aliases from scheduling a second update for the same underlying branch
> while still using the existing old-OID check for the single queued
> update.
That's not quite what the patch does though - it only checks that the
target of the symref differs from the target of HEAD. If a symref points
to another branch we still try to update it when we should skip it.
> Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
> ---
> sequencer.c | 63 +++++++++++++++++++++++++++++------
> t/t3404-rebase-interactive.sh | 2 +-
> 2 files changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 1ee4b2875b..4a83d1337c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6445,15 +6445,22 @@ static int add_decorations_to_list(const struct commit *commit,
> struct todo_add_branch_context *ctx)
> {
> const struct name_decoration *decoration = get_name_decoration(&commit->object);
> - const char *head_ref = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> - "HEAD",
> + struct ref_store *refs = get_main_ref_store(the_repository);
> + const char *head_ref = refs_resolve_ref_unsafe(refs, "HEAD",
> RESOLVE_REF_READING,
> - NULL,
> - NULL);
> + NULL, NULL);
> + char *resolved_head_ref = refs_resolve_refdup(refs, "HEAD",
> + RESOLVE_REF_READING,
> + NULL, NULL);
We need to use refs_resolve_refdup() instead of
refs_resolve_ref_unsafe() so that the return value is not overwritten by
the later calls to refs_resolve_ref_unsafe() that are added below. But
that is the only change that is needed - we do not need to add a new
variable, we just replace refs_resolve_ref_unsafe() with
refs_resole_refdup() and free "head_ref" before we return.
> + struct strbuf update_ref = STRBUF_INIT;
>
> while (decoration) {
> struct todo_item *item;
> const char *path;
> + const char *ref = decoration->name;
> + const char *resolved_ref;
> + int is_symref = 0;
> + int flags = 0;
> size_t base_offset = ctx->buf->len;
>
> /*
> @@ -6461,12 +6468,44 @@ static int add_decorations_to_list(const struct commit *commit,
> * updated by the default rebase behavior.
> * Exclude it from the list of refs to update,
> * as well as any non-branch decorations.
> + *
> + * Resolve branch symrefs after checking for the current HEAD so
> + * that aliases do not schedule duplicate updates for their
> + * referents.
> + *
> * Non-branch decorations may be present if the pretty format
> * includes "%d", which would have loaded all refs
> * into the global decoration table.
> */
> - if ((head_ref && !strcmp(head_ref, decoration->name)) ||
> - (decoration->type != DECORATION_REF_LOCAL)) {
> + if (decoration->type != DECORATION_REF_LOCAL) {
> + decoration = decoration->next;
> + continue;
> + }
> +
> + if (head_ref && !strcmp(head_ref, ref)) {
> + decoration = decoration->next;
> + continue;
> + }
This is just rewriting the existing if statement which has nothing to do
with the stated aim of this patch - lets leave it as it was.
> +
> + strbuf_reset(&update_ref);
> + resolved_ref = refs_resolve_ref_unsafe(refs, ref,
> + RESOLVE_REF_READING |
> + RESOLVE_REF_NO_RECURSE,
Why are we passing RESOLVE_REF_NO_RECURSE here? I'd have thought we want
to resolve the whole chain of symbolic refs to find out which ref is
actually going to be updated.
> + NULL, &flags);
> + if ((flags & REF_ISSYMREF) && resolved_ref) {
I think it is generally safer to check the return value before using any
of the "out" parameters from a function call. In this case the function
unconditionally clears flags at the beginning so it is safe.
> + if (!starts_with(resolved_ref, "refs/heads/")) {
> + decoration = decoration->next;
> + continue;
This is the opposite of what I was expecting - if the decoration is a
symref that resolves to a branch then that branch will also be in the
list of decorations and so will be updated. If the decoration is a
symref that resolves outside "refs/heads/" then we want to add the
decoration to the list of refs to update to keep the current behavior.
If we do that then we skip all symbolic refs that point to another
branch, instead of just skipping those that match HEAD and we don't need
any of the changes below here.
Thanks
Phillip
> + }
> +
> + strbuf_addstr(&update_ref, resolved_ref);
> + ref = update_ref.buf;
> + is_symref = 1;
> + }
> +
> + if ((is_symref && resolved_head_ref &&
> + !strcmp(resolved_head_ref, ref)) ||
> + string_list_has_string(&ctx->refs_to_oids, ref)) {
> decoration = decoration->next;
> continue;
> }
> @@ -6478,19 +6517,19 @@ static int add_decorations_to_list(const struct commit *commit,
> memset(item, 0, sizeof(*item));
>
> /* If the branch is checked out, then leave a comment instead. */
> - if ((path = branch_checked_out(decoration->name))) {
> + if ((path = branch_checked_out(ref))) {
> item->command = TODO_COMMENT;
> strbuf_commented_addf(ctx->buf, comment_line_str,
> "Ref %s checked out at '%s'\n",
> - decoration->name, path);
> + ref, path);
> } else {
> struct string_list_item *sti;
> item->command = TODO_UPDATE_REF;
> - strbuf_addf(ctx->buf, "%s\n", decoration->name);
> + strbuf_addf(ctx->buf, "%s\n", ref);
>
> sti = string_list_insert(&ctx->refs_to_oids,
> - decoration->name);
> - sti->util = init_update_ref_record(decoration->name);
> + ref);
> + sti->util = init_update_ref_record(ref);
> }
>
> item->offset_in_buf = base_offset;
> @@ -6501,6 +6540,8 @@ static int add_decorations_to_list(const struct commit *commit,
> decoration = decoration->next;
> }
>
> + strbuf_release(&update_ref);
> + free(resolved_head_ref);
> return 0;
> }
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 42ba8cc313..29447c0fc3 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1978,7 +1978,7 @@ test_expect_success '--update-refs ignores non-branch decorations' '
> test_cmp expect actual
> '
>
> -test_expect_failure '--update-refs skips branch symrefs to current branch' '
> +test_expect_success '--update-refs skips branch symrefs to current branch' '
> test_when_finished "
> test_might_fail git rebase --abort &&
> git checkout primary &&
^ permalink raw reply
* Re: [PATCH 2/2] builtin/init-db: deprecate alias for git-init(1)
From: Patrick Steinhardt @ 2026-06-01 14:20 UTC (permalink / raw)
To: phillip.wood; +Cc: Kristoffer Haugsbakk, git
In-Reply-To: <042e66b5-122b-4c86-a9a9-f75f763666a7@gmail.com>
On Mon, Jun 01, 2026 at 02:48:05PM +0100, Phillip Wood wrote:
>
>
> On 01/06/2026 13:10, Patrick Steinhardt wrote:
> > On Mon, Jun 01, 2026 at 11:31:46AM +0200, Kristoffer Haugsbakk wrote:
> > > On Mon, Jun 1, 2026, at 09:56, Patrick Steinhardt wrote:
> > > > diff --git a/git.c b/git.c
> > > > index a72394b599..6bf6a60360 100644
> > > > --- a/git.c
> > > > +++ b/git.c
> > > > @@ -591,7 +591,9 @@ static struct cmd_struct commands[] = {
> > > > { "hook", cmd_hook, RUN_SETUP_GENTLY },
> > > > { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT },
> > > > { "init", cmd_init },
> > > > +#ifndef WITH_BREAKING_CHANGES
> > > > { "init-db", cmd_init },
> > >
> > > This can be marked as deprecated.
> > >
> > > { "init-db", cmd_init, DEPRECATED },
> >
> > Ah, indeed! Added locally now, thanks.
>
> Deprecating this command seems very sensible to me. As well as marking it
> deprecated, do we want to print a warning when it is run? I imagine anyone
> who has this command in their muscle memory is unlikely to be reading the
> man page on a regular basis so wont see the warning there.
I was wondering whether we want to call `you_still_use_that()` here. I
found it to be a bit heavy-handed as it's so trivial to replace with
git-init(1), but on the other hand it's a trivial thing to do.
Patrick
^ permalink raw reply
* Re: [PATCH v2 2/2] status: improve rebase todo list parsing
From: Phillip Wood @ 2026-06-01 15:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, Patrick Steinhardt
In-Reply-To: <xmqqbjdwcsno.fsf@gitster.g>
Hi Junio
On 31/05/2026 01:46, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> +static void abbrev_oid_in_line(struct repository *r,
>> + struct strbuf *line, char **pp)
>> +{
>> ...
>> + have_oid = !repo_get_oid(r, p, &oid);
>> + *end_of_object_name = saved;
>> + if (!have_oid)
>> + goto out; /* object name was a label */
>
> Can there be a label "deadbeef123" that is unrelated to an object whose
> object name happens to abbreviate to "deadbeef123"?
In theory yes, but I had assumed it was so unlikely to happen that we
could ignore it. If we want to be more careful then we could add a "bool
maybe_label" argument for commands that accept a label or a revision and
check if "refs/rewritten/$object_name" exists before trying repo_get_oid().
>> + case TODO_MERGE:
>> + skip_dash_c(&p);
>> + while (true) {
>> + p += strspn(p, " \t");
>> + if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
>> + break;
>> + abbrev_oid_in_line(r, line, &p);
>> + }
>> + break;
>
> What does this loop do? A "merge" command may look like "merge
> [[-C|-c] <commit>] <label>", and we give each whitespace-separated
> token to abbrev_oid_in_line()? Would "<label>" that is ambiguous
> cause an issue? You may want to limit the scope of what the loop
> does a bit, e.g., massage only the token after -C/-c, or something?
The parents can be a label or any revision so we want to abbreviate the
parent if it is a hex object id. The same is true for "reset" below.
Thanks
Phillip
>
>> + case TODO_FIXUP:
>> + skip_dash_c(&p);
>> + /* fallthrough */
>> + case TODO_DROP:
>> + case TODO_EDIT:
>> + case TODO_PICK:
>> + case TODO_RESET:
>
> Doesn't RESET also take a <label>? And if it happens to be the same
> as an abbreviated object name, e.g., "deadbeef123", of an unrelated
> object, would wt-status say "reset deadbeef1", causing a mismatch?
> If this is indeed an issue, would moving this to the "no-op" section
> below, next to TODO_LABEL, solve it?
>
>> + case TODO_REVERT:
>> + case TODO_REWORD:
>> + case TODO_SQUASH:
>> + abbrev_oid_in_line(r, line, &p);
>> + break;
>> +
>> + /*
>> + * Avoid "default" and instead list all the other commands so
>> + * that -Wswitch (which is included in -Wall) warns if a new
>> + * command is added without handling it in this function.
>> + */
>> + case TODO_BREAK:
>> + case TODO_EXEC:
>> + case TODO_LABEL:
>> + case TODO_NOOP:
>> + case TODO_UPDATE_REF:
>> + break;
>> }
>> - string_list_clear(&split, 0);
>> +
>> + return true;
>> }
>
^ permalink raw reply
* [GSoC][PATCH 0/4] teach git repo info to handle path keys
From: K Jayatheerth @ 2026-06-01 15:19 UTC (permalink / raw)
To: git
Cc: jltobler, lucasseikioshiro, gitster, phillip.wood, sandals,
kumarayushjha123, a3205153416, K Jayatheerth
Hi!
The first and second patches are self-explanatory, so I will
focus more on the third and fourth patches, which introduce the
path-related fields to `git repo info`.
In the last discussion [1] we had on the mailing list about paths
in repo info, we didn't reach a definitive conclusion, but
adding both options made the most sense based on the feedback.
So in patches 3 and 4, we add both `path.<field>.absolute` and
`path.<field>.relative` for `gitdir` and `commondir`. Initially,
it was proposed by Ayush to use `path.absolute.<field>`, but
this would break the lexicographical order of the internal field
array. I tweaked it to place the variant at the end as a suffix instead.
There are still a few open questions that should be addressed
by the community. I am tagging members who were involved in the
previous discussions:
Justin Tobler, Lucas Seiki Oshiro, Junio, Phillip Wood,
brian m. carlson, and Ayush Jha.
Apologies if I missed anyone; I included everyone who reviewed
or participated in the discussions of Eslam's and Lucas's
patches.
Questions:
1. Should there still be a --path-format flag?
2. Should we consider a default option?
Currently we have path.gitdir.absolute; should we consider
an option where a plain path.gitdir returns some default?
If yes:
2.1 Should we keep the default the same as rev-parse? Or
should either relative or absolute be the default?
2.2 When printing using --all, should the default be
printed, or should we print both absolute and
relative?
3. Is printing both absolute and relative in a single call
using --all acceptable?
If no:
3.1 What's a better approach?
I have discussed these changes with both Justin and Lucas
internally. This series is presented to gather opinions from the
wider community before moving forward.
K Jayatheerth (4):
path: add strbuf_add_path for formatting paths
rev-parse: use strbuf_add_path for path formatting
repo: add path.gitdir with absolute and relative suffix formatting
repo: add path.commondir with absolute and relative suffix formatting
Documentation/git-repo.adoc | 15 ++++++
builtin/repo.c | 50 ++++++++++++++++++
builtin/rev-parse.c | 100 ++++++++----------------------------
path.c | 58 +++++++++++++++++++++
path.h | 16 ++++++
t/t1900-repo-info.sh | 32 ++++++++++++
6 files changed, 192 insertions(+), 79 deletions(-)
--
2.54.0
^ permalink raw reply
* [GSoC][PATCH 1/4] path: add strbuf_add_path for formatting paths
From: K Jayatheerth @ 2026-06-01 15:19 UTC (permalink / raw)
To: git
Cc: jltobler, lucasseikioshiro, gitster, phillip.wood, sandals,
kumarayushjha123, a3205153416, K Jayatheerth
In-Reply-To: <20260601151950.30686-1-jayatheerthkulkarni2005@gmail.com>
The `print_path()` function in `builtin/rev-parse.c` contains
logic for formatting paths as either absolute or relative based on user
preferences and default behaviors. However, this logic is currently
locked inside `rev-parse` and writes directly to stdout using `puts()`.
To allow other builtins (such as the new `git repo` command) to utilize
this same path-formatting logic, extract the core algorithm into a new
string-builder function, `strbuf_add_path()`, in `path.c`.
Additionally, extract the associated enums (`format_type` and
`default_type`), and prefix them with `path_` (e.g., `path_format_type`)
to safely expose them in `path.h` without polluting the global namespace.
Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
Mentored-by: Justin Tobler <jltobler@gmail.com>
Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
---
path.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
path.h | 16 ++++++++++++++++
2 files changed, 74 insertions(+)
diff --git a/path.c b/path.c
index d7e17bf174..914812320f 100644
--- a/path.c
+++ b/path.c
@@ -1579,6 +1579,64 @@ char *xdg_cache_home(const char *filename)
return NULL;
}
+void strbuf_add_path(struct strbuf *sb, const char *path, const char *prefix,
+ enum path_format_type format, enum path_default_type def)
+{
+ char *cwd = NULL;
+
+ /*
+ * We don't ever produce a relative path if prefix is NULL, so set the
+ * prefix to the current directory so that we can produce a relative
+ * path whenever possible. If we're using RELATIVE_IF_SHARED mode, then
+ * we want an absolute path unless the two share a common prefix, so don't
+ * set it in that case, since doing so causes a relative path to always
+ * be produced if possible.
+ */
+ if (!prefix && (format != PATH_FORMAT_DEFAULT || def != PATH_DEFAULT_RELATIVE_IF_SHARED))
+ prefix = cwd = xgetcwd();
+
+ if (format == PATH_FORMAT_DEFAULT && def == PATH_DEFAULT_UNMODIFIED) {
+ /* Case 1: Return the path exactly as-is without modifications */
+ strbuf_addstr(sb, path);
+ } else if (format == PATH_FORMAT_RELATIVE ||
+ (format == PATH_FORMAT_DEFAULT && def == PATH_DEFAULT_RELATIVE)) {
+ /*
+ * Case 2: Explicitly or implicitly relative.
+ * inside relative_path(), both targets must be absolute paths
+ * to compute a reliable relative tracking offset.
+ */
+ struct strbuf buf = STRBUF_INIT, realbuf = STRBUF_INIT, prefixbuf = STRBUF_INIT;
+
+ if (!is_absolute_path(path)) {
+ strbuf_realpath_forgiving(&realbuf, path, 1);
+ path = realbuf.buf;
+ }
+ if (!is_absolute_path(prefix)) {
+ strbuf_realpath_forgiving(&prefixbuf, prefix, 1);
+ prefix = prefixbuf.buf;
+ }
+
+ strbuf_addstr(sb, relative_path(path, prefix, &buf));
+
+ strbuf_release(&buf);
+ strbuf_release(&realbuf);
+ strbuf_release(&prefixbuf);
+ } else if (format == PATH_FORMAT_DEFAULT && def == PATH_DEFAULT_RELATIVE_IF_SHARED) {
+ /* Case 3: Relative format if they share a common root pathway */
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addstr(sb, relative_path(path, prefix, &buf));
+ strbuf_release(&buf);
+ } else {
+ /* Case 4: Forced absolute / canonical format optimization */
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_realpath_forgiving(&buf, path, 1);
+ strbuf_addbuf(sb, &buf);
+ strbuf_release(&buf);
+ }
+
+ free(cwd);
+}
+
REPO_GIT_PATH_FUNC(squash_msg, "SQUASH_MSG")
REPO_GIT_PATH_FUNC(merge_msg, "MERGE_MSG")
REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR")
diff --git a/path.h b/path.h
index 0434ba5e07..b9b626ce4a 100644
--- a/path.h
+++ b/path.h
@@ -262,6 +262,22 @@ enum scld_error safe_create_leading_directories_no_share(char *path);
int safe_create_file_with_leading_directories(struct repository *repo,
const char *path);
+enum path_format_type {
+ PATH_FORMAT_DEFAULT,
+ PATH_FORMAT_RELATIVE,
+ PATH_FORMAT_CANONICAL
+};
+
+enum path_default_type {
+ PATH_DEFAULT_RELATIVE,
+ PATH_DEFAULT_RELATIVE_IF_SHARED,
+ PATH_DEFAULT_CANONICAL,
+ PATH_DEFAULT_UNMODIFIED
+};
+
+void strbuf_add_path(struct strbuf *buf, const char *path, const char *prefix,
+ enum path_format_type format, enum path_default_type def);
+
# ifdef USE_THE_REPOSITORY_VARIABLE
# include "strbuf.h"
# include "repository.h"
--
2.54.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox