git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v2 0/1] xdiff: provide indirection to git functions
@ 2022-02-17 22:52 Edward Thomson
  2022-02-17 22:54 ` [PATCH v2 1/1] " Edward Thomson
  0 siblings, 1 reply; 8+ messages in thread
From: Edward Thomson @ 2022-02-17 22:52 UTC (permalink / raw)
  To: git
  Cc: johannes.schindelin, Phillip Wood,
	Ævar Arnfjörð Bjarmason

Hello (again) from libgit2; this is a v2 of changes to xdiff to allow
us to work together more easily.  As discussed in the previous patch
(https://lore.kernel.org/git/20220209012951.GA7@abe733c6e288/) this
adds a simple abstraction layer in `git-xdiff.h`.

Other xdiff users, like libgit2, can specify their own compatibility
functions in this header file.

Cheers-
-ed

Edward Thomson (1):
  xdiff: provide indirection to git functions

 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/1] xdiff: provide indirection to git functions
  2022-02-17 22:52 [PATCH v2 0/1] xdiff: provide indirection to git functions Edward Thomson
@ 2022-02-17 22:54 ` Edward Thomson
  2022-02-22 11:14   ` Phillip Wood
  2022-02-25 19:03   ` Junio C Hamano
  0 siblings, 2 replies; 8+ messages in thread
From: Edward Thomson @ 2022-02-17 22:54 UTC (permalink / raw)
  To: git
  Cc: johannes.schindelin, Phillip Wood,
	Ævar Arnfjörð Bjarmason

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

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,
 				 &regmatch, 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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] xdiff: provide indirection to git functions
  2022-02-17 22:54 ` [PATCH v2 1/1] " Edward Thomson
@ 2022-02-22 11:14   ` Phillip Wood
  2022-02-25 15:41     ` Johannes Schindelin
  2022-02-25 19:03   ` Junio C Hamano
  1 sibling, 1 reply; 8+ messages in thread
From: Phillip Wood @ 2022-02-22 11:14 UTC (permalink / raw)
  To: Edward Thomson, git
  Cc: johannes.schindelin, Ævar Arnfjörð Bjarmason

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,
>   				 &regmatch, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] xdiff: provide indirection to git functions
  2022-02-22 11:14   ` Phillip Wood
@ 2022-02-25 15:41     ` Johannes Schindelin
  2022-02-25 18:24       ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Johannes Schindelin @ 2022-02-25 15:41 UTC (permalink / raw)
  To: phillip.wood
  Cc: Edward Thomson, git, Ævar Arnfjörð Bjarmason,
	gitster

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] xdiff: provide indirection to git functions
  2022-02-25 15:41     ` Johannes Schindelin
@ 2022-02-25 18:24       ` Junio C Hamano
  2022-02-25 18:38         ` Edward Thomson
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2022-02-25 18:24 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: phillip.wood, Edward Thomson, git,
	Ævar Arnfjörð Bjarmason

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.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] xdiff: provide indirection to git functions
  2022-02-25 18:24       ` Junio C Hamano
@ 2022-02-25 18:38         ` Edward Thomson
  2022-02-25 18:58           ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Edward Thomson @ 2022-02-25 18:38 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin, phillip.wood, git,
	Ævar Arnfjörð Bjarmason

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] xdiff: provide indirection to git functions
  2022-02-25 18:38         ` Edward Thomson
@ 2022-02-25 18:58           ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2022-02-25 18:58 UTC (permalink / raw)
  To: Edward Thomson
  Cc: Johannes Schindelin, phillip.wood, git,
	Ævar Arnfjörð Bjarmason

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] xdiff: provide indirection to git functions
  2022-02-17 22:54 ` [PATCH v2 1/1] " Edward Thomson
  2022-02-22 11:14   ` Phillip Wood
@ 2022-02-25 19:03   ` Junio C Hamano
  1 sibling, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2022-02-25 19:03 UTC (permalink / raw)
  To: Edward Thomson
  Cc: git, johannes.schindelin, Phillip Wood,
	Ævar Arnfjörð Bjarmason

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-02-25 19:03 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-17 22:52 [PATCH v2 0/1] xdiff: provide indirection to git functions Edward Thomson
2022-02-17 22:54 ` [PATCH v2 1/1] " Edward Thomson
2022-02-22 11:14   ` Phillip Wood
2022-02-25 15:41     ` Johannes Schindelin
2022-02-25 18:24       ` Junio C Hamano
2022-02-25 18:38         ` Edward Thomson
2022-02-25 18:58           ` Junio C Hamano
2022-02-25 19:03   ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).