Re: [RFD] Libification proposal: separate internal and external interfaces

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Apr 23, 2024, 09:57 UTC
Message-ID: <cb2ffdb1-5e6c-4dd3-89d4-f0b31f2f17ec@gmail.com>
In-Reply-To: <20240422162617.308366-1-calvinwan@google.com>

Hi Calvin

On 22/04/2024 17:26, Calvin Wan wrote:

Show 26 quoted lines

> The first idea involves turning `strbuf_grow()` into a wrapper function
> that invokes its equivalent library function, eg.
> `libgit_strbuf_grow()`:
> 
> int libgit_strbuf_grow(struct strbuf *sb, size_t extra)
> {
> 	int new_buf = !sb->alloc;
> 	if (unsigned_add_overflows(extra, 1) ||
> 	    unsigned_add_overflows(sb->len, extra + 1))
> 		return -1;
> 	if (new_buf)
> 		sb->buf = NULL;
> 	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
> 	if (new_buf)
> 		sb->buf[0] = '\0';
>          return 0;
> }
> 
> void strbuf_grow(struct strbuf *sb, size_t extra)
> {
>          if (libgit_strbuf_grow(sb, extra))
>                  die("you want to use way too much memory");
> }
> 
> (Note a context object could also be added as a parameter to
> `libgit_strbuf_grow()` for error messages and possibly global variables.)

I agree with Junio that this is a good route forward and that we should not consider removing the wrappers to be part of the libification project. ALLOC_GROW() can die so I think we'd need a way to propagate an error message up to the wrapper even for relatively simple functions like this. For the allocation functions we'd either need to use a static string for the message which is not a good fit for other functions that want to dynamically format their messages (for example to include a path name into the error message), or pre-allocate the error messages.

> The shortfall of this approach is that we'd be carrying two different
> functions for every library function until we are able to remove all of
> them. It would also create additional toil for Git contributors to
> figure out which version of the function should be used.

Hopefully it should be clear if a function is part of the library or not so I don't think this should be a big problem.

Best Wishes

Phillip