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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 15, 2022, 23:40 UTC
Message-ID: <220216.86wnhvvgeh.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220209013354.GB7@abe733c6e288>

On Wed, Feb 09 2022, Edward Thomson wrote:

Show 6 quoted lines

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

Show 6 quoted lines

> +#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().

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