From: Eric Sunshine <sunshine@sunshineco.com>
Date: Fri, 06 Sep 2024 22:39:17 GMT
Subject: Re: [PATCH v3 2/6] libgit-sys: introduce Rust wrapper for libgit.a
Message-ID: <CAPig+cQbPCWS4LVF7cdw_DD6K8dsrLTCmmhqkmGZpz1CO93vRQ@mail.gmail.com>
In-Reply-To: <20240906222116.270196-2-calvinwan@google.com>

On Fri, Sep 6, 2024 at 6:21 PM Calvin Wan <calvinwan@google.com> wrote:
> From: Josh Steadmon <steadmon@google.com>

This is curious...

> Co-authored-by: Kyle Lippincott <spectral@google.com>
> Co-authored-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Kyle Lippincott <spectral@google.com>

... since there is no mention of Josh here.

> diff --git a/contrib/libgit-rs/libgit-sys/Cargo.toml b/contrib/libgit-rs/libgit-sys/Cargo.toml
> @@ -0,0 +1,12 @@
> +[package]
> +name = "libgit-sys"
> [...]
> +[dependencies]
> +libz-sys = "1.1.19"
> \ No newline at end of file

Let's give this file a proper line terminator.

> diff --git a/contrib/libgit-rs/libgit-sys/build.rs b/contrib/libgit-rs/libgit-sys/build.rs
> @@ -0,0 +1,31 @@
> +pub fn main() -> std::io::Result<()> {
> +    let crate_root = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
> +    let git_root = crate_root.join("../../..");
> +    let dst = PathBuf::from(env::var_os("OUT_DIR").unwrap());
> +
> +    let make_output = std::process::Command::new("make")

Providing a mechanism for people to override this hardcoded spelling
of "make" could be another item for your NEEDSWORK list; in
particular, I'm thinking about platforms on which GNU "make" is
installed as "gmake".

> diff --git a/contrib/libgit-rs/libgit-sys/public_symbol_export.c b/contrib/libgit-rs/libgit-sys/public_symbol_export.c
> @@ -0,0 +1,20 @@
> +// Shim to publicly export Git symbols. These must be renamed so that the
> +// original symbols can be hidden. Renaming these with a "libgit_" prefix also
> +// avoid conflicts with other libraries such as libgit2.

s/avoid/avoids/