* Re: [PATCH RFC v2 2/2] builtin/history: abort reword on same message
From: Patrick Steinhardt @ 2026-06-10 7:03 UTC (permalink / raw)
To: Junio C Hamano
Cc: Pablo Sabater, Phillip Wood, git, cat, kaartic.sivaraam,
ben.knoble
In-Reply-To: <xmqqbjdj1q1s.fsf@gitster.g>
On Tue, Jun 09, 2026 at 12:17:51PM -0700, Junio C Hamano wrote:
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
> >> > I wonder if we should check that the committer identity is unchanged as
> >> > well in case anyone is using this to fix commits after committing with
> >> > the wrong identity.
> >
> > I think that if you reword a commit committed by someone else but end
> > up with no changes I want it to be kept as it was.
>
> That depends on the reason why the feature to "reword" the commit is
> being used, and the use case Phillip is talking about is a bit
> different.
So the answer is "it depends". Maybe we should do handle this the same
as git-commit(1) does with its "--reset-author" flag?
Patrick
^ permalink raw reply
* [PATCH 7/7] treewide: drop USE_THE_REPOSITORY_VARIABLE
From: Patrick Steinhardt @ 2026-06-10 6:56 UTC (permalink / raw)
To: git
In-Reply-To: <20260610-b4-pks-setup-drop-global-state-v1-0-5dff3eec8f06@pks.im>
Adapt a couple of trivial callers of `is_bare_repository()` to instead
use a repository available via the caller's context so that we can drop
the `USE_THE_REPOSITORY_VARIABLE` macro.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/repack.c | 3 +--
mailmap.c | 6 ++----
refs/reftable-backend.c | 4 +---
setup.c | 3 +--
4 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index bbc6f51639..d0465fb4f5 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
#define DISABLE_SIGN_COMPARE_WARNINGS
#include "builtin.h"
@@ -265,7 +264,7 @@ int cmd_repack(int argc,
if (write_bitmaps < 0) {
if (write_midx == REPACK_WRITE_MIDX_NONE &&
- (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository(the_repository)))
+ (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository(repo)))
write_bitmaps = 0;
}
if (po_args.pack_kept_objects < 0)
diff --git a/mailmap.c b/mailmap.c
index 7d8590cdd6..2d5514f833 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "git-compat-util.h"
#include "environment.h"
#include "string-list.h"
@@ -219,10 +217,10 @@ int read_mailmap(struct repository *repo, struct string_list *map)
map->strdup_strings = 1;
map->cmp = namemap_cmp;
- if (!mailmap_blob && is_bare_repository(the_repository))
+ if (!mailmap_blob && is_bare_repository(repo))
mailmap_blob = xstrdup("HEAD:.mailmap");
- if (!startup_info->have_repository || !is_bare_repository(the_repository))
+ if (!startup_info->have_repository || !is_bare_repository(repo))
err |= read_mailmap_file(map, ".mailmap",
startup_info->have_repository ?
MAILMAP_NOFOLLOW : 0);
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 101ef29ac8..c151d331e7 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "../git-compat-util.h"
#include "../abspath.h"
#include "../chdir-notify.h"
@@ -288,7 +286,7 @@ static int should_write_log(struct reftable_ref_store *refs, const char *refname
{
enum log_refs_config log_refs_cfg = refs->log_all_ref_updates;
if (log_refs_cfg == LOG_REFS_UNSET)
- log_refs_cfg = is_bare_repository(the_repository) ? LOG_REFS_NONE : LOG_REFS_NORMAL;
+ log_refs_cfg = is_bare_repository(refs->base.repo) ? LOG_REFS_NONE : LOG_REFS_NORMAL;
switch (log_refs_cfg) {
case LOG_REFS_NONE:
diff --git a/setup.c b/setup.c
index 6b95bf546d..f24a805658 100644
--- a/setup.c
+++ b/setup.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
#define DISABLE_SIGN_COMPARE_WARNINGS
#include "git-compat-util.h"
@@ -2604,7 +2603,7 @@ static int create_default_files(struct repository *repo,
}
repo_config_set(repo, "core.filemode", filemode ? "true" : "false");
- if (is_bare_repository(the_repository))
+ if (is_bare_repository(repo))
repo_config_set(repo, "core.bare", "true");
else {
repo_config_set(repo, "core.bare", "false");
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 6/7] environment: stop using `the_repository` in `is_bare_repository()`
From: Patrick Steinhardt @ 2026-06-10 6:56 UTC (permalink / raw)
To: git
In-Reply-To: <20260610-b4-pks-setup-drop-global-state-v1-0-5dff3eec8f06@pks.im>
Refactor `is_bare_repository()` to take in a repository parameter so
that we no longer depend on `the_repository`. Adjust callers
accordingly.
Furthermore, move the function outside of the declarations that are only
available when `USE_THE_REPOSITORY_VARIABLE` is set, as it no longer
depends on that variable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
attr.c | 4 ++--
builtin/bisect.c | 2 +-
builtin/blame.c | 2 +-
builtin/check-attr.c | 2 +-
builtin/fetch.c | 2 +-
builtin/gc.c | 2 +-
builtin/history.c | 2 +-
builtin/repack.c | 2 +-
builtin/repo.c | 2 +-
builtin/reset.c | 2 +-
builtin/rev-parse.c | 2 +-
environment.c | 4 ++--
environment.h | 4 ++--
mailmap.c | 4 ++--
refs/files-backend.c | 2 +-
refs/reftable-backend.c | 2 +-
setup.c | 2 +-
transport.c | 4 ++--
worktree.c | 2 +-
19 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/attr.c b/attr.c
index 75369547b3..04cb284954 100644
--- a/attr.c
+++ b/attr.c
@@ -681,7 +681,7 @@ static enum git_attr_direction direction;
void git_attr_set_direction(enum git_attr_direction new_direction)
{
- if (is_bare_repository() && new_direction != GIT_ATTR_INDEX)
+ if (is_bare_repository(the_repository) && new_direction != GIT_ATTR_INDEX)
BUG("non-INDEX attr direction in a bare repo");
if (new_direction != direction)
@@ -848,7 +848,7 @@ static struct attr_stack *read_attr(struct index_state *istate,
res = read_attr_from_index(istate, path, flags);
} else if (tree_oid) {
res = read_attr_from_blob(istate, tree_oid, path, flags);
- } else if (!is_bare_repository()) {
+ } else if (!is_bare_repository(the_repository)) {
if (direction == GIT_ATTR_CHECKOUT) {
res = read_attr_from_index(istate, path, flags);
if (!res)
diff --git a/builtin/bisect.c b/builtin/bisect.c
index e7c2d2f3bb..798e28f501 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -724,7 +724,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
struct object_id oid;
const char *head;
- if (is_bare_repository())
+ if (is_bare_repository(the_repository))
no_checkout = 1;
/*
diff --git a/builtin/blame.c b/builtin/blame.c
index ffbd3ce5c5..553f4cb780 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1163,7 +1163,7 @@ int cmd_blame(int argc,
revs.disable_stdin = 1;
setup_revisions(argc, argv, &revs, NULL);
- if (!revs.pending.nr && is_bare_repository()) {
+ if (!revs.pending.nr && is_bare_repository(the_repository)) {
struct commit *head_commit;
struct object_id head_oid;
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 98f64d5b92..217d83ea7d 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -116,7 +116,7 @@ int cmd_check_attr(int argc,
struct object_id initialized_oid;
int cnt, i, doubledash, filei;
- if (!is_bare_repository())
+ if (!is_bare_repository(the_repository))
setup_work_tree(the_repository);
repo_config(the_repository, git_default_config, NULL);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index c1d7c672f4..44b8c70da1 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1764,7 +1764,7 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
if (!head_name)
goto cleanup;
- baremirror = is_bare_repository() && remote->mirror;
+ baremirror = is_bare_repository(the_repository) && remote->mirror;
create_only = follow_remote_head == FOLLOW_REMOTE_ALWAYS ? 0 : !baremirror;
if (baremirror) {
strbuf_addstr(&b_head, "HEAD");
diff --git a/builtin/gc.c b/builtin/gc.c
index 84a66d3240..61da30de9f 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -902,7 +902,7 @@ int cmd_gc(int argc,
die(_("failed to parse gc.logExpiry value %s"), cfg.gc_log_expire);
if (cfg.pack_refs < 0)
- cfg.pack_refs = !is_bare_repository();
+ cfg.pack_refs = !is_bare_repository(the_repository);
argc = parse_options(argc, argv, prefix, builtin_gc_options,
builtin_gc_usage, 0);
diff --git a/builtin/history.c b/builtin/history.c
index 091465a59e..fd83de8265 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -525,7 +525,7 @@ static int cmd_history_fixup(int argc,
if (action == REF_ACTION_DEFAULT)
action = REF_ACTION_BRANCHES;
- if (is_bare_repository()) {
+ if (is_bare_repository(repo)) {
ret = error(_("cannot run fixup in a bare repository"));
goto out;
}
diff --git a/builtin/repack.c b/builtin/repack.c
index 1524a9c13a..bbc6f51639 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -265,7 +265,7 @@ int cmd_repack(int argc,
if (write_bitmaps < 0) {
if (write_midx == REPACK_WRITE_MIDX_NONE &&
- (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
+ (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository(the_repository)))
write_bitmaps = 0;
}
if (po_args.pack_kept_objects < 0)
diff --git a/builtin/repo.c b/builtin/repo.c
index 71a5c1c29c..34e96514bc 100644
--- a/builtin/repo.c
+++ b/builtin/repo.c
@@ -58,7 +58,7 @@ struct repo_info_field {
static int get_layout_bare(struct repository *repo UNUSED, struct strbuf *buf)
{
- strbuf_addstr(buf, is_bare_repository() ? "true" : "false");
+ strbuf_addstr(buf, is_bare_repository(the_repository) ? "true" : "false");
return 0;
}
diff --git a/builtin/reset.c b/builtin/reset.c
index 3be6bd0121..78e69bd84b 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -470,7 +470,7 @@ int cmd_reset(int argc,
if (reset_type != SOFT && (reset_type != MIXED || repo_get_work_tree(the_repository)))
setup_work_tree(the_repository);
- if (reset_type == MIXED && is_bare_repository())
+ if (reset_type == MIXED && is_bare_repository(the_repository))
die(_("%s reset is not allowed in a bare repository"),
_(reset_type_names[reset_type]));
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index bb882678fe..090e5cfbb0 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -1084,7 +1084,7 @@ int cmd_rev_parse(int argc,
continue;
}
if (!strcmp(arg, "--is-bare-repository")) {
- printf("%s\n", is_bare_repository() ? "true"
+ printf("%s\n", is_bare_repository(the_repository) ? "true"
: "false");
continue;
}
diff --git a/environment.c b/environment.c
index 9d7c908c55..bf20953415 100644
--- a/environment.c
+++ b/environment.c
@@ -132,10 +132,10 @@ const char *getenv_safe(struct strvec *argv, const char *name)
return argv->v[argv->nr - 1];
}
-int is_bare_repository(void)
+int is_bare_repository(struct repository *repo)
{
/* if core.bare is not 'false', let's see if there is a work tree */
- return the_repository->bare_cfg && !repo_get_work_tree(the_repository);
+ return repo->bare_cfg && !repo_get_work_tree(repo);
}
int have_git_dir(void)
diff --git a/environment.h b/environment.h
index afb5bcf197..164a55df2c 100644
--- a/environment.h
+++ b/environment.h
@@ -125,6 +125,8 @@ int git_default_core_config(const char *var, const char *value,
void repo_config_values_init(struct repo_config_values *cfg);
+int is_bare_repository(struct repository *repo);
+
/*
* TODO: All the below state either explicitly or implicitly relies on
* `the_repository`. We should eventually get rid of these and make the
@@ -147,8 +149,6 @@ void repo_config_values_init(struct repo_config_values *cfg);
*/
int have_git_dir(void);
-int is_bare_repository(void);
-
/* Environment bits from configuration mechanism */
extern int trust_executable_bit;
extern int trust_ctime;
diff --git a/mailmap.c b/mailmap.c
index 3b2691781d..7d8590cdd6 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -219,10 +219,10 @@ int read_mailmap(struct repository *repo, struct string_list *map)
map->strdup_strings = 1;
map->cmp = namemap_cmp;
- if (!mailmap_blob && is_bare_repository())
+ if (!mailmap_blob && is_bare_repository(the_repository))
mailmap_blob = xstrdup("HEAD:.mailmap");
- if (!startup_info->have_repository || !is_bare_repository())
+ if (!startup_info->have_repository || !is_bare_repository(the_repository))
err |= read_mailmap_file(map, ".mailmap",
startup_info->have_repository ?
MAILMAP_NOFOLLOW : 0);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a4c7858787..2b27091484 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1865,7 +1865,7 @@ static int log_ref_setup(struct files_ref_store *refs,
char *logfile;
if (log_refs_cfg == LOG_REFS_UNSET)
- log_refs_cfg = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
+ log_refs_cfg = is_bare_repository(the_repository) ? LOG_REFS_NONE : LOG_REFS_NORMAL;
files_reflog_path(refs, &logfile_sb, refname);
logfile = strbuf_detach(&logfile_sb, NULL);
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4ae22922de..101ef29ac8 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -288,7 +288,7 @@ static int should_write_log(struct reftable_ref_store *refs, const char *refname
{
enum log_refs_config log_refs_cfg = refs->log_all_ref_updates;
if (log_refs_cfg == LOG_REFS_UNSET)
- log_refs_cfg = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
+ log_refs_cfg = is_bare_repository(the_repository) ? LOG_REFS_NONE : LOG_REFS_NORMAL;
switch (log_refs_cfg) {
case LOG_REFS_NONE:
diff --git a/setup.c b/setup.c
index 2b690da8ca..6b95bf546d 100644
--- a/setup.c
+++ b/setup.c
@@ -2604,7 +2604,7 @@ static int create_default_files(struct repository *repo,
}
repo_config_set(repo, "core.filemode", filemode ? "true" : "false");
- if (is_bare_repository())
+ if (is_bare_repository(the_repository))
repo_config_set(repo, "core.bare", "true");
else {
repo_config_set(repo, "core.bare", "false");
diff --git a/transport.c b/transport.c
index 0f5ec30247..fc144f0aed 100644
--- a/transport.c
+++ b/transport.c
@@ -1482,7 +1482,7 @@ int transport_push(struct repository *r,
if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
- !is_bare_repository()) {
+ !is_bare_repository(the_repository)) {
struct ref *ref = remote_refs;
struct oid_array commits = OID_ARRAY_INIT;
@@ -1509,7 +1509,7 @@ int transport_push(struct repository *r,
if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
- !pretend)) && !is_bare_repository()) {
+ !pretend)) && !is_bare_repository(the_repository)) {
struct ref *ref = remote_refs;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
struct oid_array commits = OID_ARRAY_INIT;
diff --git a/worktree.c b/worktree.c
index 7d70f2c1da..30125827fd 100644
--- a/worktree.c
+++ b/worktree.c
@@ -124,7 +124,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
worktree->path = strbuf_detach(&worktree_path, NULL);
worktree->is_current = is_current_worktree(worktree);
worktree->is_bare = (the_repository->bare_cfg == 1) ||
- is_bare_repository() ||
+ is_bare_repository(the_repository) ||
/*
* When in a secondary worktree we have to also verify if the main
* worktree is bare in $commondir/config.worktree.
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 5/7] environment: split up concerns of `is_bare_repository_cfg`
From: Patrick Steinhardt @ 2026-06-10 6:56 UTC (permalink / raw)
To: git
In-Reply-To: <20260610-b4-pks-setup-drop-global-state-v1-0-5dff3eec8f06@pks.im>
The `is_bare_repository_cfg` variable tracks two different pieces of
information:
- It tracks whether the user has invoked git with the "--bare" flag,
which makes us treat any discovered Git repository as if it was a
bare repository.
- Otherwise it tracks whether the discovered `the_repository` is bare.
This makes the flag extremely confusing and creates a bit of a challenge
when handling multiple repositories in the same process.
Split up the concerns of this variable into two pieces:
- `startup_info.force_bare_repository` tracks whether the user has
passed the "--bare" flag. This is used as a hint to treat newly set
up repositories as bare regardless of whether or not they have a
worktree.
- `struct repository::bare_cfg` tracks whether or not a repository is
considered bare. This takes into account both whether the user has
passed "--bare" and the discovered state of the repository itself.
Whether or not a repository is bare is now resolved when checking the
repository's format, and is then later applied to the repository itself
via `apply_repository_format()`.
This enables a subsequent change where we make `is_bare_repository()`
not depend on global state anymore.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/init-db.c | 2 +-
environment.c | 5 ++---
environment.h | 1 -
git.c | 2 +-
repository.c | 1 +
repository.h | 7 +++++++
setup.c | 21 ++++++++++++++-------
setup.h | 6 ++++++
worktree.c | 2 +-
9 files changed, 33 insertions(+), 14 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 52aa92fb0a..566732c9f4 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -81,7 +81,7 @@ int cmd_init_db(int argc,
const char *template_dir = NULL;
char *template_dir_to_free = NULL;
unsigned int flags = 0;
- int bare = is_bare_repository_cfg;
+ int bare = startup_info->force_bare_repository ? 1 : -1;
const char *object_format = NULL;
const char *ref_format = NULL;
const char *initial_branch = NULL;
diff --git a/environment.c b/environment.c
index 4e86335f25..9d7c908c55 100644
--- a/environment.c
+++ b/environment.c
@@ -48,7 +48,6 @@ int has_symlinks = 1;
int minimum_abbrev = 4, default_abbrev = -1;
int ignore_case;
int assume_unchanged;
-int is_bare_repository_cfg = -1; /* unspecified */
int warn_on_object_refname_ambiguity = 1;
char *git_commit_encoding;
char *git_log_output_encoding;
@@ -136,7 +135,7 @@ const char *getenv_safe(struct strvec *argv, const char *name)
int is_bare_repository(void)
{
/* if core.bare is not 'false', let's see if there is a work tree */
- return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
+ return the_repository->bare_cfg && !repo_get_work_tree(the_repository);
}
int have_git_dir(void)
@@ -342,7 +341,7 @@ int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.bare")) {
- is_bare_repository_cfg = git_config_bool(var, value);
+ the_repository->bare_cfg = git_config_bool(var, value);
return 0;
}
diff --git a/environment.h b/environment.h
index 5d6e4e6c1b..afb5bcf197 100644
--- a/environment.h
+++ b/environment.h
@@ -147,7 +147,6 @@ void repo_config_values_init(struct repo_config_values *cfg);
*/
int have_git_dir(void);
-extern int is_bare_repository_cfg;
int is_bare_repository(void);
/* Environment bits from configuration mechanism */
diff --git a/git.c b/git.c
index 36f08891ef..387eabe38c 100644
--- a/git.c
+++ b/git.c
@@ -255,7 +255,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--bare")) {
char *cwd = xgetcwd();
- is_bare_repository_cfg = 1;
+ startup_info->force_bare_repository = true;
setenv(GIT_DIR_ENVIRONMENT, cwd, 0);
free(cwd);
setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
diff --git a/repository.c b/repository.c
index 187dd471c4..c1e91eb0da 100644
--- a/repository.c
+++ b/repository.c
@@ -73,6 +73,7 @@ void initialize_repository(struct repository *repo)
ALLOC_ARRAY(repo->index, 1);
index_state_init(repo->index, repo);
repo->check_deprecated_config = true;
+ repo->bare_cfg = -1;
repo_config_values_init(&repo->config_values_private_);
/*
diff --git a/repository.h b/repository.h
index 36e2db2633..7d649e32e7 100644
--- a/repository.h
+++ b/repository.h
@@ -117,6 +117,13 @@ struct repository {
bool worktree_initialized;
bool worktree_config_is_bogus;
+ /*
+ * Whether the repository is bare, as set by "core.bare" config or
+ * inferred during repository discovery. -1 means unset/unknown, 0
+ * means non-bare, 1 means bare.
+ */
+ int bare_cfg;
+
/*
* Path from the root of the top-level superproject down to this
* repository. This is only non-NULL if the repository is initialized
diff --git a/setup.c b/setup.c
index 71fc6b33da..2b690da8ca 100644
--- a/setup.c
+++ b/setup.c
@@ -795,10 +795,16 @@ static int check_repository_format_gently(const char *gitdir,
has_common = 0;
}
- if (!has_common) {
- if (candidate->is_bare != -1)
- is_bare_repository_cfg = candidate->is_bare;
- } else {
+ if (startup_info->force_bare_repository) {
+ candidate->is_bare = 1;
+ FREE_AND_NULL(candidate->work_tree);
+ } else if (has_common) {
+ /*
+ * When sharing a common dir with another repository (e.g. a
+ * linked worktree), do not let this repository's config
+ * dictate bareness; it is inherited from the main worktree.
+ */
+ candidate->is_bare = -1;
FREE_AND_NULL(candidate->work_tree);
}
@@ -1138,7 +1144,7 @@ static const char *setup_explicit_git_dir(struct repository *repo,
/* #3, #7, #11, #15, #19, #23, #27, #31 (see t1510) */
if (work_tree_env)
set_git_work_tree(repo, work_tree_env);
- else if (is_bare_repository_cfg > 0) {
+ else if (repo_fmt->is_bare > 0) {
if (repo_fmt->work_tree) {
/* #22.2, #30 */
warning("core.bare and core.worktree do not make sense");
@@ -1225,7 +1231,7 @@ static const char *setup_discovered_git_dir(struct repository *repo,
}
/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
- if (is_bare_repository_cfg > 0) {
+ if (repo_fmt->is_bare > 0) {
set_git_dir(repo, gitdir, (offset != cwd->len));
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
@@ -1762,6 +1768,7 @@ int apply_repository_format(struct repository *repo,
alternate_object_directories = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));
}
+ repo->bare_cfg = format->is_bare;
repo_set_hash_algo(repo, format->hash_algo);
repo->objects = odb_new(repo, object_directory,
alternate_object_directories);
@@ -2571,7 +2578,7 @@ static int create_default_files(struct repository *repo,
repo_settings_set_shared_repository(repo,
init_shared_repository);
- is_bare_repository_cfg = !work_tree;
+ repo->bare_cfg = !work_tree;
/*
* We would have created the above under user's umask -- under
diff --git a/setup.h b/setup.h
index 705d1d6ff7..b9fd96bea6 100644
--- a/setup.h
+++ b/setup.h
@@ -292,6 +292,12 @@ enum sharedrepo {
int git_config_perm(const char *var, const char *value);
struct startup_info {
+ /*
+ * Whether the user is asking us to treat the repository as bare via
+ * `git --bare`, even if it's not.
+ */
+ bool force_bare_repository;
+
int have_repository;
const char *prefix;
const char *original_cwd;
diff --git a/worktree.c b/worktree.c
index 97eddc3916..7d70f2c1da 100644
--- a/worktree.c
+++ b/worktree.c
@@ -123,7 +123,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
worktree->repo = the_repository;
worktree->path = strbuf_detach(&worktree_path, NULL);
worktree->is_current = is_current_worktree(worktree);
- worktree->is_bare = (is_bare_repository_cfg == 1) ||
+ worktree->is_bare = (the_repository->bare_cfg == 1) ||
is_bare_repository() ||
/*
* When in a secondary worktree we have to also verify if the main
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 4/7] builtin/init: stop modifying `is_bare_repository_cfg`
From: Patrick Steinhardt @ 2026-06-10 6:56 UTC (permalink / raw)
To: git
In-Reply-To: <20260610-b4-pks-setup-drop-global-state-v1-0-5dff3eec8f06@pks.im>
We're modifying `is_bare_repository_cfg` in "builtin/init.c" to indicate
whether the newly created repository is supposed to be a bare repository
or not.
This is ultimately unnecessary though: when initializing the repository
in `init_db()` we eventually set `is_bare_repository_cfg = !work_tree`,
so all that matters is whether or not we have a working tree configured,
and the working tree is set up in the non-bare in "builtin/init.c".
Stop modifying the global variable in "builtin/init.c" in favor of a
local variable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/init-db.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index b4343c2804..52aa92fb0a 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -81,6 +81,7 @@ int cmd_init_db(int argc,
const char *template_dir = NULL;
char *template_dir_to_free = NULL;
unsigned int flags = 0;
+ int bare = is_bare_repository_cfg;
const char *object_format = NULL;
const char *ref_format = NULL;
const char *initial_branch = NULL;
@@ -90,7 +91,7 @@ int cmd_init_db(int argc,
const struct option init_db_options[] = {
OPT_STRING(0, "template", &template_dir, N_("template-directory"),
N_("directory from which templates will be used")),
- OPT_SET_INT(0, "bare", &is_bare_repository_cfg,
+ OPT_SET_INT(0, "bare", &bare,
N_("create a bare repository"), 1),
{
.type = OPTION_CALLBACK,
@@ -116,7 +117,7 @@ int cmd_init_db(int argc,
argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
- if (real_git_dir && is_bare_repository_cfg == 1)
+ if (real_git_dir && bare == 1)
die(_("options '%s' and '%s' cannot be used together"), "--separate-git-dir", "--bare");
if (real_git_dir && !is_absolute_path(real_git_dir))
@@ -160,7 +161,7 @@ int cmd_init_db(int argc,
} else if (0 < argc) {
usage(init_db_usage[0]);
}
- if (is_bare_repository_cfg == 1) {
+ if (bare == 1) {
char *cwd = xgetcwd();
setenv(GIT_DIR_ENVIRONMENT, cwd, argc > 0);
free(cwd);
@@ -187,7 +188,7 @@ int cmd_init_db(int argc,
*/
git_dir = xstrdup_or_null(getenv(GIT_DIR_ENVIRONMENT));
work_tree = xstrdup_or_null(getenv(GIT_WORK_TREE_ENVIRONMENT));
- if ((!git_dir || is_bare_repository_cfg == 1) && work_tree)
+ if ((!git_dir || bare == 1) && work_tree)
die(_("%s (or --work-tree=<directory>) not allowed without "
"specifying %s (or --git-dir=<directory>)"),
GIT_WORK_TREE_ENVIRONMENT,
@@ -224,10 +225,10 @@ int cmd_init_db(int argc,
strbuf_release(&sb);
}
- if (is_bare_repository_cfg < 0)
- is_bare_repository_cfg = guess_repository_type(git_dir);
+ if (bare < 0)
+ bare = guess_repository_type(git_dir);
- if (!is_bare_repository_cfg) {
+ if (!bare) {
const char *git_dir_parent = strrchr(git_dir, '/');
if (work_tree) {
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 3/7] setup: remove global `git_work_tree_cfg` variable
From: Patrick Steinhardt @ 2026-06-10 6:56 UTC (permalink / raw)
To: git
In-Reply-To: <20260610-b4-pks-setup-drop-global-state-v1-0-5dff3eec8f06@pks.im>
The global `git_work_tree_cfg` variable used to be modified by both
"setup.c" and by "builtin/init-db.c". We have refactored the latter user
to not use that variable at all anymore in a preceding commit, which
makes "setup.c" the only remaining user.
Even for "setup.c" it is unnecessary though, as we only ever set it to
the value we have stored in the discovered repository format. The
consequence is that we only ever set it in case we already have it set
to the same value in our discovered repository format, which makes it
redundant.
Refactor the code so that we instead use the worktree configuration as
discovered via the repository format. Drop the global variable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/setup.c b/setup.c
index 52228b42a1..71fc6b33da 100644
--- a/setup.c
+++ b/setup.c
@@ -31,9 +31,6 @@ enum allowed_bare_repo {
ALLOWED_BARE_REPO_ALL,
};
-/* This is set by setup_git_directory_gently() and/or git_default_config() */
-static char *git_work_tree_cfg;
-
static struct startup_info the_startup_info;
struct startup_info *startup_info = &the_startup_info;
const char *tmp_original_cwd;
@@ -799,13 +796,10 @@ static int check_repository_format_gently(const char *gitdir,
}
if (!has_common) {
- if (candidate->is_bare != -1) {
+ if (candidate->is_bare != -1)
is_bare_repository_cfg = candidate->is_bare;
- }
- if (candidate->work_tree) {
- free(git_work_tree_cfg);
- git_work_tree_cfg = xstrdup(candidate->work_tree);
- }
+ } else {
+ FREE_AND_NULL(candidate->work_tree);
}
return 0;
@@ -1145,7 +1139,7 @@ static const char *setup_explicit_git_dir(struct repository *repo,
if (work_tree_env)
set_git_work_tree(repo, work_tree_env);
else if (is_bare_repository_cfg > 0) {
- if (git_work_tree_cfg) {
+ if (repo_fmt->work_tree) {
/* #22.2, #30 */
warning("core.bare and core.worktree do not make sense");
repo->worktree_config_is_bogus = true;
@@ -1156,15 +1150,15 @@ static const char *setup_explicit_git_dir(struct repository *repo,
free(gitfile);
return NULL;
}
- else if (git_work_tree_cfg) { /* #6, #14 */
- if (is_absolute_path(git_work_tree_cfg))
- set_git_work_tree(repo, git_work_tree_cfg);
+ else if (repo_fmt->work_tree) { /* #6, #14 */
+ if (is_absolute_path(repo_fmt->work_tree))
+ set_git_work_tree(repo, repo_fmt->work_tree);
else {
char *core_worktree;
if (chdir(gitdirenv))
die_errno(_("cannot chdir to '%s'"), gitdirenv);
- if (chdir(git_work_tree_cfg))
- die_errno(_("cannot chdir to '%s'"), git_work_tree_cfg);
+ if (chdir(repo_fmt->work_tree))
+ die_errno(_("cannot chdir to '%s'"), repo_fmt->work_tree);
core_worktree = xgetcwd();
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
@@ -1217,7 +1211,7 @@ static const char *setup_discovered_git_dir(struct repository *repo,
return NULL;
/* --work-tree is set without --git-dir; use discovered one */
- if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
+ if (getenv(GIT_WORK_TREE_ENVIRONMENT) || repo_fmt->work_tree) {
char *to_free = NULL;
const char *ret;
@@ -1267,7 +1261,7 @@ static const char *setup_bare_git_dir(struct repository *repo,
setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
/* --work-tree is set without --git-dir; use discovered one */
- if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
+ if (getenv(GIT_WORK_TREE_ENVIRONMENT) || repo_fmt->work_tree) {
static const char *gitdir;
gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset);
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 2/7] builtin/init: simplify logic to configure worktree
From: Patrick Steinhardt @ 2026-06-10 6:56 UTC (permalink / raw)
To: git
In-Reply-To: <20260610-b4-pks-setup-drop-global-state-v1-0-5dff3eec8f06@pks.im>
In the preceding commit we have stopped modifying the global
`git_work_tree_cfg` variable. With this change there's now some code
paths where we end up setting the local `git_work_tree_cfg` variable,
but without actually using the value for anything.
Refactor the code a bit so that we only set the worktree configuration
in case it's actually needed. Furthermore, reflow it a bit to make the
code easier to follow.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/init-db.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 01bc27904e..b4343c2804 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -229,24 +229,29 @@ int cmd_init_db(int argc,
if (!is_bare_repository_cfg) {
const char *git_dir_parent = strrchr(git_dir, '/');
- char *git_work_tree_cfg = NULL;
- if (git_dir_parent) {
- char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
- git_work_tree_cfg = real_pathdup(rel, 1);
- free(rel);
- }
- if (!git_work_tree_cfg)
- git_work_tree_cfg = xgetcwd();
- if (work_tree)
+ if (work_tree) {
set_git_work_tree(the_repository, work_tree);
- else
- set_git_work_tree(the_repository, git_work_tree_cfg);
+ } else {
+ char *work_tree_cfg = NULL;
+
+ if (git_dir_parent) {
+ char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
+ work_tree_cfg = real_pathdup(rel, 1);
+ free(rel);
+ }
+
+ if (!work_tree_cfg)
+ work_tree_cfg = xgetcwd();
+
+ set_git_work_tree(the_repository, work_tree_cfg);
+
+ free(work_tree_cfg);
+ }
+
if (access(repo_get_work_tree(the_repository), X_OK))
die_errno (_("Cannot access work tree '%s'"),
repo_get_work_tree(the_repository));
-
- free(git_work_tree_cfg);
}
else {
if (real_git_dir)
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 1/7] builtin/init: stop modifying global `git_work_tree_cfg` variable
From: Patrick Steinhardt @ 2026-06-10 6:56 UTC (permalink / raw)
To: git
In-Reply-To: <20260610-b4-pks-setup-drop-global-state-v1-0-5dff3eec8f06@pks.im>
When executing git-init(1) we need to figure out the final location of
the worktree. This location can be configured in a couple of ways: via
an environment variable, via the preexisting "core.worktree" config in
case we're reinitializing, or implicitly when reinitializing a non-bare
repository.
When checking for the worktree location in "builtin/init-db.c" we
populate any potentially-discovered value both by setting the global
`git_work_tree_cfg` variable and via `set_git_work_tree()`, which
ultimately ends up modifying `struct repository::worktree`.
Modifying `git_work_tree_cfg` is unnecessary though: we configure the
worktree in `create_default_files()`, and that function derives the
worktree location via `repo_get_work_tree()`. Consequently, propagating
the worktree via `set_git_work_tree()` is sufficient.
Stop munging `git_work_tree_cfg` and make it file-local to "setup.c" and
function-local to `cmd_init_db()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/init-db.c | 4 ++++
environment.c | 3 ---
environment.h | 1 -
setup.c | 3 +++
4 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index c55517ad94..01bc27904e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -229,6 +229,8 @@ int cmd_init_db(int argc,
if (!is_bare_repository_cfg) {
const char *git_dir_parent = strrchr(git_dir, '/');
+ char *git_work_tree_cfg = NULL;
+
if (git_dir_parent) {
char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
git_work_tree_cfg = real_pathdup(rel, 1);
@@ -243,6 +245,8 @@ int cmd_init_db(int argc,
if (access(repo_get_work_tree(the_repository), X_OK))
die_errno (_("Cannot access work tree '%s'"),
repo_get_work_tree(the_repository));
+
+ free(git_work_tree_cfg);
}
else {
if (real_git_dir)
diff --git a/environment.c b/environment.c
index fc3ed8bb1c..4e86335f25 100644
--- a/environment.c
+++ b/environment.c
@@ -100,9 +100,6 @@ int auto_comment_line_char;
bool warn_on_auto_comment_char;
#endif /* !WITH_BREAKING_CHANGES */
-/* This is set by setup_git_directory_gently() and/or git_default_config() */
-char *git_work_tree_cfg;
-
/*
* Repository-local GIT_* environment variables; see environment.h for details.
*/
diff --git a/environment.h b/environment.h
index ccfcf37bfb..5d6e4e6c1b 100644
--- a/environment.h
+++ b/environment.h
@@ -149,7 +149,6 @@ int have_git_dir(void);
extern int is_bare_repository_cfg;
int is_bare_repository(void);
-extern char *git_work_tree_cfg;
/* Environment bits from configuration mechanism */
extern int trust_executable_bit;
diff --git a/setup.c b/setup.c
index b4652651df..52228b42a1 100644
--- a/setup.c
+++ b/setup.c
@@ -31,6 +31,9 @@ enum allowed_bare_repo {
ALLOWED_BARE_REPO_ALL,
};
+/* This is set by setup_git_directory_gently() and/or git_default_config() */
+static char *git_work_tree_cfg;
+
static struct startup_info the_startup_info;
struct startup_info *startup_info = &the_startup_info;
const char *tmp_original_cwd;
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 0/7] setup: drop global state
From: Patrick Steinhardt @ 2026-06-10 6:56 UTC (permalink / raw)
To: git
Hi,
this patch series continues to refactor "setup.c", where the focus is to
drop remaining global state that we have in "setup.c". The most
important consequence of this is that we don't need to rely on
`the_repository` in `is_bare_repository()` anymore.
This series is built on top of 1ff279f340 (The 13th batch, 2026-06-09)
with ps/setup-centralize-odb-creation at 42b9d3dc9d (setup: construct
object database in `apply_repository_format()`, 2026-06-04) merged into
it.
Thanks!
Patrick
---
Patrick Steinhardt (7):
builtin/init: stop modifying global `git_work_tree_cfg` variable
builtin/init: simplify logic to configure worktree
setup: remove global `git_work_tree_cfg` variable
builtin/init: stop modifying `is_bare_repository_cfg`
environment: split up concerns of `is_bare_repository_cfg`
environment: stop using `the_repository` in `is_bare_repository()`
treewide: drop USE_THE_REPOSITORY_VARIABLE
attr.c | 4 ++--
builtin/bisect.c | 2 +-
builtin/blame.c | 2 +-
builtin/check-attr.c | 2 +-
builtin/fetch.c | 2 +-
builtin/gc.c | 2 +-
builtin/history.c | 2 +-
builtin/init-db.c | 44 +++++++++++++++++++++++++++-----------------
builtin/repack.c | 3 +--
builtin/repo.c | 2 +-
builtin/reset.c | 2 +-
builtin/rev-parse.c | 2 +-
environment.c | 10 +++-------
environment.h | 6 ++----
git.c | 2 +-
mailmap.c | 6 ++----
refs/files-backend.c | 2 +-
refs/reftable-backend.c | 4 +---
repository.c | 1 +
repository.h | 7 +++++++
setup.c | 45 ++++++++++++++++++++++++---------------------
setup.h | 6 ++++++
transport.c | 4 ++--
worktree.c | 4 ++--
24 files changed, 91 insertions(+), 75 deletions(-)
---
base-commit: f5a08a09a0fdf0fc2a355eba7979e2cfd65659e5
change-id: 20260422-b4-pks-setup-drop-global-state-6b1374aed5db
^ permalink raw reply
* Re: [PATCH v3 0/3] Documentation: recommend the use of b4
From: Patrick Steinhardt @ 2026-06-10 6:42 UTC (permalink / raw)
To: Toon Claes
Cc: git, Junio C Hamano, Tuomas Ahola, Weijie Yuan, Ramsay Jones,
SZEDER Gábor, Kristoffer Haugsbakk
In-Reply-To: <87a4t32a4g.fsf@emacs.iotcl.com>
On Tue, Jun 09, 2026 at 02:04:15PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> Now on the other hand, looking at a few examples I see GitGitGadget does
> deep nesting. Wouldn't it make sense to be consistent?
I've chatted with Dscho about this, and he mentioned that Stolee has
already opened [1]. So yes, if we agree to change the recommendation we
should also adapt GGG.
Patrick
[1]: https://github.com/gitgitgadget/gitgitgadget/issues/2254
^ permalink raw reply
* Re: [PATCH v2] ls-files: filter pathspec before lstat
From: Tamir Duberstein @ 2026-06-09 23:15 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe, Patrick Steinhardt, Junio C Hamano
In-Reply-To: <20260609104119.GA1509396@coredump.intra.peff.net>
On Tue, Jun 9, 2026 at 3:41 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Jun 08, 2026 at 07:37:15PM -0700, Tamir Duberstein wrote:
>
> > + /*
> > + * match_pathspec() is linear in pathspec.nr, so prefilter only
> > + * the single-pathspec case. Only entries shown by show_ce()
> > + * satisfy --error-unmatch.
> > + */
> > + if (pathspec.nr == 1 &&
> > + !match_pathspec(repo->index, &pathspec, fullname.buf,
> > + fullname.len, max_prefix_len, NULL,
> > + S_ISDIR(ce->ce_mode) ||
> > + S_ISGITLINK(ce->ce_mode)))
> > + continue;
>
> This feels...kind of arbitrary, no? Surely it's also faster with
> pathspec.nr == 2, and so on up to some nr closer to the size of the
> total index. It feels weird to be making an arbitrary cutoff based on
> pathspec performance in calling code like this.
>
> It is not wrong, per se, as you are optimizing your case without trying
> to hurt any others. But what do we do when somebody profiles it and
> comes along trying to bump the number to 2, or 10?
>
> I dunno.
Yeah, absolutely it's arbitrary. The simplest answer is that others
are welcome to bump this, provided they make the case for it.
^ permalink raw reply
* Re: [PATCH v2 0/2] unpack-trees: use explicit repository in trace2 calls
From: Junio C Hamano @ 2026-06-09 21:41 UTC (permalink / raw)
To: Jayesh Daga via GitGitGadget
Cc: git, Justin Tobler, Ayush Chandekar, Siddharth Asthana,
Jayesh Daga
In-Reply-To: <xmqqldf7y95a.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Jayesh Daga via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> Jayesh Daga (2):
>> unpack-trees: use repository from index instead of global
>> unpack-trees: use repository from index instead of global
>
> That is unusual to have two commits with identical title and
> identical proposed log messages yet with different patch text.
>
> Do you perhaps want to squash them into a single commit?
After not hearing anything for full two months, I just decided to
squash these two patches into one, as the second one looked clearly
like "ooops, I screwed up and left one pice behind, so here is an
incremental update". I'll mark the result for 'next'.
^ permalink raw reply
* [PATCH] update-ref: add --rename option
From: Junio C Hamano @ 2026-06-09 21:35 UTC (permalink / raw)
To: git
Add a "--rename" option to "git update-ref" with the syntax:
$ git update-ref --rename <old-refname> <new-refname>
It renames <old-refname> together with its reflog to <new-refname>.
The command warns to use "git branch --rename" instead if old or new
refname falls inside refs/heads/. We issue this warning (but perform
the rename anyway) because "git update-ref" acts as a low-level
plumbing utility: unlike "git branch", it does not update active
worktree "HEAD" symbolic references or ".git/config" branch tracking
configuration.
Note that we do not add a corresponding "rename" verb to the "--stdin"
mode in this commit. While adding a "rename SP <oldref> SP <newref> LF"
command to the batch stream would be useful, operations in "--stdin"
mode are executed within an atomic "ref_transaction". Reference renaming
is currently implemented at the backend level via standalone, non-transactional
calls (e.g. "refs_rename_ref()"). Supporting renames in a batch transaction
would require extending "struct ref_transaction" and the reference storage
backend APIs to coordinate ref and reflog moves atomically, which is
left for a future refactoring.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git-update-ref.adoc | 9 ++++++++
builtin/update-ref.c | 36 +++++++++++++++++++++++++++++--
t/t1400-update-ref.sh | 29 +++++++++++++++++++++++++
3 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-update-ref.adoc b/Documentation/git-update-ref.adoc
index 37a5019a8b..a622f2512b 100644
--- a/Documentation/git-update-ref.adoc
+++ b/Documentation/git-update-ref.adoc
@@ -9,6 +9,7 @@ SYNOPSIS
--------
[synopsis]
git update-ref [-m <reason>] [--no-deref] -d <ref> [<old-oid>]
+git update-ref [-m <reason>] [--no-deref] --rename <old-refname> <new-refname>
git update-ref [-m <reason>] [--no-deref] [--create-reflog] <ref> <new-oid> [<old-oid>]
git update-ref [-m <reason>] [--no-deref] --stdin [-z] [--batch-updates]
@@ -39,6 +40,14 @@ the result of following the symbolic pointers.
With `-d`, it deletes the named <ref> after verifying that it
still contains <old-oid>.
+With `--rename`, it renames <old-refname> together with its reflog to
+<new-refname>. If either <old-refname> or <new-refname> falls inside
+`refs/heads/`, a warning will be issued to use `git branch --rename`
+instead, because `git update-ref` does not update active worktree
+`HEAD` symbolic references or `.git/config` tracking settings. It
+fails if <old-refname> does not exist, or if <new-refname> already
+exists.
+
With `--stdin`, update-ref reads instructions from standard input and
performs all modifications together. Specify commands of the form:
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2d68c40ecb..1ae2dac6c5 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -15,6 +15,7 @@
static const char * const git_update_ref_usage[] = {
N_("git update-ref [<options>] -d <refname> [<old-oid>]"),
N_("git update-ref [<options>] <refname> <new-oid> [<old-oid>]"),
+ N_("git update-ref [<options>] --rename <old-refname> <new-refname>"),
N_("git update-ref [<options>] --stdin [-z] [--batch-updates]"),
NULL
};
@@ -756,13 +757,14 @@ int cmd_update_ref(int argc,
{
const char *refname, *oldval;
struct object_id oid, oldoid;
- int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0;
+ int delete = 0, rename = 0, no_deref = 0, read_stdin = 0, end_null = 0;
int create_reflog = 0;
unsigned int flags = 0;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")),
OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
+ OPT_BOOL( 0 , "rename", &rename, N_("rename the reference")),
OPT_BOOL( 0 , "no-deref", &no_deref,
N_("update <refname> not the one it points to")),
OPT_BOOL('z', NULL, &end_null, N_("stdin has NUL-terminated arguments")),
@@ -787,7 +789,7 @@ int cmd_update_ref(int argc,
}
if (read_stdin) {
- if (delete || argc > 0)
+ if (delete || rename || argc > 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
@@ -800,6 +802,36 @@ int cmd_update_ref(int argc,
if (end_null)
usage_with_options(git_update_ref_usage, options);
+ if (rename) {
+ const char *oldref, *newref;
+
+ if (delete || argc != 2)
+ usage_with_options(git_update_ref_usage, options);
+
+ oldref = argv[0];
+ newref = argv[1];
+
+ if (check_refname_format(oldref, 0))
+ die("invalid ref format: %s", oldref);
+ if (check_refname_format(newref, 0))
+ die("invalid ref format: %s", newref);
+
+ if (starts_with(oldref, "refs/heads/") ||
+ starts_with(newref, "refs/heads/"))
+ warning(_("You may want 'git branch --rename' instead?"));
+
+ if (!refs_ref_exists(get_main_ref_store(the_repository), oldref))
+ die("no ref named '%s'", oldref);
+
+ if (refs_ref_exists(get_main_ref_store(the_repository), newref))
+ die("ref '%s' already exists", newref);
+
+ if (refs_rename_ref(get_main_ref_store(the_repository),
+ oldref, newref, msg))
+ die("rename failed");
+ return 0;
+ }
+
if (delete) {
if (argc < 1 || argc > 2)
usage_with_options(git_update_ref_usage, options);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b2858a9061..fcb986d485 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2455,4 +2455,33 @@ test_expect_success 'dangling symref overwritten without old oid' '
test_must_fail git rev-parse --verify refs/heads/does-not-exist
'
+test_expect_success '--rename fails if old-refname does not exist' '
+ test_must_fail git update-ref --rename refs/tags/no-such-ref refs/tags/new-ref 2>err &&
+ test_grep "no ref named .refs/tags/no-such-ref." err
+'
+
+test_expect_success '--rename fails if new-refname does exist' '
+ git update-ref refs/tags/existing HEAD &&
+ git update-ref refs/tags/old-ref HEAD &&
+ test_must_fail git update-ref --rename refs/tags/old-ref refs/tags/existing 2>err &&
+ test_grep "ref .refs/tags/existing. already exists" err
+'
+
+test_expect_success '--rename warns if old or new refname falls inside refs/heads/' '
+ git update-ref refs/heads/old-branch HEAD &&
+ git update-ref --rename refs/heads/old-branch refs/heads/new-branch 2>err &&
+ test_grep "branch --rename. instead" err &&
+ git rev-parse --verify refs/heads/new-branch &&
+ test_must_fail git rev-parse --verify refs/heads/old-branch
+'
+
+test_expect_success '--rename moves old-refname and its reflog to new-refname' '
+ git update-ref -m "initial tag" refs/tags/old-tag HEAD &&
+ git update-ref --rename refs/tags/old-tag refs/tags/new-tag 2>err &&
+ test_must_be_empty err &&
+ git rev-parse --verify refs/tags/new-tag &&
+ test_must_fail git rev-parse --verify refs/tags/old-tag &&
+ git log -g refs/tags/new-tag
+'
+
test_done
--
2.54.0-618-gdfae347457
^ permalink raw reply related
* [GSoC Blog] week 1 and 2: Complete and extend the remote-object-info command for git cat-file
From: Pablo Sabater @ 2026-06-09 21:31 UTC (permalink / raw)
To: git
Hi!
I've posted my first and second week, sorry I forgot to post the first
week here as well.
first week: https://pablosabater.dev/posts/gsoc-week-1-coding-period/
second week: https://pablosabater.dev/posts/gsoc-week-2-coding-period/
I hope you like it and I'm here for any doubt.
Thanks,
Pablo
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] builtin/history: abort reword on same message
From: Justin Tobler @ 2026-06-09 20:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: Phillip Wood, Pablo Sabater, git, cat, ps, kaartic.sivaraam,
ben.knoble
In-Reply-To: <xmqq5x3r1ph0.fsf@gitster.g>
On 26/06/09 12:30PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
>
> > I can see a situation where a user performs:
> >
> > git history reword abcd1234
> >
> > with the intention to modify a commit message, but then for some reason
> > changes their mind and doesn't want history to change. Maybe the wrong
> > commit was referenced, or they decide the current message is actually
> > fine. From my understanding, there isn't a great way to abort rewording
> > a commit during editing and thus the user would have to reset history
> > afterwards if they care enough to go back to the previous point.
> >
> > So I do see some value in a mechanism to abort rewriting a commit
> > message.
>
> I think we are saying the same thing in different ways. I want to
> see that command "succeed" either case (normally we create a new
> commit object because we record an updated committer timestamp, but
> if there is no need to create a new commit object only to record an
> updated committer timestamp, we may choose not to and leave the
> history intact) and I do not want it to *abort*.
>
> The mechanism to do so may be exactly the same, i.e., accept an
> updated log message, then try to "hash-object" (without -w) the
> commit object with everything, except for the updated commit log
> message, taken from the original commit, plus the updated log
> message. And if the resulting hash is the same as the original, do
> not do anything further and return happily. Aborting sounds more
> like complaining loudly "baa, you asked me to reword but you gave me
> the same message? is anything wrong with you?" with non-zero exit
> status, which I think the user does not deserve in such a case.
Yes, I completely agree. If the user doesn't update the commit message,
for whatever reason, that should still be considered a success since it
follows the user's intent. I don't think it makes sense to exit with a
non-zero code in such cases. I would also question if we should print
any message/note to the user at all for the same reasons.
-Justin
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] builtin/history: abort reword on same message
From: Junio C Hamano @ 2026-06-09 19:30 UTC (permalink / raw)
To: Justin Tobler
Cc: Phillip Wood, Pablo Sabater, git, cat, ps, kaartic.sivaraam,
ben.knoble
In-Reply-To: <aihH8ye-r4QuXlRD@denethor>
Justin Tobler <jltobler@gmail.com> writes:
> I can see a situation where a user performs:
>
> git history reword abcd1234
>
> with the intention to modify a commit message, but then for some reason
> changes their mind and doesn't want history to change. Maybe the wrong
> commit was referenced, or they decide the current message is actually
> fine. From my understanding, there isn't a great way to abort rewording
> a commit during editing and thus the user would have to reset history
> afterwards if they care enough to go back to the previous point.
>
> So I do see some value in a mechanism to abort rewriting a commit
> message.
I think we are saying the same thing in different ways. I want to
see that command "succeed" either case (normally we create a new
commit object because we record an updated committer timestamp, but
if there is no need to create a new commit object only to record an
updated committer timestamp, we may choose not to and leave the
history intact) and I do not want it to *abort*.
The mechanism to do so may be exactly the same, i.e., accept an
updated log message, then try to "hash-object" (without -w) the
commit object with everything, except for the updated commit log
message, taken from the original commit, plus the updated log
message. And if the resulting hash is the same as the original, do
not do anything further and return happily. Aborting sounds more
like complaining loudly "baa, you asked me to reword but you gave me
the same message? is anything wrong with you?" with non-zero exit
status, which I think the user does not deserve in such a case.
^ permalink raw reply
* [PATCH] commit-reach: remove get_reachable_subset()
From: Kristofer Karlsson via GitGitGadget @ 2026-06-09 19:28 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Kristofer Karlsson, Kristofer Karlsson
From: Kristofer Karlsson <krka@spotify.com>
get_reachable_subset() and tips_reachable_from_bases() answer the
same question: given a set of bases and a set of tips, which tips
are reachable from at least one base?
get_reachable_subset() was introduced in fcb2c0769d (2018-11-02)
for add_missing_tags() in remote.c. tips_reachable_from_bases()
was added in cbfe360b14 (2023-03-20) as part of the ahead-behind
series. The two were never consolidated.
With a commit-graph, tips_reachable_from_bases() can have an edge:
its DFS raises the generation floor as lower targets are found,
pruning more aggressively than the static floor in
get_reachable_subset(). Without generation numbers, some edge cases
may be slower with DFS instead of BFS since the date-ordered
prio_queue naturally stays near the top of the graph, but this
should not matter in practice -- worst case both visit the full
graph down from the bases.
The flag in remote.c changes from 1 (bit 0) to TMP_MARK (bit 4)
because tips_reachable_from_bases() uses SEEN (bit 0) internally.
TMP_MARK is already used for deduplication earlier in the same
function and is cleared before the reachability check.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
commit-reach: remove get_reachable_subset()
This removes get_reachable_subset() and converts its only caller to use
tips_reachable_from_bases() directly. Both answer the same category-2
reachability question ("which tips are reachable from these bases?") but
were introduced years apart and never consolidated.
On the no-commit-graph tradeoff: without generation numbers, the
date-ordered BFS in get_reachable_subset() can be more disciplined than
DFS since it naturally stays near the top of the graph. But this only
matters for repositories that are both large enough for the difference
to be measurable and missing a commit-graph -- a combination that would
already struggle for many other reasons. The fix there is to enable the
commit-graph, not to maintain two implementations of the same
reachability query.
Notes for reviewers:
* The flag in remote.c changes from 1 to TMP_MARK because
tips_reachable_from_bases() uses SEEN (bit 0) internally. TMP_MARK is
already used earlier in the same function and is cleared before the
reachability block.
* The sent_tips array is converted to a commit_list to match the
tips_reachable_from_bases() API. This is O(n) list-node allocations,
negligible compared to the graph walk.
* Test helper and test names rename from get_reachable_subset to
tips_reachable_from_bases to match the function being exercised.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2144%2Fspkrka%2Fkrka%2Fremove-get-reachable-subset-v2-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2144/spkrka/krka/remove-get-reachable-subset-v2-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2144
commit-reach.c | 73 -------------------------------------------
commit-reach.h | 13 --------
remote.c | 19 ++++++-----
t/helper/test-reach.c | 39 +++++++++++------------
t/t6600-test-reach.sh | 18 +++++------
5 files changed, 36 insertions(+), 126 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 5df471a313..e78752eb87 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -1013,79 +1013,6 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,
return result;
}
-struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from,
- struct commit **to, size_t nr_to,
- unsigned int reachable_flag)
-{
- struct commit **item;
- struct commit *current;
- struct commit_list *found_commits = NULL;
- struct commit **to_last = to + nr_to;
- struct commit **from_last = from + nr_from;
- timestamp_t min_generation = GENERATION_NUMBER_INFINITY;
- int num_to_find = 0;
-
- struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
-
- for (item = to; item < to_last; item++) {
- timestamp_t generation;
- struct commit *c = *item;
-
- repo_parse_commit(the_repository, c);
- generation = commit_graph_generation(c);
- if (generation < min_generation)
- min_generation = generation;
-
- if (!(c->object.flags & PARENT1)) {
- c->object.flags |= PARENT1;
- num_to_find++;
- }
- }
-
- for (item = from; item < from_last; item++) {
- struct commit *c = *item;
- if (!(c->object.flags & PARENT2)) {
- c->object.flags |= PARENT2;
- repo_parse_commit(the_repository, c);
-
- prio_queue_put(&queue, *item);
- }
- }
-
- while (num_to_find && (current = prio_queue_get(&queue)) != NULL) {
- struct commit_list *parents;
-
- if (current->object.flags & PARENT1) {
- current->object.flags &= ~PARENT1;
- current->object.flags |= reachable_flag;
- commit_list_insert(current, &found_commits);
- num_to_find--;
- }
-
- for (parents = current->parents; parents; parents = parents->next) {
- struct commit *p = parents->item;
-
- repo_parse_commit(the_repository, p);
-
- if (commit_graph_generation(p) < min_generation)
- continue;
-
- if (p->object.flags & PARENT2)
- continue;
-
- p->object.flags |= PARENT2;
- prio_queue_put(&queue, p);
- }
- }
-
- clear_prio_queue(&queue);
-
- clear_commit_marks_many(nr_to, to, PARENT1);
- clear_commit_marks_many(nr_from, from, PARENT2);
-
- return found_commits;
-}
-
define_commit_slab(bit_arrays, struct bitmap *);
static struct bit_arrays bit_arrays;
diff --git a/commit-reach.h b/commit-reach.h
index 3f3a563d8a..b3e7051738 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -96,19 +96,6 @@ int can_all_from_reach_with_flag(struct object_array *from,
int can_all_from_reach(struct commit_list *from, struct commit_list *to,
int commit_date_cutoff);
-
-/*
- * Return a list of commits containing the commits in the 'to' array
- * that are reachable from at least one commit in the 'from' array.
- * Also add the given 'flag' to each of the commits in the returned list.
- *
- * This method uses the PARENT1 and PARENT2 flags during its operation,
- * so be sure these flags are not set before calling the method.
- */
-struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from,
- struct commit **to, size_t nr_to,
- unsigned int reachable_flag);
-
struct ahead_behind_count {
/**
* As input, the *_index members indicate which positions in
diff --git a/remote.c b/remote.c
index f1a3681b7c..7cdb59ed87 100644
--- a/remote.c
+++ b/remote.c
@@ -1459,9 +1459,8 @@ static void add_missing_tags(struct ref *src, struct ref **dst, struct ref ***ds
* sent to the other side.
*/
if (sent_tips.nr) {
- const int reachable_flag = 1;
- struct commit_list *found_commits;
struct commit_stack src_commits = COMMIT_STACK_INIT;
+ struct commit_list *bases = NULL;
for_each_string_list_item(item, &src_tag) {
struct ref *ref = item->util;
@@ -1479,11 +1478,12 @@ static void add_missing_tags(struct ref *src, struct ref **dst, struct ref ***ds
commit_stack_push(&src_commits, commit);
}
- found_commits = get_reachable_subset(sent_tips.items,
- sent_tips.nr,
- src_commits.items,
- src_commits.nr,
- reachable_flag);
+ for (size_t i = 0; i < sent_tips.nr; i++)
+ commit_list_insert(sent_tips.items[i], &bases);
+ tips_reachable_from_bases(the_repository,
+ bases, src_commits.items,
+ src_commits.nr, TMP_MARK);
+ commit_list_free(bases);
for_each_string_list_item(item, &src_tag) {
struct ref *dst_ref;
@@ -1503,7 +1503,7 @@ static void add_missing_tags(struct ref *src, struct ref **dst, struct ref ***ds
* Is this tag, which they do not have, reachable from
* any of the commits we are sending?
*/
- if (!(commit->object.flags & reachable_flag))
+ if (!(commit->object.flags & TMP_MARK))
continue;
/* Add it in */
@@ -1513,9 +1513,8 @@ static void add_missing_tags(struct ref *src, struct ref **dst, struct ref ***ds
}
clear_commit_marks_many(src_commits.nr, src_commits.items,
- reachable_flag);
+ TMP_MARK);
commit_stack_clear(&src_commits);
- commit_list_free(found_commits);
}
string_list_clear(&src_tag, 0);
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index 5d86a96c17..eb44a64f50 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -7,6 +7,7 @@
#include "hex.h"
#include "object-name.h"
#include "ref-filter.h"
+#include "revision.h"
#include "setup.h"
#include "string-list.h"
#include "tag.h"
@@ -149,30 +150,26 @@ int cmd__reach(int ac, const char **av)
printf("%s(_,A,X,_):%d\n", av[1], commit_contains(&filter, A, X, &cache));
clear_contains_cache(&cache);
- } else if (!strcmp(av[1], "get_reachable_subset")) {
- const int reachable_flag = 1;
- int count = 0;
- struct commit_list *current;
- struct commit_list *list = get_reachable_subset(X_stack.items, X_stack.nr,
- Y_stack.items, Y_stack.nr,
- reachable_flag);
- printf("get_reachable_subset(X,Y)\n");
- for (current = list; current; current = current->next) {
- if (!(list->item->object.flags & reachable_flag))
- die(_("commit %s is not marked reachable"),
- oid_to_hex(&list->item->object.oid));
- count++;
- }
+ } else if (!strcmp(av[1], "tips_reachable_from_bases")) {
+ struct commit_list *bases = NULL;
+ struct commit_list *result = NULL;
+
+ for (size_t i = 0; i < X_stack.nr; i++)
+ commit_list_insert(X_stack.items[i], &bases);
+ tips_reachable_from_bases(the_repository,
+ bases, Y_stack.items,
+ Y_stack.nr, TMP_MARK);
+ commit_list_free(bases);
+
+ printf("tips_reachable_from_bases(X,Y)\n");
for (size_t i = 0; i < Y_stack.nr; i++) {
- if (Y_stack.items[i]->object.flags & reachable_flag)
- count--;
+ if (Y_stack.items[i]->object.flags & TMP_MARK)
+ commit_list_insert(Y_stack.items[i], &result);
}
+ print_sorted_commit_ids(result);
- if (count < 0)
- die(_("too many commits marked reachable"));
-
- print_sorted_commit_ids(list);
- commit_list_free(list);
+ clear_commit_marks_many(Y_stack.nr, Y_stack.items, TMP_MARK);
+ commit_list_free(result);
}
object_array_clear(&X_obj);
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index b5b314e570..51b140a539 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -391,7 +391,7 @@ test_expect_success 'rev-list: symmetric difference topo-order' '
run_all_modes git rev-list --topo-order commit-3-8...commit-6-6
'
-test_expect_success 'get_reachable_subset:all' '
+test_expect_success 'tips_reachable_from_bases:all' '
cat >input <<-\EOF &&
X:commit-9-1
X:commit-8-3
@@ -403,15 +403,15 @@ test_expect_success 'get_reachable_subset:all' '
Y:commit-5-6
EOF
(
- echo "get_reachable_subset(X,Y)" &&
+ echo "tips_reachable_from_bases(X,Y)" &&
git rev-parse commit-3-3 \
commit-1-7 \
commit-5-6 | sort
) >expect &&
- test_all_modes get_reachable_subset
+ test_all_modes tips_reachable_from_bases
'
-test_expect_success 'get_reachable_subset:some' '
+test_expect_success 'tips_reachable_from_bases:some' '
cat >input <<-\EOF &&
X:commit-9-1
X:commit-8-3
@@ -422,14 +422,14 @@ test_expect_success 'get_reachable_subset:some' '
Y:commit-5-6
EOF
(
- echo "get_reachable_subset(X,Y)" &&
+ echo "tips_reachable_from_bases(X,Y)" &&
git rev-parse commit-3-3 \
commit-1-7 | sort
) >expect &&
- test_all_modes get_reachable_subset
+ test_all_modes tips_reachable_from_bases
'
-test_expect_success 'get_reachable_subset:none' '
+test_expect_success 'tips_reachable_from_bases:none' '
cat >input <<-\EOF &&
X:commit-9-1
X:commit-8-3
@@ -439,8 +439,8 @@ test_expect_success 'get_reachable_subset:none' '
Y:commit-7-6
Y:commit-2-8
EOF
- echo "get_reachable_subset(X,Y)" >expect &&
- test_all_modes get_reachable_subset
+ echo "tips_reachable_from_bases(X,Y)" >expect &&
+ test_all_modes tips_reachable_from_bases
'
test_expect_success 'for-each-ref ahead-behind:linear' '
base-commit: 600fe743028cbfb640855f659e9851522214bc0b
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH RFC v2 2/2] builtin/history: abort reword on same message
From: Junio C Hamano @ 2026-06-09 19:17 UTC (permalink / raw)
To: Pablo Sabater; +Cc: Phillip Wood, git, cat, ps, kaartic.sivaraam, ben.knoble
In-Reply-To: <CAN5EUNRz9F+njb_O=Q4DzVMec-q+rDf83Ow+MPJE4yLCBq9qww@mail.gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
>> > I wonder if we should check that the committer identity is unchanged as
>> > well in case anyone is using this to fix commits after committing with
>> > the wrong identity.
>
> I think that if you reword a commit committed by someone else but end
> up with no changes I want it to be kept as it was.
That depends on the reason why the feature to "reword" the commit is
being used, and the use case Phillip is talking about is a bit
different.
A very common mistake a new user makes when starting a repository is
to make commits before they realize that they used a wrong identity
to create them. They are happy with what they committed, except
that they want these commits to be attributed to user.{name,email}
they corrected.
Also, people often use multiple identities (e.g., corp vs personal),
and when making commits to the project for their employer they do
not want to use their personal identity (and vice versa). After
making a mistake to create commits under wrong identity, they want
to fix these commits.
In such situations, there is no room for leaving the committer name
as "someone else". The user wants to get rid of the "someone else"s
identity out of these commits.
^ permalink raw reply
* Re: [PATCH GSoC RFC v12 12/12] cat-file: make remote-object-info allow-list dynamic
From: Junio C Hamano @ 2026-06-09 18:54 UTC (permalink / raw)
To: Chandra Pratap
Cc: Pablo Sabater, eric.peijian, calvinwan, chriscool, git, jltobler,
jonathantanmy, karthik.188, toon
In-Reply-To: <CA+J6zkQ22en2HgH03EedKOfC+jLcHH2UbwpH0h_bDEAHR6B2pg@mail.gmail.com>
Chandra Pratap <chandrapratap3519@gmail.com> writes:
> On Mon, 8 Jun 2026 at 15:45, Pablo Sabater <pabloosabaterr@gmail.com> wrote:
>>
>> The static allow-list in expand_atom() is hardcoded to only allow
>> "objectname" and "objectsize" for remote queries. This works because
>> ...
>> }
You just forced readers to skip over 200+ lines of quoted material,
looking for something interesting you have said in response to
comment on the patch in vain.
>> diff --git a/fetch-object-info.c b/fetch-object-info.c
>> index 51a898430d..425929a269 100644
>> --- a/fetch-object-info.c
>> +++ b/fetch-object-info.c
>> @@ -39,6 +39,12 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
>> case protocol_v2:
>> if (!server_supports_v2("object-info"))
>> die(_("object-info capability is not enabled on the server"));
>> +
>> + for (int i = args->object_info_options->nr - 1; i >= 0; i--)
>
> Isn't args->object_info_options->nr of type size_t? We should probably
> do something
> like:
>
> for (size_t i = 0; i < args->args->object_info_options->nr; i++)
>
> instead.
This is a valid observation and a careful reading like this is very
much appreciated. It is unfortunate that it was buried by 200+
lines of irrelevant material before we find it.
Thanks.
>> + if (!server_supports_feature("object-info",
>> + args->object_info_options->items[i].string, 0))
>> + unsorted_string_list_delete_item(args->object_info_options, i, 0);
>> +
>> send_object_info_request(fd_out, args);
>> break;
>> case protocol_v1:
>>
>> --
>> 2.54.0
>
> Other than these, the patch series LGTM for now.
>
> Thanks,
> Chandra.
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] builtin/history: abort reword on same message
From: Justin Tobler @ 2026-06-09 18:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: Phillip Wood, Pablo Sabater, git, cat, ps, kaartic.sivaraam,
ben.knoble
In-Reply-To: <xmqq4ijbsn2m.fsf@gitster.g>
On 26/06/09 09:20AM, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > Hi Pablo
> >
> > On 09/06/2026 11:42, Pablo Sabater wrote:
> >> static int commit_tree_ext(struct repository *repo,
> >> @@ -135,6 +136,13 @@ static int commit_tree_ext(struct repository *repo,
> >> original_body, action, &commit_message);
> >> if (ret < 0)
> >> goto out;
> >> +
> >> + if (flags & COMMIT_TREE_ABORT_ON_SAME_MESSAGE &&
> >> + !strcmp(original_body, commit_message.buf)) {
> >> + fprintf(stderr, _("Message unchanged, aborting reword.\n"));
> >> + ret = 1;
> >> + goto out;
> >> + }
> >
> > I wonder if we should check that the committer identity is unchanged as
> > well in case anyone is using this to fix commits after committing with
> > the wrong identity.
> >
> > Aborting when the message and committer identity are unchanged seems
> > like a good idea.
>
> I am not sure why it would be a good idea. The user wanted to make
> the commit have this message, and the commit ended up having the
> same message as the user gave. That message may have been identical
> to what the commit originally had, or it may be different. Why is
> the former an abort-worthy event? A simple note, I may understand,
> but aborting with an error message?
I can see a situation where a user performs:
git history reword abcd1234
with the intention to modify a commit message, but then for some reason
changes their mind and doesn't want history to change. Maybe the wrong
commit was referenced, or they decide the current message is actually
fine. From my understanding, there isn't a great way to abort rewording
a commit during editing and thus the user would have to reset history
afterwards if they care enough to go back to the previous point.
So I do see some value in a mechanism to abort rewriting a commit
message. An unchanged commit message does seem like a reasonable signal
to essentially abort the reword. I'm not sure committer identity should
be taken into consideration though since it would inhibit a users
ability to abort the reword if they ever touch a commit that they
themselves are not the previous committer.
I don't think there is a need to have an error message though. Even in
the case where the user leaves the commit message unchanged and history
is left untouched, git-history(1) would be following exactly what the
user instructed it to do. I don't really see why the user should care
whether history was actually modified or not in such a scenario.
-Justin
^ permalink raw reply
* Re: [PATCH GSoC RFC v12 03/12] cat-file: add declaration of variable i inside its for loop
From: Pablo Sabater @ 2026-06-09 17:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: eric.peijian, calvinwan, chriscool, git, jltobler, jonathantanmy,
karthik.188, toon, chandrapratap3519
In-Reply-To: <xmqqzf15w0cz.fsf@gitster.g>
El lun, 8 jun 2026 a las 16:52, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
> > From: Eric Ju <eric.peijian@gmail.com>
> > Subject: Re: [PATCH GSoC RFC v12 03/12] cat-file: add declaration of variable i inside its for loop
>
> "add" sounds a bit strange, as the existing code wouldn't have
> compiled if the variable were never declared. What the patch did
> was to move (not add) the declaration of a function scope variable
> that is used to control for() loops. Would any of these work?
>
> Subject: [PATCH GSOC v12 03/12] cat-file: narrow scope of loop counter
> Subject: [PATCH GSOC v12 03/12] cat-file: declare loop counter inside for()
>
Hi!
True, it's better to write "move" and about the title, works as well,
I'll change them for the next version.
> > Some code used in this series declares variable i and only uses it
> > in a for loop, not in any other logic outside the loop.
> >
> > Change the declaration of i to be inside the for loop for readability.
> > While at it, we also change its type from "int" to "size_t" where the latter makes more sense.
>
> Curious single line that is overly long?
True, I'll wrap it up correctly in the next version.
>
> > Helped-by: Christian Couder <chriscool@tuxfamily.org>
> > Signed-off-by: Eric Ju <eric.peijian@gmail.com>
> > Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> > ---
> > builtin/cat-file.c | 11 +++--------
> > fetch-pack.c | 3 +--
> > 2 files changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index fa45f774d7..c060fd4800 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -726,12 +726,10 @@ static void dispatch_calls(struct batch_options *opt,
> > struct queued_cmd *cmd,
> > int nr)
> > {
> > - int i;
> > -
> > if (!opt->buffer_output)
> > die(_("flush is only for --buffer mode"));
> >
> > - for (i = 0; i < nr; i++)
> > + for (size_t i = 0; i < nr; i++)
> > cmd[i].fn(opt, cmd[i].line, output, data);
>
> The loop limit "nr" will not become as large as size_t because the
> caller passes a platform natural "int" to the function. Wouldn't a
> stupid compiler give us warning on comparing unsigned size_t with
> signed int here?
Yes, I'll change "i" to be int.
>
> > @@ -739,9 +737,7 @@ static void dispatch_calls(struct batch_options *opt,
> >
> > static void free_cmds(struct queued_cmd *cmd, size_t *nr)
> > {
> > - size_t i;
> > -
> > - for (i = 0; i < *nr; i++)
> > + for (size_t i = 0; i < *nr; i++)
> > FREE_AND_NULL(cmd[i].line);
>
> No type change, so the result is as safe as the original.
>
> > @@ -768,7 +764,6 @@ static void batch_objects_command(struct batch_options *opt,
> > size_t alloc = 0, nr = 0;
> >
> > while (strbuf_getdelim_strip_crlf(&input, stdin, opt->input_delim) != EOF) {
> > - int i;
> > const struct parse_cmd *cmd = NULL;
> > const char *p = NULL, *cmd_end;
> > struct queued_cmd call = {0};
> > @@ -778,7 +773,7 @@ static void batch_objects_command(struct batch_options *opt,
> > if (isspace(*input.buf))
> > die(_("whitespace before command: '%s'"), input.buf);
> >
> > - for (i = 0; i < ARRAY_SIZE(commands); i++) {
> > + for (size_t i = 0; i < ARRAY_SIZE(commands); i++) {
> > if (!skip_prefix(input.buf, commands[i].name, &cmd_end))
> > continue;
>
> ARRAY_SIZE() is some arithmetic over sizeof(*commands) and
> sizeof(commands), which is of type size_t, so this is better than
> the original. Use of size_t i of course is a natural way to index
> into commands[] array, so the result is just fine.
>
> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index 120e01f3cf..f13951d154 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -1388,9 +1388,8 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
> > if (advertise_sid && server_supports_v2("session-id"))
> > packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
> > if (server_options && server_options->nr) {
> > - int i;
> > ensure_server_supports_v2("server-option");
> > - for (i = 0; i < server_options->nr; i++)
> > + for (size_t i = 0; i < server_options->nr; i++)
> > packet_buf_write(req_buf, "server-option=%s",
> > server_options->items[i].string);
>
> server_options is a string_list whose .nr member is of type size_t,
> so this comparison is perfectly fine. Ditto for ->items[i].string
> that is a natural way to index into an array.
>
> > }
>
> v
Thanks,
Pablo.
^ permalink raw reply
* Re: [PATCH GSoC RFC v12 04/12] t1006: split test utility functions into new "lib-cat-file.sh"
From: Pablo Sabater @ 2026-06-09 17:44 UTC (permalink / raw)
To: Chandra Pratap
Cc: eric.peijian, calvinwan, chriscool, git, jltobler, jonathantanmy,
karthik.188, toon
In-Reply-To: <CA+J6zkQe=K80QUOH8LwXCRw9nxv3tHBg+FtfDsYedY5xdHW79A@mail.gmail.com>
El mar, 9 jun 2026 a las 8:29, Chandra Pratap
(<chandrapratap3519@gmail.com>) escribió:
>
> On Mon, 8 Jun 2026 at 15:44, Pablo Sabater <pabloosabaterr@gmail.com> wrote:
> >
> > From: Eric Ju <eric.peijian@gmail.com>
> >
> > This refactor extracts utility functions from the cat-file's test
> > script "t1006-cat-file.sh" into a new "lib-cat-file.sh" dedicated
> > library file. The goal is to improve code reuse and readability,
> > enabling future tests to leverage these utilities without duplicating
> > code.
>
> Hmm, seems like a premature change to me. Do any of the subsequent
> commits require this refactor? Maybe the follow-up series that enables
> %objecttype support needs it? Did someone request this change in v11's
> feedback?
>
> If any of those are true, I think it's worthwhile mentioning it here. That will
> make it easier to determine whether this change is truly necessary.
Yes, these functions are needed for "t1017" which is created later in
the series [1] for the remote object info tests, so they are both used
in "t1006" (where they were originally) and "t1017".
>
> > Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> > ---
> > t/lib-cat-file.sh | 16 ++++++++++++++++
> > t/t1006-cat-file.sh | 13 +------------
> > 2 files changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/t/lib-cat-file.sh b/t/lib-cat-file.sh
> > new file mode 100644
> > index 0000000000..44af232d74
> > --- /dev/null
> > +++ b/t/lib-cat-file.sh
> > @@ -0,0 +1,16 @@
> > +# Library of git-cat-file related test functions.
> > +
> > +# Print a string without a trailing newline.
> > +echo_without_newline () {
> > + printf '%s' "$*"
> > +}
> > +
> > +# Print a string without newlines and replace them with a NULL character (\0).
> > +echo_without_newline_nul () {
> > + echo_without_newline "$@" | tr '\n' '\0'
> > +}
> > +
> > +# Calculate the length of a string.
> > +strlen () {
> > + echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
> > +}
> > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> > index 8e2c52652c..8360f3bbd9 100755
> > --- a/t/t1006-cat-file.sh
> > +++ b/t/t1006-cat-file.sh
> > @@ -4,6 +4,7 @@ test_description='git cat-file'
> >
> > . ./test-lib.sh
> > . "$TEST_DIRECTORY/lib-loose.sh"
> > +. "$TEST_DIRECTORY"/lib-cat-file.sh
> >
> > test_cmdmode_usage () {
> > test_expect_code 129 "$@" 2>err &&
> > @@ -99,18 +100,6 @@ do
> > '
> > done
> >
> > -echo_without_newline () {
> > - printf '%s' "$*"
> > -}
> > -
> > -echo_without_newline_nul () {
> > - echo_without_newline "$@" | tr '\n' '\0'
> > -}
> > -
> > -strlen () {
> > - echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
> > -}
> > -
> > run_tests () {
> > type=$1
> > object_name="$2"
> >
> > --
> > 2.54.0
[1]: https://lore.kernel.org/git/20260608-ps-eric-work-rebase-v12-10-5338b766e658@gmail.com/
Thanks,
Pablo.
^ permalink raw reply
* Re: [PATCH GSoC RFC v12 12/12] cat-file: make remote-object-info allow-list dynamic
From: Pablo Sabater @ 2026-06-09 17:34 UTC (permalink / raw)
To: Chandra Pratap
Cc: eric.peijian, calvinwan, chriscool, git, jltobler, jonathantanmy,
karthik.188, toon
In-Reply-To: <CA+J6zkQ22en2HgH03EedKOfC+jLcHH2UbwpH0h_bDEAHR6B2pg@mail.gmail.com>
El mar, 9 jun 2026 a las 17:32, Chandra Pratap
(<chandrapratap3519@gmail.com>) escribió:
>
> On Mon, 8 Jun 2026 at 15:45, Pablo Sabater <pabloosabaterr@gmail.com> wrote:
> >
> > The static allow-list in expand_atom() is hardcoded to only allow
> > "objectname" and "objectsize" for remote queries. This works because
> > up to this point all servers will either support object-info with name
> > and size or they do not support them at all, but we cannot expect that
> > in a future different servers with different git versions to have the
> > same object-info capabilities. Therefore, the allow_list needs to be
> > dynamic depending on what does the server advertise.
> >
> > The client will now:
> >
> > 1. Request the protocol option that the placeholder refers to (i.e.
> > "size" when "%(objectsize)").
> >
> > 2. Filters the request in fetch_object_info() dropping any option that
> > the server does not advertise.
> >
> > 3. After the fetching, the options that haven't been dropped are the ones
> > fetched and supported by the server, these supported options are
> > mapped and remote_allowed_atoms is populated with the placeholders.
> >
> > 4. expand_atom() checks remote_allowed_atoms with the same behaviour as
> > the static allow_list had.
> >
> > Move object_info_options out of get_remote_info so the caller which has
> > data can select what options will be requested instead of requesting
> > always size.
> > Move batch_object_write() out so there will always be an output even if
> > all the placeholders are not supported by the server (returns an empty
> > line).
> >
> > Include "type" in the object_info_options so once the server supports
> > it, the clients know already how to request it.
> >
> > Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> > Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
> > Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> > ---
> > builtin/cat-file.c | 85 ++++++++++++++++++++++++++++++++---------------------
> > fetch-object-info.c | 6 ++++
> > 2 files changed, 58 insertions(+), 33 deletions(-)
> >
[snip]
> > diff --git a/fetch-object-info.c b/fetch-object-info.c
> > index 51a898430d..425929a269 100644
> > --- a/fetch-object-info.c
> > +++ b/fetch-object-info.c
> > @@ -39,6 +39,12 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
> > case protocol_v2:
> > if (!server_supports_v2("object-info"))
> > die(_("object-info capability is not enabled on the server"));
> > +
> > + for (int i = args->object_info_options->nr - 1; i >= 0; i--)
>
> Isn't args->object_info_options->nr of type size_t? We should probably
> do something
> like:
>
> for (size_t i = 0; i < args->args->object_info_options->nr; i++)
>
> instead.
Hi!
void unsorted_string_list_delete_item(struct string_list *list, int i,
int free_util)
{
if (list->strdup_strings)
free(list->items[i].string);
if (free_util)
free(list->items[i].util);
list->items[i] = list->items[list->nr-1];
list->nr--;
}
I made it backwards because of "list->items[i] = list->items[list->nr
- 1];" If we made it from 0..nr and we delete the first element, for
the next iteration, the last element is at [0] but we are on [1] and
that swapped element never gets evaluated.
About size_t, yes, it is size_t but because we go backwards 0 - 1
would fail, also unsorted_string_list_delete_item() signature has "int
i". The options that can be on that list will be a small number so
there should be no problem, should I cast it explicitly?
>
> > + if (!server_supports_feature("object-info",
> > + args->object_info_options->items[i].string, 0))
> > + unsorted_string_list_delete_item(args->object_info_options, i, 0);
> > +
> > send_object_info_request(fd_out, args);
> > break;
> > case protocol_v1:
> >
> > --
> > 2.54.0
>
> Other than these, the patch series LGTM for now.
>
> Thanks,
> Chandra.
Thanks,
Pablo.
^ permalink raw reply
* Re: git-diff in a worktree is an order of magnitude slower?
From: D. Ben Knoble @ 2026-06-09 17:15 UTC (permalink / raw)
To: Jeff King; +Cc: Git
In-Reply-To: <20260609001134.GD358144@coredump.intra.peff.net>
On Mon, Jun 8, 2026 at 8:11 PM Jeff King <peff@peff.net> wrote:
>
> On Mon, Jun 08, 2026 at 07:36:45PM -0400, D. Ben Knoble wrote:
>
> > I'd like to report and offer to help fix what I view as a serious performance
> > bug:
> >
> > "git diff --no-ext-diff --quiet" performs about ~10x slower in a secondary
> > worktree than in the main worktree.
>
> Hmm, I get the opposite effect: it is much faster in the worktree!
>
> I did:
>
> git clone /path/to/linux.git
> git -C linux worktree add --detach ../wt
> hyperfine -L dir linux,wt 'git -C {dir} diff'
>
> which yielded:
>
> Benchmark 1: git -C linux diff
> Time (mean ± σ): 188.9 ms ± 2.5 ms [User: 166.4 ms, System: 130.7 ms]
> Range (min … max): 185.5 ms … 194.8 ms 16 runs
>
> Benchmark 2: git -C wt diff
> Time (mean ± σ): 20.0 ms ± 1.5 ms [User: 23.4 ms, System: 103.5 ms]
> Range (min … max): 17.2 ms … 24.6 ms 132 runs
>
> Summary
> git -C wt diff ran
> 9.43 ± 0.71 times faster than git -C linux diff
>
> Running:
>
> perf record -g git -C wt --no-pager diff
> perf record -g git -C linux --no-pager diff
> perf diff
>
> implies that the slow case is spending a lot more time computing sha1s.
> Which implies that the entries are stat dirty. And indeed, if I run:
>
> git -C linux update-index --refresh
>
> now they both take ~20ms.
Ah, TIL about --refresh. I suppose it could be nice if "git diff"
updated the index in this way, but that sounds like a band-aid. Maybe
creating a fresh worktree should do the equivalent to make sure it's
considered "fresh"?
(This also dropped my timings down to normal.)
At $DAYJOB, I _think_ some version of "git restore <stuff>" ended up
also updating the index.
> I wonder if it's just a racy-git problem? Many files are written in the
> same second as the index, so they end up with the same mtimes, and we
> have to err on the side of checking the contents.
>
> See Documentation/technical/racy-git.adoc for a larger discussion.
>
> So it is not really about worktrees at all, but just "bad luck" in
> generating that initial index (that goes away next time you actually
> make an index update that rewrites the whole thing).
Ah, that makes sense! I'm familiar with the raciness but didn't expect it here.
> I'd have thought USE_NSEC was the default these days, but looks like it
> isn't? Try building with that and I'll bet it goes away entirely.
Thanks, I'll take a look.
I can see on my Macbook that at least Meson does automatically set
either USE_ST_TIMESPEC or NO_NSEC automatically, but has no option to
enabled USE_NSEC and try that. I can probably write that patch (which
I'll do to test), and I can send it along with the "worktree add
should refresh the index" if you think that's an appropriate thing to
do.
> > PS I almost CC'd Peff and Patrick, whose names stood out in "git
> > shortlog builtin/{worktree,diff}* object-file* | sort -t\( -k2 -g",
> > but decided they'd be their own best judge of whether they can
> > understand what's going on? :)
>
> You might be interested in "git shortlog -ns". :)
>
> -Peff
Phew! Yeah, that's much nicer. Thanks! (When typing this out for
email, I even left out the "grep '^[^[:space:]]' |" filter :P)
--
D. Ben Knoble
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] builtin/history: abort reword on same message
From: Pablo Sabater @ 2026-06-09 17:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, git, cat, ps, kaartic.sivaraam, ben.knoble
In-Reply-To: <xmqq4ijbsn2m.fsf@gitster.g>
El mar, 9 jun 2026 a las 18:20, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > Hi Pablo
> >
> > On 09/06/2026 11:42, Pablo Sabater wrote:
> >> static int commit_tree_ext(struct repository *repo,
> >> @@ -135,6 +136,13 @@ static int commit_tree_ext(struct repository *repo,
> >> original_body, action, &commit_message);
> >> if (ret < 0)
> >> goto out;
> >> +
> >> + if (flags & COMMIT_TREE_ABORT_ON_SAME_MESSAGE &&
> >> + !strcmp(original_body, commit_message.buf)) {
> >> + fprintf(stderr, _("Message unchanged, aborting reword.\n"));
> >> + ret = 1;
> >> + goto out;
> >> + }
> >
> > I wonder if we should check that the committer identity is unchanged as
> > well in case anyone is using this to fix commits after committing with
> > the wrong identity.
I think that if you reword a commit committed by someone else but end
up with no changes I want it to be kept as it was.
> >
> > Aborting when the message and committer identity are unchanged seems
> > like a good idea.
>
> I am not sure why it would be a good idea. The user wanted to make
> the commit have this message, and the commit ended up having the
> same message as the user gave. That message may have been identical
> to what the commit originally had, or it may be different. Why is
> the former an abort-worthy event? A simple note, I may understand,
> but aborting with an error message?
With what you said at [1], having this in an
"--avoid-unnecessary-rewrite" I think that the abort might be too much
as with the flag the user already expects this to happen and silent
might be better.
By the way, I feel that "--avoid-unnecessary-rewrite" is too long,
could it be something shorter? If not it could be set "-r" as the
short and leave the long as it is.
>
> Thanks.
[1]: https://lore.kernel.org/git/xmqqtsrbsvcm.fsf@gitster.g/
Thanks,
Pablo
^ 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