* Re: [PATCH v3 0/2] Silence po catalog output under "make -s"
From: Johannes Sixt @ 2026-06-23 16:23 UTC (permalink / raw)
To: Harald Nordgren; +Cc: git, Harald Nordgren via GitGitGadget
In-Reply-To: <pull.2339.v3.git.git.1782053803.gitgitgadget@gmail.com>
Am 21.06.26 um 16:56 schrieb Harald Nordgren via GitGitGadget:
> * gitk: gate the quiet helpers on -s in MAKEFLAGS and give the catalog rule
> a QUIET_MSGFMT prefix, so a silent build emits no MSGFMT/GEN lines
I've picked up this one.
> * git-gui: replace the QUIET_MSGFMT0/QUIET_MSGFMT1 pair with a single
> QUIET_MSGFMT, since with --statistics gone there is no output left to
> reformat
But this one, I skipped, because I already have all of it in
https://github.com/j6t/git-gui/commits/hn/silence-make-s/
Thanks!
-- Hannes
^ permalink raw reply
* Re: [PATCH v2] help: include arguments in autocorrect=prompt message
From: Junio C Hamano @ 2026-06-23 16:09 UTC (permalink / raw)
To: Jishnu C K; +Cc: git, Justin Tobler
In-Reply-To: <6a357689.0f9b68c4.317a5d.1919@mx.google.com>
Jishnu C K <jishnuck26@gmail.com> writes:
> v2: Reworked as an incremental improvement to the existing
> autocorrect=prompt code path rather than a parallel reimplementation,
> per feedback from Junio and Justin.
>
> ---
> From a4e8fb6fd6dd6a501e565c7500cbf927d7cb0b42 Mon Sep 17 00:00:00 2001
> From: calicomills <jishnuck26@gmail.com>
> Date: Fri, 19 Jun 2026 13:01:40 +0530
> Subject: [PATCH v2 v2] help: include arguments in autocorrect=prompt message
To learn what a typical v2 of a single-patch topic should look like,
see
https://lore.kernel.org/git/aipTOsH8LKTSwglj@collabora.com/
for an example.
- Having auxiliary comments explaining why there is this v2,
including description of the difference since v1, is good, but
have it below the three-dash line after your sign off, not at the
beginning.
- Please do not include "From a4e8fb6f..." line, which is meant as
a separator in multi-patch output from the git format-patch
command; knowing the exact commit object name you took the patch
from in your repository would not help anybody.
- Do not include "From:" in the body of the message either, unless
you are relaying somebody else's patch, i.e., when the From
e-mail header (you) does not name the person who wrote the patch
(somebody else).
- Do not include "Date:" in the body of the message either, as that
is the timestamp you wrote the change, but we care more about the
time when the general public first saw the patch, which is in the
e-mail header already.
- Do not include "Subject:" in the body of the message either, as
that should be on the Subject e-mail header.
> When 'help.autocorrect=prompt' is configured and the user mistypes
> a git command, the prompt currently shows only the corrected command
> name:
>
> Run 'checkout' instead [y/N]?
>
> This leaves the user unsure whether their original arguments will be
> preserved. Update the prompt to include the full corrected invocation:
>
> Run 'git checkout neo' instead [y/N]?
>
> The help_unknown_cmd() signature is updated to accept the args vector
> so the prompt can show the original arguments alongside the corrected
> command name. Callers that do not have access to the args (e.g.
> builtin/help.c) pass NULL, which is handled gracefully.
>
> Signed-off-by: calicomills <jishnuck26@gmail.com>
Documentation/SubmittingPatches[[real-name]] prefers to see us
interacting with humans with real-sounding names, not handles.
> ---
> help.c | 49 +++++++++++++----------------------
> t/t9003-help-autocorrect.sh | 51 +++++--------------------------------
> 2 files changed, 23 insertions(+), 77 deletions(-)
>
> diff --git a/help.c b/help.c
> index 30f32a7206..9ea4c076e1 100644
> --- a/help.c
> +++ b/help.c
> @@ -739,7 +739,16 @@ char *help_unknown_cmd(const char *cmd, const struct strvec *args)
> else if (cfg.autocorrect == AUTOCORRECT_PROMPT) {
> char *answer;
> struct strbuf msg = STRBUF_INIT;
> - strbuf_addf(&msg, _("Run '%s' instead [y/N]? "), assumed);
> + struct strbuf full_cmd = STRBUF_INIT;
> + strbuf_addstr(&full_cmd, assumed);
> + if (args) {
> + for (size_t j = 1; j < args->nr; j++) {
> + strbuf_addch(&full_cmd, ' ');
> + strbuf_addstr(&full_cmd, args->v[j]);
> + }
> + }
> + strbuf_addf(&msg, _("Run 'git %s' instead [y/N]? "), full_cmd.buf);
> + strbuf_release(&full_cmd);
This change seems to match what is explained in the proposed log
message. Instead of giving the "assumed" command without its
arguments, the full command line is gprepared to be given in 'msg'.
But if we really wanted to let these be cut-and-paste, don't you
need to shell-quote the command line? If the user typed
$ git comit -m "Hello world"
the above makes
Run 'git commit -m hello world' instead [y/N]?
which would record the change made only to the file "world" with log
message "hello", which is not what the user wanted to do.
> @@ -762,37 +771,13 @@ char *help_unknown_cmd(const char *cmd, const struct strvec *args)
> fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);
>
> if (SIMILAR_ENOUGH(best_similarity)) {
> - if (n == 1 && isatty(0) && isatty(2)) {
> - char *answer;
> - struct strbuf msg = STRBUF_INIT;
> - struct strbuf full_cmd = STRBUF_INIT;
> - strbuf_addstr(&full_cmd, main_cmds.names[0]->name);
> - if (args) {
> - for (size_t j = 1; j < args->nr; j++) {
> - strbuf_addch(&full_cmd, ' ');
> - strbuf_addstr(&full_cmd, args->v[j]);
> - }
> - }
> - strbuf_addf(&msg, _("\nDid you mean 'git %s'? [y/N] "),
> - full_cmd.buf);
> - strbuf_release(&full_cmd);
> - answer = git_prompt(msg.buf, PROMPT_ECHO);
> - strbuf_release(&msg);
> - if (starts_with(answer, "y") || starts_with(answer, "Y")) {
> - char *assumed = xstrdup(main_cmds.names[0]->name);
> - cmdnames_release(&cfg.aliases);
> - cmdnames_release(&main_cmds);
> - cmdnames_release(&other_cmds);
> - return assumed;
> - }
What is this removal about? The original used to give interactive
prompt to let the user say "Yes", but with this part removed, it no
longer offers the "well we have only one candidate, do you want to
run that one?", and you instead ...
> - } else {
> - fprintf_ln(stderr,
> - Q_("\nThe most similar command is",
> - "\nThe most similar commands are",
> - n));
> - for (i = 0; i < n; i++)
> - fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
> - }
... only give "these are the possible candidates?"
> + fprintf_ln(stderr,
> + Q_("\nThe most similar command is",
> + "\nThe most similar commands are",
> + n));
> +
> + for (i = 0; i < n; i++)
> + fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
> }
Puzzled.
Worse, this [v2] does not even apply cleanly to any of the trees we
have.
Aha! Is the removal I see above a mere "oops, this was wrong, so
remove it" done on top of a previous iteration of the patch?
Please do not do that. It would mean we will keep unwanted code
that went into a wrong direction (which is v1) in our history.
The development may wander around in different directions like a
drunken man, changing course every time patches are updated, until
it finally gets right, but the name of the game around here, before
your change is merged to 'next', is to "pretend to be a more perfect
developer than you actually are" ;-). You can pretend that you
never made a design mistake you made in [v1], and instead directly
arrived at a good state from the state of our public tree. That
means [v2] (and any subsequent rerolls, until your seires gets
merged to 'next') must apply directly without any of the older
iterations to 'master' (or if it is a bugfix, 'maint') branch.
Thanks.
^ permalink raw reply
* Re: [GSoC Patch v7 1/3] path: extract append_formatted_path() and use in rev-parse
From: Phillip Wood @ 2026-06-23 15:57 UTC (permalink / raw)
To: K Jayatheerth
Cc: a3205153416, git, gitster, jltobler, kumarayushjha123,
lucasseikioshiro, phillip.wood, sandals
In-Reply-To: <20260621055534.46798-2-jayatheerthkulkarni2005@gmail.com>
On 21/06/2026 06:55, K Jayatheerth wrote:
> Path formatting logic in builtin/rev-parse.c writes directly to
> stdout. Other builtins cannot reuse it.
>
> Extract this logic into append_formatted_path() in path.c and expose
> a path_format enum in path.h.
>
> Convert rev-parse to use the new helper in the same step to validate
> the API against existing tests and avoid introducing dead code.
The new API looks good now, and so does the conversion of the existing
code. I'm very happy with this version and don't have anything to add to
Junio's comments
Thanks
Phillip
> Mentored-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
> builtin/rev-parse.c | 73 ++++++++++++++++++---------------------------
> path.c | 69 ++++++++++++++++++++++++++++++++++++++++++
> path.h | 30 +++++++++++++++++++
> 3 files changed, 128 insertions(+), 44 deletions(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index bb882678fe..6de01466db 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -653,53 +653,38 @@ enum default_type {
> DEFAULT_UNMODIFIED,
> };
>
> -static void print_path(const char *path, const char *prefix, enum format_type format, enum default_type def)
> +static void print_path(const char *path, const char *prefix,
> + enum format_type format, enum default_type def)
> {
> - char *cwd = NULL;
> - /*
> - * We don't ever produce a relative path if prefix is NULL, so set the
> - * prefix to the current directory so that we can produce a relative
> - * path whenever possible. If we're using RELATIVE_IF_SHARED mode, then
> - * we want an absolute path unless the two share a common prefix, so don't
> - * set it in that case, since doing so causes a relative path to always
> - * be produced if possible.
> - */
> - if (!prefix && (format != FORMAT_DEFAULT || def != DEFAULT_RELATIVE_IF_SHARED))
> - prefix = cwd = xgetcwd();
> - if (format == FORMAT_DEFAULT && def == DEFAULT_UNMODIFIED) {
> - puts(path);
> - } else if (format == FORMAT_RELATIVE ||
> - (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE)) {
> - /*
> - * In order for relative_path to work as expected, we need to
> - * make sure that both paths are absolute paths. If we don't,
> - * we can end up with an unexpected absolute path that the user
> - * didn't want.
> - */
> - struct strbuf buf = STRBUF_INIT, realbuf = STRBUF_INIT, prefixbuf = STRBUF_INIT;
> - if (!is_absolute_path(path)) {
> - strbuf_realpath_forgiving(&realbuf, path, 1);
> - path = realbuf.buf;
> - }
> - if (!is_absolute_path(prefix)) {
> - strbuf_realpath_forgiving(&prefixbuf, prefix, 1);
> - prefix = prefixbuf.buf;
> + struct strbuf sb = STRBUF_INIT;
> + enum path_format fmt;
> +
> + if (format == FORMAT_RELATIVE) {
> + fmt = PATH_FORMAT_RELATIVE;
> + } else if (format == FORMAT_CANONICAL) {
> + fmt = PATH_FORMAT_CANONICAL;
> + } else /* FORMAT_DEFAULT */ {
> + switch (def) {
> + case DEFAULT_RELATIVE:
> + fmt = PATH_FORMAT_RELATIVE;
> + break;
> + case DEFAULT_RELATIVE_IF_SHARED:
> + fmt = PATH_FORMAT_RELATIVE_IF_SHARED;
> + break;
> + case DEFAULT_CANONICAL:
> + fmt = PATH_FORMAT_CANONICAL;
> + break;
> + case DEFAULT_UNMODIFIED:
> + default:
> + fmt = PATH_FORMAT_UNMODIFIED;
> + break;
> }
> - puts(relative_path(path, prefix, &buf));
> - strbuf_release(&buf);
> - strbuf_release(&realbuf);
> - strbuf_release(&prefixbuf);
> - } else if (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE_IF_SHARED) {
> - struct strbuf buf = STRBUF_INIT;
> - puts(relative_path(path, prefix, &buf));
> - strbuf_release(&buf);
> - } else {
> - struct strbuf buf = STRBUF_INIT;
> - strbuf_realpath_forgiving(&buf, path, 1);
> - puts(buf.buf);
> - strbuf_release(&buf);
> }
> - free(cwd);
> +
> + append_formatted_path(&sb, path, prefix, fmt);
> + puts(sb.buf);
> +
> + strbuf_release(&sb);
> }
>
> int cmd_rev_parse(int argc,
> diff --git a/path.c b/path.c
> index d7e17bf174..6d8e892ada 100644
> --- a/path.c
> +++ b/path.c
> @@ -1579,6 +1579,75 @@ char *xdg_cache_home(const char *filename)
> return NULL;
> }
>
> +void append_formatted_path(struct strbuf *dest, const char *path,
> + const char *prefix, enum path_format format)
> +{
> + switch (format) {
> + case PATH_FORMAT_UNMODIFIED:
> + strbuf_addstr(dest, path);
> + break;
> +
> + case PATH_FORMAT_RELATIVE: {
> + struct strbuf relative_buf = STRBUF_INIT;
> + struct strbuf real_path = STRBUF_INIT;
> + struct strbuf real_prefix = STRBUF_INIT;
> + char *cwd = NULL;
> +
> + /*
> + * We don't ever produce a relative path if prefix is NULL,
> + * so set the prefix to the current directory so that we can
> + * produce a relative path whenever possible.
> + */
> + if (!prefix)
> + prefix = cwd = xgetcwd();
> +
> + if (!is_absolute_path(path)) {
> + strbuf_realpath_forgiving(&real_path, path, 1);
> + path = real_path.buf;
> + }
> + if (!is_absolute_path(prefix)) {
> + strbuf_realpath_forgiving(&real_prefix, prefix, 1);
> + prefix = real_prefix.buf;
> + }
> +
> + strbuf_addstr(dest, relative_path(path, prefix, &relative_buf));
> +
> + strbuf_release(&relative_buf);
> + strbuf_release(&real_path);
> + strbuf_release(&real_prefix);
> + free(cwd);
> + break;
> + }
> +
> + case PATH_FORMAT_RELATIVE_IF_SHARED: {
> + struct strbuf relative_buf = STRBUF_INIT;
> +
> + /*
> + * If we're using RELATIVE_IF_SHARED mode, then we want an
> + * absolute path unless the two share a common prefix, so don't
> + * default the prefix to the current working directory. Doing so
> + * would cause a relative path to always be produced if possible.
> + */
> + strbuf_addstr(dest, relative_path(path, prefix, &relative_buf));
> + strbuf_release(&relative_buf);
> + break;
> + }
> +
> + case PATH_FORMAT_CANONICAL: {
> + struct strbuf canonical_buf = STRBUF_INIT;
> +
> + strbuf_realpath_forgiving(&canonical_buf, path, 1);
> + strbuf_addbuf(dest, &canonical_buf);
> +
> + strbuf_release(&canonical_buf);
> + break;
> + }
> +
> + default:
> + BUG("unknown path_format value %d", format);
> + }
> +}
> +
> REPO_GIT_PATH_FUNC(squash_msg, "SQUASH_MSG")
> REPO_GIT_PATH_FUNC(merge_msg, "MERGE_MSG")
> REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR")
> diff --git a/path.h b/path.h
> index 4c2958a903..4d982a2c8e 100644
> --- a/path.h
> +++ b/path.h
> @@ -262,6 +262,36 @@ enum scld_error safe_create_leading_directories_no_share(char *path);
> int safe_create_file_with_leading_directories(struct repository *repo,
> const char *path);
>
> +/**
> + * The formatting strategy to apply when writing a path into a buffer.
> + */
> +enum path_format {
> + /* Output the path exactly as-is without any modifications. */
> + PATH_FORMAT_UNMODIFIED,
> +
> + /* Output a path relative to the provided directory prefix. */
> + PATH_FORMAT_RELATIVE,
> +
> + /* Output a relative path only if the path shares a root with the prefix. */
> + PATH_FORMAT_RELATIVE_IF_SHARED,
> +
> + /* Output a fully resolved, absolute canonical path. */
> + PATH_FORMAT_CANONICAL
> +};
> +
> +/**
> + * Format a path according to the specified formatting strategy and append
> + * the result to the given strbuf.
> + *
> + * `dest` : The string buffer to append the formatted path to.
> + * `path` : The path string that needs to be formatted.
> + * `prefix` : The directory prefix to calculate relative offsets against.
> + * Pass NULL to default to the current working directory where applicable.
> + * `format` : The formatting behavior rule to execute.
> + */
> +void append_formatted_path(struct strbuf *dest, const char *path,
> + const char *prefix, enum path_format format);
> +
> # ifdef USE_THE_REPOSITORY_VARIABLE
> # include "strbuf.h"
> # include "repository.h"
^ permalink raw reply
* Re: [PATCH v3 2/2] status: improve rebase todo list parsing
From: Phillip Wood @ 2026-06-23 15:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, Patrick Steinhardt
In-Reply-To: <xmqq8q86s12r.fsf@gitster.g>
On 22/06/2026 22:43, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> + if (!sequencer_parse_todo_command((const char**)&p, &cmd))
>
> Style. Missing SP between "char" and "**".
Fixed in V4
Thanks
Phillip
^ permalink raw reply
* [PATCH v4 2/2] status: improve rebase todo list parsing
From: Phillip Wood @ 2026-06-23 15:53 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Patrick Steinhardt, Junio C Hamano, Phillip Wood
In-Reply-To: <cover.1782230024.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
When there is rebase in progress "git status" displays the last couple
of completed and the next couple of pending commands from the todo
list. When it does this it tries to abbreviate the object ids of
the commits to be picked. Unfortunately it does not abbreviate the
object ids when the line starts with "fixup -C" or "merge -C". It
also mistakenly replaces the refname in "reset main" and "update-ref
refs/heads/main" with the object id that the ref points to.
Fix this by using the function added in the last commit to parse the
command name and only try to abbreviate the argument for commands that
take an object id. If a command accepts a label then try to resolve the
object name as a label first and only if that fails try to resolve it
as an object_id. When trying to abbreviate an object id, only replace
the object name if it starts with the abbreviated object id so that
tag or branch names that contain only hex digits are left unchanged.
Comments are now processed after stripping any leading
whitespace from the line. This matches what the sequencer does in
parse_insn_line(). The existing test cases are updated to test a
wider variety of commands. Only the pending commands in the tests
are changed to avoid removing existing coverage.
Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
t/t7512-status-help.sh | 74 +++++++++++++-------
wt-status.c | 154 +++++++++++++++++++++++++++++++++--------
2 files changed, 175 insertions(+), 53 deletions(-)
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 08e82f79140..aca4b6d3326 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -224,7 +224,7 @@ test_expect_success 'status when splitting a commit' '
COMMIT3=$(git rev-parse --short split_commit) &&
test_commit four_split main.txt four &&
COMMIT4=$(git rev-parse --short split_commit) &&
- FAKE_LINES="1 edit 2 3" &&
+ FAKE_LINES="reword 1 edit 2 fixup_-C 3" &&
export FAKE_LINES &&
test_when_finished "git rebase --abort" &&
ONTO=$(git rev-parse --short HEAD~3) &&
@@ -233,10 +233,10 @@ test_expect_success 'status when splitting a commit' '
cat >expected <<EOF &&
interactive rebase in progress; onto $ONTO
Last commands done (2 commands done):
- pick $COMMIT2 # two_split
+ reword $COMMIT2 # two_split
edit $COMMIT3 # three_split
Next command to do (1 remaining command):
- pick $COMMIT4 # four_split
+ fixup -C $COMMIT4 # four_split
(use "git rebase --edit-todo" to view and edit)
You are currently splitting a commit while rebasing branch '\''split_commit'\'' on '\''$ONTO'\''.
(Once your working directory is clean, run "git rebase --continue")
@@ -297,7 +297,7 @@ test_expect_success 'prepare for several edits' '
test_expect_success 'status: (continue first edit) second edit' '
- FAKE_LINES="edit 1 edit 2 3" &&
+ FAKE_LINES="edit 1 edit 2 drop 3" &&
export FAKE_LINES &&
test_when_finished "git rebase --abort" &&
COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -312,7 +312,7 @@ Last commands done (2 commands done):
edit $COMMIT2 # two_edits
edit $COMMIT3 # three_edits
Next command to do (1 remaining command):
- pick $COMMIT4 # four_edits
+ drop $COMMIT4 # four_edits
(use "git rebase --edit-todo" to view and edit)
You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
(use "git commit --amend" to amend the current commit)
@@ -327,7 +327,7 @@ EOF
test_expect_success 'status: (continue first edit) second edit and split' '
git reset --hard several_edits &&
- FAKE_LINES="edit 1 edit 2 3" &&
+ FAKE_LINES="edit 1 edit 2 squash 3" &&
export FAKE_LINES &&
test_when_finished "git rebase --abort" &&
COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -343,7 +343,7 @@ Last commands done (2 commands done):
edit $COMMIT2 # two_edits
edit $COMMIT3 # three_edits
Next command to do (1 remaining command):
- pick $COMMIT4 # four_edits
+ squash $COMMIT4 # four_edits
(use "git rebase --edit-todo" to view and edit)
You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
(Once your working directory is clean, run "git rebase --continue")
@@ -362,7 +362,7 @@ EOF
test_expect_success 'status: (continue first edit) second edit and amend' '
git reset --hard several_edits &&
- FAKE_LINES="edit 1 edit 2 3" &&
+ FAKE_LINES="edit 1 edit 2 fixup 3" &&
export FAKE_LINES &&
test_when_finished "git rebase --abort" &&
COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -378,7 +378,7 @@ Last commands done (2 commands done):
edit $COMMIT2 # two_edits
edit $COMMIT3 # three_edits
Next command to do (1 remaining command):
- pick $COMMIT4 # four_edits
+ fixup $COMMIT4 # four_edits
(use "git rebase --edit-todo" to view and edit)
You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
(use "git commit --amend" to amend the current commit)
@@ -393,7 +393,7 @@ EOF
test_expect_success 'status: (amend first edit) second edit' '
git reset --hard several_edits &&
- FAKE_LINES="edit 1 edit 2 3" &&
+ FAKE_LINES="edit 1 edit 2 fixup_-c 3" &&
export FAKE_LINES &&
test_when_finished "git rebase --abort" &&
COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -409,7 +409,7 @@ Last commands done (2 commands done):
edit $COMMIT2 # two_edits
edit $COMMIT3 # three_edits
Next command to do (1 remaining command):
- pick $COMMIT4 # four_edits
+ fixup -c $COMMIT4 # four_edits
(use "git rebase --edit-todo" to view and edit)
You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
(use "git commit --amend" to amend the current commit)
@@ -460,14 +460,20 @@ EOF
test_expect_success 'status: (amend first edit) second edit and amend' '
git reset --hard several_edits &&
- FAKE_LINES="edit 1 edit 2 3" &&
- export FAKE_LINES &&
test_when_finished "git rebase --abort" &&
COMMIT2=$(git rev-parse --short several_edits^^) &&
COMMIT3=$(git rev-parse --short several_edits^) &&
COMMIT4=$(git rev-parse --short several_edits) &&
ONTO=$(git rev-parse --short HEAD~3) &&
- git rebase -i HEAD~3 &&
+ cat >todo <<-EOF &&
+ edit several_edits^^ # two_edits
+ edit several_edits^ # three_edits
+ merge $(git rev-parse main) $(git rev-parse several_edits)
+ EOF
+ (
+ set_replace_editor todo &&
+ git rebase -i HEAD~3
+ ) &&
git commit --amend -m "c" &&
git rebase --continue &&
git commit --amend -m "d" &&
@@ -477,7 +483,7 @@ Last commands done (2 commands done):
edit $COMMIT2 # two_edits
edit $COMMIT3 # three_edits
Next command to do (1 remaining command):
- pick $COMMIT4 # four_edits
+ merge $(git rev-parse --short main) $COMMIT4
(use "git rebase --edit-todo" to view and edit)
You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
(use "git commit --amend" to amend the current commit)
@@ -525,14 +531,21 @@ EOF
test_expect_success 'status: (split first edit) second edit and split' '
git reset --hard several_edits &&
- FAKE_LINES="edit 1 edit 2 3" &&
- export FAKE_LINES &&
test_when_finished "git rebase --abort" &&
COMMIT2=$(git rev-parse --short several_edits^^) &&
COMMIT3=$(git rev-parse --short several_edits^) &&
COMMIT4=$(git rev-parse --short several_edits) &&
+ cat >todo <<-EOF &&
+ edit several_edits^^ # two_edits
+ edit several_edits^ # three_edits
+ reset $(git rev-parse main)
+ merge -C several_edits topic # title
+ EOF
ONTO=$(git rev-parse --short HEAD~3) &&
- git rebase -i HEAD~3 &&
+ (
+ set_replace_editor todo &&
+ git rebase -i HEAD~3
+ ) &&
git reset HEAD^ &&
git add main.txt &&
git commit --amend -m "f" &&
@@ -543,8 +556,9 @@ interactive rebase in progress; onto $ONTO
Last commands done (2 commands done):
edit $COMMIT2 # two_edits
edit $COMMIT3 # three_edits
-Next command to do (1 remaining command):
- pick $COMMIT4 # four_edits
+Next commands to do (2 remaining commands):
+ reset $(git rev-parse --short main)
+ merge -C $COMMIT4 topic # title
(use "git rebase --edit-todo" to view and edit)
You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
(Once your working directory is clean, run "git rebase --continue")
@@ -563,14 +577,21 @@ EOF
test_expect_success 'status: (split first edit) second edit and amend' '
git reset --hard several_edits &&
- FAKE_LINES="edit 1 edit 2 3" &&
- export FAKE_LINES &&
test_when_finished "git rebase --abort" &&
+ git branch cafe main &&
COMMIT2=$(git rev-parse --short several_edits^^) &&
COMMIT3=$(git rev-parse --short several_edits^) &&
- COMMIT4=$(git rev-parse --short several_edits) &&
+ cat >todo <<-EOF &&
+ edit several_edits^^ # two_edits
+ edit several_edits^ # three_edits
+ update-ref refs/heads/main
+ reset cafe
+ EOF
ONTO=$(git rev-parse --short HEAD~3) &&
- git rebase -i HEAD~3 &&
+ (
+ set_replace_editor todo &&
+ git rebase -i HEAD~3
+ ) &&
git reset HEAD^ &&
git add main.txt &&
git commit --amend -m "g" &&
@@ -581,8 +602,9 @@ interactive rebase in progress; onto $ONTO
Last commands done (2 commands done):
edit $COMMIT2 # two_edits
edit $COMMIT3 # three_edits
-Next command to do (1 remaining command):
- pick $COMMIT4 # four_edits
+Next commands to do (2 remaining commands):
+ update-ref refs/heads/main
+ reset cafe
(use "git rebase --edit-todo" to view and edit)
You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
(use "git commit --amend" to amend the current commit)
diff --git a/wt-status.c b/wt-status.c
index 479ccc3304b..de36303c526 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1363,6 +1363,71 @@ static int split_commit_in_progress(struct wt_status *s)
free(rebase_orig_head);
return split_in_progress;
+}
+
+/*
+ * If the whitespace-delimited token starting at or just after *pp
+ * is a hex object id that is longer than its default abbreviation,
+ * abbreviate it in-place, shrinking `line` accordingly. On return
+ * *pp points one past the (possibly abbreviated) token. Leaves both
+ * `line` and *pp-advanced-past-the-token unchanged in all other cases
+ * (non-hex token, label name, unresolvable, or a refname that happens
+ * to consist only of hex digits).
+ */
+static void abbrev_oid_in_line(struct repository *r, struct strbuf *scratch,
+ struct strbuf *line, bool maybe_label, char **pp)
+{
+ char *p = *pp;
+ char *end_of_object_name, saved;
+ const char *abbrev;
+ struct object_id oid;
+ bool have_oid;
+
+ p += strspn(p, " \t");
+ end_of_object_name = p + strcspn(p, " \t");
+ /*
+ * For "merge" and "reset" the object name may be a label or
+ * ref rather than a hex object id. Only abbreviate the object
+ * name if it is a hex object id.
+ */
+ for (const char *q = p; q < end_of_object_name; q++) {
+ if (!isxdigit(*q))
+ goto out;
+ }
+ if (maybe_label) {
+ strbuf_reset(scratch);
+ strbuf_addf(scratch, "refs/rewritten/%.*s",
+ (int)(end_of_object_name - p), p);
+ if (refs_ref_exists(get_main_ref_store(r), scratch->buf))
+ goto out; /* object name was a label */
+ }
+ saved = *end_of_object_name;
+ *end_of_object_name = '\0';
+ have_oid = !repo_get_oid(r, p, &oid);
+ *end_of_object_name = saved;
+ if (!have_oid)
+ goto out; /* invalid object name */
+ abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
+ if (!starts_with(p, abbrev))
+ goto out; /* object name was a refname containing only xdigits */
+ p += strlen(abbrev);
+ strbuf_remove(line, p - line->buf, end_of_object_name - p);
+ end_of_object_name = p;
+out:
+ *pp = end_of_object_name;
+}
+
+/* Skip "[ \t]*(-[cC])?", returns true if "-c/-C" was skipped. */
+static bool skip_dash_c(char **pp)
+{
+ bool ret;
+ char *p = *pp;
+
+ p += strspn(p, " \t");
+ ret = skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p);
+ *pp = p;
+
+ return ret;
}
/*
@@ -1371,29 +1436,68 @@ static int split_commit_in_progress(struct wt_status *s)
* into
* "pick d6a2f03 some message"
*
- * The function assumes that the line does not contain useless spaces
- * before or after the command.
+ * Returns false on comment lines, true otherwise
*/
-static void abbrev_oid_in_line(struct repository *r, struct strbuf *line)
+static bool format_todo_line(struct repository *r, struct strbuf *line)
{
- struct string_list split = STRING_LIST_INIT_DUP;
- struct object_id oid;
-
- if (starts_with(line->buf, "exec ") ||
- starts_with(line->buf, "x ") ||
- starts_with(line->buf, "label ") ||
- starts_with(line->buf, "l "))
- return;
-
- if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
- !repo_get_oid(r, split.items[1].string, &oid)) {
- strbuf_reset(line);
- strbuf_addf(line, "%s ", split.items[0].string);
- strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
- for (size_t i = 2; i < split.nr; i++)
- strbuf_addf(line, " %s", split.items[i].string);
- }
- string_list_clear(&split, 0);
+ enum todo_command cmd;
+ struct strbuf scratch = STRBUF_INIT;
+ char *p = line->buf;
+
+ if (!sequencer_parse_todo_command((const char **)&p, &cmd))
+ return true; /* keep invalid lines */
+
+ switch (cmd) {
+ case TODO_COMMENT:
+ return false;
+
+ case TODO_MERGE: {
+ /*
+ * The argument to -C cannot be a label, but the parents
+ * can be labels.
+ */
+ bool maybe_label = !skip_dash_c(&p);
+
+ while (true) {
+ p += strspn(p, " \t");
+ if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
+ break;
+ abbrev_oid_in_line(r, &scratch, line, maybe_label, &p);
+ maybe_label = true;
+ }
+ break;
+ }
+
+ case TODO_FIXUP:
+ skip_dash_c(&p);
+ /* fallthrough */
+ case TODO_DROP:
+ case TODO_EDIT:
+ case TODO_PICK:
+ case TODO_REVERT:
+ case TODO_REWORD:
+ case TODO_SQUASH:
+ abbrev_oid_in_line(r, &scratch, line, false, &p);
+ break;
+
+ case TODO_RESET:
+ abbrev_oid_in_line(r, &scratch, line, true, &p);
+ break;
+ /*
+ * Avoid "default" and instead list all the other commands so
+ * that -Wswitch (which is included in -Wall) warns if a new
+ * command is added without handling it in this function.
+ */
+ case TODO_BREAK:
+ case TODO_EXEC:
+ case TODO_LABEL:
+ case TODO_NOOP:
+ case TODO_UPDATE_REF:
+ break;
+ }
+
+ strbuf_release(&scratch);
+ return true;
}
static int read_rebase_todolist(struct repository *r, const char *fname, struct string_list *lines)
@@ -1411,13 +1515,9 @@ static int read_rebase_todolist(struct repository *r, const char *fname, struct
repo_git_path_replace(r, &buf, "%s", fname));
}
while (!strbuf_getline_lf(&buf, f)) {
- if (starts_with(buf.buf, comment_line_str))
- continue;
strbuf_trim(&buf);
- if (!buf.len)
- continue;
- abbrev_oid_in_line(r, &buf);
- string_list_append(lines, buf.buf);
+ if (format_todo_line(r, &buf))
+ string_list_append(lines, buf.buf);
}
fclose(f);
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH v4 1/2] sequencer: factor out parsing of todo commands
From: Phillip Wood @ 2026-06-23 15:53 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Patrick Steinhardt, Junio C Hamano, Phillip Wood
In-Reply-To: <cover.1782230024.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Move the code that parses todo commands into a separate function so
that it can be shared with "git status" in the next commit. As we
know the input is NUL terminated we do not pass a pointer to the end
of the line and instead test for a blank line by looking for NUL, CR
LF, or LF. We use starts_with() instead of starts_with_mem() for the
same reason. This results in slightly different behavior when there
a CR at the start of the line that is not followed by LF. Previously
such a line was treated as a comment rather than an invalid line.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 45 ++++++++++++++++++++++++++++++---------------
sequencer.h | 8 ++++++++
2 files changed, 38 insertions(+), 15 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index b7d8dca47f4..b8e860434a8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2625,6 +2625,27 @@ static int is_command(enum todo_command command, const char **bol)
return 1;
}
return 0;
+}
+
+bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd)
+{
+ const char *s = *p;
+
+ for (int i = 0; i < TODO_COMMENT; i++)
+ if (is_command(i, p)) {
+ *cmd = i;
+ return true;
+ }
+
+ if (starts_with(s, comment_line_str)) {
+ *cmd = TODO_COMMENT;
+ return true;
+ } else if (s[0] == '\n' || (s[0] == '\r' && s[1] == '\n') || !s[0]) {
+ *cmd = TODO_COMMENT;
+ return true;
+ }
+
+ return false;
}
static int check_label_or_ref_arg(enum todo_command command, const char *arg)
@@ -2716,29 +2737,23 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
{
struct object_id commit_oid;
char *end_of_object_name;
- int i, saved, status, padding;
+ int saved, status, padding;
item->flags = 0;
/* left-trim */
bol += strspn(bol, " \t");
- if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
- item->command = TODO_COMMENT;
- item->commit = NULL;
- item->arg_offset = bol - buf;
- item->arg_len = eol - bol;
- return 0;
- }
-
- for (i = 0; i < TODO_COMMENT; i++)
- if (is_command(i, &bol)) {
- item->command = i;
- break;
- }
- if (i >= TODO_COMMENT)
+ if (!sequencer_parse_todo_command(&bol, &item->command))
return error(_("invalid command '%.*s'"),
(int)strcspn(bol, " \t\r\n"), bol);
+
+ if (item->command == TODO_COMMENT) {
+ item->commit = NULL;
+ item->arg_offset = bol - buf;
+ item->arg_len = eol - bol;
+ return 0;
+ }
/* Eat up extra spaces/ tabs before object name */
padding = strspn(bol, " \t");
diff --git a/sequencer.h b/sequencer.h
index a6fa670c7c1..28fabef926f 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -262,6 +262,14 @@ int read_author_script(const char *path, char **name, char **email, char **date,
int write_basic_state(struct replay_opts *opts, const char *head_name,
struct commit *onto, const struct object_id *orig_head);
void sequencer_post_commit_cleanup(struct repository *r, int verbose);
+
+/*
+ * Try to parse the todo command pointed to by *p. On success sets cmd,
+ * advances p and returns true. On failure returns false, leaves p and
+ * cmd unchanged.
+ */
+bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd);
+
int sequencer_get_last_command(struct repository* r,
enum replay_action *action);
int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH v4 0/2] status: improve rebase todo list parsing
From: Phillip Wood @ 2026-06-23 15:53 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Patrick Steinhardt, Junio C Hamano, Phillip Wood
In-Reply-To: <cover.1776697483.git.phillip.wood@dunelm.org.uk>
When there is rebase in progress "git status" displays the last couple
of completed and the next couple of pending commands from the todo
list. When it does this is tries to abbreviate the object ids of
the commits to be picked. Unfortunately it does not abbreviate the
object ids when the line starts with "fixup -C" or "merge -C". It
also mistakenly replaces the refname in "reset main" and "update-ref
refs/heads/main" with the object id that the ref points to.
This series fixes that. The first patch factors out the sequencer
code that parses the command names in the todo list. The second patch
uses that function in "git status" to parse the command names so that
it knows whether the line may contain "-C" and whether there is an
object id that should be abbreviated.
Thanks to Junio for his comments on V3.
Changes since V3:
Patch 2 - Style fix for cast.
Changes since V2:
Patch 2 - Check if the object name is a label before trying to
abbreviate it.
Note that a number of the CI jobs fail[1] due to the rather old base,
but a test merge of this branch with "master" passes[2]
[1] https://github.com/phillipwood/git/actions/runs/27906115900
[2] https://github.com/phillipwood/git/actions/runs/27908204055
Changes since V1:
Patch 1 - Expanded commit message and added a code comment.
Patch 2 - Fixed some typos, added a code comment and clarified that -Wswitch
is included by -Wall.
Base-Commit: 8c9303b1ffae5b745d1b0a1f98330cf7944d8db0
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fimprove-status-todo-list-parsing%2Fv4
View-Changes-At: https://github.com/phillipwood/git/compare/8c9303b1f...90c4659b8
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/improve-status-todo-list-parsing/v4
Phillip Wood (2):
sequencer: factor out parsing of todo commands
status: improve rebase todo list parsing
sequencer.c | 45 ++++++++----
sequencer.h | 8 +++
t/t7512-status-help.sh | 74 +++++++++++++-------
wt-status.c | 154 +++++++++++++++++++++++++++++++++--------
4 files changed, 213 insertions(+), 68 deletions(-)
Range-diff against v3:
1: d27dddff931 = 1: d27dddff931 sequencer: factor out parsing of todo commands
2: b3514e9b1c9 ! 2: 90c4659b87e status: improve rebase todo list parsing
@@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
+ struct strbuf scratch = STRBUF_INIT;
+ char *p = line->buf;
+
-+ if (!sequencer_parse_todo_command((const char**)&p, &cmd))
++ if (!sequencer_parse_todo_command((const char **)&p, &cmd))
+ return true; /* keep invalid lines */
+
+ switch (cmd) {
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply
* Re: [PATCH v3 1/2] sequencer: factor out parsing of todo commands
From: Phillip Wood @ 2026-06-23 15:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, Patrick Steinhardt
In-Reply-To: <xmqqpl1i1pef.fsf@gitster.g>
On 22/06/2026 18:00, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Move the code that parses todo commands into a separate function so
>> that it can be shared with "git status" in the next commit. As we
>> know the input is NUL terminated we do not pass a pointer to the end
>> of the line and instead test for a blank line by looking for NUL, CR
>> LF, or LF. We use starts_with() instead of starts_with_mem() for the
>> same reason. This results in slightly different behavior when there
>> a CR at the start of the line that is not followed by LF. Previously
>> such a line was treated as a comment rather than an invalid line.
>
> Meaning that the input validation is tighter than before?
Yes
> I think
> it is fine in this case, as I do not see a reason why anybody wants
> to use a lone CR as comment introducer.
Agreed. In the unlikely event that core.commentChar starts with a CR we
still treat the line as a comment, but we don't treat lines starting
with a CR as a comment anymore. I think that behavior was a lazy way of
handling empty lines with CR LF line endings.
Thanks
Phillip
>> +bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd)
>> +{
>> + const char *s = *p;
>> +
>> + for (int i = 0; i < TODO_COMMENT; i++)
>> + if (is_command(i, p)) {
>> + *cmd = i;
>> + return true;
>> + }
>> +
>> + if (starts_with(s, comment_line_str)) {
>> + *cmd = TODO_COMMENT;
>> + return true;
>> + } else if (s[0] == '\n' || (s[0] == '\r' && s[1] == '\n') || !s[0]) {
>> + *cmd = TODO_COMMENT;
>> + return true;
>> + }
>> +
>> + return false;
>> }
>
> I notice that the order of noticing concrete comments and comment
> lines are swapped relative to the original. There is no inherently
> "natural" order between them, so the change is perfectly OK. I just
> got confused slightly while reading it until I realized that is what
> you did.
>
>> static int check_label_or_ref_arg(enum todo_command command, const char *arg)
>> @@ -2716,29 +2737,23 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
>> {
>> struct object_id commit_oid;
>> char *end_of_object_name;
>> - int i, saved, status, padding;
>> + int saved, status, padding;
>>
>> item->flags = 0;
>>
>> /* left-trim */
>> bol += strspn(bol, " \t");
>>
>> - if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
>> - item->command = TODO_COMMENT;
>> - item->commit = NULL;
>> - item->arg_offset = bol - buf;
>> - item->arg_len = eol - bol;
>> - return 0;
>> - }
>> -
>> - for (i = 0; i < TODO_COMMENT; i++)
>> - if (is_command(i, &bol)) {
>> - item->command = i;
>> - break;
>> - }
>> - if (i >= TODO_COMMENT)
>> + if (!sequencer_parse_todo_command(&bol, &item->command))
>> return error(_("invalid command '%.*s'"),
>> (int)strcspn(bol, " \t\r\n"), bol);
>> +
>> + if (item->command == TODO_COMMENT) {
>> + item->commit = NULL;
>> + item->arg_offset = bol - buf;
>> + item->arg_len = eol - bol;
>> + return 0;
>> + }
>
> And the extra stuff that are only relevant to a comment line is
> naturally processed by the caller. OK.
>
> Thanks. Looking good so far.
>
^ permalink raw reply
* Re: [PATCH v2] help: include arguments in autocorrect=prompt message
From: Jishnu C K @ 2026-06-23 15:21 UTC (permalink / raw)
To: git; +Cc: gitster, Justin Tobler
In-Reply-To: <6a357689.0f9b68c4.317a5d.1919@mx.google.com>
Any review comments?
Sent from my iPhone
> On 19 Jun 2026, at 10:34 PM, Jishnu C K <jishnuck26@gmail.com> wrote:
>
> v2: Reworked as an incremental improvement to the existing
> autocorrect=prompt code path rather than a parallel reimplementation,
> per feedback from Junio and Justin.
>
> ---
> From a4e8fb6fd6dd6a501e565c7500cbf927d7cb0b42 Mon Sep 17 00:00:00 2001
> From: calicomills <jishnuck26@gmail.com>
> Date: Fri, 19 Jun 2026 13:01:40 +0530
> Subject: [PATCH v2 v2] help: include arguments in autocorrect=prompt message
>
> When 'help.autocorrect=prompt' is configured and the user mistypes
> a git command, the prompt currently shows only the corrected command
> name:
>
> Run 'checkout' instead [y/N]?
>
> This leaves the user unsure whether their original arguments will be
> preserved. Update the prompt to include the full corrected invocation:
>
> Run 'git checkout neo' instead [y/N]?
>
> The help_unknown_cmd() signature is updated to accept the args vector
> so the prompt can show the original arguments alongside the corrected
> command name. Callers that do not have access to the args (e.g.
> builtin/help.c) pass NULL, which is handled gracefully.
>
> Signed-off-by: calicomills <jishnuck26@gmail.com>
> ---
> help.c | 49 +++++++++++++----------------------
> t/t9003-help-autocorrect.sh | 51 +++++--------------------------------
> 2 files changed, 23 insertions(+), 77 deletions(-)
>
> diff --git a/help.c b/help.c
> index 30f32a7206..9ea4c076e1 100644
> --- a/help.c
> +++ b/help.c
> @@ -739,7 +739,16 @@ char *help_unknown_cmd(const char *cmd, const struct strvec *args)
> else if (cfg.autocorrect == AUTOCORRECT_PROMPT) {
> char *answer;
> struct strbuf msg = STRBUF_INIT;
> - strbuf_addf(&msg, _("Run '%s' instead [y/N]? "), assumed);
> + struct strbuf full_cmd = STRBUF_INIT;
> + strbuf_addstr(&full_cmd, assumed);
> + if (args) {
> + for (size_t j = 1; j < args->nr; j++) {
> + strbuf_addch(&full_cmd, ' ');
> + strbuf_addstr(&full_cmd, args->v[j]);
> + }
> + }
> + strbuf_addf(&msg, _("Run 'git %s' instead [y/N]? "), full_cmd.buf);
> + strbuf_release(&full_cmd);
> answer = git_prompt(msg.buf, PROMPT_ECHO);
> strbuf_release(&msg);
> if (!(starts_with(answer, "y") ||
> @@ -762,37 +771,13 @@ char *help_unknown_cmd(const char *cmd, const struct strvec *args)
> fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);
>
> if (SIMILAR_ENOUGH(best_similarity)) {
> - if (n == 1 && isatty(0) && isatty(2)) {
> - char *answer;
> - struct strbuf msg = STRBUF_INIT;
> - struct strbuf full_cmd = STRBUF_INIT;
> - strbuf_addstr(&full_cmd, main_cmds.names[0]->name);
> - if (args) {
> - for (size_t j = 1; j < args->nr; j++) {
> - strbuf_addch(&full_cmd, ' ');
> - strbuf_addstr(&full_cmd, args->v[j]);
> - }
> - }
> - strbuf_addf(&msg, _("\nDid you mean 'git %s'? [y/N] "),
> - full_cmd.buf);
> - strbuf_release(&full_cmd);
> - answer = git_prompt(msg.buf, PROMPT_ECHO);
> - strbuf_release(&msg);
> - if (starts_with(answer, "y") || starts_with(answer, "Y")) {
> - char *assumed = xstrdup(main_cmds.names[0]->name);
> - cmdnames_release(&cfg.aliases);
> - cmdnames_release(&main_cmds);
> - cmdnames_release(&other_cmds);
> - return assumed;
> - }
> - } else {
> - fprintf_ln(stderr,
> - Q_("\nThe most similar command is",
> - "\nThe most similar commands are",
> - n));
> - for (i = 0; i < n; i++)
> - fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
> - }
> + fprintf_ln(stderr,
> + Q_("\nThe most similar command is",
> + "\nThe most similar commands are",
> + n));
> +
> + for (i = 0; i < n; i++)
> + fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
> }
>
> exit(1);
> diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
> index 6fe2da1595..75821d63e1 100755
> --- a/t/t9003-help-autocorrect.sh
> +++ b/t/t9003-help-autocorrect.sh
> @@ -70,57 +70,18 @@ test_expect_success 'autocorrect works in work tree created from bare repo' '
> git -C worktree -c help.autocorrect=immediate status
> '
>
> -# Default behaviour (no help.autocorrect set): when there is exactly one
> -# similar command but the session is non-interactive, fall back to printing
> -# the suggestion list and exiting rather than showing a prompt.
> -test_expect_success 'default: single match non-interactive shows suggestion and fails' '
> - test_might_fail git config --unset help.autocorrect &&
> -
> - test_must_fail git lfg 2>actual &&
> - grep "most similar command" actual &&
> - grep "lgf" actual
> -'
> -
> -test_expect_success 'default: multiple matches non-interactive shows list and fails' '
> - test_might_fail git config --unset help.autocorrect &&
> -
> - test_must_fail git com 2>actual &&
> - grep "most similar commands" actual &&
> - grep "commit" actual
> -'
> -
> -# Interactive prompt tests require a real TTY. On macOS the TTY prereq is
> -# skipped due to IO::Pty reliability issues; these tests run on Linux CI.
> -test_expect_success TTY 'default: single match interactive, answer y runs command' '
> - git config --unset help.autocorrect &&
> -
> - write_script git-typotest <<-\EOF &&
> - echo typotest-ran
> - EOF
> - PATH="$PATH:." export PATH &&
> -
> - # Feed "y" to /dev/tty via a wrapper that answers the prompt
> - write_script answer-prompt <<-\EOF &&
> - # Write the answer to the controlling terminal
> - printf "y\n" >/dev/tty
> - exec "$@"
> - EOF
> -
> - test_terminal ./answer-prompt git typotest 2>err >out &&
> - grep "typotest-ran" out &&
> - grep "Did you mean" err
> -'
> -
> -test_expect_success TTY 'default: single match interactive, answer n exits cleanly' '
> - git config --unset help.autocorrect &&
> +# autocorrect=prompt should include the original arguments in the prompt.
> +# Requires a TTY; skipped on macOS due to IO::Pty reliability issues.
> +test_expect_success TTY 'autocorrect=prompt includes arguments in prompt' '
> + git config help.autocorrect prompt &&
>
> write_script answer-prompt-no <<-\EOF &&
> printf "n\n" >/dev/tty
> exec "$@"
> EOF
>
> - test_must_fail test_terminal ./answer-prompt-no git typotest 2>err &&
> - grep "Did you mean" err
> + test_must_fail test_terminal ./answer-prompt-no git lfg --oneline 2>actual &&
> + grep "lgf --oneline" actual
> '
>
> test_done
> --
> 2.50.1
>
>
^ permalink raw reply
* Re: [PATCH] gpg-interface: fix strip_cr_before_lf to only remove CR before LF
From: Junio C Hamano @ 2026-06-23 15:09 UTC (permalink / raw)
To: DSAntonio08; +Cc: git
In-Reply-To: <20260623084520.9015-1-antonio.destefani08@gmail.com>
DSAntonio08 <antonio.destefani08@gmail.com> writes:
> The remove_cr_after() function was stripping all CR characters
> unconditionally, even lone \r not followed by \n. This is incorrect
> as only \r\n sequences (Windows line endings) should be normalized.
>
> Fix the loop condition to skip \r only when immediately followed by
> \n, and rename the function to strip_cr_before_lf to reflect its
> actual behavior. Update both call sites and their comments accordingly.
> ---
A few comments.
- Documentation/SubmittingPatches[[sign-off]] wants you to certify
that this patch is something you have the right to submit to this
project. Sign-off is missing at the end of the proposed log
message.
- Documentation/SubmittingPatches[[real-name]] also prefers to see
us interacting with humans with real-sounding names, not handles.
- When changing design decision made in a fairly ancient code, we
would prefer to see the proposed log message says why the code is
that way, and how and why it is safe to change it.
As to the last point, in this particular case, the NEEDSWORK comment
says "only CRs before LFs", but we must not blindly follow NEEDSWORK
comments. They just mean "somebody may want to rethink the issues
in the future but right now we stop here and leave the code in this
shape."
So, let's see if we can continue their thinking.
This "We should normalize CR/LF into LF but we are too lazy to
bother" originally came from c4adea82 (Convert CR/LF to LF in tag
signatures, 2008-07-11), and then 2f47eae2 (Split GPG interface into
its own helper library, 2011-09-07) moved the code around to make
the part interacting with GPG reusable. Later when SSH signing was
introduced in 29b31577 (ssh signing: add ssh key format and signing
code, 2021-09-10), the "remove CR" was made into a helper function,
and that is what remains today.
Having something like the above paragraph in the proposed commit log
message would have been very good. Further, there might be old
discussion that led to c4adea82 where people may have discussed if
it is sensible to be lazy, which you may further want to dig down to
the root, to make sure that even back then people were aware that
touching only CRLF, not all CRs that appear at random places, was
the right thing and the code was done only due to laziness (rather
than, e.g., leaving lone CRs in the message somehow harms other
parts of the system in a way we are not realizing in this
discussion).
That would make a very good supporting material to convince readers
why the change this patch is making a good idea.
> gpg-interface.c | 18 +++++++-----------
> 1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index dafd5371fa..87ae6503da 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -989,17 +989,13 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
> free(keyid_to_free);
> return ret;
> }
> -
> -/*
> - * Strip CR from the line endings, in case we are on Windows.
> - * NEEDSWORK: make it trim only CRs before LFs and rename
> - */
> -static void remove_cr_after(struct strbuf *buffer, size_t offset)
> +/* Strip CR before LF from the line endings, in case we are on Windows. */
> +static void strip_cr_before_lf(struct strbuf *buffer, size_t offset)
> {
> size_t i, j;
>
> for (i = j = offset; i < buffer->len; i++) {
> - if (buffer->buf[i] != '\r') {
> + if (buffer->buf[i] != '\r' || (i + 1 < buffer->len && buffer->buf[i + 1] != '\n')) {
Please avoid making lines overly long like this. Wrapping at a
logical break in the expression like this:
if (buffer->buf[i] != '\r' ||
(i + 1 < buffer->len && buffer->buf[i + 1] != '\n')) {
would not just make the line fit in a reasonable width limit, but
makes it easier to follow.
> if (i != j)
> buffer->buf[j] = buffer->buf[i];
> j++;
More importantly, isn't the above slightly buggy? If the buffer
ends with a lone CR (i.e. not followed by LF), your condition:
if (buffer->buf[i] != '\r' || (i + 1 < buffer->len && buffer->buf[i + 1] != '\n'))
will evaluate to false because both operands of the OR are false;
the byte we are looking at is CR so the LHS off OR is false, and
because we are at the end of the buffer, so we do not have the byte
that follows ours that is not LF, which makes RHS of OR also false.
The lone trailing CR will be skipped, instead of getting copied.
If I were writing this, I would have written it more like this:
for (i = j = offset; i < buffer->len; i++) {
if (buffer->buf[i] == '\r' &&
i + 1 < buffer->len && buffer->buf[i + 1] == '\n')
continue;
buffer->buf[j++] = buffer->buf[i];
}
This not only avoids the bug by keeping lone trailing CRs, but
also makes what we are special-casing stand out more clearly
(assignment is the norm, skipping is the exception), and avoids
pushing the assignment too deep in the conditional for readability.
The changes to the callers (due to function name update) below
(ellided) both looked fine.
Thanks.
^ permalink raw reply
* Fetching missing submodule refs unnecessarily fatal
From: Mike Crowe @ 2026-06-23 14:27 UTC (permalink / raw)
To: git
When Git fetches in a superproject with --recurse-submodules, it appears to
try to fetch the corresponding submodule repository commits for every new
or updated superproject branch. Presumably this is so that everthing is
ready to switch to one of those branches without further fetching.
Developers may create commits that contain submodules that reference
commits in the submodule repository, but those commits may not be pushed to
the submodule's remote repository. When the superproject commits are pushed
to a personal remote branch anyone else's Git fetch cannot find the
corresponding submodule commit and fails. For example:
$ git fetch
remote: Enumerating objects: 4, done.
remote: Counting objects: 100% (4/4), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
Unpacking objects: 100% (3/3), 355 bytes | 355.00 KiB/s, done.
From ssh://localhost/home/mac/git/git/repro/repositories/super
* [new branch] repro-branch -> origin/repro-branch
Fetching submodule submodule
error: Server does not allow request for unadvertised object f6b0ccce6e2085cf03c3fd924730f5c9f91e3db1
Errors during submodule fetch:
submodule
(when fetching via ssh)
or:
fatal: remote error: want c1f59d10bd6f24adbc96fee6a5041e9f3dc94b7c not valid
fatal: internal server error
fatal: remote error: want f91af98469911e79c2a27329d8e115dfc59f31c0 not valid
fatal: internal server error
Errors during submodule fetch:
sources/repo-1
sources/repo-2
(when using a JGit server configured to allow any SHA-1 to be fetched.)
These are hard errors that cause Git to exit with a non-zero exit status.
Repeating the operation succeeds because no there is no update to the
remote branch to trigger the submodule fetch again.
I've had a couple of goes at bisecting this but I always end up failing on
unrelated commits due to the master/main default branch change and my
attempts to work around that produce inconsistent results.
I don't believe that developers who have the ability to create personal
branches should be able to force anyone else cloning or fetching from the
repository to suffer such a failure. This is particularly a problem for CI
systems but it confuses users too.
Potential mitigations:
1. Use --no-recurse-submodules. This disables all submodule processing
though, which is not desirable.
2. Use `git fetch || git fetch` to repeat the fetch if it fails the first
time. This will work around the problem almost all of the time but is
racy since there's a small risk that the second fetch will encounter a
new branch with the same problem.
I've added a script which can be used to reproduce the problem to the end
of this message.
I'm not really sure what a good solution to this is:
1. The recursive fetch could only look for submodule commits to fetch on
the current tracking branch.
2. Treat any failure to fetch submodules as non-fatal. Hacking
fetch_submodules() to always return zero does solve this problem but at
the cost of not failing for more-serious ones! Any attempt to check out
the unfetchable commit would fail at that point though.
Thanks.
Mike.
--8<--
#!/bin/bash
set -xe
temp=$(pwd)/repro
rm -rf ${temp}
mkdir ${temp}
repositories=${temp}/repositories
# Work around protocol.file.allow = "user" by default in new
# repositories without affecting the user's global Git configuration
# by fetching over ssh from localhost.
remote=ssh://localhost${repositories}
# Create the "remote" repositories
mkdir -p ${repositories}/{super,sub}
git -C ${repositories}/super init --bare
git -C ${repositories}/sub init --bare
# Create original submodule repository contents
mkdir -p ${temp}/sub
pushd ${temp}/sub
git init .
date > submodule-file
git add submodule-file
git commit -m "Initial submodule commit"
git remote add origin ${remote}/sub
git push -u origin
popd
rm -rf ${temp}/sub
# Create original super repository contents
workspace=${temp}/workspace
mkdir -p ${workspace}
pushd ${workspace}
git clone ${remote}/super orig
pushd orig
date > file
git add file
git commit -m "Initial supermodule commit"
git submodule add ${remote}/sub submodule
git commit -m "Add initial submodule"
git push
popd
# Create a new clone of the super and submodules
git clone --recurse-submodules ${remote}/super clone
# Now create a dangling submodule commit in the original supermodule
date >> ${workspace}/orig/submodule/submodule-file
git -C ${workspace}/orig/submodule add submodule-file
git -C ${workspace}/orig/submodule commit -m "Modify submodule"
git -C ${workspace}/orig checkout -b repro-branch
git -C ${workspace}/orig add submodule
git -C ${workspace}/orig commit -m "Bump submodule"
git -C ${workspace}/orig push origin repro-branch
# Now update the second clone to show an error even though the
# missing submodule commit is on a completely different branch.
git -C ${workspace}/clone fetch
^ permalink raw reply
* Re: [PATCH/RFC 2/6] commit-reach: introduce struct paint_queue with per-side counters
From: Derrick Stolee @ 2026-06-23 14:17 UTC (permalink / raw)
To: Kristofer Karlsson
Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <CAL71e4PzjdNCaVRtXg7wh9s6DxBeA4ock1aTzq8VPxKCmE-obA@mail.gmail.com>
On 6/23/2026 10:09 AM, Kristofer Karlsson wrote:
> On Tue, 23 Jun 2026 at 15:50, Derrick Stolee <stolee@gmail.com> wrote:
>> How much of this data that you are passing into the method could be
>> state in the paint_queue struct? Could we have the paint_queue manage
>> all of the state necessary to make decisions around the walk
>> termination?
>
> Good idea, I think adding last_gen to the struct is doable and makes it cleaner.
> If needed we could also add the mb_flags there (but would be a followup patch)
> Minor note: I renamed the struct to paint_state so that I could rename
> the prio_queue to queue and not have "queue.queue" which felt
> confusing in the code.
>
>> Or, could we do a peek into the queue to see the "top" commit, and
>> check if it is a finite commit or not? I know that 'last_gen' is
>> supposed to be the commit walked in the previous cycle, but it seems
>> that we only care about "the remaining commits are finite" as our
>> condition.
>
> Yes, peeking into the queue would work too, but it would feel awkward,
>
> commit = prio_queue_peek();
> if (halt conditions) return NULL;
> prio_queue_get();
Good instinct to notice that peeking and getting from the same
method is awkward.
> And if we get first, the condition is not valid - that said, it would be doable
> to instead put the halt conditions _between_ popping the commit and
> updating the counters. I am not sure how ugly or confusing it would be,
> but I could add a comment to explain why that sequencing is important.
> (Popping the commit and updating the counters may lead to temporary
> 0 counts, but then when we enqueue parents of the commits they
> move away from the 0 anyway). It would become something like:
>
> // dry-/pseudo-coded
> commit *paint_queue_pop() {
> commit = prio_queue_pop();
> if (!commit) return NULL;
> if (halt_condition(state, commit.generation)) return NULL;
> // important: don't decrement counters before checking the halt condition
> paint_count_update(state, commit->object.flags, -1);
> return commit;
> }
I think this would be an appropriate way to handle this. If we
pop and return NULL then it's ok that we removed data from the
queue because it shouldn't be reused.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH/RFC 2/6] commit-reach: introduce struct paint_queue with per-side counters
From: Kristofer Karlsson @ 2026-06-23 14:09 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <509fa950-fb9b-468d-b917-6c0eb7823d64@gmail.com>
On Tue, 23 Jun 2026 at 15:50, Derrick Stolee <stolee@gmail.com> wrote:
>
> > For the termination conditions, I moved them into paint_queue_get()
> > as you suggested. The all-zero check was straightforward since it
> > only depends on the counters but the side-exhaustion check also
> > needs to know whether we have entered the finite-generation region,
> > so I pass last_gen (already a local in paint_down_to_common) as a
> > parameter:
> >
> > static struct commit *paint_queue_get(struct paint_state *state,
> > timestamp_t last_gen)
> >
> > Inside, the two conditions merge nicely under a shared guard:
> >
> > if (!state->pending_merge_bases) {
> > if (!state->p1_count && !state->p2_count)
> > return NULL;
> > if (last_gen < GENERATION_NUMBER_INFINITY &&
> > (!state->p1_count || !state->p2_count))
> > return NULL;
> > }
>
> This looks good to me. I'm not even bothered by the last_gen
> parameter. You do make a good point about it being a potentially
> leaky abstraction.
Agreed, I am not also bothered by it.
> > Both conditions require pending_merge_bases == 0, so the nesting
> > felt natural. The first is "nothing non-stale left" (works in any
> > region). The second is "one side exhausted" (only in the finite
> > region where topological ordering holds).
> >
> > I think passing in last_gen into paint_queue_get() feels _slightly_
> > awkward but not too bad in practice. However, we also have my
> > older (first) patch with the fast-exit if the caller only needs one
> > merge base -- that has a separate break that also could be folded
> > into paint_queue_get(). The messy part here is that we would need
> > to also pass the mb_flags parameter to paint_queue_get().
>
> How much of this data that you are passing into the method could be
> state in the paint_queue struct? Could we have the paint_queue manage
> all of the state necessary to make decisions around the walk
> termination?
Good idea, I think adding last_gen to the struct is doable and makes it cleaner.
If needed we could also add the mb_flags there (but would be a followup patch)
Minor note: I renamed the struct to paint_state so that I could rename
the prio_queue to queue and not have "queue.queue" which felt
confusing in the code.
> Or, could we do a peek into the queue to see the "top" commit, and
> check if it is a finite commit or not? I know that 'last_gen' is
> supposed to be the commit walked in the previous cycle, but it seems
> that we only care about "the remaining commits are finite" as our
> condition.
Yes, peeking into the queue would work too, but it would feel awkward,
commit = prio_queue_peek();
if (halt conditions) return NULL;
prio_queue_get();
And if we get first, the condition is not valid - that said, it would be doable
to instead put the halt conditions _between_ popping the commit and
updating the counters. I am not sure how ugly or confusing it would be,
but I could add a comment to explain why that sequencing is important.
(Popping the commit and updating the counters may lead to temporary
0 counts, but then when we enqueue parents of the commits they
move away from the 0 anyway). It would become something like:
// dry-/pseudo-coded
commit *paint_queue_pop() {
commit = prio_queue_pop();
if (!commit) return NULL;
if (halt_condition(state, commit.generation)) return NULL;
// important: don't decrement counters before checking the halt condition
paint_count_update(state, commit->object.flags, -1);
return commit;
}
> > Right now I am leaning towards simply passing in last_gen and
> > containing all of the halt conditions there
> > (except the old !FIND_ALL).
>
> This is a good start, but hopefully storing the data in the
> struct would be a good way to handle that.
Sounds good, I will massage the code a bit, store the relevant pieces
in the struct
and see how clean I can make it.
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH/RFC 2/6] commit-reach: introduce struct paint_queue with per-side counters
From: Derrick Stolee @ 2026-06-23 13:50 UTC (permalink / raw)
To: Kristofer Karlsson
Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <CAL71e4NFHz_zVCWPvmTO8UPNyaKkDFqNQdd3CJykoiGmEhfUTA@mail.gmail.com>
On 6/23/2026 6:13 AM, Kristofer Karlsson wrote:
> On Mon, 22 Jun 2026 at 22:23, Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 6/22/2026 3:14 PM, Kristofer Karlsson wrote:
>>>
>>> On Mon, 22 Jun 2026 at 20:10, Derrick Stolee <stolee@gmail.com> wrote:
>>>>
>>>> When possible, I like to try to make loops only have one terminating
>>>> condition. Should we have paint_queue_get() return NULL when it sees
>>>> this internal state condition?
>>>
>>> Possibly, but that would couple the paint_queue struct very tightly with
>>> the usage. Not a problem in practice since it only has one call site, and
>>> it's unlikely that we want to add more of them but it may feel more natural
>>> to let the paint_queue purely have the queue semantics and counters,
>>> and keep the halt condition within the function itself. I don't feel
>>> super-strongly about this and can change it if needed, I will just need to
>>> verify that nothing else gets complex as a result, I have not fully thought
>>> through the effects.
>>
>> Hm. Interesting. The coupling is perhaps expected, because the data
>> structure tracks counts that don't otherwise need to be tracked.
>> Maybe the terminating condition method could be descriptively named
>> to say why it would be completing.
>>
>
> I have been working on v2 locally and most of the changes landed
> nicely and were clear improvements but there's one point I would
> want to discuss a bit more.
>
> For the termination conditions, I moved them into paint_queue_get()
> as you suggested. The all-zero check was straightforward since it
> only depends on the counters but the side-exhaustion check also
> needs to know whether we have entered the finite-generation region,
> so I pass last_gen (already a local in paint_down_to_common) as a
> parameter:
>
> static struct commit *paint_queue_get(struct paint_state *state,
> timestamp_t last_gen)
>
> Inside, the two conditions merge nicely under a shared guard:
>
> if (!state->pending_merge_bases) {
> if (!state->p1_count && !state->p2_count)
> return NULL;
> if (last_gen < GENERATION_NUMBER_INFINITY &&
> (!state->p1_count || !state->p2_count))
> return NULL;
> }
This looks good to me. I'm not even bothered by the last_gen
parameter. You do make a good point about it being a potentially
leaky abstraction.
> Both conditions require pending_merge_bases == 0, so the nesting
> felt natural. The first is "nothing non-stale left" (works in any
> region). The second is "one side exhausted" (only in the finite
> region where topological ordering holds).
>
> I think passing in last_gen into paint_queue_get() feels _slightly_
> awkward but not too bad in practice. However, we also have my
> older (first) patch with the fast-exit if the caller only needs one
> merge base -- that has a separate break that also could be folded
> into paint_queue_get(). The messy part here is that we would need
> to also pass the mb_flags parameter to paint_queue_get().
How much of this data that you are passing into the method could be
state in the paint_queue struct? Could we have the paint_queue manage
all of the state necessary to make decisions around the walk
termination?
Or, could we do a peek into the queue to see the "top" commit, and
check if it is a finite commit or not? I know that 'last_gen' is
supposed to be the commit walked in the previous cycle, but it seems
that we only care about "the remaining commits are finite" as our
condition.
> Perhaps we should just let this remain as-is for now and follow up
> with _removing_ that optimization. I think the value of having it
> is much diminished (but not fully gone) by the side-exhaust approach.
>
> Additionally there's a correctness argument to be made -- perhaps
> all callers _should_ care about multiple merge bases existing, and
> instead bail out if it finds more than one. The only use case
> where this matters today is "git merge-base A B" without --all.
> Right now I am leaning towards simply passing in last_gen and
> containing all of the halt conditions there
> (except the old !FIND_ALL).
This is a good start, but hopefully storing the data in the
struct would be a good way to handle that.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v14 2/2] checkout: extend --track with a "fetch" mode to refresh start-point
From: Phillip Wood @ 2026-06-23 13:49 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Ramsay Jones, D. Ben Knoble, Kristoffer Haugsbakk, Marc Branchaud,
Harald Nordgren
In-Reply-To: <8518f090b1069a02d40c710975528ad118776b67.1781786652.git.gitgitgadget@gmail.com>
Hi Harald
On 18/06/2026 13:44, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Add a "fetch" mode to the "--track" option of "git checkout" / "git
> switch" that refreshes <start-point> before checking it out:
>
> git checkout -b new_branch --track=fetch origin/some-branch
>
> is shorthand for
>
> git fetch origin some-branch
> git checkout -b new_branch --track origin/some-branch
>
> Identify the remote whose configured fetch refspec maps to
> <start-point> using find_tracking_remote_for_ref() (the same lookup
> "--track" uses to pick which remote to record in
> branch.<name>.remote), then run "git fetch <remote> <src-ref>" for
> just that ref so other remote-tracking branches are left untouched.
> When <start-point> is a bare <remote> (e.g. "origin"), follow
> refs/remotes/<remote>/HEAD to learn which branch to refresh. If
> "git fetch" fails but the remote-tracking ref already exists locally,
> warn and proceed from the existing tip; otherwise abort.
This describes the feature well, but does not really explain why it is
convenient to have a shorthand for "git fetch ... && git checkout -b
...". For example if the reason is that in a fast-moving project you
want to start your new work off the latest upstream changes to minimize
the chance of merge conflicts or duplicated work it would be useful to
say that. As Junio has said the implementation looks pretty solid I've
left a few comments below, but the important thing to do first is to
convince others that this is a useful feature and why it is worth
blurring the separation between fetch and checkout. You can do that
without sending a new version.
> diff --git a/Documentation/git-checkout.adoc b/Documentation/git-checkout.adoc
> index a8b3b8c2e2..20b6cae60e 100644
> --- a/Documentation/git-checkout.adoc
> +++ b/Documentation/git-checkout.adoc
> @@ -158,11 +158,26 @@ of it").
> resets _<branch>_ to the start point instead of failing.
>
> `-t`::
> -`--track[=(direct|inherit)]`::
> +`--track[=(direct|inherit|fetch)[,...]]`::
> When creating a new branch, set up "upstream" configuration. See
> `--track` in linkgit:git-branch[1] for details. As a convenience,
> --track without -b implies branch creation.
> +
> +The argument is a comma-separated list. `direct` (the default) and
> +`inherit` select the tracking mode and are mutually exclusive. Adding
> +`fetch` requests that the remote be fetched before _<start-point>_ is
> +resolved, so the new branch starts from a fresh tip: when
> +_<start-point>_ is in _<remote>/<branch>_ form, only that branch is
> +updated; when _<start-point>_ is a bare _<remote>_ (e.g. `origin`), the
> +branch named by _<remote>/HEAD_ is updated, and the checkout fails
> +with a hint to configure that symref if it is not set. The checkout
> +also fails if no configured remote's fetch refspec maps to
> +_<start-point>_, or if more than one does (in which case the `fetch`
> +cannot be unambiguously routed). If the fetch itself fails and the
> +corresponding remote-tracking ref already exists, a warning is printed
> +and the checkout proceeds from the existing tip; otherwise the checkout
> +is aborted.
Nicely explained
> +static void fetch_remote_for_start_point(const char *arg, int quiet)
> +{
> + struct strbuf dst = STRBUF_INIT;
> + struct tracking tracking;
> + struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
> + struct string_list ambiguous_remotes = STRING_LIST_INIT_DUP;
> + struct child_process cmd = CHILD_PROCESS_INIT;
> + struct object_id oid;
> + struct remote *named_remote;
> + int bare_ns;
> +
> + strbuf_addf(&dst, "refs/remotes/%s", arg);
> + if (check_refname_format(dst.buf, 0))
> + die(_("cannot fetch start-point '%s': not a valid "
> + "remote-tracking name"), arg);
> +
> + named_remote = remote_get(arg);
> + bare_ns = !strchr(arg, '/') ||
> + (named_remote && remote_is_configured(named_remote, 1));
> + if (bare_ns) {
> + char *head_path = xstrfmt("refs/remotes/%s/HEAD", arg);
> + const char *head_target =
> + refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> + head_path,
> + RESOLVE_REF_READING |
> + RESOLVE_REF_NO_RECURSE,
Why do we use RESOLVE_REF_NO_RECURSE here? This should match whatever
"git checkout -b <remote>" does.
> + &oid, NULL);
> + if (head_target &&
> + starts_with(head_target, dst.buf) &&
> + head_target[dst.len] == '/' &&
> + !check_refname_format(head_target, 0)) {
I don't think there is any need to call check_refname_format() here -
you're using the result of reading a ref, not some untrusted input.
> + strbuf_reset(&dst);
> + strbuf_addstr(&dst, head_target);
> + bare_ns = 0;
> + }
> + free(head_path);
> + }
> +
> + memset(&tracking, 0, sizeof(tracking));
When you want to zero initialize a stack variable it is easier, clearer
and less error-prone to initialize it by adding "= {0};" where it is
declared.
> + tracking.spec.dst = dst.buf;
> + tracking.srcs = &tracking_srcs;
> + find_tracking_remote_for_ref(&tracking, &ambiguous_remotes);
> +
> + if (tracking.matches > 1) {
> + int status = die_message(_("cannot fetch start-point '%s': "
> + "fetch refspecs of multiple remotes "
> + "map to '%s'"), arg, dst.buf);
> + advise_ambiguous_fetch_refspec(dst.buf, &ambiguous_remotes);
> + exit(status);
> + }
> +
> + if (!tracking.matches) {
> + if (bare_ns && named_remote &&
> + remote_is_configured(named_remote, 1))
> + die(_("cannot fetch start-point '%s': "
> + "'refs/remotes/%s/HEAD' is not set; run "
> + "'git remote set-head %s --auto' to set it")
This is quite a long message for a single line - breaking the line and
putting the suggested command on a separate line would make it clearer.
Something like
cannot fetch start-point 'origin' because 'refs/remotes/origin/HEAD'
does not exist. To create it run
git remote set-head origin --auto
> + arg, arg, arg);
> + die(_("cannot fetch start-point '%s': no configured remote's "
> + "fetch refspec matches it"), arg);
> + }
> +
> + strvec_push(&cmd.args, "fetch");
> + if (quiet)
> + strvec_push(&cmd.args, "--quiet");
> + strvec_pushl(&cmd.args, tracking.remote,
> + tracking_srcs.items[0].string, NULL);
> + cmd.git_cmd = 1;
> + if (run_command(&cmd)) {
> + if (!refs_read_ref(get_main_ref_store(the_repository),
> + dst.buf, &oid))
You can use refs_ref_exists() to check a ref exists which avoids
declaring "oid" which we're not interested in here.
> + warning(_("failed to fetch start-point '%s'; "
> + "using existing '%s'"), arg, dst.buf);
> + else
> + die(_("failed to fetch start-point '%s'"), arg);
> + }
> +
> + string_list_clear(&tracking_srcs, 0);
> + string_list_clear(&ambiguous_remotes, 0);
> + strbuf_release(&dst);
> +}
> +
> +static int parse_opt_checkout_track(const struct option *opt,
> + const char *arg, int unset)
> +{
> + struct checkout_opts *opts = opt->value;
> + struct string_list tokens = STRING_LIST_INIT_DUP;
> + struct string_list_item *item;
> + int saw_direct = 0;
> + int ret = 0;
> +
> + opts->fetch = 0;
> + if (unset) {
> + opts->track = BRANCH_TRACK_NEVER;
> + return 0;
> + }
> + opts->track = BRANCH_TRACK_EXPLICIT;
> + if (!arg)
> + return 0;
> +
> + string_list_split(&tokens, arg, ",", -1);
> + for_each_string_list_item(item, &tokens) {
> + if (!strcmp(item->string, "fetch"))
> + opts->fetch = 1;
> + else if (!strcmp(item->string, "direct"))
> + saw_direct = 1;
> + else if (!strcmp(item->string, "inherit"))
> + opts->track = BRANCH_TRACK_INHERIT;
> + else {
> + ret = error(_("option `%s' expects \"%s\", \"%s\", "
> + "or \"%s\""),
> + "--track", "direct", "inherit", "fetch");
> + goto out;
> + }
> + }
> + if (saw_direct && opts->track == BRANCH_TRACK_INHERIT)
> + ret = error(_("option `%s' cannot combine \"%s\" and \"%s\""),
> + "--track", "direct", "inherit");
This parsing looks good
> diff --git a/t/t7201-co.sh b/t/t7201-co.sh
> index 7613b1d2a4..1e321b1512 100755
> --- a/t/t7201-co.sh
> +++ b/t/t7201-co.sh
> @@ -870,4 +870,280 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
> test_cmp_config "" --default "" branch.main2.merge
> '
I've not read the tests in detail but there seem to be an awful lot of
them. We only need to test each thing once so for example if we test
git checkout --track=fetch -b <remote-ref>
with a fetch refspec, that maps refs/heads/*:refs/remotes/origin/xxx/*
then we don't need to test it without that refspec. I notice you use
"namespace" below with is confusing because it is not referring to the
feature described in the gitnamespaces(7) man page.
Try and avoid
test $a = $b
as it makes it hard to debug failing tests. Instead I think you can use
test_cmp_rev in this case.
Thanks
Phillip
> +test_expect_success 'setup upstream for --track=fetch tests' '
> + git checkout main &&
> + git init fetch_upstream &&
> + test_commit -C fetch_upstream u_main &&
> + git remote add fetch_upstream fetch_upstream &&
> + git fetch fetch_upstream &&
> + git -C fetch_upstream checkout -b fetch_new &&
> + test_commit -C fetch_upstream u_new
> +'
> +
> +test_expect_success 'checkout --track=fetch -b picks up branch created upstream after clone' '
> + git checkout main &&
> + test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_new &&
> + git checkout --track=fetch -b local_new fetch_upstream/fetch_new &&
> + test_cmp_rev refs/remotes/fetch_upstream/fetch_new HEAD &&
> + test_cmp_config fetch_upstream branch.local_new.remote &&
> + test_cmp_config refs/heads/fetch_new branch.local_new.merge
> +'
> +
> +test_expect_success 'checkout --track=fetch <remote>/<branch> leaves other tracking branches untouched' '
> + git checkout main &&
> + git -C fetch_upstream checkout -b fetch_target &&
> + test_commit -C fetch_upstream u_target_pre &&
> + git -C fetch_upstream checkout -b fetch_other &&
> + test_commit -C fetch_upstream u_other_pre &&
> + git fetch fetch_upstream &&
> + other_before=$(git rev-parse refs/remotes/fetch_upstream/fetch_other) &&
> + git -C fetch_upstream checkout fetch_target &&
> + test_commit -C fetch_upstream u_target_post &&
> + git -C fetch_upstream checkout fetch_other &&
> + test_commit -C fetch_upstream u_other_post &&
> + git checkout --track=fetch -b local_target fetch_upstream/fetch_target &&
> + test_cmp_rev refs/remotes/fetch_upstream/fetch_target HEAD &&
> + test "$(git rev-parse refs/remotes/fetch_upstream/fetch_other)" = "$other_before"
> +'
> +
> +test_expect_success 'checkout --track=fetch with bare remote name fetches only <remote>/HEAD target' '
> + git checkout main &&
> + git -C fetch_upstream checkout main &&
> + git remote set-head fetch_upstream main &&
> + git -C fetch_upstream checkout -b fetch_unrelated &&
> + test_commit -C fetch_upstream u_unrelated_pre &&
> + git fetch fetch_upstream fetch_unrelated &&
> + unrelated_before=$(git rev-parse refs/remotes/fetch_upstream/fetch_unrelated) &&
> + git -C fetch_upstream checkout main &&
> + test_commit -C fetch_upstream u_main_post &&
> + git -C fetch_upstream checkout fetch_unrelated &&
> + test_commit -C fetch_upstream u_unrelated_post &&
> + git checkout --track=fetch -b local_from_remote fetch_upstream &&
> + test_cmp_rev refs/remotes/fetch_upstream/main HEAD &&
> + test "$(git rev-parse refs/remotes/fetch_upstream/fetch_unrelated)" = "$unrelated_before"
> +'
> +
> +test_expect_success 'checkout --track=fetch aborts and does not create branch when no existing ref' '
> + git checkout main &&
> + test_might_fail git branch -D bogus &&
> + test_must_fail git checkout --track=fetch -b bogus fetch_upstream/does_not_exist &&
> + test_must_fail git rev-parse --verify refs/heads/bogus
> +'
> +
> +test_expect_success 'checkout --track=fetch warns and proceeds when fetch fails but ref exists' '
> + git checkout main &&
> + git -C fetch_upstream checkout -b fetch_offline &&
> + test_commit -C fetch_upstream u_offline &&
> + git fetch fetch_upstream fetch_offline &&
> + saved_url=$(git config remote.fetch_upstream.url) &&
> + test_when_finished "git config remote.fetch_upstream.url \"$saved_url\"" &&
> + git config remote.fetch_upstream.url ./does-not-exist &&
> + git checkout --track=fetch -b local_offline fetch_upstream/fetch_offline 2>err &&
> + test_grep "failed to fetch" err &&
> + test_cmp_rev refs/remotes/fetch_upstream/fetch_offline HEAD
> +'
> +
> +test_expect_success 'checkout --track=fetch resolves through configured fetch refspec' '
> + git checkout main &&
> + git remote add fetch_custom ./fetch_upstream &&
> + test_when_finished "git remote remove fetch_custom" &&
> + git config --replace-all remote.fetch_custom.fetch \
> + "+refs/heads/*:refs/remotes/custom-ns/*" &&
> + git -C fetch_upstream checkout -b fetch_refspec &&
> + test_commit -C fetch_upstream u_refspec &&
> + test_must_fail git rev-parse --verify refs/remotes/custom-ns/fetch_refspec &&
> + git checkout --track=fetch -b local_refspec custom-ns/fetch_refspec &&
> + test_cmp_rev refs/remotes/custom-ns/fetch_refspec HEAD
> +'
> +
> +test_expect_success 'checkout --track=fetch on namespace bare name follows <ns>/HEAD' '
> + git checkout main &&
> + git remote add fetch_ns ./fetch_upstream &&
> + test_when_finished "git remote remove fetch_ns" &&
> + test_when_finished "git update-ref -d refs/remotes/ns_alias/HEAD" &&
> + git config --replace-all remote.fetch_ns.fetch \
> + "+refs/heads/*:refs/remotes/ns_alias/*" &&
> + git fetch fetch_ns &&
> + git symbolic-ref refs/remotes/ns_alias/HEAD refs/remotes/ns_alias/main &&
> + git -C fetch_upstream checkout main &&
> + test_commit -C fetch_upstream u_ns_post &&
> + git checkout --track=fetch -b local_ns ns_alias &&
> + test_cmp_rev refs/remotes/ns_alias/main HEAD &&
> + test_cmp_config fetch_ns branch.local_ns.remote &&
> + test_cmp_config refs/heads/main branch.local_ns.merge
> +'
> +
> +test_expect_success '--track=fetch on bare hierarchical remote name follows <ns>/HEAD' '
> + git checkout main &&
> + git remote add nested/bare ./fetch_upstream &&
> + test_when_finished "git remote remove nested/bare" &&
> + test_when_finished "git update-ref -d refs/remotes/nested/bare/HEAD" &&
> + git fetch nested/bare &&
> + git symbolic-ref refs/remotes/nested/bare/HEAD \
> + refs/remotes/nested/bare/main &&
> + git -C fetch_upstream checkout main &&
> + test_commit -C fetch_upstream u_nested_bare_post &&
> + git checkout --track=fetch -b local_nested_bare nested/bare &&
> + test_cmp_rev refs/remotes/nested/bare/main HEAD
> +'
> +
> +test_expect_success 'checkout --track=fetch handles hierarchical remote name' '
> + git checkout main &&
> + git remote add nested/remote ./fetch_upstream &&
> + test_when_finished "git remote remove nested/remote" &&
> + git -C fetch_upstream checkout -b fetch_hier &&
> + test_commit -C fetch_upstream u_hier &&
> + test_must_fail git rev-parse --verify refs/remotes/nested/remote/fetch_hier &&
> + git checkout --track=fetch -b local_hier nested/remote/fetch_hier &&
> + test_cmp_rev refs/remotes/nested/remote/fetch_hier HEAD
> +'
> +
> +test_expect_success 'checkout --track=fetch dies on bare remote name with no <ns>/HEAD' '
> + git checkout main &&
> + git remote add fetch_nohead ./fetch_upstream &&
> + test_when_finished "git remote remove fetch_nohead" &&
> + test_might_fail git symbolic-ref -d refs/remotes/fetch_nohead/HEAD &&
> + test_must_fail git checkout --track=fetch -b local_nohead fetch_nohead 2>err &&
> + test_grep "refs/remotes/fetch_nohead/HEAD" err &&
> + test_grep "git remote set-head fetch_nohead --auto" err &&
> + test_must_fail git rev-parse --verify refs/heads/local_nohead
> +'
> +
> +test_expect_success 'checkout --track=fetch on bare unknown name does not suggest set-head' '
> + git checkout main &&
> + test_must_fail git rev-parse --verify refs/remotes/no_such_ns/HEAD &&
> + test_must_fail git config --get remote.no_such_ns.url &&
> + test_must_fail git checkout --track=fetch -b local_unknown no_such_ns 2>err &&
> + test_grep "no configured remote" err &&
> + test_grep ! "set-head" err &&
> + test_must_fail git rev-parse --verify refs/heads/local_unknown
> +'
> +
> +test_expect_success 'checkout --track=fetch rejects <ns>/HEAD pointing outside namespace' '
> + git checkout main &&
> + git remote add fetch_crossns ./fetch_upstream &&
> + test_when_finished "git remote remove fetch_crossns" &&
> + test_when_finished "git update-ref -d refs/remotes/fetch_crossns/HEAD" &&
> + git fetch fetch_crossns &&
> + git symbolic-ref refs/remotes/fetch_crossns/HEAD \
> + refs/remotes/fetch_upstream/u_main &&
> + test_must_fail git checkout --track=fetch -b local_crossns fetch_crossns 2>err &&
> + test_grep "refs/remotes/fetch_crossns/HEAD" err &&
> + test_must_fail git rev-parse --verify refs/heads/local_crossns
> +'
> +
> +test_expect_success 'checkout --track=fetch dies on ambiguous fetch refspec match' '
> + git checkout main &&
> + git remote add fetch_ambig_a ./fetch_upstream &&
> + git remote add fetch_ambig_b ./fetch_upstream &&
> + test_when_finished "git remote remove fetch_ambig_a" &&
> + test_when_finished "git remote remove fetch_ambig_b" &&
> + git config --replace-all remote.fetch_ambig_a.fetch \
> + "+refs/heads/*:refs/remotes/ambig_ns/*" &&
> + git config --replace-all remote.fetch_ambig_b.fetch \
> + "+refs/heads/*:refs/remotes/ambig_ns/*" &&
> + git -C fetch_upstream checkout -b fetch_ambig &&
> + test_commit -C fetch_upstream u_ambig &&
> + test_must_fail git checkout --track=fetch -b local_ambig ambig_ns/fetch_ambig 2>err &&
> + test_grep "fetch_ambig_a" err &&
> + test_grep "fetch_ambig_b" err &&
> + test_grep "tracking namespaces" err &&
> + test_must_fail git rev-parse --verify refs/heads/local_ambig
> +'
> +
> +test_expect_success 'checkout --track=fetch rejects invalid refname components' '
> + git checkout main &&
> + test_must_fail git checkout --track=fetch -b local_invalid "foo..bar" 2>err &&
> + test_grep "valid" err &&
> + test_must_fail git rev-parse --verify refs/heads/local_invalid
> +'
> +
> +test_expect_success 'checkout --track=fetch,inherit rejects invalid refname components' '
> + git checkout main &&
> + test_must_fail git checkout --track=fetch,inherit -b local_invalid \
> + "foo..bar" 2>err &&
> + test_grep "valid" err &&
> + test_must_fail git rev-parse --verify refs/heads/local_invalid
> +'
> +
> +test_expect_success 'checkout --track=inherit,direct is rejected' '
> + test_must_fail git checkout --track=inherit,direct -b bad fetch_upstream/fetch_new 2>err &&
> + test_grep "cannot combine" err
> +'
> +
> +test_expect_success 'checkout --track=direct,inherit is rejected' '
> + test_must_fail git checkout --track=direct,inherit -b bad fetch_upstream/fetch_new 2>err &&
> + test_grep "cannot combine" err
> +'
> +
> +test_expect_success 'checkout --track=fetch then --track=direct drops fetch (last-one-wins)' '
> + git checkout main &&
> + git -C fetch_upstream checkout -b fetch_lastwin &&
> + test_commit -C fetch_upstream u_lastwin &&
> + test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_lastwin &&
> + test_must_fail git checkout --track=fetch --track=direct \
> + -b local_lastwin fetch_upstream/fetch_lastwin &&
> + test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_lastwin
> +'
> +
> +test_expect_success 'checkout --track=fetch then --no-track drops fetch' '
> + git checkout main &&
> + git -C fetch_upstream checkout -b fetch_notrack &&
> + test_commit -C fetch_upstream u_notrack &&
> + test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_notrack &&
> + test_must_fail git checkout --track=fetch --no-track \
> + -b local_notrack fetch_upstream/fetch_notrack &&
> + test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_notrack
> +'
> +
> +test_expect_success 'checkout --track=fetch,inherit fetches remote-tracking start-point' '
> + git checkout main &&
> + git -C fetch_upstream checkout -b fetch_inherit &&
> + test_commit -C fetch_upstream u_inherit &&
> + test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_inherit &&
> + git checkout --track=fetch,inherit -b local_inherit \
> + fetch_upstream/fetch_inherit &&
> + test_cmp_rev refs/remotes/fetch_upstream/fetch_inherit HEAD
> +'
> +
> +test_expect_success 'checkout --track=fetch,inherit errors when start-point does not map to a remote' '
> + git checkout main &&
> + test_must_fail git checkout --track=fetch,inherit -b bad main 2>err &&
> + test_grep "no configured remote" err &&
> + test_must_fail git rev-parse --verify refs/heads/bad
> +'
> +
> +test_expect_success 'checkout --track=fetch on local start-point errors' '
> + git checkout main &&
> + test_must_fail git checkout --track=fetch -b bad main 2>err &&
> + test_grep "no configured remote" err &&
> + test_must_fail git rev-parse --verify refs/heads/bad
> +'
> +
> +test_expect_success 'checkout --track=bogus reports an error' '
> + git checkout main &&
> + test_must_fail git checkout --track=bogus -b bogus_branch fetch_upstream/fetch_new 2>err &&
> + test_grep "expects" err
> +'
> +
> +test_expect_success 'checkout -q --track=fetch silences the fetch output' '
> + git checkout main &&
> + git -C fetch_upstream checkout -b fetch_quiet &&
> + test_commit -C fetch_upstream u_quiet &&
> + test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_quiet &&
> + git checkout -q --track=fetch -b local_quiet \
> + fetch_upstream/fetch_quiet 2>err &&
> + test_grep ! "-> fetch_upstream/fetch_quiet" err &&
> + test_cmp_rev refs/remotes/fetch_upstream/fetch_quiet HEAD
> +'
> +
> +test_expect_success 'switch --track=fetch -c picks up branch created upstream after clone' '
> + git checkout main &&
> + git -C fetch_upstream checkout -b fetch_switch &&
> + test_commit -C fetch_upstream u_switch &&
> + test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_switch &&
> + git switch --track=fetch -c local_switch fetch_upstream/fetch_switch &&
> + test_cmp_rev refs/remotes/fetch_upstream/fetch_switch HEAD
> +'
> +
> test_done
^ permalink raw reply
* Re: [GSoC] [Blog] week 4: Improving the new git repo command
From: K Jayatheerth @ 2026-06-23 13:41 UTC (permalink / raw)
To: GIT Mailing-list, Justin Tobler, Lucas Seiki Oshiro
In-Reply-To: <CA+rGoLe+n314hrbKBSU61Hn=uVQN+OqOF5AVt2gPOityUUL_AA@mail.gmail.com>
Hi!
My Week 4 GSoC blog is live!
https://jayatheerth.com/blogs/gsoc/week-4-phase2
Feel free to give it a read and share any feedback ; )
Regards,
- K Jayatheerth
^ permalink raw reply
* Re: [PATCH/RFC 3/6] commit-reach: terminate merge-base walk when one paint side is exhausted
From: Derrick Stolee @ 2026-06-23 13:40 UTC (permalink / raw)
To: Kristofer Karlsson
Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <CAL71e4O7hKM=_M4K9hJE0MH9PdHUHxo7hyAbUSLbyk2wpiGxmw@mail.gmail.com>
On 6/22/2026 5:03 PM, Kristofer Karlsson wrote:
> On Mon, 22 Jun 2026 at 22:26, Derrick Stolee <stolee@gmail.com> wrote:
>>
>> I've used hyperfine [1] when doing specific performance tests
>> in the past. You can build Git before and after and have hyperfine
>> run the two modes and compare them:
>>
>> hyperfine --warmup=3 \
>> -n 'old' "~/git-old/bin-wrappers/git -C $repo merge-base $A $B" \
>> -n 'new' "~/git-new/bin-wrappers/git -C $repo merge-base $A $B"
>>
>> [1] https://github.com/sharkdp/hyperfine
>
> I can definitely use that, but I was thinking that the overhead
> of operations such as repo_parse_commit would be high relative
> to the overhead of the new paint_queue struct such that it would
> be hard to properly measure and that it would be easier if I could
> spread out that cost across multiple internal runs (which requires
> a custom binary of some sort), but perhaps it's enough to just
> show that there's no measurable regression here and then
> hyperfine is indeed the right fit. I'll start with that and see if I need
> to do anything more complex.
Unit-level performance is nice, but doesn't tell the whole story.
We typically focus on end-to-end performance numbers when possible.
Another way to do it would be to use trace2_region_enter() and
trace2_region_leave() markers and then pull the timing data out of
the trace2 event logs. It's more complicated and usually only
needed if we are struggling to reproduce the performance impact due
to external factors.
Thanks,
-Stolee
^ permalink raw reply
* Controle de votre adresse e-mail
From: Verification @ 2026-06-23 12:51 UTC (permalink / raw)
To: git
Bonjour,
Ceci est un message automatique de controle d'adresse e-mail.
Aucune action n'est requise de votre part, vous pouvez l'ignorer.
Pour ne plus recevoir ce type de message, repondez STOP a cet e-mail.
^ permalink raw reply
* [GSoC Blog] Week 3&4 : Improve Disk Space Recovery for Partial Clones
From: Siddharth Shrimali @ 2026-06-23 11:46 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Siddharth Asthana, Siddharth Shrimali
Hello everyone,
My latest blog post, covering weeks 3 and 4, is now live:
https://siddharth.shrimali.info/#post/6
I have combined both weeks into a single post.
Why such a consolidation?
You’ll have to dive into the blog to know ;)
Please feel free to review my work and share your feedback.
Always open to discussions! :)
Regards,
Siddharth Shrimali
^ permalink raw reply
* Re: [PATCH v4] log: improve --follow following renames for non-linear history
From: Miklos Vajna @ 2026-06-23 11:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqq1pdy4udg.fsf@gitster.g>
Hi Junio,
On Mon, Jun 22, 2026 at 05:44:43AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> went on in this patch would count), but as long as the resolution
> that is in my tree (as a part of 'seen') exactly matches what your
> update contains (meaning: rerere will do the same correct resolution
> when the topic gets merged to 'master' anyway) and the conflict is
> trivial to resolve by hand for others
I see, I'll keep that in mind for the future.
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH v4 0/4] history: add squash subcommand to fold a range
From: Harald Nordgren @ 2026-06-23 10:41 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Harald Nordgren via GitGitGadget, git
In-Reply-To: <ajkijomPo_kXSXul@pks.im>
Sorry about that.
I skipped the ancestry question because I felt the implementation
would get too complex and was hoping we could do without it. Probably
would have been better to be explicit about that instead of just
skipping it. I don't want to balloon the code more than necessary, but
I'll take a look at it now.
Harald
^ permalink raw reply
* Re: [PATCH/RFC 2/6] commit-reach: introduce struct paint_queue with per-side counters
From: Kristofer Karlsson @ 2026-06-23 10:13 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <8d07f5a9-82fa-4aed-b407-363e659f6851@gmail.com>
On Mon, 22 Jun 2026 at 22:23, Derrick Stolee <stolee@gmail.com> wrote:
>
> On 6/22/2026 3:14 PM, Kristofer Karlsson wrote:
> >
> > On Mon, 22 Jun 2026 at 20:10, Derrick Stolee <stolee@gmail.com> wrote:
> >>
> >> When possible, I like to try to make loops only have one terminating
> >> condition. Should we have paint_queue_get() return NULL when it sees
> >> this internal state condition?
> >
> > Possibly, but that would couple the paint_queue struct very tightly with
> > the usage. Not a problem in practice since it only has one call site, and
> > it's unlikely that we want to add more of them but it may feel more natural
> > to let the paint_queue purely have the queue semantics and counters,
> > and keep the halt condition within the function itself. I don't feel
> > super-strongly about this and can change it if needed, I will just need to
> > verify that nothing else gets complex as a result, I have not fully thought
> > through the effects.
>
> Hm. Interesting. The coupling is perhaps expected, because the data
> structure tracks counts that don't otherwise need to be tracked.
> Maybe the terminating condition method could be descriptively named
> to say why it would be completing.
>
I have been working on v2 locally and most of the changes landed
nicely and were clear improvements but there's one point I would
want to discuss a bit more.
For the termination conditions, I moved them into paint_queue_get()
as you suggested. The all-zero check was straightforward since it
only depends on the counters but the side-exhaustion check also
needs to know whether we have entered the finite-generation region,
so I pass last_gen (already a local in paint_down_to_common) as a
parameter:
static struct commit *paint_queue_get(struct paint_state *state,
timestamp_t last_gen)
Inside, the two conditions merge nicely under a shared guard:
if (!state->pending_merge_bases) {
if (!state->p1_count && !state->p2_count)
return NULL;
if (last_gen < GENERATION_NUMBER_INFINITY &&
(!state->p1_count || !state->p2_count))
return NULL;
}
Both conditions require pending_merge_bases == 0, so the nesting
felt natural. The first is "nothing non-stale left" (works in any
region). The second is "one side exhausted" (only in the finite
region where topological ordering holds).
I think passing in last_gen into paint_queue_get() feels _slightly_
awkward but not too bad in practice. However, we also have my
older (first) patch with the fast-exit if the caller only needs one
merge base -- that has a separate break that also could be folded
into paint_queue_get(). The messy part here is that we would need
to also pass the mb_flags parameter to paint_queue_get().
Perhaps we should just let this remain as-is for now and follow up
with _removing_ that optimization. I think the value of having it
is much diminished (but not fully gone) by the side-exhaust approach.
Additionally there's a correctness argument to be made -- perhaps
all callers _should_ care about multiple merge bases existing, and
instead bail out if it finds more than one. The only use case
where this matters today is "git merge-base A B" without --all.
Right now I am leaning towards simply passing in last_gen and
containing all of the halt conditions there
(except the old !FIND_ALL).
The nicest alternative I can think of is to let this part only
break when the queue is empty:
while ((commit = paint_queue_get(&state)))
and then adding a logical halt-section at the end of the while-loop
(where all the useful variables we need are already available), and
we could logically think of that as an optimization section, never
strictly needed for correctness.
> I just worry about the idea that a negative number (or an addition
> overflow) would create conditions for termination that we did not
> intend. That's why using the nonzero status as true/false combined
> with ands and ors is better.
Good point, I have addressed that locally too.
Thanks,
Kristofer
^ permalink raw reply
* [PATCH] gpg-interface: fix strip_cr_before_lf to only remove CR before LF
From: DSAntonio08 @ 2026-06-23 8:45 UTC (permalink / raw)
To: git; +Cc: DSAntonio08
The remove_cr_after() function was stripping all CR characters
unconditionally, even lone \r not followed by \n. This is incorrect
as only \r\n sequences (Windows line endings) should be normalized.
Fix the loop condition to skip \r only when immediately followed by
\n, and rename the function to strip_cr_before_lf to reflect its
actual behavior. Update both call sites and their comments accordingly.
---
gpg-interface.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index dafd5371fa..87ae6503da 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -989,17 +989,13 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
free(keyid_to_free);
return ret;
}
-
-/*
- * Strip CR from the line endings, in case we are on Windows.
- * NEEDSWORK: make it trim only CRs before LFs and rename
- */
-static void remove_cr_after(struct strbuf *buffer, size_t offset)
+/* Strip CR before LF from the line endings, in case we are on Windows. */
+static void strip_cr_before_lf(struct strbuf *buffer, size_t offset)
{
size_t i, j;
for (i = j = offset; i < buffer->len; i++) {
- if (buffer->buf[i] != '\r') {
+ if (buffer->buf[i] != '\r' || (i + 1 < buffer->len && buffer->buf[i + 1] != '\n')) {
if (i != j)
buffer->buf[j] = buffer->buf[i];
j++;
@@ -1049,8 +1045,8 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
}
strbuf_release(&gpg_status);
- /* Strip CR from the line endings, in case we are on Windows. */
- remove_cr_after(signature, bottom);
+ /* Strip CR before LF from the line endings, in case we are on Windows. */
+ strip_cr_before_lf(signature, bottom);
return 0;
}
@@ -1136,8 +1132,8 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
ssh_signature_filename.buf);
goto out;
}
- /* Strip CR from the line endings, in case we are on Windows. */
- remove_cr_after(signature, bottom);
+ /* Strip CR before LF from the line endings, in case we are on Windows. */
+ strip_cr_before_lf(signature, bottom);
out:
if (key_file)
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 3/3] connected: search promisor objects generically
From: Christian Couder @ 2026-06-23 7:45 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260622-pks-connected-generic-promisor-checks-v1-3-25eba2698202@pks.im>
On Mon, Jun 22, 2026 at 10:50 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> When performing connectivity checks we have to figure out whether any of
> the new objects are promisor objects, as we cannot assume full
> connectivity if so.
>
> This check is performed by iterating through all packfiles in the
> repository and searching each of them for the given object. Of course,
> this mechanism is quite specific to implementation details of the object
> database, as we assume that it uses packfiles in the first place.
>
> Refactor the logic so that we instead use `odb_for_each_object_ext()`
> with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`
> flag. This will yield all objects that have the exact object name and
> that are part of a promisor pack in a generic way.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> connected.c | 39 +++++++++++++++++++++++++--------------
> 1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/connected.c b/connected.c
> index 7e26976832..9a666f0cdf 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -11,6 +11,13 @@
> #include "packfile.h"
> #include "promisor-remote.h"
>
> +static int promised_object_cb(const struct object_id *oid UNUSED,
> + struct object_info *oi UNUSED,
> + void *payload UNUSED)
> +{
> + return 1;
> +}
> +
> /*
> * If we feed all the commits we want to verify to this command
> *
> @@ -46,6 +53,11 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> }
>
> if (repo_has_promisor_remote(the_repository)) {
> + struct odb_for_each_object_options opts = {
> + .flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,
> + .prefix_hex_len = the_repository->hash_algo->hexsz,
> + };
> +
> /*
> * For partial clones, we don't want to have to do a regular
> * connectivity check because we have to enumerate and exclude
> @@ -54,31 +66,30 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> * object is a promisor object. Instead, just make sure we
> * received, in a promisor packfile, the objects pointed to by
> * each wanted ref.
> - *
> - * Before checking for promisor packs, be sure we have the
> - * latest pack-files loaded into memory.
> */
> - odb_reprepare(the_repository->objects);
Like Junio, I am not sure it's correct to remove the
`odb_reprepare(the_repository->objects)` call.
I think it was added for good reasons in b739d971 (connected.c:
reprepare packs for corner cases, 2020-03-13) and I am not sure
odb_for_each_object_ext() is performing something similar.
At least the commit message should mention this change and explain a
bit why the reasons the call was added are not valid anymore.
> do {
> - struct packed_git *p;
> -
> - repo_for_each_pack(the_repository, p) {
> - if (!p->pack_promisor)
> - continue;
> - if (find_pack_entry_one(oid, p))
> - goto promisor_pack_found;
> + opts.prefix = oid;
> +
> + err = odb_for_each_object_ext(the_repository->objects,
> + NULL, promised_object_cb,
> + NULL, &opts);
> + if (err < 0)
> + break;
> + if (err > 0) {
> + err = 0;
> + continue;
> }
> +
> /*
> * Fallback to rev-list with oid and the rest of the
> * object IDs provided by fn.
> */
> goto no_promisor_pack_found;
> -promisor_pack_found:
> - ;
> } while ((oid = fn(cb_data)) != NULL);
> +
> if (opt->err_fd)
> close(opt->err_fd);
> - return 0;
> + return err;
> }
>
> no_promisor_pack_found:
These changes are difficult to understand as there are a number of
`goto`, `break`, `return`, etc involved.
I think it comes in the first place from check_connected() doing too
many things, and adding a preparatory commit to refactor it would
help.
For example the preparatory commit could move a lot of code from
check_connected() to the following new functions:
/*
* Returns:
* 1 = all wanted OIDs found in promisor packs: connected, done.
* 0 = at least one OID not found: caller must fall back to rev-list.
* <0 = error.
* On the fallback (0) return, *oid is left pointing at the first
* not-found OID so the rev-list path can resume the iteration.
*/
static int check_connected_promisor(oid_iterate_fn fn, void *cb_data,
const struct object_id **oid);
/*
* In a non-promisor repo, pass the first OID as `oid`.
* Otherwise pass the first not-found OID resumed from
* check_connected_promisor() as `oid`.
*/
static int check_connected_rev_list(oid_iterate_fn fn, void *cb_data,
struct check_connected_options *opt,
const struct object_id *oid);
^ permalink raw reply
* Re: [PATCH 0/2] branch/push: suggest intended form when remote/branch slip given
From: Harald Nordgren @ 2026-06-23 7:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Harald Nordgren via GitGitGadget, git
In-Reply-To: <xmqqpl1is2bm.fsf@gitster.g>
By slip it meant mistake, so you can call it 'hn/branch-push-mistake-advise'
Harald
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox