* [PATCH v8 3/3] contrib: wire up osxkeychain in contrib/Makefile on macOS
From: Shardul Natu via GitGitGadget @ 2026-07-08 3:21 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble, Shardul Natu
In-Reply-To: <pull.2288.v8.git.git.1783480879.gitgitgadget@gmail.com>
From: Shardul Natu <snatu@google.com>
When running "make test" with TEST_CONTRIB_TOO=yes (which is default in
macOS CI workflows), $(MAKE) -C contrib/ test is invoked. However,
contrib/Makefile only invoked tests for diff-highlight and subtree,
meaning git-credential-osxkeychain was never built or verified during
standard CI test runs.
Add a "test" target to contrib/credential/osxkeychain/Makefile that
depends on building git-credential-osxkeychain. Additionally, wire up
credential/osxkeychain in contrib/Makefile under "all", "test", and
"clean" whenever running on macOS (Darwin).
This ensures that running "make test" or "make all" in contrib on macOS
automatically builds and links git-credential-osxkeychain, preventing
future build or symbol linking regressions from slipping through CI.
Signed-off-by: Shardul Natu <snatu@google.com>
---
contrib/Makefile | 12 ++++++++++++
contrib/credential/osxkeychain/Makefile | 4 +++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/contrib/Makefile b/contrib/Makefile
index 787cd07f52..1203c7263d 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -1,10 +1,22 @@
+include ../config.mak.uname
+-include ../config.mak.autogen
+-include ../config.mak
+
+
+ifeq ($(uname_S),Darwin)
+OS_CONTRIB += credential/osxkeychain
+endif
+
all::
+ $(foreach dir,$(OS_CONTRIB),$(MAKE) -C $(dir) $@;)
test::
$(MAKE) -C diff-highlight $@
$(MAKE) -C subtree $@
+ $(foreach dir,$(OS_CONTRIB),$(MAKE) -C $(dir) $@;)
clean::
$(MAKE) -C contacts $@
$(MAKE) -C diff-highlight $@
$(MAKE) -C subtree $@
+ $(foreach dir,$(OS_CONTRIB),$(MAKE) -C $(dir) $@;)
diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile
index 219b0d7f49..d9fba07e8d 100644
--- a/contrib/credential/osxkeychain/Makefile
+++ b/contrib/credential/osxkeychain/Makefile
@@ -10,4 +10,6 @@ install:
clean:
$(MAKE) -C ../../.. clean-git-credential-osxkeychain
-.PHONY: all git-credential-osxkeychain install clean
+test: git-credential-osxkeychain
+
+.PHONY: all git-credential-osxkeychain install clean test
--
gitgitgadget
^ permalink raw reply related
* [PATCH v8 2/3] Makefile: support universal macOS builds via RUST_TARGETS
From: Shardul Natu via GitGitGadget @ 2026-07-08 3:21 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble, Shardul Natu
In-Reply-To: <pull.2288.v8.git.git.1783480879.gitgitgadget@gmail.com>
From: Shardul Natu <snatu@google.com>
On macOS, Universal Binaries contain native executable code for
multiple architectures (such as Intel x86_64 and Apple Silicon arm64)
bundled into a single file. This is standard practice for macOS
distribution and CI packaging (such as internal distribution packages
or tooling like Burrito/Homebrew), allowing a single build artifact
to run natively across all Macs without Rosetta emulation or
maintaining separate packages.
When building Git C code for multiple architectures on macOS, the
Apple toolchain (clang) natively supports universal builds via
CFLAGS/LDFLAGS. When "-arch x86_64 -arch arm64" is passed, clang
automatically compiles and links universal binaries for all C object
files and executables out of the box.
Cargo and rustc, however, do not support multiple "-arch" flags or
emitting universal binaries in a single invocation. Instead, Cargo
requires invoking each target triple independently (e.g., passing
"--target x86_64-apple-darwin" and "--target aarch64-apple-darwin").
To bridge this gap when Rust is enabled:
1. Allow specifying space-separated target triples in RUST_TARGETS.
2. Introduce declarative pattern rules (target/%/...) to compile
each target-specific library slice via Cargo.
3. On macOS, if multiple targets are specified, use "lipo" (part of
the mandatory Xcode Command Line Tools) to combine the resulting
static libraries into target/release/libgitcore.a.
Once $(RUST_LIB) is compiled into a universal static archive, the
standard C linker seamlessly links it with the C object files to
produce universal Git executables.
Signed-off-by: Shardul Natu <snatu@google.com>
---
Makefile | 39 +++++++++++++++++++++++++++++++++++----
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/Makefile b/Makefile
index 7db38ecce9..9921af992b 100644
--- a/Makefile
+++ b/Makefile
@@ -500,6 +500,14 @@ include shared.mak
#
# Building Rust code requires Cargo.
#
+# Define RUST_TARGETS if you want to cross-compile. If left unspecified, it uses
+# the default Rust target on the system.
+#
+# On macOS, this supports specifying multiple targets, separated by a space.
+# This will produce a Universal static library using `lipo`.
+#
+# Example: RUST_TARGETS="aarch64-apple-darwin x86_64-apple-darwin"
+#
# == SHA-1 and SHA-256 defines ==
#
# === SHA-1 backend ===
@@ -941,16 +949,17 @@ LIB_FILE = libgit.a
ifndef NO_RUST
ifdef DEBUG
-RUST_TARGET_DIR = target/debug
+RUST_BUILD_CONFIG = debug
else
-RUST_TARGET_DIR = target/release
+RUST_BUILD_CONFIG = release
endif
ifeq ($(uname_S),Windows)
-RUST_LIB = $(RUST_TARGET_DIR)/gitcore.lib
+RUST_LIB_NAME = gitcore.lib
else
-RUST_LIB = $(RUST_TARGET_DIR)/libgitcore.a
+RUST_LIB_NAME = libgitcore.a
endif
+RUST_LIB = target/$(RUST_BUILD_CONFIG)/$(RUST_LIB_NAME)
endif
GITLIBS = common-main.o $(LIB_FILE)
@@ -3022,8 +3031,30 @@ $(LIB_FILE): $(LIB_OBJS)
$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
ifndef NO_RUST
+ifeq ($(RUST_TARGETS),)
$(RUST_LIB): Cargo.toml $(RUST_SOURCES) $(LIB_FILE)
$(QUIET_CARGO)cargo build $(CARGO_ARGS)
+else
+ifneq ($(words $(RUST_TARGETS)),1)
+ifneq ($(uname_S),Darwin)
+$(error Building universal Rust libraries requires macOS (lipo is not available on $(uname_S)))
+endif
+endif
+
+RUST_MEMBER_LIBS = $(foreach target,$(RUST_TARGETS),target/$(target)/$(RUST_BUILD_CONFIG)/$(RUST_LIB_NAME))
+$(RUST_MEMBER_LIBS): target/%/$(RUST_BUILD_CONFIG)/$(RUST_LIB_NAME): Cargo.toml $(RUST_SOURCES) $(LIB_FILE)
+ $(QUIET_CARGO)cargo build $(CARGO_ARGS) --target $*
+
+$(RUST_LIB): $(RUST_MEMBER_LIBS)
+ $(call mkdir_p_parent_template)
+ $(QUIET_GEN)\
+ if test $(words $(RUST_TARGETS)) -gt 1; \
+ then \
+ lipo -create $^ -output $@; \
+ else \
+ cp $< $@; \
+ fi
+endif
.PHONY: rust
rust: $(RUST_LIB)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v8 1/3] Makefile: add $(RUST_LIB) prerequisite to osxkeychain
From: Shardul Natu via GitGitGadget @ 2026-07-08 3:21 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble, Shardul Natu
In-Reply-To: <pull.2288.v8.git.git.1783480879.gitgitgadget@gmail.com>
From: Shardul Natu <snatu@google.com>
When Rust is enabled, the git-credential-osxkeychain helper depends on
Rust symbols compiled into $(RUST_LIB). While commit 522ea8ef7d
("osxkeychain: fix build with Rust") updated the linker command line to
use $(LIBS), it omitted $(RUST_LIB) from the target prerequisite list.
Without this prerequisite, running a parallel build ("make -j") from a
clean working tree can fail because Make does not know to invoke Cargo
to build libgitcore.a before linking git-credential-osxkeychain.
Note that we depend explicitly on $(LIB_FILE) and $(RUST_LIB) rather
than $(GITLIBS). Unlike standard Git builtins and programs like scalar
(which define cmd_main() and rely on common-main.o to supply main()),
git-credential-osxkeychain.c defines its own standalone int main().
If $(GITLIBS) were used, $(filter %.o,$^) in the link recipe would
match both git-credential-osxkeychain.o and common-main.o, causing a
duplicate symbol linking error for _main on macOS.
Additionally, wrap the definitions of $(RUST_LIB) and the "rust" build
target in "ifndef NO_RUST". This ensures that when NO_RUST=1 is
specified, $(RUST_LIB) evaluates to empty, making the Rust dependency a
clean no-op without needing intermediate variables.
Signed-off-by: Shardul Natu <snatu@google.com>
---
Makefile | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 1f3f099f5c..7db38ecce9 100644
--- a/Makefile
+++ b/Makefile
@@ -939,6 +939,7 @@ TEST_SHELL_PATH = $(SHELL_PATH)
LIB_FILE = libgit.a
+ifndef NO_RUST
ifdef DEBUG
RUST_TARGET_DIR = target/debug
else
@@ -950,6 +951,7 @@ RUST_LIB = $(RUST_TARGET_DIR)/gitcore.lib
else
RUST_LIB = $(RUST_TARGET_DIR)/libgitcore.a
endif
+endif
GITLIBS = common-main.o $(LIB_FILE)
EXTLIBS =
@@ -3019,11 +3021,13 @@ scalar$X: scalar.o GIT-LDFLAGS $(GITLIBS)
$(LIB_FILE): $(LIB_OBJS)
$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
+ifndef NO_RUST
$(RUST_LIB): Cargo.toml $(RUST_SOURCES) $(LIB_FILE)
$(QUIET_CARGO)cargo build $(CARGO_ARGS)
.PHONY: rust
rust: $(RUST_LIB)
+endif
export DEFAULT_EDITOR DEFAULT_PAGER
@@ -4074,7 +4078,8 @@ $(LIBGIT_HIDDEN_EXPORT): $(LIBGIT_PARTIAL_EXPORT)
contrib/libgit-sys/libgitpub.a: $(LIBGIT_HIDDEN_EXPORT)
$(AR) $(ARFLAGS) $@ $^
-contrib/credential/osxkeychain/git-credential-osxkeychain: contrib/credential/osxkeychain/git-credential-osxkeychain.o $(LIB_FILE) GIT-LDFLAGS
+# When Rust is enabled, git-credential-osxkeychain depends on Rust symbols in $(RUST_LIB)
+contrib/credential/osxkeychain/git-credential-osxkeychain: contrib/credential/osxkeychain/git-credential-osxkeychain.o $(LIB_FILE) $(RUST_LIB) GIT-LDFLAGS
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS) -framework Security -framework CoreFoundation
--
gitgitgadget
^ permalink raw reply related
* [PATCH v8 0/3] Makefile: link osxkeychain helper against Rust
From: Shardul Natu via GitGitGadget @ 2026-07-08 3:21 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble
In-Reply-To: <pull.2288.v7.git.git.1783443745.gitgitgadget@gmail.com>
This series improves macOS build reliability, automated CI verification, and
distribution support when Rust is enabled in the Git build system. It
addresses three distinct challenges: a parallel build race condition in
git-credential-osxkeychain, support for macOS Universal Binaries
(multi-architecture distribution), and missing automated CI test wiring for
macOS contrib utilities.
Why This Series is Needed
=========================
1. Parallel Build Race Condition (make -j): While commit 522ea8ef7d
("osxkeychain: fix build with Rust") updated the link command for
git-credential-osxkeychain to pass $(LIBS), it omitted $(RUST_LIB) from
the target prerequisite list. When running a parallel build (make -j)
from a clean working tree, Make can attempt to link
git-credential-osxkeychain before Cargo has finished compiling
libgitcore.a, causing linker failures.
2. macOS Universal Binary (lipo) Support: On macOS, Universal Binaries
bundle native executable code for multiple architectures (Intel x86_64
and Apple Silicon arm64) into a single file. This is standard practice
for macOS distribution and CI packaging (such as Burrito, Homebrew, and
Git's macOS CI runners), allowing a single artifact to run natively
across all Macs without Rosetta translation.
While Apple's C compiler (clang) natively supports universal builds by
passing -arch x86_64 -arch arm64 in CFLAGS and LDFLAGS, Cargo and rustc do
not support multiple -arch flags in a single invocation. Instead, Cargo must
be invoked separately for each target triple (--target x86_64-apple-darwin
and --target aarch64-apple-darwin). This series bridges that gap.
3. Automated CI Verification for Contrib on macOS: When running make test
with TEST_CONTRIB_TOO=yes (default in macOS CI workflows), $(MAKE) -C
contrib/ test is invoked. However, contrib/Makefile only invoked tests
for diff-highlight and subtree, meaning git-credential-osxkeychain was
never compiled or verified during standard CI test runs.
Overview of Patches
===================
* Patch 1: Makefile: add $(RUST_LIB) prerequisite to osxkeychain Adds
$(RUST_LIB) as a prerequisite dependency to the osxkeychain target,
eliminating the parallel build race condition. Additionally, wraps the
definitions of $(RUST_LIB) and the rust build target in ifndef NO_RUST so
that disabling Rust cleanly makes the dependency a no-op.
* Patch 2: Makefile: support universal macOS builds via RUST_TARGETS Allows
users to specify space-separated target triples in RUST_TARGETS.
Introduces declarative pattern rules (target/%/...) to compile each
target slice via Cargo, and uses lipo (part of the mandatory Xcode
Command Line Tools) to combine the resulting static archives into a
universal library at target/release/libgitcore.a. Uses
mkdir_p_parent_template to guarantee directory creation before lipo.
* Patch 3: contrib: wire up osxkeychain in contrib/Makefile on macOS Adds
a test target to contrib/credential/osxkeychain/Makefile that depends
on building git-credential-osxkeychain. Introduces a generic OS_CONTRIB
variable in contrib/Makefile to conditionally wire
credential/osxkeychain into all, test, and clean whenever running on
macOS (Darwin). This guarantees that standard CI test runs on macOS
automatically compile and link the helper, preventing build
regressions.
Changes since v7:
* Added inclusion of ../config.mak.uname to the top of contrib/Makefile in
the canonical order. This guarantees that $(uname_S) is correctly defined
on the shell, preventing the OS_CONTRIB additions from being silently
ignored.
Changes since v5:
* Reverted Patch 1 to depend explicitly on $(LIB_FILE) $(RUST_LIB) rather
than $(GITLIBS). Unlike Git builtins or scalar (which define cmd_main()),
git-credential-osxkeychain.c defines its own standalone main(), meaning
$(GITLIBS) caused a duplicate symbol error for _main during linking.
* Added Patch 3 ("contrib: wire up osxkeychain in contrib/Makefile on
macOS") using a scalable OS_CONTRIB variable so that running make test
with TEST_CONTRIB_TOO=yes in macOS CI workflows automatically verifies
compilation and linking integrity.
Changes since v4:
* Changed the osxkeychain prerequisite dependency from $(LIB_FILE)
$(RUST_LIB) to $(GITLIBS) to match the canonical prerequisite pattern
used by all other core Git targets linking $(LIBS).
Changes since v3:
* Removed leading @ from $(call mkdir_p_parent_template) so it relies on
the built-in $(QUIET_MKDIR_P_PARENT) behavior, matching existing Makefile
conventions.
* Replaced if [ with if test in Bourne shell recipe snippets to strictly
adhere to the project's CodingGuidelines.
Changes since v2:
* Split the original combined commit into a two-patch series to separate
prerequisite bug fixes from Universal Binary features.
* Added $(call mkdir_p_parent_template) prior to invoking lipo to guarantee
that parent target directories exist.
Shardul Natu (3):
Makefile: add $(RUST_LIB) prerequisite to osxkeychain
Makefile: support universal macOS builds via RUST_TARGETS
contrib: wire up osxkeychain in contrib/Makefile on macOS
Makefile | 46 ++++++++++++++++++++++---
contrib/Makefile | 12 +++++++
contrib/credential/osxkeychain/Makefile | 4 ++-
3 files changed, 56 insertions(+), 6 deletions(-)
base-commit: 00534a21ce949ef80a5b8b9d7fc20b7d381038e9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2288%2Fkiranani%2Fnext-v8
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2288/kiranani/next-v8
Pull-Request: https://github.com/git/git/pull/2288
Range-diff vs v7:
1: 8f2bd4b14a = 1: 8f2bd4b14a Makefile: add $(RUST_LIB) prerequisite to osxkeychain
2: a999be6939 = 2: a999be6939 Makefile: support universal macOS builds via RUST_TARGETS
3: 32af2c51a8 ! 3: 5659709ab4 contrib: wire up osxkeychain in contrib/Makefile on macOS
@@ Commit message
## contrib/Makefile ##
@@
++include ../config.mak.uname
+-include ../config.mak.autogen
+-include ../config.mak
+
++
+ifeq ($(uname_S),Darwin)
+OS_CONTRIB += credential/osxkeychain
+endif
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v7 3/3] contrib: wire up osxkeychain in contrib/Makefile on macOS
From: Shardul Natu @ 2026-07-08 3:15 UTC (permalink / raw)
To: gitster
Cc: ben.knoble, git, gitgitgadget, koji.nakamaru, kristofferhaugsbakk,
ps, shardul.27591, snatu
In-Reply-To: <xmqqmrw3aoas.fsf@gitster.g>
> Is $(uname_S) defined here at this point with only the above two
> includes? Don't you need to include ../config.mak.uname as well?
>
> The top-level Makefile does this:
>
> include config.mak.uname
> -include config.mak.autogen
> -include config.mak
>
> and so should this one, I think, in exactly the same order.
Ah, yes. I have updated the include sequence in contrib/Makefile to:
include ../config.mak.uname
-include ../config.mak.autogen
-include ../config.mak
^ permalink raw reply
* [PATCH 3/3] t/README: document writing concurrency-safe helpers
From: Michael Montalbo via GitGitGadget @ 2026-07-08 2:59 UTC (permalink / raw)
To: git; +Cc: Michael Montalbo, Michael Montalbo
In-Reply-To: <pull.2171.git.1783479584.gitgitgadget@gmail.com>
From: Michael Montalbo <mmontalbo@gmail.com>
The apply-one-time-script.sh and http-429.sh fixes addressed the same
underlying problem: a test helper assuming it has exclusive access to a
file when the web server can run it for several requests at once. The
atomic idioms that avoid this are not specific to CGI or to HTTP, so
document them generally, alongside the other guidance for writing tests,
and leave a pointer from the lib-httpd helper list rather than a local
comment. The note covers the anti-pattern (a "test -f" then a separate
act) and the two safe operations (mkdir to elect a winner, rename to
consume a one-shot marker), citing Git's own lockfile machinery and
make_symlink() as precedent.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
t/README | 32 ++++++++++++++++++++++++++++++++
t/lib-httpd.sh | 3 +++
2 files changed, 35 insertions(+)
diff --git a/t/README b/t/README
index 085921be4b..a9d425f392 100644
--- a/t/README
+++ b/t/README
@@ -854,6 +854,38 @@ from the test harness library. At the end of the script, call
'test_done'.
+Writing concurrency-safe helpers
+--------------------------------
+
+Some test code runs concurrently: a test may background work with '&',
+and the helper scripts installed for the web server (in t/lib-httpd) are
+run once per request, so the same script can execute for several
+requests at once. Such code cannot assume it has exclusive access to a
+file.
+
+When exactly one of several concurrent processes needs to "win" a
+decision, a single atomic filesystem operation can make it, rather than
+a check followed by a separate action. A "test -f X" then "touch X"
+(or "rm X") races: two processes can both pass the check before either
+acts. Two atomic operations avoid this:
+
+ - "mkdir dir", which fails if the directory already exists, so that
+ exactly one caller wins, electing a first or only request (see
+ t/lib-httpd/http-429.sh).
+
+ - "mv src dst" (rename), which fails if the source is gone, so that
+ exactly one caller consumes it, claiming a planted one-shot marker
+ (see t/lib-httpd/apply-one-time-script.sh).
+
+A "$$" suffix on per-request scratch files keeps concurrent invocations
+from clobbering each other's fixed-name files.
+
+This is a standard shell locking idiom, and the same reasoning behind
+Git's own lockfile machinery, which creates its lock with O_CREAT|O_EXCL,
+and make_symlink() in t/test-lib.sh, which uses an mkdir lock: an atomic
+operation whose failure indicates that another process got there first.
+
+
Test harness library
--------------------
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index fc646447d5..d64f9c8c2d 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -159,6 +159,9 @@ prepare_httpd() {
mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
+ # The web server can run any of these CGI scripts for two requests at
+ # once; a helper that keeps state between requests must do so with an
+ # atomic operation. See "Writing concurrency-safe helpers" in t/README.
install_script incomplete-length-upload-pack-v2-http.sh
install_script incomplete-body-upload-pack-v2-http.sh
install_script error-no-report.sh
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic
From: Michael Montalbo via GitGitGadget @ 2026-07-08 2:59 UTC (permalink / raw)
To: git; +Cc: Michael Montalbo, Michael Montalbo
In-Reply-To: <pull.2171.git.1783479584.gitgitgadget@gmail.com>
From: Michael Montalbo <mmontalbo@gmail.com>
http-429.sh records "already returned 429 once" with a "test -f"
followed by a "touch" of a shared state file. That check-then-act is not
atomic: Apache can run this CGI for several requests at once, and two of
them can both pass the "test -f" before either "touch"es, so both treat
themselves as the first request. The retry flow that drives this
endpoint is mostly sequential, so this has not been seen to fail, but
the race is latent.
Decide whether this is the first request with a single atomic mkdir,
which fails if the directory already exists, so exactly one of any
concurrent requests is rate-limited and the rest are forwarded.
There is no accompanying regression test. The check and the set are
adjacent commands with no external step in between to synchronize on, so
the overlap cannot be forced deterministically, only reproduced
probabilistically; the fix is preventive.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
t/lib-httpd/http-429.sh | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh
index c97b16145b..d9bbedf1ad 100644
--- a/t/lib-httpd/http-429.sh
+++ b/t/lib-httpd/http-429.sh
@@ -26,14 +26,17 @@ repo_path="${remaining#*/}" # Get rest (repo path)
# The repo name is the first component before any "/"
repo_name="${repo_path%%/*}"
-# Use current directory (HTTPD_ROOT_PATH) for state file
-# Create a safe filename from test_context, retry_after and repo_name
-# This ensures all requests for the same test context share the same state file
+# Use current directory (HTTPD_ROOT_PATH) for state.
+# Create a safe name from test_context, retry_after and repo_name so that all
+# requests for the same test context share the same state.
safe_name=$(echo "${test_context}-${retry_after}-${repo_name}" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')
-state_file="http-429-state-${safe_name}"
+state="http-429-state-${safe_name}"
-# Check if this is the first call (no state file exists)
-if test -f "$state_file"
+# Apache can run this CGI for concurrent requests, so the script decides
+# whether this is the first call with a single atomic "mkdir": it succeeds for
+# exactly one of any racing requests and fails for the rest. "permanent"
+# always rate-limits and records no state.
+if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null
then
# Already returned 429 once, forward to git-http-backend
# Set PATH_INFO to just the repo path (without retry-after value)
@@ -52,9 +55,6 @@ then
exec "$GIT_EXEC_PATH/git-http-backend"
fi
-# Mark that we've returned 429
-touch "$state_file"
-
# Output HTTP 429 response
printf "Status: 429 Too Many Requests\r\n"
@@ -67,8 +67,7 @@ case "$retry_after" in
printf "Retry-After: invalid-format-123abc\r\n"
;;
permanent)
- # Always return 429, don't set state file for success
- rm -f "$state_file"
+ # Always return 429
printf "Retry-After: 1\r\n"
printf "Content-Type: text/plain\r\n"
printf "\r\n"
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests
From: Michael Montalbo via GitGitGadget @ 2026-07-08 2:59 UTC (permalink / raw)
To: git; +Cc: Michael Montalbo, Michael Montalbo
In-Reply-To: <pull.2171.git.1783479584.gitgitgadget@gmail.com>
From: Michael Montalbo <mmontalbo@gmail.com>
apply-one-time-script.sh checks for the "one-time-script" marker, runs
it, captures the git-http-backend response in the fixed-name files "out"
and "out_modified", and removes the marker only after it has finished
serving the modified response. Because the client receives the response
body before that removal, it can start its next request while the marker
still exists. Apache can then run this CGI for two requests at once: a
partial fetch that receives a REF_DELTA against a missing promisor
object lazily fetches that base while the first response is still in
flight. The second request passes the marker check, the first request
then removes the marker, and the second fails to exec the now-missing
marker, emits no output, and the server answers HTTP 500:
fatal: ... The requested URL returned error: 500
fatal: could not fetch <oid> from promisor remote
This has been seen as a flaky failure of t5616.47 on the macOS CI
runners.
Claim the marker atomically with a rename, and only once the one-time
script has succeeded and actually changed the response; give the scratch
files per-request names. A request that loses the rename, or whose
script fails or leaves the response unchanged, serves the unmodified
body and keeps the marker for a later request. No path emits an empty
body, so the HTTP 500 no longer occurs.
Add t5567 to lock this down. The overlap depends on timing, so a live
httpd test such as t5616.47 (the real code path) passes almost every
time even against the buggy helper; t5567 instead drives the helper
directly with a fake git-http-backend and forces the overlap with FIFOs.
Against the pre-fix helper it fails with the same shell error seen in
the field:
./one-time-script: No such file or directory
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
t/lib-httpd/apply-one-time-script.sh | 38 +++++++----
t/meson.build | 1 +
t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++
3 files changed, 121 insertions(+), 14 deletions(-)
create mode 100755 t/t5567-one-time-script.sh
diff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh
index b1682944e2..a298ae89ae 100644
--- a/t/lib-httpd/apply-one-time-script.sh
+++ b/t/lib-httpd/apply-one-time-script.sh
@@ -6,21 +6,31 @@
#
# This can be used to simulate the effects of the repository changing in
# between HTTP request-response pairs.
-if test -f one-time-script
-then
- LC_ALL=C
- export LC_ALL
+#
+# Apache can run this CGI for concurrent requests (for example a partial fetch
+# that lazily fetches a missing object while the first response is still in
+# flight), so the helper claims the marker atomically with a rename, and only
+# once it has decided to modify the response. A request that loses the race
+# finds the marker already gone and serves its response unchanged; no request
+# is left emitting an empty body, which the server would report as HTTP 500.
+# Scratch files are per-request ($$) so concurrent requests do not clobber each
+# other.
+
+test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
- "$GIT_EXEC_PATH/git-http-backend" >out
- ./one-time-script out >out_modified
+LC_ALL=C
+export LC_ALL
- if cmp -s out out_modified
- then
- cat out
- else
- cat out_modified
- rm one-time-script
- fi
+out=out.$$
+modified=out-modified.$$
+"$GIT_EXEC_PATH/git-http-backend" >"$out"
+
+if ./one-time-script "$out" 2>/dev/null >"$modified" &&
+ ! cmp -s "$out" "$modified" &&
+ mv one-time-script one-time-script.$$ 2>/dev/null
+then
+ cat "$modified"
else
- "$GIT_EXEC_PATH/git-http-backend"
+ cat "$out"
fi
+rm -f "$out" "$modified" one-time-script.$$
diff --git a/t/meson.build b/t/meson.build
index 3219264fe7..a118a4d719 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -707,6 +707,7 @@ integration_tests = [
't5564-http-proxy.sh',
't5565-push-multiple.sh',
't5566-push-group.sh',
+ 't5567-one-time-script.sh',
't5570-git-daemon.sh',
't5571-pre-push-hook.sh',
't5572-pull-submodule.sh',
diff --git a/t/t5567-one-time-script.sh b/t/t5567-one-time-script.sh
new file mode 100755
index 0000000000..cd8e656005
--- /dev/null
+++ b/t/t5567-one-time-script.sh
@@ -0,0 +1,96 @@
+#!/bin/sh
+
+test_description='apply-one-time-script CGI helper is safe under concurrent requests'
+
+. ./test-lib.sh
+
+HELPER="$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh"
+
+test_expect_success PIPE 'concurrent requests: one rewritten, one passed through, neither empty' '
+ mkdir workdir fakebin &&
+ ENTERED="$PWD/entered" &&
+ GATE="$PWD/gate" &&
+ export ENTERED GATE &&
+ mkfifo "$ENTERED" "$GATE" &&
+
+ # Stand in for git-http-backend. The modify role returns a response
+ # containing "packfile", which the one-time script rewrites. The
+ # passthrough role returns a response that is left untouched, but first
+ # announces that it has entered the helper and then blocks, so that it
+ # is still in flight when the modify role claims and removes the marker.
+ write_script fakebin/git-http-backend <<-\EOF &&
+ printf "Status: 200 OK\r\n"
+ printf "Content-Type: application/x-git-result\r\n"
+ printf "\r\n"
+ if test "$ROLE" = modify
+ then
+ printf "packfile\n"
+ else
+ echo entered >"$ENTERED"
+ read -r released <"$GATE"
+ printf "refs\n"
+ fi
+ EOF
+
+ # The transform that replace_packfile would install as one-time-script:
+ # rewrite responses that contain "packfile", leave the rest alone.
+ write_script workdir/one-time-script <<-\EOF &&
+ if grep packfile "$1" >/dev/null
+ then
+ sed "/packfile/q" "$1" &&
+ printf "REPLACED\n"
+ else
+ cat "$1"
+ fi
+ EOF
+
+ GIT_EXEC_PATH="$PWD/fakebin" &&
+ export GIT_EXEC_PATH &&
+
+ # Hold GATE open read-write on fd 9 for the duration, so releasing the
+ # passthrough request below cannot block even if that request has
+ # already exited (it keeps a reader on the FIFO).
+ exec 9<>"$GATE" &&
+
+ # Launch the passthrough request in the background. It enters the
+ # helper, signals us through ENTERED, then blocks on GATE inside the
+ # fake backend. The braces keep the && chain intact while backgrounding
+ # only the subshell, so "wait" can reap it by pid; kill it on any exit
+ # so a stray blocked child cannot hold the test output open and stall a
+ # reader such as prove.
+ { (
+ cd workdir &&
+ ROLE=passthrough sh "$HELPER" >../passthrough.out 2>../passthrough.err
+ ) & } &&
+ passthrough_pid=$! &&
+ test_when_finished "kill $passthrough_pid 2>/dev/null || :" &&
+
+ # Wait until the passthrough request is past the marker check.
+ read -r entered <"$ENTERED" &&
+
+ # Run the modifying request to completion while the passthrough request
+ # is still blocked.
+ (
+ cd workdir &&
+ ROLE=modify sh "$HELPER" >../modify.out 2>../modify.err
+ ) &&
+
+ # Release the passthrough request and let it finish. Ignore the helper
+ # exit status here so a broken helper is diagnosed by the assertions
+ # below rather than aborting the test.
+ echo released >&9 &&
+ { wait "$passthrough_pid" || :; } &&
+
+ # Neither request may error out or produce an empty (HTTP 500) body,
+ # and each must have played its role: the modify request rewrote its
+ # response and the passthrough request came through untouched.
+ test_must_be_empty passthrough.err &&
+ test_must_be_empty modify.err &&
+ test_grep "Status: 200 OK" passthrough.out &&
+ test_grep "Status: 200 OK" modify.out &&
+ test_grep REPLACED modify.out &&
+ test_grep ! REPLACED passthrough.out &&
+ test_grep refs passthrough.out
+'
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/3] t/lib-httpd: make CGI test helpers concurrency-safe
From: Michael Montalbo via GitGitGadget @ 2026-07-08 2:59 UTC (permalink / raw)
To: git; +Cc: Michael Montalbo
The httpd tests share a handful of CGI helper scripts under t/lib-httpd. Two
of them keep state between requests in the shared HTTPD_ROOT_PATH on the
assumption that the web server hands them one request at a time. It does
not: Apache serves requests concurrently, and a single Git operation can
open more than one request to the same endpoint at once. A partial fetch
that receives a REF_DELTA against a missing promisor object lazily fetches
that base while the first response is still being served.
Under that overlap apply-one-time-script.sh loses: two requests both pass
its "test -f one-time-script" check, one removes the marker, and the other
fails to exec it and emits an empty body, which the server answers as HTTP
500. In the field this is an occasional failure[1] of:
t5616.47 tolerate server sending REF_DELTA against missing promisor objects
on the macOS CI runners, with:
fatal: ... The requested URL returned error: 500 fatal: could not fetch from
promisor remote
I could not reproduce it against a live server (the window is tiny and
timing-dependent), but the macOS CI error log names the exact failure, and
the new test reproduces the helper's shell error.
http-429.sh keeps its "already returned 429 once" state with the same
non-atomic test-and-set. Its retry flow is mostly sequential so it seems
less likely to fail, but it is the same latent race.
Each fix is local: claim/consume the one-shot marker with an atomic rename,
and elect the first request with an atomic mkdir, rather than a "test -f"
followed by a separate remove or touch.
* Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,
which drives the helper directly with no web server so the overlap can be
forced deterministically.
* Patch 2 makes http-429.sh atomic.
* Patch 3 documents the atomic idioms generally in t/README (they are not
specific to CGI or HTTP), citing Git's own lockfile machinery and
make_symlink(), with a pointer from the lib-httpd list.
[1]
https://github.com/gitgitgadget/git/actions/runs/28756172690/job/85263916762?pr=2169
Michael Montalbo (3):
t/lib-httpd: fix apply-one-time-script race under concurrent requests
t/lib-httpd: make http-429 first-request check atomic
t/README: document writing concurrency-safe helpers
t/README | 32 ++++++++++
t/lib-httpd.sh | 3 +
t/lib-httpd/apply-one-time-script.sh | 38 +++++++----
t/lib-httpd/http-429.sh | 21 +++---
t/meson.build | 1 +
t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++
6 files changed, 166 insertions(+), 25 deletions(-)
create mode 100755 t/t5567-one-time-script.sh
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2171%2Fmmontalbo%2Fmm%2Flib-httpd-cgi-safe-proto-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2171
--
gitgitgadget
^ permalink raw reply
* [PATCH] merge --abort: don't delete autostash before reset succeeds
From: Kris Point @ 2026-07-08 1:51 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: gitster@pobox.com
From bf4b12438a83d81f2c8df6e39f6114ddd5002430 Mon Sep 17 00:00:00 2001
From: KrisPointCSGO <KrisPointCSGO@outlook.com>
Date: Tue, 7 Jul 2026 20:10:00 +0800
Subject: [PATCH] merge --abort: don't delete autostash before reset succeeds
To: git@vger.kernel.org
Cc: gitster@pobox.com
In cmd_merge()'s --abort path, MERGE_AUTOSTASH was deleted before
cmd_reset() was called. If cmd_reset() failed (e.g. due to a locked
index), the autostash was permanently lost.
Instead, read the MERGE_AUTOSTASH OID without deleting the ref, run
cmd_reset() (which itself calls remove_branch_state() ->
save_autostash_ref() to persist the stash), and only apply the
autostash on success.
Reported-by: KrisPoint
Signed-off-by: KrisPoint <KrisPointCSGO@outlook.com>
---
builtin/merge.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 5b46a596f0..5d9a242027 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1427,15 +1427,14 @@ int cmd_merge(int argc,
if (!file_exists(git_path_merge_head(the_repository)))
die(_("There is no merge to abort (MERGE_HEAD missing)."));
- if (!refs_read_ref(get_main_ref_store(the_repository), "MERGE_AUTOSTASH", &stash_oid))
- refs_delete_ref(get_main_ref_store(the_repository),
- "", "MERGE_AUTOSTASH", &stash_oid,
- REF_NO_DEREF);
+ refs_read_ref(get_main_ref_store(the_repository), "MERGE_AUTOSTASH", &stash_oid);
- /* Invoke 'git reset --merge' */
+ /* Invoke 'git reset --merge' (which also cleans up merge state,
+ * including saving the autostash to the stash list).
+ */
ret = cmd_reset(nargc, nargv, prefix, the_repository);
- if (!is_null_oid(&stash_oid)) {
+ if (!ret && !is_null_oid(&stash_oid)) {
oid_to_hex_r(stash_oid_hex, &stash_oid);
apply_autostash_oid(stash_oid_hex);
}
--
2.53.0
I've already changed the format to plain text. I don't think I did anything wrong.
^ permalink raw reply related
* Re: [PATCH v2] t1410-reflog.sh: avoid suppressing git's exit code in pipelines
From: Junio C Hamano @ 2026-07-08 1:48 UTC (permalink / raw)
To: Gatla Vishweshwar Reddy; +Cc: git
In-Reply-To: <20260707135530.17389-1-gatlavishweshwarreddy26@gmail.com>
Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:
> Piping git commands directly to wc -l suppresses the exit code of
> git, hiding potential failures from the test suite. Capture the
> output to a temporary file first, then count the lines separately
> to preserve the exit code. Where the expected count is known ahead
> of time, use test_stdout_line_count instead.
>
> Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
> ---
> t/t1410-reflog.sh | 29 ++++++++++++++++-------------
> 1 file changed, 16 insertions(+), 13 deletions(-)
The above descripotion looks reasonble.
By the way, Documentation/SubmittingPatches has this:
Before sending another version, make sure you have answered
meaningful review comments in the existing discussion. Also
give reviewers enough time to comment before sending another
version.
> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index ce71f9a30a..8e018d172b 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -244,26 +244,30 @@ test_expect_success 'delete' '
> test_tick &&
> git commit -m tiger C &&
>
> - HEAD_entry_count=$(git reflog | wc -l) &&
> - main_entry_count=$(git reflog show main | wc -l) &&
> -
> - test $HEAD_entry_count = 5 &&
> - test $main_entry_count = 5 &&
> -
> + test_stdout_line_count = 5 git reflog &&
> + git reflog >reflog_output &&
> + HEAD_entry_count=$(wc -l <reflog_output) &&
> + test_stdout_line_count = 5 git reflog show main &&
> + git reflog show main >reflog_main_output &&
> + main_entry_count=$(wc -l <reflog_main_output) &&
>
> git reflog delete main@{1} &&
> git reflog show main > output &&
> test_line_count = $(($main_entry_count - 1)) output &&
> - test $HEAD_entry_count = $(git reflog | wc -l) &&
> + git reflog >reflog_output &&
> + test $HEAD_entry_count = $(wc -l <reflog_output) &&
> ! grep ox < output &&
Now, you no longer have new consecutive blank lines in the above,
but the above shares the same "what did the author meant to convey
with this blank line?" puzzlement.
The updated code somehow wanders around in many directions like a
drunken man. Let's comment on each line.
> + test_stdout_line_count = 5 git reflog &&
This is "Does the reflog for HEAD have exactly 5 entries?" test.
> + git reflog >reflog_output &&
> + HEAD_entry_count=$(wc -l <reflog_output) &&
As we already saw that HEAD_entry_count variable is exactly equal to
5, it is puzzling why we want to perform this computation again and
assign the result to the variable.
> + test_stdout_line_count = 5 git reflog show main &&
And then we check "Does the reflog for 'main' have exactly 5
entries?"
> + git reflog show main >reflog_main_output &&
> + main_entry_count=$(wc -l <reflog_main_output) &&
And recompute what we already know and asssign to main_entry_count
variable, which shares the same puzzlement.
>
> git reflog delete main@{1} &&
> git reflog show main > output &&
> test_line_count = $(($main_entry_count - 1)) output &&
Now, after a blank line, it goes on to test a completely different
thing, which is "after deleting an entry in main's reflog, can we
count how many there is, and does it match what we expect, which is
the previous count minus 1"? Why should we even need to do so, when
git reflog delete main@{1} &&
test_stdout_line_count = 4 git reflog show main &&
would do just fine?
> - test $HEAD_entry_count = $(git reflog | wc -l) &&
> + git reflog >reflog_output &&
> + test $HEAD_entry_count = $(wc -l <reflog_output) &&
And then it comes back to test what we already know, i.e. "does the
reflog for HEAD have 5 entries?". Which we tested earlier already.
Are we interested in checking that "reflog delete main@{1}" does
not affect the reflog for HEAD? If so, doing
test_stdout_line_count = 5 git reflog &&
again here would be simpler, no? That way, there is no need to
recompute and assign to the {HEAD,main}_entry_count variables in the
earlier part of the tests.
I guess the same comment applies to the remainder of this test,
where it is checked that a removal from HEAD reflog does not affect
the reflog of main.
> main_entry_count=$(wc -l < output) &&
>
> git reflog delete HEAD@{1} &&
> - test $(($HEAD_entry_count -1)) = $(git reflog | wc -l) &&
> - test $main_entry_count = $(git reflog show main | wc -l) &&
> + git reflog >reflog_output &&
> + test $(($HEAD_entry_count -1)) = $(wc -l <reflog_output) &&
> + git reflog show main >reflog_main_output &&
> + test $main_entry_count = $(wc -l <reflog_main_output) &&
>
> - HEAD_entry_count=$(git reflog | wc -l) &&
> + git reflog >reflog_output &&
> + HEAD_entry_count=$(wc -l <reflog_output) &&
>
> git reflog delete main@{07.04.2005.15:15:00.-0700} &&
> git reflog show main > output &&
> @@ -319,13 +323,12 @@ test_expect_success 'git reflog expire unknown reference' '
> test_must_fail git reflog expire does-not-exist 2>stderr &&
> test_grep "error: reflog could not be found: ${SQ}does-not-exist${SQ}" stderr
> '
> -
> test_expect_success 'checkout should not delete log for packed ref' '
> - test $(git reflog main | wc -l) = 4 &&
> + test_stdout_line_count = 4 git reflog main &&
> git branch foo &&
> git pack-refs --all &&
> git checkout foo &&
> - test $(git reflog main | wc -l) = 4
> + test_stdout_line_count = 4 git reflog main
> '
>
> test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' '
^ permalink raw reply
* Re: [PATCH v3 1/4] t1517: skip svn tests if svn is not installed
From: Junio C Hamano @ 2026-07-08 1:28 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Jeff King
In-Reply-To: <20260708001557.3581080-2-sandals@crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> The svn tests currently assume that git-svn's option parsing will always
> fail the tests because it exits 0 on --help, not 129. However, in a
> future commit, we'll expect it to exit 0 and the tests will then need to
> be updated to succeed in some cases and fail in others.
>
> We therefore need to have t1517 determine whether the Subversion Perl
> modules are present, since if they are not, git-svn will die on start
> and then it needs to continue to expect failure. Add a stripped down
> version of the tests in t/lib-git-svn.sh as a prerequisite we can use
> here for our svn tests.
Perfect. This is stripped down because what this models after is
broken in the statement that this version does not copy, IIRC, which
may deserve to be stated here to help future readers.
^ permalink raw reply
* Re: [PATCH v3 2/4] parse-options: add a separate case for help output on error
From: Junio C Hamano @ 2026-07-08 1:26 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Jeff King
In-Reply-To: <20260708001557.3581080-3-sandals@crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> @@ -1363,7 +1368,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
> parse_options_check_harder(opts);
>
> if (!usagestr)
> - return PARSE_OPT_HELP;
> + return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
This part, IIUC, got updated from the previous round. Looks
sensible.
> @@ -1476,7 +1481,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
> if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
> fputs("EOF\n", outfile);
>
> - return PARSE_OPT_HELP;
> + return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
> }
And the above matches this part, of course.
^ permalink raw reply
* [PATCH] SubmittingPatches: abandoning a series
From: Junio C Hamano @ 2026-07-08 1:20 UTC (permalink / raw)
To: git
The document describes an idealized life cycle for a patch series,
where an author scratches their itch, improves the patch(es) with
help from fellow reviewers, and iterate until their work becomes a
part of Git.
But sometimes a topic may have to be abandoned or retracted, with an
option to later resurrect it when they can, and it is much better
than leaving a topic in limbo. Clearly state that we encourage
contributors to explicitly retract their topic that did not succeed.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/SubmittingPatches | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches
index f042bb5aaf..f14ae20aaa 100644
--- c/Documentation/SubmittingPatches
+++ w/Documentation/SubmittingPatches
@@ -95,6 +95,21 @@ input and avoids unnecessary churn from many rapid iterations.
top, it gets merged to the 'master' branch and waits to become part
of the next major release.
+But sometimes things do not work as planned.
+
+. A discussion on the list might convince you that your changes are
+ not such a good idea, in which case you are expected to explicitly
+ retract the topic, to releave the maintainer from having to worry
+ about it.
+
+. You may have to stop pursuing the topic due to various reasons like
+ lack of time, other commitments, shifting priorities, etc. It is a
+ friendly thing to do to tell the list in such a case, so that others
+ interested in the topic can take over the topic and continue. When
+ there is no taker, the maintainer may have to discard the topic, but
+ anybody can resurrect the topic later when they (including you) can
+ spend more time on it.
+
In the following sections, many techniques and conventions are listed
to help your patches get reviewed effectively in such a life cycle.
^ permalink raw reply related
* Re: [PATCH v7 0/3] Teach git-replay(1) to linearize merge commits
From: Junio C Hamano @ 2026-07-08 1:02 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Elijah Newren, Johannes Schindelin
In-Reply-To: <20260707-toon-git-replay-drop-merges-v7-0-808ab9b4afa6@iotcl.com>
Toon Claes <toon@iotcl.com> writes:
> This series might conflict with Kristoffer's series to make
> documentation changes[2], but should be trivial to resolve. And I don't
> think there's a conflict with Patrick's series on adding "drop" to
> git-history(1)[3].
>
> dscho's series to replay merges[1] needs a bit of rework to fit on top
> of this, but I'm happy to help figuring that out. We've been discussing
> to either name the option --flatten or --linearize, but I've decided on
> "linearize" because the documentation of git-rebase(1) also mentions
> "linearize".
>
> [1]: <pull.2106.git.1778107405.gitgitgadget@gmail.com>
> [2]: <V3_CV_doc_replay_config.780@msgid.xyz>
> [3]: <20260603-b4-pks-history-drop-v2-0-742cb5b5176d@pks.im>
>
> ---
> Changes in v7:
> - Allow --revert and --linearize to be used together.
> - Because quite a lot of changes have been made since the original
> patch, change author from Johannes to Toon for the last commit.
> Johannes already told me he doesn't really care about authorship when
> he initially shared the patch with me.
Looks like all the previous review comments have been answered and
the topic is in a good shape to be merged to 'next' (and allow us to
polish incrementally as needed)?
Thanks for working on the topic. Let me mark it for 'next'.
^ permalink raw reply
* Re: [PATCH v7 3/3] contrib: wire up osxkeychain in contrib/Makefile on macOS
From: Junio C Hamano @ 2026-07-08 0:58 UTC (permalink / raw)
To: Shardul Natu via GitGitGadget
Cc: git, Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble
In-Reply-To: <32af2c51a892c2fd646a867df7eb5224d5ea39c2.1783443745.git.gitgitgadget@gmail.com>
"Shardul Natu via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Shardul Natu <snatu@google.com>
>
> When running "make test" with TEST_CONTRIB_TOO=yes (which is default in
> macOS CI workflows), $(MAKE) -C contrib/ test is invoked. However,
> contrib/Makefile only invoked tests for diff-highlight and subtree,
> meaning git-credential-osxkeychain was never built or verified during
> standard CI test runs.
>
> Add a "test" target to contrib/credential/osxkeychain/Makefile that
> depends on building git-credential-osxkeychain. Additionally, wire up
> credential/osxkeychain in contrib/Makefile under "all", "test", and
> "clean" whenever running on macOS (Darwin).
>
> This ensures that running "make test" or "make all" in contrib on macOS
> automatically builds and links git-credential-osxkeychain, preventing
> future build or symbol linking regressions from slipping through CI.
>
> Signed-off-by: Shardul Natu <snatu@google.com>
> ---
> contrib/Makefile | 10 ++++++++++
> contrib/credential/osxkeychain/Makefile | 4 +++-
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/Makefile b/contrib/Makefile
> index 787cd07f52..7962a9ff12 100644
> --- a/contrib/Makefile
> +++ b/contrib/Makefile
> @@ -1,10 +1,20 @@
> +-include ../config.mak.autogen
> +-include ../config.mak
> +
> +ifeq ($(uname_S),Darwin)
> +OS_CONTRIB += credential/osxkeychain
> +endif
Is $(uname_S) defined here at this point with only the above two
includes? Don't you need to include ../config.mak.uname as well?
The top-level Makefile does this:
include config.mak.uname
-include config.mak.autogen
-include config.mak
and so should this one, I think, in exactly the same order.
> all::
> + $(foreach dir,$(OS_CONTRIB),$(MAKE) -C $(dir) $@;)
>
> test::
> $(MAKE) -C diff-highlight $@
> $(MAKE) -C subtree $@
> + $(foreach dir,$(OS_CONTRIB),$(MAKE) -C $(dir) $@;)
>
> clean::
> $(MAKE) -C contacts $@
> $(MAKE) -C diff-highlight $@
> $(MAKE) -C subtree $@
> + $(foreach dir,$(OS_CONTRIB),$(MAKE) -C $(dir) $@;)
> diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile
> index 219b0d7f49..d9fba07e8d 100644
> --- a/contrib/credential/osxkeychain/Makefile
> +++ b/contrib/credential/osxkeychain/Makefile
> @@ -10,4 +10,6 @@ install:
> clean:
> $(MAKE) -C ../../.. clean-git-credential-osxkeychain
>
> -.PHONY: all git-credential-osxkeychain install clean
> +test: git-credential-osxkeychain
> +
> +.PHONY: all git-credential-osxkeychain install clean test
^ permalink raw reply
* [PATCH v3 4/4] parse-options: exit 0 on -h
From: brian m. carlson @ 2026-07-08 0:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260708001557.3581080-1-sandals@crustytoothpaste.net>
The standard philosophy for Unix software when a help option (such as
--help) is specified is that the software should exit 0, printing the
help output to standard output, since the standard output is for
user-requested output and the program performed the requested task
successfully. If the user specifies an incorrect option, then the help
output should be printed to standard error (since the user has made a
mistake) and it should exit unsuccessfully.
Most of our commands currently exit 129 on receiving the -h option to
print the short help, which does not line up with the standard
philosophy above. Let's change that to exit 0 instead.
This requires changes to a variety of tests which previously wanted the
129 exit code, so update them. Note that because git diff does its own
option parsing, it still exits with 129, so update some of the tests to
expect either exit status.
Some commands also now pass with -h but not --help-all, so handle those
cases differently for those commands.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
builtin/blame.c | 1 +
builtin/shortlog.c | 1 +
builtin/update-index.c | 1 +
contrib/subtree/t/t7900-subtree.sh | 2 +-
parse-options.c | 11 ++++++---
t/for-each-ref-tests.sh | 2 +-
t/t0012-help.sh | 2 +-
t/t0040-parse-options.sh | 2 +-
t/t0450-txt-doc-vs-help.sh | 2 +-
t/t0610-reftable-basics.sh | 4 +--
t/t1403-show-ref.sh | 2 +-
t/t1410-reflog.sh | 4 +--
t/t1418-reflog-exists.sh | 2 +-
t/t1502-rev-parse-parseopt.sh | 14 +++++------
t/t1517-outside-repo.sh | 39 ++++++++++++++++++------------
t/t1800-hook.sh | 4 +--
t/t1900-repo-info.sh | 2 +-
t/t1901-repo-structure.sh | 2 +-
t/t2006-checkout-index-basic.sh | 6 ++---
t/t2107-update-index-basic.sh | 2 +-
t/t3004-ls-files-basic.sh | 6 ++---
t/t3200-branch.sh | 2 +-
t/t3903-stash.sh | 4 +--
t/t4200-rerere.sh | 2 +-
t/t5200-update-server-info.sh | 2 +-
t/t5304-prune.sh | 2 +-
t/t5400-send-pack.sh | 4 +--
t/t5512-ls-remote.sh | 2 +-
t/t6300-for-each-ref.sh | 4 +--
t/t6500-gc.sh | 2 +-
t/t7030-verify-tag.sh | 4 +--
t/t7508-status.sh | 4 +--
t/t7510-signed-commit.sh | 4 +--
t/t7600-merge.sh | 2 +-
t/t7800-difftool.sh | 3 +--
t/t7900-maintenance.sh | 2 +-
usage.c | 2 +-
37 files changed, 85 insertions(+), 71 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 65d43c7d48..38749f79c2 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1013,6 +1013,7 @@ int cmd_blame(int argc,
case PARSE_OPT_UNKNOWN:
break;
case PARSE_OPT_HELP:
+ exit(0);
case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_ERROR:
case PARSE_OPT_SUBCOMMAND:
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index cd262bd376..4c78d2e5ba 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -433,6 +433,7 @@ int cmd_shortlog(int argc,
case PARSE_OPT_UNKNOWN:
break;
case PARSE_OPT_HELP:
+ exit(0);
case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_ERROR:
case PARSE_OPT_SUBCOMMAND:
diff --git a/builtin/update-index.c b/builtin/update-index.c
index ac4610ec94..6810327209 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1133,6 +1133,7 @@ int cmd_update_index(int argc,
break;
switch (parseopt_state) {
case PARSE_OPT_HELP:
+ exit(0);
case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_ERROR:
exit(129);
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 4194687cfb..c10f283b38 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -99,7 +99,7 @@ test_create_subtree_add () {
}
test_expect_success 'shows short help text for -h' '
- test_expect_code 129 git subtree -h >out 2>err &&
+ git subtree -h >out 2>err &&
test_must_be_empty err &&
grep -e "^ *or: git subtree pull" out &&
grep -F -e "--[no-]annotate" out
diff --git a/parse-options.c b/parse-options.c
index cc3a8b0fe3..08c21d9fc0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1135,8 +1135,9 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
case PARSE_OPT_UNKNOWN:
goto unknown;
case PARSE_OPT_HELP:
- case PARSE_OPT_HELP_ERROR:
goto show_usage;
+ case PARSE_OPT_HELP_ERROR:
+ goto show_usage_stderr;
case PARSE_OPT_NON_OPTION:
case PARSE_OPT_SUBCOMMAND:
case PARSE_OPT_COMPLETE:
@@ -1170,6 +1171,9 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
show_usage:
return usage_with_options_internal(ctx, usagestr, options,
USAGE_NORMAL, USAGE_TO_STDOUT);
+ show_usage_stderr:
+ return usage_with_options_internal(ctx, usagestr, options,
+ USAGE_NORMAL, USAGE_TO_STDERR);
}
int parse_options_end(struct parse_opt_ctx_t *ctx)
@@ -1201,6 +1205,7 @@ int parse_options(int argc, const char **argv,
parse_options_start_1(&ctx, argc, argv, prefix, options, flags);
switch (parse_options_step(&ctx, options, usagestr)) {
case PARSE_OPT_HELP:
+ exit(0);
case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_ERROR:
exit(129);
@@ -1500,11 +1505,11 @@ void show_usage_with_options_if_asked(int ac, const char **av,
if (!strcmp(av[1], "-h")) {
usage_with_options_internal(NULL, usagestr, opts,
USAGE_NORMAL, USAGE_TO_STDOUT);
- exit(129);
+ exit(0);
} else if (!strcmp(av[1], "--help-all")) {
usage_with_options_internal(NULL, usagestr, opts,
USAGE_FULL, USAGE_TO_STDOUT);
- exit(129);
+ exit(0);
}
}
}
diff --git a/t/for-each-ref-tests.sh b/t/for-each-ref-tests.sh
index bd2d45c971..b95e5b6ca0 100644
--- a/t/for-each-ref-tests.sh
+++ b/t/for-each-ref-tests.sh
@@ -522,7 +522,7 @@ test_expect_success 'Verify descending sort' '
'
test_expect_success 'Give help even with invalid sort atoms' '
- test_expect_code 129 ${git_for_each_ref} --sort=bogus -h >actual 2>&1 &&
+ ${git_for_each_ref} --sort=bogus -h >actual 2>&1 &&
grep "^usage: ${git_for_each_ref}" actual
'
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index c33501bdcd..7815ff14f2 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -260,7 +260,7 @@ do
(
GIT_CEILING_DIRECTORIES=$(pwd) &&
export GIT_CEILING_DIRECTORIES &&
- test_expect_code 129 git -C sub $builtin -h >output 2>err
+ git -C sub $builtin -h >output 2>err
) &&
test_must_be_empty err &&
test_grep usage output
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ca55ea8228..30895ad6d2 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -68,7 +68,7 @@ Alias
EOF
test_expect_success 'test help' '
- test_must_fail test-tool parse-options -h >output 2>output.err &&
+ test-tool parse-options -h >output 2>output.err &&
test_must_be_empty output.err &&
test_cmp expect output
'
diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
index 822b0d55a5..d2844368e4 100755
--- a/t/t0450-txt-doc-vs-help.sh
+++ b/t/t0450-txt-doc-vs-help.sh
@@ -29,7 +29,7 @@ help_to_synopsis () {
return 0
fi &&
mkdir -p "$out_dir" &&
- test_expect_code 129 git $builtin -h >"$out.raw" 2>&1 &&
+ test_might_fail git $builtin -h >"$out.raw" 2>&1 &&
sed -n \
-e '1,/^$/ {
/^$/d;
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index e19e036898..4135db95ed 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -15,9 +15,9 @@ export GIT_TEST_DEFAULT_REF_FORMAT
INVALID_OID=$(test_oid 001)
test_expect_success 'pack-refs does not crash with -h' '
- test_expect_code 129 git pack-refs -h >usage &&
+ git pack-refs -h >usage &&
test_grep "[Uu]sage: git pack-refs " usage &&
- test_expect_code 129 nongit git pack-refs -h >usage &&
+ nongit git pack-refs -h >usage &&
test_grep "[Uu]sage: git pack-refs " usage
'
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 36c903ca19..db4300da44 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -165,7 +165,7 @@ test_expect_success 'show-ref --branches, --tags, --head, pattern' '
'
test_expect_success 'show-ref --heads is deprecated and hidden' '
- test_expect_code 129 git show-ref -h >short-help &&
+ git show-ref -h >short-help &&
test_grep ! -e --heads short-help &&
git show-ref --heads >actual 2>warning &&
test_grep ! deprecated warning &&
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index ce71f9a30a..6e921bc167 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -107,12 +107,12 @@ test_expect_success setup '
'
test_expect_success 'correct usage on sub-command -h' '
- test_expect_code 129 git reflog expire -h >err &&
+ git reflog expire -h >err &&
grep "git reflog expire" err
'
test_expect_success 'correct usage on "git reflog show -h"' '
- test_expect_code 129 git reflog show -h >err &&
+ git reflog show -h >err &&
grep -F "git reflog [show]" err
'
diff --git a/t/t1418-reflog-exists.sh b/t/t1418-reflog-exists.sh
index d51ecd5e92..10387792e3 100755
--- a/t/t1418-reflog-exists.sh
+++ b/t/t1418-reflog-exists.sh
@@ -12,7 +12,7 @@ test_expect_success 'setup' '
test_expect_success 'usage' '
test_expect_code 129 git reflog exists &&
- test_expect_code 129 git reflog exists -h
+ git reflog exists -h
'
test_expect_success 'usage: unknown option' '
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 455608c429..fa97591b9f 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -75,7 +75,7 @@ EOF
'
test_expect_success 'test --parseopt help output' '
- test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec &&
+ git rev-parse --parseopt -- -h > output < optionspec &&
test_cmp "$TEST_DIRECTORY/t1502/optionspec.help" output
'
@@ -89,7 +89,7 @@ test_expect_success 'test --parseopt help output no switches' '
|EOF
|exit 0
END_EXPECT
- test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&
+ git rev-parse --parseopt -- -h > output < optionspec_no_switches &&
test_cmp expect output
'
@@ -103,7 +103,7 @@ test_expect_success 'test --parseopt help output hidden switches' '
|EOF
|exit 0
END_EXPECT
- test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&
+ git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&
test_cmp expect output
'
@@ -119,7 +119,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
|EOF
|exit 0
END_EXPECT
- test_expect_code 129 git rev-parse --parseopt -- --help-all > output < optionspec_only_hidden_switches &&
+ git rev-parse --parseopt -- --help-all > output < optionspec_only_hidden_switches &&
test_cmp expect output
'
@@ -258,7 +258,7 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
|exit 0
END_EXPECT
- test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
+ git rev-parse --parseopt -- -h <spec >actual &&
test_cmp expect actual
'
@@ -296,12 +296,12 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
|exit 0
END_EXPECT
- test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
+ git rev-parse --parseopt -- -h <spec >actual &&
test_cmp expect actual
'
test_expect_success 'test --parseopt help output for optionspec-neg' '
- test_expect_code 129 git rev-parse --parseopt -- \
+ git rev-parse --parseopt -- \
-h >output <"$TEST_DIRECTORY/t1502/optionspec-neg" &&
test_cmp "$TEST_DIRECTORY/t1502/optionspec-neg.help" output
'
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index 03fa2f9cdf..d1e915786e 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -129,18 +129,25 @@ do
archimport | citool | credential-netrc | credential-libsecret | \
credential-osxkeychain | cvsexportcommit | cvsimport | cvsserver | \
daemon | \
- difftool--helper | filter-branch | format-rev | fsck-objects | \
- get-tar-commit-id | \
+ difftool--helper | format-rev | fsck-objects | get-tar-commit-id | \
gui | gui--askpass | \
- http-backend | http-fetch | http-push | init-db | instaweb | \
- merge-octopus | merge-one-file | merge-resolve | mergetool | \
- mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \
- remote-http | remote-https | replay | request-pull | send-email | \
- sh-i18n--envsubst | shell | show | stage | submodule | svn | \
- upload-archive--writer | upload-pack | web--browse | whatchanged)
- expect_outcome=expect_failure ;;
+ http-backend | http-fetch | http-push | init-db | \
+ mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \
+ remote-http | remote-https | replay | send-email | \
+ sh-i18n--envsubst | shell | show | stage | \
+ upload-archive--writer | upload-pack | whatchanged)
+ h_expect_outcome=expect_failure
+ all_expect_outcome=expect_failure
+ ;;
+ filter-branch | merge-octopus | merge-one-file | merge-resolve | \
+ mergetool | submodule | svn | web--browse)
+ h_expect_outcome=expect_success
+ all_expect_outcome=expect_failure
+ ;;
*)
- expect_outcome=expect_success ;;
+ h_expect_outcome=expect_success
+ all_expect_outcome=expect_success
+ ;;
esac
case "$cmd" in
instaweb)
@@ -150,20 +157,20 @@ do
*)
prereq= ;;
esac
- test_$expect_outcome $prereq "'git $cmd -h' outside a repository" '
- test_expect_code 129 nongit git $cmd -h >usage &&
+ test_$h_expect_outcome $prereq "'git $cmd -h' outside a repository" '
+ nongit git $cmd -h >usage &&
test_grep "[Uu]sage: git $cmd " usage
'
- test_$expect_outcome $prereq "'git $cmd --help-all' outside a repository" '
- test_expect_code 129 nongit git $cmd --help-all >usage &&
+ test_$all_expect_outcome $prereq "'git $cmd --help-all' outside a repository" '
+ nongit git $cmd --help-all >usage &&
test_grep "[Uu]sage: git $cmd " usage
'
done
test_expect_success 'fmt-merge-msg does not crash with -h' '
- test_expect_code 129 git fmt-merge-msg -h >usage &&
+ git fmt-merge-msg -h >usage &&
test_grep "[Uu]sage: git fmt-merge-msg " usage &&
- test_expect_code 129 nongit git fmt-merge-msg -h >usage &&
+ nongit git fmt-merge-msg -h >usage &&
test_grep "[Uu]sage: git fmt-merge-msg " usage
'
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 0132e772e4..2ea9fa13c5 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -75,10 +75,10 @@ sentinel_detector () {
test_expect_success 'git hook usage' '
test_expect_code 129 git hook &&
test_expect_code 129 git hook run &&
- test_expect_code 129 git hook run -h &&
+ git hook run -h &&
test_expect_code 129 git hook run --unknown 2>err &&
test_expect_code 129 git hook list &&
- test_expect_code 129 git hook list -h &&
+ git hook list -h &&
grep "unknown option" err
'
diff --git a/t/t1900-repo-info.sh b/t/t1900-repo-info.sh
index 39bb77dda0..826686955d 100755
--- a/t/t1900-repo-info.sh
+++ b/t/t1900-repo-info.sh
@@ -150,7 +150,7 @@ test_expect_success 'git repo info --keys uses lines as its default output forma
'
test_expect_success 'git repo info -h shows only repo info usage' '
- test_must_fail git repo info -h >actual &&
+ git repo info -h >actual &&
test_grep "git repo info" actual &&
test_grep ! "git repo structure" actual
'
diff --git a/t/t1901-repo-structure.sh b/t/t1901-repo-structure.sh
index 10050abd70..02cc2b594a 100755
--- a/t/t1901-repo-structure.sh
+++ b/t/t1901-repo-structure.sh
@@ -225,7 +225,7 @@ test_expect_success 'progress meter option' '
'
test_expect_success 'git repo structure -h shows only repo structure usage' '
- test_must_fail git repo structure -h >actual &&
+ git repo structure -h >actual &&
test_grep "git repo structure" actual &&
test_grep ! "git repo info" actual
'
diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh
index fedd2cc097..6538a24c95 100755
--- a/t/t2006-checkout-index-basic.sh
+++ b/t/t2006-checkout-index-basic.sh
@@ -16,15 +16,15 @@ test_expect_success 'checkout-index -h in broken repository' '
cd broken &&
git init &&
>.git/index &&
- test_expect_code 129 git checkout-index -h >usage 2>&1
+ git checkout-index -h >usage 2>&1
) &&
test_grep "[Uu]sage" broken/usage
'
test_expect_success 'checkout-index does not crash with -h' '
- test_expect_code 129 git checkout-index -h >usage &&
+ git checkout-index -h >usage &&
test_grep "[Uu]sage: git checkout-index " usage &&
- test_expect_code 129 nongit git checkout-index -h >usage &&
+ nongit git checkout-index -h >usage &&
test_grep "[Uu]sage: git checkout-index " usage
'
diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
index 3bffe5da8a..004878322e 100755
--- a/t/t2107-update-index-basic.sh
+++ b/t/t2107-update-index-basic.sh
@@ -23,7 +23,7 @@ test_expect_success 'update-index -h with corrupt index' '
cd broken &&
git init &&
>.git/index &&
- test_expect_code 129 git update-index -h >usage 2>&1
+ git update-index -h >usage 2>&1
) &&
test_grep "[Uu]sage: git update-index" broken/usage
'
diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
index 4034a5a59f..c57afcb841 100755
--- a/t/t3004-ls-files-basic.sh
+++ b/t/t3004-ls-files-basic.sh
@@ -29,15 +29,15 @@ test_expect_success 'ls-files -h in corrupt repository' '
cd broken &&
git init &&
>.git/index &&
- test_expect_code 129 git ls-files -h >usage 2>&1
+ git ls-files -h >usage 2>&1
) &&
test_grep "[Uu]sage: git ls-files " broken/usage
'
test_expect_success 'ls-files does not crash with -h' '
- test_expect_code 129 git ls-files -h >usage &&
+ git ls-files -h >usage &&
test_grep "[Uu]sage: git ls-files " usage &&
- test_expect_code 129 nongit git ls-files -h >usage &&
+ nongit git ls-files -h >usage &&
test_grep "[Uu]sage: git ls-files " usage
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e7829c2c4b..dec0e77e3c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -33,7 +33,7 @@ test_expect_success REFFILES 'branch -h in broken repository' '
cd broken &&
git init -b main &&
>.git/refs/heads/main &&
- test_expect_code 129 git branch -h >usage 2>&1
+ git branch -h >usage 2>&1
) &&
test_grep "[Uu]sage" broken/usage
'
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ecc35aae82..bc07e2a6ec 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -27,13 +27,13 @@ test_expect_success 'usage on cmd and subcommand invalid option' '
'
test_expect_success 'usage on main command -h emits a summary of subcommands' '
- test_expect_code 129 git stash -h >usage &&
+ git stash -h >usage &&
grep -F "usage: git stash list" usage &&
grep -F "or: git stash show" usage
'
test_expect_success 'usage for subcommands should emit subcommand usage' '
- test_expect_code 129 git stash push -h >usage &&
+ git stash push -h >usage &&
grep -F "usage: git stash [push" usage
'
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 1717f407c8..e1b474cc0f 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -438,7 +438,7 @@ test_expect_success 'rerere --no-no-rerere-autoupdate' '
'
test_expect_success 'rerere -h' '
- test_must_fail git rerere -h >help &&
+ git rerere -h >help &&
test_grep [Uu]sage help
'
diff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh
index a551e955b5..a0630cc1fc 100755
--- a/t/t5200-update-server-info.sh
+++ b/t/t5200-update-server-info.sh
@@ -47,7 +47,7 @@ test_expect_success 'midx does not create duplicate pack entries' '
'
test_expect_success 'update-server-info does not crash with -h' '
- test_expect_code 129 git update-server-info -h >usage &&
+ git update-server-info -h >usage &&
test_grep "[Uu]sage: git update-server-info " usage
'
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 2be7cd30de..e26e833d89 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -365,7 +365,7 @@ test_expect_success 'gc.recentObjectsHook' '
'
test_expect_success 'prune does not crash with -h' '
- test_expect_code 129 git prune -h >usage &&
+ git prune -h >usage &&
test_grep "[Uu]sage: git prune " usage
'
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index b32a0a6aa7..6aa5838e2b 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -56,9 +56,9 @@ test_expect_success setup '
git log'
test_expect_success 'send-pack does not crash with -h' '
- test_expect_code 129 git send-pack -h >usage &&
+ git send-pack -h >usage &&
test_grep "[Uu]sage: git send-pack " usage &&
- test_expect_code 129 nongit git send-pack -h >usage &&
+ nongit git send-pack -h >usage &&
test_grep "[Uu]sage: git send-pack " usage
'
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 5930f55186..8345bc0b14 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -86,7 +86,7 @@ test_expect_success 'ls-remote -h is deprecated w/o warning' '
'
test_expect_success 'ls-remote --heads is deprecated and hidden w/o warning' '
- test_expect_code 129 git ls-remote -h >short-help &&
+ git ls-remote -h >short-help &&
test_grep ! -e --head short-help &&
git ls-remote --heads self >actual 2>warning &&
test_cmp expected.branches actual &&
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 1d9809114d..6d27b42ff1 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -8,9 +8,9 @@ test_description='for-each-ref test'
. ./test-lib.sh
test_expect_success "for-each-ref does not crash with -h" '
- test_expect_code 129 git for-each-ref -h >usage &&
+ git for-each-ref -h >usage &&
test_grep "[Uu]sage: git for-each-ref " usage &&
- test_expect_code 129 nongit git for-each-ref -h >usage &&
+ nongit git for-each-ref -h >usage &&
test_grep "[Uu]sage: git for-each-ref " usage
'
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index ea9aaad470..b40d13d7ff 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -35,7 +35,7 @@ test_expect_success 'gc -h with invalid configuration' '
cd broken &&
git init &&
echo "[gc] pruneexpire = CORRUPT" >>.git/config &&
- test_expect_code 129 git gc -h >usage 2>&1
+ git gc -h >usage 2>&1
) &&
test_grep "[Uu]sage" broken/usage
'
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 2c147072c1..3bc5d1e9a2 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -8,9 +8,9 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. "$TEST_DIRECTORY/lib-gpg.sh"
test_expect_success GPG 'verify-tag does not crash with -h' '
- test_expect_code 129 git verify-tag -h >usage &&
+ git verify-tag -h >usage &&
test_grep "[Uu]sage: git verify-tag " usage &&
- test_expect_code 129 nongit git verify-tag -h >usage &&
+ nongit git verify-tag -h >usage &&
test_grep "[Uu]sage: git verify-tag " usage
'
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c2057bc94c..de7d7beec3 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -16,7 +16,7 @@ test_expect_success 'status -h in broken repository' '
cd broken &&
git init &&
echo "[status] showuntrackedfiles = CORRUPT" >>.git/config &&
- test_expect_code 129 git status -h >usage 2>&1
+ git status -h >usage 2>&1
) &&
test_grep "[Uu]sage" broken/usage
'
@@ -28,7 +28,7 @@ test_expect_success 'commit -h in broken repository' '
cd broken &&
git init &&
echo "[status] showuntrackedfiles = CORRUPT" >>.git/config &&
- test_expect_code 129 git commit -h >usage 2>&1
+ git commit -h >usage 2>&1
) &&
test_grep "[Uu]sage" broken/usage
'
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index aa9108da54..e641f9e334 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -9,9 +9,9 @@ GNUPGHOME_NOT_USED=$GNUPGHOME
. "$TEST_DIRECTORY/lib-gpg.sh"
test_expect_success GPG 'verify-commit does not crash with -h' '
- test_expect_code 129 git verify-commit -h >usage &&
+ git verify-commit -h >usage &&
test_grep "[Uu]sage: git verify-commit " usage &&
- test_expect_code 129 nongit git verify-commit -h >usage &&
+ nongit git verify-commit -h >usage &&
test_grep "[Uu]sage: git verify-commit " usage
'
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index f877d9a433..fd3d1d67f9 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -173,7 +173,7 @@ test_expect_success 'merge -h with invalid index' '
cd broken &&
git init &&
>.git/index &&
- test_expect_code 129 git merge -h >usage
+ git merge -h >usage
) &&
test_grep "[Uu]sage: git merge" broken/usage
'
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 8a91ff3603..961de3efab 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -27,12 +27,11 @@ prompt_given ()
}
test_expect_success 'basic usage requires no repo' '
- test_expect_code 129 git difftool -h >output &&
+ git difftool -h >output &&
test_grep ^usage: output &&
# create a ceiling directory to prevent Git from finding a repo
mkdir -p not/repo &&
test_when_finished rm -r not &&
- test_expect_code 129 \
env GIT_CEILING_DIRECTORIES="$(pwd)/not" \
git -C not/repo difftool -h >output &&
test_grep ^usage: output
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index d7f82e1bec..9886f641fc 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -35,7 +35,7 @@ test_systemd_analyze_verify () {
}
test_expect_success 'help text' '
- test_expect_code 129 git maintenance -h >actual &&
+ git maintenance -h >actual &&
test_grep "usage: git maintenance <subcommand>" actual &&
test_expect_code 129 git maintenance barf 2>err &&
test_grep "unknown subcommand: \`barf'\''" err &&
diff --git a/usage.c b/usage.c
index 527edb1e79..3f0118ab2a 100644
--- a/usage.c
+++ b/usage.c
@@ -188,7 +188,7 @@ static void show_usage_if_asked_helper(const char *err, ...)
va_start(params, err);
vfreportf(stdout, _("usage: "), err, params);
va_end(params);
- exit(129);
+ exit(0);
}
void show_usage_if_asked(int ac, const char **av, const char *err)
^ permalink raw reply related
* [PATCH v3 0/4] parseopt: exit 0 on help
From: brian m. carlson @ 2026-07-08 0:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260701212442.1430084-1-sandals@crustytoothpaste.net>
The standard philosophy for Unix software when a help option (such as
--help) is specified is that the software should exit 0, printing the
help output to standard output, since the standard output is for
user-requested output and the program performed the requested task
successfully. If the user specifies an incorrect option, then the help
output should be printed to standard error (since the user has made a
mistake) and it should exit unsuccessfully.
git rev-parse --parseopt properly directs the output in both of these
cases, but it currently exits 129 when it receives a --help or -h option
on the command line, which causes its invoking script to do the same.
This is not in line with the usual behavior and it causes scripts using
this command to exit unsuccessfully on --help as well.
This series introduces some changes to distinguish the --help and -h
options from other cases in which we print help output and adjusts the
exit code to 0 from those two options. We continue to exit 129 when the
options are invalid, which is useful information to have for callers.
We also make the relevant changes such that `git rev-parse --parseopt`
does the same thing as long as it is invoked in the way specified in the
manual page (which a quick GitHub search shows almost everyone does).
Changes since v2:
* Fix inverted condition in t1517.
* Stop checking for old versions of SVN Perl libraries since they are
so old nobody is using them.
* Adjust the various cases where we choose between the error and
non-error help output.
brian m. carlson (4):
t1517: skip svn tests if svn is not installed
parse-options: add a separate case for help output on error
rev-parse: have --parseopt callers exit 0 on --help
parse-options: exit 0 on -h
builtin/blame.c | 2 ++
builtin/shortlog.c | 2 ++
builtin/update-index.c | 2 ++
contrib/subtree/t/t7900-subtree.sh | 2 +-
parse-options.c | 22 +++++++++++-----
parse-options.h | 3 ++-
t/for-each-ref-tests.sh | 2 +-
t/t0012-help.sh | 2 +-
t/t0040-parse-options.sh | 2 +-
t/t0450-txt-doc-vs-help.sh | 2 +-
t/t0610-reftable-basics.sh | 4 +--
t/t1403-show-ref.sh | 2 +-
t/t1410-reflog.sh | 4 +--
t/t1418-reflog-exists.sh | 2 +-
t/t1502-rev-parse-parseopt.sh | 23 +++++++++-------
t/t1502/optionspec-neg.help | 1 +
t/t1502/optionspec.help | 1 +
t/t1517-outside-repo.sh | 42 +++++++++++++++++++++---------
t/t1800-hook.sh | 4 +--
t/t1900-repo-info.sh | 2 +-
t/t1901-repo-structure.sh | 2 +-
t/t2006-checkout-index-basic.sh | 6 ++---
t/t2107-update-index-basic.sh | 2 +-
t/t3004-ls-files-basic.sh | 6 ++---
t/t3200-branch.sh | 2 +-
t/t3903-stash.sh | 4 +--
t/t4200-rerere.sh | 2 +-
t/t5200-update-server-info.sh | 2 +-
t/t5304-prune.sh | 2 +-
t/t5400-send-pack.sh | 4 +--
t/t5512-ls-remote.sh | 2 +-
t/t6300-for-each-ref.sh | 4 +--
t/t6500-gc.sh | 2 +-
t/t7030-verify-tag.sh | 4 +--
t/t7508-status.sh | 4 +--
t/t7510-signed-commit.sh | 4 +--
t/t7600-merge.sh | 2 +-
t/t7800-difftool.sh | 3 +--
t/t7900-maintenance.sh | 2 +-
usage.c | 2 +-
40 files changed, 113 insertions(+), 74 deletions(-)
Range-diff against v2:
1: 558a53cc20 ! 1: c8c7eac5f7 t1517: skip svn tests if svn is not installed
@@ t/t1517-outside-repo.sh: test_description='check random commands outside repo'
. ./test-lib.sh
+test_lazy_prereq SVN '
-+ test_have_prereq PERL && test -n "$NO_SVN_TESTS" && perl -w -e "
++ test_have_prereq PERL && test -z "$NO_SVN_TESTS" && perl -w -e "
+ use SVN::Core;
+ use SVN::Repos;
-+ \$SVN::Core::VERSION gt '1.1.0' or exit(42);
+ "
+'
+
2: 2b5ce2fb4c ! 2: daa7aa2534 parse-options: add a separate case for help output on error
@@ parse-options.c: int parse_options(int argc, const char **argv,
case PARSE_OPT_ERROR:
exit(129);
case PARSE_OPT_COMPLETE:
+@@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
+ parse_options_check_harder(opts);
+
+ if (!usagestr)
+- return PARSE_OPT_HELP;
++ return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
+
+ if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
+ fprintf(outfile, "cat <<\\EOF\n");
+@@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
+ if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
+ fputs("EOF\n", outfile);
+
+- return PARSE_OPT_HELP;
++ return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
+ }
+
+ void NORETURN usage_with_options(const char * const *usagestr,
## parse-options.h ##
@@ parse-options.h: enum parse_opt_option_flags {
3: e5d0167544 ! 3: af69daffc3 rev-parse: have --parseopt callers exit 0 on --help
@@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct
- fputs("EOF\n", outfile);
+ fputs("EOF\nexit 0\n", outfile);
- return PARSE_OPT_HELP;
+ return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
}
## t/t1502-rev-parse-parseopt.sh ##
4: 98481005ff ! 4: f68c53015c parse-options: exit 0 on -h
@@ parse-options.c: int parse_options(int argc, const char **argv,
case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_ERROR:
exit(129);
-@@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
- if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
- fputs("EOF\nexit 0\n", outfile);
-
-- return PARSE_OPT_HELP;
-+ return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
- }
-
- void NORETURN usage_with_options(const char * const *usagestr,
@@ parse-options.c: void show_usage_with_options_if_asked(int ac, const char **av,
if (!strcmp(av[1], "-h")) {
usage_with_options_internal(NULL, usagestr, opts,
^ permalink raw reply
* [PATCH v3 2/4] parse-options: add a separate case for help output on error
From: brian m. carlson @ 2026-07-08 0:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260708001557.3581080-1-sandals@crustytoothpaste.net>
When we parse a command line option such as -h or --help, we currently
exit 129, since that is the exit code when help output is printed. In a
future commit, we'll change this to exit 0 instead, since we're doing
what the user wanted successfully.
However, there are some cases where we print help output because the
user has provided ambiguous or invalid input, such as an ambiguous
option, and we'll want to exit unsuccessfully there. Make this easier
by defining a new return code, PARSE_OPT_HELP_ERROR, that can be used in
this case, while reserving PARSE_OPT_HELP for those cases where the user
has requested help directly.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
builtin/blame.c | 1 +
builtin/shortlog.c | 1 +
builtin/update-index.c | 1 +
parse-options.c | 11 ++++++++---
parse-options.h | 3 ++-
5 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index ffbd3ce5c5..65d43c7d48 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1013,6 +1013,7 @@ int cmd_blame(int argc,
case PARSE_OPT_UNKNOWN:
break;
case PARSE_OPT_HELP:
+ case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_ERROR:
case PARSE_OPT_SUBCOMMAND:
exit(129);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 6b2a0b93b5..cd262bd376 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -433,6 +433,7 @@ int cmd_shortlog(int argc,
case PARSE_OPT_UNKNOWN:
break;
case PARSE_OPT_HELP:
+ case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_ERROR:
case PARSE_OPT_SUBCOMMAND:
exit(129);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 3d6646c318..ac4610ec94 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1133,6 +1133,7 @@ int cmd_update_index(int argc,
break;
switch (parseopt_state) {
case PARSE_OPT_HELP:
+ case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_ERROR:
exit(129);
case PARSE_OPT_COMPLETE:
diff --git a/parse-options.c b/parse-options.c
index f4647e0099..fd8ceed82b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -583,7 +583,7 @@ static enum parse_opt_result parse_long_opt(
ambiguous.option->long_name,
(abbrev.flags & OPT_UNSET) ? "no-" : "",
abbrev.option->long_name);
- return PARSE_OPT_HELP;
+ return PARSE_OPT_HELP_ERROR;
}
if (abbrev.option) {
if (*arg_end)
@@ -1037,6 +1037,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
usage_with_options(usagestr, options);
case PARSE_OPT_COMPLETE:
case PARSE_OPT_HELP:
+ case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_ERROR:
case PARSE_OPT_DONE:
case PARSE_OPT_NON_OPTION:
@@ -1072,6 +1073,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
case PARSE_OPT_NON_OPTION:
case PARSE_OPT_SUBCOMMAND:
case PARSE_OPT_HELP:
+ case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_COMPLETE:
BUG("parse_short_opt() cannot return these");
case PARSE_OPT_DONE:
@@ -1099,6 +1101,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
case PARSE_OPT_SUBCOMMAND:
case PARSE_OPT_COMPLETE:
case PARSE_OPT_HELP:
+ case PARSE_OPT_HELP_ERROR:
BUG("parse_short_opt() cannot return these");
case PARSE_OPT_DONE:
break;
@@ -1132,6 +1135,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
case PARSE_OPT_UNKNOWN:
goto unknown;
case PARSE_OPT_HELP:
+ case PARSE_OPT_HELP_ERROR:
goto show_usage;
case PARSE_OPT_NON_OPTION:
case PARSE_OPT_SUBCOMMAND:
@@ -1197,6 +1201,7 @@ int parse_options(int argc, const char **argv,
parse_options_start_1(&ctx, argc, argv, prefix, options, flags);
switch (parse_options_step(&ctx, options, usagestr)) {
case PARSE_OPT_HELP:
+ case PARSE_OPT_HELP_ERROR:
case PARSE_OPT_ERROR:
exit(129);
case PARSE_OPT_COMPLETE:
@@ -1363,7 +1368,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
parse_options_check_harder(opts);
if (!usagestr)
- return PARSE_OPT_HELP;
+ return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
fprintf(outfile, "cat <<\\EOF\n");
@@ -1476,7 +1481,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
fputs("EOF\n", outfile);
- return PARSE_OPT_HELP;
+ return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
}
void NORETURN usage_with_options(const char * const *usagestr,
diff --git a/parse-options.h b/parse-options.h
index 0d1f738f8d..3ec8ba5cc8 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -57,7 +57,8 @@ enum parse_opt_option_flags {
};
enum parse_opt_result {
- PARSE_OPT_COMPLETE = -3,
+ PARSE_OPT_COMPLETE = -4,
+ PARSE_OPT_HELP_ERROR = -3,
PARSE_OPT_HELP = -2,
PARSE_OPT_ERROR = -1, /* must be the same as error() */
PARSE_OPT_DONE = 0, /* fixed so that "return 0" works */
^ permalink raw reply related
* [PATCH v3 3/4] rev-parse: have --parseopt callers exit 0 on --help
From: brian m. carlson @ 2026-07-08 0:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260708001557.3581080-1-sandals@crustytoothpaste.net>
The standard philosophy for Unix software when a help option (such as
--help) is specified is that the software should exit 0, printing the
help output to standard output, since the standard output is for
user-requested output and the program performed the requested task
successfully. If the user specifies an incorrect option, then the help
output should be printed to standard error (since the user has made a
mistake) and it should exit unsuccessfully.
git rev-parse --parseopt properly directs the output in both of these
cases, but it currently exits 129 when it receives a --help or -h option
on the command line, which causes its invoking script to do the same.
This is not in line with the usual behavior and it causes scripts using
this command to exit unsuccessfully on --help as well.
Note that Git subcommands implemented using scripts, such as git
submodule, don't have this problem because Git itself intercepts the
--help option and runs man (or a similar tool), which then exits 0.
However, this still affects the myriad scripts that use this
functionality because Git is widespread and the --parseopt functionality
is a good way to get sensible option parsing across shells in a portable
way.
Because git rev-parse --parseopt is intended to be eval'd by the shell,
when help output is to be printed to standard output, Git actually
prints a cat command with a heredoc since the standard output is being
evaluated by the shell. Thus, to do the right thing, simply add an
"exit 0" right after the end of the heredoc, which will cause the
invoking program to exit successfully.
The usual invocation recommended by the manual page is this:
eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
Thus, the fact that git rev-parse --parseopt still exits 129 in this
case is irrelevant, since the "echo exit $?" will print "exit 129", but
that will be after the "exit 0" printed by Git—and thus ignored, since
the shell will have already exited successfully.
Update the tests for this case. Note that we no longer need to delete
only the first and last lines in some tests, so add a command to delete
the end of the heredoc as well. We could do something clever with sed
to delete all but the last two lines or switch to head and tail, but
those would be more complicated and less readable, so just stick with
the simple approach.
In t1517, add three shell scripts to the failure case because they no
longer return 129 as expected. In a future commit, we'll change the
expected result to exit 0 and these will become successful again.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
parse-options.c | 2 +-
t/t1502-rev-parse-parseopt.sh | 9 +++++++--
t/t1502/optionspec-neg.help | 1 +
t/t1502/optionspec.help | 1 +
t/t1517-outside-repo.sh | 6 +++---
5 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index fd8ceed82b..cc3a8b0fe3 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1479,7 +1479,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
fputc('\n', outfile);
if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
- fputs("EOF\n", outfile);
+ fputs("EOF\nexit 0\n", outfile);
return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
}
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 3962f1d288..455608c429 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -12,7 +12,7 @@ check_invalid_long_option () {
cat <<-\EOF &&
error: unknown option `'${opt#--}\''
EOF
- sed -e 1d -e \$d <"$TEST_DIRECTORY/t1502/$spec.help"
+ sed -e 1d -e /EOF/d -e \$d <"$TEST_DIRECTORY/t1502/$spec.help"
} >expect &&
test_expect_code 129 git rev-parse --parseopt -- $opt \
2>output <"$TEST_DIRECTORY/t1502/$spec" &&
@@ -87,6 +87,7 @@ test_expect_success 'test --parseopt help output no switches' '
| some-command does foo and bar!
|
|EOF
+|exit 0
END_EXPECT
test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&
test_cmp expect output
@@ -100,6 +101,7 @@ test_expect_success 'test --parseopt help output hidden switches' '
| some-command does foo and bar!
|
|EOF
+|exit 0
END_EXPECT
test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&
test_cmp expect output
@@ -115,6 +117,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
| --[no-]hidden1 A hidden switch
|
|EOF
+|exit 0
END_EXPECT
test_expect_code 129 git rev-parse --parseopt -- --help-all > output < optionspec_only_hidden_switches &&
test_cmp expect output
@@ -125,7 +128,7 @@ test_expect_success 'test --parseopt invalid switch help output' '
cat <<-\EOF &&
error: unknown option `does-not-exist'\''
EOF
- sed -e 1d -e \$d <"$TEST_DIRECTORY/t1502/optionspec.help"
+ sed -e 1d -e /EOF/d -e \$d <"$TEST_DIRECTORY/t1502/optionspec.help"
} >expect &&
test_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&
test_cmp expect output
@@ -252,6 +255,7 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
| -h, --help show the help
|
|EOF
+ |exit 0
END_EXPECT
test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
@@ -289,6 +293,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
| -h, --help show the help
|
|EOF
+ |exit 0
END_EXPECT
test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
diff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help
index 7a29f8cb03..f85be7b8fd 100644
--- a/t/t1502/optionspec-neg.help
+++ b/t/t1502/optionspec-neg.help
@@ -10,3 +10,4 @@ usage: some-command [options] <args>...
--no-negative cannot be positivated
EOF
+exit 0
diff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help
index cbdd54d41b..ded35ebc82 100755
--- a/t/t1502/optionspec.help
+++ b/t/t1502/optionspec.help
@@ -34,3 +34,4 @@ Extras
--[no-]extra1 line above used to cause a segfault but no longer does
EOF
+exit 0
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index 6421bdb3c3..03fa2f9cdf 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -132,10 +132,10 @@ do
difftool--helper | filter-branch | format-rev | fsck-objects | \
get-tar-commit-id | \
gui | gui--askpass | \
- http-backend | http-fetch | http-push | init-db | \
+ http-backend | http-fetch | http-push | init-db | instaweb | \
merge-octopus | merge-one-file | merge-resolve | mergetool | \
- mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \
- remote-http | remote-https | replay | send-email | \
+ mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \
+ remote-http | remote-https | replay | request-pull | send-email | \
sh-i18n--envsubst | shell | show | stage | submodule | svn | \
upload-archive--writer | upload-pack | web--browse | whatchanged)
expect_outcome=expect_failure ;;
^ permalink raw reply related
* [PATCH v3 1/4] t1517: skip svn tests if svn is not installed
From: brian m. carlson @ 2026-07-08 0:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20260708001557.3581080-1-sandals@crustytoothpaste.net>
The svn tests currently assume that git-svn's option parsing will always
fail the tests because it exits 0 on --help, not 129. However, in a
future commit, we'll expect it to exit 0 and the tests will then need to
be updated to succeed in some cases and fail in others.
We therefore need to have t1517 determine whether the Subversion Perl
modules are present, since if they are not, git-svn will die on start
and then it needs to continue to expect failure. Add a stripped down
version of the tests in t/lib-git-svn.sh as a prerequisite we can use
here for our svn tests.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
t/t1517-outside-repo.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index c557f2f55c..6421bdb3c3 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -4,6 +4,13 @@ test_description='check random commands outside repo'
. ./test-lib.sh
+test_lazy_prereq SVN '
+ test_have_prereq PERL && test -z "$NO_SVN_TESTS" && perl -w -e "
+ use SVN::Core;
+ use SVN::Repos;
+ "
+'
+
test_expect_success 'set up a non-repo directory and test file' '
GIT_CEILING_DIRECTORIES=$(pwd) &&
export GIT_CEILING_DIRECTORIES &&
@@ -138,6 +145,8 @@ do
case "$cmd" in
instaweb)
prereq=PERL ;;
+ svn)
+ prereq=SVN ;;
*)
prereq= ;;
esac
^ permalink raw reply related
* Re: Incremental 'git fetch' downloaded everything again
From: Junio C Hamano @ 2026-07-07 22:51 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git
In-Reply-To: <ak1+dsNQIV8EeSIc@szeder.dev>
SZEDER Gábor <szeder.dev@gmail.com> writes:
> I usually try to fetch from https://github.com/git/git daily, and
> today morning something unusual happened:
>
> $ git fetch
> remote: Enumerating objects: 406099, done.
> remote: Counting objects: 100% (1229/1229), done.
> remote: Compressing objects: 100% (1044/1044), done.
> remote: Total 406093 (delta 207), reused 1189 (delta 185), pack-reused 404864 (from 2)
> Receiving objects: 100% (406093/406093), 292.22 MiB | 4.62 MiB/s, done.
> Resolving deltas: 100% (308943/308943), done.
> From https://github.com/git/git
> + ffe2b816f5...106a830b98 jch -> origin/jch (forced update)
> e9019fcafe..f85a7e6620 master -> origin/master
> + c42f45431d...00534a21ce next -> origin/next (forced update)
> + f6884212b2...73452939f9 seen -> origin/seen (forced update)
The only difference I can think of is that 'next' has been rewound
recently, but because 'seen' and 'jch' are constantly reound, it
would be very strange that it made such a big difference.
So, sorry, no idea. If I were bug-hunting this, I would first try
to eliminate whatever GitHub runs on their server end from the
picture.
> Note that it downloaded over 400k objects in an almost 300MB packfile.
>
> Looking at the objects that I already had and the objects in the newly
> downloaded packfile:
>
> $ git rev-list --objects origin/master@{1} origin/next@{1} origin/jch@{1} origin/seen@{1} | cut -d' ' -f1 | sort >existing-objects
> $ git verify-pack -v .git/objects/pack/pack-080fedc9c19f711dd1b22103b382ede8925b90a6.idx | sed -n -E -e 's/^([0-9a-f]{40}) .*/\1/p' | sort >received-objects
> $ wc -l existing-objects received-objects
> 406954 existing-objects
> 406093 received-objects
> 813047 total
> $ git diff --no-index --stat existing-objects received-objects
> existing-objects => received-objects | 3615 +++++++++++++---------------------
> 1 file changed, 1377 insertions(+), 2238 deletions(-)
>
> The vast majority of objects were already available locally.
>
> What's going on?!
>
> This might be a recurring issue: I remember a similar large download
> from 2 or 3 weeks ago, but back then I didn't have time to investigate
> or to report.
> I tried to reproduce this issue by attempting to recreate the state of
> my git repository from yesterday in a new repo, but no luck, 'git
> fetch' only downloads what's necessary.
>
> I use a Git version based on next, with a bunch of my own patches on
> top, but none of them has anything to do with object transfer and I've
> been using most of them for years. I don't have any config set under
> 'fetch.*' or 'transfer.*'.
^ permalink raw reply
* Re: [PATCH] http: preserve wwwauth_headers across redirects
From: Junio C Hamano @ 2026-07-07 22:35 UTC (permalink / raw)
To: Aaron Plattner; +Cc: git, Rahul Rameshbabu
In-Reply-To: <68c2b88f-8976-474b-8965-97733eba5a99@nvidia.com>
Aaron Plattner <aplattner@nvidia.com> writes:
>> Did anything come of that discussion? No rush, since this change
>> fixes an immediate issue and the helper suggestion is for long-term
>> future-proofing. We can treat them as separate steps.
>>
>> Thanks.
>
> No, I got sidetracked with other work and didn't get a chance to get
> back to this, sorry. It's not directly impacting my users since I can
> just tell them they have to use my server's FQDN, so fine with me to
> treat this as a low-priority issue.
Understood.
I hate to leave a topic backburnered for too long. As this topic
unfortunately has not seen enough attention by reviewers, between
two easy approach available to me to deal with such a topic, namely,
merging it to 'next' and discarding it (with invitation to resubmit
once the author can spend enough time on the topic again), I'd
probably choose the latter.
Unless somebody else steps up and promises to usher the topic
forward in its current shape, that is.
Thanks.
^ permalink raw reply
* Incremental 'git fetch' downloaded everything again
From: SZEDER Gábor @ 2026-07-07 22:32 UTC (permalink / raw)
To: git
I usually try to fetch from https://github.com/git/git daily, and
today morning something unusual happened:
$ git fetch
remote: Enumerating objects: 406099, done.
remote: Counting objects: 100% (1229/1229), done.
remote: Compressing objects: 100% (1044/1044), done.
remote: Total 406093 (delta 207), reused 1189 (delta 185), pack-reused 404864 (from 2)
Receiving objects: 100% (406093/406093), 292.22 MiB | 4.62 MiB/s, done.
Resolving deltas: 100% (308943/308943), done.
From https://github.com/git/git
+ ffe2b816f5...106a830b98 jch -> origin/jch (forced update)
e9019fcafe..f85a7e6620 master -> origin/master
+ c42f45431d...00534a21ce next -> origin/next (forced update)
+ f6884212b2...73452939f9 seen -> origin/seen (forced update)
Note that it downloaded over 400k objects in an almost 300MB packfile.
Looking at the objects that I already had and the objects in the newly
downloaded packfile:
$ git rev-list --objects origin/master@{1} origin/next@{1} origin/jch@{1} origin/seen@{1} | cut -d' ' -f1 | sort >existing-objects
$ git verify-pack -v .git/objects/pack/pack-080fedc9c19f711dd1b22103b382ede8925b90a6.idx | sed -n -E -e 's/^([0-9a-f]{40}) .*/\1/p' | sort >received-objects
$ wc -l existing-objects received-objects
406954 existing-objects
406093 received-objects
813047 total
$ git diff --no-index --stat existing-objects received-objects
existing-objects => received-objects | 3615 +++++++++++++---------------------
1 file changed, 1377 insertions(+), 2238 deletions(-)
The vast majority of objects were already available locally.
What's going on?!
This might be a recurring issue: I remember a similar large download
from 2 or 3 weeks ago, but back then I didn't have time to investigate
or to report.
I tried to reproduce this issue by attempting to recreate the state of
my git repository from yesterday in a new repo, but no luck, 'git
fetch' only downloads what's necessary.
I use a Git version based on next, with a bunch of my own patches on
top, but none of them has anything to do with object transfer and I've
been using most of them for years. I don't have any config set under
'fetch.*' or 'transfer.*'.
^ permalink raw reply
* Re: [PATCH 4/7] hash: make git_hash_discard() idempotent
From: Junio C Hamano @ 2026-07-07 22:25 UTC (permalink / raw)
To: brian m. carlson; +Cc: Jeff King, git, Patrick Steinhardt
In-Reply-To: <ak1yazHtP_OazDaO@fruit.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> Our Rust code makes calling final a second time impossible because
> finalization takes `self`, not `&mut self`, so the object is _moved_
> into the final method and you no longer have access to it after that.
That is a cute trick available to Rust but not many other languages,
I guess ;-).
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox