* [PATCH 5/7] fetch: refactor do_fetch handling of followRemoteHEAD
From: Matt Hunter @ 2026-06-12 5:55 UTC (permalink / raw)
To: git
In-Reply-To: <20260612055947.1499497-1-m@lfurio.us>
Update enum follow_remote_head_settings to include the value
FOLLOW_REMOTE_UNCONFIGURED as the new zero-initialized value for
followRemoteHEAD. This will allow us to distinguish between the option
being unset vs. explicitly set to 'create', which is ultimately the
system default. The unnecessary indentation is removed.
The do_fetch function is likewise updated to perform its own decision
making to determine the effective followRemoteHEAD mode, falling back to
the system default if necessary. This will enable the next patch to
introduce a user-configurable fallback default option.
Function set_head now accepts this value as an argument rather than only
considering the value defined by the remote.
The use of the 'warn-if-not-$branch' value is awkward in the context of
a global default option, since the branches will differ between
individual remotes. For this reason, it's left out of this scheme and
handling of the no_warn_branch variable is untouched. Since a
remote-specific setting for followRemoteHEAD takes priority, we can
assume that if remote->no_warn_branch is set, then the remote is also
asserting FOLLOW_REMOTE_WARN as the effective operating mode, and it
will be honored by do_fetch.
Signed-off-by: Matt Hunter <m@lfurio.us>
---
builtin/fetch.c | 14 ++++++++++----
remote.h | 14 ++++++++------
2 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9a45e1e7a44d..3cc7efdd83a0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1729,12 +1729,12 @@ static void warn_set_head(const char *remote, const char *head_name,
strbuf_release(&buf_prefix);
}
-static int set_head(const struct ref *remote_refs, struct remote *remote)
+static int set_head(const struct ref *remote_refs, struct remote *remote,
+ int follow_remote_head)
{
int result = 0, create_only, baremirror, was_detached;
struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT,
b_local_head = STRBUF_INIT;
- int follow_remote_head = remote->follow_remote_head;
const char *no_warn_branch = remote->no_warn_branch;
char *head_name = NULL;
struct ref *ref, *matches;
@@ -1901,6 +1901,7 @@ static int do_fetch(struct transport *transport,
struct ref_update_display_info_array display_array = { 0 };
struct strmap rejected_refs = STRMAP_INIT;
int summary_width = 0;
+ int follow_remote_head;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1916,6 +1917,11 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
+ if (transport->remote->follow_remote_head)
+ follow_remote_head = transport->remote->follow_remote_head;
+ else
+ follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
+
if (rs->nr) {
refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes);
} else {
@@ -1924,7 +1930,7 @@ static int do_fetch(struct transport *transport,
if (transport->remote->fetch.nr) {
refspec_ref_prefixes(&transport->remote->fetch,
&transport_ls_refs_options.ref_prefixes);
- if (transport->remote->follow_remote_head != FOLLOW_REMOTE_NEVER)
+ if (follow_remote_head != FOLLOW_REMOTE_NEVER)
do_set_head = 1;
}
if (branch && branch_has_merge_config(branch) &&
@@ -2131,7 +2137,7 @@ static int do_fetch(struct transport *transport,
* Way too many cases where this can go wrong so let's just
* ignore errors and fail silently for now.
*/
- set_head(remote_refs, transport->remote);
+ set_head(remote_refs, transport->remote, follow_remote_head);
}
cleanup:
diff --git a/remote.h b/remote.h
index 54b17e4b028b..72a54d84ad51 100644
--- a/remote.h
+++ b/remote.h
@@ -62,12 +62,14 @@ struct remote_state {
void remote_state_clear(struct remote_state *remote_state);
struct remote_state *remote_state_new(void);
- enum follow_remote_head_settings {
- FOLLOW_REMOTE_NEVER = -1,
- FOLLOW_REMOTE_CREATE = 0,
- FOLLOW_REMOTE_WARN = 1,
- FOLLOW_REMOTE_ALWAYS = 2,
- };
+#define BUILTIN_FOLLOW_REMOTE_HEAD_DFLT FOLLOW_REMOTE_CREATE
+enum follow_remote_head_settings {
+ FOLLOW_REMOTE_UNCONFIGURED = 0,
+ FOLLOW_REMOTE_NEVER,
+ FOLLOW_REMOTE_CREATE,
+ FOLLOW_REMOTE_WARN,
+ FOLLOW_REMOTE_ALWAYS,
+};
struct remote {
struct hashmap_entry ent;
--
2.54.0
^ permalink raw reply related
* [PATCH 6/7] fetch: add configuration option fetch.followRemoteHEAD
From: Matt Hunter @ 2026-06-12 5:55 UTC (permalink / raw)
To: git
In-Reply-To: <20260612055947.1499497-1-m@lfurio.us>
'fetch.followRemoteHEAD' is added as a generic option used by all
remotes for which 'remote.<name>.followRemoteHEAD' is undefined. If
both options are undefined, a builtin default of "create" is in effect,
matching the previous behavior.
As mentioned in the previous patch, 'fetch.followRemoteHEAD' supports
all of the values that its 'remote' counterpart does _except_
warn-if-not-$branch, due to its tighter coupling to individual remote
repositories.
Documentation and advice messages for both of the followRemoteHEAD
options are reworded to better capture the relationship between the two.
The added tests assert feature parity between the two followRemoteHEAD
options, as well as the fact that 'remote.<name>.followRemoteHEAD'
always supersedes this new configurable default.
Signed-off-by: Matt Hunter <m@lfurio.us>
---
Documentation/config/fetch.adoc | 19 ++++++
Documentation/config/remote.adoc | 21 +++----
builtin/fetch.c | 32 ++++++++--
t/t5510-fetch.sh | 105 +++++++++++++++++++++++++++++++
4 files changed, 160 insertions(+), 17 deletions(-)
diff --git a/Documentation/config/fetch.adoc b/Documentation/config/fetch.adoc
index 04ac90912d3a..f7de22a34a54 100644
--- a/Documentation/config/fetch.adoc
+++ b/Documentation/config/fetch.adoc
@@ -126,3 +126,22 @@ the new bundle URI.
The creation token values are chosen by the provider serving the specific
bundle URI. If you modify the URI at `fetch.bundleURI`, then be sure to
remove the value for the `fetch.bundleCreationToken` value before fetching.
+
+`fetch.followRemoteHEAD`::
+ When fetching using a default refspec, this option determines how to handle
+ differences between a fetched remote's `HEAD` and the local
+ `remotes/<name>/HEAD` symbolic-ref. Its value is one of
++
+--
+`create`;;
+ Create `remotes/<name>/HEAD` if a ref exists on the remote, but not locally.
+ An existing symbolic-ref will not be touched. This is the default value.
+`warn`;;
+ Display a warning if the remote advertises a different `HEAD` than what is
+ set locally. Behaves like "create" if the local symbolic-ref doesn't exist.
+`always`;;
+ Silently update `remotes/<name>/HEAD` whenever the remote advertises a new
+ value.
+`never`;;
+ Never create or modify the `remotes/<name>/HEAD` symbolic-ref.
+--
diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
index eb9c8a3c4884..761bf4ba7d14 100644
--- a/Documentation/config/remote.adoc
+++ b/Documentation/config/remote.adoc
@@ -157,15 +157,12 @@ Blank values signal to ignore all previous values, allowing a reset of
the list from broader config scenarios.
remote.<name>.followRemoteHEAD::
- How linkgit:git-fetch[1] should handle updates to `remotes/<name>/HEAD`
- when fetching using the configured refspecs of a remote.
- The default value is "create", which will create `remotes/<name>/HEAD`
- if it exists on the remote, but not locally; this will not touch an
- already existing local reference. Setting it to "warn" will print
- a message if the remote has a different value than the local one;
- in case there is no local reference, it behaves like "create".
- A variant on "warn" is "warn-if-not-$branch", which behaves like
- "warn", but if `HEAD` on the remote is `$branch` it will be silent.
- Setting it to "always" will silently update `remotes/<name>/HEAD` to
- the value on the remote. Finally, setting it to "never" will never
- change or create the local reference.
+ When fetching this remote using its default refspec, this option determines
+ how to handle differences between the remote's `HEAD` and the local
+ `remotes/<name>/HEAD` symbolic-ref. Overrides the setting for
+ `fetch.followRemoteHEAD`. See `fetch.followRemoteHEAD` for a description of
+ accepted values.
++
+In addition to the values supported by `fetch.followRemoteHEAD`, this option may
+also take on the value "warn-if-not-`$branch`", which behaves like "warn", but
+ignores the warning if the remote's `HEAD` is `remotes/<name>/$branch`.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3cc7efdd83a0..a21bb82274d4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -103,6 +103,7 @@ static struct string_list negotiation_include = STRING_LIST_INIT_NODUP;
struct fetch_config {
enum display_format display_format;
+ enum follow_remote_head_settings follow_remote_head;
int all;
int prune;
int prune_tags;
@@ -173,6 +174,22 @@ static int git_fetch_config(const char *k, const char *v,
"fetch.output", v);
}
+ if (!strcmp(k, "fetch.followremotehead")) {
+ if (!v)
+ return config_error_nonbool(k);
+ else if (!strcasecmp(v, "never"))
+ fetch_config->follow_remote_head = FOLLOW_REMOTE_NEVER;
+ else if (!strcasecmp(v, "create"))
+ fetch_config->follow_remote_head = FOLLOW_REMOTE_CREATE;
+ else if (!strcasecmp(v, "warn"))
+ fetch_config->follow_remote_head = FOLLOW_REMOTE_WARN;
+ else if (!strcasecmp(v, "always"))
+ fetch_config->follow_remote_head = FOLLOW_REMOTE_ALWAYS;
+ else
+ die(_("invalid value for '%s': '%s'"),
+ "fetch.followRemoteHEAD", v);
+ }
+
return git_default_config(k, v, ctx, cb);
}
@@ -1697,11 +1714,13 @@ static const char *strip_refshead(const char *name){
static void set_head_advice_msg(const char *remote, const char *head_name)
{
const char message_advice_set_head[] =
- N_("Run 'git remote set-head %s %s' to follow the change, or set\n"
- "'remote.%s.followRemoteHEAD' configuration option to a different value\n"
- "if you do not want to see this message. Specifically running\n"
- "'git config set remote.%s.followRemoteHEAD warn-if-not-%s'\n"
- "will disable the warning until the remote changes HEAD to something else.");
+ N_("Run 'git remote set-head %s %s' to follow the change, or modify\n"
+ "either of the 'remote.%s.followRemoteHEAD' or 'fetch.followRemoteHEAD'\n"
+ "configuration options to handle the situation differently.\n\n"
+
+ "Using this specific option\n\n"
+ " git config set remote.%s.followRemoteHEAD warn-if-not-%s\n\n"
+ "will suppress the warning until the remote changes HEAD to something else.");
advise_if_enabled(ADVICE_FETCH_SET_HEAD_WARN, _(message_advice_set_head),
remote, head_name, remote, remote, head_name);
@@ -1919,6 +1938,8 @@ static int do_fetch(struct transport *transport,
if (transport->remote->follow_remote_head)
follow_remote_head = transport->remote->follow_remote_head;
+ else if (config->follow_remote_head)
+ follow_remote_head = config->follow_remote_head;
else
follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
@@ -2477,6 +2498,7 @@ int cmd_fetch(int argc,
{
struct fetch_config config = {
.display_format = DISPLAY_FORMAT_FULL,
+ .follow_remote_head = FOLLOW_REMOTE_UNCONFIGURED,
.prune = -1,
.prune_tags = -1,
.show_forced_updates = 1,
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 43190630e714..6f0ae1bdd798 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -140,6 +140,16 @@ test_expect_success "fetch test remote HEAD change" '
)
'
+test_expect_success "fetch test default followRemoteHEAD never" '
+ git -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&
+ test_config -C two fetch.followRemoteHEAD "never" &&
+ GIT_TRACE_PACKET=$PWD/trace.out git -C two fetch &&
+ # Confirm that we do not even ask for HEAD when we are
+ # not going to act on it.
+ test_grep ! "ref-prefix HEAD" trace.out &&
+ test_must_fail git -C two rev-parse --verify refs/remotes/origin/HEAD
+'
+
test_expect_success "fetch test followRemoteHEAD never" '
git -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&
test_config -C two remote.origin.followRemoteHEAD "never" &&
@@ -150,6 +160,21 @@ test_expect_success "fetch test followRemoteHEAD never" '
test_must_fail git -C two rev-parse --verify refs/remotes/origin/HEAD
'
+test_expect_success "fetch test default followRemoteHEAD warn no change" '
+ git -C two rev-parse --verify refs/remotes/origin/other &&
+ git -C two remote set-head origin other &&
+ git -C two rev-parse --verify refs/remotes/origin/HEAD &&
+ git -C two rev-parse --verify refs/remotes/origin/main &&
+ test_config -C two fetch.followRemoteHEAD "warn" &&
+ git -C two fetch >output &&
+ echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
+ "but we have ${SQ}other${SQ} locally." >expect &&
+ test_cmp expect output &&
+ head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
+ branch=$(git -C two rev-parse refs/remotes/origin/other) &&
+ test "z$head" = "z$branch"
+'
+
test_expect_success "fetch test followRemoteHEAD warn no change" '
git -C two rev-parse --verify refs/remotes/origin/other &&
git -C two remote set-head origin other &&
@@ -165,6 +190,17 @@ test_expect_success "fetch test followRemoteHEAD warn no change" '
test "z$head" = "z$branch"
'
+test_expect_success "fetch test default followRemoteHEAD warn create" '
+ git -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&
+ test_config -C two fetch.followRemoteHEAD "warn" &&
+ git -C two rev-parse --verify refs/remotes/origin/main &&
+ output=$(git -C two fetch) &&
+ test "z" = "z$output" &&
+ head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
+ branch=$(git -C two rev-parse refs/remotes/origin/main) &&
+ test "z$head" = "z$branch"
+'
+
test_expect_success "fetch test followRemoteHEAD warn create" '
git -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&
test_config -C two remote.origin.followRemoteHEAD "warn" &&
@@ -176,6 +212,18 @@ test_expect_success "fetch test followRemoteHEAD warn create" '
test "z$head" = "z$branch"
'
+test_expect_success "fetch test default followRemoteHEAD warn detached" '
+ git -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&
+ git -C two update-ref refs/remotes/origin/HEAD HEAD &&
+ HEAD=$(git -C two log --pretty="%H") &&
+ test_config -C two fetch.followRemoteHEAD "warn" &&
+ git -C two fetch >output &&
+ echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
+ "but we have a detached HEAD pointing to" \
+ "${SQ}${HEAD}${SQ} locally." >expect &&
+ test_cmp expect output
+'
+
test_expect_success "fetch test followRemoteHEAD warn detached" '
git -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&
git -C two update-ref refs/remotes/origin/HEAD HEAD &&
@@ -188,6 +236,19 @@ test_expect_success "fetch test followRemoteHEAD warn detached" '
test_cmp expect output
'
+test_expect_success "fetch test default followRemoteHEAD warn quiet" '
+ git -C two rev-parse --verify refs/remotes/origin/other &&
+ git -C two remote set-head origin other &&
+ git -C two rev-parse --verify refs/remotes/origin/HEAD &&
+ git -C two rev-parse --verify refs/remotes/origin/main &&
+ test_config -C two fetch.followRemoteHEAD "warn" &&
+ output=$(git -C two fetch --quiet) &&
+ test "z" = "z$output" &&
+ head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
+ branch=$(git -C two rev-parse refs/remotes/origin/other) &&
+ test "z$head" = "z$branch"
+'
+
test_expect_success "fetch test followRemoteHEAD warn quiet" '
git -C two rev-parse --verify refs/remotes/origin/other &&
git -C two remote set-head origin other &&
@@ -229,6 +290,18 @@ test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is di
test "z$head" = "z$branch"
'
+test_expect_success "fetch test default followRemoteHEAD always" '
+ git -C two rev-parse --verify refs/remotes/origin/other &&
+ git -C two remote set-head origin other &&
+ git -C two rev-parse --verify refs/remotes/origin/HEAD &&
+ git -C two rev-parse --verify refs/remotes/origin/main &&
+ test_config -C two fetch.followRemoteHEAD "always" &&
+ git -C two fetch &&
+ head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
+ branch=$(git -C two rev-parse refs/remotes/origin/main) &&
+ test "z$head" = "z$branch"
+'
+
test_expect_success "fetch test followRemoteHEAD always" '
git -C two rev-parse --verify refs/remotes/origin/other &&
git -C two remote set-head origin other &&
@@ -241,6 +314,28 @@ test_expect_success "fetch test followRemoteHEAD always" '
test "z$head" = "z$branch"
'
+test_expect_success 'per-remote followRemoteHEAD takes priority over fetch default' '
+ git -C two rev-parse --verify refs/remotes/origin/other &&
+ git -C two remote set-head origin other &&
+ git -C two rev-parse --verify refs/remotes/origin/HEAD &&
+ git -C two rev-parse --verify refs/remotes/origin/main &&
+ test_config -C two fetch.followRemoteHEAD "never" &&
+ test_config -C two remote.origin.followRemoteHEAD "always" &&
+ git -C two fetch &&
+ head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
+ branch=$(git -C two rev-parse refs/remotes/origin/main) &&
+ test "z$head" = "z$branch"
+'
+
+test_expect_success 'default followRemoteHEAD does not kick in with refspecs' '
+ git -C two remote set-head origin other &&
+ test_config -C two fetch.followRemoteHEAD always &&
+ git -C two fetch origin refs/heads/main:refs/remotes/origin/main &&
+ echo refs/remotes/origin/other >expect &&
+ git -C two symbolic-ref refs/remotes/origin/HEAD >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'followRemoteHEAD does not kick in with refspecs' '
git -C two remote set-head origin other &&
test_config -C two remote.origin.followRemoteHEAD always &&
@@ -250,6 +345,16 @@ test_expect_success 'followRemoteHEAD does not kick in with refspecs' '
test_cmp expect actual
'
+test_expect_success 'default followRemoteHEAD create does not overwrite dangling symref' '
+ test_when_finished "git -C two remote remove custom-head" &&
+ git -C two remote add -m does-not-exist custom-head ../one &&
+ test_config -C two fetch.followRemoteHEAD create &&
+ git -C two fetch custom-head &&
+ echo refs/remotes/custom-head/does-not-exist >expect &&
+ git -C two symbolic-ref refs/remotes/custom-head/HEAD >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'followRemoteHEAD create does not overwrite dangling symref' '
test_when_finished "git -C two remote remove custom-head" &&
git -C two remote add -m does-not-exist custom-head ../one &&
--
2.54.0
^ permalink raw reply related
* [PATCH 7/7] fetch: fixup a misaligned comment
From: Matt Hunter @ 2026-06-12 5:55 UTC (permalink / raw)
To: git
In-Reply-To: <20260612055947.1499497-1-m@lfurio.us>
Signed-off-by: Matt Hunter <m@lfurio.us>
---
builtin/fetch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a21bb82274d4..911ac8a47221 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1792,7 +1792,7 @@ static int set_head(const struct ref *remote_refs, struct remote *remote,
strbuf_addf(&b_head, "refs/remotes/%s/HEAD", remote->name);
strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", remote->name, head_name);
}
- /* make sure it's valid */
+ /* make sure it's valid */
if (!baremirror && !refs_ref_exists(refs, b_remote_head.buf)) {
result = 1;
goto cleanup;
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v3] update-ref: add --rename option
From: Patrick Steinhardt @ 2026-06-12 6:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7bo4n4ge.fsf@gitster.g>
On Thu, Jun 11, 2026 at 02:37:53PM -0700, Junio C Hamano wrote:
> Add a "--rename" option to "git update-ref" with the syntax:
>
> $ git update-ref --rename <old-refname> <new-refname>
>
> It renames <old-refname> together with its reflog to <new-refname>;
> even when used on a local branch ref, the current value and the
> reflog of the ref are the only things that are renamed. Document it
> and redirect casual users to "git branch -m" if that is what they
> wanted to do.
This reads much better, thanks.
> diff --git a/Documentation/git-update-ref.adoc b/Documentation/git-update-ref.adoc
> index 37a5019a8b..3b4df23a86 100644
> --- a/Documentation/git-update-ref.adoc
> +++ b/Documentation/git-update-ref.adoc
> @@ -9,6 +9,7 @@ SYNOPSIS
> --------
> [synopsis]
> git update-ref [-m <reason>] [--no-deref] -d <ref> [<old-oid>]
> +git update-ref [-m <reason>] --rename <old-refname> <new-refname>
> git update-ref [-m <reason>] [--no-deref] [--create-reflog] <ref> <new-oid> [<old-oid>]
> git update-ref [-m <reason>] [--no-deref] --stdin [-z] [--batch-updates]
This slightly triggers my OCD, but oh, well. No need to change this.
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 2d68c40ecb..65ee8af08c 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -800,6 +802,32 @@ int cmd_update_ref(int argc,
> if (end_null)
> usage_with_options(git_update_ref_usage, options);
>
> + if (rename) {
> + const char *oldref, *newref;
> +
> + if (delete || argc != 2)
> + usage_with_options(git_update_ref_usage, options);
Arguably, we should also complain when either "--no-deref" or "--deref"
were given, as they don't have any effect.
A slight tangent: this is part of why I really don't like commands that
determine their mode via flags: you now have to worry about every
combination of flags and whether they even make sense. With subcommands
we at least only have to worry about the set of flags that directly
apply to that given subcommand.
Makes me wonder whether I should have a look at extending git-refs(1)
further:
git refs delete <ref> [<oldvalue>]
git refs update <ref> <newvalue> [<oldvalue>]
git refs rename <ref> <oldname> <newname>
I always wanted to do this eventually so that we have one top-level
command that knows how to do "everything refs".
Anyway, except for this nit the patch looks good to me, thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v2] update-ref: add --rename option
From: Patrick Steinhardt @ 2026-06-12 6:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwlw4nccr.fsf@gitster.g>
On Thu, Jun 11, 2026 at 11:47:16AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > One thing that I'm missing from the commit message: what's the
> > motivation for this new mode?
>
> Maintenance of merge-fix database, a kludgy way to manage evil
> merges that are needed to deal with inter-topic semantic crashes.
>
> If you are really interested, see the appendix.
Thanks for the explanation!
Patrick
^ permalink raw reply
* Re: followRemoteHEAD management question
From: Matt Hunter @ 2026-06-12 6:11 UTC (permalink / raw)
To: Bence Ferdinandy, Jeff King; +Cc: git
In-Reply-To: <DJ6IBPYNOTTY.3QKEZQ28P713V@ferdinandy.com>
On Thu Jun 11, 2026 at 4:36 PM EDT, Bence Ferdinandy wrote:
> On Thu Jun 11, 2026 at 08:01, Jeff King <peff@peff.net> wrote:
>>
>> My initial thought is that it might affect clone as well as fetch. But I
>> guess this feature does not kick in for clone, as it has its own logic
>> for handling the remote-tracking HEAD. Though arguably it should be
>> possible to configure it not to create one in the first place.
>
> If memory serves well clone has set the remote/HEAD well before this and
> I think it indeed uses a different mechanism/logic.
I'm a little interested to try to look into the clone case as well, but
I think I'll save it for a later patch series and keep the scope of this
one as it is.
> Bit late to the party, but happy to review/test patches if they come.
Greatly appreciated!
>
> Best,
> Bence
The first version of my patches went out. You two are Cc'd on the cover
letter, but that didn't propagate to the patches themselves, oops.
^ permalink raw reply
* Re: [PATCH v2 3/3] doc: git-config: escape erroneous highlight markup
From: Jeff King @ 2026-06-12 6:17 UTC (permalink / raw)
To: Jean-Noël AVILA
Cc: Tuomas Ahola, git, Kristoffer Haugsbakk, Junio C Hamano
In-Reply-To: <20260612051605.GB593075@coredump.intra.peff.net>
On Fri, Jun 12, 2026 at 01:16:06AM -0400, Jeff King wrote:
> So...weird. groff wants to add extra space for some reason. It happens
> even if I drop the bolding, and just have "#" on a line by itself. I
> guess maybe it is the trailing "." of the previous line putting groff
> into "oh, I'm starting a new sentence" mode and it uses two spaces.
>
> But I think that is all outside the scope of your fix, and this is an
> existing issue that we are now just unlucky enough to hit. I'd be
> tempted to ignore it and possibly fix it later.
Poking at the groff manpage for possible fixes, I think it is either:
1. Just turn off extra spaces between sentences, like this:
.ss 12 0
The first argument "12" is "the minimum word space is 12/12ths of
the current font's word space. The second is "also add 0/12ths
between sentences" (so, no extra space).
That prevents the problem from happening anywhere, but maybe people
really like the extra spaces in other contexts.
2. There are some magic characters marked as "this doesn't start a new
sentence". We can set that flag for "#" like this:
.cflags 32 \#
I don't think docbook has parameter support for either of those, so we'd
have to do some kind of "shove this into the header" magic, which is a
bit gross.
-Peff
^ permalink raw reply
* Re: [PATCH 3/9] setup: don't apply "GIT_REFERENCE_BACKEND" without a repository
From: Patrick Steinhardt @ 2026-06-12 6:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Karthik Nayak
In-Reply-To: <xmqqa4t2wbb5.fsf@gitster.g>
On Wed, Jun 10, 2026 at 10:32:46AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > When discovering a repository we eventually also apply the
> > "GIT_REFERENCE_BACKEND" environment variable to the repository. There's
> > two problems with that:
> >
> > - We do this unconditionally, which is rather pointless: we really
> > only have to configure the repository when we have found one.
> >
> > - We have already applied the repository format at that point in time,
> > so we need to manually reapply it.
>
> Does the second point have a small typo, i.e., "if we have a
> repository, we have already applied the ref backend to it when we
> discovered it, so NO need to manually reapply"?
No, this is correct as-is. At the point in time where we handle
GIT_REFERENCE_BACKEND we have already discovered the repository format,
applied it to the repository, configured the reference database format
et al. So because we handle GIT_REFERENCE_BACKEND _after_ that whole
dance we basically have to re-configure the reference database format,
which is awkward.
Patrick
^ permalink raw reply
* Re: [PATCH 0/9] refs: stop using `chdir_notify_reparent()`
From: Patrick Steinhardt @ 2026-06-12 6:18 UTC (permalink / raw)
To: Jeff King; +Cc: git, Karthik Nayak
In-Reply-To: <20260611065346.GD2191159@coredump.intra.peff.net>
On Thu, Jun 11, 2026 at 02:53:46AM -0400, Jeff King wrote:
> On Wed, Jun 10, 2026 at 04:57:06PM +0200, Patrick Steinhardt wrote:
>
> > this patch series is a follow-up of the discussion at [1]. It converts
> > the reference backends to always use absolute paths internally, which
> > then allows us to drop the calls to `chdir_notify_reparent()`.
>
> We added chdir-notify to suport set_work_tree(). Commit 8500e0de3f
> (set_work_tree: use chdir_notify, 2018-03-30) mentions an optimization
> from 044bbbcb63 (Make git_dir a path relative to work_tree in
> setup_work_tree(), 2008-06-19). That commit demonstrates some measurable
> speedup from using relative versus absolute paths.
Oh, that is context I wasn't aware of. Not much of a surprise though,
given that this is from 2008 :) So thanks a lot for the pointer!
> If we move to a world of all absolute paths where chdir-notify is not
> necessary, will we lose that optimization?
Probably. Unfortunately, the commit doesn't have any repeatable
benchmarks in there, so it's hard to say whether we could still
reproduce those issues or not.
> I'm not sure how much it matters in practice these days, or if those
> timings could be repeated. And they weren't all _that_ big to start
> with. I guess it may depend on how deep your repo is within your
> filesystem, too.
Ideally, we'd have the best of both worlds: absolute paths everywhere
without the performance hit. A while back I had a discussion with
Torvalds on the securiy mailing list around this issue, and ultimately
the conclusion was that the best way forward would be to use openat(3p).
This wouldn't only allow us to optimize cases like this, but it also has
the added benefit that we're much less prone to TOCTOU-style issues and
we might even be able to use flags like O_BENEATH. So it would basically
be win-win. The only problem is of course that Windows doesn't have
openat(3p), so we'd have to emulate it, and that's where I always lost
the desire to do this.
When waking up this morning though I had the thought that we shouldn't
try to emulate openat(3p) directly, but instead create a higher-level
interface.
struct fsroot;
/*
* Open a new filesystem root at the given directory. All subsequent
* calls to open will be relative to this fsroot.
*/
struct fsroot *fsroot_new(const char *dir);
/*
* Create a new fsroot from a subdirectory relative to the given
* root directory.
*/
struct fsroot *fsroot_new_subdir(struct fsroot *r, const char *dir);
/*
* Open a new file relative to the given fsroot. This will use the
* equivalent of O_BENEATH so that we only ever open files that are
* located below the fsroot.
*/
int fsroot_open(struct fsroot *r, const char *path, int oflag, ...);
This is of course heavily inspired by similar interfaces that exist in
Go [1]. By having such a higher-level abstraction it should also be way
easier to port this to different platforms, where we can then add safety
features like O_BENEATH when available on any given platform.
The idea here would be that we can then convert some subsystems to use
those structures instead of tracking paths. I'd for example love for the
repository's working tree to use this mechanism so that we can squash a
whole class of potential security issues when checking out files that
end in locations we didn't intend to.
Thanks!
Patrick
[1]: https://pkg.go.dev/io/fs#FS
^ permalink raw reply
* Re: [PATCH v3 1/3] commit-reach: handle cycles in contains walk
From: Kristofer Karlsson @ 2026-06-12 6:53 UTC (permalink / raw)
To: Tamir Duberstein
Cc: git, Jeff King, Karthik Nayak, Junio C Hamano, Victoria Dye,
Derrick Stolee, Elijah Newren
In-Reply-To: <20260611-ref-filter-memoized-contains-v3-1-b26af3dba285@gmail.com>
On Fri, 12 Jun 2026 at 05:00, Tamir Duberstein <tamird@gmail.com> wrote:
>
> The memoized contains traversal used by git tag assumes that commit
> ancestry is acyclic. Replacement refs can violate that assumption,
> causing it to keep pushing an already active commit until memory is
> exhausted.
>
The cycle detection itself makes sense, but would it be simpler to
just die() when a cycle is found rather than falling back to a
second reachability walk?
A cycle in the commit graph means replacement refs are
misconfigured. The existing code already loops forever when it
hits one, so detecting and dying is strictly an improvement. The
fallback adds a second codepath through the function, discards all
cached results (so later candidates redo work), and papers over
what is really a broken invariant.
do_lookup_replace_object() already dies when replacement refs
chain deeper than MAXREPLACEDEPTH (which covers cycles), so the
existing contract treats this as a fatal configuration error.
parse_commit_or_die() sets the same precedent within the walk
itself.
Kristofer
^ permalink raw reply
* Re: [PATCH v2 1/1] environment.c: move 'protect_hfs' and 'protect_ntfs' into 'repo_config_values'
From: Christian Couder @ 2026-06-12 7:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Tian Yuchen, git, phillip.wood123, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <xmqqse6uwdnz.fsf@gitster.g>
On Wed, Jun 10, 2026 at 6:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tian Yuchen <cat@malon.dev> writes:
>
> > +int repo_protect_ntfs(struct repository *repo)
> > +{
> > + return repo->gitdir ?
> > + repo_config_values(repo)->protect_ntfs :
> > + PROTECT_NTFS_DEFAULT;
> > +}
> > +
> > +int repo_protect_hfs(struct repository *repo)
> > +{
> > + return repo->gitdir ?
> > + repo_config_values(repo)->protect_hfs :
> > + PROTECT_HFS_DEFAULT;
> > +}
> > ...
> > @@ -123,6 +125,14 @@ int git_default_config(const char *, const char *,
> > int git_default_core_config(const char *var, const char *value,
> > const struct config_context *ctx, void *cb);
> >
> > +/*
> > + * Getters for the `protect_hfs` and `protect_ntfs` fields of `struct repo_config_values`.
> > + * They check `repo->gitdir` to prevent calling repo_config_values()
> > + * before the configuration is loaded or in bare environments.
> > + */
> > +int repo_protect_hfs(struct repository *repo);
> > +int repo_protect_ntfs(struct repository *repo);
>
> I briefly wondered what *should* happen when repo->gitdir is not
> ready, as it feels almost a bug for a caller to call these two
> functions before the repository is ready to be used.
>
> When repo is not ready, these return their respective default
> values. That's like the original code using the initial value of
> these global variables.
>
> IOW, this rewrite is bug-for-bug compatible, which is good.
>
> Shall we declare victory and mark the topic for 'next' now?
I would have preferred the commit subject to start with "environment:"
rather than "environment.c:" but it's a small nit and maybe you can
fix it while merging.
Otherwise the patch looks indeed good to me.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/3] read-cache: move 'ce_mode_from_stat()' to 'read-cache.c'
From: Christian Couder @ 2026-06-12 7:43 UTC (permalink / raw)
To: Tian Yuchen; +Cc: git, ps, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260610093635.139719-3-cat@malon.dev>
On Wed, Jun 10, 2026 at 11:37 AM Tian Yuchen <cat@malon.dev> wrote:
>
> The ce_mode_from_stat() function is declared as a static inline function
> in 'read-cache.h'. As we want to migrate configuration variables, this
> helper function will need access to corresponding repository-specific
> configuration logic. Move the implementation to 'read-cache.c' to
> cleanly encapsulate its dependencies.
>
> Note that the 'extern int trust_executable_bit, has_symlinks;' line is
> discarded because it's not necessary when the function lives in
> "read-cache.c".
>
> At present, this change has no visible impact, but it is crucial
> for our future plans to pass in the repo context. Comment
> has been added whilst we are at it.
We prefer it when comments like the one below are added in front of
the declaration of the function into the header file ("read-cache.h"
here), rather than the *.c file.
And yeah, I know that "read-cache.h" is a bad example right now
because no function has such a comment there yet.
> +/*
> + * Determine the appropriate index mode for a file based on its stat()
> + * information and the existing cache entry (if any).
> + *
> + * This function handles degradation for filesystems that lack
> + * symlink support or reliable executable bits.
> + */
> +unsigned int ce_mode_from_stat(const struct cache_entry *ce, unsigned int mode)
> +{
> + if (!has_symlinks && S_ISREG(mode) &&
> + ce && S_ISLNK(ce->ce_mode))
> + return ce->ce_mode;
> + if (!trust_executable_bit && S_ISREG(mode)) {
> + if (ce && S_ISREG(ce->ce_mode))
> + return ce->ce_mode;
> + return create_ce_mode(0666);
> + }
> + return create_ce_mode(mode);
> +}
^ permalink raw reply
* Re: [PATCH v2 3/3] environment: move trust_executable_bit into repo_config_values
From: Christian Couder @ 2026-06-12 7:48 UTC (permalink / raw)
To: Tian Yuchen; +Cc: git, ps, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260610093635.139719-4-cat@malon.dev>
On Wed, Jun 10, 2026 at 11:37 AM Tian Yuchen <cat@malon.dev> wrote:
> +/*
> + * Getters for the `repo_trust_executable_bit` fields of `struct repo_config_values`.
s/Getters/Getter/
s/fields/field/
s/repo_trust_executable_bit/trust_executable_bit/
> + * They check `repo->gitdir` to prevent calling repo_config_values()
> + * before the configuration is loaded or in bare environments.
> + */
> +int repo_trust_executable_bit(struct repository *repo);
Thanks.
^ permalink raw reply
* Re: [PATCH v2 5/7] environment: split up concerns of `is_bare_repository_cfg`
From: Toon Claes @ 2026-06-12 7:59 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Justin Tobler
In-Reply-To: <20260611-b4-pks-setup-drop-global-state-v2-5-a6f7269c841d@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The `is_bare_repository_cfg` variable tracks two different pieces of
> information:
>
> - It tracks whether the user has invoked git with the "--bare" flag,
> which makes us treat any discovered Git repository as if it was a
> bare repository.
>
> - Otherwise it tracks whether the discovered `the_repository` is bare.
>
> This makes the flag extremely confusing and creates a bit of a challenge
> when handling multiple repositories in the same process.
>
> Split up the concerns of this variable into two pieces:
>
> - `startup_info.force_bare_repository` tracks whether the user has
> passed the "--bare" flag. This is used as a hint to treat newly set
> up repositories as bare regardless of whether or not they have a
> worktree.
>
> - `struct repository::bare_cfg` tracks whether or not a repository is
> considered bare. This takes into account both whether the user has
> passed "--bare" and the discovered state of the repository itself.
>
> Whether or not a repository is bare is now resolved when checking the
> repository's format, and is then later applied to the repository itself
> via `apply_repository_format()`.
>
> This enables a subsequent change where we make `is_bare_repository()`
> not depend on global state anymore.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/init-db.c | 2 +-
> environment.c | 5 ++---
> environment.h | 1 -
> git.c | 2 +-
> repository.c | 1 +
> repository.h | 7 +++++++
> setup.c | 27 ++++++++++++++++++++-------
> setup.h | 6 ++++++
> worktree.c | 2 +-
> 9 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 52aa92fb0a..566732c9f4 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -81,7 +81,7 @@ int cmd_init_db(int argc,
> const char *template_dir = NULL;
> char *template_dir_to_free = NULL;
> unsigned int flags = 0;
> - int bare = is_bare_repository_cfg;
> + int bare = startup_info->force_bare_repository ? 1 : -1;
> const char *object_format = NULL;
> const char *ref_format = NULL;
> const char *initial_branch = NULL;
> diff --git a/environment.c b/environment.c
> index 4e86335f25..9d7c908c55 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -48,7 +48,6 @@ int has_symlinks = 1;
> int minimum_abbrev = 4, default_abbrev = -1;
> int ignore_case;
> int assume_unchanged;
> -int is_bare_repository_cfg = -1; /* unspecified */
> int warn_on_object_refname_ambiguity = 1;
> char *git_commit_encoding;
> char *git_log_output_encoding;
> @@ -136,7 +135,7 @@ const char *getenv_safe(struct strvec *argv, const char *name)
> int is_bare_repository(void)
> {
> /* if core.bare is not 'false', let's see if there is a work tree */
> - return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
> + return the_repository->bare_cfg && !repo_get_work_tree(the_repository);
> }
>
> int have_git_dir(void)
> @@ -342,7 +341,7 @@ int git_default_core_config(const char *var, const char *value,
> }
>
> if (!strcmp(var, "core.bare")) {
> - is_bare_repository_cfg = git_config_bool(var, value);
> + the_repository->bare_cfg = git_config_bool(var, value);
> return 0;
> }
>
> diff --git a/environment.h b/environment.h
> index 5d6e4e6c1b..afb5bcf197 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -147,7 +147,6 @@ void repo_config_values_init(struct repo_config_values *cfg);
> */
> int have_git_dir(void);
>
> -extern int is_bare_repository_cfg;
> int is_bare_repository(void);
>
> /* Environment bits from configuration mechanism */
> diff --git a/git.c b/git.c
> index 36f08891ef..387eabe38c 100644
> --- a/git.c
> +++ b/git.c
> @@ -255,7 +255,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> *envchanged = 1;
> } else if (!strcmp(cmd, "--bare")) {
> char *cwd = xgetcwd();
> - is_bare_repository_cfg = 1;
> + startup_info->force_bare_repository = true;
> setenv(GIT_DIR_ENVIRONMENT, cwd, 0);
> free(cwd);
> setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
> diff --git a/repository.c b/repository.c
> index 187dd471c4..c1e91eb0da 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -73,6 +73,7 @@ void initialize_repository(struct repository *repo)
> ALLOC_ARRAY(repo->index, 1);
> index_state_init(repo->index, repo);
> repo->check_deprecated_config = true;
> + repo->bare_cfg = -1;
> repo_config_values_init(&repo->config_values_private_);
>
> /*
> diff --git a/repository.h b/repository.h
> index 36e2db2633..7d649e32e7 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -117,6 +117,13 @@ struct repository {
> bool worktree_initialized;
> bool worktree_config_is_bogus;
>
> + /*
> + * Whether the repository is bare, as set by "core.bare" config or
> + * inferred during repository discovery. -1 means unset/unknown, 0
> + * means non-bare, 1 means bare.
> + */
> + int bare_cfg;
> +
> /*
> * Path from the root of the top-level superproject down to this
> * repository. This is only non-NULL if the repository is initialized
> diff --git a/setup.c b/setup.c
> index 71fc6b33da..32f14a8688 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -795,10 +795,22 @@ static int check_repository_format_gently(const char *gitdir,
> has_common = 0;
> }
>
> - if (!has_common) {
> - if (candidate->is_bare != -1)
> - is_bare_repository_cfg = candidate->is_bare;
> - } else {
> + if (startup_info->force_bare_repository) {
> + candidate->is_bare = 1;
> + FREE_AND_NULL(candidate->work_tree);
> + } else if (has_common) {
> + /*
> + * When sharing a common dir with another repository (e.g. a
> + * linked worktree), do not let this repository's config
> + * dictate bareness; it is inherited from the main worktree.
> + */
> + candidate->is_bare = -1;
> +
> + /*
> + * Furthermore, "core.worktree" is supposed to be ignored when
> + * we have a commondir configured, unless it comes from the
> + * per-worktree configuration.
> + */
> FREE_AND_NULL(candidate->work_tree);
> }
Looking at the diff, this is really hard to understand. But your
refactor makes sense and the after state is easier to comprehend.
> @@ -1138,7 +1150,7 @@ static const char *setup_explicit_git_dir(struct repository *repo,
> /* #3, #7, #11, #15, #19, #23, #27, #31 (see t1510) */
> if (work_tree_env)
> set_git_work_tree(repo, work_tree_env);
> - else if (is_bare_repository_cfg > 0) {
> + else if (repo_fmt->is_bare > 0) {
> if (repo_fmt->work_tree) {
> /* #22.2, #30 */
> warning("core.bare and core.worktree do not make sense");
> @@ -1225,7 +1237,7 @@ static const char *setup_discovered_git_dir(struct repository *repo,
> }
>
> /* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
> - if (is_bare_repository_cfg > 0) {
> + if (repo_fmt->is_bare > 0) {
> set_git_dir(repo, gitdir, (offset != cwd->len));
> if (chdir(cwd->buf))
> die_errno(_("cannot come back to cwd"));
> @@ -1762,6 +1774,7 @@ int apply_repository_format(struct repository *repo,
> alternate_object_directories = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));
> }
>
> + repo->bare_cfg = format->is_bare;
> repo_set_hash_algo(repo, format->hash_algo);
> repo->objects = odb_new(repo, object_directory,
> alternate_object_directories);
> @@ -2571,7 +2584,7 @@ static int create_default_files(struct repository *repo,
> repo_settings_set_shared_repository(repo,
> init_shared_repository);
>
> - is_bare_repository_cfg = !work_tree;
> + repo->bare_cfg = !work_tree;
I'm curious, do we still need this? If I'm not mistaken, this function
is called after check_and_apply_repository_format(), which calls
apply_repository_format() and sets repo->bare_cfg too (see the diff
above). Or is it explained by what Justin said[1]?
[1]: <airTpB8Pm6TYoMhx@denethor>
--
Cheers,
Toon
^ permalink raw reply
* Re: [PATCH 7/7] treewide: drop USE_THE_REPOSITORY_VARIABLE
From: Toon Claes @ 2026-06-12 8:02 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260610-b4-pks-setup-drop-global-state-v1-7-5dff3eec8f06@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Adapt a couple of trivial callers of `is_bare_repository()` to instead
> use a repository available via the caller's context so that we can drop
> the `USE_THE_REPOSITORY_VARIABLE` macro.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/repack.c | 3 +--
> mailmap.c | 6 ++----
> refs/reftable-backend.c | 4 +---
> setup.c | 3 +--
> 4 files changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index bbc6f51639..d0465fb4f5 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -1,4 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
This makes it all worthwhile. Great work on untangling this!
--
Cheers,
Toon
^ permalink raw reply
* Re: [PATCH v2 1/7] builtin/init: stop modifying global `git_work_tree_cfg` variable
From: Toon Claes @ 2026-06-12 8:04 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Justin Tobler
In-Reply-To: <20260611-b4-pks-setup-drop-global-state-v2-1-a6f7269c841d@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> When executing git-init(1) we need to figure out the final location of
> the worktree. This location can be configured in a couple of ways: via
> an environment variable, via the preexisting "core.worktree" config in
> case we're reinitializing, or implicitly when reinitializing a non-bare
> repository.
Do you mean:
> case we're reinitializing, or implicitly when initializing a non-bare
> repository.
So the second 'init' without the 're'?
Obviously not worth a reroll on it's own.
--
Cheers,
Toon
^ permalink raw reply
* Re: [PATCH v2 0/7] setup: drop global state
From: Toon Claes @ 2026-06-12 8:06 UTC (permalink / raw)
To: Justin Tobler, Patrick Steinhardt; +Cc: git
In-Reply-To: <airVOrTboNDDGBak@denethor>
Justin Tobler <jltobler@gmail.com> writes:
> I find the additional explaination here quite helpful. Thanks.
Yeah, hard to follow series looking at the code only, but commit
messages make more bearable.
> The changes in this version of the series looks good to me.
I've only reviewed v2 and I agree this version looks good.
--
Cheers,
Toon
^ permalink raw reply
* Re: [PATCH v2 1/3] replay: refactor enum replay_mode into a bool
From: Toon Claes @ 2026-06-12 8:19 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <airORumUxyTsN7Bz@denethor>
Justin Tobler <jltobler@gmail.com> writes:
> Naive question: Do we expect there to only be two replay modes in the
> forseeable future? I suppose if other modes were added in the future
> this change would essentially be reverted.
The enum was mainly used to determine "direction": PICK to apply commits
forward, and REVERT to apply them in opposite order. But it's a bit
twofold, because REVERT also applies the inverse change. At the moment
--onto and --advance use PICK and --revert uses REVERT. There could be
added more options in the future, but I don't expect any of them to add
a new mode. And if there is ever a new mode needed, I think it's better
to re-add the enum then, or maybe a second bool makes sense then, who
knows...
--
Cheers,
Toon
^ permalink raw reply
* Re: [RFC PATCH] MyFirstContribution: mention trimming quoted text in replies
From: Weijie Yuan @ 2026-06-12 8:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqcxxwljue.fsf@gitster.g>
On Thu, Jun 11, 2026 at 04:48:25PM -0700, Junio C Hamano wrote:
> [...]
> > diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc
> > index 607876f3d8..0e2a9313ce 100644
> > --- a/Documentation/MyFirstContribution.adoc
> > +++ b/Documentation/MyFirstContribution.adoc
> > @@ -1453,6 +1453,11 @@ effect which had not occurred to you. It is always okay to ask for clarification
> > if you aren't sure why a change was suggested, or what the reviewer is asking
> > you to do.
> >
> > +When replying to review comments, quote only the parts of the message that are
> > +relevant to your response. It is usually helpful to trim away unrelated context,
> > +such as large portions of the patch that are not being discussed, while keeping
> > +enough quoted text for readers to understand what you are responding to.
> > +
> > Make sure your email client has a plaintext email mode and it is turned on; the
> > Git list rejects HTML email. Please also follow the mailing list etiquette
> > outlined in the
>
> The insertion point is well chosen, immediately following the
> discussion on how to handle review comments and before the technical
> details of email client configuration. The text itself is clear and
> gives sound advice.
Thank you for your reply.
I first considered folding this into one of the existing paragraphs
above, but ended up making it a separate paragraph for better clarity.
Let's see whether others have better wording suggestions.
Now the expectations for contributors and reviewers are more aligned
when participating in review discussions :-)
Hopefully this addition will make review discussions a little easier to
focus and reduce the burden on reviewers.
Thanks!
^ permalink raw reply
* Re: [PATCH 2/9] setup: stop applying repository format twice
From: Karthik Nayak @ 2026-06-12 9:00 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-2-56c864b01c43@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3140 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> When discovering the repository in "setup.c" we apply the final
> repository format multiple times:
>
> - Once via `repository_format_configure()`, where we configure the
> repository format for both `struct repository_format` and `struct
> repository`.
>
> - And once via `apply_repository_format()`, where we then apply the
> `struct repository_format` to the `struct repository` again.
>
Okay so we're talking applying the repository format to the `struct
repository` specifically.
> As the format will be applied to the repository when applying the format
> it's thus somewhat unnecessary to also apply it to the repository when
> adapting the discovered format.
This was a bit confusing to read at first. Okay since we already apply
the format in the second step, the first is not necessary.
> The only reason we have to do this is
> because we call `repository_format_configure()` after we have already
> applied it.
Right, so there is a need to do this.
>
> Refactor the code so that we first configure the repository format
> before applying it to the repository so that we can stop setting the
> hash and reference storage format multiple times.
>
Makse sense.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> setup.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index a9db1f2c23..2748155964 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -2710,8 +2710,7 @@ static int read_default_format_config(const char *key, const char *value,
> return ret;
> }
>
> -static void repository_format_configure(struct repository *repo,
> - struct repository_format *repo_fmt,
> +static void repository_format_configure(struct repository_format *repo_fmt,
> int hash, enum ref_storage_format ref_format)
> {
> struct default_format_config cfg = {
> @@ -2748,7 +2747,6 @@ static void repository_format_configure(struct repository *repo,
> } else if (cfg.hash != GIT_HASH_UNKNOWN) {
> repo_fmt->hash_algo = cfg.hash;
> }
> - repo_set_hash_algo(repo, repo_fmt->hash_algo);
>
> env = getenv("GIT_DEFAULT_REF_FORMAT");
> if (repo_fmt->version >= 0 &&
> @@ -2786,9 +2784,6 @@ static void repository_format_configure(struct repository *repo,
>
> free(backend);
> }
> -
> - repo_set_ref_storage_format(repo, repo_fmt->ref_storage_format,
> - repo_fmt->ref_storage_payload);
> }
>
> int init_db(struct repository *repo,
> @@ -2830,10 +2825,10 @@ int init_db(struct repository *repo,
> * is an attempt to reinitialize new repository with an old tool.
> */
> check_repository_format_gently(repo_get_git_dir(repo), &repo_fmt, NULL);
> + repository_format_configure(&repo_fmt, hash, ref_storage_format);
> if (apply_repository_format(repo, &repo_fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
> die("%s", err.buf);
> startup_info->have_repository = 1;
> - repository_format_configure(repo, &repo_fmt, hash, ref_storage_format);
>
> /*
> * Ensure `core.hidedotfiles` is processed. This must happen after we
>
> --
> 2.54.0.1189.g8c84645362.dirty
The patch looks good.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 4/9] refs: unregister reference stores from "chdir_notify"
From: Karthik Nayak @ 2026-06-12 9:18 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-4-56c864b01c43@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6726 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> When creating reference stores we register them with the "chdir_notify"
> subsystem. This is required because some of the paths we track may be
> relative paths, so we have to reparent them in case the current working
> directory changes.
>
> But while we register the reference stores, we never unregister them.
> This can have multiple outcomes:
>
> - For a repository's main reference database we essentially keep the
> pointer alive. We never free that database, either, and our leak
> checker doesn't notice because it's still registered.
>
> - For submodule and worktree reference databases we do eventually free
> them in `repo_clear()`, so we may keep pointers to free'd memory
> registered. We never notice though as we don't tend to chdir around
> in the middle of the process.
>
So `ref_store_release()` is what is called to release a ref_store. So
in the former's case, we never release the ref_store even if the
repository is closed, wow.
> We never noticed either of these symptoms, but they are obviously bad.
>
> Partially fix those issues by unregistering the reference stores when
> releasing them. The leak of the main reference database will be fixed in
> a subsequent commit.
>
> Note that this requires us to use `chdir_notify_register()` instead of
> `chdir_notify_parent()`, as there is no infrastructure to unregister the
Shouldn't this be s/chdir_notify_parent/chdir_notify_reparent ?
> latter. It ultimately doesn't matter much though: in a subsequent commit
> we'll drop this infrastructure completely. We merely require this step
> here so that we can fix the memory leaks ahead of time.
Right, we can't unregister when using `chdir_notify_reparent()` because
it internally calls `chdir_notify_register()` with a private cb
function, and we need to supply the callback function during
un-registering. Makes sense.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> refs/files-backend.c | 22 +++++++++++++++++++---
> refs/packed-backend.c | 16 +++++++++++++++-
> refs/reftable-backend.c | 16 +++++++++++++++-
> 3 files changed, 49 insertions(+), 5 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index a4c7858787..296981584b 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -100,6 +100,23 @@ static void clear_loose_ref_cache(struct files_ref_store *refs)
> }
> }
>
> +static void files_ref_store_reparent(const char *name UNUSED,
> + const char *old_cwd,
> + const char *new_cwd,
> + void *payload)
> +{
> + struct files_ref_store *refs = payload;
> + char *tmp;
> +
> + tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
> + free(refs->base.gitdir);
> + refs->base.gitdir = tmp;
> +
> + tmp = reparent_relative_path(old_cwd, new_cwd, refs->gitcommondir);
> + free(refs->gitcommondir);
> + refs->gitcommondir = tmp;
> +}
> +
Looks similar to `void reparent_cb()` but for both the directories.
> /*
> * Create a new submodule ref cache and add it to the internal
> * set of caches.
> @@ -128,9 +145,7 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
>
> repo_config_get_bool(repo, "core.prefersymlinkrefs", &refs->prefer_symlink_refs);
>
> - chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir);
> - chdir_notify_reparent("files-backend $GIT_COMMONDIR",
> - &refs->gitcommondir);
> + chdir_notify_register(NULL, files_ref_store_reparent, refs);
>
> strbuf_release(&refdir);
>
> @@ -182,6 +197,7 @@ static void files_ref_store_release(struct ref_store *ref_store)
> free(refs->gitcommondir);
> ref_store_release(refs->packed_ref_store);
> free(refs->packed_ref_store);
> + chdir_notify_unregister(NULL, files_ref_store_reparent, refs);
> }
>
> static void files_reflog_path(struct files_ref_store *refs,
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 0acde48c45..499cb55dfa 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -211,6 +211,19 @@ static size_t snapshot_hexsz(const struct snapshot *snapshot)
> return snapshot->refs->base.repo->hash_algo->hexsz;
> }
>
> +static void packed_ref_store_reparent(const char *name UNUSED,
> + const char *old_cwd,
> + const char *new_cwd,
> + void *payload)
> +{
> + struct packed_ref_store *refs = payload;
> + char *tmp;
> +
> + tmp = reparent_relative_path(old_cwd, new_cwd, refs->path);
> + free(refs->path);
> + refs->path = tmp;
> +}
> +
> /*
> * Since packed-refs is only stored in the common dir, don't parse the
> * payload and rely on the files-backend to set 'gitdir' correctly.
> @@ -229,7 +242,7 @@ struct ref_store *packed_ref_store_init(struct repository *repo,
>
> strbuf_addf(&sb, "%s/packed-refs", gitdir);
> refs->path = strbuf_detach(&sb, NULL);
> - chdir_notify_reparent("packed-refs", &refs->path);
> + chdir_notify_register(NULL, packed_ref_store_reparent, refs);
> return ref_store;
> }
>
> @@ -274,6 +287,7 @@ static void packed_ref_store_release(struct ref_store *ref_store)
> clear_snapshot(refs);
> rollback_lock_file(&refs->lock);
> delete_tempfile(&refs->tempfile);
> + chdir_notify_unregister(NULL, packed_ref_store_reparent, refs);
> free(refs->path);
> }
>
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 4ae22922de..8c93070677 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -365,6 +365,19 @@ static int reftable_be_config(const char *var, const char *value,
> return 0;
> }
>
> +static void reftable_be_reparent(const char *name UNUSED,
> + const char *old_cwd,
> + const char *new_cwd,
> + void *payload)
> +{
> + struct reftable_ref_store *refs = payload;
> + char *tmp;
> +
> + tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
> + free(refs->base.gitdir);
> + refs->base.gitdir = tmp;
> +}
> +
> static struct ref_store *reftable_be_init(struct repository *repo,
> const char *payload,
> const char *gitdir,
> @@ -447,7 +460,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,
> goto done;
> }
>
> - chdir_notify_reparent("reftables-backend $GIT_DIR", &refs->base.gitdir);
> + chdir_notify_register(NULL, reftable_be_reparent, refs);
>
> done:
> assert(refs->err != REFTABLE_API_ERROR);
> @@ -474,6 +487,7 @@ static void reftable_be_release(struct ref_store *ref_store)
> free(be);
> }
> strmap_clear(&refs->worktree_backends, 0);
> + chdir_notify_unregister(NULL, reftable_be_reparent, refs);
> }
>
> static int reftable_be_create_on_disk(struct ref_store *ref_store,
>
> --
> 2.54.0.1189.g8c84645362.dirty
The changes here look good.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 6/9] repository: free main reference database
From: Karthik Nayak @ 2026-06-12 9:20 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-6-56c864b01c43@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> While we release worktree and submodule reference databases when
> clearing a repository, we don't ever release the main reference
> database. This memory leak went unnoticed because its pointer is
> kept alive by the "chdir_notify" subsystem.
>
> Fix the memory leak.
>
Funny, cause long ago I looked into this and thought I was clearly
missing something and eventually forgot about it. Good to know that I
was correct :)
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> repository.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/repository.c b/repository.c
> index 187dd471c4..e2b5c6712b 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -421,6 +421,11 @@ void repo_clear(struct repository *repo)
> FREE_AND_NULL(repo->remote_state);
> }
>
> + if (repo->refs_private) {
> + ref_store_release(repo->refs_private);
> + FREE_AND_NULL(repo->refs_private);
> + }
> +
> strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e)
> ref_store_release(e->value);
> strmap_clear(&repo->submodule_ref_stores, 1);
>
> --
> 2.54.0.1189.g8c84645362.dirty
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v2] commit-reach: remove get_reachable_subset()
From: Weijie Yuan @ 2026-06-12 9:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee, Kristofer Karlsson via GitGitGadget, git,
Kristofer Karlsson, Patrick Steinhardt
In-Reply-To: <xmqq7bo5nf31.fsf@gitster.g>
On Thu, Jun 11, 2026 at 10:48:18AM -0700, Junio C Hamano wrote:
> I wonder if we should talk about it in the SubmittingPatches and/or
> MyFirstContribution document?
Hi, I think it might be a good idea to cover these details in
MyFirstContribution, then cross-reference them from the part of
SubmittingPatches that discusses sending a new version.
More specifically, I think these details could fit around steps 3 and 4
of "A typical life cycle of a patch series" in SubmittingPatches,
starting around line 54. That section may need some reworking of the
existing wording, rather than just an additive change.
Also, for the part about sending a new version, I wonder whether it
would be better to summarize and fold in Patrick's explanation here,
thank you Patrick:
---
From: Patrick Steinhardt <ps@pks.im>
Message-ID: <aietF4BX1Ewt3cpG@pks.im>
> By the way, how long should I wait before sending new versions of my
> patches? I have 4 outstanding at the moment.
I typically aim to send at most one version per day per patch series.
This avoids that you're "flooding" the mailing list with too many
versions of the same series, allows you to address feedback from
multiple folks in batches, and it gives you enough time to think about
the feedback without having to rush anything.
Whether I actually do end up sending a series depends on a couple of
factors:
- How big is the series? The bigger it is the more time I give folks
to perform reviews.
- How substantial were the reviews you received? Is it just a couple
of small typos? Then it probably makes sense to wait one or two more
days to get some more involved reviews. Is it something that
requires signifciant rework? Then I'd send out soon so that others
don't review a patch series that will change significantly anyway.
- How close to being merged is the series? The closer it is the less
substantial the reviews will (hopefully) get, so it makes sense to
reroll a bit faster even if you only received minor feedback.
So there isn't really a golden rule to follow here, but a lot of this
depends on gut feeling. You probably won't have that feeling yet when
starting out in a new project, but that's fine. In case we see that
behaviour doesn't quite match the norm we'll typically give a hint that
the contributor should slow down or maybe send a new iteration.
Patrick
---
Patrick's point may be beyond the scope of this thread, so I only
mention it as an aside. Maybe these could be part of the same series.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/7] builtin/init: stop modifying global `git_work_tree_cfg` variable
From: Patrick Steinhardt @ 2026-06-12 9:27 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Justin Tobler
In-Reply-To: <87pl1wyyjw.fsf@emacs.iotcl.com>
On Fri, Jun 12, 2026 at 10:04:35AM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > When executing git-init(1) we need to figure out the final location of
> > the worktree. This location can be configured in a couple of ways: via
> > an environment variable, via the preexisting "core.worktree" config in
> > case we're reinitializing, or implicitly when reinitializing a non-bare
> > repository.
>
> Do you mean:
>
> > case we're reinitializing, or implicitly when initializing a non-bare
> > repository.
>
> So the second 'init' without the 're'?
>
> Obviously not worth a reroll on it's own.
It can actually happen in both cases. I've queued the following change
locally. Thanks!
Patrick
1: 0808dbb336 ! 1: cc6999257c builtin/init: stop modifying global `git_work_tree_cfg` variable
@@ Commit message
When executing git-init(1) we need to figure out the final location of
the worktree. This location can be configured in a couple of ways: via
an environment variable, via the preexisting "core.worktree" config in
- case we're reinitializing, or implicitly when reinitializing a non-bare
- repository.
+ case we're reinitializing, or implicitly when (re)initializing a
+ non-bare repository.
When checking for the worktree location in "builtin/init-db.c" we
populate any potentially-discovered value both by setting the global
^ permalink raw reply
* Re: [PATCH v2 5/7] environment: split up concerns of `is_bare_repository_cfg`
From: Patrick Steinhardt @ 2026-06-12 9:28 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Justin Tobler
In-Reply-To: <87wlw4yys1.fsf@emacs.iotcl.com>
On Fri, Jun 12, 2026 at 09:59:42AM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/setup.c b/setup.c
> > index 71fc6b33da..32f14a8688 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -795,10 +795,22 @@ static int check_repository_format_gently(const char *gitdir,
> > has_common = 0;
> > }
> >
> > - if (!has_common) {
> > - if (candidate->is_bare != -1)
> > - is_bare_repository_cfg = candidate->is_bare;
> > - } else {
> > + if (startup_info->force_bare_repository) {
> > + candidate->is_bare = 1;
> > + FREE_AND_NULL(candidate->work_tree);
> > + } else if (has_common) {
> > + /*
> > + * When sharing a common dir with another repository (e.g. a
> > + * linked worktree), do not let this repository's config
> > + * dictate bareness; it is inherited from the main worktree.
> > + */
> > + candidate->is_bare = -1;
> > +
> > + /*
> > + * Furthermore, "core.worktree" is supposed to be ignored when
> > + * we have a commondir configured, unless it comes from the
> > + * per-worktree configuration.
> > + */
> > FREE_AND_NULL(candidate->work_tree);
> > }
>
> Looking at the diff, this is really hard to understand. But your
> refactor makes sense and the after state is easier to comprehend.
Yeah, I'm in the same boat. Honestly, I really hope that our test suite
has enough coverage so that this refactoring won't cause any significant
regressions. Which isn't exactly a statement of confidence, but rather a
statement of "oh boy, this is awfully complex and has lots of weird edge
cases".
> > @@ -2571,7 +2584,7 @@ static int create_default_files(struct repository *repo,
> > repo_settings_set_shared_repository(repo,
> > init_shared_repository);
> >
> > - is_bare_repository_cfg = !work_tree;
> > + repo->bare_cfg = !work_tree;
>
> I'm curious, do we still need this? If I'm not mistaken, this function
> is called after check_and_apply_repository_format(), which calls
> apply_repository_format() and sets repo->bare_cfg too (see the diff
> above). Or is it explained by what Justin said[1]?
We can't drop this because after we've applied the format we call
`repo_config()` with `git_default_core_config()`, which will potentially
set the `bare_cfg` variable if "core.bare" is set.
Patrick
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox