| Message ID | cover.1723242556.git.steadmon@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce cgit-rs, a Rust wrapper around libgit.a | expand |
Josh Steadmon <steadmon@google.com> writes: > This series provides two small Rust wrapper libraries around parts of > Git: "cgit-sys", which exposes a few functions from libgit.a, and > "cgit", which provides a more Rust-friendly interface to some of those > functions. In addition to included unit tests, at $DAYJOB we have tested > building JJ[1] with our library and used it to replace some of the > libgit2-rs uses. Thanks, queued.
Still the same name for v2? Cmon.
On 2024-08-10 15:15, Jason A. Donenfeld wrote:
> Still the same name for v2? Cmon.
Yeah, I was also surprised to see that. This _isn't_ cgit.
On Sun, Aug 11, 2024 at 1:27 PM Dragan Simic <dsimic@manjaro.org> wrote: > On 2024-08-10 15:15, Jason A. Donenfeld wrote: > > Still the same name for v2? Cmon. > > Yeah, I was also surprised to see that. This _isn't_ cgit. Josh addressed this point in the v2 cover letter by saying: Known NEEDSWORK: ... * Bikeshed on the name
On 2024-08-12 01:03, Eric Sunshine wrote: > On Sun, Aug 11, 2024 at 1:27 PM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-08-10 15:15, Jason A. Donenfeld wrote: >> > Still the same name for v2? Cmon. >> >> Yeah, I was also surprised to see that. This _isn't_ cgit. > > Josh addressed this point in the v2 cover letter by saying: > > Known NEEDSWORK: > ... > * Bikeshed on the name But didn't Junio already say that the v2 of this series will be merged as-is? [1] That's what actually made me surprised and confused. [1] https://lore.kernel.org/git/xmqqfrrd9slb.fsf@gitster.g/
On Sun, Aug 11, 2024 at 7:23 PM Dragan Simic <dsimic@manjaro.org> wrote: > On 2024-08-12 01:03, Eric Sunshine wrote: > > Josh addressed this point in the v2 cover letter by saying: > > > > Known NEEDSWORK: > > ... > > * Bikeshed on the name > > But didn't Junio already say that the v2 of this series will be merged > as-is? [1] That's what actually made me surprised and confused. > > [1] https://lore.kernel.org/git/xmqqfrrd9slb.fsf@gitster.g/ That only means that Junio placed the topic in his "seen" branch, however, no promises about ultimate acceptance are attached to topics in that branch. Quoting from Junio's "What's Cooking" emails (such as [*]): Commits prefixed with '-' are only in 'seen', and aren't considered "accepted" at all and may be annotated with an URL to a message that raises issues but they are no means exhaustive. A topic without enough support may be discarded after a long period of no activity (of course they can be resubmit when new interests arise). [*]: https://lore.kernel.org/git/xmqqo762frkz.fsf@gitster.g/
On 2024-08-12 01:33, Eric Sunshine wrote: > On Sun, Aug 11, 2024 at 7:23 PM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-08-12 01:03, Eric Sunshine wrote: >> > Josh addressed this point in the v2 cover letter by saying: >> > >> > Known NEEDSWORK: >> > ... >> > * Bikeshed on the name >> >> But didn't Junio already say that the v2 of this series will be merged >> as-is? [1] That's what actually made me surprised and confused. >> >> [1] https://lore.kernel.org/git/xmqqfrrd9slb.fsf@gitster.g/ > > That only means that Junio placed the topic in his "seen" branch, > however, no promises about ultimate acceptance are attached to topics > in that branch. Quoting from Junio's "What's Cooking" emails (such as > [*]): > > Commits prefixed with '-' are only in 'seen', and aren't > considered "accepted" at all and may be annotated with an URL to a > message that raises issues but they are no means exhaustive. A > topic without enough support may be discarded after a long period > of no activity (of course they can be resubmit when new interests > arise). > > [*]: https://lore.kernel.org/git/xmqqo762frkz.fsf@gitster.g/ Ah, good, thanks for the clarification. It's all fine then, as far as I'm concerned.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Sun, Aug 11, 2024 at 1:27 PM Dragan Simic <dsimic@manjaro.org> wrote: >> On 2024-08-10 15:15, Jason A. Donenfeld wrote: >> > Still the same name for v2? Cmon. >> >> Yeah, I was also surprised to see that. This _isn't_ cgit. > > Josh addressed this point in the v2 cover letter by saying: > > Known NEEDSWORK: > ... > * Bikeshed on the name I do not quite consider it as as "addressed this point" to just slap a NEEDSWORK label and doing nothing else, though. The original iteration had this: * bikeshedding on the name (yes, really). There is an active, unrelated CGit project [4] that we only recently became aware of. We originally took the name "cgit" because at $DAYJOB we sometimes refer to git.git as "cgit" to distinguish it from jgit [5]. and then now they as well as reviewers all have seen the tentative cgit name, saw the reaction it caused, and now know that not just potentially confusing other project _exists_, but it does matter. Reviewers already have spent some time on suggesting that "git" part should not be "c"git, as well as "rs" part may better be "sys", etc.?. There should be _some_ response, even if it does not yet propose a new name. If it acknowledged that the time and knowledge reviewers gave the topic were appreciated, e.g., "The proposers of this topic saw THIS point and THAT point as a input that we WILL need to consider when we decide on the name. We acknowledge that the name "cgit-rs" is not ideal and needs to be changed. But we haven't reached any concrete alternative name yet, so this round still uses the same name", I'd call that "addressed this point", though. But just a dismissing "Bikeshed on the name", as if they do not care to be mistaken as saying "those who complain about the name are only bikeshedding and not worth listening to"? We should do better than that.
On Mon, Aug 12, 2024 at 4:15 AM Junio C Hamano <gitster@pobox.com> wrote: > The original iteration had this: > > * bikeshedding on the name (yes, really). There is an active, unrelated > CGit project [4] that we only recently became aware of. We originally > took the name "cgit" because at $DAYJOB we sometimes refer to git.git > as "cgit" to distinguish it from jgit [5]. A tangent: Speaking of external/other projects, I don't think we've seen an explanation yet as to why this Rust wrapper is proposed as a `contrib/` item of Git itself, as opposed to being a separate project. I can only think of two possible reasons why they might want it in the Git project itself... (1) Easier access to the library portions of Git ("libgit") since that portion of the code is not otherwise published as a standalone library. However, a workable alternative would be for the Rust wrapper to carry its own "vendored"[1] copy of Git. This would also ensure more reliable builds since they wouldn't have to worry about the "libgit" API changing from under them, and can adjust for "libgit" API changes when they manually pull in a new vendored copy. Hence, I'm not convinced that this is a valid reason to carry the Rust wrapper in Git. (2) Perhaps the intention is that this Rust wrapper work will allow Rust to be used within Git itself[3]? If that's the case, then `contrib/` seems the wrong resting place for this code. On the other hand, as a standalone project, a big benefit is that the Rust wrapper could have its own release cadence distinct from Git, which would likely be very beneficial since it is such a young (indeed, nascent) library; it is likely that the maintainers will want to release early and often at this stage. [1]: Other Rust projects carry vendored copies of projects upon which they rely. For instance, the "native_tls" crate has a vendored copy of OpenSSL[2]. [2]: https://docs.rs/native-tls/latest/native_tls/#cargo-features [3]: https://lore.kernel.org/git/CABPp-BFWsWCGogqQ=haMsS4OhOdSwc3frcAxa6soQR5ORTceOA@mail.gmail.com/
On 2024-08-12 10:15, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> On Sun, Aug 11, 2024 at 1:27 PM Dragan Simic <dsimic@manjaro.org> >> wrote: >>> On 2024-08-10 15:15, Jason A. Donenfeld wrote: >>> > Still the same name for v2? Cmon. >>> >>> Yeah, I was also surprised to see that. This _isn't_ cgit. >> >> Josh addressed this point in the v2 cover letter by saying: >> >> Known NEEDSWORK: >> ... >> * Bikeshed on the name > > I do not quite consider it as as "addressed this point" to just slap > a NEEDSWORK label and doing nothing else, though. > > The original iteration had this: > > * bikeshedding on the name (yes, really). There is an active, > unrelated > CGit project [4] that we only recently became aware of. We > originally > took the name "cgit" because at $DAYJOB we sometimes refer to > git.git > as "cgit" to distinguish it from jgit [5]. > > and then now they as well as reviewers all have seen the tentative > cgit name, saw the reaction it caused, and now know that not just > potentially confusing other project _exists_, but it does matter. > > Reviewers already have spent some time on suggesting that "git" part > should not be "c"git, as well as "rs" part may better be "sys", > etc.?. There should be _some_ response, even if it does not yet > propose a new name. > > If it acknowledged that the time and knowledge reviewers gave the > topic were appreciated, e.g., "The proposers of this topic saw THIS > point and THAT point as a input that we WILL need to consider when > we decide on the name. We acknowledge that the name "cgit-rs" is > not ideal and needs to be changed. But we haven't reached any > concrete alternative name yet, so this round still uses the same > name", I'd call that "addressed this point", though. > > But just a dismissing "Bikeshed on the name", as if they do not care > to be mistaken as saying "those who complain about the name are only > bikeshedding and not worth listening to"? > > We should do better than that. I really appreciate and support your response, Junio. Brushing it off originally as "bikeshedding" did leave a sour taste, because it presented the conflicting naming as a non-issue.
Eric Sunshine <sunshine@sunshineco.com> writes: > A tangent: Speaking of external/other projects, I don't think we've > seen an explanation yet as to why this Rust wrapper is proposed as a > `contrib/` item of Git itself, as opposed to being a separate project. > > I can only think of two possible reasons why they might want it in the > Git project itself... > > (1) Easier access to the library portions of Git ("libgit") since that > portion of the code is not otherwise published as a standalone > library. This is not a good reason at all, if we look at what (the real) cgit project does ;-), which is to bind us as their submodule. > (2) Perhaps the intention is that this Rust wrapper work will allow > Rust to be used within Git itself[3]? If that's the case, then > `contrib/` seems the wrong resting place for this code. The contrib/ hierarchy is a mixed bag, and we may want to clean it up by Git 3.0 happens. - We used to put things that one-way depend on Git that are (1) useless by themselves, if Git did not exist, and (2) Git can do fine without them in the contrib/ hierarchy. The primary reason for doing so was because the Git was young and relatively unknown, and our community was small. Even for an add-on that may be at most "nice to have" from Git's point of view, it made some sense to give these add-ons wider exposure by simply being bundled with Git. We stopped doing that after Git ecosystem have matured enough and encouraged them to move either up (i.e. prove that Git cannot really do fine without it because the add-on is so useful, and become the first class part of Git) or out (i.e. it may be a fine add-on, but there are other similar add-ons that aim to achieve the same, similar, or overlapping goal---unlike the nascent era of Git, it should be able to become an independent project to compete fairly with others on its merit alone). - We have some stuff like completion and prompt scripts that have proven that the end-user experience cannot be "fine without", but haven't moved out of "contrib/", mostly by inercia. - We also have things that are meant to help Git developers in the hierarchy (Cocci rules come to mind). This one could fall into that category, but I am not sure. > On the other hand, as a standalone project, a big benefit is that the > Rust wrapper could have its own release cadence distinct from Git, > which would likely be very beneficial since it is such a young > (indeed, nascent) library; it is likely that the maintainers will want > to release early and often at this stage. That's cgit approach. If something is useless if Git did not exist, but Git does not need it at all to function, that may be a sensible thing to do. If we plan to eventually be able to rewrite the logic for say history traveral or specifying "set of commits" in such a way that is more involved than just "those that can be reached from these tip commits excluding the ones that can be reached from those bottom commits" (in other words, "A..B C..D" that is not "^A B ^C D"), and find it unpleasant to do in C and want to employ Rust, for example, we'd certainly need it somewhere in _our_ tree. Perhaps by the time it happens we might have an extension/dll mechanism in such a way that those who do not want (or cannot build) certain extensions can opt out and these "optional" things would by convention live somewhere other than the main source code live, but even then, contrib/ sounds like a wrong place and we'd do so perhaps in extra/ or something. > [1]: Other Rust projects carry vendored copies of projects upon which > they rely. For instance, the "native_tls" crate has a vendored copy of > OpenSSL[2]. > > [2]: https://docs.rs/native-tls/latest/native_tls/#cargo-features > > [3]: https://lore.kernel.org/git/CABPp-BFWsWCGogqQ=haMsS4OhOdSwc3frcAxa6soQR5ORTceOA@mail.gmail.com/
On 2024.08.12 01:15, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > > On Sun, Aug 11, 2024 at 1:27 PM Dragan Simic <dsimic@manjaro.org> wrote: > >> On 2024-08-10 15:15, Jason A. Donenfeld wrote: > >> > Still the same name for v2? Cmon. > >> > >> Yeah, I was also surprised to see that. This _isn't_ cgit. > > > > Josh addressed this point in the v2 cover letter by saying: > > > > Known NEEDSWORK: > > ... > > * Bikeshed on the name > > I do not quite consider it as as "addressed this point" to just slap > a NEEDSWORK label and doing nothing else, though. > > The original iteration had this: > > * bikeshedding on the name (yes, really). There is an active, unrelated > CGit project [4] that we only recently became aware of. We originally > took the name "cgit" because at $DAYJOB we sometimes refer to git.git > as "cgit" to distinguish it from jgit [5]. > > and then now they as well as reviewers all have seen the tentative > cgit name, saw the reaction it caused, and now know that not just > potentially confusing other project _exists_, but it does matter. > > Reviewers already have spent some time on suggesting that "git" part > should not be "c"git, as well as "rs" part may better be "sys", > etc.?. There should be _some_ response, even if it does not yet > propose a new name. > > If it acknowledged that the time and knowledge reviewers gave the > topic were appreciated, e.g., "The proposers of this topic saw THIS > point and THAT point as a input that we WILL need to consider when > we decide on the name. We acknowledge that the name "cgit-rs" is > not ideal and needs to be changed. But we haven't reached any > concrete alternative name yet, so this round still uses the same > name", I'd call that "addressed this point", though. > > But just a dismissing "Bikeshed on the name", as if they do not care > to be mistaken as saying "those who complain about the name are only > bikeshedding and not worth listening to"? > > We should do better than that. I am quite surprised that people felt this was dismissive. So to be clear: yes, we need a new name before this lands in next. I thought that leaving that as a known needs-work item was sufficient to call that out, but I guess I was wrong. As far as proposed names go, I am not super happy with either "libgit" or "libgit3", because that seems confusing when we consider there are also a separate libgit2 and libgit2-rs projects. I know that we're using "libgit_" as a prefix for visible symbols at the moment, but I plan on renaming those once we have settled on the actual library name. I was avoiding addressing this topic in the hopes that folks might suggest additional name options, but as it stands right now, I'm leaning towards just "git-sys" for the low-level wrapper crate, "git" for the Rust API, and "git-rs" for the directory in contrib/ which will contain the crates.
On 2024.08.12 05:03, Eric Sunshine wrote: > On Mon, Aug 12, 2024 at 4:15 AM Junio C Hamano <gitster@pobox.com> wrote: > > The original iteration had this: > > > > * bikeshedding on the name (yes, really). There is an active, unrelated > > CGit project [4] that we only recently became aware of. We originally > > took the name "cgit" because at $DAYJOB we sometimes refer to git.git > > as "cgit" to distinguish it from jgit [5]. > > A tangent: Speaking of external/other projects, I don't think we've > seen an explanation yet as to why this Rust wrapper is proposed as a > `contrib/` item of Git itself, as opposed to being a separate project. > > I can only think of two possible reasons why they might want it in the > Git project itself... > > (1) Easier access to the library portions of Git ("libgit") since that > portion of the code is not otherwise published as a standalone > library. However, a workable alternative would be for the Rust wrapper > to carry its own "vendored"[1] copy of Git. This would also ensure > more reliable builds since they wouldn't have to worry about the > "libgit" API changing from under them, and can adjust for "libgit" API > changes when they manually pull in a new vendored copy. Hence, I'm not > convinced that this is a valid reason to carry the Rust wrapper in > Git. > > (2) Perhaps the intention is that this Rust wrapper work will allow > Rust to be used within Git itself[3]? If that's the case, then > `contrib/` seems the wrong resting place for this code. Neither, actually. We hope that by keeping the crate definition in the main Git repo and by allowing developers to optionally run them by default as part of CI and `make test`, we can catch breaking bugs as early as possible. We're also hoping that we'll get more attention from interested developers by staying in the main repo rather than in a separate project. > On the other hand, as a standalone project, a big benefit is that the > Rust wrapper could have its own release cadence distinct from Git, > which would likely be very beneficial since it is such a young > (indeed, nascent) library; it is likely that the maintainers will want > to release early and often at this stage. AFAIK, the crate releases don't have to be strictly tied to Git releases. In theory we could choose to publish updates to the crate whenever "master" or any other branch is updated; whether or not that is a good idea is a separate discussion. > [1]: Other Rust projects carry vendored copies of projects upon which > they rely. For instance, the "native_tls" crate has a vendored copy of > OpenSSL[2]. > > [2]: https://docs.rs/native-tls/latest/native_tls/#cargo-features > > [3]: https://lore.kernel.org/git/CABPp-BFWsWCGogqQ=haMsS4OhOdSwc3frcAxa6soQR5ORTceOA@mail.gmail.com/
Josh Steadmon <steadmon@google.com> writes: > I was avoiding addressing this topic in the hopes that folks might > suggest additional name options,... I've seen libgit-rs (or was it libgit-rust?) suggested already and found them reasonable. If libgit2 folks do not use libgit_ prefix, I do not see any reason for us to avoid it, either. Thanks.
Josh Steadmon <steadmon@google.com> writes: >> But just a dismissing "Bikeshed on the name", as if they do not care >> to be mistaken as saying "those who complain about the name are only >> bikeshedding and not worth listening to"? >> >> We should do better than that. > > I am quite surprised that people felt this was dismissive. So to be > clear: yes, we need a new name before this lands in next. I thought that > leaving that as a known needs-work item was sufficient to call that out, > but I guess I was wrong. Yes, I had a similar initial reaction, but I guess that comes primarily from the fact that we both misjudged how "cgit" is already deeply established name that refers to something other than this project.
On 2024-08-12 at 09:03:57, Eric Sunshine wrote: > On Mon, Aug 12, 2024 at 4:15 AM Junio C Hamano <gitster@pobox.com> wrote: > > The original iteration had this: > > > > * bikeshedding on the name (yes, really). There is an active, unrelated > > CGit project [4] that we only recently became aware of. We originally > > took the name "cgit" because at $DAYJOB we sometimes refer to git.git > > as "cgit" to distinguish it from jgit [5]. > > A tangent: Speaking of external/other projects, I don't think we've > seen an explanation yet as to why this Rust wrapper is proposed as a > `contrib/` item of Git itself, as opposed to being a separate project. > > I can only think of two possible reasons why they might want it in the > Git project itself... > > (1) Easier access to the library portions of Git ("libgit") since that > portion of the code is not otherwise published as a standalone > library. However, a workable alternative would be for the Rust wrapper > to carry its own "vendored"[1] copy of Git. This would also ensure > more reliable builds since they wouldn't have to worry about the > "libgit" API changing from under them, and can adjust for "libgit" API > changes when they manually pull in a new vendored copy. Hence, I'm not > convinced that this is a valid reason to carry the Rust wrapper in > Git. Please don't vendor packages like this. This means that every time there's a security vulnerability in Git, even if we ship a public libgit and even if the security impact doesn't affect libgit, every security scanning tool will demand that the crate be updated immediately. This wastes colossal amounts of work and it's hostile to distros as well, who will have to repackage the source to remove the vendoring. At work, I maintain a codebase using Nokogiri, which is a Ruby gem that vendors libxml2 (which has an absolutely abysmal security record), and I cannot tell you how much useless work I perform updating this gem because of security scanners screaming about the vendored libxml2, even though it's completely possible to use the system library. > (2) Perhaps the intention is that this Rust wrapper work will allow > Rust to be used within Git itself[3]? If that's the case, then > `contrib/` seems the wrong resting place for this code. I think contrib is the right place for now. We may in the future want to include some Rust code in the codebase in some context, and this is a good way for us to experiment with it on an interim basis and discover whether that's a viable approach for us. Perhaps it is, and perhaps it is not, but we'll have built the experience to know for certain. I could imagine (and this is hypothetical and speculative) that we might allow some sorts of performance optimizations and optional features in Rust, but require core functionality be in C until Rust is better supported on some platforms. That might be a nice way to improve things like multithreading, which Rust excels at, for example. As an example, I'd like to write a random-access tree parser[0] in Rust (such as for "dev:README.md"), since our current approach performs terribly on large trees (it's linear instead of logarithmic). However, I'm not willing to write that code in C because it's a giant landmine of memory unsafety and I think it's too likely to be buggy in C. [0] I am aware that due to the structure of trees, this is not _always_ possible to do, but it is possible in _many_ cases, and optimistically trying and falling back to the old approach may still be faster.
On Mon, Aug 12, 2024 at 02:32:12PM -0700, Josh Steadmon wrote: > On 2024.08.12 05:03, Eric Sunshine wrote: > > On Mon, Aug 12, 2024 at 4:15 AM Junio C Hamano <gitster@pobox.com> wrote: > > > The original iteration had this: > > > > > > * bikeshedding on the name (yes, really). There is an active, unrelated > > > CGit project [4] that we only recently became aware of. We originally > > > took the name "cgit" because at $DAYJOB we sometimes refer to git.git > > > as "cgit" to distinguish it from jgit [5]. > > > > A tangent: Speaking of external/other projects, I don't think we've > > seen an explanation yet as to why this Rust wrapper is proposed as a > > `contrib/` item of Git itself, as opposed to being a separate project. > > > > I can only think of two possible reasons why they might want it in the > > Git project itself... > > > > (1) Easier access to the library portions of Git ("libgit") since that > > portion of the code is not otherwise published as a standalone > > library. However, a workable alternative would be for the Rust wrapper > > to carry its own "vendored"[1] copy of Git. This would also ensure > > more reliable builds since they wouldn't have to worry about the > > "libgit" API changing from under them, and can adjust for "libgit" API > > changes when they manually pull in a new vendored copy. Hence, I'm not > > convinced that this is a valid reason to carry the Rust wrapper in > > Git. > > > > (2) Perhaps the intention is that this Rust wrapper work will allow > > Rust to be used within Git itself[3]? If that's the case, then > > `contrib/` seems the wrong resting place for this code. > > Neither, actually. We hope that by keeping the crate definition in the > main Git repo and by allowing developers to optionally run them by > default as part of CI and `make test`, we can catch breaking bugs as > early as possible. We're also hoping that we'll get more attention from > interested developers by staying in the main repo rather than in a > separate project. That to me raises an important question: who is the one fixing breakage? Hypothetically speaking, if I continue with my refactoring spree to drop `the_repository`, do I also have to fix the Rust bindings that I break as a consequence? Patrick
On 2024-08-16 at 11:39:24, Patrick Steinhardt wrote: > That to me raises an important question: who is the one fixing breakage? > Hypothetically speaking, if I continue with my refactoring spree to drop > `the_repository`, do I also have to fix the Rust bindings that I break > as a consequence? If we're testing it in CI, then you are responsible for not breaking it, even if you don't use it, just as I am responsible for not breaking Windows, even though I don't use that platform. That's typically been the policy here and elsewhere. That being said, there are two things I'd like to point out. First, your work to drop the implicit `the_repository` is very useful, because Rust code implicitly requires thread safety as part of the language, and you will be making the dependency on the object explicit so Rust can guard against concurrent access. I expect that you will end up breaking very few endpoints with those series because we can't really expose endpoints with implicit global thread-unsafe state (such as `the_repository`) in Rust anyway except as `unsafe`. Second, I and others will probably be happy to help if you have questions or need assistance fixing bindings. I've done a decent amount of work in Rust, including mostly porting a service at $DAYJOB from C to Rust (the one that serves Git requests, no less), so I'm familiar with a lot of those kinds of problems, as I'm sure others are on the list as well.
On Fri, Aug 16, 2024 at 5:38 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > On 2024-08-16 at 11:39:24, Patrick Steinhardt wrote: > > That to me raises an important question: who is the one fixing breakage? > > Hypothetically speaking, if I continue with my refactoring spree to drop > > `the_repository`, do I also have to fix the Rust bindings that I break > > as a consequence? I share Patrick's concern, and it's one of the reasons I posed the question in the first place about why these Rust bindings are being proposed for Git's "contrib/" rather than as a standalone project. > If we're testing it in CI, then you are responsible for not breaking it, > even if you don't use it, just as I am responsible for not breaking > Windows, even though I don't use that platform. That's typically been > the policy here and elsewhere. This is an apples-and-oranges comparison, isn't it? Although the majority of Git developers may be Unix folk, Git itself has a reasonably large Windows user population, so breaking it on Windows carries some potentially real consequences (i.e. hurting the immediate user community). On the other hand, the Rust bindings under discussion are (1) as yet effectively hypothetical, (2) will almost certainly be relied upon by a tiny number of other projects, and (3) impact only developers, not users, of Git-related tooling. As such, it seems far too early to be placing the onus on *all* Git developers to have to worry about the Rust bindings. If, at some point in the future, the Rust bindings gain significance or become indispensable to the Git project itself, then the onus might well be warranted, but at this early stage, it seems unreasonable. A more friendly approach to the overall Git developer community would be for those interested in the Rust bindings to maintain them. (The case with the CMake project files is similar. In the end, the people interested in utilizing them took on the responsibility of maintaining them.)
Eric Sunshine <sunshine@sunshineco.com> writes: > (The case with the CMake project files is similar. In the end, the > people interested in utilizing them took on the responsibility of > maintaining them.) That is a good analogy. We would all be slowed down if you have to adjust CMake stuff (or autoconf stuff for that matter) when the build procedure changes you made only to Makefile (because you only have a ready access and expertise on the "make" proper), but in practice, niche things that nobody cares about (and remember, if no stakeholder is there to volunteer, you cannot force others to care about your niche) will bitrot enough to cause somebody to care and get fixed. Another possibility is that it proves that nobody cares, and we just drop that part. Either outcome is fine in the end. I think the Rust thing will proceed the same way. At least, I am sensing that this time there are already motivated to lend help from outside the group of folks that are proposing this change.
This series provides two small Rust wrapper libraries around parts of Git: "cgit-sys", which exposes a few functions from libgit.a, and "cgit", which provides a more Rust-friendly interface to some of those functions. In addition to included unit tests, at $DAYJOB we have tested building JJ[1] with our library and used it to replace some of the libgit2-rs uses. [1] https://github.com/martinvonz/jj Please find V1 cover letter here: https://lore.kernel.org/git/cover.1723054623.git.steadmon@google.com/ Changes in V2: * Split out the exposed C APIs into a cgit-sys module, and build nicer Rust interfaces on top of that in cgit-rs * Replace the proof-of-concept cgit-info binary with unit tests * In addition to splitting common_exit() into a new file, also move the initialization code in main() to a new init_git() function in its own file, and include this in libgit.a. This allows us to drop V1 patch #2 [which added a wrapper around initialize_repo()] * Remove libc dependency (by declaring an extern "C" free function, not sure if this is the best approach) * Add git_configset_clear_and_free to also support the above * Use "std::env::home_dir" instead of the "home" crate due to desired behavior on Windows. "std::env::home_dir" is deprecated, but is only used in unit tests which will need to be rewritten anyway to provide a testdata config. * Use recommended empty array + PhantomData approach instead of empty enums for wrapping opaque C structs/pointers * Don't force CC=clang in build.rs * Simplify conversion of PathBufs to Strings in build.rs * Don't unnecessarily expose git_attr__true and git_attr__false in public_symbol_export.c. That fixes `make sparse`. * Clippy and rustfmt cleanups throughout * Whitespace fix Known NEEDSWORK: * Support older Rust versions * Investigate alternative methods of managing symbol visibility. * Figure out symbol versioning * Bikeshed on the name * Makefile cleanup, particularly adding config.mak options that developers can set to run Rust builds and tests by default * Automate the process of exporting additional functions in cgit-sys (possibly with a wrapper script around bindgen) * Provide testdata configs for unit tests * Rethink the Rust-y ConfigSet API, particularly for Path vs. &str Calvin Wan (2): contrib/cgit-rs: add repo initialization and config access contrib/cgit-rs: add a subset of configset wrappers Josh Steadmon (3): common-main: split init and exit code into new files contrib/cgit-rs: introduce Rust wrapper for libgit.a config: add git_configset_alloc() and git_configset_clear_and_free() .gitignore | 2 + Makefile | 15 +++ common-exit.c | 26 ++++ common-init.c | 63 ++++++++++ common-init.h | 6 + common-main.c | 83 +------------ config.c | 11 ++ config.h | 10 ++ contrib/cgit-rs/Cargo.lock | 14 +++ contrib/cgit-rs/Cargo.toml | 10 ++ contrib/cgit-rs/cgit-sys/Cargo.lock | 7 ++ contrib/cgit-rs/cgit-sys/Cargo.toml | 9 ++ contrib/cgit-rs/cgit-sys/README.md | 15 +++ contrib/cgit-rs/cgit-sys/build.rs | 32 +++++ .../cgit-rs/cgit-sys/public_symbol_export.c | 76 ++++++++++++ .../cgit-rs/cgit-sys/public_symbol_export.h | 28 +++++ contrib/cgit-rs/cgit-sys/src/lib.rs | 112 ++++++++++++++++++ contrib/cgit-rs/src/lib.rs | 82 +++++++++++++ 18 files changed, 520 insertions(+), 81 deletions(-) create mode 100644 common-exit.c create mode 100644 common-init.c create mode 100644 common-init.h create mode 100644 contrib/cgit-rs/Cargo.lock create mode 100644 contrib/cgit-rs/Cargo.toml create mode 100644 contrib/cgit-rs/cgit-sys/Cargo.lock create mode 100644 contrib/cgit-rs/cgit-sys/Cargo.toml create mode 100644 contrib/cgit-rs/cgit-sys/README.md create mode 100644 contrib/cgit-rs/cgit-sys/build.rs create mode 100644 contrib/cgit-rs/cgit-sys/public_symbol_export.c create mode 100644 contrib/cgit-rs/cgit-sys/public_symbol_export.h create mode 100644 contrib/cgit-rs/cgit-sys/src/lib.rs create mode 100644 contrib/cgit-rs/src/lib.rs Range-diff against v1: 1: 78c2aa2ef9 ! 1: 800b37d16b common-main: split common_exit() into a new file @@ Metadata Author: Josh Steadmon <steadmon@google.com> ## Commit message ## - common-main: split common_exit() into a new file + common-main: split init and exit code into new files Currently, object files in libgit.a reference common_exit(), which is contained in common-main.o. However, common-main.o also includes main(), @@ Commit message conflicts. Now we are trying to make libgit.a more self-contained, so hopefully we can revisit this approach. + Additionally, move the initialization code out of main() into a new + init_git() function in its own file. Include this in libgit.a as well, + so that external users can share our setup code without calling our + main(). + [1] https://lore.kernel.org/git/Yp+wjCPhqieTku3X@google.com/ [2] https://lore.kernel.org/git/20230517-unit-tests-v2-v2-1-21b5b60f4b32@google.com/ - Signed-off-by: Josh Steadmon <steadmon@google.com> - ## Makefile ## @@ Makefile: LIB_OBJS += combine-diff.o LIB_OBJS += commit-reach.o LIB_OBJS += commit.o +LIB_OBJS += common-exit.o ++LIB_OBJS += common-init.o LIB_OBJS += compat/nonblock.o LIB_OBJS += compat/obstack.o LIB_OBJS += compat/terminal.o @@ common-exit.c (new) + return code; +} + ## common-init.c (new) ## +@@ ++#define USE_THE_REPOSITORY_VARIABLE ++ ++#include "git-compat-util.h" ++#include "common-init.h" ++#include "exec-cmd.h" ++#include "gettext.h" ++#include "attr.h" ++#include "repository.h" ++#include "setup.h" ++#include "strbuf.h" ++#include "trace2.h" ++ ++/* ++ * Many parts of Git have subprograms communicate via pipe, expect the ++ * upstream of a pipe to die with SIGPIPE when the downstream of a ++ * pipe does not need to read all that is written. Some third-party ++ * programs that ignore or block SIGPIPE for their own reason forget ++ * to restore SIGPIPE handling to the default before spawning Git and ++ * break this carefully orchestrated machinery. ++ * ++ * Restore the way SIGPIPE is handled to default, which is what we ++ * expect. ++ */ ++static void restore_sigpipe_to_default(void) ++{ ++ sigset_t unblock; ++ ++ sigemptyset(&unblock); ++ sigaddset(&unblock, SIGPIPE); ++ sigprocmask(SIG_UNBLOCK, &unblock, NULL); ++ signal(SIGPIPE, SIG_DFL); ++} ++ ++void init_git(const char **argv) ++{ ++ struct strbuf tmp = STRBUF_INIT; ++ ++ trace2_initialize_clock(); ++ ++ /* ++ * Always open file descriptors 0/1/2 to avoid clobbering files ++ * in die(). It also avoids messing up when the pipes are dup'ed ++ * onto stdin/stdout/stderr in the child processes we spawn. ++ */ ++ sanitize_stdfds(); ++ restore_sigpipe_to_default(); ++ ++ git_resolve_executable_dir(argv[0]); ++ ++ setlocale(LC_CTYPE, ""); ++ git_setup_gettext(); ++ ++ initialize_repository(the_repository); ++ ++ attr_start(); ++ ++ trace2_initialize(); ++ trace2_cmd_start(argv); ++ trace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP); ++ ++ if (!strbuf_getcwd(&tmp)) ++ tmp_original_cwd = strbuf_detach(&tmp, NULL); ++} + + ## common-init.h (new) ## +@@ ++#ifndef COMMON_INIT_H ++#define COMMON_INIT_H ++ ++void init_git(const char **argv); ++ ++#endif /* COMMON_INIT_H */ + ## common-main.c ## -@@ common-main.c: int main(int argc, const char **argv) +@@ +-#define USE_THE_REPOSITORY_VARIABLE +- + #include "git-compat-util.h" +-#include "exec-cmd.h" +-#include "gettext.h" +-#include "attr.h" +-#include "repository.h" +-#include "setup.h" +-#include "strbuf.h" +-#include "trace2.h" +- +-/* +- * Many parts of Git have subprograms communicate via pipe, expect the +- * upstream of a pipe to die with SIGPIPE when the downstream of a +- * pipe does not need to read all that is written. Some third-party +- * programs that ignore or block SIGPIPE for their own reason forget +- * to restore SIGPIPE handling to the default before spawning Git and +- * break this carefully orchestrated machinery. +- * +- * Restore the way SIGPIPE is handled to default, which is what we +- * expect. +- */ +-static void restore_sigpipe_to_default(void) +-{ +- sigset_t unblock; +- +- sigemptyset(&unblock); +- sigaddset(&unblock, SIGPIPE); +- sigprocmask(SIG_UNBLOCK, &unblock, NULL); +- signal(SIGPIPE, SIG_DFL); +-} ++#include "common-init.h" + + int main(int argc, const char **argv) + { + int result; +- struct strbuf tmp = STRBUF_INIT; +- +- trace2_initialize_clock(); +- +- /* +- * Always open file descriptors 0/1/2 to avoid clobbering files +- * in die(). It also avoids messing up when the pipes are dup'ed +- * onto stdin/stdout/stderr in the child processes we spawn. +- */ +- sanitize_stdfds(); +- restore_sigpipe_to_default(); +- +- git_resolve_executable_dir(argv[0]); +- +- setlocale(LC_CTYPE, ""); +- git_setup_gettext(); +- +- initialize_repository(the_repository); +- +- attr_start(); +- +- trace2_initialize(); +- trace2_cmd_start(argv); +- trace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP); +- +- if (!strbuf_getcwd(&tmp)) +- tmp_original_cwd = strbuf_detach(&tmp, NULL); + ++ init_git(argv); + result = cmd_main(argc, argv); + /* Not exit(3), but a wrapper calling our common_exit() */ exit(result); } 2: 5f2e816cf6 < -: ---------- repository: add initialize_repo wrapper without pointer 3: 9a846c17c8 ! 2: 3589d2d6a2 contrib/cgit-rs: introduce Rust wrapper for libgit.a @@ Commit message Co-authored-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> Signed-off-by: Kyle Lippincott <spectral@google.com> - Signed-off-by: Josh Steadmon <steadmon@google.com> - ## .gitignore ## @@ .gitignore: Release/ /git.VC.db *.dSYM /contrib/buildsystems/out -+/contrib/cgit-rs/target ++/contrib/cgit-rs/cgit-sys/target ## Makefile ## @@ Makefile: CURL_CONFIG = curl-config @@ Makefile: OBJECTS += $(XDIFF_OBJS) OBJECTS += $(FUZZ_OBJS) OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS) OBJECTS += $(UNIT_TEST_OBJS) -+OBJECTS += contrib/cgit-rs/public_symbol_export.o ++OBJECTS += contrib/cgit-rs/cgit-sys/public_symbol_export.o ifndef NO_CURL OBJECTS += http.o http-walker.o remote-curl.o @@ Makefile: clean: profile-clean coverage-clean cocciclean $(RM) $(htmldocs).tar.gz $(manpages).tar.gz $(MAKE) -C Documentation/ clean $(RM) Documentation/GIT-EXCLUDED-PROGRAMS -+ $(RM) -r contrib/cgit-rs/target ++ $(RM) -r contrib/cgit-rs/cgit-sys/target ifndef NO_PERL $(RM) -r perl/build/ endif @@ Makefile: $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o \ unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X $(MAKE) -C t/ unit-tests + -+contrib/cgit-rs/partial_symbol_export.o: contrib/cgit-rs/public_symbol_export.o libgit.a reftable/libreftable.a xdiff/lib.a ++contrib/cgit-rs/cgit-sys/partial_symbol_export.o: contrib/cgit-rs/cgit-sys/public_symbol_export.o libgit.a reftable/libreftable.a xdiff/lib.a + $(LD) -r $^ -o $@ + -+contrib/cgit-rs/hidden_symbol_export.o: contrib/cgit-rs/partial_symbol_export.o ++contrib/cgit-rs/cgit-sys/hidden_symbol_export.o: contrib/cgit-rs/cgit-sys/partial_symbol_export.o + $(OBJCOPY) --localize-hidden $^ $@ + -+contrib/cgit-rs/libcgit.a: contrib/cgit-rs/hidden_symbol_export.o ++contrib/cgit-rs/cgit-sys/libcgit.a: contrib/cgit-rs/cgit-sys/hidden_symbol_export.o + $(AR) $(ARFLAGS) $@ $^ - ## contrib/cgit-rs/Cargo.lock (new) ## + ## contrib/cgit-rs/cgit-sys/Cargo.lock (new) ## @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 3 + +[[package]] -+name = "cgit" ++name = "cgit-sys" +version = "0.1.0" -+dependencies = [ -+ "libc", -+] -+ -+[[package]] -+name = "libc" -+version = "0.2.155" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "97b3888a4aecf77e811145cadf6eef5901f4782c53886191b2f693f24761847c" - ## contrib/cgit-rs/Cargo.toml (new) ## + ## contrib/cgit-rs/cgit-sys/Cargo.toml (new) ## @@ +[package] -+name = "cgit" ++name = "cgit-sys" +version = "0.1.0" +edition = "2021" +build = "build.rs" +links = "git" + -+[[bin]] -+name = "cgit-test" -+path = "src/main.rs" -+ +[lib] +path = "src/lib.rs" -+ -+[dependencies] -+libc = "0.2.155" - ## contrib/cgit-rs/README.md (new) ## + ## contrib/cgit-rs/cgit-sys/README.md (new) ## @@ +# cgit-info + @@ contrib/cgit-rs/README.md (new) +Assuming you don't make any changes to the Git source, you can just work from +`contrib/cgit-rs` and use `cargo build` or `cargo run` as usual. - ## contrib/cgit-rs/build.rs (new) ## + ## contrib/cgit-rs/cgit-sys/build.rs (new) ## @@ +use std::env; +use std::path::PathBuf; + +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 git_root = crate_root.join("../../.."); + let dst = PathBuf::from(env::var_os("OUT_DIR").unwrap()); + + let make_output = std::process::Command::new("make") + .env_remove("PROFILE") + .current_dir(git_root.clone()) -+ .args(&[ -+ "CC=clang", ++ .args([ + "CFLAGS=-fvisibility=hidden", -+ "contrib/cgit-rs/libcgit.a" ++ "contrib/cgit-rs/cgit-sys/libcgit.a", + ]) + .output() + .expect("Make failed to run"); + if !make_output.status.success() { + panic!( -+ "Make failed:\n stdout = {}\n stderr = {}\n", -+ String::from_utf8(make_output.stdout).unwrap(), -+ String::from_utf8(make_output.stderr).unwrap() ++ "Make failed:\n stdout = {}\n stderr = {}\n", ++ String::from_utf8(make_output.stdout).unwrap(), ++ String::from_utf8(make_output.stderr).unwrap() + ); + } + std::fs::copy(crate_root.join("libcgit.a"), dst.join("libcgit.a"))?; -+ println!("cargo::rustc-link-search=native={}", dst.into_os_string().into_string().unwrap()); ++ println!("cargo::rustc-link-search=native={}", dst.display()); + println!("cargo::rustc-link-lib=cgit"); + println!("cargo::rustc-link-lib=z"); -+ println!("cargo::rerun-if-changed={}", git_root.into_os_string().into_string().unwrap()); ++ println!("cargo::rerun-if-changed={}", git_root.display()); + + Ok(()) +} - ## contrib/cgit-rs/public_symbol_export.c (new) ## + ## contrib/cgit-rs/cgit-sys/public_symbol_export.c (new) ## @@ +// 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. + -+#include "contrib/cgit-rs/public_symbol_export.h" ++#include "contrib/cgit-rs/cgit-sys/public_symbol_export.h" +#include "version.h" + +#pragma GCC visibility push(default) @@ contrib/cgit-rs/public_symbol_export.c (new) + +#pragma GCC visibility pop - ## contrib/cgit-rs/public_symbol_export.h (new) ## + ## contrib/cgit-rs/cgit-sys/public_symbol_export.h (new) ## @@ +#ifndef PUBLIC_SYMBOL_EXPORT_H +#define PUBLIC_SYMBOL_EXPORT_H @@ contrib/cgit-rs/public_symbol_export.h (new) + +#endif /* PUBLIC_SYMBOL_EXPORT_H */ - ## contrib/cgit-rs/src/lib.rs (new) ## + ## contrib/cgit-rs/cgit-sys/src/lib.rs (new) ## @@ -+use libc::c_char; ++use std::ffi::c_char; + +extern "C" { + // From version.c + pub fn libgit_user_agent() -> *const c_char; + pub fn libgit_user_agent_sanitized() -> *const c_char; +} - - ## contrib/cgit-rs/src/main.rs (new) ## -@@ -+use std::ffi::CStr; -+ -+fn main() { -+ println!("Let's print some strings provided by Git"); -+ let c_buf = unsafe { cgit::libgit_user_agent() }; -+ let c_str = unsafe { CStr::from_ptr(c_buf) }; -+ println!("git_user_agent() = {:?}", c_str); -+ println!("git_user_agent_sanitized() = {:?}", -+ unsafe { CStr::from_ptr(cgit::libgit_user_agent_sanitized()) }); ++ ++#[cfg(test)] ++mod tests { ++ use std::ffi::CStr; ++ ++ use super::*; ++ ++ #[test] ++ fn user_agent_starts_with_git() { ++ let c_str = unsafe { CStr::from_ptr(libgit_user_agent()) }; ++ let agent = c_str ++ .to_str() ++ .expect("User agent contains invalid UTF-8 data"); ++ assert!( ++ agent.starts_with("git/"), ++ r#"Expected user agent to start with "git/", got: {}"#, ++ agent ++ ); ++ } ++ ++ #[test] ++ fn sanitized_user_agent_starts_with_git() { ++ let c_str = unsafe { CStr::from_ptr(libgit_user_agent_sanitized()) }; ++ let agent = c_str ++ .to_str() ++ .expect("Sanitized user agent contains invalid UTF-8 data"); ++ assert!( ++ agent.starts_with("git/"), ++ r#"Expected user agent to start with "git/", got: {}"#, ++ agent ++ ); ++ } +} 4: b84a8210a0 ! 3: 527780f816 contrib/cgit-rs: add repo initialization and config access @@ Commit message contrib/cgit-rs: add repo initialization and config access Co-authored-by: Calvin Wan <calvinwan@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> - Signed-off-by: Josh Steadmon <steadmon@google.com> - ## contrib/cgit-rs/public_symbol_export.c ## + ## contrib/cgit-rs/cgit-sys/public_symbol_export.c ## @@ // 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/cgit-rs/public_symbol_export.h" -+#include "attr.h" + #include "contrib/cgit-rs/cgit-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) @@ contrib/cgit-rs/public_symbol_export.c + +int libgit_config_get_int(const char *key, int *dest) +{ -+ return git_config_get_int(key, dest); ++ return repo_config_get_int(the_repository, key, dest); +} + -+void libgit_initialize_the_repository(void) ++void libgit_init_git(const char **argv) +{ -+ initialize_the_repository(); ++ init_git(argv); +} + +int libgit_parse_maybe_bool(const char *val) @@ contrib/cgit-rs/public_symbol_export.c const char *libgit_user_agent(void) { return git_user_agent(); -@@ contrib/cgit-rs/public_symbol_export.c: const char *libgit_user_agent_sanitized(void) - return git_user_agent_sanitized(); - } - -+const char *libgit_attr__true = git_attr__true; -+const char *libgit_attr__false = git_attr__false; -+ - #pragma GCC visibility pop - ## contrib/cgit-rs/public_symbol_export.h ## + ## contrib/cgit-rs/cgit-sys/public_symbol_export.h ## @@ #ifndef PUBLIC_SYMBOL_EXPORT_H #define PUBLIC_SYMBOL_EXPORT_H @@ contrib/cgit-rs/public_symbol_export.h + +int libgit_config_get_int(const char *key, int *dest); + -+void libgit_initialize_the_repository(void); ++void libgit_init_git(const char **argv); + +int libgit_parse_maybe_bool(const char *val); + @@ contrib/cgit-rs/public_symbol_export.h const char *libgit_user_agent_sanitized(void); - ## contrib/cgit-rs/src/lib.rs ## + ## contrib/cgit-rs/cgit-sys/src/lib.rs ## @@ --use libc::c_char; -+use libc::{c_char, c_int}; +-use std::ffi::c_char; ++use std::ffi::{c_char, c_int}; extern "C" { + pub fn libgit_setup_git_directory() -> *const c_char; + + // From config.c -+ pub fn libgit_config_get_int(key: *const c_char, dest: *mut c_int) ->c_int; ++ pub fn libgit_config_get_int(key: *const c_char, dest: *mut c_int) -> c_int; + -+ // From repository.c -+ pub fn libgit_initialize_the_repository(); ++ // From common-init.c ++ pub fn libgit_init_git(argv: *const *const c_char); + + // From parse.c + pub fn libgit_parse_maybe_bool(val: *const c_char) -> c_int; @@ contrib/cgit-rs/src/lib.rs // From version.c pub fn libgit_user_agent() -> *const c_char; pub fn libgit_user_agent_sanitized() -> *const c_char; - - ## contrib/cgit-rs/src/main.rs ## -@@ --use std::ffi::CStr; -+use std::ffi::{CStr, CString}; +@@ contrib/cgit-rs/cgit-sys/src/lib.rs: extern "C" { - fn main() { - println!("Let's print some strings provided by Git"); -@@ contrib/cgit-rs/src/main.rs: fn main() { - println!("git_user_agent() = {:?}", c_str); - println!("git_user_agent_sanitized() = {:?}", - unsafe { CStr::from_ptr(cgit::libgit_user_agent_sanitized()) }); -+ -+ println!("\nNow try passing args"); -+ let test_arg = CString::new("test_arg").unwrap(); -+ println!("git_parse_maybe_bool(...) = {:?}", -+ unsafe { cgit::libgit_parse_maybe_bool(test_arg.as_ptr()) }); -+ -+ println!("\nCan we get an int out of our config??"); -+ unsafe { -+ cgit::libgit_initialize_the_repository(); -+ cgit::libgit_setup_git_directory(); -+ let mut val: libc::c_int = 0; + #[cfg(test)] + mod tests { +- use std::ffi::CStr; ++ use std::ffi::{CStr, CString}; + + use super::*; + +@@ contrib/cgit-rs/cgit-sys/src/lib.rs: mod tests { + agent + ); + } ++ ++ #[test] ++ fn parse_bools_from_strings() { ++ let arg = CString::new("true").unwrap(); ++ assert_eq!(unsafe { libgit_parse_maybe_bool(arg.as_ptr()) }, 1); ++ ++ let arg = CString::new("yes").unwrap(); ++ assert_eq!(unsafe { libgit_parse_maybe_bool(arg.as_ptr()) }, 1); ++ ++ let arg = CString::new("false").unwrap(); ++ assert_eq!(unsafe { libgit_parse_maybe_bool(arg.as_ptr()) }, 0); ++ ++ let arg = CString::new("no").unwrap(); ++ assert_eq!(unsafe { libgit_parse_maybe_bool(arg.as_ptr()) }, 0); ++ ++ let arg = CString::new("maybe").unwrap(); ++ assert_eq!(unsafe { libgit_parse_maybe_bool(arg.as_ptr()) }, -1); ++ } ++ ++ #[test] ++ fn access_configs() { ++ // NEEDSWORK: we need to supply a testdata config ++ let fake_argv = [std::ptr::null::<c_char>()]; ++ unsafe { ++ libgit_init_git(fake_argv.as_ptr()); ++ libgit_setup_git_directory(); ++ } ++ let mut val: c_int = 0; + let key = CString::new("trace2.eventNesting").unwrap(); -+ cgit::libgit_config_get_int( -+ key.as_ptr(), -+ &mut val as *mut i32 -+ ); -+ println!( -+ "git_config_get_int(\"trace2.eventNesting\") -> {:?}", -+ val -+ ); -+ }; ++ unsafe { libgit_config_get_int(key.as_ptr(), &mut val as *mut i32) }; ++ assert_eq!(val, 5); ++ } } 5: c8befb680e ! 4: 908ad0b82f config: add git_configset_alloc @@ Metadata Author: Josh Steadmon <steadmon@google.com> ## Commit message ## - config: add git_configset_alloc + config: add git_configset_alloc() and git_configset_clear_and_free() - Add git_configset_alloc so that non-C external consumers can use - configset functions without redefining config_set. + Add git_configset_alloc() so that and git_configset_clear_and_free() + functions so that callers can manage config_set structs on the heap. + This also allows non-C external consumers to treat config_sets as opaque + structs. Co-authored-by: Calvin Wan <calvinwan@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> - Signed-off-by: Josh Steadmon <steadmon@google.com> - + Also add _clear_and_free + + ## config.c ## @@ config.c: static int config_set_element_cmp(const void *cmp_data UNUSED, return strcmp(e1->key, e2->key); @@ config.c: static int config_set_element_cmp(const void *cmp_data UNUSED, void git_configset_init(struct config_set *set) { hashmap_init(&set->config_hash, config_set_element_cmp, NULL, 0); +@@ config.c: void git_configset_clear(struct config_set *set) + set->list.items = NULL; + } + ++void git_configset_clear_and_free(struct config_set *set) ++{ ++ git_configset_clear(set); ++ free(set); ++} ++ + static int config_set_callback(const char *key, const char *value, + const struct config_context *ctx, + void *cb) ## config.h ## @@ config.h: struct config_set { @@ config.h: struct config_set { /** * Initializes the config_set `cs`. */ +@@ config.h: int git_configset_get_string_multi(struct config_set *cs, const char *key, + */ + void git_configset_clear(struct config_set *cs); + ++/** ++ * Clears and frees a heap-allocated `config_set` structure. ++ */ ++void git_configset_clear_and_free(struct config_set *cs); ++ + /* + * These functions return 1 if not found, and 0 if found, leaving the found + * value in the 'dest' pointer. 6: 1e981a6880 ! 5: 514c744ba4 contrib/cgit-rs: add a subset of configset wrappers @@ Metadata ## Commit message ## contrib/cgit-rs: add a subset of configset wrappers Signed-off-by: Calvin Wan <calvinwan@google.com> - Signed-off-by: Josh Steadmon <steadmon@google.com> - ## contrib/cgit-rs/Cargo.lock ## -@@ contrib/cgit-rs/Cargo.lock: version = 3 - name = "cgit" - version = "0.1.0" - dependencies = [ -+ "home", - "libc", - ] - -+[[package]] -+name = "home" -+version = "0.5.9" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "e3d1354bf6b7235cb4a0576c2619fd4ed18183f689b12b006a0ee7329eeff9a5" -+dependencies = [ -+ "windows-sys", -+] -+ - [[package]] - name = "libc" - version = "0.2.155" - source = "registry+https://github.com/rust-lang/crates.io-index" - checksum = "97b3888a4aecf77e811145cadf6eef5901f4782c53886191b2f693f24761847c" -+ -+[[package]] -+name = "windows-sys" -+version = "0.52.0" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "282be5f36a8ce781fad8c8ae18fa3f9beff57ec1b52cb3de0789201425d9a33d" -+dependencies = [ -+ "windows-targets", -+] + ## .gitignore ## +@@ .gitignore: Release/ + /git.VC.db + *.dSYM + /contrib/buildsystems/out ++/contrib/cgit-rs/target + /contrib/cgit-rs/cgit-sys/target + + ## Makefile ## +@@ Makefile: clean: profile-clean coverage-clean cocciclean + $(RM) $(htmldocs).tar.gz $(manpages).tar.gz + $(MAKE) -C Documentation/ clean + $(RM) Documentation/GIT-EXCLUDED-PROGRAMS +- $(RM) -r contrib/cgit-rs/cgit-sys/target ++ $(RM) -r contrib/cgit-rs/target contrib/cgit-rs/cgit-sys/target + ifndef NO_PERL + $(RM) -r perl/build/ + endif + + ## contrib/cgit-rs/Cargo.lock (new) ## +@@ ++# This file is automatically @generated by Cargo. ++# It is not intended for manual editing. ++version = 3 + +[[package]] -+name = "windows-targets" -+version = "0.52.6" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "9b724f72796e036ab90c1021d4780d4d3d648aca59e491e6b98e725b84e99973" ++name = "cgit" ++version = "0.1.0" +dependencies = [ -+ "windows_aarch64_gnullvm", -+ "windows_aarch64_msvc", -+ "windows_i686_gnu", -+ "windows_i686_gnullvm", -+ "windows_i686_msvc", -+ "windows_x86_64_gnu", -+ "windows_x86_64_gnullvm", -+ "windows_x86_64_msvc", ++ "cgit-sys", +] + +[[package]] -+name = "windows_aarch64_gnullvm" -+version = "0.52.6" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "32a4622180e7a0ec044bb555404c800bc9fd9ec262ec147edd5989ccd0c02cd3" -+ -+[[package]] -+name = "windows_aarch64_msvc" -+version = "0.52.6" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "09ec2a7bb152e2252b53fa7803150007879548bc709c039df7627cabbd05d469" -+ -+[[package]] -+name = "windows_i686_gnu" -+version = "0.52.6" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "8e9b5ad5ab802e97eb8e295ac6720e509ee4c243f69d781394014ebfe8bbfa0b" -+ -+[[package]] -+name = "windows_i686_gnullvm" -+version = "0.52.6" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "0eee52d38c090b3caa76c563b86c3a4bd71ef1a819287c19d586d7334ae8ed66" -+ -+[[package]] -+name = "windows_i686_msvc" -+version = "0.52.6" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "240948bc05c5e7c6dabba28bf89d89ffce3e303022809e73deaefe4f6ec56c66" -+ -+[[package]] -+name = "windows_x86_64_gnu" -+version = "0.52.6" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "147a5c80aabfbf0c7d901cb5895d1de30ef2907eb21fbbab29ca94c5b08b1a78" ++name = "cgit-sys" ++version = "0.1.0" + + ## contrib/cgit-rs/Cargo.toml (new) ## +@@ ++[package] ++name = "cgit" ++version = "0.1.0" ++edition = "2021" + -+[[package]] -+name = "windows_x86_64_gnullvm" -+version = "0.52.6" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "24d5b23dc417412679681396f2b49f3de8c1473deb516bd34410872eff51ed0d" ++[lib] ++path = "src/lib.rs" + -+[[package]] -+name = "windows_x86_64_msvc" -+version = "0.52.6" -+source = "registry+https://github.com/rust-lang/crates.io-index" -+checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" ++[dependencies] ++cgit-sys = { version = "0.1.0", path = "cgit-sys" } - ## contrib/cgit-rs/Cargo.toml ## -@@ contrib/cgit-rs/Cargo.toml: path = "src/lib.rs" - - [dependencies] - libc = "0.2.155" -+home = "0.5.9" - - ## contrib/cgit-rs/public_symbol_export.c ## -@@ contrib/cgit-rs/public_symbol_export.c: int libgit_parse_maybe_bool(const char *val) + ## contrib/cgit-rs/cgit-sys/public_symbol_export.c ## +@@ contrib/cgit-rs/cgit-sys/public_symbol_export.c: int libgit_parse_maybe_bool(const char *val) return git_parse_maybe_bool(val); } @@ contrib/cgit-rs/public_symbol_export.c: int libgit_parse_maybe_bool(const char * + return git_configset_alloc(); +} + ++void libgit_configset_clear_and_free(struct config_set *cs) ++{ ++ git_configset_clear_and_free(cs); ++} ++ +void libgit_configset_init(struct config_set *cs) +{ + git_configset_init(cs); @@ contrib/cgit-rs/public_symbol_export.c: int libgit_parse_maybe_bool(const char * { return git_user_agent(); - ## contrib/cgit-rs/public_symbol_export.h ## -@@ contrib/cgit-rs/public_symbol_export.h: void libgit_initialize_the_repository(void); + ## contrib/cgit-rs/cgit-sys/public_symbol_export.h ## +@@ contrib/cgit-rs/cgit-sys/public_symbol_export.h: void libgit_init_git(const char **argv); int libgit_parse_maybe_bool(const char *val); +struct config_set *libgit_configset_alloc(void); + ++void libgit_configset_clear_and_free(struct config_set *cs); ++ +void libgit_configset_init(struct config_set *cs); + +int libgit_configset_add_file(struct config_set *cs, const char *filename); @@ contrib/cgit-rs/public_symbol_export.h: void libgit_initialize_the_repository(vo const char *libgit_user_agent_sanitized(void); - ## contrib/cgit-rs/src/lib.rs ## + ## contrib/cgit-rs/cgit-sys/src/lib.rs ## @@ --use libc::{c_char, c_int}; -+use std::ffi::{CStr, CString}; +-use std::ffi::{c_char, c_int}; ++use std::ffi::{c_char, c_int, c_void}; ++ ++#[allow(non_camel_case_types)] ++#[repr(C)] ++pub struct config_set { ++ _data: [u8; 0], ++ _marker: core::marker::PhantomData<(*mut u8, core::marker::PhantomPinned)>, ++} + + extern "C" { ++ pub fn free(ptr: *mut c_void); + -+use libc::{c_char, c_int, c_void}; + pub fn libgit_setup_git_directory() -> *const c_char; + + // From config.c +@@ contrib/cgit-rs/cgit-sys/src/lib.rs: extern "C" { + // From version.c + pub fn libgit_user_agent() -> *const c_char; + pub fn libgit_user_agent_sanitized() -> *const c_char; + -+pub enum GitConfigSet {} ++ pub fn libgit_configset_alloc() -> *mut config_set; ++ pub fn libgit_configset_clear_and_free(cs: *mut config_set); + -+pub struct ConfigSet(*mut GitConfigSet); -+impl ConfigSet { ++ pub fn libgit_configset_init(cs: *mut config_set); ++ ++ pub fn libgit_configset_add_file(cs: *mut config_set, filename: *const c_char) -> c_int; ++ ++ pub fn libgit_configset_get_int( ++ cs: *mut config_set, ++ key: *const c_char, ++ int: *mut c_int, ++ ) -> c_int; + ++ pub fn libgit_configset_get_string( ++ cs: *mut config_set, ++ key: *const c_char, ++ dest: *mut *mut c_char, ++ ) -> c_int; ++ + } + + #[cfg(test)] + + ## contrib/cgit-rs/src/lib.rs (new) ## +@@ ++use std::ffi::{c_char, c_int, c_void, CStr, CString}; ++ ++use cgit_sys::*; ++ ++pub struct ConfigSet(*mut config_set); ++impl ConfigSet { + pub fn new() -> Self { + unsafe { -+ // TODO: we need to handle freeing this when the ConfigSet is dropped -+ // git_configset_clear(ptr) and then libc::free(ptr) + let ptr = libgit_configset_alloc(); + libgit_configset_init(ptr); + ConfigSet(ptr) + } + } + ++ // NEEDSWORK: maybe replace &str with &Path + pub fn add_files(&mut self, files: &[&str]) { + for file in files { + let rs = CString::new(*file).expect("Couldn't convert to CString"); @@ contrib/cgit-rs/src/lib.rs + let key = CString::new(key).expect("Couldn't convert to CString"); + let mut val: *mut c_char = std::ptr::null_mut(); + unsafe { -+ if libgit_configset_get_string(self.0, key.as_ptr(), &mut val as *mut *mut c_char) != 0 { ++ if libgit_configset_get_string(self.0, key.as_ptr(), &mut val as *mut *mut c_char) != 0 ++ { + return None; + } + let borrowed_str = CStr::from_ptr(val); + let owned_str = CString::from_vec_with_nul(borrowed_str.to_bytes_with_nul().to_vec()); -+ libc::free(val as *mut c_void); // Free the xstrdup()ed pointer from the C side ++ free(val as *mut c_void); // Free the xstrdup()ed pointer from the C side + Some(owned_str.unwrap()) + } + } +} - - extern "C" { - pub fn libgit_setup_git_directory() -> *const c_char; -@@ contrib/cgit-rs/src/lib.rs: extern "C" { - // From version.c - pub fn libgit_user_agent() -> *const c_char; - pub fn libgit_user_agent_sanitized() -> *const c_char; -+ -+ fn libgit_configset_alloc() -> *mut GitConfigSet; -+ -+ fn libgit_configset_init(cs: *mut GitConfigSet); + -+ fn libgit_configset_add_file(cs: *mut GitConfigSet, filename: *const c_char) -> c_int; ++impl Default for ConfigSet { ++ fn default() -> Self { ++ Self::new() ++ } ++} + -+ pub fn libgit_configset_get_int(cs: *mut GitConfigSet, key: *const c_char, int: *mut c_int) -> c_int; -+ pub fn libgit_configset_get_string(cs: *mut GitConfigSet, key: *const c_char, dest: *mut *mut c_char) -> c_int; ++impl Drop for ConfigSet { ++ fn drop(&mut self) { ++ unsafe { ++ libgit_configset_clear_and_free(self.0); ++ } ++ } ++} + - } - - ## contrib/cgit-rs/src/main.rs ## -@@ - use std::ffi::{CStr, CString}; -+use home::home_dir; - - fn main() { - println!("Let's print some strings provided by Git"); -@@ contrib/cgit-rs/src/main.rs: fn main() { - val - ); - }; -+ -+ println!("\nTry out our configset wrappers"); -+ let mut cs = cgit::ConfigSet::new(); -+ let mut path = home_dir().expect("cannot get home directory path"); -+ path.push(".gitconfig"); -+ let path:String = path.into_os_string().into_string().unwrap(); -+ cs.add_files(&["/etc/gitconfig", ".gitconfig", &path]); -+ /* -+ * Returns Some(x) if defined in local config, otherwise None -+ */ -+ println!("get_configset_get_int = {:?}", cs.get_int("trace2.eventNesting")); -+ println!("cs.get_str(\"garbage\") = {:?}", cs.get_str("this_string_does_not_exist")); - } ++#[cfg(test)] ++mod tests { ++ use super::*; ++ ++ #[test] ++ fn load_configs_via_configset() { ++ // NEEDSWORK: we need to supply a testdata config ++ let mut cs = ConfigSet::new(); ++ let mut path = std::env::home_dir().expect("cannot get home directory path"); ++ path.push(".gitconfig"); ++ let path: String = path.into_os_string().into_string().unwrap(); ++ cs.add_files(&["/etc/gitconfig", ".gitconfig", &path]); ++ assert_eq!(cs.get_int("trace2.eventNesting"), Some(5)); ++ assert_eq!(cs.get_str("no_such_config_item"), None); ++ } ++} base-commit: 557ae147e6cdc9db121269b058c757ac5092f9c9