Calvin Wan <calvinwan@xxxxxxxxxx> writes:
Thanks for writing this down.
> 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.)
>
> In this scenario, we would be exposing `libgit_strbuf_grow()` to
> external consumers of the library, while not having to refactor internal
> uses of `strbuf_grow()`.
Yes, this is how I envision the things will go. And it is a good
place to STOP for "git" the command line program everybody knows for
the past 20 years. It is good that you mentioned the "context"
here, too. As to the naming, I am OK with "libgit_". Just wanted
to say that "libgit2" does not seem to use it as their prefix (and
they do not use "libgit2_" as their prefix, either) so if somebody
wants to bikeshed, they do not have to worry about them.
> This method would reduce initial churn within
> the codebase, however, we would want to eventually get rid of
> `strbuf_grow()` and use `libgit_strbuf_grow()` internally as well.
The "however" and everything that follows wants justification.
I think after we pass the "Traditional API git offered are thin
wrappers around git-std-lib" point, the returns (the "clean-up"
value) will quickly diminish. Even with diminished returns, there
may still be "clean-up" value left, but it would be simpler to
consider that as outside the scope of "libification" discussion.
Once the "Traditional API are thin wrappers" state is achieved, the
"libification" is done.
> The second idea removes the need for two different functions by removing
> the wrapper function and instead refactoring all callers of
> `strbuf_grow()` (and subsequently callers of other library functions).
> ...
> One shortcoming of this approach is the need to refactor all callers of
> library functions, but that can be handled better and the churn made
There is one huge downside you did not mention.
The appraoch makes "git the command line program" to commit to the
"errors must percolate all the way up to the top", and the
addditional effort to take us there after we have achieved the
libification goals has dubious trade-off.
I do not see why it bothers you, the libification person when he
wears git-std-lib hat, that "git the command line program" that is a
mere one of customers of git-std-lib happens to have a function
whose name is strbuf_grow() and uses libgit_strbuf_grow(). It
shouldn't bother you more than "git the command line program" having
a program called cmd_show() that uses many functions from libgit_
suite. After we reach the "our implementation of traditional API
are all wrappers around git-std-lib API functions" [*] state, we may
decide to refactor further and may replace those "wrappers" with
direct calls, but that can happen far in the future, when the
libified result is rock solid.
If you try to do everything from the top to bottom at once, you
cannot easily keep the system (the combination of git-std-lib still
work-in-progress plus the client code that is "git the command line
program") as solid as the first approach.
