[PATCH 0/6] ci: improvements to our Rust infrastructure

[PATCH 0/6] ci: improvements to our Rust infrastructure

[Patrick Steinhardt]

Hi,

this small patch series introduces some improvements for our Rust
infrastructure. Most importantly, it introduces a couple of static
analysis checks to verify consistent formatting, use Clippy for linting
and to verify our minimum supported Rust version.

Furthermore, this series also introduces support for building with Rust
enabled on Windows.

The series is built on top of 45547b60ac (Merge branch 'master' of
https://github.com/j6t/gitk, 2025-10-05) with ps/rust-balloon at
e425c40aa0 (ci: enable Rust for breaking-changes jobs, 2025-10-02) and
ps/gitlab-ci-windows-improvements at 3c4925c3f5 (t8020: fix test failure
due to indeterministic tag sorting, 2025-10-02) merged into it.

Thanks!

Patrick

---
Patrick Steinhardt (6):
      ci: deduplicate calls to `apt-get update`
      ci: check formatting of our Rust code
      rust/varint: add safety comments
      ci: check for common Rust mistakes via Clippy
      ci: verify minimum supported Rust version
      rust: support for Windows

 .github/workflows/main.yml | 15 +++++++++++++++
 .gitlab-ci.yml             | 13 ++++++++++++-
 Cargo.toml                 |  1 +
 Makefile                   | 14 ++++++++++++--
 ci/install-dependencies.sh | 17 +++++++++++++----
 ci/run-rust-checks.sh      | 22 ++++++++++++++++++++++
 meson.build                |  4 ++++
 src/cargo-meson.sh         | 11 +++++++++--
 src/varint.rs              |  8 ++++++++
 9 files changed, 96 insertions(+), 9 deletions(-)


---
base-commit: 8c8e270f2aba359479c4c2b4ab3c62726e5dac9d
change-id: 20251007-b4-pks-ci-rust-8422e6a8196e

[PATCH 1/6] ci: deduplicate calls to `apt-get update`

[Patrick Steinhardt]

When installing dependencies we first check for the distribution that is
in use and then we check for the specific job. In the first step we
already install all dependencies required to build and test Git, whereas
the second step installs a couple of additional dependencies that are
only required to perform job-specific tasks.

In both steps we use `apt-get update` to update our repository sources.
This is unecessary though: all platforms that use Aptitude would have
already executed this command in the distro-specific step anyway.

Drop the redundant calls.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 0d3aa496fc..645d035250 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -120,21 +120,17 @@ esac
 
 case "$jobname" in
 ClangFormat)
-	sudo apt-get -q update
 	sudo apt-get -q -y install clang-format
 	;;
 StaticAnalysis)
-	sudo apt-get -q update
 	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
 		libexpat-dev gettext make
 	;;
 sparse)
-	sudo apt-get -q update -q
 	sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \
 		libexpat-dev gettext zlib1g-dev sparse
 	;;
 Documentation)
-	sudo apt-get -q update
 	sudo apt-get -q -y install asciidoc xmlto docbook-xsl-ns make
 
 	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||

-- 
2.51.0.764.g787ff6f08a.dirty

[PATCH 2/6] ci: check formatting of our Rust code

[Patrick Steinhardt]

Introduce a CI check that verifies that our Rust code is well-formatted.
This check uses rustfmt(1), which is the de-facto standard in the Rust
world.

The rustfmt(1) tool allows to tweak the final format in theory. In
practice though, the Rust ecosystem has aligned on style "editions".
These editions only exist to ensure that any potential changes to the
style don't cause reformats to existing code bases. Other than that,
most Rust projects out there accept this default style of a specific
edition.

Let's do the same and use that default style. It may not be anyone's
favorite, but it is consistent and by making it part of our CI we also
enforce it right from the start.

Note that we don't have to pick a specific style edition here, as the
edition is automatically derived from the edition we have specified in
our "Cargo.toml" file.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 15 +++++++++++++++
 .gitlab-ci.yml             | 11 +++++++++++
 ci/install-dependencies.sh |  5 +++++
 ci/run-rust-checks.sh      | 12 ++++++++++++
 4 files changed, 43 insertions(+)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 393ea4d1cc..9e36b5c5e3 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -458,6 +458,21 @@ jobs:
     - run: ci/install-dependencies.sh
     - run: ci/run-static-analysis.sh
     - run: ci/check-directional-formatting.bash
+  rust-analysis:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
+    env:
+      jobname: RustAnalysis
+      CI_JOB_IMAGE: ubuntu:rolling
+    runs-on: ubuntu-latest
+    container: ubuntu:rolling
+    concurrency:
+      group: rust-analysis-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
+    steps:
+    - uses: actions/checkout@v4
+    - run: ci/install-dependencies.sh
+    - run: ci/run-rust-checks.sh
   sparse:
     needs: ci-config
     if: needs.ci-config.outputs.enabled == 'yes'
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index f7d57d1ee9..a47d839e39 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -212,6 +212,17 @@ static-analysis:
     - ./ci/run-static-analysis.sh
     - ./ci/check-directional-formatting.bash
 
+rust-analysis:
+  image: ubuntu:rolling
+  stage: analyze
+  needs: [ ]
+  variables:
+    jobname: RustAnalysis
+  before_script:
+    - ./ci/install-dependencies.sh
+  script:
+    - ./ci/run-rust-checks.sh
+
 check-whitespace:
   image: ubuntu:latest
   stage: analyze
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 645d035250..a24b07edff 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -126,6 +126,11 @@ StaticAnalysis)
 	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
 		libexpat-dev gettext make
 	;;
+RustAnalysis)
+	sudo apt-get -q -y install rustup
+	rustup default stable
+	rustup component add rustfmt
+	;;
 sparse)
 	sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \
 		libexpat-dev gettext zlib1g-dev sparse
diff --git a/ci/run-rust-checks.sh b/ci/run-rust-checks.sh
new file mode 100755
index 0000000000..082eb52f11
--- /dev/null
+++ b/ci/run-rust-checks.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+. ${0%/*}/lib.sh
+
+set +x
+
+if ! group "Check Rust formatting" cargo fmt --all --check
+then
+	RET=1
+fi
+
+exit $RET

-- 
2.51.0.764.g787ff6f08a.dirty

[PATCH 3/6] rust/varint: add safety comments

[Patrick Steinhardt]

The `decode_varint()` and `encode_varint()` functions in our Rust crate
are reimplementations of the respective C functions. As such, we are
naturally forced to use the same interface in both Rust and C, which
makes use of raw pointers. The consequence is that the code needs to be
marked as unsafe in Rust.

It is common practice in Rust to provide safety documentation for every
block that is marked as unsafe. This common practice is also enforced by
Clippy, Rust's static analyser. We don't have Clippy wired up yet, and
we could of course just disable this check. But we're about to wire it
up, and it is reasonable to always enforce documentation for unsafe
blocks.

Add such safety comments to already squelch those warnings now.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 src/varint.rs | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/varint.rs b/src/varint.rs
index 6e610bdd8e..43b48debb5 100644
--- a/src/varint.rs
+++ b/src/varint.rs
@@ -1,3 +1,6 @@
+/// # Safety
+///
+/// Callers must provide a NUL-terminated array to ensure safety.
 #[no_mangle]
 pub unsafe extern "C" fn decode_varint(bufp: *mut *const u8) -> u64 {
     let mut buf = *bufp;
@@ -22,6 +25,11 @@ pub unsafe extern "C" fn decode_varint(bufp: *mut *const u8) -> u64 {
     val
 }
 
+/// # Safety
+///
+/// The provided buffer must be large enough to store the encoded varint. Callers may either provide
+/// a `[u8; 16]` here, which is guaranteed to satisfy all encodable numbers. Or they can call this
+/// function with a `NULL` pointer first to figure out array size.
 #[no_mangle]
 pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
     let mut varint: [u8; 16] = [0; 16];

-- 
2.51.0.764.g787ff6f08a.dirty

[PATCH 4/6] ci: check for common Rust mistakes via Clippy

[Patrick Steinhardt]

Introduce a CI check that uses Clippy to perform checks for common
mistakes and suggested code improvements. Clippy is the official static
analyser of the Rust project and thus the de-facto standard.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 2 +-
 ci/run-rust-checks.sh      | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index a24b07edff..dcd22ddd95 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -129,7 +129,7 @@ StaticAnalysis)
 RustAnalysis)
 	sudo apt-get -q -y install rustup
 	rustup default stable
-	rustup component add rustfmt
+	rustup component add clippy rustfmt
 	;;
 sparse)
 	sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \
diff --git a/ci/run-rust-checks.sh b/ci/run-rust-checks.sh
index 082eb52f11..fb5ea8991b 100755
--- a/ci/run-rust-checks.sh
+++ b/ci/run-rust-checks.sh
@@ -9,4 +9,9 @@ then
 	RET=1
 fi
 
+if ! group "Check for common Rust mistakes" cargo clippy --all-targets --all-features -- -Dwarnings
+then
+	RET=1
+fi
+
 exit $RET

-- 
2.51.0.764.g787ff6f08a.dirty

[PATCH 5/6] ci: verify minimum supported Rust version

[Patrick Steinhardt]

In the current state of our Rust code base we don't really have any
requirements for the minimum supported Rust version yet, as we don't use
any features introduced by a recent version of Rust. Consequently, we
have decided that we want to aim for a rather old version and edition of
Rust, where the hope is that using an old version will make alternatives
like gccrs viable earlier for compiling Git.

But while we specify the Rust edition, we don't yet specify a Rust
version. And even if we did, the Rust version would only be enforced for
our own code, but not for any of our dependencies.

We don't yet have any dependencies at the current point in time. But
let's add some safeguards by specifying the minimum supported Rust
version and using cargo-msrv(1) to verify that this version can be
satisfied for all of our dependencies.

Note that we fix the version of cargo-msrv(1) at v0.18.1. This is the
latest release supported by Ubuntu's Rust version.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Cargo.toml                 | 1 +
 ci/install-dependencies.sh | 8 ++++++++
 ci/run-rust-checks.sh      | 5 +++++
 3 files changed, 14 insertions(+)

diff --git a/Cargo.toml b/Cargo.toml
index 45c9b34981..2f51bf5d5f 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -2,6 +2,7 @@
 name = "gitcore"
 version = "0.1.0"
 edition = "2018"
+rust-version = "1.49.0"
 
 [lib]
 crate-type = ["staticlib"]
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index dcd22ddd95..29e558bb9c 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -10,6 +10,8 @@ begin_group "Install dependencies"
 P4WHENCE=https://cdist2.perforce.com/perforce/r23.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
 JGITWHENCE=https://repo1.maven.org/maven2/org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh
+CARGO_MSRV_VERSION=0.18.4
+CARGO_MSRV_WHENCE=https://github.com/foresterre/cargo-msrv/releases/download/v$CARGO_MSRV_VERSION/cargo-msrv-x86_64-unknown-linux-musl-v$CARGO_MSRV_VERSION.tgz
 
 # Make sudo a no-op and execute the command directly when running as root.
 # While using sudo would be fine on most platforms when we are root already,
@@ -130,6 +132,12 @@ RustAnalysis)
 	sudo apt-get -q -y install rustup
 	rustup default stable
 	rustup component add clippy rustfmt
+
+	wget -q "$CARGO_MSRV_WHENCE" -O "cargo-msvc.tgz"
+	sudo mkdir -p "$CUSTOM_PATH"
+	sudo tar -xf "cargo-msvc.tgz" --strip-components=1 \
+		--directory "$CUSTOM_PATH" --wildcards "*/cargo-msrv"
+	sudo chmod a+x "$CUSTOM_PATH/cargo-msrv"
 	;;
 sparse)
 	sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \
