{"thread":{"id":"57389","subject":"[PATCH 1/1] xdiff: provide indirection to git functions","startedAt":"2022-02-09T02:41:11Z","lastAt":"2022-04-15T16:07:13Z","messageCount":11,"participants":["Edward Thomson","Phillip Wood","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"448023","messageId":"20220209013354.GB7@abe733c6e288","threadId":"57389","inReplyTo":"20220209012951.GA7@abe733c6e288","subject":"[PATCH 1/1] xdiff: provide indirection to git functions","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2022-02-09T01:33:54Z","receivedAt":"2022-02-09T02:41:11Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"Provide an indirection layer into the git-specific functionality and\nutilities in `git-xdiff.h`, prefixing those types and functions with\n`xdl_` (and `XDL_` for macros).  This allows other projects that use\ngit's xdiff implementation to keep up-to-date; they can now take all the\nfiles _except_ `git-xdiff.h`, which they have customized for their own\nenvironment.\n\nSigned-off-by: Edward Thomson <ethomson@edwardthomson.com>\n---\n xdiff/git-xdiff.h | 14 ++++++++++++++\n xdiff/xdiff.h     |  8 +++-----\n xdiff/xdiffi.c    | 20 ++++++++++----------\n xdiff/xinclude.h  |  2 +-\n 4 files changed, 28 insertions(+), 16 deletions(-)\n create mode 100644 xdiff/git-xdiff.h\n\ndiff --git a/xdiff/git-xdiff.h b/xdiff/git-xdiff.h\nnew file mode 100644\nindex 0000000000..5d47576551\n--- /dev/null\n+++ b/xdiff/git-xdiff.h\n@@ -0,0 +1,14 @@\n+#ifndef GIT_XDIFF_H\n+#define GIT_XDIFF_H\n+\n+#define xdl_malloc(x) xmalloc(x)\n+#define xdl_free(ptr) free(ptr)\n+#define xdl_realloc(ptr,x) xrealloc(ptr,x)\n+\n+#define xdl_regex_t regex_t\n+#define xdl_regmatch_t regmatch_t\n+#define xdl_regexec_buf(p, b, s, n, m, f) regexec_buf(p, b, s, n, m, f)\n+\n+#define XDL_BUG(msg) BUG(msg)\n+\n+#endif\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 72e25a9ffa..fb47f63fbf 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -27,6 +27,8 @@\n extern \"C\" {\n #endif /* #ifdef __cplusplus */\n \n+#include \"git-xdiff.h\"\n+\n /* xpparm_t.flags */\n #define XDF_NEED_MINIMAL (1 << 0)\n \n@@ -82,7 +84,7 @@ typedef struct s_xpparam {\n \tunsigned long flags;\n \n \t/* -I<regex> */\n-\tregex_t **ignore_regex;\n+\txdl_regex_t **ignore_regex;\n \tsize_t ignore_regex_nr;\n \n \t/* See Documentation/diff-options.txt. */\n@@ -119,10 +121,6 @@ typedef struct s_bdiffparam {\n } bdiffparam_t;\n \n \n-#define xdl_malloc(x) xmalloc(x)\n-#define xdl_free(ptr) free(ptr)\n-#define xdl_realloc(ptr,x) xrealloc(ptr,x)\n-\n void *xdl_mmfile_first(mmfile_t *mmf, long *size);\n long xdl_mmfile_size(mmfile_t *mmf);\n \ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 69689fab24..af31b7f4b3 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -832,7 +832,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t/* Shift the group backward as much as possible: */\n \t\t\twhile (!group_slide_up(xdf, &g))\n \t\t\t\tif (group_previous(xdfo, &go))\n-\t\t\t\t\tBUG(\"group sync broken sliding up\");\n+\t\t\t\t\tXDL_BUG(\"group sync broken sliding up\");\n \n \t\t\t/*\n \t\t\t * This is this highest that this group can be shifted.\n@@ -848,7 +848,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t\tif (group_slide_down(xdf, &g))\n \t\t\t\t\tbreak;\n \t\t\t\tif (group_next(xdfo, &go))\n-\t\t\t\t\tBUG(\"group sync broken sliding down\");\n+\t\t\t\t\tXDL_BUG(\"group sync broken sliding down\");\n \n \t\t\t\tif (go.end > go.start)\n \t\t\t\t\tend_matching_other = g.end;\n@@ -873,9 +873,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t */\n \t\t\twhile (go.end == go.start) {\n \t\t\t\tif (group_slide_up(xdf, &g))\n-\t\t\t\t\tBUG(\"match disappeared\");\n+\t\t\t\t\tXDL_BUG(\"match disappeared\");\n \t\t\t\tif (group_previous(xdfo, &go))\n-\t\t\t\t\tBUG(\"group sync broken sliding to match\");\n+\t\t\t\t\tXDL_BUG(\"group sync broken sliding to match\");\n \t\t\t}\n \t\t} else if (flags & XDF_INDENT_HEURISTIC) {\n \t\t\t/*\n@@ -916,9 +916,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \n \t\t\twhile (g.end > best_shift) {\n \t\t\t\tif (group_slide_up(xdf, &g))\n-\t\t\t\t\tBUG(\"best shift unreached\");\n+\t\t\t\t\tXDL_BUG(\"best shift unreached\");\n \t\t\t\tif (group_previous(xdfo, &go))\n-\t\t\t\t\tBUG(\"group sync broken sliding to blank line\");\n+\t\t\t\t\tXDL_BUG(\"group sync broken sliding to blank line\");\n \t\t\t}\n \t\t}\n \n@@ -927,11 +927,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\tif (group_next(xdf, &g))\n \t\t\tbreak;\n \t\tif (group_next(xdfo, &go))\n-\t\t\tBUG(\"group sync broken moving to next group\");\n+\t\t\tXDL_BUG(\"group sync broken moving to next group\");\n \t}\n \n \tif (!group_next(xdfo, &go))\n-\t\tBUG(\"group sync broken at end of file\");\n+\t\tXDL_BUG(\"group sync broken at end of file\");\n \n \treturn 0;\n }\n@@ -1011,11 +1011,11 @@ static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t *xe, long flags)\n }\n \n static int record_matches_regex(xrecord_t *rec, xpparam_t const *xpp) {\n-\tregmatch_t regmatch;\n+\txdl_regmatch_t regmatch;\n \tint i;\n \n \tfor (i = 0; i < xpp->ignore_regex_nr; i++)\n-\t\tif (!regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1,\n+\t\tif (!xdl_regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1,\n \t\t\t\t &regmatch, 0))\n \t\t\treturn 1;\n \ndiff --git a/xdiff/xinclude.h b/xdiff/xinclude.h\nindex a4285ac0eb..bf66dc0a87 100644\n--- a/xdiff/xinclude.h\n+++ b/xdiff/xinclude.h\n@@ -24,6 +24,7 @@\n #define XINCLUDE_H\n \n #include \"git-compat-util.h\"\n+#include \"git-xdiff.h\"\n #include \"xmacros.h\"\n #include \"xdiff.h\"\n #include \"xtypes.h\"\n@@ -32,5 +33,4 @@\n #include \"xdiffi.h\"\n #include \"xemit.h\"\n \n-\n #endif /* #if !defined(XINCLUDE_H) */\n-- \n2.35.0\n\n"},{"id":"448028","messageId":"20220209012951.GA7@abe733c6e288","threadId":"57389","inReplyTo":null,"subject":"[PATCH 0/1] xdiff: share xdiff between git and libgit2","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2022-02-09T01:29:51Z","receivedAt":"2022-02-09T02:41:37Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"Hello from libgit2, where we borrowed your xdiff a few years ago and\nhave watched as we both hacked on it independently.  (For us, mostly it\nwas around tightening some things up around warnings and signed/unsigned\nmismatches.)  However, we'd love to share a common xdiff implementation,\nand we're happy if git is the home for that.\n\nThe next patch adds an indirection point, `git-xdiff.h`, that contains\nthe git-specific functionality in xdiff.  This keeps the core of xdiff\nto standard functions.  Other xdiff users, like libgit2, can specify\ntheir own compatibility functions in this header file.\n\nI hope that this allows us to make progress on a common xdiff; we'd love\nto go back to building it without warnings, but we'd like to not do that\nin isolation.\n\nCheers-\n-ed\n\nEdward Thomson (1):\n  xdiff: provide indirection to git functions\n\n xdiff/git-xdiff.h | 14 ++++++++++++++\n xdiff/xdiff.h     |  8 +++-----\n xdiff/xdiffi.c    | 20 ++++++++++----------\n xdiff/xinclude.h  |  2 +-\n 4 files changed, 28 insertions(+), 16 deletions(-)\n create mode 100644 xdiff/git-xdiff.h\n\n--\n2.35.0\n\n"},{"id":"448044","messageId":"94c2b081-2767-8d4a-f77e-db74d9aeda56@gmail.com","threadId":"57389","inReplyTo":"20220209013354.GB7@abe733c6e288","subject":"Re: [PATCH 1/1] xdiff: provide indirection to git functions","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-09T11:07:10Z","receivedAt":"2022-02-09T12:07:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Edward\n\nOn 09/02/2022 01:33, Edward Thomson wrote:\n> Provide an indirection layer into the git-specific functionality and\n> utilities in `git-xdiff.h`, prefixing those types and functions with\n> `xdl_` (and `XDL_` for macros).  This allows other projects that use\n> git's xdiff implementation to keep up-to-date; they can now take all the\n> files _except_ `git-xdiff.h`, which they have customized for their own\n> environment.\n\nThis seems like a sensible way to make it easier to share a common \nxdiff. The patch looks good to me apart from\n\n> diff --git a/xdiff/xinclude.h b/xdiff/xinclude.h\n> index a4285ac0eb..bf66dc0a87 100644\n> --- a/xdiff/xinclude.h\n> +++ b/xdiff/xinclude.h\n> @@ -24,6 +24,7 @@\n>   #define XINCLUDE_H\n>   \n>   #include \"git-compat-util.h\"\n\nI think you want to remove this\n\nBest Wishes\n\nPhillip\n\n"},{"id":"448500","messageId":"220216.86wnhvvgeh.gmgdl@evledraar.gmail.com","threadId":"57389","inReplyTo":"20220209013354.GB7@abe733c6e288","subject":"Re: [PATCH 1/1] xdiff: provide indirection to git functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-15T23:40:02Z","receivedAt":"2022-02-15T23:50:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 09 2022, Edward Thomson wrote:\n\n> Provide an indirection layer into the git-specific functionality and\n> utilities in `git-xdiff.h`, prefixing those types and functions with\n> `xdl_` (and `XDL_` for macros).  This allows other projects that use\n> git's xdiff implementation to keep up-to-date; they can now take all the\n> files _except_ `git-xdiff.h`, which they have customized for their own\n> environment.\n\nIt seems sensible to share code here, but...\n\n> +#ifndef GIT_XDIFF_H\n> +#define GIT_XDIFF_H\n> +\n> +#define xdl_malloc(x) xmalloc(x)\n> +#define xdl_free(ptr) free(ptr)\n> +#define xdl_realloc(ptr,x) xrealloc(ptr,x)\n\n...I don't understand the need for prefixing every function that may be\nused from git.git with xdl_*. In particular for these memory managing\nfunctions shouldn't this Just Work per 8d128513429 (grep/pcre2: actually\nmake pcre2 use custom allocator, 2021-02-18) and cbe81e653fa\n(grep/pcre2: move back to thread-only PCREv2 structures, 2021-02-18)?\nI.e. link-time use of free().\n\nOf course trivial wrappers would be needed for x*() variants...\n\n> +#define xdl_regex_t regex_t\n\nThis is a type that's in POSIX. Why do we need an xdl_* prefix for it?\n\n> +#define xdl_regmatch_t regmatch_t\n\nditto.\n\n> +#define xdl_regexec_buf(p, b, s, n, m, f) regexec_buf(p, b, s, n, m, f)\n\nBut this is our own custom function, which brings me to...\n\n> +#define XDL_BUG(msg) BUG(msg)\n\n...unless libgit2 has a regexec_buf() or BUG() why do we need this\nindirection? Let's just have xdiff() use a bug, and then either libgit2\nwill have a BUG() macro/function, or it'll fail at compile-time.\n\nThis seems to at least partly have been inspired by git.git's\n546096a5cbb (xdiff: use BUG(...), not xdl_bug(...), 2021-06-07), i.e. we\nused to have an xdl_bug(), but now we just use BUG().\n\nI then see on your libgit2 side 1458fb56e (xdiff: include new xdiff from\ngit, 2022-01-29).\n\nBut why not simply?:\n\n    #define BUG(msg) GIT_ASSERT(msg)\n\nIt would make things easier on the git.git side (etags and all).\n"},{"id":"448586","messageId":"7e6385f8-f25d-69f5-edae-6f5d6f785046@gmail.com","threadId":"57389","inReplyTo":"220216.86wnhvvgeh.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/1] xdiff: provide indirection to git functions","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-16T11:02:33Z","receivedAt":"2022-02-16T11:02:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 15/02/2022 23:40, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Feb 09 2022, Edward Thomson wrote:\n> \n>> Provide an indirection layer into the git-specific functionality and\n>> utilities in `git-xdiff.h`, prefixing those types and functions with\n>> `xdl_` (and `XDL_` for macros).  This allows other projects that use\n>> git's xdiff implementation to keep up-to-date; they can now take all the\n>> files _except_ `git-xdiff.h`, which they have customized for their own\n>> environment.\n> \n> It seems sensible to share code here, but...\n> \n>> +#ifndef GIT_XDIFF_H\n>> +#define GIT_XDIFF_H\n>> +\n>> +#define xdl_malloc(x) xmalloc(x)\n>> +#define xdl_free(ptr) free(ptr)\n>> +#define xdl_realloc(ptr,x) xrealloc(ptr,x)\n> \n> ...I don't understand the need for prefixing every function that may be\n> used from git.git with xdl_*. In particular for these memory managing\n> functions shouldn't this Just Work per 8d128513429 (grep/pcre2: actually\n> make pcre2 use custom allocator, 2021-02-18) and cbe81e653fa\n> (grep/pcre2: move back to thread-only PCREv2 structures, 2021-02-18)?\n> I.e. link-time use of free().\n\nI read that paragraph a couple of times and I'm still not sure I \nunderstand what you're saying. It is not unusual for libraries to define \ntheir own allocation functions and the code base is already using \nxdl_malloc etc so these defines seem quite reasonable. As you point out \nbelow we'd need wrappers for xmalloc() etc anyway so I'm not sure what \nthe problem is.\n\n> Of course trivial wrappers would be needed for x*() variants...\n> \n>> +#define xdl_regex_t regex_t\n> \n> This is a type that's in POSIX. Why do we need an xdl_* prefix for it?\n> \n>> +#define xdl_regmatch_t regmatch_t\n> \n> ditto.\n> \n>> +#define xdl_regexec_buf(p, b, s, n, m, f) regexec_buf(p, b, s, n, m, f)\n> \n> But this is our own custom function, which brings me to...\n> \n>> +#define XDL_BUG(msg) BUG(msg)\n> \n> ...unless libgit2 has a regexec_buf() or BUG() why do we need this\n> indirection? Let's just have xdiff() use a bug, and then either libgit2\n> will have a BUG() macro/function, or it'll fail at compile-time.\n> \n> This seems to at least partly have been inspired by git.git's\n> 546096a5cbb (xdiff: use BUG(...), not xdl_bug(...), 2021-06-07), i.e. we\n> used to have an xdl_bug(), but now we just use BUG().\n> \n> I then see on your libgit2 side 1458fb56e (xdiff: include new xdiff from\n> git, 2022-01-29).\n> \n> But why not simply?:\n> \n>      #define BUG(msg) GIT_ASSERT(msg)\n> \n> It would make things easier on the git.git side (etags and all).\n\nIf we want xdiff to be usable for other projects I think we're going to \nhave to accept that it is sensible to namespace its functions.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"448590","messageId":"220216.86leybszht.gmgdl@evledraar.gmail.com","threadId":"57389","inReplyTo":"7e6385f8-f25d-69f5-edae-6f5d6f785046@gmail.com","subject":"Re: [PATCH 1/1] xdiff: provide indirection to git functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-16T13:27:27Z","receivedAt":"2022-02-16T13:38:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 16 2022, Phillip Wood wrote:\n\n> On 15/02/2022 23:40, Ævar Arnfjörð Bjarmason wrote:\n>> On Wed, Feb 09 2022, Edward Thomson wrote:\n>> \n>>> Provide an indirection layer into the git-specific functionality and\n>>> utilities in `git-xdiff.h`, prefixing those types and functions with\n>>> `xdl_` (and `XDL_` for macros).  This allows other projects that use\n>>> git's xdiff implementation to keep up-to-date; they can now take all the\n>>> files _except_ `git-xdiff.h`, which they have customized for their own\n>>> environment.\n>> It seems sensible to share code here, but...\n>> \n>>> +#ifndef GIT_XDIFF_H\n>>> +#define GIT_XDIFF_H\n>>> +\n>>> +#define xdl_malloc(x) xmalloc(x)\n>>> +#define xdl_free(ptr) free(ptr)\n>>> +#define xdl_realloc(ptr,x) xrealloc(ptr,x)\n>> ...I don't understand the need for prefixing every function that may\n>> be\n>> used from git.git with xdl_*. In particular for these memory managing\n>> functions shouldn't this Just Work per 8d128513429 (grep/pcre2: actually\n>> make pcre2 use custom allocator, 2021-02-18) and cbe81e653fa\n>> (grep/pcre2: move back to thread-only PCREv2 structures, 2021-02-18)?\n>> I.e. link-time use of free().\n>\n> I read that paragraph a couple of times and I'm still not sure I\n> understand what you're saying. It is not unusual for libraries to\n> define their own allocation functions and the code base is already\n> using xdl_malloc etc so these defines seem quite reasonable. As you\n> point out below we'd need wrappers for xmalloc() etc anyway so I'm not\n> sure what the problem is.\n\nThat you generally don't need to define such wrappers for free() and\nmalloc(), because that's something you can handle at link-time.\n\nThis is current libgit2, which seems to have a version of this patch\nintegrated:\n    \n    $ git reference; git -P grep '\\bfree\\(' src/xdiff\n    c8450561d (Merge pull request #6216 from libgit2/ethomson/readme, 2022-02-13)\n    src/xdiff/xmerge.c:             free(c);\n    src/xdiff/xmerge.c:     free(next_m);\n\nI.e. I think instead of having xdl_free(), xdl_regcomp() etc. it makes\nsense to just slowly go in the other direction and call free(),\nregcomp() etc. Since it seems we're going to be maintaining an xdiff\nfork permanently.\n\n>> Of course trivial wrappers would be needed for x*() variants...\n>> \n>>> +#define xdl_regex_t regex_t\n>> This is a type that's in POSIX. Why do we need an xdl_* prefix for\n>> it?\n>> \n>>> +#define xdl_regmatch_t regmatch_t\n>> ditto.\n>> \n>>> +#define xdl_regexec_buf(p, b, s, n, m, f) regexec_buf(p, b, s, n, m, f)\n>> But this is our own custom function, which brings me to...\n>> \n>>> +#define XDL_BUG(msg) BUG(msg)\n>> ...unless libgit2 has a regexec_buf() or BUG() why do we need this\n>> indirection? Let's just have xdiff() use a bug, and then either libgit2\n>> will have a BUG() macro/function, or it'll fail at compile-time.\n>> This seems to at least partly have been inspired by git.git's\n>> 546096a5cbb (xdiff: use BUG(...), not xdl_bug(...), 2021-06-07), i.e. we\n>> used to have an xdl_bug(), but now we just use BUG().\n>> I then see on your libgit2 side 1458fb56e (xdiff: include new xdiff\n>> from\n>> git, 2022-01-29).\n>> But why not simply?:\n>>      #define BUG(msg) GIT_ASSERT(msg)\n>> It would make things easier on the git.git side (etags and all).\n>\n> If we want xdiff to be usable for other projects I think we're going\n> to have to accept that it is sensible to namespace its functions.\n\nWe're just talking about sharing code with libgit2, which I agree with\nas a goal. I just don't see why we'd need to have e.g. XDL_BUG() as\nopposed to libgit2 just providing a BUG() for its compatibility with our\nxdiff.\n\nWe have other in-tree code with the same goal that does that, see\nreftable/system.h.\n\nIt means that development in git.git can proceed without worrying about\nthe special-case, including stuff like this not doing what you think,\nbecause you forgot the xdiff-specific alias:\n\n    git grep -w BUG\n\nAnd as long as libgit2 doesn't have a BUG() of its own (which it's\nunlikely to do, since it's a generally usable library, and thus is\nconcerned about namespace conflicts) it can just provide the wrapper,\nand providing that will be the same amount of work o that side, no?\n\nThis proposed wrapper is also BUGgy in that it's not __VA_ARGS__. It\njust happens to work right now because none of xdiff/ uses >1 argument,\nbut that sort of thing is another reason to use BUG() and push the\ncompatibility headaches to whoever is doing the one-off import into\nother codebases.\n"},{"id":"448675","messageId":"220217.86ee41izpq.gmgdl@evledraar.gmail.com","threadId":"57389","inReplyTo":"20220217012847.GA8@e5e602f6ad40","subject":"Re: [PATCH 1/1] xdiff: provide indirection to git functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-17T09:29:23Z","receivedAt":"2022-02-17T09:56:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 17 2022, Edward Thomson wrote:\n\n[I'm assuming that dropping the list from CC was a mistake, re-CC-ing]\n\n> On Wed, Feb 16, 2022 at 02:27:27PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> This is current libgit2, which seems to have a version of this patch\n>> integrated:\n>>     \n>>     $ git reference; git -P grep '\\bfree\\(' src/xdiff\n>>     c8450561d (Merge pull request #6216 from libgit2/ethomson/readme, 2022-02-13)\n>>     src/xdiff/xmerge.c:             free(c);\n>>     src/xdiff/xmerge.c:     free(next_m);\n>\n> Yikes!  A buggy version, in fact.  More on that in a moment.\n>\n>> I.e. I think instead of having xdl_free(), xdl_regcomp() etc. it makes\n>> sense to just slowly go in the other direction and call free(),\n>> regcomp() etc. Since it seems we're going to be maintaining an xdiff\n>> fork permanently.\n>\n> Right, and that's explicitly what doesn't work for libgit2, and the goal\n> of an abstraction layer that we can both use.  git uses `xmalloc` (for\n> example), while libgit2 uses `git__alloc` to allocate memory.  This\n> seems sensible enough to replace, but there's not a 1:1 mapping in our\n> APIs because git lacks an `xfree`.\n>\n> If it were just a matter of `#define xmalloc git__alloc` then that might\n> be a reasonable strategy for libgit2 to take for code re-use.  But\n> `git__alloc` isn't necessarily just a wrapper around `malloc`, it's a\n> pluggable allocator that a library user can supply.  So we can't call\n> our allocator (`git__alloc`) and then the system's `free` because that\n> will most certainly fail.  We supply a `git__free` for this reason.\n>\n> (I appreciate you pointing out that I missed this in our update to\n> libgit2!  The ruby bindings make use of their own allocator and we can\n> ship a patch before they update.)\n\nYes. I understand. I'm saying that for the purposes of a \"free()\" in the\ntext of the xdiff code you can have your cake and eat it too. You just\nneed to adjust the compilation of xdiff within libgit2 so that it\ne.g. defines free() to git__free() or whatever before that code is\nincluded, or to do the same at link-time.\n\nThe reason I pointed at the PCRE commits in git.git is that's exactly\nwhat we ended up doing by accident with nedmalloc + PCRE. I.e. because\nwe use free() and malloc() we ended up with nedmalloc due to that shim,\nbut would then link to libpcre2 which would use the system malloc (not\ncompiled with those shims).\n\nSince in this case we're talking about someone importing libgit2 into\ntheir tree all of malloc() and free() can end up in the right place for\nyou.\n\n>> \n>> >> Of course trivial wrappers would be needed for x*() variants...\n>> >> \n>> >>> +#define xdl_regex_t regex_t\n>> >> This is a type that's in POSIX. Why do we need an xdl_* prefix for\n>> >> it?\n>\n> Precisely because it's a type in POSIX.  libgit2 doesn't necessarily\n> build on POSIX systems, and a user could - again - supply their own\n> regex engine like PCRE even if they do have the POSIX regex engine\n> available on their installations.\n\nSure, but these renames aren't needed for that. In fact PCRE ships with\na POSIX shimmy layer which makes my point for me. See pcre2posix(3),\ni.e. it'll redefine regcomp(), regexec(), regex_t etc.\n\nSo you're saying you need to renaming so you can get X, but\npcre2posix(3) is a working demonstration of X without that step :)\n\nIn this case though we do need the regexec_buf() semantics, but the\nright thing to do for xdiff/* compatibility is to just split off the\ntrivial regexec_buf() shim in git-compat-util.h say a\ncompat/regexec_buf.c for your convenience, then you could import that\nalong with xdiff/*.\n\nI haven't checked if pcre2posix supports that non-portable\n*BSD-originated trickery in regexec_buf() to make the pmatch variables\ncarry the length (if not it would be trivial to make it do so), but\neverything else above would be easy\n\n> libgit2 could - I suppose - do some magic to ensure that we call it a\n> `regex_t` even when it's a `pcre *`.  But any new person to our codebase\n> would (rightly) expect a `regex_t` to be ... well, a `regex_t` and might\n> (again, rightly) expect to find a `re_nsub` on it.  Hell, even I would\n> expect this because I don't interact with the regex code on the regular.\n\nI think if you're expecting shimmying layers for POSIX regexen to be in\nplay that people would expect it to work like pcre2posix(3). I.e. you\nuse the POSIX API but drop in a shim for a non-libc implementation.\n\nAs for the \"new person to our codebase...\" I don't think you're wrong\nthere, but that's an asthetic preference, not something that's required\nfor the stated aims of this series of dropping in compatibility shims.\n\nThe reason I started commenting here was because I was surprised that\nthis was needed at all for libgit2, since we do exactly that sort of\nshimming without the renaming here.\n\n>> We're just talking about sharing code with libgit2, which I agree with\n>> as a goal. I just don't see why we'd need to have e.g. XDL_BUG() as\n>> opposed to libgit2 just providing a BUG() for its compatibility with our\n>> xdiff.\n>\n> I care very little about `BUG`, but I care very much about allocation\n> and regex abstraction.  But now it makes more sense to me to have a\n> common prefix for the abstraction rather than piecemeal.\n>\n> I'll supply a re-roll with the issues that you and Philip pointed out\n> and I'm certain that we will continue the discussion.\n\nFor the asthetic preference?\n\nIf it's XDL_BUG() the primary project (git.git) needs to carry the\nXDL_BUG() -> BUG() shim along with libgit2's XDL_BUG() ->\nGIT_ASSERT(msg) .\n\nIf it's just BUG() we don't need the shim in git.git, but you'll need a\nBUG() -> GIT_ASSERT(msg).\n\nI don't see the benefit of requiring two shims instead of one, both in\nterms of code, and the readability of the codebase in git.git\n(i.e. grepping for \"git grep -w BUG\" or whatever, then remembering it's\nprefixing everything...).\n\n"},{"id":"448708","messageId":"nycvar.QRO.7.76.6.2202171644090.348@tvgsbejvaqbjf.bet","threadId":"57389","inReplyTo":"20220209012951.GA7@abe733c6e288","subject":"Re: [PATCH 0/1] xdiff: share xdiff between git and libgit2","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-02-17T15:44:55Z","receivedAt":"2022-02-17T15:45:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ed,\n\nOn Wed, 9 Feb 2022, Edward Thomson wrote:\n\n> Hello from libgit2, where we borrowed your xdiff a few years ago and\n> have watched as we both hacked on it independently.  (For us, mostly it\n> was around tightening some things up around warnings and signed/unsigned\n> mismatches.)  However, we'd love to share a common xdiff implementation,\n> and we're happy if git is the home for that.\n\nGreat!\n\n> The next patch adds an indirection point, `git-xdiff.h`, that contains\n> the git-specific functionality in xdiff.  This keeps the core of xdiff\n> to standard functions.  Other xdiff users, like libgit2, can specify\n> their own compatibility functions in this header file.\n\nI like this direction and looked over the patch: ACK!\n\n> I hope that this allows us to make progress on a common xdiff; we'd love\n> to go back to building it without warnings, but we'd like to not do that\n> in isolation.\n\nYes, let's combine efforts.\n\nThank you for kicking this off,\nDscho\n"},{"id":"448721","messageId":"xmqqfsohbdre.fsf@gitster.g","threadId":"57389","inReplyTo":"220217.86ee41izpq.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/1] xdiff: provide indirection to git functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-17T17:32:05Z","receivedAt":"2022-02-17T17:32:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> ...\n> If it's XDL_BUG() the primary project (git.git) needs to carry the\n> XDL_BUG() -> BUG() shim along with libgit2's XDL_BUG() ->\n> GIT_ASSERT(msg) .\n>\n> If it's just BUG() we don't need the shim in git.git, but you'll need a\n> BUG() -> GIT_ASSERT(msg).\n>\n> I don't see the benefit of requiring two shims instead of one, both in\n> terms of code, and the readability of the codebase in git.git\n> (i.e. grepping for \"git grep -w BUG\" or whatever, then remembering it's\n> prefixing everything...).\n\nRenaming symbols with preprocessor macro \"#define\"s, without forcing\npeople to change the names they have used in the code and have to\nwrite in the future, sounds like a sensible direction to go in.\n\nThanks.  \n"},{"id":"448747","messageId":"20220217225804.GC7@edef91d97c94","threadId":"57389","inReplyTo":"220217.86ee41izpq.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/1] xdiff: provide indirection to git functions","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2022-02-17T22:58:04Z","receivedAt":"2022-02-17T22:58:10Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"On Thu, Feb 17, 2022 at 10:29:23AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> [I'm assuming that dropping the list from CC was a mistake, re-CC-ing]\n\nIt was; many apologies, I don't use mutt very often any more.  Thanks!\n\n> As for the \"new person to our codebase...\" I don't think you're wrong\n> there, but that's an asthetic preference, not something that's required\n> for the stated aims of this series of dropping in compatibility shims.\n\nSure, but avoiding a prefix is also not a technical decision but an\naesthetic and ergonomic one.\n\nIs using a prefix here great?  No, it's not great, it's shit.  But it's\nshit that's easy to reason about.\n\nIf somebody sees a call to `xdl_free` in some code, they say \"wtf is\nthis `xdl_free` nonsense?\"  And they grep around and figure it out and\nunderstand the way that this project handles heap allocations.  It's\nvery transparent.\n\nIf somebody sees a call to `free` in their code, they say \"great,\n`free`\".  But it merely *appears* very transparent; in fact, there's\nsome magic behind the scenes that turns a `free` into a `git__free`\nwithout you knowing it.  You've not learned the way that this project\nhandles heap allocations, but you also don't know that there's anything\nthat you needed to learn.  These are the sorts of things that you think\nyou understand but only discover when you _need_ to discover it because\nsomething's gone very wrong.\n\nIn my experience, calling a function what it _isn't_ is the sort of thing\nthat a developer discovers the hard way, and that often leads to them\nnot trusting the codebase because it doesn't do what it says it does.\n\nCheers-\n-ed\n"},{"id":"453711","messageId":"220415.867d7qbaad.gmgdl@evledraar.gmail.com","threadId":"57389","inReplyTo":"20220217225804.GC7@edef91d97c94","subject":"Re: [PATCH 1/1] xdiff: provide indirection to git functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-15T15:55:07Z","receivedAt":"2022-04-15T16:07:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 17 2022, Edward Thomson wrote:\n\n> On Thu, Feb 17, 2022 at 10:29:23AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> [I'm assuming that dropping the list from CC was a mistake, re-CC-ing]\n>\n> It was; many apologies, I don't use mutt very often any more.  Thanks!\n\nNo worries. Also, late reply but I remembered & referenced this thread\nin\nhttps://lore.kernel.org/git/220415.86bkx2bb0p.gmgdl@evledraar.gmail.com/,\nand saw that I'd left this hanging...\n\n>> As for the \"new person to our codebase...\" I don't think you're wrong\n>> there, but that's an asthetic preference, not something that's required\n>> for the stated aims of this series of dropping in compatibility shims.\n>\n> Sure, but avoiding a prefix is also not a technical decision but an\n> aesthetic and ergonomic one.\n\nYes, I see that, but this code is maintained in git.git, not\nlibgit2.git, and having to remember to use custom malloc()/free()\nper-namespace is very much negative asthetics & ergonomics in that\ncontext.\n\nSo if the linker solution works...\n\n> Is using a prefix here great?  No, it's not great, it's shit.  But it's\n> shit that's easy to reason about.\n\nI really don't see that, as noted in the linked newer reply above we\nhave bugs due to this sort of pattern where someone uses\nmycustom_malloc(), forgets that, and then calls free() instead of\nmycustom_free().\n\nWhich is a bug and potential segfault that's entirely preventable by not\nusing such wrappers at the per-file level (some one-off \"this is where\nwe provide a custom malloc\" file might of course have such complexity).\n\n> If somebody sees a call to `xdl_free` in some code, they say \"wtf is\n> this `xdl_free` nonsense?\"  And they grep around and figure it out and\n> understand the way that this project handles heap allocations.  It's\n> very transparent.\n>\n> If somebody sees a call to `free` in their code, they say \"great,\n> `free`\".  But it merely *appears* very transparent; in fact, there's\n> some magic behind the scenes that turns a `free` into a `git__free`\n> without you knowing it.  You've not learned the way that this project\n> handles heap allocations, but you also don't know that there's anything\n> that you needed to learn.  These are the sorts of things that you think\n> you understand but only discover when you _need_ to discover it because\n> something's gone very wrong.\n\nBecause the reader assumed that when they saw malloc/free that it was\nThe Canonical Libc version, as opposed to whatever custom malloc the\nlibrary linked to?\n\n> In my experience, calling a function what it _isn't_ is the sort of thing\n> that a developer discovers the hard way, and that often leads to them\n> not trusting the codebase because it doesn't do what it says it does.\n\nBut they aren't anything until you link to something that provides them.\n\nAnyway, I think I see your point, you'd like names to always reflect\ntheir different-ness, no linker shenanigans.\n\nAnyway, since per [1] it seemed Junio was also more partial to sticking\nwith malloc/free *and* we're talking about a thing that gets\none-off-imported into libgit2 (not as a submodule, presumably) I don't\nthink there's any reason to really argue about this.\n\nI.e. instead of importing the sources as-is why not just search-replace\nmalloc to mymalloc and free to myfree?\n\nWhich can be either a dumb \"sed\" script, or even better the same (and\nguaranteed to understand C syntax) thing with coccinelle/spatch.\n\nWhich wouldn't require libgit2 to have a dependency on that, just\nwhatever dev runs that one-off import occasionally. The semantic patch\nis just:\n\t\n\t@@\n\texpression E;\n\t@@\n\t- free(E);\n\t+ myfree(E);\n\t\n\t@@\n\texpression E;\n\t@@\n\t- malloc(E);\n\t+ mymalloc(E);\n\netc.\n\nWouldn't that also give you exactly what you want? Or was the plan to\nhave libgit2 have some option to build this *directly* from git.git\nsources?\n\n1. https://lore.kernel.org/git/xmqqfsohbdre.fsf@gitster.g/\n"}]}