Re: [PATCH v3 5/6] libgit: add higher-level libgit crate

From: Calvin Wan <calvinwan@google.com>
Date: Sep 10, 2024, 19:04 UTC
Message-ID: <CAFySSZBECCQafaLEv80WoK6SMovwC97-tf9gh_btPc+8OuP4NA@mail.gmail.com>
In-Reply-To: <xmqqcylcpnah.fsf@gitster.g>

On Mon, Sep 9, 2024 at 1:47 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> +struct libgit_config_set *libgit_configset_alloc(void)
> >> +{
> >> +    return git_configset_alloc();
> >> +}
> >
> > git_configset_alloc() returns "struct config_set *" while this thing
> > returns an incompatible pointer.
> >
> > Sent out an outdated version or something?  This wouldn't have
> > passed even a compile test, I suspect.
>
> The "shim" layer should hide the details of interfacing to the git
> proper from callers, as well as it should hide the callers' from the
> git proper.  So if you really want to hide "struct config_set" from
> your library callers, you may need to do something like the
> attached, perhaps?  At least this does pass compilation test.

You're correct that this is supposed to be the shim layer, where callers aren't supposed to be able to directly interface with the config_set struct. From Rust's perspective, all pointers that come back from C code are effectively void* so the naming doesn't make a functional difference for our use case (and consequently these warnings did not show up from the build.rs script nor did the tests fail). However, I agree that the public interface should pass the compilation test and your approach does that -- will reroll with those changes and I believe that we should also fix the build.rs so that warnings also show up during cargo build.