| Message ID | 20220217225408.GB7@edef91d97c94 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xdiff: provide indirection to git functions | expand |
On 17/02/2022 22:54, 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. The changes since V1 look good, Best Wishes Phillip > Signed-off-by: Edward Thomson <ethomson@edwardthomson.com> > --- > xdiff/git-xdiff.h | 16 ++++++++++++++++ > xdiff/xdiff.h | 8 +++----- > xdiff/xdiffi.c | 20 ++++++++++---------- > xdiff/xinclude.h | 2 +- > xdiff/xmerge.c | 4 ++-- > 5 files changed, 32 insertions(+), 18 deletions(-) > create mode 100644 xdiff/git-xdiff.h > > diff --git a/xdiff/git-xdiff.h b/xdiff/git-xdiff.h > new file mode 100644 > index 0000000000..664a7c1351 > --- /dev/null > +++ b/xdiff/git-xdiff.h > @@ -0,0 +1,16 @@ > +#ifndef GIT_XDIFF_H > +#define GIT_XDIFF_H > + > +#include "git-compat-util.h" > + > +#define xdl_malloc(x) xmalloc(x) > +#define xdl_free(ptr) free(ptr) > +#define xdl_realloc(ptr,x) xrealloc(ptr,x) > + > +#define xdl_regex_t regex_t > +#define xdl_regmatch_t regmatch_t > +#define xdl_regexec_buf(p, b, s, n, m, f) regexec_buf(p, b, s, n, m, f) > + > +#define XDL_BUG(msg) BUG(msg) > + > +#endif > diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h > index 72e25a9ffa..fb47f63fbf 100644 > --- a/xdiff/xdiff.h > +++ b/xdiff/xdiff.h > @@ -27,6 +27,8 @@ > extern "C" { > #endif /* #ifdef __cplusplus */ > > +#include "git-xdiff.h" > + > /* xpparm_t.flags */ > #define XDF_NEED_MINIMAL (1 << 0) > > @@ -82,7 +84,7 @@ typedef struct s_xpparam { > unsigned long flags; > > /* -I<regex> */ > - regex_t **ignore_regex; > + xdl_regex_t **ignore_regex; > size_t ignore_regex_nr; > > /* See Documentation/diff-options.txt. */ > @@ -119,10 +121,6 @@ typedef struct s_bdiffparam { > } bdiffparam_t; > > > -#define xdl_malloc(x) xmalloc(x) > -#define xdl_free(ptr) free(ptr) > -#define xdl_realloc(ptr,x) xrealloc(ptr,x) > - > void *xdl_mmfile_first(mmfile_t *mmf, long *size); > long xdl_mmfile_size(mmfile_t *mmf); > > diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c > index 69689fab24..af31b7f4b3 100644 > --- a/xdiff/xdiffi.c > +++ b/xdiff/xdiffi.c > @@ -832,7 +832,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) { > /* Shift the group backward as much as possible: */ > while (!group_slide_up(xdf, &g)) > if (group_previous(xdfo, &go)) > - BUG("group sync broken sliding up"); > + XDL_BUG("group sync broken sliding up"); > > /* > * This is this highest that this group can be shifted. > @@ -848,7 +848,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) { > if (group_slide_down(xdf, &g)) > break; > if (group_next(xdfo, &go)) > - BUG("group sync broken sliding down"); > + XDL_BUG("group sync broken sliding down"); > > if (go.end > go.start) > end_matching_other = g.end; > @@ -873,9 +873,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) { > */ > while (go.end == go.start) { > if (group_slide_up(xdf, &g)) > - BUG("match disappeared"); > + XDL_BUG("match disappeared"); > if (group_previous(xdfo, &go)) > - BUG("group sync broken sliding to match"); > + XDL_BUG("group sync broken sliding to match"); > } > } else if (flags & XDF_INDENT_HEURISTIC) { > /* > @@ -916,9 +916,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) { > > while (g.end > best_shift) { > if (group_slide_up(xdf, &g)) > - BUG("best shift unreached"); > + XDL_BUG("best shift unreached"); > if (group_previous(xdfo, &go)) > - BUG("group sync broken sliding to blank line"); > + XDL_BUG("group sync broken sliding to blank line"); > } > } > > @@ -927,11 +927,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) { > if (group_next(xdf, &g)) > break; > if (group_next(xdfo, &go)) > - BUG("group sync broken moving to next group"); > + XDL_BUG("group sync broken moving to next group"); > } > > if (!group_next(xdfo, &go)) > - BUG("group sync broken at end of file"); > + XDL_BUG("group sync broken at end of file"); > > return 0; > } > @@ -1011,11 +1011,11 @@ static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t *xe, long flags) > } > > static int record_matches_regex(xrecord_t *rec, xpparam_t const *xpp) { > - regmatch_t regmatch; > + xdl_regmatch_t regmatch; > int i; > > for (i = 0; i < xpp->ignore_regex_nr; i++) > - if (!regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1, > + if (!xdl_regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1, > ®match, 0)) > return 1; > > diff --git a/xdiff/xinclude.h b/xdiff/xinclude.h > index a4285ac0eb..75db1d8f35 100644 > --- a/xdiff/xinclude.h > +++ b/xdiff/xinclude.h > @@ -23,7 +23,7 @@ > #if !defined(XINCLUDE_H) > #define XINCLUDE_H > > -#include "git-compat-util.h" > +#include "git-xdiff.h" > #include "xmacros.h" > #include "xdiff.h" > #include "xtypes.h" > diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c > index fff0b594f9..433e2d7415 100644 > --- a/xdiff/xmerge.c > +++ b/xdiff/xmerge.c > @@ -88,7 +88,7 @@ static int xdl_cleanup_merge(xdmerge_t *c) > if (c->mode == 0) > count++; > next_c = c->next; > - free(c); > + xdl_free(c); > } > return count; > } > @@ -456,7 +456,7 @@ static void xdl_merge_two_conflicts(xdmerge_t *m) > m->chg1 = next_m->i1 + next_m->chg1 - m->i1; > m->chg2 = next_m->i2 + next_m->chg2 - m->i2; > m->next = next_m->next; > - free(next_m); > + xdl_free(next_m); > } > > /* > -- > 2.35.1
Hi, On Tue, 22 Feb 2022, Phillip Wood wrote: > On 17/02/2022 22:54, 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. > > The changes since V1 look good, Indeed. This is the range-diff: -- snip -- 1: 52c8f141cbe1 ! 1: e05e9b5e2f27 xdiff: provide indirection to git functions @@ xdiff/git-xdiff.h (new) +#ifndef GIT_XDIFF_H +#define GIT_XDIFF_H + ++#include "git-compat-util.h" ++ +#define xdl_malloc(x) xmalloc(x) +#define xdl_free(ptr) free(ptr) +#define xdl_realloc(ptr,x) xrealloc(ptr,x) @@ xdiff/xdiffi.c: static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t ## xdiff/xinclude.h ## @@ + #if !defined(XINCLUDE_H) #define XINCLUDE_H - #include "git-compat-util.h" +-#include "git-compat-util.h" +#include "git-xdiff.h" #include "xmacros.h" #include "xdiff.h" #include "xtypes.h" -@@ - #include "xdiffi.h" - #include "xemit.h" + + ## xdiff/xmerge.c ## +@@ xdiff/xmerge.c: static int xdl_cleanup_merge(xdmerge_t *c) + if (c->mode == 0) + count++; + next_c = c->next; +- free(c); ++ xdl_free(c); + } + return count; + } +@@ xdiff/xmerge.c: static void xdl_merge_two_conflicts(xdmerge_t *m) + m->chg1 = next_m->i1 + next_m->chg1 - m->i1; + m->chg2 = next_m->i2 + next_m->chg2 - m->i2; + m->next = next_m->next; +- free(next_m); ++ xdl_free(next_m); + } -- - #endif /* #if !defined(XINCLUDE_H) */ + /* -- snap -- My ACK from https://lore.kernel.org/git/nycvar.QRO.7.76.6.2202171644090.348@tvgsbejvaqbjf.bet/ still holds. Junio could you please add it before merging it down to `next`? Thanks, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi, > > On Tue, 22 Feb 2022, Phillip Wood wrote: > >> On 17/02/2022 22:54, 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. >> >> The changes since V1 look good, > > Indeed. This is the range-diff: > > -- snip -- > 1: 52c8f141cbe1 ! 1: e05e9b5e2f27 xdiff: provide indirection to git functions > @@ xdiff/git-xdiff.h (new) > +#ifndef GIT_XDIFF_H > +#define GIT_XDIFF_H > + > ++#include "git-compat-util.h" > ++ > +#define xdl_malloc(x) xmalloc(x) > +#define xdl_free(ptr) free(ptr) > +#define xdl_realloc(ptr,x) xrealloc(ptr,x) > @@ xdiff/xdiffi.c: static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t > > ## xdiff/xinclude.h ## > @@ > + #if !defined(XINCLUDE_H) > #define XINCLUDE_H > > - #include "git-compat-util.h" > +-#include "git-compat-util.h" > +#include "git-xdiff.h" > #include "xmacros.h" > #include "xdiff.h" > #include "xtypes.h" > -@@ > - #include "xdiffi.h" > - #include "xemit.h" > + > + ## xdiff/xmerge.c ## > +@@ xdiff/xmerge.c: static int xdl_cleanup_merge(xdmerge_t *c) > + if (c->mode == 0) > + count++; > + next_c = c->next; > +- free(c); > ++ xdl_free(c); > + } > + return count; > + } > +@@ xdiff/xmerge.c: static void xdl_merge_two_conflicts(xdmerge_t *m) > + m->chg1 = next_m->i1 + next_m->chg1 - m->i1; > + m->chg2 = next_m->i2 + next_m->chg2 - m->i2; > + m->next = next_m->next; > +- free(next_m); > ++ xdl_free(next_m); > + } > > -- > - #endif /* #if !defined(XINCLUDE_H) */ > + /* > -- snap -- > > My ACK from > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2202171644090.348@tvgsbejvaqbjf.bet/ > still holds. Junio could you please add it before merging it down to > `next`? Not so fast. I still do not see a strong reason to support xdl_malloc() and other wrappers. Is the expectation for other projects when using the unified code, they do not use xdiff/git-xdiff.h and instead add xdiff/frotz-xdiff.h that defines xdl_malloc() and friends with the infrastructure they provide as part of the Frotz project (and the Xyzzy project would do the same with xdiff/xyzzy-xdiff.h header for them), making "git" the first among equal other consumers? If that is the direction this indirection is aiming for, stating it clearly may be a start of a not-so-bad justification, but then the hardcoded inclusion of "git-xdiff.h" in xdiff/xinclude.h still contradicts with it, which may want to be fixed.
On Fri, Feb 25, 2022 at 10:24:14AM -0800, Junio C Hamano wrote: > > Not so fast. I still do not see a strong reason to support > xdl_malloc() and other wrappers. git has an `xmalloc` but no matching `xfree`. libgit2 does not necessarily use the system allocator (and on Windows, you run into the question of _which_ system allocator you're using) and therefore has its own allocation _and_ deallocation functions. When libgit2 includes xdiff, I don't want to monkey around and try to redefine `free` to our deallocator. There are several options that could suffice for this. A different tactic is to have xdiff call `xfree` which is just defined as `free` in git. This would feel non-obvious to me as a git developer that in this one part of the project, I need to use `xfree` instead of `free` on memory that I have `xmalloc`ed. Using a net new name for allocation functions may help serve as a reminder that it is a different API. > Is the expectation for other projects when using the unified code, > they do not use xdiff/git-xdiff.h and instead add > xdiff/frotz-xdiff.h that defines xdl_malloc() and friends with the > infrastructure they provide as part of the Frotz project (and the > Xyzzy project would do the same with xdiff/xyzzy-xdiff.h header for > them), making "git" the first among equal other consumers? No, the thinking is that they would provide their own `git-xdiff.h` that defines the mappings to their project-specific APIs. Cheers- -ed
Edward Thomson <ethomson@edwardthomson.com> writes: > No, the thinking is that they would provide their own `git-xdiff.h` that > defines the mappings to their project-specific APIs. Is that spelled out somewhere? That would help future readers of the file to learn what they need to do when reusing the part, perhaps in a comment near the top of that file itself. If git-xdiff.h is meant to be modified to match the need for non-git codebase, it probably should be named to a more descriptive name, like xdiff-compat.h or something, I would think. git-xdiff.h that has libgit2 specific names in it would look quite strange.
Edward Thomson <ethomson@edwardthomson.com> writes: > 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. Continuing the "what do they exactly do" line of thought, the above is not quite in line with what I heard. They take all the files including git-xdiff.h and they must modify git-xdiff.h to match their environment. In any case, ... > diff --git a/xdiff/git-xdiff.h b/xdiff/git-xdiff.h > new file mode 100644 > index 0000000000..664a7c1351 > --- /dev/null > +++ b/xdiff/git-xdiff.h > @@ -0,0 +1,16 @@ > +#ifndef GIT_XDIFF_H > +#define GIT_XDIFF_H ... here is a good place to spell the expectation out, i.e. that they are expected to change this file to match their system, and that all the things they see below here (including the inclusion of git-compat-util.h) is specific to git-core they are expected to rip out and replace. > + > +#include "git-compat-util.h" > + > +#define xdl_malloc(x) xmalloc(x) > +#define xdl_free(ptr) free(ptr) > +#define xdl_realloc(ptr,x) xrealloc(ptr,x) > + > +#define xdl_regex_t regex_t > +#define xdl_regmatch_t regmatch_t > +#define xdl_regexec_buf(p, b, s, n, m, f) regexec_buf(p, b, s, n, m, f) > + > +#define XDL_BUG(msg) BUG(msg) > + > +#endif Thanks.
diff --git a/xdiff/git-xdiff.h b/xdiff/git-xdiff.h new file mode 100644 index 0000000000..664a7c1351 --- /dev/null +++ b/xdiff/git-xdiff.h @@ -0,0 +1,16 @@ +#ifndef GIT_XDIFF_H +#define GIT_XDIFF_H + +#include "git-compat-util.h" + +#define xdl_malloc(x) xmalloc(x) +#define xdl_free(ptr) free(ptr) +#define xdl_realloc(ptr,x) xrealloc(ptr,x) + +#define xdl_regex_t regex_t +#define xdl_regmatch_t regmatch_t +#define xdl_regexec_buf(p, b, s, n, m, f) regexec_buf(p, b, s, n, m, f) + +#define XDL_BUG(msg) BUG(msg) + +#endif diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h index 72e25a9ffa..fb47f63fbf 100644 --- a/xdiff/xdiff.h +++ b/xdiff/xdiff.h @@ -27,6 +27,8 @@ extern "C" { #endif /* #ifdef __cplusplus */ +#include "git-xdiff.h" + /* xpparm_t.flags */ #define XDF_NEED_MINIMAL (1 << 0) @@ -82,7 +84,7 @@ typedef struct s_xpparam { unsigned long flags; /* -I<regex> */ - regex_t **ignore_regex; + xdl_regex_t **ignore_regex; size_t ignore_regex_nr; /* See Documentation/diff-options.txt. */ @@ -119,10 +121,6 @@ typedef struct s_bdiffparam { } bdiffparam_t; -#define xdl_malloc(x) xmalloc(x) -#define xdl_free(ptr) free(ptr) -#define xdl_realloc(ptr,x) xrealloc(ptr,x) - void *xdl_mmfile_first(mmfile_t *mmf, long *size); long xdl_mmfile_size(mmfile_t *mmf); diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c index 69689fab24..af31b7f4b3 100644 --- a/xdiff/xdiffi.c +++ b/xdiff/xdiffi.c @@ -832,7 +832,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) { /* Shift the group backward as much as possible: */ while (!group_slide_up(xdf, &g)) if (group_previous(xdfo, &go)) - BUG("group sync broken sliding up"); + XDL_BUG("group sync broken sliding up"); /* * This is this highest that this group can be shifted. @@ -848,7 +848,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) { if (group_slide_down(xdf, &g)) break; if (group_next(xdfo, &go)) - BUG("group sync broken sliding down"); + XDL_BUG("group sync broken sliding down"); if (go.end > go.start) end_matching_other = g.end; @@ -873,9 +873,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) { */ while (go.end == go.start) { if (group_slide_up(xdf, &g)) - BUG("match disappeared"); + XDL_BUG("match disappeared"); if (group_previous(xdfo, &go)) - BUG("group sync broken sliding to match"); + XDL_BUG("group sync broken sliding to match"); } } else if (flags & XDF_INDENT_HEURISTIC) { /* @@ -916,9 +916,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) { while (g.end > best_shift) { if (group_slide_up(xdf, &g)) - BUG("best shift unreached"); + XDL_BUG("best shift unreached"); if (group_previous(xdfo, &go)) - BUG("group sync broken sliding to blank line"); + XDL_BUG("group sync broken sliding to blank line"); } } @@ -927,11 +927,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) { if (group_next(xdf, &g)) break; if (group_next(xdfo, &go)) - BUG("group sync broken moving to next group"); + XDL_BUG("group sync broken moving to next group"); } if (!group_next(xdfo, &go)) - BUG("group sync broken at end of file"); + XDL_BUG("group sync broken at end of file"); return 0; } @@ -1011,11 +1011,11 @@ static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t *xe, long flags) } static int record_matches_regex(xrecord_t *rec, xpparam_t const *xpp) { - regmatch_t regmatch; + xdl_regmatch_t regmatch; int i; for (i = 0; i < xpp->ignore_regex_nr; i++) - if (!regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1, + if (!xdl_regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1, ®match, 0)) return 1; diff --git a/xdiff/xinclude.h b/xdiff/xinclude.h index a4285ac0eb..75db1d8f35 100644 --- a/xdiff/xinclude.h +++ b/xdiff/xinclude.h @@ -23,7 +23,7 @@ #if !defined(XINCLUDE_H) #define XINCLUDE_H -#include "git-compat-util.h" +#include "git-xdiff.h" #include "xmacros.h" #include "xdiff.h" #include "xtypes.h" diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c index fff0b594f9..433e2d7415 100644 --- a/xdiff/xmerge.c +++ b/xdiff/xmerge.c @@ -88,7 +88,7 @@ static int xdl_cleanup_merge(xdmerge_t *c) if (c->mode == 0) count++; next_c = c->next; - free(c); + xdl_free(c); } return count; } @@ -456,7 +456,7 @@ static void xdl_merge_two_conflicts(xdmerge_t *m) m->chg1 = next_m->i1 + next_m->chg1 - m->i1; m->chg2 = next_m->i2 + next_m->chg2 - m->i2; m->next = next_m->next; - free(next_m); + xdl_free(next_m); } /*
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. Signed-off-by: Edward Thomson <ethomson@edwardthomson.com> --- xdiff/git-xdiff.h | 16 ++++++++++++++++ xdiff/xdiff.h | 8 +++----- xdiff/xdiffi.c | 20 ++++++++++---------- xdiff/xinclude.h | 2 +- xdiff/xmerge.c | 4 ++-- 5 files changed, 32 insertions(+), 18 deletions(-) create mode 100644 xdiff/git-xdiff.h -- 2.35.1