{"thread":{"id":"49739","subject":"[PATCH 0/9] saner xdiff hunk callbacks","startedAt":"2018-11-02T06:36:10Z","lastAt":"2018-11-03T00:32:11Z","messageCount":14,"participants":["Jeff King","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"362244","messageId":"20181102063156.GA30252@sigill.intra.peff.net","threadId":"49739","inReplyTo":null,"subject":"[PATCH 0/9] saner xdiff hunk callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T06:31:56Z","receivedAt":"2018-11-02T06:36:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As part of my -Wunused-parameter hunt, I noticed that parse_hunk_header()\ncan read out-of-bounds in the array it is given. But it turns out not to\nbe a big problem because we only ever use it to parse lines that xdiff\nhas just generated.\n\nThis is fixable, and I'll show my patch to do so at the end of this\nemail. But it seems rather silly that we have xdiff generate a string\njust so that we can parse it from within the same process. So instead I\nimproved the xdiff interface to pass the actual integers out, made use\nof it as appropriate, and then in the final patch we can just drop the\nbuggy function entirely.\n\nSo that's this series, which I think is the better fix:\n\n  [1/9]: xdiff: provide a separate emit callback for hunks\n  [2/9]: xdiff-interface: provide a separate consume callback for hunks\n  [3/9]: diff: avoid generating unused hunk header lines\n  [4/9]: diff: discard hunk headers for patch-ids earlier\n  [5/9]: diff: use hunk callback for word-diff\n  [6/9]: combine-diff: use an xdiff hunk callback\n  [7/9]: diff: convert --check to use a hunk callback\n  [8/9]: range-diff: use a hunk callback\n  [9/9]: xdiff-interface: drop parse_hunk_header()\n\n builtin/merge-tree.c |  3 +-\n builtin/rerere.c     |  3 +-\n combine-diff.c       | 67 ++++++++++++++++++++------------------\n diff.c               | 48 ++++++++++++++--------------\n diffcore-pickaxe.c   |  3 +-\n range-diff.c         | 10 +++++-\n xdiff-interface.c    | 76 ++++++++++++++++++--------------------------\n xdiff-interface.h    | 19 ++++++++---\n xdiff/xdiff.h        |  6 +++-\n xdiff/xutils.c       | 21 +++++++++---\n 10 files changed, 141 insertions(+), 115 deletions(-)\n\nFor reference/comparison, here's the minimal fix patch.\n\n-- >8 --\nSubject: [PATCH DO NOT APPLY] xdiff-interface: refactor parse_hunk_header\n\nThe parse_hunk_header() function takes its input as a ptr/len pair, but\ndoes not respect the \"len\" half, and instead treats it like a\nNUL-terminated string. This works out in practice because we only ever\nparse strings generated by xdiff. These do omit the trailing NUL, but\nsince they're always valid, we never parse past the newline.\n\nStill, it would be easy to misuse it, so let's fix it.\n\nWhile we're doing so, we can improve a few other things:\n\n  - by using helpers like skip_prefix_mem(), we can do the whole parse\n    within a conditional, avoiding the need to jump around (backwards,\n    even!) to the error block with a goto. The result is hopefully much\n    more readable.\n\n  - the check for the final \"@@\" would trigger an error return code if\n    it failed, but wouldn't actually emit an error message (like the\n    rest of the parse errors do)\n\n  - we used to blindly assume the caller checked that the string starts\n    with \"@@ -\"; we now check it ourselves and let the caller know what\n    we found\n\n  - the input line does not need to be modified, and can be marked\n    \"const\"\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe diff is pretty horrid, but hopefully the post-image is pleasant to\nread.\n\n combine-diff.c    | 13 +++++----\n diff.c            |  5 ++--\n xdiff-interface.c | 68 +++++++++++++++++++++++++----------------------\n xdiff-interface.h |  6 ++---\n 4 files changed, 50 insertions(+), 42 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 10155e0ec8..0318f39103 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -348,11 +348,14 @@ struct combine_diff_state {\n static void consume_line(void *state_, char *line, unsigned long len)\n {\n \tstruct combine_diff_state *state = state_;\n-\tif (5 < len && !memcmp(\"@@ -\", line, 4)) {\n-\t\tif (parse_hunk_header(line, len,\n-\t\t\t\t      &state->ob, &state->on,\n-\t\t\t\t      &state->nb, &state->nn))\n-\t\t\treturn;\n+\tswitch (maybe_parse_hunk_header(line, len,\n+\t\t\t\t\t&state->ob, &state->on,\n+\t\t\t\t\t&state->nb, &state->nn)) {\n+\tcase 0:\n+\t\tbreak; /* not a hunk header */\n+\tcase -1:\n+\t\treturn; /* parse error */\n+\tdefault:\n \t\tstate->lno = state->nb;\n \t\tif (state->nn == 0) {\n \t\t\t/* @@ -X,Y +N,0 @@ removed Y lines\ndiff --git a/diff.c b/diff.c\nindex 8647db3d30..af6c01c4d0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1921,8 +1921,9 @@ static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)\n \tstruct diff_options *opt = diff_words->opt;\n \tconst char *line_prefix;\n \n-\tif (line[0] != '@' || parse_hunk_header(line, len,\n-\t\t\t&minus_first, &minus_len, &plus_first, &plus_len))\n+\tif (maybe_parse_hunk_header(line, len,\n+\t\t\t\t    &minus_first, &minus_len,\n+\t\t\t\t    &plus_first, &plus_len) <= 0)\n \t\treturn;\n \n \tassert(opt);\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex e7af95db86..f574ee49cd 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -14,49 +14,53 @@ struct xdiff_emit_state {\n \tstruct strbuf remainder;\n };\n \n-static int parse_num(char **cp_p, int *num_p)\n+static int parse_num(const char **cp_p, size_t *len, int *num_p)\n {\n-\tchar *cp = *cp_p;\n+\tconst char *cp = *cp_p;\n+\tconst char *end = cp + *len;\n \tint num = 0;\n \n-\twhile ('0' <= *cp && *cp <= '9')\n+\twhile (cp < end && '0' <= *cp && *cp <= '9')\n \t\tnum = num * 10 + *cp++ - '0';\n \tif (!(cp - *cp_p))\n-\t\treturn -1;\n+\t\treturn 0;\n+\t*len -= cp - *cp_p;\n \t*cp_p = cp;\n \t*num_p = num;\n-\treturn 0;\n+\treturn 1;\n }\n \n-int parse_hunk_header(char *line, int len,\n-\t\t      int *ob, int *on,\n-\t\t      int *nb, int *nn)\n+static int parse_hunk_len(const char **cp_p, size_t *len, int *out)\n {\n-\tchar *cp;\n-\tcp = line + 4;\n-\tif (parse_num(&cp, ob)) {\n-\tbad_line:\n-\t\treturn error(\"malformed diff output: %s\", line);\n+\t/* hunk lengths are optional; if omitted, default to 1 */\n+\tif (skip_prefix_mem(*cp_p, *len, \",\", cp_p, len)) {\n+\t\tif (!parse_num(cp_p, len, out))\n+\t\t\treturn 0;\n+\t} else {\n+\t\t*out = 1;\n \t}\n-\tif (*cp == ',') {\n-\t\tcp++;\n-\t\tif (parse_num(&cp, on))\n-\t\t\tgoto bad_line;\n-\t}\n-\telse\n-\t\t*on = 1;\n-\tif (*cp++ != ' ' || *cp++ != '+')\n-\t\tgoto bad_line;\n-\tif (parse_num(&cp, nb))\n-\t\tgoto bad_line;\n-\tif (*cp == ',') {\n-\t\tcp++;\n-\t\tif (parse_num(&cp, nn))\n-\t\t\tgoto bad_line;\n-\t}\n-\telse\n-\t\t*nn = 1;\n-\treturn -!!memcmp(cp, \" @@\", 3);\n+\n+\treturn 1;\n+}\n+\n+int maybe_parse_hunk_header(const char *line, size_t len,\n+\t\t\t    int *ob, int *on,\n+\t\t\t    int *nb, int *nn)\n+{\n+\tconst char *cp;\n+\n+\tif (!skip_prefix_mem(line, len, \"@@ -\", &cp, &len))\n+\t\treturn 0;\n+\n+\tif (!parse_num(&cp, &len, ob) ||\n+\t    !parse_hunk_len(&cp, &len, on) ||\n+\t    !skip_prefix_mem(cp, len, \" +\", &cp, &len) ||\n+\t    !parse_num(&cp, &len, nb) ||\n+\t    !parse_hunk_len(&cp, &len, nn) ||\n+\t    !skip_prefix_mem(cp, len, \" @@\", &cp, &len))\n+\t\treturn error(\"malformed diff output: %s\", line);\n+\n+\treturn 1;\n }\n \n static void consume_one(void *priv_, char *s, unsigned long size)\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex 135fc05d72..cddd56bae6 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -17,9 +17,9 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co\n int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n \t\t  xdiff_emit_consume_fn fn, void *consume_callback_data,\n \t\t  xpparam_t const *xpp, xdemitconf_t const *xecfg);\n-int parse_hunk_header(char *line, int len,\n-\t\t      int *ob, int *on,\n-\t\t      int *nb, int *nn);\n+int maybe_parse_hunk_header(const char *line, size_t len,\n+\t\t\t    int *ob, int *on,\n+\t\t\t    int *nb, int *nn);\n int read_mmfile(mmfile_t *ptr, const char *filename);\n void read_mmblob(mmfile_t *ptr, const struct object_id *oid);\n int buffer_is_binary(const char *ptr, unsigned long size);\n-- \n2.19.1.1336.g081079ac04\n\n"},{"id":"362245","messageId":"20181102063501.GA31216@sigill.intra.peff.net","threadId":"49739","inReplyTo":"20181102063156.GA30252@sigill.intra.peff.net","subject":"[PATCH 1/9] xdiff: provide a separate emit callback for hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T06:35:01Z","receivedAt":"2018-11-02T06:36:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The xdiff library always emits hunk header lines to our callbacks as\nformatted strings like \"@@ -a,b +c,d @@\\n\". This is convenient if we're\ngoing to output a diff, but less so if we actually need to compute using\nthose numbers, which requires re-parsing the line.\n\nIn preparation for moving away from this, let's teach xdiff a new\ncallback function which gets the broken-out hunk information. To help\ncallers that don't want to use this new callback, if it's NULL we'll\ncontinue to format the hunk header into a string.\n\nNote that this function renames the \"outf\" callback to \"out_line\", as\nwell. This isn't strictly necessary, but helps in two ways:\n\n  1. Now that there are two callbacks, it's nice to use more descriptive\n     names.\n\n  2. Many callers did not zero the emit_callback_data struct, and needed\n     to be modified to set ecb.out_hunk to NULL. By changing the name of\n     the existing struct member, that guarantees that any new callers\n     from in-flight topics will break the build and be examined\n     manually.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge-tree.c |  3 ++-\n builtin/rerere.c     |  3 ++-\n xdiff-interface.c    |  2 +-\n xdiff/xdiff.h        |  6 +++++-\n xdiff/xutils.c       | 21 +++++++++++++++++----\n 5 files changed, 27 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 8fc108d305..70f6fc9167 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -110,7 +110,8 @@ static void show_diff(struct merge_list *entry)\n \txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n-\tecb.outf = show_outf;\n+\tecb.out_hunk = NULL;\n+\tecb.out_line = show_outf;\n \tecb.priv = NULL;\n \n \tsrc.ptr = origin(entry, &size);\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex e89ccbc524..d78eeaed32 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -41,7 +41,8 @@ static int diff_two(const char *file1, const char *label1,\n \txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n-\tecb.outf = outf;\n+\tecb.out_hunk = NULL;\n+\tecb.out_line = outf;\n \tret = xdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);\n \n \tfree(minus.ptr);\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex e7af95db86..25c185aff4 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -152,7 +152,7 @@ int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n \tstate.consume = fn;\n \tstate.consume_callback_data = consume_callback_data;\n \tmemset(&ecb, 0, sizeof(ecb));\n-\tecb.outf = xdiff_outf;\n+\tecb.out_line = xdiff_outf;\n \tecb.priv = &state;\n \tstrbuf_init(&state.remainder, 0);\n \tret = xdi_diff(mf1, mf2, xpp, xecfg, &ecb);\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 2356da5f78..b158369020 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -86,7 +86,11 @@ typedef struct s_xpparam {\n \n typedef struct s_xdemitcb {\n \tvoid *priv;\n-\tint (*outf)(void *, mmbuffer_t *, int);\n+\tint (*out_hunk)(void *,\n+\t\t\tlong old_begin, long old_nr,\n+\t\t\tlong new_begin, long new_nr,\n+\t\t\tconst char *func, long funclen);\n+\tint (*out_line)(void *, mmbuffer_t *, int);\n } xdemitcb_t;\n \n typedef long (*find_func_t)(const char *line, long line_len, char *buffer, long buffer_size, void *priv);\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex 88e5995535..963e1c58b9 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -54,7 +54,7 @@ int xdl_emit_diffrec(char const *rec, long size, char const *pre, long psize,\n \t\tmb[2].size = strlen(mb[2].ptr);\n \t\ti++;\n \t}\n-\tif (ecb->outf(ecb->priv, mb, i) < 0) {\n+\tif (ecb->out_line(ecb->priv, mb, i) < 0) {\n \n \t\treturn -1;\n \t}\n@@ -344,8 +344,9 @@ int xdl_num_out(char *out, long val) {\n \treturn str - out;\n }\n \n-int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2,\n-\t\t      const char *func, long funclen, xdemitcb_t *ecb) {\n+static int xdl_format_hunk_hdr(long s1, long c1, long s2, long c2,\n+\t\t\t       const char *func, long funclen,\n+\t\t\t       xdemitcb_t *ecb) {\n \tint nb = 0;\n \tmmbuffer_t mb;\n \tchar buf[128];\n@@ -387,9 +388,21 @@ int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2,\n \n \tmb.ptr = buf;\n \tmb.size = nb;\n-\tif (ecb->outf(ecb->priv, &mb, 1) < 0)\n+\tif (ecb->out_line(ecb->priv, &mb, 1) < 0)\n \t\treturn -1;\n+\treturn 0;\n+}\n \n+int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2,\n+\t\t      const char *func, long funclen,\n+\t\t      xdemitcb_t *ecb) {\n+\tif (!ecb->out_hunk)\n+\t\treturn xdl_format_hunk_hdr(s1, c1, s2, c2, func, funclen, ecb);\n+\tif (ecb->out_hunk(ecb->priv,\n+\t\t\t  c1 ? s1 : s1 - 1, c1,\n+\t\t\t  c2 ? s2 : s2 - 1, c2,\n+\t\t\t  func, funclen) < 0)\n+\t\treturn -1;\n \treturn 0;\n }\n \n-- \n2.19.1.1336.g081079ac04\n\n"},{"id":"362246","messageId":"20181102063545.GB31216@sigill.intra.peff.net","threadId":"49739","inReplyTo":"20181102063156.GA30252@sigill.intra.peff.net","subject":"[PATCH 2/9] xdiff-interface: provide a separate consume callback for hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T06:35:45Z","receivedAt":"2018-11-02T06:36:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit taught xdiff to optionally provide the hunk header\ndata to a specialized callback. But most users of xdiff actually use our\nmore convenient xdi_diff_outf() helper, which ensures that our callbacks\nare always fed whole lines.\n\nLet's plumb the special hunk-callback through this interface, too. It\nwill follow the same rule as xdiff when the hunk callback is NULL (i.e.,\ncontinue to pass a stringified hunk header to the line callback). Since\nwe add NULL to each caller, there should be no behavior change yet.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n combine-diff.c     |  4 ++--\n diff.c             | 20 ++++++++++----------\n diffcore-pickaxe.c |  2 +-\n range-diff.c       |  2 +-\n xdiff-interface.c  | 30 ++++++++++++++++++++++++++----\n xdiff-interface.h  | 10 ++++++++--\n 6 files changed, 48 insertions(+), 20 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 10155e0ec8..3c479cfc3e 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -421,8 +421,8 @@ static void combine_diff(struct repository *r,\n \tstate.num_parent = num_parent;\n \tstate.n = n;\n \n-\tif (xdi_diff_outf(&parent_file, result_file, consume_line, &state,\n-\t\t\t  &xpp, &xecfg))\n+\tif (xdi_diff_outf(&parent_file, result_file, NULL, consume_line,\n+\t\t\t  &state, &xpp, &xecfg))\n \t\tdie(\"unable to generate combined diff for %s\",\n \t\t    oid_to_hex(parent));\n \tfree(parent_file.ptr);\ndiff --git a/diff.c b/diff.c\nindex 8647db3d30..07be5879e5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2074,8 +2074,8 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \txpp.flags = 0;\n \t/* as only the hunk header will be parsed, we need a 0-context */\n \txecfg.ctxlen = 0;\n-\tif (xdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, diff_words,\n-\t\t\t  &xpp, &xecfg))\n+\tif (xdi_diff_outf(&minus, &plus, NULL, fn_out_diff_words_aux,\n+\t\t\t  diff_words, &xpp, &xecfg))\n \t\tdie(\"unable to generate word diff\");\n \tfree(minus.ptr);\n \tfree(plus.ptr);\n@@ -3526,8 +3526,8 @@ static void builtin_diff(const char *name_a,\n \t\t\txecfg.ctxlen = strtoul(v, NULL, 10);\n \t\tif (o->word_diff)\n \t\t\tinit_diff_words_data(&ecbdata, o, one, two);\n-\t\tif (xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,\n-\t\t\t\t  &xpp, &xecfg))\n+\t\tif (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume,\n+\t\t\t\t  &ecbdata, &xpp, &xecfg))\n \t\t\tdie(\"unable to generate diff for %s\", one->path);\n \t\tif (o->word_diff)\n \t\t\tfree_diff_words_data(&ecbdata);\n@@ -3637,8 +3637,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\txpp.anchors_nr = o->anchors_nr;\n \t\txecfg.ctxlen = o->context;\n \t\txecfg.interhunkctxlen = o->interhunkcontext;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,\n-\t\t\t\t  &xpp, &xecfg))\n+\t\tif (xdi_diff_outf(&mf1, &mf2, NULL, diffstat_consume,\n+\t\t\t\t  diffstat, &xpp, &xecfg))\n \t\t\tdie(\"unable to generate diffstat for %s\", one->path);\n \t}\n \n@@ -3686,8 +3686,8 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txecfg.ctxlen = 1; /* at least one context line */\n \t\txpp.flags = 0;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,\n-\t\t\t\t  &xpp, &xecfg))\n+\t\tif (xdi_diff_outf(&mf1, &mf2, NULL, checkdiff_consume,\n+\t\t\t\t  &data, &xpp, &xecfg))\n \t\t\tdie(\"unable to generate checkdiff for %s\", one->path);\n \n \t\tif (data.ws_rule & WS_BLANK_AT_EOF) {\n@@ -5771,8 +5771,8 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\txpp.flags = 0;\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = 0;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,\n-\t\t\t\t  &xpp, &xecfg))\n+\t\tif (xdi_diff_outf(&mf1, &mf2, NULL, patch_id_consume,\n+\t\t\t\t  &data, &xpp, &xecfg))\n \t\t\treturn error(\"unable to generate patch-id diff for %s\",\n \t\t\t\t     p->one->path);\n \t}\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex d2361e06a1..58df35670a 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -62,7 +62,7 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n \tecbdata.hit = 0;\n \txecfg.ctxlen = o->context;\n \txecfg.interhunkctxlen = o->interhunkcontext;\n-\tif (xdi_diff_outf(one, two, diffgrep_consume, &ecbdata, &xpp, &xecfg))\n+\tif (xdi_diff_outf(one, two, NULL, diffgrep_consume, &ecbdata, &xpp, &xecfg))\n \t\treturn 0;\n \treturn ecbdata.hit;\n }\ndiff --git a/range-diff.c b/range-diff.c\nindex bd8083f2d1..245fc17fee 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -210,7 +210,7 @@ static int diffsize(const char *a, const char *b)\n \tmf2.size = strlen(b);\n \n \tcfg.ctxlen = 3;\n-\tif (!xdi_diff_outf(&mf1, &mf2, diffsize_consume, &count, &pp, &cfg))\n+\tif (!xdi_diff_outf(&mf1, &mf2, NULL, diffsize_consume, &count, &pp, &cfg))\n \t\treturn count;\n \n \terror(_(\"failed to generate diff\"));\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 25c185aff4..16d37ce6be 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -9,7 +9,8 @@\n #include \"xdiff/xutils.h\"\n \n struct xdiff_emit_state {\n-\txdiff_emit_consume_fn consume;\n+\txdiff_emit_hunk_fn hunk_fn;\n+\txdiff_emit_line_fn line_fn;\n \tvoid *consume_callback_data;\n \tstruct strbuf remainder;\n };\n@@ -59,6 +60,22 @@ int parse_hunk_header(char *line, int len,\n \treturn -!!memcmp(cp, \" @@\", 3);\n }\n \n+static int xdiff_out_hunk(void *priv_,\n+\t\t\t  long old_begin, long old_nr,\n+\t\t\t  long new_begin, long new_nr,\n+\t\t\t  const char *func, long funclen)\n+{\n+\tstruct xdiff_emit_state *priv = priv_;\n+\n+\tif (priv->remainder.len)\n+\t\tBUG(\"xdiff emitted hunk in the middle of a line\");\n+\n+\tpriv->hunk_fn(priv->consume_callback_data,\n+\t\t      old_begin, old_nr, new_begin, new_nr,\n+\t\t      func, funclen);\n+\treturn 0;\n+}\n+\n static void consume_one(void *priv_, char *s, unsigned long size)\n {\n \tstruct xdiff_emit_state *priv = priv_;\n@@ -67,7 +84,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->consume_callback_data, s, this_size);\n+\t\tpriv->line_fn(priv->consume_callback_data, s, this_size);\n \t\tsize -= this_size;\n \t\ts += this_size;\n \t}\n@@ -141,7 +158,9 @@ 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  xdiff_emit_consume_fn fn, void *consume_callback_data,\n+\t\t  xdiff_emit_hunk_fn hunk_fn,\n+\t\t  xdiff_emit_line_fn line_fn,\n+\t\t  void *consume_callback_data,\n \t\t  xpparam_t const *xpp, xdemitconf_t const *xecfg)\n {\n \tint ret;\n@@ -149,9 +168,12 @@ int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n \txdemitcb_t ecb;\n \n \tmemset(&state, 0, sizeof(state));\n-\tstate.consume = fn;\n+\tstate.hunk_fn = hunk_fn;\n+\tstate.line_fn = line_fn;\n \tstate.consume_callback_data = consume_callback_data;\n \tmemset(&ecb, 0, sizeof(ecb));\n+\tif (hunk_fn)\n+\t\tecb.out_hunk = xdiff_out_hunk;\n \tecb.out_line = xdiff_outf;\n \tecb.priv = &state;\n \tstrbuf_init(&state.remainder, 0);\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex 135fc05d72..2dbe2feb19 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -11,11 +11,17 @@\n  */\n #define MAX_XDIFF_SIZE (1024UL * 1024 * 1023)\n \n-typedef void (*xdiff_emit_consume_fn)(void *, char *, unsigned long);\n+typedef void (*xdiff_emit_line_fn)(void *, char *, unsigned long);\n+typedef void (*xdiff_emit_hunk_fn)(void *data,\n+\t\t\t\t   long old_begin, long old_nr,\n+\t\t\t\t   long new_begin, long new_nr,\n+\t\t\t\t   const char *func, long funclen);\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  xdiff_emit_consume_fn fn, void *consume_callback_data,\n+\t\t  xdiff_emit_hunk_fn hunk_fn,\n+\t\t  xdiff_emit_line_fn line_fn,\n+\t\t  void *consume_callback_data,\n \t\t  xpparam_t const *xpp, xdemitconf_t const *xecfg);\n int parse_hunk_header(char *line, int len,\n \t\t      int *ob, int *on,\n-- \n2.19.1.1336.g081079ac04\n\n"},{"id":"362247","messageId":"20181102063606.GC31216@sigill.intra.peff.net","threadId":"49739","inReplyTo":"20181102063156.GA30252@sigill.intra.peff.net","subject":"[PATCH 3/9] diff: avoid generating unused hunk header lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T06:36:06Z","receivedAt":"2018-11-02T06:36:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some callers of xdi_diff_outf() do not look at the generated hunk header\nlines at all. By plugging in a no-op hunk callback, this tells xdiff not\nto even bother formatting them.\n\nThis patch introduces a stock no-op callback and uses it with a few\ncallers whose line callbacks explicitly ignore hunk headers (because\nthey look only for +/- lines).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c             | 4 ++--\n diffcore-pickaxe.c | 3 ++-\n xdiff-interface.c  | 6 ++++++\n xdiff-interface.h  | 6 ++++++\n 4 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 07be5879e5..d84356e007 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3637,8 +3637,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\txpp.anchors_nr = o->anchors_nr;\n \t\txecfg.ctxlen = o->context;\n \t\txecfg.interhunkctxlen = o->interhunkcontext;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, NULL, diffstat_consume,\n-\t\t\t\t  diffstat, &xpp, &xecfg))\n+\t\tif (xdi_diff_outf(&mf1, &mf2, discard_hunk_line,\n+\t\t\t\t  diffstat_consume, diffstat, &xpp, &xecfg))\n \t\t\tdie(\"unable to generate diffstat for %s\", one->path);\n \t}\n \ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 58df35670a..69fc55ea1e 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -62,7 +62,8 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n \tecbdata.hit = 0;\n \txecfg.ctxlen = o->context;\n \txecfg.interhunkctxlen = o->interhunkcontext;\n-\tif (xdi_diff_outf(one, two, NULL, diffgrep_consume, &ecbdata, &xpp, &xecfg))\n+\tif (xdi_diff_outf(one, two, discard_hunk_line, diffgrep_consume,\n+\t\t\t  &ecbdata, &xpp, &xecfg))\n \t\treturn 0;\n \treturn ecbdata.hit;\n }\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 16d37ce6be..2622981d97 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -157,6 +157,12 @@ 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+void discard_hunk_line(void *priv,\n+\t\t       long ob, long on, long nb, long nn,\n+\t\t       const char *func, long funclen)\n+{\n+}\n+\n int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n \t\t  xdiff_emit_hunk_fn hunk_fn,\n \t\t  xdiff_emit_line_fn line_fn,\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex 2dbe2feb19..7b0ccbdd1d 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -35,6 +35,12 @@ extern void xdiff_clear_find_func(xdemitconf_t *xecfg);\n extern int git_xmerge_config(const char *var, const char *value, void *cb);\n extern int git_xmerge_style;\n \n+/*\n+ * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL\n+ * one just sends the hunk line to the line_fn callback).\n+ */\n+void discard_hunk_line(void *, long, long, long, long, const char *, long);\n+\n /*\n  * Compare the strings l1 with l2 which are of size s1 and s2 respectively.\n  * Returns 1 if the strings are deemed equal, 0 otherwise.\n-- \n2.19.1.1336.g081079ac04\n\n"},{"id":"362248","messageId":"20181102063636.GD31216@sigill.intra.peff.net","threadId":"49739","inReplyTo":"20181102063156.GA30252@sigill.intra.peff.net","subject":"[PATCH 4/9] diff: discard hunk headers for patch-ids earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T06:36:36Z","receivedAt":"2018-11-02T06:36:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not include hunk header lines when computing patch-ids, since\nthe line numbers would create false negatives. Rather than detect and\nskip them in our line callback, we can simply tell xdiff to avoid\ngenerating them.\n\nThis is similar to the previous commit, but split out because it\nactually requires modifying the matching line callback.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d84356e007..8a83ce6058 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5666,10 +5666,6 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)\n \tstruct patch_id_t *data = priv;\n \tint new_len;\n \n-\t/* Ignore line numbers when computing the SHA1 of the patch */\n-\tif (starts_with(line, \"@@ -\"))\n-\t\treturn;\n-\n \tnew_len = remove_space(line, len);\n \n \tgit_SHA1_Update(data->ctx, line, new_len);\n@@ -5771,8 +5767,8 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\txpp.flags = 0;\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = 0;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, NULL, patch_id_consume,\n-\t\t\t\t  &data, &xpp, &xecfg))\n+\t\tif (xdi_diff_outf(&mf1, &mf2, discard_hunk_line,\n+\t\t\t\t  patch_id_consume, &data, &xpp, &xecfg))\n \t\t\treturn error(\"unable to generate patch-id diff for %s\",\n \t\t\t\t     p->one->path);\n \t}\n-- \n2.19.1.1336.g081079ac04\n\n"},{"id":"362249","messageId":"20181102063718.GE31216@sigill.intra.peff.net","threadId":"49739","inReplyTo":"20181102063156.GA30252@sigill.intra.peff.net","subject":"[PATCH 5/9] diff: use hunk callback for word-diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T06:37:18Z","receivedAt":"2018-11-02T06:37:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our word-diff does not look at the -/+ lines generated by xdiff at all\n(because they are not real lines to show the user, but just the\ntokenized words split into lines). Instead we use the line numbers from\nthe hunk headers to index our own data structure.\n\nAs a result, our xdi_diff_outf() callback throws away all lines except\nhunk headers. We can instead use a hunk callback, which has two\nbenefits:\n\n  1. We don't have to re-parse the generated hunk header line, but can\n     use the passed parameters directly.\n\n  2. By setting our line callback to NULL, we can tell xdiff-interface\n     that it does not even need to bother generating the other lines,\n     saving a small amount of work.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c            | 12 +++++-------\n xdiff-interface.c |  3 +++\n 2 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8a83ce6058..37035110dc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1912,19 +1912,17 @@ static int color_words_output_graph_prefix(struct diff_words_data *diff_words)\n \t}\n }\n \n-static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)\n+static void fn_out_diff_words_aux(void *priv,\n+\t\t\t\t  long minus_first, long minus_len,\n+\t\t\t\t  long plus_first, long plus_len,\n+\t\t\t\t  const char *func, long funclen)\n {\n \tstruct diff_words_data *diff_words = priv;\n \tstruct diff_words_style *style = diff_words->style;\n-\tint minus_first, minus_len, plus_first, plus_len;\n \tconst char *minus_begin, *minus_end, *plus_begin, *plus_end;\n \tstruct diff_options *opt = diff_words->opt;\n \tconst char *line_prefix;\n \n-\tif (line[0] != '@' || parse_hunk_header(line, len,\n-\t\t\t&minus_first, &minus_len, &plus_first, &plus_len))\n-\t\treturn;\n-\n \tassert(opt);\n \tline_prefix = diff_line_prefix(opt);\n \n@@ -2074,7 +2072,7 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \txpp.flags = 0;\n \t/* as only the hunk header will be parsed, we need a 0-context */\n \txecfg.ctxlen = 0;\n-\tif (xdi_diff_outf(&minus, &plus, NULL, fn_out_diff_words_aux,\n+\tif (xdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, NULL,\n \t\t\t  diff_words, &xpp, &xecfg))\n \t\tdie(\"unable to generate word diff\");\n \tfree(minus.ptr);\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 2622981d97..f2a3d9a577 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -95,6 +95,9 @@ static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n \tstruct xdiff_emit_state *priv = priv_;\n \tint i;\n \n+\tif (!priv->line_fn)\n+\t\treturn 0;\n+\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-- \n2.19.1.1336.g081079ac04\n\n"},{"id":"362250","messageId":"20181102063820.GF31216@sigill.intra.peff.net","threadId":"49739","inReplyTo":"20181102063156.GA30252@sigill.intra.peff.net","subject":"[PATCH 6/9] combine-diff: use an xdiff hunk callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T06:38:20Z","receivedAt":"2018-11-02T06:38:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A combined diff has to line up the hunks for all of the individual\npairwise diffs, and thus needs to know their line numbers and sizes. We\nget that now by parsing the hunk header line that xdiff generates.\nHowever, now that xdiff supports a hunk callback, we can just use the\nvalues directly.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI learned to love --color-moved-ws=allow-indentation-change for this\none.\n\n combine-diff.c | 67 +++++++++++++++++++++++++++-----------------------\n 1 file changed, 36 insertions(+), 31 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 3c479cfc3e..ad7752ea6b 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -345,38 +345,43 @@ struct combine_diff_state {\n \tstruct sline *lost_bucket;\n };\n \n-static void consume_line(void *state_, char *line, unsigned long len)\n+static void consume_hunk(void *state_,\n+\t\t\t long ob, long on,\n+\t\t\t long nb, long nn,\n+\t\t\t const char *funcline, long funclen)\n {\n \tstruct combine_diff_state *state = state_;\n-\tif (5 < len && !memcmp(\"@@ -\", line, 4)) {\n-\t\tif (parse_hunk_header(line, len,\n-\t\t\t\t      &state->ob, &state->on,\n-\t\t\t\t      &state->nb, &state->nn))\n-\t\t\treturn;\n-\t\tstate->lno = state->nb;\n-\t\tif (state->nn == 0) {\n-\t\t\t/* @@ -X,Y +N,0 @@ removed Y lines\n-\t\t\t * that would have come *after* line N\n-\t\t\t * in the result.  Our lost buckets hang\n-\t\t\t * to the line after the removed lines,\n-\t\t\t *\n-\t\t\t * Note that this is correct even when N == 0,\n-\t\t\t * in which case the hunk removes the first\n-\t\t\t * line in the file.\n-\t\t\t */\n-\t\t\tstate->lost_bucket = &state->sline[state->nb];\n-\t\t\tif (!state->nb)\n-\t\t\t\tstate->nb = 1;\n-\t\t} else {\n-\t\t\tstate->lost_bucket = &state->sline[state->nb-1];\n-\t\t}\n-\t\tif (!state->sline[state->nb-1].p_lno)\n-\t\t\tstate->sline[state->nb-1].p_lno =\n-\t\t\t\txcalloc(state->num_parent,\n-\t\t\t\t\tsizeof(unsigned long));\n-\t\tstate->sline[state->nb-1].p_lno[state->n] = state->ob;\n-\t\treturn;\n+\n+\tstate->ob = ob;\n+\tstate->on = on;\n+\tstate->nb = nb;\n+\tstate->nn = nn;\n+\tstate->lno = state->nb;\n+\tif (state->nn == 0) {\n+\t\t/* @@ -X,Y +N,0 @@ removed Y lines\n+\t\t * that would have come *after* line N\n+\t\t * in the result.  Our lost buckets hang\n+\t\t * to the line after the removed lines,\n+\t\t *\n+\t\t * Note that this is correct even when N == 0,\n+\t\t * in which case the hunk removes the first\n+\t\t * line in the file.\n+\t\t */\n+\t\tstate->lost_bucket = &state->sline[state->nb];\n+\t\tif (!state->nb)\n+\t\t\tstate->nb = 1;\n+\t} else {\n+\t\tstate->lost_bucket = &state->sline[state->nb-1];\n \t}\n+\tif (!state->sline[state->nb-1].p_lno)\n+\t\tstate->sline[state->nb-1].p_lno =\n+\t\t\txcalloc(state->num_parent, sizeof(unsigned long));\n+\tstate->sline[state->nb-1].p_lno[state->n] = state->ob;\n+}\n+\n+static void consume_line(void *state_, char *line, unsigned long len)\n+{\n+\tstruct combine_diff_state *state = state_;\n \tif (!state->lost_bucket)\n \t\treturn; /* not in any hunk yet */\n \tswitch (line[0]) {\n@@ -421,8 +426,8 @@ static void combine_diff(struct repository *r,\n \tstate.num_parent = num_parent;\n \tstate.n = n;\n \n-\tif (xdi_diff_outf(&parent_file, result_file, NULL, consume_line,\n-\t\t\t  &state, &xpp, &xecfg))\n+\tif (xdi_diff_outf(&parent_file, result_file, consume_hunk,\n+\t\t\t  consume_line, &state, &xpp, &xecfg))\n \t\tdie(\"unable to generate combined diff for %s\",\n \t\t    oid_to_hex(parent));\n \tfree(parent_file.ptr);\n-- \n2.19.1.1336.g081079ac04\n\n"},{"id":"362251","messageId":"20181102063903.GG31216@sigill.intra.peff.net","threadId":"49739","inReplyTo":"20181102063156.GA30252@sigill.intra.peff.net","subject":"[PATCH 7/9] diff: convert --check to use a hunk callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T06:39:03Z","receivedAt":"2018-11-02T06:39:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"diff --check\" code needs to know the line number on which each hunk\nstarts in order to generate its output. We get that now by parsing the\nhunk header line generated by xdiff, but it's much simpler to just pass\nit directly using a hunk callback.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 20 ++++++++++++--------\n 1 file changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 37035110dc..e38d1ecaf4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3128,6 +3128,15 @@ static int is_conflict_marker(const char *line, int marker_size, unsigned long l\n \treturn 1;\n }\n \n+static void checkdiff_consume_hunk(void *priv,\n+\t\t\t\t   long ob, long on, long nb, long nn,\n+\t\t\t\t   const char *func, long funclen)\n+\n+{\n+\tstruct checkdiff_t *data = priv;\n+\tdata->lineno = nb - 1;\n+}\n+\n static void checkdiff_consume(void *priv, char *line, unsigned long len)\n {\n \tstruct checkdiff_t *data = priv;\n@@ -3163,12 +3172,6 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)\n \t\t\t      data->o->file, set, reset, ws);\n \t} else if (line[0] == ' ') {\n \t\tdata->lineno++;\n-\t} else if (line[0] == '@') {\n-\t\tchar *plus = strchr(line, '+');\n-\t\tif (plus)\n-\t\t\tdata->lineno = strtol(plus, NULL, 10) - 1;\n-\t\telse\n-\t\t\tdie(\"invalid diff\");\n \t}\n }\n \n@@ -3684,8 +3687,9 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txecfg.ctxlen = 1; /* at least one context line */\n \t\txpp.flags = 0;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, NULL, checkdiff_consume,\n-\t\t\t\t  &data, &xpp, &xecfg))\n+\t\tif (xdi_diff_outf(&mf1, &mf2, checkdiff_consume_hunk,\n+\t\t\t\t  checkdiff_consume, &data,\n+\t\t\t\t  &xpp, &xecfg))\n \t\t\tdie(\"unable to generate checkdiff for %s\", one->path);\n \n \t\tif (data.ws_rule & WS_BLANK_AT_EOF) {\n-- \n2.19.1.1336.g081079ac04\n\n"},{"id":"362252","messageId":"20181102063939.GH31216@sigill.intra.peff.net","threadId":"49739","inReplyTo":"20181102063156.GA30252@sigill.intra.peff.net","subject":"[PATCH 8/9] range-diff: use a hunk callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T06:39:40Z","receivedAt":"2018-11-02T06:39:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we count the lines in a diff, we don't actually care about the\ncontents of each line. By using a hunk callback, we tell xdiff that it\ndoes not need to even bother generating a hunk header line, saving a\nsmall amount of work.\n\nArguably we could even ignore the hunk headers completely, since we're\njust computing a cost function between patches. But doing it this way\nmaintains the exact same behavior before and after.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis one might be going overboard. It can be dropped without affecting\nany of the other patches.\n\n range-diff.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 245fc17fee..3958720f00 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -197,6 +197,12 @@ static void diffsize_consume(void *data, char *line, unsigned long len)\n \t(*(int *)data)++;\n }\n \n+static void diffsize_hunk(void *data, long ob, long on, long nb, long nn,\n+\t\t\t  const char *funcline, long funclen)\n+{\n+\tdiffsize_consume(data, NULL, 0);\n+}\n+\n static int diffsize(const char *a, const char *b)\n {\n \txpparam_t pp = { 0 };\n@@ -210,7 +216,9 @@ static int diffsize(const char *a, const char *b)\n \tmf2.size = strlen(b);\n \n \tcfg.ctxlen = 3;\n-\tif (!xdi_diff_outf(&mf1, &mf2, NULL, diffsize_consume, &count, &pp, &cfg))\n+\tif (!xdi_diff_outf(&mf1, &mf2,\n+\t\t\t   diffsize_hunk, diffsize_consume, &count,\n+\t\t\t   &pp, &cfg))\n \t\treturn count;\n \n \terror(_(\"failed to generate diff\"));\n-- \n2.19.1.1336.g081079ac04\n\n"},{"id":"362253","messageId":"20181102064013.GI31216@sigill.intra.peff.net","threadId":"49739","inReplyTo":"20181102063156.GA30252@sigill.intra.peff.net","subject":"[PATCH 9/9] xdiff-interface: drop parse_hunk_header()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T06:40:13Z","receivedAt":"2018-11-02T06:40:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function was used only for parsing the hunk headers generated by\nxdiff. Now that we can use hunk callbacks to get that information\ndirectly, it has outlived its usefulness.\n\nNote to anyone who wants to resurrect it: the \"len\" parameter was\ntotally unused, meaning that the function could read past the end of the\n\"line\" array. In practice this never happened, because we only used it\nto parse xdiff's generated header lines. But it would be dangerous to\nuse it for other cases without fixing this defect.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n xdiff-interface.c | 45 ---------------------------------------------\n xdiff-interface.h |  3 ---\n 2 files changed, 48 deletions(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex f2a3d9a577..80f060d278 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -15,51 +15,6 @@ struct xdiff_emit_state {\n \tstruct strbuf remainder;\n };\n \n-static int parse_num(char **cp_p, int *num_p)\n-{\n-\tchar *cp = *cp_p;\n-\tint num = 0;\n-\n-\twhile ('0' <= *cp && *cp <= '9')\n-\t\tnum = num * 10 + *cp++ - '0';\n-\tif (!(cp - *cp_p))\n-\t\treturn -1;\n-\t*cp_p = cp;\n-\t*num_p = num;\n-\treturn 0;\n-}\n-\n-int parse_hunk_header(char *line, int len,\n-\t\t      int *ob, int *on,\n-\t\t      int *nb, int *nn)\n-{\n-\tchar *cp;\n-\tcp = line + 4;\n-\tif (parse_num(&cp, ob)) {\n-\tbad_line:\n-\t\treturn error(\"malformed diff output: %s\", line);\n-\t}\n-\tif (*cp == ',') {\n-\t\tcp++;\n-\t\tif (parse_num(&cp, on))\n-\t\t\tgoto bad_line;\n-\t}\n-\telse\n-\t\t*on = 1;\n-\tif (*cp++ != ' ' || *cp++ != '+')\n-\t\tgoto bad_line;\n-\tif (parse_num(&cp, nb))\n-\t\tgoto bad_line;\n-\tif (*cp == ',') {\n-\t\tcp++;\n-\t\tif (parse_num(&cp, nn))\n-\t\t\tgoto bad_line;\n-\t}\n-\telse\n-\t\t*nn = 1;\n-\treturn -!!memcmp(cp, \" @@\", 3);\n-}\n-\n static int xdiff_out_hunk(void *priv_,\n \t\t\t  long old_begin, long old_nr,\n \t\t\t  long new_begin, long new_nr,\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex 7b0ccbdd1d..971aa84b57 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -23,9 +23,6 @@ int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n \t\t  xdiff_emit_line_fn line_fn,\n \t\t  void *consume_callback_data,\n \t\t  xpparam_t const *xpp, xdemitconf_t const *xecfg);\n-int parse_hunk_header(char *line, int len,\n-\t\t      int *ob, int *on,\n-\t\t      int *nb, int *nn);\n int read_mmfile(mmfile_t *ptr, const char *filename);\n void read_mmblob(mmfile_t *ptr, const struct object_id *oid);\n int buffer_is_binary(const char *ptr, unsigned long size);\n-- \n2.19.1.1336.g081079ac04\n"},{"id":"362262","messageId":"xmqqh8gzpuns.fsf@gitster-ct.c.googlers.com","threadId":"49739","inReplyTo":"20181102063156.GA30252@sigill.intra.peff.net","subject":"Re: [PATCH 0/9] saner xdiff hunk callbacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-02T11:48:23Z","receivedAt":"2018-11-02T11:48:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... it seems rather silly that we have xdiff generate a string\n> just so that we can parse it from within the same process. So instead I\n> improved the xdiff interface to pass the actual integers out, made use\n> of it as appropriate, and then in the final patch we can just drop the\n> buggy function entirely.\n\nAcked-by: me.\n\nThanks.  This is exactly what I wanted to do for a very long time,\never since I did the original xdiff-interface.c\n"},{"id":"362310","messageId":"CAGZ79kZb7kMhkE=EWRtR8tpkQCY3v8rWugHvq24TFGHpBxmygw@mail.gmail.com","threadId":"49739","inReplyTo":"20181102063606.GC31216@sigill.intra.peff.net","subject":"Re: [PATCH 3/9] diff: avoid generating unused hunk header lines","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-02T19:50:05Z","receivedAt":"2018-11-02T19:50:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> +/*\n> + * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL\n> + * one just sends the hunk line to the line_fn callback).\n> + */\n> +void discard_hunk_line(void *, long, long, long, long, const char *, long);\n\nRecently we had the discussion on style and naming things.\nOn the one hand I don't know what these 4 different longs do,\nso I'd wish for some descriptive variable names in here.\nOn the other hand the docs explain clearly why I don't need\nto care (a no-op ignores all of the parameters, no need\nto take care of their order)\n\nSo to revive that discussion, I would strongly prefer\nto have *some* names there, for the sake of a\nsimply described coding style without many exceptions\n(especially those exceptions that rely on judgement).\n\nToday I read [1], which describes the analog in the\nmechanical world: To evolve and have more impact\nyou need tighter requirements on some parts. And\nI would roughly translate that to our use case as\nnot having to worry about style (it's ironic I even type\nout this email... if we could just run clang format or\nsome other tightly controlling formatter/linter, I'd be\nmuch happier as our focus should be elsewhere,\nsuch as UX or performance).\n\nApart from that, I read the whole series, and found\nit a pleasant read.\n\nThanks,\nStefan\n\n[1] https://www.nybooks.com/articles/2018/10/25/precision-accuracy-perfectionism/\n"},{"id":"362313","messageId":"20181102201549.GA17228@sigill.intra.peff.net","threadId":"49739","inReplyTo":"CAGZ79kZb7kMhkE=EWRtR8tpkQCY3v8rWugHvq24TFGHpBxmygw@mail.gmail.com","subject":"Re: [PATCH 3/9] diff: avoid generating unused hunk header lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-02T20:15:49Z","receivedAt":"2018-11-02T20:15:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 02, 2018 at 12:50:05PM -0700, Stefan Beller wrote:\n\n> > +/*\n> > + * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL\n> > + * one just sends the hunk line to the line_fn callback).\n> > + */\n> > +void discard_hunk_line(void *, long, long, long, long, const char *, long);\n> \n> Recently we had the discussion on style and naming things.\n> On the one hand I don't know what these 4 different longs do,\n> so I'd wish for some descriptive variable names in here.\n> On the other hand the docs explain clearly why I don't need\n> to care (a no-op ignores all of the parameters, no need\n> to take care of their order)\n\nRight, I actually did have the same thought while writing it. And ended\nup following that line of reasoning (since it's just a placeholder, it\ndoesn't matter).  But I'm not opposed to putting in the names.\n\n> So to revive that discussion, I would strongly prefer\n> to have *some* names there, for the sake of a\n> simply described coding style without many exceptions\n> (especially those exceptions that rely on judgement).\n\nFair enough.\n\nSquashable patch is below; it goes on 34c829621e (diff: avoid generating\nunused hunk header lines, 2018-11-02).\n\nJunio, let me know if you'd prefer a re-send of the series, but it\ndoesn't look necessary otherwise (so far).\n\n> Apart from that, I read the whole series, and found\n> it a pleasant read.\n\nThanks!\n\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex 7b0ccbdd1d..8af245eed9 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -39,7 +39,9 @@ extern int git_xmerge_style;\n  * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL\n  * one just sends the hunk line to the line_fn callback).\n  */\n-void discard_hunk_line(void *, long, long, long, long, const char *, long);\n+void discard_hunk_line(void *priv,\n+\t\t       long ob, long on, long nb, long nn,\n+\t\t       const char *func, long funclen);\n \n /*\n  * Compare the strings l1 with l2 which are of size s1 and s2 respectively.\n\n-Peff\n"},{"id":"362323","messageId":"xmqqr2g3ngql.fsf@gitster-ct.c.googlers.com","threadId":"49739","inReplyTo":"20181102201549.GA17228@sigill.intra.peff.net","subject":"Re: [PATCH 3/9] diff: avoid generating unused hunk header lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-03T00:32:02Z","receivedAt":"2018-11-03T00:32:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> to have *some* names there, for the sake of a\n>> simply described coding style without many exceptions\n>> (especially those exceptions that rely on judgement).\n>\n> Fair enough.\n\nFor the record, there aren't \"many\" exceptions to the rule that was\nsuggested earlier: if you refer to parameters by name in the comment\nto explain the interface, name them and use these names [*1*].\n\n> Squashable patch is below; it goes on 34c829621e (diff: avoid generating\n> unused hunk header lines, 2018-11-02).\n>\n> Junio, let me know if you'd prefer a re-send of the series, but it\n> doesn't look necessary otherwise (so far).\n\nGiving them proper names would serve as a readily usable sample for\nthose who write new hunk-line callbacks that do not ignore them, so\nwhat you wrote is good.  Let me squash it in.\n\nThanks.  If this were to add bunch of \"unused$n\" names to the\nparameters to this no-op function, only to \"have *some* names\nthere\", I would have said that it is not even worth the time to\ndiscuss it, though.\n\n> diff --git a/xdiff-interface.h b/xdiff-interface.h\n> index 7b0ccbdd1d..8af245eed9 100644\n> --- a/xdiff-interface.h\n> +++ b/xdiff-interface.h\n> @@ -39,7 +39,9 @@ extern int git_xmerge_style;\n>   * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL\n>   * one just sends the hunk line to the line_fn callback).\n>   */\n> -void discard_hunk_line(void *, long, long, long, long, const char *, long);\n> +void discard_hunk_line(void *priv,\n> +\t\t       long ob, long on, long nb, long nn,\n> +\t\t       const char *func, long funclen);\n>  \n>  /*\n>   * Compare the strings l1 with l2 which are of size s1 and s2 respectively.\n>\n> -Peff\n\n[Footnote]\n\n*1* You may say that the \"if you refer to parameters by name\" part\nrelies on judgment, but I think it is a good thing---it makes poor\njudgment stand out.\n\nWhen describing a function that takes two parameters of the same\ntype, for example, you could explain the interface as \"take two ints\nand return true if first int is smaller than the second int\" in\norder to leave the parameters unnamed, but if you gave such a\ndescription, it is so obvious that you are _avoiding_ names.  Not\nusing names when you do not have to is good, but nobody with half an\nintelligence would think it is good to give a bad description just\nto avoid using names.\n"}]}