diff --git a/ci/run-rust-checks.sh b/ci/run-rust-checks.sh
index fb5ea8991b..b5ad9e8dc6 100755
--- a/ci/run-rust-checks.sh
+++ b/ci/run-rust-checks.sh
@@ -14,4 +14,9 @@ then
 	RET=1
 fi
 
+if ! group "Check for minimum required Rust version" cargo msrv verify
+then
+	RET=1
+fi
+
 exit $RET

-- 
2.51.0.764.g787ff6f08a.dirty

[PATCH 6/6] rust: support for Windows

[Patrick Steinhardt]

The initial patch series that introduced Rust into the core of Git only
cared about macOS and Linux. This specifically leaves out Windows, which
indeed fails to build right now due to two issues:

  - The Rust runtime requires `GetUserProfileDirectoryW()`, but we don't
    link against "userenv.dll".

  - The path of the Rust library built on Windows is different than on
    most other systems systems.

Fix both of these issues to support Windows.

Note that this commit fixes the Meson-based job in GitHub's CI. Meson
auto-detects the availability of Rust, and as the Windows runner has
Rust installed by default it already enabled Rust support there. But due
to the above issues that job fails consistently.

Install Rust on GitLab CI, as well, to improve test coverage there.

Based-on-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Based-on-patch-by: Ezekiel Newren <ezekielnewren@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml     |  2 +-
 Makefile           | 14 ++++++++++++--
 meson.build        |  4 ++++
 src/cargo-meson.sh | 11 +++++++++--
 4 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index a47d839e39..b419a84e2c 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -161,7 +161,7 @@ test:mingw64:
     - saas-windows-medium-amd64
   before_script:
     - *windows_before_script
-    - choco install -y git meson ninja
+    - choco install -y git meson ninja rust-ms
     - Import-Module $env:ChocolateyInstall\helpers\chocolateyProfile.psm1
     - refreshenv
 
diff --git a/Makefile b/Makefile
index 7ea149598d..366fd173e7 100644
--- a/Makefile
+++ b/Makefile
@@ -929,10 +929,17 @@ TEST_SHELL_PATH = $(SHELL_PATH)
 LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 REFTABLE_LIB = reftable/libreftable.a
+
 ifdef DEBUG
-RUST_LIB = target/debug/libgitcore.a
+RUST_TARGET_DIR = target/debug
 else
-RUST_LIB = target/release/libgitcore.a
+RUST_TARGET_DIR = target/release
+endif
+
+ifeq ($(uname_S),Windows)
+RUST_LIB = $(RUST_TARGET_DIR)/gitcore.lib
+else
+RUST_LIB = $(RUST_TARGET_DIR)/libgitcore.a
 endif
 
 # xdiff and reftable libs may in turn depend on what is in libgit.a
@@ -1538,6 +1545,9 @@ ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)
 ifdef WITH_RUST
 BASIC_CFLAGS += -DWITH_RUST
 GITLIBS += $(RUST_LIB)
+ifeq ($(uname_S),Windows)
+EXTLIBS += -luserenv
+endif
 endif
 
 ifdef SANITIZE
diff --git a/meson.build b/meson.build
index ec55d6a5fd..a9c865b2af 100644
--- a/meson.build
+++ b/meson.build
@@ -1707,6 +1707,10 @@ rust_option = get_option('rust').disable_auto_if(not cargo.found())
 if rust_option.allowed()
   subdir('src')
   libgit_c_args += '-DWITH_RUST'
+
+  if host_machine.system() == 'windows'
+    libgit_dependencies += compiler.find_library('userenv')
+  endif
 else
   libgit_sources += [
     'varint.c',
diff --git a/src/cargo-meson.sh b/src/cargo-meson.sh
index 99400986d9..3998db0435 100755
--- a/src/cargo-meson.sh
+++ b/src/cargo-meson.sh
@@ -26,7 +26,14 @@ then
 	exit $RET
 fi
 
-if ! cmp "$BUILD_DIR/$BUILD_TYPE/libgitcore.a" "$BUILD_DIR/libgitcore.a" >/dev/null 2>&1
+case "$(cargo -vV | sed -s 's/^host: \(.*\)$/\1/')" in
+	*-windows-*)
+		LIBNAME=gitcore.lib;;
+	*)
+		LIBNAME=libgitcore.a;;
+esac
+
+if ! cmp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/libgitcore.a" >/dev/null 2>&1
 then
-	cp "$BUILD_DIR/$BUILD_TYPE/libgitcore.a" "$BUILD_DIR/libgitcore.a"
+	cp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/libgitcore.a"
 fi

-- 
2.51.0.764.g787ff6f08a.dirty

Re: [PATCH 1/6] ci: deduplicate calls to `apt-get update`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> When installing dependencies we first check for the distribution that is
> in use and then we check for the specific job. In the first step we
> already install all dependencies required to build and test Git, whereas
> the second step installs a couple of additional dependencies that are
> only required to perform job-specific tasks.
>
> In both steps we use `apt-get update` to update our repository sources.
> This is unecessary though: all platforms that use Aptitude would have
> already executed this command in the distro-specific step anyway.
>

Nit: s/unecessary/unnecessary

The patch looks good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 2/6] ci: check formatting of our Rust code

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Introduce a CI check that verifies that our Rust code is well-formatted.
> This check uses rustfmt(1), which is the de-facto standard in the Rust
> world.
>
> The rustfmt(1) tool allows to tweak the final format in theory. In
> practice though, the Rust ecosystem has aligned on style "editions".
> These editions only exist to ensure that any potential changes to the
> style don't cause reformats to existing code bases. Other than that,
> most Rust projects out there accept this default style of a specific
> edition.
>
> Let's do the same and use that default style. It may not be anyone's
> favorite, but it is consistent and by making it part of our CI we also
> enforce it right from the start.
>
> Note that we don't have to pick a specific style edition here, as the
> edition is automatically derived from the edition we have specified in
> our "Cargo.toml" file.

One small nit: We should mention that `cargo fmt` is simply a wrapper
around `rustfmt`, which also handles file discovery.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 2/6] ci: check formatting of our Rust code

[Patrick Steinhardt]

On Tue, Oct 07, 2025 at 06:04:41AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Introduce a CI check that verifies that our Rust code is well-formatted.
> > This check uses rustfmt(1), which is the de-facto standard in the Rust
> > world.
> >
> > The rustfmt(1) tool allows to tweak the final format in theory. In
> > practice though, the Rust ecosystem has aligned on style "editions".
> > These editions only exist to ensure that any potential changes to the
> > style don't cause reformats to existing code bases. Other than that,
> > most Rust projects out there accept this default style of a specific
> > edition.
> >
> > Let's do the same and use that default style. It may not be anyone's
> > favorite, but it is consistent and by making it part of our CI we also
> > enforce it right from the start.
> >
> > Note that we don't have to pick a specific style edition here, as the
> > edition is automatically derived from the edition we have specified in
> > our "Cargo.toml" file.
> 
> One small nit: We should mention that `cargo fmt` is simply a wrapper
> around `rustfmt`, which also handles file discovery.

Good idea, I'll include this in the next version. Thanks!

Patrick

Re: [PATCH 2/6] ci: check formatting of our Rust code

[Eric Sunshine]

On Tue, Oct 7, 2025 at 8:37 AM Patrick Steinhardt <ps@pks.im> wrote:
> Introduce a CI check that verifies that our Rust code is well-formatted.
> This check uses rustfmt(1), which is the de-facto standard in the Rust
> world.
>
> The rustfmt(1) tool allows to tweak the final format in theory. In
> practice though, the Rust ecosystem has aligned on style "editions".
> These editions only exist to ensure that any potential changes to the
> style don't cause reformats to existing code bases. Other than that,
> most Rust projects out there accept this default style of a specific
> edition.
>
> Let's do the same and use that default style. It may not be anyone's
> favorite, but it is consistent and by making it part of our CI we also
> enforce it right from the start.

In a different thread, I wrote[1]:

    There are more than a few developers on this project (including
    myself) who still use 80-column editors and terminals. As a
    general style guideline, this project does recommend wrapping code
    to fit within 80 columns (except in cases when doing so would
    severely hurt readability). I imagine that the same sort of
    guideline would be appreciated in Rust code, as well, by those who
    still stick with 80 columns.

    I bring this up because, although it hasn't been such a big deal
    with the existing C code, assuming that developers run `rustfmt`
    on the code before sending a patch series, then this may become an
    issue if different developers have `rustfmt` configured to enforce
    different maximum column width, especially since `rustfmt` is
    likely to reformat the entire file rather than just the region
    that has just been edited.  So, if this code gets checked in as-is
    with these very wide lines, and then someone else, who has
    `rustfmt` configured for 80-columns edits the file, then it
    becomes a problem.

    As such, can we also add a project-wide `rustfmt.toml` which, at
    minimum, sets the maximum line width to 80? For instance:

        max_width = 80

Later in the same thread, I wrote[2]:

    Project guidelines have long suggested 80 columns as a desirable
    maximum not only for C code, but for pretty much all other
    resources, including shell code, Perl code, and documentation
    files. This suggested maximum works well for adherents of
    80-columns and (presumably) hasn't been too onerous for developers
    who use wider windows; at least we haven't heard people clamoring
    to increase the suggested maximum column limit. As such, it does
    not seem far-fetched to expect that the project guidelines
    should/could/would also apply to Rust code.

Unfortunately, what little discussion there was petered out quickly
without resolution, but it seems that it would be a good idea to make
some sort of decision earlier (while there is still very little Rust
code committed to the project) rather than later.

[1]: https://lore.kernel.org/git/CAPig+cTZch_pvfurtjBTNphMeRQL6jSBSjNY-4mffjoXZ4eqcw@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cTdJAjuekz6YXDkxTjTRxsPEzSUxhoD8nK9k7uA4s=rHQ@mail.gmail.com/

> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Re: [PATCH 2/6] ci: check formatting of our Rust code

[Junio C Hamano]

Eric Sunshine <ericsunshine@gmail.com> writes:

>     I bring this up because, although it hasn't been such a big deal
>     with the existing C code, assuming that developers run `rustfmt`
>     on the code before sending a patch series, then this may become an
>     issue if different developers have `rustfmt` configured to enforce
>     different maximum column width, especially since `rustfmt` is
>     likely to reformat the entire file rather than just the region
>     that has just been edited.  So, if this code gets checked in as-is
>     with these very wide lines, and then someone else, who has
>     `rustfmt` configured for 80-columns edits the file, then it
>     becomes a problem.
>
>     As such, can we also add a project-wide `rustfmt.toml` which, at
>     minimum, sets the maximum line width to 80? For instance:
>
>         max_width = 80
>
> Later in the same thread, I wrote[2]:
>
>     Project guidelines have long suggested 80 columns as a desirable
>     maximum not only for C code, but for pretty much all other
>     resources, including shell code, Perl code, and documentation
>     files. This suggested maximum works well for adherents of
>     80-columns and (presumably) hasn't been too onerous for developers
>     who use wider windows; at least we haven't heard people clamoring
>     to increase the suggested maximum column limit. As such, it does
>     not seem far-fetched to expect that the project guidelines
>     should/could/would also apply to Rust code.
>
> Unfortunately, what little discussion there was petered out quickly
> without resolution, but it seems that it would be a good idea to make
> some sort of decision earlier (while there is still very little Rust
> code committed to the project) rather than later.

I do not see a particular reason to lift the 80-column limit for a
specific language, whether it is Rust or AsciiDoc.  I myself use my
terminal set to slightly wider than 80 columns these days, but that
is primarily to accomodate the fact that code in a patch that are
quoted a few times in the discussion would grow from their original
line length, not to write pieces of code that are wider than 80
columns myself.

Will it inconvenience wider Rust ecosystem when we get big (meaning,
they have to work with our code) and we as the project norm use
different line-length setting from others, perhaps by looking too
different from everybody else, or something?

Re: [PATCH 2/6] ci: check formatting of our Rust code

[Eric Sunshine]

On Tue, Oct 7, 2025 at 1:38 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <ericsunshine@gmail.com> writes:
> > Later in the same thread, I wrote[2]:
> >
> >     Project guidelines have long suggested 80 columns as a desirable
> >     maximum not only for C code, but for pretty much all other
> >     resources, including shell code, Perl code, and documentation
> >     files. This suggested maximum works well for adherents of
> >     80-columns and (presumably) hasn't been too onerous for developers
> >     who use wider windows; at least we haven't heard people clamoring
> >     to increase the suggested maximum column limit. As such, it does
> >     not seem far-fetched to expect that the project guidelines
> >     should/could/would also apply to Rust code.
>
> I do not see a particular reason to lift the 80-column limit for a
> specific language, whether it is Rust or AsciiDoc. [...]
>
> Will it inconvenience wider Rust ecosystem when we get big (meaning,
> they have to work with our code) and we as the project norm use
> different line-length setting from others, perhaps by looking too
> different from everybody else, or something?

As a general answer, I would assume that third-party projects wanting
to use Rust code from the Git project would do so by importing one or
more "crates" that the Git project publishes rather than importing raw
code directly from the Git project. In this case they never deal
directly with Git's Rust code itself, but instead interact via the Git
crate's public API.

If a third-party project does want/need to import some raw Git Rust
code directly but has no plans to actually edit the code, then there
should be no problem. If the project does plan to edit the imported
code and periodically update it from upstream Git, then it's a bit
more onerous, though perhaps not so much so; running the Git upstream
code through `rustfmt` before import into the project is one simple
step which can easily be automated.

Re: [PATCH 2/6] ci: check formatting of our Rust code

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

On 2025-10-07 at 12:36:30, Patrick Steinhardt wrote:
> Introduce a CI check that verifies that our Rust code is well-formatted.
> This check uses rustfmt(1), which is the de-facto standard in the Rust
> world.
> 
> The rustfmt(1) tool allows to tweak the final format in theory. In
> practice though, the Rust ecosystem has aligned on style "editions".
> These editions only exist to ensure that any potential changes to the
> style don't cause reformats to existing code bases. Other than that,
> most Rust projects out there accept this default style of a specific
> edition.
> 
> Let's do the same and use that default style. It may not be anyone's
> favorite, but it is consistent and by making it part of our CI we also
> enforce it right from the start.

Yes, I think this is the right decision.  We _can_ customize it if we
want, but I never do, even though I don't love some of the policies
(like the cuddled elses), since having a fixed standard avoids all of
the argument.  The Rust code I'll be sending in within the next few
weeks already uses rustfmt.

Even if we did decide to customize it, I think there's enormous value in
just being able to run the tool and accept what it outputs, saving us
enormous amounts of time not having to discuss style nits.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 2/6] ci: check formatting of our Rust code

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]

On 2025-10-07 at 17:13:18, Eric Sunshine wrote:
> Later in the same thread, I wrote[2]:
> 
>     Project guidelines have long suggested 80 columns as a desirable
>     maximum not only for C code, but for pretty much all other
>     resources, including shell code, Perl code, and documentation
>     files. This suggested maximum works well for adherents of
>     80-columns and (presumably) hasn't been too onerous for developers
>     who use wider windows; at least we haven't heard people clamoring
>     to increase the suggested maximum column limit. As such, it does
>     not seem far-fetched to expect that the project guidelines
>     should/could/would also apply to Rust code.

My preference is actually that we stick with the default.  I use (and
for a long time have used) a 132-character editor window and I find it
quite useful to have the extra space.  The DEC VT100 did 132 columns
(available on your local Linux system as `vt100-w`), so I think there's
plenty of precedent for that being an acceptable width[0].

I did previously use 80-column terminals when I had a tiny laptop
screen, but modern display resolutions over the past decade, even on
smaller laptops, have made it entirely possible to get several wider
terminal windows (or in my case, tmux panes) on one screen.  One of my
current tmux panes is now 213×54 and I really enjoy the extra space.

The default Rust behaviour is 100 characters[1], which I think is a fine
default.  I won't be enormously angsty if we say we still absolutely
must stick to 80-character lines, but I also think we should take this
opportunity to choose the Rust defaults for Rust.  C, Perl, and text
formats like AsciiDoc do not have rigid defaults about indentation
style, tabs vs. spaces, and line length; Rust does.  We wouldn't use
tabs in Rust (the default is four spaces) because we use it everywhere
else, so I think we should take the opportunity to use the Rust defaults
here as well.

Whatever we ultimately decide, I plan to send an update to our
`.editorconfig` file.  I think that's less useful in general for Rust,
where we have an automatic tidy tool and CI to check for it, but there
are some people for whom it will be useful and we might as well keep it
correct.

[0] For what it's worth, Linus also thinks longer lines and 132-column
terminals are useful.  I don't agree with him about everything,
certainly, but I think we see eye to eye here:
https://lkml.org/lkml/2020/5/29/1038.
[1] % rustfmt --print-config=default /dev/stdout | grep '^max_width'
max_width = 100
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 2/6] ci: check formatting of our Rust code

[Chris Torek]

On Tue, Oct 7, 2025 at 3:42 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> My preference is actually that we stick with the default.  I use (and
> for a long time have used) a 132-character editor window and I find it
> quite useful to have the extra space. [mass snip]

Since I started doing some Go programming (which I still do far more
than Rust) I do the same. I actually let it go to 140 or more sometimes,
with vim settings to put shadow marks at 80 and 132.

I still use my trusty 80x50 windows for holding a lot of individual
windows at a time though. :-)

Chris

Re: [PATCH 3/6] rust/varint: add safety comments

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 4468 bytes --]

On 2025-10-07 at 12:36:31, Patrick Steinhardt wrote:
> +/// # Safety
> +///
> +/// The provided buffer must be large enough to store the encoded varint. Callers may either provide
> +/// a `[u8; 16]` here, which is guaranteed to satisfy all encodable numbers. Or they can call this
> +/// function with a `NULL` pointer first to figure out array size.
>  #[no_mangle]
>  pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
>      let mut varint: [u8; 16] = [0; 16];

