Re: [PATCH 1/1] xdiff: provide indirection to git functions

[Phillip Wood <phillip.wood123@gmail.com>]

On 15/02/2022 23:40, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Feb 09 2022, Edward Thomson wrote:
> 
>> Provide an indirection layer into the git-specific functionality and
>> utilities in `git-xdiff.h`, prefixing those types and functions with
>> `xdl_` (and `XDL_` for macros).  This allows other projects that use
>> git's xdiff implementation to keep up-to-date; they can now take all the
>> files _except_ `git-xdiff.h`, which they have customized for their own
>> environment.
> 
> It seems sensible to share code here, but...
> 
>> +#ifndef GIT_XDIFF_H
>> +#define GIT_XDIFF_H
>> +
>> +#define xdl_malloc(x) xmalloc(x)
>> +#define xdl_free(ptr) free(ptr)
>> +#define xdl_realloc(ptr,x) xrealloc(ptr,x)
> 
> ...I don't understand the need for prefixing every function that may be
> used from git.git with xdl_*. In particular for these memory managing
> functions shouldn't this Just Work per 8d128513429 (grep/pcre2: actually
> make pcre2 use custom allocator, 2021-02-18) and cbe81e653fa
> (grep/pcre2: move back to thread-only PCREv2 structures, 2021-02-18)?
> I.e. link-time use of free().

I read that paragraph a couple of times and I'm still not sure I 
understand what you're saying. It is not unusual for libraries to define 
their own allocation functions and the code base is already using 
xdl_malloc etc so these defines seem quite reasonable. As you point out 
below we'd need wrappers for xmalloc() etc anyway so I'm not sure what 
the problem is.

> Of course trivial wrappers would be needed for x*() variants...
> 
>> +#define xdl_regex_t regex_t
> 
> This is a type that's in POSIX. Why do we need an xdl_* prefix for it?
> 
>> +#define xdl_regmatch_t regmatch_t
> 
> ditto.
> 
>> +#define xdl_regexec_buf(p, b, s, n, m, f) regexec_buf(p, b, s, n, m, f)
> 
> But this is our own custom function, which brings me to...
> 
>> +#define XDL_BUG(msg) BUG(msg)
> 
> ...unless libgit2 has a regexec_buf() or BUG() why do we need this
> indirection? Let's just have xdiff() use a bug, and then either libgit2
> will have a BUG() macro/function, or it'll fail at compile-time.
> 
> This seems to at least partly have been inspired by git.git's
> 546096a5cbb (xdiff: use BUG(...), not xdl_bug(...), 2021-06-07), i.e. we
> used to have an xdl_bug(), but now we just use BUG().
> 
> I then see on your libgit2 side 1458fb56e (xdiff: include new xdiff from
> git, 2022-01-29).
> 
> But why not simply?:
> 
>      #define BUG(msg) GIT_ASSERT(msg)
> 
> It would make things easier on the git.git side (etags and all).

If we want xdiff to be usable for other projects I think we're going to 
have to accept that it is sensible to namespace its functions.

Best Wishes

Phillip