[PATCH 1/2] Make xdiff_outf_{init,release} interface

[PATCH 1/2] Make xdiff_outf_{init,release} interface

[Brian Downing]

To prepare for the need to release resources when we're done with
xdiff_outf, make a new function pair to initialize and release a
struct xdiff_emit_state.  Since we can roll the two lines always
required to use xdiff_outf into one, the net number of lines at the call
site remains constant.

Old:

	ecb.outf = xdiff_outf;
	ecb.priv = &state;
	...
	xdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);

New:

	xdiff_outf_init(&ecb, &state);
	...
	xdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);
	xdiff_outf_release(&state);

Signed-off-by: Brian Downing <bdowning@lavos.net>
---
 builtin-blame.c   |    4 ++--
 combine-diff.c    |    4 ++--
 diff.c            |   20 ++++++++++----------
 xdiff-interface.c |   11 +++++++++++
 xdiff-interface.h |    2 ++
 5 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index 4ea3431..b52eff4 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -528,15 +528,15 @@ static struct patch *compare_buffer(mmfile_t *file_p, mmfile_t *file_o,
 	xpp.flags = xdl_opts;
 	memset(&xecfg, 0, sizeof(xecfg));
 	xecfg.ctxlen = context;
-	ecb.outf = xdiff_outf;
-	ecb.priv = &state;
 	memset(&state, 0, sizeof(state));
+	xdiff_outf_init(&ecb, &state);
 	state.xm.consume = process_u_diff;
 	state.ret = xmalloc(sizeof(struct patch));
 	state.ret->chunks = NULL;
 	state.ret->num = 0;
 
 	xdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);