I'm planning to do something a little different with this code by
refactoring it out into a Rust function, so at that point it will no
longer be possible to provide a buffer smaller than 16 bytes.  Note that
all callers of this function pass a 16-byte buffer, so that should be
safe.

That doesn't mean that you can't send this patch (and I think your patch
is good), just that we shouldn't tell people we can use a buffer smaller
than 16 bytes, since that will at some point no longer be true.

Here's the current version of the patch I'm planning on sending for
reference.  I can rebase onto your series once Junio picks it up.

-- >% --
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: "brian m. carlson" <sandals@crustytoothpaste.net>
Date: Wed, 8 Oct 2025 00:27:56 +0000
Subject: [PATCH] varint: write a safe Rust version of encode_varint

Our original version of encode_varint in Rust used pointers much like
the C version did.  However, if we end up using this function elsewhere
in Rust, it would be better to have a safe version that we can use.

In addition, writing our unsafe C-compatible version in terms of a safe
Rust version makes it obvious what our requirements are.  For instance,
we do not need buf to actually point anywhere and can accept a null
pointer if we just want the length, and we can clearly indicate that we
require 16 bytes worth of memory to encode data by creating an
appropriate slice.  All of our existing callers always pass a 16-byte
buffer, so we can safely assume that.

We can then improve our Rust version by performing normal bounds
checking to make sure that we don't exceed the buffer size and use the
standard usize return for lengths, converting as necessary in the
C-compatible caller.

Move the C-compatible code to a mod c to keep things tidy and allow us
to have a different Rust version.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 src/varint.rs | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/src/varint.rs b/src/varint.rs
index 6e610bdd8e..83990afe7a 100644
--- a/src/varint.rs
+++ b/src/varint.rs
@@ -22,8 +22,7 @@ pub unsafe extern "C" fn decode_varint(bufp: *mut *const u8) -> u64 {
     val
 }
 
-#[no_mangle]
-pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
+pub fn encode_varint(value: u64, buf: Option<&mut [u8]>) -> usize {
     let mut varint: [u8; 16] = [0; 16];
     let mut pos = varint.len() - 1;
 
@@ -37,16 +36,19 @@ pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
         value >>= 7;
     }
 
-    if !buf.is_null() {
-        std::ptr::copy_nonoverlapping(varint.as_ptr().add(pos), buf, varint.len() - pos);
+    let len = varint.len() - pos;
+
+    if let Some(buf) = buf {
+        buf[0..len].copy_from_slice(&varint[pos..pos + len]);
     }
 
-    (varint.len() - pos) as u8
+    len
 }
 
 #[cfg(test)]
 mod tests {
-    use super::*;
+    use super::c::encode_varint;
+    use super::decode_varint;
 
     #[test]
     fn test_decode_varint() {
@@ -90,3 +92,23 @@ mod tests {
         }
     }
 }
+
+mod c {
+    /// Encode `value` into `buf` as a variable-length integer unless `buf` is null.
+    ///
+    /// Returns the number of bytes written, or, if `buf` is null, the number of bytes that would be
+    /// used to encode the integer.
+    ///
+    /// # Safety
+    ///
+    /// `buf` must either be null or point to at least 16 bytes of memory.
+    #[no_mangle]
+    pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
+        let buffer = if buf.is_null() {
+            None
+        } else {
+            Some(std::slice::from_raw_parts_mut(buf, 16))
+        };
+        super::encode_varint(value, buffer) as u8
+    }
+}
-- >% --
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 2/6] ci: check formatting of our Rust code

[Patrick Steinhardt]

On Tue, Oct 07, 2025 at 10:42:16PM +0000, brian m. carlson wrote:
> On 2025-10-07 at 17:13:18, Eric Sunshine wrote:
> > Later in the same thread, I wrote[2]:
> > 
> >     Project guidelines have long suggested 80 columns as a desirable
> >     maximum not only for C code, but for pretty much all other
> >     resources, including shell code, Perl code, and documentation
> >     files. This suggested maximum works well for adherents of
> >     80-columns and (presumably) hasn't been too onerous for developers
> >     who use wider windows; at least we haven't heard people clamoring
> >     to increase the suggested maximum column limit. As such, it does
> >     not seem far-fetched to expect that the project guidelines
> >     should/could/would also apply to Rust code.
> 
> My preference is actually that we stick with the default.  I use (and
> for a long time have used) a 132-character editor window and I find it
> quite useful to have the extra space.  The DEC VT100 did 132 columns
> (available on your local Linux system as `vt100-w`), so I think there's
> plenty of precedent for that being an acceptable width[0].
> 
> I did previously use 80-column terminals when I had a tiny laptop
> screen, but modern display resolutions over the past decade, even on
> smaller laptops, have made it entirely possible to get several wider
> terminal windows (or in my case, tmux panes) on one screen.  One of my
> current tmux panes is now 213×54 and I really enjoy the extra space.
> 
> The default Rust behaviour is 100 characters[1], which I think is a fine
> default.  I won't be enormously angsty if we say we still absolutely
> must stick to 80-character lines, but I also think we should take this
> opportunity to choose the Rust defaults for Rust.  C, Perl, and text
> formats like AsciiDoc do not have rigid defaults about indentation
> style, tabs vs. spaces, and line length; Rust does.  We wouldn't use
> tabs in Rust (the default is four spaces) because we use it everywhere
> else, so I think we should take the opportunity to use the Rust defaults
> here as well.

I am also slightly leaning into the direction of sticking with Rust's
default of 100 characters. It's not substantially more than 80, should
be reasonable to accommodate for in most modern setups, and sticks with
what the remainder of the ecosystem is doing.

So for now I'll leave it at 80 characters. But I don't feel strongly
about this, so if there is a majority in favor of 80 characters I'm
happy to adjust.

Thanks!

Patrick

Re: [PATCH 3/6] rust/varint: add safety comments

[Patrick Steinhardt]

On Wed, Oct 08, 2025 at 12:29:38AM +0000, brian m. carlson wrote:
> On 2025-10-07 at 12:36:31, Patrick Steinhardt wrote:
> > +/// # Safety
> > +///
> > +/// The provided buffer must be large enough to store the encoded varint. Callers may either provide
> > +/// a `[u8; 16]` here, which is guaranteed to satisfy all encodable numbers. Or they can call this
> > +/// function with a `NULL` pointer first to figure out array size.
> >  #[no_mangle]
> >  pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
> >      let mut varint: [u8; 16] = [0; 16];
> 
> I'm planning to do something a little different with this code by
> refactoring it out into a Rust function, so at that point it will no
> longer be possible to provide a buffer smaller than 16 bytes.  Note that
> all callers of this function pass a 16-byte buffer, so that should be
> safe.

Ah, true. I just double-checked, and all callers pass in a 16 byte
buffer indeed. Also means that the NULL-pointer handling can go away in
theory, as we don't use it.

In any case, your direction makes sense once we have Rust-internal
callers of this functionality. We definitely don't want to propagate the
unsafety to callers and should make sure that it is contained to the C
API.

> That doesn't mean that you can't send this patch (and I think your patch
> is good), just that we shouldn't tell people we can use a buffer smaller
> than 16 bytes, since that will at some point no longer be true.
> 
> Here's the current version of the patch I'm planning on sending for
> reference.  I can rebase onto your series once Junio picks it up.

The patch makes sense to me, thanks. I'll not pick it up yet though as
there is no justifiable need as part of my series, but I'm happy to
adjust the comment.

Thanks!

Patrick

Re: [PATCH 2/6] ci: check formatting of our Rust code

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> ... but I also think we should take this
>> opportunity to choose the Rust defaults for Rust.  C, Perl, and text
>> formats like AsciiDoc do not have rigid defaults about indentation
>> style, tabs vs. spaces, and line length; Rust does.  We wouldn't use
>> tabs in Rust (the default is four spaces) because we use it everywhere
>> else, so I think we should take the opportunity to use the Rust defaults
>> here as well.
>
> I am also slightly leaning into the direction of sticking with Rust's
> default of 100 characters. It's not substantially more than 80, should
> be reasonable to accommodate for in most modern setups, and sticks with
> what the remainder of the ecosystem is doing.
>
> So for now I'll leave it at 80 characters. But I don't feel strongly
> about this, so if there is a majority in favor of 80 characters I'm
> happy to adjust.

So the question is if we want consistency across files regardless of
what language they are written in (i.e. 80-columns everywhere) or we
treat our existing rules a "fallback rules" we have adopted while
dealing with languages without their own strict rules, and use the
default for a language with its own rule (i.e. whatever rustfmt
wants is used for Rust, our own rules still apply to everything
else)?

I actually am fine with the latter myself.

If people strongly prefer, I also can be talked into adopting
slightly wider limit for our fallback rules for everything else, but
that is probably a separate discussion.  It is a bit unfriendly move
against folks with aging eyeballs like myself, though.

Thanks.

Re: [PATCH 2/6] ci: check formatting of our Rust code

[SZEDER Gábor]

On Tue, Oct 07, 2025 at 02:36:30PM +0200, Patrick Steinhardt wrote:
> Introduce a CI check that verifies that our Rust code is well-formatted.
> This check uses rustfmt(1), which is the de-facto standard in the Rust
> world.
> 
> The rustfmt(1) tool allows to tweak the final format in theory. In
> practice though, the Rust ecosystem has aligned on style "editions".
> These editions only exist to ensure that any potential changes to the
> style don't cause reformats to existing code bases. Other than that,
> most Rust projects out there accept this default style of a specific
> edition.
> 
> Let's do the same and use that default style. It may not be anyone's
> favorite, but it is consistent and by making it part of our CI we also
> enforce it right from the start.
> 
> Note that we don't have to pick a specific style edition here, as the
> edition is automatically derived from the edition we have specified in
> our "Cargo.toml" file.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

