* Re: [PATCH RFC 0/2] Move libgit.a sources into separate "lib/" directory
From: Patrick Steinhardt @ 2026-04-22 6:39 UTC (permalink / raw)
To: Derrick Stolee; +Cc: git
In-Reply-To: <4199e58b-d0fc-4240-9717-16c89ae73322@gmail.com>
On Tue, Apr 21, 2026 at 10:13:31AM -0400, Derrick Stolee wrote:
> On 4/21/2026 1:55 AM, Patrick Steinhardt wrote:
> > On Mon, Apr 20, 2026 at 08:03:44AM -0400, Derrick Stolee wrote:
>
> Thanks for your thoughts on the earlier parts of my message. I think
> you're moving in the right direction and I don't have further comments.
Thanks a lot for your feedback!
Patrick
^ permalink raw reply
* Re: [PATCH] generate-configlist: collapse depfile for older Ninja
From: Toon Claes @ 2026-04-22 7:09 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, D. Ben Knoble
In-Reply-To: <aehsikCfPm83M9dN@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Do we really have to make the logic conditional? I would expect that
> this works just fine for newer versions of Ninja and for Make, so it
> feels rather pointless to me to have two modes to worry about.
>
> So wouldn't the below simplified version be sufficient?
I considered that, but I wasn't sure. I went all-in to show what it
could be, but I'm happy to see simplifications.
Now your version doesn't remove the no-op targets from config-list.h.d.
I did some testing, and while Ninja doesn't need those no-op target,
they are also harmless to keep them. Thus I agree your version is
simpler and better. Let me roll another version.
Thanks!
> diff --git a/tools/generate-configlist.sh b/tools/generate-configlist.sh
> index e28054f9e0..f5f42492c6 100755
> --- a/tools/generate-configlist.sh
> +++ b/tools/generate-configlist.sh
> @@ -44,7 +44,9 @@ then
> {
> printf '%s\n' "$SOURCE_DIR"/Documentation/*config.adoc \
> "$SOURCE_DIR"/Documentation/config/*.adoc |
> - sed -e 's/[# ]/\\&/g' -e "s/^/$QUOTED_OUTPUT: /"
> + sed -e 's/[# ]/\\&/g' |
> + tr '\n' ' ' |
> + sed -e "s/^/$QUOTED_OUTPUT: /" -e 's/ $/\n/'
> printf '%s:\n' "$SOURCE_DIR"/Documentation/*config.adoc \
> "$SOURCE_DIR"/Documentation/config/*.adoc |
> sed -e 's/[# ]/\\&/g'
--
Cheers,
Toon
^ permalink raw reply
* [PATCH v2] generate-configlist: collapse depfile for older Ninja
From: Toon Claes @ 2026-04-22 7:21 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Patrick Steinhardt, Toon Claes
In-Reply-To: <20260421-toon-fix-almalinux8-v1-1-aec1d54addde@iotcl.com>
The tools/generate-configlist.sh script generates two files:
* config-list.h
* config-list.h.d
The former is included by the source code and the latter defines on
which files the former depends.
The contents of `config-list.h.d` consists of two sections:
config-list.h: Documentation/config.adoc
config-list.h: Documentation/git-config.adoc
config-list.h: Documentation/config/add.adoc
config-list.h: Documentation/config/advice.adoc
config-list.h: Documentation/config/alias.adoc
config-list.h: Documentation/config/am.adoc
config-list.h: Documentation/config/apply.adoc
...
This first section actually defines on which individual files
`config-list.h` depends and thus needs to be rebuild if one of those
changes.
And the second section contains content like:
Documentation/config.adoc:
Documentation/git-config.adoc:
Documentation/config/add.adoc:
Documentation/config/advice.adoc:
Documentation/config/alias.adoc:
Documentation/config/am.adoc:
Documentation/config/apply.adoc:
...
These rules exist to ensure Make won't fail with the following error if
one of the .adoc files is renamed or removed:
make: *** No rule to make target 'Documentation/config.adoc', needed by 'config-list.h'.
With the no-op targets defined in `config-list.h.d`, Make knows there's
no work to be done to generate these files, so it doesn't error out if
it doesn't exist.
For the Makefile build system this works great. And since
ebeea3c471 (build: regenerate config-list.h when Documentation changes,
2026-02-24) this script is also called from the Meson build system.
Nevertheless, on AlmaLinux 8 the following build failure is seen:
ninja: error: dependency cycle: config-list.h -> config-list.h
This version of this distro uses Ninja 1.8.2 and it seems to have some
issues with the format of the `config-list.h.d` file.
Ninja versions before 1.10.0 do not reset the depfile parser state on
newlines. This causes issues when the depfile has one dependency per
line, like we have in `config-list.h.d`:
config-list.h: Documentation/config.adoc
config-list.h: Documentation/config/add.adoc
The parser only recognizes the first "config-list.h:" as a target. On
subsequent lines it is still in dependency-parsing mode, so the repeated
output name is recorded as an input. This causes the error mentioned
above.
The bug in Ninja is fixed in 1.10, with commit
ninja-build/ninja@1daa7470ab7e (depfile_parser: remove restriction on
multiple outputs, 2019-11-20).
To be compatible with older versions of Ninja, collapse the dependencies
for `config-list.h` into a single line like:
config-list.h: Documentation/config.adoc Documentation/config/add.adoc ...
This works around the bug in older versions of Ninja, and is fully
compatible Make and with more recent versions of Ninja. And while the
no-op targets are not needed for Ninja, they also don't do any harm.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
At GitLab we build images for various distros, including AlmaLinux 8.
On this distro we got this error while compiling Git.
ninja: error: dependency cycle: config-list.h -> config-list.h
It seems this is caused by a bug in older versions of Ninja. There are
more details in the commit message, but here are a few simple steps to
reproduce:
docker run --rm -it -v $(pwd):/git -w /git almalinux:8 bash
dnf -yq install epel-release
dnf -yq install shadow-utils sudo make pkg-config gcc findutils \
diffutils perl python3 gawk gettext zlib-devel expat-devel \
openssl-devel curl-devel pcre2-devel cargo
pip3 install --prefix=/usr meson ninja==1.8.2
meson setup build --warnlevel 2 --werror
ninja -C build config-list.h
ninja -C build config-list.h # fails with dependency cycle
---
Changes in v2:
- Simplify the changes *a lot* by doing the collapsing unconditionally.
- Link to v1: https://patch.msgid.link/20260421-toon-fix-almalinux8-v1-1-aec1d54addde@iotcl.com
---
tools/generate-configlist.sh | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/generate-configlist.sh b/tools/generate-configlist.sh
index e28054f9e0..f5f42492c6 100755
--- a/tools/generate-configlist.sh
+++ b/tools/generate-configlist.sh
@@ -44,7 +44,9 @@ then
{
printf '%s\n' "$SOURCE_DIR"/Documentation/*config.adoc \
"$SOURCE_DIR"/Documentation/config/*.adoc |
- sed -e 's/[# ]/\\&/g' -e "s/^/$QUOTED_OUTPUT: /"
+ sed -e 's/[# ]/\\&/g' |
+ tr '\n' ' ' |
+ sed -e "s/^/$QUOTED_OUTPUT: /" -e 's/ $/\n/'
printf '%s:\n' "$SOURCE_DIR"/Documentation/*config.adoc \
"$SOURCE_DIR"/Documentation/config/*.adoc |
sed -e 's/[# ]/\\&/g'
---
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
change-id: 20260421-toon-fix-almalinux8-102de9138294
^ permalink raw reply related
* Re: [PATCH v2 0/3] contrib/subtree: reduce recursion during split
From: Johannes Schindelin @ 2026-04-22 9:43 UTC (permalink / raw)
To: Colin Stagner
Cc: Ian Jackson, git, Christian Heusel, george, Christian Hesse,
Phillip Wood, Junio C Hamano
In-Reply-To: <cca575ae-e5dd-4a5d-bde2-f493a3e62a87@howdoi.land>
Hi Colin & Ian,
On Wed, 22 Apr 2026, Colin Stagner wrote:
> On 4/20/26 04:57, Ian Jackson wrote:
>
> > I need to think about this some more but I doubt this can be made to
> > work well without more significant changes, including to the data
> > model. There would have to be some kind of compatibility arrangement
> > to handle existing histories.
>
> I would take a look at the test-cases for git-subtree.sh, which document
> some of the kinds of issues you will encounter. They may help you test
> compatibility.
>
> Anything you can do to limit breakage to "opt-in" points-in-time only
> would be greatly appreciated.
>
> > Colin, is that OK with you?
>
> You can name it and develop it however you like. No need to ask
> permission here.
>
> (For the record, I'm also not the maintainer of contrib/git-subtree.
> I've just been trying to fix a few issues with it.)
>
> > If you would prefer, I could choose a different name for the
> > resulting program.
>
> If I were writing it, I would give the new program a different name but
> perhaps provide a "compile-time" way to set it to "git-subtree" instead.
> My reason for this is that it may need to exist with the legacy script
> for awhile, and it's good to be able to tell them apart.
I just wanted to chime in to cheer you on, I've been following this
Rust-based `git-subtree` idea with interest. You may know it already, Git
for Windows is shipping `git subtree` with its installers for ages, and
given the abysmal performance characteristics of shell-based Git commands
on Windows, it would be really good to replace the shell-scripted version
with the Rust version (also to ensure proper error handling, which is
hard to make comprehensive in Unix shell scripts).
Thank you for pushing this forward!
Ciao,
Johannes
^ permalink raw reply
* [PATCH v2] refs/files: skip lock files during consistency checks
From: Karthik Nayak @ 2026-04-22 9:49 UTC (permalink / raw)
To: git; +Cc: gitster, Christian Couder, Karthik Nayak
In-Reply-To: <20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com>
Consistency checks in the files reference backend involve two steps:
1. Iterate over all entries within the 'refs/' directory and call
`files_fsck_ref()` on each.
2. Iterate over all root refs via `for_each_root_ref()` and call
`files_fsck_ref()` on each.
`files_fsck_ref()` then runs all fsck checks defined in
`fsck_refs_fn[]`. Step 2 goes through the refs API and only sees valid
refs, but step 1 iterates the directory directly and will also encounter
intermediate '*.lock' files.
Currently, `files_fsck_refs_name()`, one of the functions in
`fsck_refs_fn[]`, filters out lock files itself. The other function,
`files_fsck_refs_content()`, has no such check and would parse the lock
file. Any new function added to `fsck_refs_fn[]` would have the same
problem.
Move the filter up into `files_fsck_refs_dir()`, where the directory
iteration happens. Since step 2 cannot produce lock files, this is the
only site where the filter is needed, and individual checks no longer
have to re-implement it.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v2:
- Modified the commit message to clarify the changes made and reasoning.
- Modify the comment in the code to be more accurate.
- Add another additional test for bare lock files.
- Link to v1: https://patch.msgid.link/20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com
---
refs/files-backend.c | 22 +++++++++++-----------
t/t0602-reffiles-fsck.sh | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 11 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b3b0c25f84..1504a1e2f3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
struct fsck_options *o,
const char *refname,
- const char *path,
+ const char *path UNUSED,
int mode UNUSED)
{
struct strbuf sb = STRBUF_INIT;
- const char *filename;
int ret = 0;
- filename = basename((char *) path);
-
- /*
- * Ignore the files ending with ".lock" as they may be lock files
- * However, do not allow bare ".lock" files.
- */
- if (filename[0] != '.' && ends_with(filename, ".lock"))
- goto cleanup;
-
if (is_root_ref(refname))
goto cleanup;
@@ -3939,6 +3929,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
struct strbuf refname = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
struct dir_iterator *iter;
+ const char *filename;
int iter_status;
int ret = 0;
@@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
strbuf_addf(&refname, "worktrees/%s/", wt->id);
strbuf_addf(&refname, "refs/%s", iter->relative_path);
+ filename = basename((char *) iter->path.buf);
+
+ /*
+ * Ignore the files ending with ".lock" as they may be lock files.
+ * However, do not skip invalid refnames with '.lock' suffix.
+ */
+ if (filename[0] != '.' && ends_with(filename, ".lock"))
+ continue;
+
if (files_fsck_ref(ref_store, o, refname.buf,
iter->path.buf, iter->st.st_mode) < 0)
ret = -1;
diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index 3c1f553b81..13259821a0 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -87,6 +87,47 @@ test_expect_success 'ref name should be checked' '
)
'
+test_expect_success 'lock files should be ignored' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ git commit --allow-empty -m initial &&
+ git checkout -b branch-1 &&
+
+ touch .git/refs/heads/branch-1.lock &&
+ git refs verify 2>err &&
+ test_must_be_empty err &&
+
+ echo "foobar" >.git/refs/heads/branch-2 &&
+ test_must_fail git refs verify 2>err &&
+ cat >expect <<-EOF &&
+ error: refs/heads/branch-2: badRefContent: foobar
+ EOF
+ test_cmp expect err
+ )
+'
+
+test_expect_success 'bare lock files should not be ignored' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ git commit --allow-empty -m initial &&
+ git checkout -b branch-1 &&
+
+ # invalid refname should be reported
+ cp .git/refs/heads/branch-1 .git/refs/heads/.branch-1.lock &&
+ # invalid refname and content should be reported
+ touch .git/refs/heads/.lock &&
+
+ test_must_fail git refs verify 2>err &&
+ test_grep "error: refs/heads/.branch-1.lock: badRefName: invalid refname format" err &&
+ test_grep "error: refs/heads/.lock: badRefName: invalid refname format" err &&
+ test_grep "error: refs/heads/.lock: badRefContent: " err
+ )
+'
+
test_expect_success 'ref name check should be adapted into fsck messages' '
test_when_finished "rm -rf repo" &&
git init repo &&
^ permalink raw reply related
* [PATCH 0/2] builtin/history: introduce "fixup" subcommand
From: Patrick Steinhardt @ 2026-04-22 10:28 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
Hi,
this short patch series introduces a new "fixup" subcommand. This
command is the first one that I felt is missing in my day to day work,
as I end up doing fixup commits quite often.
The flow is rather simple: the user stages some changes, and then they
execute `git history fixup <commit>` to amend those changes to the given
commit. As with the other subcommands, dependent branches will then be
rebased automatically.
This is the first command that may result in merge conflicts. For now we
simply abort in such cases, but there are plans to introduce first-class
conflicts into Git. So once we have them, we'll also be able to handle
such cases more gracefully. I still think that the command is useful
even without that conflict handling.
Thanks!
Patrick
---
Patrick Steinhardt (2):
builtin/history: generalize function to commit trees
builtin/history: introduce "fixup" subcommand
Documentation/git-history.adoc | 52 ++++-
builtin/history.c | 198 ++++++++++++++--
t/meson.build | 1 +
t/t3453-history-fixup.sh | 500 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 730 insertions(+), 21 deletions(-)
---
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
change-id: 20260422-b4-pks-history-fixup-be27e0c4a03e
^ permalink raw reply
* [PATCH 1/2] builtin/history: generalize function to commit trees
From: Patrick Steinhardt @ 2026-04-22 10:28 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
In-Reply-To: <20260422-b4-pks-history-fixup-v1-0-48d4484243de@pks.im>
The function `commit_tree_with_edited_message_ext()` can be used to
commit a tree with a specific list of parents with an edited commit
message. This function is useful outside of editing the commit message
though, as it also performs the plumbing to extract the original commit
message and strip some headers from it.
Refactor the function to receive a flags field that allows the caller to
control whether or not the commit message should be edited, or whether
it should be retained as-is. This will be used in a subsequent commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/history.c | 45 ++++++++++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/builtin/history.c b/builtin/history.c
index 9526938085..549e352c74 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -91,13 +91,18 @@ static int fill_commit_message(struct repository *repo,
return 0;
}
-static int commit_tree_with_edited_message_ext(struct repository *repo,
- const char *action,
- struct commit *commit_with_message,
- const struct commit_list *parents,
- const struct object_id *old_tree,
- const struct object_id *new_tree,
- struct commit **out)
+enum commit_tree_flags {
+ COMMIT_TREE_EDIT_MESSAGE = (1 << 0),
+};
+
+static int commit_tree_ext(struct repository *repo,
+ const char *action,
+ struct commit *commit_with_message,
+ const struct commit_list *parents,
+ const struct object_id *old_tree,
+ const struct object_id *new_tree,
+ struct commit **out,
+ enum commit_tree_flags flags)
{
const char *exclude_gpgsig[] = {
/* We reencode the message, so the encoding needs to be stripped. */
@@ -122,10 +127,14 @@ static int commit_tree_with_edited_message_ext(struct repository *repo,
original_author = xmemdupz(ptr, len);
find_commit_subject(original_message, &original_body);
- ret = fill_commit_message(repo, old_tree, new_tree,
- original_body, action, &commit_message);
- if (ret < 0)
- goto out;
+ if (flags & COMMIT_TREE_EDIT_MESSAGE) {
+ ret = fill_commit_message(repo, old_tree, new_tree,
+ original_body, action, &commit_message);
+ if (ret < 0)
+ goto out;
+ } else {
+ strbuf_addstr(&commit_message, original_body);
+ }
original_extra_headers = read_commit_extra_headers(commit_with_message,
exclude_gpgsig);
@@ -168,8 +177,8 @@ static int commit_tree_with_edited_message(struct repository *repo,
oidcpy(&parent_tree_oid, repo->hash_algo->empty_tree);
}
- return commit_tree_with_edited_message_ext(repo, action, original, original->parents,
- &parent_tree_oid, tree_oid, out);
+ return commit_tree_ext(repo, action, original, original->parents,
+ &parent_tree_oid, tree_oid, out, COMMIT_TREE_EDIT_MESSAGE);
}
enum ref_action {
@@ -616,9 +625,8 @@ static int split_commit(struct repository *repo,
* The first commit is constructed from the split-out tree. The base
* that shall be diffed against is the parent of the original commit.
*/
- ret = commit_tree_with_edited_message_ext(repo, "split-out", original,
- original->parents, &parent_tree_oid,
- &split_tree->object.oid, &first_commit);
+ ret = commit_tree_ext(repo, "split-out", original, original->parents, &parent_tree_oid,
+ &split_tree->object.oid, &first_commit, COMMIT_TREE_EDIT_MESSAGE);
if (ret < 0) {
ret = error(_("failed writing first commit"));
goto out;
@@ -634,9 +642,8 @@ static int split_commit(struct repository *repo,
old_tree_oid = &repo_get_commit_tree(repo, first_commit)->object.oid;
new_tree_oid = &repo_get_commit_tree(repo, original)->object.oid;
- ret = commit_tree_with_edited_message_ext(repo, "split-out", original,
- parents, old_tree_oid,
- new_tree_oid, &second_commit);
+ ret = commit_tree_ext(repo, "split-out", original, parents, old_tree_oid,
+ new_tree_oid, &second_commit, COMMIT_TREE_EDIT_MESSAGE);
if (ret < 0) {
ret = error(_("failed writing second commit"));
goto out;
--
2.54.0.545.g6539524ca2.dirty
^ permalink raw reply related
* [PATCH 2/2] builtin/history: introduce "fixup" subcommand
From: Patrick Steinhardt @ 2026-04-22 10:28 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
In-Reply-To: <20260422-b4-pks-history-fixup-v1-0-48d4484243de@pks.im>
The newly introduced git-history(1) command provides functionality to
easily edit commit history while also rebasing dependent branches. The
functionality exposed by this command is still somewhat limited though.
One common use case when editing commit history that is not yet covered
is fixing up a specific commit. Introduce a new subcommand that allows
the user to do exactly that by performing a three-way merge into the
target's commit tree, using HEAD's tree as the merge base. The flow is
thus essentially:
$ echo changes >file
$ git add file
$ git history fixup HEAD~
Like with the other commands, this will automatically rebase dependent
branches, as well. Unlike the other commands though:
- The command does not work in a bare repository as it interacts with
the index.
- The command may run into merge conflicts. If so, the command will
simply abort.
Especially the second item limits the usefulness of this command a bit.
But there are plans to introduce first-class conflicts into Git, which
will help use cases like this one.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-history.adoc | 52 ++++-
builtin/history.c | 153 +++++++++++++
t/meson.build | 1 +
t/t3453-history-fixup.sh | 500 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 704 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc
index 24dc907033..3cdfc8ba02 100644
--- a/Documentation/git-history.adoc
+++ b/Documentation/git-history.adoc
@@ -8,6 +8,7 @@ git-history - EXPERIMENTAL: Rewrite history
SYNOPSIS
--------
[synopsis]
+git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message]
git history reword <commit> [--dry-run] [--update-refs=(branches|head)]
git history split <commit> [--dry-run] [--update-refs=(branches|head)] [--] [<pathspec>...]
@@ -22,8 +23,9 @@ THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
This command is related to linkgit:git-rebase[1] in that both commands can be
used to rewrite history. There are a couple of major differences though:
-* linkgit:git-history[1] can work in a bare repository as it does not need to
- touch either the index or the worktree.
+* Most subcommands of linkgit:git-history[1] can work in a bare repository as
+ they do not need to touch either the index or the worktree. The `fixup`
+ subcommand is an exception to this, as it reads staged changes from the index.
* linkgit:git-history[1] does not execute any linkgit:githooks[5] at the
current point in time. This may change in the future.
* linkgit:git-history[1] by default updates all branches that are descendants
@@ -53,6 +55,19 @@ COMMANDS
The following commands are available to rewrite history in different ways:
+`fixup <commit>`::
+ Apply the currently staged changes to the specified commit. The staged
+ changes are incorporated into the target commit's tree via a three-way
+ merge, using HEAD's tree as the merge base, which is equivalent to
+ linkgit:git-cherry-pick[1].
++
+The commit message and authorship of the target commit are preserved by
+default, unless you specify `--reedit-message`.
++
+If applying the staged changes would result in a conflict, the command
+aborts with an error. All branches that are descendants of the original
+commit are updated to point to the rewritten history.
+
`reword <commit>`::
Rewrite the commit message of the specified commit. All the other
details of this commit remain unchanged. This command will spawn an
@@ -87,6 +102,9 @@ OPTIONS
objects will be written into the repository, so applying these printed
ref updates is generally safe.
+`--reedit-message`::
+ Open an editor to modify the target commit's message.
+
`--update-refs=(branches|head)`::
Control which references will be updated by the command, if any. With
`branches`, all local branches that point to commits which are
@@ -96,6 +114,36 @@ OPTIONS
EXAMPLES
--------
+Fixup a commit
+~~~~~~~~~~~~~~
+
+----------
+$ git log --oneline --stat
+abc1234 (HEAD -> main) third
+ third.txt | 1 +
+def5678 second
+ second.txt | 1 +
+ghi9012 first
+ first.txt | 1 +
+
+$ echo "change" >>unrelated.txt
+$ git add unrelated.txt
+$ git history fixup ghi9012
+
+$ git log --oneline --stat
+jkl3456 (HEAD -> main) third
+ third.txt | 1 +
+mno7890 second
+ second.txt | 1 +
+pqr1234 first
+ first.txt | 1 +
+ unrelated.txt | 1 +
+----------
+
+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.
+
Split a commit
~~~~~~~~~~~~~~
diff --git a/builtin/history.c b/builtin/history.c
index 549e352c74..6299f0dfa9 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -10,6 +10,7 @@
#include "gettext.h"
#include "hex.h"
#include "lockfile.h"
+#include "merge-ort.h"
#include "oidmap.h"
#include "parse-options.h"
#include "path.h"
@@ -23,6 +24,8 @@
#include "unpack-trees.h"
#include "wt-status.h"
+#define GIT_HISTORY_FIXUP_USAGE \
+ N_("git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message]")
#define GIT_HISTORY_REWORD_USAGE \
N_("git history reword <commit> [--dry-run] [--update-refs=(branches|head)]")
#define GIT_HISTORY_SPLIT_USAGE \
@@ -434,6 +437,154 @@ static int handle_reference_updates(struct rev_info *revs,
return ret;
}
+static int cmd_history_fixup(int argc,
+ const char **argv,
+ const char *prefix,
+ struct repository *repo)
+{
+ const char * const usage[] = {
+ GIT_HISTORY_FIXUP_USAGE,
+ NULL,
+ };
+ enum ref_action action = REF_ACTION_DEFAULT;
+ int dry_run = 0;
+ enum commit_tree_flags flags = 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_BIT(0, "reedit-message", &flags,
+ N_("open an editor to modify the commit message"),
+ COMMIT_TREE_EDIT_MESSAGE),
+ OPT_END(),
+ };
+ struct merge_result merge_result = { 0 };
+ struct merge_options merge_opts = { 0 };
+ struct strbuf reflog_msg = STRBUF_INIT;
+ struct commit *head_commit, *original, *rewritten;
+ struct tree *head_tree, *original_tree, *index_tree;
+ struct rev_info revs = { 0 };
+ 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;
+
+ if (is_bare_repository()) {
+ ret = error(_("cannot run fixup in a bare repository"));
+ goto out;
+ }
+
+ /* Resolve the original commit, which is the one we want to fix up. */
+ original = lookup_commit_reference_by_name(argv[0]);
+ if (!original) {
+ ret = error(_("commit cannot be found: %s"), argv[0]);
+ goto out;
+ }
+
+ /*
+ * Resolve HEAD so we can use its tree as the merge base: the staged
+ * changes are expressed as a diff from HEAD's tree to the index tree.
+ */
+ head_commit = lookup_commit_reference_by_name("HEAD");
+ if (!head_commit) {
+ ret = error(_("cannot look up HEAD"));
+ goto out;
+ }
+
+ head_tree = repo_get_commit_tree(repo, head_commit);
+ if (!head_tree) {
+ ret = error(_("cannot get tree for HEAD"));
+ goto out;
+ }
+
+ if (repo_read_index(repo) < 0) {
+ ret = error(_("unable to read index"));
+ goto out;
+ }
+
+ if (!repo_index_has_changes(repo, head_tree, NULL)) {
+ ret = error(_("nothing to fixup: no staged changes"));
+ goto out;
+ }
+
+ /*
+ * Write the index as a tree object. This is the "theirs" side of the
+ * three-way merge: it is HEAD's tree with the staged changes applied.
+ */
+ index_tree = write_in_core_index_as_tree(repo, repo->index);
+ if (!index_tree) {
+ ret = error(_("unable to write index as a tree"));
+ goto out;
+ }
+
+ original_tree = repo_get_commit_tree(repo, original);
+ if (!original_tree) {
+ ret = error(_("cannot get tree for commit %s"), argv[0]);
+ goto out;
+ }
+
+ /*
+ * Perform the three-way merge to reapply changes in the index onto the
+ * target commit. This is using basically the same logic as a
+ * cherry-pick, where the base commit is our HEAD, ours is the original
+ * tree and theirs is the index tree.
+ */
+ init_basic_merge_options(&merge_opts, repo);
+ merge_opts.ancestor = "HEAD";
+ merge_opts.branch1 = argv[0];
+ merge_opts.branch2 = "staged";
+ merge_incore_nonrecursive(&merge_opts, head_tree,
+ original_tree, index_tree, &merge_result);
+
+ if (merge_result.clean < 0) {
+ ret = error(_("merge failed while applying fixup"));
+ goto out;
+ }
+
+ if (!merge_result.clean) {
+ ret = error(_("fixup would produce conflicts; aborting"));
+ goto out;
+ }
+
+ ret = setup_revwalk(repo, action, original, &revs);
+ if (ret)
+ goto out;
+
+ ret = commit_tree_ext(repo, "fixup", original, original->parents,
+ &original_tree->object.oid, &merge_result.tree->object.oid,
+ &rewritten, flags);
+ if (ret < 0) {
+ ret = error(_("failed writing fixed-up commit"));
+ goto out;
+ }
+
+ strbuf_addf(&reflog_msg, "fixup: updating %s", argv[0]);
+
+ ret = handle_reference_updates(&revs, action, original, rewritten,
+ reflog_msg.buf, dry_run);
+ if (ret < 0) {
+ ret = error(_("failed replaying descendants"));
+ goto out;
+ }
+
+ ret = 0;
+
+out:
+ merge_finalize(&merge_opts, &merge_result);
+ strbuf_release(&reflog_msg);
+ release_revisions(&revs);
+ return ret;
+}
+
static int cmd_history_reword(int argc,
const char **argv,
const char *prefix,
@@ -745,12 +896,14 @@ int cmd_history(int argc,
struct repository *repo)
{
const char * const usage[] = {
+ GIT_HISTORY_FIXUP_USAGE,
GIT_HISTORY_REWORD_USAGE,
GIT_HISTORY_SPLIT_USAGE,
NULL,
};
parse_opt_subcommand_fn *fn = NULL;
struct option options[] = {
+ OPT_SUBCOMMAND("fixup", &fn, cmd_history_fixup),
OPT_SUBCOMMAND("reword", &fn, cmd_history_reword),
OPT_SUBCOMMAND("split", &fn, cmd_history_split),
OPT_END(),
diff --git a/t/meson.build b/t/meson.build
index 7528e5cda5..f502ad8ec9 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -397,6 +397,7 @@ integration_tests = [
't3450-history.sh',
't3451-history-reword.sh',
't3452-history-split.sh',
+ 't3453-history-fixup.sh',
't3500-cherry.sh',
't3501-revert-cherry-pick.sh',
't3502-cherry-pick-merge.sh',
diff --git a/t/t3453-history-fixup.sh b/t/t3453-history-fixup.sh
new file mode 100755
index 0000000000..0012b1f052
--- /dev/null
+++ b/t/t3453-history-fixup.sh
@@ -0,0 +1,500 @@
+#!/bin/sh
+
+test_description='tests for git-history fixup subcommand'
+
+. ./test-lib.sh
+
+fixup_with_message () {
+ cat >message &&
+ write_script fake-editor.sh <<-\EOF &&
+ cp message "$1"
+ EOF
+ test_set_editor "$(pwd)"/fake-editor.sh &&
+ git history fixup --reedit-message "$@" &&
+ rm fake-editor.sh message
+}
+
+expect_changes () {
+ git log --format="%s" --numstat "$@" >actual.raw &&
+ sed '/^$/d' <actual.raw >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 fixup 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 fixup 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 fixup does-not-exist 2>err &&
+ test_grep "commit cannot be found: does-not-exist" err
+ )
+'
+
+test_expect_success 'errors when nothing is staged' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_must_fail git history fixup HEAD 2>err &&
+ test_grep "nothing to fixup: no staged changes" err
+ )
+'
+
+test_expect_success 'errors in a bare repository' '
+ test_when_finished "rm -rf repo repo.git" &&
+ git init repo &&
+ test_commit -C repo initial &&
+ git clone --bare repo repo.git &&
+ test_must_fail git -C repo.git history fixup HEAD 2>err &&
+ test_grep "cannot run fixup in a bare repository" err
+'
+
+test_expect_success 'can fixup the tip commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ echo content >file.txt &&
+ git add file.txt &&
+ git commit -m "add file" &&
+
+ echo fix >>file.txt &&
+ git add file.txt &&
+
+ expect_changes <<-\EOF &&
+ add file
+ 1 0 file.txt
+ initial
+ 1 0 initial.t
+ EOF
+
+ git symbolic-ref HEAD >branch-expect &&
+ git history fixup HEAD &&
+ git symbolic-ref HEAD >branch-actual &&
+ test_cmp branch-expect branch-actual &&
+
+ expect_changes <<-\EOF &&
+ add file
+ 2 0 file.txt
+ initial
+ 1 0 initial.t
+ EOF
+
+ # Verify the fix is in the tip commit tree
+ git show HEAD:file.txt >actual &&
+ printf "content\nfix\n" >expect &&
+ test_cmp expect actual &&
+
+ git reflog >reflog &&
+ test_grep "fixup: updating HEAD" reflog
+ )
+'
+
+test_expect_success 'can fixup a commit in the middle of history' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ echo content >file.txt &&
+ git add file.txt &&
+ git commit -m "add file" &&
+ test_commit third &&
+
+ echo fix >>file.txt &&
+ git add file.txt &&
+
+ expect_changes <<-\EOF &&
+ third
+ 1 0 third.t
+ add file
+ 1 0 file.txt
+ first
+ 1 0 first.t
+ EOF
+
+ git history fixup HEAD~ &&
+
+ expect_changes <<-\EOF &&
+ third
+ 1 0 third.t
+ add file
+ 2 0 file.txt
+ first
+ 1 0 first.t
+ EOF
+
+ # Verify the fix landed in the "add file" commit.
+ git show HEAD~:file.txt >actual &&
+ printf "content\nfix\n" >expect &&
+ test_cmp expect actual &&
+
+ # And verify that the replayed commit also has the change.
+ git show HEAD:file.txt >actual &&
+ printf "content\nfix\n" >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'can fixup root commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ echo initial >root.txt &&
+ git add root.txt &&
+ git commit -m "root" &&
+ test_commit second &&
+
+ expect_changes <<-\EOF &&
+ second
+ 1 0 second.t
+ root
+ 1 0 root.txt
+ EOF
+
+ echo fix >>root.txt &&
+ git add root.txt &&
+ git history fixup HEAD~ &&
+
+ expect_changes <<-\EOF &&
+ second
+ 1 0 second.t
+ root
+ 2 0 root.txt
+ EOF
+
+ git show HEAD~:root.txt >actual &&
+ printf "initial\nfix\n" >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'preserves commit message and authorship' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ echo content >file.txt &&
+ git add file.txt &&
+ git commit --author="Original <original@example.com>" -m "original message" &&
+
+ echo fix >>file.txt &&
+ git add file.txt &&
+ git history fixup HEAD &&
+
+ # Message preserved
+ git log -1 --format="%s" >actual &&
+ echo "original message" >expect &&
+ test_cmp expect actual &&
+
+ # Authorship preserved
+ git log -1 --format="%an <%ae>" >actual &&
+ echo "Original <original@example.com>" >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'updates all descendant branches by default' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit base &&
+ git branch branch &&
+ test_commit ours &&
+ git switch branch &&
+ test_commit theirs &&
+ git switch main &&
+
+ echo fix >fix.txt &&
+ git add fix.txt &&
+ git history fixup base &&
+
+ expect_changes --branches <<-\EOF &&
+ theirs
+ 1 0 theirs.t
+ ours
+ 1 0 ours.t
+ base
+ 1 0 base.t
+ 1 0 fix.txt
+ EOF
+
+ # Both branches should have the fix in the base
+ git show main~:fix.txt >actual &&
+ echo fix >expect &&
+ test_cmp expect actual &&
+ git show branch~:fix.txt >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'can fixup commit on a different branch' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ git branch theirs &&
+ test_commit ours &&
+ git switch theirs &&
+ test_commit theirs &&
+
+ # Stage a change while on "theirs"
+ echo fix >fix.txt &&
+ git add fix.txt &&
+
+ # Ensure that "ours" does not change, as it does not contain
+ # the commit in question.
+ git rev-parse ours >ours-before &&
+ git history fixup theirs &&
+ git rev-parse ours >ours-after &&
+ test_cmp ours-before ours-after &&
+
+ git show HEAD:fix.txt >actual &&
+ echo fix >expect &&
+ test_cmp expect actual
+ )
+'
+
+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 main-tip &&
+ git switch branch &&
+ test_commit branch-tip &&
+ git switch main &&
+
+ echo fix >fix.txt &&
+ git add fix.txt &&
+
+ git refs list >refs-before &&
+ git history fixup --dry-run base >updates &&
+ git refs list >refs-after &&
+ test_cmp refs-before refs-after &&
+
+ test_grep "update refs/heads/main" updates &&
+ test_grep "update refs/heads/branch" updates &&
+
+ expect_changes --branches <<-\EOF &&
+ branch-tip
+ 1 0 branch-tip.t
+ main-tip
+ 1 0 main-tip.t
+ base
+ 1 0 base.t
+ EOF
+
+ git update-ref --stdin <updates &&
+ expect_changes --branches <<-\EOF
+ branch-tip
+ 1 0 branch-tip.t
+ main-tip
+ 1 0 main-tip.t
+ base
+ 1 0 base.t
+ 1 0 fix.txt
+ EOF
+ )
+'
+
+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 &&
+ git branch branch &&
+ test_commit main-tip &&
+ git switch branch &&
+ test_commit branch-tip &&
+
+ echo fix >fix.txt &&
+ git add fix.txt &&
+
+ # Only HEAD (branch) should be updated
+ git history fixup --update-refs=head base &&
+
+ # The main branch should be unaffected.
+ expect_changes main <<-\EOF &&
+ main-tip
+ 1 0 main-tip.t
+ base
+ 1 0 base.t
+ EOF
+
+ # But the currently checked out branch should be modified.
+ expect_changes branch <<-\EOF
+ branch-tip
+ 1 0 branch-tip.t
+ base
+ 1 0 base.t
+ 1 0 fix.txt
+ EOF
+ )
+'
+
+test_expect_success '--update-refs=head refuses to rewrite commits not in HEAD ancestry' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit base &&
+ git branch other &&
+ test_commit main-tip &&
+ git switch other &&
+ test_commit other-tip &&
+
+ echo fix >fix.txt &&
+ git add fix.txt &&
+
+ test_must_fail git history fixup --update-refs=head main-tip 2>err &&
+ test_grep "rewritten commit must be an ancestor of HEAD" err
+ )
+'
+
+test_expect_success 'aborts when fixup would produce conflicts' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ echo "line one" >file.txt &&
+ git add file.txt &&
+ git commit -m "first" &&
+
+ echo "line two" >file.txt &&
+ git add file.txt &&
+ git commit -m "second" &&
+
+ echo "conflicting change" >file.txt &&
+ git add file.txt &&
+
+ git refs list >refs-before &&
+ test_must_fail git history fixup HEAD~ 2>err &&
+ test_grep "fixup would produce conflicts" err &&
+ git refs list >refs-after &&
+ test_cmp refs-before refs-after
+ )
+'
+
+test_expect_success '--reedit-message opens editor for the commit message' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ echo content >file.txt &&
+ git add file.txt &&
+ git commit -m "add file" &&
+
+ echo fix >>file.txt &&
+ git add file.txt &&
+
+ fixup_with_message HEAD <<-\EOF &&
+ add file with fix
+ EOF
+
+ expect_changes --branches <<-\EOF
+ add file with fix
+ 2 0 file.txt
+ initial
+ 1 0 initial.t
+ EOF
+ )
+'
+
+test_expect_success 'retains unstaged working tree changes after fixup' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ touch a b &&
+ git add . &&
+ git commit -m "initial commit" &&
+ echo staged >a &&
+ echo unstaged >b &&
+ git add a &&
+ git history fixup HEAD &&
+
+ # b is still modified in the worktree but not staged
+ cat >expect <<-\EOF &&
+ M b
+ EOF
+ git status --porcelain --untracked-files=no >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'index is clean after fixup when target is HEAD' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ test_commit initial &&
+ echo fix >fix.txt &&
+ git add fix.txt &&
+ git history fixup HEAD &&
+
+ git status --porcelain --untracked-files=no >actual &&
+ test_must_be_empty actual
+ )
+'
+
+test_expect_success 'index is unchanged on conflict' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ echo base >file.txt &&
+ git add file.txt &&
+ git commit -m base &&
+ echo change >file.txt &&
+ git add file.txt &&
+ git commit -m change &&
+
+ echo conflict >file.txt &&
+ git add file.txt &&
+
+ git diff --cached >index-before &&
+ test_must_fail git history fixup HEAD~ &&
+ git diff --cached >index-after &&
+ test_cmp index-before index-after
+ )
+'
+
+test_done
--
2.54.0.545.g6539524ca2.dirty
^ permalink raw reply related
* Re: [PATCH v2] generate-configlist: collapse depfile for older Ninja
From: Patrick Steinhardt @ 2026-04-22 10:30 UTC (permalink / raw)
To: Toon Claes; +Cc: git, D. Ben Knoble
In-Reply-To: <20260422-toon-fix-almalinux8-v2-1-45d8471ed0e9@iotcl.com>
On Wed, Apr 22, 2026 at 09:21:20AM +0200, Toon Claes wrote:
> Changes in v2:
> - Simplify the changes *a lot* by doing the collapsing unconditionally.
> - Link to v1: https://patch.msgid.link/20260421-toon-fix-almalinux8-v1-1-aec1d54addde@iotcl.com
Thanks. I've tested those changes with Ninja 1.8.2 and can confirm that
it does fix the issue.
Patrick
^ permalink raw reply
* Re: [PATCH v2] refs/files: skip lock files during consistency checks
From: Patrick Steinhardt @ 2026-04-22 10:41 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster, Christian Couder
In-Reply-To: <20260422-refs-fsck-skip-lock-files-v2-1-9607571ae59a@gmail.com>
On Wed, Apr 22, 2026 at 11:49:58AM +0200, Karthik Nayak wrote:
> Consistency checks in the files reference backend involve two steps:
>
> 1. Iterate over all entries within the 'refs/' directory and call
> `files_fsck_ref()` on each.
> 2. Iterate over all root refs via `for_each_root_ref()` and call
> `files_fsck_ref()` on each.
>
> `files_fsck_ref()` then runs all fsck checks defined in
> `fsck_refs_fn[]`. Step 2 goes through the refs API and only sees valid
> refs, but step 1 iterates the directory directly and will also encounter
Nit, obviously not worth a reroll: maybe do s/will/may/?
> intermediate '*.lock' files.
>
> Currently, `files_fsck_refs_name()`, one of the functions in
> `fsck_refs_fn[]`, filters out lock files itself. The other function,
> `files_fsck_refs_content()`, has no such check and would parse the lock
> file. Any new function added to `fsck_refs_fn[]` would have the same
> problem.
>
> Move the filter up into `files_fsck_refs_dir()`, where the directory
> iteration happens. Since step 2 cannot produce lock files, this is the
> only site where the filter is needed, and individual checks no longer
> have to re-implement it.
Makes sense.
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index b3b0c25f84..1504a1e2f3 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
> strbuf_addf(&refname, "worktrees/%s/", wt->id);
> strbuf_addf(&refname, "refs/%s", iter->relative_path);
>
> + filename = basename((char *) iter->path.buf);
Not a new issue, but this cast made me wonder. As it turns out,
basename(3p) is documented as "may modify the string pointed to by
path". I assume that this can happen if the path itself ends with a
slash for example, as in that case the basename should of course not
include the slash itself. So maybe it modifies the caller-provided path
directly in that case?
In any case, it shouldn't be much of an issue as we only use this on
discovered path names, and those cannot contain contain a trailing
slash.
Patrick
^ permalink raw reply
* Re: [PATCH 1/8] refs: remove unused typedef 'ref_transaction_commit_fn'
From: Patrick Steinhardt @ 2026-04-22 11:15 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <20260420-refs-move-to-generic-layer-v1-1-513e354f376b@gmail.com>
On Mon, Apr 20, 2026 at 12:11:59PM +0200, Karthik Nayak wrote:
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index d79e35fd26..2d963cc4f4 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -421,10 +421,6 @@ typedef int ref_transaction_abort_fn(struct ref_store *refs,
> struct ref_transaction *transaction,
> struct strbuf *err);
>
> -typedef int ref_transaction_commit_fn(struct ref_store *refs,
> - struct ref_transaction *transaction,
> - struct strbuf *err);
> -
> typedef int optimize_fn(struct ref_store *ref_store,
> struct refs_optimize_opts *opts);
>
I'm in general not much of a fan of these typedefs -- there's not really
much of a point why we'd need them in the first place. We don't use them
as a type anywhere but in the struct definition for the ref backend. So
we could just as well move them in there, which would also ensure that
they cannot become stale in the first place.
Patrick
^ permalink raw reply
* Re: [PATCH 2/8] refs: extract out reflog config to generic layer
From: Patrick Steinhardt @ 2026-04-22 11:15 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <20260420-refs-move-to-generic-layer-v1-2-513e354f376b@gmail.com>
On Mon, Apr 20, 2026 at 12:12:00PM +0200, Karthik Nayak wrote:
> The reference backends need to know when to create reflog entries, this
s/this/which/
> is dictated by the 'core.logallrefupdates' config. Instead of relying on
s/dictated/controlled/
> the backends to call `repo_settings_get_log_all_ref_updates()` to obtain
> this config value, let's do this in the generic layer and pass down the
> value to the backends.
>
> Instead of passing this in as a new argument, let's create a new
> `ref_init_options` structure which will house information required to
> initialize a reference backend. Move the access flags here as well.
I agree with this direction. It's also something that I'm doing for many
callbacks in the ODB layer, and I'm moving more and more into that
direction.
> diff --git a/refs.c b/refs.c
> index bfcb9c7ac3..aa66c6b28e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2295,6 +2295,10 @@ static struct ref_store *ref_store_init(struct repository *repo,
> {
> const struct ref_storage_be *be;
> struct ref_store *refs;
> + struct ref_store_init_options options = {
> + .access_flags = flags,
> + .log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo),
> + };
>
> be = find_ref_storage_backend(format);
> if (!be)
Tiniest nit, please feel free to ignore: we often call the structure
itself `_options`, but the variables just `opts`. May just be my own
preference though.
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 2d963cc4f4..eed13af4eb 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -385,6 +385,21 @@ struct ref_store;
> REF_STORE_ODB | \
> REF_STORE_MAIN)
>
> +/*
> + * Options for initializing the ref backend. All backend-agnostic information
> + * which backends required will be held here.
> + */
> +struct ref_store_init_options {
> + /* The kind of operations that the ref_store is allowed to perform. */
> + unsigned int access_flags;
> +
> + /*
> + * Denotes under what conditions reflogs should be created when updating
> + * references.
> + */
> + enum log_refs_config log_all_ref_updates;
> +};
Nit: it might've made sense to split this up into two steps: the
introduction of the struct, and then moving the config in there.
Patrick
^ permalink raw reply
* Re: [PATCH 3/8] refs: return `ref_transaction_error` from `ref_transaction_update()`
From: Patrick Steinhardt @ 2026-04-22 11:15 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <20260420-refs-move-to-generic-layer-v1-3-513e354f376b@gmail.com>
On Mon, Apr 20, 2026 at 12:12:01PM +0200, Karthik Nayak wrote:
> The `ref_transaction_update()` function is used to add updates to a
> given reference transactions. In the following commit, we'll add more
> validation to this function. As such, it would be more beneficial if the
s/more beneficial/beneficial/
> function returns specific error types, so callers can differentiate
> between different errors.
>
> To facilitate this, return `enum ref_transaction_error` from the
> function and covert the existing '-1' returns to
> 'REF_TRANSACTION_ERROR_GENERIC'. Since this retains the existing
> behavior, no changes are made to any of the callers but this sets the
> necessary infrastructure for introduction of other errors.
Yup, makes sense. This doesn't buy us anything yet, but will eventually.
Patrick
^ permalink raw reply
* Re: [PATCH 4/8] update-ref: move `print_rejected_refs()` up
From: Patrick Steinhardt @ 2026-04-22 11:15 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <20260420-refs-move-to-generic-layer-v1-4-513e354f376b@gmail.com>
On Mon, Apr 20, 2026 at 12:12:02PM +0200, Karthik Nayak wrote:
> The `print_rejected_refs()` is used to print any rejected refs when
Seems like a word is missing here. Maybe "The `print_rejected_refs()`
function"?
Patrick
^ permalink raw reply
* Re: [PATCH 5/8] update-ref: handle rejections while adding updates
From: Patrick Steinhardt @ 2026-04-22 11:15 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <20260420-refs-move-to-generic-layer-v1-5-513e354f376b@gmail.com>
On Mon, Apr 20, 2026 at 12:12:03PM +0200, Karthik Nayak wrote:
> When using git-update-ref(1) with the '--batch-updates' flag, updates
> rejected by the reference backend are displayed to the user while other
> updates are applied. This only applies during the committing of the
Nit: s/committing/commit phase/
> transaction.
>
> In the following commits, we'll also extend `ref_transaction_update()`
> to reject updates before even a transaction is prepared/committed. In
The "even" reads like it's at the wrong position.
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 5259cc7226..d1980c60c4 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -268,11 +277,13 @@ static void print_rejected_refs(const char *refname,
> */
>
> static void parse_cmd_update(struct ref_transaction *transaction,
> - const char *next, const char *end)
> + const char *next, const char *end,
> + struct command_options *opts)
Here you follow my earlier suggestion of using `_options` for the struct
name, but `opts` for the variable :)
> @@ -289,11 +300,18 @@ static void parse_cmd_update(struct ref_transaction *transaction,
> if (*next != line_termination)
> die("update %s: extra input: %s", refname, next);
>
> - if (ref_transaction_update(transaction, refname,
> - &new_oid, have_old ? &old_oid : NULL,
> - NULL, NULL,
> - update_flags | create_reflog_flag,
> - msg, &err))
> + tx_err = ref_transaction_update(transaction, refname,
> + &new_oid, have_old ? &old_oid : NULL,
> + NULL, NULL,
> + update_flags | create_reflog_flag,
> + msg, &err);
> +
> + if (tx_err && tx_err != REF_TRANSACTION_ERROR_GENERIC &&
> + opts->allow_update_failures)
> + print_rejected_refs(refname, have_old ? &old_oid : NULL,
> + &new_oid, NULL, NULL, tx_err, err.buf,
> + NULL);
> + else if (tx_err)
> die("%s", err.buf);
>
> update_flags = default_flags;
I think this could use some curly braces to become easier to read, even
if it's only single-line statements.
Does this change have an impact on the ordering the user sees for
printed errors? If so, it might make sense to point that out in the
commit message.
> @@ -341,12 +361,19 @@ static void parse_cmd_symref_update(struct ref_transaction *transaction,
> if (*next != line_termination)
> die("symref-update %s: extra input: %s", refname, next);
>
> - if (ref_transaction_update(transaction, refname, NULL,
> - have_old_oid ? &old_oid : NULL,
> - new_target,
> - have_old_oid ? NULL : old_target,
> - update_flags | create_reflog_flag,
> - msg, &err))
> + tx_err = ref_transaction_update(transaction, refname, NULL,
> + have_old_oid ? &old_oid : NULL,
> + new_target,
> + have_old_oid ? NULL : old_target,
> + update_flags | create_reflog_flag,
> + msg, &err);
> +
> + if (tx_err && tx_err != REF_TRANSACTION_ERROR_GENERIC &&
> + opts->allow_update_failures)
> + print_rejected_refs(refname, have_old_oid ? &old_oid : NULL,
> + NULL, have_old_oid ? NULL : old_target,
> + new_target, tx_err, err.buf, NULL);
> + else if (tx_err)
> die("%s", err.buf);
>
> update_flags = default_flags;
Same here, curly braces might help readability.
> @@ -644,6 +680,10 @@ static void update_refs_stdin(unsigned int flags)
> struct ref_transaction *transaction;
> int i, j;
>
> + struct command_options opts = {
> + .allow_update_failures = flags & REF_TRANSACTION_ALLOW_FAILURE,
> + };
> +
> transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> flags, &err);
> if (!transaction)
Suggestion, please feel free to ignore: we could also pass the flags
directly instead.
Patrick
^ permalink raw reply
* Re: [PATCH 6/8] refs: move object parsing to the generic layer
From: Patrick Steinhardt @ 2026-04-22 11:15 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <20260420-refs-move-to-generic-layer-v1-6-513e354f376b@gmail.com>
On Mon, Apr 20, 2026 at 12:12:04PM +0200, Karthik Nayak wrote:
> Regular reference updates made via reference transactions validate that
> the provided object ID exists in the object database, this is done by
s/this/which/
> calling 'parse_object()'. This check is done independently by the
> backends.
..., which leads to duplicated logic.
> Let's move this to the generic layer, ensuring the backends only have to
> care about reference storage and not about validation of the object IDs.
> With this also remove the 'REF_TRANSACTION_ERROR_INVALID_NEW_VALUE'
> error type as its no longer used.
>
> Since we don't iterate over individual references in
> `ref_transaction_prepare()`, we add this check to
> `ref_transaction_update()`. This means that the validation is done as
> soon as an update is queued, without needing to prepare the
> transaction. It can be argued that this is more ideal, since this
> validation has no dependency on the reference transaction being
> prepared.
>
> It must be noted that the change in behavior means that this error
> cannot be ignored even with usage of batched updates, since this happens
> when the update is being added to the transaction. But since the caller
> gets specific error codes, they can either abort the transaction or
> continue adding other updates to the transaction.
Right, this is what the preceding commits have allowed us to do.
I think this is a good step. Being less entangled with the object
database in the ref backends is a good thing.
Patrick
^ permalink raw reply
* Re: [PATCH v2] refs/files: skip lock files during consistency checks
From: Karthik Nayak @ 2026-04-22 12:04 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster, Christian Couder
In-Reply-To: <aeil2Q_Mh_fKCwGa@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2676 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Wed, Apr 22, 2026 at 11:49:58AM +0200, Karthik Nayak wrote:
>> Consistency checks in the files reference backend involve two steps:
>>
>> 1. Iterate over all entries within the 'refs/' directory and call
>> `files_fsck_ref()` on each.
>> 2. Iterate over all root refs via `for_each_root_ref()` and call
>> `files_fsck_ref()` on each.
>>
>> `files_fsck_ref()` then runs all fsck checks defined in
>> `fsck_refs_fn[]`. Step 2 goes through the refs API and only sees valid
>> refs, but step 1 iterates the directory directly and will also encounter
>
> Nit, obviously not worth a reroll: maybe do s/will/may/?
>
I thought will would go with 'intermediate' better, but 'may' is the
right choice I guess. Will add it in locally.
>> intermediate '*.lock' files.
>>
>> Currently, `files_fsck_refs_name()`, one of the functions in
>> `fsck_refs_fn[]`, filters out lock files itself. The other function,
>> `files_fsck_refs_content()`, has no such check and would parse the lock
>> file. Any new function added to `fsck_refs_fn[]` would have the same
>> problem.
>>
>> Move the filter up into `files_fsck_refs_dir()`, where the directory
>> iteration happens. Since step 2 cannot produce lock files, this is the
>> only site where the filter is needed, and individual checks no longer
>> have to re-implement it.
>
> Makes sense.
>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index b3b0c25f84..1504a1e2f3 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
>> strbuf_addf(&refname, "worktrees/%s/", wt->id);
>> strbuf_addf(&refname, "refs/%s", iter->relative_path);
>>
>> + filename = basename((char *) iter->path.buf);
>
> Not a new issue, but this cast made me wonder. As it turns out,
> basename(3p) is documented as "may modify the string pointed to by
> path". I assume that this can happen if the path itself ends with a
> slash for example, as in that case the basename should of course not
> include the slash itself. So maybe it modifies the caller-provided path
> directly in that case?
>
I guess it depends on the implementation, the glibc for example doesn't
seem to [1].
> In any case, it shouldn't be much of an issue as we only use this on
> discovered path names, and those cannot contain contain a trailing
> slash.
>
> Patrick
Yeah we should be fine here. Thanks for the review. I'll avoid
re-rolling for now and see if there are other changes needed.
[1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=string/basename.c;h=1658ba98d3ff89e8257b36219599184866798d0d;hb=refs/heads/master
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* [PATCH v5 00/10] parseopt: add subcommand autocorrection
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB080186A23FB9582AD793F0D1CE40A@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Git currently provides auto-correction for builtins and aliases, but
lacks this functionality for subcommands parsed via the parse-options
API. Subcommands are also commands, and typos will occur, too. Like:
git remote add-rul
So, this series introduces subcommand auto-correction.
By default, this implementation enables autocorrection for builtins
with mandatory subcommands. However, for those using
PARSE_OPT_SUBCOMMAND_OPTIONAL, autocorrection is skipped to avoid
misinterpreting legitimate unknown arguments as mistyped subcommands.
To allow builtins with optional subcommands to explicitly opt in,
this series adds the PARSE_OPT_SUBCOMMAND_AUTOCORRECT flag, and enables
it for git-remote and git-notes.
Additionally, the existing autocorrection logic is extracted from
help.c so subcommand handling can reuse the same config parsing and
prompt/delay logic.
Some string literals are also combined so the full text is easier to
grep for.
Changes in v5:
- Make subcommand autocorrection behave the same as command
autocorrection
- Rename PARSE_OPT_SUBCOMMAND_AUTOCORR to
PARSE_OPT_SUBCOMMAND_AUTOCORRECT
- Adjust subcommand autocorrection tests to fit new behavior
Changes in v4:
- Add missing files to Meson build
- Change API prefix from autocorr to autocorrect
- Split the commit that moves tty code
- Add API documentation
- Use standard Damerau-Levenshtein distance and common practice
fuzziness thresholds
- Rename AUTOCORRECT_HINTONLY to AUTOCORRECT_HINT
- Change commit subject prefix for tests from "help:" to "parseopt:"
- Fix coding style issues
Changes in v3:
- Align with the coding guildline
- Split patch so diffs don't get hidden by code movement
- Improve commit messages
Changes in v2:
- Reword the explanation of default autocorrection behavior
Jiamu Sun (10):
parseopt: extract subcommand handling from parse_options_step()
help: make autocorrect handling reusable
help: move tty check for autocorrection to autocorrect.c
autocorrect: use mode and delay instead of magic numbers
autocorrect: rename AUTOCORRECT_SHOW to AUTOCORRECT_HINT
autocorrect: provide config resolution API
parseopt: autocorrect mistyped subcommands
parseopt: enable subcommand autocorrection for git-remote and
git-notes
parseopt: add tests for subcommand autocorrection
doc: document autocorrect API
Makefile | 1 +
autocorrect.c | 89 +++++++++++++++
autocorrect.h | 36 ++++++
builtin/notes.c | 10 +-
builtin/remote.c | 12 +-
help.c | 120 ++++----------------
meson.build | 1 +
parse-options.c | 183 +++++++++++++++++++++++-------
parse-options.h | 1 +
t/meson.build | 1 +
t/t9004-autocorrect-subcommand.sh | 58 ++++++++++
11 files changed, 361 insertions(+), 151 deletions(-)
create mode 100644 autocorrect.c
create mode 100644 autocorrect.h
create mode 100755 t/t9004-autocorrect-subcommand.sh
base-commit: f65aba1e87db64413b6d1ed5ae5a45b5a84a0997
--
2.53.0
^ permalink raw reply
* [PATCH v5 02/10] help: make autocorrect handling reusable
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Move config parsing and prompt/delay handling into autocorrect.c and
expose them in autocorrect.h. This makes autocorrect reusable regardless
of which target links against it.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
Makefile | 1 +
autocorrect.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
autocorrect.h | 16 ++++++++++++
help.c | 64 +++------------------------------------------
meson.build | 1 +
5 files changed, 94 insertions(+), 60 deletions(-)
create mode 100644 autocorrect.c
create mode 100644 autocorrect.h
diff --git a/Makefile b/Makefile
index cedc234173e3..ced8a9b5abb4 100644
--- a/Makefile
+++ b/Makefile
@@ -1100,6 +1100,7 @@ LIB_OBJS += archive-tar.o
LIB_OBJS += archive-zip.o
LIB_OBJS += archive.o
LIB_OBJS += attr.o
+LIB_OBJS += autocorrect.o
LIB_OBJS += base85.o
LIB_OBJS += bisect.o
LIB_OBJS += blame.o
diff --git a/autocorrect.c b/autocorrect.c
new file mode 100644
index 000000000000..97145d3a53ce
--- /dev/null
+++ b/autocorrect.c
@@ -0,0 +1,72 @@
+#include "git-compat-util.h"
+#include "autocorrect.h"
+#include "config.h"
+#include "parse.h"
+#include "strbuf.h"
+#include "prompt.h"
+#include "gettext.h"
+
+static int parse_autocorrect(const char *value)
+{
+ switch (git_parse_maybe_bool_text(value)) {
+ case 1:
+ return AUTOCORRECT_IMMEDIATELY;
+ case 0:
+ return AUTOCORRECT_SHOW;
+ default: /* other random text */
+ break;
+ }
+
+ if (!strcmp(value, "prompt"))
+ return AUTOCORRECT_PROMPT;
+ if (!strcmp(value, "never"))
+ return AUTOCORRECT_NEVER;
+ if (!strcmp(value, "immediate"))
+ return AUTOCORRECT_IMMEDIATELY;
+ if (!strcmp(value, "show"))
+ return AUTOCORRECT_SHOW;
+
+ return 0;
+}
+
+void autocorrect_resolve_config(const char *var, const char *value,
+ const struct config_context *ctx, void *data)
+{
+ int *out = data;
+
+ if (!strcmp(var, "help.autocorrect")) {
+ int v = parse_autocorrect(value);
+
+ if (!v) {
+ v = git_config_int(var, value, ctx->kvi);
+ if (v < 0 || v == 1)
+ v = AUTOCORRECT_IMMEDIATELY;
+ }
+
+ *out = v;
+ }
+}
+
+void autocorrect_confirm(int autocorrect, const char *assumed)
+{
+ if (autocorrect == AUTOCORRECT_IMMEDIATELY) {
+ fprintf_ln(stderr,
+ _("Continuing under the assumption that you meant '%s'."),
+ assumed);
+ } else if (autocorrect == AUTOCORRECT_PROMPT) {
+ char *answer;
+ struct strbuf msg = STRBUF_INIT;
+
+ strbuf_addf(&msg, _("Run '%s' instead [y/N]? "), assumed);
+ answer = git_prompt(msg.buf, PROMPT_ECHO);
+ strbuf_release(&msg);
+
+ if (!(starts_with(answer, "y") || starts_with(answer, "Y")))
+ exit(1);
+ } else {
+ fprintf_ln(stderr,
+ _("Continuing in %0.1f seconds, assuming that you meant '%s'."),
+ (float)autocorrect / 10.0, assumed);
+ sleep_millisec(autocorrect * 100);
+ }
+}
diff --git a/autocorrect.h b/autocorrect.h
new file mode 100644
index 000000000000..f5fadf9d9605
--- /dev/null
+++ b/autocorrect.h
@@ -0,0 +1,16 @@
+#ifndef AUTOCORRECT_H
+#define AUTOCORRECT_H
+
+#define AUTOCORRECT_SHOW (-4)
+#define AUTOCORRECT_PROMPT (-3)
+#define AUTOCORRECT_NEVER (-2)
+#define AUTOCORRECT_IMMEDIATELY (-1)
+
+struct config_context;
+
+void autocorrect_resolve_config(const char *var, const char *value,
+ const struct config_context *ctx, void *data);
+
+void autocorrect_confirm(int autocorrect, const char *assumed);
+
+#endif /* AUTOCORRECT_H */
diff --git a/help.c b/help.c
index 3e59d07c370b..ab619ed43c7a 100644
--- a/help.c
+++ b/help.c
@@ -22,6 +22,7 @@
#include "repository.h"
#include "alias.h"
#include "utf8.h"
+#include "autocorrect.h"
#ifndef NO_CURL
#include "git-curl-compat.h" /* For LIBCURL_VERSION only */
@@ -541,34 +542,6 @@ struct help_unknown_cmd_config {
struct cmdnames aliases;
};
-#define AUTOCORRECT_SHOW (-4)
-#define AUTOCORRECT_PROMPT (-3)
-#define AUTOCORRECT_NEVER (-2)
-#define AUTOCORRECT_IMMEDIATELY (-1)
-
-static int parse_autocorrect(const char *value)
-{
- switch (git_parse_maybe_bool_text(value)) {
- case 1:
- return AUTOCORRECT_IMMEDIATELY;
- case 0:
- return AUTOCORRECT_SHOW;
- default: /* other random text */
- break;
- }
-
- if (!strcmp(value, "prompt"))
- return AUTOCORRECT_PROMPT;
- if (!strcmp(value, "never"))
- return AUTOCORRECT_NEVER;
- if (!strcmp(value, "immediate"))
- return AUTOCORRECT_IMMEDIATELY;
- if (!strcmp(value, "show"))
- return AUTOCORRECT_SHOW;
-
- return 0;
-}
-
static int git_unknown_cmd_config(const char *var, const char *value,
const struct config_context *ctx,
void *cb)
@@ -577,17 +550,7 @@ static int git_unknown_cmd_config(const char *var, const char *value,
const char *subsection, *key;
size_t subsection_len;
- if (!strcmp(var, "help.autocorrect")) {
- int v = parse_autocorrect(value);
-
- if (!v) {
- v = git_config_int(var, value, ctx->kvi);
- if (v < 0 || v == 1)
- v = AUTOCORRECT_IMMEDIATELY;
- }
-
- cfg->autocorrect = v;
- }
+ autocorrect_resolve_config(var, value, ctx, &cfg->autocorrect);
/* Also use aliases for command lookup */
if (!parse_config_key(var, "alias", &subsection, &subsection_len,
@@ -724,27 +687,8 @@ char *help_unknown_cmd(const char *cmd)
_("WARNING: You called a Git command named '%s', "
"which does not exist."),
cmd);
- if (cfg.autocorrect == AUTOCORRECT_IMMEDIATELY)
- fprintf_ln(stderr,
- _("Continuing under the assumption that "
- "you meant '%s'."),
- assumed);
- else if (cfg.autocorrect == AUTOCORRECT_PROMPT) {
- char *answer;
- struct strbuf msg = STRBUF_INIT;
- strbuf_addf(&msg, _("Run '%s' instead [y/N]? "), assumed);
- answer = git_prompt(msg.buf, PROMPT_ECHO);
- strbuf_release(&msg);
- if (!(starts_with(answer, "y") ||
- starts_with(answer, "Y")))
- exit(1);
- } else {
- fprintf_ln(stderr,
- _("Continuing in %0.1f seconds, "
- "assuming that you meant '%s'."),
- (float)cfg.autocorrect/10.0, assumed);
- sleep_millisec(cfg.autocorrect * 100);
- }
+
+ autocorrect_confirm(cfg.autocorrect, assumed);
cmdnames_release(&cfg.aliases);
cmdnames_release(&main_cmds);
diff --git a/meson.build b/meson.build
index 11488623bfd8..be20d507826d 100644
--- a/meson.build
+++ b/meson.build
@@ -290,6 +290,7 @@ libgit_sources = [
'archive-zip.c',
'archive.c',
'attr.c',
+ 'autocorrect.c',
'base85.c',
'bisect.c',
'blame.c',
--
2.53.0
^ permalink raw reply related
* [PATCH v5 01/10] parseopt: extract subcommand handling from parse_options_step()
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Move the subcommand branch out of parse_options_step() into a new
handle_subcommand() helper. Also, make parse_subcommand() return a
simple success/failure status.
This removes the switch over impossible parse_opt_result values and
makes the non-option path easier to follow and maintain.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
parse-options.c | 87 ++++++++++++++++++++++++++-----------------------
1 file changed, 46 insertions(+), 41 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index a676da86f5d6..803ce2ba4443 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -606,17 +606,44 @@ static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p,
return PARSE_OPT_ERROR;
}
-static enum parse_opt_result parse_subcommand(const char *arg,
- const struct option *options)
+static int parse_subcommand(const char *arg, const struct option *options)
{
- for (; options->type != OPTION_END; options++)
- if (options->type == OPTION_SUBCOMMAND &&
- !strcmp(options->long_name, arg)) {
- *(parse_opt_subcommand_fn **)options->value = options->subcommand_fn;
- return PARSE_OPT_SUBCOMMAND;
- }
+ for (; options->type != OPTION_END; options++) {
+ parse_opt_subcommand_fn **opt_val;
- return PARSE_OPT_UNKNOWN;
+ if (options->type != OPTION_SUBCOMMAND ||
+ strcmp(options->long_name, arg))
+ continue;
+
+ opt_val = options->value;
+ *opt_val = options->subcommand_fn;
+ return 0;
+ }
+
+ return -1;
+}
+
+static enum parse_opt_result handle_subcommand(struct parse_opt_ctx_t *ctx,
+ const char *arg,
+ const struct option *options,
+ const char * const usagestr[])
+{
+ int err = parse_subcommand(arg, options);
+
+ if (!err)
+ return PARSE_OPT_SUBCOMMAND;
+
+ /*
+ * arg is neither a short or long option nor a subcommand. Since this
+ * command has a default operation mode, we have to treat this arg and
+ * all remaining args as args meant to that default operation mode.
+ * So we are done parsing.
+ */
+ if (ctx->flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)
+ return PARSE_OPT_DONE;
+
+ error(_("unknown subcommand: `%s'"), arg);
+ usage_with_options(usagestr, options);
}
static void check_typos(const char *arg, const struct option *options)
@@ -1011,38 +1038,16 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
if (*arg != '-' || !arg[1]) {
if (parse_nodash_opt(ctx, arg, options) == 0)
continue;
- if (!ctx->has_subcommands) {
- if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
- return PARSE_OPT_NON_OPTION;
- ctx->out[ctx->cpidx++] = ctx->argv[0];
- continue;
- }
- switch (parse_subcommand(arg, options)) {
- case PARSE_OPT_SUBCOMMAND:
- return PARSE_OPT_SUBCOMMAND;
- case PARSE_OPT_UNKNOWN:
- if (ctx->flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)
- /*
- * arg is neither a short or long
- * option nor a subcommand. Since
- * this command has a default
- * operation mode, we have to treat
- * this arg and all remaining args
- * as args meant to that default
- * operation mode.
- * So we are done parsing.
- */
- return PARSE_OPT_DONE;
- error(_("unknown subcommand: `%s'"), arg);
- usage_with_options(usagestr, options);
- case PARSE_OPT_COMPLETE:
- case PARSE_OPT_HELP:
- case PARSE_OPT_ERROR:
- case PARSE_OPT_DONE:
- case PARSE_OPT_NON_OPTION:
- /* Impossible. */
- BUG("parse_subcommand() cannot return these");
- }
+
+ if (ctx->has_subcommands)
+ return handle_subcommand(ctx, arg, options,
+ usagestr);
+
+ if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
+ return PARSE_OPT_NON_OPTION;
+
+ ctx->out[ctx->cpidx++] = ctx->argv[0];
+ continue;
}
/* lone -h asks for help */
--
2.53.0
^ permalink raw reply related
* [PATCH v5 03/10] help: move tty check for autocorrection to autocorrect.c
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
TTY checking is the autocorrect config parser's responsibility. It must
ensure the parsed value is correct and reliable. Thus, move the check to
autocorrect_resolve_config().
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
autocorrect.c | 24 ++++++++++++++++--------
help.c | 6 ------
2 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/autocorrect.c b/autocorrect.c
index 97145d3a53ce..887d2396da44 100644
--- a/autocorrect.c
+++ b/autocorrect.c
@@ -33,18 +33,26 @@ void autocorrect_resolve_config(const char *var, const char *value,
const struct config_context *ctx, void *data)
{
int *out = data;
+ int parsed;
- if (!strcmp(var, "help.autocorrect")) {
- int v = parse_autocorrect(value);
+ if (strcmp(var, "help.autocorrect"))
+ return;
- if (!v) {
- v = git_config_int(var, value, ctx->kvi);
- if (v < 0 || v == 1)
- v = AUTOCORRECT_IMMEDIATELY;
- }
+ parsed = parse_autocorrect(value);
- *out = v;
+ /*
+ * Disable autocorrection prompt in a non-interactive session
+ */
+ if (parsed == AUTOCORRECT_PROMPT && (!isatty(0) || !isatty(2)))
+ parsed = AUTOCORRECT_NEVER;
+
+ if (!parsed) {
+ parsed = git_config_int(var, value, ctx->kvi);
+ if (parsed < 0 || parsed == 1)
+ parsed = AUTOCORRECT_IMMEDIATELY;
}
+
+ *out = parsed;
}
void autocorrect_confirm(int autocorrect, const char *assumed)
diff --git a/help.c b/help.c
index ab619ed43c7a..d2b29715817e 100644
--- a/help.c
+++ b/help.c
@@ -607,12 +607,6 @@ char *help_unknown_cmd(const char *cmd)
read_early_config(the_repository, git_unknown_cmd_config, &cfg);
- /*
- * Disable autocorrection prompt in a non-interactive session
- */
- if ((cfg.autocorrect == AUTOCORRECT_PROMPT) && (!isatty(0) || !isatty(2)))
- cfg.autocorrect = AUTOCORRECT_NEVER;
-
if (cfg.autocorrect == AUTOCORRECT_NEVER) {
fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);
exit(1);
--
2.53.0
^ permalink raw reply related
* [PATCH v5 04/10] autocorrect: use mode and delay instead of magic numbers
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Drop magic numbers and describe autocorrect config with a mode enum and
an integer delay. This reduces errors when mutating config values and
makes the values easier to access.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
autocorrect.c | 46 +++++++++++++++++++++++-----------------------
autocorrect.h | 20 ++++++++++++++------
help.c | 9 +++++----
3 files changed, 42 insertions(+), 33 deletions(-)
diff --git a/autocorrect.c b/autocorrect.c
index 887d2396da44..2484546fc731 100644
--- a/autocorrect.c
+++ b/autocorrect.c
@@ -6,7 +6,7 @@
#include "prompt.h"
#include "gettext.h"
-static int parse_autocorrect(const char *value)
+static enum autocorrect_mode parse_autocorrect(const char *value)
{
switch (git_parse_maybe_bool_text(value)) {
case 1:
@@ -19,49 +19,49 @@ static int parse_autocorrect(const char *value)
if (!strcmp(value, "prompt"))
return AUTOCORRECT_PROMPT;
- if (!strcmp(value, "never"))
+ else if (!strcmp(value, "never"))
return AUTOCORRECT_NEVER;
- if (!strcmp(value, "immediate"))
+ else if (!strcmp(value, "immediate"))
return AUTOCORRECT_IMMEDIATELY;
- if (!strcmp(value, "show"))
+ else if (!strcmp(value, "show"))
return AUTOCORRECT_SHOW;
-
- return 0;
+ else
+ return AUTOCORRECT_DELAY;
}
void autocorrect_resolve_config(const char *var, const char *value,
const struct config_context *ctx, void *data)
{
- int *out = data;
- int parsed;
+ struct autocorrect *conf = data;
if (strcmp(var, "help.autocorrect"))
return;
- parsed = parse_autocorrect(value);
+ conf->mode = parse_autocorrect(value);
/*
* Disable autocorrection prompt in a non-interactive session
*/
- if (parsed == AUTOCORRECT_PROMPT && (!isatty(0) || !isatty(2)))
- parsed = AUTOCORRECT_NEVER;
+ if (conf->mode == AUTOCORRECT_PROMPT && (!isatty(0) || !isatty(2)))
+ conf->mode = AUTOCORRECT_NEVER;
- if (!parsed) {
- parsed = git_config_int(var, value, ctx->kvi);
- if (parsed < 0 || parsed == 1)
- parsed = AUTOCORRECT_IMMEDIATELY;
- }
+ if (conf->mode == AUTOCORRECT_DELAY) {
+ conf->delay = git_config_int(var, value, ctx->kvi);
- *out = parsed;
+ if (!conf->delay)
+ conf->mode = AUTOCORRECT_SHOW;
+ else if (conf->delay < 0 || conf->delay == 1)
+ conf->mode = AUTOCORRECT_IMMEDIATELY;
+ }
}
-void autocorrect_confirm(int autocorrect, const char *assumed)
+void autocorrect_confirm(struct autocorrect *conf, const char *assumed)
{
- if (autocorrect == AUTOCORRECT_IMMEDIATELY) {
+ if (conf->mode == AUTOCORRECT_IMMEDIATELY) {
fprintf_ln(stderr,
_("Continuing under the assumption that you meant '%s'."),
assumed);
- } else if (autocorrect == AUTOCORRECT_PROMPT) {
+ } else if (conf->mode == AUTOCORRECT_PROMPT) {
char *answer;
struct strbuf msg = STRBUF_INIT;
@@ -71,10 +71,10 @@ void autocorrect_confirm(int autocorrect, const char *assumed)
if (!(starts_with(answer, "y") || starts_with(answer, "Y")))
exit(1);
- } else {
+ } else if (conf->mode == AUTOCORRECT_DELAY) {
fprintf_ln(stderr,
_("Continuing in %0.1f seconds, assuming that you meant '%s'."),
- (float)autocorrect / 10.0, assumed);
- sleep_millisec(autocorrect * 100);
+ conf->delay / 10.0, assumed);
+ sleep_millisec(conf->delay * 100);
}
}
diff --git a/autocorrect.h b/autocorrect.h
index f5fadf9d9605..5506a36f11a7 100644
--- a/autocorrect.h
+++ b/autocorrect.h
@@ -1,16 +1,24 @@
#ifndef AUTOCORRECT_H
#define AUTOCORRECT_H
-#define AUTOCORRECT_SHOW (-4)
-#define AUTOCORRECT_PROMPT (-3)
-#define AUTOCORRECT_NEVER (-2)
-#define AUTOCORRECT_IMMEDIATELY (-1)
-
struct config_context;
+enum autocorrect_mode {
+ AUTOCORRECT_SHOW,
+ AUTOCORRECT_NEVER,
+ AUTOCORRECT_PROMPT,
+ AUTOCORRECT_IMMEDIATELY,
+ AUTOCORRECT_DELAY,
+};
+
+struct autocorrect {
+ enum autocorrect_mode mode;
+ int delay;
+};
+
void autocorrect_resolve_config(const char *var, const char *value,
const struct config_context *ctx, void *data);
-void autocorrect_confirm(int autocorrect, const char *assumed);
+void autocorrect_confirm(struct autocorrect *conf, const char *assumed);
#endif /* AUTOCORRECT_H */
diff --git a/help.c b/help.c
index d2b29715817e..353596c17d82 100644
--- a/help.c
+++ b/help.c
@@ -538,7 +538,7 @@ int is_in_cmdlist(struct cmdnames *c, const char *s)
}
struct help_unknown_cmd_config {
- int autocorrect;
+ struct autocorrect autocorrect;
struct cmdnames aliases;
};
@@ -607,7 +607,7 @@ char *help_unknown_cmd(const char *cmd)
read_early_config(the_repository, git_unknown_cmd_config, &cfg);
- if (cfg.autocorrect == AUTOCORRECT_NEVER) {
+ if (cfg.autocorrect.mode == AUTOCORRECT_NEVER) {
fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);
exit(1);
}
@@ -673,7 +673,8 @@ char *help_unknown_cmd(const char *cmd)
n++)
; /* still counting */
}
- if (cfg.autocorrect && cfg.autocorrect != AUTOCORRECT_SHOW && n == 1 &&
+
+ if (cfg.autocorrect.mode != AUTOCORRECT_SHOW && n == 1 &&
SIMILAR_ENOUGH(best_similarity)) {
char *assumed = xstrdup(main_cmds.names[0]->name);
@@ -682,7 +683,7 @@ char *help_unknown_cmd(const char *cmd)
"which does not exist."),
cmd);
- autocorrect_confirm(cfg.autocorrect, assumed);
+ autocorrect_confirm(&cfg.autocorrect, assumed);
cmdnames_release(&cfg.aliases);
cmdnames_release(&main_cmds);
--
2.53.0
^ permalink raw reply related
* [PATCH v5 05/10] autocorrect: rename AUTOCORRECT_SHOW to AUTOCORRECT_HINT
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
AUTOCORRECT_SHOW is ambiguous. Its purpose is to show commands similar
to the unknown one and take no other action. Rename it to fit the
semantics.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
autocorrect.c | 6 +++---
autocorrect.h | 2 +-
help.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/autocorrect.c b/autocorrect.c
index 2484546fc731..de0fa282c934 100644
--- a/autocorrect.c
+++ b/autocorrect.c
@@ -12,7 +12,7 @@ static enum autocorrect_mode parse_autocorrect(const char *value)
case 1:
return AUTOCORRECT_IMMEDIATELY;
case 0:
- return AUTOCORRECT_SHOW;
+ return AUTOCORRECT_HINT;
default: /* other random text */
break;
}
@@ -24,7 +24,7 @@ static enum autocorrect_mode parse_autocorrect(const char *value)
else if (!strcmp(value, "immediate"))
return AUTOCORRECT_IMMEDIATELY;
else if (!strcmp(value, "show"))
- return AUTOCORRECT_SHOW;
+ return AUTOCORRECT_HINT;
else
return AUTOCORRECT_DELAY;
}
@@ -49,7 +49,7 @@ void autocorrect_resolve_config(const char *var, const char *value,
conf->delay = git_config_int(var, value, ctx->kvi);
if (!conf->delay)
- conf->mode = AUTOCORRECT_SHOW;
+ conf->mode = AUTOCORRECT_HINT;
else if (conf->delay < 0 || conf->delay == 1)
conf->mode = AUTOCORRECT_IMMEDIATELY;
}
diff --git a/autocorrect.h b/autocorrect.h
index 5506a36f11a7..328807242c15 100644
--- a/autocorrect.h
+++ b/autocorrect.h
@@ -4,7 +4,7 @@
struct config_context;
enum autocorrect_mode {
- AUTOCORRECT_SHOW,
+ AUTOCORRECT_HINT,
AUTOCORRECT_NEVER,
AUTOCORRECT_PROMPT,
AUTOCORRECT_IMMEDIATELY,
diff --git a/help.c b/help.c
index 353596c17d82..c7dab8395ee2 100644
--- a/help.c
+++ b/help.c
@@ -674,7 +674,7 @@ char *help_unknown_cmd(const char *cmd)
; /* still counting */
}
- if (cfg.autocorrect.mode != AUTOCORRECT_SHOW && n == 1 &&
+ if (cfg.autocorrect.mode != AUTOCORRECT_HINT && n == 1 &&
SIMILAR_ENOUGH(best_similarity)) {
char *assumed = xstrdup(main_cmds.names[0]->name);
--
2.53.0
^ permalink raw reply related
* [PATCH v5 06/10] autocorrect: provide config resolution API
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Add autocorrect_resolve(). This resolves and populates the correct
values for autocorrect config.
Make autocorrect config callback internal. The API is meant to provide
a high-level way to retrieve the config. Allowing access to the config
callback from outside violates that intent.
Additionally, in some cases, without access to the config callback, two
config iterations cannot be merged into one, which can hurt performance.
This is fine, as the code path that calls autocorrect_resolve() is cold.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
autocorrect.c | 15 ++++++++++++---
autocorrect.h | 5 +----
help.c | 40 +++++++++++++++++-----------------------
3 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/autocorrect.c b/autocorrect.c
index de0fa282c934..b2ee9f51e8c0 100644
--- a/autocorrect.c
+++ b/autocorrect.c
@@ -1,3 +1,5 @@
+#define USE_THE_REPOSITORY_VARIABLE
+
#include "git-compat-util.h"
#include "autocorrect.h"
#include "config.h"
@@ -29,13 +31,13 @@ static enum autocorrect_mode parse_autocorrect(const char *value)
return AUTOCORRECT_DELAY;
}
-void autocorrect_resolve_config(const char *var, const char *value,
- const struct config_context *ctx, void *data)
+static int resolve_autocorrect(const char *var, const char *value,
+ const struct config_context *ctx, void *data)
{
struct autocorrect *conf = data;
if (strcmp(var, "help.autocorrect"))
- return;
+ return 0;
conf->mode = parse_autocorrect(value);
@@ -53,6 +55,13 @@ void autocorrect_resolve_config(const char *var, const char *value,
else if (conf->delay < 0 || conf->delay == 1)
conf->mode = AUTOCORRECT_IMMEDIATELY;
}
+
+ return 0;
+}
+
+void autocorrect_resolve(struct autocorrect *conf)
+{
+ read_early_config(the_repository, resolve_autocorrect, conf);
}
void autocorrect_confirm(struct autocorrect *conf, const char *assumed)
diff --git a/autocorrect.h b/autocorrect.h
index 328807242c15..0d3e819262ed 100644
--- a/autocorrect.h
+++ b/autocorrect.h
@@ -1,8 +1,6 @@
#ifndef AUTOCORRECT_H
#define AUTOCORRECT_H
-struct config_context;
-
enum autocorrect_mode {
AUTOCORRECT_HINT,
AUTOCORRECT_NEVER,
@@ -16,8 +14,7 @@ struct autocorrect {
int delay;
};
-void autocorrect_resolve_config(const char *var, const char *value,
- const struct config_context *ctx, void *data);
+void autocorrect_resolve(struct autocorrect *conf);
void autocorrect_confirm(struct autocorrect *conf, const char *assumed);
diff --git a/help.c b/help.c
index c7dab8395ee2..0fff43545cc1 100644
--- a/help.c
+++ b/help.c
@@ -537,32 +537,23 @@ int is_in_cmdlist(struct cmdnames *c, const char *s)
return 0;
}
-struct help_unknown_cmd_config {
- struct autocorrect autocorrect;
- struct cmdnames aliases;
-};
-
-static int git_unknown_cmd_config(const char *var, const char *value,
- const struct config_context *ctx,
- void *cb)
+static int resolve_aliases(const char *var, const char *value UNUSED,
+ const struct config_context *ctx UNUSED, void *data)
{
- struct help_unknown_cmd_config *cfg = cb;
+ struct cmdnames *aliases = data;
const char *subsection, *key;
size_t subsection_len;
- autocorrect_resolve_config(var, value, ctx, &cfg->autocorrect);
-
- /* Also use aliases for command lookup */
if (!parse_config_key(var, "alias", &subsection, &subsection_len,
&key)) {
if (subsection) {
/* [alias "name"] command = value */
if (!strcmp(key, "command"))
- add_cmdname(&cfg->aliases, subsection,
+ add_cmdname(aliases, subsection,
subsection_len);
} else {
/* alias.name = value */
- add_cmdname(&cfg->aliases, key, strlen(key));
+ add_cmdname(aliases, key, strlen(key));
}
}
@@ -599,22 +590,26 @@ static const char bad_interpreter_advice[] =
char *help_unknown_cmd(const char *cmd)
{
- struct help_unknown_cmd_config cfg = { 0 };
+ struct cmdnames aliases = { 0 };
+ struct autocorrect autocorrect = { 0 };
int i, n, best_similarity = 0;
struct cmdnames main_cmds = { 0 };
struct cmdnames other_cmds = { 0 };
struct cmdname_help *common_cmds;
- read_early_config(the_repository, git_unknown_cmd_config, &cfg);
+ autocorrect_resolve(&autocorrect);
- if (cfg.autocorrect.mode == AUTOCORRECT_NEVER) {
+ if (autocorrect.mode == AUTOCORRECT_NEVER) {
fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);
exit(1);
}
load_command_list("git-", &main_cmds, &other_cmds);
- add_cmd_list(&main_cmds, &cfg.aliases);
+ /* Also use aliases for command lookup */
+ read_early_config(the_repository, resolve_aliases, &aliases);
+
+ add_cmd_list(&main_cmds, &aliases);
add_cmd_list(&main_cmds, &other_cmds);
QSORT(main_cmds.names, main_cmds.cnt, cmdname_compare);
uniq(&main_cmds);
@@ -674,18 +669,17 @@ char *help_unknown_cmd(const char *cmd)
; /* still counting */
}
- if (cfg.autocorrect.mode != AUTOCORRECT_HINT && n == 1 &&
+ if (autocorrect.mode != AUTOCORRECT_HINT && n == 1 &&
SIMILAR_ENOUGH(best_similarity)) {
char *assumed = xstrdup(main_cmds.names[0]->name);
fprintf_ln(stderr,
- _("WARNING: You called a Git command named '%s', "
- "which does not exist."),
+ _("WARNING: You called a Git command named '%s', which does not exist."),
cmd);
- autocorrect_confirm(&cfg.autocorrect, assumed);
+ autocorrect_confirm(&autocorrect, assumed);
- cmdnames_release(&cfg.aliases);
+ cmdnames_release(&aliases);
cmdnames_release(&main_cmds);
cmdnames_release(&other_cmds);
return assumed;
--
2.53.0
^ permalink raw reply related
* [PATCH v5 07/10] parseopt: autocorrect mistyped subcommands
From: Jiamu Sun @ 2026-04-22 12:18 UTC (permalink / raw)
To: git; +Cc: aplattner, gitster, karthik.188, Jiamu Sun
In-Reply-To: <SY0P300MB0801AE56F740AD087D22B35ACE2D2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Try to autocorrect the mistyped mandatory subcommand before showing an
error and exiting. Subcommands parsed with PARSE_OPT_SUBCOMMAND_OPTIONAL
are skipped.
The subcommand autocorrection behaves the same as the command
autocorrection.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
autocorrect.h | 4 ++
help.c | 13 ++----
parse-options.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 107 insertions(+), 12 deletions(-)
diff --git a/autocorrect.h b/autocorrect.h
index 0d3e819262ed..14ee7c4548d3 100644
--- a/autocorrect.h
+++ b/autocorrect.h
@@ -1,6 +1,10 @@
#ifndef AUTOCORRECT_H
#define AUTOCORRECT_H
+/* An empirically derived magic number */
+#define AUTOCORRECT_SIMILARITY_FLOOR 7
+#define AUTOCORRECT_SIMILAR_ENOUGH(x) ((x) < AUTOCORRECT_SIMILARITY_FLOOR)
+
enum autocorrect_mode {
AUTOCORRECT_HINT,
AUTOCORRECT_NEVER,
diff --git a/help.c b/help.c
index 0fff43545cc1..ecc5673b7243 100644
--- a/help.c
+++ b/help.c
@@ -580,10 +580,6 @@ static void add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
old->cnt = 0;
}
-/* An empirically derived magic number */
-#define SIMILARITY_FLOOR 7
-#define SIMILAR_ENOUGH(x) ((x) < SIMILARITY_FLOOR)
-
static const char bad_interpreter_advice[] =
N_("'%s' appears to be a git command, but we were not\n"
"able to execute it. Maybe git-%s is broken?");
@@ -659,7 +655,7 @@ char *help_unknown_cmd(const char *cmd)
if (main_cmds.cnt <= n) {
/* prefix matches with everything? that is too ambiguous */
- best_similarity = SIMILARITY_FLOOR + 1;
+ best_similarity = AUTOCORRECT_SIMILARITY_FLOOR + 1;
} else {
/* count all the most similar ones */
for (best_similarity = main_cmds.names[n++]->len;
@@ -670,7 +666,7 @@ char *help_unknown_cmd(const char *cmd)
}
if (autocorrect.mode != AUTOCORRECT_HINT && n == 1 &&
- SIMILAR_ENOUGH(best_similarity)) {
+ AUTOCORRECT_SIMILAR_ENOUGH(best_similarity)) {
char *assumed = xstrdup(main_cmds.names[0]->name);
fprintf_ln(stderr,
@@ -687,11 +683,10 @@ char *help_unknown_cmd(const char *cmd)
fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);
- if (SIMILAR_ENOUGH(best_similarity)) {
+ if (AUTOCORRECT_SIMILAR_ENOUGH(best_similarity)) {
fprintf_ln(stderr,
Q_("\nThe most similar command is",
- "\nThe most similar commands are",
- n));
+ "\nThe most similar commands are", n));
for (i = 0; i < n; i++)
fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
diff --git a/parse-options.c b/parse-options.c
index 803ce2ba4443..a488f9a41df8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -7,6 +7,8 @@
#include "string-list.h"
#include "strmap.h"
#include "utf8.h"
+#include "autocorrect.h"
+#include "levenshtein.h"
static int disallow_abbreviated_options;
@@ -623,13 +625,98 @@ static int parse_subcommand(const char *arg, const struct option *options)
return -1;
}
+static void find_subcommands(struct string_list *list,
+ const struct option *options)
+{
+ for (; options->type != OPTION_END; options++) {
+ if (options->type == OPTION_SUBCOMMAND)
+ string_list_append(list, options->long_name);
+ }
+}
+
+static int levenshtein_compare(const void *p1, const void *p2)
+{
+ const struct string_list_item *i1 = p1, *i2 = p2;
+ const char *s1 = i1->string, *s2 = i2->string;
+ int l1 = (intptr_t)i1->util;
+ int l2 = (intptr_t)i2->util;
+
+ return l1 != l2 ? l1 - l2 : strcmp(s1, s2);
+}
+
+static const char *autocorrect_subcommand(const char *cmd,
+ struct string_list *cmds)
+{
+ struct autocorrect autocorrect = { 0 };
+ unsigned int n = 0, best = 0;
+ struct string_list_item *cand;
+
+ autocorrect_resolve(&autocorrect);
+
+ if (autocorrect.mode == AUTOCORRECT_NEVER)
+ return NULL;
+
+ for_each_string_list_item(cand, cmds) {
+ if (starts_with(cand->string, cmd)) {
+ cand->util = 0;
+ } else {
+ int edit = levenshtein(cmd, cand->string,
+ 0, 2, 1, 3) + 1;
+
+ cand->util = (void *)(intptr_t)edit;
+ }
+ }
+
+ QSORT(cmds->items, cmds->nr, levenshtein_compare);
+
+ /* Match help.c:help_unknown_cmd */
+ for (; n < cmds->nr && !cmds->items[n].util; n++);
+
+ if (n == cmds->nr)
+ /* prefix matches with every subcommands */
+ best = AUTOCORRECT_SIMILARITY_FLOOR + 1;
+ else
+ for (best = (intptr_t)cmds->items[n++].util;
+ (n < cmds->nr && best == (intptr_t)cmds->items[n].util);
+ n++);
+
+ if (autocorrect.mode != AUTOCORRECT_HINT && n == 1 &&
+ AUTOCORRECT_SIMILAR_ENOUGH(best)) {
+ fprintf_ln(stderr,
+ _("WARNING: You called a subcommand named '%s', which does not exist."),
+ cmd);
+
+ autocorrect_confirm(&autocorrect, cmds->items[0].string);
+ return cmds->items[0].string;
+ }
+
+ if (AUTOCORRECT_SIMILAR_ENOUGH(best)) {
+ error(_("'%s' is not a subcommand."), cmd);
+
+ fprintf_ln(stderr,
+ Q_("\nThe most similar subcommand is",
+ "\nThe most similar subcommands are",
+ n));
+
+ for (unsigned int i = 0; i < n; i++)
+ fprintf(stderr, "\t%s\n", cmds->items[i].string);
+
+ exit(1);
+ }
+
+ return NULL;
+}
+
static enum parse_opt_result handle_subcommand(struct parse_opt_ctx_t *ctx,
const char *arg,
const struct option *options,
const char * const usagestr[])
{
- int err = parse_subcommand(arg, options);
+ int err;
+ const char *assumed;
+ struct string_list cmds = STRING_LIST_INIT_NODUP;
+ err = parse_subcommand(arg, options);
if (!err)
return PARSE_OPT_SUBCOMMAND;
@@ -642,8 +729,17 @@ static enum parse_opt_result handle_subcommand(struct parse_opt_ctx_t *ctx,
if (ctx->flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)
return PARSE_OPT_DONE;
- error(_("unknown subcommand: `%s'"), arg);
- usage_with_options(usagestr, options);
+ find_subcommands(&cmds, options);
+ assumed = autocorrect_subcommand(arg, &cmds);
+
+ if (!assumed) {
+ error(_("unknown subcommand: `%s'"), arg);
+ usage_with_options(usagestr, options);
+ }
+
+ string_list_clear(&cmds, 0);
+ parse_subcommand(assumed, options);
+ return PARSE_OPT_SUBCOMMAND;
}
static void check_typos(const char *arg, const struct option *options)
--
2.53.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox