On Wed, Feb 16 2022, Phillip Wood wrote:

On 15/02/2022 23:40, Ævar Arnfjörð Bjarmason wrote:

quoted

On Wed, Feb 09 2022, Edward Thomson wrote:

quoted

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

quoted

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

quoted

Of course trivial wrappers would be needed for x*() variants...

quoted

+#define xdl_regex_t regex_t

This is a type that's in POSIX. Why do we need an xdl_* prefix for
it?

quoted

+#define xdl_regmatch_t regmatch_t

ditto.

quoted

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

quoted

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