* Re: git grep bug with --column and --only-matching
From: Phillip Wood @ 2026-04-23 9:44 UTC (permalink / raw)
To: René Scharfe, Brandon Chinn
Cc: git, Taylor Blau, Jeff King, Junio C Hamano
In-Reply-To: <3ce1906a-85e3-4fb1-9ebc-a5639f3194c9@web.de>
On 21/04/2026 21:33, René Scharfe wrote:
> On 4/21/26 7:03 AM, Brandon Chinn wrote:
>> I'm encountering a bug when using `git grep` with both `--column` and
>> `--only-matching`, is this a known limitation?
>>
>> Repro:
>>
>> ```
>> $ echo 'x x x' > repro.txt
>>
>> $ grep -bo x repro.txt
>> 0:x
>> 4:x
>> 8:x
>>
>> $ git grep --no-index -o -n --column x repro.txt
>> repro.txt:1: 1:x
>> repro.txt:1: 2:x
>> repro.txt:1: 6:x
>
> The first column value matches (1-based for git grep, 0-based for
> grep(1)), the remaining ones are different. grep(1) shows the offset
> like for the first match, but git grep adds the relative position of the
> end of the previous match.
I'm having a hard time understanding what git is doing. Looking at your
example with the ruler
> (`man gitcvs-migration` or `git help cvs-migration` if git is
> ^^^ ^^^ ^^^
> 1111111111222222222233333333334444444444555555555566
> 123456789 123456789 123456789 123456789 123456789 123456789 1
>
> 7:git
> 16:git
> 38:git
The first match ends at column 9 and the second match starts at column
29 so how do we end up with 16 as the "column" of the second match when
the difference between them is 20?
> I don't know how to use the resulting numbers and I agree that it seems
> like a bug -- showing the column of each match within its line makes
> more sense for an option named --column.
Indeed
> However, the original submission of this feature in
> https://lore.kernel.org/git/cover.1529961706.git.me@ttaylorr.com/
> gave similar examples in its commit message and its tests, called
> them "as one would expect" and was not challenged on that, so I may be
> missing something.
I wonder how hard anyone looked at the examples and tests, the
discussion seemed to have focused on other things.
Thanks
Phillip
> Here's its first example with underlined matches, a ruler and the
> intended output (without unnecessary headers):
>
> (`man gitcvs-migration` or `git help cvs-migration` if git is
> ^^^ ^^^ ^^^
> 1111111111222222222233333333334444444444555555555566
> 123456789 123456789 123456789 123456789 123456789 123456789 1
>
> 7:git
> 16:git
> 38:git
>
> And here's the last line of the test file from t7810 with underlined
> matches (the other four lines are very similar), a ruler and the
> expected output (unncessary headers removed):
>
> foo_mmap bar mmap baz
> ^^^^ ^^^^
> 111111111122
> 123456789 123456789 1
>
> 5:mmap
> 13:mmap
>
> If we wanted to show the column of matches 2 and beyond then we could do
> something like this:
>
>
> diff --git a/grep.c b/grep.c
> index c7e1dc1e0e..a54e5d86a9 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -1267,6 +1267,7 @@ static void show_line(struct grep_opt *opt,
> regmatch_t match;
> enum grep_context ctx = GREP_CONTEXT_BODY;
> int eflags = 0;
> + const char *start = bol;
>
> if (want_color(opt->color)) {
> if (sign == ':')
> @@ -1285,6 +1286,7 @@ static void show_line(struct grep_opt *opt,
> if (match.rm_so == match.rm_eo)
> break;
>
> + cno = bol - start + match.rm_so + 1;
> if (opt->only_matching)
> show_line_header(opt, name, lno, cno, sign);
> else
> @@ -1294,7 +1296,6 @@ static void show_line(struct grep_opt *opt,
> if (opt->only_matching)
> opt->output(opt, "\n", 1);
> bol += match.rm_eo;
> - cno += match.rm_eo;
> rest -= match.rm_eo;
> eflags = REG_NOTBOL;
> }
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 64ac4f04ee..bd439563d6 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -322,11 +322,11 @@ do
> ${HC}file:1:5:mmap
> ${HC}file:2:5:mmap
> ${HC}file:3:5:mmap
> - ${HC}file:3:13:mmap
> + ${HC}file:3:14:mmap
> ${HC}file:4:5:mmap
> - ${HC}file:4:13:mmap
> + ${HC}file:4:14:mmap
> ${HC}file:5:5:mmap
> - ${HC}file:5:13:mmap
> + ${HC}file:5:14:mmap
> EOF
> git grep --column -n -o -e mmap $H >actual &&
> test_cmp expected actual
>
>
^ permalink raw reply
* Re: [PATCH v2 6/9] update-ref: handle rejections while adding updates
From: Patrick Steinhardt @ 2026-04-23 8:52 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-6-ae5a4f146d7d@gmail.com>
On Thu, Apr 23, 2026 at 10:40:35AM +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 commit phase of the
> transaction.
>
> In the following commits, we'll also extend `ref_transaction_update()`
> to reject updates before a transaction is prepared/committed. In
> preparation, modify the code in update-ref to also handle non-generic
> rejections from `ref_transaction_update()`. This involves propagating
> information to each of the commands on whether updates are allowed to be
> rejected, and also checking for rejections and only dying for generic
> failures.
I noticed that you didn't address my feedback on the changed ordering of
errors I posted on the preceding version. Was this intentional?
Patrick
^ permalink raw reply
* Re: [PATCH v2 3/9] refs: extract out reflog config to generic layer
From: Patrick Steinhardt @ 2026-04-23 8:52 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-3-ae5a4f146d7d@gmail.com>
On Thu, Apr 23, 2026 at 10:40:32AM +0200, Karthik Nayak wrote:
> The reference backends need to know when to create reflog entries, this
> is dictated by the 'core.logallrefupdates' config. Instead of relying on
> 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.
Great, this is now a ton easier to review. Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v2 2/9] refs: introduce `ref_store_init_options`
From: Patrick Steinhardt @ 2026-04-23 8:52 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-2-ae5a4f146d7d@gmail.com>
On Thu, Apr 23, 2026 at 10:40:31AM +0200, Karthik Nayak wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index b3b0c25f84..78150ad209 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -120,11 +120,13 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
> &ref_common_dir);
>
> base_ref_store_init(ref_store, repo, refdir.buf, &refs_be_files);
> - refs->store_flags = flags;
> +
> refs->gitcommondir = strbuf_detach(&ref_common_dir, NULL);
> refs->packed_ref_store =
> - packed_ref_store_init(repo, NULL, refs->gitcommondir, flags);
> + packed_ref_store_init(repo, payload, refs->gitcommondir, opts);
Is this change here intentional? Doesn't seem to be related to any of
the other changes here.
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 2d963cc4f4..f49b3807bf 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -385,6 +385,15 @@ 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;
Smells like something that should be converted into an enum eventually,
but that definitely is out of scope for this patch series.
Patrick
^ permalink raw reply
* [PATCH v2 9/9] refs: use peeled tag values in reference backends
From: Karthik Nayak @ 2026-04-23 8:40 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, gitster, ps
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-0-ae5a4f146d7d@gmail.com>
The reference backends peel tag objects when storing references to them.
This is to provide optimized reads which avoids hitting the odb. The
previous commits ensures that the peeled value is now propagated via the
generic layer. So modify the packed and reftable backend to directly use
this value instead of calling `peel_object()` independently.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/packed-backend.c | 6 ++----
refs/reftable-backend.c | 9 ++-------
2 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 35a0f32e1c..0acde48c45 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1531,13 +1531,11 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
*/
i++;
} else {
- struct object_id peeled;
- int peel_error = peel_object(refs->base.repo, &update->new_oid,
- &peeled, PEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE);
+ bool peeled = update->flags & REF_HAVE_PEELED;
if (write_packed_entry(out, update->refname,
&update->new_oid,
- peel_error ? NULL : &peeled))
+ peeled ? &update->peeled : NULL))
goto write_error;
i++;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index b0c010387d..8b4ac2e618 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -12,7 +12,6 @@
#include "../hex.h"
#include "../ident.h"
#include "../iterator.h"
-#include "../object.h"
#include "../parse.h"
#include "../path.h"
#include "../refs.h"
@@ -1584,17 +1583,13 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
goto done;
} else if (u->flags & REF_HAVE_NEW) {
struct reftable_ref_record ref = {0};
- struct object_id peeled;
- int peel_error;
ref.refname = (char *)u->refname;
ref.update_index = ts;
- peel_error = peel_object(arg->refs->base.repo, &u->new_oid, &peeled,
- PEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE);
- if (!peel_error) {
+ if (u->flags & REF_HAVE_PEELED) {
ref.value_type = REFTABLE_REF_VAL2;
- memcpy(ref.value.val2.target_value, peeled.hash, GIT_MAX_RAWSZ);
+ memcpy(ref.value.val2.target_value, u->peeled.hash, GIT_MAX_RAWSZ);
memcpy(ref.value.val2.value, u->new_oid.hash, GIT_MAX_RAWSZ);
} else if (!is_null_oid(&u->new_oid)) {
ref.value_type = REFTABLE_REF_VAL1;
--
2.53.GIT
^ permalink raw reply related
* [PATCH v2 8/9] refs: add peeled object ID to the `ref_update` struct
From: Karthik Nayak @ 2026-04-23 8:40 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, gitster, ps
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-0-ae5a4f146d7d@gmail.com>
Certain reference backend {packed, reftable}, have the ability to also
store the peeled object ID for a reference pointing to a tag object.
This has the added benefit that during retrieval of such references, we
also obtain the peeled object ID without having to use the ODB.
To provide this functionality, each backend independently calls the ODB
to obtain the peeled OID. To move this functionality to the generic
layer, there must be support infrastructure to pass in a peeled OID for
reference updates.
Add a `peeled` field to the `ref_update` structure and modify
`ref_transaction_add_update()` to receive and copy this object ID to the
`ref_update` structure. Finally, modify `ref_transaction_update()` to
peel tag objects and pass the peeled OID to
`ref_transaction_add_update()`.
Update all callers of these functions with the new function parameters.
Callers which only add reflog updates, need to only pass in NULL, since
for reflogs, we don't store peeled OIDs. Reference deletions also only
need to pass in NULL. For others, pass along the peeled OID if
available.
In a following commit, we'll modify the backends to use this peeled OID
instead of parsing it themselves.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 15 +++++++++++++--
refs/files-backend.c | 20 ++++++++++++--------
refs/refs-internal.h | 14 ++++++++++++++
refs/reftable-backend.c | 6 +++---
4 files changed, 42 insertions(+), 13 deletions(-)
diff --git a/refs.c b/refs.c
index 662a9e6f9e..0648df2b6c 100644
--- a/refs.c
+++ b/refs.c
@@ -1307,6 +1307,7 @@ struct ref_update *ref_transaction_add_update(
const char *refname, unsigned int flags,
const struct object_id *new_oid,
const struct object_id *old_oid,
+ const struct object_id *peeled,
const char *new_target, const char *old_target,
const char *committer_info,
const char *msg)
@@ -1339,6 +1340,8 @@ struct ref_update *ref_transaction_add_update(
update->committer_info = xstrdup_or_null(committer_info);
update->msg = normalize_reflog_message(msg);
}
+ if (flags & REF_HAVE_PEELED)
+ oidcpy(&update->peeled, peeled);
/*
* This list is generally used by the backends to avoid duplicates.
@@ -1392,6 +1395,8 @@ enum ref_transaction_error ref_transaction_update(struct ref_transaction *transa
unsigned int flags, const char *msg,
struct strbuf *err)
{
+ struct object_id peeled;
+
assert(err);
if ((flags & REF_FORCE_CREATE_REFLOG) &&
@@ -1432,10 +1437,16 @@ enum ref_transaction_error ref_transaction_update(struct ref_transaction *transa
oid_to_hex(new_oid), refname);
return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
}
+
+ if (o->type == OBJ_TAG) {
+ if (!peel_object(transaction->ref_store->repo, new_oid, &peeled,
+ PEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE))
+ flags |= REF_HAVE_PEELED;
+ }
}
ref_transaction_add_update(transaction, refname, flags,
- new_oid, old_oid, new_target,
+ new_oid, old_oid, &peeled, new_target,
old_target, NULL, msg);
return 0;
@@ -1462,7 +1473,7 @@ int ref_transaction_update_reflog(struct ref_transaction *transaction,
return -1;
update = ref_transaction_add_update(transaction, refname, flags,
- new_oid, old_oid, NULL, NULL,
+ new_oid, old_oid, NULL, NULL, NULL,
committer_info, msg);
update->index = index;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6da0cb4f1b..2ed1082d56 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1325,7 +1325,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
ref_transaction_add_update(
transaction, r->name,
REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_IS_PRUNING,
- null_oid(the_hash_algo), &r->oid, NULL, NULL, NULL, NULL);
+ null_oid(the_hash_algo), &r->oid, NULL, NULL, NULL,
+ NULL, NULL);
if (ref_transaction_commit(transaction, &err))
goto cleanup;
@@ -2468,7 +2469,7 @@ static enum ref_transaction_error split_head_update(struct ref_update *update,
new_update = ref_transaction_add_update(
transaction, "HEAD",
update->flags | REF_LOG_ONLY | REF_NO_DEREF | REF_LOG_VIA_SPLIT,
- &update->new_oid, &update->old_oid,
+ &update->new_oid, &update->old_oid, &update->peeled,
NULL, NULL, update->committer_info, update->msg);
new_update->parent_update = update;
@@ -2530,8 +2531,8 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,
transaction, referent, new_flags,
update->new_target ? NULL : &update->new_oid,
update->old_target ? NULL : &update->old_oid,
- update->new_target, update->old_target, NULL,
- update->msg);
+ &update->peeled, update->new_target, update->old_target,
+ NULL, update->msg);
new_update->parent_update = update;
@@ -2994,7 +2995,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
ref_transaction_add_update(
packed_transaction, update->refname,
REF_HAVE_NEW | REF_NO_DEREF,
- &update->new_oid, NULL,
+ &update->new_oid, NULL, NULL,
NULL, NULL, NULL, NULL);
}
}
@@ -3200,19 +3201,22 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
if (update->flags & REF_LOG_ONLY)
ref_transaction_add_update(loose_transaction, update->refname,
update->flags, &update->new_oid,
- &update->old_oid, NULL, NULL,
+ &update->old_oid, &update->peeled,
+ NULL, NULL,
update->committer_info, update->msg);
else
ref_transaction_add_update(loose_transaction, update->refname,
update->flags & ~REF_HAVE_OLD,
update->new_target ? NULL : &update->new_oid, NULL,
- update->new_target, NULL, update->committer_info,
+ &update->peeled, update->new_target,
+ NULL, update->committer_info,
NULL);
} else {
ref_transaction_add_update(packed_transaction, update->refname,
update->flags & ~REF_HAVE_OLD,
&update->new_oid, &update->old_oid,
- NULL, NULL, update->committer_info, NULL);
+ &update->peeled, NULL, NULL,
+ update->committer_info, NULL);
}
}
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index d103387ebf..307dcb277b 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -39,6 +39,13 @@ struct ref_transaction;
*/
#define REF_LOG_ONLY (1 << 7)
+/*
+ * The reference contains a peeled object ID. This is used when the
+ * new_oid is pointing to a tag object and the reference backend
+ * wants to also store the peeled value for optimized retrieval.
+ */
+#define REF_HAVE_PEELED (1 << 15)
+
/*
* Return the length of time to retry acquiring a loose reference lock
* before giving up, in milliseconds:
@@ -92,6 +99,12 @@ struct ref_update {
*/
struct object_id old_oid;
+ /*
+ * If the new_oid points to a tag object, set this to the peeled
+ * object ID for optimized retrieval without needed to hit the odb.
+ */
+ struct object_id peeled;
+
/*
* If set, point the reference to this value. This can also be
* used to convert regular references to become symbolic refs.
@@ -169,6 +182,7 @@ struct ref_update *ref_transaction_add_update(
const char *refname, unsigned int flags,
const struct object_id *new_oid,
const struct object_id *old_oid,
+ const struct object_id *peeled,
const char *new_target, const char *old_target,
const char *committer_info,
const char *msg);
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 444b0c24e5..b0c010387d 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1107,8 +1107,8 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
ref_transaction_add_update(
transaction, "HEAD",
u->flags | REF_LOG_ONLY | REF_NO_DEREF,
- &u->new_oid, &u->old_oid, NULL, NULL, NULL,
- u->msg);
+ &u->new_oid, &u->old_oid, &u->peeled, NULL, NULL,
+ NULL, u->msg);
}
ret = reftable_backend_read_ref(be, rewritten_ref,
@@ -1194,7 +1194,7 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
transaction, referent->buf, new_flags,
u->new_target ? NULL : &u->new_oid,
u->old_target ? NULL : &u->old_oid,
- u->new_target, u->old_target,
+ &u->peeled, u->new_target, u->old_target,
u->committer_info, u->msg);
new_update->parent_update = u;
--
2.53.GIT
^ permalink raw reply related
* [PATCH v2 7/9] refs: move object parsing to the generic layer
From: Karthik Nayak @ 2026-04-23 8:40 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, gitster, ps
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-0-ae5a4f146d7d@gmail.com>
Regular reference updates made via reference transactions validate that
the provided object ID exists in the object database, which is done by
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.
Modify 'builtin/receive-pack.c' to now capture the error type so that
the error propagated to the client stays the same. Also remove two of
the tests which validates batch-updates with invalid new_oid.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/receive-pack.c | 22 +++++++++++++---------
refs.c | 18 ++++++++++++++++++
refs/files-backend.c | 28 ++--------------------------
refs/reftable-backend.c | 19 -------------------
4 files changed, 33 insertions(+), 54 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 878aa7f0ed..0fb3d57de8 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1641,8 +1641,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
ret = NULL; /* good */
}
strbuf_release(&err);
- }
- else {
+ } else {
+ enum ref_transaction_error err_type;
struct strbuf err = STRBUF_INIT;
if (shallow_update && si->shallow_ref[cmd->index] &&
update_shallow_ref(cmd, si)) {
@@ -1650,14 +1650,18 @@ static const char *update(struct command *cmd, struct shallow_info *si)
goto out;
}
- if (ref_transaction_update(transaction,
- namespaced_name,
- new_oid, old_oid,
- NULL, NULL,
- 0, "push",
- &err)) {
+ err_type = ref_transaction_update(transaction,
+ namespaced_name,
+ new_oid, old_oid,
+ NULL, NULL,
+ 0, "push",
+ &err);
+ if (err_type) {
rp_error("%s", err.buf);
- ret = "failed to update ref";
+ if (err_type == REF_TRANSACTION_ERROR_GENERIC)
+ ret = "failed to update ref";
+ else
+ ret = ref_transaction_error_msg(err_type);
} else {
ret = NULL; /* good */
}
diff --git a/refs.c b/refs.c
index efa16b739d..662a9e6f9e 100644
--- a/refs.c
+++ b/refs.c
@@ -1416,6 +1416,24 @@ enum ref_transaction_error ref_transaction_update(struct ref_transaction *transa
flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
flags |= (new_target ? REF_HAVE_NEW : 0) | (old_target ? REF_HAVE_OLD : 0);
+ if ((flags & REF_HAVE_NEW) && !new_target && !is_null_oid(new_oid) &&
+ !(flags & REF_SKIP_OID_VERIFICATION) && !(flags & REF_LOG_ONLY)) {
+ struct object *o = parse_object(transaction->ref_store->repo, new_oid);
+
+ if (!o) {
+ strbuf_addf(err,
+ _("trying to write ref '%s' with nonexistent object %s"),
+ refname, oid_to_hex(new_oid));
+ return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
+ }
+
+ if (o->type != OBJ_COMMIT && is_branch(refname)) {
+ strbuf_addf(err, _("trying to write non-commit object %s to branch '%s'"),
+ oid_to_hex(new_oid), refname);
+ return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
+ }
+ }
+
ref_transaction_add_update(transaction, refname, flags,
new_oid, old_oid, new_target,
old_target, NULL, msg);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9b1fa955f8..6da0cb4f1b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -19,7 +19,6 @@
#include "../iterator.h"
#include "../dir-iterator.h"
#include "../lockfile.h"
-#include "../object.h"
#include "../path.h"
#include "../dir.h"
#include "../chdir-notify.h"
@@ -1589,7 +1588,6 @@ static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)
static enum ref_transaction_error write_ref_to_lockfile(struct files_ref_store *refs,
struct ref_lock *lock,
const struct object_id *oid,
- int skip_oid_verification,
struct strbuf *err);
static int commit_ref_update(struct files_ref_store *refs,
struct ref_lock *lock,
@@ -1737,7 +1735,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
}
oidcpy(&lock->old_oid, &orig_oid);
- if (write_ref_to_lockfile(refs, lock, &orig_oid, 0, &err) ||
+ if (write_ref_to_lockfile(refs, lock, &orig_oid, &err) ||
commit_ref_update(refs, lock, &orig_oid, logmsg, 0, &err)) {
error("unable to write current sha1 into %s: %s", newrefname, err.buf);
strbuf_release(&err);
@@ -1755,7 +1753,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
goto rollbacklog;
}
- if (write_ref_to_lockfile(refs, lock, &orig_oid, 0, &err) ||
+ if (write_ref_to_lockfile(refs, lock, &orig_oid, &err) ||
commit_ref_update(refs, lock, &orig_oid, NULL, REF_SKIP_CREATE_REFLOG, &err)) {
error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
strbuf_release(&err);
@@ -1999,32 +1997,11 @@ static int files_log_ref_write(struct files_ref_store *refs,
static enum ref_transaction_error write_ref_to_lockfile(struct files_ref_store *refs,
struct ref_lock *lock,
const struct object_id *oid,
- int skip_oid_verification,
struct strbuf *err)
{
static char term = '\n';
- struct object *o;
int fd;
- if (!skip_oid_verification) {
- o = parse_object(refs->base.repo, oid);
- if (!o) {
- strbuf_addf(
- err,
- "trying to write ref '%s' with nonexistent object %s",
- lock->ref_name, oid_to_hex(oid));
- unlock_ref(lock);
- return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
- }
- if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
- strbuf_addf(
- err,
- "trying to write non-commit object %s to branch '%s'",
- oid_to_hex(oid), lock->ref_name);
- unlock_ref(lock);
- return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
- }
- }
fd = get_lock_file_fd(&lock->lk);
if (write_in_full(fd, oid_to_hex(oid), refs->base.repo->hash_algo->hexsz) < 0 ||
write_in_full(fd, &term, 1) < 0 ||
@@ -2828,7 +2805,6 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
} else {
ret = write_ref_to_lockfile(
refs, lock, &update->new_oid,
- update->flags & REF_SKIP_OID_VERIFICATION,
err);
if (ret) {
char *write_err = strbuf_detach(err, NULL);
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 93374d25c2..444b0c24e5 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1081,25 +1081,6 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
return 0;
}
- /* Verify that the new object ID is valid. */
- if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) &&
- !(u->flags & REF_SKIP_OID_VERIFICATION) &&
- !(u->flags & REF_LOG_ONLY)) {
- struct object *o = parse_object(refs->base.repo, &u->new_oid);
- if (!o) {
- strbuf_addf(err,
- _("trying to write ref '%s' with nonexistent object %s"),
- u->refname, oid_to_hex(&u->new_oid));
- return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
- }
-
- if (o->type != OBJ_COMMIT && is_branch(u->refname)) {
- strbuf_addf(err, _("trying to write non-commit object %s to branch '%s'"),
- oid_to_hex(&u->new_oid), u->refname);
- return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;
- }
- }
-
/*
* When we update the reference that HEAD points to we enqueue
* a second log-only update for HEAD so that its reflog is
--
2.53.GIT
^ permalink raw reply related
* [PATCH v2 6/9] update-ref: handle rejections while adding updates
From: Karthik Nayak @ 2026-04-23 8:40 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, gitster, ps
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-0-ae5a4f146d7d@gmail.com>
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 commit phase of the
transaction.
In the following commits, we'll also extend `ref_transaction_update()`
to reject updates before a transaction is prepared/committed. In
preparation, modify the code in update-ref to also handle non-generic
rejections from `ref_transaction_update()`. This involves propagating
information to each of the commands on whether updates are allowed to be
rejected, and also checking for rejections and only dying for generic
failures.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/update-ref.c | 100 ++++++++++++++++++++++++++++++++++++---------------
1 file changed, 71 insertions(+), 29 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 5259cc7226..99deaac6db 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -25,6 +25,15 @@ static unsigned int default_flags;
static unsigned create_reflog_flag;
static const char *msg;
+struct command_options {
+ /*
+ * Individual updates are allowed to fail without causing
+ * update-ref to exit. This is set when using the
+ * '--batch-updates' flag.
+ */
+ bool allow_update_failures;
+};
+
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
* and append the result to arg. Return a pointer to the terminator.
@@ -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)
{
struct strbuf err = STRBUF_INIT;
char *refname;
struct object_id new_oid, old_oid;
+ enum ref_transaction_error tx_err;
int have_old;
refname = parse_refname(&next);
@@ -289,12 +300,20 @@ 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;
free(refname);
@@ -302,9 +321,11 @@ static void parse_cmd_update(struct ref_transaction *transaction,
}
static void parse_cmd_symref_update(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts)
{
char *refname, *new_target, *old_arg;
+ enum ref_transaction_error tx_err;
char *old_target = NULL;
struct strbuf err = STRBUF_INIT;
struct object_id old_oid;
@@ -341,13 +362,21 @@ 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;
free(refname);
@@ -358,7 +387,8 @@ static void parse_cmd_symref_update(struct ref_transaction *transaction,
}
static void parse_cmd_create(struct ref_transaction *transaction,
- const char *next, const char *end)
+ const char *next, const char *end,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
char *refname;
@@ -387,9 +417,9 @@ static void parse_cmd_create(struct ref_transaction *transaction,
strbuf_release(&err);
}
-
static void parse_cmd_symref_create(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
char *refname, *new_target;
@@ -417,7 +447,8 @@ static void parse_cmd_symref_create(struct ref_transaction *transaction,
}
static void parse_cmd_delete(struct ref_transaction *transaction,
- const char *next, const char *end)
+ const char *next, const char *end,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
char *refname;
@@ -450,9 +481,9 @@ static void parse_cmd_delete(struct ref_transaction *transaction,
strbuf_release(&err);
}
-
static void parse_cmd_symref_delete(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
char *refname, *old_target;
@@ -479,9 +510,9 @@ static void parse_cmd_symref_delete(struct ref_transaction *transaction,
strbuf_release(&err);
}
-
static void parse_cmd_verify(struct ref_transaction *transaction,
- const char *next, const char *end)
+ const char *next, const char *end,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
char *refname;
@@ -508,7 +539,8 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
}
static void parse_cmd_symref_verify(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf err = STRBUF_INIT;
struct object_id old_oid;
@@ -550,7 +582,8 @@ static void report_ok(const char *command)
}
static void parse_cmd_option(struct ref_transaction *transaction UNUSED,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
const char *rest;
if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
@@ -560,7 +593,8 @@ static void parse_cmd_option(struct ref_transaction *transaction UNUSED,
}
static void parse_cmd_start(struct ref_transaction *transaction UNUSED,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
if (*next != line_termination)
die("start: extra input: %s", next);
@@ -568,7 +602,8 @@ static void parse_cmd_start(struct ref_transaction *transaction UNUSED,
}
static void parse_cmd_prepare(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf error = STRBUF_INIT;
if (*next != line_termination)
@@ -579,7 +614,8 @@ static void parse_cmd_prepare(struct ref_transaction *transaction,
}
static void parse_cmd_abort(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf error = STRBUF_INIT;
if (*next != line_termination)
@@ -590,7 +626,8 @@ static void parse_cmd_abort(struct ref_transaction *transaction,
}
static void parse_cmd_commit(struct ref_transaction *transaction,
- const char *next, const char *end UNUSED)
+ const char *next, const char *end UNUSED,
+ struct command_options *opts UNUSED)
{
struct strbuf error = STRBUF_INIT;
if (*next != line_termination)
@@ -618,7 +655,8 @@ enum update_refs_state {
static const struct parse_cmd {
const char *prefix;
- void (*fn)(struct ref_transaction *, const char *, const char *);
+ void (*fn)(struct ref_transaction *, const char *, const char *,
+ struct command_options *);
unsigned args;
enum update_refs_state state;
} command[] = {
@@ -644,6 +682,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)
@@ -721,7 +763,7 @@ static void update_refs_stdin(unsigned int flags)
}
cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,
- input.buf + input.len);
+ input.buf + input.len, &opts);
}
switch (state) {
--
2.53.GIT
^ permalink raw reply related
* [PATCH v2 5/9] update-ref: move `print_rejected_refs()` up
From: Karthik Nayak @ 2026-04-23 8:40 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, gitster, ps
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-0-ae5a4f146d7d@gmail.com>
The `print_rejected_refs()` function is used to print any rejected refs
when using git-updated-ref(1) with the '--batch-updates' option. In the
following commit, we'll need to use this function in another place, so
move the function up to avoid a separate forward declaration.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/update-ref.c | 45 ++++++++++++++++++++++-----------------------
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2d68c40ecb..5259cc7226 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -234,6 +234,28 @@ static int parse_next_oid(const char **next, const char *end,
command, refname);
}
+static void print_rejected_refs(const char *refname,
+ const struct object_id *old_oid,
+ const struct object_id *new_oid,
+ const char *old_target,
+ const char *new_target,
+ enum ref_transaction_error err,
+ const char *details,
+ void *cb_data UNUSED)
+{
+ struct strbuf sb = STRBUF_INIT;
+
+ if (details && *details)
+ error("%s", details);
+
+ strbuf_addf(&sb, "rejected %s %s %s %s\n", refname,
+ new_oid ? oid_to_hex(new_oid) : new_target,
+ old_oid ? oid_to_hex(old_oid) : old_target,
+ ref_transaction_error_msg(err));
+
+ fwrite(sb.buf, sb.len, 1, stdout);
+ strbuf_release(&sb);
+}
/*
* The following five parse_cmd_*() functions parse the corresponding
@@ -567,29 +589,6 @@ static void parse_cmd_abort(struct ref_transaction *transaction,
report_ok("abort");
}
-static void print_rejected_refs(const char *refname,
- const struct object_id *old_oid,
- const struct object_id *new_oid,
- const char *old_target,
- const char *new_target,
- enum ref_transaction_error err,
- const char *details,
- void *cb_data UNUSED)
-{
- struct strbuf sb = STRBUF_INIT;
-
- if (details && *details)
- error("%s", details);
-
- strbuf_addf(&sb, "rejected %s %s %s %s\n", refname,
- new_oid ? oid_to_hex(new_oid) : new_target,
- old_oid ? oid_to_hex(old_oid) : old_target,
- ref_transaction_error_msg(err));
-
- fwrite(sb.buf, sb.len, 1, stdout);
- strbuf_release(&sb);
-}
-
static void parse_cmd_commit(struct ref_transaction *transaction,
const char *next, const char *end UNUSED)
{
--
2.53.GIT
^ permalink raw reply related
* [PATCH v2 4/9] refs: return `ref_transaction_error` from `ref_transaction_update()`
From: Karthik Nayak @ 2026-04-23 8:40 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, gitster, ps
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-0-ae5a4f146d7d@gmail.com>
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 beneficial if the
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.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 20 ++++++++++----------
refs.h | 16 ++++++++--------
2 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/refs.c b/refs.c
index 6b506aeea3..efa16b739d 100644
--- a/refs.c
+++ b/refs.c
@@ -1383,25 +1383,25 @@ static int transaction_refname_valid(const char *refname,
return 1;
}
-int ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const struct object_id *new_oid,
- const struct object_id *old_oid,
- const char *new_target,
- const char *old_target,
- unsigned int flags, const char *msg,
- struct strbuf *err)
+enum ref_transaction_error ref_transaction_update(struct ref_transaction *transaction,
+ const char *refname,
+ const struct object_id *new_oid,
+ const struct object_id *old_oid,
+ const char *new_target,
+ const char *old_target,
+ unsigned int flags, const char *msg,
+ struct strbuf *err)
{
assert(err);
if ((flags & REF_FORCE_CREATE_REFLOG) &&
(flags & REF_SKIP_CREATE_REFLOG)) {
strbuf_addstr(err, _("refusing to force and skip creation of reflog"));
- return -1;
+ return REF_TRANSACTION_ERROR_GENERIC;
}
if (!transaction_refname_valid(refname, new_oid, flags, err))
- return -1;
+ return REF_TRANSACTION_ERROR_GENERIC;
if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
diff --git a/refs.h b/refs.h
index d65de6ab5f..71d5c186d0 100644
--- a/refs.h
+++ b/refs.h
@@ -905,14 +905,14 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
* See the above comment "Reference transaction updates" for more
* information.
*/
-int ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const struct object_id *new_oid,
- const struct object_id *old_oid,
- const char *new_target,
- const char *old_target,
- unsigned int flags, const char *msg,
- struct strbuf *err);
+enum ref_transaction_error ref_transaction_update(struct ref_transaction *transaction,
+ const char *refname,
+ const struct object_id *new_oid,
+ const struct object_id *old_oid,
+ const char *new_target,
+ const char *old_target,
+ unsigned int flags, const char *msg,
+ struct strbuf *err);
/*
* Similar to `ref_transaction_update`, but this function is only for adding
--
2.53.GIT
^ permalink raw reply related
* [PATCH v2 3/9] refs: extract out reflog config to generic layer
From: Karthik Nayak @ 2026-04-23 8:40 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, gitster, ps
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-0-ae5a4f146d7d@gmail.com>
The reference backends need to know when to create reflog entries, this
is dictated by the 'core.logallrefupdates' config. Instead of relying on
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.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 1 +
refs/files-backend.c | 2 +-
refs/refs-internal.h | 6 ++++++
refs/reftable-backend.c | 2 +-
4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 8992dd6ae8..6b506aeea3 100644
--- a/refs.c
+++ b/refs.c
@@ -2297,6 +2297,7 @@ static struct ref_store *ref_store_init(struct repository *repo,
struct ref_store *refs;
struct ref_store_init_options opts = {
.access_flags = flags,
+ .log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo),
};
be = find_ref_storage_backend(format);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 78150ad209..9b1fa955f8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -125,7 +125,7 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
refs->packed_ref_store =
packed_ref_store_init(repo, payload, refs->gitcommondir, opts);
refs->store_flags = opts->access_flags;
- refs->log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo);
+ refs->log_all_ref_updates = opts->log_all_ref_updates;
repo_config_get_bool(repo, "core.prefersymlinkrefs", &refs->prefer_symlink_refs);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index f49b3807bf..d103387ebf 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -392,6 +392,12 @@ struct ref_store;
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;
};
/*
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index ad4ee2627c..93374d25c2 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -386,7 +386,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,
base_ref_store_init(&refs->base, repo, refdir.buf, &refs_be_reftable);
strmap_init(&refs->worktree_backends);
- refs->log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo);
+ refs->log_all_ref_updates = opts->log_all_ref_updates;
refs->store_flags = opts->access_flags;
switch (repo->hash_algo->format_id) {
--
2.53.GIT
^ permalink raw reply related
* [PATCH v2 2/9] refs: introduce `ref_store_init_options`
From: Karthik Nayak @ 2026-04-23 8:40 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, gitster, ps
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-0-ae5a4f146d7d@gmail.com>
Reference backends are initiated via the `init()` function. When
initiating the function, the backend is also provided flags which denote
the access levels of the initiator. Create a new structure
`ref_store_init_options` to house such options and move the access flags
to this structure.
This allows easier extension of providing further options to the
backends. In the following commit, we'll also provide config around
reflog creation to the backends via the same structure.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 6 +++++-
refs/files-backend.c | 8 +++++---
refs/packed-backend.c | 4 ++--
refs/packed-backend.h | 3 ++-
refs/refs-internal.h | 11 ++++++++++-
refs/reftable-backend.c | 4 ++--
6 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index bfcb9c7ac3..8992dd6ae8 100644
--- a/refs.c
+++ b/refs.c
@@ -2295,6 +2295,9 @@ 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 opts = {
+ .access_flags = flags,
+ };
be = find_ref_storage_backend(format);
if (!be)
@@ -2304,7 +2307,8 @@ static struct ref_store *ref_store_init(struct repository *repo,
* TODO Send in a 'struct worktree' instead of a 'gitdir', and
* allow the backend to handle how it wants to deal with worktrees.
*/
- refs = be->init(repo, repo->ref_storage_payload, gitdir, flags);
+ refs = be->init(repo, repo->ref_storage_payload, gitdir, &opts);
+
return refs;
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b3b0c25f84..78150ad209 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -108,7 +108,7 @@ static void clear_loose_ref_cache(struct files_ref_store *refs)
static struct ref_store *files_ref_store_init(struct repository *repo,
const char *payload,
const char *gitdir,
- unsigned int flags)
+ const struct ref_store_init_options *opts)
{
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
@@ -120,11 +120,13 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
&ref_common_dir);
base_ref_store_init(ref_store, repo, refdir.buf, &refs_be_files);
- refs->store_flags = flags;
+
refs->gitcommondir = strbuf_detach(&ref_common_dir, NULL);
refs->packed_ref_store =
- packed_ref_store_init(repo, NULL, refs->gitcommondir, flags);
+ packed_ref_store_init(repo, payload, refs->gitcommondir, opts);
+ refs->store_flags = opts->access_flags;
refs->log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo);
+
repo_config_get_bool(repo, "core.prefersymlinkrefs", &refs->prefer_symlink_refs);
chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 23ed62984b..35a0f32e1c 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -218,14 +218,14 @@ static size_t snapshot_hexsz(const struct snapshot *snapshot)
struct ref_store *packed_ref_store_init(struct repository *repo,
const char *payload UNUSED,
const char *gitdir,
- unsigned int store_flags)
+ const struct ref_store_init_options *opts)
{
struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
struct strbuf sb = STRBUF_INIT;
base_ref_store_init(ref_store, repo, gitdir, &refs_be_packed);
- refs->store_flags = store_flags;
+ refs->store_flags = opts->access_flags;
strbuf_addf(&sb, "%s/packed-refs", gitdir);
refs->path = strbuf_detach(&sb, NULL);
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 2c2377a356..1db48e801d 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -3,6 +3,7 @@
struct repository;
struct ref_transaction;
+struct ref_store_init_options;
/*
* Support for storing references in a `packed-refs` file.
@@ -16,7 +17,7 @@ struct ref_transaction;
struct ref_store *packed_ref_store_init(struct repository *repo,
const char *payload,
const char *gitdir,
- unsigned int store_flags);
+ const struct ref_store_init_options *options);
/*
* Lock the packed-refs file for writing. Flags is passed to
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 2d963cc4f4..f49b3807bf 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -385,6 +385,15 @@ 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;
+};
+
/*
* Initialize the ref_store for the specified gitdir. These functions
* should call base_ref_store_init() to initialize the shared part of
@@ -393,7 +402,7 @@ struct ref_store;
typedef struct ref_store *ref_store_init_fn(struct repository *repo,
const char *payload,
const char *gitdir,
- unsigned int flags);
+ const struct ref_store_init_options *opts);
/*
* Release all memory and resources associated with the ref store.
*/
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index daea30a5b4..ad4ee2627c 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -369,7 +369,7 @@ static int reftable_be_config(const char *var, const char *value,
static struct ref_store *reftable_be_init(struct repository *repo,
const char *payload,
const char *gitdir,
- unsigned int store_flags)
+ const struct ref_store_init_options *opts)
{
struct reftable_ref_store *refs = xcalloc(1, sizeof(*refs));
struct strbuf ref_common_dir = STRBUF_INIT;
@@ -386,8 +386,8 @@ static struct ref_store *reftable_be_init(struct repository *repo,
base_ref_store_init(&refs->base, repo, refdir.buf, &refs_be_reftable);
strmap_init(&refs->worktree_backends);
- refs->store_flags = store_flags;
refs->log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo);
+ refs->store_flags = opts->access_flags;
switch (repo->hash_algo->format_id) {
case GIT_SHA1_FORMAT_ID:
--
2.53.GIT
^ permalink raw reply related
* [PATCH v2 1/9] refs: remove unused typedef 'ref_transaction_commit_fn'
From: Karthik Nayak @ 2026-04-23 8:40 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, gitster, ps
In-Reply-To: <20260423-refs-move-to-generic-layer-v2-0-ae5a4f146d7d@gmail.com>
The typedef 'ref_transaction_commit_fn' is not used anywhere in our
code, let's remove it.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/refs-internal.h | 4 ----
1 file changed, 4 deletions(-)
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);
--
2.53.GIT
^ permalink raw reply related
* [PATCH v2 0/9] refs: move some of the generic logic out of the backends
From: Karthik Nayak @ 2026-04-23 8:40 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, gitster, ps
In-Reply-To: <20260420-refs-move-to-generic-layer-v1-0-513e354f376b@gmail.com>
This series came together while I was working on other reference related
code and realized that some of the individual logic implemented with the
reference backends can be moved to the generic layer.
Moving code to the generic layer, simplifies the responsibility of
individual backends and avoids deviation in logic between the backends.
The biggest changes are related to moving out usage of `parse_object()`
and `peel_object()` from reference transactions. The former is used to
validate that the OID provided points to a commit object. The latter is
an optimization technique where the packed/reftable backend store the
peeled OID whenever available, so reading such references provides the
peeled OID without having to call the ODB.
Moving object parsing to the generic layout involves moving it out of
the prepare stage of the transaction and into `ref_transaction_update()`
where every added update is checked. As such, this also involves
modifying update-ref(1) and receive-pack(1) to follow this paradigm.
---
Changes in v2:
- Split the second commit into two: one introducing
`ref_store_init_options` and the second to use it for reflog config.
- Use opts as the variable name consistently.
- A bunch of grammar fixes.
- Link to v1: https://patch.msgid.link/20260420-refs-move-to-generic-layer-v1-0-513e354f376b@gmail.com
---
builtin/receive-pack.c | 22 +++++---
builtin/update-ref.c | 145 +++++++++++++++++++++++++++++++-----------------
refs.c | 60 +++++++++++++++-----
refs.h | 16 +++---
refs/files-backend.c | 58 +++++++------------
refs/packed-backend.c | 10 ++--
refs/packed-backend.h | 3 +-
refs/refs-internal.h | 35 ++++++++++--
refs/reftable-backend.c | 40 +++----------
9 files changed, 225 insertions(+), 164 deletions(-)
Karthik Nayak (9):
refs: remove unused typedef 'ref_transaction_commit_fn'
refs: introduce `ref_store_init_options`
refs: extract out reflog config to generic layer
refs: return `ref_transaction_error` from `ref_transaction_update()`
update-ref: move `print_rejected_refs()` up
update-ref: handle rejections while adding updates
refs: move object parsing to the generic layer
refs: add peeled object ID to the `ref_update` struct
refs: use peeled tag values in reference backends
Range-diff versus v1:
1: bbcc7bff38 = 1: 807f23ee66 refs: remove unused typedef 'ref_transaction_commit_fn'
2: 57a66ae8d5 ! 2: f63583d8b0 refs: extract out reflog config to generic layer
@@ Metadata
Author: Karthik Nayak <karthik.188@gmail.com>
## Commit message ##
- refs: extract out reflog config to generic layer
+ refs: introduce `ref_store_init_options`
- The reference backends need to know when to create reflog entries, this
- is dictated by the 'core.logallrefupdates' config. Instead of relying on
- 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.
+ Reference backends are initiated via the `init()` function. When
+ initiating the function, the backend is also provided flags which denote
+ the access levels of the initiator. Create a new structure
+ `ref_store_init_options` to house such options and move the access flags
+ to this structure.
- 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.
+ This allows easier extension of providing further options to the
+ backends. In the following commit, we'll also provide config around
+ reflog creation to the backends via the same structure.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
@@ refs.c: 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 = {
++ struct ref_store_init_options opts = {
+ .access_flags = flags,
-+ .log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo),
+ };
be = find_ref_storage_backend(format);
@@ refs.c: static struct ref_store *ref_store_init(struct repository *repo,
* allow the backend to handle how it wants to deal with worktrees.
*/
- refs = be->init(repo, repo->ref_storage_payload, gitdir, flags);
-+ refs = be->init(repo, repo->ref_storage_payload, gitdir, &options);
++ refs = be->init(repo, repo->ref_storage_payload, gitdir, &opts);
+
return refs;
}
@@ refs/files-backend.c: static void clear_loose_ref_cache(struct files_ref_store *
const char *payload,
const char *gitdir,
- unsigned int flags)
-+ const struct ref_store_init_options *options)
++ const struct ref_store_init_options *opts)
{
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
@@ refs/files-backend.c: static struct ref_store *files_ref_store_init(struct repos
refs->gitcommondir = strbuf_detach(&ref_common_dir, NULL);
refs->packed_ref_store =
- packed_ref_store_init(repo, NULL, refs->gitcommondir, flags);
-- refs->log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo);
-+ packed_ref_store_init(repo, payload, refs->gitcommondir, options);
-+ refs->store_flags = options->access_flags;
-+ refs->log_all_ref_updates = options->log_all_ref_updates;
++ packed_ref_store_init(repo, payload, refs->gitcommondir, opts);
++ refs->store_flags = opts->access_flags;
+ refs->log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo);
+
repo_config_get_bool(repo, "core.prefersymlinkrefs", &refs->prefer_symlink_refs);
@@ refs/packed-backend.c: static size_t snapshot_hexsz(const struct snapshot *snaps
const char *payload UNUSED,
const char *gitdir,
- unsigned int store_flags)
-+ const struct ref_store_init_options *options)
++ const struct ref_store_init_options *opts)
{
struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
@@ refs/packed-backend.c: static size_t snapshot_hexsz(const struct snapshot *snaps
base_ref_store_init(ref_store, repo, gitdir, &refs_be_packed);
- refs->store_flags = store_flags;
-+ refs->store_flags = options->access_flags;
++ refs->store_flags = opts->access_flags;
strbuf_addf(&sb, "%s/packed-refs", gitdir);
refs->path = strbuf_detach(&sb, NULL);
@@ refs/refs-internal.h: struct ref_store;
+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;
+};
+
/*
@@ refs/refs-internal.h: struct ref_store;
const char *payload,
const char *gitdir,
- unsigned int flags);
-+ const struct ref_store_init_options *options);
++ const struct ref_store_init_options *opts);
/*
* Release all memory and resources associated with the ref store.
*/
@@ refs/reftable-backend.c: static int reftable_be_config(const char *var, const ch
const char *payload,
const char *gitdir,
- unsigned int store_flags)
-+ const struct ref_store_init_options *options)
++ const struct ref_store_init_options *opts)
{
struct reftable_ref_store *refs = xcalloc(1, sizeof(*refs));
struct strbuf ref_common_dir = STRBUF_INIT;
@@ refs/reftable-backend.c: static struct ref_store *reftable_be_init(struct reposi
base_ref_store_init(&refs->base, repo, refdir.buf, &refs_be_reftable);
strmap_init(&refs->worktree_backends);
- refs->store_flags = store_flags;
-- refs->log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo);
-+ refs->store_flags = options->access_flags;
-+ refs->log_all_ref_updates = options->log_all_ref_updates;
+ refs->log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo);
++ refs->store_flags = opts->access_flags;
switch (repo->hash_algo->format_id) {
case GIT_SHA1_FORMAT_ID:
-: ---------- > 3: 15fd026e85 refs: extract out reflog config to generic layer
3: 1c05642914 ! 4: a389011125 refs: return `ref_transaction_error` from `ref_transaction_update()`
@@ Commit message
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
+ validation to this function. As such, it would be beneficial if the
function returns specific error types, so callers can differentiate
between different errors.
4: 6d9544a834 ! 5: 3a203ace83 update-ref: move `print_rejected_refs()` up
@@ Metadata
## Commit message ##
update-ref: move `print_rejected_refs()` up
- The `print_rejected_refs()` is used to print any rejected refs when
- using git-updated-ref(1) with the '--batch-updates' option. In the
- following commit, we'll need to use this function in other places, so
+ The `print_rejected_refs()` function is used to print any rejected refs
+ when using git-updated-ref(1) with the '--batch-updates' option. In the
+ following commit, we'll need to use this function in another place, so
move the function up to avoid a separate forward declaration.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
5: fd3b66ec45 ! 6: e6b2c7b523 update-ref: handle rejections while adding updates
@@ Commit message
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
+ updates are applied. This only applies during the commit phase of the
transaction.
In the following commits, we'll also extend `ref_transaction_update()`
- to reject updates before even a transaction is prepared/committed. In
+ to reject updates before a transaction is prepared/committed. In
preparation, modify the code in update-ref to also handle non-generic
rejections from `ref_transaction_update()`. This involves propagating
information to each of the commands on whether updates are allowed to be
@@ builtin/update-ref.c: static void parse_cmd_update(struct ref_transaction *trans
+ msg, &err);
+
+ if (tx_err && tx_err != REF_TRANSACTION_ERROR_GENERIC &&
-+ opts->allow_update_failures)
++ 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)
++ } else if (tx_err) {
die("%s", err.buf);
++ }
update_flags = default_flags;
+ free(refname);
@@ builtin/update-ref.c: static void parse_cmd_update(struct ref_transaction *transaction,
}
@@ builtin/update-ref.c: static void parse_cmd_symref_update(struct ref_transaction
+ msg, &err);
+
+ if (tx_err && tx_err != REF_TRANSACTION_ERROR_GENERIC &&
-+ opts->allow_update_failures)
++ 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)
++ } else if (tx_err) {
die("%s", err.buf);
++ }
update_flags = default_flags;
+ free(refname);
@@ builtin/update-ref.c: static void parse_cmd_symref_update(struct ref_transaction *transaction,
}
6: 3be9566bf9 ! 7: 36a6284ac1 refs: move object parsing to the generic layer
@@ Commit message
refs: move object parsing to the generic layer
Regular reference updates made via reference transactions validate that
- the provided object ID exists in the object database, this is done by
+ the provided object ID exists in the object database, which is done by
calling 'parse_object()'. This check is done independently by the
- backends.
+ 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.
7: 5e6a26567a = 8: 32f3bc52f1 refs: add peeled object ID to the `ref_update` struct
8: 27914d0556 = 9: bbcdbd3d18 refs: use peeled tag values in reference backends
base-commit: f65aba1e87db64413b6d1ed5ae5a45b5a84a0997
change-id: 20260417-refs-move-to-generic-layer-f7525c5e8764
Thanks
- Karthik
^ permalink raw reply
* Re: [PATCH 0/2] builtin/history: introduce "fixup" subcommand
From: Patrick Steinhardt @ 2026-04-23 6:55 UTC (permalink / raw)
To: Tian Yuchen; +Cc: git, Elijah Newren
In-Reply-To: <d2b19306-71e9-4e17-a0c0-83309a00bd45@malon.dev>
On Thu, Apr 23, 2026 at 02:18:16AM +0800, Tian Yuchen wrote:
> Hi Patrick,
>
> On 4/22/26 18:28, Patrick Steinhardt wrote:
>
> > 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.
>
> Thank you for developing this feature. Godsend for lazy people like me ;)
>
> Nevertheless, I seem to have come across what appears to be a bug. I carried
> out the following steps:
>
> create a.txt -> git add -> git commit -m "base" ->
>
> create b.txt -> git add -> git commit -m "feature" ->
>
> create c.txt -> git add -> git commit -m "tip" ->
>
> rm b.txt -> git add ->
>
> git history fixup HEAD~ ->
>
> git log --oneline --stat...
>
> And the output looks like:
>
> 3096a65 (HEAD -> master) tip
> c.txt | 1 +
> 1 file changed, 1 insertion(+)
> 699f610 feature
> 0be07e6 base
> a.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> More specifically, the output of
>
> git show HEAD~
>
> is:
>
> Author: Tian Yuchen <cat@malon.dev>
> Date: Thu Apr 23 01:57:17 2026 +0800
>
> feature
>
> which is an empty commit. Is it what we expect to see? Sorry that I don't
> have enough time to look at the code in detail :P
I guess the answer is "maybe". I think it would most sense if we had the
equivalent of `--empty=(drop|keep|stop)` that we also have in
git-rebase(1).
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH 2/2] builtin/history: introduce "fixup" subcommand
From: Patrick Steinhardt @ 2026-04-23 6:55 UTC (permalink / raw)
To: D. Ben Knoble; +Cc: git, Elijah Newren
In-Reply-To: <CALnO6CCBA=OSvKT8D6-YR1S=x3VOa_MpzWfK6FJWPSXq0ysMPg@mail.gmail.com>
On Wed, Apr 22, 2026 at 03:06:12PM -0400, D. Ben Knoble wrote:
> On Wed, Apr 22, 2026 at 6:30 AM Patrick Steinhardt <ps@pks.im> wrote:
> > 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
> > @@ -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].
>
> I'm not quite sure what, as a user of "git history fixup," I'm
> supposed to take from this. Does it make conflicts less likely when
> creating the new fixup? I imagine it doesn't help with conflicts
> between <commit> and HEAD that newly arise.
>
> Anyway, I'd think the mechanics are less relevant than the end-user
> behavior at this point in the doc, unless the equivalence with
> cherry-pick is supposed to tell me something about that behavior.
There's at least two more or less obvious variants to do this:
- You generate the diff between HEAD and index and then try to reapply
the patch on top of the target commit.
- You perform the three-way merge.
The second item is definitely more robust compared to generating the
diff and reapplying it, and we use the exact same strategy to perform
cherry-picks nowadays.
> > diff --git a/builtin/history.c b/builtin/history.c
> > index 549e352c74..6299f0dfa9 100644
> > --- a/builtin/history.c
> > +++ b/builtin/history.c
[snip]
> > + /*
> > + * 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.
> > + */
>
> OTOH, this explanation helps quite a bit here :)
Hm, okay. I felt that this explanation here is even more technical. How
about:
`fixup <commit>`::
Apply the currently staged changes to the specified commit. This
is done by performing a three-way merge between the HEAD commit,
the target commit and the tree generated from staged changes.
This is using the same logic as linkgit:git-cherry-pick[1].
Not sure that this is an improvement? Happy to hear other suggestions.
Thanks!
Patrick
^ permalink raw reply
* Subscribe
From: ijose90 @ 2026-04-23 2:01 UTC (permalink / raw)
To: git
Sent from my iPhone
^ permalink raw reply
* [PATCH v6 09/10] parseopt: add tests for subcommand autocorrection
From: Jiamu Sun @ 2026-04-23 1:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Aaron Plattner, Karthik Nayak, Jiamu Sun
In-Reply-To: <SY0P300MB0801677A2A1E0FD38D06A841CE2A2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
These tests cover default behavior (help.autocorrect is unset), no
correction, immediate correction, delayed correction, and rejection
when the typo is too dissimilar.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
t/meson.build | 1 +
t/t9004-autocorrect-subcommand.sh | 58 +++++++++++++++++++++++++++++++
2 files changed, 59 insertions(+)
create mode 100755 t/t9004-autocorrect-subcommand.sh
diff --git a/t/meson.build b/t/meson.build
index 7528e5cda5fe..871cd7ab0a08 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -980,6 +980,7 @@ integration_tests = [
't9001-send-email.sh',
't9002-column.sh',
't9003-help-autocorrect.sh',
+ 't9004-autocorrect-subcommand.sh',
't9100-git-svn-basic.sh',
't9101-git-svn-props.sh',
't9102-git-svn-deep-rmdir.sh',
diff --git a/t/t9004-autocorrect-subcommand.sh b/t/t9004-autocorrect-subcommand.sh
new file mode 100755
index 000000000000..09d281bd105b
--- /dev/null
+++ b/t/t9004-autocorrect-subcommand.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='subcommand auto-correction test
+
+Test autocorrection for subcommands with different
+help.autocorrect mode.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' "
+ echo '^error: unknown subcommand: ' >grep_unknown
+"
+
+test_expect_success 'default is show hint only' '
+ test_must_fail git worktree lsit 2>actual &&
+ test_grep "most similar subcommand" actual
+'
+
+test_expect_success "'never' disables autocorrection" '
+ test_config help.autocorrect never &&
+
+ test_must_fail git worktree lsit 2>actual &&
+ head -n1 actual >first && test_grep -f grep_unknown first
+'
+
+for mode in false no off 0 show
+do
+ test_expect_success "'$mode' disables autocorrection and shows hints" "
+ test_config help.autocorrect $mode &&
+
+ test_must_fail git worktree lsit 2>actual &&
+ test_grep 'most similar subcommand' actual
+ "
+done
+
+for mode in -39 immediate 1
+do
+ test_expect_success "autocorrect immediately with '$mode'" - <<-EOT
+ test_config help.autocorrect $mode &&
+
+ git worktree lsit 2>actual &&
+ test_grep "you meant 'list'\.$" actual
+ EOT
+done
+
+test_expect_success 'delay path is executed' - <<-\EOT
+ test_config help.autocorrect 2 &&
+
+ git worktree lsit 2>actual &&
+ test_grep '^Continuing in 0.2 seconds, ' actual
+EOT
+
+test_expect_success 'deny if too dissimilar' - <<-\EOT
+ test_must_fail git remote rensnr 2>actual &&
+ head -n1 actual >first && test_grep -f grep_unknown first
+EOT
+
+test_done
--
2.54.0
^ permalink raw reply related
* [PATCH v6 08/10] parseopt: enable subcommand autocorrection for git-remote and git-notes
From: Jiamu Sun @ 2026-04-23 1:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Aaron Plattner, Karthik Nayak, Jiamu Sun
In-Reply-To: <SY0P300MB0801677A2A1E0FD38D06A841CE2A2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Add PARSE_OPT_SUBCOMMAND_AUTOCORRECT to enable autocorrection for
subcommands parsed with PARSE_OPT_SUBCOMMAND_OPTIONAL.
Apply this to git-remote and git-notes, so mistyped subcommands can be
automatically corrected, and builtin entry points no longer need to
handle the unknown subcommand error path themselves.
This is safe. Both builtins either resolve to a single subcommand or
take no subcommand at all, meaning any unknown argument encountered by
the parser must be a mistyped subcommand.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
builtin/notes.c | 10 +++-------
builtin/remote.c | 12 ++++--------
parse-options.c | 16 +++++++++-------
parse-options.h | 1 +
4 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 9af602bdd7b4..f9bf350df4e8 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -1149,14 +1149,10 @@ int cmd_notes(int argc,
repo_config(the_repository, git_default_config, NULL);
argc = parse_options(argc, argv, prefix, options, git_notes_usage,
- PARSE_OPT_SUBCOMMAND_OPTIONAL);
- if (!fn) {
- if (argc) {
- error(_("unknown subcommand: `%s'"), argv[0]);
- usage_with_options(git_notes_usage, options);
- }
+ PARSE_OPT_SUBCOMMAND_OPTIONAL |
+ PARSE_OPT_SUBCOMMAND_AUTOCORRECT);
+ if (!fn)
fn = list;
- }
if (override_notes_ref) {
struct strbuf sb = STRBUF_INIT;
diff --git a/builtin/remote.c b/builtin/remote.c
index de989ea3ba96..6a78ab8f4cd2 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1953,15 +1953,11 @@ int cmd_remote(int argc,
};
argc = parse_options(argc, argv, prefix, options, builtin_remote_usage,
- PARSE_OPT_SUBCOMMAND_OPTIONAL);
+ PARSE_OPT_SUBCOMMAND_OPTIONAL |
+ PARSE_OPT_SUBCOMMAND_AUTOCORRECT);
- if (fn) {
+ if (fn)
return !!fn(argc, argv, prefix, repo);
- } else {
- if (argc) {
- error(_("unknown subcommand: `%s'"), argv[0]);
- usage_with_options(builtin_remote_usage, options);
- }
+ else
return !!show_all();
- }
}
diff --git a/parse-options.c b/parse-options.c
index 4370d9c623e4..d60e7bd3c977 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -720,14 +720,16 @@ static enum parse_opt_result handle_subcommand(struct parse_opt_ctx_t *ctx,
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)
+ if (ctx->flags & PARSE_OPT_SUBCOMMAND_OPTIONAL &&
+ !(ctx->flags & PARSE_OPT_SUBCOMMAND_AUTOCORRECT)) {
+ /*
+ * 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;
+ }
find_subcommands(&cmds, options);
assumed = autocorrect_subcommand(arg, &cmds);
diff --git a/parse-options.h b/parse-options.h
index 706de9729f6b..e5fd4da4055b 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -40,6 +40,7 @@ enum parse_opt_flags {
PARSE_OPT_ONE_SHOT = 1 << 5,
PARSE_OPT_SHELL_EVAL = 1 << 6,
PARSE_OPT_SUBCOMMAND_OPTIONAL = 1 << 7,
+ PARSE_OPT_SUBCOMMAND_AUTOCORRECT = 1 << 8,
};
enum parse_opt_option_flags {
--
2.54.0
^ permalink raw reply related
* [PATCH v6 07/10] parseopt: autocorrect mistyped subcommands
From: Jiamu Sun @ 2026-04-23 1:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Aaron Plattner, Karthik Nayak, Jiamu Sun
In-Reply-To: <SY0P300MB0801677A2A1E0FD38D06A841CE2A2@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 +++++++++++++++++++++++++++++++++++++--
t/t0040-parse-options.sh | 5 +-
t/t7900-maintenance.sh | 4 +-
5 files changed, 112 insertions(+), 16 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..4370d9c623e4 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(129);
+ }
+
+ 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)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ca55ea8228c3..2a2fef1e17dc 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -632,8 +632,9 @@ test_expect_success 'subcommand - unknown subcommand shows error and usage' '
test_expect_success 'subcommand - subcommands cannot be abbreviated' '
test_expect_code 129 test-tool parse-subcommand cmd subcmd-o 2>err &&
- grep "^error: unknown subcommand: \`subcmd-o$SQ$" err &&
- grep ^usage: err
+ grep "^The most similar subcommands are$" err &&
+ grep "subcmd-one$" err &&
+ grep "subcmd-two$" err
'
test_expect_success 'subcommand - no negated subcommands' '
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4700beacc182..7174b9932829 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -37,8 +37,8 @@ test_systemd_analyze_verify () {
test_expect_success 'help text' '
test_expect_code 129 git maintenance -h >actual &&
test_grep "usage: git maintenance <subcommand>" actual &&
- test_expect_code 129 git maintenance barf 2>err &&
- test_grep "unknown subcommand: \`barf'\''" err &&
+ test_expect_code 129 git maintenance abarf 2>err &&
+ test_grep "unknown subcommand: \`abarf'\''" err &&
test_grep "usage: git maintenance" err &&
test_expect_code 129 git maintenance 2>err &&
test_grep "error: need a subcommand" err &&
--
2.54.0
^ permalink raw reply related
* [PATCH v6 05/10] autocorrect: rename AUTOCORRECT_SHOW to AUTOCORRECT_HINT
From: Jiamu Sun @ 2026-04-23 1:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Aaron Plattner, Karthik Nayak, Jiamu Sun
In-Reply-To: <SY0P300MB0801677A2A1E0FD38D06A841CE2A2@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.54.0
^ permalink raw reply related
* [PATCH v6 10/10] doc: document autocorrect API
From: Jiamu Sun @ 2026-04-23 1:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Aaron Plattner, Karthik Nayak, Jiamu Sun
In-Reply-To: <SY0P300MB0801677A2A1E0FD38D06A841CE2A2@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
Explain behaviors for autocorrect_resolve(), autocorrect_confirm(), and
struct autocorrect.
Signed-off-by: Jiamu Sun <39@barroit.sh>
---
autocorrect.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/autocorrect.h b/autocorrect.h
index 14ee7c4548d3..5bb67cf6debd 100644
--- a/autocorrect.h
+++ b/autocorrect.h
@@ -13,13 +13,24 @@ enum autocorrect_mode {
AUTOCORRECT_DELAY,
};
+/**
+ * `mode` indicates which action will be performed by autocorrect_confirm().
+ * `delay` is the timeout before autocorrect_confirm() returns, in tenths of a
+ * second. Use it only with AUTOCORRECT_DELAY.
+ */
struct autocorrect {
enum autocorrect_mode mode;
int delay;
};
+/**
+ * Resolve the autocorrect configuration into `conf`.
+ */
void autocorrect_resolve(struct autocorrect *conf);
+/**
+ * Interact with the user in different ways depending on `conf->mode`.
+ */
void autocorrect_confirm(struct autocorrect *conf, const char *assumed);
#endif /* AUTOCORRECT_H */
--
2.54.0
^ permalink raw reply related
* [PATCH v6 06/10] autocorrect: provide config resolution API
From: Jiamu Sun @ 2026-04-23 1:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Aaron Plattner, Karthik Nayak, Jiamu Sun
In-Reply-To: <SY0P300MB0801677A2A1E0FD38D06A841CE2A2@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.54.0
^ permalink raw reply related
* [PATCH v6 04/10] autocorrect: use mode and delay instead of magic numbers
From: Jiamu Sun @ 2026-04-23 1:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Aaron Plattner, Karthik Nayak, Jiamu Sun
In-Reply-To: <SY0P300MB0801677A2A1E0FD38D06A841CE2A2@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.54.0
^ permalink raw reply related
* [PATCH v6 03/10] help: move tty check for autocorrection to autocorrect.c
From: Jiamu Sun @ 2026-04-23 1:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Aaron Plattner, Karthik Nayak, Jiamu Sun
In-Reply-To: <SY0P300MB0801677A2A1E0FD38D06A841CE2A2@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.54.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