+	xdiff_outf_release(&state);
 
 	if (state.ret->num) {
 		struct chunk *chunk;
diff --git a/combine-diff.c b/combine-diff.c
index 9f80a1c..887c315 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -217,9 +217,8 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,
 	parent_file.size = sz;
 	xpp.flags = XDF_NEED_MINIMAL;
 	memset(&xecfg, 0, sizeof(xecfg));
-	ecb.outf = xdiff_outf;
-	ecb.priv = &state;
 	memset(&state, 0, sizeof(state));
+	xdiff_outf_init(&ecb, &state);
 	state.xm.consume = consume_line;
 	state.nmask = nmask;
 	state.sline = sline;
@@ -228,6 +227,7 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,
 	state.n = n;
 
 	xdi_diff(&parent_file, result_file, &xpp, &xecfg, &ecb);
+	xdiff_outf_release(&state);
 	free(parent_file.ptr);
 
 	/* Assign line numbers for this parent.
diff --git a/diff.c b/diff.c
index 6954f99..222646d 100644
--- a/diff.c
+++ b/diff.c
@@ -459,10 +459,10 @@ static void diff_words_show(struct diff_words_data *diff_words)
 
 	xpp.flags = XDF_NEED_MINIMAL;
 	xecfg.ctxlen = diff_words->minus.alloc + diff_words->plus.alloc;
-	ecb.outf = xdiff_outf;
-	ecb.priv = diff_words;
+	xdiff_outf_init(&ecb, diff_words);
 	diff_words->xm.consume = fn_out_diff_words_aux;
 	xdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);
+	xdiff_outf_release(diff_words);
 
 	free(minus.ptr);
 	free(plus.ptr);
@@ -1520,8 +1520,7 @@ static void builtin_diff(const char *name_a,
 			xecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);
 		else if (!prefixcmp(diffopts, "-u"))
 			xecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);
-		ecb.outf = xdiff_outf;
-		ecb.priv = &ecbdata;
+		xdiff_outf_init(&ecb, &ecbdata);
 		ecbdata.xm.consume = fn_out_consume;
 		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS)) {
 			ecbdata.diff_words =
@@ -1529,6 +1528,7 @@ static void builtin_diff(const char *name_a,
 			ecbdata.diff_words->file = o->file;
 		}
 		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdiff_outf_release(&ecbdata);
 		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))
 			free_diff_words_data(&ecbdata);
 	}
@@ -1579,9 +1579,9 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 
 		memset(&xecfg, 0, sizeof(xecfg));
 		xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
-		ecb.outf = xdiff_outf;
-		ecb.priv = diffstat;
+		xdiff_outf_init(&ecb, diffstat);
 		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdiff_outf_release(diffstat);
 	}
 
  free_and_return:
@@ -1627,9 +1627,9 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
 
 		memset(&xecfg, 0, sizeof(xecfg));
 		xpp.flags = XDF_NEED_MINIMAL;
-		ecb.outf = xdiff_outf;
-		ecb.priv = &data;
+		xdiff_outf_init(&ecb, &data);
 		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdiff_outf_release(&data);
 
 		if ((data.ws_rule & WS_TRAILING_SPACE) &&
 		    data.trailing_blanks_start) {
@@ -3127,9 +3127,9 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 		xpp.flags = XDF_NEED_MINIMAL;
 		xecfg.ctxlen = 3;
 		xecfg.flags = XDL_EMIT_FUNCNAMES;
-		ecb.outf = xdiff_outf;
-		ecb.priv = &data;
+		xdiff_outf_init(&ecb, &data);
 		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdiff_outf_release(&data);
 	}
 
 	SHA1_Final(sha1, &ctx);
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 61dc5c5..9c5e277 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -61,6 +61,13 @@ static void consume_one(void *priv_, char *s, unsigned long size)
 	}
 }
 
+void xdiff_outf_init(xdemitcb_t *ecb, void *priv_)
+{
+	struct xdiff_emit_state *priv = priv_;
+	ecb->outf = xdiff_outf;
+	ecb->priv = priv;
+}
+
 int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
 {
 	struct xdiff_emit_state *priv = priv_;
@@ -103,6 +110,10 @@ int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
 	return 0;
 }
 
+void xdiff_outf_release(void *priv_)
+{
+}
+
 /*
  * Trim down common substring at the end of the buffers,
  * but leave at least ctx lines at the end.
diff --git a/xdiff-interface.h b/xdiff-interface.h
index f7f791d..fca6200 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -14,7 +14,9 @@ struct xdiff_emit_state {
 };
 
 int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);
+void xdiff_outf_init(xdemitcb_t *ecb, void *priv_);
 int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf);
+void xdiff_outf_release(void *priv_);
 int parse_hunk_header(char *line, int len,
 		      int *ob, int *on,
 		      int *nb, int *nn);
-- 
1.5.6.1

Re: [PATCH 1/2] Make xdiff_outf_{init,release} interface

[Junio C Hamano]

Brian Downing <bdowning@lavos.net> writes:

> @@ -103,6 +110,10 @@ int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
>  	return 0;
>  }
>  
> +void xdiff_outf_release(void *priv_)
> +{
> +}
> +

It might make it more clear to have this function take a pointer to
"struct xdiff_emit_state", which is always the first member of the
callback private data structure.

Although I wish xdi_diff() could do the necessary clean-up immediately
before it returns (so that the caller did not have to do anything
special), it is not possible to do so cleanly, because there are "outf"
implementations other than xdiff_outf that do not even use "struct
xdiff_emit_state" in their callbacks.  So I think your patch makes sense.

Thanks.

Re: [PATCH 1/2] Make xdiff_outf_{init,release} interface

[Brian Downing]

On Wed, Aug 13, 2008 at 05:46:29PM -0700, Junio C Hamano wrote:
> Brian Downing <bdowning@lavos.net> writes:
> > @@ -103,6 +110,10 @@ int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
> >  	return 0;
> >  }
> >  
> > +void xdiff_outf_release(void *priv_)
> > +{
> > +}
> > +
> 
> It might make it more clear to have this function take a pointer to
> "struct xdiff_emit_state", which is always the first member of the
> callback private data structure.

That makes the call sites (slightly) more complicated, in that instead
of:
	xdiff_outf_release(&state);
you'd want:
	xdiff_outf_release(&state.xm);

That was not the typical usage before, in that it said "ecb.priv =
&state" rather than "ecb.priv = &state.xm", and I used the void *
argument to mirror that, but I can change it if it'd be preferable.
 
> Although I wish xdi_diff() could do the necessary clean-up immediately
> before it returns (so that the caller did not have to do anything
> special), it is not possible to do so cleanly, because there are
> "outf" implementations other than xdiff_outf that do not even use
> "struct xdiff_emit_state" in their callbacks.  So I think your patch
> makes sense.

Well, I could do something like:

	if (xecb->outf == xdiff_outf)
		/* xdiff_outf cleanup */

at the end of xdi_diff, but that's... kind of horrible I think.

For that matter, I could just make an xdi_outf_diff function that would
take the state in addition to the other xdi_diff arguments and go ahead
and set it up, do the diff, and tear it down in one step.  Maybe that
would be better if it works for everywhere this style of diff needs to
be called.

Another question:  should I go ahead and make xdiff_outf itself static?

-bcd

Re: [PATCH 1/2] Make xdiff_outf_{init,release} interface

[Junio C Hamano]

bdowning@lavos.net (Brian Downing) writes:

>> Although I wish xdi_diff() could do the necessary clean-up immediately
>> before it returns (so that the caller did not have to do anything
>> special), it is not possible to do so cleanly, because there are
>> "outf" implementations other than xdiff_outf that do not even use
>> "struct xdiff_emit_state" in their callbacks.  So I think your patch
>> makes sense.
>
> Well, I could do something like:
>
> 	if (xecb->outf == xdiff_outf)
> 		/* xdiff_outf cleanup */
>
> at the end of xdi_diff, but that's... kind of horrible I think.

Yeah, that is ugly, and that is why I said I think your patch makes sense.

> For that matter, I could just make an xdi_outf_diff function that would
> take the state in addition to the other xdi_diff arguments and go ahead
> and set it up, do the diff, and tear it down in one step.  Maybe that
> would be better if it works for everywhere this style of diff needs to
> be called.

Yeah, most of the xdi_diff() callers do use the stock outf so it would
make sense.

[PATCHv2 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs

[Brian Downing]

To prepare for the need to initialize and release resources for an
xdi_diff with the xdiff_outf output function, make a new function to
wrap this usage.

Old:

	ecb.outf = xdiff_outf;
	ecb.priv = &state;
	...
	xdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);

New:

	xdi_diff_outf(file_p, file_o, &state.xm, &xpp, &xecfg, &ecb);

Signed-off-by: Brian Downing <bdowning@lavos.net>
---

    Let's try this instead; it's quite a bit cleaner than my last
    effort.

 builtin-blame.c   |    4 +---
 combine-diff.c    |    5 ++---
 diff.c            |   20 +++++---------------
 xdiff-interface.c |   13 ++++++++++++-
 xdiff-interface.h |    4 +++-
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index 4ea3431..8cca3b1 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -528,15 +528,13 @@ static struct patch *compare_buffer(mmfile_t *file_p, mmfile_t *file_o,
 	xpp.flags = xdl_opts;
 	memset(&xecfg, 0, sizeof(xecfg));
 	xecfg.ctxlen = context;
-	ecb.outf = xdiff_outf;
-	ecb.priv = &state;
 	memset(&state, 0, sizeof(state));
 	state.xm.consume = process_u_diff;
 	state.ret = xmalloc(sizeof(struct patch));
 	state.ret->chunks = NULL;
 	state.ret->num = 0;
 
-	xdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);
+	xdi_diff_outf(file_p, file_o, &state.xm, &xpp, &xecfg, &ecb);
 
 	if (state.ret->num) {
 		struct chunk *chunk;
diff --git a/combine-diff.c b/combine-diff.c
index 9f80a1c..72dd6d2 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -217,8 +217,6 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,
 	parent_file.size = sz;
 	xpp.flags = XDF_NEED_MINIMAL;
 	memset(&xecfg, 0, sizeof(xecfg));
-	ecb.outf = xdiff_outf;
-	ecb.priv = &state;
 	memset(&state, 0, sizeof(state));
 	state.xm.consume = consume_line;
 	state.nmask = nmask;
@@ -227,7 +225,8 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,
 	state.num_parent = num_parent;
 	state.n = n;
 
-	xdi_diff(&parent_file, result_file, &xpp, &xecfg, &ecb);
+	xdi_diff_outf(&parent_file, result_file,
+		      &state.xm, &xpp, &xecfg, &ecb);
 	free(parent_file.ptr);
 
 	/* Assign line numbers for this parent.
diff --git a/diff.c b/diff.c
index bf5d5f1..913a92f 100644
--- a/diff.c
+++ b/diff.c
@@ -459,10 +459,8 @@ static void diff_words_show(struct diff_words_data *diff_words)
 
 	xpp.flags = XDF_NEED_MINIMAL;
 	xecfg.ctxlen = diff_words->minus.alloc + diff_words->plus.alloc;
-	ecb.outf = xdiff_outf;
-	ecb.priv = diff_words;
 	diff_words->xm.consume = fn_out_diff_words_aux;
-	xdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);
+	xdi_diff_outf(&minus, &plus, &diff_words->xm, &xpp, &xecfg, &ecb);
 
 	free(minus.ptr);
 	free(plus.ptr);
@@ -1521,15 +1519,13 @@ static void builtin_diff(const char *name_a,
 			xecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);
 		else if (!prefixcmp(diffopts, "-u"))
 			xecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);
-		ecb.outf = xdiff_outf;
-		ecb.priv = &ecbdata;
 		ecbdata.xm.consume = fn_out_consume;
 		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS)) {
 			ecbdata.diff_words =
 				xcalloc(1, sizeof(struct diff_words_data));
 			ecbdata.diff_words->file = o->file;
 		}
-		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, &ecbdata.xm, &xpp, &xecfg, &ecb);
 		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))
 			free_diff_words_data(&ecbdata);
 	}
@@ -1580,9 +1576,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 
 		memset(&xecfg, 0, sizeof(xecfg));
 		xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
-		ecb.outf = xdiff_outf;
-		ecb.priv = diffstat;
-		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, &diffstat->xm, &xpp, &xecfg, &ecb);
 	}
 
  free_and_return:
@@ -1628,9 +1622,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
 
 		memset(&xecfg, 0, sizeof(xecfg));
 		xpp.flags = XDF_NEED_MINIMAL;
-		ecb.outf = xdiff_outf;
-		ecb.priv = &data;
-		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);
 
 		if ((data.ws_rule & WS_TRAILING_SPACE) &&
 		    data.trailing_blanks_start) {
@@ -3128,9 +3120,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 		xpp.flags = XDF_NEED_MINIMAL;
 		xecfg.ctxlen = 3;
 		xecfg.flags = XDL_EMIT_FUNCNAMES;
-		ecb.outf = xdiff_outf;
-		ecb.priv = &data;
-		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);
 	}
 
 	SHA1_Final(sha1, &ctx);
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 61dc5c5..be448a0 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -61,7 +61,7 @@ static void consume_one(void *priv_, char *s, unsigned long size)
 	}
 }
 
-int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
+static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
 {
 	struct xdiff_emit_state *priv = priv_;
 	int i;
@@ -141,6 +141,17 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co
 	return xdl_diff(&a, &b, xpp, xecfg, xecb);
 }
 
+int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
+		  struct xdiff_emit_state *state, xpparam_t const *xpp,
+		  xdemitconf_t const *xecfg, xdemitcb_t *xecb)
+{
+	int ret;
+	xecb->outf = xdiff_outf;
+	xecb->priv = state;
+	ret = xdl_diff(mf1, mf2, xpp, xecfg, xecb);
+	return ret;
+}
+
 int read_mmfile(mmfile_t *ptr, const char *filename)
 {
 	struct stat st;
diff --git a/xdiff-interface.h b/xdiff-interface.h
index f7f791d..6f3b361 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -14,7 +14,9 @@ struct xdiff_emit_state {
 };
 
 int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);
-int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf);
+int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
+		  struct xdiff_emit_state *state, xpparam_t const *xpp,
+		  xdemitconf_t const *xecfg, xdemitcb_t *xecb);
 int parse_hunk_header(char *line, int len,
 		      int *ob, int *on,
 		      int *nb, int *nn);
-- 
1.5.6.1

[PATCHv2 2/2] Use strbuf for struct xdiff_emit_state's remainder

[Brian Downing]

Continually xreallocing and freeing the remainder member of struct
xdiff_emit_state was a noticeable performance hit.  Use a strbuf
instead.

This yields a decent performance improvement on "git blame" on certain
repositories.  For example, before this commit:

$ time git blame -M -C -C -p --incremental server.c >/dev/null
101.52user 0.17system 1:41.73elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+39561minor)pagefaults 0swaps

With this commit:

$ time git blame -M -C -C -p --incremental server.c >/dev/null
80.38user 0.30system 1:20.81elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+50979minor)pagefaults 0swaps

Signed-off-by: Brian Downing <bdowning@lavos.net>
---
 xdiff-interface.c |   32 ++++++++++----------------------
 xdiff-interface.h |    4 ++--
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/xdiff-interface.c b/xdiff-interface.c
index be448a0..c999469 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -69,36 +69,22 @@ static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
 	for (i = 0; i < nbuf; i++) {
 		if (mb[i].ptr[mb[i].size-1] != '\n') {
 			/* Incomplete line */
-			priv->remainder = xrealloc(priv->remainder,
-						   priv->remainder_size +
-						   mb[i].size);
-			memcpy(priv->remainder + priv->remainder_size,
-			       mb[i].ptr, mb[i].size);
-			priv->remainder_size += mb[i].size;
+			strbuf_add(&priv->remainder, mb[i].ptr, mb[i].size);
 			continue;
 		}
 
 		/* we have a complete line */
-		if (!priv->remainder) {
+		if (!priv->remainder.len) {
 			consume_one(priv, mb[i].ptr, mb[i].size);
 			continue;
 		}
-		priv->remainder = xrealloc(priv->remainder,
-					   priv->remainder_size +
-					   mb[i].size);
-		memcpy(priv->remainder + priv->remainder_size,
-		       mb[i].ptr, mb[i].size);
-		consume_one(priv, priv->remainder,
-			    priv->remainder_size + mb[i].size);
-		free(priv->remainder);
-		priv->remainder = NULL;
-		priv->remainder_size = 0;
+		strbuf_add(&priv->remainder, mb[i].ptr, mb[i].size);
+		consume_one(priv, priv->remainder.buf, priv->remainder.len);
+		strbuf_reset(&priv->remainder);
 	}
-	if (priv->remainder) {
-		consume_one(priv, priv->remainder, priv->remainder_size);
-		free(priv->remainder);
-		priv->remainder = NULL;
-		priv->remainder_size = 0;
+	if (priv->remainder.len) {
+		consume_one(priv, priv->remainder.buf, priv->remainder.len);
+		strbuf_reset(&priv->remainder);
 	}
 	return 0;
 }
@@ -148,7 +134,9 @@ int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
 	int ret;
 	xecb->outf = xdiff_outf;
 	xecb->priv = state;
+	strbuf_init(&state->remainder, 0);
 	ret = xdl_diff(mf1, mf2, xpp, xecfg, xecb);
+	strbuf_release(&state->remainder);
 	return ret;
 }
 
