On Fri, Jul 08 2022, Phillip Wood wrote:
Hi Ævar
On 08/07/2022 15:20, Ævar Arnfjörð Bjarmason wrote:
Remove the xdl_free() wrapper macro in favor of using free()
directly. The wrapper macro was brought in with the initial import of
xdiff/ in 3443546f6ef (Use a *real* built-in diff generator,
2006-03-24).
As subsequent discussions on the topic[1] have made clear there's no
reason to use this wrapper.
That link is to a message where you assert that there is no reason for
the wrapper, you seem to be the only person in that thread making that
argument. The libgit2 maintainer and others are arguing that it is
worth keeping to make it easy to swap the allocator.
Arguing that it's not needed for a technical reason, but an "aesthetic
and ergonomic one", per:
https://lore.kernel.org/git/20220217225804.GC7@edef91d97c94/;
Perhaps I misread it, but I took this as Junio concurring with what I
was pointing out there:
https://lore.kernel.org/git/xmqqfsohbdre.fsf@gitster.g/
Both git itself as well as any external
users such as libgit2 compile the xdiff/* code as part of their own
compilation, and can thus find the right malloc() and free() at
link-time.
I'm struggling to see how that is simpler than the current situation
with xdl_malloc().
It's simpler because when maintaining that code in git.git it's less of
a special case, e.g. we have coccinelle rules that match free(), now
every such rule needs to account for the xdiff special-case.
And it's less buggy because if you're relying on us always using a
wrapper bugs will creep in, current master has this:
$ git -P grep '\bfree\(' -- xdiff
xdiff/xdiff.h:#define xdl_free(ptr) free(ptr)
xdiff/xmerge.c: free(c);
xdiff/xmerge.c: free(next_m);
Perhaps you could show how you think libgit2 could
easily make xdiff use git__malloc() instead of malloc() without
changing the malloc() calls (the message you linked to essentially
suggests they do a search and replace). If people cannot swap in
another allocator without changing the code then you are imposing a
barrier to them contributing patches back to git's xdiff.
I was suggesting that if libgit2 wanted to maintain a copy of xdiff that
catered to the asthetic desires of the maintainer adjusting whatever
import script you use to apply a trivial coccinelle transformation would
give you what you wanted. Except without a maintenance burden on
git.git, or the bugs you'd get now since you're not catching those two
free() cases (or any future such cases).
But just having the code use malloc() and free() directly and getting
you what you get now is easy, and doesn't require any such
search-replacement.
Here's how:
I imported the xdiff/*.[ch] files at the tip of my branch into current
libgit2.git's src/libgit2/xdiff/, and then applied this on top, which is
partially re-applying libgit2's own monkeypatches, and partially
adjusting for upstream changes (including this one):
diff --git a/src/libgit2/xdiff/git-xdiff.h b/src/libgit2/xdiff/git-xdiff.h
index b75dba819..2e00764d4 100644
--- a/src/libgit2/xdiff/git-xdiff.h
+++ b/src/libgit2/xdiff/git-xdiff.h
@@ -14,6 +14,7 @@
#ifndef INCLUDE_git_xdiff_h__
#define INCLUDE_git_xdiff_h__
+#include <regex.h>
#include "regexp.h"
/* Work around C90-conformance issues */
@@ -27,11 +28,10 @@
# endif
#endif
-#define xdl_malloc(x) git__malloc(x)
-#define xdl_free(ptr) git__free(ptr)
-#define xdl_realloc(ptr, x) git__realloc(ptr, x)
+#define malloc(x) git__malloc(x)
+#define free(ptr) git__free(ptr)
-#define XDL_BUG(msg) GIT_ASSERT(msg)
+#define BUG(msg) GIT_ASSERT(msg)
#define xdl_regex_t git_regexp
#define xdl_regmatch_t git_regmatch
@@ -50,4 +50,17 @@ GIT_INLINE(int) xdl_regexec_buf(
return -1;
}
+static inline size_t st_mult(size_t a, size_t b)
+{
+ return a * b;
+}
+
+static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
+ size_t nmatch, regmatch_t pmatch[], int eflags)
+{
+ assert(nmatch > 0 && pmatch);
+ pmatch[0].rm_so = 0;
+ pmatch[0].rm_eo = size;
+ return regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);
+}
#endif
diff --git a/src/libgit2/xdiff/xdiff.h b/src/libgit2/xdiff/xdiff.h
index a37d89fcd..5e5b525c2 100644
--- a/src/libgit2/xdiff/xdiff.h
+++ b/src/libgit2/xdiff/xdiff.h
@@ -23,6 +23,8 @@
#if !defined(XDIFF_H)
#define XDIFF_H
+#include "git-xdiff.h"
+
#ifdef __cplusplus
extern "C" {
#endif /* #ifdef __cplusplus */
diff --git a/src/libgit2/xdiff/xinclude.h b/src/libgit2/xdiff/xinclude.h
index a4285ac0e..79812ad8a 100644
--- a/src/libgit2/xdiff/xinclude.h
+++ b/src/libgit2/xdiff/xinclude.h
@@ -23,7 +23,8 @@
#if !defined(XINCLUDE_H)
#define XINCLUDE_H
-#include "git-compat-util.h"
+#include "git-xdiff.h"
+#include "git-shared-util.h"
#include "xmacros.h"
#include "xdiff.h"
#include "xtypes.h"
If you then build it and run e.g.:
gdb -args ./libgit2_tests -smerge::files
You'll get stack traces like this if you break on stdalloc__malloc
(which it uses for me in its default configuration):
(gdb) bt
#0 stdalloc__malloc (len=144, file=0x87478d "/home/avar/g/libgit2/src/libgit2/xdiff/xutils.c", line=101) at /home/avar/g/libgit2/src/util/allocators/stdalloc.c:14
#1 0x00000000006ec15c in xdl_cha_alloc (cha=0x7fffffffcaa8) at /home/avar/g/libgit2/src/libgit2/xdiff/xutils.c:101
#2 0x00000000006eaee9 in xdl_prepare_ctx (pass=1, mf=0x7fffffffcc98, narec=13, xpp=0x7fffffffcca8, cf=0x7fffffffc7d0, xdf=0x7fffffffcaa8)
at /home/avar/g/libgit2/src/libgit2/xdiff/xprepare.c:194
IOW this was all seamlessly routed to your git__malloc() without us
having to maintain an xdl_{malloc,free}().
