* [PATCH v3 06/18] setup: stop using `the_repository` in `verify_filename()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Stop using `the_repository` in `verify_filename()` and instead accept
the repository as a parameter. The injection of `the_repository` is thus
bumped one level higher, where callers now pass it in explicitly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/grep.c | 2 +-
builtin/reset.c | 2 +-
builtin/rev-parse.c | 4 ++--
revision.c | 2 +-
setup.c | 5 +++--
setup.h | 3 ++-
6 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index e33285e5e6..b0e350cf89 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1163,7 +1163,7 @@ int cmd_grep(int argc,
if (!seen_dashdash) {
int j;
for (j = i; j < argc; j++)
- verify_filename(prefix, argv[j], j == i && allow_revs);
+ verify_filename(the_repository, prefix, argv[j], j == i && allow_revs);
}
parse_pathspec(&pathspec, 0,
diff --git a/builtin/reset.c b/builtin/reset.c
index 3590be57a5..1ac374d31b 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -285,7 +285,7 @@ static void parse_args(struct pathspec *pathspec,
rev = *argv++;
} else {
/* Otherwise we treat this as a filename */
- verify_filename(prefix, argv[0], 1);
+ verify_filename(the_repository, prefix, argv[0], 1);
}
}
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 2fcd6851d1..8fdb75413d 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -749,7 +749,7 @@ int cmd_rev_parse(int argc,
if (as_is) {
if (show_file(arg, output_prefix) && as_is < 2)
- verify_filename(prefix, arg, 0);
+ verify_filename(the_repository, prefix, arg, 0);
continue;
}
@@ -1173,7 +1173,7 @@ int cmd_rev_parse(int argc,
as_is = 1;
if (!show_file(arg, output_prefix))
continue;
- verify_filename(prefix, arg, 1);
+ verify_filename(the_repository, prefix, arg, 1);
}
strbuf_release(&buf);
if (verify) {
diff --git a/revision.c b/revision.c
index 599b3a66c3..5d53244379 100644
--- a/revision.c
+++ b/revision.c
@@ -3067,7 +3067,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
* but the latter we have checked in the main loop.
*/
for (j = i; j < argc; j++)
- verify_filename(revs->prefix, argv[j], j == i);
+ verify_filename(the_repository, revs->prefix, argv[j], j == i);
strvec_pushv(&prune_data, argv + i);
break;
diff --git a/setup.c b/setup.c
index 4ef6216e82..e673663cab 100644
--- a/setup.c
+++ b/setup.c
@@ -280,7 +280,8 @@ static int looks_like_pathspec(const char *arg)
* diagnose_misspelt_rev == 0 for the next ones (because we already
* saw a filename, there's not ambiguity anymore).
*/
-void verify_filename(const char *prefix,
+void verify_filename(struct repository *repo,
+ const char *prefix,
const char *arg,
int diagnose_misspelt_rev)
{
@@ -288,7 +289,7 @@ void verify_filename(const char *prefix,
die(_("option '%s' must come before non-option arguments"), arg);
if (looks_like_pathspec(arg) || check_filename(prefix, arg))
return;
- die_verify_filename(the_repository, prefix, arg, diagnose_misspelt_rev);
+ die_verify_filename(repo, prefix, arg, diagnose_misspelt_rev);
}
/*
diff --git a/setup.h b/setup.h
index c3247d7fc8..24a6f66629 100644
--- a/setup.h
+++ b/setup.h
@@ -142,7 +142,8 @@ char *prefix_path(struct repository *repo, const char *prefix, int len, const ch
char *prefix_path_gently(struct repository *repo, const char *prefix, int len, int *remaining, const char *path);
int check_filename(const char *prefix, const char *name);
-void verify_filename(const char *prefix,
+void verify_filename(struct repository *repo,
+ const char *prefix,
const char *name,
int diagnose_misspelt_rev);
void verify_non_filename(const char *prefix, const char *name);
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 05/18] setup: stop using `the_repository` in `path_inside_repo()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Stop using `the_repository` in `path_inside_repo()` and instead accept
the repository as a parameter. The injection of `the_repository` is thus
bumped one level higher, where callers now pass it in explicitly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/diff.c | 4 ++--
setup.c | 4 ++--
setup.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index 0b23c41456..7ddebce2ac 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -471,8 +471,8 @@ int cmd_diff(int argc,
* as a colourful "diff" replacement.
*/
if (nongit || ((argc == i + 2) &&
- (!path_inside_repo(prefix, argv[i]) ||
- !path_inside_repo(prefix, argv[i + 1]))))
+ (!path_inside_repo(the_repository, prefix, argv[i]) ||
+ !path_inside_repo(the_repository, prefix, argv[i + 1]))))
no_index = DIFF_NO_INDEX_IMPLICIT;
}
diff --git a/setup.c b/setup.c
index adad6ceec0..4ef6216e82 100644
--- a/setup.c
+++ b/setup.c
@@ -160,10 +160,10 @@ char *prefix_path(struct repository *repo, const char *prefix, int len, const ch
return r;
}
-int path_inside_repo(const char *prefix, const char *path)
+int path_inside_repo(struct repository *repo, const char *prefix, const char *path)
{
int len = prefix ? strlen(prefix) : 0;
- char *r = prefix_path_gently(the_repository, prefix, len, NULL, path);
+ char *r = prefix_path_gently(repo, prefix, len, NULL, path);
if (r) {
free(r);
return 1;
diff --git a/setup.h b/setup.h
index 24034572b1..c3247d7fc8 100644
--- a/setup.h
+++ b/setup.h
@@ -146,7 +146,7 @@ void verify_filename(const char *prefix,
const char *name,
int diagnose_misspelt_rev);
void verify_non_filename(const char *prefix, const char *name);
-int path_inside_repo(const char *prefix, const char *path);
+int path_inside_repo(struct repository *repo, const char *prefix, const char *path);
void sanitize_stdfds(void);
int daemonize(void);
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 04/18] setup: stop using `the_repository` in `prefix_path()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Stop using `the_repository` in `prefix_path()` and instead accept the
repository as a parameter. The injection of `the_repository` is thus
bumped one level higher, where callers now pass it in explicitly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/blame.c | 2 +-
builtin/check-attr.c | 2 +-
builtin/checkout-index.c | 4 ++--
builtin/mv.c | 5 +++--
builtin/sparse-checkout.c | 3 ++-
builtin/update-index.c | 6 +++---
line-log.c | 2 +-
object-name.c | 2 +-
pathspec.c | 2 +-
setup.c | 15 ++++++++-------
setup.h | 4 ++--
t/helper/test-path-utils.c | 2 +-
12 files changed, 26 insertions(+), 23 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index f3a11eff44..ffbd3ce5c5 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -708,7 +708,7 @@ static unsigned parse_score(const char *arg)
static char *add_prefix(const char *prefix, const char *path)
{
- return prefix_path(prefix, prefix ? strlen(prefix) : 0, path);
+ return prefix_path(the_repository, prefix, prefix ? strlen(prefix) : 0, path);
}
static int git_blame_config(const char *var, const char *value,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 51ed48ce43..04b86e42ae 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -67,7 +67,7 @@ static void check_attr(const char *prefix, struct attr_check *check,
{
char *full_path =
- prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
+ prefix_path(the_repository, prefix, prefix ? strlen(prefix) : 0, file);
if (collect_all) {
git_all_attrs(the_repository->index, full_path, check);
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 188128aebd..311b94ff31 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -303,7 +303,7 @@ int cmd_checkout_index(int argc,
die("git checkout-index: don't mix '--all' and explicit filenames");
if (read_from_stdin)
die("git checkout-index: don't mix '--stdin' and explicit filenames");
- p = prefix_path(prefix, prefix_length, arg);
+ p = prefix_path(repo, prefix, prefix_length, arg);
err |= checkout_file(repo->index, p, prefix);
free(p);
}
@@ -325,7 +325,7 @@ int cmd_checkout_index(int argc,
die("line is badly quoted");
strbuf_swap(&buf, &unquoted);
}
- p = prefix_path(prefix, prefix_length, buf.buf);
+ p = prefix_path(repo, prefix, prefix_length, buf.buf);
err |= checkout_file(repo->index, p, prefix);
free(p);
}
diff --git a/builtin/mv.c b/builtin/mv.c
index 2215d34e31..948b330639 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -71,7 +71,7 @@ static void internal_prefix_pathspec(struct strvec *out,
trimmed = xmemdupz(pathspec[i], to_copy);
maybe_basename = (flags & DUP_BASENAME) ? basename(trimmed) : trimmed;
- prefixed_path = prefix_path(prefix, prefixlen, maybe_basename);
+ prefixed_path = prefix_path(the_repository, prefix, prefixlen, maybe_basename);
strvec_push(out, prefixed_path);
free(prefixed_path);
@@ -394,7 +394,8 @@ int cmd_mv(int argc,
for (j = 0; j < last - first; j++) {
const struct cache_entry *ce = the_repository->index->cache[first + j];
const char *path = ce->name;
- char *prefixed_path = prefix_path(dst_with_slash, dst_with_slash_len, path + length + 1);
+ char *prefixed_path = prefix_path(the_repository, dst_with_slash,
+ dst_with_slash_len, path + length + 1);
strvec_push(&sources, path);
strvec_push(&destinations, prefixed_path);
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index f4aa405da9..2af50fb2f9 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -735,7 +735,8 @@ static void sanitize_paths(struct repository *repo,
int prefix_len = strlen(prefix);
for (i = 0; i < args->nr; i++) {
- char *prefixed_path = prefix_path(prefix, prefix_len, args->v[i]);
+ char *prefixed_path = prefix_path(the_repository, prefix,
+ prefix_len, args->v[i]);
strvec_replace(args, i, prefixed_path);
free(prefixed_path);
}
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 8a5907767b..7434112b8e 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -655,7 +655,7 @@ static int do_unresolve(int ac, const char **av,
for (i = 1; i < ac; i++) {
const char *arg = av[i];
- char *p = prefix_path(prefix, prefix_length, arg);
+ char *p = prefix_path(the_repository, prefix, prefix_length, arg);
err |= unresolve_one(p);
free(p);
}
@@ -1158,7 +1158,7 @@ int cmd_update_index(int argc,
}
setup_work_tree();
- p = prefix_path(prefix, prefix_length, path);
+ p = prefix_path(the_repository, prefix, prefix_length, path);
update_one(p);
if (set_executable_bit)
chmod_path(set_executable_bit, p);
@@ -1208,7 +1208,7 @@ int cmd_update_index(int argc,
die("line is badly quoted");
strbuf_swap(&buf, &unquoted);
}
- p = prefix_path(prefix, prefix_length, buf.buf);
+ p = prefix_path(the_repository, prefix, prefix_length, buf.buf);
update_one(p);
if (set_executable_bit)
chmod_path(set_executable_bit, p);
diff --git a/line-log.c b/line-log.c
index 858a899cd2..346c60c554 100644
--- a/line-log.c
+++ b/line-log.c
@@ -589,7 +589,7 @@ parse_lines(struct repository *r, struct commit *commit,
range_part = xstrndup(item->string, name_part - item->string);
name_part++;
- full_name = prefix_path(prefix, prefix ? strlen(prefix) : 0,
+ full_name = prefix_path(r, prefix, prefix ? strlen(prefix) : 0,
name_part);
spec = alloc_filespec(full_name);
diff --git a/object-name.c b/object-name.c
index 37a9ce8e87..9ac86f19c7 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1707,7 +1707,7 @@ static char *resolve_relative_path(struct repository *r, const char *rel)
die(_("relative path syntax can't be used outside working tree"));
/* die() inside prefix_path() if resolved path is outside worktree */
- return prefix_path(startup_info->prefix,
+ return prefix_path(the_repository, startup_info->prefix,
startup_info->prefix ? strlen(startup_info->prefix) : 0,
rel);
}
diff --git a/pathspec.c b/pathspec.c
index 5993c4afa0..f78b22709c 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -486,7 +486,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
match = xstrdup(copyfrom);
prefixlen = 0;
} else {
- match = prefix_path_gently(prefix, prefixlen,
+ match = prefix_path_gently(the_repository, prefix, prefixlen,
&prefixlen, copyfrom);
if (!match) {
const char *hint_path;
diff --git a/setup.c b/setup.c
index 041e08b98d..adad6ceec0 100644
--- a/setup.c
+++ b/setup.c
@@ -117,7 +117,8 @@ static int abspath_part_inside_repo(struct repository *repo, char *path)
* ../../sub1/sub2/foo -> sub1/sub2/foo (but no remaining prefix)
* `pwd`/../bar -> sub1/bar (no remaining prefix)
*/
-char *prefix_path_gently(const char *prefix, int len,
+char *prefix_path_gently(struct repository *repo,
+ const char *prefix, int len,
int *remaining_prefix, const char *path)
{
const char *orig = path;
@@ -130,7 +131,7 @@ char *prefix_path_gently(const char *prefix, int len,
free(sanitized);
return NULL;
}
- if (abspath_part_inside_repo(the_repository, sanitized)) {
+ if (abspath_part_inside_repo(repo, sanitized)) {
free(sanitized);
return NULL;
}
@@ -146,13 +147,13 @@ char *prefix_path_gently(const char *prefix, int len,
return sanitized;
}
-char *prefix_path(const char *prefix, int len, const char *path)
+char *prefix_path(struct repository *repo, const char *prefix, int len, const char *path)
{
- char *r = prefix_path_gently(prefix, len, NULL, path);
+ char *r = prefix_path_gently(repo, prefix, len, NULL, path);
if (!r) {
- const char *hint_path = repo_get_work_tree(the_repository);
+ const char *hint_path = repo_get_work_tree(repo);
if (!hint_path)
- hint_path = repo_get_git_dir(the_repository);
+ hint_path = repo_get_git_dir(repo);
die(_("'%s' is outside repository at '%s'"), path,
absolute_path(hint_path));
}
@@ -162,7 +163,7 @@ char *prefix_path(const char *prefix, int len, const char *path)
int path_inside_repo(const char *prefix, const char *path)
{
int len = prefix ? strlen(prefix) : 0;
- char *r = prefix_path_gently(prefix, len, NULL, path);
+ char *r = prefix_path_gently(the_repository, prefix, len, NULL, path);
if (r) {
free(r);
return 1;
diff --git a/setup.h b/setup.h
index 71d3f91883..24034572b1 100644
--- a/setup.h
+++ b/setup.h
@@ -138,8 +138,8 @@ const char *enter_repo(const char *path, unsigned flags);
const char *setup_git_directory_gently(int *);
const char *setup_git_directory(void);
-char *prefix_path(const char *prefix, int len, const char *path);
-char *prefix_path_gently(const char *prefix, int len, int *remaining, const char *path);
+char *prefix_path(struct repository *repo, const char *prefix, int len, const char *path);
+char *prefix_path_gently(struct repository *repo, const char *prefix, int len, int *remaining, const char *path);
int check_filename(const char *prefix, const char *name);
void verify_filename(const char *prefix,
diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index 874542ec34..163fdeefb0 100644
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -379,7 +379,7 @@ int cmd__path_utils(int argc, const char **argv)
int nongit_ok;
setup_git_directory_gently(&nongit_ok);
while (argc > 3) {
- char *pfx = prefix_path(prefix, prefix_len, argv[3]);
+ char *pfx = prefix_path(the_repository, prefix, prefix_len, argv[3]);
puts(pfx);
free(pfx);
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 03/18] setup: stop using `the_repository` in `is_inside_work_tree()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Similar as with the preceding commit, `is_inside_work_tree()` determines
whether the current working directory is located inside the worktree of
`the_repository`. Perform the same refactoring by dropping the caching
mechanism and injecting the repository that shall be checked.
Note that, same as in the preceding commit, we're also resolving the
worktree path via `realpath()`. In theory this step is not necessary as
we always set the worktree path via `repo_set_worktree()`, and that
function already resolves the path for us. But resolving the path a
second time is unlikely to matter performance-wise, and it feels fragile
to rely on the repository's worktree path being absolute. We thus
perform the same extra step even though it's ultimately not required.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/ls-files.c | 2 +-
builtin/rev-parse.c | 4 ++--
object-name.c | 2 +-
setup.c | 25 ++++++++++++++-----------
setup.h | 2 +-
submodule.c | 2 +-
6 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b148607f7a..09d95111b3 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -703,7 +703,7 @@ int cmd_ls_files(int argc,
if (dir.exclude_per_dir)
exc_given = 1;
- if (require_work_tree && !is_inside_work_tree())
+ if (require_work_tree && !is_inside_work_tree(repo))
setup_work_tree();
if (recurse_submodules &&
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a216be63cf..2fcd6851d1 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -1006,7 +1006,7 @@ int cmd_rev_parse(int argc,
}
if (!strcmp(arg, "--show-cdup")) {
const char *pfx = prefix;
- if (!is_inside_work_tree()) {
+ if (!is_inside_work_tree(the_repository)) {
const char *work_tree =
repo_get_work_tree(the_repository);
if (work_tree)
@@ -1068,7 +1068,7 @@ int cmd_rev_parse(int argc,
continue;
}
if (!strcmp(arg, "--is-inside-work-tree")) {
- printf("%s\n", is_inside_work_tree() ? "true"
+ printf("%s\n", is_inside_work_tree(the_repository) ? "true"
: "false");
continue;
}
diff --git a/object-name.c b/object-name.c
index 21dcdc4a0e..37a9ce8e87 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1703,7 +1703,7 @@ static char *resolve_relative_path(struct repository *r, const char *rel)
if (!starts_with(rel, "./") && !starts_with(rel, "../"))
return NULL;
- if (r != the_repository || !is_inside_work_tree())
+ if (r != the_repository || !is_inside_work_tree(the_repository))
die(_("relative path syntax can't be used outside working tree"));
/* die() inside prefix_path() if resolved path is outside worktree */
diff --git a/setup.c b/setup.c
index 80f3ba0d62..041e08b98d 100644
--- a/setup.c
+++ b/setup.c
@@ -26,7 +26,6 @@
#include "trace2.h"
#include "worktree.h"
-static int inside_work_tree = -1;
static int work_tree_config_is_bogus;
enum allowed_bare_repo {
ALLOWED_BARE_REPO_EXPLICIT = 0,
@@ -298,7 +297,7 @@ void verify_filename(const char *prefix,
*/
void verify_non_filename(const char *prefix, const char *arg)
{
- if (!is_inside_work_tree() || is_inside_git_dir(the_repository))
+ if (!is_inside_work_tree(the_repository) || is_inside_git_dir(the_repository))
return;
if (*arg == '-')
return; /* flag */
@@ -477,11 +476,20 @@ int is_inside_git_dir(struct repository *repo)
return ret;
}
-int is_inside_work_tree(void)
+int is_inside_work_tree(struct repository *repo)
{
- if (inside_work_tree < 0)
- inside_work_tree = is_inside_dir(repo_get_work_tree(the_repository));
- return inside_work_tree;
+ struct strbuf buf = STRBUF_INIT;
+ const char *worktree;
+ int ret;
+
+ worktree = repo_get_work_tree(repo);
+ if (!worktree)
+ return 0;
+
+ ret = is_inside_dir(strbuf_realpath(&buf, worktree, 1));
+
+ strbuf_release(&buf);
+ return ret;
}
void setup_work_tree(void)
@@ -798,13 +806,10 @@ static int check_repository_format_gently(struct repository *repo,
if (!has_common) {
if (candidate->is_bare != -1) {
is_bare_repository_cfg = candidate->is_bare;
- if (is_bare_repository_cfg == 1)
- inside_work_tree = -1;
}
if (candidate->work_tree) {
free(git_work_tree_cfg);
git_work_tree_cfg = xstrdup(candidate->work_tree);
- inside_work_tree = -1;
}
}
@@ -1251,7 +1256,6 @@ static const char *setup_discovered_git_dir(struct repository *repo,
set_git_work_tree(".");
if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
set_git_dir(repo, gitdir, 0);
- inside_work_tree = 1;
if (offset >= cwd->len)
return NULL;
@@ -1286,7 +1290,6 @@ static const char *setup_bare_git_dir(struct repository *repo,
return setup_explicit_git_dir(repo, gitdir, cwd, repo_fmt, nongit_ok);
}
- inside_work_tree = 0;
if (offset != cwd->len) {
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
diff --git a/setup.h b/setup.h
index 115bda647c..71d3f91883 100644
--- a/setup.h
+++ b/setup.h
@@ -5,7 +5,7 @@
#include "string-list.h"
int is_inside_git_dir(struct repository *repo);
-int is_inside_work_tree(void);
+int is_inside_work_tree(struct repository *repo);
int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
int get_common_dir(struct strbuf *sb, const char *gitdir);
diff --git a/submodule.c b/submodule.c
index b1a0363f9d..a939ff5072 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2620,7 +2620,7 @@ int get_superproject_working_tree(struct strbuf *buf)
int code;
ssize_t len;
- if (!is_inside_work_tree())
+ if (!is_inside_work_tree(the_repository))
/*
* FIXME:
* We might have a superproject, but it is harder
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 02/18] setup: stop using `the_repository` in `is_inside_git_dir()`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
The function `is_inside_git_dir()` verifies whether or not the current
working directory is located inside the gitdir of `the_repository`. This
is done by taking the gitdir path and verifying that it's a prefix of
the current working directory.
This information is cached so that we don't have to re-do this change
multiple times. Furthermore, we proactively set the value in multiple
locations so that we don't even have to perform the check when we have
discovered the repository.
While we could simply move the caching variable into the repository, the
current layout doesn't really feel sensible in the first place:
- It can easily lead to false positives or negatives if at any point
in time we may switch the current working directory.
- We don't call the function in a hot loop, and neither is it overly
expensive to compute.
Drop the caching infrastructure and instead compute the property ad-hoc
via an injected repository.
Note that there is one small gotcha: we often end up with relative
gitdir paths, and if so `is_inside_dir()` might fail. This wasn't an
issue before because of how we proactively set the cached value during
repository discovery. Now that we stop doing that it becomes a problem
though, which we work around by resolving the gitdir via `realpath()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/rev-parse.c | 2 +-
setup.c | 14 ++++++--------
setup.h | 2 +-
3 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 218b5f34d6..a216be63cf 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -1063,7 +1063,7 @@ int cmd_rev_parse(int argc,
continue;
}
if (!strcmp(arg, "--is-inside-git-dir")) {
- printf("%s\n", is_inside_git_dir() ? "true"
+ printf("%s\n", is_inside_git_dir(the_repository) ? "true"
: "false");
continue;
}
diff --git a/setup.c b/setup.c
index ba2898473a..80f3ba0d62 100644
--- a/setup.c
+++ b/setup.c
@@ -26,7 +26,6 @@
#include "trace2.h"
#include "worktree.h"
-static int inside_git_dir = -1;
static int inside_work_tree = -1;
static int work_tree_config_is_bogus;
enum allowed_bare_repo {
@@ -299,7 +298,7 @@ void verify_filename(const char *prefix,
*/
void verify_non_filename(const char *prefix, const char *arg)
{
- if (!is_inside_work_tree() || is_inside_git_dir())
+ if (!is_inside_work_tree() || is_inside_git_dir(the_repository))
return;
if (*arg == '-')
return; /* flag */
@@ -470,11 +469,12 @@ int is_nonbare_repository_dir(struct strbuf *path)
return ret;
}
-int is_inside_git_dir(void)
+int is_inside_git_dir(struct repository *repo)
{
- if (inside_git_dir < 0)
- inside_git_dir = is_inside_dir(repo_get_git_dir(the_repository));
- return inside_git_dir;
+ struct strbuf buf = STRBUF_INIT;
+ int ret = is_inside_dir(strbuf_realpath(&buf, repo_get_git_dir(repo), 1));
+ strbuf_release(&buf);
+ return ret;
}
int is_inside_work_tree(void)
@@ -1251,7 +1251,6 @@ static const char *setup_discovered_git_dir(struct repository *repo,
set_git_work_tree(".");
if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
set_git_dir(repo, gitdir, 0);
- inside_git_dir = 0;
inside_work_tree = 1;
if (offset >= cwd->len)
return NULL;
@@ -1287,7 +1286,6 @@ static const char *setup_bare_git_dir(struct repository *repo,
return setup_explicit_git_dir(repo, gitdir, cwd, repo_fmt, nongit_ok);
}
- inside_git_dir = 1;
inside_work_tree = 0;
if (offset != cwd->len) {
if (chdir(cwd->buf))
diff --git a/setup.h b/setup.h
index 80bc6e5f07..115bda647c 100644
--- a/setup.h
+++ b/setup.h
@@ -4,7 +4,7 @@
#include "refs.h"
#include "string-list.h"
-int is_inside_git_dir(void);
+int is_inside_git_dir(struct repository *repo);
int is_inside_work_tree(void);
int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
int get_common_dir(struct strbuf *sb, const char *gitdir);
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 01/18] setup: replace use of `the_repository` in static functions
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260519-pks-setup-wo-the-repository-v3-0-a00d8ea8b07f@pks.im>
Replace the use of `the_repository` in "setup.c" for all static
functions. For now, we simply add `the_repository` to invocations of
these functions. This will be addressed in subsequent commits, where
we'll move up `the_repository` one more layer to callers of "setup.c".
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 188 ++++++++++++++++++++++++++++++++++------------------------------
1 file changed, 100 insertions(+), 88 deletions(-)
diff --git a/setup.c b/setup.c
index 7ec4427368..ba2898473a 100644
--- a/setup.c
+++ b/setup.c
@@ -50,13 +50,13 @@ const char *tmp_original_cwd;
* /dir/repolink/file (repolink points to /dir/repo) -> file
* /dir/repo (exactly equal to work tree) -> (empty string)
*/
-static int abspath_part_inside_repo(char *path)
+static int abspath_part_inside_repo(struct repository *repo, char *path)
{
size_t len;
size_t wtlen;
char *path0;
int off;
- const char *work_tree = precompose_string_if_needed(repo_get_work_tree(the_repository));
+ const char *work_tree = precompose_string_if_needed(repo_get_work_tree(repo));
struct strbuf realpath = STRBUF_INIT;
if (!work_tree)
@@ -132,7 +132,7 @@ char *prefix_path_gently(const char *prefix, int len,
free(sanitized);
return NULL;
}
- if (abspath_part_inside_repo(sanitized)) {
+ if (abspath_part_inside_repo(the_repository, sanitized)) {
free(sanitized);
return NULL;
}
@@ -509,7 +509,7 @@ void setup_work_tree(void)
initialized = 1;
}
-static void setup_original_cwd(void)
+static void setup_original_cwd(struct repository *repo)
{
struct strbuf tmp = STRBUF_INIT;
const char *worktree = NULL;
@@ -535,9 +535,9 @@ static void setup_original_cwd(void)
/* Normalize the directory */
if (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
- trace2_data_string("setup", the_repository,
+ trace2_data_string("setup", repo,
"realpath-path", tmp_original_cwd);
- trace2_data_string("setup", the_repository,
+ trace2_data_string("setup", repo,
"realpath-failure", strerror(errno));
free((char*)tmp_original_cwd);
tmp_original_cwd = NULL;
@@ -552,7 +552,7 @@ static void setup_original_cwd(void)
* Get our worktree; we only protect the current working directory
* if it's in the worktree.
*/
- worktree = repo_get_work_tree(the_repository);
+ worktree = repo_get_work_tree(repo);
if (!worktree)
goto no_prevention_needed;
@@ -747,7 +747,10 @@ static int check_repo_format(const char *var, const char *value,
return read_worktree_config(var, value, ctx, vdata);
}
-static int check_repository_format_gently(const char *gitdir, struct repository_format *candidate, int *nongit_ok)
+static int check_repository_format_gently(struct repository *repo,
+ const char *gitdir,
+ struct repository_format *candidate,
+ int *nongit_ok)
{
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
@@ -776,7 +779,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
die("%s", err.buf);
}
- the_repository->repository_format_precious_objects = candidate->precious_objects;
+ repo->repository_format_precious_objects = candidate->precious_objects;
string_list_clear(&candidate->unknown_extensions, 0);
string_list_clear(&candidate->v1_only_extensions, 0);
@@ -1034,7 +1037,8 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
return error_code ? NULL : path;
}
-static void setup_git_env_internal(const char *git_dir,
+static void setup_git_env_internal(struct repository *repo,
+ const char *git_dir,
bool skip_initializing_odb)
{
char *git_replace_ref_base;
@@ -1052,7 +1056,7 @@ static void setup_git_env_internal(const char *git_dir,
args.disable_ref_updates = true;
args.skip_initializing_odb = skip_initializing_odb;
- repo_set_gitdir(the_repository, git_dir, &args);
+ repo_set_gitdir(repo, git_dir, &args);
strvec_clear(&to_free);
if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
@@ -1064,7 +1068,7 @@ static void setup_git_env_internal(const char *git_dir,
shallow_file = getenv(GIT_SHALLOW_FILE_ENVIRONMENT);
if (shallow_file)
- set_alternate_shallow_file(the_repository, shallow_file, 0);
+ set_alternate_shallow_file(repo, shallow_file, 0);
if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0))
fetch_if_missing = 0;
@@ -1072,30 +1076,31 @@ static void setup_git_env_internal(const char *git_dir,
void setup_git_env(const char *git_dir)
{
- setup_git_env_internal(git_dir, false);
+ setup_git_env_internal(the_repository, git_dir, false);
}
-static void set_git_dir_1(const char *path, bool skip_initializing_odb)
+static void set_git_dir_1(struct repository *repo, const char *path, bool skip_initializing_odb)
{
xsetenv(GIT_DIR_ENVIRONMENT, path, 1);
- setup_git_env_internal(path, skip_initializing_odb);
+ setup_git_env_internal(repo, path, skip_initializing_odb);
}
static void update_relative_gitdir(const char *name UNUSED,
const char *old_cwd,
const char *new_cwd,
- void *data UNUSED)
+ void *data)
{
+ struct repository *repo = data;
char *path = reparent_relative_path(old_cwd, new_cwd,
- repo_get_git_dir(the_repository));
+ repo_get_git_dir(repo));
trace_printf_key(&trace_setup_key,
"setup: move $GIT_DIR to '%s'",
path);
- set_git_dir_1(path, true);
+ set_git_dir_1(repo, path, true);
free(path);
}
-static void set_git_dir(const char *path, int make_realpath)
+static void set_git_dir(struct repository *repo, const char *path, int make_realpath)
{
struct strbuf realpath = STRBUF_INIT;
@@ -1104,14 +1109,15 @@ static void set_git_dir(const char *path, int make_realpath)
path = realpath.buf;
}
- set_git_dir_1(path, false);
+ set_git_dir_1(repo, path, false);
if (!is_absolute_path(path))
- chdir_notify_register(NULL, update_relative_gitdir, NULL);
+ chdir_notify_register(NULL, update_relative_gitdir, repo);
strbuf_release(&realpath);
}
-static const char *setup_explicit_git_dir(const char *gitdirenv,
+static const char *setup_explicit_git_dir(struct repository *repo,
+ const char *gitdirenv,
struct strbuf *cwd,
struct repository_format *repo_fmt,
int *nongit_ok)
@@ -1139,7 +1145,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
die(_("not a git repository: '%s'"), gitdirenv);
}
- if (check_repository_format_gently(gitdirenv, repo_fmt, nongit_ok)) {
+ if (check_repository_format_gently(repo, gitdirenv, repo_fmt, nongit_ok)) {
free(gitfile);
return NULL;
}
@@ -1155,7 +1161,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
}
/* #18, #26 */
- set_git_dir(gitdirenv, 0);
+ set_git_dir(repo, gitdirenv, 0);
free(gitfile);
return NULL;
}
@@ -1177,7 +1183,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
}
else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
/* #16d */
- set_git_dir(gitdirenv, 0);
+ set_git_dir(repo, gitdirenv, 0);
free(gitfile);
return NULL;
}
@@ -1185,18 +1191,18 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
set_git_work_tree(".");
/* set_git_work_tree() must have been called by now */
- worktree = repo_get_work_tree(the_repository);
+ worktree = repo_get_work_tree(repo);
/* both repo_get_work_tree() and cwd are already normalized */
if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */
- set_git_dir(gitdirenv, 0);
+ set_git_dir(repo, gitdirenv, 0);
free(gitfile);
return NULL;
}
offset = dir_inside_of(cwd->buf, worktree);
if (offset >= 0) { /* cwd inside worktree? */
- set_git_dir(gitdirenv, 1);
+ set_git_dir(repo, gitdirenv, 1);
if (chdir(worktree))
die_errno(_("cannot chdir to '%s'"), worktree);
strbuf_addch(cwd, '/');
@@ -1205,17 +1211,18 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
}
/* cwd outside worktree */
- set_git_dir(gitdirenv, 0);
+ set_git_dir(repo, gitdirenv, 0);
free(gitfile);
return NULL;
}
-static const char *setup_discovered_git_dir(const char *gitdir,
+static const char *setup_discovered_git_dir(struct repository *repo,
+ const char *gitdir,
struct strbuf *cwd, int offset,
struct repository_format *repo_fmt,
int *nongit_ok)
{
- if (check_repository_format_gently(gitdir, repo_fmt, nongit_ok))
+ if (check_repository_format_gently(repo, gitdir, repo_fmt, nongit_ok))
return NULL;
/* --work-tree is set without --git-dir; use discovered one */
@@ -1227,14 +1234,14 @@ static const char *setup_discovered_git_dir(const char *gitdir,
gitdir = to_free = real_pathdup(gitdir, 1);
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
- ret = setup_explicit_git_dir(gitdir, cwd, repo_fmt, nongit_ok);
+ ret = setup_explicit_git_dir(repo, gitdir, cwd, repo_fmt, nongit_ok);
free(to_free);
return ret;
}
/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
if (is_bare_repository_cfg > 0) {
- set_git_dir(gitdir, (offset != cwd->len));
+ set_git_dir(repo, gitdir, (offset != cwd->len));
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
return NULL;
@@ -1243,7 +1250,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
/* #0, #1, #5, #8, #9, #12, #13 */
set_git_work_tree(".");
if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
- set_git_dir(gitdir, 0);
+ set_git_dir(repo, gitdir, 0);
inside_git_dir = 0;
inside_work_tree = 1;
if (offset >= cwd->len)
@@ -1258,13 +1265,14 @@ static const char *setup_discovered_git_dir(const char *gitdir,
}
/* #16.1, #17.1, #20.1, #21.1, #22.1 (see t1510) */
-static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
+static const char *setup_bare_git_dir(struct repository *repo,
+ struct strbuf *cwd, int offset,
struct repository_format *repo_fmt,
int *nongit_ok)
{
int root_len;
- if (check_repository_format_gently(".", repo_fmt, nongit_ok))
+ if (check_repository_format_gently(repo, ".", repo_fmt, nongit_ok))
return NULL;
setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
@@ -1276,7 +1284,7 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset);
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
- return setup_explicit_git_dir(gitdir, cwd, repo_fmt, nongit_ok);
+ return setup_explicit_git_dir(repo, gitdir, cwd, repo_fmt, nongit_ok);
}
inside_git_dir = 1;
@@ -1286,10 +1294,10 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
die_errno(_("cannot come back to cwd"));
root_len = offset_1st_component(cwd->buf);
strbuf_setlen(cwd, offset > root_len ? offset : root_len);
- set_git_dir(cwd->buf, 0);
+ set_git_dir(repo, cwd->buf, 0);
}
else
- set_git_dir(".", 0);
+ set_git_dir(repo, ".", 0);
return NULL;
}
@@ -1827,7 +1835,7 @@ const char *enter_repo(const char *path, unsigned flags)
}
if (is_git_directory(".")) {
- set_git_dir(".", 0);
+ set_git_dir(the_repository, ".", 0);
check_repository_format(NULL);
return path;
}
@@ -1891,18 +1899,18 @@ const char *setup_git_directory_gently(int *nongit_ok)
switch (setup_git_directory_gently_1(&dir, &gitdir, &report, 1)) {
case GIT_DIR_EXPLICIT:
- prefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);
+ prefix = setup_explicit_git_dir(the_repository, gitdir.buf, &cwd, &repo_fmt, nongit_ok);
break;
case GIT_DIR_DISCOVERED:
if (dir.len < cwd.len && chdir(dir.buf))
die(_("cannot change to '%s'"), dir.buf);
- prefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,
+ prefix = setup_discovered_git_dir(the_repository, gitdir.buf, &cwd, dir.len,
&repo_fmt, nongit_ok);
break;
case GIT_DIR_BARE:
if (dir.len < cwd.len && chdir(dir.buf))
die(_("cannot change to '%s'"), dir.buf);
- prefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);
+ prefix = setup_bare_git_dir(the_repository, &cwd, dir.len, &repo_fmt, nongit_ok);
break;
case GIT_DIR_HIT_CEILING:
if (!nongit_ok)
@@ -2044,7 +2052,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
free(payload);
}
- setup_original_cwd();
+ setup_original_cwd(the_repository);
strbuf_release(&dir);
strbuf_release(&gitdir);
@@ -2110,7 +2118,7 @@ void check_repository_format(struct repository_format *fmt)
struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
if (!fmt)
fmt = &repo_fmt;
- check_repository_format_gently(repo_get_git_dir(the_repository), fmt, NULL);
+ check_repository_format_gently(the_repository, repo_get_git_dir(the_repository), fmt, NULL);
startup_info->have_repository = 1;
repo_set_hash_algo(the_repository, fmt->hash_algo);
repo_set_compat_hash_algo(the_repository, fmt->compat_hash_algo);
@@ -2239,7 +2247,9 @@ const char *get_template_dir(const char *option_template)
#define GIT_DEFAULT_HASH_ENVIRONMENT "GIT_DEFAULT_HASH"
-static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
+static void copy_templates_1(struct repository *repo,
+ struct strbuf *path,
+ struct strbuf *template_path,
DIR *dir)
{
size_t path_baselen = path->len;
@@ -2253,7 +2263,7 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
* with the way the namespace under .git/ is organized, should
* be really carefully chosen.
*/
- safe_create_dir(the_repository, path->buf, 1);
+ safe_create_dir(repo, path->buf, 1);
while ((de = readdir(dir)) != NULL) {
struct stat st_git, st_template;
int exists = 0;
@@ -2281,7 +2291,7 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
die_errno(_("cannot opendir '%s'"), template_path->buf);
strbuf_addch(path, '/');
strbuf_addch(template_path, '/');
- copy_templates_1(path, template_path, subdir);
+ copy_templates_1(repo, path, template_path, subdir);
closedir(subdir);
}
else if (exists)
@@ -2306,7 +2316,7 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
}
}
-static void copy_templates(const char *option_template)
+static void copy_templates(struct repository *repo, const char *option_template)
{
const char *template_dir = get_template_dir(option_template);
struct strbuf path = STRBUF_INIT;
@@ -2347,9 +2357,9 @@ static void copy_templates(const char *option_template)
goto close_free_return;
}
- strbuf_addstr(&path, repo_get_common_dir(the_repository));
+ strbuf_addstr(&path, repo_get_common_dir(repo));
strbuf_complete(&path, '/');
- copy_templates_1(&path, &template_path, dir);
+ copy_templates_1(repo, &path, &template_path, dir);
close_free_return:
closedir(dir);
free_return:
@@ -2443,13 +2453,13 @@ void initialize_repository_version(int hash_algo,
strbuf_release(&repo_version);
}
-static int is_reinit(void)
+static int is_reinit(struct repository *repo)
{
struct strbuf buf = STRBUF_INIT;
char junk[2];
int ret;
- repo_git_path_replace(the_repository, &buf, "HEAD");
+ repo_git_path_replace(repo, &buf, "HEAD");
ret = !access(buf.buf, R_OK) || readlink(buf.buf, junk, sizeof(junk) - 1) != -1;
strbuf_release(&buf);
return ret;
@@ -2459,7 +2469,7 @@ void create_reference_database(const char *initial_branch, int quiet)
{
struct strbuf err = STRBUF_INIT;
char *to_free = NULL;
- int reinit = is_reinit();
+ int reinit = is_reinit(the_repository);
if (ref_store_create_on_disk(get_main_ref_store(the_repository), 0, &err))
die("failed to set up refs db: %s", err.buf);
@@ -2493,7 +2503,8 @@ void create_reference_database(const char *initial_branch, int quiet)
free(to_free);
}
-static int create_default_files(const char *template_path,
+static int create_default_files(struct repository *repo,
+ const char *template_path,
const char *original_git_dir,
const struct repository_format *fmt,
int init_shared_repository)
@@ -2502,7 +2513,7 @@ static int create_default_files(const char *template_path,
struct strbuf path = STRBUF_INIT;
int reinit;
int filemode;
- const char *work_tree = repo_get_work_tree(the_repository);
+ const char *work_tree = repo_get_work_tree(repo);
/*
* First copy the templates -- we might have the default
@@ -2513,19 +2524,19 @@ static int create_default_files(const char *template_path,
* values (since we've just potentially changed what's available on
* disk).
*/
- copy_templates(template_path);
- repo_config_clear(the_repository);
- repo_settings_reset_shared_repository(the_repository);
- repo_config(the_repository, git_default_config, NULL);
+ copy_templates(repo, template_path);
+ repo_config_clear(repo);
+ repo_settings_reset_shared_repository(repo);
+ repo_config(repo, git_default_config, NULL);
- reinit = is_reinit();
+ reinit = is_reinit(repo);
/*
* We must make sure command-line options continue to override any
* values we might have just re-read from the config.
*/
if (init_shared_repository != -1)
- repo_settings_set_shared_repository(the_repository,
+ repo_settings_set_shared_repository(repo,
init_shared_repository);
is_bare_repository_cfg = !work_tree;
@@ -2534,14 +2545,14 @@ static int create_default_files(const char *template_path,
* We would have created the above under user's umask -- under
* shared-repository settings, we would need to fix them up.
*/
- if (repo_settings_get_shared_repository(the_repository)) {
- adjust_shared_perm(the_repository, repo_get_git_dir(the_repository));
+ if (repo_settings_get_shared_repository(repo)) {
+ adjust_shared_perm(repo, repo_get_git_dir(repo));
}
initialize_repository_version(fmt->hash_algo, fmt->ref_storage_format, reinit);
/* Check filemode trustability */
- repo_git_path_replace(the_repository, &path, "config");
+ repo_git_path_replace(repo, &path, "config");
filemode = TEST_FILEMODE;
if (TEST_FILEMODE && !lstat(path.buf, &st1)) {
struct stat st2;
@@ -2552,22 +2563,22 @@ static int create_default_files(const char *template_path,
if (filemode && !reinit && (st1.st_mode & S_IXUSR))
filemode = 0;
}
- repo_config_set(the_repository, "core.filemode", filemode ? "true" : "false");
+ repo_config_set(repo, "core.filemode", filemode ? "true" : "false");
if (is_bare_repository())
- repo_config_set(the_repository, "core.bare", "true");
+ repo_config_set(repo, "core.bare", "true");
else {
- repo_config_set(the_repository, "core.bare", "false");
+ repo_config_set(repo, "core.bare", "false");
/* allow template config file to override the default */
- if (repo_settings_get_log_all_ref_updates(the_repository) == LOG_REFS_UNSET)
- repo_config_set(the_repository, "core.logallrefupdates", "true");
+ if (repo_settings_get_log_all_ref_updates(repo) == LOG_REFS_UNSET)
+ repo_config_set(repo, "core.logallrefupdates", "true");
if (needs_work_tree_config(original_git_dir, work_tree))
- repo_config_set(the_repository, "core.worktree", work_tree);
+ repo_config_set(repo, "core.worktree", work_tree);
}
if (!reinit) {
/* Check if symlink is supported in the work tree */
- repo_git_path_replace(the_repository, &path, "tXXXXXX");
+ repo_git_path_replace(repo, &path, "tXXXXXX");
if (!close(xmkstemp(path.buf)) &&
!unlink(path.buf) &&
!symlink("testing", path.buf) &&
@@ -2575,12 +2586,12 @@ static int create_default_files(const char *template_path,
S_ISLNK(st1.st_mode))
unlink(path.buf); /* good */
else
- repo_config_set(the_repository, "core.symlinks", "false");
+ repo_config_set(repo, "core.symlinks", "false");
/* Check if the filesystem is case-insensitive */
- repo_git_path_replace(the_repository, &path, "CoNfIg");
+ repo_git_path_replace(repo, &path, "CoNfIg");
if (!access(path.buf, F_OK))
- repo_config_set(the_repository, "core.ignorecase", "true");
+ repo_config_set(repo, "core.ignorecase", "true");
probe_utf8_pathname_composition();
}
@@ -2588,23 +2599,23 @@ static int create_default_files(const char *template_path,
return reinit;
}
-static void create_object_directory(void)
+static void create_object_directory(struct repository *repo)
{
struct strbuf path = STRBUF_INIT;
size_t baselen;
- strbuf_addstr(&path, repo_get_object_directory(the_repository));
+ strbuf_addstr(&path, repo_get_object_directory(repo));
baselen = path.len;
- safe_create_dir(the_repository, path.buf, 1);
+ safe_create_dir(repo, path.buf, 1);
strbuf_setlen(&path, baselen);
strbuf_addstr(&path, "/pack");
- safe_create_dir(the_repository, path.buf, 1);
+ safe_create_dir(repo, path.buf, 1);
strbuf_setlen(&path, baselen);
strbuf_addstr(&path, "/info");
- safe_create_dir(the_repository, path.buf, 1);
+ safe_create_dir(repo, path.buf, 1);
strbuf_release(&path);
}
@@ -2682,7 +2693,8 @@ static int read_default_format_config(const char *key, const char *value,
return ret;
}
-static void repository_format_configure(struct repository_format *repo_fmt,
+static void repository_format_configure(struct repository *repo,
+ struct repository_format *repo_fmt,
int hash, enum ref_storage_format ref_format)
{
struct default_format_config cfg = {
@@ -2719,7 +2731,7 @@ static void repository_format_configure(struct repository_format *repo_fmt,
} else if (cfg.hash != GIT_HASH_UNKNOWN) {
repo_fmt->hash_algo = cfg.hash;
}
- repo_set_hash_algo(the_repository, repo_fmt->hash_algo);
+ repo_set_hash_algo(repo, repo_fmt->hash_algo);
env = getenv("GIT_DEFAULT_REF_FORMAT");
if (repo_fmt->version >= 0 &&
@@ -2758,7 +2770,7 @@ static void repository_format_configure(struct repository_format *repo_fmt,
free(backend);
}
- repo_set_ref_storage_format(the_repository, repo_fmt->ref_storage_format,
+ repo_set_ref_storage_format(repo, repo_fmt->ref_storage_format,
repo_fmt->ref_storage_payload);
}
@@ -2782,12 +2794,12 @@ int init_db(const char *git_dir, const char *real_git_dir,
if (!exist_ok && !stat(real_git_dir, &st))
die(_("%s already exists"), real_git_dir);
- set_git_dir(real_git_dir, 1);
+ set_git_dir(the_repository, real_git_dir, 1);
git_dir = repo_get_git_dir(the_repository);
separate_git_dir(git_dir, original_git_dir);
}
else {
- set_git_dir(git_dir, 1);
+ set_git_dir(the_repository, git_dir, 1);
git_dir = repo_get_git_dir(the_repository);
}
startup_info->have_repository = 1;
@@ -2800,7 +2812,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
*/
check_repository_format(&repo_fmt);
- repository_format_configure(&repo_fmt, hash, ref_storage_format);
+ repository_format_configure(the_repository, &repo_fmt, hash, ref_storage_format);
/*
* Ensure `core.hidedotfiles` is processed. This must happen after we
@@ -2811,12 +2823,12 @@ int init_db(const char *git_dir, const char *real_git_dir,
safe_create_dir(the_repository, git_dir, 0);
- reinit = create_default_files(template_dir, original_git_dir,
+ reinit = create_default_files(the_repository, template_dir, original_git_dir,
&repo_fmt, init_shared_repository);
if (!(flags & INIT_DB_SKIP_REFDB))
create_reference_database(initial_branch, flags & INIT_DB_QUIET);
- create_object_directory();
+ create_object_directory(the_repository);
if (repo_settings_get_shared_repository(the_repository)) {
char buf[10];
--
2.54.0.771.g3ed373ac14.dirty
^ permalink raw reply related
* [PATCH v3 00/18] setup: drop uses of `the_repository`
From: Patrick Steinhardt @ 2026-05-19 9:52 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260420-pks-setup-wo-the-repository-v1-0-f4a81c4988e8@pks.im>
Hi,
I've had enough of "setup.c" and its complexities, so I finally decided
to take the bullet and start refactoring this subsystem. This here is
the first out of the following three steps:
1. Drop all uses of `the_repository`. This doesn't yet allow us to get
rid of `USE_THE_REPOSITORY_VARIABLE`.
2. Convert a couple of global variables and drop
`is_bare_repository_cfg`, which then allows us to drop
`USE_THE_REPOSITORY_VARIABLE`.
3. Refactor the subsystem a bit so that we stop intermixing repository
discovery and repository initialization. This is my original
motivation as I want to get rid of `odb_prepare_alternates()`, but
due to the way we initialize the repository it has proven to be
extremely tedious.
Most of the patches in this series here are rather mechanical. There's
only a handful of patches that warrant more attention:
- 2/18: setup: stop using `the_repository` in `is_inside_worktree()`
- 3/18: setup: stop using `the_repository` in `is_inside_git_dir()`
- 9/18: setup: stop using `the_repository` in `setup_work_tree()`
- 10/18: setup: stop using `the_repository` in `set_git_work_tree()`
Those patches don't only mechanical move stuff around, but also change
some logic to make it work.
Changes in v3:
- Reverse the order of the commits that refactor `is_inside_gitdir()`
and `is_inside_work_tree()` and clarify the logic around why we do
(or do not) have to use realpath(3p). The code is ultimately not
changed though, we still resolve the realpath for both even though
it's not strictly necessary to do so for the working tree.
- Link to v2: https://patch.msgid.link/20260518-pks-setup-wo-the-repository-v2-0-6933c0f1d568@pks.im
Changes in v2:
- Drop some static variables.
- Rebase on top of origin/master at 59ff4886a5 (Merge branch
'ps/clang-w-glibc-2.43-and-_Generic', 2026-05-13). This resolves a
single conflict, but more importantly the rebase fixes CI.
- Link to v1: https://patch.msgid.link/20260420-pks-setup-wo-the-repository-v1-0-f4a81c4988e8@pks.im
Changes in v1 (relative to the botched-up [1]):
- Remove static `initialized` variable in `setup_work_tree()`.
- Use enum values to initialize fields.
- Fix up a comment.
- Link to v1: https://lore.kernel.org/all/20260330-pks-setup-wo-the-repository-v1-0-0d2e822837aa@pks.im/
Thanks!
Patrick
[1]: <20260330-pks-setup-wo-the-repository-v1-0-0d2e822837aa@pks.im>
---
Patrick Steinhardt (18):
setup: replace use of `the_repository` in static functions
setup: stop using `the_repository` in `is_inside_git_dir()`
setup: stop using `the_repository` in `is_inside_work_tree()`
setup: stop using `the_repository` in `prefix_path()`
setup: stop using `the_repository` in `path_inside_repo()`
setup: stop using `the_repository` in `verify_filename()`
setup: stop using `the_repository` in `verify_non_filename()`
setup: stop using `the_repository` in `enter_repo()`
setup: stop using `the_repository` in `setup_work_tree()`
setup: stop using `the_repository` in `set_git_work_tree()`
setup: stop using `the_repository` in `setup_git_env()`
setup: stop using `the_repository` in `setup_git_directory_gently()`
setup: stop using `the_repository` in `setup_git_directory()`
setup: stop using `the_repository` in `upgrade_repository_format()`
setup: stop using `the_repository` in `check_repository_format()`
setup: stop using `the_repository` in `initialize_repository_version()`
setup: stop using `the_repository` in `create_reference_database()`
setup: stop using `the_repository` in `init_db()`
archive.c | 2 +-
blame.c | 2 +-
builtin/blame.c | 2 +-
builtin/check-attr.c | 4 +-
builtin/check-ref-format.c | 5 +-
builtin/checkout-index.c | 4 +-
builtin/checkout.c | 2 +-
builtin/clone.c | 12 +-
builtin/describe.c | 2 +-
builtin/diff-index.c | 2 +-
builtin/diff.c | 10 +-
builtin/difftool.c | 2 +-
builtin/grep.c | 8 +-
builtin/hash-object.c | 4 +-
builtin/help.c | 2 +-
builtin/init-db.c | 8 +-
builtin/ls-files.c | 4 +-
builtin/merge-file.c | 2 +-
builtin/mv.c | 5 +-
builtin/read-tree.c | 2 +-
builtin/receive-pack.c | 2 +-
builtin/reset.c | 6 +-
builtin/rev-parse.c | 14 +-
builtin/rm.c | 2 +-
builtin/sparse-checkout.c | 19 +-
builtin/stripspace.c | 2 +-
builtin/submodule--helper.c | 2 +-
builtin/update-index.c | 16 +-
builtin/upload-archive.c | 2 +-
builtin/upload-pack.c | 2 +-
daemon.c | 4 +-
environment.h | 2 -
git.c | 10 +-
http-backend.c | 2 +-
http-fetch.c | 2 +-
http-push.c | 2 +-
imap-send.c | 2 +-
line-log.c | 2 +-
list-objects-filter-options.c | 2 +-
object-name.c | 4 +-
pathspec.c | 2 +-
refs.c | 2 +-
remote-curl.c | 4 +-
repository.h | 4 +-
revision.c | 6 +-
scalar.c | 4 +-
setup.c | 451 ++++++++++++++-------------
setup.h | 43 ++-
submodule.c | 2 +-
t/helper/test-advise.c | 2 +-
t/helper/test-bitmap.c | 2 +-
t/helper/test-bloom.c | 2 +-
t/helper/test-cache-tree.c | 2 +-
t/helper/test-config.c | 2 +-
t/helper/test-dump-cache-tree.c | 2 +-
t/helper/test-dump-fsmonitor.c | 2 +-
t/helper/test-dump-split-index.c | 2 +-
t/helper/test-dump-untracked-cache.c | 2 +-
t/helper/test-find-pack.c | 2 +-
t/helper/test-fsmonitor-client.c | 2 +-
t/helper/test-lazy-init-name-hash.c | 2 +-
t/helper/test-match-trees.c | 2 +-
t/helper/test-pack-deltas.c | 2 +-
t/helper/test-pack-mtimes.c | 2 +-
t/helper/test-partial-clone.c | 4 +-
t/helper/test-path-utils.c | 4 +-
t/helper/test-path-walk.c | 2 +-
t/helper/test-reach.c | 2 +-
t/helper/test-read-cache.c | 2 +-
t/helper/test-read-graph.c | 2 +-
t/helper/test-read-midx.c | 2 +-
t/helper/test-ref-store.c | 2 +-
t/helper/test-revision-walking.c | 2 +-
t/helper/test-scrap-cache-tree.c | 2 +-
t/helper/test-serve-v2.c | 2 +-
t/helper/test-submodule-config.c | 2 +-
t/helper/test-submodule-nested-repo-config.c | 2 +-
t/helper/test-submodule.c | 10 +-
t/helper/test-subprocess.c | 6 +-
t/helper/test-userdiff.c | 2 +-
t/helper/test-write-cache.c | 2 +-
worktree.c | 2 +-
wt-status.c | 2 +-
83 files changed, 402 insertions(+), 383 deletions(-)
Range-diff versus v2:
1: 55bee14899 = 1: 2ebdbac05f setup: replace use of `the_repository` in static functions
3: eaedc9e0d6 ! 2: 23ab6d8d17 setup: stop using `the_repository` in `is_inside_git_dir()`
@@ Metadata
## Commit message ##
setup: stop using `the_repository` in `is_inside_git_dir()`
- Similar as with the preceding commit, `is_inside_git_dir()` determines
- whether the current working directory is located inside the gitdir of
- `the_repository`. Perform the same refactoring by dropping the caching
- mechanism and injecting the repository that shall be checked.
+ The function `is_inside_git_dir()` verifies whether or not the current
+ working directory is located inside the gitdir of `the_repository`. This
+ is done by taking the gitdir path and verifying that it's a prefix of
+ the current working directory.
+
+ This information is cached so that we don't have to re-do this change
+ multiple times. Furthermore, we proactively set the value in multiple
+ locations so that we don't even have to perform the check when we have
+ discovered the repository.
+
+ While we could simply move the caching variable into the repository, the
+ current layout doesn't really feel sensible in the first place:
+
+ - It can easily lead to false positives or negatives if at any point
+ in time we may switch the current working directory.
+
+ - We don't call the function in a hot loop, and neither is it overly
+ expensive to compute.
+
+ Drop the caching infrastructure and instead compute the property ad-hoc
+ via an injected repository.
+
+ Note that there is one small gotcha: we often end up with relative
+ gitdir paths, and if so `is_inside_dir()` might fail. This wasn't an
+ issue before because of how we proactively set the cached value during
+ repository discovery. Now that we stop doing that it becomes a problem
+ though, which we work around by resolving the gitdir via `realpath()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
@@ setup.c
#include "worktree.h"
-static int inside_git_dir = -1;
+ static int inside_work_tree = -1;
static int work_tree_config_is_bogus;
enum allowed_bare_repo {
- ALLOWED_BARE_REPO_EXPLICIT = 0,
@@ setup.c: void verify_filename(const char *prefix,
*/
void verify_non_filename(const char *prefix, const char *arg)
{
-- if (!is_inside_work_tree(the_repository) || is_inside_git_dir())
-+ if (!is_inside_work_tree(the_repository) || is_inside_git_dir(the_repository))
+- if (!is_inside_work_tree() || is_inside_git_dir())
++ if (!is_inside_work_tree() || is_inside_git_dir(the_repository))
return;
if (*arg == '-')
return; /* flag */
@@ setup.c: int is_nonbare_repository_dir(struct strbuf *path)
+ return ret;
}
- int is_inside_work_tree(struct repository *repo)
+ int is_inside_work_tree(void)
@@ setup.c: static const char *setup_discovered_git_dir(struct repository *repo,
set_git_work_tree(".");
if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
set_git_dir(repo, gitdir, 0);
- inside_git_dir = 0;
+ inside_work_tree = 1;
if (offset >= cwd->len)
return NULL;
-
@@ setup.c: static const char *setup_bare_git_dir(struct repository *repo,
return setup_explicit_git_dir(repo, gitdir, cwd, repo_fmt, nongit_ok);
}
- inside_git_dir = 1;
+ inside_work_tree = 0;
if (offset != cwd->len) {
if (chdir(cwd->buf))
- die_errno(_("cannot come back to cwd"));
## setup.h ##
@@
@@ setup.h
-int is_inside_git_dir(void);
+int is_inside_git_dir(struct repository *repo);
- int is_inside_work_tree(struct repository *repo);
+ int is_inside_work_tree(void);
int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
int get_common_dir(struct strbuf *sb, const char *gitdir);
2: b9aef04ffe ! 3: 9f9aa3869d setup: stop using `the_repository` in `is_inside_worktree()`
@@ Metadata
Author: Patrick Steinhardt <ps@pks.im>
## Commit message ##
- setup: stop using `the_repository` in `is_inside_worktree()`
+ setup: stop using `the_repository` in `is_inside_work_tree()`
- The function `is_inside_worktree()` verifies whether or not the current
- working directory is located inside the worktree of `the_repository`.
- This is done by taking the worktree path and verifying that it's a
- prefix of the current working directory.
+ Similar as with the preceding commit, `is_inside_work_tree()` determines
+ whether the current working directory is located inside the worktree of
+ `the_repository`. Perform the same refactoring by dropping the caching
+ mechanism and injecting the repository that shall be checked.
- This information is cached so that we don't have to re-do this change
- multiple times. Furthermore, we proactively set the value in multiple
- locations so that we don't even have to perform the check when we have
- discovered the repository.
-
- While we could simply move the caching variable into the repository, the
- current layout doesn't really feel sensible in the first place:
-
- - It can easily lead to false positives or negatives if at any point
- in time we may switch the current working directory.
-
- - We don't call the function in a hot loop, and neither is it overly
- expensive to compute.
-
- Drop the caching infrastructure and instead compute the property ad-hoc
- via an injected repository.
-
- Note that there is one small gotcha: we sometimes may end up with
- relative directory paths, and if so `is_inside_dir()` might fail. This
- wasn't an issue before because of how we proactively set the cached
- value during repository discovery. Now that we stop doing that it
- becomes a problem though, but it is worked around by resolving the
- repository directory via `realpath()`.
+ Note that, same as in the preceding commit, we're also resolving the
+ worktree path via `realpath()`. In theory this step is not necessary as
+ we always set the worktree path via `repo_set_worktree()`, and that
+ function already resolves the path for us. But resolving the path a
+ second time is unlikely to matter performance-wise, and it feels fragile
+ to rely on the repository's worktree path being absolute. We thus
+ perform the same extra step even though it's ultimately not required.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
@@ object-name.c: static char *resolve_relative_path(struct repository *r, const ch
## setup.c ##
@@
+ #include "trace2.h"
#include "worktree.h"
- static int inside_git_dir = -1;
-static int inside_work_tree = -1;
static int work_tree_config_is_bogus;
enum allowed_bare_repo {
@@ setup.c: void verify_filename(const char *prefix,
*/
void verify_non_filename(const char *prefix, const char *arg)
{
-- if (!is_inside_work_tree() || is_inside_git_dir())
-+ if (!is_inside_work_tree(the_repository) || is_inside_git_dir())
+- if (!is_inside_work_tree() || is_inside_git_dir(the_repository))
++ if (!is_inside_work_tree(the_repository) || is_inside_git_dir(the_repository))
return;
if (*arg == '-')
return; /* flag */
-@@ setup.c: int is_inside_git_dir(void)
- return inside_git_dir;
+@@ setup.c: int is_inside_git_dir(struct repository *repo)
+ return ret;
}
-int is_inside_work_tree(void)
@@ setup.c: static int check_repository_format_gently(struct repository *repo,
}
@@ setup.c: static const char *setup_discovered_git_dir(struct repository *repo,
+ set_git_work_tree(".");
if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
set_git_dir(repo, gitdir, 0);
- inside_git_dir = 0;
- inside_work_tree = 1;
if (offset >= cwd->len)
return NULL;
@@ setup.c: static const char *setup_bare_git_dir(struct repository *repo,
+ return setup_explicit_git_dir(repo, gitdir, cwd, repo_fmt, nongit_ok);
}
- inside_git_dir = 1;
- inside_work_tree = 0;
if (offset != cwd->len) {
if (chdir(cwd->buf))
@@ setup.h
@@
#include "string-list.h"
- int is_inside_git_dir(void);
+ int is_inside_git_dir(struct repository *repo);
-int is_inside_work_tree(void);
+int is_inside_work_tree(struct repository *repo);
int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
4: 60accc2ec9 = 4: ce095cefaa setup: stop using `the_repository` in `prefix_path()`
5: ced765c68e = 5: dd21e47a5c setup: stop using `the_repository` in `path_inside_repo()`
6: 2d119a5570 = 6: ba8b6bfe0d setup: stop using `the_repository` in `verify_filename()`
7: 1867994202 = 7: f6bf7e5477 setup: stop using `the_repository` in `verify_non_filename()`
8: c17b96c516 = 8: 886c1a6e20 setup: stop using `the_repository` in `enter_repo()`
9: e3ce4b5056 = 9: a127018412 setup: stop using `the_repository` in `setup_work_tree()`
10: e26be95df8 = 10: 8f9a63b82f setup: stop using `the_repository` in `set_git_work_tree()`
11: 83c38c45d2 = 11: 35a9857ef0 setup: stop using `the_repository` in `setup_git_env()`
12: 153ce789de = 12: 2873901d89 setup: stop using `the_repository` in `setup_git_directory_gently()`
13: 1f10dd9c99 = 13: 7e63ec5f01 setup: stop using `the_repository` in `setup_git_directory()`
14: 9f19eba3df = 14: c227ac83dd setup: stop using `the_repository` in `upgrade_repository_format()`
15: ff19c1691d = 15: 70bff29c9a setup: stop using `the_repository` in `check_repository_format()`
16: dc8072eaf7 = 16: 31e794edc3 setup: stop using `the_repository` in `initialize_repository_version()`
17: e9c035694e = 17: 4bbab827ab setup: stop using `the_repository` in `create_reference_database()`
18: 0fde37999c = 18: 74c9379b28 setup: stop using `the_repository` in `init_db()`
---
base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
change-id: 20260330-pks-setup-wo-the-repository-81e51bc55b91
^ permalink raw reply
* Re: [PATCH v2 00/18] setup: drop uses of `the_repository`
From: Patrick Steinhardt @ 2026-05-19 9:51 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <CAOLa=ZSxhtorR+t-4M_COxfu6HwpcB0hr43OhqcwgkU+VLX6qQ@mail.gmail.com>
On Tue, May 19, 2026 at 02:26:35AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Hi,
> >
> > I've had enough of "setup.c" and its complexities, so I finally decided
> > to take the bullet and start refactoring this subsystem. This here is
> > the first out of the following three steps:
> >
> > 1. Drop all uses of `the_repository`. This doesn't yet allow us to get
> > rid of `USE_THE_REPOSITORY_VARIABLE`.
> >
> > 2. Convert a couple of global variables and drop
> > `is_bare_repository_cfg`, which then allows us to drop
> > `USE_THE_REPOSITORY_VARIABLE`.
> >
> > 3. Refactor the subsystem a bit so that we stop intermixing repository
> > discovery and repository initialization. This is my original
> > motivation as I want to get rid of `odb_prepare_alternates()`, but
> > due to the way we initialize the repository it has proven to be
> > extremely tedious.
> >
> > Most of the patches in this series here are rather mechanical. There's
> > only a handful of patches that warrant more attention:
> >
> > - 2/18: setup: stop using `the_repository` in `is_inside_worktree()`
> > - 3/18: setup: stop using `the_repository` in `is_inside_git_dir()`
> > - 9/18: setup: stop using `the_repository` in `setup_work_tree()`
> > - 10/18: setup: stop using `the_repository` in `set_git_work_tree()`
> >
>
> I think the series looks to be in a good state, I left some small nits,
> feel free to ignore.
I'll send one more round to address your feedback. Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH 02/18] setup: stop using `the_repository` in `is_inside_worktree()`
From: Patrick Steinhardt @ 2026-05-19 9:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tian Yuchen, git, Karthik Nayak, Elijah Newren
In-Reply-To: <xmqqqzn89os3.fsf@gitster.g>
On Tue, May 19, 2026 at 10:22:20AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> I do not offhand know if other code paths that are called from this
> >> function are thread-safe, but yeah, this use of file-scope static is
> >> not a safe thing to do.
> >
> > In the current status quo this is a safe conversion to do, as we still
> > have the assumption ingrained that this only works for a single repo.
> > We were using static variables before, and we're still using static
> > variables now.
> >
> > That being said, I wouldn't mind dropping the static variable if this is
> > something we'd rather want to get rid of. It's a smell that I'm not
> > particularly happy about myself.
> >
> > I'll send a revised version in a bit, thanks!
>
> OK. In the v2 (perhaps v3?) series, I can see the removal of two
> static strbuf based "optimizations" is the only change since the
> previous round (after merging the previous one to the base of the
> new iteration and comparing the result with the new iteration).
>
> Looking very good. Shall we mark the topic for 'next' now?
I'll send one final round to address Karthik's comment on the commit
message, but once that's out I think this series is ready. Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v2 02/18] setup: stop using `the_repository` in `is_inside_worktree()`
From: Patrick Steinhardt @ 2026-05-19 9:47 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <CAOLa=ZT2Qo+oJ1rx2iM3QRcXMGaucFRJdxBAgxBdz3ZtgSb=Qg@mail.gmail.com>
On Tue, May 19, 2026 at 01:23:21AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > The function `is_inside_worktree()` verifies whether or not the current
>
> Nit: Here and the subject s/is_inside_worktree/is_inside_work_tree/
>
> Honestly, I would prefer `worktree` as that's what we've used through
> out the code base, but that is probably out of scope here.
Oh, good eyes.
> > Note that there is one small gotcha: we sometimes may end up with
> > relative directory paths, and if so `is_inside_dir()` might fail. This
> > wasn't an issue before because of how we proactively set the cached
> > value during repository discovery. Now that we stop doing that it
> > becomes a problem though, but it is worked around by resolving the
> > repository directory via `realpath()`.
> >
>
> Curious if you discovered this via a test. But running the tests
> without `realpath()` didn't trigger any failures. Is this something we
> can add a test for?
No, I wasn't able to come up with a test for this. I think overall the
paragraph could use some tweaking as it's warning about something that
cannot happen right now.
Originally, I had the order of `is_inside_work_tree()` and
`is_inside_git_dir()` reversed, and I had most of the commit message in
`is_inside_git_dir()`. So this explanation here was originally part of
the gitdir commit, and here it is correct: when setting the repo's
gitdir we don't resolve it to its real path. And not using the real path
does make tests fail in this case.
But this is different for the work tree: the only caller that sets
the repository's worktree is `repo_set_worktree()`, and that function
does use `real_pathdup()` to resolve the path. So we know that the
worktree is always the absolute resolved path, and consequently it's
unnecessary to resolve the variable again.
So there's two ways to handle that:
- We could be defensive about it and continue to allow a relative or
non-resolved worktree path here and manually resolve.
- We could rely on the fact that currently the worktree path is always
resolved anyway.
I think a world where we only ever use absolute paths is preferable, but
I honestly don't trust our repository setup infrastructure enough to say
that this assumption will never break. Furthermore, it would create some
asymmetry between `is_inside_git_dir()` and `is_inside_work_tree()` that
is overall a bit puzzling.
I guess the proper fix for this would be to also change the repository's
gitdir to be an absolute path. But that would result in a bunch of
user-visible changes, both in error messages and in commands like `git
rev-parse --git-common-dir`.
Meanwhile I'm more leaning into the direction of keeping the code
as-is, but correct the commit message.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH] revision: use priority queue in limit_list()
From: Kristofer Karlsson @ 2026-05-19 9:33 UTC (permalink / raw)
To: Jeff King
Cc: Derrick Stolee, Junio C Hamano,
Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <20260519005429.GD1612961@coredump.intra.peff.net>
On Tue, 19 May 2026 at 02:54, Jeff King <peff@peff.net> wrote:
>
> On Sun, May 17, 2026 at 05:26:06PM +0200, Kristofer Karlsson wrote:
>
> > Another note - I think I managed to apply the same change to
> > get_revision_1 too - speeding up a monorepo "git rev-list HEAD" by
> > 3.3x so it seems like a reasonable thing to do.
> > This simplifies process_parents and also makes
> > commit_list_insert_by_date dead code.
> >
> > The only caveat is that get_revision_1 starts to get messier and the
> > rev_info struct needs both a prio_queue and a linked list of commits -
> > and then flushing everything
> > from the list into the prio_queue when executing get_revision_1.
>
> IMHO it is worth replacing rev_info's list with a prio_queue and letting
> that be the source of authority. You do have to be careful to cover
> cases where the list _isn't_ date-sorted, but prio_queue supports that
> with a NULL comparator.
>
> You do still have to convert between list and queue at a few spots, but
> I think in the long run many of those could be converted to use a queue.
>
> You can see my patches to do so at:
>
> https://github.com/peff/git jk/revs-commits-prio-queue
>
> I've been running with them locally for a few years. Mostly I hadn't
> gotten around to polishing them, and I think I had wanted to do some
> more perf testing. It sounds like you have a good candidate repo for
> showing off the improvement. ;)
>
> If you'd like to go in that direction, please feel free to pick out
> whatever is useful from what you find on that branch.
>
> > I don't want to pollute this patch with that change - should I start a
> > separate thread for it or just revisit this later?
> > (Perhaps I have too many optimization patches in flux already)
>
> Yes, it definitely makes sense to do that as a separate change. If you
> look at the patches I linked above, note that they'll get a bit simpler
> by rebasing on top of your limit_list() changes, since it does some of
> the same things.
>
> -Peff
I didn't know about your prior work on this -- very cool!
I took a look at your branch. Our approaches differ mainly in
how broadly the prio_queue replaces the linked list. Here's a summary
of the tradeoffs as I see them:
Your approach: replace commits entirely with struct prio_queue.
Every access site is converted, and boundary cases (bisect,
topo-sort, simplify_merges) convert queue->list->queue when they need
list-based APIs.
My approach: keep the linked list for setup and add a separate
commit_queue for the walk phase. External callers that read the
list between prepare_revision_walk() and the walk are unchanged.
The conversion happens once when the walk begins.
A quick size comparison:
Your branch: 12 files changed, 167 insertions, 152 deletions
My branch: 4 files changed, 138 insertions, 78 deletions
The main reason mine touches fewer files is that the list stays as a
list during setup, so bisect.c, builtin/rev-list.c, line-log.c, and
list-objects.c don't need changes.
On the walk side, my second and third commits refactor
get_revision_1() to use a vtable ("walk_ops") that selects the right
pop/expand strategy once and caches it:
struct revision_walk_ops {
void (*init)(struct rev_info *);
struct commit *(*next)(struct rev_info *);
int (*expand)(struct rev_info *, struct commit *);
};
static struct revision_walk_ops streaming_ops =
{ rev_info_commit_list_to_queue, next_streaming, expand_streaming };
static struct revision_walk_ops limited_ops =
{ NULL, next_commit_list, NULL };
/* ...reflog_ops, topo_ops, no_walk_ops... */
This replaces the nested if/else chain and makes each walk mode
self-contained. The init function for streaming_ops drains the list
into the queue; limited_ops just pops from the list directly.
The thing I'm less sure about is the prio_queue dual-mode usage in
your branch -- using compare=NULL for FIFO mode. It works, but it
means call sites need to reason about which mode the queue is in
(heap vs array), and the queue<->list conversions at boundaries add
up. In the two-field approach, the list is always a list and the
queue is always a heap.
That said, your approach is clearly cleaner long-term if the
remaining list consumers eventually migrate. And the single-field
design avoids the "only one should be non-empty" invariant that
mine relies on.
I benchmarked both approaches against a 2.4M-commit squash-merge-
heavy monorepo (best of 3 runs each, commit-graph present):
Benchmark mainline kk jk
rev-list HEAD (streaming, full DAG) 21.8s 6.9s 6.9s
--ancestry-path ~100K (limited) 21.8s 4.8s 5.0s
rev-list --count HEAD~10000..HEAD 17.7s 3.7s 3.8s
log --oneline -1000 0.1s 0.1s 0.1s
Both give ~3-5x speedups over mainline. The streaming walk is
identical. On limited walks kk is ~4% faster, which I think comes
from avoiding the queue rebuild at the end of limit_list() -- jk's
commit_list_to_queue() drains the result list back into the queue,
while kk leaves the result as a linked list (which the limited walk
then just pops from directly).
The perf profiles confirm this: compare_commits_by_commit_date is
11.3% in jk vs 8.0% in kk for the ancestry-path case, and
sift_down_root is 7.3% vs 5.8%. The rest of the profile is
identical.
I put up a draft PR with the two follow-up commits (on top of the
limit_list change) so you can see the full picture if you're
curious:
https://github.com/gitgitgadget/git/pull/2118
I don't have a strong preference for which approach we end up with,
since both will achieve the same performance. So it's mainly a
question about which one is easier to maintain, where everyone else
in this thread has more stake than I have :)
- Kristofer
^ permalink raw reply
* Re: [PATCH v2 00/18] setup: drop uses of `the_repository`
From: Karthik Nayak @ 2026-05-19 9:26 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260518-pks-setup-wo-the-repository-v2-0-6933c0f1d568@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> Hi,
>
> I've had enough of "setup.c" and its complexities, so I finally decided
> to take the bullet and start refactoring this subsystem. This here is
> the first out of the following three steps:
>
> 1. Drop all uses of `the_repository`. This doesn't yet allow us to get
> rid of `USE_THE_REPOSITORY_VARIABLE`.
>
> 2. Convert a couple of global variables and drop
> `is_bare_repository_cfg`, which then allows us to drop
> `USE_THE_REPOSITORY_VARIABLE`.
>
> 3. Refactor the subsystem a bit so that we stop intermixing repository
> discovery and repository initialization. This is my original
> motivation as I want to get rid of `odb_prepare_alternates()`, but
> due to the way we initialize the repository it has proven to be
> extremely tedious.
>
> Most of the patches in this series here are rather mechanical. There's
> only a handful of patches that warrant more attention:
>
> - 2/18: setup: stop using `the_repository` in `is_inside_worktree()`
> - 3/18: setup: stop using `the_repository` in `is_inside_git_dir()`
> - 9/18: setup: stop using `the_repository` in `setup_work_tree()`
> - 10/18: setup: stop using `the_repository` in `set_git_work_tree()`
>
I think the series looks to be in a good state, I left some small nits,
feel free to ignore.
- Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v5 0/8] fetch: rework negotiation tip options
From: Matthew John Cheetham @ 2026-05-19 9:13 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, ps, Derrick Stolee
In-Reply-To: <pull.2085.v5.git.1779135575.gitgitgadget@gmail.com>
On 2026-05-18 21:19, Derrick Stolee via GitGitGadget wrote:
> Range-diff vs v4:
>
> 1: 7409a479d6 ! 1: 538913a327 t5516: fix test order flakiness
> @@ Commit message
>
> Use 'sort -k 3' to match the actual number of columns in the output.
>
> + Reviewed-by: Matthew John Cheetham <mcheetham@outlook.com>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
mjcheetham@outlook.com, not mcheetham@outlook.com.
> 6: b4cd458fe0 ! 6: e86c9791e2 fetch: add --negotiation-include option for negotiation
> @@ Commit message
>
> Also add --negotiation-include to 'git pull' passthrough options.
>
> + Reviewed-by: Matthew John Cheetham <mcheetham@outlook.com>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
>
> ## Documentation/fetch-options.adoc ##
> @@ builtin/fetch.c: static int add_oid(const struct reference *ref, void *cb_data)
>
> -static void add_negotiation_restrict_tips(struct git_transport_options *smart_options)
> +static void add_negotiation_tips(struct string_list *input_list,
> -+ struct oid_array **output_list)
> ++ struct oid_array **output_list,
> ++ const char *argname)
> {
> struct oid_array *oids = xcalloc(1, sizeof(*oids));
> int i;
> @@ builtin/fetch.c: static int add_oid(const struct reference *ref, void *cb_data)
> continue;
> }
> @@ builtin/fetch.c: static void add_negotiation_restrict_tips(struct git_transport_options *smart_op
> + add_oid, oids, &opts);
> + if (old_nr == oids->nr)
> warning(_("ignoring %s=%s because it does not match any refs"),
> - "--negotiation-restrict", s);
> +- "--negotiation-restrict", s);
> ++ argname, s);
> }
> - smart_options->negotiation_restrict_tips = oids;
> + *output_list = oids;
> @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
> if (transport->smart_options)
> - add_negotiation_restrict_tips(transport->smart_options);
> + add_negotiation_tips(&negotiation_restrict,
> -+ &transport->smart_options->negotiation_restrict_tips);
> ++ &transport->smart_options->negotiation_restrict_tips,
> ++ "--negotiation-restrict");
> else
> warning(_("ignoring %s because the protocol does not support it"),
> "--negotiation-restrict");
> @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
> if (transport->smart_options)
> - add_negotiation_restrict_tips(transport->smart_options);
> + add_negotiation_tips(&negotiation_restrict,
> -+ &transport->smart_options->negotiation_restrict_tips);
> ++ &transport->smart_options->negotiation_restrict_tips,
> ++ "--negotiation-restrict");
> else {
> struct strbuf config_name = STRBUF_INIT;
> strbuf_addf(&config_name, "remote.%s.negotiationRestrict", remote->name);
> @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
> + if (negotiation_include.nr) {
> + if (transport->smart_options)
> + add_negotiation_tips(&negotiation_include,
> -+ &transport->smart_options->negotiation_include_tips);
> ++ &transport->smart_options->negotiation_include_tips,
> ++ "--negotiation-include");
> + else
> + warning(_("ignoring %s because the protocol does not support it"),
> + "--negotiation-include");
Perfect!
> @@ Documentation/config/remote.adoc: values are not used.
> +This option is additive with the normal negotiation process: the
> +negotiation algorithm still runs and advertises its own selected commits,
> +but the refs matching `remote.<name>.negotiationInclude` are sent
> -+unconditionally on top of those heuristically selected commits. This
> -+option is also used during push negotiation when `push.negotiate` is
> -+enabled.
> ++unconditionally on top of those heuristically selected commits.
> ++
> +Blank values signal to ignore all previous values, allowing a reset of
> +the list from broader config scenarios.
> @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
> + } else if (remote->negotiation_include.nr) {
> + if (transport->smart_options) {
> + add_negotiation_tips(&remote->negotiation_include,
> -+ &transport->smart_options->negotiation_include_tips);
> ++ &transport->smart_options->negotiation_include_tips,
> ++ "--negotiation-include");
> + } else {
> + struct strbuf config_name = STRBUF_INIT;
> + strbuf_addf(&config_name, "remote.%s.negotiationInclude", remote->name);
Great
> 8: 5b968245eb ! 8: ed0be32e2c send-pack: pass negotiation config in push
> @@ Commit message
> are passed as --negotiation-include to ensure their tips are always
> sent as 'have' lines during push negotiation.
>
> - This change also updates the use of --negotiation-tip into
> - --negotiation-restrict now that the new synonym exists.
> -
> + Reviewed-by: Matthew John Cheetham <mcheetham@outlook.com>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
>
> ## Documentation/config/remote.adoc ##
> @@ Documentation/config/remote.adoc: command-line option. If `--negotiation-restri
> Blank values signal to ignore all previous values, allowing a reset of
> the list from broader config scenarios.
>
> -@@ Documentation/config/remote.adoc: unconditionally on top of those heuristically selected commits. This
> - option is also used during push negotiation when `push.negotiate` is
> - enabled.
> +@@ Documentation/config/remote.adoc: negotiation algorithm still runs and advertises its own selected commits,
> + but the refs matching `remote.<name>.negotiationInclude` are sent
> + unconditionally on top of those heuristically selected commits.
> +
> +These values also influence negotiation during `git push` if
> +`push.negotiate` is enabled.
>
LGTM
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH] git-jump: pick a mode automatically when invoked without arguments
From: Greg Hurrell @ 2026-05-19 9:03 UTC (permalink / raw)
To: Erik Cervin Edin, git; +Cc: Jeff King
In-Reply-To: <agXb1SXKnA69L9ak@mbp>
On Thu, May 14, 2026, at 5:40 PM, Erik Cervin Edin wrote:
> On 26/05/08 09:07AM, Greg Hurrell via GitGitGadget wrote:
> > -usage: git jump [--stdout] <mode> [<args>]
> > +usage: git jump [--stdout] [<mode>] [<args>]
>
> The usage message makes <mode> optional but doesn't explain what
> happens when you omit it. Seems worth documenting the auto-detect behavior
> there too.
>
> If we're going to teach git-jump how to be more clever about where to jump,
> does it also make sense to bake `git jump ws` into this?
>
> Also, if this is going to grow into a proper auto-detect heuristic, it
> might be cleaner as a first-class mode rather than logic spliced into the
> argument parser. Something like:
>
> mode_auto() {
> if test -n "$(git ls-files -u)"; then
> mode_merge "$@"
> elif ! git diff --quiet; then
> mode_diff "$@"
> elif ! git diff --cached --check >/dev/null 2>&1; then
> mode_ws --cached "$@"
> else
> return 0
> fi
> }
>
> That way `git jump auto` works explicitly, bare `git jump` defaults
> to it (just `set -- auto` when $# -lt 1), and the usage text can
> document the heuristic. It also keeps the detection and dispatch in
> one place in case someone wants to tweak the priority later.
All of those suggestions sound reasonable to me. Jeff, do you agree?
If so, I can update the patch.
Best wishes,
Greg
^ permalink raw reply
* Re: [PATCH v10] checkout: extend --track with a "fetch" mode to refresh start-point
From: Harald Nordgren @ 2026-05-19 8:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: Harald Nordgren via GitGitGadget, git, Ramsay Jones,
D. Ben Knoble, Kristoffer Haugsbakk, Marc Branchaud, Phillip Wood
In-Reply-To: <CAHwyqnVtZDsBiGSk5rvMJGGk_KRg7XY_rJO4Q37hOxgoO_SJaA@mail.gmail.com>
> > > Yeah, good point. I will try to address this and send a new patch.
> >
> > Please don't.
> >
> > Next time, think deeply yourself and do not rob my time to think
> > these things for you. I do not have infinite amount of time. A
> > good balance may be if I find one issue in your current code, it is
> > likely that you'd better three more issues and fix them before
> > sending the next round, or something like that.
> >
> > Thanks.
>
> I already sent the patch. Sorry about that!
I didn't realize how much time this took away from you. My motivation
was always to put the topic into a better position ASAP, to avoid
anyone else reviewing an old version.
But I do know that it lands in your inbox each time there is a new
patch. So I will try to cut down on the amount of patches from now
one, since they are not free in terms of yours and other time. Sounds
ok?
Harald
^ permalink raw reply
* Re: [PATCH] describe: bail of --contains --all is used with --exclude or --match
From: Tuomas Ahola @ 2026-05-19 8:35 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Jacob Keller
In-Reply-To: <20190226215348.5119-1-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> If you try to use git describe --contains with --all, the exclude and
> match patterns are silently ignored.
>
> This results in unexpected behavior, as you may try to provide patterns
> and expect it to change the result.
>
I got just bitten by that, and yes, it was quite unexpected.
> Check for this, and have describe die when it encounters this, instead
> of silently ignoring the provided options.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>
> I just found this while trying to use it, the patterns weren't being applied
> properly.
>
> This is pretty quick/dirty, I haven't had time to write a test, or anything.
>
Would you like to resurrect the patch? It seems it was never merged,
nor the underlying problem fixed:
```
$ git describe --contains --all --match=bogus
master
$ git describe --contains --all --exclude="*"
master
```
> builtin/describe.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> [...]
--Tuomas
^ permalink raw reply
* Re: [PATCH v10] checkout: extend --track with a "fetch" mode to refresh start-point
From: Harald Nordgren @ 2026-05-19 8:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Harald Nordgren via GitGitGadget, git, Ramsay Jones,
D. Ben Knoble, Kristoffer Haugsbakk, Marc Branchaud, Phillip Wood
In-Reply-To: <xmqqfr3n7r1a.fsf@gitster.g>
> > Yeah, good point. I will try to address this and send a new patch.
>
> Please don't.
>
> Next time, think deeply yourself and do not rob my time to think
> these things for you. I do not have infinite amount of time. A
> good balance may be if I find one issue in your current code, it is
> likely that you'd better three more issues and fix them before
> sending the next round, or something like that.
>
> Thanks.
I already sent the patch. Sorry about that!
Harald
^ permalink raw reply
* Re: [PATCH v2] remote: qualify "git pull" advice for non-upstream branches
From: Junio C Hamano @ 2026-05-19 8:29 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Harald Nordgren
In-Reply-To: <pull.2301.v2.git.git.1778665812261.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> When "git status" reports the local branch is behind the push
> branch, the advice suggested a bare "git pull". That follows the
> upstream, which may live on a different remote, so emit
> "git pull <remote> <branch>" instead.
Hmph, shouldn't this be done conditionally, though? Most new users
follow the recommended pattern to set branch.<name>.merge so that
"git pull" would do the right thing for them, I presume, even when
they are using triangular workflow to push to a different remote
than the remote they pull from, so the new and more verbose message
would not help the users any more than the existing message, right?
Can the code tell the situation where the extra part of the message
would help and give it only then?
^ permalink raw reply
* Re: [PATCH v2 03/18] setup: stop using `the_repository` in `is_inside_git_dir()`
From: Karthik Nayak @ 2026-05-19 8:25 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260518-pks-setup-wo-the-repository-v2-3-6933c0f1d568@pks.im>
[-- Attachment #1: Type: text/plain, Size: 678 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
[snip]
> -int is_inside_git_dir(void)
> +int is_inside_git_dir(struct repository *repo)
> {
> - if (inside_git_dir < 0)
> - inside_git_dir = is_inside_dir(repo_get_git_dir(the_repository));
> - return inside_git_dir;
> + struct strbuf buf = STRBUF_INIT;
> + int ret = is_inside_dir(strbuf_realpath(&buf, repo_get_git_dir(repo), 1));
> + strbuf_release(&buf);
> + return ret;
> }
>
For `is_inside_work_tree()` we also check the return value of
`repo_get_work_tree(repo)`, but here `repo_get_git_dir(repo)` is
directly fed into `is_inside_dir()`. That's because the latter already
BUGs out if the value is not set. Looks good.
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v2 02/18] setup: stop using `the_repository` in `is_inside_worktree()`
From: Karthik Nayak @ 2026-05-19 8:23 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260518-pks-setup-wo-the-repository-v2-2-6933c0f1d568@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> The function `is_inside_worktree()` verifies whether or not the current
Nit: Here and the subject s/is_inside_worktree/is_inside_work_tree/
Honestly, I would prefer `worktree` as that's what we've used through
out the code base, but that is probably out of scope here.
> working directory is located inside the worktree of `the_repository`.
> This is done by taking the worktree path and verifying that it's a
> prefix of the current working directory.
>
> This information is cached so that we don't have to re-do this change
> multiple times. Furthermore, we proactively set the value in multiple
> locations so that we don't even have to perform the check when we have
> discovered the repository.
>
> While we could simply move the caching variable into the repository, the
> current layout doesn't really feel sensible in the first place:
>
> - It can easily lead to false positives or negatives if at any point
> in time we may switch the current working directory.
>
> - We don't call the function in a hot loop, and neither is it overly
> expensive to compute.
>
> Drop the caching infrastructure and instead compute the property ad-hoc
> via an injected repository.
So the caching mechanism is simply a global variable pre-set to -1.
Okay, so that's a good improvement.
>
> Note that there is one small gotcha: we sometimes may end up with
> relative directory paths, and if so `is_inside_dir()` might fail. This
> wasn't an issue before because of how we proactively set the cached
> value during repository discovery. Now that we stop doing that it
> becomes a problem though, but it is worked around by resolving the
> repository directory via `realpath()`.
>
Curious if you discovered this via a test. But running the tests
without `realpath()` didn't trigger any failures. Is this something we
can add a test for?
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v1 11/11] git-gui: add gui and pick as explicit subcommands
From: Johannes Sixt @ 2026-05-19 8:21 UTC (permalink / raw)
To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <fad43240-1089-4447-b97d-ee553c34eef1@gmail.com>
Am 16.05.26 um 17:42 schrieb Mark Levedahl:
> On 5/16/26 4:18 AM, Johannes Sixt wrote:
>> Am 14.05.26 um 16:33 schrieb Mark Levedahl:
>>> diff --git a/git-gui.sh b/git-gui.sh
>>> index 3a83dd5..c56aeef 100755
>>> --- a/git-gui.sh
>>> +++ b/git-gui.sh
>>> @@ -1021,6 +1021,7 @@ proc load_config {include_global} {
>>> ##
>>> ## feature option selection
>>>
>>> +set run_picker_on_error 1
>>> if {[regexp {^git-(.+)$} [file tail $argv0] _junk subcommand]} {
>>> unset _junk
>>> } else {
>>> @@ -1030,6 +1031,7 @@ if {$subcommand eq {gui.sh}} {
>>> set subcommand gui
>>> }
>>> if {$subcommand eq {gui} && [llength $argv] > 0} {
>>> + set run_picker_on_error 0
>>> set subcommand [lindex $argv 0]
>>> set argv [lrange $argv 1 end]
>>> }
>>> @@ -1047,6 +1049,7 @@ blame {
>>> disable_option multicommit
>>> disable_option branch
>>> disable_option transport
>>> + set run_picker_on_error 0
>>> }
>>> citool {
>>> enable_option singlecommit
>>> @@ -1055,6 +1058,7 @@ citool {
>>> disable_option multicommit
>>> disable_option branch
>>> disable_option transport
>>> + set run_picker_on_error 0
>>>
>>> while {[llength $argv] > 0} {
>>> set a [lindex $argv 0]
>> Can we please use the available disable_option and enable_option feature
>> instead of a new variable. Just for consistency around repository discovery.
>>
>>> @@ -1162,14 +1166,28 @@ proc pick_repo {} {
>>> set picked 1
>>> }
>>>
>>> +# run repository picker if explicitly requested
>>> +switch -- $subcommand {
>>> + pick {
>>> + pick_repo
>>> + set subcommand gui
>>> + set run_picker_on_error 0
>>> + }
>>> +}
>>> +
>> It just feels wrong to have a new pick_repo call before repository
>> discovery. Can we not treat this case below as if regular repository
>> discovery failed and then end up in the existing call of pick_repo?
>
> So, your suggestion is to create an error inside the catch clause, assure GIT_VAR and
> GIT_WORK_TREE are unset so we don't throw and error message and abort, and then fall
> through to the existing pick_repo clause?
I think I would be happier with the structure
if not subcommand pick
discover gitdir
if error
set subcommand pick
if subcommand pick
pick_repo
set subcommand gui
because this clarifies that pick_repo must erase all current traces of
GIT_DIR and GIT_WORK_TREE from the envionment and must complete with a
valid setup.
With the structure in the proposed patch
if subcommand pick
pick_repo
set subcommand gui
discover gitdir
if error
pick_repo
we still need the same operation of pick_repo, but after it runs due to
a pick command, we go into "discover gitdir" mode in an already modified
environment, something that does not happen if pick_repo runs due to the
error in the gitdir discovery.
-- Hannes
^ permalink raw reply
* Re: [PATCH v10] checkout: extend --track with a "fetch" mode to refresh start-point
From: Junio C Hamano @ 2026-05-19 8:16 UTC (permalink / raw)
To: Harald Nordgren
Cc: Harald Nordgren via GitGitGadget, git, Ramsay Jones,
D. Ben Knoble, Kristoffer Haugsbakk, Marc Branchaud, Phillip Wood
In-Reply-To: <CAHwyqnUx=59MK5zKL0uuFXKrZ6PEc1j_2WT-_xtsGewVH3gBKQ@mail.gmail.com>
Harald Nordgren <haraldnordgren@gmail.com> writes:
> Yeah, good point. I will try to address this and send a new patch.
Please don't.
Next time, think deeply yourself and do not rob my time to think
these things for you. I do not have infinite amount of time. A
good balance may be if I find one issue in your current code, it is
likely that you'd better three more issues and fix them before
sending the next round, or something like that.
Thanks.
^ permalink raw reply
* Re: [PATCH v1 10/11] git-gui: improve worktree discovery
From: Johannes Sixt @ 2026-05-19 8:16 UTC (permalink / raw)
To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <f654eab3-2157-4591-9ae0-440efb052e8e@gmail.com>
Am 16.05.26 um 17:28 schrieb Mark Levedahl:
> On 5/16/26 4:16 AM, Johannes Sixt wrote:
>> Am 14.05.26 um 16:33 schrieb Mark Levedahl:
>>> + if {[is_gitvars_error $err]} {
>>> + exit 1
>>> + }
>>> + set _gitworktree {}
>>> + set _prefix {}
>>> + if {[is_enabled bare]} {
>>> + cd $_gitdir
>> Why change the directory here? If we run `git gui browser master dir` we
>> do not want to change the directory in an uncontrolled manner. The
>> argument parser will want to check for the existence of files, and then
>> we do not want to operate from a random directory.
>>
>> Also, I think that the check must be for [is_bare] and not [is_enabled
>> bare].
>
> [is_enabled_bare] is correct. This code handles the case:
> - neither the startup directory nor GIT_WORK_TREE are useable worktrees, so [is_bare]
> is currently true.
> - the command given is browser or blame so a worktree is not needed. We can proceed.
But in the case where the command is browser or blame, the argument
parser must later check for the existence of files, provided that a
worktree is present. But this conditional would change directory to
somewhere that is not a worktree at all even though a worktree is
available. So, I am still convinced that [is_bare] is correct.
> The bigger question is whether to change directory at all: git-gui should never touch
> files that are neither in the worktree nor in the repository. Leaving the current
> directory as neither of those could be troublesome. I have no strong feeling here though,
> will delete this.
OK. We need the conditional, but not change the directory.
>>> + } elseif {![is_parent_worktree]} {
>>> + catch {wm withdraw .}
>>> + error_popup [strcat [mc "Cannot use bare repository:"] "\n\n" $_gitdir]
>>> + exit 1
>>> + }
>>> +}
>>> +
>>> +# repository and worktree config are complete, export them
>>> +set_gitdir_vars
>>>
>>> # Use object format as hash algorithm (either "sha1" or "sha256")
>>> set hashalgorithm [git rev-parse --show-object-format]
>> This moves code around. In particular, we see load_config and
>> apply_config in the context below, which now happens only after these
>> calls. How certain are we that these have no effect on the code that
>> runs now earlier?
> We need to load the system and user global config before running the repository picker. We
> (re-) load the full config including the repository after we have a repository. I think
> this is correct: git-config explicitly lists worktree dependent includeif statements,
> meaning the config can be worktree dependent, and we must not load the final config until
> repository and worktree discovery are complete.
Good point.
>
> git rev-parse, etc., perform discovery and config file loading each time they are invoked,
> those are unaffected by git-gui's internal config.
Also very true. Only calls to proc get_config would be affected by a
different setup, but there are none in the code that has swapped places
with working tree discovery. Only --show-object-format would be affected
by the discovered environment, and for that case it is more correct to
operate in the final setup.
-- Hannes
^ permalink raw reply
* Re: [PATCH] log: let --follow follow renames in merge commits
From: Junio C Hamano @ 2026-05-19 8:14 UTC (permalink / raw)
To: Miklos Vajna; +Cc: git, Patrick Steinhardt, brian m. carlson
In-Reply-To: <xmqqo6ib7vlp.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>>> Could you please review this?
>>
>> I'm a bit confused regarding what can be a next step here. I
>> understanding you were away for 3 weeks, so there is a lot to process.
>> :-) Should I just wait more or should I resend this?
>
> Rather, ask other reviewers; when I do not comment on a patch, I
> often am not interested, or too busy and the change does not look
> interesting enough to me to make me drop what I am doing.
Addendum. As I said in
https://lore.kernel.org/git/xmqqqzni967o.fsf@gitster.g/
and the subsequent discussion concluded, the "==follow" checkbox
feature is meant to work well only in a linear history, and that is
inherent to the way it "follows" the single path.
It does not follow different pathname(s) while following a set of
different histories merged, e.g., in a history like this (as usual
time flows from left to right)
----o----A----o
\
M----o----o----o
/
----x----B----x
you may start following path F at the HEAD, and after crossing the
merge M, one history may find out that path F came from path G.
The traveral starts with "F" as the sole element in the pathspec,
but once the traversal hits that commit (say, A), the traversal
switches to use "G" as the sole element in the pathspec and follows
the history down. Even if the other history (i.e., 'x' on the lower
history) had path F all along, once the pathspec is swapped to
follow "G" on the upper lineage of the history, traversal of the
lower lineage that happens after the traversal passes 'A" _will_ try
to follow "G" that may not exist at all. Or 'x' may have done the
same rename from "G" to "F" at "B". Depending on the order in which
"A" and any of these commits on the lower history are visited, the
commit that is a child of "B" (which has the path at "F") may be
visited after "A", in which case the path in question "F" will not
be looked for in it.
A minor "tweak" that does not solve this inherent design issue does
not interest me, so...
^ permalink raw reply
* Re: [PATCH v2 01/18] setup: replace use of `the_repository` in static functions
From: Karthik Nayak @ 2026-05-19 8:03 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Elijah Newren, Junio C Hamano, Tian Yuchen
In-Reply-To: <20260518-pks-setup-wo-the-repository-v2-1-6933c0f1d568@pks.im>
[-- Attachment #1: Type: text/plain, Size: 762 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> Replace the use of `the_repository` in "setup.c" for all static
> functions. For now, we simply add `the_repository` to invocations of
> these functions. This will be addressed in subsequent commits, where
> we'll move up `the_repository` one more layer to callers of "setup.c".
>
This commit is straight forward. We simply pass forward the
`the_repository` variable to static functions, so they have an incoming
`strict repository *`. I like this approach of:
1. Fixing up all static functions in a file to no longer use
`the_repository`.
2. Fixing up the other external functions one-by-one to receive a
`struct repository *`.
3. Fixing up any other global variables / config used.
Makes it easy to follow.
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ 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