From 04949538203ba7aeb298a607280715d4f26dbe0a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 5 Mar 2026 15:34:46 -0800 Subject: [PATCH 01/61] sideband: mask control characters The output of `git clone` is a vital component for understanding what has happened when things go wrong. However, these logs are partially under the control of the remote server (via the "sideband", which typically contains what the remote `git pack-objects` process sends to `stderr`), and is currently not sanitized by Git. This makes Git susceptible to ANSI escape sequence injection (see CWE-150, https://cwe.mitre.org/data/definitions/150.html), which allows attackers to corrupt terminal state, to hide information, and even to insert characters into the input buffer (i.e. as if the user had typed those characters). To plug this vulnerability, disallow any control character in the sideband, replacing them instead with the common `^` (e.g. `^[` for `\x1b`, `^A` for `\x01`). There is likely a need for more fine-grained controls instead of using a "heavy hammer" like this, which will be introduced subsequently. Helped-by: Phillip Wood Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sideband.c | 17 +++++++++++++++-- t/t5409-colorize-remote-messages.sh | 12 ++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/sideband.c b/sideband.c index ea7c25211ef7e1..c1bbadccac682b 100644 --- a/sideband.c +++ b/sideband.c @@ -66,6 +66,19 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref list_config_item(list, prefix, keywords[i].keyword); } +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n) +{ + strbuf_grow(dest, n); + for (; n && *src; src++, n--) { + if (!iscntrl(*src) || *src == '\t' || *src == '\n') { + strbuf_addch(dest, *src); + } else { + strbuf_addch(dest, '^'); + strbuf_addch(dest, *src == 0x7f ? '?' : 0x40 + *src); + } + } +} + /* * Optionally highlight one keyword in remote output if it appears at the start * of the line. This should be called for a single line only, which is @@ -81,7 +94,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) int i; if (!want_color_stderr(use_sideband_colors())) { - strbuf_add(dest, src, n); + strbuf_add_sanitized(dest, src, n); return; } @@ -114,7 +127,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) } } - strbuf_add(dest, src, n); + strbuf_add_sanitized(dest, src, n); } diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index fa5de4500a4f50..aa5b57057148e0 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -98,4 +98,16 @@ test_expect_success 'fallback to color.ui' ' grep "error: error" decoded ' +test_expect_success 'disallow (color) control sequences in sideband' ' + write_script .git/color-me-surprised <<-\EOF && + printf "error: Have you \\033[31mread\\033[m this?\\n" >&2 + exec "$@" + EOF + test_config_global uploadPack.packObjectsHook ./color-me-surprised && + test_commit need-at-least-one-commit && + git clone --no-local . throw-away 2>stderr && + test_decode_color decoded && + test_grep ! RED decoded +' + test_done From 9ed1625a581a35d7ec2d851258cf4c7fc08c1ed7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 5 Mar 2026 15:34:47 -0800 Subject: [PATCH 02/61] sideband: introduce an "escape hatch" to allow control characters The preceding commit fixed the vulnerability whereas sideband messages (that are under the control of the remote server) could contain ANSI escape sequences that would be sent to the terminal verbatim. However, this fix may not be desirable under all circumstances, e.g. when remote servers deliberately add coloring to their messages to increase their urgency. To help with those use cases, give users a way to opt-out of the protections: `sideband.allowControlCharacters`. Suggested-by: brian m. carlson Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- Documentation/config.adoc | 2 ++ Documentation/config/sideband.adoc | 5 +++++ sideband.c | 10 ++++++++++ t/t5409-colorize-remote-messages.sh | 8 +++++++- 4 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 Documentation/config/sideband.adoc diff --git a/Documentation/config.adoc b/Documentation/config.adoc index 62eebe7c54501c..dcea3c0c15e2a9 100644 --- a/Documentation/config.adoc +++ b/Documentation/config.adoc @@ -523,6 +523,8 @@ include::config/sequencer.adoc[] include::config/showbranch.adoc[] +include::config/sideband.adoc[] + include::config/sparse.adoc[] include::config/splitindex.adoc[] diff --git a/Documentation/config/sideband.adoc b/Documentation/config/sideband.adoc new file mode 100644 index 00000000000000..3fb5045cd79581 --- /dev/null +++ b/Documentation/config/sideband.adoc @@ -0,0 +1,5 @@ +sideband.allowControlCharacters:: + By default, control characters that are delivered via the sideband + are masked, to prevent potentially unwanted ANSI escape sequences + from being sent to the terminal. Use this config setting to override + this behavior. diff --git a/sideband.c b/sideband.c index c1bbadccac682b..682f1cbbedb9b8 100644 --- a/sideband.c +++ b/sideband.c @@ -26,6 +26,8 @@ static struct keyword_entry keywords[] = { { "error", GIT_COLOR_BOLD_RED }, }; +static int allow_control_characters; + /* Returns a color setting (GIT_COLOR_NEVER, etc). */ static enum git_colorbool use_sideband_colors(void) { @@ -39,6 +41,9 @@ static enum git_colorbool use_sideband_colors(void) if (use_sideband_colors_cached != GIT_COLOR_UNKNOWN) return use_sideband_colors_cached; + repo_config_get_bool(the_repository, "sideband.allowcontrolcharacters", + &allow_control_characters); + if (!repo_config_get_string_tmp(the_repository, key, &value)) use_sideband_colors_cached = git_config_colorbool(key, value); else if (!repo_config_get_string_tmp(the_repository, "color.ui", &value)) @@ -68,6 +73,11 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n) { + if (allow_control_characters) { + strbuf_add(dest, src, n); + return; + } + strbuf_grow(dest, n); for (; n && *src; src++, n--) { if (!iscntrl(*src) || *src == '\t' || *src == '\n') { diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index aa5b57057148e0..9caee9a07f1556 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -105,9 +105,15 @@ test_expect_success 'disallow (color) control sequences in sideband' ' EOF test_config_global uploadPack.packObjectsHook ./color-me-surprised && test_commit need-at-least-one-commit && + git clone --no-local . throw-away 2>stderr && test_decode_color decoded && - test_grep ! RED decoded + test_grep ! RED decoded && + + rm -rf throw-away && + git -c sideband.allowControlCharacters clone --no-local . throw-away 2>stderr && + test_decode_color decoded && + test_grep RED decoded ' test_done From 12f0fda905b4af3a15c125f96808e49ddbe39742 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 5 Mar 2026 15:34:48 -0800 Subject: [PATCH 03/61] sideband: do allow ANSI color sequences by default The preceding two commits introduced special handling of the sideband channel to neutralize ANSI escape sequences before sending the payload to the terminal, and `sideband.allowControlCharacters` to override that behavior. However, as reported by brian m. carlson, some `pre-receive` hooks that are actively used in practice want to color their messages and therefore rely on the fact that Git passes them through to the terminal, even though they have no way to determine whether the receiving side can actually handle Escape sequences (think e.g. about the practice recommended by Git that third-party applications wishing to use Git functionality parse the output of Git commands). In contrast to other ANSI escape sequences, it is highly unlikely that coloring sequences can be essential tools in attack vectors that mislead Git users e.g. by hiding crucial information. Therefore we can have both: Continue to allow ANSI coloring sequences to be passed to the terminal by default, and neutralize all other ANSI Escape sequences. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- Documentation/config/sideband.adoc | 18 ++++++-- sideband.c | 66 +++++++++++++++++++++++++++-- t/t5409-colorize-remote-messages.sh | 16 ++++++- 3 files changed, 91 insertions(+), 9 deletions(-) diff --git a/Documentation/config/sideband.adoc b/Documentation/config/sideband.adoc index 3fb5045cd79581..b55c73726fe2c7 100644 --- a/Documentation/config/sideband.adoc +++ b/Documentation/config/sideband.adoc @@ -1,5 +1,17 @@ sideband.allowControlCharacters:: By default, control characters that are delivered via the sideband - are masked, to prevent potentially unwanted ANSI escape sequences - from being sent to the terminal. Use this config setting to override - this behavior. + are masked, except ANSI color sequences. This prevents potentially + unwanted ANSI escape sequences from being sent to the terminal. Use + this config setting to override this behavior: ++ +-- + `default`:: + `color`:: + Allow ANSI color sequences, line feeds and horizontal tabs, + but mask all other control characters. This is the default. + `false`:: + Mask all control characters other than line feeds and + horizontal tabs. + `true`:: + Allow all control characters to be sent to the terminal. +-- diff --git a/sideband.c b/sideband.c index 682f1cbbedb9b8..eeba6fa2ca8dd6 100644 --- a/sideband.c +++ b/sideband.c @@ -26,7 +26,12 @@ static struct keyword_entry keywords[] = { { "error", GIT_COLOR_BOLD_RED }, }; -static int allow_control_characters; +static enum { + ALLOW_NO_CONTROL_CHARACTERS = 0, + ALLOW_ANSI_COLOR_SEQUENCES = 1<<0, + ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES, + ALLOW_ALL_CONTROL_CHARACTERS = 1<<1, +} allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES; /* Returns a color setting (GIT_COLOR_NEVER, etc). */ static enum git_colorbool use_sideband_colors(void) @@ -41,8 +46,26 @@ static enum git_colorbool use_sideband_colors(void) if (use_sideband_colors_cached != GIT_COLOR_UNKNOWN) return use_sideband_colors_cached; - repo_config_get_bool(the_repository, "sideband.allowcontrolcharacters", - &allow_control_characters); + switch (repo_config_get_maybe_bool(the_repository, "sideband.allowcontrolcharacters", &i)) { + case 0: /* Boolean value */ + allow_control_characters = i ? ALLOW_ALL_CONTROL_CHARACTERS : + ALLOW_NO_CONTROL_CHARACTERS; + break; + case -1: /* non-Boolean value */ + if (repo_config_get_string_tmp(the_repository, "sideband.allowcontrolcharacters", + &value)) + ; /* huh? `get_maybe_bool()` returned -1 */ + else if (!strcmp(value, "default")) + allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES; + else if (!strcmp(value, "color")) + allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES; + else + warning(_("unrecognized value for `sideband." + "allowControlCharacters`: '%s'"), value); + break; + default: + break; /* not configured */ + } if (!repo_config_get_string_tmp(the_repository, key, &value)) use_sideband_colors_cached = git_config_colorbool(key, value); @@ -71,9 +94,41 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref list_config_item(list, prefix, keywords[i].keyword); } +static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int n) +{ + int i; + + /* + * Valid ANSI color sequences are of the form + * + * ESC [ [ [; ]*] m + * + * These are part of the Select Graphic Rendition sequences which + * contain more than just color sequences, for more details see + * https://en.wikipedia.org/wiki/ANSI_escape_code#SGR. + */ + + if (allow_control_characters != ALLOW_ANSI_COLOR_SEQUENCES || + n < 3 || src[0] != '\x1b' || src[1] != '[') + return 0; + + for (i = 2; i < n; i++) { + if (src[i] == 'm') { + strbuf_add(dest, src, i + 1); + return i; + } + if (!isdigit(src[i]) && src[i] != ';') + break; + } + + return 0; +} + static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n) { - if (allow_control_characters) { + int i; + + if (allow_control_characters == ALLOW_ALL_CONTROL_CHARACTERS) { strbuf_add(dest, src, n); return; } @@ -82,6 +137,9 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n) for (; n && *src; src++, n--) { if (!iscntrl(*src) || *src == '\t' || *src == '\n') { strbuf_addch(dest, *src); + } else if ((i = handle_ansi_color_sequence(dest, src, n))) { + src += i; + n -= i; } else { strbuf_addch(dest, '^'); strbuf_addch(dest, *src == 0x7f ? '?' : 0x40 + *src); diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index 9caee9a07f1556..e5092d3b426cd3 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -100,7 +100,7 @@ test_expect_success 'fallback to color.ui' ' test_expect_success 'disallow (color) control sequences in sideband' ' write_script .git/color-me-surprised <<-\EOF && - printf "error: Have you \\033[31mread\\033[m this?\\n" >&2 + printf "error: Have you \\033[31mread\\033[m this?\\a\\n" >&2 exec "$@" EOF test_config_global uploadPack.packObjectsHook ./color-me-surprised && @@ -108,12 +108,24 @@ test_expect_success 'disallow (color) control sequences in sideband' ' git clone --no-local . throw-away 2>stderr && test_decode_color decoded && + test_grep RED decoded && + test_grep "\\^G" stderr && + tr -dc "\\007" actual && + test_must_be_empty actual && + + rm -rf throw-away && + git -c sideband.allowControlCharacters=false \ + clone --no-local . throw-away 2>stderr && + test_decode_color decoded && test_grep ! RED decoded && + test_grep "\\^G" stderr && rm -rf throw-away && git -c sideband.allowControlCharacters clone --no-local . throw-away 2>stderr && test_decode_color decoded && - test_grep RED decoded + test_grep RED decoded && + tr -dc "\\007" actual && + test_file_not_empty actual ' test_done From 128914438a0d2d55ae34314a0881f55a797024d5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 5 Mar 2026 15:34:49 -0800 Subject: [PATCH 04/61] sideband: add options to allow more control sequences to be passed through Even though control sequences that erase characters are quite juicy for attack scenarios, where attackers are eager to hide traces of suspicious activities, during the review of the side band sanitizing patch series concerns were raised that there might be some legimitate scenarios where Git server's `pre-receive` hooks use those sequences in a benign way. Control sequences to move the cursor can likewise be used to hide tracks by overwriting characters, and have been equally pointed out as having legitimate users. Let's add options to let users opt into passing through those ANSI Escape sequences: `sideband.allowControlCharacters` now supports also `cursor` and `erase`, and it parses the value as a comma-separated list. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- Documentation/config/sideband.adoc | 9 ++- sideband.c | 91 ++++++++++++++++++++++++----- t/t5409-colorize-remote-messages.sh | 38 ++++++++++++ 3 files changed, 123 insertions(+), 15 deletions(-) diff --git a/Documentation/config/sideband.adoc b/Documentation/config/sideband.adoc index b55c73726fe2c7..2bf04262840b02 100644 --- a/Documentation/config/sideband.adoc +++ b/Documentation/config/sideband.adoc @@ -2,13 +2,20 @@ sideband.allowControlCharacters:: By default, control characters that are delivered via the sideband are masked, except ANSI color sequences. This prevents potentially unwanted ANSI escape sequences from being sent to the terminal. Use - this config setting to override this behavior: + this config setting to override this behavior (the value can be + a comma-separated list of the following keywords): + -- `default`:: `color`:: Allow ANSI color sequences, line feeds and horizontal tabs, but mask all other control characters. This is the default. + `cursor:`: + Allow control sequences that move the cursor. This is + disabled by default. + `erase`:: + Allow control sequences that erase charactrs. This is + disabled by default. `false`:: Mask all control characters other than line feeds and horizontal tabs. diff --git a/sideband.c b/sideband.c index eeba6fa2ca8dd6..0b420ca3193888 100644 --- a/sideband.c +++ b/sideband.c @@ -29,9 +29,43 @@ static struct keyword_entry keywords[] = { static enum { ALLOW_NO_CONTROL_CHARACTERS = 0, ALLOW_ANSI_COLOR_SEQUENCES = 1<<0, + ALLOW_ANSI_CURSOR_MOVEMENTS = 1<<1, + ALLOW_ANSI_ERASE = 1<<2, ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES, - ALLOW_ALL_CONTROL_CHARACTERS = 1<<1, -} allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES; + ALLOW_ALL_CONTROL_CHARACTERS = 1<<3, +} allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES; + +static inline int skip_prefix_in_csv(const char *value, const char *prefix, + const char **out) +{ + if (!skip_prefix(value, prefix, &value) || + (*value && *value != ',')) + return 0; + *out = value + !!*value; + return 1; +} + +static void parse_allow_control_characters(const char *value) +{ + allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS; + while (*value) { + if (skip_prefix_in_csv(value, "default", &value)) + allow_control_characters |= ALLOW_DEFAULT_ANSI_SEQUENCES; + else if (skip_prefix_in_csv(value, "color", &value)) + allow_control_characters |= ALLOW_ANSI_COLOR_SEQUENCES; + else if (skip_prefix_in_csv(value, "cursor", &value)) + allow_control_characters |= ALLOW_ANSI_CURSOR_MOVEMENTS; + else if (skip_prefix_in_csv(value, "erase", &value)) + allow_control_characters |= ALLOW_ANSI_ERASE; + else if (skip_prefix_in_csv(value, "true", &value)) + allow_control_characters = ALLOW_ALL_CONTROL_CHARACTERS; + else if (skip_prefix_in_csv(value, "false", &value)) + allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS; + else + warning(_("unrecognized value for `sideband." + "allowControlCharacters`: '%s'"), value); + } +} /* Returns a color setting (GIT_COLOR_NEVER, etc). */ static enum git_colorbool use_sideband_colors(void) @@ -55,13 +89,8 @@ static enum git_colorbool use_sideband_colors(void) if (repo_config_get_string_tmp(the_repository, "sideband.allowcontrolcharacters", &value)) ; /* huh? `get_maybe_bool()` returned -1 */ - else if (!strcmp(value, "default")) - allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES; - else if (!strcmp(value, "color")) - allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES; else - warning(_("unrecognized value for `sideband." - "allowControlCharacters`: '%s'"), value); + parse_allow_control_characters(value); break; default: break; /* not configured */ @@ -94,7 +123,7 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref list_config_item(list, prefix, keywords[i].keyword); } -static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int n) +static int handle_ansi_sequence(struct strbuf *dest, const char *src, int n) { int i; @@ -106,14 +135,47 @@ static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int * These are part of the Select Graphic Rendition sequences which * contain more than just color sequences, for more details see * https://en.wikipedia.org/wiki/ANSI_escape_code#SGR. + * + * The cursor movement sequences are: + * + * ESC [ n A - Cursor up n lines (CUU) + * ESC [ n B - Cursor down n lines (CUD) + * ESC [ n C - Cursor forward n columns (CUF) + * ESC [ n D - Cursor back n columns (CUB) + * ESC [ n E - Cursor next line, beginning (CNL) + * ESC [ n F - Cursor previous line, beginning (CPL) + * ESC [ n G - Cursor to column n (CHA) + * ESC [ n ; m H - Cursor position (row n, col m) (CUP) + * ESC [ n ; m f - Same as H (HVP) + * + * The sequences to erase characters are: + * + * + * ESC [ 0 J - Clear from cursor to end of screen (ED) + * ESC [ 1 J - Clear from cursor to beginning of screen (ED) + * ESC [ 2 J - Clear entire screen (ED) + * ESC [ 3 J - Clear entire screen + scrollback (ED) - xterm extension + * ESC [ 0 K - Clear from cursor to end of line (EL) + * ESC [ 1 K - Clear from cursor to beginning of line (EL) + * ESC [ 2 K - Clear entire line (EL) + * ESC [ n M - Delete n lines (DL) + * ESC [ n P - Delete n characters (DCH) + * ESC [ n X - Erase n characters (ECH) + * + * For a comprehensive list of common ANSI Escape sequences, see + * https://www.xfree86.org/current/ctlseqs.html */ - if (allow_control_characters != ALLOW_ANSI_COLOR_SEQUENCES || - n < 3 || src[0] != '\x1b' || src[1] != '[') + if (n < 3 || src[0] != '\x1b' || src[1] != '[') return 0; for (i = 2; i < n; i++) { - if (src[i] == 'm') { + if (((allow_control_characters & ALLOW_ANSI_COLOR_SEQUENCES) && + src[i] == 'm') || + ((allow_control_characters & ALLOW_ANSI_CURSOR_MOVEMENTS) && + strchr("ABCDEFGHf", src[i])) || + ((allow_control_characters & ALLOW_ANSI_ERASE) && + strchr("JKMPX", src[i]))) { strbuf_add(dest, src, i + 1); return i; } @@ -128,7 +190,7 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n) { int i; - if (allow_control_characters == ALLOW_ALL_CONTROL_CHARACTERS) { + if ((allow_control_characters & ALLOW_ALL_CONTROL_CHARACTERS)) { strbuf_add(dest, src, n); return; } @@ -137,7 +199,8 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n) for (; n && *src; src++, n--) { if (!iscntrl(*src) || *src == '\t' || *src == '\n') { strbuf_addch(dest, *src); - } else if ((i = handle_ansi_color_sequence(dest, src, n))) { + } else if (allow_control_characters != ALLOW_NO_CONTROL_CHARACTERS && + (i = handle_ansi_sequence(dest, src, n))) { src += i; n -= i; } else { diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index e5092d3b426cd3..896e790bf955cd 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -128,4 +128,42 @@ test_expect_success 'disallow (color) control sequences in sideband' ' test_file_not_empty actual ' +test_decode_csi() { + awk '{ + while (match($0, /\033/) != 0) { + printf "%sCSI ", substr($0, 1, RSTART-1); + $0 = substr($0, RSTART + RLENGTH, length($0) - RSTART - RLENGTH + 1); + } + print + }' +} + +test_expect_success 'control sequences in sideband allowed by default' ' + write_script .git/color-me-surprised <<-\EOF && + printf "error: \\033[31mcolor\\033[m\\033[Goverwrite\\033[Gerase\\033[K\\033?25l\\n" >&2 + exec "$@" + EOF + test_config_global uploadPack.packObjectsHook ./color-me-surprised && + test_commit need-at-least-one-commit-at-least && + + rm -rf throw-away && + git clone --no-local . throw-away 2>stderr && + test_decode_color color-decoded && + test_decode_csi decoded && + test_grep ! "CSI \\[K" decoded && + test_grep ! "CSI \\[G" decoded && + test_grep "\\^\\[?25l" decoded && + + rm -rf throw-away && + git -c sideband.allowControlCharacters=erase,cursor,color \ + clone --no-local . throw-away 2>stderr && + test_decode_color color-decoded && + test_decode_csi decoded && + test_grep "RED" decoded && + test_grep "CSI \\[K" decoded && + test_grep "CSI \\[G" decoded && + test_grep ! "\\^\\[\\[K" decoded && + test_grep ! "\\^\\[\\[G" decoded +' + test_done From 602c83f0efed46c2e86a36273673bf8776ded04e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 5 Mar 2026 15:34:50 -0800 Subject: [PATCH 05/61] sideband: offer to configure sanitizing on a per-URL basis The main objection against sanitizing the sideband that was raised during the review of the sideband sanitizing patches, first on the git-security mailing list, then on the public mailing list, was that there are some setups where server-side `pre-receive` hooks want to error out, giving colorful messages to the users on the client side (if they are not redirecting the output into a file, that is). To avoid breaking such setups, the default chosen by the sideband sanitizing patches is to pass through ANSI color sequences. Still, there might be some use case out there where that is not enough. Therefore the `sideband.allowControlCharacters` config setting allows for configuring levels of sanitizing. As Junio Hamano pointed out, to keep users safe by default, we need to be able to scope this to some servers because while a user may trust their company's Git server, the same might not apply to other Git servers. To allow for this, let's imitate the way `http..*` offers to scope config settings to certain URLs, by letting users override the `sideband.allowControlCharacters` setting via `sideband..allowControlCharacters`. Suggested-by: Junio Hamano Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- Documentation/config/sideband.adoc | 4 ++ sideband.c | 81 ++++++++++++++++++++--------- sideband.h | 14 +++++ t/t5409-colorize-remote-messages.sh | 24 +++++++++ transport.c | 3 ++ 5 files changed, 102 insertions(+), 24 deletions(-) diff --git a/Documentation/config/sideband.adoc b/Documentation/config/sideband.adoc index 2bf04262840b02..32088bbf2f0a40 100644 --- a/Documentation/config/sideband.adoc +++ b/Documentation/config/sideband.adoc @@ -22,3 +22,7 @@ sideband.allowControlCharacters:: `true`:: Allow all control characters to be sent to the terminal. -- + +sideband..*:: + Apply the `sideband.*` option selectively to specific URLs. The + same URL matching logic applies as for `http..*` settings. diff --git a/sideband.c b/sideband.c index 0b420ca3193888..a90db9e2880cba 100644 --- a/sideband.c +++ b/sideband.c @@ -10,6 +10,7 @@ #include "help.h" #include "pkt-line.h" #include "write-or-die.h" +#include "urlmatch.h" struct keyword_entry { /* @@ -27,13 +28,14 @@ static struct keyword_entry keywords[] = { }; static enum { - ALLOW_NO_CONTROL_CHARACTERS = 0, - ALLOW_ANSI_COLOR_SEQUENCES = 1<<0, - ALLOW_ANSI_CURSOR_MOVEMENTS = 1<<1, - ALLOW_ANSI_ERASE = 1<<2, - ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES, - ALLOW_ALL_CONTROL_CHARACTERS = 1<<3, -} allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES; + ALLOW_CONTROL_SEQUENCES_UNSET = -1, + ALLOW_NO_CONTROL_CHARACTERS = 0, + ALLOW_ANSI_COLOR_SEQUENCES = 1<<0, + ALLOW_ANSI_CURSOR_MOVEMENTS = 1<<1, + ALLOW_ANSI_ERASE = 1<<2, + ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES, + ALLOW_ALL_CONTROL_CHARACTERS = 1<<3, +} allow_control_characters = ALLOW_CONTROL_SEQUENCES_UNSET; static inline int skip_prefix_in_csv(const char *value, const char *prefix, const char **out) @@ -45,8 +47,19 @@ static inline int skip_prefix_in_csv(const char *value, const char *prefix, return 1; } -static void parse_allow_control_characters(const char *value) +int sideband_allow_control_characters_config(const char *var, const char *value) { + switch (git_parse_maybe_bool(value)) { + case 0: + allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS; + return 0; + case 1: + allow_control_characters = ALLOW_ALL_CONTROL_CHARACTERS; + return 0; + default: + break; + } + allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS; while (*value) { if (skip_prefix_in_csv(value, "default", &value)) @@ -62,9 +75,37 @@ static void parse_allow_control_characters(const char *value) else if (skip_prefix_in_csv(value, "false", &value)) allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS; else - warning(_("unrecognized value for `sideband." - "allowControlCharacters`: '%s'"), value); + warning(_("unrecognized value for '%s': '%s'"), var, value); } + return 0; +} + +static int sideband_config_callback(const char *var, const char *value, + const struct config_context *ctx UNUSED, + void *data UNUSED) +{ + if (!strcmp(var, "sideband.allowcontrolcharacters")) + return sideband_allow_control_characters_config(var, value); + + return 0; +} + +void sideband_apply_url_config(const char *url) +{ + struct urlmatch_config config = URLMATCH_CONFIG_INIT; + char *normalized_url; + + if (!url) + BUG("must not call sideband_apply_url_config(NULL)"); + + config.section = "sideband"; + config.collect_fn = sideband_config_callback; + + normalized_url = url_normalize(url, &config.url); + repo_config(the_repository, urlmatch_config_entry, &config); + free(normalized_url); + string_list_clear(&config.vars, 1); + urlmatch_config_release(&config); } /* Returns a color setting (GIT_COLOR_NEVER, etc). */ @@ -80,20 +121,12 @@ static enum git_colorbool use_sideband_colors(void) if (use_sideband_colors_cached != GIT_COLOR_UNKNOWN) return use_sideband_colors_cached; - switch (repo_config_get_maybe_bool(the_repository, "sideband.allowcontrolcharacters", &i)) { - case 0: /* Boolean value */ - allow_control_characters = i ? ALLOW_ALL_CONTROL_CHARACTERS : - ALLOW_NO_CONTROL_CHARACTERS; - break; - case -1: /* non-Boolean value */ - if (repo_config_get_string_tmp(the_repository, "sideband.allowcontrolcharacters", - &value)) - ; /* huh? `get_maybe_bool()` returned -1 */ - else - parse_allow_control_characters(value); - break; - default: - break; /* not configured */ + if (allow_control_characters == ALLOW_CONTROL_SEQUENCES_UNSET) { + if (!repo_config_get_value(the_repository, "sideband.allowcontrolcharacters", &value)) + sideband_allow_control_characters_config("sideband.allowcontrolcharacters", value); + + if (allow_control_characters == ALLOW_CONTROL_SEQUENCES_UNSET) + allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES; } if (!repo_config_get_string_tmp(the_repository, key, &value)) diff --git a/sideband.h b/sideband.h index 5a25331be55d30..d15fa4015fa0a3 100644 --- a/sideband.h +++ b/sideband.h @@ -30,4 +30,18 @@ int demultiplex_sideband(const char *me, int status, void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max); +/* + * Apply sideband configuration for the given URL. This should be called + * when a transport is created to allow URL-specific configuration of + * sideband behavior (e.g., sideband..allowControlCharacters). + */ +void sideband_apply_url_config(const char *url); + +/* + * Parse and set the sideband allow control characters configuration. + * The var parameter should be the key name (without section prefix). + * Returns 0 if the variable was recognized and handled, non-zero otherwise. + */ +int sideband_allow_control_characters_config(const char *var, const char *value); + #endif diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index 896e790bf955cd..3010913bb113e4 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -166,4 +166,28 @@ test_expect_success 'control sequences in sideband allowed by default' ' test_grep ! "\\^\\[\\[G" decoded ' +test_expect_success 'allow all control sequences for a specific URL' ' + write_script .git/eraser <<-\EOF && + printf "error: Ohai!\\r\\033[K" >&2 + exec "$@" + EOF + test_config_global uploadPack.packObjectsHook ./eraser && + test_commit one-more-please && + + rm -rf throw-away && + git clone --no-local . throw-away 2>stderr && + test_decode_color color-decoded && + test_decode_csi decoded && + test_grep ! "CSI \\[K" decoded && + test_grep "\\^\\[\\[K" decoded && + + rm -rf throw-away && + git -c "sideband.file://.allowControlCharacters=true" \ + clone --no-local "file://$PWD" throw-away 2>stderr && + test_decode_color color-decoded && + test_decode_csi decoded && + test_grep "CSI \\[K" decoded && + test_grep ! "\\^\\[\\[K" decoded +' + test_done diff --git a/transport.c b/transport.c index c7f06a7382e605..1602065953a54e 100644 --- a/transport.c +++ b/transport.c @@ -29,6 +29,7 @@ #include "object-name.h" #include "color.h" #include "bundle-uri.h" +#include "sideband.h" static enum git_colorbool transport_use_color = GIT_COLOR_UNKNOWN; static char transport_colors[][COLOR_MAXLEN] = { @@ -1245,6 +1246,8 @@ struct transport *transport_get(struct remote *remote, const char *url) ret->hash_algo = &hash_algos[GIT_HASH_SHA1_LEGACY]; + sideband_apply_url_config(ret->url); + return ret; } From 826cc4722088a02d0ae240c1267b5b74d476b153 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 5 Mar 2026 15:34:51 -0800 Subject: [PATCH 06/61] sideband: drop 'default' configuration The topic so far allows users to tweak the configuration variable sideband.allowControlCharacters to override the hardcoded default, but among which there is the value called 'default'. The plan [*] of the series is to loosen the setting by a later commit in the series and schedule it to tighten at the Git 3.0 boundary for end users, at which point, the meaning of this 'default' value will change. Which is a dubious design. A user expresses their preference by setting configuration variable in order to guard against sudden change brought in by changes to the hardcoded default behaviour, and letting them set it to 'default' that will change at the Git 3.0 boundary defeats its purpose. If a user wants to say "I am easy and can go with whatever hardcoded default Git implementors choose for me", they simply leave the configuration variable unspecified. Let's remove it from the state before Git 3.0 so that those users who set it to 'default' will not see the behaviour changed under their feet all of sudden. Signed-off-by: Junio C Hamano --- Documentation/config/sideband.adoc | 1 - sideband.c | 6 ++---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/Documentation/config/sideband.adoc b/Documentation/config/sideband.adoc index 32088bbf2f0a40..96fade7f5fee39 100644 --- a/Documentation/config/sideband.adoc +++ b/Documentation/config/sideband.adoc @@ -6,7 +6,6 @@ sideband.allowControlCharacters:: a comma-separated list of the following keywords): + -- - `default`:: `color`:: Allow ANSI color sequences, line feeds and horizontal tabs, but mask all other control characters. This is the default. diff --git a/sideband.c b/sideband.c index a90db9e2880cba..04282a568edd90 100644 --- a/sideband.c +++ b/sideband.c @@ -33,8 +33,8 @@ static enum { ALLOW_ANSI_COLOR_SEQUENCES = 1<<0, ALLOW_ANSI_CURSOR_MOVEMENTS = 1<<1, ALLOW_ANSI_ERASE = 1<<2, - ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES, ALLOW_ALL_CONTROL_CHARACTERS = 1<<3, + ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES } allow_control_characters = ALLOW_CONTROL_SEQUENCES_UNSET; static inline int skip_prefix_in_csv(const char *value, const char *prefix, @@ -62,9 +62,7 @@ int sideband_allow_control_characters_config(const char *var, const char *value) allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS; while (*value) { - if (skip_prefix_in_csv(value, "default", &value)) - allow_control_characters |= ALLOW_DEFAULT_ANSI_SEQUENCES; - else if (skip_prefix_in_csv(value, "color", &value)) + if (skip_prefix_in_csv(value, "color", &value)) allow_control_characters |= ALLOW_ANSI_COLOR_SEQUENCES; else if (skip_prefix_in_csv(value, "cursor", &value)) allow_control_characters |= ALLOW_ANSI_CURSOR_MOVEMENTS; From 1d980196adfd79ae0936e681e8d98c57d9900785 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= Date: Sat, 4 Apr 2026 17:12:44 +0000 Subject: [PATCH 07/61] doc: convert git-difftool manual page to synopsis style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * convert commands to synopsis style * use __ for arguments * fix conditional text to sentence limits Signed-off-by: Jean-Noël Avila Signed-off-by: Junio C Hamano --- Documentation/config/difftool.adoc | 24 ++++----- Documentation/config/mergetool.adoc | 8 +-- Documentation/git-difftool.adoc | 80 ++++++++++++++--------------- 3 files changed, 56 insertions(+), 56 deletions(-) diff --git a/Documentation/config/difftool.adoc b/Documentation/config/difftool.adoc index 4f7d40ce242b78..1b8d48381357aa 100644 --- a/Documentation/config/difftool.adoc +++ b/Documentation/config/difftool.adoc @@ -1,43 +1,43 @@ -diff.tool:: +`diff.tool`:: Controls which diff tool is used by linkgit:git-difftool[1]. This variable overrides the value configured in `merge.tool`. The list below shows the valid built-in values. Any other value is treated as a custom diff tool and requires - that a corresponding difftool..cmd variable is defined. + that a corresponding `difftool..cmd` variable is defined. -diff.guitool:: +`diff.guitool`:: Controls which diff tool is used by linkgit:git-difftool[1] when - the -g/--gui flag is specified. This variable overrides the value + the `-g`/`--gui` flag is specified. This variable overrides the value configured in `merge.guitool`. The list below shows the valid built-in values. Any other value is treated as a custom diff tool - and requires that a corresponding difftool..cmd variable + and requires that a corresponding `difftool..cmd` variable is defined. include::{build_dir}/mergetools-diff.adoc[] -difftool..cmd:: +`difftool..cmd`:: Specify the command to invoke the specified diff tool. The specified command is evaluated in shell with the following - variables available: 'LOCAL' is set to the name of the temporary - file containing the contents of the diff pre-image and 'REMOTE' + variables available: `LOCAL` is set to the name of the temporary + file containing the contents of the diff pre-image and `REMOTE` is set to the name of the temporary file containing the contents of the diff post-image. + See the `--tool=` option in linkgit:git-difftool[1] for more details. -difftool..path:: +`difftool..path`:: Override the path for the given tool. This is useful in case your tool is not in the PATH. -difftool.trustExitCode:: +`difftool.trustExitCode`:: Exit difftool if the invoked diff tool returns a non-zero exit status. + See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details. -difftool.prompt:: +`difftool.prompt`:: Prompt before each invocation of the diff tool. -difftool.guiDefault:: +`difftool.guiDefault`:: Set `true` to use the `diff.guitool` by default (equivalent to specifying the `--gui` argument), or `auto` to select `diff.guitool` or `diff.tool` depending on the presence of a `DISPLAY` environment variable value. The diff --git a/Documentation/config/mergetool.adoc b/Documentation/config/mergetool.adoc index 7064f5a462cb56..7afdcad92b3934 100644 --- a/Documentation/config/mergetool.adoc +++ b/Documentation/config/mergetool.adoc @@ -52,13 +52,13 @@ if `merge.tool` is configured as __), Git will consult `mergetool..layout` to determine the tool's layout. If the variant-specific configuration is not available, `vimdiff` ' s is used as - fallback. If that too is not available, a default layout with 4 windows - will be used. To configure the layout, see the 'BACKEND SPECIFIC HINTS' + fallback. If that too is not available, a default layout with 4 windows + will be used. ifdef::git-mergetool[] - section. +To configure the layout, see the 'BACKEND SPECIFIC HINTS' section. endif::[] ifndef::git-mergetool[] - section in linkgit:git-mergetool[1]. +To configure the layout, see the 'BACKEND SPECIFIC HINTS' section in linkgit:git-mergetool[1]. endif::[] `mergetool.hideResolved`:: diff --git a/Documentation/git-difftool.adoc b/Documentation/git-difftool.adoc index 064bc683471f21..dd7cacf95e35df 100644 --- a/Documentation/git-difftool.adoc +++ b/Documentation/git-difftool.adoc @@ -7,64 +7,64 @@ git-difftool - Show changes using common diff tools SYNOPSIS -------- -[verse] -'git difftool' [] [ []] [--] [...] +[synopsis] +git difftool [] [ []] [--] [...] DESCRIPTION ----------- -'git difftool' is a Git command that allows you to compare and edit files -between revisions using common diff tools. 'git difftool' is a frontend -to 'git diff' and accepts the same options and arguments. See +`git difftool` is a Git command that allows you to compare and edit files +between revisions using common diff tools. `git difftool` is a frontend +to `git diff` and accepts the same options and arguments. See linkgit:git-diff[1]. OPTIONS ------- --d:: ---dir-diff:: +`-d`:: +`--dir-diff`:: Copy the modified files to a temporary location and perform a directory diff on them. This mode never prompts before launching the diff tool. --y:: ---no-prompt:: +`-y`:: +`--no-prompt`:: Do not prompt before launching a diff tool. ---prompt:: +`--prompt`:: Prompt before each invocation of the diff tool. This is the default behaviour; the option is provided to override any configuration settings. ---rotate-to=:: - Start showing the diff for the given path, +`--rotate-to=`:: + Start showing the diff for __, the paths before it will move to the end and output. ---skip-to=:: - Start showing the diff for the given path, skipping all +`--skip-to=`:: + Start showing the diff for __, skipping all the paths before it. --t :: ---tool=:: - Use the diff tool specified by . Valid values include +`-t `:: +`--tool=`:: + Use the diff tool specified by __. Valid values include emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` - for the list of valid settings. + for the list of valid __ settings. + -If a diff tool is not specified, 'git difftool' +If a diff tool is not specified, `git difftool` will use the configuration variable `diff.tool`. If the -configuration variable `diff.tool` is not set, 'git difftool' +configuration variable `diff.tool` is not set, `git difftool` will pick a suitable default. + You can explicitly provide a full path to the tool by setting the configuration variable `difftool..path`. For example, you can configure the absolute path to kdiff3 by setting -`difftool.kdiff3.path`. Otherwise, 'git difftool' assumes the +`difftool.kdiff3.path`. Otherwise, `git difftool` assumes the tool is available in PATH. + Instead of running one of the known diff tools, -'git difftool' can be customized to run an alternative program +`git difftool` can be customized to run an alternative program by specifying the command line to invoke in a configuration variable `difftool..cmd`. + -When 'git difftool' is invoked with this tool (either through the +When `git difftool` is invoked with this tool (either through the `-t` or `--tool` option or the `diff.tool` configuration variable) the configured command line will be invoked with the following variables available: `$LOCAL` is set to the name of the temporary @@ -74,30 +74,30 @@ of the diff post-image. `$MERGED` is the name of the file which is being compared. `$BASE` is provided for compatibility with custom merge tool commands and has the same value as `$MERGED`. ---tool-help:: +`--tool-help`:: Print a list of diff tools that may be used with `--tool`. ---symlinks:: ---no-symlinks:: - 'git difftool''s default behavior is to create symlinks to the +`--symlinks`:: +`--no-symlinks`:: + `git difftool`'s default behavior is to create symlinks to the working tree when run in `--dir-diff` mode and the right-hand side of the comparison yields the same content as the file in the working tree. + -Specifying `--no-symlinks` instructs 'git difftool' to create copies +Specifying `--no-symlinks` instructs `git difftool` to create copies instead. `--no-symlinks` is the default on Windows. --x :: ---extcmd=:: +`-x `:: +`--extcmd=`:: Specify a custom command for viewing diffs. - 'git-difftool' ignores the configured defaults and runs + `git-difftool` ignores the configured defaults and runs ` $LOCAL $REMOTE` when this option is specified. Additionally, `$BASE` is set in the environment. --g:: ---gui:: ---no-gui:: - When 'git-difftool' is invoked with the `-g` or `--gui` option +`-g`:: +`--gui`:: +`--no-gui`:: + When `git-difftool` is invoked with the `-g` or `--gui` option the default diff tool will be read from the configured `diff.guitool` variable instead of `diff.tool`. This may be selected automatically using the configuration variable @@ -106,20 +106,20 @@ instead. `--no-symlinks` is the default on Windows. fallback in the order of `merge.guitool`, `diff.tool`, `merge.tool` until a tool is found. ---trust-exit-code:: ---no-trust-exit-code:: +`--trust-exit-code`:: +`--no-trust-exit-code`:: Errors reported by the diff tool are ignored by default. - Use `--trust-exit-code` to make 'git-difftool' exit when an + Use `--trust-exit-code` to make `git-difftool` exit when an invoked diff tool returns a non-zero exit code. + -'git-difftool' will forward the exit code of the invoked tool when +`git-difftool` will forward the exit code of the invoked tool when `--trust-exit-code` is used. See linkgit:git-diff[1] for the full list of supported options. CONFIGURATION ------------- -'git difftool' falls back to 'git mergetool' config variables when the +`git difftool` falls back to `git mergetool` config variables when the difftool equivalents have not been defined. include::includes/cmd-config-section-rest.adoc[] From 5594be68eaa0fc9c87f7a50be09b85762415f070 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= Date: Sat, 4 Apr 2026 17:12:45 +0000 Subject: [PATCH 08/61] doc: convert git-range-diff manual page to synopsis style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * convert commands and options to synopsis style * use __ for arguments * small style fixes Signed-off-by: Jean-Noël Avila Signed-off-by: Junio C Hamano --- Documentation/git-range-diff.adoc | 50 +++++++++++++++---------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/Documentation/git-range-diff.adoc b/Documentation/git-range-diff.adoc index b5e85d37f1bee7..880557084533fb 100644 --- a/Documentation/git-range-diff.adoc +++ b/Documentation/git-range-diff.adoc @@ -7,8 +7,8 @@ git-range-diff - Compare two commit ranges (e.g. two versions of a branch) SYNOPSIS -------- -[verse] -'git range-diff' [--color=[]] [--no-color] [] +[synopsis] +git range-diff [--color=[]] [--no-color] [] [--no-dual-color] [--creation-factor=] [--left-only | --right-only] [--diff-merges=] [--remerge-diff] @@ -21,14 +21,14 @@ DESCRIPTION This command shows the differences between two versions of a patch series, or more generally, two commit ranges (ignoring merge commits). -In the presence of `` arguments, these commit ranges are limited +In the presence of __ arguments, these commit ranges are limited accordingly. To that end, it first finds pairs of commits from both commit ranges that correspond with each other. Two commits are said to correspond when the diff between their patches (i.e. the author information, the commit message and the commit diff) is reasonably small compared to the -patches' size. See ``Algorithm`` below for details. +patches' size. See 'Algorithm' below for details. Finally, the list of matching commits is shown in the order of the second commit range, with unmatched commits being inserted just after @@ -37,7 +37,7 @@ all of their ancestors have been shown. There are three ways to specify the commit ranges: - ` `: Either commit range can be of the form - `..`, `^!` or `^-`. See `SPECIFYING RANGES` + `..`, `^!` or `^-`. See 'SPECIFYING RANGES' in linkgit:gitrevisions[7] for more details. - `...`. This is equivalent to @@ -48,7 +48,7 @@ There are three ways to specify the commit ranges: OPTIONS ------- ---no-dual-color:: +`--no-dual-color`:: When the commit diffs differ, `git range-diff` recreates the original diffs' coloring, and adds outer -/+ diff markers with the *background* being red/green to make it easier to see e.g. @@ -56,33 +56,33 @@ OPTIONS + Additionally, the commit diff lines that are only present in the first commit range are shown "dimmed" (this can be overridden using the `color.diff.` -config setting where `` is one of `contextDimmed`, `oldDimmed` and +config setting where __ is one of `contextDimmed`, `oldDimmed` and `newDimmed`), and the commit diff lines that are only present in the second commit range are shown in bold (which can be overridden using the config -settings `color.diff.` with `` being one of `contextBold`, +settings `color.diff.` with __ being one of `contextBold`, `oldBold` or `newBold`). + This is known to `range-diff` as "dual coloring". Use `--no-dual-color` to revert to color all lines according to the outer diff markers (and completely ignore the inner diff when it comes to color). ---creation-factor=:: - Set the creation/deletion cost fudge factor to ``. +`--creation-factor=`:: + Set the creation/deletion cost fudge factor to __. Defaults to 60. Try a larger value if `git range-diff` erroneously considers a large change a total rewrite (deletion of one commit and addition of another), and a smaller one in the reverse case. - See the ``Algorithm`` section below for an explanation of why this is + See the 'Algorithm' section below for an explanation of why this is needed. ---left-only:: +`--left-only`:: Suppress commits that are missing from the first specified range - (or the "left range" when using the `...` format). + (or the "left range" when using the `...` form). ---right-only:: +`--right-only`:: Suppress commits that are missing from the second specified range - (or the "right range" when using the `...` format). + (or the "right range" when using the `...` form). ---diff-merges=:: +`--diff-merges=`:: Instead of ignoring merge commits, generate diffs for them using the corresponding `--diff-merges=` option of linkgit:git-log[1], and include them in the comparison. @@ -93,30 +93,30 @@ have produced. In other words, if a merge commit is the result of a non-conflicting `git merge`, the `remerge` mode will represent it with an empty diff. ---remerge-diff:: +`--remerge-diff`:: Convenience option, equivalent to `--diff-merges=remerge`. ---notes[=]:: ---no-notes:: +`--notes[=]`:: +`--no-notes`:: This flag is passed to the `git log` program (see linkgit:git-log[1]) that generates the patches. - :: +` `:: Compare the commits specified by the two ranges, where - `` is considered an older version of ``. + __ is considered an older version of __. -...:: +`...`:: Equivalent to passing `..` and `..`. - :: +` `:: Equivalent to passing `..` and `..`. - Note that `` does not need to be the exact branch point + Note that __ does not need to be the exact branch point of the branches. Example: after rebasing a branch `my-topic`, `git range-diff my-topic@{u} my-topic@{1} my-topic` would show the differences introduced by the rebase. `git range-diff` also accepts the regular diff options (see -linkgit:git-diff[1]), most notably the `--color=[]` and +linkgit:git-diff[1]), most notably the `--color[=]` and `--no-color` options. These options are used when generating the "diff between patches", i.e. to compare the author, commit message and diff of corresponding old/new commits. There is currently no means to tweak most of the From f4c1b8e3fe855355f3e4c84d3e1a50b9957bd240 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= Date: Sat, 4 Apr 2026 17:12:46 +0000 Subject: [PATCH 09/61] doc: convert git-shortlog manual page to synopsis style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * convert commands and options to synopsis style * use __ for arguments * small style fixes Signed-off-by: Jean-Noël Avila Signed-off-by: Junio C Hamano --- Documentation/git-shortlog.adoc | 60 ++++++++++++++++----------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/Documentation/git-shortlog.adoc b/Documentation/git-shortlog.adoc index a11b57c1cd7b2d..e067d39b3880a8 100644 --- a/Documentation/git-shortlog.adoc +++ b/Documentation/git-shortlog.adoc @@ -3,63 +3,63 @@ git-shortlog(1) NAME ---- -git-shortlog - Summarize 'git log' output +git-shortlog - Summarize `git log` output SYNOPSIS -------- -[verse] -'git shortlog' [] [] [[--] ...] -git log --pretty=short | 'git shortlog' [] +[synopsis] +git shortlog [] [] [[--] ...] +git log --pretty=short | git shortlog [] DESCRIPTION ----------- -Summarizes 'git log' output in a format suitable for inclusion +Summarizes `git log` output in a format suitable for inclusion in release announcements. Each commit will be grouped by author and title. Additionally, "[PATCH]" will be stripped from the commit description. If no revisions are passed on the command line and either standard input -is not a terminal or there is no current branch, 'git shortlog' will +is not a terminal or there is no current branch, `git shortlog` will output a summary of the log read from standard input, without reference to the current repository. OPTIONS ------- --n:: ---numbered:: +`-n`:: +`--numbered`:: Sort output according to the number of commits per author instead of author alphabetic order. --s:: ---summary:: +`-s`:: +`--summary`:: Suppress commit description and provide a commit count summary only. --e:: ---email:: +`-e`:: +`--email`:: Show the email address of each author. ---format[=]:: +`--format[=]`:: Instead of the commit subject, use some other information to - describe each commit. '' can be any string accepted - by the `--format` option of 'git log', such as '* [%h] %s'. - (See the "PRETTY FORMATS" section of linkgit:git-log[1].) + describe each commit. __ can be any string accepted + by the `--format` option of `git log`, such as '* [%h] %s'. + (See the 'PRETTY FORMATS' section of linkgit:git-log[1].) + Each pretty-printed commit will be rewrapped before it is shown. ---date=:: +`--date=`:: Show dates formatted according to the given date string. (See - the `--date` option in the "Commit Formatting" section of + the `--date` option in the 'Commit Formatting' section of linkgit:git-log[1]). Useful with `--group=format:`. ---group=:: - Group commits based on ``. If no `--group` option is - specified, the default is `author`. `` is one of: +`--group=`:: + Group commits based on __. If no `--group` option is + specified, the default is `author`. __ is one of: + -- - `author`, commits are grouped by author - `committer`, commits are grouped by committer (the same as `-c`) - - `trailer:`, the `` is interpreted as a case-insensitive + - `trailer:`, the __ is interpreted as a case-insensitive commit message trailer (see linkgit:git-interpret-trailers[1]). For example, if your project uses `Reviewed-by` trailers, you might want to see who has been reviewing with @@ -76,7 +76,7 @@ unless the `--email` option is specified. If the value cannot be parsed as an identity, it will be taken literally and completely. - `format:`, any string accepted by the `--format` option of - 'git log'. (See the "PRETTY FORMATS" section of + `git log`. (See the 'PRETTY FORMATS' section of linkgit:git-log[1].) -- + @@ -85,11 +85,11 @@ value (but again, only once per unique value in that commit). For example, `git shortlog --group=author --group=trailer:co-authored-by` counts both authors and co-authors. --c:: ---committer:: +`-c`:: +`--committer`:: This is an alias for `--group=committer`. --w[[,[,]]]:: +`-w[[,[,]]]`:: Linewrap the output by wrapping each line at `width`. The first line of each entry is indented by `indent1` spaces, and the second and subsequent lines are indented by `indent2` spaces. `width`, @@ -98,16 +98,16 @@ counts both authors and co-authors. If width is `0` (zero) then indent the lines of the output without wrapping them. -:: +``:: Show only commits in the specified revision range. When no - is specified, it defaults to `HEAD` (i.e. the + __ is specified, it defaults to `HEAD` (i.e. the whole history leading to the current commit). `origin..HEAD` specifies all the commits reachable from the current commit (i.e. `HEAD`), but not from `origin`. For a complete list of - ways to spell , see the "Specifying Ranges" + ways to spell __, see the 'Specifying Ranges' section of linkgit:gitrevisions[7]. -[--] ...:: +`[--] ...`:: Consider only commits that are enough to explain how the files that match the specified paths came to be. + From 80f4b802e964559c65b08641c07a8acb95d0617e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= Date: Sat, 4 Apr 2026 17:12:47 +0000 Subject: [PATCH 10/61] doc: convert git-describe manual page to synopsis style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * convert commands and options to synopsis style * use __ for arguments Signed-off-by: Jean-Noël Avila Signed-off-by: Junio C Hamano --- Documentation/git-describe.adoc | 96 ++++++++++++++++----------------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/Documentation/git-describe.adoc b/Documentation/git-describe.adoc index 08ff715709ccd1..b2cb1e47e46c67 100644 --- a/Documentation/git-describe.adoc +++ b/Documentation/git-describe.adoc @@ -7,10 +7,10 @@ git-describe - Give an object a human readable name based on an available ref SYNOPSIS -------- -[verse] -'git describe' [--all] [--tags] [--contains] [--abbrev=] [...] -'git describe' [--all] [--tags] [--contains] [--abbrev=] --dirty[=] -'git describe' +[synopsis] +git describe [--all] [--tags] [--contains] [--abbrev=] [...] +git describe [--all] [--tags] [--contains] [--abbrev=] --dirty[=] +git describe DESCRIPTION ----------- @@ -22,70 +22,70 @@ abbreviated object name of the most recent commit. The result is a "human-readable" object name which can also be used to identify the commit to other git commands. -By default (without --all or --tags) `git describe` only shows +By default (without `--all` or `--tags`) `git describe` only shows annotated tags. For more information about creating annotated tags -see the -a and -s options to linkgit:git-tag[1]. +see the `-a` and `-s` options to linkgit:git-tag[1]. If the given object refers to a blob, it will be described as `:`, such that the blob can be found -at `` in the ``, which itself describes the +at __ in the __, which itself describes the first commit in which this blob occurs in a reverse revision walk -from HEAD. +from `HEAD`. OPTIONS ------- -...:: - Commit-ish object names to describe. Defaults to HEAD if omitted. +`...`:: + Commit-ish object names to describe. Defaults to `HEAD` if omitted. ---dirty[=]:: ---broken[=]:: +`--dirty[=]`:: +`--broken[=]`:: Describe the state of the working tree. When the working - tree matches HEAD, the output is the same as "git describe - HEAD". If the working tree has local modification "-dirty" + tree matches `HEAD`, the output is the same as `git describe HEAD`. + If the working tree has local modification, `-dirty` is appended to it. If a repository is corrupt and Git cannot determine if there is local modification, Git will - error out, unless `--broken' is given, which appends - the suffix "-broken" instead. + error out, unless `--broken` is given, which appends + the suffix `-broken` instead. ---all:: +`--all`:: Instead of using only the annotated tags, use any ref found in `refs/` namespace. This option enables matching any known branch, remote-tracking branch, or lightweight tag. ---tags:: +`--tags`:: Instead of using only the annotated tags, use any tag found in `refs/tags` namespace. This option enables matching a lightweight (non-annotated) tag. ---contains:: +`--contains`:: Instead of finding the tag that predates the commit, find the tag that comes after the commit, and thus contains it. - Automatically implies --tags. + Automatically implies `--tags`. ---abbrev=:: +`--abbrev=`:: Instead of using the default number of hexadecimal digits (which will vary according to the number of objects in the repository with - a default of 7) of the abbreviated object name, use digits, or - as many digits as needed to form a unique object name. An of 0 + a default of 7) of the abbreviated object name, use __ digits, or + as many digits as needed to form a unique object name. An __ of 0 will suppress long format, only showing the closest tag. ---candidates=:: +`--candidates=`:: Instead of considering only the 10 most recent tags as candidates to describe the input commit-ish consider - up to candidates. Increasing above 10 will take + up to __ candidates. Increasing __ above 10 will take slightly longer but may produce a more accurate result. - An of 0 will cause only exact matches to be output. + An __ of 0 will cause only exact matches to be output. ---exact-match:: +`--exact-match`:: Only output exact matches (a tag directly references the - supplied commit). This is a synonym for --candidates=0. + supplied commit). This is a synonym for `--candidates=0`. ---debug:: +`--debug`:: Verbosely display information about the searching strategy being employed to standard error. The tag name will still be printed to standard out. ---long:: +`--long`:: Always output the long format (the tag, the number of commits and the abbreviated commit name) even when it matches a tag. This is useful when you want to see parts of the commit object name @@ -94,8 +94,8 @@ OPTIONS describe such a commit as v1.2-0-gdeadbee (0th commit since tag v1.2 that points at object deadbee....). ---match :: - Only consider tags matching the given `glob(7)` pattern, +`--match `:: + Only consider tags matching the given `glob`(7) pattern, excluding the "refs/tags/" prefix. If used with `--all`, it also considers local branches and remote-tracking references matching the pattern, excluding respectively "refs/heads/" and "refs/remotes/" @@ -104,22 +104,22 @@ OPTIONS matching any of the patterns will be considered. Use `--no-match` to clear and reset the list of patterns. ---exclude :: - Do not consider tags matching the given `glob(7)` pattern, excluding +`--exclude `:: + Do not consider tags matching the given `glob`(7) pattern, excluding the "refs/tags/" prefix. If used with `--all`, it also does not consider local branches and remote-tracking references matching the pattern, - excluding respectively "refs/heads/" and "refs/remotes/" prefix; + excluding respectively "`refs/heads/`" and "`refs/remotes/`" prefix; references of other types are never considered. If given multiple times, a list of patterns will be accumulated and tags matching any of the - patterns will be excluded. When combined with --match a tag will be - considered when it matches at least one --match pattern and does not - match any of the --exclude patterns. Use `--no-exclude` to clear and + patterns will be excluded. When combined with `--match` a tag will be + considered when it matches at least one `--match` pattern and does not + match any of the `--exclude` patterns. Use `--no-exclude` to clear and reset the list of patterns. ---always:: +`--always`:: Show uniquely abbreviated commit object as fallback. ---first-parent:: +`--first-parent`:: Follow only the first parent commit upon seeing a merge commit. This is useful when you wish to not match tags on branches merged in the history of the target commit. @@ -139,8 +139,8 @@ an abbreviated object name for the commit itself ("2414721") at the end. The number of additional commits is the number -of commits which would be displayed by "git log v1.0.4..parent". -The hash suffix is "-g" + an unambiguous abbreviation for the tip commit +of commits which would be displayed by `git log v1.0.4..parent`. +The hash suffix is "`-g`" + an unambiguous abbreviation for the tip commit of parent (which was `2414721b194453f058079d897d13c4e377f92dc6`). The length of the abbreviation scales as the repository grows, using the approximate number of objects in the repository and a bit of math @@ -149,12 +149,12 @@ The "g" prefix stands for "git" and is used to allow describing the version of a software depending on the SCM the software is managed with. This is useful in an environment where people may use different SCMs. -Doing a 'git describe' on a tag-name will just show the tag name: +Doing a `git describe` on a tag-name will just show the tag name: [torvalds@g5 git]$ git describe v1.0.4 v1.0.4 -With --all, the command can use branch heads as references, so +With `--all`, the command can use branch heads as references, so the output shows the reference path as well: [torvalds@g5 git]$ git describe --all --abbrev=4 v1.0.5^2 @@ -163,7 +163,7 @@ the output shows the reference path as well: [torvalds@g5 git]$ git describe --all --abbrev=4 HEAD^ heads/lt/describe-7-g975b -With --abbrev set to 0, the command can be used to find the +With `--abbrev` set to 0, the command can be used to find the closest tagname without any suffix: [torvalds@g5 git]$ git describe --abbrev=0 v1.0.5^2 @@ -179,13 +179,13 @@ be sufficient to disambiguate these commits. SEARCH STRATEGY --------------- -For each commit-ish supplied, 'git describe' will first look for +For each commit-ish supplied, `git describe` will first look for a tag which tags exactly that commit. Annotated tags will always be preferred over lightweight tags, and tags with newer dates will always be preferred over tags with older dates. If an exact match is found, its name will be output and searching will stop. -If an exact match was not found, 'git describe' will walk back +If an exact match was not found, `git describe` will walk back through the commit history to locate an ancestor commit which has been tagged. The ancestor's tag will be output along with an abbreviation of the input commit-ish's SHA-1. If `--first-parent` was @@ -203,7 +203,7 @@ BUGS Tree objects as well as tag objects not pointing at commits, cannot be described. When describing blobs, the lightweight tags pointing at blobs are ignored, -but the blob is still described as : despite the lightweight +but the blob is still described as `:` despite the lightweight tag being favorable. GIT From 521731213c905f0dfec6a55393f010d185492c85 Mon Sep 17 00:00:00 2001 From: David Lin Date: Mon, 6 Apr 2026 15:27:11 -0400 Subject: [PATCH 11/61] cache-tree: fix inverted object existence check in cache_tree_fully_valid The negation in front of the object existence check in cache_tree_fully_valid() was lost in 062b914c84 (treewide: convert users of `repo_has_object_file()` to `has_object()`, 2025-04-29), turning `!repo_has_object_file(...)` into `has_object(...)` instead of `!has_object(...)`. This makes cache_tree_fully_valid() always report the cache tree as invalid when objects exist (the common case), forcing callers like write_index_as_tree() to call cache_tree_update() on every invocation. An odb_has_object() check inside update_one() avoids a full tree rebuild, but the unnecessary call still pays the cost of opening an ODB transaction and, in partial clones, a promisor remote check. Restore the missing negation and add a test that verifies write-tree takes the cache-tree shortcut when the cache tree is valid. Helped-by: Derrick Stolee Signed-off-by: David Lin Signed-off-by: Junio C Hamano --- cache-tree.c | 2 +- t/t0090-cache-tree.sh | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/cache-tree.c b/cache-tree.c index 66ef2becbe01a4..366b1d7dcd8081 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -239,7 +239,7 @@ int cache_tree_fully_valid(struct cache_tree *it) if (!it) return 0; if (it->entry_count < 0 || - odb_has_object(the_repository->objects, &it->oid, + !odb_has_object(the_repository->objects, &it->oid, HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR)) return 0; for (i = 0; i < it->subtree_nr; i++) { diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh index d901588294668c..0964718d7f33f5 100755 --- a/t/t0090-cache-tree.sh +++ b/t/t0090-cache-tree.sh @@ -278,4 +278,12 @@ test_expect_success 'switching trees does not invalidate shared index' ' ) ' +test_expect_success 'cache-tree is used by write-tree when valid' ' + test_commit use-valid && + + # write-tree with a valid cache-tree should skip cache_tree_update + GIT_TRACE2_PERF="$(pwd)/trace.output" git write-tree && + test_grep ! region_enter.*cache_tree.*update trace.output +' + test_done From 8808e61fd3e953c3534633b8b5adc5b243dd696f Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 7 Apr 2026 13:52:34 +0200 Subject: [PATCH 12/61] promisor-remote: try accepted remotes before others in get_direct() When a server advertises promisor remotes and the client accepts some of them, those remotes carry the server's intent: 'fetch missing objects preferably from here', and the client agrees with that for the remotes it accepts. However promisor_remote_get_direct() actually iterates over all promisor remotes in list order, which is the order they appear in the config files (except perhaps for the one appearing in the `extensions.partialClone` config variable which is tried last). This means an existing, but not accepted, promisor remote, could be tried before the accepted ones, which does not reflect the intent of the agreement between client and server. If the client doesn't care about what the server suggests, it should accept nothing and rely on its remotes as they are already configured. To better reflect the agreement between client and server, let's make promisor_remote_get_direct() try the accepted promisor remotes before the non-accepted ones. Concretely, let's extract a try_promisor_remotes() helper and call it twice from promisor_remote_get_direct(): - first with an `accepted_only=true` argument to try only the accepted remotes, - then with `accepted_only=false` to fall back to any remaining remote. Ensuring that accepted remotes are preferred will be even more important if in the future a mechanism is developed to allow the client to auto-configure remotes that the server advertises. This will in particular avoid fetching from the server (which is already configured as a promisor remote) before trying the auto-configured remotes, as these new remotes would likely appear at the end of the config file, and as the server might not appear in the `extensions.partialClone` config variable. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- Documentation/gitprotocol-v2.adoc | 4 ++ promisor-remote.c | 44 ++++++++++++----- t/t5710-promisor-remote-capability.sh | 69 +++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 13 deletions(-) diff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc index f985cb4c474953..4fcb1a7bda1be7 100644 --- a/Documentation/gitprotocol-v2.adoc +++ b/Documentation/gitprotocol-v2.adoc @@ -848,6 +848,10 @@ advertised, it can reply with "promisor-remote=" where where `pr-name` is the urlencoded name of a promisor remote the server advertised and the client accepts. +The promisor remotes that the client accepted will be tried before the +other configured promisor remotes when the client attempts to fetch +missing objects. + Note that, everywhere in this document, the ';' and ',' characters MUST be encoded if they appear in `pr-name` or `field-value`. diff --git a/promisor-remote.c b/promisor-remote.c index 96fa215b06a924..7ce7d22f952e70 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -268,11 +268,35 @@ static int remove_fetched_oids(struct repository *repo, return remaining_nr; } +static int try_promisor_remotes(struct repository *repo, + struct object_id **remaining_oids, + int *remaining_nr, int *to_free, + bool accepted_only) +{ + struct promisor_remote *r = repo->promisor_remote_config->promisors; + + for (; r; r = r->next) { + if (accepted_only != r->accepted) + continue; + if (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) { + if (*remaining_nr == 1) + continue; + *remaining_nr = remove_fetched_oids(repo, remaining_oids, + *remaining_nr, *to_free); + if (*remaining_nr) { + *to_free = 1; + continue; + } + } + return 1; /* all fetched */ + } + return 0; +} + void promisor_remote_get_direct(struct repository *repo, const struct object_id *oids, int oid_nr) { - struct promisor_remote *r; struct object_id *remaining_oids = (struct object_id *)oids; int remaining_nr = oid_nr; int to_free = 0; @@ -283,19 +307,13 @@ void promisor_remote_get_direct(struct repository *repo, promisor_remote_init(repo); - for (r = repo->promisor_remote_config->promisors; r; r = r->next) { - if (fetch_objects(repo, r->name, remaining_oids, remaining_nr) < 0) { - if (remaining_nr == 1) - continue; - remaining_nr = remove_fetched_oids(repo, &remaining_oids, - remaining_nr, to_free); - if (remaining_nr) { - to_free = 1; - continue; - } - } + /* Try accepted remotes first (those the server told us to use) */ + if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr, + &to_free, true)) + goto all_fetched; + if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr, + &to_free, false)) goto all_fetched; - } for (i = 0; i < remaining_nr; i++) { if (is_promisor_object(repo, &remaining_oids[i])) diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh index 357822c01a7530..bf0eed9f109742 100755 --- a/t/t5710-promisor-remote-capability.sh +++ b/t/t5710-promisor-remote-capability.sh @@ -166,6 +166,75 @@ test_expect_success "init + fetch with promisor.advertise set to 'true'" ' check_missing_objects server 1 "$oid" ' +test_expect_success "clone with two promisors but only one advertised" ' + git -C server config promisor.advertise true && + test_when_finished "rm -rf client unused_lop" && + + # Create a promisor that will be configured but not be used + git init --bare unused_lop && + + # Clone from server to create a client + GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \ + -c remote.unused_lop.promisor=true \ + -c remote.unused_lop.fetch="+refs/heads/*:refs/remotes/unused_lop/*" \ + -c remote.unused_lop.url="file://$(pwd)/unused_lop" \ + -c remote.lop.promisor=true \ + -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ + -c remote.lop.url="file://$(pwd)/lop" \ + -c promisor.acceptfromserver=All \ + --no-local --filter="blob:limit=5k" server client && + + # Check that "unused_lop" appears before "lop" in the config + printf "remote.%s.promisor true\n" "unused_lop" "lop" "origin" >expect && + git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual && + test_cmp expect actual && + + # Check that "lop" was tried + test_grep " fetch lop " trace && + # Check that "unused_lop" was not contacted + # This means "lop", the accepted promisor, was tried first + test_grep ! " fetch unused_lop " trace && + + # Check that the largest object is still missing on the server + check_missing_objects server 1 "$oid" +' + +test_expect_success "init + fetch two promisors but only one advertised" ' + git -C server config promisor.advertise true && + test_when_finished "rm -rf client unused_lop" && + + # Create a promisor that will be configured but not be used + git init --bare unused_lop && + + mkdir client && + git -C client init && + git -C client config remote.unused_lop.promisor true && + git -C client config remote.unused_lop.fetch "+refs/heads/*:refs/remotes/unused_lop/*" && + git -C client config remote.unused_lop.url "file://$(pwd)/unused_lop" && + git -C client config remote.lop.promisor true && + git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" && + git -C client config remote.lop.url "file://$(pwd)/lop" && + git -C client config remote.server.url "file://$(pwd)/server" && + git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" && + git -C client config promisor.acceptfromserver All && + + # Check that "unused_lop" appears before "lop" in the config + printf "remote.%s.promisor true\n" "unused_lop" "lop" >expect && + git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual && + test_cmp expect actual && + + GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git -C client fetch --filter="blob:limit=5k" server && + + # Check that "lop" was tried + test_grep " fetch lop " trace && + # Check that "unused_lop" was not contacted + # This means "lop", the accepted promisor, was tried first + test_grep ! " fetch unused_lop " trace && + + # Check that the largest object is still missing on the server + check_missing_objects server 1 "$oid" +' + test_expect_success "clone with promisor.acceptfromserver set to 'KnownName'" ' git -C server config promisor.advertise true && test_when_finished "rm -rf client" && From 720b7c26c82ef212852897bedb0d38eee78cb531 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 7 Apr 2026 13:52:35 +0200 Subject: [PATCH 13/61] promisor-remote: pass config entry to all_fields_match() directly The `in_list == 0` path of all_fields_match() looks up the remote in `config_info` by `advertised->name` repeatedly, even though every caller in should_accept_remote() has already performed this lookup and holds the result in `p`. To avoid this useless work, let's replace the `int in_list` parameter with a `struct promisor_info *config_entry` pointer: - When NULL (ACCEPT_ALL mode): scan the whole `config_info` list, as the old `in_list == 1` path did. - When non-NULL: match against that single config entry directly, avoiding the redundant string_list_lookup() call. This removes the hidden dependency on `advertised->name` inside all_fields_match(), which would be wrong if in the future auto-configured remotes are implemented, as the local config name may differ from the server's advertised name. While at it, let's also add a comment before all_fields_match() and match_field_against_config() to help understand how things work and help avoid similar issues. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index 7ce7d22f952e70..6c935f855af752 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -575,6 +575,12 @@ enum accept_promisor { ACCEPT_ALL }; +/* + * Check if a specific field and its advertised value match the local + * configuration of a given promisor remote. + * + * Returns 1 if they match, 0 otherwise. + */ static int match_field_against_config(const char *field, const char *value, struct promisor_info *config_info) { @@ -586,9 +592,18 @@ static int match_field_against_config(const char *field, const char *value, return 0; } +/* + * Check that the advertised fields match the local configuration. + * + * When 'config_entry' is NULL (ACCEPT_ALL mode), every checked field + * must match at least one remote in 'config_info'. + * + * When 'config_entry' points to a specific remote's config, the + * checked fields are compared against that single remote only. + */ static int all_fields_match(struct promisor_info *advertised, struct string_list *config_info, - int in_list) + struct promisor_info *config_entry) { struct string_list *fields = fields_checked(); struct string_list_item *item_checked; @@ -597,7 +612,6 @@ static int all_fields_match(struct promisor_info *advertised, int match = 0; const char *field = item_checked->string; const char *value = NULL; - struct string_list_item *item; if (!strcasecmp(field, promisor_field_filter)) value = advertised->filter; @@ -607,7 +621,11 @@ static int all_fields_match(struct promisor_info *advertised, if (!value) return 0; - if (in_list) { + if (config_entry) { + match = match_field_against_config(field, value, + config_entry); + } else { + struct string_list_item *item; for_each_string_list_item(item, config_info) { struct promisor_info *p = item->util; if (match_field_against_config(field, value, p)) { @@ -615,12 +633,6 @@ static int all_fields_match(struct promisor_info *advertised, break; } } - } else { - item = string_list_lookup(config_info, advertised->name); - if (item) { - struct promisor_info *p = item->util; - match = match_field_against_config(field, value, p); - } } if (!match) @@ -640,7 +652,7 @@ static int should_accept_remote(enum accept_promisor accept, const char *remote_url = advertised->url; if (accept == ACCEPT_ALL) - return all_fields_match(advertised, config_info, 1); + return all_fields_match(advertised, config_info, NULL); /* Get config info for that promisor remote */ item = string_list_lookup(config_info, remote_name); @@ -652,7 +664,7 @@ static int should_accept_remote(enum accept_promisor accept, p = item->util; if (accept == ACCEPT_KNOWN_NAME) - return all_fields_match(advertised, config_info, 0); + return all_fields_match(advertised, config_info, p); if (accept != ACCEPT_KNOWN_URL) BUG("Unhandled 'enum accept_promisor' value '%d'", accept); @@ -663,7 +675,7 @@ static int should_accept_remote(enum accept_promisor accept, } if (!strcmp(p->url, remote_url)) - return all_fields_match(advertised, config_info, 0); + return all_fields_match(advertised, config_info, p); warning(_("known remote named '%s' but with URL '%s' instead of '%s'"), remote_name, p->url, remote_url); From 4ed9283b36bc8652954578c3024a00b6e70f8960 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 7 Apr 2026 13:52:36 +0200 Subject: [PATCH 14/61] promisor-remote: clarify that a remote is ignored In should_accept_remote() and parse_one_advertised_remote(), when a remote is ignored, we tell users why it is ignored in a warning, but we don't tell them that the remote is actually ignored. Let's clarify that, so users have a better idea of what's actually happening. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index 6c935f855af752..8e062ec16098ac 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -670,15 +670,16 @@ static int should_accept_remote(enum accept_promisor accept, BUG("Unhandled 'enum accept_promisor' value '%d'", accept); if (!remote_url || !*remote_url) { - warning(_("no or empty URL advertised for remote '%s'"), remote_name); + warning(_("no or empty URL advertised for remote '%s', " + "ignoring this remote"), remote_name); return 0; } if (!strcmp(p->url, remote_url)) return all_fields_match(advertised, config_info, p); - warning(_("known remote named '%s' but with URL '%s' instead of '%s'"), - remote_name, p->url, remote_url); + warning(_("known remote named '%s' but with URL '%s' instead of '%s', " + "ignoring this remote"), remote_name, p->url, remote_url); return 0; } @@ -722,8 +723,8 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info string_list_clear(&elem_list, 0); if (!info->name || !info->url) { - warning(_("server advertised a promisor remote without a name or URL: %s"), - remote_info); + warning(_("server advertised a promisor remote without a name or URL: '%s', " + "ignoring this remote"), remote_info); promisor_info_free(info); return NULL; } From 3b4f0403d19738a26f0da58f4efc6f4e2473fcac Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 7 Apr 2026 13:52:37 +0200 Subject: [PATCH 15/61] promisor-remote: reject empty name or URL in advertised remote In parse_one_advertised_remote(), we check for a NULL remote name and remote URL, but not for empty ones. An empty URL seems possible as url_percent_decode("") doesn't return NULL. In promisor_config_info_list(), we ignore remotes with empty URLs, so a Git server should not advertise remotes with empty URLs. It's possible that a buggy or malicious server would do it though. So let's tighten the check in parse_one_advertised_remote() to also reject empty strings at parse time. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/promisor-remote.c b/promisor-remote.c index 8e062ec16098ac..8322349ae8ba87 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -722,7 +722,7 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info string_list_clear(&elem_list, 0); - if (!info->name || !info->url) { + if (!info->name || !*info->name || !info->url || !*info->url) { warning(_("server advertised a promisor remote without a name or URL: '%s', " "ignoring this remote"), remote_info); promisor_info_free(info); From 64f0f6b88aea33546afd1271862b486fafe7e9cc Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 7 Apr 2026 13:52:38 +0200 Subject: [PATCH 16/61] promisor-remote: refactor should_accept_remote() control flow A previous commit made sure we now reject empty URLs early at parse time. This makes the existing warning() in case a remote URL is NULL or empty very unlikely to be useful. In future work, we also plan to add URL-based acceptance logic into should_accept_remote(). To adapt to previous changes and prepare for upcoming changes, let's restructure the control flow in should_accept_remote(). Concretely, let's: - Replace the warning() in case of an empty URL with a BUG(), as a previous commit made sure empty URLs are rejected early at parse time. - Move that modified empty-URL check to the very top of the function, so that every acceptance mode, instead of only ACCEPT_KNOWN_URL, is covered. - Invert the URL comparison: instead of returning on match and warning on mismatch, return early on mismatch and let the match case fall through. This opens a single exit path at the bottom of the function for future commits to extend. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index 8322349ae8ba87..5860a3d3f36e09 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -651,6 +651,11 @@ static int should_accept_remote(enum accept_promisor accept, const char *remote_name = advertised->name; const char *remote_url = advertised->url; + if (!remote_url || !*remote_url) + BUG("no or empty URL advertised for remote '%s'; " + "this remote should have been rejected earlier", + remote_name); + if (accept == ACCEPT_ALL) return all_fields_match(advertised, config_info, NULL); @@ -669,19 +674,14 @@ static int should_accept_remote(enum accept_promisor accept, if (accept != ACCEPT_KNOWN_URL) BUG("Unhandled 'enum accept_promisor' value '%d'", accept); - if (!remote_url || !*remote_url) { - warning(_("no or empty URL advertised for remote '%s', " - "ignoring this remote"), remote_name); + if (strcmp(p->url, remote_url)) { + warning(_("known remote named '%s' but with URL '%s' instead of '%s', " + "ignoring this remote"), + remote_name, p->url, remote_url); return 0; } - if (!strcmp(p->url, remote_url)) - return all_fields_match(advertised, config_info, p); - - warning(_("known remote named '%s' but with URL '%s' instead of '%s', " - "ignoring this remote"), remote_name, p->url, remote_url); - - return 0; + return all_fields_match(advertised, config_info, p); } static int skip_field_name_prefix(const char *elem, const char *field_name, const char **value) From 16a4372a3df7579429b7bc23e984bd797a4b7b8d Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 7 Apr 2026 13:52:39 +0200 Subject: [PATCH 17/61] promisor-remote: refactor has_control_char() In a future commit we are going to check if some strings contain control characters, so let's refactor the logic to do that in a new has_control_char() helper function. It cleans up the code a bit anyway. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index 5860a3d3f36e09..d60518f19c053e 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -642,6 +642,14 @@ static int all_fields_match(struct promisor_info *advertised, return 1; } +static bool has_control_char(const char *s) +{ + for (const char *c = s; *c; c++) + if (iscntrl(*c)) + return true; + return false; +} + static int should_accept_remote(enum accept_promisor accept, struct promisor_info *advertised, struct string_list *config_info) @@ -772,18 +780,14 @@ static bool valid_filter(const char *filter, const char *remote_name) return !res; } -/* Check that a token doesn't contain any control character */ static bool valid_token(const char *token, const char *remote_name) { - const char *c = token; - - for (; *c; c++) - if (iscntrl(*c)) { - warning(_("invalid token '%s' for remote '%s' " - "will not be stored"), - token, remote_name); - return false; - } + if (has_control_char(token)) { + warning(_("invalid token '%s' for remote '%s' " + "will not be stored"), + token, remote_name); + return false; + } return true; } From 7557a562434804d27f1417fe94c4081e2ee7e68b Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 7 Apr 2026 13:52:40 +0200 Subject: [PATCH 18/61] promisor-remote: refactor accept_from_server() In future commits, we are going to add more logic to filter_promisor_remote() which is already doing a lot of things. Let's alleviate that by moving the logic that checks and validates the value of the `promisor.acceptFromServer` config variable into its own accept_from_server() helper function. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index d60518f19c053e..8d80ef6040534c 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -862,20 +862,12 @@ static bool promisor_store_advertised_fields(struct promisor_info *advertised, return reload_config; } -static void filter_promisor_remote(struct repository *repo, - struct strvec *accepted, - const char *info) +static enum accept_promisor accept_from_server(struct repository *repo) { const char *accept_str; enum accept_promisor accept = ACCEPT_NONE; - struct string_list config_info = STRING_LIST_INIT_NODUP; - struct string_list remote_info = STRING_LIST_INIT_DUP; - struct store_info *store_info = NULL; - struct string_list_item *item; - bool reload_config = false; - struct string_list accepted_filters = STRING_LIST_INIT_DUP; - if (!repo_config_get_string_tmp(the_repository, "promisor.acceptfromserver", &accept_str)) { + if (!repo_config_get_string_tmp(repo, "promisor.acceptfromserver", &accept_str)) { if (!*accept_str || !strcasecmp("None", accept_str)) accept = ACCEPT_NONE; else if (!strcasecmp("KnownUrl", accept_str)) @@ -889,6 +881,21 @@ static void filter_promisor_remote(struct repository *repo, accept_str, "promisor.acceptfromserver"); } + return accept; +} + +static void filter_promisor_remote(struct repository *repo, + struct strvec *accepted, + const char *info) +{ + struct string_list config_info = STRING_LIST_INIT_NODUP; + struct string_list remote_info = STRING_LIST_INIT_DUP; + struct store_info *store_info = NULL; + struct string_list_item *item; + bool reload_config = false; + struct string_list accepted_filters = STRING_LIST_INIT_DUP; + enum accept_promisor accept = accept_from_server(repo); + if (accept == ACCEPT_NONE) return; From e0f80d8876960442dd2645215c4fe5e1b1d80fc3 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 7 Apr 2026 13:52:41 +0200 Subject: [PATCH 19/61] promisor-remote: keep accepted promisor_info structs alive In filter_promisor_remote(), the instances of `struct promisor_info` for accepted remotes are dismantled into separate parallel data structures (the 'accepted' strvec for server names, and 'accepted_filters' for filter strings) and then immediately freed. Instead, let's keep these instances on an 'accepted_remotes' list. This way the post-loop phase can iterate a single list to build the protocol reply, apply advertised filters, and mark remotes as accepted, rather than iterating three separate structures. This refactoring also prepares for a future commit that will add a 'local_name' member to 'struct promisor_info'. Since struct instances stay alive, downstream code will be able to simply read both names from them rather than needing yet another parallel strvec. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 42 +++++++++++++++++------------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index 8d80ef6040534c..74e65e9dd0de48 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -890,10 +890,10 @@ static void filter_promisor_remote(struct repository *repo, { struct string_list config_info = STRING_LIST_INIT_NODUP; struct string_list remote_info = STRING_LIST_INIT_DUP; + struct string_list accepted_remotes = STRING_LIST_INIT_NODUP; struct store_info *store_info = NULL; struct string_list_item *item; bool reload_config = false; - struct string_list accepted_filters = STRING_LIST_INIT_DUP; enum accept_promisor accept = accept_from_server(repo); if (accept == ACCEPT_NONE) @@ -922,17 +922,10 @@ static void filter_promisor_remote(struct repository *repo, if (promisor_store_advertised_fields(advertised, store_info)) reload_config = true; - strvec_push(accepted, advertised->name); - - /* Capture advertised filters for accepted remotes */ - if (advertised->filter) { - struct string_list_item *i; - i = string_list_append(&accepted_filters, advertised->name); - i->util = xstrdup(advertised->filter); - } + string_list_append(&accepted_remotes, advertised->name)->util = advertised; + } else { + promisor_info_free(advertised); } - - promisor_info_free(advertised); } promisor_info_list_clear(&config_info); @@ -942,24 +935,23 @@ static void filter_promisor_remote(struct repository *repo, if (reload_config) repo_promisor_remote_reinit(repo); - /* Apply accepted remote filters to the stable repo state */ - for_each_string_list_item(item, &accepted_filters) { - struct promisor_remote *r = repo_promisor_remote_find(repo, item->string); - if (r) { - free(r->advertised_filter); - r->advertised_filter = item->util; - item->util = NULL; - } - } + /* Apply accepted remotes to the stable repo state */ + for_each_string_list_item(item, &accepted_remotes) { + struct promisor_info *info = item->util; + struct promisor_remote *r = repo_promisor_remote_find(repo, info->name); - string_list_clear(&accepted_filters, 1); + strvec_push(accepted, info->name); - /* Mark the remotes as accepted in the repository state */ - for (size_t i = 0; i < accepted->nr; i++) { - struct promisor_remote *r = repo_promisor_remote_find(repo, accepted->v[i]); - if (r) + if (r) { r->accepted = 1; + if (info->filter) { + free(r->advertised_filter); + r->advertised_filter = xstrdup(info->filter); + } + } } + + promisor_info_list_clear(&accepted_remotes); } void promisor_remote_reply(const char *info, char **accepted_out) From d56e483b03bfe46340af5cdbcddec8858661d2e9 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 7 Apr 2026 13:52:42 +0200 Subject: [PATCH 20/61] promisor-remote: remove the 'accepted' strvec In a previous commit, filter_promisor_remote() was refactored to keep accepted 'struct promisor_info' instances alive instead of dismantling them into separate parallel data structures. Let's go one step further and replace the 'struct strvec *accepted' argument passed to filter_promisor_remote() with a 'struct string_list *accepted_remotes' argument. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index 74e65e9dd0de48..38fa05054227f6 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -885,12 +885,11 @@ static enum accept_promisor accept_from_server(struct repository *repo) } static void filter_promisor_remote(struct repository *repo, - struct strvec *accepted, + struct string_list *accepted_remotes, const char *info) { struct string_list config_info = STRING_LIST_INIT_NODUP; struct string_list remote_info = STRING_LIST_INIT_DUP; - struct string_list accepted_remotes = STRING_LIST_INIT_NODUP; struct store_info *store_info = NULL; struct string_list_item *item; bool reload_config = false; @@ -922,7 +921,7 @@ static void filter_promisor_remote(struct repository *repo, if (promisor_store_advertised_fields(advertised, store_info)) reload_config = true; - string_list_append(&accepted_remotes, advertised->name)->util = advertised; + string_list_append(accepted_remotes, advertised->name)->util = advertised; } else { promisor_info_free(advertised); } @@ -936,12 +935,10 @@ static void filter_promisor_remote(struct repository *repo, repo_promisor_remote_reinit(repo); /* Apply accepted remotes to the stable repo state */ - for_each_string_list_item(item, &accepted_remotes) { + for_each_string_list_item(item, accepted_remotes) { struct promisor_info *info = item->util; struct promisor_remote *r = repo_promisor_remote_find(repo, info->name); - strvec_push(accepted, info->name); - if (r) { r->accepted = 1; if (info->filter) { @@ -950,23 +947,23 @@ static void filter_promisor_remote(struct repository *repo, } } } - - promisor_info_list_clear(&accepted_remotes); } void promisor_remote_reply(const char *info, char **accepted_out) { - struct strvec accepted = STRVEC_INIT; + struct string_list accepted_remotes = STRING_LIST_INIT_NODUP; - filter_promisor_remote(the_repository, &accepted, info); + filter_promisor_remote(the_repository, &accepted_remotes, info); if (accepted_out) { - if (accepted.nr) { + if (accepted_remotes.nr) { struct strbuf reply = STRBUF_INIT; - for (size_t i = 0; i < accepted.nr; i++) { - if (i) + struct string_list_item *item; + + for_each_string_list_item(item, &accepted_remotes) { + if (reply.len) strbuf_addch(&reply, ';'); - strbuf_addstr_urlencode(&reply, accepted.v[i], allow_unsanitized); + strbuf_addstr_urlencode(&reply, item->string, allow_unsanitized); } *accepted_out = strbuf_detach(&reply, NULL); } else { @@ -974,7 +971,7 @@ void promisor_remote_reply(const char *info, char **accepted_out) } } - strvec_clear(&accepted); + promisor_info_list_clear(&accepted_remotes); } void mark_promisor_remotes_as_accepted(struct repository *r, const char *remotes) From 8eb863597f630efe08f96ed12f8defbe5a5f0b1d Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 7 Apr 2026 13:52:43 +0200 Subject: [PATCH 21/61] t5710: use proper file:// URIs for absolute paths In t5710, we frequently construct local file URIs using `file://$(pwd)`. On Unix-like systems, $(pwd) returns an absolute path starting with a slash (e.g., `/tmp/repo`), resulting in a valid 3-slash URI with an empty host (`file:///tmp/repo`). However, on Windows, $(pwd) returns a path starting with a drive letter (e.g., `D:/a/repo`). This results in a 2-slash URI (`file://D:/a/repo`). Standard URI parsers misinterpret this format, treating `D:` as the host rather than part of the absolute path. This is to be expected because RFC 8089 says that the `//` prefix with an empty local host must be followed by an absolute path starting with a slash. While this hasn't broken the existing tests (because the old `promisor.acceptFromServer` logic relies entirely on strict `strcmp()` without normalizing the URLs), it will break future commits that pass these URLs through `url_normalize()` or similar functions. To future-proof the tests and ensure cross-platform URI compliance, let's introduce a $TRASH_DIRECTORY_URL helper variable that explicitly guarantees a leading slash for the path component, ensuring valid 3-slash `file:///` URIs on all operating systems. While at it, let's also introduce $ENCODED_TRASH_DIRECTORY_URL to handle some common special characters in directory paths. To be extra safe, let's skip all the tests if there are uncommon special characters in the directory path. Then let's replace all instances of `file://$(pwd)` with $TRASH_DIRECTORY_URL across the test script, and let's simplify the `sendFields` and `checkFields` tests to use $ENCODED_TRASH_DIRECTORY_URL directly. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- t/t5710-promisor-remote-capability.sh | 79 +++++++++++++++++---------- 1 file changed, 51 insertions(+), 28 deletions(-) diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh index bf0eed9f109742..b404ad9f0a9e3d 100755 --- a/t/t5710-promisor-remote-capability.sh +++ b/t/t5710-promisor-remote-capability.sh @@ -76,6 +76,31 @@ copy_to_lop () { cp "$path" "$path2" } +# On Windows, `pwd` returns a path like 'D:/foo/bar'. Prepend '/' to turn +# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows. +# On Unix, the path already starts with '/', so this is a no-op. +pwd_path=$(pwd) +case "$pwd_path" in +[a-zA-Z]:*) pwd_path="/$pwd_path" ;; +esac + +# Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -), +# and those percent-encoded below (% space = , ;) +rest=$(printf "%s" "$pwd_path" | tr -d 'a-zA-Z0-9_.~/:% =,;-') +if test -n "$rest" +then + skip_all="PWD contains unsupported special characters" + test_done +fi + +TRASH_DIRECTORY_URL="file://$pwd_path" + +encoded_path=$(printf "%s" "$pwd_path" | + sed -e 's/%/%25/g' -e 's/ /%20/g' -e 's/=/%3D/g' \ + -e 's/;/%3B/g' -e 's/,/%2C/g') + +ENCODED_TRASH_DIRECTORY_URL="file://$encoded_path" + test_expect_success "setup for testing promisor remote advertisement" ' # Create another bare repo called "lop" (for Large Object Promisor) git init --bare lop && @@ -88,7 +113,7 @@ test_expect_success "setup for testing promisor remote advertisement" ' initialize_server 1 "$oid" && # Configure lop as promisor remote for server - git -C server remote add lop "file://$(pwd)/lop" && + git -C server remote add lop "$TRASH_DIRECTORY_URL/lop" && git -C server config remote.lop.promisor true && git -C lop config uploadpack.allowFilter true && @@ -104,7 +129,7 @@ test_expect_success "clone with promisor.advertise set to 'true'" ' # Clone from server to create a client GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=All \ --no-local --filter="blob:limit=5k" server client && @@ -119,7 +144,7 @@ test_expect_success "clone with promisor.advertise set to 'false'" ' # Clone from server to create a client GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=All \ --no-local --filter="blob:limit=5k" server client && @@ -137,7 +162,7 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" ' # Clone from server to create a client GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=None \ --no-local --filter="blob:limit=5k" server client && @@ -156,8 +181,8 @@ test_expect_success "init + fetch with promisor.advertise set to 'true'" ' git -C client init && git -C client config remote.lop.promisor true && git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" && - git -C client config remote.lop.url "file://$(pwd)/lop" && - git -C client config remote.server.url "file://$(pwd)/server" && + git -C client config remote.lop.url "$TRASH_DIRECTORY_URL/lop" && + git -C client config remote.server.url "$TRASH_DIRECTORY_URL/server" && git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" && git -C client config promisor.acceptfromserver All && GIT_NO_LAZY_FETCH=0 git -C client fetch --filter="blob:limit=5k" server && @@ -177,10 +202,10 @@ test_expect_success "clone with two promisors but only one advertised" ' GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \ -c remote.unused_lop.promisor=true \ -c remote.unused_lop.fetch="+refs/heads/*:refs/remotes/unused_lop/*" \ - -c remote.unused_lop.url="file://$(pwd)/unused_lop" \ + -c remote.unused_lop.url="$TRASH_DIRECTORY_URL/unused_lop" \ -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=All \ --no-local --filter="blob:limit=5k" server client && @@ -210,11 +235,11 @@ test_expect_success "init + fetch two promisors but only one advertised" ' git -C client init && git -C client config remote.unused_lop.promisor true && git -C client config remote.unused_lop.fetch "+refs/heads/*:refs/remotes/unused_lop/*" && - git -C client config remote.unused_lop.url "file://$(pwd)/unused_lop" && + git -C client config remote.unused_lop.url "$TRASH_DIRECTORY_URL/unused_lop" && git -C client config remote.lop.promisor true && git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" && - git -C client config remote.lop.url "file://$(pwd)/lop" && - git -C client config remote.server.url "file://$(pwd)/server" && + git -C client config remote.lop.url "$TRASH_DIRECTORY_URL/lop" && + git -C client config remote.server.url "$TRASH_DIRECTORY_URL/server" && git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" && git -C client config promisor.acceptfromserver All && @@ -242,7 +267,7 @@ test_expect_success "clone with promisor.acceptfromserver set to 'KnownName'" ' # Clone from server to create a client GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=KnownName \ --no-local --filter="blob:limit=5k" server client && @@ -257,7 +282,7 @@ test_expect_success "clone with 'KnownName' and different remote names" ' # Clone from server to create a client GIT_NO_LAZY_FETCH=0 git clone -c remote.serverTwo.promisor=true \ -c remote.serverTwo.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.serverTwo.url="file://$(pwd)/lop" \ + -c remote.serverTwo.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=KnownName \ --no-local --filter="blob:limit=5k" server client && @@ -294,7 +319,7 @@ test_expect_success "clone with promisor.acceptfromserver set to 'KnownUrl'" ' # Clone from server to create a client GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=KnownUrl \ --no-local --filter="blob:limit=5k" server client && @@ -311,7 +336,7 @@ test_expect_success "clone with 'KnownUrl' and different remote urls" ' # Clone from server to create a client GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/serverTwo" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/serverTwo" \ -c promisor.acceptfromserver=KnownUrl \ --no-local --filter="blob:limit=5k" server client && @@ -326,7 +351,7 @@ test_expect_success "clone with 'KnownUrl' and url not configured on the server" git -C server config promisor.advertise true && test_when_finished "rm -rf client" && - test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" && + test_when_finished "git -C server config set remote.lop.url \"$TRASH_DIRECTORY_URL/lop\"" && git -C server config unset remote.lop.url && # Clone from server to create a client @@ -335,7 +360,7 @@ test_expect_success "clone with 'KnownUrl' and url not configured on the server" # missing, so the remote name will be used instead which will fail. test_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=KnownUrl \ --no-local --filter="blob:limit=5k" server client && @@ -347,7 +372,7 @@ test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" ' git -C server config promisor.advertise true && test_when_finished "rm -rf client" && - test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" && + test_when_finished "git -C server config set remote.lop.url \"$TRASH_DIRECTORY_URL/lop\"" && git -C server config set remote.lop.url "" && # Clone from server to create a client @@ -356,7 +381,7 @@ test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" ' # so the remote name will be used instead which will fail. test_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=KnownUrl \ --no-local --filter="blob:limit=5k" server client && @@ -380,13 +405,12 @@ test_expect_success "clone with promisor.sendFields" ' GIT_TRACE_PACKET="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \ -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=All \ --no-local --filter="blob:limit=5k" server client && # Check that fields are properly transmitted - ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") && - PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" && + PR1="name=lop,url=$ENCODED_TRASH_DIRECTORY_URL/lop,partialCloneFilter=blob:none" && PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" && test_grep "clone< promisor-remote=$PR1;$PR2" trace && test_grep "clone> promisor-remote=lop;otherLop" trace && @@ -411,15 +435,14 @@ test_expect_success "clone with promisor.checkFields" ' GIT_TRACE_PACKET="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \ -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c remote.lop.partialCloneFilter="blob:none" \ -c promisor.acceptfromserver=All \ -c promisor.checkFields=partialcloneFilter \ --no-local --filter="blob:limit=5k" server client && # Check that fields are properly transmitted - ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") && - PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" && + PR1="name=lop,url=$ENCODED_TRASH_DIRECTORY_URL/lop,partialCloneFilter=blob:none" && PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" && test_grep "clone< promisor-remote=$PR1;$PR2" trace && test_grep "clone> promisor-remote=lop" trace && @@ -449,7 +472,7 @@ test_expect_success "clone with promisor.storeFields=partialCloneFilter" ' GIT_TRACE_PACKET="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \ -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c remote.lop.token="fooYYY" \ -c remote.lop.partialCloneFilter="blob:none" \ -c promisor.acceptfromserver=All \ @@ -501,7 +524,7 @@ test_expect_success "clone and fetch with --filter=auto" ' GIT_TRACE_PACKET="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \ -c remote.lop.promisor=true \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=All \ --no-local --filter=auto server client 2>err && @@ -558,7 +581,7 @@ test_expect_success "clone with promisor.advertise set to 'true' but don't delet # Clone from server to create a client GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ - -c remote.lop.url="file://$(pwd)/lop" \ + -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \ -c promisor.acceptfromserver=All \ --no-local --filter="blob:limit=5k" server client && From b886f0b5dc71030bc9dcf58376533cf8e1098e9a Mon Sep 17 00:00:00 2001 From: Shreyansh Paliwal Date: Sat, 4 Apr 2026 19:28:38 +0530 Subject: [PATCH 22/61] refs: add struct repository parameter in get_files_ref_lock_timeout_ms() get_files_ref_lock_timeout_ms() calls repo_config_get_int() using the_repository, as no repository instance is available in its scope. Add a struct repository parameter and use it instead of the_repository. Update all callers accordingly. In files-backend.c, lock_raw_ref() can obtain repository instance from the struct ref_transaction via transaction->ref_store->repo and pass it down. For create_reflock(), which is used as a callback, introduce a small wrapper struct to pass both struct lock_file and struct repository through the callback data. This reduces reliance on the_repository global, though the function still uses static variables and is not yet fully repository-scoped. This can be addressed in a follow-up change. Signed-off-by: Shreyansh Paliwal Acked-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 4 ++-- refs/files-backend.c | 19 +++++++++++++------ refs/refs-internal.h | 2 +- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/refs.c b/refs.c index 5d1d28523d617f..4ab746a3cb555c 100644 --- a/refs.c +++ b/refs.c @@ -989,7 +989,7 @@ enum ref_worktree_type parse_worktree_ref(const char *maybe_worktree_ref, return REF_WORKTREE_SHARED; } -long get_files_ref_lock_timeout_ms(void) +long get_files_ref_lock_timeout_ms(struct repository *repo) { static int configured = 0; @@ -997,7 +997,7 @@ long get_files_ref_lock_timeout_ms(void) static int timeout_ms = 100; if (!configured) { - repo_config_get_int(the_repository, "core.filesreflocktimeout", &timeout_ms); + repo_config_get_int(repo, "core.filesreflocktimeout", &timeout_ms); configured = 1; } diff --git a/refs/files-backend.c b/refs/files-backend.c index 0537a72b2af9e0..10e4388d2ca01a 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -792,7 +792,7 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs, if (hold_lock_file_for_update_timeout( &lock->lk, ref_file.buf, LOCK_NO_DEREF, - get_files_ref_lock_timeout_ms()) < 0) { + get_files_ref_lock_timeout_ms(transaction->ref_store->repo)) < 0) { int myerr = errno; errno = 0; if (myerr == ENOENT && --attempts_remaining > 0) { @@ -1190,13 +1190,17 @@ static int remove_empty_directories(struct strbuf *path) return remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY); } +struct create_reflock_cb { + struct lock_file *lk; + struct repository *repo; +}; + static int create_reflock(const char *path, void *cb) { - struct lock_file *lk = cb; - + struct create_reflock_cb *data = cb; return hold_lock_file_for_update_timeout( - lk, path, LOCK_NO_DEREF, - get_files_ref_lock_timeout_ms()) < 0 ? -1 : 0; + data->lk, path, LOCK_NO_DEREF, + get_files_ref_lock_timeout_ms(data->repo)) < 0 ? -1 : 0; } /* @@ -1208,6 +1212,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, { struct strbuf ref_file = STRBUF_INIT; struct ref_lock *lock; + struct create_reflock_cb cb_data; files_assert_main_repository(refs, "lock_ref_oid_basic"); assert(err); @@ -1229,8 +1234,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, lock->ref_name = xstrdup(refname); lock->count = 1; + cb_data.lk = &lock->lk; + cb_data.repo = refs->base.repo; - if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) { + if (raceproof_create_file(ref_file.buf, create_reflock, &cb_data)) { unable_to_lock_message(ref_file.buf, errno, err); goto error_return; } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index d79e35fd269a6c..e4cfd9e19ee74f 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -43,7 +43,7 @@ struct ref_transaction; * Return the length of time to retry acquiring a loose reference lock * before giving up, in milliseconds: */ -long get_files_ref_lock_timeout_ms(void); +long get_files_ref_lock_timeout_ms(struct repository *repo); /* * Return true iff refname is minimally safe. "Safe" here means that From 9a03f165a41d708c672e18e69d43f69689981e7d Mon Sep 17 00:00:00 2001 From: Shreyansh Paliwal Date: Sat, 4 Apr 2026 19:28:39 +0530 Subject: [PATCH 23/61] refs: remove the_hash_algo global state refs.c uses the_hash_algo in multiple places, relying on global state for the object hash algorithm. Replace these uses with the appropriate repository-specific hash_algo. In transaction-related functions (ref_transaction_create, ref_transaction_delete, migrate_one_ref, and transaction_hook_feed_stdin), use transaction->ref_store->repo->hash_algo. In other cases, such as repo_get_submodule_ref_store(), use repo->hash_algo. Signed-off-by: Shreyansh Paliwal Acked-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/refs.c b/refs.c index 4ab746a3cb555c..d13ca9a37c63e3 100644 --- a/refs.c +++ b/refs.c @@ -1472,7 +1472,7 @@ int ref_transaction_create(struct ref_transaction *transaction, return 1; } return ref_transaction_update(transaction, refname, new_oid, - null_oid(the_hash_algo), new_target, NULL, flags, + null_oid(transaction->ref_store->repo->hash_algo), new_target, NULL, flags, msg, err); } @@ -1491,7 +1491,7 @@ int ref_transaction_delete(struct ref_transaction *transaction, if (old_target && !(flags & REF_NO_DEREF)) BUG("delete cannot operate on symrefs with deref mode"); return ref_transaction_update(transaction, refname, - null_oid(the_hash_algo), old_oid, + null_oid(transaction->ref_store->repo->hash_algo), old_oid, NULL, old_target, flags, msg, err); } @@ -2379,7 +2379,7 @@ struct ref_store *repo_get_submodule_ref_store(struct repository *repo, subrepo = xmalloc(sizeof(*subrepo)); if (repo_submodule_init(subrepo, repo, submodule, - null_oid(the_hash_algo))) { + null_oid(repo->hash_algo))) { free(subrepo); goto done; } @@ -2571,14 +2571,14 @@ static int transaction_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_ strbuf_reset(buf); if (!(update->flags & REF_HAVE_OLD)) - strbuf_addf(buf, "%s ", oid_to_hex(null_oid(the_hash_algo))); + strbuf_addf(buf, "%s ", oid_to_hex(null_oid(transaction->ref_store->repo->hash_algo))); else if (update->old_target) strbuf_addf(buf, "ref:%s ", update->old_target); else strbuf_addf(buf, "%s ", oid_to_hex(&update->old_oid)); if (!(update->flags & REF_HAVE_NEW)) - strbuf_addf(buf, "%s ", oid_to_hex(null_oid(the_hash_algo))); + strbuf_addf(buf, "%s ", oid_to_hex(null_oid(transaction->ref_store->repo->hash_algo))); else if (update->new_target) strbuf_addf(buf, "ref:%s ", update->new_target); else @@ -3146,6 +3146,7 @@ struct migration_data { static int migrate_one_ref(const struct reference *ref, void *cb_data) { struct migration_data *data = cb_data; + const struct git_hash_algo *hash_algo = data->transaction->ref_store->repo->hash_algo; struct strbuf symref_target = STRBUF_INIT; int ret; @@ -3154,7 +3155,7 @@ static int migrate_one_ref(const struct reference *ref, void *cb_data) if (ret < 0) goto done; - ret = ref_transaction_update(data->transaction, ref->name, NULL, null_oid(the_hash_algo), + ret = ref_transaction_update(data->transaction, ref->name, NULL, null_oid(hash_algo), symref_target.buf, NULL, REF_SKIP_CREATE_REFLOG | REF_NO_DEREF, NULL, data->errbuf); if (ret < 0) From 57c590feb96b2298e0966bea3ce88c72fca37bbd Mon Sep 17 00:00:00 2001 From: Shreyansh Paliwal Date: Sat, 4 Apr 2026 19:28:40 +0530 Subject: [PATCH 24/61] refs/reftable-backend: drop uses of the_repository reftable_be_init() and reftable_be_create_on_disk() use the_repository even though a repository instance is already available, either directly or via struct ref_store. Replace these uses with the appropriate local repository instance (repo or ref_store->repo) to avoid relying on global state. Note that USE_THE_REPOSITORY_VARIABLE cannot be removed yet, as is_bare_repository() is still there in the file. Signed-off-by: Shreyansh Paliwal Acked-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/reftable-backend.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index b124404663edf6..7c8a992fcb40b9 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -404,13 +404,13 @@ static struct ref_store *reftable_be_init(struct repository *repo, default: BUG("unknown hash algorithm %d", repo->hash_algo->format_id); } - refs->write_options.default_permissions = calc_shared_perm(the_repository, 0666 & ~mask); + refs->write_options.default_permissions = calc_shared_perm(repo, 0666 & ~mask); refs->write_options.disable_auto_compact = !git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1); refs->write_options.lock_timeout_ms = 100; refs->write_options.fsync = reftable_be_fsync; - repo_config(the_repository, reftable_be_config, &refs->write_options); + repo_config(repo, reftable_be_config, &refs->write_options); /* * It is somewhat unfortunate that we have to mirror the default block @@ -492,7 +492,7 @@ static int reftable_be_create_on_disk(struct ref_store *ref_store, struct strbuf sb = STRBUF_INIT; strbuf_addf(&sb, "%s/reftable", refs->base.gitdir); - safe_create_dir(the_repository, sb.buf, 1); + safe_create_dir(ref_store->repo, sb.buf, 1); strbuf_reset(&sb); strbuf_release(&sb); From 6077dc8a427950392be15a5507908d5e87a721a6 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Thu, 9 Apr 2026 22:44:31 +0000 Subject: [PATCH 25/61] docs: update version with default Rust support We missed the cut-off for Rust by default in 2.53, but we still can enable it by default for 2.54, so update our breaking changes document accordingly. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- Documentation/BreakingChanges.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc index f814450d2f65ac..510ed98b65d755 100644 --- a/Documentation/BreakingChanges.adoc +++ b/Documentation/BreakingChanges.adoc @@ -190,7 +190,7 @@ milestones for the introduction of Rust: 1. Initially, with Git 2.52, support for Rust will be auto-detected by Meson and disabled in our Makefile so that the project can sort out the initial infrastructure. -2. In Git 2.53, both build systems will default-enable support for Rust. +2. In Git 2.54, both build systems will default-enable support for Rust. Consequently, builds will break by default if Rust is not available on the build host. The use of Rust can still be explicitly disabled via build flags. From 40c789dfc250486e60b7d7cdba47d8423a754abf Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Thu, 9 Apr 2026 22:44:32 +0000 Subject: [PATCH 26/61] ci: install cargo on Alpine We'll make Rust the default in a future commit, so be sure to install Cargo (which will also install Rust) to prepare for that case. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- ci/install-dependencies.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index c55441d9df91fd..10c3530d1aacdd 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -29,7 +29,7 @@ alpine-*) apk add --update shadow sudo meson ninja-build gcc libc-dev curl-dev openssl-dev expat-dev gettext \ zlib-ng-dev pcre2-dev python3 musl-libintl perl-utils ncurses \ apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \ - bash cvs gnupg perl-cgi perl-dbd-sqlite perl-io-tty >/dev/null + bash cvs gnupg perl-cgi perl-dbd-sqlite perl-io-tty cargo >/dev/null ;; fedora-*|almalinux-*) case "$jobname" in From 30e6f7adf626af926a02897294363dbf5f3bbe65 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Thu, 9 Apr 2026 22:44:33 +0000 Subject: [PATCH 27/61] Linux: link against libdl Older versions of Rust on Linux, such as that used in Debian 11 in our CI, require linking against libdl. Were we linking with Cargo, this would be included automatically, but since we're not, explicitly set it in the system-specific config. This library is part of libc, so linking against it if it happens to be unnecessary will add no dependencies to the resulting binary. In addition, it is provided by both glibc and musl, so it should be portable to almost all Linux systems. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- config.mak.uname | 1 + 1 file changed, 1 insertion(+) diff --git a/config.mak.uname b/config.mak.uname index ccb3f718812740..7aab56c590069e 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -63,6 +63,7 @@ ifeq ($(uname_S),Linux) PROCFS_EXECUTABLE_PATH = /proc/self/exe HAVE_PLATFORM_PROCINFO = YesPlease COMPAT_OBJS += compat/linux/procinfo.o + EXTLIBS += -ldl # centos7/rhel7 provides gcc 4.8.5 and zlib 1.2.7. ifneq ($(findstring .el7.,$(uname_R)),) BASIC_CFLAGS += -std=c99 From 32d5b905909e781786c4735e6bd71503b23e4fb1 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Thu, 9 Apr 2026 22:44:34 +0000 Subject: [PATCH 28/61] Enable Rust by default Our breaking changes document says that we'll enable Rust by default in Git 2.54. Adjust the Makefile to switch the option from WITH_RUST to NO_RUST to enable it by default and update the help text accordingly. Similarly, for Meson, enable the option by default and do not automatically disable it if Cargo is missing, since the goal is to help users find where they are likely to have problems in the future. Update our CI tests to swap out the single Linux job with Rust to a single job without, both for Makefile and Meson. Similarly, update the Windows Makefile job to not use Rust, while the Meson job (which does not build with ci/lib.sh) will default to having it enabled. Move the check for Cargo in the Meson build because it is no longer needed in the main script. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- Makefile | 10 +++++----- ci/lib.sh | 3 +++ ci/run-build-and-tests.sh | 6 ++++-- meson.build | 3 +-- meson_options.txt | 2 +- src/meson.build | 1 + 6 files changed, 15 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index dbf00220541ce1..84b59959dedc5a 100644 --- a/Makefile +++ b/Makefile @@ -498,9 +498,9 @@ include shared.mak # # == Optional Rust support == # -# Define WITH_RUST if you want to include features and subsystems written in -# Rust into Git. For now, Rust is still an optional feature of the build -# process. With Git 3.0 though, Rust will always be enabled. +# Define NO_RUST if you want to disable features and subsystems written in Rust +# from being compiled into Git. For now, Rust is still an optional feature of +# the build process. With Git 3.0 though, Rust will always be enabled. # # Building Rust code requires Cargo. # @@ -1351,7 +1351,7 @@ LIB_OBJS += urlmatch.o LIB_OBJS += usage.o LIB_OBJS += userdiff.o LIB_OBJS += utf8.o -ifndef WITH_RUST +ifdef NO_RUST LIB_OBJS += varint.o endif LIB_OBJS += version.o @@ -1590,7 +1590,7 @@ endif ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND) ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND) -ifdef WITH_RUST +ifndef NO_RUST BASIC_CFLAGS += -DWITH_RUST GITLIBS += $(RUST_LIB) ifeq ($(uname_S),Windows) diff --git a/ci/lib.sh b/ci/lib.sh index 42a2b6a318b874..1cfc8c6efce09e 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -372,6 +372,9 @@ linux-asan-ubsan) osx-meson) MESONFLAGS="$MESONFLAGS -Dcredential_helpers=osxkeychain" ;; +windows-*) + export NO_RUST=UnfortunatelyYes + ;; esac MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}" diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index 28cfe730ee5aed..e2d783d90b0181 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -8,11 +8,12 @@ export TEST_CONTRIB_TOO=yes case "$jobname" in +linux-musl-meson) + MESONFLAGS="$MESONFLAGS -Drust=disabled" + ;; fedora-breaking-changes-musl|linux-breaking-changes) export WITH_BREAKING_CHANGES=YesPlease - export WITH_RUST=YesPlease MESONFLAGS="$MESONFLAGS -Dbreaking_changes=true" - MESONFLAGS="$MESONFLAGS -Drust=enabled" ;; linux-TEST-vars) export OPENSSL_SHA1_UNSAFE=YesPlease @@ -30,6 +31,7 @@ linux-TEST-vars) export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 ;; linux-clang) + export NO_RUST=UnfortunatelyYes export GIT_TEST_DEFAULT_HASH=sha1 ;; linux-sha256) diff --git a/meson.build b/meson.build index 8309942d184847..deff129cf6d33a 100644 --- a/meson.build +++ b/meson.build @@ -1745,8 +1745,7 @@ version_def_h = custom_target( ) libgit_sources += version_def_h -cargo = find_program('cargo', dirs: program_path, native: true, required: get_option('rust')) -rust_option = get_option('rust').disable_auto_if(not cargo.found()) +rust_option = get_option('rust') if rust_option.allowed() subdir('src') libgit_c_args += '-DWITH_RUST' diff --git a/meson_options.txt b/meson_options.txt index 659cbb218f46e0..80a8025f20be6e 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -77,7 +77,7 @@ option('zlib_backend', type: 'combo', choices: ['auto', 'zlib', 'zlib-ng'], valu # Build tweaks. option('breaking_changes', type: 'boolean', value: false, description: 'Enable upcoming breaking changes.') -option('rust', type: 'feature', value: 'auto', +option('rust', type: 'feature', value: 'enabled', description: 'Enable building with Rust.') option('macos_use_homebrew_gettext', type: 'boolean', value: true, description: 'Use gettext from Homebrew instead of the slightly-broken system-provided one.') diff --git a/src/meson.build b/src/meson.build index 45739957b451c9..41a4b231e660c4 100644 --- a/src/meson.build +++ b/src/meson.build @@ -29,6 +29,7 @@ libgit_rs = custom_target('git_rs', ) libgit_dependencies += declare_dependency(link_with: libgit_rs) +cargo = find_program('cargo', dirs: program_path, native: true, required: get_option('rust')) if get_option('tests') test('rust', cargo, args: [ From 8d2ffcf4b4a3a55c56c57c8df617516c25d98380 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Fri, 10 Apr 2026 12:05:56 +0300 Subject: [PATCH 29/61] repository: fix repo_init() memleak due to missing _clear() There is an old pre-existing memory leak in repo_init() due to failing to call clear_repository_format() in the error case. It went undetected because a specific bug is required to trigger it: enable a v1 extension in a repository with format v0. Obviously this can only happen in a development environment, so it does not trigger in normal usage, however the memleak is real and needs fixing. Fix it by also calling clear_repository_format() in the error case. Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- repository.c | 1 + 1 file changed, 1 insertion(+) diff --git a/repository.c b/repository.c index 9e5537f53961ed..192d6dc9c477fa 100644 --- a/repository.c +++ b/repository.c @@ -323,6 +323,7 @@ int repo_init(struct repository *repo, return 0; error: + clear_repository_format(&format); repo_clear(repo); return -1; } From 1c9e5b3fa235e0da6f62359af36afea8e7617074 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Fri, 10 Apr 2026 12:05:57 +0300 Subject: [PATCH 30/61] config: add a repo_config_get_uint() helper Next commits add a 'hook.jobs' config option of type 'unsigned int', so add a helper to parse it since the API only supports int and ulong. An alternative is to make 'hook.jobs' an 'int' or parse it as an 'int' then cast it to unsigned, however it's better to use proper helpers for the type. Using 'ulong' is another option which already has helpers, but it's a bit excessive in size for just the jobs number. Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- config.c | 28 ++++++++++++++++++++++++++++ config.h | 13 +++++++++++++ parse.c | 9 +++++++++ parse.h | 1 + 4 files changed, 51 insertions(+) diff --git a/config.c b/config.c index 156f2a24fa0027..a1b92fe083cf43 100644 --- a/config.c +++ b/config.c @@ -1212,6 +1212,15 @@ int git_config_int(const char *name, const char *value, return ret; } +unsigned int git_config_uint(const char *name, const char *value, + const struct key_value_info *kvi) +{ + unsigned int ret; + if (!git_parse_uint(value, &ret)) + die_bad_number(name, value, kvi); + return ret; +} + int64_t git_config_int64(const char *name, const char *value, const struct key_value_info *kvi) { @@ -1907,6 +1916,18 @@ int git_configset_get_int(struct config_set *set, const char *key, int *dest) return 1; } +int git_configset_get_uint(struct config_set *set, const char *key, unsigned int *dest) +{ + const char *value; + struct key_value_info kvi; + + if (!git_configset_get_value(set, key, &value, &kvi)) { + *dest = git_config_uint(key, value, &kvi); + return 0; + } else + return 1; +} + int git_configset_get_ulong(struct config_set *set, const char *key, unsigned long *dest) { const char *value; @@ -2356,6 +2377,13 @@ int repo_config_get_int(struct repository *repo, return git_configset_get_int(repo->config, key, dest); } +int repo_config_get_uint(struct repository *repo, + const char *key, unsigned int *dest) +{ + git_config_check_init(repo); + return git_configset_get_uint(repo->config, key, dest); +} + int repo_config_get_ulong(struct repository *repo, const char *key, unsigned long *dest) { diff --git a/config.h b/config.h index ba426a960af9f4..bf47fb3afc61bf 100644 --- a/config.h +++ b/config.h @@ -267,6 +267,12 @@ int git_config_int(const char *, const char *, const struct key_value_info *); int64_t git_config_int64(const char *, const char *, const struct key_value_info *); +/** + * Identical to `git_config_int`, but for unsigned ints. + */ +unsigned int git_config_uint(const char *, const char *, + const struct key_value_info *); + /** * Identical to `git_config_int`, but for unsigned longs. */ @@ -560,6 +566,7 @@ int git_configset_get_value(struct config_set *cs, const char *key, int git_configset_get_string(struct config_set *cs, const char *key, char **dest); int git_configset_get_int(struct config_set *cs, const char *key, int *dest); +int git_configset_get_uint(struct config_set *cs, const char *key, unsigned int *dest); int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest); int git_configset_get_bool(struct config_set *cs, const char *key, int *dest); int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest); @@ -650,6 +657,12 @@ int repo_config_get_string_tmp(struct repository *r, */ int repo_config_get_int(struct repository *r, const char *key, int *dest); +/** + * Similar to `repo_config_get_int` but for unsigned ints. + */ +int repo_config_get_uint(struct repository *r, + const char *key, unsigned int *dest); + /** * Similar to `repo_config_get_int` but for unsigned longs. */ diff --git a/parse.c b/parse.c index 48313571aab129..d77f28046a0916 100644 --- a/parse.c +++ b/parse.c @@ -107,6 +107,15 @@ int git_parse_int64(const char *value, int64_t *ret) return 1; } +int git_parse_uint(const char *value, unsigned int *ret) +{ + uintmax_t tmp; + if (!git_parse_unsigned(value, &tmp, maximum_unsigned_value_of_type(unsigned int))) + return 0; + *ret = tmp; + return 1; +} + int git_parse_ulong(const char *value, unsigned long *ret) { uintmax_t tmp; diff --git a/parse.h b/parse.h index ea32de9a91fbfb..a6dd37c4cba273 100644 --- a/parse.h +++ b/parse.h @@ -5,6 +5,7 @@ int git_parse_signed(const char *value, intmax_t *ret, intmax_t max); int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max); int git_parse_ssize_t(const char *, ssize_t *); int git_parse_ulong(const char *, unsigned long *); +int git_parse_uint(const char *value, unsigned int *ret); int git_parse_int(const char *value, int *ret); int git_parse_int64(const char *value, int64_t *ret); int git_parse_double(const char *value, double *ret); From b9a4c9ad247a09602e0e6d0eccec6a43857f62da Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Fri, 10 Apr 2026 12:05:58 +0300 Subject: [PATCH 31/61] hook: parse the hook.jobs config The hook.jobs config is a global way to set hook parallelization for all hooks, in the sense that it is not per-event nor per-hook. Finer-grained configs will be added in later commits which can override it, for e.g. via a per-event type job options. Next commits will also add to this item's documentation. Parse hook.jobs config key in hook_config_lookup_all() and store its value in hook_all_config_cb.jobs, then transfer it into r->jobs after the config pass completes. This is mostly plumbing and the cached value is not yet used. Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 4 ++++ hook.c | 23 +++++++++++++++++++++-- repository.h | 3 +++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index 9e78f264396ca5..b7847f9338c65f 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -22,3 +22,7 @@ hook..enabled:: configuration. This is particularly useful when a hook is defined in a system or global config file and needs to be disabled for a specific repository. See linkgit:git-hook[1]. + +hook.jobs:: + Specifies how many hooks can be run simultaneously during parallelized + hook execution. If unspecified, defaults to 1 (serial execution). diff --git a/hook.c b/hook.c index cc23276d27f035..b8cce00e578d3c 100644 --- a/hook.c +++ b/hook.c @@ -123,11 +123,13 @@ struct hook_config_cache_entry { * commands: friendly-name to command map. * event_hooks: event-name to list of friendly-names map. * disabled_hooks: set of friendly-names with hook..enabled = false. + * jobs: value of the global hook.jobs key. Defaults to 0 if unset (stored in r->hook_jobs). */ struct hook_all_config_cb { struct strmap commands; struct strmap event_hooks; struct string_list disabled_hooks; + unsigned int jobs; }; /* repo_config() callback that collects all hook.* configuration in one pass. */ @@ -143,6 +145,20 @@ static int hook_config_lookup_all(const char *key, const char *value, if (parse_config_key(key, "hook", &name, &name_len, &subkey)) return 0; + /* Handle plain hook. entries that have no hook name component. */ + if (!name) { + if (!strcmp(subkey, "jobs") && value) { + unsigned int v; + if (!git_parse_uint(value, &v)) + warning(_("hook.jobs must be a positive integer, ignoring: '%s'"), value); + else if (!v) + warning(_("hook.jobs must be positive, ignoring: 0")); + else + data->jobs = v; + } + return 0; + } + if (!value) return config_error_nonbool(key); @@ -240,7 +256,7 @@ void hook_cache_clear(struct strmap *cache) /* Populate `cache` with the complete hook configuration */ static void build_hook_config_map(struct repository *r, struct strmap *cache) { - struct hook_all_config_cb cb_data; + struct hook_all_config_cb cb_data = { 0 }; struct hashmap_iter iter; struct strmap_entry *e; @@ -248,7 +264,7 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) strmap_init(&cb_data.event_hooks); string_list_init_dup(&cb_data.disabled_hooks); - /* Parse all configs in one run. */ + /* Parse all configs in one run, capturing hook.* including hook.jobs. */ repo_config(r, hook_config_lookup_all, &cb_data); /* Construct the cache from parsed configs. */ @@ -292,6 +308,9 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) strmap_put(cache, e->key, hooks); } + if (r) + r->hook_jobs = cb_data.jobs; + strmap_clear(&cb_data.commands, 1); string_list_clear(&cb_data.disabled_hooks, 0); strmap_for_each_entry(&cb_data.event_hooks, &iter, e) { diff --git a/repository.h b/repository.h index 078059a6e02b10..58e46853d089bf 100644 --- a/repository.h +++ b/repository.h @@ -172,6 +172,9 @@ struct repository { */ struct strmap *hook_config_cache; + /* Cached value of hook.jobs config (0 if unset, defaults to serial). */ + unsigned int hook_jobs; + /* Configurations related to promisor remotes. */ char *repository_format_partial_clone; struct promisor_remote_config *promisor_remote_config; From 680e69f60d2b3838bb98938dbd3e8881bdfde7d6 Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Fri, 10 Apr 2026 12:05:59 +0300 Subject: [PATCH 32/61] hook: allow parallel hook execution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hooks always run in sequential order due to the hardcoded jobs == 1 passed to run_process_parallel(). Remove that hardcoding to allow users to run hooks in parallel (opt-in). Users need to decide which hooks to run in parallel, by specifying "parallel = true" in the config, because Git cannot know if their specific hooks are safe to run or not in parallel (for e.g. two hooks might write to the same file or call the same program). Some hooks are unsafe to run in parallel by design: these will marked in the next commit using RUN_HOOKS_OPT_INIT_FORCE_SERIAL. The hook.jobs config specifies the default number of jobs applied to all hooks which have parallelism enabled. Signed-off-by: Emily Shaffer Helped-by: Ævar Arnfjörð Bjarmason Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 13 +++ hook.c | 79 ++++++++++++++++-- hook.h | 25 ++++++ t/t1800-hook.sh | 142 +++++++++++++++++++++++++++++++++ 4 files changed, 253 insertions(+), 6 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index b7847f9338c65f..21800db648dca5 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -23,6 +23,19 @@ hook..enabled:: in a system or global config file and needs to be disabled for a specific repository. See linkgit:git-hook[1]. +hook..parallel:: + Whether the hook `hook.` may run in parallel with other hooks + for the same event. Defaults to `false`. Set to `true` only when the + hook script is safe to run concurrently with other hooks for the same + event. If any hook for an event does not have this set to `true`, + all hooks for that event run sequentially regardless of `hook.jobs`. + Only configured (named) hooks need to declare this. Traditional hooks + found in the hooks directory do not need to, and run in parallel when + the effective job count is greater than 1. See linkgit:git-hook[1]. + hook.jobs:: Specifies how many hooks can be run simultaneously during parallelized hook execution. If unspecified, defaults to 1 (serial execution). ++ +This setting has no effect unless all configured hooks for the event have +`hook..parallel` set to `true`. diff --git a/hook.c b/hook.c index b8cce00e578d3c..85c0de5e47b426 100644 --- a/hook.c +++ b/hook.c @@ -116,6 +116,7 @@ struct hook_config_cache_entry { char *command; enum config_scope scope; bool disabled; + bool parallel; }; /* @@ -123,12 +124,14 @@ struct hook_config_cache_entry { * commands: friendly-name to command map. * event_hooks: event-name to list of friendly-names map. * disabled_hooks: set of friendly-names with hook..enabled = false. + * parallel_hooks: friendly-name to parallel flag. * jobs: value of the global hook.jobs key. Defaults to 0 if unset (stored in r->hook_jobs). */ struct hook_all_config_cb { struct strmap commands; struct strmap event_hooks; struct string_list disabled_hooks; + struct strmap parallel_hooks; unsigned int jobs; }; @@ -219,6 +222,15 @@ static int hook_config_lookup_all(const char *key, const char *value, default: break; /* ignore unrecognised values */ } + } else if (!strcmp(subkey, "parallel")) { + int v = git_parse_maybe_bool(value); + if (v >= 0) + strmap_put(&data->parallel_hooks, hook_name, + (void *)(uintptr_t)v); + else + warning(_("hook.%s.parallel must be a boolean," + " ignoring: '%s'"), + hook_name, value); } free(hook_name); @@ -263,6 +275,7 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) strmap_init(&cb_data.commands); strmap_init(&cb_data.event_hooks); string_list_init_dup(&cb_data.disabled_hooks); + strmap_init(&cb_data.parallel_hooks); /* Parse all configs in one run, capturing hook.* including hook.jobs. */ repo_config(r, hook_config_lookup_all, &cb_data); @@ -282,6 +295,7 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) struct hook_config_cache_entry *entry; char *command; + bool is_par = !!strmap_get(&cb_data.parallel_hooks, hname); bool is_disabled = !!unsorted_string_list_lookup( &cb_data.disabled_hooks, hname); @@ -302,6 +316,7 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) entry->command = xstrdup_or_null(command); entry->scope = scope; entry->disabled = is_disabled; + entry->parallel = is_par; string_list_append(hooks, hname)->util = entry; } @@ -312,6 +327,7 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) r->hook_jobs = cb_data.jobs; strmap_clear(&cb_data.commands, 1); + strmap_clear(&cb_data.parallel_hooks, 0); /* values are uintptr_t, not heap ptrs */ string_list_clear(&cb_data.disabled_hooks, 0); strmap_for_each_entry(&cb_data.event_hooks, &iter, e) { string_list_clear(e->value, 0); @@ -389,6 +405,7 @@ static void list_hooks_add_configured(struct repository *r, entry->command ? xstrdup(entry->command) : NULL; hook->u.configured.scope = entry->scope; hook->u.configured.disabled = entry->disabled; + hook->parallel = entry->parallel; string_list_append(list, friendly_name)->util = hook; } @@ -538,21 +555,75 @@ static void run_hooks_opt_clear(struct run_hooks_opt *options) strvec_clear(&options->args); } +/* Determine how many jobs to use for hook execution. */ +static unsigned int get_hook_jobs(struct repository *r, + struct run_hooks_opt *options, + struct string_list *hook_list) +{ + /* + * Hooks needing separate output streams must run sequentially. + * Next commit will allow parallelizing these as well. + */ + if (!options->stdout_to_stderr) + return 1; + + /* + * An explicit job count overrides everything else: this covers both + * FORCE_SERIAL callers (for hooks that must never run in parallel) + * and the -j flag from the CLI. The CLI override is intentional: users + * may want to serialize hooks declared parallel or to parallelize more + * aggressively than the default. + */ + if (options->jobs) + return options->jobs; + + /* + * Use hook.jobs from the already-parsed config cache (in-repo), or + * fallback to a direct config lookup (out-of-repo). + * Default to 1 (serial execution) on failure. + */ + options->jobs = 1; + if (r) { + if (r->gitdir && r->hook_config_cache && r->hook_jobs) + options->jobs = r->hook_jobs; + else + repo_config_get_uint(r, "hook.jobs", &options->jobs); + } + + /* + * Cap to serial any configured hook not marked as parallel = true. + * This enforces the parallel = false default, even for "traditional" + * hooks from the hookdir which cannot be marked parallel = true. + */ + for (size_t i = 0; i < hook_list->nr; i++) { + struct hook *h = hook_list->items[i].util; + if (h->kind == HOOK_CONFIGURED && !h->parallel) { + options->jobs = 1; + break; + } + } + + return options->jobs; +} + int run_hooks_opt(struct repository *r, const char *hook_name, struct run_hooks_opt *options) { + struct string_list *hook_list = list_hooks(r, hook_name, options); struct hook_cb_data cb_data = { .rc = 0, .hook_name = hook_name, + .hook_command_list = hook_list, .options = options, }; int ret = 0; + unsigned int jobs = get_hook_jobs(r, options, hook_list); const struct run_process_parallel_opts opts = { .tr2_category = "hook", .tr2_label = hook_name, - .processes = options->jobs, - .ungroup = options->jobs == 1, + .processes = jobs, + .ungroup = jobs == 1, .get_next_task = pick_next_hook, .start_failure = notify_start_failure, @@ -568,9 +639,6 @@ int run_hooks_opt(struct repository *r, const char *hook_name, if (options->path_to_stdin && options->feed_pipe) BUG("options path_to_stdin and feed_pipe are mutually exclusive"); - if (!options->jobs) - BUG("run_hooks_opt must be called with options.jobs >= 1"); - /* * Ensure cb_data copy and free functions are either provided together, * or neither one is provided. @@ -581,7 +649,6 @@ int run_hooks_opt(struct repository *r, const char *hook_name, if (options->invoked_hook) *options->invoked_hook = 0; - cb_data.hook_command_list = list_hooks(r, hook_name, options); if (!cb_data.hook_command_list->nr) { if (options->error_if_missing) ret = error("cannot find a hook named %s", hook_name); diff --git a/hook.h b/hook.h index 5c5628dd1f822c..ba7056f8723b73 100644 --- a/hook.h +++ b/hook.h @@ -35,6 +35,13 @@ struct hook { } configured; } u; + /** + * Whether this hook may run in parallel with other hooks for the same + * event. Only useful for configured (named) hooks. Traditional hooks + * always default to 0 (serial). Set via `hook..parallel = true`. + */ + bool parallel; + /** * Opaque data pointer used to keep internal state across callback calls. * @@ -72,6 +79,8 @@ struct run_hooks_opt { * * If > 1, output will be buffered and de-interleaved (ungroup=0). * If == 1, output will be real-time (ungroup=1). + * If == 0, the 'hook.jobs' config is used or, if the config is unset, + * defaults to 1 (serial execution). */ unsigned int jobs; @@ -152,7 +161,23 @@ struct run_hooks_opt { hook_data_free_fn feed_pipe_cb_data_free; }; +/** + * Default initializer for hooks. Parallelism is opt-in: .jobs = 0 defers to + * the 'hook.jobs' config, falling back to serial (1) if unset. + */ #define RUN_HOOKS_OPT_INIT { \ + .env = STRVEC_INIT, \ + .args = STRVEC_INIT, \ + .stdout_to_stderr = 1, \ + .jobs = 0, \ +} + +/** + * Initializer for hooks that must always run sequentially regardless of + * 'hook.jobs'. Use this when git knows the hook cannot safely be parallelized + * .jobs = 1 is non-overridable. + */ +#define RUN_HOOKS_OPT_INIT_FORCE_SERIAL { \ .env = STRVEC_INIT, \ .args = STRVEC_INIT, \ .stdout_to_stderr = 1, \ diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 33decc66c0ea8d..a3011a01ca2908 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -21,6 +21,57 @@ setup_hookdir () { test_when_finished rm -rf .git/hooks } +# write_sentinel_hook [sentinel] +# +# Writes a hook that marks itself as started, sleeps for a few seconds, then +# marks itself done. The sleep must be long enough that sentinel_detector can +# observe .started before .done appears when both hooks +# run concurrently in parallel mode. +write_sentinel_hook () { + sentinel="${2:-sentinel}" + write_script "$1" <<-EOF + touch ${sentinel}.started && + sleep 2 && + touch ${sentinel}.done + EOF +} + +# sentinel_detector +# +# Returns a shell command string suitable for use as hook..command. +# The detector must be registered after the sentinel: +# 1. In serial mode, the sentinel has completed (and .done exists) +# before the detector starts. +# 2. In parallel mode, both run concurrently so .done has not appeared +# yet and the detector just sees .started. +# +# At start, poll until .started exists to absorb startup jitter, then +# write to : +# 1. 'serial' if .done exists (sentinel finished before we started), +# 2. 'parallel' if only .started exists (sentinel still running), +# 3. 'timeout' if .started never appeared. +# +# The command ends with ':' so when git appends "$@" for hooks that receive +# positional arguments (e.g. pre-push), the result ': "$@"' is valid shell +# rather than a syntax error 'fi "$@"'. +sentinel_detector () { + cat <<-EOF + i=0 + while ! test -f ${1}.started && test \$i -lt 10; do + sleep 1 + i=\$((i+1)) + done + if test -f ${1}.done; then + echo serial >${2} + elif test -f ${1}.started; then + echo parallel >${2} + else + echo timeout >${2} + fi + : + EOF +} + test_expect_success 'git hook usage' ' test_expect_code 129 git hook && test_expect_code 129 git hook run && @@ -658,4 +709,95 @@ test_expect_success 'server push-to-checkout hook expects stdout redirected to s check_stdout_merged_to_stderr push-to-checkout ' +test_expect_success 'hook.jobs=1 config runs hooks in series' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + + # Use two configured hooks so the execution order is deterministic: + # hook-1 (sentinel) is listed before hook-2 (detector), so hook-1 + # always runs first even in serial mode. + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command \ + "touch sentinel.started; sleep 2; touch sentinel.done" && + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + + test_config hook.jobs 1 && + + git hook run --allow-unknown-hook-name test-hook >out 2>err && + echo serial >expect && + test_cmp expect hook.order +' + +test_expect_success 'hook.jobs=2 config runs hooks in parallel' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + test_when_finished "rm -rf .git/hooks" && + + mkdir -p .git/hooks && + write_sentinel_hook .git/hooks/test-hook && + + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + test_config hook.hook-2.parallel true && + + test_config hook.jobs 2 && + + git hook run --allow-unknown-hook-name test-hook >out 2>err && + echo parallel >expect && + test_cmp expect hook.order +' + +test_expect_success 'hook..parallel=true enables parallel execution' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command \ + "touch sentinel.started; sleep 2; touch sentinel.done" && + test_config hook.hook-1.parallel true && + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + test_config hook.hook-2.parallel true && + + test_config hook.jobs 2 && + + git hook run --allow-unknown-hook-name test-hook >out 2>err && + echo parallel >expect && + test_cmp expect hook.order +' + +test_expect_success 'hook..parallel=false (default) forces serial execution' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command \ + "touch sentinel.started; sleep 2; touch sentinel.done" && + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + + test_config hook.jobs 2 && + + git hook run --allow-unknown-hook-name test-hook >out 2>err && + echo serial >expect && + test_cmp expect hook.order +' + +test_expect_success 'one non-parallel hook forces the whole event to run serially' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command \ + "touch sentinel.started; sleep 2; touch sentinel.done" && + test_config hook.hook-1.parallel true && + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + # hook-2 has no parallel=true: should force serial for all + + test_config hook.jobs 2 && + + git hook run --allow-unknown-hook-name test-hook >out 2>err && + echo serial >expect && + test_cmp expect hook.order +' + test_done From f776b77f0032fb342d567156626ef3fe9586443f Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Fri, 10 Apr 2026 12:06:00 +0300 Subject: [PATCH 33/61] hook: allow pre-push parallel execution pre-push is the only hook that keeps stdout and stderr separate (for backwards compatibility with git-lfs and potentially other users). This prevents parallelizing it because run-command needs stdout_to_stderr=1 to buffer and de-interleave parallel outputs. Since we now default to jobs=1, backwards compatibility is maintained without needing any extension or extra config: when no parallelism is requested, pre-push behaves exactly as before. When the user explicitly opts into parallelism via hook.jobs > 1, hook..jobs > 1, or -jN, they accept the changed output behavior. Document this and let get_hook_jobs() set stdout_to_stderr=1 automatically when jobs > 1, removing the need for any extension infrastructure. Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 4 ++++ hook.c | 24 ++++++++++++++++-------- hook.h | 6 ++++-- t/t1800-hook.sh | 32 ++++++++++++++++++++++++++++++++ transport.c | 6 ++++-- 5 files changed, 60 insertions(+), 12 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index 21800db648dca5..94c7a9808e29ef 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -39,3 +39,7 @@ hook.jobs:: + This setting has no effect unless all configured hooks for the event have `hook..parallel` set to `true`. ++ +For `pre-push` hooks, which normally keep stdout and stderr separate, +setting this to a value greater than 1 (or passing `-j`) will merge stdout +into stderr to allow correct de-interleaving of parallel output. diff --git a/hook.c b/hook.c index 85c0de5e47b426..25762b6c8d18f9 100644 --- a/hook.c +++ b/hook.c @@ -555,18 +555,24 @@ static void run_hooks_opt_clear(struct run_hooks_opt *options) strvec_clear(&options->args); } +/* + * When running in parallel, stdout must be merged into stderr so + * run-command can buffer and de-interleave outputs correctly. This + * applies even to hooks like pre-push that normally keep stdout and + * stderr separate: the user has opted into parallelism, so the output + * stream behavior changes accordingly. + */ +static void merge_output_if_parallel(struct run_hooks_opt *options) +{ + if (options->jobs > 1) + options->stdout_to_stderr = 1; +} + /* Determine how many jobs to use for hook execution. */ static unsigned int get_hook_jobs(struct repository *r, struct run_hooks_opt *options, struct string_list *hook_list) { - /* - * Hooks needing separate output streams must run sequentially. - * Next commit will allow parallelizing these as well. - */ - if (!options->stdout_to_stderr) - return 1; - /* * An explicit job count overrides everything else: this covers both * FORCE_SERIAL callers (for hooks that must never run in parallel) @@ -575,7 +581,7 @@ static unsigned int get_hook_jobs(struct repository *r, * aggressively than the default. */ if (options->jobs) - return options->jobs; + goto cleanup; /* * Use hook.jobs from the already-parsed config cache (in-repo), or @@ -603,6 +609,8 @@ static unsigned int get_hook_jobs(struct repository *r, } } +cleanup: + merge_output_if_parallel(options); return options->jobs; } diff --git a/hook.h b/hook.h index ba7056f8723b73..01db4226a60306 100644 --- a/hook.h +++ b/hook.h @@ -106,8 +106,10 @@ struct run_hooks_opt { * Send the hook's stdout to stderr. * * This is the default behavior for all hooks except pre-push, - * which has separate stdout and stderr streams for backwards - * compatibility reasons. + * which keeps stdout and stderr separate for backwards compatibility. + * When parallel execution is requested (jobs > 1), get_hook_jobs() + * overrides this to 1 for all hooks so run-command can de-interleave + * their outputs correctly. */ unsigned int stdout_to_stderr:1; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index a3011a01ca2908..4a978aff5e0c1e 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -800,4 +800,36 @@ test_expect_success 'one non-parallel hook forces the whole event to run seriall test_cmp expect hook.order ' +test_expect_success 'client hooks: pre-push parallel execution merges stdout to stderr' ' + test_when_finished "rm -rf remote-par stdout.actual stderr.actual" && + git init --bare remote-par && + git remote add origin-par remote-par && + test_commit par-commit && + mkdir -p .git/hooks && + setup_hooks pre-push && + test_config hook.jobs 2 && + git push origin-par HEAD:main >stdout.actual 2>stderr.actual && + check_stdout_merged_to_stderr pre-push +' + +test_expect_success 'client hooks: pre-push runs in parallel when hook.jobs > 1' ' + test_when_finished "rm -rf repo-parallel remote-parallel" && + git init --bare remote-parallel && + git init repo-parallel && + git -C repo-parallel remote add origin ../remote-parallel && + test_commit -C repo-parallel A && + + write_sentinel_hook repo-parallel/.git/hooks/pre-push && + git -C repo-parallel config hook.hook-2.event pre-push && + git -C repo-parallel config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + git -C repo-parallel config hook.hook-2.parallel true && + + git -C repo-parallel config hook.jobs 2 && + + git -C repo-parallel push origin HEAD >out 2>err && + echo parallel >expect && + test_cmp expect repo-parallel/hook.order +' + test_done diff --git a/transport.c b/transport.c index e53936d87b641f..9406ec4f2d682a 100644 --- a/transport.c +++ b/transport.c @@ -1391,8 +1391,10 @@ static int run_pre_push_hook(struct transport *transport, opt.feed_pipe_cb_data_free = pre_push_hook_data_free; /* - * pre-push hooks expect stdout & stderr to be separate, so don't merge - * them to keep backwards compatibility with existing hooks. + * pre-push hooks keep stdout and stderr separate by default for + * backwards compatibility. When the user opts into parallel execution + * via hook.jobs > 1 or -j, get_hook_jobs() will set stdout_to_stderr=1 + * automatically so run-command can de-interleave the outputs. */ opt.stdout_to_stderr = 0; From ae25764e50f38b6625e11c3a7d7de290a0075b9c Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Fri, 10 Apr 2026 12:06:01 +0300 Subject: [PATCH 34/61] hook: mark non-parallelizable hooks Several hooks are known to be inherently non-parallelizable, so initialize them with RUN_HOOKS_OPT_INIT_FORCE_SERIAL. This pins jobs=1 and overrides any hook.jobs or runtime -j flags. These hooks are: applypatch-msg, pre-commit, prepare-commit-msg, commit-msg, post-commit, post-checkout, and push-to-checkout. Signed-off-by: Emily Shaffer Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 14 ++++++++++++++ builtin/am.c | 8 +++++--- builtin/checkout.c | 19 +++++++++++++------ builtin/clone.c | 6 ++++-- builtin/receive-pack.c | 3 ++- builtin/worktree.c | 2 +- commit.c | 2 +- t/t1800-hook.sh | 16 ++++++++++++++++ 8 files changed, 56 insertions(+), 14 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index 94c7a9808e29ef..6f60775c28a902 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -36,6 +36,20 @@ hook..parallel:: hook.jobs:: Specifies how many hooks can be run simultaneously during parallelized hook execution. If unspecified, defaults to 1 (serial execution). + Some hooks always run sequentially regardless of this setting because + they operate on shared data and cannot safely be parallelized: ++ +-- +`applypatch-msg`;; +`prepare-commit-msg`;; +`commit-msg`;; + Receive a commit message file and may rewrite it in place. +`pre-commit`;; +`post-checkout`;; +`push-to-checkout`;; +`post-commit`;; + Access the working tree, index, or repository state. +-- + This setting has no effect unless all configured hooks for the event have `hook..parallel` set to `true`. diff --git a/builtin/am.c b/builtin/am.c index fe6e087eee9ff5..e9623b8307793f 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -490,9 +490,11 @@ static int run_applypatch_msg_hook(struct am_state *state) assert(state->msg); - if (!state->no_verify) - ret = run_hooks_l(the_repository, "applypatch-msg", - am_path(state, "final-commit"), NULL); + if (!state->no_verify) { + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL; + strvec_push(&opt.args, am_path(state, "final-commit")); + ret = run_hooks_opt(the_repository, "applypatch-msg", &opt); + } if (!ret) { FREE_AND_NULL(state->msg); diff --git a/builtin/checkout.c b/builtin/checkout.c index e031e6188613a6..ac0186a33e559a 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -31,6 +31,7 @@ #include "resolve-undo.h" #include "revision.h" #include "setup.h" +#include "strvec.h" #include "submodule.h" #include "symlinks.h" #include "trace2.h" @@ -123,13 +124,19 @@ static void branch_info_release(struct branch_info *info) static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit, int changed) { - return run_hooks_l(the_repository, "post-checkout", - oid_to_hex(old_commit ? &old_commit->object.oid : null_oid(the_hash_algo)), - oid_to_hex(new_commit ? &new_commit->object.oid : null_oid(the_hash_algo)), - changed ? "1" : "0", NULL); - /* "new_commit" can be NULL when checking out from the index before - a commit exists. */ + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL; + /* + * "new_commit" can be NULL when checking out from the index before + * a commit exists. + */ + strvec_pushl(&opt.args, + oid_to_hex(old_commit ? &old_commit->object.oid : null_oid(the_hash_algo)), + oid_to_hex(new_commit ? &new_commit->object.oid : null_oid(the_hash_algo)), + changed ? "1" : "0", + NULL); + + return run_hooks_opt(the_repository, "post-checkout", &opt); } static int update_some(const struct object_id *oid, struct strbuf *base, diff --git a/builtin/clone.c b/builtin/clone.c index fba3c9c508bc06..d23b0cafcfec30 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -647,6 +647,7 @@ static int checkout(int submodule_progress, struct tree *tree; struct tree_desc t; int err = 0; + struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL; if (option_no_checkout) return 0; @@ -697,8 +698,9 @@ static int checkout(int submodule_progress, if (write_locked_index(the_repository->index, &lock_file, COMMIT_LOCK)) die(_("unable to write new index file")); - err |= run_hooks_l(the_repository, "post-checkout", oid_to_hex(null_oid(the_hash_algo)), - oid_to_hex(&oid), "1", NULL); + strvec_pushl(&hook_opt.args, oid_to_hex(null_oid(the_hash_algo)), + oid_to_hex(&oid), "1", NULL); + err |= run_hooks_opt(the_repository, "post-checkout", &hook_opt); if (!err && (option_recurse_submodules.nr > 0)) { struct child_process cmd = CHILD_PROCESS_INIT; diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index dada55884a0b06..6da60f640ce3af 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1455,7 +1455,8 @@ static const char *push_to_checkout(unsigned char *hash, struct strvec *env, const char *work_tree) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL; + opt.invoked_hook = invoked_hook; strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree)); diff --git a/builtin/worktree.c b/builtin/worktree.c index 4fd6f7575f9f76..d21c43fde38b5e 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -609,7 +609,7 @@ static int add_worktree(const char *path, const char *refname, * is_junk is cleared, but do return appropriate code when hook fails. */ if (!ret && opts->checkout && !opts->orphan) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL; strvec_pushl(&opt.env, "GIT_DIR", "GIT_WORK_TREE", NULL); strvec_pushl(&opt.args, diff --git a/commit.c b/commit.c index 80d8d078757dbc..4385ae4329e921 100644 --- a/commit.c +++ b/commit.c @@ -1970,7 +1970,7 @@ size_t ignored_log_message_bytes(const char *buf, size_t len) int run_commit_hook(int editor_is_used, const char *index_file, int *invoked_hook, const char *name, ...) { - struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL; va_list args; const char *arg; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 4a978aff5e0c1e..63fa25bca23c51 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -832,4 +832,20 @@ test_expect_success 'client hooks: pre-push runs in parallel when hook.jobs > 1' test_cmp expect repo-parallel/hook.order ' +test_expect_success 'hook.jobs=2 is ignored for force-serial hooks (pre-commit)' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + test_config hook.hook-1.event pre-commit && + test_config hook.hook-1.command \ + "touch sentinel.started; sleep 2; touch sentinel.done" && + test_config hook.hook-1.parallel true && + test_config hook.hook-2.event pre-commit && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + test_config hook.hook-2.parallel true && + test_config hook.jobs 2 && + git commit --allow-empty -m "test: verify force-serial on pre-commit" && + echo serial >expect && + test_cmp expect hook.order +' + test_done From 091d2dbeb452b2c8223c622b54e96ebd273b5a78 Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Fri, 10 Apr 2026 12:06:02 +0300 Subject: [PATCH 35/61] hook: add -j/--jobs option to git hook run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expose the parallel job count as a command-line flag so callers can request parallelism without relying only on the hook.jobs config. Add tests covering serial/parallel execution and TTY behaviour under -j1 vs -jN. Signed-off-by: Emily Shaffer Helped-by: Ævar Arnfjörð Bjarmason Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/git-hook.adoc | 23 +++++- builtin/hook.c | 5 +- hook.c | 17 +++++ t/t1800-hook.sh | 135 ++++++++++++++++++++++++++++++++++-- 4 files changed, 170 insertions(+), 10 deletions(-) diff --git a/Documentation/git-hook.adoc b/Documentation/git-hook.adoc index 318c637bd8eba5..46ea52db55f268 100644 --- a/Documentation/git-hook.adoc +++ b/Documentation/git-hook.adoc @@ -8,7 +8,8 @@ git-hook - Run git hooks SYNOPSIS -------- [verse] -'git hook' run [--allow-unknown-hook-name] [--ignore-missing] [--to-stdin=] [-- ] +'git hook' run [--allow-unknown-hook-name] [--ignore-missing] [--to-stdin=] [(-j|--jobs) ] + [-- ] 'git hook' list [--allow-unknown-hook-name] [-z] [--show-scope] DESCRIPTION @@ -147,6 +148,23 @@ OPTIONS mirroring the output style of `git config --show-scope`. Traditional hooks from the hookdir are unaffected. +-j:: +--jobs:: + Only valid for `run`. ++ +Specify how many hooks to run simultaneously. If this flag is not specified, +the value of the `hook.jobs` config is used, see linkgit:git-config[1]. If +neither is specified, defaults to 1 (serial execution). ++ +When greater than 1, it overrides the per-hook `hook..parallel` +setting, allowing all hooks for the event to run concurrently, even if they +are not individually marked as parallel. ++ +Some hooks always run sequentially regardless of this flag or the +`hook.jobs` config, because git knows they cannot safely run in parallel: +`applypatch-msg`, `pre-commit`, `prepare-commit-msg`, `commit-msg`, +`post-commit`, `post-checkout`, and `push-to-checkout`. + WRAPPERS -------- @@ -169,7 +187,8 @@ running: git hook run --allow-unknown-hook-name mywrapper-start-tests \ # providing something to stdin --stdin some-tempfile-123 \ - # execute hooks in serial + # execute multiple hooks in parallel + --jobs 3 \ # plus some arguments of your own... -- \ --testname bar \ diff --git a/builtin/hook.c b/builtin/hook.c index c0585587e5e4fa..bea0668b475931 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -8,7 +8,8 @@ #include "parse-options.h" #define BUILTIN_HOOK_RUN_USAGE \ - N_("git hook run [--allow-unknown-hook-name] [--ignore-missing] [--to-stdin=] [-- ]") + N_("git hook run [--allow-unknown-hook-name] [--ignore-missing] [--to-stdin=] [(-j|--jobs) ]\n" \ + " [-- ]") #define BUILTIN_HOOK_LIST_USAGE \ N_("git hook list [--allow-unknown-hook-name] [-z] [--show-scope] ") @@ -132,6 +133,8 @@ static int run(int argc, const char **argv, const char *prefix, N_("silently ignore missing requested ")), OPT_STRING(0, "to-stdin", &opt.path_to_stdin, N_("path"), N_("file to read into hooks' stdin")), + OPT_UNSIGNED('j', "jobs", &opt.jobs, + N_("run up to hooks simultaneously")), OPT_END(), }; int ret; diff --git a/hook.c b/hook.c index 25762b6c8d18f9..c0b71322cf2ef6 100644 --- a/hook.c +++ b/hook.c @@ -568,6 +568,22 @@ static void merge_output_if_parallel(struct run_hooks_opt *options) options->stdout_to_stderr = 1; } +static void warn_non_parallel_hooks_override(unsigned int jobs, + struct string_list *hook_list) +{ + /* Don't warn for hooks running sequentially. */ + if (jobs == 1) + return; + + for (size_t i = 0; i < hook_list->nr; i++) { + struct hook *h = hook_list->items[i].util; + if (h->kind == HOOK_CONFIGURED && !h->parallel) + warning(_("hook '%s' is not marked as parallel=true, " + "running in parallel anyway due to -j%u"), + h->u.configured.friendly_name, jobs); + } +} + /* Determine how many jobs to use for hook execution. */ static unsigned int get_hook_jobs(struct repository *r, struct run_hooks_opt *options, @@ -611,6 +627,7 @@ static unsigned int get_hook_jobs(struct repository *r, cleanup: merge_output_if_parallel(options); + warn_non_parallel_hooks_override(options->jobs, hook_list); return options->jobs; } diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 63fa25bca23c51..aa37a5181a0e0e 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -268,10 +268,20 @@ test_expect_success 'git -c core.hooksPath= hook run' ' ' test_hook_tty () { - cat >expect <<-\EOF - STDOUT TTY - STDERR TTY - EOF + expect_tty=$1 + shift + + if test "$expect_tty" != "no_tty"; then + cat >expect <<-\EOF + STDOUT TTY + STDERR TTY + EOF + else + cat >expect <<-\EOF + STDOUT NO TTY + STDERR NO TTY + EOF + fi test_when_finished "rm -rf repo" && git init repo && @@ -289,12 +299,21 @@ test_hook_tty () { test_cmp expect repo/actual } -test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY' ' - test_hook_tty hook run pre-commit +test_expect_success TTY 'git hook run -j1: stdout and stderr are connected to a TTY' ' + # hooks running sequentially (-j1) are always connected to the tty for + # optimum real-time performance. + test_hook_tty tty hook run -j1 pre-commit +' + +test_expect_success TTY 'git hook run -jN: stdout and stderr are not connected to a TTY' ' + # Hooks are not connected to the tty when run in parallel, instead they + # output to a pipe through which run-command collects and de-interlaces + # their outputs, which then gets passed either to the tty or a sideband. + test_hook_tty no_tty hook run -j2 pre-commit ' test_expect_success TTY 'git commit: stdout and stderr are connected to a TTY' ' - test_hook_tty commit -m"B.new" + test_hook_tty tty commit -m"B.new" ' test_expect_success 'git hook list orders by config order' ' @@ -709,6 +728,108 @@ test_expect_success 'server push-to-checkout hook expects stdout redirected to s check_stdout_merged_to_stderr push-to-checkout ' +test_expect_success 'parallel hook output is not interleaved' ' + test_when_finished "rm -rf .git/hooks" && + + write_script .git/hooks/test-hook <<-EOF && + echo "Hook 1 Start" + sleep 1 + echo "Hook 1 End" + EOF + + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "echo \"Hook 2 Start\"; sleep 2; echo \"Hook 2 End\"" && + test_config hook.hook-2.parallel true && + test_config hook.hook-3.event test-hook && + test_config hook.hook-3.command \ + "echo \"Hook 3 Start\"; sleep 3; echo \"Hook 3 End\"" && + test_config hook.hook-3.parallel true && + + git hook run --allow-unknown-hook-name -j3 test-hook >out 2>err.parallel && + + # Verify Hook 1 output is grouped + sed -n "/Hook 1 Start/,/Hook 1 End/p" err.parallel >hook1_out && + test_line_count = 2 hook1_out && + + # Verify Hook 2 output is grouped + sed -n "/Hook 2 Start/,/Hook 2 End/p" err.parallel >hook2_out && + test_line_count = 2 hook2_out && + + # Verify Hook 3 output is grouped + sed -n "/Hook 3 Start/,/Hook 3 End/p" err.parallel >hook3_out && + test_line_count = 2 hook3_out +' + +test_expect_success 'git hook run -j1 runs hooks in series' ' + test_when_finished "rm -rf .git/hooks" && + + test_config hook.series-1.event "test-hook" && + test_config hook.series-1.command "echo 1" --add && + test_config hook.series-2.event "test-hook" && + test_config hook.series-2.command "echo 2" --add && + + mkdir -p .git/hooks && + write_script .git/hooks/test-hook <<-EOF && + echo 3 + EOF + + cat >expected <<-\EOF && + 1 + 2 + 3 + EOF + + git hook run --allow-unknown-hook-name -j1 test-hook 2>actual && + test_cmp expected actual +' + +test_expect_success 'git hook run -j2 runs hooks in parallel' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + test_when_finished "rm -rf .git/hooks" && + + mkdir -p .git/hooks && + write_sentinel_hook .git/hooks/test-hook && + + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + test_config hook.hook-2.parallel true && + + git hook run --allow-unknown-hook-name -j2 test-hook >out 2>err && + echo parallel >expect && + test_cmp expect hook.order +' + +test_expect_success 'git hook run -j2 overrides parallel=false' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command \ + "touch sentinel.started; sleep 2; touch sentinel.done" && + # hook-1 intentionally has no parallel=true + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + # hook-2 also has no parallel=true + + # -j2 overrides parallel=false; hooks run in parallel with a warning. + git hook run --allow-unknown-hook-name -j2 test-hook >out 2>err && + echo parallel >expect && + test_cmp expect hook.order +' + +test_expect_success 'git hook run -j2 warns for hooks not marked parallel=true' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "true" && + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command "true" && + # neither hook has parallel=true + + git hook run --allow-unknown-hook-name -j2 test-hook >out 2>err && + grep "hook .hook-1. is not marked as parallel=true" err && + grep "hook .hook-2. is not marked as parallel=true" err +' + test_expect_success 'hook.jobs=1 config runs hooks in series' ' test_when_finished "rm -f sentinel.started sentinel.done hook.order" && From 084a55b3adf33f70c84091d5957b8bede9b01174 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Fri, 10 Apr 2026 12:06:03 +0300 Subject: [PATCH 36/61] hook: add per-event jobs config Add a hook..jobs count config that allows users to override the global hook.jobs setting for specific hook events. This allows finer-grained control over parallelism on a per-event basis. For example, to run `post-receive` hooks with up to 4 parallel jobs while keeping other events at their global default: [hook] post-receive.jobs = 4 Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 19 +++++++++++ hook.c | 46 +++++++++++++++++++++++--- repository.c | 1 + repository.h | 3 ++ t/t1800-hook.sh | 59 ++++++++++++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 5 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index 6f60775c28a902..d4fa29d936d6e2 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -33,9 +33,28 @@ hook..parallel:: found in the hooks directory do not need to, and run in parallel when the effective job count is greater than 1. See linkgit:git-hook[1]. +hook..jobs:: + Specifies how many hooks can be run simultaneously for the `` + hook event (e.g. `hook.post-receive.jobs = 4`). Overrides `hook.jobs` + for this specific event. The same parallelism restrictions apply: this + setting has no effect unless all configured hooks for the event have + `hook..parallel` set to `true`. Must be a positive int, + zero is rejected with a warning. See linkgit:git-hook[1]. ++ +Note on naming: although this key resembles `hook..*` +(a per-hook setting), `` must be the event name, not a hook +friendly name. The key component is stored literally and looked up by +event name at runtime with no translation between the two namespaces. +A key like `hook.my-hook.jobs` is stored under `"my-hook"` but the +lookup at runtime uses the event name (e.g. `"post-receive"`), so +`hook.my-hook.jobs` is silently ignored even when `my-hook` is +registered for that event. Use `hook.post-receive.jobs` or any other +valid event name when setting `hook..jobs`. + hook.jobs:: Specifies how many hooks can be run simultaneously during parallelized hook execution. If unspecified, defaults to 1 (serial execution). + Can be overridden on a per-event basis with `hook..jobs`. Some hooks always run sequentially regardless of this setting because they operate on shared data and cannot safely be parallelized: + diff --git a/hook.c b/hook.c index c0b71322cf2ef6..d98b01156366a4 100644 --- a/hook.c +++ b/hook.c @@ -125,6 +125,7 @@ struct hook_config_cache_entry { * event_hooks: event-name to list of friendly-names map. * disabled_hooks: set of friendly-names with hook..enabled = false. * parallel_hooks: friendly-name to parallel flag. + * event_jobs: event-name to per-event jobs count (stored as uintptr_t, NULL == unset). * jobs: value of the global hook.jobs key. Defaults to 0 if unset (stored in r->hook_jobs). */ struct hook_all_config_cb { @@ -132,6 +133,7 @@ struct hook_all_config_cb { struct strmap event_hooks; struct string_list disabled_hooks; struct strmap parallel_hooks; + struct strmap event_jobs; unsigned int jobs; }; @@ -231,6 +233,18 @@ static int hook_config_lookup_all(const char *key, const char *value, warning(_("hook.%s.parallel must be a boolean," " ignoring: '%s'"), hook_name, value); + } else if (!strcmp(subkey, "jobs")) { + unsigned int v; + if (!git_parse_uint(value, &v)) + warning(_("hook.%s.jobs must be a positive integer," + " ignoring: '%s'"), + hook_name, value); + else if (!v) + warning(_("hook.%s.jobs must be positive," + " ignoring: 0"), hook_name); + else + strmap_put(&data->event_jobs, hook_name, + (void *)(uintptr_t)v); } free(hook_name); @@ -276,6 +290,7 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) strmap_init(&cb_data.event_hooks); string_list_init_dup(&cb_data.disabled_hooks); strmap_init(&cb_data.parallel_hooks); + strmap_init(&cb_data.event_jobs); /* Parse all configs in one run, capturing hook.* including hook.jobs. */ repo_config(r, hook_config_lookup_all, &cb_data); @@ -323,8 +338,10 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) strmap_put(cache, e->key, hooks); } - if (r) + if (r) { r->hook_jobs = cb_data.jobs; + r->event_jobs = cb_data.event_jobs; + } strmap_clear(&cb_data.commands, 1); strmap_clear(&cb_data.parallel_hooks, 0); /* values are uintptr_t, not heap ptrs */ @@ -587,6 +604,7 @@ static void warn_non_parallel_hooks_override(unsigned int jobs, /* Determine how many jobs to use for hook execution. */ static unsigned int get_hook_jobs(struct repository *r, struct run_hooks_opt *options, + const char *hook_name, struct string_list *hook_list) { /* @@ -606,16 +624,34 @@ static unsigned int get_hook_jobs(struct repository *r, */ options->jobs = 1; if (r) { - if (r->gitdir && r->hook_config_cache && r->hook_jobs) - options->jobs = r->hook_jobs; - else + if (r->gitdir && r->hook_config_cache) { + void *event_jobs; + + if (r->hook_jobs) + options->jobs = r->hook_jobs; + + event_jobs = strmap_get(&r->event_jobs, hook_name); + if (event_jobs) + options->jobs = (unsigned int)(uintptr_t)event_jobs; + } else { + unsigned int event_jobs; + char *key; + repo_config_get_uint(r, "hook.jobs", &options->jobs); + + key = xstrfmt("hook.%s.jobs", hook_name); + if (!repo_config_get_uint(r, key, &event_jobs) && event_jobs) + options->jobs = event_jobs; + free(key); + } } /* * Cap to serial any configured hook not marked as parallel = true. * This enforces the parallel = false default, even for "traditional" * hooks from the hookdir which cannot be marked parallel = true. + * The same restriction applies whether jobs came from hook.jobs or + * hook..jobs. */ for (size_t i = 0; i < hook_list->nr; i++) { struct hook *h = hook_list->items[i].util; @@ -642,7 +678,7 @@ int run_hooks_opt(struct repository *r, const char *hook_name, .options = options, }; int ret = 0; - unsigned int jobs = get_hook_jobs(r, options, hook_list); + unsigned int jobs = get_hook_jobs(r, options, hook_name, hook_list); const struct run_process_parallel_opts opts = { .tr2_category = "hook", .tr2_label = hook_name, diff --git a/repository.c b/repository.c index 192d6dc9c477fa..4030db4460714d 100644 --- a/repository.c +++ b/repository.c @@ -426,6 +426,7 @@ void repo_clear(struct repository *repo) hook_cache_clear(repo->hook_config_cache); FREE_AND_NULL(repo->hook_config_cache); } + strmap_clear(&repo->event_jobs, 0); /* values are uintptr_t, not heap ptrs */ if (repo->promisor_remote_config) { promisor_remote_clear(repo->promisor_remote_config); diff --git a/repository.h b/repository.h index 58e46853d089bf..6b67ec02e2984c 100644 --- a/repository.h +++ b/repository.h @@ -175,6 +175,9 @@ struct repository { /* Cached value of hook.jobs config (0 if unset, defaults to serial). */ unsigned int hook_jobs; + /* Cached map of event-name -> jobs count (as uintptr_t) from hook..jobs. */ + struct strmap event_jobs; + /* Configurations related to promisor remotes. */ char *repository_format_partial_clone; struct promisor_remote_config *promisor_remote_config; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index aa37a5181a0e0e..24a3c92b6deb80 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -969,4 +969,63 @@ test_expect_success 'hook.jobs=2 is ignored for force-serial hooks (pre-commit)' test_cmp expect hook.order ' +test_expect_success 'hook..jobs overrides hook.jobs for that event' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command \ + "touch sentinel.started; sleep 2; touch sentinel.done" && + test_config hook.hook-1.parallel true && + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + test_config hook.hook-2.parallel true && + + # Global hook.jobs=1 (serial), but per-event override allows parallel. + test_config hook.jobs 1 && + test_config hook.test-hook.jobs 2 && + + git hook run --allow-unknown-hook-name test-hook >out 2>err && + echo parallel >expect && + test_cmp expect hook.order +' + +test_expect_success 'hook..jobs=1 forces serial even when hook.jobs>1' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command \ + "touch sentinel.started; sleep 2; touch sentinel.done" && + test_config hook.hook-1.parallel true && + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + test_config hook.hook-2.parallel true && + + # Global hook.jobs=4 allows parallel, but per-event override forces serial. + test_config hook.jobs 4 && + test_config hook.test-hook.jobs 1 && + + git hook run --allow-unknown-hook-name test-hook >out 2>err && + echo serial >expect && + test_cmp expect hook.order +' + +test_expect_success 'hook..jobs still requires hook..parallel=true' ' + test_when_finished "rm -f sentinel.started sentinel.done hook.order" && + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command \ + "touch sentinel.started; sleep 2; touch sentinel.done" && + # hook-1 intentionally has no parallel=true + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command \ + "$(sentinel_detector sentinel hook.order)" && + # hook-2 also has no parallel=true + + # Per-event jobs=2 but no hook has parallel=true: must still run serially. + test_config hook.test-hook.jobs 2 && + + git hook run --allow-unknown-hook-name test-hook >out 2>err && + echo serial >expect && + test_cmp expect hook.order +' + test_done From 5e57b209ff21bf1087dd8539c458737c89b03150 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Fri, 10 Apr 2026 12:06:04 +0300 Subject: [PATCH 37/61] hook: warn when hook..jobs is set Issue a warning when the user confuses the hook process and event namespaces by setting hook..jobs. Detect this by checking whether the name carrying .jobs also has .command, .event, or .parallel configured. Extract is_friendly_name() as a helper for this check, to be reused by future per-event config handling. Suggested-by: Junio C Hamano Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 40 ++++++++++++++++++++++++++++++++++++++++ t/t1800-hook.sh | 30 ++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/hook.c b/hook.c index d98b01156366a4..0493993bbe6738 100644 --- a/hook.c +++ b/hook.c @@ -279,6 +279,44 @@ void hook_cache_clear(struct strmap *cache) strmap_clear(cache, 0); } +/* + * Return true if `name` is a hook friendly-name, i.e. it has at least one of + * .command, .event, or .parallel configured. These are the reliable clues + * that distinguish a friendly-name from an event name. Note: .enabled is + * deliberately excluded because it can appear under both namespaces. + */ +static int is_friendly_name(struct hook_all_config_cb *cb, const char *name) +{ + struct hashmap_iter iter; + struct strmap_entry *e; + + if (strmap_get(&cb->commands, name) || strmap_get(&cb->parallel_hooks, name)) + return 1; + + strmap_for_each_entry(&cb->event_hooks, &iter, e) { + if (unsorted_string_list_lookup(e->value, name)) + return 1; + } + + return 0; +} + +/* Warn if any name in event_jobs is also a hook friendly-name. */ +static void warn_jobs_on_friendly_names(struct hook_all_config_cb *cb_data) +{ + struct hashmap_iter iter; + struct strmap_entry *e; + + strmap_for_each_entry(&cb_data->event_jobs, &iter, e) { + if (is_friendly_name(cb_data, e->key)) + warning(_("hook.%s.jobs is set but '%s' looks like a " + "hook friendly-name, not an event name; " + "hook..jobs uses the event name " + "(e.g. hook.post-receive.jobs), so this " + "setting will be ignored"), e->key, e->key); + } +} + /* Populate `cache` with the complete hook configuration */ static void build_hook_config_map(struct repository *r, struct strmap *cache) { @@ -295,6 +333,8 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) /* Parse all configs in one run, capturing hook.* including hook.jobs. */ repo_config(r, hook_config_lookup_all, &cb_data); + warn_jobs_on_friendly_names(&cb_data); + /* Construct the cache from parsed configs. */ strmap_for_each_entry(&cb_data.event_hooks, &iter, e) { struct string_list *hook_names = e->value; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 24a3c92b6deb80..89fedc48ff497f 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -1028,4 +1028,34 @@ test_expect_success 'hook..jobs still requires hook..parallel=true' test_cmp expect hook.order ' +test_expect_success 'hook..jobs warns when name has .command' ' + test_config hook.my-hook.command "true" && + test_config hook.my-hook.jobs 2 && + git hook run --allow-unknown-hook-name --ignore-missing test-hook >out 2>err && + test_grep "hook.my-hook.jobs.*friendly-name" err +' + +test_expect_success 'hook..jobs warns when name has .event' ' + test_config hook.my-hook.event test-hook && + test_config hook.my-hook.command "true" && + test_config hook.my-hook.jobs 2 && + git hook run --allow-unknown-hook-name --ignore-missing test-hook >out 2>err && + test_grep "hook.my-hook.jobs.*friendly-name" err +' + +test_expect_success 'hook..jobs warns when name has .parallel' ' + test_config hook.my-hook.event test-hook && + test_config hook.my-hook.command "true" && + test_config hook.my-hook.parallel true && + test_config hook.my-hook.jobs 2 && + git hook run --allow-unknown-hook-name --ignore-missing test-hook >out 2>err && + test_grep "hook.my-hook.jobs.*friendly-name" err +' + +test_expect_success 'hook..jobs does not warn for a real event name' ' + test_config hook.test-hook.jobs 2 && + git hook run --allow-unknown-hook-name --ignore-missing test-hook >out 2>err && + test_grep ! "friendly-name" err +' + test_done From 2eb541e8f2a9b0dd923279421c741d0a0c00420d Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Fri, 10 Apr 2026 12:06:05 +0300 Subject: [PATCH 38/61] hook: move is_known_hook() to hook.c for wider use Move is_known_hook() from builtin/hook.c (static) into hook.c and export it via hook.h so it can be reused. Make it return bool and the iterator `h` for clarity (iterate hooks). Both meson.build and the Makefile are updated to reflect that the header is now used by libgit, not the builtin sources. The next commit will use this to reject hook friendly-names that collide with known event names. Co-authored-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Makefile | 2 +- builtin/hook.c | 10 ---------- hook.c | 10 ++++++++++ hook.h | 6 ++++++ meson.build | 24 ++++++++++++------------ 5 files changed, 29 insertions(+), 23 deletions(-) diff --git a/Makefile b/Makefile index 5d22394c2ec1a6..c4e83823e4a547 100644 --- a/Makefile +++ b/Makefile @@ -2675,7 +2675,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS) help.sp help.s help.o: command-list.h builtin/bugreport.sp builtin/bugreport.s builtin/bugreport.o: hook-list.h -builtin/hook.sp builtin/hook.s builtin/hook.o: hook-list.h +hook.sp hook.s hook.o: hook-list.h builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \ diff --git a/builtin/hook.c b/builtin/hook.c index bea0668b475931..1839412dca3edc 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -4,7 +4,6 @@ #include "environment.h" #include "gettext.h" #include "hook.h" -#include "hook-list.h" #include "parse-options.h" #define BUILTIN_HOOK_RUN_USAGE \ @@ -13,15 +12,6 @@ #define BUILTIN_HOOK_LIST_USAGE \ N_("git hook list [--allow-unknown-hook-name] [-z] [--show-scope] ") -static int is_known_hook(const char *name) -{ - const char **p; - for (p = hook_name_list; *p; p++) - if (!strcmp(*p, name)) - return 1; - return 0; -} - static const char * const builtin_hook_usage[] = { BUILTIN_HOOK_RUN_USAGE, BUILTIN_HOOK_LIST_USAGE, diff --git a/hook.c b/hook.c index 0493993bbe6738..19076f8f2baba6 100644 --- a/hook.c +++ b/hook.c @@ -5,6 +5,7 @@ #include "environment.h" #include "gettext.h" #include "hook.h" +#include "hook-list.h" #include "parse.h" #include "path.h" #include "run-command.h" @@ -12,6 +13,15 @@ #include "strbuf.h" #include "strmap.h" +bool is_known_hook(const char *name) +{ + const char **h; + for (h = hook_name_list; *h; h++) + if (!strcmp(*h, name)) + return true; + return false; +} + const char *find_hook(struct repository *r, const char *name) { static struct strbuf path = STRBUF_INIT; diff --git a/hook.h b/hook.h index 01db4226a60306..5a93f56618e123 100644 --- a/hook.h +++ b/hook.h @@ -234,6 +234,12 @@ void hook_free(void *p, const char *str); */ void hook_cache_clear(struct strmap *cache); +/** + * Returns true if `name` is a recognized hook event name + * (e.g. "pre-commit", "post-receive"). + */ +bool is_known_hook(const char *name); + /** * Returns the path to the hook file, or NULL if the hook is missing * or disabled. Note that this points to static storage that will be diff --git a/meson.build b/meson.build index 8309942d184847..f438d5545dafb7 100644 --- a/meson.build +++ b/meson.build @@ -563,6 +563,18 @@ libgit_sources += custom_target( env: script_environment, ) +libgit_sources += custom_target( + input: 'Documentation/githooks.adoc', + output: 'hook-list.h', + command: [ + shell, + meson.current_source_dir() + '/tools/generate-hooklist.sh', + meson.current_source_dir(), + '@OUTPUT@', + ], + env: script_environment, +) + builtin_sources = [ 'builtin/add.c', 'builtin/am.c', @@ -739,18 +751,6 @@ builtin_sources += custom_target( env: script_environment, ) -builtin_sources += custom_target( - input: 'Documentation/githooks.adoc', - output: 'hook-list.h', - command: [ - shell, - meson.current_source_dir() + '/tools/generate-hooklist.sh', - meson.current_source_dir(), - '@OUTPUT@', - ], - env: script_environment, -) - # This contains the variables for GIT-BUILD-OPTIONS, which we use to propagate # build options to our tests. build_options_config = configuration_data() From dcfb5af67e7d7156c4d1ede66de18088c990356c Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Fri, 10 Apr 2026 12:06:06 +0300 Subject: [PATCH 39/61] hook: add hook..enabled switch Add a hook..enabled config key that disables all hooks for a given event, when set to false, acting as a high-level switch above the existing per-hook hook..enabled. Event-disabled hooks are shown in "git hook list" with an "event-disabled" tab-separated prefix before the name: $ git hook list test-hook event-disabled hook-1 event-disabled hook-2 With --show-scope: $ git hook list --show-scope test-hook local event-disabled hook-1 When a hook is both per-hook disabled and event-disabled, only "event-disabled" is shown: the event-level switch is the more relevant piece of information, and the per-hook "disabled" status will surface once the event is re-enabled. Using an event name as a friendly-name (e.g. hook..enabled) can cause ambiguity, so a fatal error is issued when using a known event name and a warning is issued for unknown event name, since a collision cannot be detected with certainty for unknown events. Suggested-by: Patrick Steinhardt Suggested-by: Junio C Hamano Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 20 ++++++++ builtin/hook.c | 20 +++++--- hook.c | 47 +++++++++++++++++-- hook.h | 1 + repository.c | 1 + repository.h | 4 ++ t/t1800-hook.sh | 83 ++++++++++++++++++++++++++++++++++ 7 files changed, 165 insertions(+), 11 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index d4fa29d936d6e2..e0db3afa194080 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -15,6 +15,12 @@ hook..event:: events, specify the key more than once. An empty value resets the list of events, clearing any previously defined events for `hook.`. See linkgit:git-hook[1]. ++ +The `` must not be the same as a known hook event name +(e.g. do not use `hook.pre-commit.event`). Using a known event name as +a friendly-name is a fatal error because it creates an ambiguity with +`hook..enabled` and `hook..jobs`. For unknown event names, +a warning is issued when `` matches the event value. hook..enabled:: Whether the hook `hook.` is enabled. Defaults to `true`. @@ -33,6 +39,20 @@ hook..parallel:: found in the hooks directory do not need to, and run in parallel when the effective job count is greater than 1. See linkgit:git-hook[1]. +hook..enabled:: + Switch to enable or disable all hooks for the `` hook event. + When set to `false`, no hooks fire for that event, regardless of any + per-hook `hook..enabled` settings. Defaults to `true`. + See linkgit:git-hook[1]. ++ +Note on naming: `` must be the event name (e.g. `pre-commit`), +not a hook friendly-name. Since using a known event name as a +friendly-name is disallowed (see `hook..event` above), +there is no ambiguity between event-level and per-hook `.enabled` +settings for known events. For unknown events, if a friendly-name +matches the event name despite the warning, `.enabled` is treated +as per-hook only. + hook..jobs:: Specifies how many hooks can be run simultaneously for the `` hook event (e.g. `hook.post-receive.jobs = 4`). Overrides `hook.jobs` diff --git a/builtin/hook.c b/builtin/hook.c index 1839412dca3edc..8e47e22e2a1e5f 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -87,14 +87,22 @@ static int list(int argc, const char **argv, const char *prefix, const char *name = h->u.configured.friendly_name; const char *scope = show_scope ? config_scope_name(h->u.configured.scope) : NULL; + /* + * Show the most relevant disable reason. Event-level + * takes precedence: if the whole event is off, that + * is what the user needs to know. The per-hook + * "disabled" surfaces once the event is re-enabled. + */ + const char *disability = + h->u.configured.event_disabled ? "event-disabled\t" : + h->u.configured.disabled ? "disabled\t" : + ""; if (scope) - printf("%s\t%s%s%c", scope, - h->u.configured.disabled ? "disabled\t" : "", - name, line_terminator); + printf("%s\t%s%s%c", scope, disability, name, + line_terminator); else - printf("%s%s%c", - h->u.configured.disabled ? "disabled\t" : "", - name, line_terminator); + printf("%s%s%c", disability, name, + line_terminator); break; } default: diff --git a/hook.c b/hook.c index 19076f8f2baba6..bc990d4ed4d754 100644 --- a/hook.c +++ b/hook.c @@ -133,7 +133,9 @@ struct hook_config_cache_entry { * Callback struct to collect all hook.* keys in a single config pass. * commands: friendly-name to command map. * event_hooks: event-name to list of friendly-names map. - * disabled_hooks: set of friendly-names with hook..enabled = false. + * disabled_hooks: set of all names with hook..enabled = false; after + * parsing, names that are not friendly-names become event-level + * disables stored in r->disabled_events. This collects all. * parallel_hooks: friendly-name to parallel flag. * event_jobs: event-name to per-event jobs count (stored as uintptr_t, NULL == unset). * jobs: value of the global hook.jobs key. Defaults to 0 if unset (stored in r->hook_jobs). @@ -189,8 +191,21 @@ static int hook_config_lookup_all(const char *key, const char *value, strmap_for_each_entry(&data->event_hooks, &iter, e) unsorted_string_list_remove(e->value, hook_name, 0); } else { - struct string_list *hooks = - strmap_get(&data->event_hooks, value); + struct string_list *hooks; + + if (is_known_hook(hook_name)) + die(_("hook friendly-name '%s' collides with " + "a known event name; please choose a " + "different friendly-name"), + hook_name); + + if (!strcmp(hook_name, value)) + warning(_("hook friendly-name '%s' is the " + "same as its event; this may cause " + "ambiguity with hook.%s.enabled"), + hook_name, hook_name); + + hooks = strmap_get(&data->event_hooks, value); if (!hooks) { CALLOC_ARRAY(hooks, 1); @@ -345,6 +360,22 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) warn_jobs_on_friendly_names(&cb_data); + /* + * Populate disabled_events: names in disabled_hooks that are not + * friendly-names are event-level switches (hook..enabled = false). + * Names that are friendly-names are already handled per-hook via the + * hook_config_cache_entry.disabled flag below. + */ + if (r) { + string_list_clear(&r->disabled_events, 0); + string_list_init_dup(&r->disabled_events); + for (size_t i = 0; i < cb_data.disabled_hooks.nr; i++) { + const char *n = cb_data.disabled_hooks.items[i].string; + if (!is_friendly_name(&cb_data, n)) + string_list_append(&r->disabled_events, n); + } + } + /* Construct the cache from parsed configs. */ strmap_for_each_entry(&cb_data.event_hooks, &iter, e) { struct string_list *hook_names = e->value; @@ -446,6 +477,8 @@ static void list_hooks_add_configured(struct repository *r, { struct strmap *cache = get_hook_config_cache(r); struct string_list *configured_hooks = strmap_get(cache, hookname); + bool event_is_disabled = r ? !!unsorted_string_list_lookup(&r->disabled_events, + hookname) : 0; /* Iterate through configured hooks and initialize internal states */ for (size_t i = 0; configured_hooks && i < configured_hooks->nr; i++) { @@ -472,6 +505,7 @@ static void list_hooks_add_configured(struct repository *r, entry->command ? xstrdup(entry->command) : NULL; hook->u.configured.scope = entry->scope; hook->u.configured.disabled = entry->disabled; + hook->u.configured.event_disabled = event_is_disabled; hook->parallel = entry->parallel; string_list_append(list, friendly_name)->util = hook; @@ -484,6 +518,8 @@ static void list_hooks_add_configured(struct repository *r, if (!r || !r->gitdir) { hook_cache_clear(cache); free(cache); + if (r) + string_list_clear(&r->disabled_events, 0); } } @@ -515,7 +551,7 @@ int hook_exists(struct repository *r, const char *name) for (size_t i = 0; i < hooks->nr; i++) { struct hook *h = hooks->items[i].util; if (h->kind == HOOK_TRADITIONAL || - !h->u.configured.disabled) { + (!h->u.configured.disabled && !h->u.configured.event_disabled)) { exists = 1; break; } @@ -538,7 +574,8 @@ static int pick_next_hook(struct child_process *cp, if (hook_cb->hook_to_run_index >= hook_list->nr) return 0; h = hook_list->items[hook_cb->hook_to_run_index++].util; - } while (h->kind == HOOK_CONFIGURED && h->u.configured.disabled); + } while (h->kind == HOOK_CONFIGURED && + (h->u.configured.disabled || h->u.configured.event_disabled)); cp->no_stdin = 1; strvec_pushv(&cp->env, hook_cb->options->env.v); diff --git a/hook.h b/hook.h index 5a93f56618e123..b4372b636ff4de 100644 --- a/hook.h +++ b/hook.h @@ -32,6 +32,7 @@ struct hook { const char *command; enum config_scope scope; bool disabled; + bool event_disabled; } configured; } u; diff --git a/repository.c b/repository.c index 4030db4460714d..db57b8308b94e7 100644 --- a/repository.c +++ b/repository.c @@ -427,6 +427,7 @@ void repo_clear(struct repository *repo) FREE_AND_NULL(repo->hook_config_cache); } strmap_clear(&repo->event_jobs, 0); /* values are uintptr_t, not heap ptrs */ + string_list_clear(&repo->disabled_events, 0); if (repo->promisor_remote_config) { promisor_remote_clear(repo->promisor_remote_config); diff --git a/repository.h b/repository.h index 6b67ec02e2984c..4969d8b8ebed60 100644 --- a/repository.h +++ b/repository.h @@ -2,6 +2,7 @@ #define REPOSITORY_H #include "strmap.h" +#include "string-list.h" #include "repo-settings.h" #include "environment.h" @@ -178,6 +179,9 @@ struct repository { /* Cached map of event-name -> jobs count (as uintptr_t) from hook..jobs. */ struct strmap event_jobs; + /* Cached list of event names with hook..enabled = false. */ + struct string_list disabled_events; + /* Configurations related to promisor remotes. */ char *repository_format_partial_clone; struct promisor_remote_config *promisor_remote_config; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 89fedc48ff497f..c4ff25f6b088ea 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -1058,4 +1058,87 @@ test_expect_success 'hook..jobs does not warn for a real event name' ' test_grep ! "friendly-name" err ' +test_expect_success 'hook..enabled=false skips all hooks for event' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "echo ran" && + test_config hook.test-hook.enabled false && + git hook run --allow-unknown-hook-name test-hook >out 2>err && + test_must_be_empty out +' + +test_expect_success 'hook..enabled=true does not suppress hooks' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "echo ran" && + test_config hook.test-hook.enabled true && + git hook run --allow-unknown-hook-name test-hook >out 2>err && + test_grep "ran" err +' + +test_expect_success 'hook..enabled=false does not affect other events' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "echo ran" && + test_config hook.other-event.enabled false && + git hook run --allow-unknown-hook-name test-hook >out 2>err && + test_grep "ran" err +' + +test_expect_success 'hook..enabled=false still disables that hook' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "echo hook-1" && + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command "echo hook-2" && + test_config hook.hook-1.enabled false && + git hook run --allow-unknown-hook-name test-hook >out 2>err && + test_grep ! "hook-1" err && + test_grep "hook-2" err +' + +test_expect_success 'git hook list shows event-disabled hooks as event-disabled' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "echo ran" && + test_config hook.hook-2.event test-hook && + test_config hook.hook-2.command "echo ran" && + test_config hook.test-hook.enabled false && + git hook list --allow-unknown-hook-name test-hook >actual && + test_grep "^event-disabled hook-1$" actual && + test_grep "^event-disabled hook-2$" actual +' + +test_expect_success 'git hook list shows scope with event-disabled' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "echo ran" && + test_config hook.test-hook.enabled false && + git hook list --allow-unknown-hook-name --show-scope test-hook >actual && + test_grep "^local event-disabled hook-1$" actual +' + +test_expect_success 'git hook list still shows hooks when event is disabled' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "echo ran" && + test_config hook.test-hook.enabled false && + git hook list --allow-unknown-hook-name test-hook >actual && + test_grep "event-disabled" actual +' + +test_expect_success 'friendly-name matching known event name is rejected' ' + test_config hook.pre-commit.event pre-commit && + test_config hook.pre-commit.command "echo oops" && + test_must_fail git hook run pre-commit 2>err && + test_grep "collides with a known event name" err +' + +test_expect_success 'friendly-name matching known event name is rejected even for different event' ' + test_config hook.pre-commit.event post-commit && + test_config hook.pre-commit.command "echo oops" && + test_must_fail git hook run post-commit 2>err && + test_grep "collides with a known event name" err +' + +test_expect_success 'friendly-name matching unknown event warns' ' + test_config hook.test-hook.event test-hook && + test_config hook.test-hook.command "echo ran" && + git hook run --allow-unknown-hook-name test-hook >out 2>err && + test_grep "same as its event" err +' + test_done From 495b7d54dc006556548e2fd3ca15c4f533917329 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Fri, 10 Apr 2026 12:06:07 +0300 Subject: [PATCH 40/61] hook: allow hook.jobs=-1 to use all available CPU cores Allow -1 as a value for hook.jobs, hook..jobs, and the -j CLI flag to mean "use as many jobs as there are CPU cores", matching the convention used by fetch.parallel and other Git subsystems. The value is resolved to online_cpus() at parse time so the rest of the code always works with a positive resolved count. Other non-positive values (0, -2, etc) are rejected with a warning (config) or die (CLI). Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 4 ++- builtin/hook.c | 15 +++++++-- hook.c | 60 ++++++++++++++++++++++++---------- t/t1800-hook.sh | 49 +++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 20 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index e0db3afa194080..a9dc0063c12102 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -58,7 +58,8 @@ hook..jobs:: hook event (e.g. `hook.post-receive.jobs = 4`). Overrides `hook.jobs` for this specific event. The same parallelism restrictions apply: this setting has no effect unless all configured hooks for the event have - `hook..parallel` set to `true`. Must be a positive int, + `hook..parallel` set to `true`. Set to `-1` to use the + number of available CPU cores. Must be a positive integer or `-1`; zero is rejected with a warning. See linkgit:git-hook[1]. + Note on naming: although this key resembles `hook..*` @@ -74,6 +75,7 @@ valid event name when setting `hook..jobs`. hook.jobs:: Specifies how many hooks can be run simultaneously during parallelized hook execution. If unspecified, defaults to 1 (serial execution). + Set to `-1` to use the number of available CPU cores. Can be overridden on a per-event basis with `hook..jobs`. Some hooks always run sequentially regardless of this setting because they operate on shared data and cannot safely be parallelized: diff --git a/builtin/hook.c b/builtin/hook.c index 8e47e22e2a1e5f..cceeb3586e5daf 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -5,6 +5,7 @@ #include "gettext.h" #include "hook.h" #include "parse-options.h" +#include "thread-utils.h" #define BUILTIN_HOOK_RUN_USAGE \ N_("git hook run [--allow-unknown-hook-name] [--ignore-missing] [--to-stdin=] [(-j|--jobs) ]\n" \ @@ -123,6 +124,7 @@ static int run(int argc, const char **argv, const char *prefix, struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; int ignore_missing = 0; int allow_unknown = 0; + int jobs = 0; const char *hook_name; struct option run_options[] = { OPT_BOOL(0, "allow-unknown-hook-name", &allow_unknown, @@ -131,8 +133,8 @@ static int run(int argc, const char **argv, const char *prefix, N_("silently ignore missing requested ")), OPT_STRING(0, "to-stdin", &opt.path_to_stdin, N_("path"), N_("file to read into hooks' stdin")), - OPT_UNSIGNED('j', "jobs", &opt.jobs, - N_("run up to hooks simultaneously")), + OPT_INTEGER('j', "jobs", &jobs, + N_("run up to hooks simultaneously (-1 for CPU count)")), OPT_END(), }; int ret; @@ -141,6 +143,15 @@ static int run(int argc, const char **argv, const char *prefix, builtin_hook_run_usage, PARSE_OPT_KEEP_DASHDASH); + if (jobs == -1) + opt.jobs = online_cpus(); + else if (jobs < 0) + die(_("invalid value for -j: %d" + " (use -1 for CPU count or a" + " positive integer)"), jobs); + else + opt.jobs = jobs; + if (!argc) goto usage; diff --git a/hook.c b/hook.c index bc990d4ed4d754..d10eef4763c679 100644 --- a/hook.c +++ b/hook.c @@ -12,6 +12,7 @@ #include "setup.h" #include "strbuf.h" #include "strmap.h" +#include "thread-utils.h" bool is_known_hook(const char *name) { @@ -165,13 +166,17 @@ static int hook_config_lookup_all(const char *key, const char *value, /* Handle plain hook. entries that have no hook name component. */ if (!name) { if (!strcmp(subkey, "jobs") && value) { - unsigned int v; - if (!git_parse_uint(value, &v)) - warning(_("hook.jobs must be a positive integer, ignoring: '%s'"), value); - else if (!v) - warning(_("hook.jobs must be positive, ignoring: 0")); - else + int v; + if (!git_parse_int(value, &v)) + warning(_("hook.jobs must be an integer, ignoring: '%s'"), value); + else if (v == -1) + data->jobs = online_cpus(); + else if (v > 0) data->jobs = v; + else + warning(_("hook.jobs must be a positive integer" + " or -1, ignoring: '%s'"), + value); } return 0; } @@ -259,17 +264,21 @@ static int hook_config_lookup_all(const char *key, const char *value, " ignoring: '%s'"), hook_name, value); } else if (!strcmp(subkey, "jobs")) { - unsigned int v; - if (!git_parse_uint(value, &v)) - warning(_("hook.%s.jobs must be a positive integer," + int v; + if (!git_parse_int(value, &v)) + warning(_("hook.%s.jobs must be an integer," " ignoring: '%s'"), hook_name, value); - else if (!v) - warning(_("hook.%s.jobs must be positive," - " ignoring: 0"), hook_name); - else + else if (v == -1) + strmap_put(&data->event_jobs, hook_name, + (void *)(uintptr_t)online_cpus()); + else if (v > 0) strmap_put(&data->event_jobs, hook_name, (void *)(uintptr_t)v); + else + warning(_("hook.%s.jobs must be a positive" + " integer or -1, ignoring: '%s'"), + hook_name, value); } free(hook_name); @@ -688,6 +697,25 @@ static void warn_non_parallel_hooks_override(unsigned int jobs, } } +/* Resolve a hook.jobs config key, handling -1 as online_cpus(). */ +static void resolve_hook_config_jobs(struct repository *r, + const char *key, + unsigned int *jobs) +{ + int v; + + if (repo_config_get_int(r, key, &v)) + return; + + if (v == -1) + *jobs = online_cpus(); + else if (v > 0) + *jobs = v; + else + warning(_("%s must be a positive integer or -1," + " ignoring: %d"), key, v); +} + /* Determine how many jobs to use for hook execution. */ static unsigned int get_hook_jobs(struct repository *r, struct run_hooks_opt *options, @@ -721,14 +749,12 @@ static unsigned int get_hook_jobs(struct repository *r, if (event_jobs) options->jobs = (unsigned int)(uintptr_t)event_jobs; } else { - unsigned int event_jobs; char *key; - repo_config_get_uint(r, "hook.jobs", &options->jobs); + resolve_hook_config_jobs(r, "hook.jobs", &options->jobs); key = xstrfmt("hook.%s.jobs", hook_name); - if (!repo_config_get_uint(r, key, &event_jobs) && event_jobs) - options->jobs = event_jobs; + resolve_hook_config_jobs(r, key, &options->jobs); free(key); } } diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index c4ff25f6b088ea..41b2b2c7460066 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -1058,6 +1058,55 @@ test_expect_success 'hook..jobs does not warn for a real event name' ' test_grep ! "friendly-name" err ' +test_expect_success 'hook.jobs=-1 resolves to online_cpus()' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "true" && + test_config hook.hook-1.parallel true && + + test_config hook.jobs -1 && + + cpus=$(test-tool online-cpus) && + GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ + git hook run --allow-unknown-hook-name test-hook >out 2>err && + grep "\"region_enter\".*\"hook\".*\"test-hook\".*\"max:$cpus\"" trace.txt +' + +test_expect_success 'hook..jobs=-1 resolves to online_cpus()' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "true" && + test_config hook.hook-1.parallel true && + + test_config hook.test-hook.jobs -1 && + + cpus=$(test-tool online-cpus) && + GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ + git hook run --allow-unknown-hook-name test-hook >out 2>err && + grep "\"region_enter\".*\"hook\".*\"test-hook\".*\"max:$cpus\"" trace.txt +' + +test_expect_success 'git hook run -j-1 resolves to online_cpus()' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "true" && + test_config hook.hook-1.parallel true && + + cpus=$(test-tool online-cpus) && + GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ + git hook run --allow-unknown-hook-name -j-1 test-hook >out 2>err && + grep "\"region_enter\".*\"hook\".*\"test-hook\".*\"max:$cpus\"" trace.txt +' + +test_expect_success 'hook.jobs rejects values less than -1' ' + test_config hook.jobs -2 && + git hook run --allow-unknown-hook-name --ignore-missing test-hook >out 2>err && + test_grep "hook.jobs must be a positive integer or -1" err +' + +test_expect_success 'hook..jobs rejects values less than -1' ' + test_config hook.test-hook.jobs -5 && + git hook run --allow-unknown-hook-name --ignore-missing test-hook >out 2>err && + test_grep "hook.test-hook.jobs must be a positive integer or -1" err +' + test_expect_success 'hook..enabled=false skips all hooks for event' ' test_config hook.hook-1.event test-hook && test_config hook.hook-1.command "echo ran" && From 75b7cb5e14f03965cf87a976356bcbdcfb4edbad Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 10 Apr 2026 12:06:08 +0300 Subject: [PATCH 41/61] t1800: test SIGPIPE with parallel hooks We recently fixed a bug in commit 2226ffaacd (run_processes_parallel(): fix order of sigpipe handling, 2026-04-08) where a hook that caused us to get SIGPIPE would accidentally trigger the run_processes_parallel() cleanup handler killing the child processes. For a single hook, this meant killing the already-exited hook. This case was triggered by our tests, but was only a problem on some platforms. But if you have multiple hooks running in parallel, this causes a problem everywhere, since one hook failing to read its input would take down all hooks. Now that we have parallel hook support, we can add a test for this case. It should pass already, due to the existing fix. Signed-off-by: Jeff King Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- t/t1800-hook.sh | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 41b2b2c7460066..0132e772e472e2 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -1190,4 +1190,42 @@ test_expect_success 'friendly-name matching unknown event warns' ' test_grep "same as its event" err ' +test_expect_success 'hooks in parallel that do not read input' ' + # Add this to our $PATH to avoid having to write the whole trash + # directory into our config options, which would require quoting. + mkdir bin && + PATH=$PWD/bin:$PATH && + + write_script bin/hook-fast <<-\EOF && + # This hook does not read its input, so the parent process + # may see SIGPIPE if it is not ignored. It should happen + # relatively quickly. + exit 0 + EOF + + write_script bin/hook-slow <<-\EOF && + # This hook is slow, so we expect it to still be running + # when the other hook has exited (and the parent has a pipe error + # writing to it). + # + # So we want to be slow enough that we expect this to happen, but not + # so slow that the test takes forever. 1 second is probably enough + # in practice (and if it is occasionally not on a loaded system, we + # will err on the side of having the test pass). + sleep 1 + exit 0 + EOF + + git init --bare parallel.git && + git -C parallel.git config hook.fast.command "hook-fast" && + git -C parallel.git config hook.fast.event pre-receive && + git -C parallel.git config hook.fast.parallel true && + git -C parallel.git config hook.slow.command "hook-slow" && + git -C parallel.git config hook.slow.event pre-receive && + git -C parallel.git config hook.slow.parallel true && + git -C parallel.git config hook.jobs 2 && + + git push ./parallel.git "+refs/heads/*:refs/heads/*" +' + test_done From 955c88fbc5ac916f8dababa458a963ebbeba9b41 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Wed, 15 Apr 2026 02:27:42 +0000 Subject: [PATCH 42/61] userdiff: tighten word-diff test case of the scheme driver The scheme driver separates identifiers only at parentheses of all sorts and whitespace, except that vertical bars act as brackets that enclose an identifier. The test case attempts to demonstrate the vertical bars with a change from 'some-text' to '|a greeting|'. However, this misses the goal because the same word coloring would be applied if '|a greeting|' were parsed as two words. Have an identifier between vertical bars with a space in both the pre- and the post-image and change only one side of the space to show that the single word exists between the vertical bars. Also add cases that change parentheses of all kinds in a sequence of parentheses to show that they are their own word each. Signed-off-by: Johannes Sixt Signed-off-by: Scott L. Burson Signed-off-by: Junio C Hamano --- t/t4034/scheme/expect | 5 +++-- t/t4034/scheme/post | 1 + t/t4034/scheme/pre | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/t/t4034/scheme/expect b/t/t4034/scheme/expect index 496cd5de8c9af3..138abe9f56b38f 100644 --- a/t/t4034/scheme/expect +++ b/t/t4034/scheme/expect @@ -2,10 +2,11 @@ index 74b6605..63b6ac4 100644 --- a/pre +++ b/post -@@ -1,6 +1,6 @@ +@@ -1,7 +1,7 @@ (define (myfunc a bmy-func first second) ; This is a really(moderately) cool function. (this\placethat\place (+ 3 4)) - (define some-text|a greeting| "hello") + (define |the greeting||a greeting| "hello") + ({}(([](func-n)[])){}) (let ((c (+ a badd1 first))) (format "one more than the total is %d" (add1+ c second)))) diff --git a/t/t4034/scheme/post b/t/t4034/scheme/post index 63b6ac4f8754d8..0e3bab101da03e 100644 --- a/t/t4034/scheme/post +++ b/t/t4034/scheme/post @@ -2,5 +2,6 @@ ; This is a (moderately) cool function. (that\place (+ 3 4)) (define |a greeting| "hello") + ({(([(func-n)]))}) (let ((c (add1 first))) (format "one more than the total is %d" (+ c second)))) diff --git a/t/t4034/scheme/pre b/t/t4034/scheme/pre index 74b66053574b67..03d77c7c430e07 100644 --- a/t/t4034/scheme/pre +++ b/t/t4034/scheme/pre @@ -1,6 +1,7 @@ (define (myfunc a b) ; This is a really cool function. (this\place (+ 3 4)) - (define some-text "hello") + (define |the greeting| "hello") + ({}(([](func-n)[])){}) (let ((c (+ a b))) (format "one more than the total is %d" (add1 c)))) From b79f7a3ad3ffde16b2cbc2457561669f4833f861 Mon Sep 17 00:00:00 2001 From: "Scott L. Burson" Date: Wed, 15 Apr 2026 02:27:43 +0000 Subject: [PATCH 43/61] userdiff: extend Scheme support to cover other Lisp dialects Common Lisp has top-level forms, such as 'defun' and 'defmacro', that are not matched by the current Scheme pattern. Also, it is more common in CL, when defining user macros intended as top-level forms, to prefix their names with "def" instead of "define"; such forms are also not matched. And some top-level forms don't even begin with "def". On the other hand, it is an established formatting convention in the Lisp community that only top-level forms start at the left margin. So matching any unindented line starting with an open parenthesis is an acceptable heuristic; false positives will be rare. However, there are also cases where notionally top-level forms are grouped together within some containing form. At least in the Common Lisp community, it is conventional to indent these by two spaces, or sometimes one. But matching just an open parenthesis indented by two spaces would be too broad; so the pattern added by this commit requires an indented form to start with "(def". It is believed that this strikes a good balance between potential false positives and false negatives. Signed-off-by: Scott L. Burson Acked-by: Johannes Sixt Signed-off-by: Junio C Hamano --- Documentation/gitattributes.adoc | 3 ++- t/t4018/scheme-lisp-defun-a | 4 ++++ t/t4018/scheme-lisp-defun-b | 4 ++++ t/t4018/scheme-lisp-eval-when | 4 ++++ t/t4018/{scheme-module => scheme-module-a} | 0 t/t4018/scheme-module-b | 6 ++++++ t/t4034/scheme/expect | 2 +- t/t4034/scheme/post | 2 +- t/t4034/scheme/pre | 2 +- userdiff.c | 22 ++++++++++++++++------ 10 files changed, 39 insertions(+), 10 deletions(-) create mode 100644 t/t4018/scheme-lisp-defun-a create mode 100644 t/t4018/scheme-lisp-defun-b create mode 100644 t/t4018/scheme-lisp-eval-when rename t/t4018/{scheme-module => scheme-module-a} (100%) create mode 100644 t/t4018/scheme-module-b diff --git a/Documentation/gitattributes.adoc b/Documentation/gitattributes.adoc index f20041a323d174..bd76167a45eb71 100644 --- a/Documentation/gitattributes.adoc +++ b/Documentation/gitattributes.adoc @@ -911,7 +911,8 @@ patterns are available: - `rust` suitable for source code in the Rust language. -- `scheme` suitable for source code in the Scheme language. +- `scheme` suitable for source code in most Lisp dialects, + including Scheme, Emacs Lisp, Common Lisp, and Clojure. - `tex` suitable for source code for LaTeX documents. diff --git a/t/t4018/scheme-lisp-defun-a b/t/t4018/scheme-lisp-defun-a new file mode 100644 index 00000000000000..c3c750f76d7b07 --- /dev/null +++ b/t/t4018/scheme-lisp-defun-a @@ -0,0 +1,4 @@ +(defun some-func (x y z) RIGHT + (let ((a x) + (b y)) + (ChangeMe a b))) diff --git a/t/t4018/scheme-lisp-defun-b b/t/t4018/scheme-lisp-defun-b new file mode 100644 index 00000000000000..21be305968bf6b --- /dev/null +++ b/t/t4018/scheme-lisp-defun-b @@ -0,0 +1,4 @@ +(macrolet ((foo (x) `(bar ,x))) + (defun mumble (x) ; RIGHT + (when (> x 0) + (foo x)))) ; ChangeMe diff --git a/t/t4018/scheme-lisp-eval-when b/t/t4018/scheme-lisp-eval-when new file mode 100644 index 00000000000000..5d941d7e0edda2 --- /dev/null +++ b/t/t4018/scheme-lisp-eval-when @@ -0,0 +1,4 @@ +(eval-when (:compile-toplevel :load-toplevel :execute) ; RIGHT + (set-macro-character #\? + (lambda (stream char) + `(make-pattern-variable ,(read stream))))) ; ChangeMe diff --git a/t/t4018/scheme-module b/t/t4018/scheme-module-a similarity index 100% rename from t/t4018/scheme-module rename to t/t4018/scheme-module-a diff --git a/t/t4018/scheme-module-b b/t/t4018/scheme-module-b new file mode 100644 index 00000000000000..77bc0c5eff4775 --- /dev/null +++ b/t/t4018/scheme-module-b @@ -0,0 +1,6 @@ +(module A + (export with-display-exception) + (extern (display-exception display-exception)) + (def (with-display-exception thunk) RIGHT + (with-catch (lambda (e) (display-exception e (current-error-port)) e) + thunk ChangeMe))) diff --git a/t/t4034/scheme/expect b/t/t4034/scheme/expect index 138abe9f56b38f..fb7f2616fea547 100644 --- a/t/t4034/scheme/expect +++ b/t/t4034/scheme/expect @@ -6,7 +6,7 @@ (define (myfunc a bmy-func first second) ; This is a really(moderately) cool function. (this\placethat\place (+ 3 4)) - (define |the greeting||a greeting| "hello") + (define |the \| \greeting||a \greeting| |hello there|) ({}(([](func-n)[])){}) (let ((c (+ a badd1 first))) (format "one more than the total is %d" (add1+ c second)))) diff --git a/t/t4034/scheme/post b/t/t4034/scheme/post index 0e3bab101da03e..450cc234f75aea 100644 --- a/t/t4034/scheme/post +++ b/t/t4034/scheme/post @@ -1,7 +1,7 @@ (define (my-func first second) ; This is a (moderately) cool function. (that\place (+ 3 4)) - (define |a greeting| "hello") + (define |a \greeting| |hello there|) ({(([(func-n)]))}) (let ((c (add1 first))) (format "one more than the total is %d" (+ c second)))) diff --git a/t/t4034/scheme/pre b/t/t4034/scheme/pre index 03d77c7c430e07..e16ee7584946e4 100644 --- a/t/t4034/scheme/pre +++ b/t/t4034/scheme/pre @@ -1,7 +1,7 @@ (define (myfunc a b) ; This is a really cool function. (this\place (+ 3 4)) - (define |the greeting| "hello") + (define |the \| \greeting| |hello there|) ({}(([](func-n)[])){}) (let ((c (+ a b))) (format "one more than the total is %d" (add1 c)))) diff --git a/userdiff.c b/userdiff.c index fe710a68bfdfa6..b5412e6bc3ecd3 100644 --- a/userdiff.c +++ b/userdiff.c @@ -344,14 +344,24 @@ PATTERNS("rust", "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?" "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"), PATTERNS("scheme", - "^[\t ]*(\\(((define|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$", /* - * R7RS valid identifiers include any sequence enclosed - * within vertical lines having no backslashes + * An unindented opening parenthesis identifies a top-level + * expression in all Lisp dialects. */ - "\\|([^\\\\]*)\\|" - /* All other words should be delimited by spaces or parentheses */ - "|([^][)(}{[ \t])+"), + "^(\\(.*)$\n" + /* For Scheme: a possibly indented left paren followed by a keyword. */ + "^[\t ]*(\\(((define|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$\n" + /* + * For all Lisp dialects: a slightly indented line starting with "(def". + */ + "^ ?(\\([Dd][Ee][Ff].*)$", + /* + * The union of R7RS and Common Lisp symbol syntax: allows arbitrary + * strings between vertical bars, including any escaped characters. + */ + "\\|([^|\\\\]|\\\\.)*\\|" + /* All other words should be delimited by spaces or parentheses. */ + "|([^][)(}{ \t])+"), PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$", "\\\\[a-zA-Z@]+|\\\\.|([a-zA-Z0-9]|[^\x01-\x7f])+"), { .name = "default", .binary = -1 }, From a8faa7a56033486c576b6386798dca4591e163eb Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 16 Apr 2026 09:20:57 +0000 Subject: [PATCH 44/61] http: extract http_reauth_prepare() from retry paths All three HTTP retry paths (http_request_recoverable, post_rpc, probe_rpc) call credential_fill() directly when handling HTTP_REAUTH. Extract this into a helper function so that a subsequent commit can add pre-fill logic (such as attempting empty-auth before prompting) in one place. No functional change. Signed-off-by: Matthew John Cheetham Signed-off-by: Junio C Hamano --- http.c | 7 ++++++- http.h | 6 ++++++ remote-curl.c | 4 ++-- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index 67c9c6fc60673d..f3ba2964b943ef 100644 --- a/http.c +++ b/http.c @@ -665,6 +665,11 @@ static void init_curl_http_auth(CURL *result) } } +void http_reauth_prepare(int all_capabilities) +{ + credential_fill(the_repository, &http_auth, all_capabilities); +} + /* *var must be free-able */ static void var_override(char **var, char *value) { @@ -2398,7 +2403,7 @@ static int http_request_recoverable(const char *url, sleep(retry_delay); } } else if (ret == HTTP_REAUTH) { - credential_fill(the_repository, &http_auth, 1); + http_reauth_prepare(1); } ret = http_request(url, result, target, options); diff --git a/http.h b/http.h index f9ee888c3ed67e..729c51904d39ad 100644 --- a/http.h +++ b/http.h @@ -76,6 +76,12 @@ extern int http_is_verbose; extern ssize_t http_post_buffer; extern struct credential http_auth; +/** + * Prepare for an HTTP re-authentication retry. This fills credentials + * via credential_fill() so the next request can include them. + */ +void http_reauth_prepare(int all_capabilities); + extern char curl_errorstr[CURL_ERROR_SIZE]; enum http_follow_config { diff --git a/remote-curl.c b/remote-curl.c index aba60d571282d3..affdb880f7b3bf 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -946,7 +946,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece do { err = probe_rpc(rpc, &results); if (err == HTTP_REAUTH) - credential_fill(the_repository, &http_auth, 0); + http_reauth_prepare(0); } while (err == HTTP_REAUTH); if (err != HTTP_OK) return -1; @@ -1068,7 +1068,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece rpc->any_written = 0; err = run_slot(slot, NULL); if (err == HTTP_REAUTH && !large_request) { - credential_fill(the_repository, &http_auth, 0); + http_reauth_prepare(0); curl_slist_free_all(headers); goto retry; } From 5dbc8c1367226cae6acaa1626d31f01bd186a28c Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 16 Apr 2026 09:20:58 +0000 Subject: [PATCH 45/61] http: attempt Negotiate auth in http.emptyAuth=auto mode When a server advertises Negotiate (SPNEGO) authentication, the "auto" mode of http.emptyAuth should detect this as an "exotic" method and proactively send empty credentials, allowing libcurl to use the system Kerberos ticket without prompting the user. However, two features interact to prevent this from working: The Negotiate-stripping logic, introduced in 4dbe66464b (remote-curl: fall back to Basic auth if Negotiate fails, 2015-01-08), removes CURLAUTH_GSSNEGOTIATE from the allowed methods on the first 401 response. The empty-auth auto-detection, introduced in 40a18fc77c (http: add an "auto" mode for http.emptyauth, 2017-02-25), then checks the remaining methods for anything "exotic" -- but Negotiate has already been removed, so auto mode never activates for servers whose only non-Basic/Digest method is Negotiate (e.g., Apache with mod_auth_kerb offering Basic + Negotiate). Fix this by delaying the Negotiate stripping in auto mode: on the first 401, keep Negotiate in the allowed methods so that auto mode can detect it and retry with empty credentials. If that attempt fails (no valid Kerberos ticket), strip Negotiate on the second 401 and fall through to credential_fill() as usual. To support this, also teach http_reauth_prepare() to skip credential_fill() when empty auth is about to be attempted, since filling real credentials would bypass the empty-auth mechanism. The true and false modes are unchanged: true sends empty credentials on the very first request (before any 401), and false never sends them. Signed-off-by: Matthew John Cheetham Signed-off-by: Junio C Hamano --- http.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/http.c b/http.c index f3ba2964b943ef..412f7af2504b26 100644 --- a/http.c +++ b/http.c @@ -138,6 +138,7 @@ static unsigned long empty_auth_useless = CURLAUTH_BASIC | CURLAUTH_DIGEST_IE | CURLAUTH_DIGEST; +static int empty_auth_try_negotiate; static struct curl_slist *pragma_header; static struct string_list extra_http_headers = STRING_LIST_INIT_DUP; @@ -667,6 +668,17 @@ static void init_curl_http_auth(CURL *result) void http_reauth_prepare(int all_capabilities) { + /* + * If we deferred stripping Negotiate to give empty auth a + * chance (auto mode), skip credential_fill on this retry so + * that init_curl_http_auth() sends empty credentials and + * libcurl can attempt Negotiate with the system ticket cache. + */ + if (empty_auth_try_negotiate && + !http_auth.password && !http_auth.credential && + (http_auth_methods & CURLAUTH_GSSNEGOTIATE)) + return; + credential_fill(the_repository, &http_auth, all_capabilities); } @@ -1895,7 +1907,18 @@ static int handle_curl_result(struct slot_results *results) http_proactive_auth = PROACTIVE_AUTH_NONE; return HTTP_NOAUTH; } else { - http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE; + if (curl_empty_auth == -1 && + !empty_auth_try_negotiate && + (results->auth_avail & CURLAUTH_GSSNEGOTIATE)) { + /* + * In auto mode, give Negotiate a chance via + * empty auth before stripping it. If it fails, + * we will strip it on the next 401. + */ + empty_auth_try_negotiate = 1; + } else { + http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE; + } if (results->auth_avail) { http_auth_methods &= results->auth_avail; http_auth_methods_restricted = 1; From 9b1630b97273beceb64ea8f740c5820317aaa8b3 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 16 Apr 2026 09:20:59 +0000 Subject: [PATCH 46/61] t5563: add tests for http.emptyAuth with Negotiate Add tests exercising the interaction between http.emptyAuth and servers that advertise Negotiate (SPNEGO) authentication. Verify that auto mode gives Negotiate a chance via empty auth (resulting in two 401 responses before falling through to credential_fill with Basic credentials), and that false mode strips Negotiate immediately (only one 401 response). Signed-off-by: Matthew John Cheetham Signed-off-by: Junio C Hamano --- t/t5563-simple-http-auth.sh | 74 +++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh index 00635816156ba3..a7d475dd68dbd7 100755 --- a/t/t5563-simple-http-auth.sh +++ b/t/t5563-simple-http-auth.sh @@ -719,4 +719,78 @@ test_expect_success 'access using three-legged auth' ' EOF ' +test_lazy_prereq SPNEGO 'curl --version | grep -qi "SPNEGO\|GSS-API\|Kerberos\|negotiate"' + +test_expect_success SPNEGO 'http.emptyAuth=auto attempts Negotiate before credential_fill' ' + test_when_finished "per_test_cleanup" && + + set_credential_reply get <<-EOF && + username=alice + password=secret-passwd + EOF + + # Basic base64(alice:secret-passwd) + cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF && + id=1 creds=Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== + EOF + + cat >"$HTTPD_ROOT_PATH/custom-auth.challenge" <<-EOF && + id=1 status=200 + id=default response=WWW-Authenticate: Negotiate + id=default response=WWW-Authenticate: Basic realm="example.com" + EOF + + test_config_global credential.helper test-helper && + GIT_TRACE_CURL="$TRASH_DIRECTORY/trace-auto" \ + git -c http.emptyAuth=auto \ + ls-remote "$HTTPD_URL/custom_auth/repo.git" && + + # In auto mode with a Negotiate+Basic server, there should be + # three 401 responses: (1) initial no-auth request, (2) empty-auth + # retry where Negotiate fails (no Kerberos ticket), (3) libcurl + # internal Negotiate retry. The fourth attempt uses Basic + # credentials from credential_fill and succeeds. + grep "HTTP/[0-9.]* 401" "$TRASH_DIRECTORY/trace-auto" >actual_401s && + test_line_count = 3 actual_401s && + + expect_credential_query get <<-EOF + capability[]=authtype + capability[]=state + protocol=http + host=$HTTPD_DEST + wwwauth[]=Negotiate + wwwauth[]=Basic realm="example.com" + EOF +' + +test_expect_success SPNEGO 'http.emptyAuth=false skips Negotiate' ' + test_when_finished "per_test_cleanup" && + + set_credential_reply get <<-EOF && + username=alice + password=secret-passwd + EOF + + # Basic base64(alice:secret-passwd) + cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF && + id=1 creds=Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== + EOF + + cat >"$HTTPD_ROOT_PATH/custom-auth.challenge" <<-EOF && + id=1 status=200 + id=default response=WWW-Authenticate: Negotiate + id=default response=WWW-Authenticate: Basic realm="example.com" + EOF + + test_config_global credential.helper test-helper && + GIT_TRACE_CURL="$TRASH_DIRECTORY/trace-false" \ + git -c http.emptyAuth=false \ + ls-remote "$HTTPD_URL/custom_auth/repo.git" && + + # With emptyAuth=false, Negotiate is stripped immediately and + # credential_fill is called right away. Only one 401 response. + grep "HTTP/[0-9.]* 401" "$TRASH_DIRECTORY/trace-false" >actual_401s && + test_line_count = 1 actual_401s +' + test_done From b96490241e342fe1aecbd3c4f40de6998d2a3eaa Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 10 Apr 2026 11:10:48 -0700 Subject: [PATCH 47/61] CodingGuidelines: st_mtimespec vs st_mtim vs st_mtime Most unfortunately macOS does not support st_[amc]tim for timestamps down to nanosecond resolution as POSIX systems. Signed-off-by: Junio C Hamano --- Documentation/CodingGuidelines | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index b8670751f5c705..b9a29e39f2eec4 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -681,6 +681,12 @@ For C programs: char *dogs[] = ...; walk_all_dogs(dogs); + - For file timestamps, do not use "st_mtim" (and other timestamp + members in "struct stat") unconditionally; not everybody is POSIX + (grep for USE_ST_TIMESPEC). If you only need a timestamp in whole + second resolution, "st_mtime" should work fine everywhere. + + For Perl programs: - Most of the C guidelines above apply. From 6d7f5241d61c60710982fe5ab9037d153dc2822d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:20 +0200 Subject: [PATCH 48/61] t: prepare `test_match_signal ()` calls for `set -e` We have a couple of calls to `test_match_signal ()` where we execute a Git command and expect it to die with a specific signal. These calls will essentially execute the process in a subshell via `foo; echo $?`, but as we expect `foo` to fail this will cause the overall subshell to fail once we `set -e`. Fix this issue by using `foo && echo 0 || echo $?` instead. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t0005-signals.sh | 4 ++-- t/t3600-rm.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh index afba0fc3fc673e..84319cf169e571 100755 --- a/t/t0005-signals.sh +++ b/t/t0005-signals.sh @@ -42,12 +42,12 @@ test_expect_success 'create blob' ' ' test_expect_success !MINGW 'a constipated git dies with SIGPIPE' ' - OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) && + OUT=$( ((large_git && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) && test_match_signal 13 "$OUT" ' test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' ' - OUT=$( ((trap "" PIPE && large_git; echo $? 1>&3) | :) 3>&1 ) && + OUT=$( ((trap "" PIPE && large_git && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) && test_match_signal 13 "$OUT" ' diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 1f16e6b52285bc..a371ea690ef79d 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -260,7 +260,7 @@ test_expect_success 'choking "git rm" should not let it die with cruft (induce S test_expect_success !MINGW 'choking "git rm" should not let it die with cruft (induce and check SIGPIPE)' ' choke_git_rm_setup && - OUT=$( ((trap "" PIPE && git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) && + OUT=$( ((trap "" PIPE && git rm -n "some-file-*" && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) && test_match_signal 13 "$OUT" && test_path_is_missing .git/index.lock ' From aab3b7acff35511bee6a983ac4a88190c5e418e9 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:21 +0200 Subject: [PATCH 49/61] t: prepare `test_must_fail ()` for `set -e` The helper function `test_must_fail ()` executes a specific Git command that may or may not fail in a specific way. This is done by executing the command in question and then comparing its exit code against a set of conditions. This works, but once we run our test suite with `set -e` we may bail out of `test_must_fail ()` early in case the command actually fails, even though we expect it to fail. Prepare for this change by handling the failed case with `||`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/test-lib-functions.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index f3af10fb7e0205..5fd5494ef1030c 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1195,8 +1195,9 @@ test_must_fail () { echo >&7 "test_must_fail: only 'git' is allowed: $*" return 1 fi - "$@" 2>&7 - exit_code=$? + + exit_code=0; "$@" 2>&7 || exit_code=$? + if test $exit_code -eq 0 && ! list_contains "$_test_ok" success then echo >&4 "test_must_fail: command succeeded: $*" From 7fc23c03183761f0eb4e854d241f875c0add8bf7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:22 +0200 Subject: [PATCH 50/61] t: prepare `stop_git_daemon ()` for `set -e` We have a couple of calls to `stop_git_daemon ()` outside of specific test cases that will kill a backgrounded git-daemon(1) process and expect the process with a specific error code. While these function calls do end up killing git-daemon(1), the error handling we have in those contexts is basically ineffective. So while we expect the process to exit with a specific error code, we will just continue with any error in case it doesn't. This will change once we enable `set -e` in a subsequent commit. There's two issues though that will make this _always_ fail: - Our call to `wait` is expected to fail, but because it's not part of a condition it will cause us to bail out immediately with `set -e`. - We try to kill git-daemon(1) a second time via the pidfile. We can generally expect that this is the same PID though as we had in the "GIT_DAEMON_PID" environment variable, and thus it's more likely than not that we have already killed it, and the call to kill will fail. Prepare for this change by handling the failure of `wait` with `||` and by silencing failures of the second call to `kill`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/lib-git-daemon.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh index e62569222b55aa..d172aa51f0c386 100644 --- a/t/lib-git-daemon.sh +++ b/t/lib-git-daemon.sh @@ -85,14 +85,16 @@ stop_git_daemon() { # kill git-daemon child of git say >&3 "Stopping git daemon ..." + kill "$GIT_DAEMON_PID" - wait "$GIT_DAEMON_PID" >&3 2>&4 - ret=$? + ret=0; wait "$GIT_DAEMON_PID" >&3 2>&4 || ret=$? + if ! test_match_signal 15 $ret then error "git daemon exited with status: $ret" fi - kill "$(cat "$GIT_DAEMON_PIDFILE")" 2>/dev/null + + kill "$(cat "$GIT_DAEMON_PIDFILE")" 2>/dev/null || : GIT_DAEMON_PID= rm -f git_daemon_output "$GIT_DAEMON_PIDFILE" } From 17111338aedae587f58f3a612f7bd27f53ec5e27 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:23 +0200 Subject: [PATCH 51/61] t: prepare `git config --unset` calls for `set -e` We have a couple of calls to `git config --unset` that ultimately end up as no-ops as the configuration variables aren't set (anymore) in the first place. These calls are mostly intended to recover unconditionally from tests that may have executed only partially, but they'll ultimately fail during a normal test run. This hasn't been a problem until now as we aren't running tests with `set -e`. This is about to change though, so let's silence the case where we cannot unset the config keys. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t4032-diff-inter-hunk-context.sh | 2 +- t/t7508-status.sh | 4 ++-- t/t9138-git-svn-authors-prog.sh | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh index bada0cbd32f764..c98eb6abb2e3e5 100755 --- a/t/t4032-diff-inter-hunk-context.sh +++ b/t/t4032-diff-inter-hunk-context.sh @@ -17,7 +17,7 @@ f() { t() { use_config= - git config --unset diff.interHunkContext + git config --unset diff.interHunkContext || : case $# in 4) hunks=$4; cmd="diff -U$3";; diff --git a/t/t7508-status.sh b/t/t7508-status.sh index a5e21bf8bffb45..1167b835a4635c 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -773,8 +773,8 @@ test_expect_success TTY 'status --porcelain ignores color.status' ' ' # recover unconditionally from color tests -git config --unset color.status -git config --unset color.ui +git config --unset color.status || : +git config --unset color.ui || : test_expect_success 'status --porcelain respects -b' ' diff --git a/t/t9138-git-svn-authors-prog.sh b/t/t9138-git-svn-authors-prog.sh index 784ec7fc2d6e4d..5bb38cb23a64e9 100755 --- a/t/t9138-git-svn-authors-prog.sh +++ b/t/t9138-git-svn-authors-prog.sh @@ -68,8 +68,8 @@ test_expect_success 'authors-file overrode authors-prog' ' ) ' -git --git-dir=x/.git config --unset svn.authorsfile -git --git-dir=x/.git config --unset svn.authorsprog +git --git-dir=x/.git config --unset svn.authorsfile || : +git --git-dir=x/.git config --unset svn.authorsprog || : test_expect_success 'authors-prog imported user without email' ' svn mkdir -m gg --username gg-hermit "$svnrepo"/gg && From 3fb4792a1c983f24b5d14718b7483776535a723a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:24 +0200 Subject: [PATCH 52/61] t: prepare conditional test execution for `set -e` We have some test in our test suite where we use the pattern of `test ... && test_expect_succeess` to conditionally execute a test. The problem is that when we decide to not execute the test, we'll indeed skip the test, but the overall statement will also be unsuccessful. This will become a problem once we enable `set -e`. Prepare for this future by turning this into a proper conditional, which is also a bit easier to read overall. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t4032-diff-inter-hunk-context.sh | 12 +++++++----- t/t7450-bad-git-dotfiles.sh | 24 +++++++++++++----------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh index c98eb6abb2e3e5..2d216fb70f982b 100755 --- a/t/t4032-diff-inter-hunk-context.sh +++ b/t/t4032-diff-inter-hunk-context.sh @@ -40,11 +40,13 @@ t() { test $(git $cmd $file | grep '^@@ ' | wc -l) = $hunks " - test -f $expected && - test_expect_success "$label: check output" " - git $cmd $file | grep -v '^index ' >actual && - test_cmp $expected actual - " + if test -f $expected + then + test_expect_success "$label: check output" " + git $cmd $file | grep -v '^index ' >actual && + test_cmp $expected actual + " + fi } cat <expected.f1.0.1 || exit 1 diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh index f512eed278c46b..8cc86522b27d9b 100755 --- a/t/t7450-bad-git-dotfiles.sh +++ b/t/t7450-bad-git-dotfiles.sh @@ -220,17 +220,19 @@ check_dotx_symlink () { ) ' - test -n "$refuse_index" && - test_expect_success "refuse to load symlinked $name into index ($type)" ' - test_must_fail \ - git -C $dir \ - -c core.protectntfs \ - -c core.protecthfs \ - read-tree $tree 2>err && - grep "invalid path.*$name" err && - git -C $dir ls-files -s >out && - test_must_be_empty out - ' + if test -n "$refuse_index" + then + test_expect_success "refuse to load symlinked $name into index ($type)" ' + test_must_fail \ + git -C $dir \ + -c core.protectntfs \ + -c core.protecthfs \ + read-tree $tree 2>err && + grep "invalid path.*$name" err && + git -C $dir ls-files -s >out && + test_must_be_empty out + ' + fi } check_dotx_symlink gitmodules vanilla .gitmodules From 5d2224146b57dc7b4345b5eaaf80137dd557902f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:25 +0200 Subject: [PATCH 53/61] t: prepare execution of potentially failing commands for `set -e` Several of our tests verify whether a certain binary can be executed, potentially skipping tests in case we cannot, for example because the binary doesn't exist. In those cases we often run the binary outside of any conditionally. This will start to fail once we enable `set -e`, as that will cause us to bail out the test immediately. Improve these tests by executing them inside of a conditional instead. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/lib-git-svn.sh | 7 +++---- t/lib-httpd.sh | 3 +-- t/t1410-reflog.sh | 2 +- t/t3901-i18n-patch.sh | 3 ++- t/t5000-tar-tree.sh | 4 ++-- t/t7422-submodule-output.sh | 2 +- t/t9200-git-cvsexportcommit.sh | 3 +-- t/t9400-git-cvsserver-server.sh | 5 +++-- t/t9401-git-cvsserver-crlf.sh | 4 ++-- t/t9402-git-cvsserver-refs.sh | 4 ++-- t/test-lib-functions.sh | 3 +-- t/test-lib.sh | 10 ++++++---- 12 files changed, 25 insertions(+), 25 deletions(-) diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh index 2fde2353fd3835..52843f667de0d6 100644 --- a/t/lib-git-svn.sh +++ b/t/lib-git-svn.sh @@ -15,8 +15,7 @@ GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn SVN_TREE=$GIT_SVN_DIR/svn-tree test_set_port SVNSERVE_PORT -svn >/dev/null 2>&1 -if test $? -ne 1 +if ! svn help >/dev/null 2>&1 then skip_all='skipping git svn tests, svn not found' test_done @@ -27,13 +26,13 @@ export svnrepo svnconf=$PWD/svnconf export svnconf +x=0 perl -w -e " use SVN::Core; use SVN::Repos; \$SVN::Core::VERSION gt '1.1.0' or exit(42); system(qw/svnadmin create --fs-type fsfs/, \$ENV{svnrepo}) == 0 or exit(41); -" >&3 2>&4 -x=$? +" >&3 2>&4 || x=$? if test $x -ne 0 then if test $x -eq 42; then diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index 4c76e813e396bf..fc646447d5c038 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -235,11 +235,10 @@ start_httpd() { test_atexit stop_httpd - "$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \ + if ! "$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \ -f "$TEST_PATH/apache.conf" $HTTPD_PARA \ -c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start \ >&3 2>&4 - if test $? -ne 0 then cat "$HTTPD_ROOT_PATH"/error.log >&4 2>/dev/null test_skip_or_die GIT_TEST_HTTPD "web server setup failed" diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh index ce71f9a30ae1ee..f289fc11e90b58 100755 --- a/t/t1410-reflog.sh +++ b/t/t1410-reflog.sh @@ -23,7 +23,7 @@ check_have () { } check_fsck () { - git fsck --full >fsck.output + git fsck --full >fsck.output || true case "$1" in '') test_must_be_empty fsck.output ;; diff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh index f03601b49a9397..ef7d7e1edc2aee 100755 --- a/t/t3901-i18n-patch.sh +++ b/t/t3901-i18n-patch.sh @@ -28,7 +28,8 @@ check_encoding () { 8859) grep "^encoding ISO8859-1" ;; *) - grep "^encoding ISO8859-1"; test "$?" != 0 ;; + ret=0; grep "^encoding ISO8859-1" || ret=$? + test "$ret" != 0 ;; esac || return 1 j=$i i=$(($i+1)) diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh index 5465054f1779f0..a8c28533dc73f8 100755 --- a/t/t5000-tar-tree.sh +++ b/t/t5000-tar-tree.sh @@ -503,8 +503,8 @@ test_expect_success LONG_IS_64BIT 'set up repository with huge blob' ' # would generate the whole 64GB). test_expect_success LONG_IS_64BIT 'generate tar with huge size' ' { - git archive HEAD - echo $? >exit-code + { ret=0 && git archive HEAD || ret=$?; } && + echo "$ret" >exit-code } | test_copy_bytes 4096 >huge.tar && echo 141 >expect && test_cmp expect exit-code diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh index aea1ddf117e8ee..852136fdfd3ec8 100755 --- a/t/t7422-submodule-output.sh +++ b/t/t7422-submodule-output.sh @@ -198,7 +198,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE' ( cd repo && GIT_ALLOW_PROTOCOL=file git submodule add "$(pwd)"/../submodule && - { git submodule status --recursive 2>err; echo $?>status; } | + { { ret=0 && git submodule status --recursive 2>err || ret=$?; } && echo $ret >status; } | grep -q recursive-submodule-path-1 && test_must_be_empty err && test_match_signal 13 "$(cat status)" diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh index 14cbe9652779bc..581cf3d28fc05b 100755 --- a/t/t9200-git-cvsexportcommit.sh +++ b/t/t9200-git-cvsexportcommit.sh @@ -11,8 +11,7 @@ if ! test_have_prereq PERL; then test_done fi -cvs >/dev/null 2>&1 -if test $? -ne 1 +if ! cvs version >/dev/null 2>&1 then skip_all='skipping git cvsexportcommit tests, cvs not found' test_done diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index e499c7f955125e..4b45398bab2295 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -17,12 +17,13 @@ if ! test_have_prereq PERL; then skip_all='skipping git cvsserver tests, perl not available' test_done fi -cvs >/dev/null 2>&1 -if test $? -ne 1 + +if ! cvs version >/dev/null 2>&1 then skip_all='skipping git-cvsserver tests, cvs not found' test_done fi + perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || { skip_all='skipping git-cvsserver tests, Perl SQLite interface unavailable' test_done diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh index a34805acdc25cc..6b4cbb165131e0 100755 --- a/t/t9401-git-cvsserver-crlf.sh +++ b/t/t9401-git-cvsserver-crlf.sh @@ -60,12 +60,12 @@ check_status_options() { return $stat } -cvs >/dev/null 2>&1 -if test $? -ne 1 +if ! cvs version >/dev/null 2>&1 then skip_all='skipping git-cvsserver tests, cvs not found' test_done fi + if ! test_have_prereq PERL then skip_all='skipping git-cvsserver tests, perl not available' diff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh index 2ee41f9443eefb..65f2ceedecb2e7 100755 --- a/t/t9402-git-cvsserver-refs.sh +++ b/t/t9402-git-cvsserver-refs.sh @@ -68,12 +68,12 @@ check_diff() { ######### -cvs >/dev/null 2>&1 -if test $? -ne 1 +if ! cvs version >/dev/null 2>&1 then skip_all='skipping git-cvsserver tests, cvs not found' test_done fi + if ! test_have_prereq PERL then skip_all='skipping git-cvsserver tests, perl not available' diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 5fd5494ef1030c..879ee1ee597715 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1248,8 +1248,7 @@ test_might_fail () { test_expect_code () { want_code=$1 shift - "$@" 2>&7 - exit_code=$? + exit_code=0; "$@" 2>&7 || exit_code=$? if test $exit_code = $want_code then return 0 diff --git a/t/test-lib.sh b/t/test-lib.sh index 70fd3e9bafb800..de7d9e7b925049 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -143,8 +143,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME ################################################################ # It appears that people try to run tests without building... GIT_BINARY="${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" -"$GIT_BINARY" >/dev/null -if test $? != 1 + +if ! "$GIT_BINARY" version >/dev/null then if test -n "$GIT_TEST_INSTALLED" then @@ -454,8 +454,10 @@ then # from any previous runs. >"$GIT_TEST_TEE_OUTPUT_FILE" - (GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1; - echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE" + ( + ret=0 && GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1 || ret=$? + echo "$ret" >"$TEST_RESULTS_BASE.exit" + ) | tee -a "$GIT_TEST_TEE_OUTPUT_FILE" test "$(cat "$TEST_RESULTS_BASE.exit")" = 0 exit fi From 04077e18a3a5b92f9d5d7726fe9e2ec28ebcc506 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:26 +0200 Subject: [PATCH 54/61] t: prepare `test_when_finished ()`/`test_atexit()` for `set -e` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both `test_when_finished ()` and `test_atexit ()` build up a chain of cleanup commands by prepending each new command to the existing cleanup string. To preserve the exit code of the test body across cleanup execution, we append the following logic: } && (exit "$eval_ret"); eval_ret=$?; ... The intent of this is to run the cleanup block and then unconditionally restore `eval_ret`. The original behaviour of this is is: +------------------+---------+------------------------------------+ |test body │ cleanup │ old behaviour │ +------------------+---------+------------------------------------+ │pass (eval_ret=0) | pass │ && taken -> (exit 0) -> eval_ret=0 | +------------------+---------+------------------------------------+ │pass (eval_ret=0) | fail │ && not taken -> eval_ret=$? | +------------------+---------+------------------------------------+ │fail (eval_ret=1) | pass │ && taken -> (exit 1) -> eval_ret=1 | +------------------+---------+------------------------------------+ │fail (eval_ret=1) | fail | && not taken -> eval_ret=$? | +------------------+---------+------------------------------------+ This logic will start to fail once we enable `set -e`. When `$eval_ret` is non-zero, the subshell we create will fail, and with `set -e` we'll thus bail out without evaluating the logic after the semicolon. Fix this issue by instead using `|| eval_ret=\$?; ...`. Besides being a bit simpler, it also retains the original behaviour: +------------------+---------+------------------------------------+ |test body │ cleanup │ old behaviour │ +------------------+---------+------------------------------------+ │pass (eval_ret=0) | pass │ || not taken -> eval_ret unchanged | +------------------+---------+------------------------------------+ │pass (eval_ret=0) | fail │ || taken -> eval_ret=$? | +------------------+---------+------------------------------------+ │fail (eval_ret=1) | pass │ || not taken -> eval_ret unchanged | +------------------+---------+------------------------------------+ │fail (eval_ret=1) | fail | || taken -> eval_ret=$? | +------------------+---------+------------------------------------+ Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/test-lib-functions.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 879ee1ee597715..502bb0ddcbca31 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1512,7 +1512,7 @@ test_when_finished () { test "${BASH_SUBSHELL-0}" = 0 || BUG "test_when_finished does nothing in a subshell" test_cleanup="{ $* - } && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup" + } || eval_ret=\$?; $test_cleanup" } # This function can be used to schedule some commands to be run @@ -1540,7 +1540,7 @@ test_atexit () { test "${BASH_SUBSHELL-0}" = 0 || BUG "test_atexit does nothing in a subshell" test_atexit_cleanup="{ $* - } && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup" + } || eval_ret=\$?; $test_atexit_cleanup" } # Deprecated wrapper for "git init", use "git init" directly instead From 8e2ba9cf0bda342b7e65c52c8dfbae90641018bd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:27 +0200 Subject: [PATCH 55/61] t0008: silence error in subshell when using `grep -v` In t0008 we use `grep -v` in a subshell, but expect that this command will sometimes not match anything. This would cause grep(1) to return an error code, but given that we don't run with `set -e` we swallow this error. We're about to enable `set -e`. Prepare for this by ignoring any errors. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t0008-ignores.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh index e716b5cdfa1bee..d77a179bddcbed 100755 --- a/t/t0008-ignores.sh +++ b/t/t0008-ignores.sh @@ -122,8 +122,8 @@ test_expect_success_multiple () { fi testname="$1" expect_all="$2" code="$3" - expect_verbose=$( echo "$expect_all" | grep -v '^:: ' ) - expect=$( echo "$expect_verbose" | sed -e 's/.* //' ) + expect_verbose=$(echo "$expect_all" | grep -v '^:: ' || :) + expect=$(echo "$expect_verbose" | sed -e 's/.* //') test_expect_success $prereq "$testname${no_index_opt:+ with $no_index_opt}" ' expect "$expect" && From 585f990272e1f696dab81f415567584c1efa869d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:28 +0200 Subject: [PATCH 56/61] t1301: don't fail in case setfacl(1) doesn't exist or fails In t1301 we're trying to remove any potentially-existing default ACLs that might exist on the transh directory by executing setfacl(1). According to 8ed0a740dd (t1301-shared-repo.sh: don't let a default ACL interfere with the test, 2008-10-16), this is done because we play around with permissions and umasks in this test suite. The setfacl(1) binary may not exist on some systems though, even though tests ultimately still pass. This doesn't matter currently, but will cause the test to fail once we start running with `set -e`. Silence such failures by ignoring failures here. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t1301-shared-repo.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index 630a47af21e655..0e0d07a1a1e7d8 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -12,7 +12,7 @@ TEST_CREATE_REPO_NO_TEMPLATE=1 . ./test-lib.sh # Remove a default ACL from the test dir if possible. -setfacl -k . 2>/dev/null +setfacl -k . 2>/dev/null || : # User must have read permissions to the repo -> failure on --shared=0400 test_expect_success 'shared = 0400 (faulty permission u-w)' ' From b2e636f3995d0cc6555a60223745b59ae93c175e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:29 +0200 Subject: [PATCH 57/61] t6002: fix use of `expr` with `set -e` In `test_bisection_diff ()` we use `expr` to perform some math. This command has some gotchas though in that it will only return success when the result is neither null nor zero. In some of our cases though it actually _is_ zero, and that will cause the expressions to fail once we enable `set -e`. Prepare for this change by instead using `$(( ))`, which doesn't have the same issue. While at it, modernize the function a tiny bit. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t6002-rev-list-bisect.sh | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh index daa009c9a1b4b6..f2de40b5ed8f14 100755 --- a/t/t6002-rev-list-bisect.sh +++ b/t/t6002-rev-list-bisect.sh @@ -27,13 +27,16 @@ test_bisection_diff() # Test if bisection size is close to half of list size within # tolerance. # - _bisect_err=$(expr $_list_size - $_bisection_size \* 2) - test "$_bisect_err" -lt 0 && _bisect_err=$(expr 0 - $_bisect_err) - _bisect_err=$(expr $_bisect_err / 2) ; # floor - - test_expect_success \ - "bisection diff $_bisect_option $_head $* <= $_max_diff" \ - 'test $_bisect_err -le $_max_diff' + _bisect_err=$(($_list_size - $_bisection_size * 2)) + if test "$_bisect_err" -lt 0 + then + _bisect_err=$((0 - $_bisect_err)) + fi + _bisect_err=$(($_bisect_err / 2)) ; # floor + + test_expect_success "bisection diff $_bisect_option $_head $* <= $_max_diff" ' + test $_bisect_err -le $_max_diff + ' } date >path0 From 84250632a0d98f3ae2759d983d4195d5a546d31e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:30 +0200 Subject: [PATCH 58/61] t9902: fix use of `read` with `set -e` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In t9902 we're using the `read` builtin to read some values into a variable. This is done by using `-d ""`, which cause us to read until the end of the heredoc. As the read is terminated by EOF, the command will end up returning a non-zero error code. This hasn't been an issue until now as we didn't run with `set -e`, but that'll change in a subsequent commit. Prepare for this change by not using read at all, as we can simply store the multi-line value directly. Suggested-by: SZEDER Gábor Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t9902-completion.sh | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 2f9a597ec7f493..28f61f08fb4cec 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -590,12 +590,10 @@ test_expect_success '__gitcomp - doesnt fail because of invalid variable name' ' __gitcomp "$invalid_variable_name" ' -read -r -d "" refs <<-\EOF -main +refs='main maint next -seen -EOF +seen' test_expect_success '__gitcomp_nl - trailing space' ' test_gitcomp_nl "m" "$refs" <<-EOF From e390e6af0b414e9291d47678f3583ffa81acf634 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Apr 2026 09:27:31 +0200 Subject: [PATCH 59/61] t: detect errors outside of test cases We have recently merged a patch series that had a simple misspelling of `test_expect_success`. Instead of making our tests fail though, this typo went completely undetected and all of our tests passed, which is of course unfortunate. This is a more general issue with our test suite: all commands that run outside of a specific test case can fail, and if we don't explicitly check for such failure then this failure will be silently ignored. Improve the status quo by enabling the errexit option so that any such unchecked failures will cause us to abort immediately. Note that for now, we only enable this option for Bash 5 and newer. This is because other shells have wildly different behaviour, and older versions of Bash (especially on macOS) are buggy. The list of enabled shells may be extended going forward. Helped-by: Jeff King Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- ci/run-build-and-tests.sh | 6 ++++++ t/test-lib.sh | 25 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index 28cfe730ee5aed..de08a08d59fc8f 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -7,6 +7,12 @@ export TEST_CONTRIB_TOO=yes +case "$jobname" in +almalinux-*|debian-*|fedora-*|linux-*) + export GIT_TEST_USE_SET_E=yes + ;; +esac + case "$jobname" in fedora-breaking-changes-musl|linux-breaking-changes) export WITH_BREAKING_CHANGES=YesPlease diff --git a/t/test-lib.sh b/t/test-lib.sh index de7d9e7b925049..cded7bd693e6f6 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -15,6 +15,31 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see https://www.gnu.org/licenses/ . +# Enable the use of errexit so that any unexpected failures will cause us to +# abort tests, even when outside of a specific test case. +# +# Note that we only enable this on Bash 5 and newer, or when explicitly +# requested by the user via `GIT_TEST_USE_SET_E=true`. This ib secause `set -e` +# has wildly different behaviour across shells. The list of default-enabled +# shells may be extended going forward. +if test -z "$GIT_TEST_USE_SET_E" && test "${BASH_VERSINFO:=0}" -ge 5 +then + GIT_TEST_USE_SET_E=true +fi + +# We cannot use `test-tool env-helper` here, as it's not yet available. +case "${GIT_TEST_USE_SET_E:-false}" in +1|on|true|yes) + set -e + ;; +0|off|false|no) + ;; +*) + echo "GIT_TEST_USE_SET_E requires a boolean" >&2 + exit 1 + ;; +esac + # Test the binaries we have just built. The tests are kept in # t/ subdirectory and are run in 'trash directory' subdirectory. if test -z "$TEST_DIRECTORY" From 418ba15599e0a971c43ce8079b8436f3dbc29b75 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 20 Apr 2026 10:14:26 -0700 Subject: [PATCH 60/61] ### From 13626a754ce67db8190015952b5c289393a46f80 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 20 Apr 2026 10:14:27 -0700 Subject: [PATCH 61/61] ### match next