> diff --git a/ci/run-rust-checks.sh b/ci/run-rust-checks.sh
> new file mode 100755
> index 0000000000..082eb52f11
> --- /dev/null
> +++ b/ci/run-rust-checks.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +
> +. ${0%/*}/lib.sh
> +
> +set +x
> +
> +if ! group "Check Rust formatting" cargo fmt --all --check
> +then
> +	RET=1
> +fi
> +
> +exit $RET

Our ci/*.sh scripts usually rely on 'set -e' to catch failed commands.
Either this script should follow that convention as well, or the
commit message should justify the deviation from convention.

Re: [PATCH 2/6] ci: check formatting of our Rust code

[Patrick Steinhardt]

On Wed, Oct 08, 2025 at 08:34:22AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> ... but I also think we should take this
> >> opportunity to choose the Rust defaults for Rust.  C, Perl, and text
> >> formats like AsciiDoc do not have rigid defaults about indentation
> >> style, tabs vs. spaces, and line length; Rust does.  We wouldn't use
> >> tabs in Rust (the default is four spaces) because we use it everywhere
> >> else, so I think we should take the opportunity to use the Rust defaults
> >> here as well.
> >
> > I am also slightly leaning into the direction of sticking with Rust's
> > default of 100 characters. It's not substantially more than 80, should
> > be reasonable to accommodate for in most modern setups, and sticks with
> > what the remainder of the ecosystem is doing.
> >
> > So for now I'll leave it at 80 characters. But I don't feel strongly
> > about this, so if there is a majority in favor of 80 characters I'm
> > happy to adjust.
> 
> So the question is if we want consistency across files regardless of
> what language they are written in (i.e. 80-columns everywhere) or we
> treat our existing rules a "fallback rules" we have adopted while
> dealing with languages without their own strict rules, and use the
> default for a language with its own rule (i.e. whatever rustfmt
> wants is used for Rust, our own rules still apply to everything
> else)?

Yeah, exactly.

> I actually am fine with the latter myself.

Okay.

> If people strongly prefer, I also can be talked into adopting
> slightly wider limit for our fallback rules for everything else, but
> that is probably a separate discussion.  It is a bit unfriendly move
> against folks with aging eyeballs like myself, though.

Yup, this feels like a separate question indeed.

Thanks!

Patrick

Re: [PATCH 2/6] ci: check formatting of our Rust code

[Patrick Steinhardt]

On Wed, Oct 08, 2025 at 10:55:43PM +0200, SZEDER Gábor wrote:
> On Tue, Oct 07, 2025 at 02:36:30PM +0200, Patrick Steinhardt wrote:
> > Introduce a CI check that verifies that our Rust code is well-formatted.
> > This check uses rustfmt(1), which is the de-facto standard in the Rust
> > world.
> > 
> > The rustfmt(1) tool allows to tweak the final format in theory. In
> > practice though, the Rust ecosystem has aligned on style "editions".
> > These editions only exist to ensure that any potential changes to the
> > style don't cause reformats to existing code bases. Other than that,
> > most Rust projects out there accept this default style of a specific
> > edition.
> > 
> > Let's do the same and use that default style. It may not be anyone's
> > favorite, but it is consistent and by making it part of our CI we also
> > enforce it right from the start.
> > 
> > Note that we don't have to pick a specific style edition here, as the
> > edition is automatically derived from the edition we have specified in
> > our "Cargo.toml" file.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> 
> > diff --git a/ci/run-rust-checks.sh b/ci/run-rust-checks.sh
> > new file mode 100755
> > index 0000000000..082eb52f11
> > --- /dev/null
> > +++ b/ci/run-rust-checks.sh
> > @@ -0,0 +1,12 @@
> > +#!/bin/sh
> > +
> > +. ${0%/*}/lib.sh
> > +
> > +set +x
> > +
> > +if ! group "Check Rust formatting" cargo fmt --all --check
> > +then
> > +	RET=1
> > +fi
> > +
> > +exit $RET
> 
> Our ci/*.sh scripts usually rely on 'set -e' to catch failed commands.
> Either this script should follow that convention as well, or the
> commit message should justify the deviation from convention.

Ah, good point. The reason is that subsequent commits add more checks,
and I want to make sure that they all run even if previous checks
failed. It's otherwise annoying to fix a first set of errors surfaced by
the CI only to then notice that later checks also fail.

I'll mention this in the commit message.

Patrick

Re: [PATCH 1/6] ci: deduplicate calls to `apt-get update`

[Justin Tobler]

On 25/10/07 02:36PM, Patrick Steinhardt wrote:
> When installing dependencies we first check for the distribution that is
> in use and then we check for the specific job. In the first step we
> already install all dependencies required to build and test Git, whereas
> the second step installs a couple of additional dependencies that are
> only required to perform job-specific tasks.
> 
> In both steps we use `apt-get update` to update our repository sources.
> This is unecessary though: all platforms that use Aptitude would have
> already executed this command in the distro-specific step anyway.

The distro-specific setup always executes first and does make these call
redundant. Make sense.

Not related to this change, but at a glance it looks like this job
specific setup relies on using an Aptitude based distro. This does seem
slightly fragile if a job were to be configured with an unsupported
distro. Not anything we need to change here though.

-Justin

[PATCH v3 0/6] ci: improvements to our Rust infrastructure

[Patrick Steinhardt]

Hi,

this small patch series introduces some improvements for our Rust
infrastructure. Most importantly, it introduces a couple of static
analysis checks to verify consistent formatting, use Clippy for linting
and to verify our minimum supported Rust version.

Furthermore, this series also introduces support for building with Rust
enabled on Windows.

The series is built on top of 45547b60ac (Merge branch 'master' of
https://github.com/j6t/gitk, 2025-10-05) with ps/rust-balloon at
e425c40aa0 (ci: enable Rust for breaking-changes jobs, 2025-10-02) and
ps/gitlab-ci-windows-improvements at 3c4925c3f5 (t8020: fix test failure
due to indeterministic tag sorting, 2025-10-02) merged into it.

Changes in v3:
  - Clarify why scripts don't use `set -e` exclusively for error
    handling.
  - Link to v2: https://lore.kernel.org/r/20251008-b4-pks-ci-rust-v2-0-d556ee83c381@pks.im

Changes in v2:
  - Adjust comments for `encode_varint()` and `decode_varint()` based on
    brian's feedback.
  - Some small improvements to commit messages.
  - Not changed is the default column limit used by Rust. I think using
    the column limit of 100 used by the Rust ecosystem is sensible, but
    if there is a majority advocating for a limit of 80 I'll adapt this.
  - Link to v1: https://lore.kernel.org/r/20251007-b4-pks-ci-rust-v1-0-394502abe7ea@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (6):
      ci: deduplicate calls to `apt-get update`
      ci: check formatting of our Rust code
      rust/varint: add safety comments
      ci: check for common Rust mistakes via Clippy
      ci: verify minimum supported Rust version
      rust: support for Windows

 .github/workflows/main.yml | 15 +++++++++++++++
 .gitlab-ci.yml             | 13 ++++++++++++-
 Cargo.toml                 |  1 +
 Makefile                   | 14 ++++++++++++--
 ci/install-dependencies.sh | 17 +++++++++++++----
 ci/run-rust-checks.sh      | 22 ++++++++++++++++++++++
 meson.build                |  4 ++++
 src/cargo-meson.sh         | 11 +++++++++--
 src/varint.rs              | 15 +++++++++++++++
 9 files changed, 103 insertions(+), 9 deletions(-)

Range-diff versus v2:

1:  dc9d75f47c = 1:  cac74c6387 ci: deduplicate calls to `apt-get update`
2:  8537190491 ! 2:  6164bbd971 ci: check formatting of our Rust code
    @@ Commit message
         edition is automatically derived from the edition we have specified in
         our "Cargo.toml" file.
     
    +    The implemented script looks somewhat weird as we perfom manual error
    +    handling instead of using something like `set -e`. The intent here is
    +    that subsequent commits will add more checks, and we want to execute all
    +    of these checks regardless of whether or not a previous check failed.
    +
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## .github/workflows/main.yml ##
3:  8f7232e650 = 3:  1c87940646 rust/varint: add safety comments
4:  09810edff2 = 4:  0b09774307 ci: check for common Rust mistakes via Clippy
5:  bdb4e9df32 = 5:  93d6111ae7 ci: verify minimum supported Rust version
6:  40edae19a8 = 6:  3f58a9b9df rust: support for Windows

---
base-commit: 8c8e270f2aba359479c4c2b4ab3c62726e5dac9d
change-id: 20251007-b4-pks-ci-rust-8422e6a8196e

[PATCH v3 1/6] ci: deduplicate calls to `apt-get update`

[Patrick Steinhardt]

When installing dependencies we first check for the distribution that is
in use and then we check for the specific job. In the first step we
already install all dependencies required to build and test Git, whereas
the second step installs a couple of additional dependencies that are
only required to perform job-specific tasks.

In both steps we use `apt-get update` to update our repository sources.
This is unnecessary though: all platforms that use Aptitude would have
already executed this command in the distro-specific step anyway.

Drop the redundant calls.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 0d3aa496fc..645d035250 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -120,21 +120,17 @@ esac
 
 case "$jobname" in
 ClangFormat)
-	sudo apt-get -q update
 	sudo apt-get -q -y install clang-format
 	;;
 StaticAnalysis)
-	sudo apt-get -q update
 	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
 		libexpat-dev gettext make
 	;;
 sparse)
-	sudo apt-get -q update -q
 	sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \
 		libexpat-dev gettext zlib1g-dev sparse
 	;;
 Documentation)
-	sudo apt-get -q update
 	sudo apt-get -q -y install asciidoc xmlto docbook-xsl-ns make
 
 	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||

-- 
2.51.0.869.ge66316f041.dirty

[PATCH v3 2/6] ci: check formatting of our Rust code

[Patrick Steinhardt]

Introduce a CI check that verifies that our Rust code is well-formatted.
This check uses `cargo fmt`, which is a wrapper around rustfmt(1) that
executes formatting for all Rust source files. rustfmt(1) itself is the
de-facto standard for formatting code in the Rust ecosystem.

The rustfmt(1) tool allows to tweak the final format in theory. In
practice though, the Rust ecosystem has aligned on style "editions".
These editions only exist to ensure that any potential changes to the
style don't cause reformats to existing code bases. Other than that,
most Rust projects out there accept this default style of a specific
edition.

Let's do the same and use that default style. It may not be anyone's
favorite, but it is consistent and by making it part of our CI we also
enforce it right from the start.

Note that we don't have to pick a specific style edition here, as the
edition is automatically derived from the edition we have specified in
our "Cargo.toml" file.

The implemented script looks somewhat weird as we perfom manual error
handling instead of using something like `set -e`. The intent here is
that subsequent commits will add more checks, and we want to execute all
of these checks regardless of whether or not a previous check failed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 15 +++++++++++++++
 .gitlab-ci.yml             | 11 +++++++++++
 ci/install-dependencies.sh |  5 +++++
 ci/run-rust-checks.sh      | 12 ++++++++++++
 4 files changed, 43 insertions(+)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 393ea4d1cc..9e36b5c5e3 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -458,6 +458,21 @@ jobs:
     - run: ci/install-dependencies.sh
     - run: ci/run-static-analysis.sh
     - run: ci/check-directional-formatting.bash
+  rust-analysis:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
+    env:
+      jobname: RustAnalysis
+      CI_JOB_IMAGE: ubuntu:rolling
+    runs-on: ubuntu-latest
+    container: ubuntu:rolling
+    concurrency:
+      group: rust-analysis-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
+    steps:
+    - uses: actions/checkout@v4
+    - run: ci/install-dependencies.sh
+    - run: ci/run-rust-checks.sh
   sparse:
     needs: ci-config
     if: needs.ci-config.outputs.enabled == 'yes'
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index f7d57d1ee9..a47d839e39 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -212,6 +212,17 @@ static-analysis:
     - ./ci/run-static-analysis.sh
     - ./ci/check-directional-formatting.bash
 
+rust-analysis:
+  image: ubuntu:rolling
+  stage: analyze
+  needs: [ ]
+  variables:
+    jobname: RustAnalysis
+  before_script:
+    - ./ci/install-dependencies.sh
+  script:
+    - ./ci/run-rust-checks.sh
+
 check-whitespace:
   image: ubuntu:latest
   stage: analyze
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 645d035250..a24b07edff 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -126,6 +126,11 @@ StaticAnalysis)
 	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
 		libexpat-dev gettext make
 	;;
+RustAnalysis)
+	sudo apt-get -q -y install rustup
+	rustup default stable
+	rustup component add rustfmt
+	;;
 sparse)
 	sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \
 		libexpat-dev gettext zlib1g-dev sparse
diff --git a/ci/run-rust-checks.sh b/ci/run-rust-checks.sh
new file mode 100755
index 0000000000..082eb52f11
--- /dev/null
+++ b/ci/run-rust-checks.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+. ${0%/*}/lib.sh
+
+set +x
+
+if ! group "Check Rust formatting" cargo fmt --all --check
+then
+	RET=1
+fi
+
+exit $RET

-- 
2.51.0.869.ge66316f041.dirty

[PATCH v3 3/6] rust/varint: add safety comments

[Patrick Steinhardt]

The `decode_varint()` and `encode_varint()` functions in our Rust crate
are reimplementations of the respective C functions. As such, we are
naturally forced to use the same interface in both Rust and C, which
makes use of raw pointers. The consequence is that the code needs to be
marked as unsafe in Rust.

It is common practice in Rust to provide safety documentation for every
block that is marked as unsafe. This common practice is also enforced by
Clippy, Rust's static analyser. We don't have Clippy wired up yet, and
we could of course just disable this check. But we're about to wire it
up, and it is reasonable to always enforce documentation for unsafe
blocks.

Add such safety comments to already squelch those warnings now. While at
it, also document the functions' behaviour.

Helped-by: "brian m. carlson" <sandals@crustytoothpaste.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 src/varint.rs | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/varint.rs b/src/varint.rs
index 6e610bdd8e..06492dfc5e 100644
--- a/src/varint.rs
+++ b/src/varint.rs
@@ -1,3 +1,10 @@
+/// Decode the variable-length integer stored in `bufp` and return the decoded value.
+///
+/// Returns 0 in case the decoded integer would overflow u64::MAX.
+///
+/// # Safety
+///
+/// The buffer must be NUL-terminated to ensure safety.
 #[no_mangle]
 pub unsafe extern "C" fn decode_varint(bufp: *mut *const u8) -> u64 {
     let mut buf = *bufp;
@@ -22,6 +29,14 @@ pub unsafe extern "C" fn decode_varint(bufp: *mut *const u8) -> u64 {
     val
 }
 
+/// Encode `value` into `buf` as a variable-length integer unless `buf` is null.
+///
+/// Returns the number of bytes written, or, if `buf` is null, the number of bytes that would be
+/// written to encode the integer.
+///
+/// # Safety
+///
+/// `buf` must either be null or point to at least 16 bytes of memory.
 #[no_mangle]
 pub unsafe extern "C" fn encode_varint(value: u64, buf: *mut u8) -> u8 {
     let mut varint: [u8; 16] = [0; 16];

-- 
2.51.0.869.ge66316f041.dirty

[PATCH v3 4/6] ci: check for common Rust mistakes via Clippy

[Patrick Steinhardt]

Introduce a CI check that uses Clippy to perform checks for common
mistakes and suggested code improvements. Clippy is the official static
analyser of the Rust project and thus the de-facto standard.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 2 +-
 ci/run-rust-checks.sh      | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index a24b07edff..dcd22ddd95 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -129,7 +129,7 @@ StaticAnalysis)
 RustAnalysis)
 	sudo apt-get -q -y install rustup
 	rustup default stable
-	rustup component add rustfmt
+	rustup component add clippy rustfmt
 	;;
 sparse)
 	sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \
diff --git a/ci/run-rust-checks.sh b/ci/run-rust-checks.sh
index 082eb52f11..fb5ea8991b 100755
--- a/ci/run-rust-checks.sh
+++ b/ci/run-rust-checks.sh
@@ -9,4 +9,9 @@ then
 	RET=1
 fi
 
+if ! group "Check for common Rust mistakes" cargo clippy --all-targets --all-features -- -Dwarnings
+then
+	RET=1
+fi
+
 exit $RET

-- 
2.51.0.869.ge66316f041.dirty

[PATCH v3 5/6] ci: verify minimum supported Rust version

[Patrick Steinhardt]

In the current state of our Rust code base we don't really have any
requirements for the minimum supported Rust version yet, as we don't use
any features introduced by a recent version of Rust. Consequently, we
have decided that we want to aim for a rather old version and edition of
Rust, where the hope is that using an old version will make alternatives
like gccrs viable earlier for compiling Git.

But while we specify the Rust edition, we don't yet specify a Rust
version. And even if we did, the Rust version would only be enforced for
our own code, but not for any of our dependencies.

We don't yet have any dependencies at the current point in time. But
let's add some safeguards by specifying the minimum supported Rust
version and using cargo-msrv(1) to verify that this version can be
satisfied for all of our dependencies.

Note that we fix the version of cargo-msrv(1) at v0.18.1. This is the
latest release supported by Ubuntu's Rust version.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Cargo.toml                 | 1 +
 ci/install-dependencies.sh | 8 ++++++++
 ci/run-rust-checks.sh      | 5 +++++
 3 files changed, 14 insertions(+)

diff --git a/Cargo.toml b/Cargo.toml
index 45c9b34981..2f51bf5d5f 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -2,6 +2,7 @@
 name = "gitcore"
 version = "0.1.0"
 edition = "2018"
+rust-version = "1.49.0"
 
 [lib]
 crate-type = ["staticlib"]
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index dcd22ddd95..29e558bb9c 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -10,6 +10,8 @@ begin_group "Install dependencies"
 P4WHENCE=https://cdist2.perforce.com/perforce/r23.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
 JGITWHENCE=https://repo1.maven.org/maven2/org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh
+CARGO_MSRV_VERSION=0.18.4
+CARGO_MSRV_WHENCE=https://github.com/foresterre/cargo-msrv/releases/download/v$CARGO_MSRV_VERSION/cargo-msrv-x86_64-unknown-linux-musl-v$CARGO_MSRV_VERSION.tgz
 
 # Make sudo a no-op and execute the command directly when running as root.
 # While using sudo would be fine on most platforms when we are root already,
@@ -130,6 +132,12 @@ RustAnalysis)
 	sudo apt-get -q -y install rustup
 	rustup default stable
 	rustup component add clippy rustfmt
+
+	wget -q "$CARGO_MSRV_WHENCE" -O "cargo-msvc.tgz"
+	sudo mkdir -p "$CUSTOM_PATH"
+	sudo tar -xf "cargo-msvc.tgz" --strip-components=1 \
+		--directory "$CUSTOM_PATH" --wildcards "*/cargo-msrv"
+	sudo chmod a+x "$CUSTOM_PATH/cargo-msrv"
 	;;
 sparse)
 	sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \
diff --git a/ci/run-rust-checks.sh b/ci/run-rust-checks.sh
index fb5ea8991b..b5ad9e8dc6 100755
--- a/ci/run-rust-checks.sh
+++ b/ci/run-rust-checks.sh
@@ -14,4 +14,9 @@ then
 	RET=1
 fi
 
+if ! group "Check for minimum required Rust version" cargo msrv verify
+then
+	RET=1
+fi
+
 exit $RET

-- 
2.51.0.869.ge66316f041.dirty

[PATCH v3 6/6] rust: support for Windows

[Patrick Steinhardt]

The initial patch series that introduced Rust into the core of Git only
cared about macOS and Linux. This specifically leaves out Windows, which
indeed fails to build right now due to two issues:

  - The Rust runtime requires `GetUserProfileDirectoryW()`, but we don't
    link against "userenv.dll".

  - The path of the Rust library built on Windows is different than on
    most other systems systems.

Fix both of these issues to support Windows.

Note that this commit fixes the Meson-based job in GitHub's CI. Meson
auto-detects the availability of Rust, and as the Windows runner has
Rust installed by default it already enabled Rust support there. But due
to the above issues that job fails consistently.

Install Rust on GitLab CI, as well, to improve test coverage there.

Based-on-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Based-on-patch-by: Ezekiel Newren <ezekielnewren@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml     |  2 +-
 Makefile           | 14 ++++++++++++--
 meson.build        |  4 ++++
 src/cargo-meson.sh | 11 +++++++++--
 4 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index a47d839e39..b419a84e2c 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -161,7 +161,7 @@ test:mingw64:
     - saas-windows-medium-amd64
   before_script:
     - *windows_before_script
-    - choco install -y git meson ninja
+    - choco install -y git meson ninja rust-ms
     - Import-Module $env:ChocolateyInstall\helpers\chocolateyProfile.psm1
     - refreshenv
 
diff --git a/Makefile b/Makefile
index 7ea149598d..366fd173e7 100644
--- a/Makefile
+++ b/Makefile
@@ -929,10 +929,17 @@ TEST_SHELL_PATH = $(SHELL_PATH)
 LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 REFTABLE_LIB = reftable/libreftable.a
+
 ifdef DEBUG
-RUST_LIB = target/debug/libgitcore.a
+RUST_TARGET_DIR = target/debug
 else
-RUST_LIB = target/release/libgitcore.a
+RUST_TARGET_DIR = target/release
+endif
+
+ifeq ($(uname_S),Windows)
+RUST_LIB = $(RUST_TARGET_DIR)/gitcore.lib
+else
+RUST_LIB = $(RUST_TARGET_DIR)/libgitcore.a
 endif
 
 # xdiff and reftable libs may in turn depend on what is in libgit.a
@@ -1538,6 +1545,9 @@ ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)
 ifdef WITH_RUST
 BASIC_CFLAGS += -DWITH_RUST
 GITLIBS += $(RUST_LIB)
+ifeq ($(uname_S),Windows)
+EXTLIBS += -luserenv
+endif
 endif
 
 ifdef SANITIZE
diff --git a/meson.build b/meson.build
index ec55d6a5fd..a9c865b2af 100644
--- a/meson.build
+++ b/meson.build
@@ -1707,6 +1707,10 @@ rust_option = get_option('rust').disable_auto_if(not cargo.found())
 if rust_option.allowed()
   subdir('src')
   libgit_c_args += '-DWITH_RUST'
+
+  if host_machine.system() == 'windows'
+    libgit_dependencies += compiler.find_library('userenv')
+  endif
 else
   libgit_sources += [
     'varint.c',
diff --git a/src/cargo-meson.sh b/src/cargo-meson.sh
index 99400986d9..3998db0435 100755
--- a/src/cargo-meson.sh
+++ b/src/cargo-meson.sh
@@ -26,7 +26,14 @@ then
 	exit $RET
 fi
 
-if ! cmp "$BUILD_DIR/$BUILD_TYPE/libgitcore.a" "$BUILD_DIR/libgitcore.a" >/dev/null 2>&1
+case "$(cargo -vV | sed -s 's/^host: \(.*\)$/\1/')" in
+	*-windows-*)
+		LIBNAME=gitcore.lib;;
+	*)
+		LIBNAME=libgitcore.a;;
+esac
+
+if ! cmp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/libgitcore.a" >/dev/null 2>&1
 then
-	cp "$BUILD_DIR/$BUILD_TYPE/libgitcore.a" "$BUILD_DIR/libgitcore.a"
+	cp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/libgitcore.a"
 fi

-- 
2.51.0.869.ge66316f041.dirty

Re: [PATCH v3 0/6] ci: improvements to our Rust infrastructure

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> this small patch series introduces some improvements for our Rust
> infrastructure. Most importantly, it introduces a couple of static
> analysis checks to verify consistent formatting, use Clippy for linting
> and to verify our minimum supported Rust version.
>
> Furthermore, this series also introduces support for building with Rust
> enabled on Windows.

The updated structure is definite improvement (although I have no
idea on the Windows part, but presumably that has been written for
and tested on a real Windows box).

Will replace.

Re: [PATCH 2/6] ci: check formatting of our Rust code

[SZEDER Gábor]

