[pull] master from git:master#204
Merged
Merged
Conversation
… on abnormal exit" This reverts commit dd3693e. The goal of that commit was to avoid zombie child processes hanging around when the parent git process is killed. But it doesn't quite work when the child command is run by the shell: 1. If there is a shell, then we kill and wait for the shell, not the process spawned by the shell. And so the child process, even if it eventually exits, will hang around as a zombie forever. And this is true of most (all?) shells: bash, dash, etc. So we are not really accomplishing our goal in the first place. 2. Not all shells will exit immediately upon receiving a signal. In particular, mksh will wait for its children to exit (but not actually propagate the signal to them!) leaving us with a potential deadlock: git is wait()ing on mksh, which is wait()ing on a child process, but that child process is waiting on git to produce more input (or EOF) over a pipe. You can see several examples of this deadlock in the test suite, for example by running: make SHELL_PATH=/bin/mksh cd t ./t5702-protocol-v2.sh Because this is a regression for mksh users, and because we did not achieve our goal even with other shells, let's revert the commit for now. If there is a more clever way of doing the same thing, we can consider applying it separately on top (or do nothing and just accept the zombies and rely on PID 1 to reap them). Reported-by: Jan Palus <jpalus@fastmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The SOCKS proxy test introduced in 0ca365c (http: do not ignore proxy path, 2024-08-02) creates a Unix domain socket in `$TRASH_DIRECTORY`. When the trash directory path is long (e.g. when running from a deeply nested worktree), the socket path can exceed the 108-character limit for `struct sockaddr_un.sun_path` on Linux, causing the test to fail with "Path length ... is longer than maximum supported length (108)". We cannot work around this using the chdir trick our own socket code employs, because both sides of the connection are outside our control: the socket is created by socks4-proxy.pl via Perl's IO::Socket::UNIX, and the client side is libcurl. Use `mktemp -d` to create a unique temporary directory with a short path, and place the socket inside it. This avoids collisions between concurrent test runs (e.g. `--stress`) and tmpdir-race vulnerabilities that a static `/tmp` path would be susceptible to. Helped-by: Jeff King <peff@peff.net> Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The v2 of `microsoft/setup-msbuild` runs on Node.js 20, which GitHub is phasing out of the Actions runners. v3 is a minimal release whose only substantive change is moving the action's runtime to Node.js 24, so that our Visual Studio build jobs keep working once Node.js 20 is removed from the runners. The risk of this bump is very low: v3 contains no functional changes to the action itself -- it merely adds `msbuild.exe` to `PATH`, with no change to command-line flags, inputs, outputs, or default tool resolution. The only precondition is a recent-enough Actions Runner, which the github.com-hosted runners already satisfy. See also: - Release notes: https://github.com/microsoft/setup-msbuild/releases - Compare: microsoft/setup-msbuild@v2...v3 Originally-authored-by: dependabot[bot] <support@github.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
`actions/upload-artifact` and `actions/download-artifact` are tightly
coupled: the upload action writes artifact archives in a format that
the download action then reads. Because of this coupling, the two
actions should always be bumped together so that the artifact format
contract between them is satisfied.
All of our `actions/upload-artifact` uses are still on v5, with one
stray v4 occurrence. Keeping them on these versions would leave the
artifact-upload steps running on Node.js 20, which GitHub is phasing
out, and would eventually cause all upload steps to fail.
Going from v5 directly to v7 folds in two release bumps:
- v6 switches the action's default runtime from Node.js 20 to
Node.js 24 (v5 had preliminary Node 24 support but still defaulted
to Node 20). This is the main motivation for bumping now: it gets
us off the deprecated runtime.
- v7 adds two opt-in features: direct (unzipped) single-file uploads
via a new `archive: false` parameter, and an internal conversion of
the action to ESM to match the updated `@actions/*` packages.
Risk analysis: we never pass `archive`, so the zip-as-usual behavior
is unchanged. We also do not `require('@actions/*')` from any calling
workflow, so the ESM migration cannot affect us. The upload steps we
care about -- tracked files/build artifacts and failing-test
directories -- keep the same inputs (`name`, `path`) and outputs, so
the diff is purely the `@vN` identifier. The main precondition is a
recent Actions Runner (>= 2.327.1), which the github.com-hosted
runners used by our CI already satisfy.
While at it, align the one remaining `@v4` occurrence with the rest
so that every `upload-artifact` step uses the same version.
See also:
- Release notes: https://github.com/actions/upload-artifact/releases
- Compare: actions/upload-artifact@v5...v7
We use `actions/download-artifact` to pass build artifacts between
the "windows-build" / "vs-build" / "windows-meson-build" jobs and
their corresponding test jobs. All callers are currently on v6;
bumping to v8 keeps this action in lockstep with the `upload-artifact`
bump above.
What v7 and v8 change:
- v7 switches the default runtime from Node.js 20 to Node.js 24 (v6
had preliminary Node 24 support but still defaulted to Node 20).
This is the main motivation: it gets us off the deprecated runtime.
- v8 makes three further changes:
* The package is converted to ESM (invisible to workflow authors).
* The action now checks the `Content-Type` header before
attempting to unzip a download, so that directly-uploaded
(unzipped) artifacts from `upload-artifact` v7 are downloaded
correctly.
* The `digest-mismatch` behaviour is changed from warn-and-
continue to a hard failure by default.
Risk analysis: defaulting hash-mismatch to a hard failure is
strictly safer than the previous warn-and-continue behaviour -- a
mismatch points to real corruption or tampering and should stop the
run. We download archives that the same workflow just uploaded, on
the same runner fleet, so false positives are not expected. Our
usage is limited to the `name` and `path` inputs, which are
unchanged between v6 and v8, so the diff is purely the `@vN`
identifier.
See also:
- Release notes: https://github.com/actions/download-artifact/releases
- Compare: actions/download-artifact@v6...v8
Originally-authored-by: dependabot[bot] <support@github.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The only use we have of `actions/github-script` is the "skip if the
commit or tree was already tested" step in `main.yml`, which checks
whether an identical tree-SHA was already built successfully. It
currently pins v8; v9 is the latest release.
What v9 changes:
- The `ACTIONS_ORCHESTRATION_ID` environment variable is now
appended to the HTTP user-agent string. This is transparent to
our script.
- A new injected `getOctokit` factory lets scripts create
additional authenticated clients in the same step without
importing `@actions/github`. We do not use it.
- Two breaking changes affect scripts that either call
`require('@actions/github')` (fails at runtime, because
`@actions/github` v9 is now ESM-only) or that shadow the
implicit `getOctokit` parameter via `const`/`let` (syntax
error). Our script does neither -- it only uses the pre-supplied
`github` REST client and `core` helpers -- so the upgrade is
safe.
Risk analysis: the step is advisory. It sets `enabled=' but skip'`
as an optimization to avoid re-running CI on a tree that was already
tested successfully. Even if the v9 upgrade broke the script, the
surrounding `try { ... } catch (e) { core.warning(e); }` block would
degrade it to a warning and CI would still run normally. In practice
the script continues to work identically on v9.
See also:
- Release notes: https://github.com/actions/github-script/releases
- Compare: actions/github-script@v8...v9
Originally-authored-by: dependabot[bot] <support@github.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Every workflow currently pins `actions/checkout` to v5, which was
introduced primarily to move to the Node.js 24 runtime. v6 is the
next release and worth picking up so we stay on a maintained version
of the action.
The one behaviorally interesting change in v6:
`persist-credentials` now stores the helper credentials under
`$RUNNER_TEMP` instead of writing them directly into the local
`.git/config`. Two implications follow:
1. In the normal case this is an unambiguous improvement -- the
token no longer lands in `.git/config`, reducing the risk of
inadvertently leaking it through workspace archiving
(`upload-artifact` snapshots, cache entries, core dumps, ...).
2. Docker container actions require an Actions Runner of at least
v2.329.0 to find the credentials in their new location. The
github.com-hosted runners our CI uses are already past that
version, so this does not affect us. Downstream users running
self-hosted runners may need to update them before adopting
this version of the action.
Risk analysis: our checkout steps either check out the default
repository (no special credential requirements) or, in the `vs-build`
job, explicitly set `repository: microsoft/vcpkg` and
`path: compat/vcbuild/vcpkg`. Neither case relies on the precise
location of the persisted credentials -- subsequent steps interact
with the API via the runner-provided `GITHUB_TOKEN` directly -- so
the v6 credential-storage change is transparent to our workflows.
The diff is purely the `@vN` identifier; there are no input or
output changes.
See also:
- Release notes: https://github.com/actions/checkout/releases
- Changelog: https://github.com/actions/checkout/blob/main/CHANGELOG.md
- Compare: actions/checkout@v5...v6
Originally-authored-by: dependabot[bot] <support@github.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The v1 of `git-for-windows/setup-git-for-windows-sdk` runs on Node.js 20, which GitHub is phasing out of the Actions runners. v2 moves the action to Node.js 24 so that the CI jobs relying on a Git for Windows SDK keep working once Node.js 20 is removed. The risk is very low: v2 contains no functional changes to the SDK setup itself, only the runtime upgrade. The action still provisions the same minimal SDK and exposes the same outputs. The sole precondition is a recent Actions Runner (>= 2.327.1), which the github.com-hosted runners already satisfy. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The l10n workflow uses `mshick/add-pr-comment` to post git-po-helper reports as comments on translation pull requests. It was still pinned to v2, which runs on Node.js 20. GitHub is phasing out the Node.js 20 runtime on Actions runners, so staying on v2 will eventually cause the "Create comment in pull request for report" step to fail. The sole breaking change in v3 is the switch from Node.js 20 to Node.js 24 (https://github.com/mshick/add-pr-comment/releases/tag/v3.0.0). The action's inputs and outputs are unchanged, so the upgrade is a drop-in replacement. Subsequent v3.x releases added new opt-in features (message truncation, retry with exponential backoff, file attachments, commit comment support, "delete on status") but none of them affect existing callers that do not opt in. See also: - Changelog: https://github.com/mshick/add-pr-comment/blob/main/CHANGELOG.md - Compare: mshick/add-pr-comment@v2...v3 Pointed-out-by: Christoph Grüninger <foss@grueninger.de> Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
At some point between Windows 10 Build 17134.1304 and Build 18363.657, the default behavior of `DeleteFileW()` was changed to use POSIX semantics (https://stackoverflow.com/a/60512798). Under those semantics, a file can be deleted even when another process holds an active `MapViewOfFile` view on it: the directory entry is removed immediately, but the underlying data persists until the last handle is closed. On older Windows versions (and Windows 10 builds before that change), `DeleteFileW()` uses legacy semantics where deletion fails outright if any process holds a file mapping. To allow testing code paths that depend on the legacy behavior, introduce a `GIT_TEST_LEGACY_DELETE` environment variable. When set, `mingw_unlink()` uses `SetFileInformationByHandle()` with `FileDispositionInfo` (the non-POSIX variant) instead of `DeleteFileW()`, forcing legacy delete semantics regardless of the Windows version. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
As is done for all the other maintenance tasks, let's release the ODB also before starting the geometric repacking. That way, the `.idx` files won't be `mmap()`ed when they are to be deleted (which does not work on Windows because you cannot delete files on that platform as long as they are kept open by a process). This regression was introduced by 9bc1518 (builtin/maintenance: introduce "geometric-repack" task, 2025-10-24), but was only noticed once geometric repacking was made the default in 452b12c (builtin/ maintenance: use "geometric" strategy by default, 2026-02-24). The fix recapitulates my work from df76ee7 (run-command: offer to close the object store before running, 2021-09-09) & friends. To guard against future regressions of this kind, add a check to `run_and_verify_geometric_pack()` in `t7900` that detects orphaned `.idx` files left behind after repacking. Contrary to interactive calls, the `git maintenance` call in that test case would _not_ block on Windows, asking whether to retry deleting that file, which is the reason why this bug was not caught earlier. Furthermore, since the default behavior of `DeleteFileW()` was changed at some point between Windows 10 Build 17134.1304 and Build 18363.657 to use POSIX semantics (see https://stackoverflow.com/a/60512798), the added orphaned-`.idx` check would be insufficient to catch this regression on modern Windows without emulating legacy delete semantics via `GIT_TEST_LEGACY_DELETE=1`. This fixes git-for-windows#6210. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "test_expect_success 'tag following always works over v0 http'" test in t5551 fails when it tries to run "git init tags", but this happens only when EXPENSIVE test is allowed to run. This is because the step tries to create a repository with "git init tags" but the EXPENSIVE test that runs way before it creates and leaves around a temporary file "tags". Have the EXPENSIVE test clean it up after itself. Acked-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Revert a recent change that introduced a regression to help mksh users. * jk/revert-aa-reap-transport-child-processes: Revert "transport-helper, connect: use clean_on_exit to reap children on abnormal exit"
Update various GitHub Actions versions.
* js/ci-github-actions-update:
l10n: bump mshick/add-pr-comment from v2 to v3
ci: bump git-for-windows/setup-git-for-windows-sdk from v1 to v2
ci: bump actions/checkout from v5 to v6
ci: bump actions/github-script from v8 to v9
ci: bump actions/{upload,download}-artifact to v7 and v8
ci: bump microsoft/setup-msbuild from v2 to v3
Avoid hitting the pathname limit for socks proxy socket during the test.. * js/t5564-socks-use-short-path: t5564: use a short path for the SOCKS proxy socket
Test fix. * jc/t5551-fix-expensive: t5551: "GIT_TEST_LONG=Yes make test" is broken
To help Windows 10 installations, avoid removing files whose contents are still mmap()'ed. * js/maintenance-fix-deadlock-on-win10: maintenance(geometric): do release the `.idx` files before repacking mingw: optionally use legacy (non-POSIX) delete semantics
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )