{"thread":{"id":"61252","subject":"[RFD] Libification proposal: separate internal and external interfaces","startedAt":"2024-04-02T14:19:03Z","lastAt":"2024-05-10T21:36:07Z","messageCount":18,"participants":["Calvin Wan","brian m. carlson","rsbecker@nexbridge.com","Junio C Hamano","Phillip Wood","phillip.wood123@gmail.com","Kyle Lippincott"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"492057","messageId":"CAFySSZAB09QB7U6UxntK2jRJF0df5R7YGnnLSsYc9MYhHsBhWA@mail.gmail.com","threadId":"61252","inReplyTo":null,"subject":"[RFD] Libification proposal: separate internal and external interfaces","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-04-02T14:18:51Z","receivedAt":"2024-04-02T14:19:03Z","isPatch":false,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"This proposal was originally written by Kyle Lippincott, but he’s\ncurrently on vacation for the next two weeks so I’m helping start this\ndiscussion for him (from here on out Kyle is the “I”).\n\nTL;DR: I'm proposing that when creating a library for code from the Git\ncodebase, we have two interfaces to this library: the \"internal\" one\nthat the rest of the Git codebase uses, and the \"external\" one for use\nby other projects. The external interface will have a different coding\nstyle and platform support than the rest of the codebase.\n\nWhen thinking about potential issues and complications with\nlibification, I encountered a few broad categories of issues, and I'd\nlike to list them briefly (edit: turns out I can't be brief to save my\nlife) and float a proposal that may help minimize them.\n\nDefinitions\n-----------\n- When I say \"Git\" or \"the git executable/binary\" or whatever, I'm\n referring to \"the collection of binaries, tests, etc. that are part of\n the main git repo\" unless I say otherwise.\n- Similarly, when I say \"internal\" I mean \"for use by \". When I say \"external\" I mean for use by stuff that's\n not part of the Git repository.\n\nAssumptions\n-----------\n- Libraries that we're providing can be either statically or dynamically\n linked. Git will link statically to its own Git libraries. External\n projects may use either.\n- Git must continue to be compilable and usable on all platforms it's\n currently supported on. Libification can't take that away. However,\n since libification is producing new interfaces for new use cases,\n there is no requirement that we make these new interfaces usable on\n all platforms, especially at first.\n- We'd like as little churn and \"uglification\" of the main codebase as\n possible.\n\nIssues\n------\n- Symbol name collisions: Since C doesn't have namespacing or other\n official name mangling mechanisms, all of the symbols inside of the\n library that aren't static are going to be at risk of colliding with\n symbols in the external project. This is especially a problem for\n common symbols like \"error()\".\n\n- Header files: This is actually several related problems:\n - Git codebase's header files assume that anything that's brought in\n via is available; this includes system header\n files, but also macro definitions, including ones that change how\n various headers behave. Example: _GNU_SOURCE and\n _FILE_OFFSET_BITS=64 cause headers like to change\n behavior; _GNU_SOURCE makes it provide different/additional\n functionality, and _FILE_OFFSET_BITS=64 makes types like `off_t` be\n 64-bit (on some platforms it might be 32-bit without this define).\n - is expected to be included as the first header\n file in the translation unit, so as to make _GNU_SOURCE and similar\n #defines have the desired effect. If a translation unit (in an\n external library consumer) has already included , we can't\n rely on them having had _GNU_SOURCE defined ahead of time\n - We can't just `#include ` at the top of our\n external interface headers,\n - Git's header files make regular use of inlining. We can't assume\n that external projects are going to use static linking, and we can't\n assume that external projects are going to use a C-compatible\n language (they might not use our header files at all), so inline\n functions seem risky at the interface layer.\n\n- Compatibility: Using code from the git codebase as a library is a new\n use case, we do not have the backwards compatibility requirements that\n we do for Git itself. We should take full advantage of this, and\n explicitly state what compatibility guarantees we are providing (or\n not providing).\n\nProposal\n--------\nLet's have a distinction between the \"internal\" interface (used by Git),\nand the \"external\" interface (used by everyone else). The \"external\"\ninterface has several differences from the rest of the git codebase:\n\n- Minimal. Only include symbols and types that we explicitly want to be\n part of the interface\n - This is both for API evolution abilities and providing a \"well-lit\n path\" to usage. Internal header files may have a lot of similar\n but slightly different functions that can be very confusing, or\n are highly specialized.\n - Most languages will not be able to include our headers. Reducing\n the interface to the minimal necessary means it's easier to\n identify when the interfaces change and update the\n non-C-compatible-language bindings.\n - The external interface should have as little code/new\n functionality as possible. All actual functionality should be in\n the internal interface(s).\n\n- No inline functions. This is similar to minimal. We should put as\n little as possible in the header files, especially since many use\n cases involve using the library from a language that can't even\n #include them at all.\n\n- Self Contained. The header files must work if they are the first/only\n #include in the external project. They must include everything they\n need, and not assume it was already handled for them.\n\n- Tolerant. The header files probably won't be the first/only #include\n in the external project's translation unit, and they should still\n work. This means not using types like `off_t` or `struct stat` in the\n interfaces provided, since their sizes are dependent on the execution\n environment (what's been included, #defines, CFLAGS, etc.)\n\n- Non-interfering. Our header files must not change fundamental things\n about the execution environment. This means they must not do things\n like #define _GNU_SOURCE or #define _FILE_OFFSET_BITS=64\n\n- Limited Platform Compatibility. The external interfaces are able to\n assume that and other C99 (or maybe even C11+)\n functionality exists and use it immediately, without weather balloons\n or #ifdefs. If some platform requires special handling, that platform\n isn't supported, at least initially.\n\n- Non-colliding. Symbol names in these external interface headers should\n have a standard, obvious prefix as a manual namespacing solution.\n Something like `gitlib_`. (Prior art: cURL uses a `curl_` prefix for\n externally-visible symbols, libgit2 uses `git__foo`\n names; for internal symbols, they use `Curl_` and `git___foo`\n (with a double underscore), respectively)\n\n- Translating. The external interface provides \"external\" symbol names,\n and potentially more compatible function interfaces than the internal\n interface does, and exists to translate from one domain to another.\n Most functions in the external interface will be just a single call to\n the internal interface. Examples:\n - Internal interface is `void foo();`; external interface would be\n `void gitlib_foo() { foo(); }`\n - Internal interface is `void foo(off_t val);`; external interface\n could be `void gitlib_foo(int64_t val) { foo(val); }` -- here we\n accept int64_t instead of off_t due to the issues around the size\n of off_t\n - Internal interface is `void foo(strbuf *s);`; the external\n interface might be `void gitlib_foo(char *s, size_t s_len) {\n strbuf sb; strbuf_init(&sb, s_len + 1); strbuf_add(&sb, s, s_len);\n foo(&sb); } ` -- since strbufs own the memory they hold, strings\n that come via the external interface might need to be copied to be\n memory safe.\n - Internal interface was `void foo();` but gained a new parameter.\n We don't need to expose this parameter in the external interface,\n and instead can just use a sensible default. External interface\n can remain `void gitlib_foo() { foo(NULL); }`\n\nProof of Concept\n----------------\nI think we should continue with the git-std-lib work as a manual\nseparation of the .c files and associated header files that comprise the\nvery lowest level of functionality in the git codebase. This manual\nseparation would only produce a library with an \"internal\" interface. We\nshould also start to apply these ideas by defining an \"external\"\ninterface which has a subset of the functionality in git-std-lib.\n\nAutomatic symbol hiding\n-----------------------\nOne of the main driving forces behind my proposal above is avoiding\nsignificant churn in the git codebase, for example needing to rename\nevery function in the codebase that's not static. While many function\nnames are unlikely to collide, such as `parse_oid_hex`, others are\nsignificantly more likely, like `error` or `hex_to_bytes`. Needing to\nrename all \"plausible\" collisions to things that are unlikely to\ncollide, like `GIT_error` or `GIT_hex_to_bytes` is tedious, error prone,\nand unpleasant.\n\nI have possibly discovered a truly remarkable solution, but this\nfootnote is too small to contain it. Wait, no it's not. This isn't fully\ntested yet, but has shown promise in my initial tests using clang on a\nLinux machine.\n- Compile the \"internal\" interface(s) and all supporting code with\n `-fvisibility=hidden` to produce .o files for each .c file\n- Compile the \"external\" interface(s) without hiding the symbols\n- Produce a .a file that contains that code, for use by git itself\n- \"Partially link\" the everything, using `ld -r`, to produce a single .o\n file\n- Use `objcopy --localize-hidden` to actually hide the internal symbols\n from the \"partially linked\" .o file\n\nThis should leave us with two static libraries: one that has the symbols\nmarked as \"hidden\" but still usable, for use in git itself, and one that\ncontains the external interface, but doesn't expose the hidden symbols.\n\nThere may be similar solutions possible on other platforms, or there may\nnot, and we may need to do the great renaming (either in the code\nitself, or via something like a giant set of linker scripts). While my\nproposal to have a separation between the internal and external\ninterfaces is a requirement for making this automatic symbol hiding\nsolution work, I don't think that a failure to make the automatic symbol\nhiding solution work means that we shouldn't have the internal/external\nsplit. It's only one contributing point in favor of having the\ninternal/external split.\n"},{"id":"492479","messageId":"ZhMRNxgwRJ25P4Ud@tapette.crustytoothpaste.net","threadId":"61252","inReplyTo":"CAFySSZAB09QB7U6UxntK2jRJF0df5R7YGnnLSsYc9MYhHsBhWA@mail.gmail.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-04-07T21:33:43Z","receivedAt":"2024-04-07T21:33:53Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-04-02 at 14:18:51, Calvin Wan wrote:\n> Issues\n> ------\n> - Symbol name collisions: Since C doesn't have namespacing or other\n> official name mangling mechanisms, all of the symbols inside of the\n> library that aren't static are going to be at risk of colliding with\n> symbols in the external project. This is especially a problem for\n> common symbols like \"error()\".\n\nYes, I think this is important. We'll want to avoid the mistake that\nOpenSSL and Perl made by just exporting random symbols or using more\nthan one namespace.\n\nWe'll also need to consider that libgit2 is currently using `git_` and\nthus we'll either need to use something different or avoid conflicts.\nPerhaps `gitlib_` might be useful.\n\nI might also suggest that we enable symbol versioning on platforms that\nsupport it, which is a nice way to avoid conflicts.\n\n> - Header files: This is actually several related problems:\n> - Git codebase's header files assume that anything that's brought in\n> via is available; this includes system header\n> files, but also macro definitions, including ones that change how\n> various headers behave. Example: _GNU_SOURCE and\n> _FILE_OFFSET_BITS=64 cause headers like to change\n> behavior; _GNU_SOURCE makes it provide different/additional\n> functionality, and _FILE_OFFSET_BITS=64 makes types like `off_t` be\n> 64-bit (on some platforms it might be 32-bit without this define).\n\nI should point out that _FILE_OFFSET_BITS=64 is effectively standard\nthese days. Nobody wants their files limited to 2 GiB.\n\n> - Tolerant. The header files probably won't be the first/only #include\n> in the external project's translation unit, and they should still\n> work. This means not using types like `off_t` or `struct stat` in the\n> interfaces provided, since their sizes are dependent on the execution\n> environment (what's been included, #defines, CFLAGS, etc.)\n\nSure. If we need a file size type, it should be something like\n`int64_t` or `uint64_t` and not `off_t`.\n\n> - Limited Platform Compatibility. The external interfaces are able to\n> assume that and other C99 (or maybe even C11+)\n> functionality exists and use it immediately, without weather balloons\n> or #ifdefs. If some platform requires special handling, that platform\n> isn't supported, at least initially.\n\nI think this is fine. It's 2024. We should assume that C11 (a\n13-year-old spec) is available and so is POSIX 1003.1-2008 (except for\nWindows). We may want to have a nice `#ifdef __STDC__ < 200112L` (and a\nsimilar check for POSIX) to just produce an `#error` if the system is\ntoo old.\n\nWe should therefore also assume that threading is available because\nPOSIX 1003.1-2008 requires it as part of the base specification (and\nWindows obviously also supports it). Because this is a library, we'll\nwant to avoid interfaces that are clearly not thread safe and document\nthe requirements for thread safety (either that the object must be\nexternally synchronized or that it is internally synchronized).\n\n> - Non-colliding. Symbol names in these external interface headers should\n> have a standard, obvious prefix as a manual namespacing solution.\n> Something like `gitlib_`. (Prior art: cURL uses a `curl_` prefix for\n> externally-visible symbols, libgit2 uses `git__foo`\n> names; for internal symbols, they use `Curl_` and `git___foo`\n> (with a double underscore), respectively)\n\nYeah, I think we came up with the same idea. I do like the\n`gitlib_module_foo` approach.\n\nOverall, I read through this and I didn't see anything I obviously\ndisagreed with. Everything seemed to be either reasonable or something\nI didn't have a strong opinion on.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"492480","messageId":"037001da8935$4a6e3720$df4aa560$@nexbridge.com","threadId":"61252","inReplyTo":"ZhMRNxgwRJ25P4Ud@tapette.crustytoothpaste.net","subject":"RE: [RFD] Libification proposal: separate internal and external interfaces","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-04-07T21:48:07Z","receivedAt":"2024-04-07T21:48:25Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Sunday, April 7, 2024 5:34 PM, brian m. carlson wrote:\n>On 2024-04-02 at 14:18:51, Calvin Wan wrote:\n>> Issues\n>> ------\n>> - Symbol name collisions: Since C doesn't have namespacing or other\n>> official name mangling mechanisms, all of the symbols inside of the\n>> library that aren't static are going to be at risk of colliding with\n>> symbols in the external project. This is especially a problem for\n>> common symbols like \"error()\".\n>\n>Yes, I think this is important. We'll want to avoid the mistake that OpenSSL and Perl\n>made by just exporting random symbols or using more than one namespace.\n>\n>We'll also need to consider that libgit2 is currently using `git_` and thus we'll either\n>need to use something different or avoid conflicts.\n>Perhaps `gitlib_` might be useful.\n\nOr `GITL_`\n\n>\n>I might also suggest that we enable symbol versioning on platforms that support it,\n>which is a nice way to avoid conflicts.\n>\n>> - Header files: This is actually several related problems:\n>> - Git codebase's header files assume that anything that's brought in\n>> via is available; this includes system header\n>> files, but also macro definitions, including ones that change how\n>> various headers behave. Example: _GNU_SOURCE and\n>> _FILE_OFFSET_BITS=64 cause headers like to change\n>> behavior; _GNU_SOURCE makes it provide different/additional\n>> functionality, and _FILE_OFFSET_BITS=64 makes types like `off_t` be\n>> 64-bit (on some platforms it might be 32-bit without this define).\n>\n>I should point out that _FILE_OFFSET_BITS=64 is effectively standard these days.\n>Nobody wants their files limited to 2 GiB.\n\nThis is not the default on some platforms. It is still required but can be put into a knob.\n\n>> - Tolerant. The header files probably won't be the first/only #include\n>> in the external project's translation unit, and they should still\n>> work. This means not using types like `off_t` or `struct stat` in the\n>> interfaces provided, since their sizes are dependent on the execution\n>> environment (what's been included, #defines, CFLAGS, etc.)\n>\n>Sure. If we need a file size type, it should be something like `int64_t` or `uint64_t`\n>and not `off_t`. Not all platforms have explicit 64-bit APIs compiles. NonStop and S/390 do require explicit controls to make this work.\n\nThis will result in compile warnings with some platforms with APIs that do use off_t for arguments and results.\n\n>\n>> - Limited Platform Compatibility. The external interfaces are able to\n>> assume that and other C99 (or maybe even C11+)\n>> functionality exists and use it immediately, without weather balloons\n>> or #ifdefs. If some platform requires special handling, that platform\n>> isn't supported, at least initially.\n>\n>I think this is fine. It's 2024. We should assume that C11 (a 13-year-old spec) is\n>available and so is POSIX 1003.1-2008 (except for Windows). We may want to\n>have a nice `#ifdef __STDC__ < 200112L` (and a similar check for POSIX) to just\n>produce an `#error` if the system is too old.\n\nI wish this were the case. I have 2 years before C11 is guaranteed to be available on platforms that I maintain. Can we wait until mid-late 2025 before doing this? I do not want to lose access to git.\n\n>We should therefore also assume that threading is available because POSIX 1003.1-\n>2008 requires it as part of the base specification (and Windows obviously also\n>supports it). Because this is a library, we'll want to avoid interfaces that are clearly\n>not thread safe and document the requirements for thread safety (either that the\n>object must be externally synchronized or that it is internally synchronized).\n\nThis is not a particularly good assumption. Some architectures do not support kernel-level threading. PUT threading is possible on some platforms but attempts to port git to PUT require that all dependencies also support PUT, which is not a good assumption. Most of the dependencies that I have to work with actually do not support PUT or any other form of threading.\n\n>> - Non-colliding. Symbol names in these external interface headers should\n>> have a standard, obvious prefix as a manual namespacing solution.\n>> Something like `gitlib_`. (Prior art: cURL uses a `curl_` prefix for\n>> externally-visible symbols, libgit2 uses `git__foo`\n>> names; for internal symbols, they use `Curl_` and `git___foo`\n>> (with a double underscore), respectively)\n>\n>Yeah, I think we came up with the same idea. I do like the `gitlib_module_foo`\n>approach.\n>\n>Overall, I read through this and I didn't see anything I obviously disagreed with.\n>Everything seemed to be either reasonable or something I didn't have a strong\n>opinion on.\n\nSignificantly changing git dependencies and requirement are not intended to be part of the libification project. There are version serious consequences to doing that. Losing access to git is definitely not desirable from my point of view.\n\n--Randall\n\n"},{"id":"492482","messageId":"ZhND3hZXlzDxff5e@tapette.crustytoothpaste.net","threadId":"61252","inReplyTo":"037001da8935$4a6e3720$df4aa560$@nexbridge.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-04-08T01:09:50Z","receivedAt":"2024-04-08T01:09:53Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-04-07 at 21:48:07, rsbecker@nexbridge.com wrote:\n> On Sunday, April 7, 2024 5:34 PM, brian m. carlson wrote:\n> >We'll also need to consider that libgit2 is currently using `git_` and thus we'll either\n> >need to use something different or avoid conflicts.\n> >Perhaps `gitlib_` might be useful.\n> \n> Or `GITL_`\n\nIt's more common to use lowercase, so I think we should do that.\n\n> >I should point out that _FILE_OFFSET_BITS=64 is effectively standard these days.\n> >Nobody wants their files limited to 2 GiB.\n> \n> This is not the default on some platforms. It is still required but can be put into a knob.\n\nTrue, but everyone compiles with it. If you're a software vendor and\nyour build only supports 2 GiB files, everyone's going to be mad. We\nshould be able to assume that vendors are prudent and reasonable people\nwho will compile their binaries accordingly and not have to return an\nerror if a requested file size is larger than 32 bits.\n\n> >Sure. If we need a file size type, it should be something like `int64_t` or `uint64_t`\n> >and not `off_t`. Not all platforms have explicit 64-bit APIs compiles. NonStop and S/390 do require explicit controls to make this work.\n> \n> This will result in compile warnings with some platforms with APIs\n> that do use off_t for arguments and results.\n\nThere will need to be a cast. Our APIs should use a standard 64-bit\ntype and not expose the platform `off_t`.\n\n> >\n> >> - Limited Platform Compatibility. The external interfaces are able to\n> >> assume that and other C99 (or maybe even C11+)\n> >> functionality exists and use it immediately, without weather balloons\n> >> or #ifdefs. If some platform requires special handling, that platform\n> >> isn't supported, at least initially.\n> >\n> >I think this is fine. It's 2024. We should assume that C11 (a 13-year-old spec) is\n> >available and so is POSIX 1003.1-2008 (except for Windows). We may want to\n> >have a nice `#ifdef __STDC__ < 200112L` (and a similar check for POSIX) to just\n> >produce an `#error` if the system is too old.\n> \n> I wish this were the case. I have 2 years before C11 is guaranteed to\n> be available on platforms that I maintain. Can we wait until mid-late\n> 2025 before doing this? I do not want to lose access to git.\n\nAs mentioned in the original proposal, we don't have to support all\nplatforms in the libified code. The main Git binaries will continue to\nfunction and be supported, but the new libified code will rely on newer\nfeatures. You will still be able to have all the Git binaries and\nfunctionality, but if you want the new shared library to compile\nagainst, you'll have to furnish a newer compiler.\n\nAs a side note, I don't think requiring porters to support a 13-year-old\nspec is unreasonable for new, independent parts of the codebase. As a\nside note, if your platform supported GCC or LLVM, then this would be an\neasy goal to achieve. I know it doesn't right now, but it might be an\nincentive to get it there.\n\n> This is not a particularly good assumption. Some architectures do not\n> support kernel-level threading. PUT threading is possible on some\n> platforms but attempts to port git to PUT require that all\n> dependencies also support PUT, which is not a good assumption. Most of\n> the dependencies that I have to work with actually do not support PUT\n> or any other form of threading.\n\nI don't believe this is true. Every architecture which supports Linux\nor any other modern Unix has threading support. You're compiling on\nia64 and x86, which definitely do support threading and have had it on\nLinux and NetBSD for years (before they removed ia64 support).\n\nThreading is a reasonable thing to have on a modern operating system,\nand if we're adding new, independent functionality, that should be able\nto safely work with threading. You can continue to use the binaries\nwithout threading, just not the new shared libraries.\n\nI'll add on that I'm not opposed to continuing to support NonStop in\nprinciple, but I _am_ opposed to having to continue to support really\nancient versions of common standards[0]. POSIX 1003.1-2008 is about 16\nyears old; it could drive a car in much of North America. I think it's\nthe responsibility of porters to provide this functionality, and in\ngeneral most open source OSes do this with little or no paid staff, so\nit should be reasonable to expect corporate-backed operating systems to\ndo so.\n\nI would strongly recommend that NonStop start supporting modern compiler\nand POSIX standards, as well at least one of GCC or LLVM, and that you\npush very hard to make that happen. It's not just Git that's pushing\nforward here; there's a lot of open source software that simply will not\ncompile with the functionality you support, including the increasing\namount of software written in Go or Rust, and it's unreasonable to\nexpect people not to want to use standard functionality that's over a\ndecade old. If NonStop doesn't move in that direction, I anticipate we\nwill eventually drop support for it sooner or later.\n\n[0] This is not just NonStop; Microsoft's long-time refusal to implement\nC99 or a newer version of C really ground my gears and I basically gave\nup on supporting it for all personal projects as a result. Fortunately,\nthey've now come up to a modern standard.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"492511","messageId":"043c01da89a4$ec6efef0$c54cfcd0$@nexbridge.com","threadId":"61252","inReplyTo":"ZhND3hZXlzDxff5e@tapette.crustytoothpaste.net","subject":"RE: [RFD] Libification proposal: separate internal and external interfaces","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-04-08T11:07:13Z","receivedAt":"2024-04-08T11:07:29Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Sunday, April 7, 2024 9:10 PM, brian m. carlson wrote:\n>On 2024-04-07 at 21:48:07, rsbecker@nexbridge.com wrote:\n>> On Sunday, April 7, 2024 5:34 PM, brian m. carlson wrote:\n>> >We'll also need to consider that libgit2 is currently using `git_`\n>> >and thus we'll either need to use something different or avoid conflicts.\n>> >Perhaps `gitlib_` might be useful.\n>>\n>> Or `GITL_`\n>\n>It's more common to use lowercase, so I think we should do that.\n>\n>> >I should point out that _FILE_OFFSET_BITS=64 is effectively standard these days.\n>> >Nobody wants their files limited to 2 GiB.\n>>\n>> This is not the default on some platforms. It is still required but can be put into a\n>knob.\n>\n>True, but everyone compiles with it. If you're a software vendor and your build only\n>supports 2 GiB files, everyone's going to be mad. We should be able to assume that\n>vendors are prudent and reasonable people who will compile their binaries\n>accordingly and not have to return an error if a requested file size is larger than 32\n>bits.\n\nI am not disagreeing that it \"should be\" standard. But it is not a compiler default, so still needs to be supplied when git is built. Removing the define will drop the limit to 2Gb on some platforms. I'm not sure where this is going.\n\n>> >Sure. If we need a file size type, it should be something like\n>> >`int64_t` or `uint64_t` and not `off_t`. Not all platforms have explicit 64-bit APIs\n>compiles. NonStop and S/390 do require explicit controls to make this work.\n>>\n>> This will result in compile warnings with some platforms with APIs\n>> that do use off_t for arguments and results.\n\nWhere _FILE_OFFSET_BITS=64, off_t should be 64 bits. If we are saying that git is going to force 64-bit types for offsets, then the types should be explicitly different, like git_off_t rather than hard-binding them to int64_t. That would allow git_compat_util.h or others to force the type broadly instead of making assumptions about the size.\n>\n>There will need to be a cast. Our APIs should use a standard 64-bit type and not\n>expose the platform `off_t`.\n>\n>> >\n>> >> - Limited Platform Compatibility. The external interfaces are able to\n>> >> assume that and other C99 (or maybe even C11+)\n>> >> functionality exists and use it immediately, without weather balloons\n>> >> or #ifdefs. If some platform requires special handling, that platform\n>> >> isn't supported, at least initially.\n>> >\n>> >I think this is fine. It's 2024. We should assume that C11 (a\n>> >13-year-old spec) is available and so is POSIX 1003.1-2008 (except\n>> >for Windows). We may want to have a nice `#ifdef __STDC__ < 200112L`\n>> >(and a similar check for POSIX) to just produce an `#error` if the system is too old.\n>>\n>> I wish this were the case. I have 2 years before C11 is guaranteed to\n>> be available on platforms that I maintain. Can we wait until mid-late\n>> 2025 before doing this? I do not want to lose access to git.\n>\n>As mentioned in the original proposal, we don't have to support all platforms in the\n>libified code. The main Git binaries will continue to function and be supported, but\n>the new libified code will rely on newer features. You will still be able to have all the\n>Git binaries and functionality, but if you want the new shared library to compile\n>against, you'll have to furnish a newer compiler.\n\nI am not sure this original assumption still holds, if git is now going to depend on the libified code, which seems to be the direction after recent discussion.\n\n>As a side note, I don't think requiring porters to support a 13-year-old spec is\n>unreasonable for new, independent parts of the codebase. As a side note, if your\n>platform supported GCC or LLVM, then this would be an easy goal to achieve. I\n>know it doesn't right now, but it might be an incentive to get it there.\n>\n>> This is not a particularly good assumption. Some architectures do not\n>> support kernel-level threading. PUT threading is possible on some\n>> platforms but attempts to port git to PUT require that all\n>> dependencies also support PUT, which is not a good assumption. Most of\n>> the dependencies that I have to work with actually do not support PUT\n>> or any other form of threading.\n>\n>I don't believe this is true. Every architecture which supports Linux or any other\n>modern Unix has threading support. You're compiling on\n>ia64 and x86, which definitely do support threading and have had it on Linux and\n>NetBSD for years (before they removed ia64 support).\n\nJust be cause the chipset supports threading, which it does, does not mean that every platform has kernel-level threading available to applications. For SMP, sure, you can make that assumption, but for MPP architectures (few as they are), PUT has been the standard - it is available on NonStop and works correctly, but porting git to PUT on NonStop has been tried at least twice since I have been involved, with bad results each time - the failures may have been in the test suite, but if one does not have a test suite that supports what is being tested, one should not release it. Where I have an issue with requiring PUT as a minimum, is that dependencies for git, and the list is not small, do not generally support PUT, even if git does; rather make a similar assumption that hardware threading is sufficient - It is not and there is ample evidence of that. Cooperative threading (PUT) is much more complex to code than a kernel thread implementation, and our attempts at porting git to PUT on NonStop failed because of problems in dependencies and the git test suites. This is a much longer discussion, but I do not have confidence that everything git requires is properly reentrant and can handle PUT. They all \"should\", but that does not appear to be the case.\n\nWith that said, I can try spinning up an effort again to try the PUT build and dive deeper into this, but I do not relish having to debug someone else's thread code, at least not without some explicit explanation or help of the thread design. My $team will avoid trying to debug threaded code that does not work in favour of a rewrite, unless the problems are glaringly obvious. My $DAYJOB is already unhappy with the time I have spent debugging other project's thread problems - ones that would impact git PUT builds.\n\nI am not saying it is impossible to do, but it is a very big ask, to justify not only git but all git dependencies to implement PUT threading correctly. If kernel threads are ever externalized on NonStop, the topic becomes much easier to handle. Again, ia64 goes to the curl because PUT is all we will ever get on ia64. Whenever this requirement is put in place, it locks git on NonStop to that version. The upshot is that I do not think libification has taken into account threading issues, so this may be moot.\n\n>Threading is a reasonable thing to have on a modern operating system, and if we're\n>adding new, independent functionality, that should be able to safely work with\n>threading. You can continue to use the binaries without threading, just not the new\n>shared libraries.\n>\n>I'll add on that I'm not opposed to continuing to support NonStop in principle, but I\n>_am_ opposed to having to continue to support really ancient versions of common\n>standards[0]. POSIX 1003.1-2008 is about 16 years old; it could drive a car in much\n>of North America. I think it's the responsibility of porters to provide this\n>functionality, and in general most open source OSes do this with little or no paid\n>staff, so it should be reasonable to expect corporate-backed operating systems to\n>do so.\n\nI agree with that, but I have very little influence on what the NonStop team does internally. I am not on team.\n\n>I would strongly recommend that NonStop start supporting modern compiler and\n>POSIX standards, as well at least one of GCC or LLVM, and that you push very hard\n>to make that happen. It's not just Git that's pushing forward here; there's a lot of\n>open source software that simply will not compile with the functionality you\n>support, including the increasing amount of software written in Go or Rust, and it's\n>unreasonable to expect people not to want to use standard functionality that's over\n>a decade old. If NonStop doesn't move in that direction, I anticipate we will\n>eventually drop support for it sooner or later.\n\nThis is a great recommendation that will not happen any time soon. It is something the community has requested repeatedly but there are inside reasons for not doing this. My team spent years trying to port GCC without success (I cannot go into why, but it is non-trivial and will not happen). At least on the more modern NonStop x86, we do have C11, just not on ia64. With the latter going off support relatively soon, we could move to C11 safely in about 2 years. With that notice, it would give us time to adapt, but would absolutely kick git to the curb on ia64.\n\nThis does bring up the question of how much notice is needed (or at worst considered \"considerate\") to allow platforms to cope with fundamental architectural dependency changes in git.\n\n>[0] This is not just NonStop; Microsoft's long-time refusal to implement\n>C99 or a newer version of C really ground my gears and I basically gave up on\n>supporting it for all personal projects as a result. Fortunately, they've now come up\n>to a modern standard.\n>--\n>brian m. carlson (they/them or he/him)\n>Toronto, Ontario, CA\n\nI have to say that I this thread is making me very nervous about the future of a supportable git on anything but Linux.\n\n"},{"id":"492586","messageId":"xmqqr0ff8rwo.fsf@gitster.g","threadId":"61252","inReplyTo":"ZhND3hZXlzDxff5e@tapette.crustytoothpaste.net","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T21:29:27Z","receivedAt":"2024-04-08T21:29:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> As mentioned in the original proposal, we don't have to support all\n> platforms in the libified code. The main Git binaries will continue to\n> function and be supported, but the new libified code will rely on newer\n> features. You will still be able to have all the Git binaries and\n> functionality, but if you want the new shared library to compile\n> against, you'll have to furnish a newer compiler.\n\nI thought one of the yardstick to gauge the success of this\n\"libification\" effort, if not the purpose of this effort, is to\nallow Git to be its first client.\n\nI am not sure how it would supposed to work. Unless you are giving\nparallel implementations of \"main Git binaries\", one with the native\ncode and the other replaced the native code with thin wrappers\naround the library calls, that is.\n\nPuzzled...\n\n"},{"id":"492601","messageId":"ZhSNPTiaPbfekOrJ@tapette.crustytoothpaste.net","threadId":"61252","inReplyTo":"xmqqr0ff8rwo.fsf@gitster.g","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-04-09T00:35:09Z","receivedAt":"2024-04-09T00:35:18Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-04-08 at 21:29:27, Junio C Hamano wrote:\n> I thought one of the yardstick to gauge the success of this\n> \"libification\" effort, if not the purpose of this effort, is to\n> allow Git to be its first client.\n> \n> I am not sure how it would supposed to work. Unless you are giving\n> parallel implementations of \"main Git binaries\", one with the native\n> code and the other replaced the native code with thin wrappers\n> around the library calls, that is.\n\nI think the plan as proposed in the original file was to have an\ninternal and external library and to have the binaries use the internal\nlibrary. However, perhaps I misunderstood the proposal, in which case\nclarification on the part of the proposers would be helpful.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"492625","messageId":"5663500c-ea40-45a6-bb7d-c906aee4350c@gmail.com","threadId":"61252","inReplyTo":"xmqqr0ff8rwo.fsf@gitster.g","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-09T09:40:17Z","receivedAt":"2024-04-09T09:40:21Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/04/2024 22:29, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n>> As mentioned in the original proposal, we don't have to support all\n>> platforms in the libified code. The main Git binaries will continue to\n>> function and be supported, but the new libified code will rely on newer\n>> features. You will still be able to have all the Git binaries and\n>> functionality, but if you want the new shared library to compile\n>> against, you'll have to furnish a newer compiler.\n> \n> I thought one of the yardstick to gauge the success of this\n> \"libification\" effort, if not the purpose of this effort, is to\n> allow Git to be its first client.\n\nIndeed, the last set of patches allow git to be built with the same \nlibrary that external programs can use which I thought was very welcome. \nThis proposal seems to be backing away from that.\n\nWe could have a single version of the library with a set of external \nheaders that export a limited set of functions in a gitlib_ namespace \nand are wrapped internally with definitions like\n\nstatic inline int foo(int x)\n{\n\treturn libgit_foo(x);\n}\n\nbut that still leaves the problem of symbol visibility for the symbols \nthat are consumed internally but not exposed in the external headers.\n\nBest Wishes\n\nPhillip\n"},{"id":"492643","messageId":"CAFySSZBMGx_Uvak7WEurwWk00=6o3SciJnN9zAGbvTTbBk=Axg@mail.gmail.com","threadId":"61252","inReplyTo":"ZhSNPTiaPbfekOrJ@tapette.crustytoothpaste.net","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-04-09T17:26:26Z","receivedAt":"2024-04-09T17:26:39Z","isPatch":false,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Mon, Apr 8, 2024 at 5:35 PM brian m. carlson\n wrote:\n>\n> On 2024-04-08 at 21:29:27, Junio C Hamano wrote:\n> > I thought one of the yardstick to gauge the success of this\n> > \"libification\" effort, if not the purpose of this effort, is to\n> > allow Git to be its first client.\n> >\n> > I am not sure how it would supposed to work. Unless you are giving\n> > parallel implementations of \"main Git binaries\", one with the native\n> > code and the other replaced the native code with thin wrappers\n> > around the library calls, that is.\n>\n> I think the plan as proposed in the original file was to have an\n> internal and external library and to have the binaries use the internal\n> library. However, perhaps I misunderstood the proposal, in which case\n> clarification on the part of the proposers would be helpful.\n\nWe are still working on the specifics of how we would accomplish this\n(part of the reason this is marked RFD). Thin wrappers make sense for\nfunctions that should be slightly altered for external users (e.g.\nanything with a die() should have their error codes bubbled up). On\nthe other hand, there are functions that are already suitable for\nexternal users out of the box, so we could just provide a separate\nheader for those. But those functions may still need to be renamed\nanyways to avoid symbol collision, so thin wrappers may end up being\nthe default going forward.\n"},{"id":"492644","messageId":"CAFySSZBC6RMC3rqcMVNKUbzM_a6n5sAnKCo2rqpHmwTS4R0=GQ@mail.gmail.com","threadId":"61252","inReplyTo":"5663500c-ea40-45a6-bb7d-c906aee4350c@gmail.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-04-09T17:30:54Z","receivedAt":"2024-04-09T17:31:06Z","isPatch":false,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Tue, Apr 9, 2024 at 2:40 AM Phillip Wood wrote:\n>\n> Indeed, the last set of patches allow git to be built with the same\n> library that external programs can use which I thought was very welcome.\n> This proposal seems to be backing away from that.\n\nOne of the questions I was grappling with was, do we want to expose\nall functions to external users? While it is debatable whether we\nwould like that exposure for some of the files in libstdgit, functions\nin wrapper.c and usage.c seem like clear candidates for functions that\nhave no value being exposed externally while also making it more\ndifficult to maintain such a library in the future.\n"},{"id":"493312","messageId":"20240422162617.308366-1-calvinwan@google.com","threadId":"61252","inReplyTo":"CAFySSZAB09QB7U6UxntK2jRJF0df5R7YGnnLSsYc9MYhHsBhWA@mail.gmail.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-04-22T16:26:17Z","receivedAt":"2024-04-22T16:26:29Z","isPatch":false,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Thanks everyone for your initial comments on this discussion. I wanted\nto provide some examples of how an internal/external interface could\nlook in practice -- originally I had intended to use git-std-lib v6 as\nthat example, but found that it fell short due to feedback that only\nbeing able to expose a smaller subset of functions in that library would\nbe insufficient for users since they should have the same tools that we\nhave for building Git. In this reply, I have two examples of paths\nforward that such an interface could look like for future libraries\n(both methods would require a non-trivial amount of code change so this\nseemed like a better idea than completely refactoring git-std-lib twice).\n\nPart of the reason for wanting to expose a smaller subset of library\nfunctions initially was to avoid having to expose functions that do\nthings a library function shouldn't, mainly those with die() calls. I\nchose `strbuf_grow()` as the example function to be libified with an\ninternal/external interface since it has a die() call and in a library,\nwe would want to pass that error up rather than die()ing. I have two\nideas for how such an interface could look. For reference, this is how\n`strbuf_grow()` currently looks:\n\nvoid strbuf_grow(struct strbuf *sb, size_t extra)\n{\n\tint new_buf = !sb->alloc;\n\tif (unsigned_add_overflows(extra, 1) ||\n\t unsigned_add_overflows(sb->len, extra + 1))\n\t\tdie(\"you want to use way too much memory\");\n\tif (new_buf)\n\t\tsb->buf = NULL;\n\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n\tif (new_buf)\n\t\tsb->buf[0] = '\\0';\n}\n\nThe first idea involves turning `strbuf_grow()` into a wrapper function\nthat invokes its equivalent library function, eg.\n`libgit_strbuf_grow()`:\n\nint libgit_strbuf_grow(struct strbuf *sb, size_t extra)\n{\n\tint new_buf = !sb->alloc;\n\tif (unsigned_add_overflows(extra, 1) ||\n\t unsigned_add_overflows(sb->len, extra + 1))\n\t\treturn -1;\n\tif (new_buf)\n\t\tsb->buf = NULL;\n\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n\tif (new_buf)\n\t\tsb->buf[0] = '\\0';\n return 0;\n}\n\nvoid strbuf_grow(struct strbuf *sb, size_t extra)\n{\n if (libgit_strbuf_grow(sb, extra))\n die(\"you want to use way too much memory\");\n}\n\n(Note a context object could also be added as a parameter to\n`libgit_strbuf_grow()` for error messages and possibly global variables.)\n\nIn this scenario, we would be exposing `libgit_strbuf_grow()` to\nexternal consumers of the library, while not having to refactor internal\nuses of `strbuf_grow()`. This method would reduce initial churn within\nthe codebase, however, we would want to eventually get rid of\n`strbuf_grow()` and use `libgit_strbuf_grow()` internally as well. I\nenvision that it would be easier to remove die()'s all at once, from top\ndown, once libification has progressed further since top level callers\ndo not have to worry about refactoring any callers to accomodate passing\nup error messages/codes. \n\nThe shortfall of this approach is that we'd be carrying two different\nfunctions for every library function until we are able to remove all of\nthem. It would also create additional toil for Git contributors to\nfigure out which version of the function should be used.\n\nThe second idea removes the need for two different functions by removing\nthe wrapper function and instead refactoring all callers of\n`strbuf_grow()` (and subsequently callers of other library functions).\n\nint libgit_strbuf_grow(struct strbuf *sb, size_t extra)\n{\n\tint new_buf = !sb->alloc;\n\tif (unsigned_add_overflows(extra, 1) ||\n\t unsigned_add_overflows(sb->len, extra + 1))\n\t\treturn -1;\n\tif (new_buf)\n\t\tsb->buf = NULL;\n\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n\tif (new_buf)\n\t\tsb->buf[0] = '\\0';\n return 0;\n}\n\nvoid strbuf_grow_caller() {\n\tstrbuf *sb;\n\tsize_t extra;\n\n\t// if only success/failure is passed up\n\tif (libgit_strbuf_grow(sb, extra))\n die(\"you want to use way too much memory\");\n\t\n\t// if context object is used\n\tif (libgit_strbuf_grow(sb, extra, context_obj))\n die(context_obj->error_msg);\n\t\n\t// if there are multiple error codes that can be passed up\n\tif (libgit_strbuf_grow(sb, extra) == -1)\n die(\"you want to use way too much memory\");\n\telse if (libgit_strbuf_grow(sb, extra) == -2)\n\t\tdie(\"some other error\");\n}\n\nOne shortcoming of this approach is the need to refactor all callers of\nlibrary functions, but that can be handled better and the churn made\nmore visible with a coccinelle patch. Another shortcoming is the need\nfor lengthier code blocks whenever calling a library function, however,\nit could also be seen as a benefit since the caller would understand the\nfunction can die(). These error messages would also ideally be passed up\nas well in the future rather than die()ing.\n\nWhile I tried to find a solution that avoided the shortcomings of both\napproaches, I think that answer simply does not exist so the ideas above\nare what I believe to be the least disruptive options. I'm wondering\nwhich interface would be more suitable, and also open to hearing if\nthere are any other ideas!\n"},{"id":"493318","messageId":"xmqqpluhuoos.fsf@gitster.g","threadId":"61252","inReplyTo":"20240422162617.308366-1-calvinwan@google.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-22T20:28:51Z","receivedAt":"2024-04-22T20:28:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\nThanks for writing this down.\n\n> The first idea involves turning `strbuf_grow()` into a wrapper function\n> that invokes its equivalent library function, eg.\n> `libgit_strbuf_grow()`:\n>\n> int libgit_strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> \tint new_buf = !sb->alloc;\n> \tif (unsigned_add_overflows(extra, 1) ||\n> \t unsigned_add_overflows(sb->len, extra + 1))\n> \t\treturn -1;\n> \tif (new_buf)\n> \t\tsb->buf = NULL;\n> \tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> \tif (new_buf)\n> \t\tsb->buf[0] = '\\0';\n> return 0;\n> }\n>\n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> if (libgit_strbuf_grow(sb, extra))\n> die(\"you want to use way too much memory\");\n> }\n>\n> (Note a context object could also be added as a parameter to\n> `libgit_strbuf_grow()` for error messages and possibly global variables.)\n>\n> In this scenario, we would be exposing `libgit_strbuf_grow()` to\n> external consumers of the library, while not having to refactor internal\n> uses of `strbuf_grow()`.\n\nYes, this is how I envision the things will go. And it is a good\nplace to STOP for \"git\" the command line program everybody knows for\nthe past 20 years. It is good that you mentioned the \"context\"\nhere, too. As to the naming, I am OK with \"libgit_\". Just wanted\nto say that \"libgit2\" does not seem to use it as their prefix (and\nthey do not use \"libgit2_\" as their prefix, either) so if somebody\nwants to bikeshed, they do not have to worry about them.\n\n> This method would reduce initial churn within\n> the codebase, however, we would want to eventually get rid of\n> `strbuf_grow()` and use `libgit_strbuf_grow()` internally as well. \n\nThe \"however\" and everything that follows wants justification.\n\nI think after we pass the \"Traditional API git offered are thin\nwrappers around git-std-lib\" point, the returns (the \"clean-up\"\nvalue) will quickly diminish. Even with diminished returns, there\nmay still be \"clean-up\" value left, but it would be simpler to\nconsider that as outside the scope of \"libification\" discussion.\nOnce the \"Traditional API are thin wrappers\" state is achieved, the\n\"libification\" is done.\n\n> The second idea removes the need for two different functions by removing\n> the wrapper function and instead refactoring all callers of\n> `strbuf_grow()` (and subsequently callers of other library functions).\n> ...\n> One shortcoming of this approach is the need to refactor all callers of\n> library functions, but that can be handled better and the churn made\n\nThere is one huge downside you did not mention.\n\nThe appraoch makes \"git the command line program\" to commit to the\n\"errors must percolate all the way up to the top\", and the\naddditional effort to take us there after we have achieved the\nlibification goals has dubious trade-off.\n\nI do not see why it bothers you, the libification person when he\nwears git-std-lib hat, that \"git the command line program\" that is a\nmere one of customers of git-std-lib happens to have a function\nwhose name is strbuf_grow() and uses libgit_strbuf_grow(). It\nshouldn't bother you more than \"git the command line program\" having\na program called cmd_show() that uses many functions from libgit_\nsuite. After we reach the \"our implementation of traditional API\nare all wrappers around git-std-lib API functions\" [*] state, we may\ndecide to refactor further and may replace those \"wrappers\" with\ndirect calls, but that can happen far in the future, when the\nlibified result is rock solid.\n\nIf you try to do everything from the top to bottom at once, you\ncannot easily keep the system (the combination of git-std-lib still\nwork-in-progress plus the client code that is \"git the command line\nprogram\") as solid as the first approach.\n"},{"id":"493356","messageId":"cb2ffdb1-5e6c-4dd3-89d4-f0b31f2f17ec@gmail.com","threadId":"61252","inReplyTo":"20240422162617.308366-1-calvinwan@google.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-23T09:57:31Z","receivedAt":"2024-04-23T09:57:36Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Calvin\n\nOn 22/04/2024 17:26, Calvin Wan wrote:\n> The first idea involves turning `strbuf_grow()` into a wrapper function\n> that invokes its equivalent library function, eg.\n> `libgit_strbuf_grow()`:\n> \n> int libgit_strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> \tint new_buf = !sb->alloc;\n> \tif (unsigned_add_overflows(extra, 1) ||\n> \t unsigned_add_overflows(sb->len, extra + 1))\n> \t\treturn -1;\n> \tif (new_buf)\n> \t\tsb->buf = NULL;\n> \tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> \tif (new_buf)\n> \t\tsb->buf[0] = '\\0';\n> return 0;\n> }\n> \n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> if (libgit_strbuf_grow(sb, extra))\n> die(\"you want to use way too much memory\");\n> }\n> \n> (Note a context object could also be added as a parameter to\n> `libgit_strbuf_grow()` for error messages and possibly global variables.)\n\nI agree with Junio that this is a good route forward and that we should \nnot consider removing the wrappers to be part of the libification \nproject. ALLOC_GROW() can die so I think we'd need a way to propagate an \nerror message up to the wrapper even for relatively simple functions \nlike this. For the allocation functions we'd either need to use a static \nstring for the message which is not a good fit for other functions that \nwant to dynamically format their messages (for example to include a path \nname into the error message), or pre-allocate the error messages.\n\n> The shortfall of this approach is that we'd be carrying two different\n> functions for every library function until we are able to remove all of\n> them. It would also create additional toil for Git contributors to\n> figure out which version of the function should be used.\n\nHopefully it should be clear if a function is part of the library or not \nso I don't think this should be a big problem.\n\nBest Wishes\n\nPhillip\n"},{"id":"494340","messageId":"CAO_smVg_RmL2_ERXG0uZ+CgEHZBm34PvmNucbSTdsEDLjU-fhA@mail.gmail.com","threadId":"61252","inReplyTo":"20240422162617.308366-1-calvinwan@google.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-05-09T01:00:24Z","receivedAt":"2024-05-09T01:00:41Z","isPatch":false,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"Apologies for being absent from a thread that I was the initial author\nof, there was a family emergency.\n\nOn Mon, Apr 22, 2024 at 9:26 AM Calvin Wan wrote:\n>\n> Thanks everyone for your initial comments on this discussion. I wanted\n> to provide some examples of how an internal/external interface could\n> look in practice -- originally I had intended to use git-std-lib v6 as\n> that example, but found that it fell short due to feedback that only\n> being able to expose a smaller subset of functions in that library would\n> be insufficient for users since they should have the same tools that we\n> have for building Git. In this reply, I have two examples of paths\n> forward that such an interface could look like for future libraries\n> (both methods would require a non-trivial amount of code change so this\n> seemed like a better idea than completely refactoring git-std-lib twice).\n>\n> Part of the reason for wanting to expose a smaller subset of library\n> functions initially was to avoid having to expose functions that do\n> things a library function shouldn't, mainly those with die() calls. I\n> chose `strbuf_grow()` as the example function to be libified with an\n> internal/external interface since it has a die() call and in a library,\n> we would want to pass that error up rather than die()ing. I have two\n> ideas for how such an interface could look. For reference, this is how\n> `strbuf_grow()` currently looks:\n>\n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> int new_buf = !sb->alloc;\n> if (unsigned_add_overflows(extra, 1) ||\n> unsigned_add_overflows(sb->len, extra + 1))\n> die(\"you want to use way too much memory\");\n> if (new_buf)\n> sb->buf = NULL;\n> ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> if (new_buf)\n> sb->buf[0] = '\\0';\n> }\n>\n> The first idea involves turning `strbuf_grow()` into a wrapper function\n> that invokes its equivalent library function, eg.\n> `libgit_strbuf_grow()`:\n>\n> int libgit_strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> int new_buf = !sb->alloc;\n> if (unsigned_add_overflows(extra, 1) ||\n> unsigned_add_overflows(sb->len, extra + 1))\n> return -1;\n> if (new_buf)\n> sb->buf = NULL;\n> ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> if (new_buf)\n> sb->buf[0] = '\\0';\n> return 0;\n> }\n>\n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> if (libgit_strbuf_grow(sb, extra))\n> die(\"you want to use way too much memory\");\n> }\n>\n\nWhen writing the initial document, I was thinking of something almost\nthe exact opposite of this, but I hadn't been thinking about the\nproblems with the code calling `die` and similar. I was thinking along\nthe lines of:\n\n// In a .c file that is \"library internal\".\n// This translation unit can assume that we've done #include\n\"git-compat-util.h\" and anything else it wants.\nint strbuf_grow(struct strbuf *sb, size_t extra)\n{\n int new_buf = !sb->alloc;\n if (unsigned_add_overflows(extra, 1) ||\n unsigned_add_overflows(sb->len, extra + 1))\n return -1;\n if (new_buf)\n sb->buf = NULL;\n ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n if (new_buf)\n sb->buf[0] = '\\0';\n return 0;\n}\n\n// In a .c file for the interface as used by other projects:\nint gitlib_strbuf_grow(struct strbuf *sb, size_t extra) // Or maybe int64_t?\n{\n return strbuf_grow(sb, extra);\n}\n\nAfter reading this thread, I agree that this isn't sufficient to avoid\nchurn, and I like the proposed interface, but with one main tweak:\n\n// RENAMED from previous code block (no other changes)\n// In a .c file that is \"library internal\".\n// This translation unit can assume that we've done #include\n\"git-compat-util.h\" and anything else it wants.\nint strbuf_grow_impl(struct strbuf *sb, size_t extra)\n{\n int new_buf = !sb->alloc;\n if (unsigned_add_overflows(extra, 1) ||\n unsigned_add_overflows(sb->len, extra + 1))\n return -1;\n if (new_buf)\n sb->buf = NULL;\n ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n if (new_buf)\n sb->buf[0] = '\\0';\n return 0;\n}\n\n// In a .c file for the interface as used by other projects:\nint gitlib_strbuf_grow(struct strbuf *sb, size_t extra)\n{\n return strbuf_grow_impl(sb, extra);\n}\n\n// NEW from previous code block\n// In a .c file for the interface as used by the git project itself:\nvoid strbuf_grow(struct strbuf *sb, size_t extra)\n{\n if (strbuf_grow_impl(sb, extra))\n die(\"you want to use way too much memory\")\n}\n\nI'm recommending this pattern primarily because of our platform\nsupport concerns. If we can't elevate the entire project to assume\nthat C99 is available in a standards compliant way, we can't have\nheader files that look like this be part of building the `git` binary\nitself (or any of the helper binaries):\n\n#include /* Our platform support policy doesn't allow this */\nint gitlib_strbuf_grow(struct strbuf *sb, size_t extra);\n\nIt's not just the #includes, though. As stated in the original\ndocument, we run into problems with platform-defined types and\neverything else that's tweaked in/provided by git-compat-util.h:\n- This header file that's included in the non-git projects can't use\n`off_t` or `struct stat`.\n- This header file can't assume that any types related to sockets are\navailable, because those come from on Linux and from\nwinsock2.h on Windows.\n- It can't assume that we have `NORETURN` (and it can't assume that we\ndon't need it), or `MAYBE_UNUSED`, or ...\n\nMost of those issues _may_ be able to be resolved by having a\n\"gitlib-compat-util.h\" file included at the top of the \"external\nproject\" .h file. But that's insufficient. Example:\n\n#include \n#include \"git/gitlib.h\" // Oops, the `#define _GNU_SOURCE` in the\ntransitive \"gitlib-compat-util.h\" has no effect!\n\nOr the opposite:\n\n#include \"git/gitlib.h\" // Oops, this set _FILE_OFFSET_BITS=64 when\nthe project wasn't expecting it!\n#include // For this translation unit only, `off_t` might\nbe a different size than elsewhere in the project, I hope you like\ndebugging segfaults.\n\nThe only ways I could come up with to solve these problems were to\nhold the \"external interface\" to a different standard, that is\nsimultaneously both more permissive (it can assume C99), and\nrestrictive (it can't rely on things like off_t), and this makes it\nincompatible with these external interfaces being used by the git\nproject itself, which has a broader set of platforms it needs to\nsupport. But the external interfaces must be very simple wrappers\naround code that _is_ shared with the git executable.\n\n> (Note a context object could also be added as a parameter to\n> `libgit_strbuf_grow()` for error messages and possibly global variables.)\n>\n> In this scenario, we would be exposing `libgit_strbuf_grow()` to\n> external consumers of the library, while not having to refactor internal\n> uses of `strbuf_grow()`. This method would reduce initial churn within\n> the codebase, however, we would want to eventually get rid of\n> `strbuf_grow()` and use `libgit_strbuf_grow()` internally as well. I\n> envision that it would be easier to remove die()'s all at once, from top\n> down, once libification has progressed further since top level callers\n> do not have to worry about refactoring any callers to accomodate passing\n> up error messages/codes.\n>\n> The shortfall of this approach is that we'd be carrying two different\n> functions for every library function until we are able to remove all of\n> them. It would also create additional toil for Git contributors to\n> figure out which version of the function should be used.\n>\n> The second idea removes the need for two different functions by removing\n> the wrapper function and instead refactoring all callers of\n> `strbuf_grow()` (and subsequently callers of other library functions).\n>\n> int libgit_strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> int new_buf = !sb->alloc;\n> if (unsigned_add_overflows(extra, 1) ||\n> unsigned_add_overflows(sb->len, extra + 1))\n> return -1;\n> if (new_buf)\n> sb->buf = NULL;\n> ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> if (new_buf)\n> sb->buf[0] = '\\0';\n> return 0;\n> }\n>\n> void strbuf_grow_caller() {\n> strbuf *sb;\n> size_t extra;\n>\n> // if only success/failure is passed up\n> if (libgit_strbuf_grow(sb, extra))\n> die(\"you want to use way too much memory\");\n>\n> // if context object is used\n> if (libgit_strbuf_grow(sb, extra, context_obj))\n> die(context_obj->error_msg);\n>\n> // if there are multiple error codes that can be passed up\n> if (libgit_strbuf_grow(sb, extra) == -1)\n> die(\"you want to use way too much memory\");\n> else if (libgit_strbuf_grow(sb, extra) == -2)\n> die(\"some other error\");\n> }\n>\n> One shortcoming of this approach is the need to refactor all callers of\n> library functions, but that can be handled better and the churn made\n> more visible with a coccinelle patch. Another shortcoming is the need\n> for lengthier code blocks whenever calling a library function, however,\n> it could also be seen as a benefit since the caller would understand the\n> function can die(). These error messages would also ideally be passed up\n> as well in the future rather than die()ing.\n>\n> While I tried to find a solution that avoided the shortcomings of both\n> approaches, I think that answer simply does not exist so the ideas above\n> are what I believe to be the least disruptive options. I'm wondering\n> which interface would be more suitable, and also open to hearing if\n> there are any other ideas!\n"},{"id":"494377","messageId":"CAO_smVhjtvHeDR6KYNJL+hzc5uheWjshaayQPN1PkBhRaFTqOQ@mail.gmail.com","threadId":"61252","inReplyTo":"20240422162617.308366-1-calvinwan@google.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-05-09T19:45:51Z","receivedAt":"2024-05-09T19:46:12Z","isPatch":false,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Mon, Apr 22, 2024 at 9:26 AM Calvin Wan wrote:\n>\n> Thanks everyone for your initial comments on this discussion. I wanted\n> to provide some examples of how an internal/external interface could\n> look in practice -- originally I had intended to use git-std-lib v6 as\n> that example, but found that it fell short due to feedback that only\n> being able to expose a smaller subset of functions in that library would\n> be insufficient for users since they should have the same tools that we\n> have for building Git. In this reply, I have two examples of paths\n> forward that such an interface could look like for future libraries\n> (both methods would require a non-trivial amount of code change so this\n> seemed like a better idea than completely refactoring git-std-lib twice).\n>\n> Part of the reason for wanting to expose a smaller subset of library\n> functions initially was to avoid having to expose functions that do\n> things a library function shouldn't, mainly those with die() calls. I\n> chose `strbuf_grow()` as the example function to be libified with an\n> internal/external interface since it has a die() call and in a library,\n> we would want to pass that error up rather than die()ing. I have two\n> ideas for how such an interface could look. For reference, this is how\n> `strbuf_grow()` currently looks:\n>\n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> int new_buf = !sb->alloc;\n> if (unsigned_add_overflows(extra, 1) ||\n> unsigned_add_overflows(sb->len, extra + 1))\n> die(\"you want to use way too much memory\");\n> if (new_buf)\n> sb->buf = NULL;\n> ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> if (new_buf)\n> sb->buf[0] = '\\0';\n> }\n>\n> The first idea involves turning `strbuf_grow()` into a wrapper function\n> that invokes its equivalent library function, eg.\n> `libgit_strbuf_grow()`:\n>\n> int libgit_strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> int new_buf = !sb->alloc;\n> if (unsigned_add_overflows(extra, 1) ||\n> unsigned_add_overflows(sb->len, extra + 1))\n> return -1;\n> if (new_buf)\n> sb->buf = NULL;\n> ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> if (new_buf)\n> sb->buf[0] = '\\0';\n> return 0;\n> }\n>\n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> if (libgit_strbuf_grow(sb, extra))\n> die(\"you want to use way too much memory\");\n> }\n>\n> (Note a context object could also be added as a parameter to\n> `libgit_strbuf_grow()` for error messages and possibly global variables.)\n>\n> In this scenario, we would be exposing `libgit_strbuf_grow()` to\n> external consumers of the library, while not having to refactor internal\n> uses of `strbuf_grow()`. This method would reduce initial churn within\n> the codebase, however, we would want to eventually get rid of\n> `strbuf_grow()` and use `libgit_strbuf_grow()` internally as well. I\n> envision that it would be easier to remove die()'s all at once, from top\n> down, once libification has progressed further since top level callers\n> do not have to worry about refactoring any callers to accomodate passing\n> up error messages/codes.\n>\n> The shortfall of this approach is that we'd be carrying two different\n> functions for every library function until we are able to remove all of\n> them. It would also create additional toil for Git contributors to\n> figure out which version of the function should be used.\n>\n> The second idea removes the need for two different functions by removing\n> the wrapper function and instead refactoring all callers of\n> `strbuf_grow()` (and subsequently callers of other library functions).\n>\n> int libgit_strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> int new_buf = !sb->alloc;\n> if (unsigned_add_overflows(extra, 1) ||\n> unsigned_add_overflows(sb->len, extra + 1))\n> return -1;\n> if (new_buf)\n> sb->buf = NULL;\n> ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> if (new_buf)\n> sb->buf[0] = '\\0';\n> return 0;\n> }\n>\n> void strbuf_grow_caller() {\n> strbuf *sb;\n> size_t extra;\n>\n> // if only success/failure is passed up\n> if (libgit_strbuf_grow(sb, extra))\n> die(\"you want to use way too much memory\");\n>\n> // if context object is used\n> if (libgit_strbuf_grow(sb, extra, context_obj))\n> die(context_obj->error_msg);\n>\n> // if there are multiple error codes that can be passed up\n> if (libgit_strbuf_grow(sb, extra) == -1)\n> die(\"you want to use way too much memory\");\n> else if (libgit_strbuf_grow(sb, extra) == -2)\n> die(\"some other error\");\n> }\n\nThought about this some more last night, and I think we'll _need_ to\ngo with this approach for most of the libraries (all but the ones used\nby builtin/*.c, which can have a wrapper that preserves existing\nfunctionality), otherwise the libraries aren't composable. If we have\nthree library interfaces, with interface C depending on B which\ndepends on A, C needs to call the library safe versions of B, and B\nneeds to call the library safe versions of A, which means that the\n\"internal\" version needs to be the library safe version. So I think\nthat we're back to my original proposal:\n\n// Library-safe method, used internally\nint strbuf_grow(struct strbuf *sb, size_t extra)\n{\n int new_buf = !sb->alloc;\n if (unsigned_add_overflows(extra, 1) ||\n unsigned_add_overflows(sb->len, extra + 1))\n return -1;\n if (new_buf)\n sb->buf = NULL;\n ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n if (new_buf)\n sb->buf[0] = '\\0';\n return 0;\n}\n\n// \"External\" version\nint libgit_strbuf_grow(struct strbuf *sb, size_t extra) // Or maybe int64_t?\n{\n strbuf_grow(sb, extra);\n}\n\nI don't expect there's many cases where we want to create a wrapper\nthat maintains the existing interface and error handling, because that\nwrapper can _only_ by used by the git project binaries, not any of the\ncode that's in a library.\n\n>\n> One shortcoming of this approach is the need to refactor all callers of\n> library functions, but that can be handled better and the churn made\n> more visible with a coccinelle patch. Another shortcoming is the need\n> for lengthier code blocks whenever calling a library function, however,\n> it could also be seen as a benefit since the caller would understand the\n> function can die(). These error messages would also ideally be passed up\n> as well in the future rather than die()ing.\n>\n> While I tried to find a solution that avoided the shortcomings of both\n> approaches, I think that answer simply does not exist so the ideas above\n> are what I believe to be the least disruptive options. I'm wondering\n> which interface would be more suitable, and also open to hearing if\n> there are any other ideas!\n"},{"id":"494378","messageId":"xmqqttj6spy4.fsf@gitster.g","threadId":"61252","inReplyTo":"CAO_smVhjtvHeDR6KYNJL+hzc5uheWjshaayQPN1PkBhRaFTqOQ@mail.gmail.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-09T20:14:27Z","receivedAt":"2024-05-09T20:14:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott writes:\n\n> I don't expect there's many cases where we want to create a wrapper\n> that maintains the existing interface and error handling, because that\n> wrapper can _only_ by used by the git project binaries, not any of the\n> code that's in a library.\n\nIn other words, such a code belongs to the \"glue\" layer that updates\nthe internal implementation detail of \"git\" so that we no longer use\nthe original set of helper functions and instead make use of the\n\"library\". They are not part of \"library\" proper, as among users of\nthe same \"library\", the \"glue\" is needed only by \"git\" itself.\n\nSo, yes, you may have to create such wrappers if the library wants\nto be used by \"git\" itself, but the wrappers should be kept outside\nthe library.\n\n"},{"id":"494414","messageId":"12a4cd37-1cef-45a0-9b96-36a978e52dba@gmail.com","threadId":"61252","inReplyTo":"CAO_smVg_RmL2_ERXG0uZ+CgEHZBm34PvmNucbSTdsEDLjU-fhA@mail.gmail.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-05-10T09:52:14Z","receivedAt":"2024-05-10T09:52:16Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Kyle\n\nOn 09/05/2024 02:00, Kyle Lippincott wrote:\n> // RENAMED from previous code block (no other changes)\n> // In a .c file that is \"library internal\".\n> // This translation unit can assume that we've done #include\n> \"git-compat-util.h\" and anything else it wants.\n> int strbuf_grow_impl(struct strbuf *sb, size_t extra)\n> {\n> int new_buf = !sb->alloc;\n> if (unsigned_add_overflows(extra, 1) ||\n> unsigned_add_overflows(sb->len, extra + 1))\n> return -1;\n> if (new_buf)\n> sb->buf = NULL;\n> ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> if (new_buf)\n> sb->buf[0] = '\\0';\n> return 0;\n> }\n> \n> // In a .c file for the interface as used by other projects:\n> int gitlib_strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> return strbuf_grow_impl(sb, extra);\n> }\n> \n> // NEW from previous code block\n> // In a .c file for the interface as used by the git project itself:\n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> if (strbuf_grow_impl(sb, extra))\n> die(\"you want to use way too much memory\")\n> }\n> \n> I'm recommending this pattern primarily because of our platform\n> support concerns. If we can't elevate the entire project to assume\n> that C99 is available in a standards compliant way, we can't have\n> header files that look like this be part of building the `git` binary\n> itself (or any of the helper binaries):\n> \n> #include /* Our platform support policy doesn't allow this */\n> int gitlib_strbuf_grow(struct strbuf *sb, size_t extra);\n\nWe have had a test balloon [1] requiring C99 for two and a half years \nwithout any bug reports so I think we are probably safe to assume the \ntest balloon has succeeded and that we can depend on on the presence of \n. Note that the [u]intptr_t types are optional and so we'd \nneed to make sure we avoid them in public interfaces.\n\n[1] 7bc341e21b5 (git-compat-util: add a test balloon for C99 support, \n2021-12-01)\n\n> It's not just the #includes, though. As stated in the original\n> document, we run into problems with platform-defined types and\n> everything else that's tweaked in/provided by git-compat-util.h:\n> - This header file that's included in the non-git projects can't use\n> `off_t` or `struct stat`.\n\nIn principle we could change the interfaces that currently use `off_t` \nto use `int64_t` and convert to `off_t` in the function body which would \navoid having to have separate wrappers for the internal and external \ncallers. I'm not sure how invasive that would be though. `struct stat` \nis trickier - where do we expose that in our interfaces?\n\n> - This header file can't assume that any types related to sockets are\n> available, because those come from on Linux and from\n> winsock2.h on Windows.\n> - It can't assume that we have `NORETURN` (and it can't assume that we\n> don't need it), or `MAYBE_UNUSED`, or ...\n\nThese problems and the _GNU_SOURCE on you mention below must be pretty \ncommon for cross-platform libraries - how do other projects handle them?\nOn the face of it this seems like it would be fairly simple to solve by \nincluding a file that contains the subset of git-compat-util.h that \ndefines these macros (with a suitable LIBGIT_ prefix) in libgit.h.\n\n> Most of those issues _may_ be able to be resolved by having a\n> \"gitlib-compat-util.h\" file included at the top of the \"external\n> project\" .h file. But that's insufficient. Example:\n> \n> #include \n> #include \"git/gitlib.h\" // Oops, the `#define _GNU_SOURCE` in the\n> transitive \"gitlib-compat-util.h\" has no effect!\n> \n> Or the opposite:\n> \n> #include \"git/gitlib.h\" // Oops, this set _FILE_OFFSET_BITS=64 when\n> the project wasn't expecting it!\n> #include // For this translation unit only, `off_t` might\n> be a different size than elsewhere in the project, I hope you like\n> debugging segfaults.\n> \n> The only ways I could come up with to solve these problems were to\n> hold the \"external interface\" to a different standard, that is\n> simultaneously both more permissive (it can assume C99), and\n> restrictive (it can't rely on things like off_t),\n >\n> incompatible with these external interfaces being used by the git\n> project itself, which has a broader set of platforms it needs to\n> support. But the external interfaces must be very simple wrappers\n> around code that _is_ shared with the git executable.\n\nI agree we should minimize the amount of code in the wrappers for \nexternal callers.\n\nBest Wishes\n\nPhillip\n"},{"id":"494513","messageId":"CAO_smVgEkLN=rFDEDM-JV9mXQK4CCx0u5W1d+YXse0Qy3QMnsA@mail.gmail.com","threadId":"61252","inReplyTo":"12a4cd37-1cef-45a0-9b96-36a978e52dba@gmail.com","subject":"Re: [RFD] Libification proposal: separate internal and external interfaces","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-05-10T21:35:52Z","receivedAt":"2024-05-10T21:36:07Z","isPatch":false,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Fri, May 10, 2024 at 2:52 AM Phillip Wood wrote:\n>\n> Hi Kyle\n>\n> On 09/05/2024 02:00, Kyle Lippincott wrote:\n> > // RENAMED from previous code block (no other changes)\n> > // In a .c file that is \"library internal\".\n> > // This translation unit can assume that we've done #include\n> > \"git-compat-util.h\" and anything else it wants.\n> > int strbuf_grow_impl(struct strbuf *sb, size_t extra)\n> > {\n> > int new_buf = !sb->alloc;\n> > if (unsigned_add_overflows(extra, 1) ||\n> > unsigned_add_overflows(sb->len, extra + 1))\n> > return -1;\n> > if (new_buf)\n> > sb->buf = NULL;\n> > ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> > if (new_buf)\n> > sb->buf[0] = '\\0';\n> > return 0;\n> > }\n> >\n> > // In a .c file for the interface as used by other projects:\n> > int gitlib_strbuf_grow(struct strbuf *sb, size_t extra)\n> > {\n> > return strbuf_grow_impl(sb, extra);\n> > }\n> >\n> > // NEW from previous code block\n> > // In a .c file for the interface as used by the git project itself:\n> > void strbuf_grow(struct strbuf *sb, size_t extra)\n> > {\n> > if (strbuf_grow_impl(sb, extra))\n> > die(\"you want to use way too much memory\")\n> > }\n> >\n> > I'm recommending this pattern primarily because of our platform\n> > support concerns. If we can't elevate the entire project to assume\n> > that C99 is available in a standards compliant way, we can't have\n> > header files that look like this be part of building the `git` binary\n> > itself (or any of the helper binaries):\n> >\n> > #include /* Our platform support policy doesn't allow this */\n> > int gitlib_strbuf_grow(struct strbuf *sb, size_t extra);\n>\n> We have had a test balloon [1] requiring C99 for two and a half years\n> without any bug reports so I think we are probably safe to assume the\n> test balloon has succeeded and that we can depend on on the presence of\n> .\n\nIn the past 5 months, I have been told that this isn't a valid\nassumption to make yet. I will note that the test balloon just checks\nfor the compiler claiming it supports C99, not that it actually does\nso properly. This is why stdbool is still in test balloon stage\n(8277dbe987 (git-compat-util: convert skip_{prefix,suffix}{,_mem} to\nbool, 2023-12-16)).\n\nIf that's no longer the case, and we _can_ rely on functionality\nspecified by the C99 standard, such as stdint.h existing, then using\nthe same interfaces internally that we expose externally becomes a lot\neasier.\n\n> Note that the [u]intptr_t types are optional and so we'd\n> need to make sure we avoid them in public interfaces.\n>\n> [1] 7bc341e21b5 (git-compat-util: add a test balloon for C99 support,\n> 2021-12-01)\n>\n> > It's not just the #includes, though. As stated in the original\n> > document, we run into problems with platform-defined types and\n> > everything else that's tweaked in/provided by git-compat-util.h:\n> > - This header file that's included in the non-git projects can't use\n> > `off_t` or `struct stat`.\n>\n> In principle we could change the interfaces that currently use `off_t`\n> to use `int64_t` and convert to `off_t` in the function body which would\n> avoid having to have separate wrappers for the internal and external\n> callers. I'm not sure how invasive that would be though. `struct stat`\n> is trickier - where do we expose that in our interfaces?\n\nI hadn't actually checked if we do expose `struct stat` in any\nfunction declaration, it was just an example of something that changes\nsize/behavior due to the #defines. But we do use it in the following\nplaces:\n\nentry.h's `fstat_checkout_output` and `update_ce_after_write`\nreftable/stack.h's `struct reftable_stack` has a `struct stat` member.\n4 uses in git-compat-util.h\nseveral uses in read-cache-ll.h (which sounds like it probably\nwouldn't be exposed in an interface, being lowlevel :))\nstatinfo.h\nobject-file.h's `index_fd` and `index_path`\ndir-iterator.h's `struct dir_iterator` has a `struct stat` member\n\nI may have missed one or two .h files. Maybe all of these are internal\nthings that we won't be exposing in an \"external\" interface, and\nthat's great: no need to worry about it then.\n\n>\n> > - This header file can't assume that any types related to sockets are\n> > available, because those come from on Linux and from\n> > winsock2.h on Windows.\n> > - It can't assume that we have `NORETURN` (and it can't assume that we\n> > don't need it), or `MAYBE_UNUSED`, or ...\n>\n> These problems and the _GNU_SOURCE on you mention below must be pretty\n> common for cross-platform libraries - how do other projects handle them?\n> On the face of it this seems like it would be fairly simple to solve by\n> including a file that contains the subset of git-compat-util.h that\n> defines these macros (with a suitable LIBGIT_ prefix) in libgit.h.\n\nI think that either the library interface just #includes whatever they\nneed, and if that doesn't work, you held the library wrong and it's up\nto you to fix it (by doing whatever #defines are necessary either on\nthe compiler commandline or before including the library's header), or\nthey wrap everything in a layer of indirection, so only the library's\ncode itself needs to be compiled with any special options/#defines.\nI'm advocating for the second here.\n\n>\n> > Most of those issues _may_ be able to be resolved by having a\n> > \"gitlib-compat-util.h\" file included at the top of the \"external\n> > project\" .h file. But that's insufficient. Example:\n> >\n> > #include \n> > #include \"git/gitlib.h\" // Oops, the `#define _GNU_SOURCE` in the\n> > transitive \"gitlib-compat-util.h\" has no effect!\n> >\n> > Or the opposite:\n> >\n> > #include \"git/gitlib.h\" // Oops, this set _FILE_OFFSET_BITS=64 when\n> > the project wasn't expecting it!\n> > #include // For this translation unit only, `off_t` might\n> > be a different size than elsewhere in the project, I hope you like\n> > debugging segfaults.\n> >\n> > The only ways I could come up with to solve these problems were to\n> > hold the \"external interface\" to a different standard, that is\n> > simultaneously both more permissive (it can assume C99), and\n> > restrictive (it can't rely on things like off_t),\n> >\n> > incompatible with these external interfaces being used by the git\n> > project itself, which has a broader set of platforms it needs to\n> > support. But the external interfaces must be very simple wrappers\n> > around code that _is_ shared with the git executable.\n>\n> I agree we should minimize the amount of code in the wrappers for\n> external callers.\n>\n> Best Wishes\n>\n> Phillip\n"}]}