On 2024.09.10 08:42, Patrick Steinhardt wrote:
> On Fri, Sep 06, 2024 at 10:21:13PM +0000, Calvin Wan wrote:
> > diff --git a/contrib/libgit-rs/libgit-sys/public_symbol_export.c b/contrib/libgit-rs/libgit-sys/public_symbol_export.c
> > index 39c27d9c1a..65d1620d28 100644
> > --- a/contrib/libgit-rs/libgit-sys/public_symbol_export.c
> > +++ b/contrib/libgit-rs/libgit-sys/public_symbol_export.c
> > @@ -2,11 +2,37 @@
> > // original symbols can be hidden. Renaming these with a "libgit_" prefix also
> > // avoid conflicts with other libraries such as libgit2.
> >
> > +#include "git-compat-util.h"
> > #include "contrib/libgit-rs/libgit-sys/public_symbol_export.h"
> > +#include "common-init.h"
> > +#include "config.h"
> > +#include "setup.h"
> > #include "version.h"
> >
> > +extern struct repository *the_repository;
> > +
> > #pragma GCC visibility push(default)
> >
> > +const char *libgit_setup_git_directory(void)
> > +{
> > + return setup_git_directory();
> > +}
> > +
> > +int libgit_config_get_int(const char *key, int *dest)
> > +{
> > + return repo_config_get_int(the_repository, key, dest);
> > +}
> > +
> > +void libgit_init_git(const char **argv)
> > +{
> > + init_git(argv);
> > +}
> > +
> > +int libgit_parse_maybe_bool(const char *val)
> > +{
> > + return git_parse_maybe_bool(val);
> > +}
> > +
>
> I don't quite get why we expose functionality that is inherently not
> libified. Exposing the current state to library users certainly does not
> feel right to me, doubly so because `the_repository` is deprecated and
> will eventually go away. So we already know that we'll have to break the
> API here once that has happened. I'd rather want to see that introducing
> the library makes us double down on providing properly encapsulated
> interfaces from hereon.
>
> Also, we already have ways to initialize a repository and read their
> config without relying on `the_repository`. So shouldn't we expose that
> instead?
>
> Patrick
Specifically in this case, yeah, we should have started with the
libified version. This is an artifact due to the way we are figuring out
C/Rust interop as we go, and it was easier to convert it this way
without making the Rust side care about handling repository pointers.
But you're right, and I'll fix this soon for V4.
In general though, we're treating the initial version of libgit-rs as a
proof-of-concept that we can call from JJ into Git without blowing
things up. We might not always have the option of exposing
fully-libified code paths, and for our $DAYJOB purposes, we may have to
deal with non-ideal interfaces for the early versions. However, we do
want to use this as a way to motivate development of better, libified
APIs when we find real-world use cases for them.
We've said before (even before we started libgit-rs) that we're not
going to be able to make guarantees about early libified APIs, because
we're learning as we go along. I don't want to use that as an excuse to
cover up bad design, but I do want to be upfront that we can't commit to
fully libifying any given code path before we expose it through the shim
library or through libgit-rs.