On Thu, Oct 09, 2025 at 07:29:42AM +0200, Patrick Steinhardt wrote:
> On Wed, Oct 08, 2025 at 10:55:43PM +0200, SZEDER Gábor wrote:
> > On Tue, Oct 07, 2025 at 02:36:30PM +0200, Patrick Steinhardt wrote:
> > > diff --git a/ci/run-rust-checks.sh b/ci/run-rust-checks.sh
> > > new file mode 100755
> > > index 0000000000..082eb52f11
> > > --- /dev/null
> > > +++ b/ci/run-rust-checks.sh
> > > @@ -0,0 +1,12 @@
> > > +#!/bin/sh
> > > +
> > > +. ${0%/*}/lib.sh
> > > +
> > > +set +x
> > > +
> > > +if ! group "Check Rust formatting" cargo fmt --all --check
> > > +then
> > > +	RET=1
> > > +fi
> > > +
> > > +exit $RET
> > 
> > Our ci/*.sh scripts usually rely on 'set -e' to catch failed commands.
> > Either this script should follow that convention as well, or the
> > commit message should justify the deviation from convention.
> 
> Ah, good point. The reason is that subsequent commits add more checks,
> and I want to make sure that they all run even if previous checks
> failed. It's otherwise annoying to fix a first set of errors surfaced by
> the CI only to then notice that later checks also fail.

Well, OTOH, it is annoying when the error messages from a failed CI
run are not at the bottom of the logs.

Re: [PATCH 2/6] ci: check formatting of our Rust code

[SZEDER Gábor]

On Wed, Oct 08, 2025 at 08:34:22AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> ... but I also think we should take this
> >> opportunity to choose the Rust defaults for Rust.  C, Perl, and text
> >> formats like AsciiDoc do not have rigid defaults about indentation
> >> style, tabs vs. spaces, and line length; Rust does.  We wouldn't use
> >> tabs in Rust (the default is four spaces) because we use it everywhere
> >> else, so I think we should take the opportunity to use the Rust defaults
> >> here as well.
> >
> > I am also slightly leaning into the direction of sticking with Rust's
> > default of 100 characters. It's not substantially more than 80, should
> > be reasonable to accommodate for in most modern setups, and sticks with
> > what the remainder of the ecosystem is doing.
> >
> > So for now I'll leave it at 80 characters. But I don't feel strongly
> > about this, so if there is a majority in favor of 80 characters I'm
> > happy to adjust.
> 
> So the question is if we want consistency across files regardless of
> what language they are written in (i.e. 80-columns everywhere) or we
> treat our existing rules a "fallback rules" we have adopted while
> dealing with languages without their own strict rules, and use the
> default for a language with its own rule (i.e. whatever rustfmt
> wants is used for Rust, our own rules still apply to everything
> else)?

Consistency across files regardless of language was great, because I,
for one, prefer to use the same editor for all files regardless of
language.

I find 100 columns much worse than 80, because on my laptop I can put
three 80 columns editors next to each other (and still have a bit of
room to spare), but with 100 columns there is only room for two.

> I actually am fine with the latter myself.
> 
> If people strongly prefer, I also can be talked into adopting
> slightly wider limit for our fallback rules for everything else, but
> that is probably a separate discussion.  It is a bit unfriendly move
> against folks with aging eyeballs like myself, though.
> 
> Thanks.
> 
> 

Re: [PATCH v3 6/6] rust: support for Windows

[Ezekiel Newren]

This is a retrospective review. I completely missed this patch series,
and only noticed its existence after it was merged into master. The
core problem is that these changes assume that windows builds only
ever use the MSVC compiler, but that's not true.

On Wed, Oct 15, 2025 at 12:04 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> The initial patch series that introduced Rust into the core of Git only
> cared about macOS and Linux. This specifically leaves out Windows, which
> indeed fails to build right now due to two issues:
>
>   - The Rust runtime requires `GetUserProfileDirectoryW()`, but we don't
>     link against "userenv.dll".
>
>   - The path of the Rust library built on Windows is different than on
>     most other systems systems.

That is true, but the build systems also need to check if the C
compiler is gnu or msvc. Also you used the word "systems" twice.

> diff --git a/Makefile b/Makefile
> index 7ea149598d..366fd173e7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -929,10 +929,17 @@ TEST_SHELL_PATH = $(SHELL_PATH)
>  LIB_FILE = libgit.a
>  XDIFF_LIB = xdiff/lib.a
>  REFTABLE_LIB = reftable/libreftable.a
> +
>  ifdef DEBUG
> -RUST_LIB = target/debug/libgitcore.a
> +RUST_TARGET_DIR = target/debug
>  else
> -RUST_LIB = target/release/libgitcore.a
> +RUST_TARGET_DIR = target/release
> +endif
> +
> +ifeq ($(uname_S),Windows)
> +RUST_LIB = $(RUST_TARGET_DIR)/gitcore.lib
> +else
> +RUST_LIB = $(RUST_TARGET_DIR)/libgitcore.a
>  endif
>
>  # xdiff and reftable libs may in turn depend on what is in libgit.a
> @@ -1538,6 +1545,9 @@ ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)
>  ifdef WITH_RUST
>  BASIC_CFLAGS += -DWITH_RUST
>  GITLIBS += $(RUST_LIB)
> +ifeq ($(uname_S),Windows)
> +EXTLIBS += -luserenv
> +endif
>  endif
>  ifdef SANITIZE

This is not fully correct for Makefile. If Windows AND using MSVC ->
gitcore.lib. However this bug doesn't show up because github ci
doesn't test the windows+msvc+makefile combo.

> diff --git a/meson.build b/meson.build
> index ec55d6a5fd..a9c865b2af 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1707,6 +1707,10 @@ rust_option = get_option('rust').disable_auto_if(not cargo.found())
>  if rust_option.allowed()
>    subdir('src')
>    libgit_c_args += '-DWITH_RUST'
> +
> +  if host_machine.system() == 'windows'
> +    libgit_dependencies += compiler.find_library('userenv')
> +  endif
>  else
>    libgit_sources += [
>      'varint.c',

Same issue as above, but it doesn't show up because the github ci
doesn't test the windows+gnu+meson combo.

> diff --git a/src/cargo-meson.sh b/src/cargo-meson.sh
> index 99400986d9..3998db0435 100755
> --- a/src/cargo-meson.sh
> +++ b/src/cargo-meson.sh
> @@ -26,7 +26,14 @@ then
>         exit $RET
>  fi
>
> -if ! cmp "$BUILD_DIR/$BUILD_TYPE/libgitcore.a" "$BUILD_DIR/libgitcore.a" >/dev/null 2>&1
> +case "$(cargo -vV | sed -s 's/^host: \(.*\)$/\1/')" in
> +       *-windows-*)
> +               LIBNAME=gitcore.lib;;
> +       *)
> +               LIBNAME=libgitcore.a;;
> +esac
> +
> +if ! cmp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/libgitcore.a" >/dev/null 2>&1
>  then
> -       cp "$BUILD_DIR/$BUILD_TYPE/libgitcore.a" "$BUILD_DIR/libgitcore.a"
> +       cp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/libgitcore.a"
>  fi

Same issue again. This needs to test for windows AND msvc.

Re: [PATCH v3 6/6] rust: support for Windows

[Johannes Schindelin]

Hi Ezekiel,

On Thu, 20 Nov 2025, Ezekiel Newren wrote:

> This is a retrospective review. I completely missed this patch series,
> and only noticed its existence after it was merged into master. The
> core problem is that these changes assume that windows builds only
> ever use the MSVC compiler, but that's not true.

Correct. I had actually worked on a solution to this in Git for Windows,
but due to time constraints (after factoring in the usual time tax, other
priorities dictated that I wouldn't have time to see it through) I hadn't
had time to contribute it, let alone engage in reviewing Patrick's patches
(I had actually not even seen them until I had written the patch and
verified that it fixed the issue).

Here is my patch (with proper handling of MSVC, but obviously it no longer
applies without conflicts):
https://github.com/git-for-windows/git/commit/0949ff2ad5d1d085b10c63029c65293416732851

-- snipsnap --
From 0949ff2ad5d1d085b10c63029c65293416732851 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 24 Oct 2025 14:49:22 +0200
Subject: [PATCH] meson(cargo): support Windows again

For over a year, Git has been moved to a more modern build system than
it had before (GNU make, or on Windows optionally CMake). Naturally,
this new system breaks Windows support left and right.

For example, c184795fc0e (meson: add infrastructure to build internal
Rust library, 2025-10-02) added support for building a Rust library, and
it fails when Visual C is configured as compiler.

This is the reason that the `win+Meson` job of Git's `master` branch
fails for the past 16 days, i.e. the latest 9 pushes of the `master`
branch as of time of writing. The symptom is:

  [697/905] Generating src/git_rs with a custom command
  FAILED: [code=1] src/libgitcore.a "C:\Program Files\Git\bin\sh.exe" "D:/a/git/git/src/cargo-meson.sh" "D:/a/git/git" "D:/a/git/git/build/src" "--release"
  cp: cannot stat 'D:/a/git/git/build/src/release/libgitcore.a': No such file or directory

The reason is that Visual C's output is called `gitcore.lib`, not
`libgitcore.a`. Let's special-case Visual C and use the correct filename
in all cases.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 src/cargo-meson.sh |  7 +++++--
 src/meson.build    | 10 +++++++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/cargo-meson.sh b/src/cargo-meson.sh
index 99400986d93..c14a08c592d 100755
--- a/src/cargo-meson.sh
+++ b/src/cargo-meson.sh
@@ -5,6 +5,9 @@ then
 	exit 1
 fi
 
+target="$1"
+shift
+
 SOURCE_DIR="$1"
 BUILD_DIR="$2"
 BUILD_TYPE=debug
@@ -26,7 +29,7 @@ then
 	exit $RET
 fi
 
-if ! cmp "$BUILD_DIR/$BUILD_TYPE/libgitcore.a" "$BUILD_DIR/libgitcore.a" >/dev/null 2>&1
+if ! cmp "$BUILD_DIR/$BUILD_TYPE/$target" "$BUILD_DIR/$target" >/dev/null 2>&1
 then
-	cp "$BUILD_DIR/$BUILD_TYPE/libgitcore.a" "$BUILD_DIR/libgitcore.a"
+	cp "$BUILD_DIR/$BUILD_TYPE/$target" "$BUILD_DIR/$target"
 fi
diff --git a/src/meson.build b/src/meson.build
index 25b9ad5a147..b2473c46994 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -3,6 +3,13 @@ libgit_rs_sources = [
   'varint.rs',
 ]
 
+# The exact file name depends on the compiler
+if meson.get_compiler('c').get_id() == 'msvc'
+  target = 'gitcore.lib'
+else
+  target = 'libgitcore.a'
+endif
+
 # Unfortunately we must use a wrapper command to move the output file into the
 # current build directory. This can fixed once `cargo build --artifact-dir`
 # stabilizes. See https://github.com/rust-lang/cargo/issues/6790 for that
@@ -10,6 +17,7 @@ libgit_rs_sources = [
 cargo_command = [
   shell,
   meson.current_source_dir() / 'cargo-meson.sh',
+  target,
   meson.project_source_root(),
   meson.current_build_dir(),
 ]
@@ -21,7 +29,7 @@ libgit_rs = custom_target('git_rs',
   input: libgit_rs_sources + [
     meson.project_source_root() / 'Cargo.toml',
   ],
-  output: 'libgitcore.a',
+  output: target,
   command: cargo_command,
 )
 libgit_dependencies += declare_dependency(link_with: libgit_rs)
-- 
2.51.1.windows.1

Re: [PATCH v3 6/6] rust: support for Windows

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Here is my patch (with proper handling of MSVC, but obviously it no longer
> applies without conflicts):

Thanks.  Here is my attempt to make it apply to 'master'.

It seems to pass win+Meson build & test for 'master'.

  https://github.com/git/git/actions/runs/19583133885

But curiously, the tip of 'master' has been happy without this fix,
and it does not help when brian's SHA-256 interop topic merged
further on top, but I didn't look any further.

--- >8 ---
Subject: [PATCH] meson(cargo): Visual C produces gitcore.lib, not libgitcore.a

On Windows, when Visual C compiler is used to compile, the resulting
library that is created is called gitcore.lib instead of libgitcore.a
library archive.

Johannes sent a fix in <dc753c0e-eb93-948c-55f7-bb0e91772c83@gmx.de>
that was based on an older code base, which I attempted to forward
port it to apply to today's codebase.

Based-on-the-patch-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 src/cargo-meson.sh | 14 ++++----------
 src/meson.build    | 11 ++++++++++-
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/cargo-meson.sh b/src/cargo-meson.sh
index 3998db0435..4a46f731d8 100755
--- a/src/cargo-meson.sh
+++ b/src/cargo-meson.sh
@@ -7,9 +7,10 @@ fi
 
 SOURCE_DIR="$1"
 BUILD_DIR="$2"
+LIBNAME="$3"
 BUILD_TYPE=debug
 
-shift 2
+shift 3
 
 for arg
 do
@@ -26,14 +27,7 @@ then
 	exit $RET
 fi
 
-case "$(cargo -vV | sed -s 's/^host: \(.*\)$/\1/')" in
-	*-windows-*)
-		LIBNAME=gitcore.lib;;
-	*)
-		LIBNAME=libgitcore.a;;
-esac
-
-if ! cmp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/libgitcore.a" >/dev/null 2>&1
+if ! cmp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/$LIBNAME" >/dev/null 2>&1
 then
-	cp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/libgitcore.a"
+	cp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/$LIBNAME"
 fi
diff --git a/src/meson.build b/src/meson.build
index 25b9ad5a14..f37f0a5f58 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -3,6 +3,14 @@ libgit_rs_sources = [
   'varint.rs',
 ]
 
+# The exact file name depends on the compiler
+if meson.get_compiler('c').get_id() == 'msvc'
+  libname = 'gitcore.lib'
+else
+  libname = 'libgitcore.a'
+endif
+
+
 # Unfortunately we must use a wrapper command to move the output file into the
 # current build directory. This can fixed once `cargo build --artifact-dir`
 # stabilizes. See https://github.com/rust-lang/cargo/issues/6790 for that
@@ -12,6 +20,7 @@ cargo_command = [
   meson.current_source_dir() / 'cargo-meson.sh',
   meson.project_source_root(),
   meson.current_build_dir(),
+  libname,
 ]
 if get_option('buildtype') == 'release'
   cargo_command += '--release'
@@ -21,7 +30,7 @@ libgit_rs = custom_target('git_rs',
   input: libgit_rs_sources + [
     meson.project_source_root() / 'Cargo.toml',
   ],
-  output: 'libgitcore.a',
+  output: libname,
   command: cargo_command,
 )
 libgit_dependencies += declare_dependency(link_with: libgit_rs)
-- 
2.52.0-168-gcf2c56d51e