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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Feb 16 2022, Phillip Wood wrote:

> 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.

That you generally don't need to define such wrappers for free() and
malloc(), because that's something you can handle at link-time.

This is current libgit2, which seems to have a version of this patch
integrated:
    
    $ git reference; git -P grep '\bfree\(' src/xdiff
    c8450561d (Merge pull request #6216 from libgit2/ethomson/readme, 2022-02-13)
    src/xdiff/xmerge.c:             free(c);
    src/xdiff/xmerge.c:     free(next_m);

I.e. I think instead of having xdl_free(), xdl_regcomp() etc. it makes
sense to just slowly go in the other direction and call free(),
regcomp() etc. Since it seems we're going to be maintaining an xdiff
fork permanently.

>> 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.

We're just talking about sharing code with libgit2, which I agree with
as a goal. I just don't see why we'd need to have e.g. XDL_BUG() as
opposed to libgit2 just providing a BUG() for its compatibility with our
xdiff.

We have other in-tree code with the same goal that does that, see
reftable/system.h.

It means that development in git.git can proceed without worrying about
the special-case, including stuff like this not doing what you think,
because you forgot the xdiff-specific alias:

    git grep -w BUG

And as long as libgit2 doesn't have a BUG() of its own (which it's
unlikely to do, since it's a generally usable library, and thus is
concerned about namespace conflicts) it can just provide the wrapper,
and providing that will be the same amount of work o that side, no?

This proposed wrapper is also BUGgy in that it's not __VA_ARGS__. It
just happens to work right now because none of xdiff/ uses >1 argument,
but that sort of thing is another reason to use BUG() and push the
compatibility headaches to whoever is doing the one-off import into
other codebases.