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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 16, 2022, 13:27 UTC
Message-ID: <220216.86leybszht.gmgdl@evledraar.gmail.com>
In-Reply-To: <7e6385f8-f25d-69f5-edae-6f5d6f785046@gmail.com>

On Wed, Feb 16 2022, Phillip Wood wrote:

Show 31 quoted lines

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

Show 28 quoted lines

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