From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 28 Jan 2025 04:56:52 GMT
Subject: Re: [PATCH v7 4/4] libgit: add higher-level libgit crate
Message-ID: <xmqqzfjbo7sb.fsf@gitster.g>
In-Reply-To: <88425bb0b1b90cf6b3d2d5a1af57e46c7a03ad39.1738023208.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

> -.PHONY: libgit-sys
> +.PHONY: libgit-sys libgit-rs
>  libgit-sys:
>  	$(QUIET)(\
>  		cd contrib/libgit-sys && \
>  		cargo build \
>  	)
> +libgit-rs:
> +	$(QUIET)(\
> +		cd contrib/libgit-rs && \
> +		cargo build \
> +	)
>  ifdef INCLUDE_LIBGIT_RS
> -all:: libgit-sys
> +all:: libgit-sys libgit-rs
>  endif

I somehow would have expected this part of the patch to do

	libgit-sys libgit-rs:
		$(QUIET)( \
			cd contrib/$@ && cargo build \
		)

but the above longhand is fine.

> +    /// Load the value for the given key and attempt to parse it as an i32. Dies with a fatal error
> +    /// if the value cannot be parsed. Returns None if the key is not present.
> +    pub fn get_int(&mut self, key: &str) -> Option<i32> {
> +        let key = CString::new(key).expect("Couldn't convert to CString");
> +        let mut val: c_int = 0;
> +        unsafe {
> +            if libgit_configset_get_int(self.0, key.as_ptr(), &mut val as *mut c_int) != 0 {
> +                return None;
> +            }
> +        }
> +
> +        Some(val.into())
> +    }

Nice.

I was wondering why libgit_configset_get_int() in [3/4] does not do
better than just wrapping the raw C interface, which returns the
error/success status separately from the value we parsed.  With this
higher-layer wrapper around it, the interface becomes a bit more
like a higher-level language.

Thanks, will queue.