* Re: [PATCH v4 03/10] reset: rename `reset_head()`
From: Phillip Wood @ 2026-06-10 13:10 UTC (permalink / raw)
To: Patrick Steinhardt, git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
In-Reply-To: <20260610-b4-pks-history-drop-v4-3-70d5f0ae8c25@pks.im>
Hi Patrick
On 10/06/2026 09:52, Patrick Steinhardt wrote:
> In a subsequent commit we're about to adapt `reset_head()` so that the
> reference update to HEAD is optional, only. At this point the function
> starts to feel misnamed, as it doesn't necessarily have anything to do
> with the HEAD reference anymore. The gist of the function then is that
> we reset the working tree to a specific new commit, updating both the
> index and the checked-out files.
>
> Rename it to `reset_working_tree()` to better reflect that.
That sounds good. Because we defer renaming the flags this patch is very
straight forward.
Thanks
Phillip
> Note that we don't adjust the flags yet. This will happen in a
> subsequent commit.
>
> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
> ---
> builtin/rebase.c | 20 ++++++++++----------
> reset.c | 5 +++--
> reset.h | 4 ++--
> sequencer.c | 8 ++++----
> 4 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index fa4f5d9306..22fbba3c62 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -592,7 +592,7 @@ static int finish_rebase(struct rebase_options *opts)
> static int move_to_original_branch(struct rebase_options *opts)
> {
> struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
> - struct reset_head_opts ropts = { 0 };
> + struct reset_working_tree_options ropts = { 0 };
> int ret;
>
> if (!opts->head_name)
> @@ -610,7 +610,7 @@ static int move_to_original_branch(struct rebase_options *opts)
> ropts.flags = RESET_HEAD_REFS_ONLY;
> ropts.branch_msg = branch_reflog.buf;
> ropts.head_msg = head_reflog.buf;
> - ret = reset_head(the_repository, &ropts);
> + ret = reset_working_tree(the_repository, &ropts);
>
> strbuf_release(&branch_reflog);
> strbuf_release(&head_reflog);
> @@ -685,7 +685,7 @@ static int run_am(struct rebase_options *opts)
>
> status = run_command(&format_patch);
> if (status) {
> - struct reset_head_opts ropts = { 0 };
> + struct reset_working_tree_options ropts = { 0 };
> unlink(rebased_patches);
> free(rebased_patches);
> child_process_clear(&am);
> @@ -693,7 +693,7 @@ static int run_am(struct rebase_options *opts)
> ropts.oid = &opts->orig_head->object.oid;
> ropts.branch = opts->head_name;
> ropts.default_reflog_action = opts->reflog_action;
> - reset_head(the_repository, &ropts);
> + reset_working_tree(the_repository, &ropts);
> error(_("\ngit encountered an error while preparing the "
> "patches to replay\n"
> "these revisions:\n"
> @@ -855,7 +855,7 @@ static int rebase_config(const char *var, const char *value,
> static int checkout_up_to_date(struct rebase_options *options)
> {
> struct strbuf buf = STRBUF_INIT;
> - struct reset_head_opts ropts = { 0 };
> + struct reset_working_tree_options ropts = { 0 };
> int ret = 0;
>
> strbuf_addf(&buf, "%s: checkout %s",
> @@ -866,7 +866,7 @@ static int checkout_up_to_date(struct rebase_options *options)
> if (!ropts.branch)
> ropts.flags |= RESET_HEAD_DETACH;
> ropts.head_msg = buf.buf;
> - if (reset_head(the_repository, &ropts) < 0)
> + if (reset_working_tree(the_repository, &ropts) < 0)
> ret = error(_("could not switch to %s"), options->switch_to);
> strbuf_release(&buf);
>
> @@ -1116,7 +1116,7 @@ int cmd_rebase(int argc,
> int reschedule_failed_exec = -1;
> int allow_preemptive_ff = 1;
> int preserve_merges_selected = 0;
> - struct reset_head_opts ropts = { 0 };
> + struct reset_working_tree_options ropts = { 0 };
> struct option builtin_rebase_options[] = {
> OPT_STRING(0, "onto", &options.onto_name,
> N_("revision"),
> @@ -1385,7 +1385,7 @@ int cmd_rebase(int argc,
> rerere_clear(the_repository, &merge_rr);
> string_list_clear(&merge_rr, 1);
> ropts.flags = RESET_HEAD_HARD;
> - if (reset_head(the_repository, &ropts) < 0)
> + if (reset_working_tree(the_repository, &ropts) < 0)
> die(_("could not discard worktree changes"));
> remove_branch_state(the_repository, 0);
> if (read_basic_state(&options))
> @@ -1410,7 +1410,7 @@ int cmd_rebase(int argc,
> ropts.head_msg = head_msg.buf;
> ropts.branch = options.head_name;
> ropts.flags = RESET_HEAD_HARD;
> - if (reset_head(the_repository, &ropts) < 0)
> + if (reset_working_tree(the_repository, &ropts) < 0)
> die(_("could not move back to %s"),
> oid_to_hex(&options.orig_head->object.oid));
> strbuf_release(&head_msg);
> @@ -1880,7 +1880,7 @@ int cmd_rebase(int argc,
> RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> ropts.head_msg = msg.buf;
> ropts.default_reflog_action = options.reflog_action;
> - if (reset_head(the_repository, &ropts)) {
> + if (reset_working_tree(the_repository, &ropts)) {
> ret = error(_("Could not detach HEAD"));
> goto cleanup_autostash;
> }
> diff --git a/reset.c b/reset.c
> index 3b3cb74dab..799596398b 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -12,7 +12,7 @@
> #include "hook.h"
>
> static int update_refs(struct repository *repo,
> - const struct reset_head_opts *opts,
> + const struct reset_working_tree_options *opts,
> const struct object_id *oid,
> const struct object_id *head)
> {
> @@ -85,7 +85,8 @@ static int update_refs(struct repository *repo,
> return ret;
> }
>
> -int reset_head(struct repository *r, const struct reset_head_opts *opts)
> +int reset_working_tree(struct repository *r,
> + const struct reset_working_tree_options *opts)
> {
> const struct object_id *oid = opts->oid;
> const char *switch_to_branch = opts->branch;
> diff --git a/reset.h b/reset.h
> index a28f81829d..f130152014 100644
> --- a/reset.h
> +++ b/reset.h
> @@ -17,7 +17,7 @@
> /* Update ORIG_HEAD as well as HEAD */
> #define RESET_ORIG_HEAD (1<<4)
>
> -struct reset_head_opts {
> +struct reset_working_tree_options {
> /*
> * The commit to checkout/reset to. Defaults to HEAD.
> */
> @@ -55,6 +55,6 @@ struct reset_head_opts {
> const char *default_reflog_action;
> };
>
> -int reset_head(struct repository *r, const struct reset_head_opts *opts);
> +int reset_working_tree(struct repository *r, const struct reset_working_tree_options *opts);
>
> #endif
> diff --git a/sequencer.c b/sequencer.c
> index 1ee4b2875b..d73ecf0384 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4677,7 +4677,7 @@ static void create_autostash_internal(struct repository *r,
> if (has_unstaged_changes(r, 1) ||
> has_uncommitted_changes(r, 1)) {
> struct child_process stash = CHILD_PROCESS_INIT;
> - struct reset_head_opts ropts = { .flags = RESET_HEAD_HARD };
> + struct reset_working_tree_options ropts = { .flags = RESET_HEAD_HARD };
> struct object_id oid;
>
> strvec_pushl(&stash.args,
> @@ -4707,7 +4707,7 @@ static void create_autostash_internal(struct repository *r,
>
> if (!silent)
> printf(_("Created autostash: %s\n"), buf.buf);
> - if (reset_head(r, &ropts) < 0)
> + if (reset_working_tree(r, &ropts) < 0)
> die(_("could not reset --hard"));
> discard_index(r->index);
> if (repo_read_index(r) < 0)
> @@ -4867,7 +4867,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
> const char *onto_name, const struct object_id *onto,
> const struct object_id *orig_head)
> {
> - struct reset_head_opts ropts = {
> + struct reset_working_tree_options ropts = {
> .oid = onto,
> .orig_head = orig_head,
> .flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
> @@ -4876,7 +4876,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
> onto_name),
> .default_reflog_action = sequencer_reflog_action(opts)
> };
> - if (reset_head(r, &ropts)) {
> + if (reset_working_tree(r, &ropts)) {
> apply_autostash(rebase_path_autostash());
> sequencer_remove_state(opts);
> return error(_("could not detach HEAD"));
>
^ permalink raw reply
* Re: [PATCH] read_gitfile_gently(): return non-repo path on error
From: Junio C Hamano @ 2026-06-10 13:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, git, Tian Yuchen
In-Reply-To: <ah6WEtk2pXyViEQA@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> On Tue, Jun 02, 2026 at 02:11:59AM -0400, Jeff King wrote:
> [snip]
>> Two other points of interest.
>>
>> One, I'm not sure how useful printing the pointed-to directory is. We
>> _could_ just say:
>>
>> fatal: gitfile does not point to a valid repository: /path/to/.git
>>
>> which is enough for somebody to investigate themselves. That would
>> certainly make the patch smaller.
>
> I have to agree that the patch is somewhat gross, and I myself don't
> really see much of an issue to move to an error message like the above
> if it ends up simplifying the logic.
So we are in agreement among three of us that simplifying the code
to lose error message with dubious value would be a good way
forward.
Peff, can we have a formal [v2] then?
>> diff --git a/setup.c b/setup.c
>> index 075bf89fa9..2df6fbf595 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -1641,9 +1650,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>> return GIT_DIR_INVALID_GITFILE;
>> default:
>> if (die_on_error)
>> - read_gitfile_error_die(error_code, dir->buf, NULL);
>> - else
>> + read_gitfile_error_die(error_code, dir->buf, error_dst);
>> + else {
>> + free(error_dst);
>> return GIT_DIR_INVALID_GITFILE;
>> + }
>
> The `if` branch should also gain some curly braces here.
True.
Thanks.
^ permalink raw reply
* [PATCH v2 1/1] environment.c: move 'protect_hfs' and 'protect_ntfs' into 'repo_config_values'
From: Tian Yuchen @ 2026-06-10 12:43 UTC (permalink / raw)
To: git
Cc: phillip.wood123, Tian Yuchen, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260610124353.149874-1-cat@malon.dev>
Move the global 'protect_hfs' and 'protect_ntfs' configurations
into the repository-specific 'repo_config_values' struct.
This will help with the elimination of 'the_repository'
To ensure code readability, the getter functions
'repo_protect_hfs()' and 'repo_protect_ntfs()'
have been introduced.
For now, associated functions access this configuration by
explicitly falling back to 'the_repository', which needs to
be addressed in the future.
Note: In 't/helper/test-path-utils.c', there is a function
'protect_ntfs_hfs_benchmark()' where these two global
variables are used as loop iterators. New local variables
have been created to replace them.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
compat/mingw.c | 2 +-
environment.c | 22 ++++++++++++++++++----
environment.h | 12 ++++++++++--
read-cache.c | 7 ++++---
t/helper/test-path-utils.c | 24 +++++++++++++++---------
5 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index aa7525f419..af87df77fd 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3392,7 +3392,7 @@ int is_valid_win32_path(const char *path, int allow_literal_nul)
const char *p = path;
int preceding_space_or_period = 0, i = 0, periods = 0;
- if (!protect_ntfs)
+ if (!repo_protect_ntfs(the_repository))
return 1;
skip_dos_drive_prefix((char **)&path);
diff --git a/environment.c b/environment.c
index fc3ed8bb1c..683fe1b4d3 100644
--- a/environment.c
+++ b/environment.c
@@ -82,12 +82,10 @@ unsigned long pack_size_limit_cfg;
#ifndef PROTECT_HFS_DEFAULT
#define PROTECT_HFS_DEFAULT 0
#endif
-int protect_hfs = PROTECT_HFS_DEFAULT;
#ifndef PROTECT_NTFS_DEFAULT
#define PROTECT_NTFS_DEFAULT 1
#endif
-int protect_ntfs = PROTECT_NTFS_DEFAULT;
/*
* The character that begins a commented line in user-editable file
@@ -142,6 +140,20 @@ int is_bare_repository(void)
return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
}
+int repo_protect_ntfs(struct repository *repo)
+{
+ return repo->gitdir ?
+ repo_config_values(repo)->protect_ntfs :
+ PROTECT_NTFS_DEFAULT;
+}
+
+int repo_protect_hfs(struct repository *repo)
+{
+ return repo->gitdir ?
+ repo_config_values(repo)->protect_hfs :
+ PROTECT_HFS_DEFAULT;
+}
+
int have_git_dir(void)
{
return startup_info->have_repository
@@ -541,12 +553,12 @@ int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.protecthfs")) {
- protect_hfs = git_config_bool(var, value);
+ cfg->protect_hfs = git_config_bool(var, value);
return 0;
}
if (!strcmp(var, "core.protectntfs")) {
- protect_ntfs = git_config_bool(var, value);
+ cfg->protect_ntfs = git_config_bool(var, value);
return 0;
}
@@ -720,5 +732,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
{
cfg->attributes_file = NULL;
cfg->apply_sparse_checkout = 0;
+ cfg->protect_hfs = PROTECT_HFS_DEFAULT;
+ cfg->protect_ntfs = PROTECT_NTFS_DEFAULT;
cfg->branch_track = BRANCH_TRACK_REMOTE;
}
diff --git a/environment.h b/environment.h
index 9eb97b3869..fdd9775900 100644
--- a/environment.h
+++ b/environment.h
@@ -91,6 +91,8 @@ struct repo_config_values {
/* section "core" config values */
char *attributes_file;
int apply_sparse_checkout;
+ int protect_hfs;
+ int protect_ntfs;
/* section "branch" config values */
enum branch_track branch_track;
@@ -123,6 +125,14 @@ int git_default_config(const char *, const char *,
int git_default_core_config(const char *var, const char *value,
const struct config_context *ctx, void *cb);
+/*
+ * Getters for the `protect_hfs` and `protect_ntfs` fields of `struct repo_config_values`.
+ * They check `repo->gitdir` to prevent calling repo_config_values()
+ * before the configuration is loaded or in bare environments.
+ */
+int repo_protect_hfs(struct repository *repo);
+int repo_protect_ntfs(struct repository *repo);
+
void repo_config_values_init(struct repo_config_values *cfg);
/*
@@ -173,8 +183,6 @@ extern int pack_compression_level;
extern unsigned long pack_size_limit_cfg;
extern int precomposed_unicode;
-extern int protect_hfs;
-extern int protect_ntfs;
extern int core_sparse_checkout_cone;
extern int sparse_expect_files_outside_of_patterns;
diff --git a/read-cache.c b/read-cache.c
index 21829102ae..2c6a60c756 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1002,7 +1002,7 @@ static enum verify_path_result verify_path_internal(const char *path,
return PATH_OK;
if (is_dir_sep(c)) {
inside:
- if (protect_hfs) {
+ if (repo_protect_hfs(the_repository)) {
if (is_hfs_dotgit(path))
return PATH_INVALID;
@@ -1011,7 +1011,7 @@ static enum verify_path_result verify_path_internal(const char *path,
return PATH_INVALID;
}
}
- if (protect_ntfs) {
+ if (repo_protect_ntfs(the_repository)) {
#if defined GIT_WINDOWS_NATIVE || defined __CYGWIN__
if (c == '\\')
return PATH_INVALID;
@@ -1035,7 +1035,8 @@ static enum verify_path_result verify_path_internal(const char *path,
if (c == '\0')
return S_ISDIR(mode) ? PATH_DIR_WITH_SEP :
PATH_INVALID;
- } else if (c == '\\' && protect_ntfs) {
+ } else if (c == '\\' &&
+ repo_protect_ntfs(the_repository)) {
if (is_ntfs_dotgit(path))
return PATH_INVALID;
if (S_ISLNK(mode)) {
diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index 15eb44485c..f77b3f9d70 100644
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -250,6 +250,7 @@ static int protect_ntfs_hfs_benchmark(int argc, const char **argv)
double m[3][2], v[3][2];
uint64_t cumul;
double cumul2;
+ int ntfs, hfs;
if (argc > 1 && !strcmp(argv[1], "--with-symlink-mode")) {
file_mode = 0120000;
@@ -276,8 +277,13 @@ static int protect_ntfs_hfs_benchmark(int argc, const char **argv)
names[i][--len] = (char)(' ' + (my_random() % ('\x7f' - ' ')));
}
- for (protect_ntfs = 0; protect_ntfs < 2; protect_ntfs++)
- for (protect_hfs = 0; protect_hfs < 2; protect_hfs++) {
+ if (!the_repository->gitdir)
+ the_repository->gitdir = xstrdup(".git");
+
+ for (ntfs = 0; ntfs < 2; ntfs++)
+ for (hfs = 0; hfs < 2; hfs++) {
+ repo_config_values(the_repository)->protect_ntfs = ntfs;
+ repo_config_values(the_repository)->protect_hfs = hfs;
cumul = 0;
cumul2 = 0;
for (i = 0; i < repetitions; i++) {
@@ -285,18 +291,18 @@ static int protect_ntfs_hfs_benchmark(int argc, const char **argv)
for (j = 0; j < nr; j++)
verify_path(names[j], file_mode);
end = getnanotime();
- printf("protect_ntfs = %d, protect_hfs = %d: %lfms\n", protect_ntfs, protect_hfs, (end-begin) / (double)1e6);
+ printf("protect_ntfs = %d, protect_hfs = %d: %lfms\n", ntfs, hfs, (end-begin) / (double)1e6);
cumul += end - begin;
cumul2 += (end - begin) * (end - begin);
}
- m[protect_ntfs][protect_hfs] = cumul / (double)repetitions;
- v[protect_ntfs][protect_hfs] = my_sqrt(cumul2 / (double)repetitions - m[protect_ntfs][protect_hfs] * m[protect_ntfs][protect_hfs]);
- printf("mean: %lfms, stddev: %lfms\n", m[protect_ntfs][protect_hfs] / (double)1e6, v[protect_ntfs][protect_hfs] / (double)1e6);
+ m[ntfs][hfs] = cumul / (double)repetitions;
+ v[ntfs][hfs] = my_sqrt(cumul2 / (double)repetitions - m[ntfs][hfs] * m[ntfs][hfs]);
+ printf("mean: %lfms, stddev: %lfms\n", m[ntfs][hfs] / (double)1e6, v[ntfs][hfs] / (double)1e6);
}
- for (protect_ntfs = 0; protect_ntfs < 2; protect_ntfs++)
- for (protect_hfs = 0; protect_hfs < 2; protect_hfs++)
- printf("ntfs=%d/hfs=%d: %lf%% slower\n", protect_ntfs, protect_hfs, (m[protect_ntfs][protect_hfs] - m[0][0]) * 100 / m[0][0]);
+ for (ntfs = 0; ntfs < 2; ntfs++)
+ for (hfs = 0; hfs < 2; hfs++)
+ printf("ntfs=%d/hfs=%d: %lf%% slower\n", ntfs, hfs, (m[ntfs][hfs] - m[0][0]) * 100 / m[0][0]);
return 0;
}
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/1] environment: move protect_hfs and protect_ntfs into repo_config_values
From: Tian Yuchen @ 2026-06-10 12:43 UTC (permalink / raw)
To: git
Cc: phillip.wood123, Tian Yuchen, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260606143412.15443-1-cat@malon.dev>
Hi everyone,
This series continues the ongoing libification effort by moving the
global filesystem variables, 'protect_hfs' and 'protect_ntfs', into
'struct repo_config_values'.
Place them within the per-repository configuration structure
aligns with our goal of removing global states.
For reviewers familiar with previous libification efforts, Derrick Stolee
attempted to wrap this kind of filesystem-level variable using a
lazy-loaded global accessor get_int_config_global() [1].
However, as Glen Choo pointed out in his review of that series [2],
it is strongly preferred to use plain fields in a repository-scoped
struct over global lazy-loaders, provided those fields are properly
initialized during the setup process.
By moving these variables into repo_config_values and parsing
them eagerly, we successfully tie the filesystem security flags
to the specific repository instance without altering the timing
of configuration warnings or introducing new global states.
Thanks!
Recent related patch (environment.c: migrate 'trust_executable_bit' into 'repo_config_values'): [3]
[1] https://lore.kernel.org/git/a42dd9397d07b2dc4a0d7e75bfe1af2e46cad262.1685716420.git.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/kl6lbkhpzujf.fsf@chooglen-macbookpro.roam.corp.google.com/
[3] https://lore.kernel.org/git/20260610093635.139719-1-cat@malon.dev/
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
Tian Yuchen (1):
environment.c: move 'protect_hfs' and 'protect_ntfs' into
'repo_config_values'
compat/mingw.c | 2 +-
environment.c | 22 ++++++++++++++++++----
environment.h | 12 ++++++++++--
read-cache.c | 7 ++++---
t/helper/test-path-utils.c | 24 +++++++++++++++---------
5 files changed, 48 insertions(+), 19 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [GSoC PATCH v2 0/4] teach git repo info to handle path keys
From: Lucas Seiki Oshiro @ 2026-06-10 12:42 UTC (permalink / raw)
To: K Jayatheerth
Cc: Junio C Hamano, git, a3205153416, jltobler, kumarayushjha123,
phillip.wood, sandals
In-Reply-To: <CA+rGoLf39iQH9X-xKW7HeTS3sMv-N-QzGiqm0Y=RYGOAqDcaoA@mail.gmail.com>
Junio has a good point here.
This is a plumbing command and we should design it
for machines instead of humans.
^ permalink raw reply
* [PATCH v3] ref-filter: restore prefix-scoped iteration
From: Tamir Duberstein @ 2026-06-10 12:29 UTC (permalink / raw)
To: git
Cc: Karthik Nayak, Patrick Steinhardt, Junio C Hamano, Victoria Dye,
ZheNing Hu, Tamir Duberstein
In-Reply-To: <20260608-fix-git-branch-regression-v2-1-fd82075a8520@gmail.com>
dabecb9db2 (for-each-ref: introduce a '--start-after' option,
2025-07-15) changed branch, remote-tracking branch, and tag enumeration
from constructing an iterator with the namespace prefix to constructing
an unscoped iterator and seeking to the prefix.
The files backend constructs its loose-ref iterator with cache priming
enabled. cache_ref_iterator_begin() immediately applies the construction
prefix through cache_ref_iterator_set_prefix(), reading loose refs
beneath it before packed refs are opened. An empty prefix therefore
reads every loose ref, and a later seek cannot undo that I/O.
For these single-kind filters, construct the iterator with the namespace
prefix when start_after is not set. Keep the existing unscoped
construction for start_after, whose seek position may differ from the
namespace prefix.
With 10,000 unrelated loose refs, the p6300 tests improve as follows:
before after
branch 2.74 s 0.11 s
branch --remotes 2.81 s 0.12 s
tag 3.01 s 0.11 s
Link: https://lore.kernel.org/git/aGZidwwlToWThkn8@pks.im/
Link: https://lore.kernel.org/git/xmqqikjq7s16.fsf@gitster.g/
Link: https://lore.kernel.org/r/CAOLa=ZRHKNNymXGk31YgECjUmF9nZ8GsPUdQb7aKBH5DKMz7=w@mail.gmail.com
Fixes: dabecb9db2b2 ("for-each-ref: introduce a '--start-after' option")
Suggested-by: Karthik Nayak <karthik.188@gmail.com>
Assisted-by: Codex gpt-5.5
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
The series is based on a89346e34a (maint) because the regression has
been present in released versions since Git 2.51.0.
---
Changes in v3:
- Construct the iterator directly with the namespace prefix.
- Explain when the files backend primes its loose-ref cache.
- Condense the commit message and performance results.
- Link to v2: https://patch.msgid.link/20260608-fix-git-branch-regression-v2-1-fd82075a8520@gmail.com
Changes in v2:
- Extract local variable `store`.
- Link to v1: https://patch.msgid.link/20260605-fix-git-branch-regression-v1-1-02f40ad40929@gmail.com
---
ref-filter.c | 13 ++++++-------
t/perf/p6300-for-each-ref.sh | 39 ++++++++++++++++++++++++++++++++++++++-
2 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 1da4c0e60d..9b04e3af85 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -3316,15 +3316,14 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, refs_for
if (prefix) {
struct ref_iterator *iter;
+ struct ref_store *store = get_main_ref_store(the_repository);
- iter = refs_ref_iterator_begin(get_main_ref_store(the_repository),
- "", NULL, 0, 0);
-
- if (filter->start_after)
+ if (filter->start_after) {
+ iter = refs_ref_iterator_begin(store, "", NULL, 0, 0);
ret = start_ref_iterator_after(iter, filter->start_after);
- else
- ret = ref_iterator_seek(iter, prefix,
- REF_ITERATOR_SEEK_SET_PREFIX);
+ } else {
+ iter = refs_ref_iterator_begin(store, prefix, NULL, 0, 0);
+ }
if (!ret)
ret = do_for_each_ref_iterator(iter, fn, cb_data);
diff --git a/t/perf/p6300-for-each-ref.sh b/t/perf/p6300-for-each-ref.sh
index fa7289c752..ed9c1c6a19 100755
--- a/t/perf/p6300-for-each-ref.sh
+++ b/t/perf/p6300-for-each-ref.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-test_description='performance of for-each-ref'
+test_description='performance of ref-filter users'
. ./perf-lib.sh
test_perf_fresh_repo
@@ -84,4 +84,41 @@ test_expect_success 'pack refs' '
'
run_tests "packed"
+test_expect_success REFFILES 'setup many unrelated loose refs' '
+ git init scoped &&
+ test_commit -C scoped --no-tag base &&
+ test_seq $ref_count_per_type |
+ sed "s,.*,update refs/custom/unrelated_& HEAD," |
+ git -C scoped update-ref --stdin &&
+ git -C scoped update-ref refs/remotes/origin/main HEAD &&
+ git -C scoped update-ref refs/tags/only HEAD
+'
+
+test_perf "branch (many unrelated loose refs)" --prereq REFFILES "
+ (
+ cd scoped &&
+ for i in \$(test_seq $test_iteration_count); do
+ git branch --format='%(refname)' >/dev/null
+ done
+ )
+"
+
+test_perf "branch --remotes (many unrelated loose refs)" --prereq REFFILES "
+ (
+ cd scoped &&
+ for i in \$(test_seq $test_iteration_count); do
+ git branch --remotes --format='%(refname)' >/dev/null
+ done
+ )
+"
+
+test_perf "tag (many unrelated loose refs)" --prereq REFFILES "
+ (
+ cd scoped &&
+ for i in \$(test_seq $test_iteration_count); do
+ git tag --format='%(refname)' >/dev/null
+ done
+ )
+"
+
test_done
---
base-commit: a89346e34a937f001e5d397ee62224e3e9852040
change-id: 20260605-fix-git-branch-regression-9e4236f18091
Best regards,
--
Tamir Duberstein <tamird@gmail.com>
^ permalink raw reply related
* Re: [PATCH v2] ref-filter: restore prefix-scoped iteration
From: Tamir Duberstein @ 2026-06-10 12:25 UTC (permalink / raw)
To: Karthik Nayak
Cc: git, Patrick Steinhardt, Junio C Hamano, Victoria Dye, ZheNing Hu
In-Reply-To: <CAOLa=ZRHKNNymXGk31YgECjUmF9nZ8GsPUdQb7aKBH5DKMz7=w@mail.gmail.com>
On Wed, Jun 10, 2026 at 3:50 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>
> Tamir Duberstein <tamird@gmail.com> writes:
>
> > Commit dabecb9db2 (for-each-ref: introduce a '--start-after' option,
> > 2025-07-15) changed single-kind branch, remote-tracking branch, and tag
> > enumeration in do_filter_refs() from constructing an iterator with the
> > namespace prefix to constructing an unscoped iterator and applying the
> > prefix with ref_iterator_seek().
> >
> > Before that change, refs_for_each_fullref_in() passed the namespace
> > prefix during iterator construction. That helper has since been
> > replaced by refs_for_each_ref_ext().
> >
> > The files backend primes its loose-ref cache for the construction
> > prefix before it opens packed refs. An empty construction prefix
> > therefore reads every loose ref, and a later seek cannot undo that I/O.
> > Consequently, git branch, git branch --remotes, and git tag scale with
> > unrelated loose refs.
> >
>
> And this is the crux of the issue. Currently we do
>
> - refs_ref_iterator_begin()
> - ref_iterator_seek()
>
> And between the two `cache_ref_iterator_set_prefix()` is already called
> which caches all the loose refs. This is the IO intensive operation this
> patch tries to avoid.
>
> I think it would be worthwhile to add this information in the commit
> message.
Agreed. I will explain that `cache_ref_iterator_set_prefix()` primes
the loose-ref cache during iterator construction, before the later
seek can narrow it.
>
> >
> > Patrick Steinhardt observed during review that iterator construction
> > and seeking accepted similar strings but assigned them different state
> > semantics. Junio C Hamano then pointed out that no current command can
> > combine start_after with this single-kind path, but future branch or
> > tag support would need to keep the namespace while moving the cursor.
> >
> > Keep the existing start_after path unchanged. The iterator API cannot
> > currently seek to one string while retaining another as its prefix:
> > an unflagged seek clears the prefix, while REF_ITERATOR_SEEK_SET_PREFIX
> > replaces it with the seek string.
> >
> > For the commands affected by this regression, which do not set
> > start_after, pass the namespace prefix during iterator construction so
> > that loose refs are scoped before the packed-refs snapshot is opened.
> > This fixes the current regression without deleting the ref-filter state
> > discussed during review or changing its dormant behavior.
> >
> > Add REFFILES-gated performance cases with one branch, one
> > remote-tracking branch, one tag, and 10,000 unrelated loose refs. The
> > benchmarks were run with:
> >
> > GIT_PERF_REPEAT_COUNT=5 GIT_PERF_MAKE_OPTS=-j8 \
> > t/perf/run a89346e34a . -- p6300-for-each-ref.sh
> >
> > The following are the best of five runs, with each run invoking the
> > command ten times. Times are elapsed seconds with user and system CPU
> > seconds in parentheses:
> >
> > a89346e34a this commit
> > branch 2.74(0.13+2.56) 0.11(0.04+0.04)
> > branch --remotes 2.81(0.13+2.62) 0.12(0.04+0.04)
> > tag 3.01(0.14+2.82) 0.11(0.04+0.04)
> >
> > Both revisions used the default -O2 build flags and a config.mak
> > containing only "NO_REGEX = NeedsStartEnd". They were built with Apple
> > clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro (Mac16,6)
> > with a 16-core Apple M4 Max (12 performance and four efficiency cores)
> > and 128 GB RAM.
> >
> > Link: https://lore.kernel.org/git/aGZidwwlToWThkn8@pks.im/
> > Link: https://lore.kernel.org/git/xmqqikjq7s16.fsf@gitster.g/
> > Fixes: dabecb9db2b2 ("for-each-ref: introduce a '--start-after' option")
> > Assisted-by: Codex gpt-5.5
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> > The series is based on a89346e34a (maint) because the regression has
> > been present in released versions since Git 2.51.0.
> > ---
> > Changes in v2:
> > - Extract local variable `store`.
> > - Link to v1: https://patch.msgid.link/20260605-fix-git-branch-regression-v1-1-02f40ad40929@gmail.com
> > ---
> > ref-filter.c | 28 +++++++++++++++++++---------
> > t/perf/p6300-for-each-ref.sh | 39 ++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 57 insertions(+), 10 deletions(-)
> >
> > diff --git a/ref-filter.c b/ref-filter.c
> > index 1da4c0e60d..5cbc007d64 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -3315,19 +3315,29 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, refs_for
> > prefix = "refs/tags/";
> >
> > if (prefix) {
> > - struct ref_iterator *iter;
> > + struct ref_store *store = get_main_ref_store(the_repository);
> >
> > - iter = refs_ref_iterator_begin(get_main_ref_store(the_repository),
> > - "", NULL, 0, 0);
> > + if (filter->start_after) {
> > + struct ref_iterator *iter;
> > +
> > + iter = refs_ref_iterator_begin(store, "", NULL, 0, 0);
> >
> > - if (filter->start_after)
> > ret = start_ref_iterator_after(iter, filter->start_after);
> > - else
> > - ret = ref_iterator_seek(iter, prefix,
> > - REF_ITERATOR_SEEK_SET_PREFIX);
> > + if (!ret)
> > + ret = do_for_each_ref_iterator(iter, fn,
> > + cb_data);
> > + } else {
> > + /*
> > + * Pass the prefix during construction because the files
> > + * backend primes loose refs before a later seek can
> > + * narrow the iterator.
> > + */
> > + struct refs_for_each_ref_options opts = {
> > + .prefix = prefix,
> > + };
> >
> > - if (!ret)
> > - ret = do_for_each_ref_iterator(iter, fn, cb_data);
> > + ret = refs_for_each_ref_ext(store, fn, cb_data, &opts);
> > + }
>
> This would work, as now we separate out the regular path to use
> `do_for_each_ref_iterator()` instead.
>
> But this causes a bit of confusion, why do we need to use
> `do_for_each_ref_iterator()` and why not simply provide the prefix to
> `refs_ref_iterator_begin()`, like before?
We do not. Your version is simpler and preserves the existing iterator
flow. I have adopted it for v3. Thanks!
> [...]
>
> Thanks for the patch, this is indeed a regression we must fix and the
> benchmarks are a clear indication of it.
Thank you! I'll try not to break threading on the next roll.
^ permalink raw reply
* Re: [PATCH v2 2/2] ref-filter: memoize --contains with generations
From: Tamir Duberstein @ 2026-06-10 12:20 UTC (permalink / raw)
To: Karthik Nayak
Cc: git, Jeff King, Junio C Hamano, Victoria Dye, Derrick Stolee,
Elijah Newren
In-Reply-To: <CAOLa=ZRFSuGrqFXhTuQ7Dk5GCQQGHom++78xwONoiNdt1h_gWQ@mail.gmail.com>
On Wed, Jun 10, 2026 at 4:47 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>
> Tamir Duberstein <tamird@gmail.com> writes:
>
> > git branch and git for-each-ref call repo_is_descendant_of() for
> > each candidate selected by --contains or --no-contains. Each call
> > starts a new graph walk, so refs with shared history repeatedly
> > traverse the same commits.
> >
> > ffc4b8012d (tag: speed up --contains calculation, 2011-06-11)
> > introduced a depth-first walk for git tag that caches positive and
> > negative answers across candidates. ee2bd06b0f (ref-filter: implement
> > '--contains' option, 2015-07-07) preserved both implementations when
> > ref-filter learned --contains.
> >
> > The memoized walk is not always faster. Without generation numbers,
> > a negative check can walk to the root even when the breadth-first
> > merge-base walk finds a nearby divergence. With generation numbers,
> > the depth-first walk can stop below the oldest target while still
> > reusing answers across candidates.
> >
> > Keep the existing memoized selection for git tag. Select it for other
> > ref-filter callers when generation numbers are enabled, and retain
> > the breadth-first walk otherwise.
> >
> > When generation numbers are unavailable, repo_is_descendant_of() can
> > return -1 if ancestry cannot be read. The ref-filter Boolean interface
> > treated that error as a match. Check it and exit instead. The memoized
> > path already dies on the same parse failure, so both selected paths now
> > fail rather than return a result.
> >
> > Add p1500 cases for up to 8,192 packed refs along one first-parent
> > history and for sibling refs near the tip with generation numbers
> > forced off.
> >
> > On a checkout with 62,174 remote-tracking refs and generation numbers
> > enabled, I ran:
> >
> > hyperfine --warmup 0 --runs 3 \
> > --command-name parent \
> > '"$parent" branch -r --contains c78ae85f3ce7e >/dev/null' \
> > --command-name this-commit \
> > '"$this" branch -r --contains c78ae85f3ce7e >/dev/null'
> >
> > The results were:
> >
> > parent this commit
> > elapsed 104.365 s 467.7 ms
> > user 93.702 s 220.2 ms
> > system 0.723 s 182.7 ms
> >
> > The wall-time standard deviations were 11.356 seconds and 133.8
> > milliseconds, respectively. Separate runs without redirection produced
> > the same output with SHA-256
> > 2466f6e2b72aa16b1a2126eddb81c8a1b2764ee251204ac034c191a925aa896f.
> >
> > Both revisions were built with the default -O2 flags using Apple
> > clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro (Mac16,6)
> > with a 16-core Apple M4 Max (12 performance and four efficiency
> > cores) and 128 GB RAM.
> >
> > Link: https://lore.kernel.org/git/1445163904-24611-1-git-send-email-Karthik.188@gmail.com/
> > Link: https://lore.kernel.org/r/20230324191009.GA536967@coredump.intra.peff.net
> > Link: https://lore.kernel.org/git/20260527070510.3510836-1-krka@spotify.com/
> > Link: https://lore.kernel.org/r/20260608223430.GA340696@coredump.intra.peff.net
> > Suggested-by: Jeff King <peff@peff.net>
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> > commit-reach.c | 13 +++++++++--
> > commit-reach.h | 7 ++++++
> > t/perf/p1500-graph-walks.sh | 49 +++++++++++++++++++++++++++++++++++++++++-
> > t/t6301-for-each-ref-errors.sh | 22 +++++++++++++++++++
> > 4 files changed, 88 insertions(+), 3 deletions(-)
> >
> > diff --git a/commit-reach.c b/commit-reach.c
> > index 65b618959b..83a48004ef 100644
> > --- a/commit-reach.c
> > +++ b/commit-reach.c
> > @@ -821,9 +821,18 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
> > int commit_contains(struct ref_filter *filter, struct commit *commit,
> > struct commit_list *list, struct contains_cache *cache)
> > {
> > - if (filter->with_commit_tag_algo)
> > + int result;
> > +
> > + if (!list)
> > + return 1;
> > + if (filter->with_commit_tag_algo ||
> > + generation_numbers_enabled(the_repository))
>
> What's stopping us from dropping `filter->with_commit_tag_algo`
> completely and then doing?
>
> if (generation_numbers_enabled(the_repository))
> return contains_algo(commit, list, cache) == CONTAINS_YES;
> return repo_is_descendant_of(the_repository, commit, list);
Jeff raised this distinction during the v1 review:
https://lore.kernel.org/r/20260608223430.GA340696@coredump.intra.peff.net/
`with_commit_tag_algo` preserves the existing behavior of `git tag` when
generation numbers are unavailable. `git tag --contains` has used the
memoized walk since ffc4b8012d (tag: speed up --contains calculation,
2011-06-11). Dropping the flag would send it back through repeated
`repo_is_descendant_of()` walks in repositories without usable generation
numbers.
The condition in v2 implements the rule discussed there: retain the
existing memoized path for `git tag`, and use it for other ref-filter
callers when generation numbers make the depth-first walk reliably
advantageous.
This is probably my fault for breaking the threading between this and
v1. Sorry about that.
^ permalink raw reply
* Re: [GSoC PATCH v2 3/4] repo: add path.gitdir with absolute and relative suffix formatting
From: K Jayatheerth @ 2026-06-10 12:11 UTC (permalink / raw)
To: Justin Tobler
Cc: git, a3205153416, gitster, kumarayushjha123, lucasseikioshiro,
phillip.wood, sandals
In-Reply-To: <aighAZXRtLaz6sg8@denethor>
>
> Intercepting PATH_FORMAT_DEFAULT in print_path() and overriding it to
> the appropriate default needed for the specific path printed by
> git-rev-parse(1), as shown above, seems reasonable to me.
>
> But I do think that PATH_FORMAT_DEFAULT should have an actual default in
> format_path(). Otherwise we would have an enum value that requires
> callers to explicitly handle prior to invoking format_path() which would
> also be rather awkward. IMO, it probably wouldn't be a big deal to just
> say PATH_FORMAT_DEFAULT is treated as PATH_FORMAT_UNMODIFIED when passed
> to format_path() and document it. In practice, our rev-parse use-case
> would always replace PATH_FORMAT_DEFAULT with the appropriate value
> prior to invoking format_path().
>
Makes sense. I'll have format_path() treat PATH_FORMAT_DEFAULT as
PATH_FORMAT_UNMODIFIED
internally and document it, so there's no unhandled enum value.
print_path() in rev-parse will still intercept PATH_FORMAT_DEFAULT
and replace it with the path-specific default before calling format_path(),
so in practice it won't fall through to the unmodified behavior.
Almost done with v3 this was the only one left.
Thanks for taking time!
^ permalink raw reply
* Re: [PATCH v2 2/2] ref-filter: memoize --contains with generations
From: Karthik Nayak @ 2026-06-10 11:47 UTC (permalink / raw)
To: Tamir Duberstein, git
Cc: Jeff King, Junio C Hamano, Victoria Dye, Derrick Stolee,
Elijah Newren
In-Reply-To: <20260608-ref-filter-memoized-contains-v2-2-e72720344a7c@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4109 bytes --]
Tamir Duberstein <tamird@gmail.com> writes:
> git branch and git for-each-ref call repo_is_descendant_of() for
> each candidate selected by --contains or --no-contains. Each call
> starts a new graph walk, so refs with shared history repeatedly
> traverse the same commits.
>
> ffc4b8012d (tag: speed up --contains calculation, 2011-06-11)
> introduced a depth-first walk for git tag that caches positive and
> negative answers across candidates. ee2bd06b0f (ref-filter: implement
> '--contains' option, 2015-07-07) preserved both implementations when
> ref-filter learned --contains.
>
> The memoized walk is not always faster. Without generation numbers,
> a negative check can walk to the root even when the breadth-first
> merge-base walk finds a nearby divergence. With generation numbers,
> the depth-first walk can stop below the oldest target while still
> reusing answers across candidates.
>
> Keep the existing memoized selection for git tag. Select it for other
> ref-filter callers when generation numbers are enabled, and retain
> the breadth-first walk otherwise.
>
> When generation numbers are unavailable, repo_is_descendant_of() can
> return -1 if ancestry cannot be read. The ref-filter Boolean interface
> treated that error as a match. Check it and exit instead. The memoized
> path already dies on the same parse failure, so both selected paths now
> fail rather than return a result.
>
> Add p1500 cases for up to 8,192 packed refs along one first-parent
> history and for sibling refs near the tip with generation numbers
> forced off.
>
> On a checkout with 62,174 remote-tracking refs and generation numbers
> enabled, I ran:
>
> hyperfine --warmup 0 --runs 3 \
> --command-name parent \
> '"$parent" branch -r --contains c78ae85f3ce7e >/dev/null' \
> --command-name this-commit \
> '"$this" branch -r --contains c78ae85f3ce7e >/dev/null'
>
> The results were:
>
> parent this commit
> elapsed 104.365 s 467.7 ms
> user 93.702 s 220.2 ms
> system 0.723 s 182.7 ms
>
> The wall-time standard deviations were 11.356 seconds and 133.8
> milliseconds, respectively. Separate runs without redirection produced
> the same output with SHA-256
> 2466f6e2b72aa16b1a2126eddb81c8a1b2764ee251204ac034c191a925aa896f.
>
> Both revisions were built with the default -O2 flags using Apple
> clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro (Mac16,6)
> with a 16-core Apple M4 Max (12 performance and four efficiency
> cores) and 128 GB RAM.
>
> Link: https://lore.kernel.org/git/1445163904-24611-1-git-send-email-Karthik.188@gmail.com/
> Link: https://lore.kernel.org/r/20230324191009.GA536967@coredump.intra.peff.net
> Link: https://lore.kernel.org/git/20260527070510.3510836-1-krka@spotify.com/
> Link: https://lore.kernel.org/r/20260608223430.GA340696@coredump.intra.peff.net
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
> commit-reach.c | 13 +++++++++--
> commit-reach.h | 7 ++++++
> t/perf/p1500-graph-walks.sh | 49 +++++++++++++++++++++++++++++++++++++++++-
> t/t6301-for-each-ref-errors.sh | 22 +++++++++++++++++++
> 4 files changed, 88 insertions(+), 3 deletions(-)
>
> diff --git a/commit-reach.c b/commit-reach.c
> index 65b618959b..83a48004ef 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -821,9 +821,18 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
> int commit_contains(struct ref_filter *filter, struct commit *commit,
> struct commit_list *list, struct contains_cache *cache)
> {
> - if (filter->with_commit_tag_algo)
> + int result;
> +
> + if (!list)
> + return 1;
> + if (filter->with_commit_tag_algo ||
> + generation_numbers_enabled(the_repository))
What's stopping us from dropping `filter->with_commit_tag_algo`
completely and then doing?
if (generation_numbers_enabled(the_repository))
return contains_algo(commit, list, cache) == CONTAINS_YES;
return repo_is_descendant_of(the_repository, commit, list);
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v3 3/3] b4: introduce configuration for the Git project
From: Karthik Nayak @ 2026-06-10 11:13 UTC (permalink / raw)
To: Patrick Steinhardt, git
Cc: Junio C Hamano, Tuomas Ahola, Weijie Yuan, Ramsay Jones,
SZEDER Gábor, Kristoffer Haugsbakk, Toon Claes
In-Reply-To: <20260608-pks-b4-v3-3-f5e497d10c56@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> We're about to extend our documentation to recommend b4 for sending
Nit: This is in the past now
> patch series to the mailing list. Prepare for this by introducing a b4
> configuration so that the tool knows to honor our preferences. For now,
> this configuration does two things:
>
> - It configures "send-same-thread = shallow", which tells b4 to always
> send subsequent versions of the same patch series as a reply to the
> cover letter of the first version.
>
> - It configures "prep-cover-template", which tells b4 to use a custom
> template for the cover letter. The most important change compared to
> the default template is that our custom template also includes a
> range-diff.
>
> There's potentially more things that we may want to configure going
> forward, like for example auto-configuration of folks to Cc on certain
> patches. But these two tweaks feel like a good place to start.
>
> Note that these values only serve as defaults, and users may want to
> tweak those defaults based on their own preference. Luckily, users can
> do that without having to touch `.b4-config` at all, as b4 allows them
> to override values via Git configuration:
>
> ```
> $ git config set b4.prep-cover-template /does/not/exist
> $ b4 send --dry-run
> ERROR: prep-cover-template says to use x, but it does not exist
> ```
>
> So this gives users an easy way to override our defaults without having
> to touch ".b4-config", which would dirty the tree.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> .b4-config | 6 ++++++
> .b4-cover-template | 11 +++++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/.b4-config b/.b4-config
> new file mode 100644
> index 0000000000..fd4fb56b6d
> --- /dev/null
> +++ b/.b4-config
> @@ -0,0 +1,6 @@
> +# Note that these are default values that you can tweak via the typical
> +# git-config(1) machinery. You thus shouldn't ever have to change this file.
> +# See also https://b4.docs.kernel.org/en/latest/config.html.
> +[b4]
> +send-same-thread = shallow
> +prep-cover-template = ./.b4-cover-template
> diff --git a/.b4-cover-template b/.b4-cover-template
> new file mode 100644
> index 0000000000..ab864933b5
> --- /dev/null
> +++ b/.b4-cover-template
> @@ -0,0 +1,11 @@
> +${cover}
> +
> +---
> +${shortlog}
> +
> +${diffstat}
> +
> +${range_diff}
> +---
> +base-commit: ${base_commit}
> +${prerequisites}
>
This looks similar to what I have locally too, happy to see this land.
> --
> 2.54.0.1136.gdb2ca164c4.dirty
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/3] MyFirstContribution: recommend shallow threading of cover letters
From: Karthik Nayak @ 2026-06-10 11:08 UTC (permalink / raw)
To: Patrick Steinhardt, git
Cc: Junio C Hamano, Tuomas Ahola, Weijie Yuan, Ramsay Jones,
SZEDER Gábor, Kristoffer Haugsbakk, Toon Claes
In-Reply-To: <20260608-pks-b4-v3-1-f5e497d10c56@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3654 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> The "MyFirstContribution" document recommends the use of deep threading
> of cover letters: every cover letter of subsequent iterations shall be
> linked to the cover letter of the preceding version. The result of this
> is that eventually, threads with many versions are getting nested so
> deep that it becomes hard to follow.
>
> Adapt the recommendation to instead propose shallow threading of cover
> letters: instead of linking the cover letter to the previous cover
> letter, the user is supposed to always link it to the first cover
> letter. This still makes it easy to follow the iterations, but has the
> benefit of nesting to a much shallower level.
Should we also modify 'Documentation/SubmittingPatches'? Which states:
All subsequent versions of a patch series and other related patches
should be grouped into their own e-mail thread to help readers find
all parts of the series. To that end, send them as replies to either
an additional "cover letter" message (see below), the first patch, or
the respective preceding patch. Here is a
link:MyFirstContribution.html#v2-git-send-email[step-by-step guide] on
how to submit updated versions of a patch series.
Personally, I find it a bit awkward when new versions are sent as a new
separate thread, especially when the subject is changed over versions.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> Documentation/MyFirstContribution.adoc | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc
> index b9fdefce02..984b7f5aa8 100644
> --- a/Documentation/MyFirstContribution.adoc
> +++ b/Documentation/MyFirstContribution.adoc
> @@ -790,7 +790,7 @@ We can note a few things:
> v3", etc. in place of "PATCH". For example, "[PATCH v2 1/3]" would be the first of
> three patches in the second iteration. Each iteration is sent with a new cover
> letter (like "[PATCH v2 0/3]" above), itself a reply to the cover letter of the
> - previous iteration (more on that below).
> + first iteration (more on that below).
>
> NOTE: A single-patch topic is sent with "[PATCH]", "[PATCH v2]", etc. without
> _i_/_n_ numbering (in the above thread overview, no single-patch topic appears,
> @@ -1214,7 +1214,7 @@ between your last version and now, if it's something significant. You do not
> need the exact same body in your second cover letter; focus on explaining to
> reviewers the changes you've made that may not be as visible.
>
> -You will also need to go and find the Message-ID of your previous cover letter.
> +You will also need to go and find the Message-ID of your first cover letter.
> You can either note it when you send the first series, from the output of `git
> send-email`, or you can look it up on the
> https://lore.kernel.org/git[mailing list]. Find your cover letter in the
> @@ -1227,8 +1227,8 @@ Message-ID: <foo.12345.author@example.com>
>
> Your Message-ID is `<foo.12345.author@example.com>`. This example will be used
> below as well; make sure to replace it with the correct Message-ID for your
> -**previous cover letter** - that is, if you're sending v2, use the Message-ID
> -from v1; if you're sending v3, use the Message-ID from v2.
> +**first cover letter** - that is, for any subsequent version that you send,
> +always use the Message-ID from v1.
>
> While you're looking at the email, you should also note who is CC'd, as it's
> common practice in the mailing list to keep all CCs on a thread. You can add
>
> --
> 2.54.0.1136.gdb2ca164c4.dirty
The patch looks good.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* [BUG] rebase --update-refs emits unqualified "update-ref HEAD" into the todo
From: betel_taxis4h @ 2026-06-10 11:00 UTC (permalink / raw)
To: git
What did you do before the bug happened? (Steps to reproduce your issue)
With rebase.updateRefs=true, an interactive rebase of the checked-out branch generates a todo containing the literal line "update-ref HEAD”, which git's own todo parser then rejects.
Minimal reproduction (plain repo, no worktrees, no remotes required):
git init -b main repro && cd repro
git -c user.email=t@t.t -c user.name=t commit --allow-empty -m base
git checkout -b feat
git -c user.email=t@t.t -c user.name=t commit --allow-empty -m c1
git -c user.email=t@t.t -c user.name=t commit --allow-empty -m c2
git -c rebase.updateRefs=true rebase -i feat~2
The generated todo contains:
pick <c1> c1
pick <c2> c2
update-ref HEAD <-- emitted for HEAD, a symref to the branch being rebased
update-ref refs/heads/feat (correctly placed; this one is fine)
Letting the editor save the auto-generated todo verbatim (or running `git rebase --continue`) fails immediately with:
error: update-ref requires a fully qualified refname e.g. refs/heads/HEAD
error: invalid line 3: update-ref HEAD
You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.
What did you expect to happen? (Expected behavior)
--update-refs should not emit an "update-ref HEAD" line. HEAD is a symbolic alias of the branch being rebased; the branch ref itself is (correctly) excluded from the update-ref set, so its HEAD alias should be excluded too. The todo should contain only fully-qualified refs/heads/... lines.
What happened instead? (Actual behavior)
git emits a todo line ("update-ref HEAD") that its own sequencer parser rejects as not fully qualified, breaking the rebase. The only recovery is `git rebase --edit-todo` to manually delete the line.
What's different between what you expected and what actually happened?
git generated a todo command it refuses to execute. The unqualified "HEAD" should either be expanded to its target ref or omitted entirely.
Anything else you want to add:
- Reproduces identically in a plain single-worktree repo and in a bare-repo + linked-worktree layout, so it is not worktree-specific.
- An in-sync remote-tracking ref (origin/feat) on the tip adds a second, valid "update-ref refs/remotes/origin/feat" line but is not required to trigger the fatal "update-ref HEAD".
- Workaround: unset rebase.updateRefs (or pass -c rebase.updateRefs=false), or delete the "update-ref HEAD" line via `git rebase --edit-todo`.
[System Info]
git version:
git version 2.54.0
cpu: aarch64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
rust: disabled
gettext: enabled
libcurl: 8.14.1
OpenSSL: OpenSSL 3.5.6 7 Apr 2026
zlib: 1.3.1
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1
uname: Linux 7.0.11-orbstack-00360-gc9bc4d96ac70 #1 SMP PREEMPT Thu Jun 4 16:40:25 UTC 2026 aarch64
compiler info: gnuc: 14.2
libc info: glibc: 2.41
$SHELL (typically, interactive shell): /usr/bin/zsh
^ permalink raw reply
* Re: [PATCH v2] ref-filter: restore prefix-scoped iteration
From: Karthik Nayak @ 2026-06-10 10:50 UTC (permalink / raw)
To: Tamir Duberstein, git
Cc: Patrick Steinhardt, Junio C Hamano, Victoria Dye, ZheNing Hu
In-Reply-To: <20260608-fix-git-branch-regression-v2-1-fd82075a8520@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8571 bytes --]
Tamir Duberstein <tamird@gmail.com> writes:
> Commit dabecb9db2 (for-each-ref: introduce a '--start-after' option,
> 2025-07-15) changed single-kind branch, remote-tracking branch, and tag
> enumeration in do_filter_refs() from constructing an iterator with the
> namespace prefix to constructing an unscoped iterator and applying the
> prefix with ref_iterator_seek().
>
> Before that change, refs_for_each_fullref_in() passed the namespace
> prefix during iterator construction. That helper has since been
> replaced by refs_for_each_ref_ext().
>
> The files backend primes its loose-ref cache for the construction
> prefix before it opens packed refs. An empty construction prefix
> therefore reads every loose ref, and a later seek cannot undo that I/O.
> Consequently, git branch, git branch --remotes, and git tag scale with
> unrelated loose refs.
>
And this is the crux of the issue. Currently we do
- refs_ref_iterator_begin()
- ref_iterator_seek()
And between the two `cache_ref_iterator_set_prefix()` is already called
which caches all the loose refs. This is the IO intensive operation this
patch tries to avoid.
I think it would be worthwhile to add this information in the commit
message.
>
> Patrick Steinhardt observed during review that iterator construction
> and seeking accepted similar strings but assigned them different state
> semantics. Junio C Hamano then pointed out that no current command can
> combine start_after with this single-kind path, but future branch or
> tag support would need to keep the namespace while moving the cursor.
>
> Keep the existing start_after path unchanged. The iterator API cannot
> currently seek to one string while retaining another as its prefix:
> an unflagged seek clears the prefix, while REF_ITERATOR_SEEK_SET_PREFIX
> replaces it with the seek string.
>
> For the commands affected by this regression, which do not set
> start_after, pass the namespace prefix during iterator construction so
> that loose refs are scoped before the packed-refs snapshot is opened.
> This fixes the current regression without deleting the ref-filter state
> discussed during review or changing its dormant behavior.
>
> Add REFFILES-gated performance cases with one branch, one
> remote-tracking branch, one tag, and 10,000 unrelated loose refs. The
> benchmarks were run with:
>
> GIT_PERF_REPEAT_COUNT=5 GIT_PERF_MAKE_OPTS=-j8 \
> t/perf/run a89346e34a . -- p6300-for-each-ref.sh
>
> The following are the best of five runs, with each run invoking the
> command ten times. Times are elapsed seconds with user and system CPU
> seconds in parentheses:
>
> a89346e34a this commit
> branch 2.74(0.13+2.56) 0.11(0.04+0.04)
> branch --remotes 2.81(0.13+2.62) 0.12(0.04+0.04)
> tag 3.01(0.14+2.82) 0.11(0.04+0.04)
>
> Both revisions used the default -O2 build flags and a config.mak
> containing only "NO_REGEX = NeedsStartEnd". They were built with Apple
> clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro (Mac16,6)
> with a 16-core Apple M4 Max (12 performance and four efficiency cores)
> and 128 GB RAM.
>
> Link: https://lore.kernel.org/git/aGZidwwlToWThkn8@pks.im/
> Link: https://lore.kernel.org/git/xmqqikjq7s16.fsf@gitster.g/
> Fixes: dabecb9db2b2 ("for-each-ref: introduce a '--start-after' option")
> Assisted-by: Codex gpt-5.5
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
> The series is based on a89346e34a (maint) because the regression has
> been present in released versions since Git 2.51.0.
> ---
> Changes in v2:
> - Extract local variable `store`.
> - Link to v1: https://patch.msgid.link/20260605-fix-git-branch-regression-v1-1-02f40ad40929@gmail.com
> ---
> ref-filter.c | 28 +++++++++++++++++++---------
> t/perf/p6300-for-each-ref.sh | 39 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 1da4c0e60d..5cbc007d64 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -3315,19 +3315,29 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, refs_for
> prefix = "refs/tags/";
>
> if (prefix) {
> - struct ref_iterator *iter;
> + struct ref_store *store = get_main_ref_store(the_repository);
>
> - iter = refs_ref_iterator_begin(get_main_ref_store(the_repository),
> - "", NULL, 0, 0);
> + if (filter->start_after) {
> + struct ref_iterator *iter;
> +
> + iter = refs_ref_iterator_begin(store, "", NULL, 0, 0);
>
> - if (filter->start_after)
> ret = start_ref_iterator_after(iter, filter->start_after);
> - else
> - ret = ref_iterator_seek(iter, prefix,
> - REF_ITERATOR_SEEK_SET_PREFIX);
> + if (!ret)
> + ret = do_for_each_ref_iterator(iter, fn,
> + cb_data);
> + } else {
> + /*
> + * Pass the prefix during construction because the files
> + * backend primes loose refs before a later seek can
> + * narrow the iterator.
> + */
> + struct refs_for_each_ref_options opts = {
> + .prefix = prefix,
> + };
>
> - if (!ret)
> - ret = do_for_each_ref_iterator(iter, fn, cb_data);
> + ret = refs_for_each_ref_ext(store, fn, cb_data, &opts);
> + }
This would work, as now we separate out the regular path to use
`do_for_each_ref_iterator()` instead.
But this causes a bit of confusion, why do we need to use
`do_for_each_ref_iterator()` and why not simply provide the prefix to
`refs_ref_iterator_begin()`, like before?
On top of master, the below diff seems to fix the issue and works with
the benchmarks provided in this patch. (I haven't tested it with out
test suite though).
modified ref-filter.c
@@ -3316,15 +3316,16 @@ static int do_filter_refs(struct ref_filter
*filter, unsigned int type, refs_for
if (prefix) {
struct ref_iterator *iter;
+ struct ref_store *store;
- iter = refs_ref_iterator_begin(get_main_ref_store(the_repository),
- "", NULL, 0, 0);
+ store = get_main_ref_store(the_repository);
- if (filter->start_after)
+ if (filter->start_after) {
+ iter = refs_ref_iterator_begin(store, "", NULL, 0, 0);
ret = start_ref_iterator_after(iter, filter->start_after);
- else
- ret = ref_iterator_seek(iter, prefix,
- REF_ITERATOR_SEEK_SET_PREFIX);
+ } else {
+ iter = refs_ref_iterator_begin(store, prefix, NULL, 0, 0);
+ }
if (!ret)
ret = do_for_each_ref_iterator(iter, fn, cb_data);
I would say something like this would make more sense, since it still
keeps the current structure without introducing a new command.
> } else if (filter->kind & FILTER_REFS_REGULAR) {
> ret = for_each_fullref_in_pattern(filter, fn, cb_data);
> }
> diff --git a/t/perf/p6300-for-each-ref.sh b/t/perf/p6300-for-each-ref.sh
> index fa7289c752..ed9c1c6a19 100755
> --- a/t/perf/p6300-for-each-ref.sh
> +++ b/t/perf/p6300-for-each-ref.sh
> @@ -1,6 +1,6 @@
> #!/bin/sh
>
> -test_description='performance of for-each-ref'
> +test_description='performance of ref-filter users'
> . ./perf-lib.sh
>
> test_perf_fresh_repo
> @@ -84,4 +84,41 @@ test_expect_success 'pack refs' '
> '
> run_tests "packed"
>
> +test_expect_success REFFILES 'setup many unrelated loose refs' '
> + git init scoped &&
> + test_commit -C scoped --no-tag base &&
> + test_seq $ref_count_per_type |
> + sed "s,.*,update refs/custom/unrelated_& HEAD," |
> + git -C scoped update-ref --stdin &&
> + git -C scoped update-ref refs/remotes/origin/main HEAD &&
> + git -C scoped update-ref refs/tags/only HEAD
> +'
> +
> +test_perf "branch (many unrelated loose refs)" --prereq REFFILES "
> + (
> + cd scoped &&
> + for i in \$(test_seq $test_iteration_count); do
> + git branch --format='%(refname)' >/dev/null
> + done
> + )
> +"
> +
> +test_perf "branch --remotes (many unrelated loose refs)" --prereq REFFILES "
> + (
> + cd scoped &&
> + for i in \$(test_seq $test_iteration_count); do
> + git branch --remotes --format='%(refname)' >/dev/null
> + done
> + )
> +"
> +
> +test_perf "tag (many unrelated loose refs)" --prereq REFFILES "
> + (
> + cd scoped &&
> + for i in \$(test_seq $test_iteration_count); do
> + git tag --format='%(refname)' >/dev/null
> + done
> + )
> +"
> +
> test_done
>
> ---
> base-commit: a89346e34a937f001e5d397ee62224e3e9852040
> change-id: 20260605-fix-git-branch-regression-9e4236f18091
>
> Best regards,
> --
> Tamir Duberstein <tamird@gmail.com>
Thanks for the patch, this is indeed a regression we must fix and the
benchmarks are a clear indication of it.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* [PATCH v2 3/3] environment: move trust_executable_bit into repo_config_values
From: Tian Yuchen @ 2026-06-10 9:36 UTC (permalink / raw)
To: git; +Cc: ps, Tian Yuchen, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260610093635.139719-1-cat@malon.dev>
Move the global 'trust_executable_bit' configurations
into the repository-specific 'repo_config_values'
struct. To ensure code readability, the getter functions
'repo_trust_executable_bit()' has been introduced.
For now, associated functions access this configuration by
explicitly falling back to 'the_repository'.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
apply.c | 2 +-
environment.c | 11 +++++++++--
environment.h | 9 ++++++++-
read-cache.c | 8 ++++----
4 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/apply.c b/apply.c
index 249248d4f2..fbb907d3c0 100644
--- a/apply.c
+++ b/apply.c
@@ -3893,7 +3893,7 @@ static int check_preimage(struct apply_state *state,
if (*ce && !(*ce)->ce_mode)
BUG("ce_mode == 0 for path '%s'", old_name);
- if (trust_executable_bit || !S_ISREG(st->st_mode))
+ if (repo_trust_executable_bit(the_repository) || !S_ISREG(st->st_mode))
st_mode = ce_mode_from_stat(*ce, st->st_mode);
else if (*ce)
st_mode = (*ce)->ce_mode;
diff --git a/environment.c b/environment.c
index fc3ed8bb1c..75069a884d 100644
--- a/environment.c
+++ b/environment.c
@@ -41,7 +41,6 @@
static int pack_compression_seen;
static int zlib_compression_seen;
-int trust_executable_bit = 1;
int trust_ctime = 1;
int check_stat = 1;
int has_symlinks = 1;
@@ -142,6 +141,13 @@ int is_bare_repository(void)
return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
}
+int repo_trust_executable_bit(struct repository *repo)
+{
+ return repo->gitdir?
+ repo_config_values(repo)->trust_executable_bit :
+ 1;
+}
+
int have_git_dir(void)
{
return startup_info->have_repository
@@ -305,7 +311,7 @@ int git_default_core_config(const char *var, const char *value,
/* This needs a better name */
if (!strcmp(var, "core.filemode")) {
- trust_executable_bit = git_config_bool(var, value);
+ cfg->trust_executable_bit = git_config_bool(var, value);
return 0;
}
if (!strcmp(var, "core.trustctime")) {
@@ -720,5 +726,6 @@ void repo_config_values_init(struct repo_config_values *cfg)
{
cfg->attributes_file = NULL;
cfg->apply_sparse_checkout = 0;
+ cfg->trust_executable_bit = 1;
cfg->branch_track = BRANCH_TRACK_REMOTE;
}
diff --git a/environment.h b/environment.h
index 123a71cdc8..44b97be654 100644
--- a/environment.h
+++ b/environment.h
@@ -91,6 +91,7 @@ struct repo_config_values {
/* section "core" config values */
char *attributes_file;
int apply_sparse_checkout;
+ int trust_executable_bit;
/* section "branch" config values */
enum branch_track branch_track;
@@ -123,6 +124,13 @@ int git_default_config(const char *, const char *,
int git_default_core_config(const char *var, const char *value,
const struct config_context *ctx, void *cb);
+/*
+ * Getters for the `repo_trust_executable_bit` fields of `struct repo_config_values`.
+ * They check `repo->gitdir` to prevent calling repo_config_values()
+ * before the configuration is loaded or in bare environments.
+ */
+int repo_trust_executable_bit(struct repository *repo);
+
void repo_config_values_init(struct repo_config_values *cfg);
/*
@@ -160,7 +168,6 @@ int is_bare_repository(void);
extern char *git_work_tree_cfg;
/* Environment bits from configuration mechanism */
-extern int trust_executable_bit;
extern int trust_ctime;
extern int check_stat;
extern int has_symlinks;
diff --git a/read-cache.c b/read-cache.c
index cb4f4878c8..89f5c88c58 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -214,7 +214,7 @@ unsigned int ce_mode_from_stat(const struct cache_entry *ce, unsigned int mode)
if (!has_symlinks && S_ISREG(mode) &&
ce && S_ISLNK(ce->ce_mode))
return ce->ce_mode;
- if (!trust_executable_bit && S_ISREG(mode)) {
+ if (!repo_trust_executable_bit(the_repository) && S_ISREG(mode)) {
if (ce && S_ISREG(ce->ce_mode))
return ce->ce_mode;
return create_ce_mode(0666);
@@ -228,7 +228,7 @@ static unsigned int st_mode_from_ce(const struct cache_entry *ce)
case S_IFLNK:
return has_symlinks ? S_IFLNK : (S_IFREG | 0644);
case S_IFREG:
- return (ce->ce_mode & (trust_executable_bit ? 0755 : 0644)) | S_IFREG;
+ return (ce->ce_mode & (repo_trust_executable_bit(the_repository) ? 0755 : 0644)) | S_IFREG;
case S_IFGITLINK:
return S_IFDIR | 0755;
case S_IFDIR:
@@ -338,7 +338,7 @@ static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)
/* We consider only the owner x bit to be relevant for
* "mode changes"
*/
- if (trust_executable_bit &&
+ if (repo_trust_executable_bit(the_repository) &&
(0100 & (ce->ce_mode ^ st->st_mode)))
changed |= MODE_CHANGED;
break;
@@ -759,7 +759,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
ce->ce_flags |= CE_INTENT_TO_ADD;
- if (trust_executable_bit && has_symlinks) {
+ if (repo_trust_executable_bit(the_repository) && has_symlinks) {
ce->ce_mode = create_ce_mode(st_mode);
} else {
/* If there is an existing entry, pick the mode bits and type
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/3] read-cache: move 'ce_mode_from_stat()' to 'read-cache.c'
From: Tian Yuchen @ 2026-06-10 9:36 UTC (permalink / raw)
To: git; +Cc: ps, Tian Yuchen, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260610093635.139719-1-cat@malon.dev>
The ce_mode_from_stat() function is declared as a static inline function
in 'read-cache.h'. As we want to migrate configuration variables, this
helper function will need access to corresponding repository-specific
configuration logic. Move the implementation to 'read-cache.c' to
cleanly encapsulate its dependencies.
Note that the 'extern int trust_executable_bit, has_symlinks;' line is
discarded because it's not necessary when the function lives in
"read-cache.c".
At present, this change has no visible impact, but it is crucial
for our future plans to pass in the repo context. Comment
has been added whilst we are at it.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
read-cache.c | 20 ++++++++++++++++++++
read-cache.h | 16 ++--------------
2 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index c44e4d128f..cb4f4878c8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -202,6 +202,26 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st
}
}
+/*
+ * Determine the appropriate index mode for a file based on its stat()
+ * information and the existing cache entry (if any).
+ *
+ * This function handles degradation for filesystems that lack
+ * symlink support or reliable executable bits.
+ */
+unsigned int ce_mode_from_stat(const struct cache_entry *ce, unsigned int mode)
+{
+ if (!has_symlinks && S_ISREG(mode) &&
+ ce && S_ISLNK(ce->ce_mode))
+ return ce->ce_mode;
+ if (!trust_executable_bit && S_ISREG(mode)) {
+ if (ce && S_ISREG(ce->ce_mode))
+ return ce->ce_mode;
+ return create_ce_mode(0666);
+ }
+ return create_ce_mode(mode);
+}
+
static unsigned int st_mode_from_ce(const struct cache_entry *ce)
{
switch (ce->ce_mode & S_IFMT) {
diff --git a/read-cache.h b/read-cache.h
index 043da1f1aa..3c4af2faeb 100644
--- a/read-cache.h
+++ b/read-cache.h
@@ -5,20 +5,8 @@
#include "object.h"
#include "pathspec.h"
-static inline unsigned int ce_mode_from_stat(const struct cache_entry *ce,
- unsigned int mode)
-{
- extern int trust_executable_bit, has_symlinks;
- if (!has_symlinks && S_ISREG(mode) &&
- ce && S_ISLNK(ce->ce_mode))
- return ce->ce_mode;
- if (!trust_executable_bit && S_ISREG(mode)) {
- if (ce && S_ISREG(ce->ce_mode))
- return ce->ce_mode;
- return create_ce_mode(0666);
- }
- return create_ce_mode(mode);
-}
+unsigned int ce_mode_from_stat(const struct cache_entry *ce,
+ unsigned int mode);
static inline int ce_to_dtype(const struct cache_entry *ce)
{
--
2.43.0
^ permalink raw reply related
* [PATCH v2 1/3] read-cache: remove redundant extern declarations
From: Tian Yuchen @ 2026-06-10 9:36 UTC (permalink / raw)
To: git; +Cc: ps, Tian Yuchen, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260610093635.139719-1-cat@malon.dev>
The 'read-cache.c' file already includes 'environment.h', which provides
the extern declarations for variables like 'trust_executable_bit' and
'has_symlinks'.
Remove the redundant extern declarations inside 'st_mode_from_ce()' to
clean up the code.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
read-cache.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 38a04b8de3..c44e4d128f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -204,8 +204,6 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st
static unsigned int st_mode_from_ce(const struct cache_entry *ce)
{
- extern int trust_executable_bit, has_symlinks;
-
switch (ce->ce_mode & S_IFMT) {
case S_IFLNK:
return has_symlinks ? S_IFLNK : (S_IFREG | 0644);
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/3] environment: migrate 'trust_executable_bit' into 'repo_config_values'
From: Tian Yuchen @ 2026-06-10 9:36 UTC (permalink / raw)
To: git; +Cc: ps, Tian Yuchen, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260530160520.77859-1-cat@malon.dev>
The 'core.filemode' (stored as 'trust_executable_bit') configuration
act as a core filesystem capability flag.
This series moves it into 'struct repo_config_values' to tie it to
the specific repository instance it was read from. Eager parsing
is maintained because this flag is heavily consulted in hot paths.
Note: 'repo_config_values()' still does not support any struct
repository other than the_repository due to how deeply these flags
are accessed. In other words, this series of patches is laying
the groundwork for the eventual elimination of the_repository.
Previous related work:
[PATCH 2/6] config: add trust_executable_bit to global config [1]
[PATCH] Refactor 'trust_executable_bit' to repository-scoped setting [2]
(This previous attempt was unsuccessful because the target location
selected was 'struct repo_settings', which our analysis indicated
was not the optimal choice. For further details, please see: [3])
Changes since V1:
Dropped istate plumbing: Dropped the last commit ("read-cache: pass
'istate' to stat/mode helper functions") to keep the patch focused.
Introduced safe getter: Getter function 'repo_trust_executable_bit()'
has been introduced to retrieve configuration values safely.
Added comments for 'ce_mode_from_stat()'.
Thanks!
[1] https://lore.kernel.org/git/837b5360b40f992351f489a0ae05fedf49884c6e.1685716420.git.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/20260301190017.53539-1-dronarajgyawali@gmail.com/
[3] https://lore.kernel.org/git/xmqq1pht6nyx.fsf@gitster.g/
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
Tian Yuchen (3):
read-cache: remove redundant extern declarations
read-cache: move 'ce_mode_from_stat()' to 'read-cache.c'
environment: move trust_executable_bit into repo_config_values
apply.c | 2 +-
environment.c | 11 +++++++++--
environment.h | 9 ++++++++-
read-cache.c | 28 +++++++++++++++++++++++-----
read-cache.h | 16 ++--------------
5 files changed, 43 insertions(+), 23 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] builtin/history: abort reword on same message
From: Phillip Wood @ 2026-06-10 9:33 UTC (permalink / raw)
To: Patrick Steinhardt, Junio C Hamano
Cc: Pablo Sabater, git, cat, kaartic.sivaraam, ben.knoble
In-Reply-To: <aikMLBCC9Rc7q9S7@pks.im>
Hi Patrick
On 10/06/2026 08:03, Patrick Steinhardt wrote:
> 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?
For the case I was talking about we'd want to reset the committer but I
agree that making "reset the committer" explicit and just comparing the
old and new messages when deciding whether to skip rewriting is a good
way forward.
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] builtin/history: abort reword on same message
From: Phillip Wood @ 2026-06-10 9:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pablo Sabater, git, cat, ps, kaartic.sivaraam, ben.knoble
In-Reply-To: <xmqq4ijbsn2m.fsf@gitster.g>
On 09/06/2026 17:20, 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?
What I really meant was skipping rewriting history makes sense, I don't
have a strong opinion on the exit code. My feeling is that some kind of
message saying we haven't rewritten anything probably a good idea.
Thanks
Phillip
^ permalink raw reply
* [PATCH v4 10/10] builtin/history: implement "drop" subcommand
From: Patrick Steinhardt @ 2026-06-10 8:52 UTC (permalink / raw)
To: git; +Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
In-Reply-To: <20260610-b4-pks-history-drop-v4-0-70d5f0ae8c25@pks.im>
A common operation when editing the commit history is to drop a specific
commit from the history entirely, but this operation is not currently
covered by git-history(1).
A couple of noteworthy bits:
- This is the first git-history(1) command that will ultimately result
in changes to both the index and the working tree. We thus have to
add logic to merge resulting changes into those.
- It is still not possible to replay merge commits, so this limitation
is inherited for the new "drop" command.
- For now we refuse to drop root commits. While we _can_ indeed drop
root commits in the general case, there are edge cases where the
resulting history would become completely empty. This is thus left
to a subsequent patch series.
Other than that, most of the logic is rather straight-forward as we can
continue to build on the preexisting logic in git-history(1) for most of
the part.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-history.adoc | 38 ++-
builtin/history.c | 186 ++++++++++++++
t/meson.build | 1 +
t/t3454-history-drop.sh | 537 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 761 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc
index 2ba8121795..28b477cd37 100644
--- a/Documentation/git-history.adoc
+++ b/Documentation/git-history.adoc
@@ -8,6 +8,7 @@ git-history - EXPERIMENTAL: Rewrite history
SYNOPSIS
--------
[synopsis]
+git history drop <commit> [--dry-run] [--update-refs=(branches|head)] [--empty=(drop|keep|abort)]
git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)]
git history reword <commit> [--dry-run] [--update-refs=(branches|head)]
git history split <commit> [--dry-run] [--update-refs=(branches|head)] [--] [<pathspec>...]
@@ -51,13 +52,28 @@ be stateful operations. The limitation can be lifted once (if) Git learns about
first-class conflicts.
When using `fixup` with `--empty=drop`, dropping the root commit is not yet
-supported.
+supported. Likewise, `drop` cannot remove the root commit or a merge commit.
COMMANDS
--------
The following commands are available to rewrite history in different ways:
+`drop <commit>`::
+ Remove the specified commit from the history. All descendants of the
+ commit are replayed directly onto its parent.
++
+The root commit cannot be dropped as that may lead to edge cases where refs
+end up with no commits anymore. Merge commits cannot be dropped either; see
+LIMITATIONS.
++
+If `HEAD` points at a commit that is to be rewritten, the index and working
+tree are updated to match the new `HEAD`. The command aborts before any
+references are updated in case local modifications would be overwritten.
++
+If replaying any descendant would result in a conflict, the command aborts
+with an error.
+
`fixup <commit>`::
Apply the currently staged changes to the specified commit. This is
similar in nature to `git commit --fixup=<commit>` followed by `git
@@ -170,6 +186,26 @@ The staged addition of `unrelated.txt` has been incorporated into the `first`
commit. All descendant commits have been replayed on top of the rewritten
history.
+Drop a commit
+~~~~~~~~~~~~~
+
+----------
+$ git log --oneline
+abc1234 (HEAD -> main) third
+def5678 second
+ghi9012 first
+
+$ git history drop 'main^{/second}'
+
+$ git log --oneline
+jkl3456 (HEAD -> main) third
+ghi9012 first
+----------
+
+The `second` commit has been removed from the history, and `third` has been
+replayed directly on top of `first`. All branches that pointed at the dropped
+commit have been moved to its parent.
+
Split a commit
~~~~~~~~~~~~~~
diff --git a/builtin/history.c b/builtin/history.c
index 4fadf38c32..e59f69233b 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -17,13 +17,17 @@
#include "read-cache.h"
#include "refs.h"
#include "replay.h"
+#include "reset.h"
#include "revision.h"
#include "sequencer.h"
#include "strvec.h"
#include "tree.h"
+#include "tree-walk.h"
#include "unpack-trees.h"
#include "wt-status.h"
+#define GIT_HISTORY_DROP_USAGE \
+ N_("git history drop <commit> [--dry-run] [--update-refs=(branches|head)] [--empty=(drop|keep|abort)]")
#define GIT_HISTORY_FIXUP_USAGE \
N_("git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)]")
#define GIT_HISTORY_REWORD_USAGE \
@@ -1001,12 +1005,193 @@ static int cmd_history_split(int argc,
return ret;
}
+static int update_worktree(struct repository *repo,
+ const struct commit *old_head,
+ const struct commit *new_head,
+ bool dry_run)
+{
+ struct reset_working_tree_options opts = {
+ .oid_from = &old_head->object.oid,
+ .oid = &new_head->object.oid,
+ };
+ if (dry_run)
+ opts.flags |= RESET_WORKING_TREE_DRY_RUN;
+ return reset_working_tree(repo, &opts);
+}
+
+static int find_head_tree_change(struct repository *repo,
+ const struct replay_result *result,
+ struct commit **old_head,
+ struct commit **new_head,
+ bool *changed)
+{
+ const struct replay_ref_update *head_update = NULL;
+ struct commit *old_head_commit, *new_head_commit;
+ struct tree *old_head_tree, *new_head_tree;
+ const char *head_target;
+ int head_flags;
+
+ *changed = false;
+
+ head_target = refs_resolve_ref_unsafe(get_main_ref_store(repo),
+ "HEAD", RESOLVE_REF_NO_RECURSE,
+ NULL, &head_flags);
+ if (!head_target)
+ return error(_("cannot look up HEAD"));
+ if (!(head_flags & REF_ISSYMREF))
+ head_target = "HEAD";
+
+ for (size_t i = 0; i < result->updates_nr; i++) {
+ if (!strcmp(result->updates[i].refname, head_target)) {
+ head_update = &result->updates[i];
+ break;
+ }
+ }
+
+ if (!head_update)
+ return 0;
+
+ old_head_commit = lookup_commit_reference(repo, &head_update->old_oid);
+ new_head_commit = lookup_commit_reference(repo, &head_update->new_oid);
+ if (!old_head_commit || !new_head_commit)
+ return error(_("cannot resolve HEAD commit"));
+
+ old_head_tree = repo_get_commit_tree(repo, old_head_commit);
+ new_head_tree = repo_get_commit_tree(repo, new_head_commit);
+ if (!old_head_tree || !new_head_tree)
+ return error(_("cannot resolve tree for HEAD"));
+
+ if (oideq(&old_head_tree->object.oid, &new_head_tree->object.oid))
+ return 0;
+
+ *old_head = old_head_commit;
+ *new_head = new_head_commit;
+ *changed = true;
+
+ return 0;
+}
+
+static int cmd_history_drop(int argc,
+ const char **argv,
+ const char *prefix,
+ struct repository *repo)
+{
+ const char * const usage[] = {
+ GIT_HISTORY_DROP_USAGE,
+ NULL,
+ };
+ enum replay_empty_commit_action empty = REPLAY_EMPTY_COMMIT_DROP;
+ enum ref_action action = REF_ACTION_DEFAULT;
+ int dry_run = 0;
+ struct option options[] = {
+ OPT_CALLBACK_F(0, "update-refs", &action, "(branches|head)",
+ N_("control which refs should be updated"),
+ PARSE_OPT_NONEG, parse_ref_action),
+ OPT_BOOL('n', "dry-run", &dry_run,
+ N_("perform a dry-run without updating any refs")),
+ OPT_CALLBACK_F(0, "empty", &empty, "(drop|keep|abort)",
+ N_("how to handle descendants that become empty"),
+ PARSE_OPT_NONEG, parse_opt_empty),
+ OPT_END(),
+ };
+ struct strbuf reflog_msg = STRBUF_INIT;
+ struct commit *original, *rewritten;
+ struct rev_info revs = { 0 };
+ struct replay_result result = { 0 };
+ struct commit *old_head, *new_head;
+ bool head_moves = false;
+ int ret;
+
+ argc = parse_options(argc, argv, prefix, options, usage, 0);
+ if (argc != 1) {
+ ret = error(_("command expects a single revision"));
+ goto out;
+ }
+ repo_config(repo, git_default_config, NULL);
+
+ if (action == REF_ACTION_DEFAULT)
+ action = REF_ACTION_BRANCHES;
+
+ original = lookup_commit_reference_by_name(argv[0]);
+ if (!original) {
+ ret = error(_("commit cannot be found: %s"), argv[0]);
+ goto out;
+ }
+
+ if (!original->parents) {
+ ret = error(_("cannot drop root commit %s: "
+ "it has no parent to replay onto"),
+ argv[0]);
+ goto out;
+ } else if (original->parents->next) {
+ ret = error(_("cannot drop merge commit: %s"), argv[0]);
+ goto out;
+ }
+
+ ret = setup_revwalk(repo, action, original, &revs);
+ if (ret)
+ goto out;
+
+ rewritten = original->parents->item;
+
+ ret = compute_pending_ref_updates(&revs, action, original, rewritten,
+ empty, &result);
+ if (ret) {
+ ret = error(_("failed replaying descendants"));
+ goto out;
+ }
+
+ /*
+ * If HEAD will move as a result of the rewrite then we'll have to
+ * merge in the changes into the worktree and index. This merge can of
+ * course conflict, which will cause the whole operation to abort.
+ *
+ * If we had already updated the refs at that point then we'd have an
+ * inconsistent repository state. So we first perform a dry-run merge
+ * here before updating refs.
+ */
+ if (!is_bare_repository()) {
+ ret = find_head_tree_change(repo, &result, &old_head,
+ &new_head, &head_moves);
+ if (ret < 0)
+ goto out;
+
+ if (head_moves && update_worktree(repo, old_head, new_head, true) < 0) {
+ ret = error(_("dropping this commit would "
+ "overwrite local changes; aborting"));
+ goto out;
+ }
+ }
+
+ strbuf_addf(&reflog_msg, "drop: dropping %s", argv[0]);
+ ret = apply_pending_ref_updates(repo, &result, reflog_msg.buf, dry_run);
+ if (ret < 0) {
+ ret = error(_("failed to update references"));
+ goto out;
+ }
+
+ if (!dry_run && head_moves && update_worktree(repo, old_head, new_head, false) < 0) {
+ ret = error(_("could not update working tree to new commit %s"),
+ oid_to_hex(&new_head->object.oid));
+ goto out;
+ }
+
+ ret = 0;
+
+out:
+ replay_result_release(&result);
+ strbuf_release(&reflog_msg);
+ release_revisions(&revs);
+ return ret;
+}
+
int cmd_history(int argc,
const char **argv,
const char *prefix,
struct repository *repo)
{
const char * const usage[] = {
+ GIT_HISTORY_DROP_USAGE,
GIT_HISTORY_FIXUP_USAGE,
GIT_HISTORY_REWORD_USAGE,
GIT_HISTORY_SPLIT_USAGE,
@@ -1014,6 +1199,7 @@ int cmd_history(int argc,
};
parse_opt_subcommand_fn *fn = NULL;
struct option options[] = {
+ OPT_SUBCOMMAND("drop", &fn, cmd_history_drop),
OPT_SUBCOMMAND("fixup", &fn, cmd_history_fixup),
OPT_SUBCOMMAND("reword", &fn, cmd_history_reword),
OPT_SUBCOMMAND("split", &fn, cmd_history_split),
diff --git a/t/meson.build b/t/meson.build
index 2af8d01279..d5e71056b2 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -399,6 +399,7 @@ integration_tests = [
't3451-history-reword.sh',
't3452-history-split.sh',
't3453-history-fixup.sh',
+ 't3454-history-drop.sh',
't3500-cherry.sh',
't3501-revert-cherry-pick.sh',
't3502-cherry-pick-merge.sh',
diff --git a/t/t3454-history-drop.sh b/t/t3454-history-drop.sh
new file mode 100755
index 0000000000..0f33247212
--- /dev/null
+++ b/t/t3454-history-drop.sh
@@ -0,0 +1,537 @@
+#!/bin/sh
+
+test_description='tests for git-history drop subcommand'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-log-graph.sh"
+
+expect_graph () {
+ cat >expect &&
+ lib_test_cmp_graph --format=%s "$@"
+}
+
+expect_log () {
+ git log --format="%s" "$@" >actual &&
+ cat >expect &&
+ test_cmp expect actual
+}
+
+test_expect_success 'errors on missing commit argument' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_must_fail git history drop 2>err &&
+ test_grep "command expects a single revision" err
+ )
+'
+
+test_expect_success 'errors on too many arguments' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_must_fail git history drop HEAD HEAD 2>err &&
+ test_grep "command expects a single revision" err
+ )
+'
+
+test_expect_success 'errors on unknown revision' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_must_fail git history drop does-not-exist 2>err &&
+ test_grep "commit cannot be found: does-not-exist" err
+ )
+'
+
+test_expect_success 'errors with invalid --empty= value' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_commit second &&
+ test_must_fail git history drop --empty=bogus HEAD 2>err &&
+ test_grep "unrecognized.*--empty.*bogus" err
+ )
+'
+
+test_expect_success 'drops a commit in the middle and replays descendants' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+
+ git symbolic-ref HEAD >expect &&
+ git history drop HEAD~ &&
+ git symbolic-ref HEAD >actual &&
+ test_cmp expect actual &&
+
+ expect_log <<-\EOF &&
+ third
+ first
+ EOF
+
+ test_must_fail git show HEAD:second.t &&
+ test_path_is_missing second.t &&
+
+ git reflog >reflog &&
+ test_grep "drop: dropping HEAD~" reflog
+ )
+'
+
+test_expect_success 'drops the HEAD commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+
+ git history drop HEAD &&
+
+ expect_log <<-\EOF
+ first
+ EOF
+ )
+'
+
+test_expect_success 'drops a commit on detached HEAD' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+ git checkout --detach HEAD &&
+
+ git history drop HEAD~ &&
+
+ expect_log <<-\EOF
+ third
+ first
+ EOF
+ )
+'
+
+# Note: in this case it would actually be fine to drop the root commit, as we
+# do have a descendant commit, and no reference points to the root commit
+# directly. So this is something that we may relax eventually.
+test_expect_success 'refuses to drop the root commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+
+ test_must_fail git history drop HEAD~ 2>err &&
+ test_grep "cannot drop root commit" err
+ )
+'
+
+# In contrast to the above case, we actually don't want to drop the root commit
+# here as that would cause us to end up with an empty commit graph.
+test_expect_success 'refuses to drop the root commit when branch becomes empty' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+
+ test_must_fail git history drop HEAD 2>err &&
+ test_grep "cannot drop root commit" err
+ )
+'
+
+test_expect_success 'refuses to drop a merge commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ git branch branch &&
+ test_commit ours &&
+ git switch branch &&
+ test_commit theirs &&
+ git switch - &&
+ git merge theirs &&
+
+ test_must_fail git history drop HEAD 2>err &&
+ test_grep "cannot drop merge commit" err
+ )
+'
+
+test_expect_success 'refuses when descendants contain a merge commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit middle &&
+ git branch branch &&
+ test_commit ours &&
+ git switch branch &&
+ test_commit theirs &&
+ git switch - &&
+ git merge theirs &&
+
+ test_must_fail git history drop middle 2>err &&
+ test_grep "replaying merge commits is not supported yet" err
+ )
+'
+
+test_expect_success 'works in a bare repository' '
+ test_when_finished "rm -rf repo repo.git" &&
+
+ git init repo &&
+ test_commit -C repo first &&
+ test_commit -C repo second &&
+ test_commit -C repo third &&
+
+ git clone --bare repo repo.git &&
+ (
+ cd repo.git &&
+
+ git history drop HEAD~ &&
+ expect_log <<-\EOF
+ third
+ first
+ EOF
+ )
+'
+
+test_expect_success 'updates branches on other lines of descent' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit target &&
+ git branch theirs &&
+ test_commit ours &&
+ git switch theirs &&
+ test_commit theirs &&
+
+ expect_graph --branches <<-\EOF &&
+ * theirs
+ | * ours
+ |/
+ * target
+ * base
+ EOF
+
+ git history drop target &&
+
+ expect_graph --branches <<-\EOF
+ * ours
+ | * theirs
+ |/
+ * base
+ EOF
+ )
+'
+
+test_expect_success 'moves branch pointing at dropped commit to its parent' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ git branch points-at-second &&
+ test_commit third &&
+
+ git rev-parse first >expect &&
+ git history drop second &&
+ git rev-parse points-at-second >actual &&
+ test_cmp expect actual &&
+
+ expect_log --format="%s %D" --branches <<-\EOF
+ third HEAD -> main
+ first tag: first, points-at-second
+ EOF
+ )
+'
+
+test_expect_success '--dry-run prints ref updates without modifying repo' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit base &&
+ git branch branch &&
+ test_commit middle &&
+ test_commit ours &&
+ git switch branch &&
+ test_commit theirs &&
+
+ git refs list >refs-expect &&
+ git history drop --dry-run main~ >updates &&
+ git refs list >refs-actual &&
+ test_cmp refs-expect refs-actual &&
+ test_grep "update refs/heads/main" updates &&
+
+ git update-ref --stdin <updates &&
+ expect_log main <<-\EOF
+ ours
+ base
+ EOF
+ )
+'
+
+test_expect_success '--dry-run detects conflicts with modified working tree' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second modify-me &&
+ echo modified >modify-me &&
+
+ git refs list >refs-expect &&
+ git diff >diff-expect &&
+ test_must_fail git history drop --dry-run HEAD 2>err &&
+ test_grep "dropping this commit would overwrite local changes" err &&
+ git diff >diff-actual &&
+ git refs list >refs-actual &&
+
+ test_cmp diff-expect diff-actual &&
+ test_cmp refs-expect refs-actual
+ )
+'
+
+test_expect_success '--update-refs=head updates only HEAD' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit target &&
+ git branch theirs &&
+ test_commit ours &&
+ git switch theirs &&
+ test_commit theirs &&
+
+ # When told to update HEAD only, the command refuses to
+ # rewrite commits that are not an ancestor of HEAD.
+ test_must_fail git history drop --update-refs=head main 2>err &&
+ test_grep "rewritten commit must be an ancestor of HEAD" err &&
+
+ expect_graph --branches <<-\EOF &&
+ * theirs
+ | * ours
+ |/
+ * target
+ * base
+ EOF
+
+ git switch main &&
+ git history drop --update-refs=head target &&
+
+ expect_graph --branches <<-\EOF
+ * ours
+ | * theirs
+ | * target
+ |/
+ * base
+ EOF
+ )
+'
+
+test_expect_success 'conflict with replayed commit aborts cleanly' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit conflict-a file &&
+ test_commit conflict-b file &&
+
+ git refs list >refs-expect &&
+ test_must_fail git history drop HEAD~ 2>err &&
+ test_grep "failed replaying descendants" err &&
+ git refs list >refs-actual &&
+ test_cmp refs-expect refs-actual
+ )
+'
+
+# Build a history where a descendant of the drop target reverts the change
+# introduced by the drop target. After dropping, the descendant's diff applies
+# against a tree that already lacks the change, so it becomes empty.
+setup_empty_descendant_repo () {
+ git init "$1" &&
+ (
+ cd "$1" &&
+ echo C1 >file &&
+ git add file &&
+ git commit -m "base" &&
+ git tag base &&
+ echo C2 >file &&
+ git add file &&
+ git commit -m "drop-me" &&
+ git tag drop-me &&
+ test_commit middle &&
+ echo C1 >file &&
+ git add file &&
+ git commit -m "revert-drop-me" &&
+ git tag revert-drop-me
+ )
+}
+
+test_expect_success '--empty=drop drops descendants that become empty' '
+ test_when_finished "rm -rf repo" &&
+ setup_empty_descendant_repo repo &&
+ (
+ cd repo &&
+
+ git history drop --empty=drop drop-me &&
+
+ expect_log <<-\EOF
+ middle
+ base
+ EOF
+ )
+'
+
+test_expect_success '--empty=keep keeps descendants that become empty' '
+ test_when_finished "rm -rf repo" &&
+ setup_empty_descendant_repo repo &&
+ (
+ cd repo &&
+
+ git history drop --empty=keep drop-me &&
+
+ expect_log <<-\EOF &&
+ revert-drop-me
+ middle
+ base
+ EOF
+ git diff HEAD~ HEAD >diff &&
+ test_must_be_empty diff
+ )
+'
+
+test_expect_success '--empty=abort errors out when a descendant becomes empty' '
+ test_when_finished "rm -rf repo" &&
+ setup_empty_descendant_repo repo &&
+ (
+ cd repo &&
+
+ test_must_fail git history drop --empty=abort drop-me 2>err &&
+ test_grep "became empty after replay" err
+ )
+'
+
+test_expect_success 'updates index and worktree when HEAD moves' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+
+ git history drop second &&
+
+ # Worktree should no longer contain second.t.
+ test_path_is_missing second.t &&
+ test_path_is_file first.t &&
+ test_path_is_file third.t &&
+
+ # Index and worktree should both match the new HEAD.
+ git status --porcelain --untracked-files=no >status &&
+ test_must_be_empty status
+ )
+'
+
+test_expect_success 'updates worktree when dropping HEAD itself' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+
+ git history drop HEAD &&
+
+ test_path_is_missing second.t &&
+ test_path_is_file first.t &&
+
+ git status --porcelain --untracked-files=no >status &&
+ test_must_be_empty status
+ )
+'
+
+test_expect_success 'preserves unrelated unstaged modifications' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ echo first-content >unrelated.txt &&
+ git add unrelated.txt &&
+ git commit -m "add unrelated" &&
+ test_commit second &&
+ test_commit third &&
+
+ echo locally-modified >unrelated.txt &&
+
+ git diff >diff-expect &&
+ git history drop second &&
+ git diff >diff-actual &&
+ test_cmp diff-expect diff-actual &&
+ test_path_is_missing second.t
+ )
+'
+
+test_expect_success 'preserves unrelated staged changes' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ echo first-content >unrelated.txt &&
+ git add unrelated.txt &&
+ git commit -m "add unrelated" &&
+ test_commit second &&
+ test_commit third &&
+
+ echo staged-change >unrelated.txt &&
+ git add unrelated.txt &&
+
+ git diff --cached >diff-expect &&
+ git history drop second &&
+ git diff --cached >diff-actual &&
+ test_cmp diff-expect diff-actual &&
+ test_path_is_missing second.t
+ )
+'
+
+test_expect_success 'aborts when local modifications would be overwritten' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit conflict &&
+
+ echo local-edit >conflict.t &&
+ git diff >diff-expect &&
+ test_must_fail git history drop HEAD 2>err &&
+ test_grep "would overwrite local changes" err &&
+ git diff >diff-actual &&
+ test_cmp diff-expect diff-actual
+ )
+'
+
+test_done
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v4 09/10] builtin/history: split handling of ref updates into two phases
From: Patrick Steinhardt @ 2026-06-10 8:52 UTC (permalink / raw)
To: git; +Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
In-Reply-To: <20260610-b4-pks-history-drop-v4-0-70d5f0ae8c25@pks.im>
The function `handle_reference_updates()` is used by git-history(1) to
update all references that refer to commits that have been rewritten. As
such, it performs two steps:
- It gathers the references that need to be updated in the first
place.
- It prepares and commits the reference transaction.
In a subsequent commit we'll want to handle those two steps separately.
Prepare for this by splitting up the function into two.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/history.c | 102 ++++++++++++++++++++++++++++++++++--------------------
1 file changed, 64 insertions(+), 38 deletions(-)
diff --git a/builtin/history.c b/builtin/history.c
index 0fc06fb204..4fadf38c32 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -333,21 +333,17 @@ static int handle_ref_update(struct ref_transaction *transaction,
NULL, NULL, 0, reflog_msg, err);
}
-static int handle_reference_updates(struct rev_info *revs,
- enum ref_action action,
- struct commit *original,
- struct commit *rewritten,
- const char *reflog_msg,
- int dry_run,
- enum replay_empty_commit_action empty)
+static int compute_pending_ref_updates(struct rev_info *revs,
+ enum ref_action action,
+ struct commit *original,
+ struct commit *rewritten,
+ enum replay_empty_commit_action empty,
+ struct replay_result *result)
{
const struct name_decoration *decoration;
struct replay_revisions_options opts = {
.empty = empty,
};
- struct replay_result result = { 0 };
- struct ref_transaction *transaction = NULL;
- struct strbuf err = STRBUF_INIT;
char hex[GIT_MAX_HEXSZ + 1];
bool detached_head;
int head_flags = 0;
@@ -359,34 +355,13 @@ static int handle_reference_updates(struct rev_info *revs,
opts.onto = oid_to_hex_r(hex, &rewritten->object.oid);
- ret = replay_revisions(revs, &opts, &result);
+ ret = replay_revisions(revs, &opts, result);
if (ret)
- goto out;
+ return ret;
if (action != REF_ACTION_BRANCHES && action != REF_ACTION_HEAD)
BUG("unsupported ref action %d", action);
- if (!dry_run) {
- transaction = ref_store_transaction_begin(get_main_ref_store(revs->repo), 0, &err);
- if (!transaction) {
- ret = error(_("failed to begin ref transaction: %s"), err.buf);
- goto out;
- }
- }
-
- for (size_t i = 0; i < result.updates_nr; i++) {
- ret = handle_ref_update(transaction,
- result.updates[i].refname,
- &result.updates[i].new_oid,
- &result.updates[i].old_oid,
- reflog_msg, &err);
- if (ret) {
- ret = error(_("failed to update ref '%s': %s"),
- result.updates[i].refname, err.buf);
- goto out;
- }
- }
-
/*
* `replay_revisions()` only updates references that are
* ancestors of `rewritten`, so we need to manually
@@ -414,14 +389,43 @@ static int handle_reference_updates(struct rev_info *revs,
!detached_head)
continue;
+ ALLOC_GROW(result->updates, result->updates_nr + 1, result->updates_alloc);
+ result->updates[result->updates_nr].refname = xstrdup(decoration->name);
+ result->updates[result->updates_nr].old_oid = original->object.oid;
+ result->updates[result->updates_nr].new_oid = rewritten->object.oid;
+ result->updates_nr++;
+ }
+
+ return 0;
+}
+
+static int apply_pending_ref_updates(struct repository *repo,
+ const struct replay_result *result,
+ const char *reflog_msg,
+ int dry_run)
+{
+ struct ref_transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;
+ int ret;
+
+ if (!dry_run) {
+ transaction = ref_store_transaction_begin(get_main_ref_store(repo),
+ 0, &err);
+ if (!transaction) {
+ ret = error(_("failed to begin ref transaction: %s"), err.buf);
+ goto out;
+ }
+ }
+
+ for (size_t i = 0; i < result->updates_nr; i++) {
ret = handle_ref_update(transaction,
- decoration->name,
- &rewritten->object.oid,
- &original->object.oid,
+ result->updates[i].refname,
+ &result->updates[i].new_oid,
+ &result->updates[i].old_oid,
reflog_msg, &err);
if (ret) {
ret = error(_("failed to update ref '%s': %s"),
- decoration->name, err.buf);
+ result->updates[i].refname, err.buf);
goto out;
}
}
@@ -435,11 +439,33 @@ static int handle_reference_updates(struct rev_info *revs,
out:
ref_transaction_free(transaction);
- replay_result_release(&result);
strbuf_release(&err);
return ret;
}
+static int handle_reference_updates(struct rev_info *revs,
+ enum ref_action action,
+ struct commit *original,
+ struct commit *rewritten,
+ const char *reflog_msg,
+ int dry_run,
+ enum replay_empty_commit_action empty)
+{
+ struct replay_result result = { 0 };
+ int ret;
+
+ ret = compute_pending_ref_updates(revs, action, original, rewritten,
+ empty, &result);
+ if (ret)
+ goto out;
+
+ ret = apply_pending_ref_updates(revs->repo, &result, reflog_msg, dry_run);
+
+out:
+ replay_result_release(&result);
+ return ret;
+}
+
static int commit_became_empty(struct repository *repo,
struct commit *original,
struct tree *result)
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v4 08/10] reset: stop assuming that the caller passes in a clean index
From: Patrick Steinhardt @ 2026-06-10 8:52 UTC (permalink / raw)
To: git; +Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
In-Reply-To: <20260610-b4-pks-history-drop-v4-0-70d5f0ae8c25@pks.im>
In 652bd0211d (rebase: use 'skip_cache_tree_update' option, 2022-11-10),
we updated `reset_working_tree()` to stop updating the index tree cache.
This was done as a performance optimization: the function is only called
by "sequencer.c" and "rebase.c", both of which assume a clean index
before they perform their operation, so we know that the end result will
be a clean index, too. Consequently, we can skip recomputing the cache
as we can instead use `prime_cache_tree()` directly.
In a subsequent commit we're about to add a new caller though where the
assumption doesn't hold anymore: the index may be dirty before calling
`reset_working_tree()`, and consequently we cannot prime the cache with
a given tree anymore as the index and tree will mismatch.
Adapt the logic so that we only skip the cache tree update in case we're
doing a hard reset. While we could introduce logic that only skips the
update in case the incoming index was dirty already, that doesn't really
feel worth it: after all, the mentioned commit says itself that the
performance improvement was negligible anyway.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reset.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/reset.c b/reset.c
index abe36ba116..2b82e80c67 100644
--- a/reset.c
+++ b/reset.c
@@ -164,10 +164,11 @@ int reset_working_tree(struct repository *r,
unpack_tree_opts.dry_run = dry_run;
unpack_tree_opts.merge = 1;
unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
- unpack_tree_opts.skip_cache_tree_update = 1;
init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
- if (reset_hard)
+ if (reset_hard) {
+ unpack_tree_opts.skip_cache_tree_update = 1;
unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
+ }
if (!reset_hard && !fill_tree_descriptor(r, &desc[nr++], &head_oid)) {
ret = error(_("failed to find tree of %s"),
@@ -194,7 +195,8 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
- prime_cache_tree(r, r->index, tree);
+ if (reset_hard)
+ prime_cache_tree(r, r->index, tree);
if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {
ret = error(_("could not write index"));
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v4 07/10] reset: allow the caller to specify the current HEAD object
From: Patrick Steinhardt @ 2026-06-10 8:52 UTC (permalink / raw)
To: git; +Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
In-Reply-To: <20260610-b4-pks-history-drop-v4-0-70d5f0ae8c25@pks.im>
When calling `reset_working_tree()` we automatically derive the commit
that the callers wants to move from by reading the HEAD commit. Some
callers may already have resolved it, or they may want to move from a
different commit that doesn't match HEAD.
Introduce a new `oid_from` option that lets the caller specify the
commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reset.c | 5 ++++-
reset.h | 5 +++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/reset.c b/reset.c
index 3ac99a51c0..abe36ba116 100644
--- a/reset.c
+++ b/reset.c
@@ -119,7 +119,10 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
- if (!repo_get_oid(r, "HEAD", &head_oid)) {
+ if (opts->oid_from) {
+ oidcpy(&head_oid, opts->oid_from);
+ head = &head_oid;
+ } else if (!repo_get_oid(r, "HEAD", &head_oid)) {
head = &head_oid;
} else if (!oid || !reset_hard) {
ret = error(_("could not determine HEAD revision"));
diff --git a/reset.h b/reset.h
index 38b2891b53..4c992ba671 100644
--- a/reset.h
+++ b/reset.h
@@ -37,6 +37,11 @@ struct reset_working_tree_options {
* The commit to checkout/reset to. Defaults to HEAD.
*/
const struct object_id *oid;
+ /*
+ * The commit to checkout/reset from when doing a two-way merge. This
+ * is used as one of the sides to merge.
+ */
+ const struct object_id *oid_from;
/*
* Optional value to set ORIG_HEAD. Defaults to HEAD.
*/
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v4 06/10] reset: introduce ability to skip updating HEAD
From: Patrick Steinhardt @ 2026-06-10 8:52 UTC (permalink / raw)
To: git; +Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
In-Reply-To: <20260610-b4-pks-history-drop-v4-0-70d5f0ae8c25@pks.im>
In a subsequent commit we'll introduce a new caller to
`reset_working_tree()` that really only wants to update the index and
working tree, without updating any references. Introduce a new flag that
makes the caller opt in to updating HEAD and adapt all callers to set
that flag.
Note that in a previous iteration we instead introduced a flag that made
callers opt out of updating any references. This was somewhat awkward
though because we already have the `UPDATE_ORIG_HEAD` flag, so the
result was somewhat inconsistent.
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/rebase.c | 14 ++++++++++----
reset.c | 6 ++++--
reset.h | 9 ++++++---
sequencer.c | 4 +++-
4 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 06dcbaf5e8..10a306310c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -607,7 +607,8 @@ static int move_to_original_branch(struct rebase_options *opts)
strbuf_addf(&head_reflog, "%s (finish): returning to %s",
opts->reflog_action, opts->head_name);
ropts.branch = opts->head_name;
- ropts.flags = RESET_WORKING_TREE_REFS_ONLY;
+ ropts.flags = RESET_WORKING_TREE_REFS_ONLY |
+ RESET_WORKING_TREE_UPDATE_HEAD;
ropts.branch_msg = branch_reflog.buf;
ropts.head_msg = head_reflog.buf;
ret = reset_working_tree(the_repository, &ropts);
@@ -693,6 +694,7 @@ static int run_am(struct rebase_options *opts)
ropts.oid = &opts->orig_head->object.oid;
ropts.branch = opts->head_name;
ropts.default_reflog_action = opts->reflog_action;
+ ropts.flags = RESET_WORKING_TREE_UPDATE_HEAD;
reset_working_tree(the_repository, &ropts);
error(_("\ngit encountered an error while preparing the "
"patches to replay\n"
@@ -862,7 +864,8 @@ static int checkout_up_to_date(struct rebase_options *options)
options->reflog_action, options->switch_to);
ropts.oid = &options->orig_head->object.oid;
ropts.branch = options->head_name;
- ropts.flags = RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
+ ropts.flags = RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK |
+ RESET_WORKING_TREE_UPDATE_HEAD;
if (!ropts.branch)
ropts.flags |= RESET_WORKING_TREE_DETACH;
ropts.head_msg = buf.buf;
@@ -1384,7 +1387,8 @@ int cmd_rebase(int argc,
rerere_clear(the_repository, &merge_rr);
string_list_clear(&merge_rr, 1);
- ropts.flags = RESET_WORKING_TREE_HARD;
+ ropts.flags = RESET_WORKING_TREE_HARD |
+ RESET_WORKING_TREE_UPDATE_HEAD;
if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not discard worktree changes"));
remove_branch_state(the_repository, 0);
@@ -1409,7 +1413,8 @@ int cmd_rebase(int argc,
ropts.oid = &options.orig_head->object.oid;
ropts.head_msg = head_msg.buf;
ropts.branch = options.head_name;
- ropts.flags = RESET_WORKING_TREE_HARD;
+ ropts.flags = RESET_WORKING_TREE_HARD |
+ RESET_WORKING_TREE_UPDATE_HEAD;
if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not move back to %s"),
oid_to_hex(&options.orig_head->object.oid));
@@ -1877,6 +1882,7 @@ int cmd_rebase(int argc,
ropts.oid = &options.onto->object.oid;
ropts.orig_head = &options.orig_head->object.oid;
ropts.flags = RESET_WORKING_TREE_DETACH |
+ RESET_WORKING_TREE_UPDATE_HEAD |
RESET_WORKING_TREE_UPDATE_ORIG_HEAD |
RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
ropts.head_msg = msg.buf;
diff --git a/reset.c b/reset.c
index 99f2c1b012..3ac99a51c0 100644
--- a/reset.c
+++ b/reset.c
@@ -92,6 +92,7 @@ int reset_working_tree(struct repository *r,
const char *switch_to_branch = opts->branch;
unsigned reset_hard = opts->flags & RESET_WORKING_TREE_HARD;
unsigned refs_only = opts->flags & RESET_WORKING_TREE_REFS_ONLY;
+ unsigned update_head = opts->flags & RESET_WORKING_TREE_UPDATE_HEAD;
unsigned update_orig_head = opts->flags & RESET_WORKING_TREE_UPDATE_ORIG_HEAD;
unsigned dry_run = opts->flags & RESET_WORKING_TREE_DRY_RUN;
struct object_id *head = NULL, head_oid;
@@ -129,7 +130,7 @@ int reset_working_tree(struct repository *r,
oid = &head_oid;
if (refs_only) {
- if (!dry_run)
+ if (update_head)
return update_refs(r, opts, oid, head);
return 0;
}
@@ -197,7 +198,8 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
- if (oid != &head_oid || update_orig_head || switch_to_branch)
+ if (update_head &&
+ (oid != &head_oid || update_orig_head || switch_to_branch))
ret = update_refs(r, opts, oid, head);
leave_reset_head:
diff --git a/reset.h b/reset.h
index 898e4a1e95..38b2891b53 100644
--- a/reset.h
+++ b/reset.h
@@ -19,14 +19,17 @@ enum reset_working_tree_flags {
/* Only update refs, do not touch the worktree */
RESET_WORKING_TREE_REFS_ONLY = (1 << 3),
- /* Update ORIG_HEAD as well as HEAD */
- RESET_WORKING_TREE_UPDATE_ORIG_HEAD = (1 << 4),
+ /* Update HEAD */
+ RESET_WORKING_TREE_UPDATE_HEAD = (1 << 4),
+
+ /* Update ORIG_HEAD */
+ RESET_WORKING_TREE_UPDATE_ORIG_HEAD = (1 << 5),
/*
* Perform a dry-run by performing the operation without updating
* any user-visible state.
*/
- RESET_WORKING_TREE_DRY_RUN = (1 << 5),
+ RESET_WORKING_TREE_DRY_RUN = (1 << 6),
};
struct reset_working_tree_options {
diff --git a/sequencer.c b/sequencer.c
index 4efe831178..e905b1b2d9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4678,7 +4678,8 @@ static void create_autostash_internal(struct repository *r,
has_uncommitted_changes(r, 1)) {
struct child_process stash = CHILD_PROCESS_INIT;
struct reset_working_tree_options ropts = {
- .flags = RESET_WORKING_TREE_HARD,
+ .flags = RESET_WORKING_TREE_HARD |
+ RESET_WORKING_TREE_UPDATE_HEAD,
};
struct object_id oid;
@@ -4873,6 +4874,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
.oid = onto,
.orig_head = orig_head,
.flags = RESET_WORKING_TREE_DETACH |
+ RESET_WORKING_TREE_UPDATE_HEAD |
RESET_WORKING_TREE_UPDATE_ORIG_HEAD |
RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK,
.head_msg = reflog_message(opts, "start", "checkout %s",
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox