{"thread":{"id":"14972","subject":"[PATCH 1/2] Make xdiff_outf_{init,release} interface","startedAt":"2008-08-13T07:05:09Z","lastAt":"2008-08-21T06:29:14Z","messageCount":14,"participants":["Brian Downing","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"87015","messageId":"20080813070508.GB4396@lavos.net","threadId":"14972","inReplyTo":null,"subject":"[PATCH 1/2] Make xdiff_outf_{init,release} interface","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2008-08-13T07:05:09Z","receivedAt":"2008-08-13T07:05:09Z","isPatch":true,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"To prepare for the need to release resources when we're done with\nxdiff_outf, make a new function pair to initialize and release a\nstruct xdiff_emit_state.  Since we can roll the two lines always\nrequired to use xdiff_outf into one, the net number of lines at the call\nsite remains constant.\n\nOld:\n\n\tecb.outf = xdiff_outf;\n\tecb.priv = &state;\n\t...\n\txdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);\n\nNew:\n\n\txdiff_outf_init(&ecb, &state);\n\t...\n\txdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);\n\txdiff_outf_release(&state);\n\nSigned-off-by: Brian Downing <bdowning@lavos.net>\n---\n builtin-blame.c   |    4 ++--\n combine-diff.c    |    4 ++--\n diff.c            |   20 ++++++++++----------\n xdiff-interface.c |   11 +++++++++++\n xdiff-interface.h |    2 ++\n 5 files changed, 27 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 4ea3431..b52eff4 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -528,15 +528,15 @@ static struct patch *compare_buffer(mmfile_t *file_p, mmfile_t *file_o,\n \txpp.flags = xdl_opts;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = context;\n-\tecb.outf = xdiff_outf;\n-\tecb.priv = &state;\n \tmemset(&state, 0, sizeof(state));\n+\txdiff_outf_init(&ecb, &state);\n \tstate.xm.consume = process_u_diff;\n \tstate.ret = xmalloc(sizeof(struct patch));\n \tstate.ret->chunks = NULL;\n \tstate.ret->num = 0;\n \n \txdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);\n+\txdiff_outf_release(&state);\n \n \tif (state.ret->num) {\n \t\tstruct chunk *chunk;\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 9f80a1c..887c315 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -217,9 +217,8 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n \tparent_file.size = sz;\n \txpp.flags = XDF_NEED_MINIMAL;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n-\tecb.outf = xdiff_outf;\n-\tecb.priv = &state;\n \tmemset(&state, 0, sizeof(state));\n+\txdiff_outf_init(&ecb, &state);\n \tstate.xm.consume = consume_line;\n \tstate.nmask = nmask;\n \tstate.sline = sline;\n@@ -228,6 +227,7 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n \tstate.n = n;\n \n \txdi_diff(&parent_file, result_file, &xpp, &xecfg, &ecb);\n+\txdiff_outf_release(&state);\n \tfree(parent_file.ptr);\n \n \t/* Assign line numbers for this parent.\ndiff --git a/diff.c b/diff.c\nindex 6954f99..222646d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -459,10 +459,10 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \n \txpp.flags = XDF_NEED_MINIMAL;\n \txecfg.ctxlen = diff_words->minus.alloc + diff_words->plus.alloc;\n-\tecb.outf = xdiff_outf;\n-\tecb.priv = diff_words;\n+\txdiff_outf_init(&ecb, diff_words);\n \tdiff_words->xm.consume = fn_out_diff_words_aux;\n \txdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);\n+\txdiff_outf_release(diff_words);\n \n \tfree(minus.ptr);\n \tfree(plus.ptr);\n@@ -1520,8 +1520,7 @@ static void builtin_diff(const char *name_a,\n \t\t\txecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);\n \t\telse if (!prefixcmp(diffopts, \"-u\"))\n \t\t\txecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = &ecbdata;\n+\t\txdiff_outf_init(&ecb, &ecbdata);\n \t\tecbdata.xm.consume = fn_out_consume;\n \t\tif (DIFF_OPT_TST(o, COLOR_DIFF_WORDS)) {\n \t\t\tecbdata.diff_words =\n@@ -1529,6 +1528,7 @@ static void builtin_diff(const char *name_a,\n \t\t\tecbdata.diff_words->file = o->file;\n \t\t}\n \t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdiff_outf_release(&ecbdata);\n \t\tif (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))\n \t\t\tfree_diff_words_data(&ecbdata);\n \t}\n@@ -1579,9 +1579,9 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = diffstat;\n+\t\txdiff_outf_init(&ecb, diffstat);\n \t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdiff_outf_release(diffstat);\n \t}\n \n  free_and_return:\n@@ -1627,9 +1627,9 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = XDF_NEED_MINIMAL;\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = &data;\n+\t\txdiff_outf_init(&ecb, &data);\n \t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdiff_outf_release(&data);\n \n \t\tif ((data.ws_rule & WS_TRAILING_SPACE) &&\n \t\t    data.trailing_blanks_start) {\n@@ -3127,9 +3127,9 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \t\txpp.flags = XDF_NEED_MINIMAL;\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = &data;\n+\t\txdiff_outf_init(&ecb, &data);\n \t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdiff_outf_release(&data);\n \t}\n \n \tSHA1_Final(sha1, &ctx);\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 61dc5c5..9c5e277 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -61,6 +61,13 @@ static void consume_one(void *priv_, char *s, unsigned long size)\n \t}\n }\n \n+void xdiff_outf_init(xdemitcb_t *ecb, void *priv_)\n+{\n+\tstruct xdiff_emit_state *priv = priv_;\n+\tecb->outf = xdiff_outf;\n+\tecb->priv = priv;\n+}\n+\n int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n {\n \tstruct xdiff_emit_state *priv = priv_;\n@@ -103,6 +110,10 @@ int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n \treturn 0;\n }\n \n+void xdiff_outf_release(void *priv_)\n+{\n+}\n+\n /*\n  * Trim down common substring at the end of the buffers,\n  * but leave at least ctx lines at the end.\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex f7f791d..fca6200 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -14,7 +14,9 @@ struct xdiff_emit_state {\n };\n \n int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);\n+void xdiff_outf_init(xdemitcb_t *ecb, void *priv_);\n int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf);\n+void xdiff_outf_release(void *priv_);\n int parse_hunk_header(char *line, int len,\n \t\t      int *ob, int *on,\n \t\t      int *nb, int *nn);\n-- \n1.5.6.1\n"},{"id":"87130","messageId":"7vljz0iftm.fsf@gitster.siamese.dyndns.org","threadId":"14972","inReplyTo":"20080813070508.GB4396@lavos.net","subject":"Re: [PATCH 1/2] Make xdiff_outf_{init,release} interface","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-14T00:46:29Z","receivedAt":"2008-08-14T00:46:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Downing <bdowning@lavos.net> writes:\n\n> @@ -103,6 +110,10 @@ int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n>  \treturn 0;\n>  }\n>  \n> +void xdiff_outf_release(void *priv_)\n> +{\n> +}\n> +\n\nIt might make it more clear to have this function take a pointer to\n\"struct xdiff_emit_state\", which is always the first member of the\ncallback private data structure.\n\nAlthough I wish xdi_diff() could do the necessary clean-up immediately\nbefore it returns (so that the caller did not have to do anything\nspecial), it is not possible to do so cleanly, because there are \"outf\"\nimplementations other than xdiff_outf that do not even use \"struct\nxdiff_emit_state\" in their callbacks.  So I think your patch makes sense.\n\nThanks.\n"},{"id":"87139","messageId":"20080814020614.GD4396@lavos.net","threadId":"14972","inReplyTo":"7vljz0iftm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Make xdiff_outf_{init,release} interface","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2008-08-14T02:06:14Z","receivedAt":"2008-08-14T02:06:14Z","isPatch":true,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"On Wed, Aug 13, 2008 at 05:46:29PM -0700, Junio C Hamano wrote:\n> Brian Downing <bdowning@lavos.net> writes:\n> > @@ -103,6 +110,10 @@ int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n> >  \treturn 0;\n> >  }\n> >  \n> > +void xdiff_outf_release(void *priv_)\n> > +{\n> > +}\n> > +\n> \n> It might make it more clear to have this function take a pointer to\n> \"struct xdiff_emit_state\", which is always the first member of the\n> callback private data structure.\n\nThat makes the call sites (slightly) more complicated, in that instead\nof:\n\txdiff_outf_release(&state);\nyou'd want:\n\txdiff_outf_release(&state.xm);\n\nThat was not the typical usage before, in that it said \"ecb.priv =\n&state\" rather than \"ecb.priv = &state.xm\", and I used the void *\nargument to mirror that, but I can change it if it'd be preferable.\n \n> Although I wish xdi_diff() could do the necessary clean-up immediately\n> before it returns (so that the caller did not have to do anything\n> special), it is not possible to do so cleanly, because there are\n> \"outf\" implementations other than xdiff_outf that do not even use\n> \"struct xdiff_emit_state\" in their callbacks.  So I think your patch\n> makes sense.\n\nWell, I could do something like:\n\n\tif (xecb->outf == xdiff_outf)\n\t\t/* xdiff_outf cleanup */\n\nat the end of xdi_diff, but that's... kind of horrible I think.\n\nFor that matter, I could just make an xdi_outf_diff function that would\ntake the state in addition to the other xdi_diff arguments and go ahead\nand set it up, do the diff, and tear it down in one step.  Maybe that\nwould be better if it works for everywhere this style of diff needs to\nbe called.\n\nAnother question:  should I go ahead and make xdiff_outf itself static?\n\n-bcd\n"},{"id":"87140","messageId":"7viqu4gx8c.fsf@gitster.siamese.dyndns.org","threadId":"14972","inReplyTo":"20080814020614.GD4396@lavos.net","subject":"Re: [PATCH 1/2] Make xdiff_outf_{init,release} interface","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-14T02:13:23Z","receivedAt":"2008-08-14T02:13:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"bdowning@lavos.net (Brian Downing) writes:\n\n>> Although I wish xdi_diff() could do the necessary clean-up immediately\n>> before it returns (so that the caller did not have to do anything\n>> special), it is not possible to do so cleanly, because there are\n>> \"outf\" implementations other than xdiff_outf that do not even use\n>> \"struct xdiff_emit_state\" in their callbacks.  So I think your patch\n>> makes sense.\n>\n> Well, I could do something like:\n>\n> \tif (xecb->outf == xdiff_outf)\n> \t\t/* xdiff_outf cleanup */\n>\n> at the end of xdi_diff, but that's... kind of horrible I think.\n\nYeah, that is ugly, and that is why I said I think your patch makes sense.\n\n> For that matter, I could just make an xdi_outf_diff function that would\n> take the state in addition to the other xdi_diff arguments and go ahead\n> and set it up, do the diff, and tear it down in one step.  Maybe that\n> would be better if it works for everywhere this style of diff needs to\n> be called.\n\nYeah, most of the xdi_diff() callers do use the stock outf so it would\nmake sense.\n"},{"id":"87147","messageId":"1218690802-30536-1-git-send-email-bdowning@lavos.net","threadId":"14972","inReplyTo":"7viqu4gx8c.fsf@gitster.siamese.dyndns.org","subject":"[PATCHv2 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2008-08-14T05:13:21Z","receivedAt":"2008-08-14T05:13:21Z","isPatch":false,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"To prepare for the need to initialize and release resources for an\nxdi_diff with the xdiff_outf output function, make a new function to\nwrap this usage.\n\nOld:\n\n\tecb.outf = xdiff_outf;\n\tecb.priv = &state;\n\t...\n\txdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);\n\nNew:\n\n\txdi_diff_outf(file_p, file_o, &state.xm, &xpp, &xecfg, &ecb);\n\nSigned-off-by: Brian Downing <bdowning@lavos.net>\n---\n\n    Let's try this instead; it's quite a bit cleaner than my last\n    effort.\n\n builtin-blame.c   |    4 +---\n combine-diff.c    |    5 ++---\n diff.c            |   20 +++++---------------\n xdiff-interface.c |   13 ++++++++++++-\n xdiff-interface.h |    4 +++-\n 5 files changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 4ea3431..8cca3b1 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -528,15 +528,13 @@ static struct patch *compare_buffer(mmfile_t *file_p, mmfile_t *file_o,\n \txpp.flags = xdl_opts;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = context;\n-\tecb.outf = xdiff_outf;\n-\tecb.priv = &state;\n \tmemset(&state, 0, sizeof(state));\n \tstate.xm.consume = process_u_diff;\n \tstate.ret = xmalloc(sizeof(struct patch));\n \tstate.ret->chunks = NULL;\n \tstate.ret->num = 0;\n \n-\txdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);\n+\txdi_diff_outf(file_p, file_o, &state.xm, &xpp, &xecfg, &ecb);\n \n \tif (state.ret->num) {\n \t\tstruct chunk *chunk;\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 9f80a1c..72dd6d2 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -217,8 +217,6 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n \tparent_file.size = sz;\n \txpp.flags = XDF_NEED_MINIMAL;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n-\tecb.outf = xdiff_outf;\n-\tecb.priv = &state;\n \tmemset(&state, 0, sizeof(state));\n \tstate.xm.consume = consume_line;\n \tstate.nmask = nmask;\n@@ -227,7 +225,8 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n \tstate.num_parent = num_parent;\n \tstate.n = n;\n \n-\txdi_diff(&parent_file, result_file, &xpp, &xecfg, &ecb);\n+\txdi_diff_outf(&parent_file, result_file,\n+\t\t      &state.xm, &xpp, &xecfg, &ecb);\n \tfree(parent_file.ptr);\n \n \t/* Assign line numbers for this parent.\ndiff --git a/diff.c b/diff.c\nindex bf5d5f1..913a92f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -459,10 +459,8 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \n \txpp.flags = XDF_NEED_MINIMAL;\n \txecfg.ctxlen = diff_words->minus.alloc + diff_words->plus.alloc;\n-\tecb.outf = xdiff_outf;\n-\tecb.priv = diff_words;\n \tdiff_words->xm.consume = fn_out_diff_words_aux;\n-\txdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);\n+\txdi_diff_outf(&minus, &plus, &diff_words->xm, &xpp, &xecfg, &ecb);\n \n \tfree(minus.ptr);\n \tfree(plus.ptr);\n@@ -1521,15 +1519,13 @@ static void builtin_diff(const char *name_a,\n \t\t\txecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);\n \t\telse if (!prefixcmp(diffopts, \"-u\"))\n \t\t\txecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = &ecbdata;\n \t\tecbdata.xm.consume = fn_out_consume;\n \t\tif (DIFF_OPT_TST(o, COLOR_DIFF_WORDS)) {\n \t\t\tecbdata.diff_words =\n \t\t\t\txcalloc(1, sizeof(struct diff_words_data));\n \t\t\tecbdata.diff_words->file = o->file;\n \t\t}\n-\t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, &ecbdata.xm, &xpp, &xecfg, &ecb);\n \t\tif (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))\n \t\t\tfree_diff_words_data(&ecbdata);\n \t}\n@@ -1580,9 +1576,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = diffstat;\n-\t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, &diffstat->xm, &xpp, &xecfg, &ecb);\n \t}\n \n  free_and_return:\n@@ -1628,9 +1622,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = XDF_NEED_MINIMAL;\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = &data;\n-\t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);\n \n \t\tif ((data.ws_rule & WS_TRAILING_SPACE) &&\n \t\t    data.trailing_blanks_start) {\n@@ -3128,9 +3120,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \t\txpp.flags = XDF_NEED_MINIMAL;\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = &data;\n-\t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);\n \t}\n \n \tSHA1_Final(sha1, &ctx);\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 61dc5c5..be448a0 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -61,7 +61,7 @@ static void consume_one(void *priv_, char *s, unsigned long size)\n \t}\n }\n \n-int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n+static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n {\n \tstruct xdiff_emit_state *priv = priv_;\n \tint i;\n@@ -141,6 +141,17 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co\n \treturn xdl_diff(&a, &b, xpp, xecfg, xecb);\n }\n \n+int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n+\t\t  struct xdiff_emit_state *state, xpparam_t const *xpp,\n+\t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb)\n+{\n+\tint ret;\n+\txecb->outf = xdiff_outf;\n+\txecb->priv = state;\n+\tret = xdl_diff(mf1, mf2, xpp, xecfg, xecb);\n+\treturn ret;\n+}\n+\n int read_mmfile(mmfile_t *ptr, const char *filename)\n {\n \tstruct stat st;\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex f7f791d..6f3b361 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -14,7 +14,9 @@ struct xdiff_emit_state {\n };\n \n int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);\n-int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf);\n+int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n+\t\t  struct xdiff_emit_state *state, xpparam_t const *xpp,\n+\t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb);\n int parse_hunk_header(char *line, int len,\n \t\t      int *ob, int *on,\n \t\t      int *nb, int *nn);\n-- \n1.5.6.1\n"},{"id":"87148","messageId":"1218690802-30536-2-git-send-email-bdowning@lavos.net","threadId":"14972","inReplyTo":"1218690802-30536-1-git-send-email-bdowning@lavos.net","subject":"[PATCHv2 2/2] Use strbuf for struct xdiff_emit_state's remainder","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2008-08-14T05:13:22Z","receivedAt":"2008-08-14T05:13:22Z","isPatch":false,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"Continually xreallocing and freeing the remainder member of struct\nxdiff_emit_state was a noticeable performance hit.  Use a strbuf\ninstead.\n\nThis yields a decent performance improvement on \"git blame\" on certain\nrepositories.  For example, before this commit:\n\n$ time git blame -M -C -C -p --incremental server.c >/dev/null\n101.52user 0.17system 1:41.73elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+39561minor)pagefaults 0swaps\n\nWith this commit:\n\n$ time git blame -M -C -C -p --incremental server.c >/dev/null\n80.38user 0.30system 1:20.81elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+50979minor)pagefaults 0swaps\n\nSigned-off-by: Brian Downing <bdowning@lavos.net>\n---\n xdiff-interface.c |   32 ++++++++++----------------------\n xdiff-interface.h |    4 ++--\n 2 files changed, 12 insertions(+), 24 deletions(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex be448a0..c999469 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -69,36 +69,22 @@ static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n \tfor (i = 0; i < nbuf; i++) {\n \t\tif (mb[i].ptr[mb[i].size-1] != '\\n') {\n \t\t\t/* Incomplete line */\n-\t\t\tpriv->remainder = xrealloc(priv->remainder,\n-\t\t\t\t\t\t   priv->remainder_size +\n-\t\t\t\t\t\t   mb[i].size);\n-\t\t\tmemcpy(priv->remainder + priv->remainder_size,\n-\t\t\t       mb[i].ptr, mb[i].size);\n-\t\t\tpriv->remainder_size += mb[i].size;\n+\t\t\tstrbuf_add(&priv->remainder, mb[i].ptr, mb[i].size);\n \t\t\tcontinue;\n \t\t}\n \n \t\t/* we have a complete line */\n-\t\tif (!priv->remainder) {\n+\t\tif (!priv->remainder.len) {\n \t\t\tconsume_one(priv, mb[i].ptr, mb[i].size);\n \t\t\tcontinue;\n \t\t}\n-\t\tpriv->remainder = xrealloc(priv->remainder,\n-\t\t\t\t\t   priv->remainder_size +\n-\t\t\t\t\t   mb[i].size);\n-\t\tmemcpy(priv->remainder + priv->remainder_size,\n-\t\t       mb[i].ptr, mb[i].size);\n-\t\tconsume_one(priv, priv->remainder,\n-\t\t\t    priv->remainder_size + mb[i].size);\n-\t\tfree(priv->remainder);\n-\t\tpriv->remainder = NULL;\n-\t\tpriv->remainder_size = 0;\n+\t\tstrbuf_add(&priv->remainder, mb[i].ptr, mb[i].size);\n+\t\tconsume_one(priv, priv->remainder.buf, priv->remainder.len);\n+\t\tstrbuf_reset(&priv->remainder);\n \t}\n-\tif (priv->remainder) {\n-\t\tconsume_one(priv, priv->remainder, priv->remainder_size);\n-\t\tfree(priv->remainder);\n-\t\tpriv->remainder = NULL;\n-\t\tpriv->remainder_size = 0;\n+\tif (priv->remainder.len) {\n+\t\tconsume_one(priv, priv->remainder.buf, priv->remainder.len);\n+\t\tstrbuf_reset(&priv->remainder);\n \t}\n \treturn 0;\n }\n@@ -148,7 +134,9 @@ int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n \tint ret;\n \txecb->outf = xdiff_outf;\n \txecb->priv = state;\n+\tstrbuf_init(&state->remainder, 0);\n \tret = xdl_diff(mf1, mf2, xpp, xecfg, xecb);\n+\tstrbuf_release(&state->remainder);\n \treturn ret;\n }\n \ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex 6f3b361..f6a1ec2 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -2,6 +2,7 @@\n #define XDIFF_INTERFACE_H\n \n #include \"xdiff/xdiff.h\"\n+#include \"strbuf.h\"\n \n struct xdiff_emit_state;\n \n@@ -9,8 +10,7 @@ typedef void (*xdiff_emit_consume_fn)(void *, char *, unsigned long);\n \n struct xdiff_emit_state {\n \txdiff_emit_consume_fn consume;\n-\tchar *remainder;\n-\tunsigned long remainder_size;\n+\tstruct strbuf remainder;\n };\n \n int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);\n-- \n1.5.6.1\n"},{"id":"87150","messageId":"20080814053156.GE4396@lavos.net","threadId":"14972","inReplyTo":"1218690802-30536-1-git-send-email-bdowning@lavos.net","subject":"Re: [PATCHv2 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2008-08-14T05:31:58Z","receivedAt":"2008-08-14T05:31:58Z","isPatch":false,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"On Thu, Aug 14, 2008 at 12:13:21AM -0500, Brian Downing wrote:\n> @@ -141,6 +141,17 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co\n>  \treturn xdl_diff(&a, &b, xpp, xecfg, xecb);\n>  }\n>  \n> +int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n> +\t\t  struct xdiff_emit_state *state, xpparam_t const *xpp,\n> +\t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb)\n> +{\n> +\tint ret;\n> +\txecb->outf = xdiff_outf;\n> +\txecb->priv = state;\n> +\tret = xdl_diff(mf1, mf2, xpp, xecfg, xecb);\n\nSorry, this was wrong.  It should call xdi_diff.  (Unfortunately, the\ntests passed anyway.)  I'll resubmit.\n\n> +\treturn ret;\n> +}\n> +\n>  int read_mmfile(mmfile_t *ptr, const char *filename)\n>  {\n>  \tstruct stat st;\n\n-bcd\n"},{"id":"87151","messageId":"1218692211-26045-1-git-send-email-bdowning@lavos.net","threadId":"14972","inReplyTo":"20080814053156.GE4396@lavos.net","subject":"[PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2008-08-14T05:36:50Z","receivedAt":"2008-08-14T05:36:50Z","isPatch":false,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"To prepare for the need to initialize and release resources for an\nxdi_diff with the xdiff_outf output function, make a new function to\nwrap this usage.\n\nOld:\n\n\tecb.outf = xdiff_outf;\n\tecb.priv = &state;\n\t...\n\txdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);\n\nNew:\n\n\txdi_diff_outf(file_p, file_o, &state.xm, &xpp, &xecfg, &ecb);\n\nSigned-off-by: Brian Downing <bdowning@lavos.net>\n---\n\n    Let's try this instead; it's quite a bit cleaner than my last\n    effort.  [Especially now that it doesn't skip the xdi_diff\n    function.]\n\n builtin-blame.c   |    4 +---\n combine-diff.c    |    5 ++---\n diff.c            |   20 +++++---------------\n xdiff-interface.c |   13 ++++++++++++-\n xdiff-interface.h |    4 +++-\n 5 files changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 4ea3431..8cca3b1 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -528,15 +528,13 @@ static struct patch *compare_buffer(mmfile_t *file_p, mmfile_t *file_o,\n \txpp.flags = xdl_opts;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = context;\n-\tecb.outf = xdiff_outf;\n-\tecb.priv = &state;\n \tmemset(&state, 0, sizeof(state));\n \tstate.xm.consume = process_u_diff;\n \tstate.ret = xmalloc(sizeof(struct patch));\n \tstate.ret->chunks = NULL;\n \tstate.ret->num = 0;\n \n-\txdi_diff(file_p, file_o, &xpp, &xecfg, &ecb);\n+\txdi_diff_outf(file_p, file_o, &state.xm, &xpp, &xecfg, &ecb);\n \n \tif (state.ret->num) {\n \t\tstruct chunk *chunk;\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 9f80a1c..72dd6d2 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -217,8 +217,6 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n \tparent_file.size = sz;\n \txpp.flags = XDF_NEED_MINIMAL;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n-\tecb.outf = xdiff_outf;\n-\tecb.priv = &state;\n \tmemset(&state, 0, sizeof(state));\n \tstate.xm.consume = consume_line;\n \tstate.nmask = nmask;\n@@ -227,7 +225,8 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n \tstate.num_parent = num_parent;\n \tstate.n = n;\n \n-\txdi_diff(&parent_file, result_file, &xpp, &xecfg, &ecb);\n+\txdi_diff_outf(&parent_file, result_file,\n+\t\t      &state.xm, &xpp, &xecfg, &ecb);\n \tfree(parent_file.ptr);\n \n \t/* Assign line numbers for this parent.\ndiff --git a/diff.c b/diff.c\nindex bf5d5f1..913a92f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -459,10 +459,8 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \n \txpp.flags = XDF_NEED_MINIMAL;\n \txecfg.ctxlen = diff_words->minus.alloc + diff_words->plus.alloc;\n-\tecb.outf = xdiff_outf;\n-\tecb.priv = diff_words;\n \tdiff_words->xm.consume = fn_out_diff_words_aux;\n-\txdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);\n+\txdi_diff_outf(&minus, &plus, &diff_words->xm, &xpp, &xecfg, &ecb);\n \n \tfree(minus.ptr);\n \tfree(plus.ptr);\n@@ -1521,15 +1519,13 @@ static void builtin_diff(const char *name_a,\n \t\t\txecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);\n \t\telse if (!prefixcmp(diffopts, \"-u\"))\n \t\t\txecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = &ecbdata;\n \t\tecbdata.xm.consume = fn_out_consume;\n \t\tif (DIFF_OPT_TST(o, COLOR_DIFF_WORDS)) {\n \t\t\tecbdata.diff_words =\n \t\t\t\txcalloc(1, sizeof(struct diff_words_data));\n \t\t\tecbdata.diff_words->file = o->file;\n \t\t}\n-\t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, &ecbdata.xm, &xpp, &xecfg, &ecb);\n \t\tif (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))\n \t\t\tfree_diff_words_data(&ecbdata);\n \t}\n@@ -1580,9 +1576,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = diffstat;\n-\t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, &diffstat->xm, &xpp, &xecfg, &ecb);\n \t}\n \n  free_and_return:\n@@ -1628,9 +1622,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = XDF_NEED_MINIMAL;\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = &data;\n-\t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);\n \n \t\tif ((data.ws_rule & WS_TRAILING_SPACE) &&\n \t\t    data.trailing_blanks_start) {\n@@ -3128,9 +3120,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \t\txpp.flags = XDF_NEED_MINIMAL;\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n-\t\tecb.outf = xdiff_outf;\n-\t\tecb.priv = &data;\n-\t\txdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);\n \t}\n \n \tSHA1_Final(sha1, &ctx);\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 61dc5c5..828b496 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -61,7 +61,7 @@ static void consume_one(void *priv_, char *s, unsigned long size)\n \t}\n }\n \n-int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n+static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n {\n \tstruct xdiff_emit_state *priv = priv_;\n \tint i;\n@@ -141,6 +141,17 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co\n \treturn xdl_diff(&a, &b, xpp, xecfg, xecb);\n }\n \n+int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n+\t\t  struct xdiff_emit_state *state, xpparam_t const *xpp,\n+\t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb)\n+{\n+\tint ret;\n+\txecb->outf = xdiff_outf;\n+\txecb->priv = state;\n+\tret = xdi_diff(mf1, mf2, xpp, xecfg, xecb);\n+\treturn ret;\n+}\n+\n int read_mmfile(mmfile_t *ptr, const char *filename)\n {\n \tstruct stat st;\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex f7f791d..6f3b361 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -14,7 +14,9 @@ struct xdiff_emit_state {\n };\n \n int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);\n-int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf);\n+int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n+\t\t  struct xdiff_emit_state *state, xpparam_t const *xpp,\n+\t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb);\n int parse_hunk_header(char *line, int len,\n \t\t      int *ob, int *on,\n \t\t      int *nb, int *nn);\n-- \n1.5.6.1\n"},{"id":"87152","messageId":"1218692211-26045-2-git-send-email-bdowning@lavos.net","threadId":"14972","inReplyTo":"1218692211-26045-1-git-send-email-bdowning@lavos.net","subject":"[PATCHv3 2/2] Use strbuf for struct xdiff_emit_state's remainder","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2008-08-14T05:36:51Z","receivedAt":"2008-08-14T05:36:51Z","isPatch":false,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"Continually xreallocing and freeing the remainder member of struct\nxdiff_emit_state was a noticeable performance hit.  Use a strbuf\ninstead.\n\nThis yields a decent performance improvement on \"git blame\" on certain\nrepositories.  For example, before this commit:\n\n$ time git blame -M -C -C -p --incremental server.c >/dev/null\n101.52user 0.17system 1:41.73elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+39561minor)pagefaults 0swaps\n\nWith this commit:\n\n$ time git blame -M -C -C -p --incremental server.c >/dev/null\n80.38user 0.30system 1:20.81elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+50979minor)pagefaults 0swaps\n\nSigned-off-by: Brian Downing <bdowning@lavos.net>\n---\n xdiff-interface.c |   32 ++++++++++----------------------\n xdiff-interface.h |    4 ++--\n 2 files changed, 12 insertions(+), 24 deletions(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 828b496..bf98866 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -69,36 +69,22 @@ static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n \tfor (i = 0; i < nbuf; i++) {\n \t\tif (mb[i].ptr[mb[i].size-1] != '\\n') {\n \t\t\t/* Incomplete line */\n-\t\t\tpriv->remainder = xrealloc(priv->remainder,\n-\t\t\t\t\t\t   priv->remainder_size +\n-\t\t\t\t\t\t   mb[i].size);\n-\t\t\tmemcpy(priv->remainder + priv->remainder_size,\n-\t\t\t       mb[i].ptr, mb[i].size);\n-\t\t\tpriv->remainder_size += mb[i].size;\n+\t\t\tstrbuf_add(&priv->remainder, mb[i].ptr, mb[i].size);\n \t\t\tcontinue;\n \t\t}\n \n \t\t/* we have a complete line */\n-\t\tif (!priv->remainder) {\n+\t\tif (!priv->remainder.len) {\n \t\t\tconsume_one(priv, mb[i].ptr, mb[i].size);\n \t\t\tcontinue;\n \t\t}\n-\t\tpriv->remainder = xrealloc(priv->remainder,\n-\t\t\t\t\t   priv->remainder_size +\n-\t\t\t\t\t   mb[i].size);\n-\t\tmemcpy(priv->remainder + priv->remainder_size,\n-\t\t       mb[i].ptr, mb[i].size);\n-\t\tconsume_one(priv, priv->remainder,\n-\t\t\t    priv->remainder_size + mb[i].size);\n-\t\tfree(priv->remainder);\n-\t\tpriv->remainder = NULL;\n-\t\tpriv->remainder_size = 0;\n+\t\tstrbuf_add(&priv->remainder, mb[i].ptr, mb[i].size);\n+\t\tconsume_one(priv, priv->remainder.buf, priv->remainder.len);\n+\t\tstrbuf_reset(&priv->remainder);\n \t}\n-\tif (priv->remainder) {\n-\t\tconsume_one(priv, priv->remainder, priv->remainder_size);\n-\t\tfree(priv->remainder);\n-\t\tpriv->remainder = NULL;\n-\t\tpriv->remainder_size = 0;\n+\tif (priv->remainder.len) {\n+\t\tconsume_one(priv, priv->remainder.buf, priv->remainder.len);\n+\t\tstrbuf_reset(&priv->remainder);\n \t}\n \treturn 0;\n }\n@@ -148,7 +134,9 @@ int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n \tint ret;\n \txecb->outf = xdiff_outf;\n \txecb->priv = state;\n+\tstrbuf_init(&state->remainder, 0);\n \tret = xdi_diff(mf1, mf2, xpp, xecfg, xecb);\n+\tstrbuf_release(&state->remainder);\n \treturn ret;\n }\n \ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex 6f3b361..f6a1ec2 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -2,6 +2,7 @@\n #define XDIFF_INTERFACE_H\n \n #include \"xdiff/xdiff.h\"\n+#include \"strbuf.h\"\n \n struct xdiff_emit_state;\n \n@@ -9,8 +10,7 @@ typedef void (*xdiff_emit_consume_fn)(void *, char *, unsigned long);\n \n struct xdiff_emit_state {\n \txdiff_emit_consume_fn consume;\n-\tchar *remainder;\n-\tunsigned long remainder_size;\n+\tstruct strbuf remainder;\n };\n \n int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);\n-- \n1.5.6.1\n"},{"id":"87156","messageId":"7v1w0sf7bl.fsf@gitster.siamese.dyndns.org","threadId":"14972","inReplyTo":"1218692211-26045-1-git-send-email-bdowning@lavos.net","subject":"Re: [PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-14T06:18:22Z","receivedAt":"2008-08-14T06:18:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Downing <bdowning@lavos.net> writes:\n\n>     Let's try this instead; it's quite a bit cleaner than my last\n>     effort.  [Especially now that it doesn't skip the xdi_diff\n>     function.]\n\nMuch nicer.  xdi_diff() is just a performance thing that only kicks in\nwhen you are running -U0 diff, so it is unsurprising that you did not see\nany test failures.\n\nHowever,\n\n * xm.consume is always set to something by the caller but all the other\n   fields in xm are initialized to zero --- perhaps xdi_diff_outf() can\n   take the pointer to the consume function as its parameter?\n\n * the whole set-up still requires that the xdiff_emit_state structure is\n   the first member of callback data structure, so making the third\n   parameter to xdi_diff_outf() to \"struct xdiff_emit_state\" and casting\n   it up to containing structure like your patch does does not offer any\n   additional type safety.\n\nI wonder if something like this on top of your patch would be worth doing\nas a further clean-up.\n\n-- >8 --\nxdiff-interface: hide the whole \"xdiff_emit_state\" business from the caller\n\nThis further enhances xdi_diff_outf() interface so that it takes two\ncommon parameters: the callback function that processes one line at a\ntime, and a pointer to its application specific callback data structure.\nxdi_diff_outf() creates its own \"xdiff_emit_state\" structure and stashes\nthese two away inside it, which is used by the lowest level output\nfunction in the xdiff_outf() callchain, consume_one(), to call back to the\napplication layer.  With this restructuring, we lift the requirement that\nthe caller supplied callback data structure embeds xdiff_emit_state\nstructure as its first member.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-blame.c   |    4 +---\n combine-diff.c    |    7 ++-----\n diff.c            |   27 ++++++++++-----------------\n xdiff-interface.c |   23 ++++++++++++++++++-----\n xdiff-interface.h |   11 ++---------\n 5 files changed, 33 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 8cca3b1..e4d12de 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -465,7 +465,6 @@ struct patch {\n };\n \n struct blame_diff_state {\n-\tstruct xdiff_emit_state xm;\n \tstruct patch *ret;\n \tunsigned hunk_post_context;\n \tunsigned hunk_in_pre_context : 1;\n@@ -529,12 +528,11 @@ static struct patch *compare_buffer(mmfile_t *file_p, mmfile_t *file_o,\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = context;\n \tmemset(&state, 0, sizeof(state));\n-\tstate.xm.consume = process_u_diff;\n \tstate.ret = xmalloc(sizeof(struct patch));\n \tstate.ret->chunks = NULL;\n \tstate.ret->num = 0;\n \n-\txdi_diff_outf(file_p, file_o, &state.xm, &xpp, &xecfg, &ecb);\n+\txdi_diff_outf(file_p, file_o, process_u_diff, &state, &xpp, &xecfg, &ecb);\n \n \tif (state.ret->num) {\n \t\tstruct chunk *chunk;\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 72dd6d2..31ec0c5 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -143,8 +143,6 @@ static void append_lost(struct sline *sline, int n, const char *line, int len)\n }\n \n struct combine_diff_state {\n-\tstruct xdiff_emit_state xm;\n-\n \tunsigned int lno;\n \tint ob, on, nb, nn;\n \tunsigned long nmask;\n@@ -218,15 +216,14 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n \txpp.flags = XDF_NEED_MINIMAL;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \tmemset(&state, 0, sizeof(state));\n-\tstate.xm.consume = consume_line;\n \tstate.nmask = nmask;\n \tstate.sline = sline;\n \tstate.lno = 1;\n \tstate.num_parent = num_parent;\n \tstate.n = n;\n \n-\txdi_diff_outf(&parent_file, result_file,\n-\t\t      &state.xm, &xpp, &xecfg, &ecb);\n+\txdi_diff_outf(&parent_file, result_file, consume_line, &state,\n+\t\t      &xpp, &xecfg, &ecb);\n \tfree(parent_file.ptr);\n \n \t/* Assign line numbers for this parent.\ndiff --git a/diff.c b/diff.c\nindex 913a92f..f18a4be 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -369,7 +369,6 @@ static void diff_words_append(char *line, unsigned long len,\n }\n \n struct diff_words_data {\n-\tstruct xdiff_emit_state xm;\n \tstruct diff_words_buffer minus, plus;\n \tFILE *file;\n };\n@@ -459,9 +458,8 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \n \txpp.flags = XDF_NEED_MINIMAL;\n \txecfg.ctxlen = diff_words->minus.alloc + diff_words->plus.alloc;\n-\tdiff_words->xm.consume = fn_out_diff_words_aux;\n-\txdi_diff_outf(&minus, &plus, &diff_words->xm, &xpp, &xecfg, &ecb);\n-\n+\txdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, &diff_words,\n+\t\t      &xpp, &xecfg, &ecb);\n \tfree(minus.ptr);\n \tfree(plus.ptr);\n \tdiff_words->minus.text.size = diff_words->plus.text.size = 0;\n@@ -475,7 +473,6 @@ static void diff_words_show(struct diff_words_data *diff_words)\n typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);\n \n struct emit_callback {\n-\tstruct xdiff_emit_state xm;\n \tint nparents, color_diff;\n \tunsigned ws_rule;\n \tsane_truncate_fn truncate;\n@@ -706,8 +703,6 @@ static char *pprint_rename(const char *a, const char *b)\n }\n \n struct diffstat_t {\n-\tstruct xdiff_emit_state xm;\n-\n \tint nr;\n \tint alloc;\n \tstruct diffstat_file {\n@@ -1129,7 +1124,6 @@ static void free_diffstat_info(struct diffstat_t *diffstat)\n }\n \n struct checkdiff_t {\n-\tstruct xdiff_emit_state xm;\n \tconst char *filename;\n \tint lineno;\n \tstruct diff_options *o;\n@@ -1519,13 +1513,13 @@ static void builtin_diff(const char *name_a,\n \t\t\txecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);\n \t\telse if (!prefixcmp(diffopts, \"-u\"))\n \t\t\txecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);\n-\t\tecbdata.xm.consume = fn_out_consume;\n \t\tif (DIFF_OPT_TST(o, COLOR_DIFF_WORDS)) {\n \t\t\tecbdata.diff_words =\n \t\t\t\txcalloc(1, sizeof(struct diff_words_data));\n \t\t\tecbdata.diff_words->file = o->file;\n \t\t}\n-\t\txdi_diff_outf(&mf1, &mf2, &ecbdata.xm, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,\n+\t\t\t      &xpp, &xecfg, &ecb);\n \t\tif (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))\n \t\t\tfree_diff_words_data(&ecbdata);\n \t}\n@@ -1576,7 +1570,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\n-\t\txdi_diff_outf(&mf1, &mf2, &diffstat->xm, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,\n+\t\t\t      &xpp, &xecfg, &ecb);\n \t}\n \n  free_and_return:\n@@ -1597,7 +1592,6 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \t\treturn;\n \n \tmemset(&data, 0, sizeof(data));\n-\tdata.xm.consume = checkdiff_consume;\n \tdata.filename = name_b ? name_b : name_a;\n \tdata.lineno = 0;\n \tdata.o = o;\n@@ -1622,7 +1616,8 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = XDF_NEED_MINIMAL;\n-\t\txdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,\n+\t\t\t      &xpp, &xecfg, &ecb);\n \n \t\tif ((data.ws_rule & WS_TRAILING_SPACE) &&\n \t\t    data.trailing_blanks_start) {\n@@ -3010,7 +3005,6 @@ static void diff_summary(FILE *file, struct diff_filepair *p)\n }\n \n struct patch_id_t {\n-\tstruct xdiff_emit_state xm;\n \tSHA_CTX *ctx;\n \tint patchlen;\n };\n@@ -3055,7 +3049,6 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \tSHA1_Init(&ctx);\n \tmemset(&data, 0, sizeof(struct patch_id_t));\n \tdata.ctx = &ctx;\n-\tdata.xm.consume = patch_id_consume;\n \n \tfor (i = 0; i < q->nr; i++) {\n \t\txpparam_t xpp;\n@@ -3120,7 +3113,8 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \t\txpp.flags = XDF_NEED_MINIMAL;\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n-\t\txdi_diff_outf(&mf1, &mf2, &data.xm, &xpp, &xecfg, &ecb);\n+\t\txdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,\n+\t\t\t      &xpp, &xecfg, &ecb);\n \t}\n \n \tSHA1_Final(sha1, &ctx);\n@@ -3197,7 +3191,6 @@ void diff_flush(struct diff_options *options)\n \t\tstruct diffstat_t diffstat;\n \n \t\tmemset(&diffstat, 0, sizeof(struct diffstat_t));\n-\t\tdiffstat.xm.consume = diffstat_consume;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \t\t\tif (check_pair_status(p))\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex bf98866..944ad98 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -1,5 +1,12 @@\n #include \"cache.h\"\n #include \"xdiff-interface.h\"\n+#include \"strbuf.h\"\n+\n+struct xdiff_emit_state {\n+\txdiff_emit_consume_fn consume;\n+\tvoid *consume_callback_data;\n+\tstruct strbuf remainder;\n+};\n \n static int parse_num(char **cp_p, int *num_p)\n {\n@@ -55,7 +62,7 @@ static void consume_one(void *priv_, char *s, unsigned long size)\n \t\tunsigned long this_size;\n \t\tep = memchr(s, '\\n', size);\n \t\tthis_size = (ep == NULL) ? size : (ep - s + 1);\n-\t\tpriv->consume(priv, s, this_size);\n+\t\tpriv->consume(priv->consume_callback_data, s, this_size);\n \t\tsize -= this_size;\n \t\ts += this_size;\n \t}\n@@ -128,15 +135,21 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co\n }\n \n int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n-\t\t  struct xdiff_emit_state *state, xpparam_t const *xpp,\n+\t\t  xdiff_emit_consume_fn fn, void *consume_callback_data,\n+\t\t  xpparam_t const *xpp,\n \t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb)\n {\n \tint ret;\n+\tstruct xdiff_emit_state state;\n+\n+\tmemset(&state, 0, sizeof(state));\n+\tstate.consume = fn;\n+\tstate.consume_callback_data = consume_callback_data;\n \txecb->outf = xdiff_outf;\n-\txecb->priv = state;\n-\tstrbuf_init(&state->remainder, 0);\n+\txecb->priv = &state;\n+\tstrbuf_init(&state.remainder, 0);\n \tret = xdi_diff(mf1, mf2, xpp, xecfg, xecb);\n-\tstrbuf_release(&state->remainder);\n+\tstrbuf_release(&state.remainder);\n \treturn ret;\n }\n \ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex f6a1ec2..558492b 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -2,20 +2,13 @@\n #define XDIFF_INTERFACE_H\n \n #include \"xdiff/xdiff.h\"\n-#include \"strbuf.h\"\n-\n-struct xdiff_emit_state;\n \n typedef void (*xdiff_emit_consume_fn)(void *, char *, unsigned long);\n \n-struct xdiff_emit_state {\n-\txdiff_emit_consume_fn consume;\n-\tstruct strbuf remainder;\n-};\n-\n int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);\n int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n-\t\t  struct xdiff_emit_state *state, xpparam_t const *xpp,\n+\t\t  xdiff_emit_consume_fn fn, void *consume_callback_data,\n+\t\t  xpparam_t const *xpp,\n \t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb);\n int parse_hunk_header(char *line, int len,\n \t\t      int *ob, int *on,\n"},{"id":"87158","messageId":"20080814063412.GF4396@lavos.net","threadId":"14972","inReplyTo":"7v1w0sf7bl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2008-08-14T06:34:12Z","receivedAt":"2008-08-14T06:34:12Z","isPatch":false,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"On Wed, Aug 13, 2008 at 11:18:22PM -0700, Junio C Hamano wrote:\n> I wonder if something like this on top of your patch would be worth doing\n> as a further clean-up.\n> \n> -- >8 --\n> xdiff-interface: hide the whole \"xdiff_emit_state\" business from the caller\n\nNice, looks good to me.\n\n-bcd\n"},{"id":"87940","messageId":"20080821033756.GC31114@lavos.net","threadId":"14972","inReplyTo":"7v1w0sf7bl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2008-08-21T03:37:57Z","receivedAt":"2008-08-21T03:37:57Z","isPatch":false,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"On Wed, Aug 13, 2008 at 11:18:22PM -0700, Junio C Hamano wrote:\n> Much nicer.  xdi_diff() is just a performance thing that only kicks in\n> when you are running -U0 diff, so it is unsurprising that you did not see\n> any test failures.\n\nInteresting point here.  In playing with trying to cache the diff hashes\nto speed up blame, I had to basically disable the xdi_diff tail trimming\nwhen building the hash the first time, because it needed to see the\nwhole file.  In doing this, I discovered that just changing from\nxdi_diff to xdl_diff /does/ change the blame -M -C -C --incremental\nresult for my test case.  (Unfortunately, my test case is proprietary\ncode...)\n\nIs this expected, or some kind of serious bug with xdi_diff?\n\n    :; diff proper-output other-output\n    980c980\n    < dee86dd25736e1778122cfde7d7455a3ef85e37d 173 173 2\n    ---\n    > dee86dd25736e1778122cfde7d7455a3ef85e37d 172 172 3\n    982c982\n    < dee86dd25736e1778122cfde7d7455a3ef85e37d 183 183 2\n    ---\n    > dee86dd25736e1778122cfde7d7455a3ef85e37d 184 184 1\n    1509c1509\n    < c6966941ebfaa1dc9b29489e53d6d7f41e52d357 287 384 1\n    ---\n    > c6966941ebfaa1dc9b29489e53d6d7f41e52d357 284 381 1\n    1511c1511\n    < c6966941ebfaa1dc9b29489e53d6d7f41e52d357 301 399 2\n    ---\n    > c6966941ebfaa1dc9b29489e53d6d7f41e52d357 286 383 2\n    1513c1513\n    < c6966941ebfaa1dc9b29489e53d6d7f41e52d357 304 402 1\n    ---\n    > c6966941ebfaa1dc9b29489e53d6d7f41e52d357 301 399 1\n    1608c1608\n    < ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 252 381 1\n    ---\n    > ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 252 385 1\n    1610c1610\n    < ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 255 383 1\n    ---\n    > ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 255 400 1\n    1612c1612\n    < ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 257 385 1\n    ---\n    > ecebfe8121dfd9c5836d47bbeb910fbb8f96f35c 257 402 1\n    1945c1945\n    < a325ab86914b15107bf0211550c7d0568fb0854c 138 172 1\n    ---\n    > a325ab86914b15107bf0211550c7d0568fb0854c 138 183 1\n\n-bcd\n"},{"id":"87944","messageId":"7vzln7j5yx.fsf@gitster.siamese.dyndns.org","threadId":"14972","inReplyTo":"20080821033756.GC31114@lavos.net","subject":"Re: [PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-21T05:24:22Z","receivedAt":"2008-08-21T05:24:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"bdowning@lavos.net (Brian Downing) writes:\n\n> On Wed, Aug 13, 2008 at 11:18:22PM -0700, Junio C Hamano wrote:\n>> Much nicer.  xdi_diff() is just a performance thing that only kicks in\n>> when you are running -U0 diff, so it is unsurprising that you did not see\n>> any test failures.\n>\n> Interesting point here.  In playing with trying to cache the diff hashes\n> to speed up blame, I had to basically disable the xdi_diff tail trimming\n> when building the hash the first time, because it needed to see the\n> whole file.  In doing this, I discovered that just changing from\n> xdi_diff to xdl_diff /does/ change the blame -M -C -C --incremental\n> result for my test case.  (Unfortunately, my test case is proprietary\n> code...)\n\nIs the reason why you mention \"incremental\" specifically because you only\ntested incremental, or you get identical result in non-incremental mode?\n\nIf your material is repetitive, say you have lines \"A A A B C A A A\" in\nthe parent blob and \"A A A B A A A\" in the child blob, and you are trying\nto pass blame on three line block \"A A A\" at the beginning of the child,\nwe can pass blame to the three lines at the beginning part, or to the end\npart, without Linus's common tail trimming optimization.  But there is no\nway it can match the end part with the optimization.\n\nYou cannot say one result is more correct than the other --- both are\nequally correct.  Of course, you could argue that with such a highly\nrepetitive material, it may be better to match closer ones, but it's a\njudgement call.\n"},{"id":"87946","messageId":"20080821062914.GD31114@lavos.net","threadId":"14972","inReplyTo":"7vzln7j5yx.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv3 1/2] Make xdi_diff_outf interface for running xdiff_outf diffs","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2008-08-21T06:29:14Z","receivedAt":"2008-08-21T06:29:14Z","isPatch":false,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"On Wed, Aug 20, 2008 at 10:24:22PM -0700, Junio C Hamano wrote:\n> Is the reason why you mention \"incremental\" specifically because you only\n> tested incremental, or you get identical result in non-incremental mode?\n\nI've only tested incremental at this point.\n\n> If your material is repetitive, say you have lines \"A A A B C A A A\" in\n> the parent blob and \"A A A B A A A\" in the child blob, and you are trying\n> to pass blame on three line block \"A A A\" at the beginning of the child,\n> we can pass blame to the three lines at the beginning part, or to the end\n> part, without Linus's common tail trimming optimization.  But there is no\n> way it can match the end part with the optimization.\n> \n> You cannot say one result is more correct than the other --- both are\n> equally correct.  Of course, you could argue that with such a highly\n> repetitive material, it may be better to match closer ones, but it's a\n> judgement call.\n\nOkay, that makes sense.  Thanks.\n\n-bcd\n"}]}