diff --git a/xdiff-interface.h b/xdiff-interface.h
index 6f3b361..f6a1ec2 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -2,6 +2,7 @@
 #define XDIFF_INTERFACE_H
 
 #include "xdiff/xdiff.h"
+#include "strbuf.h"
 
 struct xdiff_emit_state;
 
@@ -9,8 +10,7 @@ typedef void (*xdiff_emit_consume_fn)(void *, char *, unsigned long);
 
 struct xdiff_emit_state {
 	xdiff_emit_consume_fn consume;
-	char *remainder;
-	unsigned long remainder_size;
+	struct strbuf remainder;
 };
 
 int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);
-- 
1.5.6.1

Re: [PATCHv2 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs

[Brian Downing]

On Thu, Aug 14, 2008 at 12:13:21AM -0500, Brian Downing wrote:
> @@ -141,6 +141,17 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co
>  	return xdl_diff(&a, &b, xpp, xecfg, xecb);
>  }
>  
> +int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
> +		  struct xdiff_emit_state *state, xpparam_t const *xpp,
> +		  xdemitconf_t const *xecfg, xdemitcb_t *xecb)
> +{
> +	int ret;
> +	xecb->outf = xdiff_outf;
> +	xecb->priv = state;
> +	ret = xdl_diff(mf1, mf2, xpp, xecfg, xecb);

Sorry, this was wrong.  It should call xdi_diff.  (Unfortunately, the
tests passed anyway.)  I'll resubmit.

> +	return ret;
> +}
> +
>  int read_mmfile(mmfile_t *ptr, const char *filename)
>  {
>  	struct stat st;

-bcd

[PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs

[Brian Downing]

To prepare for the need to initialize and release resources for an
xdi_diff with the xdiff_outf output function, make a new function to
wrap this usage.

Old:

	ecb.outf = xdiff_outf;
	ecb.priv = &state;
	...
	xdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);

New:

	xdi_diff_outf(file_p, file_o, &state.xm, &xpp, &xecfg, &ecb);

Signed-off-by: Brian Downing <bdowning@lavos.net>
---

    Let's try this instead; it's quite a bit cleaner than my last
    effort.  [Especially now that it doesn't skip the xdi_diff
    function.]

 builtin-blame.c   |    4 +---
 combine-diff.c    |    5 ++---
 diff.c            |   20 +++++---------------
 xdiff-interface.c |   13 ++++++++++++-
 xdiff-interface.h |    4 +++-
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index 4ea3431..8cca3b1 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -528,15 +528,13 @@ static struct patch *compare_buffer(mmfile_t *file_p, mmfile_t *file_o,
 	xpp.flags = xdl_opts;
 	memset(&xecfg, 0, sizeof(xecfg));
 	xecfg.ctxlen = context;
-	ecb.outf = xdiff_outf;
-	ecb.priv = &state;
 	memset(&state, 0, sizeof(state));
 	state.xm.consume = process_u_diff;
 	state.ret = xmalloc(sizeof(struct patch));
 	state.ret->chunks = NULL;
 	state.ret->num = 0;
 
-	xdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);
+	xdi_diff_outf(file_p, file_o, &state.xm, &xpp, &xecfg, &ecb);
 
 	if (state.ret->num) {
 		struct chunk *chunk;
diff --git a/combine-diff.c b/combine-diff.c
index 9f80a1c..72dd6d2 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -217,8 +217,6 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,
 	parent_file.size = sz;
 	xpp.flags = XDF_NEED_MINIMAL;
 	memset(&xecfg, 0, sizeof(xecfg));
-	ecb.outf = xdiff_outf;
-	ecb.priv = &state;
 	memset(&state, 0, sizeof(state));
 	state.xm.consume = consume_line;
 	state.nmask = nmask;
@@ -227,7 +225,8 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,
 	state.num_parent = num_parent;
 	state.n = n;
 
-	xdi_diff(&parent_file, result_file, &xpp, &xecfg, &ecb);
+	xdi_diff_outf(&parent_file, result_file,
+		      &state.xm, &xpp, &xecfg, &ecb);
 	free(parent_file.ptr);
 
 	/* Assign line numbers for this parent.
diff --git a/diff.c b/diff.c
index bf5d5f1..913a92f 100644
--- a/diff.c
+++ b/diff.c
@@ -459,10 +459,8 @@ static void diff_words_show(struct diff_words_data *diff_words)
 
 	xpp.flags = XDF_NEED_MINIMAL;
 	xecfg.ctxlen = diff_words->minus.alloc + diff_words->plus.alloc;
-	ecb.outf = xdiff_outf;
-	ecb.priv = diff_words;
 	diff_words->xm.consume = fn_out_diff_words_aux;
-	xdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);
+	xdi_diff_outf(&minus, &plus, &diff_words->xm, &xpp, &xecfg, &ecb);
 
 	free(minus.ptr);
 	free(plus.ptr);
@@ -1521,15 +1519,13 @@ static void builtin_diff(const char *name_a,
 			xecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);
 		else if (!prefixcmp(diffopts, "-u"))
 			xecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);
-		ecb.outf = xdiff_outf;
-		ecb.priv = &ecbdata;
 		ecbdata.xm.consume = fn_out_consume;
 		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS)) {
 			ecbdata.diff_words =
 				xcalloc(1, sizeof(struct diff_words_data));
 			ecbdata.diff_words->file = o->file;
 		}
-		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, &ecbdata.xm, &xpp, &xecfg, &ecb);
 		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))
 			free_diff_words_data(&ecbdata);
 	}
@@ -1580,9 +1576,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 
 		memset(&xecfg, 0, sizeof(xecfg));
 		xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
-		ecb.outf = xdiff_outf;
-		ecb.priv = diffstat;
-		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, &diffstat->xm, &xpp, &xecfg, &ecb);
 	}
 
  free_and_return:
@@ -1628,9 +1622,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
 
 		memset(&xecfg, 0, sizeof(xecfg));
 		xpp.flags = XDF_NEED_MINIMAL;
-		ecb.outf = xdiff_outf;
-		ecb.priv = &data;
-		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);
 
 		if ((data.ws_rule & WS_TRAILING_SPACE) &&
 		    data.trailing_blanks_start) {
@@ -3128,9 +3120,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 		xpp.flags = XDF_NEED_MINIMAL;
 		xecfg.ctxlen = 3;
 		xecfg.flags = XDL_EMIT_FUNCNAMES;
-		ecb.outf = xdiff_outf;
-		ecb.priv = &data;
-		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);
 	}
 
 	SHA1_Final(sha1, &ctx);
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 61dc5c5..828b496 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -61,7 +61,7 @@ static void consume_one(void *priv_, char *s, unsigned long size)
 	}
 }
 
-int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
+static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
 {
 	struct xdiff_emit_state *priv = priv_;
 	int i;
@@ -141,6 +141,17 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co
 	return xdl_diff(&a, &b, xpp, xecfg, xecb);
 }
 
+int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
+		  struct xdiff_emit_state *state, xpparam_t const *xpp,
+		  xdemitconf_t const *xecfg, xdemitcb_t *xecb)
+{
+	int ret;
+	xecb->outf = xdiff_outf;
+	xecb->priv = state;
+	ret = xdi_diff(mf1, mf2, xpp, xecfg, xecb);
+	return ret;
+}
+
 int read_mmfile(mmfile_t *ptr, const char *filename)
 {
 	struct stat st;
diff --git a/xdiff-interface.h b/xdiff-interface.h
index f7f791d..6f3b361 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -14,7 +14,9 @@ struct xdiff_emit_state {
 };
 
 int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);
-int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf);
+int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
+		  struct xdiff_emit_state *state, xpparam_t const *xpp,
+		  xdemitconf_t const *xecfg, xdemitcb_t *xecb);
 int parse_hunk_header(char *line, int len,
 		      int *ob, int *on,
 		      int *nb, int *nn);
-- 
1.5.6.1

[PATCHv3 2/2] Use strbuf for struct xdiff_emit_state's remainder

[Brian Downing]

Continually xreallocing and freeing the remainder member of struct
xdiff_emit_state was a noticeable performance hit.  Use a strbuf
instead.

This yields a decent performance improvement on "git blame" on certain
repositories.  For example, before this commit:

$ time git blame -M -C -C -p --incremental server.c >/dev/null
101.52user 0.17system 1:41.73elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+39561minor)pagefaults 0swaps

With this commit:

$ time git blame -M -C -C -p --incremental server.c >/dev/null
80.38user 0.30system 1:20.81elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+50979minor)pagefaults 0swaps

Signed-off-by: Brian Downing <bdowning@lavos.net>
---
 xdiff-interface.c |   32 ++++++++++----------------------
 xdiff-interface.h |    4 ++--
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/xdiff-interface.c b/xdiff-interface.c
index 828b496..bf98866 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -69,36 +69,22 @@ static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
 	for (i = 0; i < nbuf; i++) {
 		if (mb[i].ptr[mb[i].size-1] != '\n') {
 			/* Incomplete line */
-			priv->remainder = xrealloc(priv->remainder,
-						   priv->remainder_size +
-						   mb[i].size);
-			memcpy(priv->remainder + priv->remainder_size,
-			       mb[i].ptr, mb[i].size);
-			priv->remainder_size += mb[i].size;
+			strbuf_add(&priv->remainder, mb[i].ptr, mb[i].size);
 			continue;
 		}
 
 		/* we have a complete line */
-		if (!priv->remainder) {
+		if (!priv->remainder.len) {
 			consume_one(priv, mb[i].ptr, mb[i].size);
 			continue;
 		}
-		priv->remainder = xrealloc(priv->remainder,
-					   priv->remainder_size +
-					   mb[i].size);
-		memcpy(priv->remainder + priv->remainder_size,
-		       mb[i].ptr, mb[i].size);
-		consume_one(priv, priv->remainder,
-			    priv->remainder_size + mb[i].size);
-		free(priv->remainder);
-		priv->remainder = NULL;
-		priv->remainder_size = 0;
+		strbuf_add(&priv->remainder, mb[i].ptr, mb[i].size);
+		consume_one(priv, priv->remainder.buf, priv->remainder.len);
+		strbuf_reset(&priv->remainder);
 	}
-	if (priv->remainder) {
-		consume_one(priv, priv->remainder, priv->remainder_size);
-		free(priv->remainder);
-		priv->remainder = NULL;
-		priv->remainder_size = 0;
+	if (priv->remainder.len) {
+		consume_one(priv, priv->remainder.buf, priv->remainder.len);
+		strbuf_reset(&priv->remainder);
 	}
 	return 0;
 }
@@ -148,7 +134,9 @@ int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
 	int ret;
 	xecb->outf = xdiff_outf;
 	xecb->priv = state;
+	strbuf_init(&state->remainder, 0);
 	ret = xdi_diff(mf1, mf2, xpp, xecfg, xecb);
+	strbuf_release(&state->remainder);
 	return ret;
 }
 
diff --git a/xdiff-interface.h b/xdiff-interface.h
index 6f3b361..f6a1ec2 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -2,6 +2,7 @@
 #define XDIFF_INTERFACE_H
 
 #include "xdiff/xdiff.h"
+#include "strbuf.h"
 
 struct xdiff_emit_state;
 
@@ -9,8 +10,7 @@ typedef void (*xdiff_emit_consume_fn)(void *, char *, unsigned long);
 
 struct xdiff_emit_state {
 	xdiff_emit_consume_fn consume;
-	char *remainder;
-	unsigned long remainder_size;
+	struct strbuf remainder;
 };
 
 int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);
-- 
1.5.6.1

Re: [PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs

[Junio C Hamano]

Brian Downing <bdowning@lavos.net> writes:

>     Let's try this instead; it's quite a bit cleaner than my last
>     effort.  [Especially now that it doesn't skip the xdi_diff
>     function.]

Much nicer.  xdi_diff() is just a performance thing that only kicks in
when you are running -U0 diff, so it is unsurprising that you did not see
any test failures.

However,

 * xm.consume is always set to something by the caller but all the other
   fields in xm are initialized to zero --- perhaps xdi_diff_outf() can
   take the pointer to the consume function as its parameter?

 * the whole set-up still requires that the xdiff_emit_state structure is
   the first member of callback data structure, so making the third
   parameter to xdi_diff_outf() to "struct xdiff_emit_state" and casting
   it up to containing structure like your patch does does not offer any
   additional type safety.

I wonder if something like this on top of your patch would be worth doing
as a further clean-up.

-- >8 --
xdiff-interface: hide the whole "xdiff_emit_state" business from the caller

This further enhances xdi_diff_outf() interface so that it takes two
common parameters: the callback function that processes one line at a
time, and a pointer to its application specific callback data structure.
xdi_diff_outf() creates its own "xdiff_emit_state" structure and stashes
these two away inside it, which is used by the lowest level output
function in the xdiff_outf() callchain, consume_one(), to call back to the
application layer.  With this restructuring, we lift the requirement that
the caller supplied callback data structure embeds xdiff_emit_state
structure as its first member.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-blame.c   |    4 +---
 combine-diff.c    |    7 ++-----
 diff.c            |   27 ++++++++++-----------------
 xdiff-interface.c |   23 ++++++++++++++++++-----
 xdiff-interface.h |   11 ++---------
 5 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index 8cca3b1..e4d12de 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -465,7 +465,6 @@ struct patch {
 };
 
 struct blame_diff_state {
-	struct xdiff_emit_state xm;
 	struct patch *ret;
 	unsigned hunk_post_context;
 	unsigned hunk_in_pre_context : 1;
@@ -529,12 +528,11 @@ static struct patch *compare_buffer(mmfile_t *file_p, mmfile_t *file_o,
 	memset(&xecfg, 0, sizeof(xecfg));
 	xecfg.ctxlen = context;
 	memset(&state, 0, sizeof(state));
-	state.xm.consume = process_u_diff;
 	state.ret = xmalloc(sizeof(struct patch));
 	state.ret->chunks = NULL;
 	state.ret->num = 0;
 
-	xdi_diff_outf(file_p, file_o, &state.xm, &xpp, &xecfg, &ecb);
+	xdi_diff_outf(file_p, file_o, process_u_diff, &state, &xpp, &xecfg, &ecb);
 
 	if (state.ret->num) {
 		struct chunk *chunk;
diff --git a/combine-diff.c b/combine-diff.c
index 72dd6d2..31ec0c5 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -143,8 +143,6 @@ static void append_lost(struct sline *sline, int n, const char *line, int len)
 }
 
 struct combine_diff_state {
-	struct xdiff_emit_state xm;
-
 	unsigned int lno;
 	int ob, on, nb, nn;
 	unsigned long nmask;
@@ -218,15 +216,14 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,
 	xpp.flags = XDF_NEED_MINIMAL;
 	memset(&xecfg, 0, sizeof(xecfg));
 	memset(&state, 0, sizeof(state));
-	state.xm.consume = consume_line;
 	state.nmask = nmask;
 	state.sline = sline;
 	state.lno = 1;
 	state.num_parent = num_parent;
 	state.n = n;
 
-	xdi_diff_outf(&parent_file, result_file,
-		      &state.xm, &xpp, &xecfg, &ecb);
+	xdi_diff_outf(&parent_file, result_file, consume_line, &state,
+		      &xpp, &xecfg, &ecb);
 	free(parent_file.ptr);
 
 	/* Assign line numbers for this parent.
diff --git a/diff.c b/diff.c
index 913a92f..f18a4be 100644
--- a/diff.c
+++ b/diff.c
@@ -369,7 +369,6 @@ static void diff_words_append(char *line, unsigned long len,
 }
 
 struct diff_words_data {
-	struct xdiff_emit_state xm;
 	struct diff_words_buffer minus, plus;
 	FILE *file;
 };
@@ -459,9 +458,8 @@ static void diff_words_show(struct diff_words_data *diff_words)
 
 	xpp.flags = XDF_NEED_MINIMAL;
 	xecfg.ctxlen = diff_words->minus.alloc + diff_words->plus.alloc;
-	diff_words->xm.consume = fn_out_diff_words_aux;
-	xdi_diff_outf(&minus, &plus, &diff_words->xm, &xpp, &xecfg, &ecb);
-
+	xdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, &diff_words,
+		      &xpp, &xecfg, &ecb);
 	free(minus.ptr);
 	free(plus.ptr);
 	diff_words->minus.text.size = diff_words->plus.text.size = 0;
@@ -475,7 +473,6 @@ static void diff_words_show(struct diff_words_data *diff_words)
 typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
 
 struct emit_callback {
-	struct xdiff_emit_state xm;
 	int nparents, color_diff;
 	unsigned ws_rule;
 	sane_truncate_fn truncate;
@@ -706,8 +703,6 @@ static char *pprint_rename(const char *a, const char *b)
 }
 
 struct diffstat_t {
-	struct xdiff_emit_state xm;
-
 	int nr;
 	int alloc;
 	struct diffstat_file {
@@ -1129,7 +1124,6 @@ static void free_diffstat_info(struct diffstat_t *diffstat)
 }
 
 struct checkdiff_t {
-	struct xdiff_emit_state xm;
 	const char *filename;
 	int lineno;
 	struct diff_options *o;
@@ -1519,13 +1513,13 @@ static void builtin_diff(const char *name_a,
 			xecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);
 		else if (!prefixcmp(diffopts, "-u"))
 			xecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);
-		ecbdata.xm.consume = fn_out_consume;
 		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS)) {
 			ecbdata.diff_words =
 				xcalloc(1, sizeof(struct diff_words_data));
 			ecbdata.diff_words->file = o->file;
 		}
-		xdi_diff_outf(&mf1, &mf2, &ecbdata.xm, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,
+			      &xpp, &xecfg, &ecb);
 		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))
 			free_diff_words_data(&ecbdata);
 	}
@@ -1576,7 +1570,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 
 		memset(&xecfg, 0, sizeof(xecfg));
 		xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
-		xdi_diff_outf(&mf1, &mf2, &diffstat->xm, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
+			      &xpp, &xecfg, &ecb);
 	}
 
  free_and_return:
@@ -1597,7 +1592,6 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
 		return;
 
 	memset(&data, 0, sizeof(data));
-	data.xm.consume = checkdiff_consume;
 	data.filename = name_b ? name_b : name_a;
 	data.lineno = 0;
 	data.o = o;
@@ -1622,7 +1616,8 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
 
 		memset(&xecfg, 0, sizeof(xecfg));
 		xpp.flags = XDF_NEED_MINIMAL;
-		xdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,
+			      &xpp, &xecfg, &ecb);
 
 		if ((data.ws_rule & WS_TRAILING_SPACE) &&
 		    data.trailing_blanks_start) {
@@ -3010,7 +3005,6 @@ static void diff_summary(FILE *file, struct diff_filepair *p)
 }
 
 struct patch_id_t {
-	struct xdiff_emit_state xm;
 	SHA_CTX *ctx;
 	int patchlen;
 };
@@ -3055,7 +3049,6 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 	SHA1_Init(&ctx);
 	memset(&data, 0, sizeof(struct patch_id_t));
 	data.ctx = &ctx;
-	data.xm.consume = patch_id_consume;
 
 	for (i = 0; i < q->nr; i++) {
 		xpparam_t xpp;
@@ -3120,7 +3113,8 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 		xpp.flags = XDF_NEED_MINIMAL;
 		xecfg.ctxlen = 3;
 		xecfg.flags = XDL_EMIT_FUNCNAMES;
-		xdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);
+		xdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,
+			      &xpp, &xecfg, &ecb);
 	}
 
 	SHA1_Final(sha1, &ctx);
@@ -3197,7 +3191,6 @@ void diff_flush(struct diff_options *options)
 		struct diffstat_t diffstat;
 
 		memset(&diffstat, 0, sizeof(struct diffstat_t));
-		diffstat.xm.consume = diffstat_consume;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))
diff --git a/xdiff-interface.c b/xdiff-interface.c
index bf98866..944ad98 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -1,5 +1,12 @@
 #include "cache.h"
 #include "xdiff-interface.h"
+#include "strbuf.h"
+
+struct xdiff_emit_state {
+	xdiff_emit_consume_fn consume;
+	void *consume_callback_data;
+	struct strbuf remainder;
+};
 
 static int parse_num(char **cp_p, int *num_p)
 {
@@ -55,7 +62,7 @@ static void consume_one(void *priv_, char *s, unsigned long size)
 		unsigned long this_size;
 		ep = memchr(s, '\n', size);
 		this_size = (ep == NULL) ? size : (ep - s + 1);
-		priv->consume(priv, s, this_size);
+		priv->consume(priv->consume_callback_data, s, this_size);
 		size -= this_size;
 		s += this_size;
 	}
@@ -128,15 +135,21 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co
 }
 
 int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
-		  struct xdiff_emit_state *state, xpparam_t const *xpp,
+		  xdiff_emit_consume_fn fn, void *consume_callback_data,
+		  xpparam_t const *xpp,
 		  xdemitconf_t const *xecfg, xdemitcb_t *xecb)
 {
 	int ret;
+	struct xdiff_emit_state state;
+
+	memset(&state, 0, sizeof(state));
+	state.consume = fn;
+	state.consume_callback_data = consume_callback_data;
 	xecb->outf = xdiff_outf;
-	xecb->priv = state;
-	strbuf_init(&state->remainder, 0);
+	xecb->priv = &state;
+	strbuf_init(&state.remainder, 0);
 	ret = xdi_diff(mf1, mf2, xpp, xecfg, xecb);
-	strbuf_release(&state->remainder);
+	strbuf_release(&state.remainder);
 	return ret;
 }
 
diff --git a/xdiff-interface.h b/xdiff-interface.h
index f6a1ec2..558492b 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -2,20 +2,13 @@
 #define XDIFF_INTERFACE_H
 
 #include "xdiff/xdiff.h"
-#include "strbuf.h"
-
-struct xdiff_emit_state;
 
 typedef void (*xdiff_emit_consume_fn)(void *, char *, unsigned long);
 
-struct xdiff_emit_state {
-	xdiff_emit_consume_fn consume;
-	struct strbuf remainder;
-};
-
 int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);
 int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
-		  struct xdiff_emit_state *state, xpparam_t const *xpp,
+		  xdiff_emit_consume_fn fn, void *consume_callback_data,
+		  xpparam_t const *xpp,
 		  xdemitconf_t const *xecfg, xdemitcb_t *xecb);
 int parse_hunk_header(char *line, int len,
 		      int *ob, int *on,

Re: [PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs

[Brian Downing]

On Wed, Aug 13, 2008 at 11:18:22PM -0700, Junio C Hamano wrote:
> I wonder if something like this on top of your patch would be worth doing
> as a further clean-up.
> 
> -- >8 --
> xdiff-interface: hide the whole "xdiff_emit_state" business from the caller

Nice, looks good to me.

-bcd

Re: [PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs

[Brian Downing]

On Wed, Aug 13, 2008 at 11:18:22PM -0700, Junio C Hamano wrote:
> Much nicer.  xdi_diff() is just a performance thing that only kicks in
> when you are running -U0 diff, so it is unsurprising that you did not see
> any test failures.

Interesting point here.  In playing with trying to cache the diff hashes
to speed up blame, I had to basically disable the xdi_diff tail trimming
when building the hash the first time, because it needed to see the
whole file.  In doing this, I discovered that just changing from
xdi_diff to xdl_diff /does/ change the blame -M -C -C --incremental
result for my test case.  (Unfortunately, my test case is proprietary
code...)

Is this expected, or some kind of serious bug with xdi_diff?

    :; diff proper-output other-output
    980c980
    < dee86dd25736e1778122cfde7d7455a3ef85e37d 173 173 2
    ---
    > dee86dd25736e1778122cfde7d7455a3ef85e37d 172 172 3
    982c982
    < dee86dd25736e1778122cfde7d7455a3ef85e37d 183 183 2
    ---
    > dee86dd25736e1778122cfde7d7455a3ef85e37d 184 184 1
    1509c1509
    < c6966941ebfaa1dc9b29489e53d6d7f41e52d357 287 384 1
    ---
    > c6966941ebfaa1dc9b29489e53d6d7f41e52d357 284 381 1
    1511c1511
    < c6966941ebfaa1dc9b29489e53d6d7f41e52d357 301 399 2
    ---
    > c6966941ebfaa1dc9b29489e53d6d7f41e52d357 286 383 2
    1513c1513
    < c6966941ebfaa1dc9b29489e53d6d7f41e52d357 304 402 1
    ---
    > c6966941ebfaa1dc9b29489e53d6d7f41e52d357 301 399 1
    1608c1608
    < ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 252 381 1
    ---
    > ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 252 385 1
    1610c1610
    < ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 255 383 1
    ---
    > ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 255 400 1
    1612c1612
    < ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 257 385 1
    ---
    > ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 257 402 1
    1945c1945
    < a325ab86914b15107bf0211550c7d0568fb0854c 138 172 1
    ---
    > a325ab86914b15107bf0211550c7d0568fb0854c 138 183 1

-bcd

Re: [PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs

[Junio C Hamano]

bdowning@lavos.net (Brian Downing) writes:

> On Wed, Aug 13, 2008 at 11:18:22PM -0700, Junio C Hamano wrote:
>> Much nicer.  xdi_diff() is just a performance thing that only kicks in
>> when you are running -U0 diff, so it is unsurprising that you did not see
>> any test failures.
>
> Interesting point here.  In playing with trying to cache the diff hashes
> to speed up blame, I had to basically disable the xdi_diff tail trimming
> when building the hash the first time, because it needed to see the
> whole file.  In doing this, I discovered that just changing from
> xdi_diff to xdl_diff /does/ change the blame -M -C -C --incremental
> result for my test case.  (Unfortunately, my test case is proprietary
> code...)

Is the reason why you mention "incremental" specifically because you only
tested incremental, or you get identical result in non-incremental mode?

If your material is repetitive, say you have lines "A A A B C A A A" in
the parent blob and "A A A B A A A" in the child blob, and you are trying
to pass blame on three line block "A A A" at the beginning of the child,
we can pass blame to the three lines at the beginning part, or to the end
part, without Linus's common tail trimming optimization.  But there is no
way it can match the end part with the optimization.

You cannot say one result is more correct than the other --- both are
equally correct.  Of course, you could argue that with such a highly
repetitive material, it may be better to match closer ones, but it's a
judgement call.

Re: [PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs

[Brian Downing]

On Wed, Aug 20, 2008 at 10:24:22PM -0700, Junio C Hamano wrote:
> Is the reason why you mention "incremental" specifically because you only
> tested incremental, or you get identical result in non-incremental mode?

I've only tested incremental at this point.

> If your material is repetitive, say you have lines "A A A B C A A A" in
> the parent blob and "A A A B A A A" in the child blob, and you are trying
> to pass blame on three line block "A A A" at the beginning of the child,
> we can pass blame to the three lines at the beginning part, or to the end
> part, without Linus's common tail trimming optimization.  But there is no
> way it can match the end part with the optimization.
> 
> You cannot say one result is more correct than the other --- both are
> equally correct.  Of course, you could argue that with such a highly
> repetitive material, it may be better to match closer ones, but it's a
> judgement call.

Okay, that makes sense.  Thanks.

-bcd